The problem: some permissions should clearly NEVER be permitted for anonymous users. For example, "administer users". The fact that it is possible to select these checkboxes is a usability problem and makes it easy for new site owners to make potentially devastating security mistakes by allowing

The proposed solution: let's try to make Drupal as secure as possible by default. Permissions could have a flag to indicate that they are appropriate for anonymous users, but absent that flag would be prohibited for anonymous users. Similarly we may want a flag to for some especially dangerous permissions to prevent them from being assigned to all authenticated users.

note - this may be even more valuable if input filter's are regulated by
normal" permissions: http://drupal.org/node/110242

Since we've already switched to keyed arrays for perms, like:

function menu_perm() {
  return array(
    'administer menu' => t('Manage menus and menu items.'),
  );
}

A possible (Drupalish) solution would be to make the elements keyed arrays something like:

function menu_perm() {
  $perms = array();

  $perms['administer menu'] => array(
    'description' => t('Manage menus and menu items.'),
    'access' => PERMISSION_NOT_ANONYMOUS,
  );

  return $perms;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs work
FileSize
5.89 KB

starter patch - most of the necessary changes made in user module, but not yet in other modules.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
28.63 KB

seems to be working with all core modules. After discussion with catch on IRC - I opted for a list of 'denied' roles, rather than my initial idea of just toggling denial of anonymous or authenticated. This might be useful in combination with the _alter hook.

For example - if I have a 'provisional' role on my site for newly registered users, I could make sure that certain permissions are never assigned to that role.

Thoughts, feedback?

Note also - I switched a little of the code in function user_admin_perm() to match the code used in user_access().

However, I think the way perms are saved to the DB should be rethought as a separate feature request. At the least, they should be a serialized array. Even better might be to have one row for each perm for each rid.

floretan’s picture

This is a great idea, and it also has the benefit to make that huge permissions form more readable.

One of the issues is the upgrade path. This patch has the side-effect of deleting every "unsafe" permissions when the permissions form is submitted, which would happen separately from the upgrade script and give the user the impression that it just happened randomly. In the case where someone had a single-user site, or one where every authenticated user is a trusted administrator (bad practice I know, but I'm sure there's a good amount of those on the web), we might suddenly be keeping people from accessing essential functionalities. Once the permissions are gone, it could be difficult to create a new role and figure out what the proper permissions were.

An alternative could be to disable checkboxes if they are unsafe and unchecked, and flag them with a message if they are unsafe but checked. This solution would mix the upgrade path with the live code, so it's definitely not optimal. An update function that disables those permissions and informs the user would be bad for usability but would definitely have cleaner code.

pwolanin’s picture

@flobruit - yeah, I've been unsure how to proceed in terms of an update path. As above, it wold be nice to have a cleaner DB structure, which could also make this easier.

Damien Tournoud’s picture

That's clearly a good idea. But I'm thinking we perhaps need an intermediary between "denied" and "allowed" that could be "discouraged".

For example, the "administer blocks" should be discouraged for anonymous and authenticated users, as well as potentially all "administer*" permissions (administer comments, administer site-wide contact form, etc.).

David_Rothstein’s picture

This is a really great idea.

I agree with Damien that an intermediate "discouraged" flag would make sense. This would allow more permissions to be flagged and therefore ultimately lead to better security.

If there are separate 'denied' and 'discouraged' flags, then I wonder what the right balance is? Certainly things that are 100% security problems in all imaginable situations (e.g., allowing anonymous users access to the PHP input filter) should be 'denied'... but where to draw the line? It's definitely possible to have a small, controlled site where ALL authenticated users are completely trusted, and forcing those sites to add a new admin role (which they would then need to put every single one of their users into) might be overkill.

The "alter" hook you added could perhaps be useful here (either in a contrib module or maybe even a setting in core?)... with just a few lines of code you could change all 'discouraged' flags to 'denied' and thereby put the site in a stricter security mode.

If the decision is made to go with the additional 'discouraged' flag, then I was trying to think about how to indicate this to the user. My first thought was maybe a red border around the checkbox? Then I noticed that there are already a bunch of permissions that have the following in their descriptions:

array('%warning' => t('Warning: Give to trusted roles only; this permission has security implications.'))

Maybe this could be pulled out of the individual descriptions and instead be something that gets added automatically? So any permission that is flagged as either 'denied' or 'discouraged' for any role would get this text automatically appended to its description on the permissions page?

David_Rothstein’s picture

However, I think the way perms are saved to the DB should be rethought as a separate feature request.

See #73874: Normalize permissions table...

Susurrus’s picture

I think Drupal should definitely be careful about how it limits its users (the people who use Drupal, not the site's users). We're making broad assumptions about the audience that the site is exposed to. If the site is used only internally within a company where the network is locked down, the site might be setup so that everyone can do everything. I think ONLY allowing discouraged tags on permissions should be how this starts.

Dries’s picture

I'm not sure about this. In certain setups, it might be OK to let anonymous users use PHP filters.

David_Rothstein’s picture

Hm, good point... there are indeed setups in which even anonymous users are trusted.

However it's worth pointing out that because the above patch introduces hook_perm_alter(), even 'denied' permissions are never truly denied. It's just a question of the default behavior, and what behavior you need extra work (e.g., a contrib module) in order to get on your site.

Or would the idea I mentioned above ("security modes") make sense here more generally? Suppose modules were to define their permissions as allowed/discouraged/denied based on what is appropriate for the vast majority of sites. The default Drupal behavior would be to honor these suggestions, but you could also have a "strict" mode (which turned all the 'discouraged' checkboxes into 'denied') as well as a "lenient" mode (which turned all the 'denied' checkboxes into 'discouraged'). This is thinking aloud and might be overkill, but it would be simple and quick to code... and perhaps it would allow a good mix between the competing usability and flexibility concerns?

Just having 'discouraged' as a first step would not be bad either. Although I think one of the reasons @pwolanin wanted to disable the checkboxes completely was the security implications of accidentally clicking on the wrong checkbox and not noticing it... which you can't truly deal with unless the checkbox is disabled.

David_Rothstein’s picture

Or an even more general idea: Instead of having modules label permissions as allowed/denied/etc, maybe have them label the permission with tags like SECURITY_RELATED or ADMINISTRATIVE_RELATED (e.g., a tag that somehow indicates the "required trust level" needed to use the permission)?

Then have roles be labeled with the same kinds of tags... which would let you guarantee that a role never gets access to a permission "higher" than the trust level which has been assigned to the role.

So the default Drupal behavior would be for anonymous and authenticated users to both be labeled untrustworthy, and thus the security/administrative permission checkboxes would be disabled. However, if you have a site where you trust those roles, you could just change their trust level and then suddenly the checkboxes would be available to you.

This may be a bit of "feature creep" but I'm posting it here anyway ;)... although a little more complicated, I think it might be a more general solution. (The one weakness is that it requires every module author to actually think carefully when tagging their permissions; otherwise, the trust levels quickly become meaningless.)

Susurrus’s picture

@David_Rothstein: If there's a concern about accidently selecting a permission that Drupal thinks should be discouraged or denied, whichever path we end up taking, we should just add a confirm page. This is already done on the module page for when additional modules need to be enabled, so it would have precedent.

I just worry about people having to fight Drupal in order for it to be usable, even if it is "in the name of national security." At some point more secure relies on users being better educated and not the system being more restrictive.

pwolanin’s picture

Since D7 now allows a description of each permission, there has already begun a process of "flagging" permissions within that text.

I too can imagine rare, edge cases where even anonymous users could have some admin-level permission, but as mentioned above, the code needed implement the _alter function to unblock all permissions is about 3 lines.

I really don't like the idea of a confirm form, since dozens of permissions can be changed for different roles at the same time. How would you sort that out for the confirm?

Susurrus’s picture

I would say if it's only 3 lines to add that permission in, then to leave the default less restrictive and let people change it if need be. The other way around seems silly to me. I would imagine many more support issues where someone is saying "but I need to activate this permission" that when someone says "I don't want admins to be able to activate this permission" is another way to think about it.

Regarding the confirm form, it'd be fairly easy. There is only 3 default roles in a default Drupal setup: administrator, authenticated, anonymous IIRC. This means that there should really only be 3 roles that could possibly have permissions restricted to them. In reality, permissions would probably only be restricted for anonymous or authenticated users. With just two roles having dangerous permissions changed, it shouldn't be too large of a confirm page for even large permissions changes.

pwolanin’s picture

@Susurrus - No - you misunderstood me. It's 3 lines to remove any restrictions. Adding restrictions (i.e. this whole patch) is much more work. This is why I'd like the default to be more restrictive, since this (by design) makes it relative easy to remove any or all of the restrictions.

Susurrus’s picture

I guess my question is really: why isn't warning them about a permissions enough? Why do we need to tell them that they're not allowed to do that?

This issue was created to address the usability issue of being able to configure permissions however they want. I would think a bigger usability issue is the user not being able to configure permissions however they want.

And while three lines isn't a lot of code to add to remove restrictions, it doesn't actually matter how many lines of code are required; any amount of code above 0 has the same cost to the user if it's a simple copy & paste. This user, who may not be comfortable with code, has to tinker with it. Doesn't matter how "easy" it is if the user isn't comfortable doing it.

Because of this additional cost and, as I see it, reduction in usability, I suggest only a mere warning about possible permissions issues.

David_Rothstein’s picture

One slight advantage of disabled checkboxes over a confirm form is that it's actually a usability improvement if done correctly. Assuming you actually don't want to click on these checkboxes, then it's nice to have them grayed out because it focuses you towards the ones you might actually want to click.

@Susurrus, I think your basic ideas make a lot of sense here. Although the site admin wouldn't necessarily have to copy and paste code (there could easily be a contrib module), it's still a similar issue -- they have to go out and find something, just to remove a restriction that Drupal core places on them. That's a bit of a frustrating experience. There is no reward of "ooh, I get a new feature" which most contrib modules give you.

Are there any thoughts about my idea in #11 as a solution? It would build directly off the current patch. I don't think it's unreasonable to say that before Drupal lets you assign permissions to a role willy-nilly, you first have to tell it "this is a role I trust." It's a one-time decision, and then all the checkboxes you want are open to you. Some other possible benefits I can think of:

  • It would help with a separate security problem of poorly-named roles. If Administrator A creates a role with a poorly-chosen name, then Administrator B might accidentally give it more permissions than it deserves. Having a binary (or near-binary) classification of roles as trusted vs. untrusted would prevent the most egregious of such mistakes. (In fact, one could argue that the potential for this kind of confusion already exists in core with the "authenticated users" role... there is not necessarily anything "authentic" about them ;)
  • It would provide better enforcement of code separation. The current patch requires hook_perm() to "know" something about Drupal roles, but with my approach it would only have to return information about the permission itself.
  • It would essentially be a (very rudimentary) version of "role weights" in core, which would provide all the benefits listed here: http://drupal.org/node/25530#comment-332712.

Of course, we would need to avoid having too many categories for the roles/permissions... otherwise it would just be "permissions on top of permissions" which would be a little silly ;) But with a very limited number of trust levels (probably 2-3) it seems to me like there might be real benefits here for both usability and security. Comments? Am I onto something here or totally off base?

pwolanin’s picture

@David_Rothstein - that's a nice idea. Basically it would just block for those roles the perms that are denied for authenticated users. It could even be an implementation of the _alter hook in core. I'll try to code that up.

macgirvin’s picture

Here's a thought... have the checkboxes for those items with severe security implications disabled by default as was mentioned earlier. Then you have an 'Advanced' tab which provides the same page but where everything is active. The advanced tab is *only* available to user/1 - period.

David_Rothstein’s picture

@pwolanin: Cool. I wonder if it would be good to wait for an opinion from Dries before going too far with the coding though? Right now there are, like, three competing ideas for how to proceed here (mine, #12, and #19)!!! I think they're all reasonable ideas, actually... I prefer mine because I think it naturally allows the permission page to behave sensibly and securely in a customized way for each site.

For coding my idea, you might be right that the hook could be used... although actually my thought was that we would go through your patch and everywhere you have 'denied roles' = something replace that with e.g. 'restricted permission' = TRUE (upon further thought I definitely think a simple binary classification is the best way to go for starters). Then a simple change to the roles admin page so that each role can be assigned 'trusted' = TRUE or 'trusted' = FALSE (we probably need a better term for this though). The code for the permission page would then simply disable the checkbox whenever the perm had restricted permission = TRUE and the role had trusted = FALSE.

macgirvin’s picture

@david_r - the problem I see in #20 is that we're creating the same situation all over again. I originally thought of doing something similar, but now you've got a paradox where if 'trusted' can be set for a role, you need to disable it (the trusted setting) because it can set unsafe permissions. That's why I suggested that we leave the ability to delegate 'dangerous' administration tasks only to somebody we know positively has the authority to grant anything and everything, which is user/1. They need to have the power to do this, and we only want to keep him/her from shooting themselves in the foot. How best to do that is the question of the moment. I'm sure it can be done in a lot of ways.

And a brainstorm on how to filter all these dangerous things might be as easy as looking for the string 'administer' in the rights name. I'm certain that won't catch everything dangerous but it's a good start.

pwolanin’s picture

Let me reiterate my goal - to prevent blatant mistakes or extremely insecure configurations due to misunderstandings by either developers or site administrators. I also think this is fairly close to the minimum change that would accomplish the goal.

I like David_Rothstein's suggestion, but it's also clear that the proposed code and hooks would allow this to be easily accomplished in contrib. It also allows for arbitrary customization in contrib.

This patch is a first step- it introduces the possibility of more meta-data for permissions beyond just a description, in addition to making the permission system a little less error-prone.

So, any reviews on the posted code, thoughts about the upgrade path, or alternative patches?

macgirvin’s picture

Actually the more I think about this, the most effective solution may be to simply move everything that includes the word 'administer' to a new tab or to the page bottom and away from all the 'access/read/post' settings. It doesn't have to be any more complicated than that. If it is, somebody will open it as an issue six months from now as to why we made it too hard for them to give administer user rights to anonymous and somebody will have to bloody fix it so they can.

Crell’s picture

The "administer *" permission are, IMO, a bug. They're far too broad to be useful, and need to be factored out into more targeted permissions anyway.

catch’s picture

OK here's my take, partly taken from conversations with pwolanina and webchick in IRC:

Permissions grouped by type of thing rather than role (so content permissions, rather than having book and node split). This requires metadata, but not necessary safe/dangerous markers.

"administer" permissions changed into a super permission for each group - these could be in their own section down the bottom like macgarvin says - "administer all content" "administer all users" or whatever. Ideally individual permissions when these are set would be both disabled and checked to make it clear what's going on.

I also agree that stuff like 'administer nodes' needs splitting - there's a patch somewhere for a 'publish content' permission - I think we need to make it that granular but provide an easier way to assign multiple permissions at once (hence group level permissions).

Crell’s picture

@catch: So it sounds like you're describing "Permission group" to be along the same lines as module "Packages". Then each Group gets a system-created "everything in this group" meta permission (or perhaps just a check-all checkbox?).

I would definitely support that, as it helps break up the by-module restrictions we have even further (just as the Drupal 5 menu reorganization did). And if we get properly more granular permissions out of it in the process, score. :-)

So then hook_perm() would looks something like:

function example_perm() {
  return array(
    'create things' => array(
      'description' => t('Lets the user create things'),
      'group' => t('System administration'),
    ),
    'access things' => array(
      'description' => t('Lets the user get a grip on things'),
      'group' => t('System administration'),
    ),
    'use things' => array(
      'description' => t('Lets a user make use of things defined by an administrator'),
      'group' => t('Content'),
    ),
  );
}

Which would then visually result in 2 fieldsets on the permissions table along the lines of the modules page, System administration and Content. Is that what you're describing?

David_Rothstein’s picture

Making permissions more granular and reorganizing the permissions page by group sound like great ideas to me in their own right... but I think maybe they're a little out of scope for this issue?

No matter how you reorganize things, there will still be dangerous permissions, and the issue at hand is how to prevent the vast majority of sites from mistakenly assigning these permissions to anonymous (and other roles they do not trust).

@macgirvin's basic idea of moving the dangerous permissions to the bottom of the page is definitely interesting, and I guess it would be helped by an overall regrouping of the permissions page (however, what would you do about things like "delete any"... that's certainly a dangerous permission, but does it belong under "content" or "administer"?)

I agree with @pwolanin that we should keep this focused on the original issue. The current patch takes the approach of disabling checkboxes for dangerous permissions. My idea was an extension of that which would disable the checkboxes by default but provide the administrator a way to enable them again for any role of their choosing, within core. Then there are the other methods that have been suggested (confirmation form, moving dangerous permissions to the bottom of the page, moving dangerous permissions to a different page, highlighting dangerous permissions via theming, etc).

So really I think the basic question to focus on is this: Is disabling the checkboxes a good idea or not?

My own tentative answer is yes. Although I like some of the other methods too, I don't think being intrusive is a bad idea here... We are talking about some seriously harmful permissions, so forcing the administrator to stop and think very hard before assigning them is probably a good thing.

catch’s picture

Crell, yeah that sounds about right to me.

David, out of scope maybe, but the main technical change here is metadata for permissions. If we could eliminated the 'administer nodes' permission (in the same way we seperated edit and delete permissions between D5 and D6), then we'll end up with less dangerous permissions, or at least more clearly defined ones. Which might give us a better idea of what actually needs disabling.

dmitrig01’s picture

@David: Yes. Disabling is good.

@Dries: You can hook_perm_alter in that case.

David_Rothstein’s picture

@catch, that's true, and we definitely need to be careful not to go overboard with the disabling. For example, the "delete any..." permission would be considered dangerous in most cases, but I just realized that there are definitely situations where it would not be considered dangerous for a particular content type. This whole idea of classifying permissions is a tough one, and for any metadata scheme we probably need to be careful not to make too many assumptions.

Crell’s picture

I believe better organization of permission as catch suggested would go a long way to eliminating the need for removing checkboxes. "Dangerous" is a very iffy flag to set. However, if all "dangerous" permissions were under a permission group called "Administration", then that serves as a fairly good indicator for the person setting up the site that they probably shouldn't give that permission to anonymous users. If they do anyway, then they either have a good reason to do so or can't actually read. I think at that point we run up against the same problem as the password strength checker: At some point, trying to over-protect the user actually makes the system harder to use, not easier. (OMG big scary red box I broke it!)

Dries’s picture

I read up on the thread and I'm still not convinced about being more restrictive. Having to install a module to unlock some functionality is annoying, even if it is a 3 line module.

I'm also somewhat nervous about permission groups. I was against free-form module groups for the longest time. People told me it would lead to better organization of modules. I believed that it would lead to inconsistent and messy grouping of modules. Its exactly what happened, and I somewhat regret the decision to commit that patch. Many of the module groups are less than helpful, and module grouping often make things worse -- using Firefox's built-in search feature is typically the best way to find a module. The same would happen to permissions and permission groups.

Crell’s picture

What if, unlike modules, we built a series of permission categories straight into Drupal? So you get "Content", "Administration", "User-related", and "Other" (or whatever). Specifying anything other than one of those will get translated to "Other". That way we're not at the mercy of module authors to group intelligently.

It's also, I think, largely a matter of education. The documentation for module groupings was rather unclear for a long time about how package should be used. However, as of D5 we have a grouped set of administrative menus, and those are, for the most part, fairly well-observed (with Site configuration being the "Other"). If we document and educate properly, random extra groups is less of a problem.

David_Rothstein’s picture

Status: Needs review » Needs work

I think I might go ahead and submit my idea in #20 as a patch, then. I'm not sure I love it, but at this point it might be quicker to just code it and get it reviewed... ;) It doesn't require installing a module to unlock functionality, and it only forces permissions to have a single TRUE/FALSE classification. (What Dries says is exactly why I eventually proposed a binary classification. Although I guess it's possible that even that's too complicated to trust contrib modules not to screw up.)

To the extent that it's possible to have an objective definition of security, I think it's possible to have an objective definition of a "dangerous" permission: A dangerous permission is one that allows you to compromise the site's security. Period ;)

So actually, I might just start with a simpler patch that abstracts out the "Warning: Give to trusted roles only; this permission has security implications" text and inserts it automatically for permissions classified as dangerous. I'll think about it over the weekend.

macgirvin’s picture

If we could define a dangerous permission to the letter this would be a piece of cake to solve. On a public facing site you have a different set of potential issues than behind a firewall. Anonymous in some settings may be completely trust able. It isn't a binary problem.

My biggest objection is the continued assumption here that development costs are zero (they most certainly are not) and the relative abandon at making changes which affect the entire suite of modules. If this was a major evolution of the permission mechanism it might be worth the downstream effort. Eventually every module which declares a permission has to be involved (which is a very large number). But to do all of this for the sake of stopping the admin from handing out a permission that he now has every right to hand out is an entire waste of code effort - which could be better utilized fixing the modules that are still lagging in conformity to 6.x.

I'm not saying we shouldn't prevent the admin from shooting themselves in the foot. It's a noble cause. But unless we're going to radically fix and improve the permission setting ability generally, the ultimate cost of the proposed fixes are way out of proportion to what could be achieved simply through better documentation or a change in page layout.

webchick’s picture

Something I said in #drupal-dev when this was brought up is that I think this is time and energy spent in the wrong area if it's intended to protect people from making mistakes that compromise the security of their site.

I've never once heard of someone getting their Drupal site pwned because of accidentally giving anonymous users "administer site configuration" permissions. On the other hand, I've heard of countless sites getting pwned (and there's an easy finger print for this) for enabling PHP or Full HTML as the default input formats and allowing anonymous comments.

If you want to improve Drupal's security, the undisputed biggest bang for your buck is improve the input format system to make it more intuitive to understand, and more difficult to assign completely absurd rights to certain input formats. There are several issues in the queue atm that are addressing this problem.

As to this particular patch, in the usability testing we did at U of M, every single user understood what this page was for. The sticky table headers that were added in 6.x make it clear what role is being assigned what permission. The permission descriptions that were added in 7.x make it clear what the permission actually does if there is doubt. I don't feel that further UI tweaks are necessary at this stage, and it's debatable whether this is even an actual improvement, or instead makes things more confusing (can't wait until http://drupal.org/project/usability_suite is done so we can test this kind of stuff :D).

Futhermore, the prospect of a hook_perm_alter() scares the complete bejeezus out of me. That means I could potentially install one buggy-assed module and my entire site's security is gone. This can currently happen with node_access modules, but at least that "only" affects my site content. This would affect everything: permissions to access administrative pages, turn on modules, hand out additional permissions, etc.

Unless something more is gained than UI niceties (which even that is debatable; for example in intranet sites, all users might be anonymous), I'm -1 this patch. Sorry.

pwolanin’s picture

@webchick - I saw this as interacting with the proposed changes to input formats to make use of them full-blown permissions. So, I agree that for the most part this is minor until/unless that patch goes in.

Also, this is a first step towards moving towards a permission API that allows more meta data. Maybe I'll refactor the patch for just that portion of it and post as a separate feature request.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
28.27 KB

The reason I think this issue is important is because it's fundamentally about building security directly into Drupal's permission system and using it to guide the administrator to make intelligent and secure choices. Preventing accidental clicking is only a very small part of that.

The attached patch is based heavily on @pwolanin's original work but takes a very simple, very conservative approach (no disabled checkboxes for now):

  • The most direct effect of this patch is that it takes the '%warning' => t('Warning: Give to trusted roles only; this permission has security implications.') text which currently exists in four separate places, puts it in a single place, and makes it themeable. That part ought to be uncontroversial ;)
  • The specific way it does this is by allowing permissions to be tagged with a 'security risk' flag. I deliberately implemented this as a tag rather than a boolean, which makes for slightly uglier code. The reason I didn't use a boolean is that I think it would be really bad to allow code to be written that labeled a permission as NOT a security risk. Some permissions we can certainly label as having unequivocal security implications (as core already does), but very few permissions are security risks nowhere... someone's site out there somewhere might consider it to be a very critical permission.
  • Because we now have permissions tagged as security risks, this makes it very easy to implement a lot of future possible options in either core or contrib (e.g., the original suggestion of disabled checkboxes, etc). For now, though, all it does is print the above warning message.
  • I left the hook_perm_alter() from @pwolanin's original patch in here for now. (I think most of @webchick's concerns might not apply here because the hook only allows altering of the form, not the permission assignments themselves, but this may require more thought?) Having the hook in there is important because it allows individual sites to define exactly which permissions they consider to be security risks. However, I think a VERY strong argument could be made that an interface for doing this should exist within core. So it might turn out that the hook is not really needed.
  • If (but only if) such an interface is added, one very neat and random side benefit of this patch is that it would allow "administer permissions" to be split up into two separate permissions of its own: "administer regular permissions" and "administer all/dangerous permissions". A user who only had "administer regular permissions" would not be able to change the security-risk permissions, but would be able to change the others. So you could grant someone else the right to administer permissions on your site, but not allow them to do anything that would compromise your site's security.
David_Rothstein’s picture

Also, I'd like to change my "tentative support" for the idea of (eventually) disabling checkboxes into "resounding support" ;) I have a much longer argument in my head, but the gist of it is that if clicking on the checkbox causes harm to the site -- and if Drupal has enough information to know that it will cause harm to the site -- then it should not be possible to select it. Anything else is just dancing around the issue, in my opinion. (Don't get me wrong: I think the permission descriptions/etc are absolutely great and in many situations are probably sufficient; however, I can also think of TONS of situations where they would not help one bit. When it comes to security issues, the 90% solution isn't good enough.)

However..... in order to do this correctly, Drupal needs enough information to be able to make a correct decision about each checkbox on a site-by-site basis. Otherwise, it doesn't work. So I think it probably is appropriate to expand the scope of this issue a bit.

Dries’s picture

I'm tempted to mark this issue as "postponed" for now. I think we've more important tasks to tackle at this point, and I'm not convinced this is the best way forward.

pwolanin’s picture

@Dries - I think we should make the switch to a structure that supports more meta-data, but postpone any other changes for now.

David_Rothstein’s picture

I agree (reluctantly). It seems there are just too many other patches that need to go in first before it's possible to do this the right way.

In my opinion, it's an extremely important issue, and one that is hopefully realistic for Drupal 7. (Not the disabled checkboxes themselves, but the broader idea of a "security API" that was at the heart of the original patch; there are a whole host of unique and amazing things that can come from that.) I personally think it's http://www.lullabot.com/articles/how_drupal_will_save_world kind-of-important, and I don't mean that link as a joke...

Damien Tournoud’s picture

#182023: Add a third default role to core for handling Administrative duties has some related discussions about implementing a default "administrator role".

Senpai’s picture

I marked that issue #182023 as Postponed. Let's hope this one doesn't meet the same fate.

Garrett Albright’s picture

I unknowingly started a dupe issue here. Please check it out for my thoughts on what hook_perm() should look like.

After reading through this issue, here's my thoughts on the ideas brought up here;

I don't like the idea of disabling check boxes. It smacks way too much of hand-holding; of the computer trying to be smarter than the user, which is always frustrating. I think sensible warnings are plenty.

I also think that hook_perm_alter() and splitting the perms page into separate pages, levels, groups, etc is over-thinking the issue, at least for now. There's an 80/20 thing going on here; just a little work can add a lot of functionality to the perms page, especially when it comes to clarifying what each of those rows of check boxes will do to the site and why enabling some of them for unprivileged roles is a poor idea. (But, again, it should not be impossible.) Again, please see my issue for my ideas in that regard.

The correct path we should take is to guide users to make wise decisions… but not force them to.

Anonymous’s picture

What about introducing role types that we require in the hook_perm? Something like

function hook_perm() {
  return array(
    'administer my module' => array(
      ROLE_ADMIN,
      t('Perform maintenance tasks for my module'),
    ),
  );
}

Other types might be ROLE_ANON, ROLE_USER, ROLE_ACCESS, etc. So when a role is created these role types can be listed in the role record and then the permissions page can be set with defaults based on the hook_perm and the role types indicated in the role record.

These role types could be many and may be better served as a separate table rather than a static set of types but that would complicate the remedy.

pwolanin’s picture

so, per the Jaspan school-of-DX, I'd suggest that the attributes be defines, rather than strings:

'attributes' => array('security risk'),

something like:

define('SECURITY_RISK', 99);

...

'attributes' => array(SECURITY_RISK),
Garrett Albright’s picture

…And I would prefer not to introduce new constants into the global namespace, especially when they'll rarely be used. Something like 'security risk' => TRUE would be just as legible.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Issue tags: +anonymous users
chx’s picture

Sure it's not a good idea in general to grant access user permissions. But in a specific case you can not possibly know which permissions are explosive so by marking some as explosive just because they are generally so we are actually making quite some real world use cases less secure.

Jaza’s picture

Some thoughts:

  • On the big question of "to disable or not to disable checkboxes", my thoughts are that we should only disable them for anonymous users, and that we should only disable them in cases where it literally makes no sense to assign them to anonymous users. I don't care how much of a security risk it would be to give "administer content" permission to anon users, because when you think about it logically, it still makes sense that you could let anon users administer content (e.g. in a totally insulated intranet setup). However, for the following core permissions, I think we can safely say that assigning them to anonymous users is simply impossible: edit own / delete own [content type / etc]; cancel own vote; select different theme; change own username; cancel account; select method for cancelling own account.
  • I'm in favour of permission groups, but as has been suggested, we should avoid namespace hell by only allowing groups to be chosen from a closed list, rather than letting module authors make up any old group. I'm against having a group called 'administration', because virtually all of the 'administer' permissions would then belong in 'administration' as well as in another group (e.g. 'content'). Instead, I suggest the following small list of groups, based on the grouping of the admin menu items on the '/admin' page: content, user, site configuration, site features.
  • -1 to grouping permissions based on any kind of "trust level" (as suggested in #11). That's just building a permissions system on top of a permissions system. Also -1 to disabling the checkboxes for any "dangerous" permissions, for any roles. Warnings in the descriptions is quite adequate for these. As webchick said in #36, assigning permissions to roles was actually one of the highest-scoring admin tasks in recent usability testing sessions. Users DO generally understand what most permissions are, and which roles they should be assigned to. We ain't gonna be smart makin' it broke if it's already fixed :P.
catch’s picture

The current interface works, but we did have a couple of people assigning variously insecure permissions (edit all content type as well as edit own for example) - and at least one person who wanted to set Full HTML as the default text filter - so there's a bit of an issue there, although it might require better text rather than an API change.

I really like the idea of disabling permissions which anonymous users can never have under any circumstances.

Damien Tournoud’s picture

We have got bitten by that just earlier on the Drupalcon Paris website. Anonymous users were granted the "edit own forum topic", and could thus edit all the anonymous forum topics.

David_Rothstein’s picture

Title: Some permissions should never be for anonymous » Label permissions which should be warned about in the user interface
Status: Needs work » Needs review
FileSize
10.89 KB

One day before code freeze seems like a GREAT time to revive this issue :)

But here is a patch nonetheless, and I think it's much improved.

  1. First, I changed the issue title. With this patch (and for quite some time in this issue), we are talking not about preventing permissions from being assigned, but rather about labeling permissions in such a way that contrib modules could do this if they wanted to, as well as do lots of other good things described above.
  2. We are talking about consolidating code/text that is currently repeated verbatim many places throughout core (just look at the patch).
  3. We are talking about making it easy for contrib modules to label their permissions in a way that is consistent with how core already does it.
  4. We are talking about dealing with the issue @chx mentioned in #51, where core currently hardcodes some permissions as dangerous but not others, even though on an individual site things might be different. That is one of the main rationales for having this patch and including hook_permission_alter() as part of it.

Note: To remove any controversial mentioning of "security" from the patch itself, I've made the API here simply use 'warn' => TRUE or 'warn' => FALSE, to reflect the way that Drupal core is actually using this labeling of the permissions. Nice and simple.

David_Rothstein’s picture

Title: Label permissions which should be warned about in the user interface » Label permissions which are warned about in the user interface

The issue title is still a bit wrong :) Drupal core already does warn about them in the user interface, and this patch doesn't change that one way or another.

It just makes it so the warnings are done in a more consistent, flexible and useful way.

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up
FileSize
16.63 KB

Here's an updated patch which also cleans up the API a bit so we can ensure that drupal_alter() is called wherever a list of permissions is requested.

Two new reasons why I think we need this patch:

  1. At #275811-52: Warn about potentially insecure filter configurations Barry Jaspan points out that the current security warnings on the permission page are extremely difficult to notice, even when you are looking for them. (Because they are themed as placeholders, they blend right into the background description.) This patch allows them to be themed differently, which is impossible to do (at least in any reasonable way) when they are not a separate piece of data.
  2. At #67234-92: Update script access rights Dave Reid points out that if a module wants to add functionality onto an existing permission, it's not clear how to make the permission description accurate for both cases. With this patch, hook_permission_alter() allows a module to expand another module's permission description, thereby solving that problem.

#1 is especially important, in my opinion.

sun’s picture

One thing is for sure.

This patch would've hit core already if the original proposal was - simply - not negated. Seriously, why are we over-complicating things?

function menu_permission() {
  $perms['administer menu'] => array(
    'description' => t('Manage menus and menu items.'),
    'access' => DRUPAL_AUTHENTICATED_RID,
  );
  return $perms;
}

All permissions flagged that way are, simply, not available for the anonymous role.

Put an hook_permissions_alter() somewhere and be done with it. The rare edge-case of Demo module wanting to expose administrative functionality to anonymous users can be dealt with that way. (And, well, Demo would take additional actions anyway to ensure that anonymous users can't alter certain permissions...)

And, yes, contributed modules can expand on that and push the requirements for certain permissions to another level than anonymous.

Outputting a warning is a usability decrease. The system shouldn't allow the action in the first place for the regular user.

pwolanin’s picture

@sun - interesting suggestion. Probably I went with the original negative approach in order to push a "secure by default" agenda. Either way would be an improvement over the system now where an elevated permission can not be prevented from being assigned to anonymous (barring some creative use of hook_form_alter())

sun’s picture

Title: Label permissions which are warned about in the user interface » Some permissions should never be granted for anonymous users
Component: base system » user system
Assigned: pwolanin » Unassigned
Category: feature » task
Priority: Normal » Critical
Issue tags: +Security improvements, +Usability, +#d7ux

Tagging. Reverting title and scope.

David_Rothstein’s picture

Title: Some permissions should never be granted for anonymous users » Label permissions which are warned about in the user interface

@sun, I really don't understand how going back to the original proposal would simplify the scope of this issue at all; on the contrary, it would complicate it. The necessary code would be similar either way, but your proposal also involves changes to the UI, whereas the patch in #58 makes no changes to the existing UI at all.

And the two people who can commit code to Drupal 7 are already on record above as being very opposed to those particular UI changes (with good arguments to back them up). So I think the chances of that approach getting in are very low.

In addition to avoiding UI changes, the patch in #58 deliberately avoids anything to do with roles at all, since we've seen that as soon we try to graft role information onto the permission system, things get complicated. It instead simply tries to organize the existing hook_permission() information in a more useful way. This information could then be used (among other places) by a security-minded module that used hook_form_alter() to disable permission checkboxes for whichever roles it wants to -- thereby making it possible to meet the original goal of the issue in contrib.

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
17.89 KB

I think this is still critical for D7. Drupal has too many security advisories that are along the lines of "cross-site scripting vulnerability for users with the 'administer some module' permission". Module authors should be able to clearly opt out of that for their most dangerous permissions (the way that core already does with the permissions listed at http://drupal.org/node/475848), and then these can be regular bugs rather than security issues.

Forcing them all be security issues adds work to the security team and hurts Drupal's reputation.

Here is an updated patch:

  1. I changed 'warn' => TRUE to 'restrict access' => TRUE since that is much clearer; the intention of the label is that site administrators should restrict access to these permissions to users they trust.
  2. I removed the ability for each permission to set a custom 'warning message' - this was not necessary and too much freedom. We don't want each permission to set a different message since that defeats the purpose of having a consistent labeling. It's still possible to override the message in the theme function.
  3. I improved the documentation in several places.

There are API changes in this patch, but they are relatively minor, and it is designed to make Drupal more secure. If we can't get this into Drupal 7, then we should really think about whether we want to continue displaying these hardcoded security warnings on the permission page at all; without this patch, they are uneditable, not likely to be consistently used by contributed modules, and displayed as placeholders which makes them almost impossible to notice...

Bojhan’s picture

Can we have a UX-understandable explanation Seems its mostly David evangalizing for this.

pwolanin’s picture

@Bojan - the UX is really obvious here - you are warned not to make you site insecure.

catch’s picture

From a UX standpoint there's no change except:

#1 Instead of hardcoding a security warning into the description of all permissions, we fill it automatically based on a flag - the text remains "Warning: Give to trusted roles only; this permission has security implications.".

#2 Contrib modules which have similar site-pwning permissions can add exactly the same warning using the API, instead of creating their own (so CCK's "DANGER" for PHP permissions, would instead be the same as the one above).

There's no CSS changes or anything here, but an admin theme, if it wanted to, could add spans around the warning and put them in red and etc.

And I agree 100% with David and pwolanin on this - we have a real issue in terms of both user education and perception about which permissions should be given to who, and this is a step towards rationalizing some of that, and reducing the number of SAs we put out which depend on 'Administer foo' permissions, of which there are a lot.

Bojhan’s picture

So which permissions are affected, just assuring we don't turn this into a warning fest.

David_Rothstein’s picture

FileSize
57.65 KB

This patch does not directly change either the UI or the affected permissions - Drupal core looks exactly the same with or without it :)

For the current UI, see the attached screenshot. It might be useful to explore theming it differently in a followup issue (which, like @catch said, this patch allows, but does not do). The italicized text that we are basically stuck with without this patch seems to me like it clogs up the page a bit and doesn't do much else...

As for the affected permissions, it's up to contrib modules how many permissions they want to label in this way. The API docs try to encourage them not to go overboard and to save it for permissions that really deserve it.

yoroy’s picture

Thanks catch.

Bojhan: 6 of the 54 core permissions get this label:
- Select method for cancelling own account
- Administer permissions
- Bypass node access
- Administer nodes
- Use the Full HTML text format
- Use the Filtered HTML text format
- Administer filters

Last three are near eachother, so it gets a bit repetitive there but alas.

I don't like the actual phrase though, doesn't need the actual 'warning' word and the other two parts should be switched. It gives solution first, problem second currently which is slightly annoying (" Hm…Drupal telling me what I should or shouldn't do again… oh wait, that sounds fair"). Should be "This permission has security implications; give to trusted roles only."
I also understand that this text is not introduced by this patch though, so don't hold it up on that.

Leaving it to code reviewers to mark it RTBC but no UX objections here.

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
17.88 KB

Here's a quick reroll.

Thanks for the reviews. I think the list in #70 is actually not 100% correct :) "Use PHP for settings" and "Administer unit tests" get the warning too (not really sure if the last one needs it, but again, that is not introduced by this patch). Presumably the exact list will need to be tweaked slightly before release anyway, as per #594412: Correctly label all site-owning super-admin permissions.

Also, the "Full HTML" and "Filtered HTML" permission warnings are a bit of a special case. The intention is that those warnings will be handled dynamically (based on the actual security of each text format) via #275811: Warn about potentially insecure filter configurations.

yoroy’s picture

Ah true, I only counted the ones visible in a default core install, not with all modules enabled.

ksenzee’s picture

I tried out the patch and reviewed the code, and I don't see a single thing to object to. The API docs for hook_permission_alter are particularly useful. The problem I do see is that it's an API change 13 days after the fact, to a hook that pretty much everybody implements. However, it definitely simplifies what D7 expects you to do if you provide a permission with security implications. Also, hook_permission_alter is a needed security improvement if we're going to start going around telling people which permissions do and do not have security implications -- and that part is already in core. So on balance I think this should go in, but because we're in freeze I'll try to get a security team member to opine before I mark it RTBC.

JacobSingh’s picture

I like the idea

although I would approach it as "administration perms vs other perms". Then you could make the permissions screen *much* shorter when the common use case is that the "admin" role gets everything, and there are only a dozen perms that matter for everyone else.

Anyway, it would be nice to ask the D7CX people what they think about this API change. There has already been a lot of traction there, and a couple sprints with significant cost. If we lay it on them to go back in and change all modules which are already upgraded, they may not be happy. Maybe it's fine, but I share @ksenzee's concern.

Still, RTBC for me outside of that.

David_Rothstein’s picture

So regarding the API change, it should hopefully be very minimal. It adds a new key to hook_permission(), but it's totally optional - if you don't use it, nothing will break. And most modules probably won't want to use it anyway.

The other API change in the patch is basically replacing module_invoke_all('permission') and friends with user_list_permissions() and friends. I'm pretty sure most modules out there never invoke that hook anyway, and even ones that do, it still won't break anything really if they keep using it - the only difference would be that they don't get the results of the drupal_alter(). So for the most part, this is just a small API change that would be nice for a few modules to pick up, but nothing breaks terribly if they don't.

sun’s picture

This patch does not do any API change that would break already ported modules.

+++ modules/simpletest/drupal_web_test_case.php	28 Oct 2009 04:43:54 -0000
@@ -931,7 +931,7 @@ class DrupalWebTestCase extends DrupalTe
-      $available = array_keys(module_invoke_all('permission'));
+      $available = array_keys(user_list_permissions());

Please change the signature to [subject]_[verb](), i.e.

user_permission_list()

+++ modules/system/system.module	28 Oct 2009 04:43:54 -0000
@@ -2545,7 +2545,8 @@ function system_get_module_admin_tasks($
-  if (in_array($module, module_implements('permission')) && $admin_access) {
+  $permissions = user_list_module_permissions($module);

Same function signature issue here.

Additionally, I don't see why we need to different functions to just do some optional filtering.

+++ modules/user/user.admin.inc	28 Oct 2009 04:43:54 -0000
@@ -651,9 +651,9 @@ function user_admin_permissions($form, $
+  foreach (user_list_permissions_by_module() as $module => $permissions) {
+++ modules/user/user.module	28 Oct 2009 04:43:55 -0000
@@ -611,6 +615,68 @@ function user_password($length = 10) {
+function user_list_module_permissions($module) {
...
+function user_list_permissions_by_module() {

huh?

Instead of 3 functions in total, we want user_permission_list() to return what this user_list_permissions_by_module() returns, optionally for one module only.

There is only one consumer of user_permission_list() that can happily do a call_user_func_array('array_merge_recursive', $permissions) on its own - if required at all.

+++ modules/user/user.admin.inc	28 Oct 2009 04:43:54 -0000
@@ -741,6 +741,37 @@ function theme_user_admin_permissions($v
+ * @param $variables
+ *   An associative array containing:
+ *   - permission_item: An associative array representing the permission whose
+ *     description is being themed. Useful keys include:
+ *     - 'description': The text of the permission description.
+ *     - 'warning': A security-related warning message about the permission (if
+ *       there is one).
+ *   - hide: A boolean indicating whether or not the permission description was
+ *     requested to be hidden rather than shown.
...
+function theme_user_permission_description($variables) {

(and elsewhere) Either all array keys with single quotes or without, but not both. Actually, there is no reason for using quotes, and the API parser output looks a bit nicer and more readable to me without, so I generally prefer without quotes.

+++ modules/user/user.module	28 Oct 2009 04:43:55 -0000
@@ -611,6 +615,68 @@ function user_password($length = 10) {
+function user_list_permissions() {
+  $permissions = array();
+  foreach (module_implements('permission') as $module) {
+    $permissions = array_merge($permissions, user_list_module_permissions($module));
+  }
+  return $permissions;

This should be the only function, and for performance reasons, we also want to statically cache the permissions after alteration.

This review is powered by Dreditor.

David_Rothstein’s picture

1. OK, I fixed the issue with array key quotes in the API docs (removed the quotes completely, as per @sun's recommendation).

2. I'm not sure about consolidating it all down to a single user_permission_list() function. The three functions introduced by this patch are each used at least twice in core, so that suggests they are generally useful. Replacing module_invoke_all('permission') with user_permission_list() seems like it improves code readability, but replacing it with call_user_func_array('array_merge_recursive', user_permission_list()) does not look too good to me. I'd like to get a second opinion on that one.

3. Also, about statically caching the permissions, I had thought about that, but I didn't want to complicate the patch. After all, we don't statically cache them now in core - we just invoke the hook directly. These aren't functions that ever really have much reason to be called outside admin pages, I think, so I'm not sure a static cache is worth it. Plus, it would complicate things, since there are a number of API functions outside of the user module that might then need to clear this static cache sometimes. For example, adding/deleting a text format or content type affects the permission list, so it seems like both node.module and filter.module would need to know about this (or maybe user.module would have to implement hooks to respond to these events, etc). This sounds complicated to me for something where the need for high performance isn't obvious, so I'd like to push that discussion to a followup if we can.

sun’s picture

- Still missing the subject_verb() function signature change.

- re: 2) right, I was wrong, and we can merge by default. If absolutely necessary, then I can live with 2 functions (one that doesn't merge, and one that takes the other's result and merges), but not with 3. Especially, because those function signatures are very confusing (what's the difference between "module permissions" and "permissions by module" ?!). In addition, it seems like some of the calling code could be heavily simplified, since you just replaced the module_invoke() lines, but not the actual surrounding code logic. Hence, I guess that at least one of those 3 functions is not really used at all.

- My main concern are the admin* pages, where one of these functions is invoked (as visible in the patch). However, we can definitely skip the static caching and defer any resulting performance stuff to a separate issue.

David_Rothstein’s picture

Yeah, I figured I'd wait until we figured out what we wanted the functions to be before renaming them :)

OK, so I think I can get behind the idea of two functions. So perhaps:

  • user_permission_list($module = NULL)
  • user_permission_list_by_module()

where the first one either returns an uncategorized list of all permissions or only those permissions for a particular module - depending on how it is called - and the second one returns the full categorized list.

If sounds good, I'll reroll with those changes, and yeah, we'll defer the static caching idea. (If I'm not mistaken, there's no place in core that would actually benefit from the static caching because I don't think any page request invokes the permission hook twice for the same module, but I could be wrong about that.)

pwolanin’s picture

There is really no need for static caching here - I'd generally see that as relevant where there is large amounts of processing or analysis.

the use of 'restrict access' as the array key seems misleading. Unless I'm missing something, this does not actually restrict anything, it just adds the warning. Is this supposed to be disabling the checkboxes?

I'd really still prefer the inverse logic - e.g. the default is 'safe permission' => FALSE or some such, so when you add a new permission you have to add this to specify that it does not have security effects.

ksenzee’s picture

I'd really still prefer the inverse logic - e.g. the default is 'safe permission' => FALSE or some such, so when you add a new permission you have to add this to specify that it does not have security effects.

If we do that, then this API change really does affect all the modules that have already been ported...

pwolanin’s picture

@ksenzee - yes, I realize that. it's what I'd prefer, but just changing the name of the array key might be the best we can do for D7.

Mike Wacker’s picture

Status: Needs review » Needs work

I have some larger thoughts on the approach to this as a whole, but here's two things that stick out.

  1. Let's take the user_permission* functions out of this patch and move those into a separate issue. This is a separate thing which does not have any direct implications on dangerous permissions. That refactoring is making the patch much bigger and giving people who want to review this patch for usability and/or security more than they bargained for. And there's always the risk that we miss something and inadvertently introduce a bug into Drupal. That's a high cost for something with no direct connection to dangerous permissions.
  2. Along the lines of what webchick said earlier, I'm really hesitant to introduced hook_permissions_alter(), especially this late into the product cycle. The threat of this hook being misused is much greater than the risk of not dealing with some obscure Intranet scenario with trusted anonymous users. Even if we make it API-compatible, this change still has a lot of implications for Drupal 7, ones which we shouldn't deal with at this late stage.

1 is why I'm marking this as "needs work", though 2 is also very important in its own right.

sun’s picture

Status: Needs work » Needs review

We can try to skip 2) (the alter hook), but 1) is required - otherwise, this patch will introduce a lot of duplicated code.

David_Rothstein’s picture

the use of 'restrict access' as the array key seems misleading. Unless I'm missing something, this does not actually restrict anything, it just adds the warning. Is this supposed to be disabling the checkboxes?

Better suggestions for a name? :)

I think we've been through several possible choices, and 'restrict access' was the best I could come up with... The intention is that it tells the site administrator that they should be restricting access to the permission. (Also, of course, a contrib module could use it to restrict access automatically -- e.g., by disabling the checkboxes.)

I think I'd be fine with something like 'unsafe permission' => TRUE but earlier in this issue people were very wary of using any kind of security-related words here (because the boundary between a security risk and not can be nebulous)... so I tried to stay away from that in the recent patches.

David_Rothstein’s picture

We can try to skip 2) (the alter hook), but 1) is required - otherwise, this patch will introduce a lot of duplicated code.

Hm, I was about to say the opposite actually... the reason we need the new API functions is to ensure that the alter hook is consistently called.

We could try to do this patch without the alter hook (and therefore without the new API functions as well), but it would be pretty sad and miserable. The fact is that what is and is not a "security risk" can differ from site to site. It has absolutely nothing to do with "some obscure Intranet scenario with trusted anonymous users"...

If you believe that there is a risk of the hook being misused, please explain what it is. Any hook in Drupal can be misused :) This one is considerably safer than most.

Mike Wacker’s picture

I'll need more clarification on 1. In the patch, I don't see anyplace where we add a new call to a user_permissions* function (aside from a user_permissions* function calling a similar function), but I see a whole lot of places where we refactor existing code to use this new function.

Would we have to change all this code anyway in light of #2? If not, then we definitely must move this into a separate issue.

If so, then I'm even more hesitant to introduce hook_permissions_alter, as in addition to everything I've said before, this new hook is causing a lot of code churn.

I'm not saying that code churn is bad, and I think there's a good debate to be had in regards to #2, but permissions, unlike content, affect the entire website. I personally think it's too late in the product cycle to be having this debate or introducing this amount of code churn.

pwolanin’s picture

@David - something like 'label as unsafe' 'label as admin' .. dunno - the core code will be labeling the permissions, not restricting them.

I think we can really forgo an alter hook if needed since lots can still be altered at the form level if that's the main point of interest. Perhaps we should make sure to add an extra CSS class if possible for visual effects?

David_Rothstein’s picture

I'll need more clarification on 1. In the patch, I don't see anyplace where we add a new call to a user_permissions* function (aside from a user_permissions* function calling a similar function), but I see a whole lot of places where we refactor existing code to use this new function.

Yes, that's exactly the point :) If we add an alter hook, we need to ensure that the hook is called everywhere in the existing codebase where permissions are listed. Otherwise, it isn't a real alter hook... The only way to do that is to add a new API function and then use it.

I think we can really forgo an alter hook if needed since lots can still be altered at the form level if that's the main point of interest.

OK, for a site-specific case, I can sort of imagine a solution where you can do these kinds of alterations by combining hook_form_alter() and a theme preprocess function.... I think it's kind of ugly, since ultimately this is meta-data about the permissions, so the data should be really be altered in a way that anyone who uses the data has access to the same information.

But if the consensus really is that we should save the rest for D8, I guess I could live with that. I'm not quite sure we're in the "deliberately write bad code to solve the problem with the smallest patch possible" stage of D7 development yet, but maybe we are :)

Perhaps we should make sure to add an extra CSS class if possible for visual effects?

That's a good idea. I used theme('placeholder') in the patch because that was there before, but that is really a relic of the previous approach; a CSS class themed to be in italics would look the same visually but make a million times more sense :)

David_Rothstein’s picture

OK, let's move this along and try it without the hook_permission_alter(). I'm still a little uneasy about having meta-data that can change from site to site but is not directly alterable, but on the other hand, it's still a lot better than the current situation in core (where the meta-data is embedded in a text string where it's literally impossible to find or alter via any kind of reasonable method).

This patch also adds the CSS class as per #89. It does not change the key name from 'restrict access' because I'm still not sure of a better name... Dries and webchick like to make suggestions on this kind of thing when they're committing patches, so perhaps we can let them weigh in :)

pwolanin’s picture

looks about done to me.

Minor comment though:

+          '#description' => theme('user_permission_description', array('permission_item' => $perm_item, 'hide' => $hide_descriptions)),

seems a little funny to me - the original code just omits the description - what's the use case for a theme function that's supposed to do nothing if that 2nd param is TRUE?

David_Rothstein’s picture

My thought was that especially with the security warning, a theme might choose to still display that (in some form or another) even when the main permission description is hidden.

And even for the main permission description, maybe there is a use case for "hiding" it via some other method than not putting it in the HTML at all - for example, maybe it appears on hover? I think it's properly up to the theme layer to decide what "hidden" actually means.

pwolanin’s picture

If that's the motivation, then the entire form item should pass through a theme function, something like:

         $form['permission'][$perm] = array(
           '#type' => 'item',
           '#title' => $perm_item['title'],
           '#description' => isset($perm_item['description']) ? $perm_item['description'] : NULL,
           '#hide_description' => $hide_descriptions,
           '#theme' => 'permission_item'
         );
RobLoach’s picture

Subscribing!

moshe weitzman’s picture

So, are we still refining this? If not, lets go to rtbc

cburschka’s picture

It's nice that this patch abstracts the warning to a central part, but it also lets us customize the message. Shouldn't we actually use that capability rather than letting it default to a generic message?

"Allowing a user to administer users will enable them to gain further permissions at will or lock other users out of the site." seems like a good description, for example...

And "Allowing a user to bypass filter access restrictions or use the PHP filter will enable them to execute arbitrary PHP code and severely damage the site."

sun.core’s picture

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

There is too much controversy in this issue to be able to remotely apply a critical priority for D7 to it. "Nice to have" for UX, but not really required work with D7.

moshe weitzman’s picture

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

If thats true, then Drieschick can postpone this. Its a major security improvement.

pwolanin’s picture

Status: Needs review » Needs work
Issue tags: +Security improvements, +Usability, +anonymous users, +#d7ux, +API clean-up

The last submitted patch, permission-warnings-248598-91.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
10.87 KB

Here is a straight reroll of #91.

I think #94 makes sense, and I've tried it before and tried it again now, but not sure how to make it work without ripping things apart. As far as I can tell, the current code uses a #type intended for form elements but tricks the theme system to mostly output it not like a form element by using #markup for the title and then #description for everything else.... If you put a theme function in the middle of all that, everything changes. I'm sure it's possible to make work and maybe I'm just missing something, but I don't have the energy to keep trying - I think it's still an improvement as is. Someone else can tweak it further if they want to.

For #97, let's not do that in this issue - possibly discuss it in a followup. This patch makes that kind of thing possible, but for the purposes of this issue there is no reason to change the current UI.

yoroy’s picture

Agreed with David that suggestion in #97 is for a follow up (and probably D8 material since strings are frozen). If you all can agree on the code and testbot is happy then this is rtbc.

Status: Needs review » Needs work
Issue tags: -Security improvements, -Usability, -anonymous users, -#d7ux, -API clean-up

The last submitted patch, permission-warnings-248598-102.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements, +Usability, +anonymous users, +#d7ux, +API clean-up
David_Rothstein’s picture

I couldn't reproduce those test failures locally - and they certainly seem unrelated to the patch...

yoroy’s picture

Reviewing the patch once more for UX:

- Lets drop the 'Warning: ' part from the actual warning string. That's Drupal being bossy again, and even in this important case it's not necessary to actually spell it out that this is a warning. The warning is in the actual wording of the message.

- A little bit of html is allowed inside t(), no? We could then semantically emphasize this sentence by wrapping it inside [em] tags.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Nevermind, #107 is follow up material as well, as this patch does not capture all uses of "Warning: " strings on the permissions page.

#102 is RTBC then

bgm’s picture

I have reviewed the patch as well and it looks OK.

aspilicious’s picture

Reroll (chasing head)

David_Rothstein’s picture

Thanks! But in the interim, a couple more permissions in core have been given the warning label, so we need to convert the additional ones to the new system as well.

Also, @yoroy's second suggestion in #107 of using <em> rather than <span> makes a lot of sense - rather than having this patch remove the <em> and then add custom CSS for the italics, we should just keep the existing <em> there. (And by the way, to clarify again, since a lot of people above were mentioning UX issues: The only change this patch makes to the HTML output by Drupal is that it replaces <em class="placeholder"> with <em class="permission-warning"> on these permission warnings. The UI and text stay the same.)

Still RTBC, I think, since these changes are completely trivial.

sun’s picture

+++ modules/system/system.api.php	17 Mar 2010 18:28:11 -0000
@@ -1012,13 +1012,22 @@ function hook_system_info_alter(&$info, 
+ *   - title: Required. The human-readable name of the permission, to be shown
...
+ *   - description: Optional. A description of what the permission does. This
...
+ *   - restrict access: Optional. A boolean which can be set to TRUE to

In general, we do not document "Required."

"Optional." should be "(optional)"

EDIT: Apparently, this was still missing in the coding standards, albeit applied to almost everything in core already. Now clarified: http://drupal.org/node/1354#general

Powered by Dreditor.

David_Rothstein’s picture

Fixed those.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

This needed to be documented since it is an API addition from Drupal 6 (and a minor API change from earlier in the D7 cycle). I did so here:
http://drupal.org/update/modules/6/7#permissions_restrict_access

I also created an issue (for Drupal 8) about adding hook_permission_alter():
#763074: Add hook_permission_alter() so that permission descriptions and other properties can be altered

Gábor Hojtsy’s picture

This was committed past Drupal 7 alpha3, so moving the listing of this change past that mark in the update docs (adding the alpha4 mark).