it should be possible to have one default input filter for comments and another one for content.
or the default input filter should depend on the user role.

that's not an urgent feature request, but it would make it more efficent.

Files: 
CommentFileSizeAuthor
#214 drupal.text-formats-default.patch1.46 KBsun
Failed: Failed to apply patch.
[ View ]
#210 drupal-filter-perms.211.patch83.59 KBsun
Passed: 13099 passes, 0 fails, 0 exceptions
[ View ]
#209 drupal-filter-perms.209.patch82.57 KBsun
Failed: 13086 passes, 11 fails, 0 exceptions
[ View ]
#207 drupal-filter-perms.207.patch82.08 KBsun
Failed: 13085 passes, 11 fails, 0 exceptions
[ View ]
#205 format-permissions-and-defaults-11218-205.patch82.07 KBmgifford
Failed: Failed to apply patch.
[ View ]
#203 format-permissions-and-defaults-11218-203.patch82.04 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#198 format-permissions-and-defaults-11218-197.patch83.02 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#193 drupal-filter-perms.193.patch83.5 KBsun
Failed: 13036 passes, 7 fails, 0 exceptions
[ View ]
#190 format-permissions-and-defaults-11218-190.patch82.53 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#180 format-permissions-and-defaults-11218-180.patch82.54 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#176 format-permissions-and-defaults-11218-176.patch82.87 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#175 format-permissions-and-defaults-11218-175.patch82.81 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#171 format-permissions-and-defaults-11218-171.patch81.69 KBdropcube
Failed: Failed to apply patch.
[ View ]
#168 format-permissions-and-defaults-11218-168.patch83.62 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#162 format-permissions-and-defaults-11218-162.patch82.39 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#159 format-permissions-and-defaults-11218-159.patch51.56 KBDavid_Rothstein
Failed: 12362 passes, 42 fails, 3 exceptions
[ View ]
#143 format_permissions_and_defaults_6.patch85.85 KBDavid_Rothstein
Failed: 10853 passes, 0 fails, 1 exception
[ View ]
#137 format_permissions_and_defaults_5.patch87.69 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#126 text_formats_permission_page.png46.32 KBDavid_Rothstein
#126 text_formats_edit_page.png67.96 KBDavid_Rothstein
#126 text_formats_admin_page.png18.47 KBDavid_Rothstein
#126 text_formats_create_content_admin.png41.69 KBDavid_Rothstein
#126 text_formats_create_content_regular_user.png34.71 KBDavid_Rothstein
#126 text_formats_create_content_anonymous_user.png31.12 KBDavid_Rothstein
#125 format_permissions_and_defaults_4.patch87.25 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#118 format_permissions_and_defaults_3.patch53.85 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#113 format_permissions_and_defaults.patch38.03 KBquicksketch
Failed: Failed to apply patch.
[ View ]
#111 format_permissions_and_defaults_2.patch42.31 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#100 format_permissions_and_defaults.patch35.78 KBquicksketch
Failed: Failed to apply patch.
[ View ]
#99 Picture 2.png66.97 KBquicksketch
#63 filter_16.patch5.75 KBawood456
#22 filter.module.patch.txt832 bytesduncf

Comments

Title:default input filterDefault input formats per role
Version:4.5.0-rc»

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).

Version:» 4.7.0-beta4
Priority:Normal» Critical

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)

Version:4.7.0-beta4» x.y.z

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:

// Set the id of the top-level form tag
  $form['#id'] = 'node-form';

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

if (!isset($node->nid))

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

StatusFileSize
new832 bytes

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.

Priority:Critical» Normal

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

Status:Active» Needs work

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

Status:Needs work» Active

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.

Version:x.y.z» 4.7.x-dev

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

Version:4.7.x-dev» 6.x-dev

Features go against next version.

Status:Active» Fixed

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

Status:Fixed» Active

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:

<?php
print check_markup($node->body, 5); // substitute 5 with whatever format you want to use.
?>

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

<?php
if (in_array('administrator', $user->roles)) {
  print
check_markup($node->body, 5);
}
else {
  print
check_markup($node->body, 1);
}
?>

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.

StatusFileSize
new5.75 KB

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.

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

Version:6.x-dev» 7.x-dev

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.

Status:Active» Closed (won't fix)

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.

Status:Closed (won't fix)» Active

@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.

StatusFileSize
new66.97 KB

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.

Assigned:Unassigned» quicksketch
Status:Active» Needs work
StatusFileSize
new35.78 KB
Failed: Failed to apply patch.
[ View ]

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

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.

StatusFileSize
new42.31 KB
Failed: Failed to apply patch.
[ View ]

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.

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.

-      drupal_set_message(t('The default format cannot be deleted.'));
+      drupal_set_message(t('The %format format cannot be deleted.', array('%format' => $format->name)));

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.

+    $result = db_query('SELECT * FROM {filter_formats} ORDER BY weight');
+    while ($format = db_fetch_object($result)) {
+      $formats['all'][$format->format] = $format;
+    }

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.

StatusFileSize
new38.03 KB
Failed: Failed to apply patch.
[ View ]

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.

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:

<?php
    $result
= db_select('SELECT * FROM {filter_formats} ORDER BY weight'); // This should be db_query()!
   
foreach ($result as $format) {
     
$formats['all'][$format->format] = $format;
    }
?>

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

<?php
$formats
['all'] = db_query('SELECT * FROM {filter_formats} ORDER BY weight')->fetchAllAssoc('format');
?>

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:

<?php
// This
db_query("UPDATE {filter_formats} SET cache = %d, name='%s' WHERE format = %d", $cache, $name, $format);
// Becomes this
db_update('filter_formats')
  ->
fields(array(
   
'cache' => $cache,
   
'name' => $name,
  ))
  ->
condition('format', $format)
  ->
execute();
?>

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

Title:Default input formats per roleAllow default input formats per role, and integrate input format permissions

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.

Title:Allow default input formats per role, and integrate input format permissionsDefault input formats per role

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).

Title:Default input formats per roleAllow default input formats per role, and integrate input format permissions
Assigned:quicksketch» Unassigned

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

StatusFileSize
new53.85 KB
Failed: Failed to apply patch.
[ View ]

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.

subscribe

Issue tags:+Favorite-of-Dries

Issue tags:+wysiwyg

Subscribe.

['field_kampagne_baggrund']

Subscribe.

Status:Needs work» Needs review
StatusFileSize
new87.25 KB
Failed: Failed to apply patch.
[ View ]

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!!

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:

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.

Status:Needs review» Needs work

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.

Status:Needs work» Needs review

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

subscribing

Status:Needs work» Needs review
StatusFileSize
new87.69 KB
Failed: Failed to apply patch.
[ View ]

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

Priority:Normal» Critical

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.

Status:Needs review» Needs work

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.

@@ -202,7 +202,7 @@ function blogapi_blogger_new_post($appke
-  $edit['format'] = FILTER_FORMAT_DEFAULT;
+  $edit['format'] = filter_default_format($user);

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

@@ -231,6 +231,12 @@ function blogapi_blogger_new_post($appke
+  // Attach the node format to the body field, in the manner expected by the
+  // validation and submission handlers.
+  // @see form_process_text_format()
+  $edit['body_format'] = $edit['format'];
+  unset($edit['format']);

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

   foreach ($formats as $id => $format) {
+    // Check whether this is the fallback text format. This format is
+    // available to all roles and cannot be configured via the admin
+    // interface.
+    $is_fallback_format = ($id == filter_fallback_format());

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

+function filter_admin_delete(&$form_state, $format) {
...
+  return confirm_form($form, t('Are you sure you want to delete the text format %format?', array('%format' => $format->name)), 'admin/settings/filter', t('If you have any content left in this text format, it will be displayed as %plain_text until you manually assign it a new format. This action cannot be undone.', array('%plain_text' => filter_fallback_format_title())), t('Delete'), t('Cancel'));
}

Can we squeeze some line breaks in there?

+  // 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.

+/**
+ * Move text format access to the user permissions handler, create a fallback
+ * (plain text) text format, and explicitly set the text format in cases that
+ * used to rely on a single site-wide default.
+ */

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

+function filter_update_7003() {
...
+  // Add a fallback text format which appears last on the last for all users.

Double "last" in here.

@@ -221,26 +193,15 @@ function filter_admin_format_form_submit
+  db_update('filter_format')
+    ->fields(array(
+      'cache' => $cache,
+      'name' => $name,
+    ))
+    ->condition('format', $format)
+    ->execute();
+  filter_format_reset_cache();
   cache_clear_all($format . ':', 'cache_filter', TRUE);
@@ -148,12 +171,45 @@ function filter_admin_format_title($form
function filter_perm() {
...

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.

+ * Retrieve a list of text formats, ordered by weight.
+ *
+ * @param $account
+ *   Optional. If provided, only those formats that are allowed for this
+ *   user account will be returned. All formats will be returned otherwise.
+ * @param $reset
+ *   Whether to reset the internal cache of all text formats. Defaults to
+ *   FALSE.
+ */
+function filter_formats($account = NULL, $reset = FALSE) {
+  $formats = &drupal_static(__FUNCTION__, array());
+  if ($reset) {
     $formats = array();
+  }

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

+function filter_default_format($account = NULL) {
...
+  // All users have access to the fallback format, so we use that here if
+  // no formats were available above (however, under normal circumstances
+  // this code should never be reached).
+  return filter_fallback_format();

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.

+/**
+ * Return the ID of the fallback text format that all users have access to.
+ */
+function filter_fallback_format() {
+  return variable_get('filter_fallback_format', 3);
+}

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

/**
- * Special format ID which means "use the default format".
+ * Special format ID which is used to indicate that a piece of text does
+ * not have a text format assigned.
  *
- * This value can be passed to the filter APIs as a format ID: this is
- * equivalent to not passing an explicit format at all.
+ * This value can be passed to the filter APIs as a format ID; this is
+ * equivalent to not passing an explicit format at all. In particular, if
+ * text with this format is ever displayed, it will be filtered using the
+ * fallback format available to all users.
  */
-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".

@@ -435,9 +536,6 @@ function check_markup($text, $format = F
-    // See if caching is allowed for this format.
-    $cache = filter_format_allowcache($format);
...
     // Store in cache with a minimum expiration time of 1 day.
-    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.

+function filter_form($value = FILTER_FORMAT_UNASSIGNED, $weight = NULL, $parents = array('format')) {
...
+  // 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...

+function filter_access($format, $account = NULL) {
...
+  // Otherwise, retrieve the full format object if one was not provided.
+  if (!isset($format->format)) {
+    $format = filter_format_load($format);
   }

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

@@ -548,11 +718,13 @@ function filter_access($format) {
  * Helper function for fetching filter tips.
  */
function _filter_tips($format, $long = FALSE) {
+  global $user;
+
   if ($format == -1) {
-    $formats = filter_formats();
+    $formats = filter_formats($user);
   }

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

@@ -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?

--- modules/user/user.test 31 Mar 2009 01:49:55 -0000 1.33
+++ modules/user/user.test 7 Apr 2009 04:33:12 -0000
@@ -724,13 +724,8 @@ class UserPermissionsTestCase extends Dr
   function setUp() {
     parent::setUp();
-
     $this->admin_user = $this->drupalCreateUser(array('administer permissions', 'access user profiles'));
-
-    // 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.

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: 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: Remove profile module from core 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.

Status:Needs work» Needs review
StatusFileSize
new85.85 KB
Failed: 10853 passes, 0 fails, 1 exception
[ View ]

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:

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.

Status:Needs review» Needs work

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

Issue tags:+FilterSystemRevamp

.

sub

sub

Issue tags:+input filter, +input formats

Subscribe.

For Drupal 6.x, the Better Formats module fixes this. http://drupal.org/project/better_formats (then go to Defaults tab on Input Filter config page).

For Drupal 5.x., try out Filter Default module http://drupal.org/project/filter_default

Code freeze is coming soon. Let's get this in.

Here is an updated version of the patch that works with the latest HEAD. Two changes are worth noting:

  1. Since the node body is now a field, we can happily get rid of the ugly constant FILTER_FORMAT_UNASSIGNED discussed above - it's no longer necessary. This simplifies the patch in some places.
  2. Thinking a bit more about what we're doing here, there is no good reason to prevent people from editing the fallback (plain text) format, as the previous patches did. There are legitimate reasons they may want to configure it a bit - e.g., to enable the URL filter - which we don't really want to prevent. Allowing this also makes the fallback format a bit less of a special case. In addition, for people upgrading from Drupal 6, we are forcing a new "plain text" format to appear on their site and automatically be available to all users, so if we don't give them any ability to configure it, it might conflict in odd ways with other formats they already have.

    So, the new version of the patch restores the ability to configure this format. This doesn't really change the security hardening aspects of the patch at all, in my opinion -- although we are allowing them to configure the fallback format via the UI, we are not allowing them to switch the format to something else, and the switching to something else is what would allow them to blow away all their permissions with one click of a radio button. As long as that's gone, we're OK security-wise, especially since we are printing a big warning on the filter configuration page anyway that tells them to be careful because all roles have access to the format.

Status:Needs work» Needs review
StatusFileSize
new51.56 KB
Failed: 12362 passes, 42 fails, 3 exceptions
[ View ]

Here is the patch. I've removed the SimpleTest changes from this patch for now so we can see where we're at with the testbot. I'll then add the tests back in and make the minimal changes to existing tests required to get all of them passing.

Status:Needs review» Needs work

The last submitted patch failed testing.

By the way, while testing out this patch I came across a number of bugs which turned out to be unrelated (i.e., I could reproduce all of them in an unpatched version of HEAD also):

#560738: Trying to delete a text format gives "page not found"
#560740: "Escape all HTML" filter does not escape any HTML
#560744: Filter module breadcrumbs are broken
#560750: Text format widget broken for users who have an empty vertical tabs section

Clearly, the filter module tests could use some work, if they do not pick up any of these bugs, especially the first two :)

I also can't get update.php to run (either with or without this patch), but I think that's a known issue - however, it makes testing the upgrade path currently impossible.

However, all the functionality of this patch that is possible to test seems to work for me with the latest code.

Status:Needs work» Needs review
StatusFileSize
new82.39 KB
Failed: Failed to apply patch.
[ View ]

Wow, every recent commit to HEAD seems to be breaking this patch.... but here is a version with the new tests added back in and relatively minimal fixes to existing tests to make them work (we can submit further test improvements as followup patches).

This patch also necessarily contains the fix from #560740: "Escape all HTML" filter does not escape any HTML, since without it (thankfully) the new tests would not pass.

Title:Allow default input formats per role, and integrate input format permissionsAllow default text formats per role, and integrate text format permissions

OK, tests passed (whew!).

Summary of the patch for reviewers who don't want to read the previous 160 comments:

  1. For reasons of security and usability, the patch changes the "roles which may use this text format" feature into standard Drupal permissions. They now appear on the permissions page like everything else, and they use the same underlying system.
  2. The previous concept of a "default text format" (which meant two things at once - the text format that all users automatically had access to, and the text format that was automatically preselected for all users when creating new content) is now gone. This is also a big security and usability win, because it means that it is no longer is it possible to accidentally grant all users access to a dangerous text format by clicking a radio button that you are intending to click in order to make Full HTML your personal administrative default.
  3. Appearing naturally out of the above is the "default text formats per role" feature. A user's default preselected text format (when they are creating new content) is now simply the first text format they have permission to use. Therefore, by sorting the list of text formats and appropriately controlling the permissions, the site administrator can achieve a large amount of freedom in assigning different default formats to different roles.
  4. Because we always need to be able to filter text somehow, and because we don't want a usability nightmare where the "create content" permission isn't actually enough to create content for users who don't have permission to use a text format, we also introduce the concept of a "fallback format". This is simply a format that all users have access to and that can't be deleted - otherwise, it is the same as any other text format. By default, the fallback format is plain text, but it can be edited by the administrator. Unfortunately, we require a bit of special case code in some places to deal with this format, but it replaces special case code that was there before to deal with the default format, so overall, it should be fine.

Screenshots:

The screenshots shown in #126 above are still relatively up to date. The main difference in terms of this patch is that the Plain Text format now has a "configure" link next to it (obviously there are other changes too due to other patches that went into core - for example, the improved text format widget).

The main usability issue I'm concerned about is that I'm not necessarily a big fan of using the yellow message to communicate the role information on the filter configuration page. Ideally, we need to somehow replace the role information that Drupal currently provides on that page (so people can see what roles the format has been assigned to, learn how to change them, and understand the security consequences) -- so suggestions for that are certainly welcome. However, I'd like to focus on getting this patch in before code freeze, so keep in mind that small UI changes can always be made later.

Next steps?

The patch was written at various times by Gábor, quicksketch, and me (maybe others?), and previous versions have been reviewed extensively, by Dries, sun, chx, etc. And the issue is very, very old :) So I think we probably just need one final review to drive this home before code freeze?

So far at first glance I see a pretty glaring problem with the caching. Rather than using filter_format_reset_cache();, we should just be using drupal_static_reset('filter_formats');. The $reset parameter should be removed entirely from the filter_formats() function, since that's what we have drupal_static() to begin with.

A few other concerns:

+ * Checks if a user has access to a particular text format.

Should be "input format", not "text".

+function filter_fallback_format() {
+  return variable_get('filter_fallback_format');
+}

A default needs to be provided to variable_get() as a second parameter, otherwise NULL is going to get passed in filter_format_load() and return FALSE right? I'm not quite sure what's going on here regarding the default format (is it supposed to be NULL)?

Status:Needs review» Needs work

+++ includes/form.inc 28 Aug 2009 20:10:51 -0000
@@ -1894,7 +1894,7 @@ function form_process_radios($element) {
+ *     '#text_format' => isset($node->format) ? $node->format : filter_default_format(),

(minor) Currently node bodies are fields, $node->format doesn't exist, so would be good to replace this with other real example.

+++ modules/filter/filter.module 28 Aug 2009 20:10:51 -0000
@@ -536,8 +667,8 @@ function filter_list_format($format) {
- *   The format of the text to be filtered. Specify FILTER_FORMAT_DEFAULT for
- *   the default format.
+ *    The format of the text to be filtered. If no format is assigned, the
+ *    fallback format will be used.

(minor) Wrong indentation.

+++ modules/filter/filter.module 28 Aug 2009 20:10:51 -0000
@@ -599,9 +732,16 @@ function check_markup($text, $format = F
+function filter_form($value = NULL, $weight = NULL, $parents = array('format')) {

$value is the format ID right ? Please, let's rename it.

I'm on crack. Are you, too?

Also correcting myself here:

Should be "input format", not "text".

"Text format" is correct in D7, I'm just not with the times apparently.

+++ modules/filter/filter.install 28 Aug 2009 20:10:51 -0000
@@ -260,3 +258,99 @@ function filter_update_7004() {
+        db_insert('role_permission')
+          ->fields(array(
+            'rid' => $format_role,
+            'permission' => filter_permission_name($format),
+          ))
+          ->execute();

Consider using the API function user_role_set_permissions() to do this.

+++ modules/filter/filter.install 28 Aug 2009 20:10:51 -0000
@@ -260,3 +258,99 @@ function filter_update_7004() {
+  $fallback_format = db_insert('filter_format')
+    ->fields(array(
+      'name' => 'Plain text',
+      'cache' => 1,
+      'weight' => 1,
+    ))
+    ->execute();

If a site already has a text format named "Plain text" this query will fail since {filter_format}.name is a unique key.

+++ modules/filter/filter.install 28 Aug 2009 20:10:51 -0000
@@ -260,3 +258,99 @@ function filter_update_7004() {
+  db_insert('filter')
+    ->fields(array('format', 'module', 'name', 'weight', 'status'))
+    ->values(array(
+      'format' => $fallback_format,
+      'module' => 'filter',
+      'name' => 'filter_html_escape',
+      'weight' => 0,
+      'status' => 1,

Also consider the API function filter_format_save() to do this.

+++ modules/filter/filter.install 28 Aug 2009 20:10:51 -0000
@@ -260,3 +258,99 @@ function filter_update_7004() {
+  foreach (array('block_custom', 'comment') as $table) {
+    db_update($table)
+      ->fields(array('format' => $default_format))
+      ->condition('format', 0)
+      ->execute();
+  }

We can't assume that these tables exist.

These updates should be handled in the corresponding modules.

Status:Needs work» Needs review
StatusFileSize
new83.62 KB
Failed: Failed to apply patch.
[ View ]

Thanks for the reviews:

  1. I fixed the indentation, user_role_set_permissions(), and filter_format_save() issues in the attached patch -- I think there may actually be a bug in the latter API function that might make it not right for this purpose, but using this API function is certainly the right thing to do in the long run, and we can fix bugs later.
  2. So far at first glance I see a pretty glaring problem with the caching. Rather than using filter_format_reset_cache();, we should just be using drupal_static_reset('filter_formats');. The $reset parameter should be removed entirely from the filter_formats() function, since that's what we have drupal_static() to begin with.

    I'm still confused about how drupal_static_reset() is and isn't supposed to be used -- for a long time after it was committed, Dries felt strongly that $reset parameters should still be there, and there is a lot of inconsistency in core right now around that. Anyway, in the attached patch, I've removed the reset parameter and made the helper function look like this:

    <?php
    function filter_format_reset_cache() {
     
    drupal_static_reset('filter_formats');
    }
    ?>

    The above is at least consistent with some other newer code, e.g. http://api.drupal.org/api/function/form_clear_error/7.

    I'll change it again at most once - i.e., if a core committer tells me I have to :)

  3. A default needs to be provided to variable_get() as a second parameter, otherwise NULL is going to get passed in filter_format_load() and return FALSE right?

    In a previous version of the patch, I used 3 as the default fallback format (since plain text was the third format inserted on new installations), but I since realized that's actually not safe; due to database auto-increment and offsets, 3 could actually be a different format. The only safe thing to do is save the variable in the database for everyone (which the current patch does), so the default of NULL only gets used if something went really really wrong with your database anyway. I've added this as a code comment to the patch:

    <?php
    /**
    * Returns the ID of the fallback text format that all users have access to.
    */
    function filter_fallback_format() {
     
    // This variable is automatically set in the database for all installations
      // of Drupal. In the event that it gets deleted somehow, there is no safe
      // default to return, since we do not want to risk making an existing (and
      // potentially unsafe) text format on the site automatically available to all
      // users. Returning NULL at least guarantees that this cannot happen.
     
    return variable_get('filter_fallback_format');
    }
    ?>
  4. Currently node bodies are fields, $node->format doesn't exist, so would be good to replace this with other real example.

    I filed a separate issue at #563270: Incorrect code examples in form_process_text_format(), because it turns out the existing example is using node body/format all over the place, not just in the one line touched by this patch.

  5. $value is the format ID right ? Please, let's rename it.

    Agreed, but this patch only touches a small part of the function, so it should be a separate issue: #563272: Change confusing $value parameter in filter_form()

  6. We can't assume that these tables exist.... These updates should be handled in the corresponding modules.

    Surprisingly, I think we actually can assume that -- block module is required in D6, and although comment module isn't, it's database tables are always installed by Drupal core anyway (bug?) and are never uninstalled. However, to be safe, I've added db_table_exists() to the attached patch.

    These updates are so simple that I didn't think it was worth moving them to the separate modules -- seems more readable to keep them where they are. Also, in the case of comment module, that could be disabled at the time of update, and given the mess surrounding that module and its dependencies, I think it's best to make sure the update happens to its tables no matter what.

  7. If a site already has a text format named "Plain text" this query will fail since {filter_format}.name is a unique key.

    Great catch. Hm, I don't think we want to risk editing people's existing formats, and trying to generate a unique name could get very ugly, so in the attached patch, I fixed this by simply removing the unique key constraint on that database column. Does anyone know any reason why it needed to be there? I couldn't find anything in the code -- there was user interface text telling you it needed to be unique, and filter_admin_format_form_validate() ensured it, but I simply removed those. Certainly it seems odd to me to require that human-readable names be unique, and there are plenty of other areas of Drupal that don't require that (e.g., content types).

    In the case of text formats, I think this might actually even be a useful feature; for example, I might want different users on my site to have access to a different set of tags in Filtered HTML, and now I can simply create two separate text formats, both named Filtered HTML, and do it that way.

    Does this make sense?

  8. I also discovered another bug and fixed it; filter.module was implementing hook_user_role_delete() but now no longer should because it no longer keeps track of roles, so I removed that in the attached patch as well.

Excellent, I'm happy with both the solutions to #2 (static variable reset) and #3 (a variable_get() default).

Removal of the UNIQUE constraint makes sense to me.

I fixed this by simply removing the unique key constraint on that database column. Does anyone know any reason why it needed to be there?

I think it's there because it's a requirement. Each text format human-readable name should be unique, as it's the resource that users have to identify them among others text formats. Having several text formats with the same human-readable names is a potential security risk, will be easy for users and admins to forget what is each text format intended for, what and configuration should be applied to each. On the final user side, how will them know which text format to use if they have exactly the same name.

there are plenty of other areas of Drupal that don't require that (e.g., content types)

Content types, custom block names, roles, custom menus, etc... have unique names.

In the case of text formats, I think this might actually even be a useful feature; for example, I might want different users on my site to have access to a different set of tags in Filtered HTML, and now I can simply create two separate text formats, both named Filtered HTML, and do it that way.

That would be very insecure. For example, in the user permissions page, you will see two permissions (with the same name):
- Use the 'Filtered HTML' text format
- Use the 'Filtered HTML' text format
How do you identify in this moment what is the format that allow Only local images are allowed. tags, for example, and which not, if both have the same name ?

Also, if you have a role that is granted to use both text formats (and admin, for example), the user will be confused, and only inspecting carefully the set of tags allowed will be able to decide what text format to use.

StatusFileSize
new81.69 KB
Failed: Failed to apply patch.
[ View ]

The attached patch, reverts the following, and adds a simple logic to generate a unique name for the fallback format before creating it in the update function.

+++ modules/filter/filter.admin.inc 29 Aug 2009 18:01:59 -0000
@@ -108,40 +103,16 @@ function filter_admin_format_page($forma
-    '#description' => t('Specify a unique name for this text format.'),
+    '#description' => t('Specify a name for this text format.'),

Reverted.

+++ modules/filter/filter.admin.inc 29 Aug 2009 18:01:59 -0000
@@ -181,19 +152,6 @@ function filter_admin_format_form(&$form
- * Validate text format form submissions.
- */
-function filter_admin_format_form_validate($form, &$form_state) {
-  if (!isset($form_state['values']['format'])) {
-    $format_name = trim($form_state['values']['name']);
-    $result = db_query("SELECT format FROM {filter_format} WHERE name = :name", array(':name' => $format_name))->fetchField();
-    if ($result) {
-      form_set_error('name', t('Text format names must be unique. A format named %name already exists.', array('%name' => $format_name)));
-    }
-  }
-}
-
-/**

Reverted.

+++ modules/filter/filter.install 29 Aug 2009 18:01:59 -0000
@@ -100,9 +93,6 @@ function filter_schema() {
-    'unique keys' => array(
-      'name' => array('name'),
-    ),

Reverted.

+++ modules/filter/filter.install 29 Aug 2009 18:01:59 -0000
@@ -260,3 +255,89 @@ function filter_update_7004() {
+/**
+ * Do not require human-readable text format names to be unique.
+ */
+function filter_update_7005() {
+  $ret = array();
+  db_drop_unique_key(&$ret, 'filter_format', 'name');
+  return $ret;
+}

Reverted.

Added:

<?php
 
// Generate a unique name for the fallback text format, start with 'Plain text'.
 
$start_name = 'Plain text';
 
$id = '';
  do {
   
$format_name = $start_name . $id;
   
$format = db_query('SELECT format FROM {filter_format} WHERE name = ' . $format_name)->fetchField();
   
$id = empty($id) ? 1 : $id++;
  } while (!
$format);
 
$fallback_format = new stdClass();
 
$fallback_format->name = $format_name;
...
?>

I'm not happy to post this, because I really like some parts of this patch.

We should strip down this patch to its essentials and/or split it into multiple issues. I was highly worried when "the other" issue (don't know which exactly) was merged into this one, but didn't state so, sorry.

1) To begin with: I'm no longer sold on the idea that user roles should be configured separately from the filter administration UI. Properly configuring text formats, access to them, and the contained/executed filters is highly critical to the security of a site. Detaching the access configuration from the filter configuration may bring the technical concepts in line, but bears a potential security risk. That 80% of Drupal users, which is always referred to, has absolutely no idea of text formats and content filtering - so splitting the configuration into two, completely unrelated pages will introduce a new, fundamental security problem.

2) I'm also not sold on the "Pain text" "fallback" format. I do not see a difference to the current default format. The better_formats module can live without, so I don't understand why it is required in here - since all this patch should affect are forms. Furthermore, an unconfigurable "plain-text" text format basically equals a text format with the HTML escaping filter enabled (which is a wrapper around check_plain()). That is not the intention of text formats (and let me state that I'd like to advance on that statement here, but I currently cannot).

3) I agree with all points mentioned by dropcube. Some of the earlier ones have not been addressed.

Most of what this patch tries to do already works in the better_formats module under D6.

To move forward, I'd suggest:

- remove the new, introduced default/fallback format handling, resp. revert to FILTER_FORMAT_DEFAULT.

- keep the user permissions, but leave (duplicate) the granting of user roles in the filter admin UI.

- revert the change to filter_format_load()

...

Hm. Reading the user permission description once again, I don't think at all that it makes sense to move format permissions to the user permissions page.

+        'description' => t('Use !text_format in forms when entering or editing content. %warning', array('!text_format' => $format_name_replacement, '%warning' => t('Warning: This permission may have security implications depending on how the text format is configured.'))),

So what? Better revoke the permissions for all roles? What security risk am I running into here? (neither this page nor the filter admin UI explains, and it's rather impossible to explain, because the security risk highly depends on the enabled/disabled filtes as well as on the configuration of configurable filters...)

The more I think about the effect of that separation, the more I'm opposed to it. Sorry. :-/

Hrm.

I know that this must sound highly disappointing.

So, whatever you do, please try to remove all not strictly necessary changes - focusing on the default text format per role.

Status:Needs review» Needs work

I agree with sun, the correct configuration of text formats, the access settings, enabled filters and it's order is highly critical to the security of a site. Splitting the configuration pages even more, will introduce potential security risks and confusion. The user should be able to see, in ONE page, all the configuration options that should apply to maintain text filtering secure. In fact, that issue try to fix that 'broken' UI: #558666: UX/security: Revamp text format/filter configuration.

@sun, here are some responses:

I'm also not sold on the "Plain text" "fallback" format. I do not see a difference to the current default format. The better_formats module can live without, so I don't understand why it is required in here

I define "fallback format" as the format that all roles automatically have access to. So it is the same as the current default format, which also has that property. The fallback format is simply what you're left with when you start with core's current default format and remove the "this-is-the-format-that's-automatically-selected-on-all-forms" and "I-can-unknowingly-grant-dangerous-permissions-by-clicking-on-an-apparently-unrelated-radio-button" features from it. Nothing more, nothing less.

The Better Formats module doesn't live without it; it builds an entire structure around it that tries to work around core's limitations. Here we are trying to address the limitations directly.

Furthermore, an unconfigurable "plain-text" text format basically equals a text format with the HTML escaping filter enabled

Starting with the patch in #158, it is no longer unconfigurable. It starts off as plain text (because that's both useful and 100% secure), but you can configure it to do anything else you want.

I agree with all points mentioned by dropcube. Some of the earlier ones have not been addressed.

Which? I thought I addressed them all.

Most of what this patch tries to do already works in the better_formats module under D6.

The Better Formats module deals with the default formats per role issue, but not any of the security hardening aspects.

- keep the user permissions, but leave (duplicate) the granting of user roles in the filter admin UI.

I would certainly be happy with this solution. I'd just rather take the time to think carefully about the correct user interface (post code freeze) so we don't have to keep reimplementing it. But at least that solution would keep the interface closer to the way it is currently, so I'm not opposed to doing it now if that's what it takes to get the patch committed. It does raise the question, though, of why we duplicate the permission granting here but not in other places that work similarly (e.g., the content type administration screens, from which you cannot grant any of the "edit/delete article"-type permissions)...

"Warning: This permission may have security implications depending on how the text format is configured." .... What security risk am I running into here? (neither this page nor the filter admin UI explains, and it's rather impossible to explain, because the security risk highly depends on the enabled/disabled filtes as well as on the configuration of configurable filters...)

There is a solution to this problem, and it lies in #275811: Warn about potentially insecure filter configurations. Although it's an API change, Heine has marked that as a "release blocker", and I'm hoping that since he's the security team lead that will carry some weight :) And it's also only a relatively small API change. With that in, we would be able to identify -- with close to 100% certainty -- which text formats are and aren't safe, and then we can easily replace the wishy-washy permission descriptions that we are forced to use now with a definitive statement about which permissions are security risks and which ones aren't.

So, whatever you do, please try to remove all not strictly necessary changes - focusing on the default text format per role.

While it's probably possible to have defaults per role without the user permission changes, it's not possible to go the other way around. And if you look at the history of this issue, all three people who put tons of time writing code for this over the past year and a half started off at #110242: make access to filters first class permissions -- that's the primary thing we've been trying to solve, and default formats per role is just a nice feature that automatically (and cleanly) falls out of it. But it's hardly a critical feature, given that contrib modules can already provide it.

So that is why the issues were merged (although in retrospect, maybe they should have been merged in the other direction). There have been tons of arguments over the years as to why having text format roles assignments become real user permissions is a really good idea, and in my opinion, they all still stand.

I suggest we proceed with the patch as it is, but with the following caveats:

  • Either (a) agree to defer any further UI improvements until after code freeze, or (b) implement it now with the "duplicate" interface for granting the permissions from both places (and still defer further UI improvements to after the code freeze).
  • Make sure to work on #275811: Warn about potentially insecure filter configurations soon, and agree that as a minor API change but a major security hardening improvement, it can be considered an actual release blocker, like Heine labeled it.

Status:Needs work» Needs review
StatusFileSize
new82.81 KB
Failed: Failed to apply patch.
[ View ]

Back to the patch: #171 is fine with me. Thanks!

I rerolled it to fix a comment in node.install (that was made out of date by a change in the number of the filter update function), and also made it so that the non-duplicate format names would be "Plain text 1" rather than "Plain text1" (a little cleaner). I also fixed the new database query to use ":name" syntax. No other changes for now.

Indeed, it seems I was mistaken.... content types do need to have unique names (it's not enforced at the database level, but via the UI only, which is why I missed it). Custom menu names do not have be unique, though, so Drupal is not totally consistent here... However, given the possibility of security mixups, and the fact that we don't want to start adding features to this patch now unless we absolutely have to, keeping text format names unique makes sense.

StatusFileSize
new82.87 KB
Failed: Failed to apply patch.
[ View ]

Rerolled for #563272: Change confusing $value parameter in filter_form() -- I like it when my own patches get committed and break my other patches :)

Issue tags:+API change

Adding the "API change" tag...

Note that I've posted an initial proof-of-concept patch at #275811: Warn about potentially insecure filter configurations which successfully labels text formats as secure or insecure, depending on how they are configured. And it really only involves a very small API change.

Hopefully that removes any objection to turning these into regular user permissions that can be configured on the permissions page along with everything else, as the other security and usability benefits to treating them that way are enormous.

Whether or not the permissions for each text format should also be configurable from each individual text format page (as Drupal currently does it), or whether the permissions should simply be listed on that page (as the above patches currently does it) is a separate question, but less important to get perfectly right before code freeze, in my opinion.

I'm open to suggestions for what can be done to the latest patch in order to get it RTBC in the next couple of days...

This is excellent. Combined with #275811 we can intelligently give warnings on the permissions page.

I agree with the decision to remove the role settings from the format admin (why should we have role-based permissions duplicated in random places just to resist change?)

I still need to test it to fully review.

StatusFileSize
new82.54 KB
Failed: Failed to apply patch.
[ View ]

New version of the patch based on feedback from @chx in IRC: In previous versions of the patch, filter_format_roles() and filter_access() took as input either a format ID or a full format object. Now they only take a full format object, which cleans up the code a bit.

Also, I realized that the update code for generating a unique format name seemed buggy, so I fixed that too... (It's still impossible to test, because for me at least, D6->D7 updates still do not work at all with the latest HEAD.)

RTBC, hopefully?

I promise to give this patch a thorough review during the con, but as of now, I am highly worried about this patch.

I have not been part of this months-long discussion, so feel free to ignore me entirely. I do think I've read the relevant issues, though, and at least I have something of a fresh perspective. I agree with sun in #172 that if this interface is unclear, we're risking fundamental security problems. The problem is that the interface as is, and even as improved in #558666: UX/security: Revamp text format/filter configuration, is still quite unclear. It would have been nice to get some interaction design help with this problem while we were busy redesigning every other part of the admin interface. :/

I believe this patch by itself will not solve the problem - sun's point below the code snippet in #172 is well taken. We'd be telling people on the permissions page that their filters were a security risk, when in fact some would not be. That would be a mistake.

However: This patch, when combined with #275811: Warn about potentially insecure filter configurations, is a huge improvement. Suddenly all the permissions in Drupal are on the permissions page, and you can see right there whether you have a security problem or not. In addition, if I've read #275811 right, when you're editing your format and you introduce a security hole, you get a warning right there in place, not just the next time you check the permissions page.

I also support the plain text format. It's a more sensible default than assuming people want their visitors to use HTML, when in reality most visitors wouldn't know a bold tag if it bit them on the nose.

Overall, I strongly support this patch, but only if we get #275811 in as well. Together, I think they are a big step forward, both for UX and for security.

Ok, this applies well against the CVS, but not even sure where to begin testing it.

Is there a summary of where you want us to look and compare it with a fresh version of the system?

Where is this all being set?

@mgifford: Thanks!

Try comment #126 above for a set of screenshots showing what the patch does -- that's still an almost completely up to date summary. So mostly you'd want to look at the permissions page and text format configuration pages to see the differences with the current D7 HEAD.

Comment #163 also has a good summary.

If anything's unclear, just say so :)

I was making my way through the checklist. Seems generally to be checking out fine.

However, there's now just a blank screen under /admin/settings

Sorry, need to clarify that the blank screen is after seven's menu. Not the white screen of death, but no content on the settings page under:
Home » Administer
Site configuration

I've just done a fresh install of the CVS, so pretty sure this is coming from this patch.

@mgifford: I can actually reproduce that issue with the current CVS HEAD also. I think it's just due to the fact that that page is being deprecated, and items are being moved out of there to the Configuration and Modules page, mostly. So on a default Drupal install, none of the modules have any items on that old page anymore.

So, not due to this patch.

@mgifford: that is a ghost page and is gonna be removed in a day or two hopefully in Drupal 7 :) There is still one item there if you enable all modules.

If it's a feature in core (or progression of a feature) and not a bug in this patch (which seems to be the case) then I'm totally fine with the code.

Seems like a good enhancement that should be in core. I don't think I can review it well enough to mark it RTBC though. Hopefully sun will do that after DrupalCon.

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new82.53 KB
Failed: Failed to apply patch.
[ View ]

Here's a new version of the patch that chases HEAD. The only change in the patch is due to #568224: Filter formats must be typecasted to int because "0" is "" and consists of replacing this:

-    $format = (object)array('name' => '', 'roles' => '', 'format' => '');
+    $format = (object)array('name' => '', 'format' => '');

with this:

-    $format = (object)array('name' => '', 'roles' => '', 'format' => FILTER_FORMAT_DEFAULT);
+    $format = (object)array('name' => '', 'format' => 0);

Anyway, where do we stand with this patch? If there are any outstanding concerns, it would be really good to hear them now. If not, this should be marked RTBC - it's been reviewed a million times.

The only remaining issue I'm aware of is that in order to make the current patch work the way we want it, we also need #275811: Warn about potentially insecure filter configurations. Several of us have been working on writing and reviewing that patch, and it's at an excellent state right now; I expect to have it at an even better state (i.e., hopefully committable) before Monday. Anyway, I cannot foresee any scenario where D7 will be released without it -- it is an absolute security game-changer.

Based on that, what else needs to be done in this issue for someone to mark it RTBC before code freeze?

Status:Needs work» Needs review

Oops.

Category:feature» task

We discussed this patch during DrupalCon and some of the changes were clarified.

However, we want to keep the user role configuration on the text format edit page, i.e. in fact, duplicate the user role configuration, so it is displayed both on the text format edit page AND on the user permissions page. That is, because users can assign arbitrary names to text formats -- but it is the actual filter configuration in a text format that really matters and decides on whether a text format can be safely granted to certain user roles.

I personally always sit on front of that text format name form element and need some time to think about a name that actual users of a site AND other administrators really grok.

It was additionally questioned whether we can make the user role configuration on the text format editing page read-only, so user permissions are solely granted in a single place on a site -- however, that would result in the same security concerns: Site administrators would grant user permissions to a highly complex and highly critical aspect of their site by solely looking at the name of a text format.

We do not have this UX pattern in Drupal core yet, but we have a general consensus that it is required in this case. Aside from that, I personally believe that we will see much more in-context configuration in D8, so this might be the start for a new UX pattern in Drupal.

StatusFileSize
new83.5 KB
Failed: 13036 passes, 7 fails, 0 exceptions
[ View ]

- Re-added user role configuration to the text format editing form. Implements a temporary workaround until #300993: User roles and permissions API is fixed.

- Removed warning about potentially unsafe configuration. That's a different issue and does not belong into this patch.

- Made the fallback format 'undraggable' (which is currently unsupported by tabledrag, but is necessary to make any sense of this new fallback format). Fixing tabledrag is a different issue.

- Changed the weight of the fallback format to 100. The code should additionally ensure that it's always last, but currently does not.

- Cleaned up lots of stuff; most importantly filter_get_roles_by_format($format) and filter_get_formats_by_role($rid).

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

The patch looks good, haven't reviewed it deeply, just found a couple of minor issues.

+++ modules/filter/filter.admin.inc 8 Sep 2009 02:52:10 -0000
@@ -201,6 +195,22 @@ function filter_admin_format_form_submit
   $status = filter_format_save($format);
+  if ($permission = filter_permission_name($format)) {
+    $grant = array();
+    foreach ($format->roles as $rid => $enabled) {
+      // @todo http://drupal.org/node/300993
+      if ($enabled) {
+        user_role_set_permissions($rid, array($permission), TRUE);
+      }
+      else {
+        db_delete('role_permission')
+          ->condition('rid', $rid)
+          ->condition('permission', $permission)
+          ->execute();
+      }
+    }
+  }
+

Consider moving this to the API function filter_format_save().

+++ profiles/default/default.install 7 Sep 2009 21:15:28 -0000
@@ -184,9 +184,8 @@ function default_install() {
+  user_role_set_permissions(DRUPAL_ANONYMOUS_RID, array('access content', 'use text format 1'));

Can we be sure at this point that "Filtered HTML" format has ID 1 ? Also, filter_permission_name() should be used to properly get the permission name.

Status:Needs review» Needs work

Opps, patch failed

Status:Needs work» Reviewed & tested by the community

ok, re-reading the issue and taking some more details of DrupalCon discussions into account:

- The current incarnation allows the fallback format to be configurable. The intention is to still allow it to be configurable, so for example, anonymous users wouldn't see a text format selector widget. (Because you can alter the filters applied to the fallback format.)

- Reading Steven's considerations in #12 again, it seems like the separation between "default" and "fallback" format accounts for the potential problems.

- Several people already mentioned in 2006 that it is important to duplicate the user role configuration, so this is definitely the way to go.

- In #85 Steven actually gave the proper procedure to move forward: We need to eradicate FILTER_FORMAT_DEFAULT first, then work on per role formats. -- that's why this patch is sooo big, because it now does everything in one fell swoop - which is hard to review.

David Rothstein just explained in IRC to me that the main idea of separating the default format from the fallback format is:

- the default format has nothing to do with user permissions. It just takes the first available. While we ensure that the fallback format is always available.

- the fallback format simply is that hard-coded, always available text format.

Therefore, we can safely make the fallback format draggable, too. Although that makes the term "fallback" a bit clumsy. "fallback" is not exposed in the UI, but developers may have a hard time to grok the entire fallback/default concept, when the fallback format comes first.

1) That means, we need a certain amount of API documentation here.

2) Looked up once again how filter_format_reset_cache() is used, and it's invoked by filter.module's own API functions and unit tests only. So I agree with quicksketch that we should definitely remove that function and replace the invocations with with drupal_static_reset('filter_formats').

Additionally, I'm not sure

- whether the fallback format shouldn't come first for fresh installed Drupal sites.

- whether the fallback format shouldn't replace "Filtered HTML".

- whether the fallback format shouldn't be the previously configured 'default format' on sites that are upgrading to D7.

All of those last 3 questions can be tackled in follow-up issues.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new83.02 KB
Failed: Failed to apply patch.
[ View ]

This should fix the test failures from #193.

Also, after discussion with @sun, I reverted these changes:

Made the fallback format 'undraggable' (which is currently unsupported by tabledrag, but is necessary to make any sense of this new fallback format). Fixing tabledrag is a different issue.

- Changed the weight of the fallback format to 100. The code should additionally ensure that it's always last, but currently does not.

The idea is that we actually don't want to prevent people from dragging the plain text format; they may have legitimate reasons to move it up in the list. Its place in the list no longer has any relationship to the permissions, so this should be safe to do - especially when #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x finally gets ported to Drupal 7...

Looking into @dropcube's comments now.

Status:Needs review» Reviewed & tested by the community

Great.

It would be awesome to incorporate the issues mentioned by dropcube, 2) of my list, and perhaps also 1) of my list.

Aside from that, this belongs into the RTBC list.

Excellent - it took 200 comments to get here, but hopefully worth it :)

Some thoughts for the follow-ups:

  1. @dropcube's comment about moving the roles stuff to filter_format_save(): Probably it's a good idea... depends a bit if you believe the roles are really "part" of the format object or not. We can probably look at that in a followup.
  2. Can we be sure at this point that "Filtered HTML" format has ID 1 ?

    Indeed, that seems like a minor bug (due to the possibility of databases with different offsets). It might be a little bit of a pain to fix (since the formats are saved in system.install but the permissions saved in the installation profile), but I promise to do it in a followup issue indeed... don't want to mess with RTBC this close to whenever the official code freeze is :)

  3. Agreed that we should look at the documentation around fallback and default formats and make sure it's clear.
  4. I thought @quicksketch was happy with the end solution of having filter_format_reset_cache() as a wrapper around the static reset? It seems a little cleaner to me to keep it that way (even if outside modules aren't intended to call it), but I don't have a really strong opinion either way.
  5. - whether the fallback format shouldn't come first for fresh installed Drupal sites.

    - whether the fallback format shouldn't replace "Filtered HTML".

    I agree both of these are worth thinking about, although depends a bit on how useful you consider the plain text format to be.

  6. - whether the fallback format shouldn't be the previously configured 'default format' on sites that are upgrading to D7.

    This I think we probably shouldn't do, since we are imposing restrictions on the fallback format (i.e., that it cannot be deleted) that I'm not sure we want to impose on an existing format on someone's site that they are already using.

  7. Regarding overall comments in #192, as I said above, I'm happy with keeping the roles configurable in both places (and therefore happy with this final patch!) but I wonder a bit if some of that reasoning would change with #275811: Warn about potentially insecure filter configurations. For example, if you're allowed to assign roles to a format while you're creating a new one, then all that might happen before the system can warn you that your new format is insecure, etc. But, we don't have #275811: Warn about potentially insecure filter configurations in yet (although I think it's getting very close), so we can revisit the UI issues later.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Subscribe

Status:Needs work» Needs review
StatusFileSize
new82.04 KB
Failed: Failed to apply patch.
[ View ]

Recent commits changed several of the functions used here, in particular filter_format_save(), filter_formats() and filter_access() - although the changes to the latter two were more or less in the same direction this patch was taking them anyway. So here's a version of the patch updated to chase HEAD that takes those changes into account.

I also realized that the previous patch had some changes to the filter tests along the lines of this:

-    $edit['roles[2]'] = 1;

These were necessary before when we had taken the role selection checkboxes off the filter configuration page entirely, but since the version of the patch we slipped in right before code freeze doesn't do that anymore, we no longer have to remove those lines from the tests. This should result in a (slightly) smaller patch file :)

I think this is RTBC again assuming the tests pass, but it might benefit from one final set of eyes.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new82.07 KB
Failed: Failed to apply patch.
[ View ]

Keeping up with CVS

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new82.08 KB
Failed: 13085 passes, 11 fails, 0 exceptions
[ View ]

Straight re-roll.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new82.57 KB
Failed: 13086 passes, 11 fails, 0 exceptions
[ View ]

- Fixed filter update function.

- Cleaned up theme_filter_admin_overview() function.

- Renamed filter_format_reset_cache() to filter_formats_reset() - @see #581626: Use a consistent/clean pattern for using $reset or drupal_static()

Commit message proposal: DIE filter_resolve_format() DIE!!!! :P

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new83.59 KB
Passed: 13099 passes, 0 fails, 0 exceptions
[ View ]

Missed another instance of user_role_set_permissions(), which has been replaced.

Also, I need this in the list of API clean-up issues to push.

Status:Reviewed & tested by the community» Fixed

I had a good look at this patch, and it looks great -- no need to bikeshed about details, it is 10 times better than before. Committed to CVS HEAD.

DIE filter_resolve_format() DIE

Let me mention this.

Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG! Awesome! Thanks! Amazing! Thank you very much! OMG!

Awesome!

Druplicon: Just like Drupal.

Also many thanks to all that pushed this patch forward - especially, quicksketch and David Rothstein!

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new1.46 KB
Failed: Failed to apply patch.
[ View ]

Sorry, testbot doesn't catch fatal errors currently. (confirmed in IRC)

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Fixed

Thanks! - yeah, this is great, and lots of people helped push this patch in (the commit message has something like 30 people listed on it :)

It looks like the patch in #214 has already been committed, which is why the testbot says it no longer applies...

Status:Fixed» Needs work
Issue tags:+Needs Documentation

Actually, there are a number of API changes here that need to be documented...

I should be able to add something to the D6-D7 upgrade guide sometime tomorrow (if not before).

Sorry for the delay on this, but there was a fair amount that needed to be documented...

I added these three sections to the module upgrade guide:
http://drupal.org/update/modules/6/7#default-text-formats
http://drupal.org/update/modules/6/7#text-format-permissions
http://drupal.org/update/modules/6/7#filter-formats-parameters

And also updated this older section appropriately:
http://drupal.org/update/modules/6/7#text_format

We probably still have a few small followup issues that need to be filed as per comments above, but otherwise this one is fixed!

Status:Needs work» Fixed
Issue tags:-Needs Documentation

[accidental duplicate comment]

Let's deal with all the rest in the follow-up issue: #588882: Filter fallback format clean-ups

Late to the final celebration, but thanks sun and David_Rothstein!! 1000 times better than before, great work guys!

.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

I did not understood whether now Drupal 7 allows input-per-role. I ma using Drupal 7 now (donwloaded one month ago) an I don't see any such feature.

How caan I upgrade from one dev release to another?

Priority:Critical» Normal
Status:Closed (fixed)» Active

I echo David's confusion above. I'm using Drupal 7 alpha 1 and cannot find the default filter setting anywhere. If it is there, it needs to be easier to find.

Status:Active» Closed (fixed)

There is no explicit "default" setting in Drupal 7 core.

Why would we need one? It wouldn't provide anything useful beyond the functionality that now exists. For example, as described in the Filter module help text:

Users with access to more than one text format can use the <em>Text format</em> fieldset to choose between available text formats when creating or editing multi-line content. Administrators can define the text formats available to each user role, and control the order of formats listed in the <em>Text format</em> fieldset on the <a href="/admin/config/content/formats">Text formats page</a>.

Please open a separate issue (and link back here) if you think the help text should be changed to make the functionality more clear :)

Thanks, David. It was not clear to me that changing the order of the text formats would change which one is selected. That is, in <select> elements generally, which one appears first in the list and which one is selected are two separate questions, as illustrated by this pseudocode:

<select name="format">
<option value="1">Full HTML</option>
<option value="0" selected="selected">Filtered HTML</option>
</select>

In Drupal 6, the site default format is always selected unless you override it for a role with the Better Formats module. Coming from that background, I was looking for the word "default," not the word "order," which has a different meaning. So yes, I will start a new issue to request a change to the wording. Thanks!