- Issue created by @codebymikey
codebymikey β changed the visibility of the branch 3418353-restrict-who-can to hidden.
- Merge request !8251Issue #3418353: Restrict who can "Rebuild permissions" to users with a new "rebuild node access permissions" permission β (Open) created by codebymikey
- Status changed to Needs work
2 months ago 12:22am 31 March 2025 - π¦πΊAustralia acbramley
This would need an upgrade path to add the new permission to any role that has administer nodes so there's no access regression for existing sites. Also needs a reroll.
- First commit to issue fork.
- πΊπ¦Ukraine quadrexdev Lutsk
Updated the issue branch with changes from 11.x, added post_update hook to grant the new permission
The errors in the pipeline look to be unrelated to this issue, so I would assume MR could be reviewed now
- πΊπΈUnited States smustgrave
I'm 50/50 that we should add test coverage for the update hook. We add coverage for most update hooks but this one is pretty simple.
But moving to NW as this would be a new permission to be declared.
- πΊπ¦Ukraine quadrexdev Lutsk
Thanks for the feedback, @smustgrave
I added a simple test to verify that the new permission is added by the post update hook, just in case
- π¦πΊAustralia acbramley
CR added, I'm not sure if we also need a presave hook here for exported config in modules/profiles
- πΊπΈUnited States smustgrave
All feedback for this one appears to be addressed!
- π¬π§United Kingdom catch
The title doesn't match either the issue summary or the MR. Also I'm not entirely sure why that option wasn't taken - e.g. requiring both 'administer site configuration' and 'administer nodes' at the same time.
- π¦πΊAustralia acbramley
Updated title. I think a new permission is the best approach here, if we used administer site configuration we'd still need an upgrade path.
Keeping at NW because I think we do need the ConfigUpdater dance here in a presave hook
- π¦πΊAustralia acbramley
Added the presave hook but debugging locally and it doesn't fire during the post update...
- π¦πΊAustralia acbramley
After all that I'm actually unsure how we should do this. With this presave hook it's impossible now to save a role with
administer nodes
and withoutrebuild node access permissions
...Throwing a deprecation didn't feel right either because it's a valid case to have one permission without the other, but we need to be able to notify modules with exported config.
@catch any ideas?
- π¬π§United Kingdom catch
I don't really count roles as exported config to be shipped with modules to be honest, it's pretty rare in general. So in the case I think it's fine to skip the presave. It's not like we're renaming a permission, exported roles will be validz they just might not cover this permission.
- π¦πΊAustralia acbramley
Perfect, in that case I've reverted all the presave code.
- π©π°Denmark ressa Copenhagen
Working on https://www.drupal.org/community/contributor-guide/reference-information... β I tried the patch here, which is used as an example.
I then looked for "Rebuild node access permissions" in the user interface at
/admin/people/permissions
, but then realized that it is called "Rebuild content access permissions" ...Should it perhaps be one or the other both places, for consistency?