Use case:
Install and enable Admin Toolbar and Admin Toolbar Extra Tools modules.
Create editor role, assign 'Use the administration toolbar' permission to editor.
Create user with editor role.
Login with editor user

Problem:
User can see menu item: "Run database updates" - when clicking on item user get error message but menu item should be neither displayed.

Comments

animaci created an issue. See original summary.

animaci’s picture

animaci’s picture

Status: Active » Needs review

I think the menu item should require permission "administer software updates".
Please check my suggested solution I attached. Thanks!

cilefen’s picture

Does this patch hide the menu link?

animaci’s picture

Yes, only users who has "administer software updates" permission, will see the menu item.

But as it is my first patch, I really would like somebody to make review :)

animaci’s picture

Assigned: animaci » Unassigned

Assigned to me but set to unassigned as I would like to ask for review.

amateescu’s picture

Title: Permission problem for menuitem "Run database updates" » The 'system.db_update' route should restrict access via the 'access_check.db_update' service
Version: 8.0.6 » 8.1.x-dev
StatusFileSize
new3.66 KB
new4.07 KB

@animaci, good find and congrats for your first patch! :)

+++ b/core/modules/system/system.routing.yml
@@ -455,7 +455,7 @@ system.db_update:
-    _access: 'TRUE'
+    _permission: 'administer software updates'

Access to the db update page is also granted based on the 'update_free_access' setting, so it would be better to use the access_check.db_update service instead.

Here's also a small test for this.

The last submitted patch, 7: 2701851-7-test-only.patch, failed testing.

dawehner’s picture

Is there no place in core where we link to the updates and ideally should check access as well?

amateescu’s picture

StatusFileSize
new5.74 KB
new1.67 KB

There are a couple of places where we could do that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
index 3a3e273..6c14ce1 100644
--- a/core/modules/system/src/Controller/DbUpdateController.php

--- a/core/modules/system/src/Controller/DbUpdateController.php
+++ b/core/modules/system/src/Controller/DbUpdateController.php

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -374,6 +374,7 @@ protected function selection(Request $request) {

@@ -374,6 +374,7 @@ protected function selection(Request $request) {
         '#attributes' => array('class' => array('button', 'button--primary')),
         '#weight' => 5,
         '#url' => $url,
+        '#access' => $url->access($this->currentUser()),
       );

Yeah its a bit pointless here to be honest, I just though that there is a good link somewhere else, so we could have skipped the custom test controller, but nevermind.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c3e0c5b and pushed to 8.1.x ad 8.2.x. Thanks!

Committed to 8.1.x as the fix is a patch release safe bug fix.

  • alexpott committed 2b4da58 on 8.2.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...

  • alexpott committed c3e0c5b on 8.1.x
    Issue #2701851 by amateescu, animaci: The 'system.db_update' route...
animaci’s picture

@amateescu: Thanks for the fix! I can see that my patch was not really useful, only the finding, but I have learnt from your solution, really thanks for that! :)

Status: Fixed » Closed (fixed)

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