- Issue created by @jonathanshaw
- π¬π§United Kingdom jonathanshaw Stroud, UK
Let's handle 'create host ... registration' permissions seperately.
- Merge request !81#3479435 Add manage & administer host permissions β (Open) created by jonathanshaw
- π¬π§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
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.