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;
}
Comment | File | Size | Author |
---|---|---|---|
#113 | permission-warnings-248598-112.patch | 11.22 KB | David_Rothstein |
#111 | permission-warnings-248598-111.patch | 11.23 KB | David_Rothstein |
#110 | permission-warnings-248598-110.patch | 10.41 KB | aspilicious |
#102 | permission-warnings-248598-102.patch | 10.87 KB | David_Rothstein |
#91 | permission-warnings-248598-91.patch | 11.91 KB | David_Rothstein |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedstarter patch - most of the necessary changes made in user module, but not yet in other modules.
Comment #2
pwolanin CreditAttribution: pwolanin commentedseems 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.
Comment #3
floretan CreditAttribution: floretan commentedThis 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.
Comment #4
pwolanin CreditAttribution: pwolanin commented@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.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat'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.).
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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:
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?
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedSee #73874: Normalize permissions table...
Comment #8
Susurrus CreditAttribution: Susurrus commentedI 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.
Comment #9
Dries CreditAttribution: Dries commentedI'm not sure about this. In certain setups, it might be OK to let anonymous users use PHP filters.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedOr 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.)
Comment #12
Susurrus CreditAttribution: Susurrus commented@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.
Comment #13
pwolanin CreditAttribution: pwolanin commentedSince 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?
Comment #14
Susurrus CreditAttribution: Susurrus commentedI 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.
Comment #15
pwolanin CreditAttribution: pwolanin commented@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.
Comment #16
Susurrus CreditAttribution: Susurrus commentedI 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.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedOne 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:
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?
Comment #18
pwolanin CreditAttribution: pwolanin commented@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.
Comment #19
macgirvin CreditAttribution: macgirvin commentedHere'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.
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commented@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 hadrestricted permission = TRUE
and the role hadtrusted = FALSE
.Comment #21
macgirvin CreditAttribution: macgirvin commented@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.
Comment #22
pwolanin CreditAttribution: pwolanin commentedLet 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?
Comment #23
macgirvin CreditAttribution: macgirvin commentedActually 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.
Comment #24
Crell CreditAttribution: Crell commentedThe "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.
Comment #25
catchOK 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).
Comment #26
Crell CreditAttribution: Crell commented@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:
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?
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedMaking 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.
Comment #28
catchCrell, 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.
Comment #29
dmitrig01 CreditAttribution: dmitrig01 commented@David: Yes. Disabling is good.
@Dries: You can hook_perm_alter in that case.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commented@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.
Comment #31
Crell CreditAttribution: Crell commentedI 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!)
Comment #32
Dries CreditAttribution: Dries commentedI 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.
Comment #33
Crell CreditAttribution: Crell commentedWhat 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.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.Comment #35
macgirvin CreditAttribution: macgirvin commentedIf 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.
Comment #36
webchickSomething 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.
Comment #37
pwolanin CreditAttribution: pwolanin commented@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.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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):
'%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 ;)Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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.
Comment #40
Dries CreditAttribution: Dries commentedI'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.
Comment #41
pwolanin CreditAttribution: pwolanin commented@Dries - I think we should make the switch to a structure that supports more meta-data, but postpone any other changes for now.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedI 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...
Comment #43
Damien Tournoud CreditAttribution: Damien Tournoud commented#182023: Add a third default role to core for handling Administrative duties has some related discussions about implementing a default "administrator role".
Comment #44
Senpai CreditAttribution: Senpai commentedI marked that issue #182023 as Postponed. Let's hope this one doesn't meet the same fate.
Comment #45
Garrett Albright CreditAttribution: Garrett Albright commentedI 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.
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat about introducing role types that we require in the hook_perm? Something like
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.
Comment #47
pwolanin CreditAttribution: pwolanin commentedso, per the Jaspan school-of-DX, I'd suggest that the attributes be defines, rather than strings:
something like:
Comment #48
Garrett Albright CreditAttribution: Garrett Albright commented…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.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #50
Dave ReidComment #51
chx CreditAttribution: chx commentedSure 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.
Comment #52
Jaza CreditAttribution: Jaza commentedSome thoughts:
Comment #53
catchThe 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.
Comment #54
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe 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.
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedOne 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.
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.Comment #56
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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 is especially important, in my opinion.
Comment #59
sunOne 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?
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.
Comment #60
pwolanin CreditAttribution: pwolanin commented@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())
Comment #61
sunTagging. Reverting title and scope.
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commented@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.
Comment #64
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
'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.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...
Comment #65
Bojhan CreditAttribution: Bojhan commentedCan we have a UX-understandable explanation Seems its mostly David evangalizing for this.
Comment #66
pwolanin CreditAttribution: pwolanin commented@Bojan - the UX is really obvious here - you are warned not to make you site insecure.
Comment #67
catchFrom 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.
Comment #68
Bojhan CreditAttribution: Bojhan commentedSo which permissions are affected, just assuring we don't turn this into a warning fest.
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #70
yoroy CreditAttribution: yoroy commentedThanks 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.
Comment #72
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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.
Comment #73
yoroy CreditAttribution: yoroy commentedAh true, I only counted the ones visible in a default core install, not with all modules enabled.
Comment #74
ksenzeeI 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.
Comment #75
JacobSingh CreditAttribution: JacobSingh commentedI 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.
Comment #76
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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 withuser_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.Comment #77
sunThis patch does not do any API change that would break already ported modules.
Please change the signature to [subject]_[verb](), i.e.
user_permission_list()
Same function signature issue here.
Additionally, I don't see why we need to different functions to just do some optional filtering.
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.
(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.
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.
Comment #78
David_Rothstein CreditAttribution: David_Rothstein commented1. 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')
withuser_permission_list()
seems like it improves code readability, but replacing it withcall_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.
Comment #79
sun- 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.
Comment #80
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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:
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.)
Comment #81
pwolanin CreditAttribution: pwolanin commentedThere 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.Comment #82
ksenzeeI'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...
Comment #83
pwolanin CreditAttribution: pwolanin commented@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.
Comment #84
Mike Wacker CreditAttribution: Mike Wacker commentedI have some larger thoughts on the approach to this as a whole, but here's two things that stick out.
1 is why I'm marking this as "needs work", though 2 is also very important in its own right.
Comment #85
sunWe can try to skip 2) (the alter hook), but 1) is required - otherwise, this patch will introduce a lot of duplicated code.
Comment #86
David_Rothstein CreditAttribution: David_Rothstein commentedBetter 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.Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #88
Mike Wacker CreditAttribution: Mike Wacker commentedI'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.
Comment #89
pwolanin CreditAttribution: pwolanin commented@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?
Comment #90
David_Rothstein CreditAttribution: David_Rothstein commentedYes, 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.
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 :)
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 :)
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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 :)
Comment #92
pwolanin CreditAttribution: pwolanin commentedlooks about done to me.
Minor comment though:
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?
Comment #93
David_Rothstein CreditAttribution: David_Rothstein commentedMy 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.
Comment #94
pwolanin CreditAttribution: pwolanin commentedIf that's the motivation, then the entire form item should pass through a theme function, something like:
Comment #95
RobLoachSubscribing!
Comment #96
moshe weitzman CreditAttribution: moshe weitzman commentedSo, are we still refining this? If not, lets go to rtbc
Comment #97
cburschkaIt'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."
Comment #98
sun.core CreditAttribution: sun.core commentedThere 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.
Comment #99
moshe weitzman CreditAttribution: moshe weitzman commentedIf thats true, then Drieschick can postpone this. Its a major security improvement.
Comment #100
pwolanin CreditAttribution: pwolanin commented#91: permission-warnings-248598-91.patch queued for re-testing.
Comment #102
David_Rothstein CreditAttribution: David_Rothstein commentedHere 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.
Comment #103
yoroy CreditAttribution: yoroy commentedAgreed 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.
Comment #105
David_Rothstein CreditAttribution: David_Rothstein commented#102: permission-warnings-248598-102.patch queued for re-testing.
Comment #106
David_Rothstein CreditAttribution: David_Rothstein commentedI couldn't reproduce those test failures locally - and they certainly seem unrelated to the patch...
Comment #107
yoroy CreditAttribution: yoroy commentedReviewing 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.
Comment #108
yoroy CreditAttribution: yoroy commentedNevermind, #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
Comment #109
bgm CreditAttribution: bgm commentedI have reviewed the patch as well and it looks OK.
Comment #110
aspilicious CreditAttribution: aspilicious commentedReroll (chasing head)
Comment #111
David_Rothstein CreditAttribution: David_Rothstein commentedThanks! 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.
Comment #112
sunIn 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.
Comment #113
David_Rothstein CreditAttribution: David_Rothstein commentedFixed those.
Comment #114
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #116
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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
Comment #117
Gábor HojtsyThis was committed past Drupal 7 alpha3, so moving the listing of this change past that mark in the update docs (adding the alpha4 mark).