Problem/Motivation
\Drupal\user\AccountForm programmatically checks entity field access for several User base field definitions. However, the 'mail' field has no such access check
Steps to reproduce
Implement a hook_entity_field_access() and forbid the mail field:
if ($items && $items->getEntity()->getEntityTypeId() === 'user' && $field_definition->getName() === 'mail' && $operation == 'edit') {
return AccessResult::forbidden();
}
Proposed resolution
Add an access check for the Accountform 'mail' field.
Remaining tasks
- Write a patch
- Review
- Commit
User interface changes
None.
API changes
None.
Data model changes
None.
Release note
The user account form now applies entity field access to the 'Email address' field in line with other user account form fields. As a result, it is now possible to forbid accessing the 'Email address' field using hook_entity_field_access().
Comments
Comment #2
idebr commentedAttached patch adds an access check for the Accountform 'mail' field.
Comment #3
idebr commentedRemoved a debug statement
Comment #7
idebr commentedThe test hook implementation that forbids editing the mail field is now enable by a state variable to allow existing tests to pass.
Comment #8
idebr commentedRemoved a duplicate newline, so the code adheres to coding standards
Comment #10
lendudeGood to get this in line with the other fields in the form. Bad DX that we only do this for some fields I feel.
Not sure if we should worry about BC. People could have really broad access checks that depend on this not being called for the mail field like
And expect the mail field to still be accessible.
Which sounds unlikely and having to make access checks more explicit doesn't sound bad either way. So gonna RTBC and let's see what others think.
Comment #11
idebr commented#2227381: Apply formatters and widgets to User base fields 'name' and 'email' strives to remove some of the form overrides in the AccountForm so these field-by-field access check will no longer be necessary.
Comment #12
xjmThanks for working on this and for providing test coverage. The fix looks correct to me.
Based on #10, I think this needs a change record and a release note. (We typically provide release notes when we fix access control rules in a way that might require contrib or custom code, or site owners, to change access configuration.)
Savin credit for @idebr for the patch and for @Lendude for review.
Thanks!
Comment #13
idebr commentedAdded a change record: https://www.drupal.org/node/3336695
Added a release note to the issue summary:
Comment #14
smustgrave commentedChange record and release notes have been added. Since this was previously RTBC and there has been no code change adding back.
Comment #16
lauriiiCommitted c490d4c and pushed to 10.1.x. Thanks!
Not backporting to 10.0.x or 9.5.x because of the potential disruption to custom code that is not taking into account that access is being checked for the mail field, but still want the mail field to be editable.