Create additional host permissions

Created on 8 October 2024, 2 months ago

Problem/Motivation

Now that we have a host access handler (#[3464498]) it makes sense to complete the pattern in which access can be made to flow down from the host.

This also is a useful step towards removing the 'administer own' and 'manage own' permissions with their confusions in πŸ› RegistrationAccessControlHandler checks 'administer own $type registration' incorrectly Active .

Currently we have:
view host registration
update host registration
delete host registration

Proposed resolution

It would make sense to add:
manage host registration
administer host registration

I'd like to add
create self host registration
create other users host registration
create anonymous host registration

'create self host registration' might seem strange as these are staff-type permissions, but I suspect that without it staff wouldn't be able to register themselves even if they could register others, which would be odd.

For these create permissions we will have to be cautious of πŸ› EntityAccessControlHandler::createAccess() and EntityAccessControlHandler::access() return false positive cache hits because it ignores context Needs work which makes it tricky to pass the host in the context.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

✨ Feature request
Status

Active

Version

3.3

Component

Registration Core

Created by

πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK

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

Merge Requests

Comments & Activities

  • Issue created by @jonathanshaw
  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK
  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK

    Let's handle 'create host ... registration' permissions seperately.

  • Pipeline finished with Failed
    5 days ago
    Total: 407s
    #371480
  • Pipeline finished with Failed
    5 days ago
    Total: 389s
    #371492
  • Pipeline finished with Failed
    5 days ago
    Total: 403s
    #371515
  • Pipeline finished with Failed
    4 days ago
    Total: 435s
    #371598
  • Pipeline finished with Failed
    4 days ago
    Total: 454s
    #371740
  • Pipeline finished with Failed
    4 days ago
    Total: 396s
    #371753
  • Pipeline finished with Failed
    4 days ago
    Total: 468s
    #371763
  • Pipeline finished with Success
    4 days ago
    Total: 403s
    #371774
  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK
  • Pipeline finished with Success
    4 days ago
    Total: 405s
    #371779
  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK

    The diffs look a little messy, partly because I found some semi-related things that needed tweaking, but mostly because some of the logic needed refactoring. But actually the changes here are very simple.

    I suggest checking https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... β†’ first, just in case seeing everything laid out like that raises any bigger questions.

  • πŸ‡ΊπŸ‡ΈUnited States john.oltman

    Setting to needs work while we sort out my MR review comment.

  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK

    Having two operations differ by a single letter is likely to create confusion

    Agreed. This got me thinking a bit more about operations.

    And touches upon an inconsistency in our API I realised last night.:
    - On the RegistrationHostAccessControlHandler we check $host->access('administer registrations', ... when we want to consider administer permissions when access checking operations like 'view registrations' etc. So host access hook fires twice, once with 'view registrations' and once with 'administer registrations'.
    - But in RegistrationAccessControlHandler, in very similar circumstances, we check the administer permissions directly when evaluating 'view', instead of checking $registration->('administer',...
    It's right and important that in the RegistrationHostAccessControlHandler we sometimes check $host->access('manage', which does cause a second firing of the access hook - this is an important point of extensibility. But the consistency with how we're treating administering doesn't seem quite right.

    Sitting back, considering all this, and reviewing the access docs I wrote yesterday at https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... β†’ I have these thoughts:
    - Operations should be 1:1 with the things users actually do.
    - The 'manage registrations' host operation should be removed. It's not a real operation, just a consequence of being a bit literal when creating these operations based on existing permissions. Access checking for the /{host}/registrations route should depend on the user having access to either the 'view registrations' or 'manage settings' or 'manage broadcast' host operations.
    - We should not create an 'administer registration' operation, and we should not check the 'administer registrations' operation inside other host operations. We can check these permissions using a helper method, but not an operation.

    What if the new permissions were per bundle, this would provide some separation, and has the nice side effect that they could become drop-in replacements for the soon to be retired "own" permissions.
    Downside is loss of symmetry with the existing host permissions which are not per bundle.

    I suggest not having these per bundle. I think having permissions tied to the host, and generic bundle permissions, provides a useful kind of orthogonality, and altogether I imagine it is more than sufficient for 99% of use cases. I think registration core is very generous in OOTB the permissions it provides; if starting from scratch I'm not sure we should be as generous as we are.

    if there is a migration path for the "own" permissions.

    If a user has the own permission for all registration types, we can give them the host permission. Otherwise, we have to block the module update using hook_requirements and require the sitebuilder to remove the own permissions and consider what permissions to give in light of their site's needs.

  • πŸ‡ΊπŸ‡ΈUnited States john.oltman

    I suggest not having these per bundle.

    Agreed, I prefer this as well.

    we have to block the module update using hook_requirements

    By the time this is released we could have close to 1,000 installs. Even if only 5% use an own permission varying by type, that is 50 sites that we have broken by removing functionality without a replacement. I don't want to do that. I don't have an alternative yet. Let's mull this over further.

  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK
  • πŸ‡¬πŸ‡§United Kingdom jonathanshaw Stroud, UK

    Note to self: I'm having doubt about the name "administer host registration". Need to consider naming carefully.

    By the time this is released we could have close to 1,000 installs. Even if only 5% use an own permission varying by type, that is 50 sites that we have broken by removing functionality without a replacement. I don't want to do that. I don't have an alternative yet. Let's mull this over further.

    Spitballing:
    1. Add a hook_requirements warning now, wait until a major version release to remove. We can see how many people show up in the issue queue.
    2. Keep them, mark them deprecated, add a hook_requirements warning, don't plan to do anything about it for years.
    3. Create a legacy module that provides the permissions and makes them work as before. Make it a seperate project, not a child module, so you get stats on its usage and can eventually stop maintaining it. Or even mark it as not maintained, give them the responsibility.
    4. Decide that we are not breaking anyone's site, they don't have to upgrade. Maybe make it a major version and keep backporting bugs to the previous version.
    5. Keep the permissions around, but rename them "administer $type host registration settings" and mark them deprecated.
    6. Create a "legacy" submodule that the sitebuilder enabled in an update hook if the site needs it.

    I think it would be best to have a way of thinking that enabled breaking changes to be made to the module.

Production build 0.71.5 2024