Check the administer operation when checking other registration operations

I'm OK with A, but I think B is slightly preferrable as it provides more granular extensibility by the access hooks. With B, an access hook could for example be neutral about deletion while allowing overriding settings, rather than having to allow both when allowing administering.

I prefer A, so as to not to introduce even more names of things. When developers see a check for "administer" they know instantly what that means and they can move on mentally. The name "override settings" is amorphous. In the highly unlikely event the granularity is not sufficient, a developer can override the access handler itself or take any number of other approaches.

One thing to add to this issue - I notice in some places in the RegistrationAccessControlHandler (I did this in the edit state commit) we orIf a new result. In other places, we replace a result, and thereby lose the cacheability of some earlier result. I wonder if we should use orIf in all places. In practical terms, I am guessing the cacheability will not change for our use cases, but as people add extensions, the possibility some cacheability is lost increases.

Example code I am thinking of, lines 69-76:

$result = AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');

if ($result->isNeutral()) {
  $result = $this->checkEntityUserPermissions($entity, $operation, $account);
  // All of these checks depend on the registration type, host or
  // registrant.
  $result->addCacheableDependency($entity);
}

Maybe it should become this:

$result = AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');

if ($result->isNeutral()) {
  $entity_result = $this->checkEntityUserPermissions($entity, $operation, $account);
  // All of these checks depend on the registration type, host or
  // registrant.
  $entity_result->addCacheableDependency($entity);
  $result = $result->orIf($entity_result);
}

$result = AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');
if ($result->isNeutral()) 
  $result = $this->checkEntityUserPermissions($entity, $operation, $account);{

I get what you mean.

Ah, yes. A child access control handler extending this one could modify checkEntityUserPermissions and not necessarily use AccessResult::allowedIfHasPermissions().

Ready for review. I implemented option A, with some cacheability fixes in there as well.

Bump

Sorry @jonathan I think the order in which your comments are appearing in the MR is throwing me for a loop. Can you clarify. I believe the gist of what you are looking for is to check "real" operations first, and if they return neutral, then checking administer. But it hard for me to tell.

This is ready for review again

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