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

  1. Write a patch
  2. Review
  3. 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

idebr created an issue. See original summary.

idebr’s picture

Status: Active » Needs review
StatusFileSize
new1.55 KB
new2.32 KB

Attached patch adds an access check for the Accountform 'mail' field.

idebr’s picture

StatusFileSize
new1.49 KB
new2.25 KB
new632 bytes

Removed a debug statement

The last submitted patch, 2: 3331947-2-test-only.patch, failed testing. View results

The last submitted patch, 2: 3331947-2.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 3331947-3.patch, failed testing. View results

idebr’s picture

Status: Needs work » Needs review
StatusFileSize
new1.61 KB
new1.64 KB
new2.41 KB

The test hook implementation that forbids editing the mail field is now enable by a state variable to allow existing tests to pass.

idebr’s picture

StatusFileSize
new727 bytes
new1.64 KB
new2.41 KB

Removed a duplicate newline, so the code adheres to coding standards

The last submitted patch, 8: 3331947-8-test-only.patch, failed testing. View results

lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DX (Developer Experience)

Good 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

if ($operation === 'edit' && $items && $items->getEntity()->getEntityTypeId() === 'user') {
  return AccessResult::forbidden();
}

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.

idebr’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs release note

Thanks 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!

idebr’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs release note

Added a change record: https://www.drupal.org/node/3336695

Added a release note to the issue summary:

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Change record and release notes have been added. Since this was previously RTBC and there has been no code change adding back.

  • lauriii committed c490d4ca on 10.1.x
    Issue #3331947 by idebr, Lendude, xjm: Accountform is missing an entity...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.