Fields a user can't edit lose values when entity saved

Created on 7 November 2023, about 1 year ago

Problem/Motivation

When saving an entity that contains term reference fields a user can't edit, the fields lose their values when the entity is saved unless the user has the associated permission to select terms in that vocabulary. Users should not have permissions to select terms in vocabularies for fields they cannot edit.

Steps to reproduce

Here's how this is set up on the site where I noticed the issue:

I have four term reference fields on user entities where the values are set by admins. Each field contains terms from different vocabularies. Admins have the associated permission to select terms from the vocabularies, non-admin users do not. The fields are not accessible to non-admin users, access to edit those is determined in a hook_entity_field_access().

Any values that are set in those fields by admins are removed from the fields if the user updates their account.

Proposed resolution

Only enforce select permissions for fields the user can edit.

🐛 Bug report
Status

Active

Version

4.0

Component

Code

Created by

🇬🇧United Kingdom jeni_dc

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jeni_dc
  • Status changed to Postponed: needs info about 1 year ago
  • 🇩🇪Germany FeyP

    Thanks for reporting this issue. I tried to reproduce on Drupal 10.0.11 and unfortunately, I wasn't able to do so. I did the following:

    1. I created a vocabulary named "categories".
    2. I created an entity reference field "field_test" for the user entity, referencing taxonomy terms and restricted it to a vocabulary named "categories".
    3. I created two published terms in the "Categories" taxonomy, "Term 1" and "Term 2".
    4. I set up a role and gave it none of the permissions provided by Taxonomy or by Taxonomy Access Fix.
    5. I created a user with that role and set field_test to reference "Term 1" for that user.
    6. I added a hook (code below) to hide field_test for the edit operation and cleared all caches for the hook to become active.
    7. I logged in as the new user and verified that I wasn't able to see field_test on the user edit form.
    8. I saved the form.
    9. I edited the hook to apply to field_xtest instead of field_test.
    10. I looked at the form again both as the user I created and with user 1. For both users, the field was visible again on the form. With user 1 I was able to see the reference to "Term 1". For the user I created for testing, the field showed "- Restricted access - (1)".
    11. I tried this again editing various other fields on the user edit form instead of clicking just save when the hook was active, no luck.

    Here is the hook I used for testing:

    use \Drupal\Core\Access\AccessResult;
    
    function example_access_fix_entity_field_access($operation, \Drupal\Core\Field\FieldDefinitionInterface $field_definition, \Drupal\Core\Session\AccountInterface $account, \Drupal\Core\Field\FieldItemListInterface $items = NULL) {
      if ($field_definition
        ->getName() == 'field_test' && $operation == 'edit') {
        return AccessResult::forbidden();
      }
      return AccessResult::neutral();
    }
    

    I'd appreciate it, if you could provide more detailed info on how to reproduce the issue. Do you use a different version of core? I didn't yet have a chance to test on 9.5 or 10.1. Can you share your hook, maybe there is something different that triggers the issue? Anything else I might be missing? Any other contributed modules/custom code you use that might be candidates needed to trigger this?

  • 🇬🇧United Kingdom jeni_dc

    Thanks, I've been able to reproduce this on a fresh D10 (10.1.6) install with standard profile. Here's the steps I took to reproduce it:

    1. Install Drupal.
    2. Add non-admin user.
    3. Add vocabulary.
    4. Add three terms to the vocab.
    5. Add term reference field to user accounts. This field only allows references to the vocab set up in step 3. The field allows unlimited values and uses check boxes/radio buttons on the form.
    6. Disallow access to the field for non-admins in a hook_entity_field_access(). Code to follow.
    7. Add and enable Taxonomy Access Fix 4.0.1.
    8. As an admin user, set two values on the field for the non-admin user that was added.
    9. Load the user edit form as the non-admin user. I was already on that page logged in as that user so I refreshed the page. The term reference field is not available to edit for the non-admin user.
    10. Save the user edit form as the non-admin user.
    11. Refresh the user edit form as the admin user and see the values set in the field are now gone.

    If I then give the authenticated user role the TAF permission to select terms in that vocabulary, with access still denied to that field, and then edit the account as the non-admin user, the values in the field persist.

    Here's the code used to deny access to the field. I did this in a taf_test module that only contains the hook_entity_field_access and one custom permission since that's how my sites are set up where I've seen this happen. Here's the contents of taf_test.module:

    use Drupal\Core\Access\AccessResult;
    use Drupal\Core\Field\FieldDefinitionInterface;
    use Drupal\Core\Field\FieldItemListInterface;
    use Drupal\Core\Session\AccountInterface;
    
    /**
     * Implements hook_entity_field_access().
     */
    function taf_test_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
      if ($field_definition->getTargetEntityTypeId() == 'user' && $operation == 'edit' && $field_definition->getName() == 'field_term_ref' && !$account->hasPermission('edit taf_test fields')) {
        return AccessResult::forbidden();
      }
      return AccessResult::neutral();
    }
    
    

    On the sites where this is happening there are four term reference fields on the user. Three allow multiple values, one does not. The multiple value fields all use check boxes, the other one uses a select element. All of them don't have any field values after the user edits their own account. At first I thought that the one field that only allowed one value in a select element didn't lose it's value, but I then realised that was just my browser since when I held shift and hit refresh that field didn't have the term selected any more.

  • Status changed to Needs review about 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.4 + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    11 pass
  • 🇬🇧United Kingdom jeni_dc

    Here's a quick patch that checks if the current user has access to edit term reference fields. If they do not it just returns the options from the original TermSelection class.

    With this in place and the setup described in #3 when the non-admin user edits their account the values persist.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.4 + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    11 pass
  • 🇬🇧United Kingdom jeni_dc

    D'oh, I realised as soon as I posted that we can't just return the options from the parent like that, the rest of the fields won't be taken into account as soon as one comes up that the current user can't edit. Here's a new patch that overrides $options only if the user cannot edit the field.

  • 🇩🇪Germany FeyP

    Looked at this again after way too long, sorry for that.

    I started by writing an automated test based on #3. Some things I noticed on the way:

    1. You don't need to implement hook_entity_field_access() for this issue. The issue happens as soon as you enable Taxonomy Access Fix as long as the non-admin user doesn't have permission to select terms from the vocabulary. It makes no difference at all whether the hook is in use or not.
    2. It happens in core as well. If Taxonomy Access Fix is disabled and one of the terms you assign as an admin user is unpublished, once the non-admin user saves their profile, the previously assigned unpublished term is gone. Again, it happens regardless of whether the hook is in use. This is not actually surprising, since our implementation is based on the implementation in core, we just check for select access instead of term status.

    I'm not yet sure what would be the best way to fix this. Since it doesn't make a difference, if the non-admin user has permission to edit the field, I guess the approach used in the patch is not yet the final fix.

  • 🇩🇪Germany FeyP

    Okay, so the MR has the automated test, which demonstrates the core issue, not the TAF issue. In the pipeline for previous major (D10) you'll get this:

    Drupal\Tests\taxonomy_access_fix\Functional\TermReferenceFieldAccessTest::testTermReferenceFieldAccess
    Field values as expected for authenticated user
    Failed asserting that two arrays are identical.
    --- Expected
    +++ Actual
    @@ @@
         0 => Array &1 (
             'target_id' => '1'
         )
    +    1 => Array &2 (
    +        'target_id' => '2'
    +    )
     )
    

    To test for the TAF issue instead, uncomment the module in the $modules property to install TAF as well and both terms will be gone. Optionally set the status of the second term to published, but it won't make a difference. Only the permission to assign to fix the issue will be different. Uncomment the select (any) terms permission for the authenticated user and the test will pass.

    Setting to needs work for filing a core issue and then either postpone on that one and wait for core or find a good fix in TAF.

Production build 0.71.5 2024