Permission based access checks should return neutral if a permission is not granted

[pfrenssen]

Fixes #691

Very happy with how this turned out.

It is remarkable how this small change cleans up our access code. By leveraging the core functionality we no longer need to handle the access checks and merging of cacheability metadata ourselves and the end result is very lean and readable code.

This is building on top of #684 so marking this as a draft until that PR is in.

[claudiu-cristea]

Opening the access check to 3rd party is a good decision. However, the proposal here aligns with the entity access check, where the a result of neutral reads as "allow access". I'm just curious why you chose entity over route access. Drupal core handles route access different than entity access. On routes, only an allowed result reads as "allow access", neutral means deny.

Relevant:

• https://www.drupal.org/docs/8/api/routing-system/access-checking-on-routes/route-access-checking-basics#s-multiple-access-checks
• \Drupal\Core\Access\AccessManager

[pfrenssen]

The reason for this is that we are indeed doing entity access checks. This code can also be called for route access but then the caller will need to translate a neutral result to forbidden. I haven't given much thought yet to route access. I see that indeed in AccessManager::checkRequest() the andIf() access merging strategy is used, while EntityAccessControlHandler uses orIf().

The code path that is being followed to reach these methods is: $entity->access($operation) -> EntityAccessControlHandler::check() -> og_entity_access() hook implementation -> OgAccess::userAccessEntityOperation()

So I am confident that this is using the right approach for this use case, but I need to research how route access checking relates to entity access checking. From a first glance they seem to be completely independent implementations. We might need to support this also in OG.

[pfrenssen]

Assigning to me, I will update the documentation for the affected methods to make it clear which access result will be returned under which conditions.

[MPParsley]

Glad to see this cleaned up some of the access code.

Just one question here: I know we're in alpha but changing the default from deny to neutral might break for some users, shouldn't we release this in a new major version (or document this)?

[amitaibu]

IMO A new alpha would be the right move.

[pfrenssen]

We are still in alpha and as such it is OK to make B/C breaking changes. We are also very clear about this, it is mentioned on the project page, and in the sticky post with the 8.0 roadmap.

[claudiu-cristea]

Nice work. I've anticipated somehow the reply to my question #692 (review), but I wanted to make sure we're doing right.

[pfrenssen]

Thanks! Let's finish up this track so we can start preparing the next release.