Problem/Motivation

Navigation Blocks page (/admin/config/user-interface/navigation-block) is the place to go to edit the navigation layout and manage the blocks to be included there.

Current NavigationSectionStorage::access() implementation just gives free access to the page. That added to logic in LayoutBuilderAccessCheck::access() ends up being just like checking for configure any layout permission.

That permission is not very restrictive nor representative for Navigation module.

Would be great to have a more representative and intuitive way to manage access to that page.

Proposed resolution

Create a new Configure Navigation Layout permission
Override NavigationSectionStorage::access() to check for that permission
Add handles_permission_check to NavigationSectionStorage class attributes to not grant access based on configure any layout permission.

Remaining tasks

Review the change record.

User interface changes

None

API changes

New permission added.

Data model changes

Release notes snippet

Issue fork drupal-3447881

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:

About issue forks

Comments

plopesc created an issue. See original summary.

plopesc opened merge request !8140

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue summary: View changes
Status: Active » Needs review

MR created

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
There was 1 failure:
1) Drupal\Tests\navigation\FunctionalJavascript\NavigationBlockUiTest::testNavigationBlockAdminUiPage
Invalid permission configure navigation layout.
/builds/issue/drupal-3447881/core/modules/user/tests/src/Traits/UserCreationTrait.php:298
/builds/issue/drupal-3447881/core/modules/user/tests/src/Traits/UserCreationTrait.php:253
/builds/issue/drupal-3447881/core/modules/user/tests/src/Traits/UserCreationTrait.php:156
/builds/issue/drupal-3447881/core/modules/navigation/tests/src/FunctionalJavascript/NavigationBlockUiTest.php:61
FAILURES!
Tests: 1, Assertions: 3, Failures: 1.

Shows the test coverage.

I believe since this is experimental changing permissions should be fine without needing any kind of upgrade

Manually testing on 11.x the permission is working as advertised.

Believe this to be good.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs release manager review

Because navigation is beta stability, this does need an upgrade path, although it's only been in a beta release of 10.3, not any stable releases yet, so I have double checked with the other release managers - not sure this specific situation has come up before. Upgrade paths are 100% needed when beta stability modules are in release candidates or stable releases at least, and it probably applies here too.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Thank you for the heads-up. I was not completely sure whether this change might require an upgrade path or not.

Added post_update hook that assigns configure navigation layout permission to roles with configure any layout permission. Those were the roles with access to modify the navigation layout, so the change will be transparent for them.

I did not add any replacement for administer navigation_block permission, given that it was not being used anywhere after the LB refactor.

catch’s picture

catch
he/him
Primary language English
commented

I did not add any replacement for administer navigation_block permission, given that it was not being used anywhere after the LB refactor.

I think the post update would probably need to delete this permission from any roles that have it.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Added logic to get rid of the unused permission administer navigation_block.

Thank you for the guidance here.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs release manager review

Thanks I think this is fine. We would normally also need hook_role_presave() bc layer to update the config for module-shipped roles as well, but that is really not going to have happened one week after beta and it would be runtime code that would need to stay in navigation until 12.x if we added it, so I think it's better to skip that step here and just support any actual sites that potentially could have installed the beta.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Thanks for fixing this, just one minor question on the MR about the update path

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

Resolved the question opened by @larowlan.

I think it is safe to move it back to RTBC now.

Thank you!

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

catch and I agreed that a change record would be helpful here. I have added one but it needs to be reviewed. So, setting this to NR for that.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thank you for taking care of the initial CR.

Took a look into that and made some minor changes to add some extra information.

Back to RTBC now.

  • catch committed 65625e63 on 10.3.x 
    Issue #3447881 by plopesc, catch, larowlan, smustgrave, quietone:...

  • catch committed e20f1241 on 10.4.x 
    Issue #3447881 by plopesc, catch, larowlan, smustgrave, quietone:...

  • catch committed e9707944 on 11.0.x 
    Issue #3447881 by plopesc, catch, larowlan, smustgrave, quietone:...

  • catch committed d7f114cd on 11.x 
    Issue #3447881 by plopesc, catch, larowlan, smustgrave, quietone:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

All looks good to me now, thanks for the changes. We might want to tweak the experimental module allowed changes to make upgrades optional in the 'beta-in-beta' situation but at least this one is pretty straightforward.

Committed/pushed to 11.x and cherry-picked to back through to 10.3.x, thanks!

Status: Fixed » Closed (fixed)

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