Permissions By Term usurps normal node access

Created on 5 February 2024, 5 months ago

While looking at whether we can safely upgrade Permissions By Term from 3.1.22, I noticed that the canUserAccessByNode method in src/Service/AccessCheck.php implements a bunch of checks that aren't core to the module's functionality (and make no provision for other access control a user might have in place). The normal Drupal way of doing things is to only worry about your module's part of the picture, and leave core to call other modules that implement the other aspects. I therefore initially wanted to assert that canUserAccessByNode should be cleaned up to remove the non-PBT focused code.

Going on from there, I found that the method is called by src/Listener/KernelEventListener and src/Service/AccessStorage.php, as well as the unused handleNode and dispatchDeniedEventOnRestricedAccess methods later in the same class. In AccessStorage, it is found in computePermittedTids, which is invoked from getGids, which also appears to be unused.

This leaves the only actual caller as the KernelEventListener class. It implements onKernelRequest, which runs very early in the handling of a request. I think this is the root of our problem: PBT should be hooking into the normal entity access system, not usurping it. It could then return the normal ALLOWED, FORBIDDEN or NEUTRAL responses (together with approrpriate cache tags) and work with other modules, not needing much of the code in canUserAccessByNode.

If I'm not missing something that invalidates my argument, we could start coming up with a patch to correct this.

πŸ› Bug report
Status

Active

Version

3.1

Component

Code

Created by

πŸ‡¦πŸ‡ΊAustralia Nigel Cunningham Geelong

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

Comments & Activities

Production build 0.69.0 2024