search

jazzband / django-newsletter

An email newsletter application for the Django web application framework, including an extended admin interface, web (un)subscription, dynamic e-mail templates, an archive and HTML email support.
GNU Affero General Public License v3.0
846 stars 204 forks source link

Subscriber can change email address without confirmation #108

Closed kieronb closed 6 years ago

kieronb commented 10 years ago

When NEWSLETTER_CONFIRM_EMAIL is True (default), a subscriber must confirm subscription by clicking on an activation link in an email.

A user can change their email address without confirmation by receiving an update URL via email, accessing the form and changing the email address. I think that either: a) this field should not be allowed to be changed or b) this should trigger another confirmation email to be sent to the new address

Otherwise, a user can subscribe others to the newsletter without authorization.

EDIT: This is also possible during the initial subscription process. 1) Submit email address 2) Receive email, follow activation link 3) Enter different email address 4) Submit

dokterbob commented 10 years ago

Very good point. This should be taken seriously.

Recently, I discovered It's Dangerous, which would allow us to sign the URL's and would allow us to do away with storing keys in the database. Does that seem like a sensible approach to you?

kieronb commented 10 years ago

I'm happy to answer your question, but I should warn you that my use case may be a little different than most users, as my first priority is compliance with some fairly strict anti-spam legislation that has just been introduced. The sensible approach for me is the one which best ensures that no one can end up as a confirmed subscriber without having requested it themselves.

These are the pros of django-newsletter as I see them:

If "It's Dangerous" offers benefits without impacting the points above, then it sounds sensible to me. In my case, though, I'd sooner lose the ability for a recipient to change their email address at all, and instead make them unsubscribe address 1 and subscribe address 2, than to compromise in other areas. However, this would be a degraded user experience, and may not sit well with most of your user base.

With that said, I'm not sure that it has to be one or the other. I think re-triggering the confirmation process when a user requests to change their email address would be sufficient. When accessing the "activate" URL, I don't think the email address field is required, as the activation code would be unique to each user, and the email address could be gleaned from this database record.

dokterbob commented 10 years ago

Thanks for the feedback. As I see it, any possible security issues should have priority. Removing the update mechanism will still allow people to potentially unsubscribe other users, hence I think I'd like to go with the signed URL option.

If you are able to work on this it'd be great, I'd love to merge it in. I myself am rather time contrained at the moment (but I think I can make some time to review any patch and put out a new release).

dokterbob commented 9 years ago

I think we should really have this fixed in the upcoming 0.6 release.

dokterbob commented 9 years ago

Ref: https://docs.djangoproject.com/en/1.4/topics/signing/

Proposed implementation: stateless (i.e. not storing activation code), preventing replays by using existing email address in salt, similar to Django's builtin password reset token.

Proper security requires a single authentication secret in the URL for (un)subscribe and one for the old email and the new for updates.

Notes:

robert-kisteleki commented 8 years ago

What's the reason for presenting a form for this? The way I see it, the logic could be as simple as to check the token presented (checking if the email address matches that is really optional!) and if found, su/unsub as needed. This is simple and secure, as long as the token is unpredictable (signing, randomness or hmac).

One can "change" the address by subscribing with a new address and unsubscribing with the old.

I'm willing to send a PR to this extent, if that helps ;-)

dokterbob commented 8 years ago

I'm not sure if the form is necessary per-se but let me think about it for a bit. However, I do really like to have a POST for the confirmation. This could be an automatic POST generated by JS.

At the time I modelled the interface after other mailing list packages (that I deemed were well-designed). And still, if you look at MailChimp today they do in fact have some kind of form for unsubscribing, which is in fact automatically posted.

I'm not sure if they list the email address and I'm not sure whether this is necessary. I'll look at it again tomorrow, with less of a hangover, to see if there are any reasons against your suggested approach.

dokterbob commented 8 years ago

I've changed the milestone to 0.7 - to be released ASAP after 0.6 but if I get some code before releasing 0.6 some time next week I'd still love to ship it with 0.6.

robert-kisteleki commented 8 years ago

Cool. Note that this makes the update function obsolete, which I guess makes the code cleaner too.

I personally don't see a good reason to forcing a POST, a GET is fine as well as long as it's idempotent (which it would be). Being RESTful is nice but one shouldn't overdo it, IMO. Also, it would not work with JS turned off.

dokterbob commented 8 years ago

I thought about it for a second. I guess it makes sense. Less features and equivalent functionality sounds like a big win to me.

In the process, we could also do away with the distinction between ‘User’ subscribers and normal subscribers with regards to unsubscribing and/or updating (currently users have to login first to be able to unsubscribe - yielding yet another information leak).

Then again, I think we should support some kind of flow where a user unsubscribes with one address and subscribes with another. This allows to have an ‘update email’ link for better user experience AND makes it much easier to maintain the list of subscribers current. Plus it would provide better insight in subscriptions.

I guess, within this context, the optimal flow would be to allow direct subscription / unsubscription using a signed link. Moreover, we should be able to include an update link on the bottom of emails that lead users straight to a subscribe form which would generate a slightly different update link. As the previous address has already been confirmed through the reception of the initial activation link this should work.

To clarify, the flows would be as follows:

Subscribe non-User, email not already subscribed to

  1. Fill in email form, submit
  2. NO database changes
  3. Signed subscription link emailed
  4. Subscribe link click
  5. Subscription added
  6. Confirmation view

Subscribe as User (email already subscribed irrelevant here)

No changes here: form listing newsletters and subscribe status - no confirmation necessary

Subscribe non-user email already subscribed to

User-facing part of normal subscribe flow, though no database changes should take place.

Unsubscribe non-User

  1. User clicks signed subscribe link
  2. User unsubscribed in DB
  3. Confirmation view

Unsubscribe User

Same as non-User flow.

Update non-User

  1. User clicks update link
  2. Update form
  3. NO database changes
  4. Signed update link emailed
  5. Link clicked
  6. Subscription updated in database
  7. Confirmation view

The user experience and a large part of the code, except for the update form and an ‘existing email’ field in the link or something similar, should overlap with the ordinary subscribe view.

Update User

  1. User click update link
  2. Redirect to profile URL (including requiring login)
  3. Everything is handled by whatever is handling the profile from there on

I think we have all user interactions covered now. I hope you would still like to work on this one and I hope you see my point in maintaining the update flow. If its any help perhaps we could split the ticket up so we can collaborate, though due to the large overlap in features it might be easier to have a single person implement this.

robert-kisteleki commented 8 years ago

TBH I don't know much about the update procedure (which as I said should go, IMO) or the logged-in-user part. I was volunteering for something simpler :)

dokterbob commented 6 years ago

There was some old WIP on this in https://github.com/dokterbob/django-newsletter/tree/confirmation_fix. It's not finished but I pushed it so that whoever feels like it might finish the job.