make access to filters first class permissions

Filter access control permissions should definitely become standard Drupal permissions, thumbs up.

But, I'm not so sure about being able to configure them from both locations. I'm not a usability expert, but I think it should be removed from the filter configuration because it adds unnecessary bulk. If I were a huge fan of being able to do things in multiple ways, I'm be programming in Perl, not PHP. ;) I don't feel that strongly about this though so I'd like to know what others think.

+1 to put filter access control under the standard Drupal permissions UI. It would have saved me, and others, a lot of head scratching.

I agree with #1 to leave the (rest of the) configuration of the filters in its own place. Don't introduce two UI's to do the same thing, that 'll bring a whole bunch of new usability issues.

Wow, we have been thinking about the same stuff at the same time. I even had a brief writeup of this in an email I did not send to the devel list finally, as I figured I probably not going to have the time to implement this adequately. My premise was that some modules use permissions "creatively" (the set of node permissions are dynamically generated for example), while some permissions like the filter access are not even exposed as permissions on the permissions UI. On the other hand, the permissions UI is already very cluttered. With the permission descriptions included in Drupal 7, "administer filters" is explained. However, making that page longer is not an ultimate idea anyway. So while we expose these as permissions, a ground-up rethinking of that interface could be in order.

I thought about:

- tabs for the permissions page to separate bigger chunks like node permissions, filter permissions, etc AND/OR
- reusable forms (like the settings forms wrapper) for permission settings UI generation

The later would allow the node type form to include a permissions chunk, the filters form to include a permissions chunk, etc. This is all only desired of course, if multiple places for the same setting is desired. There is no such setting in Drupal yet AFAIK.

The reason I did not send that email, and discarded my draft was that (without repeating the permissions form) this might decouple settings too much. Drupal 7 is going to have a "filter limitation by input widget" feature if all goes well (per node type, comment form, block form, etc), so these limitation settings will be on the filter interface while permissions will be on the permissions page. This is the same however with node types now, so it might even be a concept/UI cleanup, not a confusing practice to move all permissions to the permissions page finally.

Worked on making input format role associations real permissions. This input format association to roles was a custom and ugly comma separated list solution, so integrating it into the standard (but also ugly and still comma separated) permission list cleans up the API and provides direct UI clues to the user on the permission nature of allowing input formats for people.

Allowing PHP input format access or full HTML to whoever is an important permission question. Right now this is tucked behind the input format selection interface (unlike contact types, which have their permissions exposed on the permission page for example). So the idea here it to remove the custom data storage for filter permissions and integrate them into the normal permission system. I have hopes that the normal permission system will also be normalized in this release, but that's for a different issue.

This patch moves all permission setup to the permissions screen but keeps the easy to do overview of permissions on the input format overview. This should be an API cleanup, UI consistency and security improvement.

No time to look at the patch right now but a huge +1 to this in general.

With the long permissions page - Dmitri already has a patch for live js filtering of the based on keywords. I think we could also look into some show/hide and/or arguments (admin/user/permissions/node admin/user/permissions/user) - which would allow filter module to link to it's own permissions page for example (and potentially load it inline on the configure page when the popups patch is in). Either way, centralising permissions into one interface is 'a good thing', and if the page keeps getting longer that's a separate issue to be dealt with.

I definitely agree that having the same default input format for all users is not good and needs to be changed. However, I see some very complex permission issues when we combine permissions on the node type with permissions on the input filters (and potentially different default input filters per content type).

Example: It is very likely that someone will make two roles "writer" and "editor" with the respective permissions "filtered HTML" and "full HTML". Writers can edit their own posts, and editors can edit all posts. Now, what happens when an editor (which doesn't have the "filtered HTML" permission) edits a writer's post?

This kind of edge cases are already possible, but turning input filter access rights into permissions makes it a lot easier for users to make apparently logical decisions that end up conflicting at some point. In order to prevent this kind of bad configuration, we would either need some hierarchy in the permissions, roles or filter structure.

@dww: I'm in favor of the proposed change, it does make the interface more consistent and more intuitive. I got caught thinking of some permission-related issues and didn't realize I was going on a tangent.

Patch applies cleanly and works.

The upgrade path in filter_update_7001() is broken. The query to get the list of roles must query the 'role' table instead of 'user_roles', and update query has some problem in the WHERE clause. Unless someone else wants to fix it, I'll work on it tonight.

flobruit: that would be great, thanks!

Here's an updated patch for a working filter_update_7001().

I've reviewed this patch and it looks good.

"Choose %input_format when a field with format support is included in a form." felt slightly technical though. "format support" my turn people off. ;-) How about: "Use %input_format in forms when entering or editing content."?

In fact, 'input format' might be fairly technical too: "Use filtered HTML" sounds easier and less confusin than "Use filtered HTML input format".

flobruit: can you please upload your updated patch, so we can work off of that.

Oops, I should have attached the patch.

Here's the patch, including the changes suggested by Dries in #12 and #13.

Screenshots of the latest patch before and after.

I've thought about it a bit and I think permissions should be per 'filters' not per 'formats'. Users can give anonymous access to 'Filtered HTML' format, then later, by mistake, enable php.module and add 'PHP evaluator' filter to 'Filtered HTML' format and setup a time bomb.

This way permissions would be more complex, since we don't want to restrict access to Filtered HTML but, instead, enforce it for low profile users. And maybe not every filter needs to have a permission, e.g. like 'url filter' and 'html cleanup' ones.

Just my 2 cents.

@recidive: Well, the current patch just standardizes on using permissions. I can imagine a follow up patch to make permissions per filter. That would complicate the input format selection however. You would be able to choose the Filtered HTML format (per your example) but not use all of its features. That would make eg. the Filtered HTML format different per role, which would affect how filter processing and input filter help is done. IMHO this needs a bit more thought, so I'd advocate getting this patch in and then building on that.

Just to emphasise how important this patch is.

This week in #drupal-support I've personally answered questions by two people whose users had mysteriously disappearing edit tabs. And there's a classic example of the same thing here: http://drupal.org/node/243728

Unless you already know how this might happen, it's a very hard thing to track down - since usually the first thing people hear about it is a message from a site visitor saying "I can't edit x post". It's stung me before, and of course the first thing you look at when people talk about 'missing edit tabs' is at the permissions page...

Tested the patch and it works fine. Didn't yet run the upgrade path.

In the screenshot you can see that the capitalization of the words in the permission is odd. I think we still need to fix that to be proper. We could add quotes around the input format name's or something but we have to make it stand out a bit better, or remove the capital letter in the second word.

Hm, well capital letters some from the input format name. Content types have "machine readable names", so they use that fr permissions. Input formats does not. Lowercasing the name automatically for permissions would be strange (eg. "filtered html" or "full html with wiki formatting"). Especially for foreign languages, since PHP cannot lowercase UTF-8 strings properly, so a format names "ADÁS" (not a suitable name just an example) would come down to "adÁs".

We can introduce machine readable names for input formats to make this more consistent.

I had the same impressions about the uppercased first letter. I wonder if we could write a drupal_lcfirst() as counterpart of our drupal_ucfirst() UTF-8 aware function.

So the options are:

user_access('use "'. check_plain($format->name) .'"');

=> use "Full HTML"

The quotes are probably confusing for users.

user_access('use '. check_plain(strtolower(substr($format->name, 0, 1)) . substr($format->name, 1)));

=> use full HTML

Code too messy and unclear.

drupal_lcfirst()

I thought about this too, but besides this specific issue such a function wouldn't make any sense.

Gabor's suggestion to have machine-readable names for input formats is probably the way to go. I'm willing to work on this, but I have a question: what should the machine-readable name for "Full HTML" be (other than lowercasing the first letter)?

dww is dead on. I've been trying to think of additional uses for machine readable names, but can't, and that's the best of the solutions to what is a really minor issue. So at the risk of a pushing match, I'll mark this back to needs review.

I would suggest we rename the permissions to:

Use the 'Full HTML' input format

or just:

Use 'Full HTML' formatting

For now, I'd +1 Use the 'Full HTML' input format

I've opened a new issue to look at 'input format' naming in general: http://drupal.org/node/244904 - I think there's good reasons to try to change that across the board.

Considering the changes proposed in #244904: UMN usability: Rename 'input formats' and #245052: Rename input formats to reflect their purpose., I would go with Use 'Safe HTML' text filtering.

The upgrade path will be cleaner if these two issues are committed first, but both of them are mostly string changes, so we shouldn't have to wait too long.

subscribe.

Rerolled. There is still one @todo in this patch, regarding permissions for the default format. Drupal 6 and before always allowed all roles to use the default format in the interest of not blocking user input based on not having access to any format at all. This code does not ensure that at all, but includes a @todo to that effect.

The previous code had the role checkboxes for permissions and then the default format was overriding that. So when the default format changed, the values set in checkboxes took over to be the permission limits. With this code, permissions are not ensured for default formats (although in the overview it is printed as if it is ensured).

I am afraid if we ensure that all roles can access the default format, when you change the default format, the old one still keeps being accessible to all, while we set the new to be accessible to all. We don't have fallback data to use there for the previous format.

So the question is: should we ensure that all roles have permission for the default format and risk leaving formats all open or should we get tight instead and risk no permission for users to post in any format?

subscribe

Subscribe. I started to review and test this patch, but didn't finish yet... I hope to be able to post the results later in the week.

Based on what I've seen/read, I prefer to tighten things up, but help the user detect if no roles have access to the default format. One way to accomplish that is to generate a form_set_error() if you try to make such a change.

This patch also has a problem when renaming an input format, the permissions do not get moved over. For example, if you change "Filtered HTML" to "Basic HTML", all the checkbox permissions for the renamed format are empty. I'll re-roll with this fix and the suggestion in #34 by Dries.

I hate blocking such a nicely progressing issue but I do not think this should get in before http://drupal.org/node/229193

Alright, this got a bit more complicated than I thought. If filter module has essentially "dynamic" permissions, user module needs to provide a way for modules to update permissions. So this patch adds a public function in user module for getting permissions and changing the name of an existing permission (certainly a nice thing to have for contrib modules anyway).

To solve the default input format problem, I simply made the checkboxes for the default input format disabled in a checked state on the user permissions page. Note that I'd be *huge* advocate of removing "default input format" entirely. We'll save that for a later patch. Please review.

Perfect timing! I've reviewed and tested the most recent patch. For the most part, things look good, but there are a number of problems as well as some more general suggestions I have. In no particular order:

1. Although this patch is definitely a major security improvement, there is one sense in which it seems to make things worse. Previously, it was very nice to be able to see a list of roles when you were editing an input format, because that is when some important security decisions are made. (If I'm deciding to add the "PHP evaluator" filter to an input format, I definitely want to know who I'm giving that permission to.) So, I would recommend just printing a list of roles on that page. Possible text might be something along the lines of:
   

   $roles = array_merge(user_roles(FALSE, 'administer filters'), user_roles(FALSE, _filter_perm_name($format->name)));
   if (!empty($roles)) {
     $description = t('This input format is available to the following roles: %roles. Since input formats can have security implications depending on how they are configured, you should be careful making changes here if any of these roles are untrusted.', array('%roles' => implode(', ', $roles)));
   }
   

   At a minimum this text should appear on the "Edit" page, but maybe on the "Configure" and "Rearrange" pages too (?).

2. One thing that jumped out at me about this patch is visible in Gabor's screenshot from #17. The fact that a security warning is shown for "administer filters" but not for the permissions right below it strongly suggests to the reader that these permissions aren't security risks, which is not a good impression to give. Since we don't have an easy way to tell which input formats are dangerous, I would suggest adding text like the following to each permission description:
   

   $warning = t('Warning: Input formats can have security implications depending on how they are configured.');
   // And maybe this too, which would make the description a little long, though.
   if (user_access('administer filters')) {
     $warning .= ' ' . t('Before assigning this permission to an untrusted role, @check_the_configuration to make sure this input format is safe.', array('@check_the_configuration' => l(t('check the configuration'), 'admin/settings/filters/' . $format->format)));
   }
   // Then add it to the main description.
   $description .= ' ' . theme('placeholder', $warning);
   

   The only problem is that this looks a little odd on a new installation, since Drupal would be warning you about the possible security implications of a permission it has assigned to all roles on your site by default. But that's probably not the end of the world. (Another approach might be to try to label each input format's security individually, by running a simple XSS attack/PHP code/etc through it and seeing what comes out the other end, but that would probably be a task for another patch.)

3. Dries' and others' above suggestions for rewording the permission names didn't make it into this patch. While we're changing them, I would suggest breaking the dynamic permission naming into its own function, since it's used in so many places and the code is somewhat ugly otherwise. So you could have something like the following (as used in my first point above):
   

   function _filter_perm_name($format_name) {
     return "use the '" . check_plain($format_name) . "' input format";
   }
   

4. The PHP filter module is broken as a result of this patch. At a minimum there is a line in php.install that needs to be changed: db_query("INSERT INTO {filter_formats} (name, roles, cache) VALUES ('PHP code', '', 0)");. I haven't checked if any other parts are broken, though, but the above causes a PHP warning, as well as some other weirdness.
5. The new filter_form_alter() function causes a problem when viewing the permissions of an individual role (e.g. at admin/user/permissions/1). The problem is that it always creates disabled checkboxes for all roles, whereas in that case it should only create one... so you wind up with a bunch of extra checkboxes on the screen. I guess the fix is probably to loop through $form['checkboxes'] rather than looping through user_roles()?
6. I'm not sure I fully understand why this patch needs to introduce a new user_permissions() function, but I may just be missing something? In any case, there are a couple of typos in that function's comments, and I'm not sure I understand why it needs to bother doing the whole module_invoke_all('perm') thing...
7. The new user_rename_permission() function looks very good to me, and I've tested that it works (renaming filters preserves the permissions). Note that this function could probably be reduced to about two lines if #73874: Normalize permissions table gets in ;)
8. I discovered this quite accidentally while testing the upgrade path, but this part of filter_update_7002() does not always work correctly:
   

   $ret[] = update_sql("UPDATE {permission} SET perm = CONCAT(perm, ', use " . db_escape_string(check_plain($format->name)) . "') WHERE rid = " . $format_role);
   

   It assumes that each role already has some permissions in the database, but that isn't necessarily the case (a role could have been created just for the purpose of giving it an input filter permission, which is of course exactly what I did while testing this patch ;) I guess the easiest fix is to do a "SELECT 1 FROM {permission} WHERE rid = $format_role" query first and then use that to determine if an INSERT statement needs to be done instead of the UPDATE.

9. The disabled checkboxes on the permission page work OK, but at admin/settings/filters it's still possible to switch the default input format to one that all roles don't have access to. Somewhat unexpectedly, the result of this actually does seem to be that all roles get to use that format (regardless of their permissions), which is not good security-wise. Dries' suggestion of doing form_set_error() here seems like the quickest solution. Or....
10. @quicksketch's comment about "removing 'default input format' entirely": Yes!! I was thinking that too when I first started reviewing this patch, but wondered if I was just missing something... glad someone else thinks so too. Removing it seems like it would basically make everything Just Work, and it would lead to a much cleaner admin interface. (The default fallback behavior for content with no input format could just be exactly what Drupal does with other user input: assume it's totally untrusted and run it through check_plain -- or maybe also the line break and URL filters in this case.) I guess that is a topic for another patch, but doing it here would really simplify a number of problems...

Whew!

About default filter formats, http://drupal.org/node/11218#comment-305707

Thanks David for the extensive review! I'll take these suggestions into consideration as the patch develops. I've posted an updated patch at http://drupal.org/node/11218#comment-830924 (the default input formats issue). Try as we might, I don't think separating these two issues will be feasible. We're going to have trouble expressing the "default input format" on the permissions page and noting why it's special. We can avoid that issue (and some dirty hacks) by solving both of these problems at the same time.

(Let's start writing tests too ...)

subscribe

David integrated this to #11218: Allow default text formats per role, and integrate text format permissions, so marking this one as duplicate.

Thanks goodness! I'll dust off that issue and pick it back up. Thanks David and Gábor.