Add permissions to the update.module to hide warnings

I don't think we're going to add any new features to this module. You need to try to get this change included in core. If it's in D7, and by some miracle, D6, we'll back port them into the D5 contrib. Otherwise, the best you can do is get this in D7.

That's a more accurate status...

This would be a really phenomenal feature. There's nothing worse than displaying confusing messages to the wrong users!

subscribing

Tagging.

Marking as a duplicate of #67234: Update script access rights. Let's fix this all over there.

Reopening issue to split out what's happening with #67234: Update script access rights.

So the patch in #67234: Update script access rights is going to add a new system.module permission 'administer software updates'. The question is should this permission apply to update.module menu paths or not.

Sounds like this still "Needs usability review" then. The question basically boils down to this:

"Should users that don't have permission to do anything about it be able to see if the site is missing software updates?"

When posed that way, it seems pretty obvious the answer should be "no". Especially since the new permission is going to be used over at #538660: Move update manager upgrade process into new authorize.php file (and make it actually work) which is in theory how many site admins are going to correct the problems listed on the available updates report.

So, yes, I think we should use 'administer software updates' (as soon as #67234 lands) to control access to all output generated by Update status in D7.

That makes a lot of sense. Here's the split out patch I had been working on. It depends on #67234: Update script access rights.

I'm not so sure the permission should be reused... it seems to me like there is a class of site administrators who might care enough to be able to see if their site is out of date, but not actually ever want to perform the updates themselves in case something breaks.

So I would suggest something like a "view update status information" permission for this. And then (the key usability issue), make sure that in the case of a user who has "view update status information" but doesn't have "administer software updates", we don't give them lots of broken links and dire warnings that they must run update.php this instant - but rather, something more informational in nature :)

If we do wind up sharing the permission, though, we'll need a solution to the issue that Dave Reid brought up at #67234-92: Update script access rights:

What should be put for the permission description since it adds features when update.module is installed?

As a blantant advertisement, I'll mention that the solution for that problem can be found in the patch at #248598: Label permissions which are warned about in the user interface, which would provide a hook_permission_alter() that allows modules to alter the permission descriptions in situations like this.

David Rothstein's point in #16 is a good one. Let's make them separate.

I think the underlying problem is that if you use more than a couple contributed modules, then you learn to ignore the upgrade warnings, almost like you would a banner ad, which means that you end up sometimes ignoring important error messages because you're so used to ignoring messages at the top of the page.

So the real UX problem is that the update status message should not persist across every admin page.

Yes an "administer software updates" permission is a good idea, but the underlying usability problem cannot be solved solely through permissions.

What if the status message is displayed only once every 24 hours, or only when a new session starts? Or there could be a "hide this message" link so you could acknowledge that you've seen the message but don't want to see it on every consecutive page. Alternatively the update status module could send an email to selected role(s) when new versions of installed modules become available.

subscribing...

Agree with #14 and #19.

subscribing...

Workaround for 6.x:
#342128: how I can hide error messages for anonymous users?
#371157: Don't show aggregator messages to anonymous users

expressing interest...

I actually want it the other way around to the original post. Some of my clients should not have "administer site configuration" or they will break their own sites, however I want them to know that they have updates available so that it is their responsibility to arrange for updates to be installed. If they don't know there are updates, and their site gets exploited then I will get blamed. So therefore I vote for a permission "view update status reports".

Hey mate, you could alternatively have the email reports about updates get forwarded to your clients. That would do it ;)

Thanks klonos however the email doesn't give enough information. It would be great if the entire available updates report page was emailed to the client. Perhaps that is an idea for a contrib module

Another duplicate, which had some potentially viable patches: #1177752: Update module should implement hook_permission for security update notifications

Moving to Drupal 8.x-dev where feature requests belong.

Dont see how this needs UX review atm.

subscribe - is there any way to hide security messages based on perms with a contrib module in 7.x right now?

I'm only aware of Disable Messages. One of its features is:

...

• Permissions to specifically hide all messages of a given type from any role.

...

#5: add_perms2.patch queued for re-testing.

This patch was originally posted on Update module should implement hook_permission for security update notifications. Since 1177752 issue is closed I am attaching that patch here.

This patch is failing because of change in testing framework. As of version "8a87e13ad5f6f4b6185f2a8bc13b96abf5202caa" update.test used to be there it got disappeared over period of time..... please check out screenshots current version and "8a87e13ad5f6f4b6185f2a8bc13b96abf5202caa"

Unassigning stale issue. Hopefully someone else will pursue this.

add a related issue about adding a global checkbox instead of a permissions.

12 years later this is still relevant and doesn't make a lot of sense to me to show the Error Message:

Error message
There is a security update available for your version of Drupal. To ensure the security of your server, you should update immediately! See the available updates page for more information.

to users WITHOUT the "Administer software updates" permission.

Each month a client asks us what he / she should do now with the message if a security update appears and updates haven't run yet.

I really don't understand why this message is not tied to the permission. I'm absolutely sure it's a more special case to show this message to users without that permission that the other way around... It's simply irritating. If we can't find a solution here, please let's at least add a setting for this to the update module: #2059375: Update notification messages as an option in settings

Rerolled #35
please review

Thak you @voleger, just a wording question:

Should the permission label not better be something like "See Update notifications" instead of "Update notification"? I think that would be clearer?

Sure, no problem

Thank you very much @voleger for your great work on this. I've created a third - simpler - approach to be discussed: #3239970: Show update notifications only with permission "administer software updates" and added all three to the issue summary.

We now need feedback from the core maintainers which way they'd like to go. I like all three, the problem with THIS issue is, that if a user has the "see update notifications" permission, still needs the "administer software updates" permission to see the notification (in the current implementation). This might be confusing, as it's untypical that you need two permissions to make the other one work as expected. So I now think adding this permission isn't the best possible option.

If core maintainers would like to split permission logic from the notification settings, adding a setting will be the better choice, as it doesn't introduce this permission confusion.

So I'd vote for #332796: Add permissions to the update.module to hide warnings instead of THIS issue. If a more simple solution is okay vor all I'd definitely vote for #3239970: Show update notifications only with permission "administer software updates"

--
Review: Please also add a test in testModulePageSecurityUpdate() to make it perfect. You only added one in testModulePageRegularUpdate().

Usability review

We discussed this issue at #3239070: Drupal Usability Meeting 2021-10-01. That issue has a link to a recording of the meeting.

We also reviewed the related issues listed in the issue description. The usability group did not have a strong consensus, but a majority of us preferred the solution in this issue.

Drupal has to accommodate different use cases. Consider

• A small site with an on-call administrator (superuser) and a few content editors
• A large site that is actively maintained and has many content editors

Both sites will give the administrators, not the content editors, permission to administer updates. The first site will want the content editors to see the warnings, so that they can tell the part-time administrator that the site needs an update. The second site will not want to distract its content editors with these warnings, and certainly does not want all of them messaging the administrator when an update becomes available.

Adding a new permission, as in this issue, lets each site show the warnings to the appropriate users. Making the warnings depend on the existing permission does not give such flexibility.

Thank you @benjifisher, if this is the way to go and ready for RTBC from the Usability / Concept perspective, we should finalize code review here.

I have three points:

1. Logical conditions issue / permission combination

In #55 I wrote:

I like all three, the problem with THIS issue is, that if a user has the "see update notifications" permission, still needs the "administer software updates" permission to see the notification (in the current implementation). This might be confusing, as it's untypical that you need two permissions to make the other one work as expected. So I now think adding this permission isn't the best possible option.

and I think it's still true, look at the part the if condition $currentUser->hasPermission('administer site configuration') wraps:

/**
 * Implements hook_page_top().
 */
function update_page_top() {
  /** @var \Drupal\Core\Routing\AdminContext $admin_context */
  $admin_context = \Drupal::service('router.admin_context');
  $route_match = \Drupal::routeMatch();
  $currentUser = \Drupal::currentUser();
  if ($admin_context->isAdminRoute($route_match->getRouteObject()) && $currentUser->hasPermission('administer site configuration')) {
    $route_name = $route_match->getRouteName();
    switch ($route_name) {
      // These pages don't need additional nagging.
      case 'update.theme_update':
      case 'system.theme_install':
      case 'update.module_update':
      case 'update.module_install':
      case 'update.status':
      case 'update.report_update':
      case 'update.report_install':
      case 'update.settings':
      case 'system.status':
      case 'update.confirmation_page':
        return;

      // If we are on the appearance or modules list, display a detailed report
      // of the update status.
      case 'system.themes_page':
      case 'system.modules_list':
        $verbose = $currentUser->hasPermission('see update notifications');
        break;

    }
    module_load_install('update');
    $status = update_requirements('runtime');
    foreach (['core', 'contrib'] as $report_type) {
      $type = 'update_' . $report_type;
      // hook_requirements() supports render arrays therefore we need to render
      // them before using
      // \Drupal\Core\Messenger\MessengerInterface::addStatus().
      if (isset($status[$type]['description']) && is_array($status[$type]['description'])) {
        $status[$type]['description'] = \Drupal::service('renderer')->renderPlain($status[$type]['description']);
      }
      if (!empty($verbose)) {
        if (isset($status[$type]['severity'])) {
          if ($status[$type]['severity'] == REQUIREMENT_ERROR) {
            \Drupal::messenger()->addError($status[$type]['description']);
          }
          elseif ($status[$type]['severity'] == REQUIREMENT_WARNING) {
            \Drupal::messenger()->addWarning($status[$type]['description']);
          }
        }
      }
      // Otherwise, if we're on *any* admin page and there's a security
      // update missing, print an error message about it.
      else {
        if (isset($status[$type])
            && isset($status[$type]['reason'])
            && $status[$type]['reason'] === UpdateManagerInterface::NOT_SECURE) {
          \Drupal::messenger()->addError($status[$type]['description']);
        }
      }
    }
  }
}

I guess that would have to be changed to allow users to see the notifications only with permission "see update notifications". Otherwise it will be confusing if you gave the permissions, but notifications still don't show up!

2. Add / merge tests

Would be cool if someone could check if added tests from the other two issues would make sense to be added here

3. Update hook

To keep the behaviour as-is on existing sites, this update hook would have to be changed to use "administer site configuration" instead of "administer software updates" - as that was the condition to see the notifications before!

/**
 * Implements hook_update_N().
 *
 * Grant 'see update notifications' permission to users with permission to
 * 'administer site configuration'.
 *
 * Up to now everyone with 'administer site configuration' permission
 * receives error messages on admin pages whenever there are security updates
 * to Drupal or contrib projects. As of update 8000, these notifications can be
 * enabled or disabled by granting/revoking 'see update notifications' permission.
 * This update grants 'see update notifications' permission to anyone who has had
 * this permission through 'administer site configuration' in the past.
 */
function update_update_9300(&$sandbox) {
  $roles = user_roles(FALSE, 'administer software updates');
  foreach (array_keys($roles) as $rid) {
    user_role_grant_permissions($rid, ['see update notifications']);
  }
}

I still personally think that the "permission clutter" and these conceptual points are good reasons for the simpler solution (#3239970), but I'm absolutely fine to commit this issue once these points have been solved.

See my comment #10 in #3239970: Show update notifications only with permission "administer software updates" for my feedback on choosing one of the solutions.

Addressed #58.1 and #58.3

Apologies for not reviewing all this sooner. I'm still leaning towards #3239970: Show update notifications only with permission "administer software updates" (as I did in 2009 at #14, even though it wasn't open as a distinct issue then). I can see the points made by the UX team at #57. However, for the specific scenario described where someone else needs to know about missing updates, it seems a little wonky to show them to users that can't do anything about it, only so they can pass on the message to someone else who can. Why not use the "Email addresses to notify when updates are available" feature in cases like that? Just put the part-time admin's email address in there and presto, they get notified whenever they need to know. 0 interaction from content editors required.

If we do what this issue proposes, we have to:

• Live with yet more permission bloat.
• Update existing site roles that have 1 perm to add this other perm.
• Write new update tests.
• Deal with cases on new sites where someone has 1 perm but not the other.
• Change default perms shipped with various roles from core install profiles.
• ...

I still think not showing the messages to people that can't take action is the simplest (and therefore, best) solution. It's doesn't require any new perms, doesn't require an update hook, doesn't require update tests, etc. If someone who's not regularly browsing the site needs to know about updates, we already have a mechanism to inform them.

However, if we are going to add new perms as proposed here, we need to fix failing tests before this is ready for serious consideration...

I'm not sure it's worth more implementation or a closer code review until we decide what approach we're really taking.

Timezones really make attending UX meetings hard for me, but maybe I can discuss with the team via Slack asynchronously...

As written in #58 I totally agree with @dww:

I still personally think that the "permission clutter" and these conceptual points are good reasons for the simpler solution (#3239970), but I'm absolutely fine to commit this issue once these points have been solved.

See my comment #10 in #3239970: Show update notifications only with permission "administer software updates" for my feedback on choosing one of the solutions.

Looking forward to a final decision by whoever ;) Dries would surely be happy to decide this ;D haha

Updated test which missing the introduced permission for the testing user.
See https://git.drupalcode.org/project/drupal/-/merge_requests/1139/diffs?co...

I think I need to clarify a few things about the usability group.

We meet once a week for an hour over Zoom, and typically review two issues in that hour. I count the different proposals for the warnings as a single issue. You can watch the recording if you like: there is a link at #3239070: Drupal Usability Meeting 2021-10-01. Six people attended that meeting.

Half an hour is not a lot of time to understand an issue you have not seen before. That is one reason for our policy: "Agenda is first come, first serve and set by attendees." That is also why I have a canned response that basically says, "Update the issue summary. Do not make me read the comments." It is a lot easier if someone can attend the meeting who is familiar with the issue.

The next meeting is in about 10 hours. I know that @dww cannot make it at that time, but maybe someone else who has worked on the issue can.

Usability is one factor to consider. On our side, that means we do not have to consider implementation, just the result. On the maintainers' side, it means that usability should be considered along with other factors, such as maintainability, performance, and so on. See the Drupal core gates.

With that context, let me repeat part of my earlier comment:

The usability group did not have a strong consensus, but a majority of us preferred the solution in this issue.

A majority but not a strong consensus means a recommendation but not a strong one. Take that into consideration as you balance the group's recommendation against other factors.

Personally, I am not convinced that an e-mail notification is an adequate replacement for the constant warning of the update notices.

Implementation is not part of the usability review. Whichever permission(s) you end up with, find an implementation that makes sense. (I am thinking of the last part of #58.1.)

@dww, are you sure that the BC implications of this proposal are worse than for the other approach? By default (before thinking about a possible update function) we are showing the warnings to a different set of users. I think in both cases we are showing the warnings to fewer users. If you do consider an update function, then I think it is easier to keep the status quo if you have the new permission that you can assign.

Needs decision and reroll.

Just rebased the existing MR, thanks!

Indeed, we need a decision. I'm still on the fence and wish we could do #3239970: Show update notifications only with permission "administer software updates". However, for minimum disruption, this issue's approach is probably preferred. I'm tagging both issues for a product manager review to decide how to proceed.

Rebased MR over 9.4.x branch
Reverted unrelated changes
Updated new related tests with missing new permission.
MR has to be good to review again.

Fixed CS errors.

Sorry for the delays on this! Upon further discussion / reflection, I've changed my mind and agree that this issue (#332796) is indeed the right solution for this problem. I closed #3239970: Show update notifications only with permission "administer software updates" as won't fix. Everyone who meaningfully contributed there is already credited here, so there's no remaining credit to transfer. Also removing the product manager tag, since that review is no longer needed.

Updating the issue summary to be accurate about what this issue is doing, its relationship to the other issues, remaining tasks, etc.

@ranjith_kumar_k_u: Now that there's an MR open for this issue, please contribute new code by pushing commits to the MR branch, not uploading patches. It really confuses things and creates more work for everyone to try to interleave patch contributions and MR commits. Let's all stick to the MR from now on. Thanks!

Opened some MR threads for stuff that needs work before this is merge-able.

I addressed all my own MR threads. @voleger or a core committer is invited to mark them resolved (if y'all agree with my points and the solutions). 😅 I also just added an update path test for the post_update(), which is passing locally. Hopefully the bot agrees. 🤞

I think this is pretty close to RTBC now, but I'm no longer eligible to say so myself. 😉

Given how many issues this problem has spawned, how many folks are participating and/or following them, that apparently folks uninstall the Update Manager as the only way to hide these warnings, and that this is a much-needed feature, I'm going to bump this up to major. Sort of irrelevant for feature requests, but I think it's worth upping the priority.

Now we need to track down someone(s) willing to RTBC this...

@dww thanks for pushing this forward. Changes look good for me. +1 for RTBC

Found a few things, but nothing major. Nice work!

Thanks for the great review, @phenaproxima! Working on the fixes now. Sadly, UpdateSemverCoreTest is slow on my laptop, so it's taking a little while to verify any changes to that. But I should be ready to push a slew of new commits in a bit. Assigning to myself so no one else wastes time duplicating my effort.

Fixed everything @phenaproxima pointed out, except this 1 thread is still unresolved:

https://git.drupalcode.org/project/drupal/-/merge_requests/1139#note_77821

Not sure what the best description should be, since it's a little complicated exactly how it all works. See the MR thread for details.

Also adding credit for @phenaproxima's thorough review.

Almost there!

For the record, the attendees at the April 22 usability meeting were AaronMcHale, andregp, Antoniya, bnjmnm, cedewey, worldlinemine, shaal, rkoller, and me.

We did not come back to this issue at the April 29 meeting, but perhaps we will discuss it at the May 6 meeting.

We discussed this issue at #3277946: Drupal Usability Meeting 2022-05-06. That issue will have a link to a recording of the meeting. Since we discussed this issue at previous meetings, we only talked about the wording of the permission title and description.

We agreed that "View update notifications" is a good title for the permission.

We suggest a slightly shorter description than the ones suggested on the merge request:

These messages are displayed on most administrative pages when updates become available.

This avoids repeating words from the permission title, and it follows the same pattern as this permission from the System module:

View the administration theme

This is only used when the site is configured to use a separate administration theme on the Appearance page.

For the record, the attendees at the 2022-05-06 usability meeting were @AaronMcHale, @bnjmnm, @ckrina, @rkoller, @worldlinemine, @shaal, and me.

Sweet, thanks for the review and the clear direction! I'll rename things right now.

Also rebased to 9.4.x. Not sure if we could still sneak this in before beta...

GREAT work all, just had a final look at the code and tried it! Just perfect :) Thank you so much, this look minor, but is a huge improvement for us! Looking so much forward to 9.4!

I think we need a change record about the new permission and a release note. I'm not sure this is going to make it before the beta.

Thanks for the review. The summary already has a release note snippet. But I'll work on a CR now. Stay tuned.

Draft CR: https://www.drupal.org/node/3280750

Fixed the release note per #79.

s/see/view/ in a few more spots in the summary.

Committed and pushed 1f89713bc3 to 10.0.x and ec5b3639ef to 9.5.x and a67b0a58ff to 9.4.x. Thanks!

Whoot, thanks! 🎉

@alexpott those commits aren't appearing on Gitlab

Hm, this issue should have gotten release manager signoff before commit. @catch and I have some concerns with the current approach.

We haven't decided to revert it at present, and it'd be a pain to have to have an upgrade path to undo the upgrade path, but I'm at least reopening the issue to make sure we follow up on this.

So I think the patch here is probably fine, but I'm not sure there was an investigation into why things were like they were in the first place.

When these warnings were originally added, 'administer site configuration' was used so that the maximum number of site admins would see it, and it also controls access to admin/reports/update. Whether a user has permission to 'administer site updates' is irrelevant, because that only controls using update module to download updates via the UI, whereas even one person running a site by themselves might be doing updates via git (or ftping tarballs) and not care about that permission at all. So we don't know whether the person who has the permission can actually do updates or not, their account might not be the only access they have.

In many cases, this are customers who are then afraid as they don't understand what they can (not) do and what are the consequences.

To some extent this is by design - if you have an only loosely-maintained site, then those customers can ask the developer to do the update. Otherwise no-one might get notified at all.

Having said all this, we now have the administrative role which gets all permissions - and people with this role will still get the notifications, so that probably fulfils the original intent of using 'administer site configuration' to catch most admins.

I think my primary concern is that it is the permission is not named something like:

MAKE SURE to give this VERY IMPORTANT permission to anyone who should ever be informed that the site needs to be updated

(Kind of joking but also kind of not)

The upgrade path covers existing sites, but that does not help new sites that have any roles between auth and admin to set the permission correctly. The help docs are OK, but not everyone reads help docs or even has the module enabled.

The main thing that gets people to turn the permission on (or not) is the permission label, and secondarily the permission description. Neither of those ATM indicate that this permission is critical to site operation and so somebody better darn well have it.

Actually I do also have an issue with the help docs:
<a href=":update_permissions">permission to view update notifications</a>

Should be:
<a href=":update_permissions">the <em>view update notifications</em> permission</a>

(Or whatever better name we give the permission if we come up with a better name.)

Also, does a help topic need updates for this yet?

Thank you @catch and @xjm,

while I'm still 100% sure this improves the situation for many projects and should definitely be kept, I agree the permission texts could still be improved to reflect the relevance of this setting. I'd suggest to improve both the title and the description?

Original:

view update notifications:
  title: 'View update notifications'
  description: 'These messages are displayed on most administrative pages when updates become available.'

Some ideas, surely not perfect yet:

"View update notifications":

1. "Display update notifications"
2. "Display available update notifications"
3. "Display the available update notifications"
4. "Show available update notifications on administrative pages"
5. ... ?

"View" isn't the perfect choice here, I think as it's typically used for "View XYZ entity" access permission, which is very different?

"These messages are displayed on most administrative pages when updates become available."

1. "(Security) update notifications are displayed on most administrative pages when updates become available."
2. "Pending (security) update notifications are displayed on most administrative pages when updates become available."
3. "(Security) update notifications are displayed on most administrative pages when updates become available. Ensure to assign it to all relevant roles to keep the system up to date."
4. ... ?

What do you think?

Thanks @Anybody. I think we need to explicitly tell the user that they will not receive notifications of security updates without the permission, in the permissions UI. "Update notifications" is also vague.

So a better label for the permission might be something like:
Receive notifications about available site updates

(...because it's not just about display in the UI; it's also about receiving emails, no?)

And then the description should mention the risk of not granting it. E.g., add a sentence something like:
Ensure that site administrators have this permission so that security updates are applied promptly.

Both those suggestions could use UX review.

I also think this should be a restrict access permission -- granting it to a less-trusted role would tell untrusted users that the site is insecure, which is also bad.

Sorry I don’t have time for a more thorough reply, but a few quick points:

1. The new perm only impacts the messages injected by update manager via update_page_top() on most admin pages.
2. The new perm doesn’t control who has access to the Available updates report itself.
3. The perm doesn’t have any impact on email notifications at all. Your site can be configured to email anyone you want, whether they have an account or not, much less what permissions their account might have.
4. I don’t think we want to call this a restricted perm. The whole reason we went this approach instead of requiring “administer software updates” (or whatever it’s called) is that some people liked having this message printed for non-super-admins so they would know they need to ping their developer to install stuff.

All that said, very happy to consider renaming the perm and updating the description. But I don’t have time right now to more closely consider the proposals and assess the details, come up with other suggestions, etc. Hopefully next week. But the UX team is meeting tomorrow morning, so maybe they could give this a look. I’ll at least ping the UX Slack channel about it.

Thanks/ sorry, -Derek

Pinged Slack and added to #3284204: Drupal Usability Meeting 2022-06-10.

We discussed this issue at #3284204: Drupal Usability Meeting 2022-06-10. That issue will have a link to a recording of the meeting. Thanks to @dww for bringing it to our attention. We do not have any firm recommendations, but we do have some suggestions.

For the record, the attendees at today's usability meeting were @AaronMcHale, @andregp, @rkoller, @shaal, @simohell, @worldlinemine, and me.

Based on that discussion, I added #3285176: Warning message links to "available updates" even if user does not have permission for that page as a follow-up. This is related to #95 and #100.2.

We feel that the most important point raised in #94-#96 is that we might be in a situation where

• before this issue, someone would see the messages;
• after this issue, no one will see the messages.

Since the Administrator role automatically gets all permissions, that means there is a role that

• has the "Administer site configuration" permission
• does not have the new "View update notifications" permission

We feel that the suggestion of adding a description to the new permission is not an effective way to get the site builder's attention. Here are some alternatives:

1. Add a description to the existing "Administer site configuration" permission, suggesting that any role with this permission should also get the new "View update notifications" permission.
2. Automatically grant the new "View update notifications" permission whenever "Administer site configuration" permission is granted in the UI. The site builder can un-check the box, and trigger a status message to alert the site builder when the form is submitted.
3. Add a warning to the site status report if there is a role with the "Administer site configuration" permission but not the new "View update notifications" permission.

Probably (2) is too heavy-handed, but it is an option.

I would also like to point out that there are many cases where this new permission could lead to more people seeing the warning message.

The first time that this issue came up at a usability meeting (2021-10-01), I added #57 and suggested some different scenarios where this permission might be granted or not. If anyone is still nervous about this new permission, I would like to know the specific scenario they have in mind. Is it more or less common than the case I described of a small-ish site with an absentee administrator and one or a few more active content editors?

Thanks, @benjifisher!

Re 1 vs. 2 vs. 3:
I think (3) is the most realistic, helpful, visible and lightweight solution. I think (1) might not be seen and (2) is way too much and perhaps even confusing.

Re: more people seeing the warning message:

I would also like to point out that there are many cases where this new permission could lead to more people seeing the warning message.

Could you clarify that please? In which case? Through the update or manually? I only see that it's now possible, but not that it may happen unintended?
Perhaps that's what you mean with "could", but that's no disadvantage, I think.

Re: Use cases:
As written above, in our experience:

• the typical case is that we need to exclude site owners / customers from seeing the notification (on every page!), which they don't understand and which confuses / frightens them
• others may want to inform certain roles about available updates

But I'll repeat myself, see above, so skipping here.
See the alternative approaches and discussions, there are so many possible ways to solve this, but at least any way of configuration / separation for this message display is needed... I'm happy with any solution ;) And now that we reached the 100 comments my fear is that it's becoming "too perfect" ... ;) :)

Also, there seems to have been some confusion for @xjm about the impact of this MR (mail, ... which isn't the case)

So my personal vote would be to clarify the texts, which would definitely be useful and add a message to the status report for "dangerous cases".

In #102, I wrote,

... there are many cases where this new permission could lead to more people seeing the warning message.

What I have in mind is that the new permission can be given to content editors or other roles that would not be appropriate for the "Administer site configuration" permission. So "could" rather than "will" because it depends on how permissions are assigned.

This is in the context of a new site, not an existing one, since that is the case of concern in #94-#96.

I agree with most of #103, except for the value of #102.1.

Thanks @benjifisher for clarification! I'm also fine with #102.1 if there's a consensus. It's not useless, but I'd like it as an addition to #102.3 then. Let's hear what the others think. :)

To sum it up, I'd like to propose the following improvements to the MR:

1. Improve permission label and description - as inspiration, see #98
2. Add a message to the status report like described in #102.3
3. Improve description of "Administer site configuration" permission as of #102.1
4. Fix #3285176: Warning message links to "available updates" even if user does not have permission for that page

Did I forget anything?

Who can / should decide about the wordings? I think it won't make sense to implement anything here until we have a final decision?

Sorry for the delays, finally able to make some space for a real reply.

@xjm re: #94:

this issue should have gotten release manager signoff before commit

How would we have known that? There were no dependency changes, deprecations, API changes, etc. It had passed UX review. Is it because it has an upgrade path? Should all issues with an upgrade path get a release manager review? Just wondering how to do better next time. Thanks!

@catch re: #95:

I'm not sure there was an investigation into why things were like they were in the first place...

As the person who first had the idea to add these warnings, as the author of all the original code (most of which is still here 15 years later!), as someone who's participated in this issue from the beginning, and as a subsystem maintainer, I thought such an investigation was implicit in my work on the MR. 😀 Yes, we want as many users as reasonably possible to see these warnings. We always have. But since some people were turning off update.module entirely to hide them, that's worse than giving sites flexibility to decide. Since we always have the super admin role, at least those folks will automatically have it. I believe that sites which have a more complex role setup can be trusted to make this decision for themselves. Again, worst case that they don't grant the permission to some roles, the status report will still complain if there are missing updates for anyone that can view that.

Re: #97: Fair points. Do those want to be addressed here in this re-opened issue, or would it be easier / better for scope to open follow-ups for those?

Re: #98: Thanks for the suggestions. Sadly, all those names read more like checkboxes in a settings UI page, not the names of permissions. None of them describe access to something, which is what permission names should convey. I think "View" is accurate, since that's the action that this permission grants: the ability to view a specific kind of warning message. Seems very similar to entity access "view" perms.

Re: #99: Agree that "Update notifications" is vague.

Re: #102: Thanks for re-reviewing this at the UX meeting! Following #3285176 and hope to work on that next. Agreed that there will be cases where new sites that want more layers of admins might not always grant this permission as widely as we might want them to. For the specific suggestions:

1. Hrm. That perm is already restricted, so it gets a pretty big description already. If we add more text there, are we improving the UX or making a wall of words no one will actually read?
2. That seems very heavy-handed, complicated to implement, and would be the first time (I know of) that core tried to automatically add other perms when you try to add another. I suspect people would think it's a bug, not a feature. 😅 Plus what about API First? Would this auto-suggestion only happen via the UI?
3. I guess the status report warning could be sufficiently annoying to be noticed but not so annoying that people turn off update.module over it. 😉 Again, seems like something I'd rather handle in a follow-up than to do directly in here.

Just re-read #57 and completely agree with that assessment / scenario.

Re: #105: Thanks for trying to keep this moving and summarizing.

Who can / should decide about the wordings?

People propose ideas, someone like me (subsystem maintainer) tries to get agreement, make sure the UX team and core committers are happy, etc.

However, I think the first step is agreement on if we should really keep discussing all of this in this issue, or open follow-up child issues for the suggested improvements and address each change separately. Usually, we don't re-open committed issues like this and do additional commits with the same nid. It makes the git history and issue credit system weird. I know xjm re-opened this in case it was going to be reverted, but thankfully it didn't come to that, and 9.4.0 is now out and includes the new permission. We've already got #3285176: Warning message links to "available updates" even if user does not have permission for that page as a follow-up. I'd like to propose at least 2 more:

• Update the new permission's label and description text to address the various concerns (e.g. #97, #99, etc)
• The status report warning for roles that have 'administer site configuration' but not 'view update notifications'.

We can open one to explore #102.1, but I'm less convinced that's going to be worth doing. Happy to at least open an issue to discuss more if folks prefer.

Would a release manager be willing to confirm if "Needs followup" is the right move here, or if we want to open a new branch/ MR and keep hashing out these other changes directly in this issue? I'd go ahead and make the follow-ups myself, but I don't want to preempt the release manager decision and I'll wait until they approve / reject that strategy. Back to RTBC simply to escalate this to the release managers for the next decision, not since there's something for them to commit...

Thanks!
-Derek

Building on the UX review in #102 and the subsequent discussion. Here's some additional thoughts, which I may or may not have mentioned during the UX meeting:

1. I feel like the "Administer site configuration" permission it too broad in its scope, (in a separate issue) it may be worth exploring ho we break that out into more specific permissions for the various actions/pages it grants access to.
2. One thing we could do is, if the user has "administer software updates" we always show the notifications regardless of whether they have the new "view updated notifications" permission. This behaviour would be consistent with other permissions, where for example "administer content" grants all permissions related to the node module, even if the user doesn't explicitly have any of the "view" permissions they can still view all content.
3. Building on my first point above, if we're going to display a warning in the status report, I would suggest that the warning is triggered if the user has "administer software updates" but not "view update notification" (rather than "administer site configuration"), as there's much more of a cognitive link between these two permissions.
4. Building on that, if we introduce such a warning, it would be a new pattern of basically warning when there is a potential permissions mismatch, so we should design it in such a way that it is extensible. By that I mean, the title of the warning could be something like "A potential permissions mismatch or conflict has been detected", with a bulleted list containing the specific warning description and recommended action. That way we have a pattern which can be used for any future cases.
5. I agree with @dww that all of this should be done in follow-up issues. The key discussion here centres around something which is practically a very unlikely edge case.

Will add any more things that come to mind.

Thanks,
-Aaron

Thanks @AaronMcHale!

++ on (1). (2), (5) plus refining the label / description.
I think (3) and (4) would be obsolete then, and your suggestion is even more intuitive! Thanks!

++ on (1). (2), (5) plus refining the label / description.
I think (3) and (4) would be obsolete then, and your suggestion is even more intuitive! Thanks!

Thank you :)

Yeah you're probably right there, if (2) is done, then there's even less of a need for (3) and (4).

However, there was an expressed use case during the UX Call where you might have a type of user who has access to perform software updates, but it's not part of their day-to-day role, so it's not necessary for them to see the notification messages. For example, an on-call support admin, who is not the person responsible for the regular maintenance and only steps in during planned periods (for instance if the regular administrator goes on holiday). I don't think that's an uncommon scenario, but even in that scenario the on-call admin probably has full permissions anyway and access to the hosting environment, because not giving them full permissions could prevent them from being able to respond to an unplanned incident, so the chances are they will still see the warnings because they'll have the admin role.

That then brings me back round to saying that, if we just ensure that those with "administer software updates" also see the warnings, then honestly, I think we've covered 99% of sites out there (he says plucking a number out of thin air).

I chatted with @xjm in Slack about this. We agreed:

1. Basically any issue about update.module is inherently interesting to the release manager. Update manager only exists to help site owners understand releases. 😅
2. Ultimately it's committers who need to understand the governance stuff, and what sign-offs are needed for each issue before commit. But it helps for subsystem maintainers (and anyone else) to know these things, too.
3. We should open followups now that the change has already gone out in a tagged release.

Changing tags, status and assigning to myself to open those followups and add credit as needed.

Some quotes from @xjm:

1. This issue is just tricky because I didn't notice it until after it went out in a tagged release, so I couldn't just revert it.

2. Yeah I mostly reopened it because that's what I could do at 6p on Memorial Day :joy: A new issue is probably OK if we add all the people who should be credited for the followup so far. It's not so much "refinements" as "fixing a regression" IMO.

3. Because really ordinarily it would've been reverted, if there were either of "not in beta1" or "not a data upgrade path"

Thanks! -Derek

All follow-ups created as children of this issue, credits transferred:

• #3285176: Warning message links to "available updates" even if user does not have permission for that page
• #3295078: Fix the label and description of the new "view update notifications" permission
• #3295081: Show Update manager /admin/* UI warnings to anyone with "administer software updates" permission, too
• #3295082: [PP-1] Fix any help topics about the update manager UI warnings from #332796
• #3295086: [PP-1] Fix help text on /admin/reports/updates/settings about permission added in #332796
• #3295087: [PP-1] Add a status report warning if there are roles configured with "administer site configuration" that do not have "view update notifications"

(The last few are postponed on resolution to #3295078...)

If I missed credit for anyone at any of those, please comment there and I can add you.

Restoring this issue to Fixed.

Thanks, everyone!
-Derek