When a security update comes out for Drupal, Update module displays a red notification on admin pages saying, "There is a security update available for your version of Drupal". If you're a site administrator, this message is helpful. But if you're not the person responsible for keeping the site up to date, this message can be scary. As a result, many site-builders turn off Update before handing a website over to end-users to prevent end-users (content managers, etc.) from getting upset or worried when it's time to update the site.

Since Update doesn't implement hook_permission at all, security update notifications are "all or nothing". If Update module is enabled, everyone who has access to admin pages see the warning. If you don't like this behavior, the only option is to turn Update off.

Wouldn't it be better to have Update module provide a permission for seeing (or not seeing) these security update notifications? This seems like a better solution for two reasons:

1. It just seems like a more elegant solution to have access to the warning message be permission-based / role-based.

2. For modules that depend on Update module, the current state of things is no good. Custom modules can't take advantage of Drupal core's Update module without forcing site builders to choose between added functionality and user friendliness. (I happen to be writing a module right now that depends on Update module. Which is why I'm scratching this itch right now.)

CommentFileSizeAuthor
#20 Screen Shot 2012-06-19 at 8.51.07 PM.png290.01 KBykhadilkar
#20 Screen Shot 2012-06-19 at 8.32.41 PM.png191.87 KBykhadilkar
#17 pretend-d8-insecure.patch3.87 KBbryanhirsch
#16 pretend-d8-insecure.patch3.87 KBbryanhirsch
#14 update-module-should-implement-hook-permission-1177752-14--8.x.patch8.28 KBbryanhirsch
#11 update-module-should-implement-hook-permission-1177752-7--8.x.patch8.21 KBbryanhirsch
#9 update-module-should-implement-hook-permission-1177752-7--8.x.patch8.21 KBbryanhirsch
#8 update-module-should-implement-hook-permission-1177752-7--7.x.patch8.08 KBbryanhirsch
#7 update-module-should-implement-hook-permission-1177752-7--7.x.patch8.08 KBbryanhirsch
#7 update-module-should-implement-hook-permission-1177752-7--8.x.patch8.21 KBbryanhirsch
#3 update-module-should-implement-hook-permission-1177752-1.patch6.71 KBbryanhirsch

Comments

bryanhirsch’s picture

bryanhirsch commented
Title: Update module should implement hook_permission for security update notifications » Patch
Status: Active » Needs review

(I opened a similar issue and posted a patch for 8.x-dev here: http://drupal.org/node/1177754.)

This patch implements hook_permission in Update module to give site administrators more granular control over who sees the scary red security update notification that appears on all admin pages when it's time to update the code base. Without this patch, Update module displays the red update notification to anyone with permission to access admin pages, no matter what their role is.

Update 7002 "grandfathers" in anyone who previously had access to admin pages, automatically granting them 'update notification' permission. This way, we won't confuse anyone who is accustomed to seeing these notifications.

In the future, if this patch is committed, people can be given permission to receive the security update notification by granting them the 'update notification' permission.

Note Re. Testing: This patch includes updates to update.test. Applying this patch to 7.x-dev causes two SimpleTest fails under "Update Contrib Functionality". I can't replicate this problem in a real site. I suspect this is actually an issue in update_test module (a module that provides dummy update info specifically for running SimpleTest tests). If people like this patch, I'll look into this further.

bryanhirsch’s picture

bryanhirsch commented
Title: Patch » Update module should implement hook_permission for security update notifications
bryanhirsch’s picture

bryanhirsch commented
StatusFileSize
new update-module-should-implement-hook-permission-1177752-1.patch6.71 KB

Oops. Seems like my patch didn't upload with comment #1. Trying again.

Status: Needs review » Needs work

The last submitted patch, update-module-should-implement-hook-permission-1177752-1.patch, failed testing.

bfroehle’s picture

bfroehle commented 
+  $rids = array();
+  foreach ($roles as $rid => $role) {
+    $rids[] = $rid;
+  }

Could instead just be $rids = array_keys($roles).

+      user_role_grant_permissions($rid, array('update notification'));

instead you should be using _update_7000_user_role_grant_permissions.

In fact, you should model the update off of system_update_7067.

The role name 'update notification' isn't very descriptive. I'm not sure what would be better, maybe 'view update notifications'?

bryanhirsch’s picture

bryanhirsch commented

Thanks for these helpful suggestions @bfroehle.

bryanhirsch’s picture

bryanhirsch commented
StatusFileSize
new update-module-should-implement-hook-permission-1177752-7--8.x.patch8.21 KB
new update-module-should-implement-hook-permission-1177752-7--7.x.patch8.08 KB
bryanhirsch’s picture

bryanhirsch commented
Status: Needs work » Needs review
StatusFileSize
new update-module-should-implement-hook-permission-1177752-7--7.x.patch8.08 KB

The patches I just uploaded say "Ignored" in the Status column. Is that because I uploaded two at once?

Trying again, one patch at a time.

This is for the 7.x branch.

bryanhirsch’s picture

bryanhirsch commented
StatusFileSize
new update-module-should-implement-hook-permission-1177752-7--8.x.patch8.21 KB

This is the patch for the 8.x branch.

Status: Needs review » Needs work

The last submitted patch, update-module-should-implement-hook-permission-1177752-7--8.x.patch, failed testing.

bryanhirsch’s picture

bryanhirsch commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
StatusFileSize
new update-module-should-implement-hook-permission-1177752-7--8.x.patch8.21 KB

Reviewing the details from the 8.x patch test, it looks like test bot is trying to apply the patch to the 7.x branch. Presumably this is because the version on this issue post is 7.x-dev.

I'm changing the version number to 8.x-dev and re-uploading my patch.

moshe weitzman’s picture

moshe weitzman commented

Shouldn’t admin/reports/updates and similar pages be shown only to the users with the new permission? Same for email recipients - see update_mail(). It seems a little weird to have a permission that just applies to the drupal_set_message().

dww’s picture

dww
we/he/they
commented
Status: Needs review » Closed (duplicate)

#332796: Add permissions to the update.module to hide warnings

bryanhirsch’s picture

bryanhirsch commented
StatusFileSize
new update-module-should-implement-hook-permission-1177752-14--8.x.patch8.28 KB

@moshe weitzman, I feel like an idiot. My first reaction to your comment was: Of course! That makes sense. Then my attention got diverted and now months later I'm coming back to resolve this. But looking at the UI, I see, there's no change necessary. Update module doesn't send email notifications to everyone who sees the scary red error messages on admin pages. It only sends updates to people whose email addresses are explicitly designated on admin/reports/updates/settings. Here's a screenshot:
https://skitch.com/bryan.hirsch/g2msj/available-updates-drupal8.dev

Attached is a rerolled patch that applies cleanly as of commit 8a87e13ad5f6f4b6185f2a8bc13b96abf5202caa.

barnettech’s picture

barnettech commented

This patch looks good to me, I think it is ready to be commited.

There is a similar issue in system.admin.inc line 14 which should have similar permission handling. I'll open a separate issue for that.

The new issue that is similar is located at: http://drupal.org/node/1403466

bryanhirsch’s picture

bryanhirsch commented
Status: Closed (duplicate) » Reviewed & tested by the community
StatusFileSize
new pretend-d8-insecure.patch3.87 KB

For people interested in testing this patch, you need drupal 8 to think there's an available security release. You can apply this patch to fool drupal into thinking drupal 8.x-1.0 has been released.

bryanhirsch’s picture

bryanhirsch commented
StatusFileSize
new pretend-d8-insecure.patch3.87 KB

oops. commenting out dsm() for patch to fool drupal into thinking there's a 8.x-1.0 release.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, pretend-d8-insecure.patch, failed testing.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Closed (duplicate)

Thanks for the patches, but this is still a duplicate issue. Please continue to discuss this and post your patches at #332796: Add permissions to the update.module to hide warnings. However, when you do, I'd strongly recommend using a more self-documenting name for the permission than 'update notification'. Anyway, please read the other issue, there's been a lot of useful discussion over there.

Thanks!
-Derek

ykhadilkar’s picture

ykhadilkar commented
StatusFileSize
new Screen Shot 2012-06-19 at 8.32.41 PM.png191.87 KB
new Screen Shot 2012-06-19 at 8.51.07 PM.png290.01 KB

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"