ECA Access: Do not set AccessResult in applies()

Created on 5 April 2024, about 1 year ago

Problem/Motivation

In โœจ Use case: Complex access control based on workflow state Active , we describe how we want to deny all access to certain fields in one model, and then selectively grant access to those fields only during specific workflow states. The plugin created to satisfy that use-case relies on the patch in ๐Ÿ“Œ Make it easier to extend existing plugins Active , which allows it to inherit from the FieldAccess plugin. We then add fields to the Event config form to allow us to specify in which workflow states the Event should apply.

This works quite nicely, except that any event that applies to one of these fields returns an AccessResultNeutral, even if the workflow state doesn't apply. This appears to be due to calling parent::applies() from our plugin, which eventually hits EntityAccess::applies() where we see:

// Initialize with a neutral result.
$this->accessResult = AccessResult::neutral();

As a work-around, we had to deny access unconditionally again, as the first successor to all of our events. We traced this back to the above line of code. Commenting it out resolves the issue for us.

AFAICT it is not required, since we return a neutral result as a fallback in the all implementations of hook_entity_field_access().

Proposed resolution

Do not set an AccessResult in applies(). That is, just remove the problematic line.

Remaining tasks

  1. Provide a patch against 1.1.x branch
  2. Create an MR from the 2.0.x branch
๐Ÿ› Bug report
Status

Active

Version

1.1

Component

Code

Created by

๐Ÿ‡จ๐Ÿ‡ฆCanada ergonlogic Montrรฉal, Quรฉbec ๐Ÿ‡จ๐Ÿ‡ฆ

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

Merge Requests

Comments & Activities

  • Issue created by @ergonlogic
  • Status changed to Postponed about 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada ergonlogic Montrรฉal, Quรฉbec ๐Ÿ‡จ๐Ÿ‡ฆ

    Here's a patch.

    Marking as postponed until I have a chance to create an MR against 2.0.x

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States freelock Seattle

    We hit an issue where we had multiple events that use the "Determining Entity Access" plugin, and hit this issue. After reviewing, I think the solution proposed by @ergonlogic is incorrect -- I do think the result should get initialized with AccessResult::neutral(). However, if there is more than one of these events in your site, the last one wins because of this reset.

    In our case two access checks applied to a particular action:

    node:application, update
    node:*, view,update

    The first was setting an AccessResult::allowed, the second AccessResult::neutral. After processing the ECA event, AccessResult::neutral was the final result, and access ended up being denied.

    I think the correct solution is to make use of AccessResult::orIf(). In the event plugin, setAccessResult() method, I think it should see if it already has an access result set, and if it does, replace it with $this->accessResult->orIf($result).

    This allows all the access rules to get checked, and reasons are available for all of the models.

    The logic for the orIf() call is to return AccessResult::forbidden if either the existing or new accessresult is forbidden; return AccessResult::allowed if either the existing or new result is allowed, and return AccessResult::neutral if both are neutral.

    The previous solution is still subject to the "last check wins" if multiple checks return something other than neutral.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States freelock Seattle

    Proposed fix in MR.

  • Pipeline finished with Failed
    3 months ago
    Total: 602s
    #391668
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany jurgenhaas Gottmadingen

    In ECA 2.0, the initialisation with neutral has already been built in as part of ๐Ÿ“Œ Enable event plugins to determine appliance Fixed . It's now contained in \Drupal\eca_access\Plugin\ECA\Event\AccessEvent::appliesForWildcard. The problem with that is in fact, if multiple ECA events are subscribing to the same Drupal event, then each of them initializes the access result again.

    But the proposed MR resolves that issue correctly. Thanks @freelock for that suggestion. I've just left a couple of comments that are simple to apply.

  • Pipeline finished with Success
    3 months ago
    Total: 562s
    #394643
Production build 0.71.5 2024