Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In conversation with @kevin.dutra on #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access it became clear that we had a different understanding of the usage of the field access interface. We are not aware of any security issues in stable code. However this confusion affects the security of patches currently under review and could easily cause security issues, hence marking as Major.
Background
The field access API includes:
- functions to query access, e.g.
\Drupal\Core\Field\FieldItemListInterface::access()and\Drupal\Core\Entity\EntityAccessControlHandlerInterface::fieldAccess() - functions to specify access, e.g.
hook_entity_field_access()
In general the parameters that affect the result are:
- the field value, $items
- the target entity (implied from $items->getEntity)
- the operation
- the user accessing
Confusion
The confusion relates to the interpretation of the $items parameter when changing a field from X to Y.
- Option A: access API is always called with $items=Y.
- Option B: access API maybe called with either X or Y, it's not defined.
Pros and Cons
Disclaimer: I have tried to present arguments from both sides, but I am in favour of option A.
1) A = Usefulness: it is a valid use case to want to grant access to a field conditional on only allowing certain values. A particularly important case is the user->roles field, to allow a "sub-admin" to assign "safe" roles but not to make themselves an admin. As far as I can see this scenario is not possible to implement with B.
2) A = Security: under option B, any code that uses the hook_entity_field_access() $items parameter (except to call ->getEntity) is likely to be insecure - the developer expects they have prevented a user from assigning certain values, but it's not true. This makes it a dangerously misleading API.
3) B = Security. Most of the time code will naturally use the new value (bulk operation, REST, etc). However when building a UI element, it is natural to call FieldItemListInterface::Access to determine whether to show a field.
$my_field['#access'] = $entity->my_field->access('edit');
This code passes the old value - which is the best way I can see with the existing interface to determine whether to show the field. However this code alone would not be secure under A - there also needs to be code in the form validation to the check access again with the new value.
Next steps
- Get input from @kevin.dutra and relevant experts.
- Decide between the two options.
- Change the documentation to be more precise (including when the NULL case is used - one case seems to be to control column display in views).
- Follow on actions to patches in the two related issues #2854252: User forms broken for admin without 'administer users', #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access.
- Check other code for any potential problems (obviously raising as a security issue - not here).
Comments
Comment #2
BerdirI think the reality is option B? You don't really know. And also with entity forms, while @hchonov and I had several discussions in the past on whether the entity form should be called/built with the original entity, sometimes it will be the already changed entity, e.g. with node preview, so sometimes you just can't know for certain.
Comment #3
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@Berdir Thanks for reading this issue.
That's a good question, but it seems like a slightly different one. If the entity form was written to make an access check during verification (as I suggest in "Pros and Cons (3)") then it would conform to option A no matter whether the form was built with the original or changed entity.
Are you aware of any specific code in Core that calls an access check passing the old value?
It seems that we can either:
In the second case, then I have a specific example: Administer Users by Role module 8.x-3.0-alpha1 would need to have some function removed. With B, it seems to be impossible to safely allow a user without 'administer permissions' to run user_add_role_action.
Comment #4
Berdir> then it would conform to option A no matter whether the form was built with the original or changed entity.
But building also matters for whether fields are visible or not in the first place, e.g. when doing ajax updates and rebuilding/updating parts of the form. What if fields suddenly vanished because you no longer have access to them if you change a certain value?
Comment #5
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedFor example it could be field F which is checkboxes: X, Y, Z as the values. Module M has determined that the user U in question doesn't have access to Z. Good point - what options do we have?
Option B
Under this option, field access checks are not allowed to depend on the field value. In the access check, module M must deny U access to F whatever the value of $items, and the field doesn't appear at all. Unfortunately this also prevents U from setting F to Y using views bulk/REST/actions etc.
Option A
Under this option, the form checks access on build and on validate. You make a good point, there is a risk that the field F will disappear if user U ticks box Z and then the access check is re-run with AJAX.
Module M could prevent this by hooking the form and removing access to value Z - and Administer Users by Role does this for the roles field. However I agree that won't work if a custom module creates a different form to edit users, and it won't work for an integer value where legal values are '<=100'. It would be secure, just poor UX.
Option C?
Extend the API so that there is a mechanism to ask for access to display a field for editing - this can be used when building forms. Access for the specific value must be check in validate. We could use it for the user form and anywhere else in core.
This seems like the right design, but it would near care with BC. If contrib module M1 is overriding access of an entity created in contrib Module M2, M1 would have to check the M2 form does use this mechanism before granting access based on the value.
PS I am still interested to know "Are you aware of any specific code in Core that calls an access check passing the old value?"
Why continue exploring options?
Because option B loses function and leaves the interface with an $items parameter that leads to security bugs if developers use it. Maybe we have to live with that, but I feel it's worth at least trying.
Comment #6
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI did some more exploration of core and hit upon a good search expression. It turns out that there are several examples of code assuming option B, including
I did not find examples of code assuming option A.
So it looks like it is option B. Thanks @Berdir for pointing me in the right direction.
Anyone else got anything to add?
If not, proposed next steps:
Comment #17
Amber Himes MatzWe triaged this in #documentation and discovered that #2958744: hook_entity_field_access() should explain when $items is not given appears to have addressed the latest updated concerns (in #6). The last concern relates to a contributed module (https://www.drupal.org/project/administerusersbyrole) and should go in that module's issue queue if it's still a problem.
Closing as outdated. Thank you everyone who participated in this issue and for the original report.