Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Wouldn't it be great to have a user_access_alter hook? It's a line of code (drupal_alter() placed in user_access just before the return statement) and it would open great new possibilities.
Comments
Comment #1
Walt Haas CreditAttribution: Walt Haas commentedI would really like to see this myself. Right now I'm building a site where one role is "current member". An authenticated user is a current member if their dues are up-to-date. To test this we need to compare today's date with the expiration date in the user's account. In D7 there is no straightforward way to add this test.
Comment #2
Xano@Walt Haas: You can use an extra role for this and use cron to remove it if a user' membership has expired.
My use case is to be able to set per-user permissions, for instance when selling products, for which using roles would make the administration screens too cluttered.
Comment #3
westwesterson CreditAttribution: westwesterson commentedHere is a patch for drupal 8.
My machine added a bunch of junk to the top of the patch, the good stuff is at the bottom. only 4 lines of code.
Comment #4
westwesterson CreditAttribution: westwesterson commentedSame patch; less junk :)
Comment #5
westwesterson CreditAttribution: westwesterson commentedbetter patch; use more standard drupal_alter instead of a foreach loop.
Now only 1 line of code!
Comment #7
westwesterson CreditAttribution: westwesterson commentedTest bot does not seem to like my previous patch format; reposting again using the 'official drupal patch format'
Comment #9
westwesterson CreditAttribution: westwesterson commentedchanging back name of issue.
Comment #10
westwesterson CreditAttribution: westwesterson commentedChanging the title to make it more clear what the benefit is.
Comment #11
univate CreditAttribution: univate commentedI am looking for a creative way to alter the permissions for a user based on various conditions and this appears to be a clean and efficient way.
Although I can't see how the previous patch could work, I think the drupal_alter should be shifted up one line.
Attached an updated patch and one for D7 as well.
There are various applications for this feature, where instead of adding or removing roles on a users you could use this alter to remove or add permissions on an account on more dynamic features.
Comment #12
westwesterson CreditAttribution: westwesterson commentedGood catch!
Should have been up one line.
Comment #13
Fabianx CreditAttribution: Fabianx commentedYou have around 14 days to get this in (till feature freeze). This will need tests :) as it is new functionality.
Comment #14
westwesterson CreditAttribution: westwesterson commentedI have created tests for drupal access alter. See attached drupal 8 patch!
Changes from previous patch:
Comment #15
westwesterson CreditAttribution: westwesterson commentedignore this comment, see #16
Comment #16
westwesterson CreditAttribution: westwesterson commentedWhoops, forgot to post patch in last comment.
This patch includes everything from patch in comment 14
Changes from previous patch:
Comment #17
Fabianx CreditAttribution: Fabianx commentedLooks very good to me. You can use diff -C to show renames instead of all the --- and +++ that makes the patch easier to read and verify.
Besides that:
Adds one line, has tests, passes those tests, is an useful addition => RTBC
This will need a change notice.
Comment #18
Xanohook_user_access_alter() needs documentation in user.api.php.
Comment #19
Fabianx CreditAttribution: Fabianx commented#18: Yup, adding tag ...
Comment #20
westwesterson CreditAttribution: westwesterson commentedChanges since last patch:
Comment #21
westwesterson CreditAttribution: westwesterson commentedLast patch was weird... rerolling. Same fixes.
Comment #22
westwesterson CreditAttribution: westwesterson commentedChange notification added:
http://drupal.org/node/1840642
Comment #23
XanoFunction names should be suffixed by brackets: user_access().
Again, brackets.
This is not necessarily true. It's the account access/permissions are checked for, but it hasn't necessarily been passed on to user_access().
I'm not sure if it's required yet, but I would add array type hinting for the first argument.
Constants are uppercase.
Comment #24
westwesterson CreditAttribution: westwesterson commentedChanges since last patch:
to
+function hook_user_access_alter(array &$permissions, $account) {
I did check and array type hinting hasn't landed for the user module yet, but it's emminent, so I added it to this patch.
See #1800174: Add missing type hinting to User module docblocks
Comment #25
westwesterson CreditAttribution: westwesterson commentedFound a few minor areas of clean up since last patch.
Changes since last patch:
Comment #26
Crell CreditAttribution: Crell commentedWhy is there a change notice when this patch hasn't been committed yet? We (generally) don't do that.
Comment #27
westwesterson CreditAttribution: westwesterson commented@crell I'll take the blame for that... pardon my ignorance.
The issue was tagged with needs change notification, and I wasn't aware of an order of operations on the change notification.
Comment #28
Fabianx CreditAttribution: Fabianx commentedAccording to coding standards there should be a new line (\n) at the end of file to prevent these messages.
This should probably say that is alter hook is only invoked once per user - as the result is cached per user.
Besides that this looks really good already in my opinion.
Leaving for others to review this more and need to review myself more closely again before it can go back to RTBC :).
The change notice, though preliminary, should be more detailed and at least include the example from user.api.php.
Comment #29
westwesterson CreditAttribution: westwesterson commentedChanges since last patch:
Comment #30
Fabianx CreditAttribution: Fabianx commentedI personally would say:
"This hook is only invoked once per user during a request as a result of user_access() being statically cached."
Comment #31
westwesterson CreditAttribution: westwesterson commentedChanges since last patch:
I agree, this description is more specific. Rerolled patch with wording change.
Comment #33
westwesterson CreditAttribution: westwesterson commented#31: D8-user_access_alter-1536612-31.patch queued for re-testing.
Comment #34
Fabianx CreditAttribution: Fabianx commentedNo need for re-test, HEAD is currently broken so this will fail independently of this patch.
Comment #35
Crell CreditAttribution: Crell commentedI'll be honest, I'm not even sure this issue is a good idea. What's the use case? "Does user X have permission Y" is, and should be, a deterministic operation. Introducing runtime alteration to it really messes with predictability, and I can easily see causing security holes.
There's already plans that are nearly-commitable (needs reroll) to allow routes (where most permission checks happen) to have multiple access checks: #1793520: Add access control mechanism for new router system. Wouldn't that handle the common case here without introducing even more non-deterministic behavior?
Comment #36
westwesterson CreditAttribution: westwesterson commentedThis change would allow alternative permissions systems to coexist with the core permission system.
Such as potential use cases:
I am sure there are more use cases that are not mentioned in the above.
Is this more insecure than the module.routing.yml file which allows for user authentication systems to be changed programmatically on a page or request basis?
I encourage a full security review.
If the proposed change #1793520: Add access control mechanism for new router system can handle the above use cases, I will agree that this change is unnecessary.
Comment #37
Fabianx CreditAttribution: Fabianx commentedCould you give an example?
I also see some use cases, where roles are just not good enough and you only want to give user X, permission Y.
It is contribs task to make sure this is right, but how is this alter_hook more insecure than going into settings and allowing anonymous "override content permissions" or using hook_menu_alter and setting "admin/..."[access_callback] = TRUE?
Then you would also need to remove hook_menu_alter and equivalents ...
Comment #38
Crell CreditAttribution: Crell commentedhook_menu_alter is a "compile time" hook. hook_user_access_alter would be a "runtime" hook. Runtime hooks are more unpredictable. Do you know if someone is pw0ning your permission so that "administer site configuration" sometimes means something else? It means that I as a module author cannot know if my permission check is actually a permission check, or just a runtime "I am doing something kinda sorta like this, which maybe someone will change out from under me." And then the UI could end up being incorrect, too, because of some alter hook, so what you see in the UI configuration is not actually true. That's asking for trouble, and for user misconfiguration.
The aforementioned router issue makes route permissions work more like hook_node_access(), but even more explicit. You specify on a route a _permission key, which is a permission that grants a user access to that route. But you can also specify any other keys you want, say _uri_token for some GET parameter, or _user_in_og, or whatever. If any one of those returns TRUE, and none return FALSE, then access is granted. Because each of those is managed by a checker object registered in the DIC, you can replace it, too. So if you wanted to *remove* the object that does role-based checks to find user permission and replace it with one that does per-user permissions, you can. Or if you want to add a second checker that also uses _permission and does per-user checks in addition to role-based checks, you can. There's a ton of flexibility there, and it's reasonably performant because the checking of which checkers care about which route is done at compile time, not at runtime.
So given that, I don't think this hook is even necessary. The above patch already offers a more flexible, more performant, more predictable alternative.
Comment #39
westwesterson CreditAttribution: westwesterson commented@crell Thanks for taking the time to explain this!
Now that #1793520: Add access control mechanism for new router system has landed, I have a question.
User access callbacks are sometimes used outside of page (or route) loading in order to control the display of specific parts of the page, will the method described (overriding the dependency injection controller) still solve for this use case of not being used in the initial loading of a route?
Comment #40
Crell CreditAttribution: Crell commentedNo, the mechanism above is at the moment strictly for routing, and may dovetail over into menu links, TBD.
That said, my point about the security risks inherent in this alter still remain. The only other place you ever should be calling user_access() (which I hope turns into a service anyway) would be to set the #access property of a form... which you can already alter directly via hook_form_alter().
So I'm still -1 on this hook. We don't need more runtime alters.
Comment #41
Grayside CreditAttribution: Grayside commentedThis is in the category of stuff that is very tricky. I'd rather see dynamic role assignment or permissions as a swappable system (sounds good, don't know what it would really mean :). I would not want to take over a large project that made clever use of this hook.
Comment #42
rajasekar33 CreditAttribution: rajasekar33 commented#31: D8-user_access_alter-1536612-31.patch queued for re-testing.
Comment #44
xjmWe don't generally add the change notice tag until the patch is committed. :)
Comment #45
meba CreditAttribution: meba commentedConsidering we are beyond freeze this should probably be either won't fix or postponed?
Comment #46
Crell CreditAttribution: Crell commentedI'm going to go ahead and won't-fix, per above discussion.
Comment #47
Vergil.K CreditAttribution: Vergil.K as a volunteer commentedFor D7?
Looking at the discussion above I understand the we have an alternate way (may be not) instead of using this hook in D8 but what about D7?
An example use case among others (a lot) would be:
A member of an OG Group with the OG Role "Editor" needs access to the Admin Menu but the Admin Menu is restricted by a Drupal Role Permission.
Right now, the "Editor" member cannot access the Admin Menu unless he has a Drupal Role with that permission.
hook_user_access_alter and hook_og_permission can be used together to accomplish the above use case easily.
I know that we have other serious alter hooks (hook_menu_alter - mentioned by Fabianx) in Drupal but why not this?
Comment #48
Crell CreditAttribution: Crell at Platform.sh commentedPlease don't reopen long-dead issues.