\Drupal\system\Controller\DbUpdateController::helpfulLinks checks hard-coded permissions but should check access on the url

Created on 11 March 2025, about 1 month ago

Problem/Motivation

\Drupal\system\Controller\DbUpdateController::helpfulLinks checks $this->account->hasPermission which hard-codes the permissions required for those routes.

This was identified in new changes in Add link to Status Report from database updates completion page Active and decided to resolve in a followup because there is an existing pattern.

Instead it should make use of Url->access and avoid hard-coding

Steps to reproduce

Alter the system.admin route to require different permissions
Run update.php and view the helpful links on post update page
Click the link to 'Administration pages' and note access is denied.

Proposed resolution

Make use of Url::access instead

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component

base system

Created by

🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @larowlan
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • First commit to issue fork.
  • Pipeline finished with Success
    about 1 month ago
    Total: 3130s
    #447338
  • Pipeline finished with Success
    about 1 month ago
    Total: 398s
    #447379
  • 🇨🇴Colombia juandhr Manizales

    I can confirm that the changes introduced in the dwwed branch work as expected.

    While testing these changes in a local environment, I:

    • Added the 'access administration pages', 'administer site configuration', and 'administer software updates' permissions to a test role.
    • Logged in with that role and navigated to the /update.php/selection page in the default update process.
    • Verified that the "Administration pages" and "Status report" links display correctly.

    This confirms that the modification to use the access() method from Drupal\Core\Url is working as intended.

  • 🇳🇿New Zealand quietone

    @juandhr, thanks for testing this issue and reporting the steps you followed. That is good practice. Is also helps to provide confirmation of the problem, before the patch is applied. And you can save yourself some time and effort because in Drupal core there is no need to upload screenshots of your IDE. There is some information about that at https://www.drupal.org/about/core/policies/maintainers/how-is-credit-gra... .

    Can we add a test for this?

    Not sure what I am doing wrong but I can't duplicate the problem.

  • 🇦🇺Australia mstrelan

    @quietone the problem is more hypothetical and the update here is for best practice. I'll try to explain the problem.

    • In \Drupal\system\Controller\DbUpdateController::helpfulLinks we check if the user has the 'administer site configuration' permission before displaying the "Status report" link.
    • In \Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testSuccessfulMultilingualUpdateFunctionality we create a user with a bunch of permissions. This includes both 'access site reports' and 'administer site configuration'. Then we assert that the "Status report" link exists.
    • Core later decides that the permission required to access the 'system.status' route should actually be 'access site reports' and not 'administer site configuration' so the route is updated. The test still passes because that user has both permissions. But a user with 'administer site configuration' and not 'access site reports' will still see the link, even though they won't be able to access the route.
    • The same problem could arise if a contrib or custom module decides to alter the permissions required for the route. For example, they might override the status report page and provide a custom permission for it. The link will still appear for users with the original permission.

    By changing to Url::access we don't need to worry about what permission the route requires, just whether the logged in user can access it or not.

    Can we add a test for this?

    I think the test mentioned above is sufficient, not sure it's worth the effort to expand that further.

Production build 0.71.5 2024