\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 #3508449: Add link to Status Report from database updates completion page 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
See https://git.drupalcode.org/project/drupal/-/merge_requests/11267#note_47... for example
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.
Make use of Url::access instead
Add a test?
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | Screenshot-1.png | 25.8 KB | juandhr |
| #6 | Screenshot-2.png | 81.34 KB | juandhr |
Show commandsStart 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:
changes
Comments
Comment #2
larowlan,
Comment #5
annmarysruthy commentedComment #6
juandhr commentedI can confirm that the changes introduced in the drupal-3512116/3512116-drupalsystemcontrollerdbupdatecontrollerhelpfullinks-checks-hard-coded branch work as expected.
While testing these changes in a local environment, I:
This confirms that the modification to use the access() method from Drupal\Core\Url is working as intended.
Comment #7
quietone commented@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.
Comment #8
mstrelan commented@quietone the problem is more hypothetical and the update here is for best practice. I'll try to explain the problem.
\Drupal\system\Controller\DbUpdateController::helpfulLinkswe check if the user has the'administer site configuration'permission before displaying the "Status report" link.\Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testSuccessfulMultilingualUpdateFunctionalitywe 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.'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.By changing to
Url::accesswe don't need to worry about what permission the route requires, just whether the logged in user can access it or not.I think the test mentioned above is sufficient, not sure it's worth the effort to expand that further.
Comment #9
smustgrave commented@quietone that work for you? Moving to RTBC to put in committer eyes.
Comment #11
longwaveI could argue that
helpfuLinks()should be filtered after the fact, maybe thelinkstemplate could have an option to check access before rendering each link, but this works for me as a simple solution given there are only two links to check. I agree also that this meets the criteria for not needing an explicit test, the manual test results are enough.Committed 9a1d0b1 and pushed to 11.x. Thanks!