Skip to content

Implement publish notifications opt-out #9359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Aug 29, 2024

It looks like some people are rather unhappy about the new publish notifications introduced in #9341 (see https://rust-lang.zulipchat.com/#narrow/stream/318791-t-crates-io/topic/publish.20notification.20emails/near/465824072 and #9355).

This PR adds a per-user settings flag to opt-out of these notifications:

Bildschirmfoto 2024-09-03 um 17 40 57

This PR is best reviewed commit by commit 😉

Resolves #9355

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Aug 29, 2024
@Turbo87 Turbo87 force-pushed the publish-notifications-opt-out branch from c5bc766 to 520453d Compare August 29, 2024 16:27
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 97.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.09%. Comparing base (cdb554c) to head (fc92785).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
src/controllers/user/update.rs 95.16% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9359      +/-   ##
==========================================
+ Coverage   89.06%   89.09%   +0.03%     
==========================================
  Files         285      286       +1     
  Lines       28938    29032      +94     
==========================================
+ Hits        25773    25867      +94     
  Misses       3165     3165              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Turbo87 Turbo87 force-pushed the publish-notifications-opt-out branch 3 times, most recently from d4225c1 to 4a86805 Compare September 3, 2024 15:42
@Turbo87 Turbo87 requested a review from a team September 3, 2024 15:42
@Turbo87 Turbo87 marked this pull request as ready for review September 3, 2024 15:43
@epage
Copy link

epage commented Sep 3, 2024

I assume the email should be updated with an "Unsubscribe" link to make sure people are aware of this.

In general, I think enabled-by-default will make security worse, not better. First, the concern over the lack of an opt-out was raised about a year ago. In that discussion, I called out the security risks of spamming people with this. Even when "unsubscribe" links are provided, it seems users are more inclined to click the "spam" button than to unsubscribe. Email providers seem to cross-pollinate that information between accounts and start treating it as spam even if you never said it was. This means one user not wanting email notifications can effectively opt-out other users from getting these emails! A user who wants the emails is trusting in the system to receive them and think everything is ok because they haven't received one. It can be easy to overlook the lack of emails being sent when you do your own publishes, if you are in that scenario.

@abonander

This comment was marked as off-topic.

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 4, 2024

Without diminishing the pain this is causing. Why has this been such a large issue in our community when many other ecosystems do not provide an opt out? pypi, npn, and others do not have an opt out. what is different about the implementation or the community to cause the disparate impact?

A user who wants the emails is trusting in the system to receive them and think everything is ok because they haven't received one.

A non-distributed version of this problem is also a problem if we provide an opt out. An attacker can opt out then publish then opt back in so that the website still says your opted in but you never got the email for the publish.

@epage
Copy link

epage commented Sep 4, 2024

pypi, npn, and others do not have an opt out. what is different about the implementation or the community to cause the disparate impact?

As I brought up on zulip, I have no idea what policy PyPI uses but I've never received a notification for a publish to PyPI. I am a sole owner on a couple of packages and published one this morning.

We also shouldn't blindly copy from other ecosystems but see what lessons we can learn from them and adapt accordingly.

@pietroalbini
Copy link
Member

Without diminishing the pain this is causing. Why has this been such a large issue in our community when many other ecosystems do not provide an opt out? pypi, npn, and others do not have an opt out. what is different about the implementation or the community to cause the disparate impact?

At least what makes me really want this feature is the "owner accounts" pattern that is popular in crates from large organizations. For example, I receive notifications from @rust-lang-owner, and that means every monday I get ~35 emails about rust-analyzer being published, or ~10 emails when cargo gets published after a new release, and random emails whenever a CI workflow somewhere in the rust-lang org publishes a crate.

Those emails are pure noise, because I am not involved in any way about those crate publishes, and I can't verify whether those were intentional or not.

A non-distributed version of this problem is also a problem if we provide an opt out. An attacker can opt out then publish then opt back in so that the website still says your opted in but you never got the email for the publish.

We can have the website send an email whenever notifications are disabled and enabled, to ensure there are at least some mails receive. We can make that "notifications disabled" email as scary as it needs to be.

@workingjubilee

This comment was marked as off-topic.

@workingjubilee

This comment was marked as off-topic.

@Turbo87
Copy link
Member Author

Turbo87 commented Sep 11, 2024

@rust-lang/crates-io this still needs a review... 😉

Copy link
Contributor

@eth3lbert eth3lbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some minor feedback.

This implementation focuses on user-level settings. I'm unsure if anyone needs more granular settings like crate-level settings, but for now, this LGTM 👍

This field can be used to check if the logged in user is currently subscribed to publish notifications.
Currently only the `email` field is supported, but that is about to change. Unfortunately with `serde_json` it is hard to differentiate between `email: null` and the `email` field not existing, since both are decoded as `Option::None`. Since our frontend does not send `email: null` and the endpoint does not appear to be used via token authentication this seems like a viable change.

Side note: This endpoint is behaving like a `PATCH` endpoint, but is currently using the `PUT` HTTP method for legacy reasons.
This field can be used to un-/resubscribe to publish notifications. When a user unsubscribes they get an email notification.
@Turbo87 Turbo87 force-pushed the publish-notifications-opt-out branch from 4a86805 to 1fb1163 Compare September 12, 2024 15:28
@Turbo87 Turbo87 merged commit 3e0da33 into rust-lang:main Sep 18, 2024
11 checks passed
@Turbo87 Turbo87 deleted the publish-notifications-opt-out branch September 18, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Publish email notification can not be turned off, and are not rolled up
7 participants