Problem/Motivation

Given the need of the Node Access Rebuild Progressive module existing, you might not want any users with access to content editing all node content (e.g. normal mid-level content users), to be able to trigger a rebuild and potentially take the site content offline or in inconsistent state whilst its rebuilding.

So would rather have that left for a user with a higher administrative or more specific permission.

Proposed resolution

Restrict access to users with a more granular rebuild node access permissions permission rather than administer nodes.

In the user interface it is called "Rebuild content access permissions".

Remaining tasks

Provide issue fork/patch.

User interface changes

N/A

Release notes snippet

A new rebuild node access permissions permission has now been introduced to handle who may trigger site rebuilds.

Issue fork drupal-3418353

Command icon 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

codebymikey created an issue. See original summary.

codebymikey’s picture

Issue summary: View changes
StatusFileSize
new1.43 KB
codebymikey’s picture

Attached a patch supporting Drupal 10.0.x.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

codebymikey changed the visibility of the branch 3418353-restrict-who-can to hidden.

codebymikey’s picture

acbramley’s picture

Status: Active » Needs work
Issue tags: +Needs reroll, +Needs upgrade path

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.

quadrexdev made their first commit to this issue’s fork.

quadrexdev’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -Needs upgrade path

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

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

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.

quadrexdev’s picture

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

acbramley’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

CR added, I'm not sure if we also need a presave hook here for exported config in modules/profiles

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

All feedback for this one appears to be addressed!

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

acbramley’s picture

Title: Restrict who can "Rebuild permissions" to users with the "administer site configuration" permission » Add new permission for rebuild permissions form
Issue summary: View changes

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

acbramley’s picture

Added the presave hook but debugging locally and it doesn't fire during the post update...

acbramley’s picture

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 without rebuild 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?

catch’s picture

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.

acbramley’s picture

Status: Needs work » Needs review

Perfect, in that case I've reverted all the presave code.

ressa’s picture

Issue summary: View changes

Working on Setting up an environment to test merge requests 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?

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Updated the CR for 11.3

Applied one suggestion to the MR as I saw recently there's a push to not have assertion messages.

Everything else seems fine. Update hook runs without issue.

benjifisher’s picture

I did not notice this issue before today, or I would have commented earlier.

I am not sure that we should add a new permission for rebuilding permissions.

First, the administer nodes permission is already marked as restricted. It should not be granted to "normal mid-level content users" (to use the phrase from the issue summary). Instead of introducing a new, restricted permission I think we should encourage site owners to be more careful about granting the existing one.

Along with https://www.drupal.org/sa-core-2025-002, I released the Granular Node Permissions module. That provides permissions suitable for less-privileged roles, so that common tasks can be done without the administer nodes permission. There is also an issue to add one of those permissions to core: #214190: Add administer node published status permission. The goals of the contrib module and the core issue are the same: make it easier to get work done without the restricted permission, so that site owners can be more stingy about granting it.

smustgrave’s picture

Should we put back into review ?

codebymikey’s picture

I think having a granular permission makes sense in this case. Mainly for the reason that the administer node permission is typically used in relation to regular node entity operations, and flagging that the user is allowed to make those changes to the node without needing a specific permission for the bundle or field.

My current use case is for a "site admin" role that can essentially do most of the same actions as a regular admin as far as nodes are concerned (apart from rebuilding access), such as view and edit nodes of all bundle types without needing to be explicitly granted permission every time a new one is introduced, the current administer nodes is also currently used for other functionality like views and potentially in other contrib modules, that make use of the permission, so trying to move to the contrib solution probably wouldn't work for me here.

The rebuilding node action seems like a special operation that doesn't necessarily need to be linked to administer node permission, same as the current bypass node access permission.

acbramley’s picture

I agree with @codebymikey, it seems strange to want more granularity to control specific options on a node but not to detach the rebuilding of permissions from administer nodes.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

Status: Needs work » Reviewed & tested by the community
needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems like a good rebase.

berdir’s picture

Just to add my thoughts, I think the long-term plan should be to completely drop administer nodes permission. It's been a very weird permission every since bypass node access and access content overviews was split off from it and it's just all kinds of random bits that are still using it. It's very confusing to understand what it actually does. The status permission is being split, the same should possibly be done for promote/sticky. And because it allows access to status/promote/sticky, you either need to use contrib projects or it is in fact actually given to mid-level content users.

berdir’s picture

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 09d8cb8 and pushed to 11.x. Thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • alexpott committed 09d8cb80 on 11.x
    Issue #3418353 by codebymikey, acbramley, quadrexdev, smustgrave, catch...

Status: Fixed » Closed (fixed)

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