Problem/Motivation

Since Drupal 9.2.0 the access checking must be explicitly specified on entity query prior to the query being executed. See the following change record for more details: https://www.drupal.org/node/3201242.

I found 2 spots in Token module, that should be handled:

  1. https://git.drupalcode.org/project/token/-/blob/8.x-1.x/token.tokens.inc...
  2. https://git.drupalcode.org/project/token/-/blob/8.x-1.x/tests/src/Functi...

Proposed resolution

Explicitly set `accessCheck` for the list entity queries.

Remaining tasks

1. Review listed usages and decide whether the access should be checked or not;
2. Submit a merge request with proposed changes;

3. Review, commit.

Issue fork token-3231436

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Matroskeen created an issue. See original summary.

Matroskeen’s picture

Assigned: Matroskeen » Unassigned
Issue summary: View changes
Status: Active » Needs review

1) I think it should be TRUE in https://git.drupalcode.org/project/token/-/blob/8.x-1.x/token.tokens.inc..., so user can see the amount of nodes he has access to (default value was correct);
2) I think it should be FALSE in https://git.drupalcode.org/project/token/-/blob/8.x-1.x/tests/src/Functi..., because the test should care whether the link exists disregarding the access.

Matroskeen’s picture

Issue tags: +LutskCodeSprint2021
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks reasonable to keep both as false

TR’s picture

Status: Reviewed & tested by the community » Needs work

@andypost: Why the RTBC if you want to make both FALSE? Right now the patch sets one to TRUE and one to FALSE.

As explained in the change record https://www.drupal.org/node/3201242, the query without the call to accessCheck() defaults to accessCheck(TRUE).

Until Drupal 9.2, if ::accessCheck() is not called then the query would default to checking access, i.e. behave as if ::accessCheck(TRUE) had been called.

So the MR and the suggestion in #5 both amount to a functional change in the token module. IMO as a "task" to address a deprecation in D9.2, the only change in scope of this issue would be to add accessCheck(TRUE) to both queries to preserve the existing behavior.

But yes, the question of what is the proper access control to use for these queries should be discussed, and if access is changed then a test should be provided to ensure that this module respects the access control settings.

andypost’s picture

Probably I was tired, thank you for review!

andregp’s picture

Assigned: Unassigned » andregp

I'll change it.

andregp’s picture

Status: Needs work » Needs review
andregp’s picture

Assigned: andregp » Unassigned

Sorry, I forgot to unassign myself

Berdir made their first commit to this issue’s fork.

  • Berdir committed fe6df40 on 8.x-1.x authored by Matroskeen
    Issue #3231436 by Matroskeen, andregp: Access checking must be...
Berdir’s picture

Status: Needs review » Fixed

Merged, thanks.

Status: Fixed » Closed (fixed)

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