Allow access to unsubscribe links without subscribe permission

I vote against this patch as it is an edge case. Further it is not so easy as it seams since simplenews uses combined subscribe and unsubscribe forms. You want to prevent people from unchecking a checkbox.

I refer to the form at /newsletter/subscriptions and at /user/nid/edit/newsletter. You only take care of anonymous users.

I'm looking for a solution to this as well. Though in my instance I don't want to allow people to subscribe to newsletters generically because I have a few of them that I only add people to manually.

So an alternate fix for me would be to allow some kind of per newsletter permissioning.

rsevero: your patch is very similar to the one proposed in #776370: Unsubscribe link fails for non-administrators so it looks like you're going to get your way.
The patch provided there needs more polishing and testing, so your contribution would be a great help.

To make this patch more complete, perhaps it could rename the existing newsletter/confirm menu entry to newsletter/confirm/add, so that we can let more of the 404s be generated by drupal's menu system rather than manually in simplenews_confirm_subscription().

Also, the patch should remove the ``// @todo: open this permission -check later - allow unsubscribe'' comment from the newsletter/confirm menu entry as this patch _does_ that TODO.

I think that any newly created permission should be named 'unsubscribe from newsletters'. This appears somewhat more consistent with the existing 'subscribe to newsletters' permission. However, is such a permission even needed? Isn't it bad practice to disallow someone from unsubscribing from a mailing list? ideally, such a permission shouldn't exist.

However, requiring users to log in to unsubscribe (the current situation) does protect against one possible problem: the hash used to prove that the user is the one who received the particular email is both truncated from an MD5 and is always the same hash that user uses. Thus, if someone finds out someone's subscribe/unsubscribe hash and unsubscribing didn't require logging in, one could maliciously unsubscribe a certain user from the mailing list continuously when the user wants to remain subscribed. But for now, I think that getting anonymous unsubscriptions is much more important than this concern...

Here's my take on the patch, simplifying its implementation and having an 'unsubscribe from newsletters' permission instead of 'opt out from newsletter' permission. This would allow an admin to give the anonymous user the 'unsubscribe from newsletters' permission and thus have a correct mailing list setup.

Just wanted to refer to issues that are somehow related to this discussion:
#776370: Unsubscribe link fails for non-administrators
#927072: Unsubscribe link does not work
#891870: Unsubscribing doesn't work

And there have been many more duplicates.
I consider the unsubscribe issue a blocker for 6.x-2.x. Please help us make this work seamless somehow.
I'll go through all issues and provide my updated opinion soon (again).

Anyway, simply adding two callbacks is by far not enough. (think about the unpermitted links still appearing in mails and much more to come...)

Duplicated by:
#966706: Allow anonymous users to opt-out to authenticated newsletters

Also switching this to 6.x-2.x

@Miro, since your commit from #776370: Unsubscribe link fails for non-administrators, there is no permission needed any more to unsubscribe, so I guess we can close this one too, what do you think ?

I think we should leave this open... but push it to D7 queue.

I'd like to see a much more versatile permission setup in future releases.

Setting to active as we need to rethink the whole process then.

I'm also thinking that unsubscribe should always be possible as it's even legally required to be able to do so in many countries.

*If* it's like that currently, then I think it's safe to set this to low and look at this later..

I agree this setting is needed.

My client adds hundreds of names via the import function, but legally these people still need to unsubscribe. However if I give anonymous users the permission to unsubscribe, then I get spammed by bots subscribing. Now that Google has stopped backlinks for search ranking, hopefully all of us will get less spam!

This is super important. *Nobody* on the web should be forced to sign into an account in order to unsubscribe from a newsletter.

Alternative solution proposal #2063847: Improve unsubscription, bounce handling & their integration

Integrate with the Unsubscribe module for a global solution.

@Berdir

*If* it's like that currently, then I think it's safe to set this to low and look at this later..

However it is not like that currently. The URL /newsletter/confirm/remove/XXX is governed by the route simplenews.newsletter_confirm_subscription and requires permission 'subscribe to newsletters'.

@Berdir

I'm also thinking that unsubscribe should always be possible as it's even legally required to be able to do so in many countries.

Often that's true I agree. However I believe it is also permissible by law to say that users will receive certain emails as a condition of membership, in which case opt out is not allowed (the user would have to cancel membership). Hidden emails already don't have an unsubscribe link. So some sites might need more detailed control over access to unsubscribe.

This issue seems closely related to #1140606: Permissions Per-Newsletter: Limit subscription options by role which discusses detailed control over access to subscribe.

Another aspect: we probably shouldn't insert an unsubscribe link into mails to users that won't have permission to use it.

Sadly I can confirm this issue still exists in Drupal 7. Should we perhaps set this back to 7.x and use #1140606: Permissions Per-Newsletter: Limit subscription options by role to document the requirement for separate permissions in Drupal 8 per newsletter?

Has anyone found a workaround or written a patch in the meantime for D7 we could use as starting point here?

@Anybody Although the two issues are related I think there are two different cases and so it's worth keeping both open for D8 please.

My feeling is that on Drupal.org we generally allow one issue to cover both D7 and D8. We would most likely fix it in D8 first, then possibly backport if if someone is keen to provide a patch.

Thank you for your quick reply, AdamPS!
I've written a quick and dirty patch for Drupal 7.

It does:
Provide a new permission: Unsubscribe from newsletters
Add a separate menu hook item for newsletter/confirm/remove:

$items['newsletter/confirm/remove'] = array(
    'title' => 'Confirm newsletter unsubscriptions',
    'type' => MENU_CALLBACK,
    'page callback' => 'simplenews_confirm_subscription',
    'page arguments' => array(2),
    'access arguments' => array('unsubscribe from newsletters'),
    'file' => 'includes/simplenews.subscription.inc',
  );

It doesn't yet:
Add the set permissions for the roles from the "Subscribe to newsletters" permission

We can use this as starting point for Drupal 7 perhaps... I just needed a quick and dirty solution. Perhaps it may help others too.

Patch attached, sorry.

Patch in #26 is vulnerable to CSRF and needs a token in the callback path.

There's some really minimal documentation at https://www.drupal.org/docs/7/security/writing-secure-code/create-forms-...

Regardless, as @AdamPS said in #24, this should be fixed in D8 first then backported if there is interest.

The latest comments prompted me to think about this issue again. Here is my latest suggestion:

If a user clicks the unsubscribe link in an email footer, then it must always work - there should be no need to log in and no check against permissions. Therefore we should remove the access check on the URL /newsletter/confirm/remove/XXX is governed by the route simplenews.newsletter_confirm_subscription. The fact that someone received the email in their inbox is sufficient verification.

No other changes needed:

• The permission 'subscribe to newsletters' is still checked for any attempt to subscribe or unsubscribe using forms on the website.
• Hidden newsletters can't be subscribed or unsubscribed by any means and don't have an unsubscribe link in the email footer.

So it could be a 1-line fix - plus tests of course.

@AdamPS: I completely agree! And we should fix it in D8 and D7 then... both versions are used very widely. What do you think about the #27 CSRF risk?

Here is a initial D8 patch for review please (without tests). I have removed access checks on all confirmation pages. My theory is that a user can only get a valid token if they are entitled to access, either

• using a form (requires the right permission)
• from an unsubscribe link (they have access to the subscribed email account)

@Anybody I'm not a maintainer for D7, only D8, but I agree with @TR. The D8 codebase has the correct code including tokens in simplenews.routing.yml. Potentially the backport will require adding similar code back to D7. However please let's try and get D8 finished before starting the D7 discussion.

Yes, that will work for D8, but instead of doing all the {hash} generation and checking explicitly, D8 supports using csrf tokens in the route, which will do this for you. That will simplify the code a bit.

No, standard csrf tokens can't replace our hashes, the csrf token is based on a session, our tokens are put in newsletters for all users who receive it, they are very different how they work.

Ah, OK. thanks for the clarification.

@Berdir this is important security question so please can you comment whether the solution of this patch looks good? If you agree then I will write a quick test then we can commit. This is a non-BC change so it seems worth getting in before we release the first 2.x beta.

Great great news, thank you very much for your work @AdamPS! :)

BTW should we backport this to Drupal 7?

Thanks @Anybody. My view is that now it's so close to the end of D7, it's not worth backporting anything except perhaps major bug fixes. However I am only maintaining D8 so it's not up to me. But I believe the D7 maintainers are very busy people:-)