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.

Files: 
CommentFileSizeAuthor
#31 D8-user_access_alter-1536612-31.patch5.08 KBwestwesterson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D8-user_access_alter-1536612-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 D8-user_access_alter-1536612-29.patch5.05 KBwestwesterson
FAILED: [[SimpleTest]]: [MySQL] 48,127 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
#25 D8-user_access_alter-1536612-25.patch4.99 KBwestwesterson
PASSED: [[SimpleTest]]: [MySQL] 48,117 pass(es).
[ View ]
#24 D8-user_access_alter-1536612-24.patch5.02 KBwestwesterson
PASSED: [[SimpleTest]]: [MySQL] 48,131 pass(es).
[ View ]
#21 D8-user_access_alter-1536612-21.patch4.98 KBwestwesterson
PASSED: [[SimpleTest]]: [MySQL] 48,114 pass(es).
[ View ]
#20 D8-user_access_alter-1536612-20.patch9.41 KBwestwesterson
FAILED: [[SimpleTest]]: [MySQL] 48,107 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 D8-user_access_alter-1536612-15.patch8.35 KBwestwesterson
PASSED: [[SimpleTest]]: [MySQL] 48,087 pass(es).
[ View ]
#14 D8-user_access_alter-1536612-14.patch8.39 KBwestwesterson
PASSED: [[SimpleTest]]: [MySQL] 48,105 pass(es).
[ View ]
#11 D8-user_access_alter-1536612-11.patch414 bytesunivate
PASSED: [[SimpleTest]]: [MySQL] 47,991 pass(es).
[ View ]
#11 D7-user_access_alter-1536612-11-do-not-test.patch394 bytesunivate
#7 drupal-add_user_access_alter-1536612-5.patch414 byteswestwesterson
PASSED: [[SimpleTest]]: [MySQL] 46,450 pass(es).
[ View ]
#5 add_user_access_alter.patch514 byteswestwesterson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add_user_access_alter_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 add_user_access_alter.patch633 byteswestwesterson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add_user_access_alter_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 add_user_access_alter.patch109.55 KBwestwesterson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add_user_access_alter.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

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

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

Status:Active» Needs review
StatusFileSize
new109.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add_user_access_alter.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new633 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add_user_access_alter_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Same patch; less junk :)

StatusFileSize
new514 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch add_user_access_alter_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

better patch; use more standard drupal_alter instead of a foreach loop.

Now only 1 line of code!

Status:Needs review» Needs work

The last submitted patch, add_user_access_alter.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new414 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,450 pass(es).
[ View ]

Test bot does not seem to like my previous patch format; reposting again using the 'official drupal patch format'

changing back name of issue.

Title:hook_user_access_alterAdd hook_user_access_alter allowing contrib to do more advanced things with basic user access.
Assigned:Unassigned» westwesterson

Changing the title to make it more clear what the benefit is.

StatusFileSize
new394 bytes
new414 bytes
PASSED: [[SimpleTest]]: [MySQL] 47,991 pass(es).
[ View ]

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

Good catch!
Should have been up one line.

Status:Needs review» Needs work
Issue tags:+Needs tests

You have around 14 days to get this in (till feature freeze). This will need tests :) as it is new functionality.

Status:Needs work» Needs review
StatusFileSize
new8.39 KB
PASSED: [[SimpleTest]]: [MySQL] 48,105 pass(es).
[ View ]

I have created tests for drupal access alter. See attached drupal 8 patch!

Changes from previous patch:

  • Added tests to check for user_access_alter.
  • user_access_alter now passes through the account variable from user_access
  • user_form_test was moved to a folder user_form_test inside the core/module/user/tests where it resided before patch.

ignore this comment, see #16

StatusFileSize
new8.35 KB
PASSED: [[SimpleTest]]: [MySQL] 48,087 pass(es).
[ View ]

Whoops, forgot to post patch in last comment.

This patch includes everything from patch in comment 14

Changes from previous patch:

  • Comment cleanup.
  • Ordering of the tests is now correct
  • Fixed an issue with test script getting an invalid property
  • This includes everything from the patch in comment #14

Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs change record

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

Status:Reviewed & tested by the community» Needs work

hook_user_access_alter() needs documentation in user.api.php.

Issue tags:+Needs Documentation

#18: Yup, adding tag ...

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new9.41 KB
FAILED: [[SimpleTest]]: [MySQL] 48,107 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Changes since last patch:

  • More comment cleanup.
  • Remove extra unneeded information from test.
  • Add api documentation.

StatusFileSize
new4.98 KB
PASSED: [[SimpleTest]]: [MySQL] 48,114 pass(es).
[ View ]

Last patch was weird... rerolling. Same fixes.

Change notification added:
http://drupal.org/node/1840642

+++ b/core/modules/user/user.api.php
@@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
+ * Alter user_access for a user.

Function names should be suffixed by brackets: user_access().

+++ b/core/modules/user/user.api.php
@@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
+ *   An associative array of permissions user_access validates as true.

Again, brackets.

+++ b/core/modules/user/user.api.php
@@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
+ *   The account object passed to user_access().

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

+++ b/core/modules/user/user.api.php
@@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
+function hook_user_access_alter(&$permissions, $account) {

I'm not sure if it's required yet, but I would add array type hinting for the first argument.

+++ b/core/modules/user/user.api.php
@@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
+    $permissions['user lives in santiago'] = true;

Constants are uppercase.

StatusFileSize
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 48,131 pass(es).
[ View ]

Changes since last patch:

  • Fixed: Function names should be suffixed by brackets: user_access().
  • Fixed: Again, brackets.
  • Changed:
    +++ b/core/modules/user/user.api.php
    @@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
    + *   The account object passed to user_access().
    to
    + * @param object $account
    + *   The account object to which user access permissions apply.
  • Fixed: Add array type hinting to first argument. +function hook_user_access_alter(array &$permissions, $account) {
  • Fixed: Constants are now upper case.

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

StatusFileSize
new4.99 KB
PASSED: [[SimpleTest]]: [MySQL] 48,117 pass(es).
[ View ]

Found a few minor areas of clean up since last patch.

Changes since last patch:

  • Add array type hinting to user_access_alter_test.module
  • Add brackets to user_access_alter_test.module

Why is there a change notice when this patch hasn't been committed yet? We (generally) don't do that.

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

+++ b/core/modules/user/tests/user_access_alter_test/user_access_alter_test.moduleundefined
@@ -0,0 +1,18 @@
+}
\ No newline at end of file
diff --git a/core/modules/user/tests/user_form_test.info b/core/modules/user/tests/user_form_test/user_form_test.info

According to coding standards there should be a new line (\n) at the end of file to prevent these messages.

+++ b/core/modules/user/user.api.phpundefined
@@ -195,6 +195,28 @@ function hook_user_format_name_alter(&$name, $account) {
+ * Alter user_access() for a user.
+ *
+ * Called by user_access() to allow modules to alter user_access() permissions.
+ * Can be used to modify existing user access or create alternative or additional
+ * user access schemes.

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.

StatusFileSize
new5.05 KB
FAILED: [[SimpleTest]]: [MySQL] 48,127 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

Changes since last patch:

  • Fixed: According to coding standards there should be a new line (\n) at the end of file user_access_alter_test.module to prevent this messages.
  • Fixed: This should probably say that is alter hook is only invoked once per user as the result is cached per user.

+++ b/core/modules/user/user.api.phpundefined
@@ -195,6 +195,29 @@ function hook_user_format_name_alter(&$name, $account) {
+ * user access schemes. This hook is only invoked once per user as a result of
+ * user_access() being cached.

I personally would say:

"This hook is only invoked once per user during a request as a result of user_access() being statically cached."

StatusFileSize
new5.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D8-user_access_alter-1536612-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Changes since last patch:

  • Change description on alter function to use: "This hook is only invoked once per user during a request as a result of user_access() being statically cached."

I agree, this description is more specific. Rerolled patch with wording change.

Status:Needs review» Needs work
Issue tags:-Needs Documentation, -Needs change record

The last submitted patch, D8-user_access_alter-1536612-31.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs Documentation, +Needs change record

No need for re-test, HEAD is currently broken so this will fail independently of this patch.

I'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?

This change would allow alternative permissions systems to coexist with the core permission system.
Such as potential use cases:

  1. A user relationship driven access system.
  2. An entities or views based access system. (contrib can decide if this is performant enough to make sense)
  3. A rule or condition based access system
  4. Any other systems which are not directly attributed to a user role. With user roles, you have to create a unique permutation for every level of access, where as with this change, you are no longer necessarily tied to that.
  5. Any of the above systems layered on top of or in addition to the core permission system

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.

Issue tags:+needs security review

I'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.

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

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

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

No, 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.

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

Status:Needs review» Needs work
Issue tags:+Needs Documentation, +needs security review, +Needs change record

The last submitted patch, D8-user_access_alter-1536612-31.patch, failed testing.

Issue tags:-Needs change record

We don't generally add the change notice tag until the patch is committed. :)

Status:Needs work» Postponed

Considering we are beyond freeze this should probably be either won't fix or postponed?

Status:Postponed» Closed (won't fix)

I'm going to go ahead and won't-fix, per above discussion.