The newsletter subscription form on the user registration page currently subscribes newsletters without verifying the email address (as an anonymous user would be). The problem with this is that at registration time the user has not yet verified the email address for the new account so it would be possible for anyone to subscribe ANY email address to the available newsletters with no verification whatsoever.

The attached patch sends the confirmation email. A more user friendly solution may be to delay the subscription until the user has verified their account email.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Petra’s picture

Many thanks!!
Not good to send newsletters to unverified accounts.

Sutharsan’s picture

Assigned: Unassigned » Sutharsan

This may be a temporary solution, but a delayed subscription is more user friendly.

Sutharsan’s picture

Title: Subscribe to newsletter at registration time » Security: Newsletters should not subscribe without email verification at registration time
Category: feature » bug
Priority: Normal » Critical
FileSize
3.7 KB

I made a patch that delays the subscription until the user has logged in. Please test and comment.
Patch is made against 5.x-1.x-dev.
In this patch there is no permission checks used, I'll explain why and like to have some discussion on this part.

There are two point at which permission check can be applied:
1. when presenting the subscription form (as part of the registration phase. The user is now anonymous.
2. when performing the actual subscription, this is after the fist login. The user is now authenticated

A. Permission check at point 2 must be combined with the same permission check at point 1. It is useless to present the user a form which can not be processed at a later stage. However the roles a point 1 and 2 are different. I need to bypass the standard permission check to do this, not very elegant.

B. In the Simplenews settings there is a checkbox to enable/disable the subscription during registration. Admin can select whether the user will see the newsletter subscription as part of the registration form.

A and B are both forms of access control. A is spreadout over two roles; B always applies to a subscribing user.

My question to Simplenews admin's: do you require permission check (A) for newsletter subscription during registration or is the enable/disable (B) option enough?

Sutharsan’s picture

Version: 7.x-1.x-dev » 5.x-1.x-dev
Assigned: Sutharsan » Unassigned
Status: Active » Needs review

Before this patch can go in it needs to be tested by several people. Currently I want to get a new stable release out. But if this patch is not tested sufficiently I will remove the complete subscription at registration time functionality and it will not get into the next stable. Therefore

Please help to get the subscription at registration functionality in and test the patch.

Sutharsan’s picture

Title: Security: Newsletters should not subscribe without email verification at registration time » Subscribe to newsletter at registration time
FileSize
4.43 KB

I have removed the the functionality for subscription at registration time from HEAD and 5.x-1.x-dev. I can not let this unfinished discussion stand in the way of a new stable release and Drupal-6 development of Simplenews module. The attached patch contains the removed code for future inclusion into the module.

Title of the issue changed for the changed situation.

Sutharsan’s picture

Category: bug » feature
Priority: Critical » Normal
yngens’s picture

Title: Security: Newsletters should not subscribe without email verification at registration time » Subscribe to newsletter at registration time
Category: bug » feature
Priority: Critical » Normal

patching file simplenews.module
Hunk #1 FAILED at 476.
Hunk #2 succeeded at 497 (offset -7 lines).
1 out of 2 hunks FAILED -- saving rejects to file simplenews.module.rej

please fix the patch, i really need this feature

gostram’s picture

Is the patch operational now? It would be nice to have this feature indeed.

Sutharsan’s picture

FileSize
4.36 KB

patch re-rolled against 5.x-1.x-dev.

yngens’s picture

unfortunately, this patch also gives an error:

can't find file to patch at input line 8
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: simplenews.module
|===================================================================
|RCS file: /cvs/drupal-contrib/contributions/modules/simplenews/simplenews.module,v
|retrieving revision 1.48.2.18
|diff -u -p -r1.48.2.18 simplenews.module
|--- simplenews.module  17 Dec 2007 19:28:20 -0000      1.48.2.18
|+++ simplenews.module  8 Jan 2008 23:02:42 -0000
--------------------------

Sutharsan’s picture

I see nothing wrong with the patch.

yngens’s picture

Probably, I did smth wrong previously. I confirm the last patch works correctly. Thank you, Sutharsan!

bchoc’s picture

I've applied the patch and so far so good. I'm glad to see this function return.

Christefano-oldaccount’s picture

This feature has been greatly missed. Thanks, Sutharsan! I hope this patch gets enough testing and feedback this time.

It seems that applying this patch makes one of the old user module bugs reappear. Even though no authentication modules are enabled, I now have the "Note: if you have an account with one of our affiliates (), you may login now instead of registering." message on the user registration page.

Sutharsan’s picture

Perhaps I was not clear enough. You users of simplenews asking for this functionality, you are the ones that need to test the module and comment of the functionality.
Thanks in advance.

Christefano-oldaccount’s picture

Status: Needs review » Needs work

Sutharsan, are you directing that comment to me? I am testing the patch and providing feedback, so I'm not sure if you're singling me out.

I think the patch needs work. The "Note: if you have an account with one of our affiliates" message appears when Simplenews is patched with #9 and then enabled. I've looked through the code that displays the form during registration (lines 577-605) and didn't see anything funny.

bluecobalt’s picture

Hi Sutharsan,

Yes, thank you much. This is a much needed function on several client sites that I am working on.

The patch in #9 applies cleanly for me to the latest 5.x-1.x-dev, the option to choose a default subscription newsletter shows up under the general settings, but new registrations are not automatically subscribed to the newsletter that is chosen.

thanks,
blue

Sutharsan’s picture

@christefano, I did not understand that you were testing the module. Sometimes I get frustated because there is hardly anyone who is willing to test and report back. Sorry for my misunderstanding and thank you for testing.

The patch at #9 is a wrong patch, it subscribes before the user uses confirms the creation of the account. It should have been more like #3, but that needs work too.
I will come back with a new patch.

scedwar’s picture

I had been using the original release without problems. With 5.x-1.2 I'm now without this very useful functionality - I'm keen to help with testing when the patch is updated. Thanks for all your work on this.

jpsalter’s picture

The patch at #9 applied with offsets to version 5.x-1.1 (what we're running currently) and works as advertised. Thanks for a great patch. Every client that I have who uses simplenews.module has asked for this functionality.

jonlesser’s picture

I was able to apply the patch referenced in #9 to 5.x-1.2 without any problems. So far it seems to be working. I'll leave another comment after I spend some time working with the patched version. Thanks for your work on a great module Sutharsan!

DanielJohnston’s picture

Hi there,
Just to add my voice, this would be an incredibly useful feature. I'm manually checking my site logs and subscribing new users at the moment. The majority of new users don't understand the interface enough to subscribe manually. I'm on Simplenews 5.x-1.2 at the moment. Do I need to update to 1.x-dev then install the patch using Patch -p0 in order to test this?

nvoyageur’s picture

Hi,

I used the #9 patch against Simplenews 5.x-1.2. It seems to work as desired. I am able to make my newsletter selected by default on the register page. You can check it out at https://www.wildpaddle.com/user/register

This of course is a much needed feature as most users will not take the extra effort to sign up for the newsletter. It's just simple Steve Krug "Don't Make Me Think" stuff.

wmostrey’s picture

Status: Needs work » Reviewed & tested by the community

Patch #9 deploys perfectly against -dev and this really is a much needed functionality. RTBC.

christefano’s picture

I'm leaning towards wmostrey's opinion but I'm concerned about the bug in user.module. In all fairness, though, I think it's too minor to hold up this patch and it's really a problem with user.module and not simplenews.

Sutharsan’s picture

Status: Reviewed & tested by the community » Needs work

I have to put the status of this issue back to 'code needs work'. I realize there is a high demand for this function, but the patch it is not ready yet. A user is subscribed before the email address is verified (it has been reported somewhere). This is not correct. It does not take a long time to correct this, but all my spare time is now taken by Simplenews for Drupal 6 development. So this has to wait, sorry.

Dave Cohen’s picture

How about something like the attached patch, which will not send a newsletter if the subscribed user has never accessed the site? This means users who subscribed upon registration but never visited the site will not receive newsletters. Presumably, if they've visited the site, the email they provided upon registration is valid.

Note that accounts created by administrators have the access column set for them, so these users are considered to have visited the site already.

Ignore the first chunk of the patch, it solves a different issue. The patch is against DRUPAL-5, and I haven't actually tested it yet, I just wanted to share the idea. It's meant to be in conjunction with the patch from #9.

Dave Cohen’s picture

Status: Needs work » Needs review
FileSize
5.32 KB

The patch #9 gave me errors regarding _simplenews_get_vid(). Here's a re-roll against DRUPAL-5, combined with my previous comment.

Sutharsan’s picture

Status: Needs review » Needs work

This is not my preferred kind of solution. There is a problem in one part of the code, but we solve it in an other... It is a creative solution but it changes the synchronization between user account records and subscription records, in fact it uses it.

What I am looking for as a solution is some kind of trigger that occurs during or just after first login with witch we can set the subscription.

scottrigby’s picture

When a solution for comment in #29 is proposed, I will gladly test. Thanks guys for all this hard work! -Scott

scedwar’s picture

Why not send an email at for Newsletter registration at the the time of user registration, pretending that the user isn't yet a user? Yes, the user will get two emails, but at least you can know whether it is verified or not.

I'm assuming this will work. If it doesn't this may illustrate another related problem: What happens if a user registers with a site but does not confirm their email address? They then (say a month later) subscribe to a mailing list using that same email address. Is the subscription associated with the unconfirmed user account? Or just registered as if there is no (unconfirmed) user account?

coltrane’s picture

What about not integrating on the register form? I'm thinking of a solution where users are signed up by default and only unsubscribe through their account rather than unselecting on the register form.

Here's a patch to implement newsletter subscription when the user logs in for the first time rather than on registration. The default signup newsletter is set under the subscription options of the main newsletter settings.

*edit* leftover watchdog() line in this patch, please check followup

coltrane’s picture

Status: Needs work » Needs review
FileSize
1.99 KB

Please disregard previous patch, I had left a watchdog() line in there.

holydrupal’s picture

is there any plan to integrate this patch in to module?

christefano’s picture

FYI, there's now a Simplenews Register module in contrib. I don't know how its existence affects this issue.

Sutharsan’s picture

If simplenews register modulle fullfills the needs (I have not tested it myself) this issue will be closed.

holydrupal’s picture

Version: 5.x-1.x-dev » 5.x-1.3

Thank you for your answer.
But the Simplenews Register module doesn't worked for me. after installation when I opened the simplenews setting page it returned a blank page!?

Sutharsan’s picture

Please file an issue in the issue queue of Simplenews Register: http://drupal.org/project/issues/Simplenews_Register

glass.dimly’s picture

Simplenews register, as far as I am aware, does not wait until email verification to subscribe.

Please correct me if I am wrong. I haven't checked this feature.

-jmjohn

PS patch for Simplenews Register here: http://drupal.org/node/261693 . Essentially, it fixes the same error that comment #28 had, _simplnews_get_vid function was elimimated in the new simplenews release.

marcoBauli’s picture

tried the latest patch of Simplenews_register at http://drupal.org/node/261693 and works just fine. This was so needed, great :)

burningdog’s picture

Status: Needs review » Closed (fixed)

Simplenews_register does indeed allow a user to to sign up for newsletter from the user registration page.

markivbagav’s picture

any one can help me..
how to list out subscriber email addtress in simple news module