Skip to content

Implement publish notification emails #9341

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 3 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

use crate::app::AppState;
use crate::auth::AuthCheck;
use crate::worker::jobs::{self, CheckTyposquat, UpdateDefaultVersion};
use crate::worker::jobs::{
self, CheckTyposquat, SendPublishNotificationsJob, UpdateDefaultVersion,
};
use axum::body::Bytes;
use axum::Json;
use cargo_manifest::{Dependency, DepsSet, TargetDepsSet};
Expand Down Expand Up @@ -442,6 +444,8 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra

jobs::enqueue_sync_to_index(&krate.name, conn)?;

SendPublishNotificationsJob::new(version.id).enqueue(conn)?;

// If this is a new version for an existing crate it is sufficient
// to update the default version asynchronously in a background job.
if inserted_default_versions == 0 {
Expand Down
2 changes: 2 additions & 0 deletions src/tests/krate/publish/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ async fn new_wrong_token() {
assert_eq!(response.status(), StatusCode::FORBIDDEN);
assert_snapshot!(response.text(), @r###"{"errors":[{"detail":"authentication failed"}]}"###);
assert_that!(app.stored_files().await, empty());
assert_that!(app.emails(), empty());
}

#[tokio::test(flavor = "multi_thread")]
Expand All @@ -49,4 +50,5 @@ async fn new_krate_wrong_user() {
assert_json_snapshot!(response.json());

assert_that!(app.stored_files().await, empty());
assert_that!(app.emails(), empty());
}
2 changes: 2 additions & 0 deletions src/tests/krate/publish/basics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ async fn new_krate() {
.unwrap();
assert_eq!(email, "foo@example.com");
});

assert_snapshot!(app.emails_snapshot());
}

#[tokio::test(flavor = "multi_thread")]
Expand Down
2 changes: 2 additions & 0 deletions src/tests/krate/publish/emails.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ async fn new_krate_without_any_email_fails() {
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
assert_json_snapshot!(response.json());
assert_that!(app.stored_files().await, empty());
assert_that!(app.emails(), empty());
}

#[tokio::test(flavor = "multi_thread")]
Expand All @@ -39,4 +40,5 @@ async fn new_krate_with_unverified_email_fails() {
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
assert_json_snapshot!(response.json());
assert_that!(app.stored_files().await, empty());
assert_that!(app.emails(), empty());
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
source: src/tests/krate/publish/basics.rs
expression: app.emails_snapshot()
---
To: foo@example.com
From: noreply@crates.io
Subject: crates.io: Successfully published foo_new@1.0.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello foo!

A new version of the package foo_new (1.0.0) was published by your account =
(https://crates.io/users/foo) at [0000-00-00T00:00:00Z].

If you have questions or security concerns, you can contact us at help@crat=
es.io.
2 changes: 2 additions & 0 deletions src/tests/owners.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ async fn new_crate_owner() {
.publish_crate(crate_to_publish)
.await
.good();

assert_snapshot!(app.emails_snapshot());
}

async fn create_and_add_owner(
Expand Down
61 changes: 61 additions & 0 deletions src/tests/snapshots/all__owners__new_crate_owner-2.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
---
source: src/tests/owners.rs
expression: app.emails_snapshot()
---
To: foo@example.com
From: noreply@crates.io
Subject: crates.io: Successfully published foo_owner@1.0.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello foo!

A new version of the package foo_owner (1.0.0) was published by your accoun=
t (https://crates.io/users/foo) at [0000-00-00T00:00:00Z].

If you have questions or security concerns, you can contact us at help@crat=
es.io.
----------------------------------------

To: Bar@example.com
From: noreply@crates.io
Subject: crates.io: Ownership invitation for "foo_owner"
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

foo has invited you to become an owner of the crate foo_owner!

Visit https://crates.io/accept-invite/[invite-token] to accept =
this invitation,
or go to https://crates.io/me/pending-invites to manage all of your crate o=
wnership invitations.
----------------------------------------

To: foo@example.com
From: noreply@crates.io
Subject: crates.io: Successfully published foo_owner@2.0.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello foo!

A new version of the package foo_owner (2.0.0) was published by Bar (https:=
//crates.io/users/Bar) at [0000-00-00T00:00:00Z].

If you have questions or security concerns, you can contact us at help@crat=
es.io.
----------------------------------------

To: Bar@example.com
From: noreply@crates.io
Subject: crates.io: Successfully published foo_owner@2.0.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello Bar!

A new version of the package foo_owner (2.0.0) was published by your accoun=
t (https://crates.io/users/Bar) at [0000-00-00T00:00:00Z].

If you have questions or security concerns, you can contact us at help@crat=
es.io.
15 changes: 15 additions & 0 deletions src/tests/snapshots/all__owners__new_crate_owner.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,21 @@
source: src/tests/owners.rs
expression: app.emails_snapshot()
---
To: foo@example.com
From: noreply@crates.io
Subject: crates.io: Successfully published foo_owner@1.0.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello foo!

A new version of the package foo_owner (1.0.0) was published by your accoun=
t (https://crates.io/users/foo) at [0000-00-00T00:00:00Z].

If you have questions or security concerns, you can contact us at help@crat=
es.io.
----------------------------------------

To: Bar@example.com
From: noreply@crates.io
Subject: crates.io: Ownership invitation for "foo_owner"
Expand Down
17 changes: 17 additions & 0 deletions src/tests/snapshots/all__team__publish_owned.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
source: src/tests/team.rs
expression: app.emails_snapshot()
---
To: user-all-teams@example.com
From: noreply@crates.io
Subject: crates.io: Successfully published foo_team_owned@2.0.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello user-all-teams!

A new version of the package foo_team_owned (2.0.0) was published by user-o=
ne-team (https://crates.io/users/user-one-team) at [0000-00-00T00:00:00Z].

If you have questions or security concerns, you can contact us at help@crat=
es.io.
3 changes: 3 additions & 0 deletions src/tests/team.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crates_io::{

use diesel::*;
use http::StatusCode;
use insta::assert_snapshot;

impl crate::util::MockAnonymousUser {
/// List the team owners of the specified crate.
Expand Down Expand Up @@ -399,6 +400,8 @@ async fn publish_owned() {
.publish_crate(crate_to_publish)
.await
.good();

assert_snapshot!(app.emails_snapshot());
}

/// Test trying to change owners (when only on an owning team)
Expand Down
4 changes: 4 additions & 0 deletions src/tests/util/test_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@ impl TestApp {
static EMAIL_HEADER_REGEX: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"(Message-ID|Date): [^\r\n]+\r\n").unwrap());

static DATE_TIME_REGEX: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z").unwrap());

static INVITE_TOKEN_REGEX: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"/accept-invite/\w+").unwrap());

Expand All @@ -186,6 +189,7 @@ impl TestApp {
.into_iter()
.map(|email| {
let email = EMAIL_HEADER_REGEX.replace_all(&email, "");
let email = DATE_TIME_REGEX.replace_all(&email, "[0000-00-00T00:00:00Z]");
let email = INVITE_TOKEN_REGEX.replace_all(&email, "/accept-invite/[invite-token]");
email.to_string()
})
Expand Down
2 changes: 2 additions & 0 deletions src/worker/jobs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ mod git;
mod index_version_downloads_archive;
mod readmes;
pub mod rss;
mod send_publish_notifications;
mod sync_admins;
mod typosquat;
mod update_default_version;
Expand All @@ -29,6 +30,7 @@ pub use self::expiry_notification::SendTokenExpiryNotifications;
pub use self::git::{NormalizeIndex, SquashIndex, SyncToGitIndex, SyncToSparseIndex};
pub use self::index_version_downloads_archive::IndexVersionDownloadsArchive;
pub use self::readmes::RenderAndUploadReadme;
pub use self::send_publish_notifications::SendPublishNotificationsJob;
pub use self::sync_admins::SyncAdmins;
pub use self::typosquat::CheckTyposquat;
pub use self::update_default_version::UpdateDefaultVersion;
Expand Down
162 changes: 162 additions & 0 deletions src/worker/jobs/send_publish_notifications.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
use crate::email::Email;
use crate::models::OwnerKind;
use crate::schema::{crate_owners, crates, emails, users, versions};
use crate::tasks::spawn_blocking;
use crate::worker::Environment;
use anyhow::anyhow;
use chrono::{NaiveDateTime, SecondsFormat};
use crates_io_worker::BackgroundJob;
use diesel::prelude::*;
use diesel_async::{AsyncPgConnection, RunQueryDsl};
use std::sync::Arc;

/// Background job that sends email notifications to all crate owners when a
/// new crate version is published.
#[derive(Serialize, Deserialize)]
pub struct SendPublishNotificationsJob {
version_id: i32,
}

impl SendPublishNotificationsJob {
pub fn new(version_id: i32) -> Self {
Self { version_id }
}
}

impl BackgroundJob for SendPublishNotificationsJob {
const JOB_NAME: &'static str = "send_publish_notifications";

type Context = Arc<Environment>;

async fn run(&self, ctx: Self::Context) -> anyhow::Result<()> {
let mut conn = ctx.deadpool.get().await?;

// Get crate name, version and other publish details
let publish_details = PublishDetails::for_version(self.version_id, &mut conn).await?;

let publish_time = publish_details
.publish_time
.and_utc()
.to_rfc3339_opts(SecondsFormat::Secs, true);

// Find names and email addresses of all crate owners
let recipients = crate_owners::table
.filter(crate_owners::deleted.eq(false))
.filter(crate_owners::owner_kind.eq(OwnerKind::User))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also be looking up team owners?

Copy link
Member Author

Choose a reason for hiding this comment

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

we should, I'm just not sure that we can... :D

our publishing code currently performs a "get all owner teams, iterate through teams and query GitHub API to see if publisher is member of the team" operation, but that does not list the members of these teams. in other words: we'd need new GitHub API calls since we don't "cache" the team memberships on our side. if/when we have non-GitHub teams implemented this will probably become a lot easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, I forgot that we don't cache that. Sorry, some Sourcegraph slipped through there.

Not blocking for this PR, but I'm wondering if it's worth the effort to figure that1 out sooner rather than later — given that part of the motivation here (at least from what I see) is supply chain security, not sending notifications for crates that are entirely owned by teams (such as the AWS crate ecosystem) feels problematic.

I'll give this some thought.

Footnotes

  1. For clarity, I'm more thinking just some level of caching here; not so much a full blown decoupled-from-GitHub team design.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if it's worth the effort to figure that out sooner rather than later

it then becomes a problem of cache invalidation. I think I'd personally rather see a proper teams concept in crates.io with a feature to optionally keep the team memberships in sync with some GitHub team. Pietro mentioned a while ago that this sync might be possible by having people install a GitHub app (?) in their orgs, but I haven't looked at any of this in detail yet.

.filter(crate_owners::crate_id.eq(publish_details.crate_id))
.inner_join(users::table)
.inner_join(emails::table.on(users::id.eq(emails::user_id)))
.filter(emails::verified.eq(true))
.select((users::gh_login, emails::email))
.load::<(String, String)>(&mut conn)
.await?;

// Sending emails is currently a blocking operation, so we have to use
// `spawn_blocking()` to run it in a separate thread.
spawn_blocking(move || {
let results = recipients
.into_iter()
.map(|(ref recipient, email_address)| {
let krate = &publish_details.krate;
let version = &publish_details.version;

let publisher_info = match &publish_details.publisher {
Some(publisher) if publisher == recipient => &format!(
" by your account (https://{domain}/users/{publisher})",
domain = ctx.config.domain_name
),
Some(publisher) => &format!(
" by {publisher} (https://{domain}/users/{publisher})",
domain = ctx.config.domain_name
),
None => "",
};

let email = PublishNotificationEmail {
recipient,
krate,
version,
publish_time: &publish_time,
publisher_info,
};

ctx.emails.send(&email_address, email).inspect_err(|err| {
warn!("Failed to send publish notification for {krate}@{version} to {email_address}: {err}")
})
})
.collect::<Vec<_>>();

// Check if any of the emails succeeded to send, in which case we
// consider the job successful enough and not worth retrying.
match results.iter().any(|result| result.is_ok()) {
true => Ok(()),
false => Err(anyhow!("Failed to send publish notifications")),
}
})
.await?;

Ok(())
}
}

#[derive(Debug, Queryable, Selectable)]
struct PublishDetails {
#[diesel(select_expression = crates::columns::id)]
crate_id: i32,
#[diesel(select_expression = crates::columns::name)]
krate: String,
#[diesel(select_expression = versions::columns::num)]
version: String,
#[diesel(select_expression = versions::columns::created_at)]
publish_time: NaiveDateTime,
#[diesel(select_expression = users::columns::gh_login.nullable())]
publisher: Option<String>,
}

impl PublishDetails {
async fn for_version(version_id: i32, conn: &mut AsyncPgConnection) -> QueryResult<Self> {
versions::table
.find(version_id)
.inner_join(crates::table)
.left_join(users::table)
.select(PublishDetails::as_select())
.first(conn)
.await
}
}

/// Email template for notifying crate owners about a new crate version
/// being published.
#[derive(Debug, Clone)]
struct PublishNotificationEmail<'a> {
recipient: &'a str,
krate: &'a str,
version: &'a str,
publish_time: &'a str,
publisher_info: &'a str,
}

impl Email for PublishNotificationEmail<'_> {
fn subject(&self) -> String {
let Self { krate, version, .. } = self;
format!("crates.io: Successfully published {krate}@{version}")
}

fn body(&self) -> String {
let Self {
recipient,
krate,
version,
publish_time,
publisher_info,
} = self;

format!(
"Hello {recipient}!

A new version of the package {krate} ({version}) was published{publisher_info} at {publish_time}.

If you have questions or security concerns, you can contact us at help@crates.io."
)
}
}
Loading