Make it possible to customise who can administer registrations

I think this is a simple and straightforward win.

For example, if using the group module it allows group admins to override a capacity setting on a registration on a group, if a hook_ENTITY_access() implementation allows the 'administer' operation for users with a certain group permission for the group that is the registration host.

I'm on board conceptually. The MR is failing tests, and somewhere you will need to implement the "administer" access such that it translates into checking the various administer permissions - I do not see that logic anywhere - it seems to fall through to a case where it checks the administer permissions, plus permissions that do not exist ("administer any xxx").

somewhere you will need to implement the "administer" access such that it translates into checking the various administer permissions - I do not see that logic anywhere - it seems to fall through to a case where it checks the administer permissions, plus permissions that do not exist ("administer any xxx").

I think it falls through to RegistrationAccessControlHAndler::checkEntityUserPermissions(), which will end up checking the existing permissions:
administer registration
administer host registration
administer own {$entity->bundle()} registration

and the non-existent but permissions:
administer own registration
administer {$registration->bundle()} registration",
administer any registration",
administer any {$registration->bundle()} registration"

The problem is, there's a bug in RegistrationAccessControlHandler::checkEntityUserPermissions(). Currently it has:

    if ($account->id() && ($account->id() == $entity->getUserId())) {
      $own_result = AccessResult::allowedIfHasPermissions($account, [
        "administer own {$entity->bundle()} registration",
        "$operation own registration",

But in the case of the 'administer own' permission the meaning is different from that of the 'view own' etc; the own refers to the host not the registration.

As the permissions explains:

      "administer own $type_id registration" => [
        'title' => $this->t('%type_name: Administer own registrations', $type_params),
        'description' => $this->t('View, edit and delete own registrations of this type. Manage registrations and registration settings of this type for host entities to which a user has edit access.'),
      ],
      "manage own $type_id registration" => [
        'title' => $this->t('%type_name: Manage registrations for editable entities', $type_params),
        'description' => $this->t('Manage registrations of this type for host entities to which a user has edit access.'),
      ],

      "view own $type_id registration" => [
        'title' => $this->t('%type_name: View own registrations', $type_params),
      ],

view host registration:
  title: 'View host registrations'
  description: 'View all registrations for host entities to which a user has edit access, regardless of type. See this <a href="https://www.drupal.org/node/3440834">change record</a> for information about this permission.'
view own registration:
  title: 'View own registrations'
  description: 'View own registrations, regardless of type.'

I think the solution might be to use 📌 Add a host access handler Needs review to pass some of this work off.

I opened 🐛 RegistrationAccessControlHandler checks 'administer own $type registration' incorrectly Active for discussion of the existing bug with "administer own $type registration".

The key change that's being made here is:

       -   $admin = $this->currentUser()->hasPermissions(["administer registration") || $this->currentUser()->hasPermission("administer $type registration");
       +  $admin = $registration->access('administer', $this->currentUser());

So currently only the 'administer registration' and 'administer $type registration' permissions are considered.

I think:
1. For consistency, $registration->access('administer') should check also "administer own $type registration" (which it does in the current MR, but in a buggy way). This would be a behavior change.
2. I'm not sure about "manage $type registration". Its semantics are undocumented. Adding it to the 'administer' operation would also lead to a behavior change.
3. I have wondered whether there should be an 'administer host registration' permission added here and allowing the 'administer' operation too.

I'm tempted to think this should be postponed on 📌 Add a host access handler Needs review and that RegistrationAccessControlHandler's logic for the 'administer' operation should include:
$result->orIf($registration->getHostEntity()->access('administer_registrations');

I think this can be committed as is. It's a simple win.

The only behavior change with the MR as it currently stands is that users with the "administer own {$entity->bundle()} registration" will become able to do certain administery kind of things on their own registrations that they currently can't. This increases the consistency in the meaning of 'administer' between the own/any contexts.

This looks good, I will try it out locally within the next couple days and will likely merge it. Thanks @jonathanshaw!

@jonathanshaw after looking at this again I think this would take things down the wrong path because 'administer' should remain a permission and not be overloaded into an operation as well - that will only confuse other developers. Also Drupal core and all the contribs I checked that deal with entities have "administer" permissions without an "administer" operation - this makes sense because operations are individual actions you can take against an entity and often have the same in the UI - the entity operation buttons View Edit and Delete for example - having a button labeled "Administer" doesn't make sense because administer can mean many things and implies permission to perform many different types of operations.

For your use case, you can implement hook_entity_access and if the entity is a RegistrationSettings instance and the operation is "update", you can return AccessResult:allowed if the current user is a group admin for the host entity associated with those settings. If you want to make this available to all users of the registration module perhaps a registration_group submodule with this functionality is appropriate.

The only behavior change with the MR as it currently stands is that users with the "administer own {$entity->bundle()} registration" will become able to do certain administery kind of things on their own registrations that they currently can't. This increases the consistency in the meaning of 'administer' between the own/any contexts.

If you can solve the above without a new operation named "administer", feel free to reopen this with a new MR.

BTW I like the work in the MR related to administering overrides and the wait list update etc. One idea is to create a new issue "Create an AccessControl service" that would centralize access check logic. It would be called by the various access control handlers (including the new one mentioned in https://www.drupal.org/project/registration/issues/3464498 📌 Add a host access handler Needs review ) and would take an optional entity parameter (Registration or RegistrationSettings). Then reopen this issue and call the new service passing "administer" as the requested access. In this way we sort of dance around the "administer is not an operation" problem. The access levels that could be passed to the service would correspond more to permissions than to operations - perhaps "administer", "manage", "edit", "view" and "delete". Something to ponder.

It's true that core, and where I've found in contrib, don't use 'administer' as an operation name. But I don't understand the benefit of your proposed solution, so I will try to set out here clearly how I'm seeing it and hope you can see what I'm not seeing.

Let's not hardcode permissions
It seems to me that hardcoding permission names when doing access checks in forms, route controllers, validators etc is a bit of an anti-pattern in an ecosystem contrib module like registration, as it makes it hard for developers to customise the access. So we should do something to fix the places I identified here where administer permission are currently hard-coded.

How to not hardcode permissions
We have to allow code that wants to check access to call out to some centralised place, which then checks certain permissions by default but also allows for customisation. The normal drupal pattern is $entity->access($op) backed up by the access handler, so the access handler receives the $entity and the $op. The operation is kind of like a simplifed explanation to the access handler about what access is being sought.

Using a service instead of the handler>/strong>
As you suggest we could also use a service for our centralised extensible access logic. As you point out we would still have to pass it the entity and something like an operation, a simplifed explanation about what access is being sought:

call the new service passing "administer" as the requested access. ... The access levels that could be passed to the service ... perhaps "administer", "manage", "edit", "view" and "delete".

I don't understand the advantage of using this design pattern in addition to the normal access control handler. It seems to have basically the same inputs and basically the same purpose. The added complication/confusion would seem to outweight any benefit.

Could we just use the access handler with an operation
Core has many operations: view, view label, update, delete, revert, delete revision, edit, approve, download, use, disable, view all revisions, revert revision, publish.
So adding an additional operation of our own to describe a specific access scenario seems like a standard Drupal pattern.

What operation or operations?
I agree administer may be a bad choice. With permissions it usually means "can do absolutely anything", it has a kind of catch-all quality, whereas maybe operations are intrinsically specific.

The 3 places where we hardcode administer permissions that we are trying to fix have a lot in common: they are all places where some normal business-logic constraint on what can be done with a registration is being overridden. They are:
- the validator
- the overrides checker
- whether or not to show the status field on the registration form

I think probably these have enough in common to be treated as a single operation.
Possible names would include: 'override', 'bypass constraints' or 'manage'.

My conclusion
Add an additional operation to the existing access control handler, so we don't have hard-coded administer permissions, but name it something better than 'administer'.

Reopening, I forgot about 'view label' which is a "pseudo-operation" similar to what you are proposing - so there is a precedent. You won't find a button named "View Label" anywhere but they added the operation to solve a use case. We can do the same.

For the name, we may eventually need a separate "manage" operation someday, so let's stick with "administer" here.

I left a comment on the MR - would feel more comfortable if the "administer" operation was handled more explicitly. Also need tests. Thanks Jonathan!

Addressed feedback.

If 📌 Add a host access handler Needs review is committed first this would need updating, or vice versa.

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