Problem/Motivation
Now that we have a host access handler (#3464498: Add a host access handler) 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 #3464704: Refine semantic of 'administer own $type registration' permission.
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
Lets defer create permissions to #3494658: Add 'create * host permissions'
Remaining tasks
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork registration-3479435
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
jonathanshawComment #3
jonathanshawLet's handle 'create host ... registration' permissions seperately.
Comment #5
jonathanshawComment #6
jonathanshawThe 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.
Comment #7
john.oltman commentedSetting to needs work while we sort out my MR review comment.
Comment #8
jonathanshawAgreed. 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
Whether we should check the 'administer registrations' operation inside other host operations (or the 'administer' operation when checking other registration operations), rather than checking these permissions using a helper method instead of an operation, I'm not sure about. The downside of not using the operation (in both host and registration handler) is that if you customise a hook to give a user access to the 'administer' operation, it doesn't give them access to the 'view', 'update' and 'delete' operations automatically which you might expect, based on how the semantic of 'administer' is usually understood in Drupal access. Perhaps in #3464137: Make it possible to customise who can administer registrations we should have called the operation (on both registration and host) 'override settings' as that is what it actually is about.
I will wait to implement anything (in this issue or another) until I hear your thoughts.
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 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.
Comment #9
john.oltman commentedAgreed, I prefer this as well.
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.
Comment #10
jonathanshawComment #11
jonathanshawNote to self: I'm having doubt about the name "administer host registration". Need to consider naming carefully.
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.
Comment #12
john.oltman commentedGiven that #3493421: Remove "administer own $type_id registration settings" & "manage own $type_id registration" is postponed indefinitely (probably until 4.0), I believe the only issue stopping completion of this is the naming of the operations. I think it is settled that these are not per-bundle permissions, in keeping with the other "host" permissions.
Comment #13
jonathanshawI think I need to create some follow-ups for some of the issues in #8 as well.
Comment #14
jonathanshawI think this is best postponed on #3500492: Check the administer operation when checking other operations
Comment #15
jonathanshawComment #16
jonathanshaw