See parent issue #3200985: [meta] Fix undesirable access checking on entity query usages for context and test coverage policy.

Drupal provides validation to enforce that the value of a field is unique among all entities of the entity type. This validation operates by querying existing entities to see if any of them already have the value that needs to be unique. This querying should find all entities, including ones that are not accessible to the current user.

Fixes needed:
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php validate
core/modules/migrate/src/Plugin/migrate/process/MakeUniqueEntityField.php exists

This is a critical because it may compromise data integrity.

Issue fork drupal-3202125

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw opened merge request !398

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Active » Needs review
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Issue summary: View changes
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

Kernel test coverage for the migrate bug would be obviously desirable, but there currently is none for this porcess plugin, I can't provide it, and I'm not sure it's important enough to hold up the wider issue tree.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Related issues: +#3171047: Entity query in UniqueFieldValueValidator must not use access checking

Let's make #3171047: Entity query in UniqueFieldValueValidator must not use access checking into a follow-up.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

Berdir pointed out in #3171047: Entity query in UniqueFieldValueValidator must not use access checking:

By fixing the bug, we might break the ability to save these then on existing sites, and depending on how it is used, it might or might not be easy to fix it. Unique-ness might be validated on a field that can't be changed afterwards?

In order to unblock the parent issue, I suggest we make the bug explicit by adding accessCheck(TRUE) and a @todo linking to the other issue for changing it to FALSE.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

So to recap, there's 2 bugs here:
- core/modules/migrate/src/Plugin/migrate/process/MakeUniqueEntityField.php exists is fixable and also involves a simple tweak to its unit tests
- core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php is hard for the reasons discussed in #3171047: Entity query in UniqueFieldValueValidator must not use access checking and I'm suggesting we add an explicit accessCheck to unblock the parent issue, but make it TRUE to preserve the buggy existing behavior for now.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Priority: Critical » Major

If we're not actually working on the main bug here, just the unblocking, then it's only major.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Hm. I'm not sure if I'm saying it is a hard blocker. Just that we need to consider the implications and decide if that is acceptable, which might need a release/framework manager decision or so? Wasn't very clear, sorry.

One idea would be to add a setting to be able to control it if someone wants that behavior for some weird reason. Combined with a release note/change record that says, we now no longer do access checks, which might result in some edge cases now. If you for some reason want to keep the existing behavior, change it to ->addValidationConstraint('UniqueThing', ['access_check' => TRUE]) (not sure if that's the exact syntax, but you get the idea).

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I think adding accessCheck(TRUE), then opening a critical bug to fix it properly, makes sense to unblock the parent issue.

I cant really see a way that the current behaviour would be relied upon consciously, but I can definitely see us exposing some data integrity issue by fixing it.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

I've made #3171047: Entity query in UniqueFieldValueValidator must not use access checking the critical fix. The MR here is ready for review on the lines of #8 & #11.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

RTBC based on #11, in the difficult case this is just confirming the existing behaviour and leaving any possible change to it to the followup.

The migrate process plugin is more straightforward and this just shouldn't check access at all.

  • catch committed 96aa5ca on 9.2.x 
    Issue #3202125 by jonathanshaw, Berdir, catch, longwave: EntityQuery...

  • catch committed 88e3ff9 on 9.1.x 
    Issue #3202125 by jonathanshaw, Berdir, catch, longwave: EntityQuery...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Status: Fixed » Closed (fixed)

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