update.module provides a setting for site admins to enter a set of email addresses to notify when the site is out of date. Should we make it so that by default, the site email address is filled in as the value for this setting, perhaps during the default core installer? If so, how (if at all) should we handle upgrades from D5? I guess we're not enabling update.module in that case anymore, but perhaps the warning message about enabling the module should mention it?

As per http://drupal.org/node/199685, the security newsletter is woefully under utilized, so it'd be nice to make drupal more secure out of the box by generating notification emails without the site admin having to hunt down this setting.

Short of the installer filling in the site email as the default, any other suggestions on how to make this setting more visible or more likely that site admins will use it? I hate to make the installer page too busy, but maybe another checkbox next to the one about opt in for "Send e-mail notification to the site email address when there are available updates"?

Maybe there should be a warning message printed on the available updates report if there are no email addresses configured for notification?

Just brain storming here, ideas welcome before someone tries to roll a patch. At this stage, talk is still gold. ;)

@Dries/Gabor: any hope this would land before the RC if someone comes up with an RTCB'ed patch? No sense wasting time on this if it's going to inevitably die in the end...

CommentFileSizeAuthor
#33 199774-31.automatic_update_emails.patch4.87 KBugerhard
#31 199774-31.automatic_update_emails.patch4.87 KBdww
#31 199774-31.automatic_update_emails.png40.76 KBdww
#29 199774-automatic_update_emails-rev5.patch5.09 KBbrianV
#29 199774-email-updates-ui-change.png34.22 KBbrianV
#21 email-updates1.png34.64 KBbrianV
#21 email-updates-unchecked.png28.75 KBbrianV
#19 199774-19.automatic_update_emails.patch4.73 KBdww
#16 199774-automatic-security-emails-rev3.patch4.84 KBbrianV
#15 199774-automatic-security-emails-rev2.patch4.8 KBbrianV
#9 199774-automatic-security-emails.patch1.62 KBbrianV
#9 email-updates.png37.95 KBbrianV
#7 install-notify-update.patch988 bytesJohn Morahan
#2 install-notify-update.patch845 bytesJohn Morahan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Version: 6.x-dev » 7.x-dev
John Morahan’s picture

John Morahan CreditAttribution: John Morahan commented
Status: Active » Needs review
FileSize
install-notify-update.patch845 bytes

We should definitely do something.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou CreditAttribution: lilou commented
Status: Needs work » Needs review

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

I missed this setting for a long time. Good idea, tests pass, RTBC.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work

Please add a code comment explaining (i) what it does and (ii) why we do this. :-)

John Morahan’s picture

John Morahan CreditAttribution: John Morahan commented
Status: Needs work » Needs review
FileSize
install-notify-update.patch988 bytes

Added a comment. I also changed it to use the administrator email instead of the site email, on the theory that if they are different, mail to the administrator is more likely to reach the right person.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

I think this is a good idea, but we need to change the help text to say we're going to send e-mails to the administrator - otherwise we'll probably get complaints.

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Needs work » Needs review
FileSize
email-updates.png37.95 KB
199774-automatic-security-emails.patch1.62 KB

Personally, I like having it in a selectable checkbox on the install page. I think it's an important security feature, but at the same time, I don't need it for most of the sites I build & maintain, so the ability to turn it off at install time is appealing.

To that end, I've put it into the install screen with help text. See the screenshot.

Patch for D7 attached.

brianV’s picture

brianV CreditAttribution: brianV commented
Issue tags: +Security improvements, +Usability

tagging

Status: Needs review » Needs work

The last submitted patch failed testing.

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Needs work » Needs review

Setting to needs review - testbot was temporarily broken by #361277: Remove the 'post settings' admin screen and relocate contents

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Needs work

Thanks for moving this forward. I was happy to see it show up in my issues page with a patch. ;)

Note: the variable you're setting isn't just about security updates. There's a related setting, the "E-mail notification threshold", which controls if the emails are sent anytime there are available updates, or just if there are security updates available. Therefore...

A) The help text in the installer is therefore wrong in this patch/screenshot, since the default for the notification threshold is for all updates.

B) I don't think we want to also ask about the notification threshold in the installer. But, perhaps we should mention it in the footer of the emails. Something like:

Your site is currently configured to send these emails when (any available updates|any security updates) are available. To get notified (only for security updates|for any available updates), please visit [url for settings page on this site]

?

C) Also, perhaps there should be a wee bit of JS to hide the email setting whenever the "Check for updates automatically" setting is unselected?

brianV’s picture

brianV CreditAttribution: brianV commented

Your suggestions sound good. I will reroll a patch later with the suggested changes.

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Needs work » Needs review
FileSize
199774-automatic-security-emails-rev2.patch4.8 KB

New patch attached, with everything recommended by dww in #13.

brianV’s picture

brianV CreditAttribution: brianV commented
FileSize
199774-automatic-security-emails-rev3.patch4.84 KB

oops - typo in that last patch. New one attached!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Let's get a screenshot of how this affects the installer, please.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Needs work

Looks good. Haven't tested (my laptop can no longer run D7 once dbtng landed and PDO is a requirement -- it's on my list to recompile everything!). My two lingering concerns:

D) (probably should move to a separate issue) Why don't we have system.installer.js or misc/installer.js or something for JS that's only used in the installer? Seems sort of a shame to put functions that are only ever invoked via the installer into system.js itself to bloat other pages. Luckily, system.js is *not* installed on every page load, so it's less of a concern, but still...

E) The translators are going to have a fit with this. This split assumes every language will work grammatically the same way as English. They're also going to complain that the $notification and $alternate fragments don't have enough context. I think what we really need is to just have two complete strings for the whole thing, and let the conditional select which complete string to use based on the current setting.

Moving to "needs work" for (E). (D) is really more a "needs review" question... ;)

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs work » Needs review
FileSize
199774-19.automatic_update_emails.patch4.73 KB

Fixed (E). Still wondering about (D).

@webchick: Screenshot (slightly stale wording, but the basic idea) is at http://drupal.org/files/issues/email-updates.png from comment #9.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

Side note: this JS stuff cries out for the '#dependency' goodness from CTools. ;)

brianV’s picture

brianV CreditAttribution: brianV commented
FileSize
email-updates-unchecked.png28.75 KB
email-updates1.png34.64 KB

@dww - I think (D) is something that should be done. However, I think that's a separate issue.

Good work on fixing the t() problems. I thought about that as I was writing it.... and promptly forgot about it. :p

@webchick - I am attaching two new screenshots (although the text may be *slightly* changed by dww's patch). One screenshot is with them both checked, the other is with the automatic updates box unchecked, and the new text stuff hidden.

Note that the JS unchecks the new checkbox as it is hidden, so it will always have an appropriate value when submitted.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Sorry, but it is not needed to show this as option. Put it away in some configuration somewhere, but not on the installer.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

The case has been made for why it needs to go in the installer, but I agree that this is tossing an awful lot of information at someone who doesn't even know what Drupal is yet. I think we need to at least:

1. Move update stuff to its own fieldset.
2. Shorten the description by at least 50 words. :P

Perhaps:

--- Update notifications ---

[ ] Check for updates automatically
[ ] Receive e-mail notifications

[description here]
dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Needs work

@Bojhan: Once upon a time I wanted update.module to be on by default, but people raised a stink about that it has to be opt-in and to put it on the installer (#178581: beta 3 breaker: update.module: opt-in, not opt-out). So, we can't remove all this from the installer without reopening that (thorny) issue. Furthermore, this other piece, the email notifications, is already an option somewhere. The point of this issue is that to enhance the security of sites, it's nice if this option is on by default...

@webchick: Sure, we could combine it into a single "question". I have no time for that today, so maybe BrianV can take another stab at it...

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

ugh, I am really not liking this trend. Why can't someone just install Drupal and do all the other stuff later. It makes no sense at all, there should be a sensible default on this and an easy changer.

brianV’s picture

brianV CreditAttribution: brianV commented

@dww: No time to take a stab at it today - gonna have to do it Monday, unless you beat me to it.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

@Bojhan: I completely agree. Please reopen #178581 and try to convince everyone there that the "sensible default" for this should be that update.module is enabled and it's automatically configured to send emails to uid 1's email address until configured otherwise. Good luck. ;)

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Spoke to webchick, I will just give up. Battling a security decision is rather useless, especially when you will also have to battle an opt-in discussion. Catch suggested doing something with the normal and minimal install profile, but that probally won't fly either.

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Needs work » Needs review
FileSize
199774-email-updates-ui-change.png34.22 KB
199774-automatic_update_emails-rev5.patch5.09 KB

Almost two months later, I finally got around to making another revision of this. It should satisfy webchick's suggestions in #23.

I've also attached a screenshot of the interface change. When the 'Check for updates automatically' checkbox is deselected, the 'Recieve e-mail notifications' checkbox is hidden via jQuery magic.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Needs work

Cool, thanks for re-rolling! Some minor visual gripes:

F) "Update Notifications" should be "Update notifications".

G) It's too bad we have the same title twice, once on the fieldset, once on the set of checkboxes. Maybe we can skip the #title on the checkboxes and just use the one from the fieldset?

Also, still wondering about (D) from #18. Anyone know of an issue to move installer-specific js out of system.js?

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs work » Needs review
FileSize
199774-31.automatic_update_emails.png40.76 KB
199774-31.automatic_update_emails.patch4.87 KB

Fixes (F) and (G). Tested and it works as advertised. This is RTBC as far as I'm concerned. (D) is clearly for a separate issue. Anyone else care to give this a look before it's really RTBC? Thanks.

ugerhard’s picture

ugerhard CreditAttribution: ugerhard commented
Status: Needs review » Needs work

It should be "receive", not "recieve" (see screenshot).

ugerhard’s picture

ugerhard CreditAttribution: ugerhard commented
Status: Needs work » Needs review
FileSize
199774-31.automatic_update_emails.patch4.87 KB

Corrected the typo

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Needs review » Reviewed & tested by the community

@dww - Nice - those are definite improvements. With respect to creating an installer.js, it may or may not be necessary. How much installer-specific JS is in the system.js?

@ugerhard - thanks for catching that!

I think this patch looks pretty good all around. I am going to RTBC it, since we have a bunch of people in favour of it, and only one person who doesn't like it.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Security improvements, -Usability

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Security improvements, +Usability

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.