From 38db0a20ae2144d2f96563fd295814de9988f473 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 26 Aug 2024 17:46:34 +0200 Subject: [PATCH 1/3] TestApp: Adjust `emails_snapshot()` to replace ISO8601 dates Most `created_at` rows in the database default to `now()`, which can cause `insta` snapshots to be unstable when the content changes between runs due to the nature of time itself. This change adjusts the `emails_snapshot()` fn to replace the ISO8601 dates with a placeholder, which should cause the snapshots to become stable. --- src/tests/util/test_app.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index 9ef2abb4a19..32992eb4e78 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -177,6 +177,9 @@ impl TestApp { static EMAIL_HEADER_REGEX: LazyLock = LazyLock::new(|| Regex::new(r"(Message-ID|Date): [^\r\n]+\r\n").unwrap()); + static DATE_TIME_REGEX: LazyLock = + 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 = LazyLock::new(|| Regex::new(r"/accept-invite/\w+").unwrap()); @@ -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() }) From afc48ee081734533565f22935d6d61f163401cb1 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Sat, 24 Aug 2024 13:43:07 +0200 Subject: [PATCH 2/3] tests: Add more email assertions --- src/tests/krate/publish/auth.rs | 2 ++ src/tests/krate/publish/basics.rs | 2 ++ src/tests/krate/publish/emails.rs | 2 ++ ...all__krate__publish__basics__new_krate-4.snap | 5 +++++ src/tests/owners.rs | 2 ++ .../all__owners__new_crate_owner-2.snap | 16 ++++++++++++++++ .../snapshots/all__team__publish_owned.snap | 5 +++++ src/tests/team.rs | 3 +++ 8 files changed, 37 insertions(+) create mode 100644 src/tests/krate/publish/snapshots/all__krate__publish__basics__new_krate-4.snap create mode 100644 src/tests/snapshots/all__owners__new_crate_owner-2.snap create mode 100644 src/tests/snapshots/all__team__publish_owned.snap diff --git a/src/tests/krate/publish/auth.rs b/src/tests/krate/publish/auth.rs index ad9df86a456..e4ae4fe5a59 100644 --- a/src/tests/krate/publish/auth.rs +++ b/src/tests/krate/publish/auth.rs @@ -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")] @@ -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()); } diff --git a/src/tests/krate/publish/basics.rs b/src/tests/krate/publish/basics.rs index 2f5a01ad31f..7d305293330 100644 --- a/src/tests/krate/publish/basics.rs +++ b/src/tests/krate/publish/basics.rs @@ -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")] diff --git a/src/tests/krate/publish/emails.rs b/src/tests/krate/publish/emails.rs index 90b97ce51c1..2b0b487db43 100644 --- a/src/tests/krate/publish/emails.rs +++ b/src/tests/krate/publish/emails.rs @@ -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")] @@ -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()); } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__basics__new_krate-4.snap b/src/tests/krate/publish/snapshots/all__krate__publish__basics__new_krate-4.snap new file mode 100644 index 00000000000..195dd50b438 --- /dev/null +++ b/src/tests/krate/publish/snapshots/all__krate__publish__basics__new_krate-4.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/krate/publish/basics.rs +expression: app.emails_snapshot() +--- + diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 690168331f5..c8935e0575c 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -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( diff --git a/src/tests/snapshots/all__owners__new_crate_owner-2.snap b/src/tests/snapshots/all__owners__new_crate_owner-2.snap new file mode 100644 index 00000000000..971376cf84a --- /dev/null +++ b/src/tests/snapshots/all__owners__new_crate_owner-2.snap @@ -0,0 +1,16 @@ +--- +source: src/tests/owners.rs +expression: app.emails_snapshot() +--- +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. diff --git a/src/tests/snapshots/all__team__publish_owned.snap b/src/tests/snapshots/all__team__publish_owned.snap new file mode 100644 index 00000000000..1e4e02a95a3 --- /dev/null +++ b/src/tests/snapshots/all__team__publish_owned.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/team.rs +expression: app.emails_snapshot() +--- + diff --git a/src/tests/team.rs b/src/tests/team.rs index 399224727f1..8fa2d35de81 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -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. @@ -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) From 4c00c3c6afba930898eec42601f413b4b08a1949 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 26 Aug 2024 17:47:07 +0200 Subject: [PATCH 3/3] Implement publish notification emails --- src/controllers/krate/publish.rs | 6 +- ...__krate__publish__basics__new_krate-4.snap | 12 ++ .../all__owners__new_crate_owner-2.snap | 45 +++++ .../all__owners__new_crate_owner.snap | 15 ++ .../snapshots/all__team__publish_owned.snap | 12 ++ src/worker/jobs/mod.rs | 2 + src/worker/jobs/send_publish_notifications.rs | 162 ++++++++++++++++++ src/worker/mod.rs | 1 + 8 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 src/worker/jobs/send_publish_notifications.rs diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index a79e8801c2a..8715b952353 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -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}; @@ -442,6 +444,8 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult Self { + Self { version_id } + } +} + +impl BackgroundJob for SendPublishNotificationsJob { + const JOB_NAME: &'static str = "send_publish_notifications"; + + type Context = Arc; + + 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)) + .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::>(); + + // 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, +} + +impl PublishDetails { + async fn for_version(version_id: i32, conn: &mut AsyncPgConnection) -> QueryResult { + 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." + ) + } +} diff --git a/src/worker/mod.rs b/src/worker/mod.rs index 04906a13c22..70bb93871d1 100644 --- a/src/worker/mod.rs +++ b/src/worker/mod.rs @@ -36,6 +36,7 @@ impl RunnerExt for Runner> { .register_job_type::() .register_job_type::() .register_job_type::() + .register_job_type::() .register_job_type::() .register_job_type::() .register_job_type::()