Allow default input formats per role, and integrate input format permissions

Currently I can't assign a default input format to one role without all other roles defaulting to that format as well. What I want to do is assign a more restrictive input format for anonymous users but not make that the default for say, authenticated users. One of the biggest hurdles is the interface and probably the first thing to discuss.

Current:
http://www.asitis.org/tmp/input_formats-per-role_OLD.png

Proposed:
http://www.asitis.org/tmp/input_formats-per-role.png

I agree. That was the interface I had in mind too.

+1. I'd really like this feature, since authenticated users of my site have access to the TinyMCE HTML editor by default. At the moment they have to switch to 'Full HTML' before posting, which can cause strange problems when they don't. Of course anonymous users shouldn't be able to post full html.

+1 Yes, please, please!

Sounds like a very good idea, let's get this active again, I hear berkes chiming in sometime soon (he fostered the redesign of filters and I helped too).

I have exactly the same problem/request so I second this feature request. From the attached screenshots I gather that the original post was for a 4.6 Drupal, but it is also current for the latest beta4 of 4.7. So I upgraded it. Also because it can potentially introduce possibilities for entering malicious html in a site, I escalated the priority to critical.

I first want to know what the general opinion is on this issue. I feel that this choice of using the default input format for all roles is to restrictive. And because you can also (implicitly) incorporate the anonymous user into a format that has no HTML restriction, the choice is also not related to "a default way to do 'things' in a secure and safe way".

I propose that it should be the full responsibility of the administrator (and not of the CMS) to verify that for each role in the system a secure input format for that particular role has been enabled and defaulted.

Another approach to the problem is http://drupal.org/node/34205 (which has a preliminary patch)

new features will only happen in the "cvs" version now that 4.7.0 is out.

I certainly in favor of this functionality being available in terms of useability, and I'm trying to think about way to accomplish it.

I'm still trying to get my head around the Forms API, but could the desired effects (altering the default input format per role) be achieved through a contrib module that used hook_form_alter()?

Please fix this, I'm begging. It's a real pain to not have a default format by role. My admins should have the ability to add more sophisticated content by default than people posting content. If the admin forgets to change the format then their content gets stripped. A simple thing like adding a mapquest "get directions to our business location" html form will get mangled or stripped by the default format. This is extremely frustrating for users and is a "workflow bug" IMHO. I'm a programmer, and know this kind of thing is a pain to fix, but it really shouldn't be that hard?

Basically (ahem),

1. You have to have at least one default format by role. You can't have a role without a default. The system default format is "filtered html" just like today's implementation. You can't delete this input format.
2. If you try to unselect all formats for a role then the role edit screen should throw an error saying "you need to have a least one default input format" for this role. Don't care what it is.
3. An admin with privs can define default formats per role and change the default.
4. If only one format is chosen then this option should not appear on node edit/add screens. Most admins will want to give descreet input format privs to roles. Anonymous can do x, Editors y and Admins z.

I'm lobbying that this "fix" gets put into 4.7.3 or 4.7.4 since it is really a bug if you consider how end-users are interacting. Also, many of the docs on the site are outdated and suggest that you can set default input formats by role. Therefore, it feels like a bug to users. Bugs aren't just binary, workflow bugs are real. Usability is something that Drupal needs to pay much more attention to. No hostility here, 4.7 is still great. Thanks.

This problem is tricky for several reasons:

• UI wise, we need to make it clear which formats apply to which roles without losing the big picture.
• Some modules still use check_output() without specifying the format. This means that we need to figure out the right format to use on output. Simply using the default format of the viewer will not work and will even pose a security risk: someone could post XSS... it would not work for them, but it would work if the admin views it.
• The effect of posting by default in a possibly restrictive format needs to be taken into account. If Drupal.org had all admins post in Full HTML by default, then any book page made by them would not be editable by less priviledged users (you can only edit pages in a format you have access to).

No amount of lobbying is going to make them disappear. If there is a patch which addresses these points, it will get reviewed, improved and possibly committed.

Some replies to schaub123:

The system default format is "filtered html" just like today's implementation. You can't delete this input format.

Not being able to delete filtered HTML is unacceptable for anyone who wishes to use a non-HTML format across their site. The current implementation allows you to delete filtered HTML just fine, provided you first choose a different default format. This is a reasonable limitation.

Not being able to remove any format which is at least the default for a role (the logical extension of this rule in the proposed system) is IMO unacceptable though. We would need to rethink this strategy.

Also, many of the docs on the site are outdated and suggest that you can set default input formats by role.

We never had this ability. The docs are not outdated, they are either wrong or being misinterpreted.

And the significant effects of this change, both in UI and filter system usage, mean that there is no chance whatsoever it will make it into 4.7.x. Your only chance is to help 4.8.x come out as soon as possible.

I just marked another thread on the same topic as duplicate: http://drupal.org/node/48262

However, you may find some other users there who are interested in sloving this.

Is check_output() used in Drupal 4.7? I don't even see it in the API.

http://api.drupal.org/apis/4.7/check

I do see check_markup():

http://api.drupal.org/api/4.7/function/check_markup

If there are people who are interested in solving this problem, let them show up! The issue is open for almost two years.

Friendly note: nitpicking on Steven (who, in the middle of final exams spent time to answer here) is not the best idea. Let me assure, he among all knows best check_output / check_markup because he has written the whole thing :D

Trust me, I had no intention to nitpick- I'm still getting a handle on this and I just thought I was missing something in the API. I very much appreciate all expert advice.

I am interested in solving this (though it's not extremely high on my list). I'd still appreciated some feedback from someone with Forms API experience as to whether this could be addressed by an add-on module that changes which checkbox is selected when generating the node creation form.

I see the real usability issue as this: when a user goes to create new content, if they have access to filters other than the default, the format that's checked without their intervention should be selectable per user role (or maybe even per user).

> I am interested in solving this (though it's not extremely high on my list). I'd still appreciated some feedback from someone with Forms API experience as to whether this could be addressed by an add-on module that changes which checkbox is selected when generating the node creation form.

Yeah I don't see why it couldn't. You'd have to store the role / input filter relationship somehow (since there would be relatively few of these, you could probably throw stuff in the variables table), and then hook_form_alter the node edit form to check for the user's current role and select whichever box by default. Or, you could just throw out the logic and simply select the highest #'ed input filter that the user has access to. that would be "Filtered HTML" for anon and "Full HTML" for everyone who has access to it.

Ok, a quick look in node.module to try to get a handle on this. Looks like the same form is used (of course) to add or edit a node.

Here's the relevant form ID, which I think is what we need to use to grab the right form in hook_form:

Here's a test we can use to see if it's a new node:

However, I'm not sure what happens if we preview our new node? I assume the above test would still evaluate to TRUE. This could be bad, since it might reset the input format if the user had changed it? A (hackish) approach might be to also test whether the body and title are empty or !isset() ?? Any better ideas?

I'm a little lost still in trying to understand how the submission of comments works, since I assume that changing the default format here is an equally (or more) important part of the goal. I don't see anywhere that a form ID is set in function comment_form()- am I missing something? Isn't this form ID necessary to alter the form?

Ok, trying to still untangle the web of FAPI logic, it seems that within filter.module, the function that actually generates the filter-selection part of the node or comment submission form is function filter_form(). So any attempt to alter the default would involve recapitulating a good fraction of the logic from this function.

For those that use TinyMCE, take a look at this related issue: http://drupal.org/node/66014

Has anyone tried the Remember filter module? http://drupal.org/node/47983

This is definitely a dirty hack, but it works for me, so it might work for anyone else interested in this bug. It's by no means a fix.

This patch selects the "last" format that is accessible to the user as the default for new content. In a default install, this would probably be Full HTML, so you can set the system-wide default to be Filtered HTML, and this patch will cause nodes to be created in Full HTML if that's available to the user.

Attachment	Size
filter.module.patch.txt	832 bytes

I would love this function, too.
It's hardly critical, though ;)

There's a patch. Needs work though, as it is currently indeed in 'dirty hack' mode. :)

Has anyone tried the Remember filter module? http://drupal.org/node/47983

nice find. It is a very simple module that works fine (use today's cvs version for a 4.7 site, as the official 4.7 release is broken).
It works well for my immediate needs, but it is not exactly the same as this feature request.

A Default input formats per role feature could similarly be fairly easily implemented with a contrib module, should anyone take the trouble to actually do it.

remember_filter.module doesn't not work for comments.
http://drupal.org/node/74613 .

I'd be interested in people's uggestions for the data storage and UI for this feature.

Here's one possible suggestion:

1) On the administer >> input formats page, include a tab to a page with a matrix of roles and (allowed) formats.

2) Save the results in the variable table as a per role value. Catch roles or filters being deleted. Any roles not set with a specifc default get the site-wide default (as it is now).

3) when loading a user, store their default filter as part of the $user object. Use this as the default when setting up the node edit form.

questions: how do you resolve the conflict if a user has roles with two different default inputs? Obviously,including role wieghts in core would make this easy, or each input format could get a weight?

seems like potentially a *lot* of settings to store, and all settings get loaded on every page, all the time, no matter what. you only really need to see this data in the more rare case when a user is viewing a form with text input and the system needs to know what filter(s) to present as choices. so, i'd vote for just using a separate table of (role_id, filter_id) to hold the mappings. any roles with no records in the table would get the site-wide default.

then, you only have to incur the cost of looking up this info when you need it, and when you do, it can be a fast lookup for just the records (roles) you need, instead of always loading everything on every page.

Well, the filter_formats table already stores a column with roles allowed to use each filter, so it could be sotred as another coumn in the table. Alternate, it could be another coumn in the role table. Is there any reason to have an entirely separate table?

replies to powlan ...

1) On the administer >> input formats page, include a tab to a page with a matrix of roles and (allowed) formats.

This would be cool, but I think the ability to specify the default is still primary since you can already specify which roles can do what filter on the filter page? But this would be nice. I think the ability to have a default format and weighted filters would solve this issue for most people.

2) Save the results in the variable table as a per role value. Catch roles or filters being deleted. Any roles not set with a specifc default get the site-wide default (as it is now).

This seems good. I don't think the data moving around is too much really, just an array with ids to the filters table. A few bytes at best.

3) when loading a user, store their default filter as part of the $user object. Use this as the default when setting up the node edit form.
questions: how do you resolve the conflict if a user has roles with two different default inputs? Obviously,including role wieghts in core would make this easy, or each input format could get a weight?

Yes, there needs to be a heirarchy of filters by weight. This should solve most issues and configurations. I can't think of any security issues with doing this in the user object. Hmmm ...

+1 from me. I'd like to use ecto to make updates. But with the default input format set on filtered HTML, I'd have to load my browser up and edit the post anyway.

Please see http://drupal.org/project/filter_default which provides some of the functionality this issue requests.

Features go against next version.

the module mentioned above corresponds to the requested feature, so it's fixed, no?
Or do people think it should be part of core?

I think this should be in core. It's seems pretty common to have a situation where you want anonymous users to only have (default) access to filtered HTML, while an admin role has Full HTML defaulted (so admins don't have to switch to Full HTML for all their pages) but can access to both formats.

Yes, I agree this should be in core. i'd suggest "default filter" and "role weight" columns for the {role} table.

Other issues request default input filters for content types, too, and I've mostly implemented it in this module (no patches available yet). Putting a default filter column in the role table would put half the solution in one core while leaving the other half out.

These two features would seem to be mutually exclusive, right? At least, it would be hard to have both per-role and per-content-type filter settins at the same time without an overly complicated rule system for resolving conflicts.

what's the more common/general need? I would gues it's per-role, but that may just be my personal bias.

I can see a use for both, but I'd think by node type would be more common, personally. A forum post probably never needs full HTML. A page node probably does on a regular basis. I know I very much do NOT want my users putting HTML in forum nodes at all, but do want them to be able to put it in a page node or similar.

Per role is definitely the way to go here IMO. You define input formats pretty much based on how much permission you want to give a user. Do you want them to just have bold/italic, can they get crazy with any kind of HTML, can they use PHP to execute god knows what? You could always have an additional module that form alters certain options out based on content-type (that would be easy), but the main push here is allowing for different default input formats for varying levels of access.

Agreed on per-role. Per-node type could be done at the theme layer.

+1 on per role.

filters are about permissions, security and trust. frankly, "use full html" and "use php code" could be actual drupal permissions on the access control page. or, more generally, access to any given filter could be its own permission. then, instead of the separate UI for assigning filter access per role in admin/settings/filters, you could assign filter access just like every other permission on the site. keep in mind, granting php code filter access to a role is equivalent to making every user in that role a super-user with *full* access to the entire site...

however, keeping all the filter-related configuration in one place is awfully nice, too. but conceptually, these are exactly like permissions. perhaps we could have both UIs in both places, and they'd end up updating the same permission info in the DB (so if you assign a given filter permission to a new role on one page, that would be shown the next time you visit the other).

we'd still want to add the feature this issue is talking about: a way to assign a different default filter to each role. but, again, it's all about permissions and access, so it fits in with roles much more naturally than node types.

thanks,
-derek

subscribing, plus, is anyone working on per-content type filters? There are loads of cases where I only want 1 filter to work on a given content type, no matter what the role of the user is.

Can this be done at the moment?

I think it's not too hard using hook_form_alter to write a little custom module that would force a single filter option for a particular content type.

a little more evidence to support my claims in #42... behold the confusion in our forums on this point:

http://drupal.org/node/61434
http://drupal.org/node/107971

in the first one, someone who didn't understand what was going on decided to grant "adminster filters" permission to a user, just so they could edit the page. *sigh*

bold (unsubstantiated) claim: if access to each kind of filter just showed up as another permission on the regular access control page, people would understand what was going on a lot better, and would be more likely to configure drupal correctly.

@pwolanin, thanks, I know I could write a module to do that - and I am about to do that for a site I am making now.

I would like to see per-content type filters in core though - to me it's as important as per-role filters. If anyone else is working on this, I'd be interested, if not I'll make my module first and see if I can make a patch for core I guess.

@dww - That's an interesting idea and I think I like it. So under filter module on access control we'd have something like this (with better wording) 'create Filtered HTML content', 'create PHP code content', 'create Full HTML content', (and any user added filters).

Then on the filters page, maybe we could just specify filter weights like this:

Full HTML - Weight -10
Filtered HTML - Weight 0
PHP code - Weight 10

Auth users can access Full HTML so they'll get that as the default. And if anon users can't access Full HTML, they will cascade down to Filtered HTML set as the default when creating new content.

Does that make sense? Then, if it's desired, we could even have different weighting for each content type easily on the latter page, while keeping the role access control for filters on the "correct" page like dww brought up.

sort of... a few nits:

a) the general should be filtered HTML. full HTML is an XSS risk.

b) i don't particularly mind that there's 2 places to assign permission to use a filter to a given role:
- 1) when assigning other permissions to roles [roll-centric view]
- 2) when configuring other properties of a filter [filter-centric view]

so long as when you change it in 1 place you see the change reflected when you view the other, i see this as a usability feature, not a bug. not every task wants or needs the same view of the data and same means to manipulate it. when you're defining a new filter, it makes sense to be able to assign roles to it right there on the spot. if you're configuring a given role for which permissions on the site it should have, it makes sense to be able to select which filters that role should be able to use, too.

c) i think the ordering of the permissions on the admin/user/access page should use the same weights as defined on the filter page. not sure if that's what you're saying, but just trying to state clearly what i'm proposing. ;)

thanks,
-derek

When you say the general should be filtered HTML, I agree.

I was saying we could just assign weights to filters (and scrap the default radio button that one of the last screenshots proposes) to figure out a default input format per role just by shifting off the top input format until we get to an input format that the current user has perms for. But....that might not be super intuitive and the more I think about it is not a good idea for the case when two roles can access all input formats but you want them to have a different default.

So never mind on my idea, but I still agree with what's been proposed so far. :P

I don't see why it should even be two separate forms. It seems like a simple 2D form to me.

You have a table that lists node types on one axis and roles on the other. Each cell is a checkbox. The form is not valid unless each row and each column has at least one checkbox checked. Defaults are determined by weight of the filters. Simple, quick, one-stop-shopping. The form could be placed with filter config or with access controls, depending on how you want to think about it, but it shouldn't be split into multiple forms.

Could people please post some screenshots of some simple html mockups to go along with these ui suggestions? My head is exploding. ;)

i was starting to reply to webchick's request for screenshots, and Crell's post (see below), when i realized that i've hijacked this thread too much already.

so, all discussion about filter access as permissions should now go here: http://drupal.org/node/110242

meanwhile, back to this issue...

this is my last plea to keep this issue about default filters per role, as originally proposed. as i've now stated numerous times, filter access is all about security and trust, it's a permission, and permissions are assigned to roles, not content types. what if you define a default filter for a certain node type, and not all users with permission to create that node type have access to use that filter? i smell much confusion (both for users and for the resulting code to implement this). if you want that, make a contrib for it, but i don't think it really belongs in core. if you really want to continue advocating for defaults per node-type in core, please make a new issue (followup here with a link) and let's leave this poor, abused issue to focus on the relatively small change it's talking about.

in terms of the exact UI for this, i don't have any brilliant ideas right now or screenshots to offer. my biggest contribution with this followup is moving all the extra crap out of this issue that doesn't belong here, so folks can re-focus on the relatively simple task at hand. ;)

p.s. @crell: your 2D table form makes no sense to me. are you talking about having such a grid of checkboxes for *each* filter? i don't see how filters fit into your table at all. plus, while potentially powerful, it sounds like an absolute nightmare to use. giant tables of checkboxes are a big enough pain to deal with as it is (thought the jQuery checkbox stuff in D5 is a *vast* improvement, yet i don't see how we could leverage that functionality if you had 2D of checkboxes...).

Matt Westgates screenshot #1 at http://www.asitis.org/tmp/input_formats-per-role.png seems pretty close to me.

Ok, but how do you resolve the case of a user with 2 roles with different defaults? The asy case is "authenticated user" vs. any other role, then any other role wins. But for the harder cases, seems like you need role weights in core?

@dww: The reason I keep bringing up by-node type is because by-role and by-node type could very easily clash to create a maintenance nightmare if we don't account for both at the same time. The logic for adding either one on from contrib gets more complicated. As for the 2D grid, I mean something like the access control page. That's already a grid, but since we only have maybe 4 input formats on a typical site (at least IME), the table is much smaller. Heck, if you wanted to you could just stick them into that table as "real" permissions the same way CCK does.

I actually can't think of a case where I'd want the default format for role X to be one format, and the default for role Y to be a different format. I can think of many cases where I'd want the available and default formats per-node type to be different, but not by role. Can someone suggest a use-case for that? It seems defaults are the only complex question.

As for the 2D grid, I mean something like the access control page. That's already a grid, but since we only have maybe 4 input formats on a typical site (at least IME), the table is much smaller.

to be clear, then 1 axis of your 2D table is really filter names (e.g. permissions in the access control grid, as you say). and the other axis is... roles? node types? please pick one, or come up with the a 3D UI that fits on a 2D screen (Eduard Tufte and "escaping flatland", anyone?), because you keep saying "2D" but you're talking about 3 independent things: 1) filter name, 2) node type, 3) role. i'm not trying to pick a fight, and maybe i'm being totally dense, but i still don't see how you plan to address all 3 in a single table with rows as filter names, columns as (node type|user role) and checkboxes to indicate which filter(s) the given X should have access to.

Heck, if you wanted to you could just stick them into that table as "real" perminssions the same way CCK does.

yes, i want this very much. that's exactly what i'm proposing in http://drupal.org/node/110242

i don't know how else to say that input filters are not primarily about how you want nodes to be displayed, but about how much trust you have in your users. taking your example: forums should be no HTML, pages should be full HTML -- then you have to ensure that only users you trust with full HTML have access to create page nodes, or you've opened yourself up to XSS. these sorts of implicit trust relationships are harder for people to keep clear in their minds, and i think it'd be far too easy for people to screw this up, just like they do already (see the forum posts i've already linked to).

so, either you open yourself up to trouble by always giving users access to the default input filter for a given node type, or, you have yet another "hidden" reason why user foo can't edit content of type bar, even though the role claims they can. and, if user foo is allowed to use both filtered HTML and full HTML, then they're going to have a choice of input formats whenever they create nodes of type bar anyway, so the fact there's an admin-controlled default doesn't really win you much.

if you read the initial justification for this issue, it's pretty clear:
- anonymous users should be stuck with a highly restrictive input format
- authenticated users can (potentially) be trusted with a more risky input format that provides additional functionality (a WYSIWYG, whatever).

that's the problem we're trying to solve here. all this default-per-node-type stuff is a digression into territory that's much more complicated, harder to configure correctly, and more likely to be insecure. i appreciate your desire to "solve all problems at once, or else it gets complicated". however, the whole notion of default input formats per node type is inherently complicated, and it's going to be no easier (or harder) to solve in core than in contrib.

but, talk is silver, code is gold... and i have some gold to generate elsewhere. ;) i don't actually care that much about this particular feature, so i'm done here (for now). i hope i've at least helped shed some light, without generating too much heat.

cheers,
-derek

Filter per node type (pseudocode):

in node-XXX.tpl.php:

This we can do already. You could combine this with default input formats per role to do something like:

But, you can't hack around default input types per role without some sort of module/core enhancement.

pwolanin's point at #54 is a valid one. We'll need some type of weighting in there. What about weighting the input formats instead of roles? Probably makes more sense. Then, if a user belongs to multiple roles, we'll select the default input format by weight.

input filters are not primarily about how you want nodes to be displayed, but about how much trust you have in your users.

+1 on Derek's comment (#56).

+1 on RobRoy's suggestion about weighting by input filter. It makes more sense this way, precisely because it is consistent with what Derek said above. Some input filters are more permissive, and will only be used by the most trusted users. The hierarchy between input filters is clear: Full HTML > Filtered HTML with links > Filtered HTML without links (i.e. user can't use 'a href=""' and URL are not filtered into links).

subscribing, extremely needed feature

I think we should weigh inputformats androles, they both could need some kind of hierarchy.

While we're at default inputformat for roles, what about a per-user setting doing the same and overriding the per-role setting?

And a setting on the content-type (or should it be per-field?) could be used to restrict the possible input-formats there, with the role- or user-setting choosing the desired option from the now reduced set.

So an algorithm could work like this:

1. possible input-formats = union of all input-format enables on all roles the user has (and all formats for someone with 'administer filters' permission?)
2. reduce possible format-list according to settings on content-type or field
3. if the users preferred format is in this set, use and and stop here
4. loop over the roles in order of descending weight, if we find one with a default-format in the set use it and stop
5. if the site-wide default is in the set, use it and stop
6. use "filtered html", or other named lowest-weight input format (and lowest-weight format always must be usable)

do I make sense?

subscribing. Huge +1 for me.

Okay, I don't have time to read all the threads on this. But I had an itch so i scratched it.

The attached patch adds a weight to each input format. The format with the heaviest weight available to the user (based on his roles) is chosen as the default.

For example, I have the following:

• Filtered HTML - all roles - weight: -5
• Full HTML - site manager - weight: 5
• Rich Text Editing - author,editor,site manager - weight: 0

So my anonymous users get (only) Filtered HTML. My authors/editors get (by default) Rich Text Editing (and also TinyMCE). My site manager gets (by default) Full HTML.

I know this doesn't help for content-type issues, or comment blocks, etc. Still, feedback on the patch or improvements are welcome.

Attachment	Size
filter_16.patch	5.75 KB

As described above, your use of weight is the opposite of most other uses in Drupal (where the "lightest" weight wins).

Getting the input formats right is important for the security of one's site, so we should take the UI design serious and make it so that it isn't ambigious. With that in mind, I think we should stay clear from using weights. It needs to be 100% transparent what the user will see, and it feels like weight could be a source of confusion, and thus, security mistakes. I'm support this feature, but we'll want to get the UI 100% right and that might take a little bit more work.

@Dries- ideally this change should not have any security implications, since it only would change the default input format, not which formats are accessible to a particular role. I agree, however, that the UI is going to take work to get right. I think that weights or some similar mechanism needs to come into play to handle the case where a user has 2 or 3 different roles which have different default formats.

Subsribing - I have almost 10 journalists using Drupal sites daily and they all have to manually select BBCode, since "Comments"-filter is the default so anonymous people can't post HTML as comments..

+1 from me. This is something that I've bothered by, been new to Drupal and trying to figure out how to let my authenticated users have BBCode as default and only input format and still let me and other trusted content creators have full html input format as default. Reading this I understand that it's just not possible right now.

For both this and access control, it would be a nice UI improvement to show all roles as selected & disabled if authenticated user is selected.

@Dries: The problem is that if a user is in multiple roles, which takes priority in determining the default? Roles are a simple aggregate currently. A weight wouldn't affect what a user can do, but would make determining which input format is the default simple: It's the sum of all input formats the current user has access to on a given node, ordered by weight, defaulting to the lowest weight. I don't think that's a major security risk.

If we don't use a weight, how do we handle users in multiple roles?

I think for the time being we shouldn't bother with role-weight, but just define an order of input format.
And the highest input format available to a user and define as default in one of his roles wins.

That's weights. :-) Well, format weights, which is what I'm talking about. Weighting roles against each other is another can of worms I don't think we want to get into at this point.

to disagree - I think role weights is a broadly useful concept that could be in core, and is obviously easily achieved with an extra column in {role}. Then the display order of the roles could also correspond to their precedence in terms of formats, etc.

I agree that role-weights are a useful concept for the core.
But I think that a ordering of input-formats is more important for us here and we don't need role-weights to do that.

+1 on input format weights. it seems the most natural way to order them, and we clearly need an ordering scheme to deal with what a given user in N roles with different default formats gets as the default format. in this case, the weight is essential to control the underlying functionality, not just how things look in the UI.

-1 on role weights, both for this problem, and probably in general. roles are a set of permissions. if a user is in more than 1 role, their permissions are a simple union of the different sets. the *only* thing that role weights would be useful for is to re-order the roles in various parts of the admin UI (basically, anywhere a list of roles is presented). however, in this case, the weight wouldn't impact anything about the underlying behavior, just how it looks. given how complicated the role/permission UI is already, and how essential it is for security, i'd be against adding yet another UI widget that only offers mildly useful (if any) flexibility in how things are displayed. i'd be especially against it if we had input filter weights (which would impact functionality, not just appearance) and role weights (which wouldn't impact functionality, only appearance), since it seems to me this would just add further confusion to an already confusing topic.

all that said, there seems to be some disagreement on what an input filter weight would mean. in drupal, "light floats to the top", and in general, if something's on top, it "wins". but, how does that map to input filters? i think of filters being on a line from "secure, even for untrusted users" (highly filtered html) to "utterly dangerous, only admins should have access" (php code). naturally, you'd want to define the weights so that the filters on your site lined up in the same order along this axis. when you're in 2 roles (say, "content moderator" and "authenticated user"), and they have different defaults ("full html" for the moderator, "filtered html" for the auth user), you'd expect the "most powerful, dangerous" filter that the user has access to would "win". therefore, we should probably weight the default filters like so:

php code -10
full html 0
filtered html 10

then, if you're in N roles, the behavior is easy to code and explain: the "lightest" of the default filters for all of your roles wins.

we should make it clear in the help text that the "higher/lighter" the filter, the "more dangerous it is". this also seems to map nicely into the idea of "elevated privileges", i.e. the more you trust your users, the higher on the list they should be allowed to climb...

Unless someone comes up with another brilliant idea, I agree that input format weights are the way to go. I just spend a few minutes writing how I thought the weight ordering that dww described should be reversed, then re-read it and realized he was describing what I was thinking. So, agreed on the ordering scheme and triple agreed on a need for explaining it very clearly since even I almost screwed it up.

I agree with dww except on one point: Even if a user has access to a more-dangerous input format, they shouldn't have that as their default. If a user has permission to submit text as filtered HTML or PHP code, they should have to KNOW that they're changing to PHP code. It shouldn't be something that just happens.

Besides, that locks pages out to other users. Imagine if any book page on drupal.org that was created by a site admin was suddenly locked against edits by the docs team because they happened to default to PHP code for the input format, even though the page itself is really just a few li tags.

@Crell: to clarify, here's what i wrote:

"the 'lightest' of the default filters for all of your roles wins." (emphasis added).

of course, the default shouldn't be the lightest of all possible filters. i just mean that if you have 2 roles, with different defaults, you get the lightest of the 2 default filters, not the lightest of all possible filters for either role.

oh, and to further clarify: on d.o itself, all roles would still have "filtered HTML" as the default, so in our case, this whole discussion is irrelevant. ;) this discussion only matters for other sites, especially the ones that use WYSIWYGs, where you want a less-restrictive HTML filter for authenticated users, for example.

Agreed, but I have one little problem with the end of the discussion: people seem to use the terms "input format" and "filter" in the same context without realizing that's 2 different things.
So, do we weigh "input formats", and it's the admins task to set the weights sensible, or do we weigh the "filters" used in the input formats, and calculate the input formats weight from there?

FWIW, I'm voting for the former, since that's the option with less surprises ...

yes, good point. i'm in favor of weighting the input formats (which are themselves unordered sets of filters) not trying to order the filters themselves. thanks for the reminder and clarification.

the filters *are* ordered/weighted within each input format already.

hehe, dww finally notices the "Rearrange" tab on the admin/settings/filters/1 page... learn something new every day. ;) i must say, looking into this closely now, the filter/format UI is even more convoluted than i already thought. :( perhaps i'll fille some UI-specific bug reports in other issues.

anyway, those filter weights are clearly tied *per* format, so there'd be no way (and no reason) to do global weights of individual filters across all formats. i'm still in favor of adding weights to the formats themselves, and actually displaying formats in weighted order (unlike the filters on admin/settings/filters/1 (!)) and having the weights solve the question of "what default format should be used for a user in multiple roles with different defaults?". it's my original answer from #75 above: "the lightest of all the default formats for all the roles you belong to".

Yes, that sounds right to me too.

Apparently none of you noticed that filter_resolve_format() is currently called both on view as well as creation? The patch is one big security hole.

The filter system is complex, and issues like these where people make blind decisions without understanding the underlying code are pointless. But then, it's not like input formats have been implemented, documented and commented for say 2.5 years, right?

We need to eradicate FILTER_FORMAT_DEFAULT first, then work on per role formats.

i haven't looked at any of the patches, i've just been approaching this issue conceptually and from the UI perspective, both of which have already been the source of massive confusion in this issue. to me, it's pointless to generate (or review) patches until you actually understand what you're trying to accomplish.

subscribing

Moving... :( This is something I want to see in D7 for sure.

subscribing.

Hi:

I agree dww. The Input formats (IF) (he says filters but I think he's talking about Input formats ??) are related to permissions and I think they would be placed in the admin/access page to assign permissions on a per role basis.
This is basic.

After thar you can:

- In the input formats page, you can assign a default IF to each role from the IF available for each one.
AND/OR
- Assign available Input Formats to each text field (and perhaps weight them)

subscribe

just so you know, i just tested the http://drupal.org/project/default_filter module and it allows to assign default input format on an per content type + role basis

Yes, that's my module and I meant to update this issue about it.

Filter Default lets you specify the default input format for all text areas *in nodes and comments* (but nowhere else) based on user role. I have not followed this issue in detail and do not claim this module is the best way to solve the problem, but it does work.

A BIG -1 on this feature

...as long as there is not a working solution to the following problem:

1. User A is allowed to use input format "Full HTML". Creates a content.

2. User B is allowed to edit User A's contents, but only allowed to use input format "Filtered HTML". Edits the previously created content. Half of the content is gone.

3. User A needs to edit that content, reset the input format to "Full HTML" and save it again.

This issue already exists in the filter_default module for quite some time now. Yes, it basically works out if all users are allowed to use the same input format. However, that is an edge case and totally error-prone. Tons of bug reports and support issues in the Drupal queue following.

I urge *everyone* who replied here to find a working solution for this unsolved equation in filter_default module. If one is ever found and thoroughly tested in many different site setups, we can start to think about adding that feature to core.

@sun: If node X is set to the full HTML filter by user A, and user B doesn't have access to that filter, user B can never edit node X. Period. Your tale of woe here doesn't have any bearing on the default filter per role discussion that has been going on.

Moreover, your story makes it sound like you don't really understand how filtering works at all. The *whole point* of output filtering (what we should really be calling this functionality) is that the content is filtered *on output*. Changing the filter setting on a node just changes what Drupal does with the raw content on the way from the DB to the screen. The original, raw content is never changed or lost.

@sun: Additionally, following up on #95, this issue is for the core filter.module. I commented on what my Filter Default contrib module does just to provide the information, in case it is helpful for the discussion.

Furthermore, the scenario you describe, besides being disallowed, would constitute a site design flaw even if it were allowed. If you allow less-privileged users to edit content created by more-privileged users, you are always going to have problems, so don't do that. As dww points out, in Drupal's case you'd only lose output fidelity, not actual data, because the raw data is always stored unmodified.

with the sight of relief that posts exists.....subscribed. Thanks!

As well: if you did want a user with a "lower" filter capacity (or lets assume there is no clear lower/higher) to edit other users with a different capacity-- and presuming there was an issue despite the data being stored nonetheless-- simply turn node revisions on by default.

I'm working on a patch in conjunction with Make access to filters first class permissions. Taking that patch into account at the same time makes this particular task easier, as we don't have to present the user with a flood of checkboxes and radio buttons at the same time, as RobRoy presented in #53:

http://www.asitis.org/tmp/input_formats-per-role.png

Instead, by moving the permissions to the permissions page where they belong, assigning a default becomes an easier task. See attached screenshot.

However, there's a complication that I'd like to address with this issue. What happens when a user is assigned multiple roles? I tried to explain it briefly in the screenshot, but here's a more verbose explanation.

Input formats:
- Filtered HTML (weight 0)
- Filtered HTML with images (weight 1)
- Full HTML (weight 2)
- PHP Code (weight 3)

Default role input formats:
anonymous: Filtered HTML
authenticated: Filtered HTML
editor: Filtered HTML with images
administrator: Full HTML

Okay, so what happens when a user is assigned both the "editor" and "administrator" roles? This effectively means they have a default set 3 times (adding in the authenticated user role automatically)!

My solution is to use the lowest weighted input format for all roles, excluding the anonymous and authenticated roles. So since "Filtered HTML with images" is the lowest weighted default between the "editor" and "administrator" roles, it would become the default for that user.

If the user were only assigned the "administrator" role, Full HTML would be the default (even though Authenticated Users only get Filtered HTML). Make sense? Anyway, besides that snafu, please comment on the UI here.

Attachment	Size
Picture 2.png	66.97 KB

Here's my not-yet-working patch, but UI is in place.

Attachment	Size
format_permissions_and_defaults.patch	35.78 KB

does this mean we need role weights in core?

Gábor already got role weight in: http://drupal.org/node/222183, in anticipation of similar functionality I believe.

I took a look through this patch and it seems like a good start to me. (Some of my comments in http://drupal.org/node/110242#comment-829042 still apply here, of course.)

Using the weights as a fallback makes a lot of sense, I think. Besides the situation you described, another situation where you might need to fall back on the weights is if a role loses access to its default format... for example, if "Full HTML" is assigned as the default for authenticated users and then that permission is removed on the permissions page, you need to automatically assign authenticated users a new default somehow.

In fact, hm.... rather than handling all these as special cases.... wouldn't it be possible to always use the lowest weighted input format as the default??? In other words, get rid of the concept of hardcoded "default input format" settings altogether, as we discussed at the other issue!? It looks like that has been suggested already here, too, in #47 and #63 above, as well as by Dries at http://drupal.org/node/222183#comment-736813.

Granted, this would not give you complete freedom to assign "default formats per role", but it would work just fine for the most common use cases. For example, if you ordered your input formats like this:

1. "Full HTML" (allowed roles = administrator)
2. "Filtered HTML with images" (allowed roles = editor)
3. "Filtered HTML" (allowed roles = anonymous user, authenticated user)

then administrators would have "Full HTML" as their default, editors would have "Filtered HTML with images" as their default, and everyone else would default to "Filtered HTML" (i.e., each user's default is the first one in the list they have access to).

Whereas if you reordered it like this:

1. "Filtered HTML" (allowed roles = anonymous user, authenticated user)
2. "Full HTML" (allowed roles = administrator)
3. "Filtered HTML with images" (allowed roles = editor)

then all users would default to "Filtered HTML".

Is that good enough for a first stab at solving this issue? More complicated and fine-grained assignment of default formats in different cases (as discussed above as well as at http://groups.drupal.org/node/8911) sound like they could wait for later work or for contrib modules to solve... but I think (maybe?) doing it this way would be the simplest, cleanest change and the easiest patch to write.

Another way to look at this is that no matter what, we have to deal with the absence of default input formats in some cases; as discussed at #110242: make access to filters first class permissions, there may be users who have no permission to use any of the site's input formats, and we have to figure out what to do when they try to create content (as stated there, I think maybe content created with "no format" should default to check_plain + line break filter + URL filter?). So if we have to deal with the fact that some people won't have a default anyway, perhaps we might as well just meet the issue head on and try to get rid of hardcoded defaults for everyone...

I like the idea of having the lowest weighted available filter as the default. My initial thought is that this would be beneficial to keep most data on the lowest weighted filter across different roles. It also makes calculations much easier as well as validation. +1 from me, even if we loose a tiny bit of flexibility that true defaults would have allowed us.

Is posting to a thread the only way to subscribe? I see a lot of people simply posting "Subscribing" which is why I'm posting this...

I also like #103. And a 'plain text + line breaks + links' input format sounds good for the no access case.

#103 sounds like it's on the right track, though for "no format" I'd omit the conversion of URLs and just have check_plain + line break filter

Would be great to drive #110242: make access to filters first class permissions home first.

Gábor, the latest patch in this issue already encompasses #110242: make access to filters first class permissions (we should maybe just retitle this one and mark the other one as a duplicate). It really is almost impossible to implement one without the other, unless you want to have lots of crazy special case code.

By the way, I've been working on a reroll of this patch that integrates the ideas from comment #103 above. It's slower going than I had hoped, but I will have something to post here tomorrow. At a minimum we'll have some UI issues that need to be figured out: Specifically, if we have a hardcoded "plain text" fallback format that all roles have access to, then how do we present that option to the user? If we put it in the input format fieldset, then it means the fieldset will be visible to lots of users who currently don't see it (e.g., most users would get a fieldset with radio buttons for "plain text" as well as for "filtered HTML", whereas now they don't get a fieldset since they only have access to the single "filtered HTML" option). This seems a little ugly to me, so UI ideas are welcome.

#304330: Text format widget reworks the widget so that adding one more format to the options would not have that big of an effect anymore. Marked #110242: make access to filters first class permissions as duplicate per your suggestion.

OK, I've rerolled most of @quicksketch's patch from #100 and incorporated the ideas from #103... see the attached patch. It's still not finished (still needs to take into account some feedback from above as well as from the other issue, and it's almost completely untested), but it's a start.

Some issues to consider:

• I started off writing this patch with the "no format" option completely hardcoded in, but then eventually realized that didn't seem like the best approach -- it led to too many special cases and was too rigid. So instead, the attached patch implements the "no format" (plain text) case to work mostly like a normal input format (i.e., stored in the database), but with the UI for editing this format removed, and a call to check_plain() hardcoded. The idea is to completely lock editing of this input format within core, but allow a contrib module (some) freedom to change it while still maintaining a good degree security (i.e., making it so you'd really have to go out of your way to turn this "plain text" format into a dangerous one).
• I'm using @pwolanin's suggestion of only using the line break filter (no URL-to-link conversion). It's a shame not to have links since we want to make the plain text format as useful as possible, but I guess we can't have it by default because it opens things up too much to spammers. However, with the above implementation, it's something a contrib module could easily change.
• Based on Gábor's link to #304330: Text format widget (thanks for pointing that out, Gábor), I haven't tried to do anything special with the user interface here and am treating "plain text" like a completely normal input format in the UI when creating content.
• Finally, there's an interesting question of what to do with the profile module. Before, textareas in user profiles were hardcoded to use the site-wide default input format, which is already a little weird... now, we're using the default input format by role, which is possibly even a little weirder (since the user's default input format can change depending on whether they filled out the profile before or after they got their account, etc). It might be better if these textareas were fully input-format enabled (i.e., with the input format selector), but then we'd need to worry about storing the format in the database, etc, so it's a whole other issue, and I didn't want to implement that in this already-large patch right now without further discussion.

Attachment	Size
format_permissions_and_defaults_2.patch	42.31 KB

Some comments:

1. Overall this patch looks good. It is mostly easy to read/understand.

2. +    $fallback = ($id == filter_fallback_format());
Would be good to document the $fallback. Some code comments would help.

3. A CHANGELOG.txt update would be appropriate.

4.

It would be useful to provide a reason, e.g. "The %format format cannot be deleted becuase ...". It would help the user in case he/she would end up on that page, but frankly, it also improve the readability of the code.

5. +        'description' => t('Use !input_format in forms when entering or editing content. %warning', array_merge($format_name_replacement, array('%warning' => t('Warning: This permission may have security implications depending on how the input format is configured.')))),
The use of %warning and array_merge() is a bit weird here.

6.

Should we try using the new DBTNG format here?

7. The term 'use input format 1' is a bit unfortunate; i.e. it is not very user friendly.

8. Does the scheme definition needs to be updated?

Looks like we are close! Good job.

Thanks David for your tremendous effort updating this patch and merging it with the filter permissions patch. I totally agree that keeping the two separate was much more trouble than it was worth.

This revised patch makes a few critical changes, which I hope truly makes the "fallback" format nothing but a fallback.

- The fallback format is hard-coded into the module, no longer stored in the database.
- The fallback format is given the defined constant ID of 0, so we don't need to keep a variable for which format is the fallback.
- The fallback format is never listed if users have access to other formats. Instead it is only available if a user has access to no other formats at all.
- Addresses Dries' concerns regarding CHANGELOG, confusing code bits, and small amounts of documentation.

This patch does not:
- Use DBTNG, as I couldn't find any reason to replace one line of code with 7. This might just be my newness to the system, but according to our current docs, db_query() is appropriate to "Use this function for SELECT queries that do not require a query builder." It doesn't seem necessary for such a direct SELECT statement.

These concerns:

7. The term 'use input format 1' is a bit unfortunate; i.e. it is not very user friendly.

Or "developer friendly" really, since this machine name is never shown to end-users. However, I think it's more appropriate than trying to keep track of the name of input formats as they are changed by administrators.

8. Does the schema definition needs to be updated?

We've already updated the schema definition to remove the "roles" column in this patch. Other changes to the schema shouldn't be necessary.

Finally Davids concern about profile.module:

Finally, there's an interesting question of what to do with the profile module [...] now, we're using the default input format by role, which is possibly even a little weirder (since the user's default input format can change depending on whether they filled out the profile before or after they got their account, etc). It might be better if these textareas were fully input-format enabled [...]

I think the implementation in this patch is suitable for the time being and shouldn't be handled here. If anything, the users permissions to input formats will increase if they become an authenticated user, so I don't see much harm in switching from the anonymous default to the authenticated default (or other defaults depending on the changing roles of the user). We'll handle this with a true input format for profile fields if we feel it's necessary in a followup issue.

Attachment	Size
format_permissions_and_defaults.patch	38.03 KB

Re #113:

Yes, db_select() should be used only if you are building a query conditionally and/or need the extra modifiability of query_alter and such. There's no reason to use it here. In fact using db_select() with a literal query, as this patch does, is a syntax error. :-)

However, db_fetch_* is deprecated so if we're going to DBTNG-ify the patch we should replace that with foreach(), like so:

However, we can now do even better, since this is a common idiom:

That one line replaces the loop above.

However, the UPDATE query in filter.admin.inc DOES need to use a builder, because all insert/update statements now do. To wit:

(The insert statements in the .install file we can fix later when we update system.install itself.)

Wow, thanks for the quick update of the patch (and thanks Dries/Crell for the early feedback). @quicksketch, you are giving me too much credit, since actually it was your earlier patch above that was the first one to merge the two issues together, and I was just working off of that ;)

Anyway, it seems we still have a fair amount of feedback to incorporate. I should have some time over the weekend to work on this and hopefully can make some of the small changes that still remain (and maybe one or two larger things, such as writing tests). However, I do want to add a couple comments now about the biggest change in the latest version of the patch:

This revised patch makes a few critical changes, which I hope truly makes the "fallback" format nothing but a fallback.

- The fallback format is hard-coded into the module, no longer stored in the database.
- The fallback format is given the defined constant ID of 0, so we don't need to keep a variable for which format is the fallback.
- The fallback format is never listed if users have access to other formats. Instead it is only available if a user has access to no other formats at all.

I see a potential problem with the last point in that list. What happens if a user with no permissions creates content in the fallback format, and then an administrator comes along and edits it? I'm not sure exactly what would happen (I've only skimmed the latest patch, not actually tried it), but it seems like the administrator would be forced to save the content in a new format, which would be not-so-good for a variety of reasons (the original author would no longer have access to it, it would be easier for an XSS attack to slip through, etc). Is that correct? I think that changing content to a new input format should always be a conscious decision.

So... it was actually issues like that which made me want to move away from the original approach of making the "plain text" fallback format completely hardcoded. By storing it in the database, I think we actually do gain some good things: Instead of it being a "second-class input format", it gets treated more like a normal input format (e.g., you can rearrange it on the input format administration page like the others, etc). Also, the more I thought about it, the more that actually made sense to me, because in many situations "plain text" is exactly what you want to type, and the ability to add even "filtered HTML" is too much freedom and can sometimes cause problems (for example, imagine you are typing algebra into the body of the node, and watch what happens if you type If x<y, then blah blah blah... into a textarea with the Filtered HTML format... yikes!).

So to me, it actually seems like a usability improvement to keep this fallback format in the database and make it available in all cases. However, my enthusiasm for this is definitely dependent on #304330: Text format widget (or something similar) getting in, because the input format fieldset right now is probably not user-friendly enough to want to make it suddenly appear for lots of users...

Any thoughts? Sounds like we might want to try to come to a consensus on this last big issue before going too much farther with the code.

Thanks David, I think you're right. I didn't consider a user with higher privs editing an existing piece of content. I'm still lingering in the thought process of input formats before #304330: Text format widget gets in. Once that's in place, always showing the fallback format isn't so bad anymore. I also see the flexibility you built into having the fallback format be a variable, since that means an administrator could potentially setup a new format, then set a variable in $init and effectively create a new fallback format with a module at all.

So let's scrap most of my changes in #113 and start again from #111. I think it'd still be nice to find a way to not show the fallback format if the user has access to other ones (so it's really a last-resort sort of format), but there are all kinds of special exceptions that arise in trying to do such a thing. So it comes down to either having an inconsistent display of the fallback (in the case of an admin editing a piece of content that uses the fallback) or basically having at least 2 formats for every user (filtered HTML + the fallback).

Sorry didn't mean to change the title. I'll unassign myself also.

Sorry for the delay in getting a new patch posted. I think this version is basically ready, except for tests (it breaks some old ones, and needs new ones to be written).

Changes since my last patch:

• Incorporated the feedback from Dries and Crell above, including some that was rolled in by @quicksketch in the other patch. Also incorporated my own (much older) feedback from #110242: make access to filters first class permissions (most importantly fixing the php.module installer).
• Lots of small bug fixes found reviewing my own patch.
• Security/usability: Added a message at the top of each input format configuration page that informs the site admin which (if any) roles have access to the input format (since we took away the configuration form that previously provided that info).
• Security: In the case of the plain text format, added a call to filter_xss() after all filters have been applied (in addition to the check_plain call at the beginning) to make sure it bears some resemblance to "plain" regardless of what filters have run on it.
• Biggest change: I realized that the filter update function had some issues, in particular with regard to the (former) default input format. It seems that some places in Drupal allowed the FILTER_FORMAT_DEFAULT constant to actually be stored in the database (which would then be resolved into the actual filter format each time check_markup was run on the text). This won't work anymore, so I needed a DB update to convert everything in the core tables to use the explicit default format. Then, we have to consider what would happen if a contrib module had similar issues and forgot to make this change... check_markup() wouldn't know what to do with the unknown format. So I had to refactor check_markup() a bit to make it more robust in this situation.

Attachment	Size
format_permissions_and_defaults_3.patch	53.85 KB

subscribe

Subscribe.

['field_kampagne_baggrund']

Subscribe.

Sorry for the slow-motion nature of this patch. However, I've just put a ton of work into this and now have a patch that is ready for final review!

My DrupalCon talk on text formats next week will be a lot better if this is in core before then, so I would like to push as hard as I can here and hopefully get something in this week :).... the attached patch was built up over a long time based on the work of several people, so hopefully we can get it in.

The changes from the previous version that are worth noting are as follows:

• Fixed existing tests and added a whole bunch of new ones. This basically required an almost complete rewrite of the existing filter.test test cases.
• Removed some code from the previous patch which was designed to subtly address a problem that eventually turned into a Drupal 5-6 security fix back in December.... this is because the security issue for Drupal 7 is now being handled at #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x. However, this patch should be consistent with the fix that is planned there.
• For the case of the fallback format, I removed the hardcoded calls to check_plain()/etc. that were in the previous version of this patch, and instead escaped HTML using the normal method (i.e., by including the appropriate filter in the default configuration of this fallback format). The hardcoded calls were originally added out of a desire for security, but ultimately, they led to too much special-case code and not enough flexibility. The real security here is provided by the fact that Drupal core is now providing a safe "plain text" format for all users and does not allow the site administrator to edit this format and make it unsafe. If someone wants to change the fallback format by hand in settings.php, or write a contrib module that allows site administrators to change it, ultimately we have to assume they know what they're doing, and trying to hardcode these checks within the core API just gets in the way of that. (In fact, on certain types of sites, there might be a legitimate use case for having the fallback format be something unsafe, so Drupal core shouldn't forbid it at the API level.)
• Finally, I discovered some extra complication in the upgrade path due to the fact that in Drupal 6, a format code of "0" (FILTER_DEFAULT_FORMAT) can be used in two different ways. This is explained more fully in the code comments, but the end result is that we have to preserve some of this ability via a new defined constant (FILTER_FORMAT_UNASSIGNED). Mainly this is there to support node types that do not have a body field enabled (they need to store something in the "format" column of the database). It would be nice to get rid of this, but I think we're stuck with it for now (probably it can go away if/when the node body gets converted to use the Field API, though).

Please review and test if you want to see this get in... thanks!!

Attachment	Size
format_permissions_and_defaults_4.patch	87.25 KB

So much for last week - let's try again...

Please understand that the patch is not as big as it looks since a lot of it is tests and a lot of it is scattered changes which make the patch file appear bigger. This actually should be relatively straightforward to review.

To whet your appetite, here are some screenshots of the patch in action.

First, text formats are now on the standard Drupal permission page. In this example, only administrators have been granted access to Full HTML, while other roles have fewer permissions:

Clicking on the "Filtered HTML" link to edit that format, there is a warning that reminds me who has access to it so that I don't grant them access to anything dangerous:

Going to the main text format administration page, I've arranged the formats so that Full HTML is on top:

Consequently, when I log in as a user with the administrator role, Full HTML is my default:

And when I log in as a regular authenticated user, Filtered HTML is my default, since it's the first one on the list I have access to:

Finally, as an anonymous user, I was not granted permission to use any formats, so the content I create will automatically be in plain text:

Attachment	Size
text_formats_permission_page.png	46.32 KB
text_formats_edit_page.png	67.96 KB
text_formats_admin_page.png	18.47 KB
text_formats_create_content_admin.png	41.69 KB
text_formats_create_content_regular_user.png	34.71 KB
text_formats_create_content_anonymous_user.png	31.12 KB

Does this provide any format-per-node-type control, or at this point format-per-field? (I have always missed that far far more than per-role control, which I've never understood the need for.)

Does this provide any format-per-node-type control, or at this point format-per-field? (I have always missed that far far more than per-role control, which I've never understood the need for.)

Crell: The per-role control is quite mandatory in terms of **permissions**, and input formats are obviously used outside of the content types realm, so let's get the permissions issue solved first, and sort out the rest later!

Exactly. From my point of view, this issue is mainly about getting the text format permissions working correctly, and the per-role defaults is just a nice side effect of doing that the right way (although it's a feature that a lot of people want, obviously, based on the comments above).

So the answer is no, this issue does not do anything for per-node-type or per-field text format controls. However, a contrib module that cleanly provides this will probably be a lot easier to write in D7, due to the new #text_format property introduced in #125315: Textarea with input format attached. Also see #375907: Make text field use D7 '#text_format' selector which is probably mandatory if you want per-field.

subscribing.

Tagging so it's on the radar of the usability team.

I applied this patch to a working install of head.

First, it made the PHP format filter available to everyone, including anonymous users. Whoops!

Then, when I tried to run update.php, I was informed that 'An HTTP error 500 occurred.'

Watchdog says,
PDOException: INSERT INTO {role_permission} (rid, permission) VALUES (1, 'use text format 1') - Array ( ) SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1-use text format 1' for key 1 in filter_update_7003() (line 153 of /var/www/vc/drupal-head/drupal/modules/filter/filter.install).

Note that it does seem to work sensibly if the patch is applied pre-install. I have not tested a D6 upgrade, so maybe there's no real issue, since I did things backward.

Thanks for testing this. Sorry, I should have mentioned before: You do indeed need to apply the patch before installing D7, since the patch affects the installation process. So I'll set this back to "needs review" for now.

Tests of the D6 upgrade process would be great. I tried it a bit myself while writing the code and it seemed to work, but the upgrade path is a bit complicated and would be very good to test more. So if anyone has a good backup of a D6 site they could try it on (especially a site with a history of input formats being created/deleted/etc...) that would definitely help a lot.

The last submitted patch failed testing.

subscribing

Coming back to this... here is an updated patch to chase HEAD.

Attachment	Size
format_permissions_and_defaults_5.patch	87.69 KB

Also, I think this issue should be considered critical. It removes a major source of confusion that leads many, many Drupal sites to be configured insecurely. Site administrators I've talked to are desperately looking for text formats to be handled through the normal permission system rather than through the odd back door channel that they are now, and that's what this patch provides. So let's not release Drupal 7 without it :)

Please do not commit yet I will give a review when I am not sleepy -- tomorrow morning. Just asking.

ilter_admin_display_warning has a doxygen summary of more than one line. that's surely wrong -- but aside from trivial formatting issue, the text makes little sense "Display a warning when the administrator is editing a format that potentially-untrusted users have permission to use." -- what about just "Display a warning about which roles can use the format" or something like that?

filter_format_load should be collapsed into $formats = filter_formats(); return isset($formats[$id]) ? $formats[$id] : FALSE but also can't we add an optional parameter to filter_formats..? Even more so, as I am looking into filter_formats, it already has that optional parameter so can this function just be filter_formats($id) ?

"Get a list of formats for this user, ordered by weight. The first one available is that user's default format." -- "the user's, please.

blogapi_validate_user() authenticates and sets the global $user object.

Why can't we directly store the format in 'body_format' ?

I think we should call filter_fallback_format() outside of the loop.

Can we squeeze some line breaks in there?

Ugh. Until here, this looked promising. There are countless of modules that store the text format for configurable text, but also for user content. At the very least, they should be able to add their tables/columns to this list. However, even then, or even when deferring this to additional form submit handlers added by other modules, this part makes me worry - not only about DX, but primarily about security.

Like chx pointed out, summary should be on one line. A longer description can follow after the summary though.

Double "last" in here.

In the past, filter.module used its own permission handling. Now that it uses regular user permissions, either me or the patch is missing a bit here: When adding/deleting formats, hook_perm() has to run again.

I think the Standardize caching patch went in to prevent further adding of $reset arguments...?

IMHO, we should avoid such code. Either the system works or it does not. Magic is the last thing we need with regard to weird text formats. +1 for removing that additional return.

3? Why 3? (Don't explain me, explain it in the code)

After reading more and more code and this constant used in some places to use the fallback format, I really don't understand why the constant is named "UNASSIGNED" instead of "FALLBACK".

Sorry, but this finally makes me believe that this patch contains gazillion of other improvements in one giant patch.

Likewise, this part is the heart of #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x...

"...if a format id was provided".

Next bit I don't grok: The special format -1 seems to be removed by this patch?

FALSE? Another bug fixed while being there?

Sorry, but I must repeat: A monster patch trying to fix and improve too many things at once.

Thanks for the detailed review, guys! Some responses are below. Points you brought up which I did not specifically respond to are those which I agreed with 100%, so I'll definitely be making those changes in the next version of the patch (which hopefully I'll be able to get to tomorrow). In the meantime:

filter_format_load should be collapsed into $formats = filter_formats(); return isset($formats[$id]) ? $formats[$id] : FALSE but also can't we add an optional parameter to filter_formats..? Even more so, as I am looking into filter_formats, it already has that optional parameter so can this function just be filter_formats($id) ?

The old version of filter_formats() had that parameter, but the new one actually doesn't... so I guess I'll go with your first suggestion. I'm not sure I can think of a good reason to keep that parameter around, given the way the new code works?

+  $edit['body_format'] = $edit['format'];
+  unset($edit['format']);

Why can't we directly store the format in 'body_format' ?

We might be able to, but the array gets sent off to various hooks and such beforehand, and I didn't want to mess with existing behavior in this patch. Actually, though, it's possible this code doesn't have to be in this patch anymore at all... It did originally, because it fixed a broken test, but I think the patch may have changed enough since then that the test no longer fails. I think I'll just try leaving this out and seeing what happens :) If it works, then I'll file this part as a separate bug report.

+  // Replace existing instances of the deleted format with the fallback format.
+  $fallback_format = filter_fallback_format();
+  $database_tables = array('node_revision', 'comment', 'box');
+  foreach ($database_tables as $table) {
+    if (db_table_exists($table)) {
+      db_update($table)
+        ->fields(array('format' => $fallback_format))
+        ->condition('format', $form_state['values']['format'])
+        ->execute();
+    }
+  }

Ugh. Until here, this looked promising. There are countless of modules that store the text format for configurable text, but also for user content. At the very least, they should be able to add their tables/columns to this list. However, even then, or even when deferring this to additional form submit handlers added by other modules, this part makes me worry - not only about DX, but primarily about security.

I agree with you completely, but that code was not added by the current patch :) The existing code already does the same thing. All this patch does is change the queries from using the default format to using the fallback format. In doing so, I also DBTNG-ified the query (which maybe was a mistake since it made it more confusing to review, but at one point I believe there was an unofficial rule for D7 that if your patch changes a database query, it should also DBTNG-ify it...)

We should definitely create a separate issue for this. At the very least, yes, this needs to be done via a module_invoke_all() rather than a hardcoded list of database tables.

In the past, filter.module used its own permission handling. Now that it uses regular user permissions, either me or the patch is missing a bit here: When adding/deleting formats, hook_perm() has to run again.

Hm, does it? That would be true if the results of hook_perm() were stored in a cache somewhere, but I don't think they are? If I'm right and they're not, then I think it's OK. The next time someone runs hook_perm(), whenever that happens, the new permissions should automatically be picked up.

I think the Standardize caching patch went in to prevent further adding of $reset arguments...?

I sort of thought so too, but that's not what Dries says. Since the whole static caching thing is still a bit up in the air, I think it's probably best if we leave the $reset parameter there for now? It can always be removed later if we need to, and there are plenty of others in core already so it's not like it's inconsistent to add a new one.

-define('FILTER_FORMAT_DEFAULT', 0);
+define('FILTER_FORMAT_UNASSIGNED', 0);

After reading more and more code and this constant used in some places to use the fallback format, I really don't understand why the constant is named "UNASSIGNED" instead of "FALLBACK".

I'm sort of on the fence with this one, but I think "UNASSIGNED" makes more sense. The constant is used to represent content that does not have any format assigned to it whatsoever. Now, we do treat such content the same way we treat content with the fallback format (e.g., when displaying it), but maybe someone else will want to write code that treats it differently, so the constant should be defined in a way that makes sense for them too. Now, in the long run, we might be able to change it to "FALLBACK" or hopefully just get rid of it -- some of that will depend on #372743: Upgrade docs for body and teaser as fields -- but at the moment, a value of 0 can appear in the "format" column of the {node_revision} table to indicate that a node has never been assigned a format, so I think we should leave it.

-    if ($cache) {
+    if (filter_format_allowcache($format)) {
       cache_set($cache_id, $text, 'cache_filter', REQUEST_TIME + (60 * 60 * 24));

Sorry, but this finally makes me believe that this patch contains gazillion of other improvements in one giant patch.

Oops, sorry. That's a leftover from an earlier version of the patch where we needed to do some reorganizing of check_markup(), but in the end we did not need to do any so that should have been removed with the rest of the changes. I'll remove it and file it as a separate issue.

+  // If the user doesn't have access to the currently-selected format, it
+  // could be because the format doesn't exist any more. The fallback text
+  // format is designed for this situation, so we pre-select that format
+  // on the form.
+  if (!isset($formats[$value])) {
+    $value = filter_fallback_format();
+  }

Likewise, this part is the heart of #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x...

Nope, not the heart of it -- #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x is mainly about making sure the text is filtered securely, but this is about preselecting the correct format on the form when the text is being edited. It's a little bit of a chicken and egg problem (since the fallback format is only being introduced here, in this patch), but I think you're probably right that this code can be edited to refer to the old default format and then moved to the other issue for now. I'll double check that's the right thing to do, then do it :)

Next bit I don't grok: The special format -1 seems to be removed by this patch?

No, I don't think this patch touches that. As far as I can tell, http://api.drupal.org/api/function/filter_tips_long/7 is the only place that uses this special -1 format? If so, it seems like we don't need to make any changes to it in this issue.

@@ -299,7 +299,7 @@ function profile_view_field($user, $fiel
     switch ($field->type) {
       case 'textarea':
-        return check_markup($value);
+        return check_markup($value, filter_default_format($user), '', FALSE);

FALSE? Another bug fixed while being there?

Nope, this change is required by the patch. Before, the way the code worked was that profile textareas were always displayed using the sitewide default format. Now, since there is no site-wide default, we do the next closest thing which is to use the default format of the user who created it. But since the person viewing the profile is not guaranteed to have access to that format, we have to specify the FALSE to make sure they can see it.

This whole thing is a bit of a mess, of course, but in discussion earlier in this issue (see e.g. comment #113 by @quicksketch) we decided we didn't want to go any further with it in this patch. Hopefully, #301071: Replace parts of Profile Module with Fields API will make this all obsolete anyway :)

-    // Find the new role ID - it must be the maximum.
-    $all_rids = array_keys($this->admin_user->roles);
-    sort($all_rids);
-    $this->rid = array_pop($all_rids);
+    $this->rid = $this->drupalGetNewestRole($this->admin_user);
   }

Sorry, but I must repeat: A monster patch trying to fix and improve too many things at once.

I had a feeling someone was going to say something about that one :) Let me try to explain why I put this in the patch. First, I need to introduce this drupalGetNewestRole() function since I used it in the filter tests. So I looked through the existing code to see if there were any existing functions that did this already, and I found that both user.test and upload.test had their own (slightly different) implementations. So at that point, I could have added a third implementation, but that basically would have been duplicating code already in Drupal. So I thought it was better to consolidate them as part of this patch. If you really feel strongly about it, I could do it differently, though.

Otherwise, let me know if you think there are other parts of the patch that aren't strictly necessary. I'm guessing I went a tiny bit overboard while fixing the PHP module tests, but the thing is, it seems pointless to "fix" a test when you look at it and realize it actually isn't even testing anything the way it's currently written :) I'm sure I snuck in some improvements to filter.test as well, but this patch broke so many of the old tests that it basically required rewriting that whole file anyway, so I'm not sure if it makes sense to try to distinguish.

Here is an updated patch that I believe addresses all the other issues that need to be addressed.

I've also created the following issues based on things that were removed from this patch or otherwise discussed above:

• #428242: Minor code cleanup for caching in check_markup()
• #428264: Blog API does not pass along text formats correctly
• #428296: Filter system doesn't communicate with modules about altered text formats

And I also moved a small amount of the code discussed above to the existing patch at #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x.

Attachment	Size
format_permissions_and_defaults_6.patch	85.85 KB

The last submitted patch failed testing.

I am reading the patch and really do not like the approach taken. You are doing all sorts of wizardry every time we need the default format and IMO that's not a good idea. Currently we have user->roles filled in by session with a key of rid and a value of the role name. However, the role name is only displayed on the user edit page so we could add something else there -- we could use somethign else there. Let's add a unique identifier to each format which is ordered by weight -- it changes only on weighting -- and in role table keep the number for the lowest weight format -- the default format for that role. If you do that then the default format of a user is min($user->roles). Simple enough?

Edit: actually it's all $all_formats[min($user->roles)] but we can keep $all_formats in an array as it only contains two integers per input format so it's a very small array.

Well, I think one problem with that approach is that it's mixing together the user roles and text formats way too much. If I do print $user->roles, I would very much like to see a list of roles, not a list of text formats :)

In addition, the way the patch currently works, we always use filter_access() to determine the allowed text formats. That is simple, clean and very safe. I'd be a little uncomfortable caching the fact that "role X has format Y as its default" in a totally different place, because then if role X ever loses access to format Y and the {role} table doesn't get correctly updated, suddenly people might start getting access to formats that they aren't supposed to. For example, every time someone submits the permissions page, text format permissions can change, so we'd have to check for that and update the {role} table, etc, and that is just one avenue where this can happen. Theoretically, if Drupal had perfect APIs and everyone always used the APIs, this would be possible to solve, but that's not likely to happen any time soon :) Given how important text format access is for security, I'd much rather sacrifice a tiny bit of performance in exchange for making the system as safe as it possibly can be.

Also, how bad is the performance really? Well, you're right that the filter_default_format() function introduced by this patch is doing more than it needs to. It calls the filter_formats() function, which loads all the formats from the database and then checks filter_access() on each of them, even though we only care about the first one. However, there are a couple things to point out. First, filter_formats() caches it results, so this only happens once per page load. Second, let's look at where the filter_default_format() function is being used. When I reroll the patch, I should definitely add some comments in the PHP docblock to this effect, but the idea is, this is not a function that people should be calling that often. The vast majority of time you need to know the default format, it's because you're constructing a form and figuring out what to preselect. In those cases, you're already loading all the formats and checking their access on that page load anyway, so it's no big deal, right? There do exist special cases where you might need the default format outside that context, but it seems like they're less important to optimize for. (Somewhat embarrassingly, though, at the moment one of these "special cases" is actually "on every page load, if you have text.module enabled" -- but that's a transient issue that I believe will go away once #375907: Make text field use D7 '#text_format' selector gets in.)

So I definitely agree that we should optimize filter_default_format() a bit if we can, but I don't think it's worth going overboard with it.

Major +1 for this - and it's amazing the issue's been active for years now. Probably one of the most annoying missing features from Drupal. I'm almost tempted to say 'screw security' and let all roles use Full HTML because it works with WYSIWYG so much better.

@geerlingguy: This patch does not improve the text format access permission disaster people are facing with text formats and client-side editors.

On that note, and as mentioned in #414424: Introduce Form API #type 'text_format' I'd like to see us cleaning up filter.module, so it can be optional, meaning: People can think about and start to work on better approaches in contrib. The current filter system had its time. At least I am slowly getting a vision of how it should and needs to work.

@ sun - after posting yesterday, I found out about the "Better Formats" module, and now I'm satisfied. Some of the features in that module would be nice to have in core, though ;-)

sub

sub

.

sub

sub