Warning message links to "available updates" even if user does not have permission for that page

I would test for the permission and render the message differently.

1. Good find! Thanks for noticing and opening this.
2. Agree with #4 that the solution is to have the code generate two versions of the message, depending on the perms of the current user.
3. xjm had some concerns on the message in the parent issue, not sure if we should fix those here or there. Edit: sorry, that was a different message. This one should be fixable on its own.

Thought I’d have a chance to get this started, so I clicked the branch button, but my day just changed. I probably won’t be able to work on this until Tuesday now. So if anyone else wants to run with it in the meantime, please do.

For tracking, calling this a child of #332796: Add permissions to the update.module to hide warnings.

Could it be this simple?

This one includes some simple tests (still very novice with testing).

Thanks for your patch.

+ if (\Drupal::currentUser()->hasPermission('administer site configuration')) {

I think it would be better to just call Url::access() instead of checking for a specific permission. That way, if the access requirements for the route ever change, we don't have to change the code again.

In the tests, I see that the patch tests whether the access check for /admin/reports/updates works as intended. But that's not really what this issue is about. I think we should test whether the link in the warning message is present or not with the different permissions as detailed in the steps to reproduce in the issue summary.

Can take another crack at it!

Thoughts?

Thanks, this is going in the right direction! I see that you found a few more places in the code that needed changes :).

+  if ($url->access(\Drupal::currentUser())) {

We don't have to pass an argument to access() in this case. If $account is NULL, which is the default, the access check will already be performed for the current user.

+  $url = Url::fromRoute('update.status', [], ['absolute' => TRUE, 'language' => $language]);
+  if ($url->access(\Drupal::currentUser())) {
+    $message['body'][] = t('See the available updates page for more information:', [], ['langcode' => $langcode]) . "\n" . Url::fromRoute('update.status', [], ['absolute' => TRUE, 'language' => $language])->toString();
+  }

I think we should use $url->toString(); in the message parameters. It will be shorter, more clear that the access check is performed for the same Url and less prone to error, if the Url ever needs updating.

+      $url = Url::fromRoute('update.status', [], [
+        'absolute' => TRUE,
+        'language' => $langcode,
+      ]);
       if ($msg_type == 'core') {
-        $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [':update-report' => Url::fromRoute('update.status')->toString()], ['langcode' => $langcode]);
+        if ($url->access(\Drupal::currentUser())) {
+          $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [
+            ':update-report' => Url::fromRoute('update.status')->toString(),
+          ], ['langcode' => $langcode]);
+        }
       }
       else {
-        $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [':update-report' => Url::fromRoute('update.status')->toString()], ['langcode' => $langcode]);
+        if ($url->access(\Drupal::currentUser())) {
+          $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [
+            ':update-report' => Url::fromRoute('update.status')->toString(),
+          ], ['langcode' => $langcode]);
+        }

Why do we use different parameters here for the access check and for the Url we print?

For the tests, they now test what we want to test. That's great! I think we could probably test both cases in one test method. That would save us one additional bootstrap. You could just call drupalLogin() again to re-login with the changed permissions. We might even put this into testModulePageRegularUpdate(), where other aspects of the message display are already tested, but I'm not entirely
sure on that one, so maybe take a look yourself, if you think that would be better.

For the next iteration, it would also be good to have a tests only patch that contains only the changes to the tests. Use the suffix -tests-only.patch. Upload that patch first and then your actual patch that contains all changes. We'd expect the tests only patch to fail and the patch with all changes to pass. That way, we can proof, that the tests will properly fail, if the the bug is present.

Let's set this to needs review so that the tests are run.

Great work, thanks!

Agree with most of #14. Here's a detailed review on #15:

1. +++ b/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php
   @@ -389,6 +389,68 @@ public function testModulePageRegularUpdate() {
   +    $this->setProjectInstalledVersion('8.0.0');
   +    // Instead of using refreshUpdateStatus(), set these manually.
   +    $this->config('update.settings')
   +      ->set('fetch.url', Url::fromRoute('update_test.update_test')
   +        ->setAbsolute()
   +        ->toString())
   +      ->save();
   +    // Use update XML that has no information to simulate a broken response from
   +    // the update server.
   +    $this->config('update_test.settings')
   +      ->set('xml_map', ['drupal' => 'broken'])
   +      ->save();
   +
   +    // This will retrieve broken updates.
   +    $this->cronRun();
   

   We already did all of this above, and it should persist across a drupalLogin(). I think all we need for the 2nd half of this test is:
   - drupalLogin()
   - drupalGet('/admin/appearance')
   - assert links...

2. +++ b/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php
   @@ -389,6 +389,68 @@ public function testModulePageRegularUpdate() {
   +  public function testAvailableUpdateLink() {
   

   I also wonder if these new assertions could be folded into an existing test, so we don't have to completely re-install Drupal for this.

3. +++ b/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php
   @@ -389,6 +389,68 @@ public function testModulePageRegularUpdate() {
   +    // Test that a user without the correct permissions can not view the available update link.
   

   Maybe this is more accurate:

   // Test that a user without the correct permissions is not shown the available updates link.
   

4. +++ b/core/modules/update/update.module
   @@ -401,7 +401,10 @@ function update_mail($key, &$message, $params) {
   -  $message['body'][] = t('See the available updates page for more information:', [], ['langcode' => $langcode]) . "\n" . Url::fromRoute('update.status', [], ['absolute' => TRUE, 'language' => $language])->toString();
   +  $url = Url::fromRoute('update.status', [], ['absolute' => TRUE, 'language' => $language]);
   +  if ($url->access()) {
   +    $message['body'][] = t('See the available updates page for more information:', [], ['langcode' => $langcode]) . "\n" . $url->toString();
   +  }
   

   I think we need to revert this part of the patch. update_mail() is generating email messages to arbitrary accounts. We don't care about the permissions of the current user to determine the contents of these emails. They could be sent to users without an account on the site at all, in fact.

5. +++ b/core/modules/update/update.module
   @@ -475,11 +478,23 @@ function _update_message_text($msg_type, $msg_reason, $langcode = NULL) {
   -        $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [':update-report' => Url::fromRoute('update.status')->toString()], ['langcode' => $langcode]);
   +        if ($url->access()) {
   +          $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [
   +            ':update-report' => $url->toString(),
   +          ], ['langcode' => $langcode]);
   +        }
   

   This is tricky, since _update_message_text() is reused by both update_mail() (where we do *not* want access checks, per above), and update_requirements() where we do need the checks.

   So probably we need to add another param to _update_message_text() that controls whether it does the link access checks or not.

   Looking at all this with "fresh" eyes, it seems like a bug that this version of the link is relative, even when used by update_mail() where we generally want absolute links.

   So, perhaps the new bool param should really be called something like $is_email and that toggles 2 things:
   - access checks
   - absolute vs. relative

6. +++ b/core/modules/update/update.module
   @@ -475,11 +478,23 @@ function _update_message_text($msg_type, $msg_reason, $langcode = NULL) {
   +        if ($url->access()) {
   +          $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [
   +            ':update-report' => $url->toString(),
   +          ], ['langcode' => $langcode]);
   +        }
   

   For both of these $url->access() checks, we need an else clause to still generate the text of the message, but don't format it as a link. E.g.:

   if ($url->access()) {
     $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [
       ':update-report' => $url->toString(),
     ], ['langcode' => $langcode]);
   }
   else {
     $text = t('There was a problem checking available updates for your modules or themes.', [], ['langcode' => $langcode]);
   }
   

Thanks again!
-Derek

I'll take a look but when I tried to add the tests into the function above testModulePageRegularUpdate they just stopped working.

p.s. Looking more closely at update.module, there's another spot in update.install where this could be a problem:

function _update_requirement_check($project, $type) {
  ...
      if (_update_manager_access()) {
        $requirement['description'][] = ['#prefix' => ' ', '#markup' => t('See the <a href=":available_updates">available updates</a> page for more information and to install your missing updates.', [':available_updates' => Url::fromRoute('update.report_update')->toString()])];
      }
      else {
        $requirement['description'][] = ['#prefix' => ' ', '#markup' => t('See the <a href=":available_updates">available updates</a> page for more information.', [':available_updates' => Url::fromRoute('update.status')->toString()])];
      }
...

However, I just convinced myself we're currently safe, since both the status report and available updates report require only 'administer site configuration' permission. 😅 So the only people seeing these requirement check warnings are users that can see the status report, and therefore can also see the available updates report. However, if we ever get fancy and have a new permission to control access to the available updates report, this is probably going to break. 🤓

Whoops, nope. I was wrong/right at #19. That *also* needs to be fixed. We reuse update_requirements() in update_page_top(). So we can be showing these requirements check messages to users that don't have administer site configuration perm, which is exactly what this issue is trying to fix. 😅

Sorry, one last note, copied from Slack. @smustgrave asked if the new bool $is_email param to _update_message_text() should default to TRUE or FALSE. I vote FALSE:

• update_requirements() is calling it as _update_message_text($type, $status)
• update_mail() is already explicitly defining the $langcode, and calls it as $message['body'][] = _update_message_text($msg_type, $msg_reason, $langcode);

So, it's easier for the caller in the mail case to override both 3rd and 4th default args with what makes sense for email, while the caller in the requirements case only needs to specify 2 params and get the right behavior.

p.s. Since this issue is definitely changing some translatable strings, it will require a new minor version of core to fix, so the earliest this could now be backported to is 9.5.x to be included in 9.5.0.

Is it just the else statement you are concerned about?

$requirement['description'][] = ['#prefix' => ' ', '#markup' => t('See the <a href=":available_updates">available updates</a> page for more information.', [':available_updates' => Url::fromRoute('update.status')->toString()])];

Since the other line of text deals with update.report_update

Re: #23. Exactly. The code I pasted into #19 actually needs to handle 3 cases:

1. User can access Update Manager (via administer software updates).
2. User can access Available updates report (via administer site configuration).
3. User can access neither (but is seeing the message from having view update notifications)

In this instance, the 3rd case probably shouldn't see the line at all, since "See the available updates page for more information." is useless text for someone who can't see that page...

Also instead of doing a url check should we just make a function for checking if they have the administer site configuration. Like the one currently exists for update-manager-access

Good question. But I think no need. _update_manager_access() is a helper since it’s dealing with both a settings.php killswitch and permission checks. The else case doesn’t care about all that, and it can just be a regular url access check.

Thanks,
-Derek

From #17 believe I have addressed 1, 3, 4, 5, and 6. I didn't address 2 because I couldn't get the tests working when adding to testModulePageRegularUpdate

Also believe I covered #24

Let me know if anything needs to be changed.

Typo

Sweet, thanks! Almost there...

Here's an interdiff between #15 and #28. Mostly looks really good. Confirming that all of #17 and #19-#24 are now addressed.

However, a few last minor problems / nits with #28:

1. +++ b/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php
   @@ -389,6 +389,50 @@ public function testModulePageRegularUpdate() {
   +   * Test the available link appears to users with the right permissions.
   

   s/Test/Tests/

2. +++ b/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php
   @@ -389,6 +389,50 @@ public function testModulePageRegularUpdate() {
   +    // Test that a user with the correct permissions is shown the available updates link.
   ...
   +    // Test that a user without the correct permissions is not shown the available updates link.
   

   We're supposed to wrap these at 80 chars.

3. +++ b/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php
   @@ -389,6 +389,50 @@ public function testModulePageRegularUpdate() {
   +    $this->assertSession()->linkNotExists('available updates');
   

   Seems we should also assert that the non-link text we expect *is* present.

4. +++ b/core/modules/update/update.module
   @@ -426,13 +426,16 @@ function update_mail($key, &$message, $params) {
   + * @param null $langcode
   

   This isn't the right type. I think we have to call this ?string since it supports both NULL and a langcode string.

   However, changing this at all seems out of scope for this issue. I think we should revert this.

5. +++ b/core/modules/update/update.module
   @@ -426,13 +426,16 @@ function update_mail($key, &$message, $params) {
   + * @param bool $is_email
   + *   (optional) Specific if this function is used for generated mail. Will toggle
   

   a. We should say it defaults to FALSE.
   b. Needs to wrap to 80 chars.
   c. Probably more helpful to say how access checks and relative URLs are impacted, not just "Will toggle".

   So how about:

    * @param bool $is_email
    *   (optional) Controls if this function is used for generating mail. If TRUE,
    *   URLs will be absolute and will be included without access checks. If FALSE,
    *   links will be relative and will only be included if the current user can
    *   access them. Defaults to FALSE.
   
   

6. +++ b/core/modules/update/update.module
   @@ -426,13 +426,16 @@ function update_mail($key, &$message, $params) {
   + * @return \Drupal\Core\StringTranslation\TranslatableMarkup|string
   

   This also seems out of scope for this bug fix.

7. +++ b/core/modules/update/update.module
   @@ -426,13 +426,16 @@ function update_mail($key, &$message, $params) {
   +function _update_message_text($msg_type, $msg_reason, $langcode = NULL, $is_email = FALSE) {
   

   We should add the type for at least the new arg.

   It'd be scope creep to add them for the existing 3, but I'd love to see those fixed, too. 😅 #NeedsFollowup. Although probably this function should completely go away and we can replace it with something OO with proper types.

8. +++ b/core/modules/update/update.module
   @@ -475,11 +478,29 @@ function _update_message_text($msg_type, $msg_reason, $langcode = NULL) {
   +        'absolute' => (bool) $is_email,
   

   If we define the param type, there's no need to cast it to a (bool) here. We already know it's a bool. 😉

Addressed points 1, 2, 4, 5, 6, 7, and 8 of comment #29, just point 3 is left, so needs work for that.

Thanks, y'all!

1. All review points addressed. I reviewed both interdiffs and the final #31 patch.
2. We have new tests, and they fail as expected in the test-only patch:
   

   There was 1 error:
   
   1) Drupal\Tests\update\Functional\UpdateSemverCoreTest::testAvailableUpdateLink
   Behat\Mink\Exception\ExpectationException: Link with label available updates not found.
   
   /var/www/html/core/tests/Drupal/Tests/WebAssert.php:560
   /var/www/html/core/tests/Drupal/Tests/WebAssert.php:362
   /var/www/html/core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php:435
   

   With the test-only patch applied, UpdateSemverCoreTest.php:435 is here:

       $this->assertSession()->linkNotExists('available updates');
   

   Which is in the "user without the right perms" part of the test.

3. I just updated the summary with a more plausible steps to reproduce scenario, and embedded before/after screenshots showing the change to the warning message.
4. This doesn't require a change record or release note.

Nothing left to do to improve this (that I can see). Therefore, RTBC!

Thanks again,
-Derek

Full disclosure: This patch is changing more code paths than the new test coverage is exercising. The changes impact multiple places where these messages are printed, but the tests are only showing one possible scenario. I think it's probably fine as-is, but a committer might want more thorough test coverage. Let's see what they think...

+++ b/core/modules/update/update.module
@@ -475,11 +480,29 @@ function _update_message_text($msg_type, $msg_reason, $langcode = NULL) {
       if ($msg_type == 'core') {
-        $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [':update-report' => Url::fromRoute('update.status')->toString()], ['langcode' => $langcode]);
+        if ($url->access() && !$is_email) {
+          $text = t('There was a problem checking <a href=":update-report">available updates</a> for Drupal.', [
+            ':update-report' => $url->toString(),
+          ], ['langcode' => $langcode]);
+        }
+        else {
+          $text = t('There was a problem checking available updates for Drupal.', [], ['langcode' => $langcode]);
+        }
       }
       else {
-        $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [':update-report' => Url::fromRoute('update.status')->toString()], ['langcode' => $langcode]);
+        if ($url->access() && !$is_email) {
+          $text = t('There was a problem checking <a href=":update-report">available updates</a> for your modules or themes.', [
+            ':update-report' => $url->toString(),
+          ], ['langcode' => $langcode]);
+        }
+        else {
+          $text = t('There was a problem checking available updates for Drupal.', [], ['langcode' => $langcode]);

eek, nested if/else in a switch statement 🤢

Personally I'd prefer to start putting some early returns here instead of else (but only on the lines we're touching).

For the rest of the function we should add a follow up to get rid of the switch and move to early returns (or even better, to match in D10).

Setting to needs review to see if anyone else agrees or its just me going too far with my else is a code-smell campaign here

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Argh, I totally missed this. It moved from RTBC to NR while I was offline for a month, and then it was totally buried/lost in my issue tracker backlog.

Converted this into an MR, and ported to main. Once the bot is happy, I'll push a commit to address #35 and make sure that doesn't break anything.

Added a comment on the MR, also it looks like the IS and screenshots need an update as there is no mention about the email part of this change and the screenshots show messages that don't align with the code changes.

OMFG, can we please skip InlineBlockTest?!?! That's randomly failed like 5 times for me today.

@acbramley: Thanks for the review! I pushed some commits to simplify the code / logic. It was indeed broken. Per the param description for $is_email we should always be including links in that case. The previous code did not accomplish that, but the latest commit I just pushed should cover it.

Updated the summary to mention we're not changing the format of the emails (and why).

But the screenshots are accurate. Those messages at the top of admin pages are from hook_page_top(). UpdateHooks::pageTop() does this:

$status = \Drupal::moduleHandler()->invoke('update', 'runtime_requirements');

The MR diff against src/hooks/UpdateRequirements.php is for dealing with the last line in the error message. If the current UI user doesn't have permissions to view the available updates report, the warning message doesn't include the final sentence about that report.

Pushed another commit to update a code comment to better explain this (I hope). 😅

Looking good, I wonder if we should have a follow up to add test coverage for that email?

@acbramley: Thanks for the re-review!

We've already got core/modules/update/tests/src/Unit/UpdateMailTest.php

I suspect that didn't fail with the broken logic here due to assumptions in the mocks. I'd have to spend a lot more time digging into that than I have time for right now. But if you want a follow-up to investigate that, I'm willing to open it.

Re: UpdateMailTest - hah, right. We never added cases for the NOT_FETCHED or UNKNOWN cases. 😅 And those are the ones that were broken in here. Whoops. #3571896: Add coverage to UpdateMailTest for NOT_FETCHED and other error conditions

One question: since there's no way to check access on email recipients, does it make sense not to provide the link on emails at all? Or maybe a link to the home page with a directive to log in and check?

@godotislate: That's a reasonable question. However, I vote we spin that off to a follow-up, if we want to explore it at all. The emails have been formatted basically like this for close to 20 years now. I don't remember any support requests or bug reports about it. I think generally speaking, if site admins configure email notifications at all, they have them go to full site admins. A direct link to the relevant report seems more useful than a link to the site front page and instructions how to navigate to the page you care about. But anyway, all that seems out of scope for this bug fix, which is ultimately a very minor change to the UI to hide links from a new class of site quasi-admins that have permissions to see these warnings, but not the underlying report. A hypothetical role in this situation is only possible after #332796: Add permissions to the update.module to hide warnings landed. Honestly, it's the UX team and myself the subsystem maintainer that noticed or care. Again, I don't remember ever seeing other support requests or bug reports about this. That said, I'd rather this bug didn't linger for another 4 years while we sort out the UX questions about reformatting the notification emails that "no one" has complained about for decades. 😅 Thanks!

IMO emails with potentially inaccessible links are fine. Most sites will redirect to a login page with a destination back to the page. But if we do want to change that it can/should be in a follow-up.

Agreed that it'd be a follow up, if anything, since it's always been like this, but wanted to confirm. I don't have strong feelings about it. Thanks!

Committed f5b6b5e and pushed to main and 420765c and pushed to 11.x. Thanks!