Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Walt Haas’s picture

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.

Xano’s picture

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

westwesterson’s picture

Status: Active » Needs review
FileSize
109.55 KB

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.

westwesterson’s picture

FileSize
633 bytes

Same patch; less junk :)

westwesterson’s picture

FileSize
514 bytes

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.

westwesterson’s picture

Status: Needs work » Needs review
FileSize
414 bytes

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

westwesterson’s picture

changing back name of issue.

westwesterson’s picture

Title: hook_user_access_alter » Add 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.

univate’s picture

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.

westwesterson’s picture

Good catch!
Should have been up one line.

Fabianx’s picture

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.

westwesterson’s picture

Status: Needs work » Needs review
FileSize
8.39 KB

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.
westwesterson’s picture

ignore this comment, see #16

westwesterson’s picture

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
Fabianx’s picture

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.

Xano’s picture

Status: Reviewed & tested by the community » Needs work

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

Fabianx’s picture

Issue tags: +Needs documentation

#18: Yup, adding tag ...

westwesterson’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
9.41 KB

Changes since last patch:

  • More comment cleanup.
  • Remove extra unneeded information from test.
  • Add api documentation.
westwesterson’s picture

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

westwesterson’s picture

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

Xano’s picture

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

westwesterson’s picture

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

westwesterson’s picture

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
Crell’s picture

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

westwesterson’s picture

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

Fabianx’s picture

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

westwesterson’s picture

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.
Fabianx’s picture

+++ 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."

westwesterson’s picture

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.

westwesterson’s picture

Status: Needs work » Needs review
Issue tags: +Needs documentation, +Needs change record
Fabianx’s picture

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

Crell’s picture

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?

westwesterson’s picture

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.

Fabianx’s picture

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

Crell’s picture

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.

westwesterson’s picture

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

Crell’s picture

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.

Grayside’s picture

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.

rajasekar33’s picture

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.

xjm’s picture

Issue tags: -Needs change record

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

meba’s picture

Status: Needs work » Postponed

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

Crell’s picture

Status: Postponed » Closed (won't fix)

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

Vergil.K’s picture

Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes
Status: Closed (won't fix) » Active

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

Crell’s picture

Status: Active » Closed (won't fix)

Please don't reopen long-dead issues.