Skip to content

backend: Implement sending notification email when a new version is published #2705

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

Closed
Closed
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
14 changes: 12 additions & 2 deletions src/background_jobs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use diesel::r2d2::PoolError;
use swirl::PerformError;

use crate::db::{DieselPool, DieselPooledConn};
use crate::email::Emails;
use crate::git::Repository;
use crate::uploaders::Uploader;

Expand All @@ -26,6 +27,7 @@ pub struct Environment {
index: Arc<Mutex<Repository>>,
pub uploader: Uploader,
http_client: AssertUnwindSafe<Client>,
pub emails: Arc<Emails>,
}

impl Clone for Environment {
Expand All @@ -34,24 +36,32 @@ impl Clone for Environment {
index: self.index.clone(),
uploader: self.uploader.clone(),
http_client: AssertUnwindSafe(self.http_client.0.clone()),
emails: self.emails.clone(),
}
}
}

impl Environment {
pub fn new(index: Repository, uploader: Uploader, http_client: Client) -> Self {
Self::new_shared(Arc::new(Mutex::new(index)), uploader, http_client)
pub fn new(index: Repository, uploader: Uploader, http_client: Client, emails: Emails) -> Self {
Self::new_shared(
Arc::new(Mutex::new(index)),
uploader,
http_client,
Arc::new(emails),
)
}

pub fn new_shared(
index: Arc<Mutex<Repository>>,
uploader: Uploader,
http_client: Client,
emails: Arc<Emails>,
) -> Self {
Self {
index,
uploader,
http_client: AssertUnwindSafe(http_client),
emails,
}
}

Expand Down
12 changes: 10 additions & 2 deletions src/bin/background-worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#![warn(clippy::all, rust_2018_idioms)]

use cargo_registry::email::Emails;
use cargo_registry::git::{Repository, RepositoryConfig};
use cargo_registry::{background_jobs::*, db};
use diesel::r2d2;
Expand Down Expand Up @@ -39,9 +40,16 @@ fn main() {
));
println!("Index cloned");

let emails = Emails::from_environment();
let emails = Arc::new(emails);

let build_runner = || {
let environment =
Environment::new_shared(repository.clone(), config.uploader.clone(), Client::new());
let environment = Environment::new_shared(
repository.clone(),
config.uploader.clone(),
Client::new(),
emails.clone(),
);
let db_config = r2d2::Pool::builder().min_idle(Some(0));
swirl::Runner::builder(environment)
.connection_pool_builder(&db_url, db_config)
Expand Down
6 changes: 4 additions & 2 deletions src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {

let hex_cksum = cksum.encode_hex::<String>();

// Register this crate in our local git repo.
// Register this crate in our local git repo and send notification emails
// to owners who haven't opted out of them.
let git_crate = git::Crate {
name: name.0,
vers: vers.to_string(),
Expand All @@ -214,7 +215,8 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {
yanked: Some(false),
links,
};
git::add_crate(git_crate).enqueue(&conn)?;
let emails = krate.owners_with_notification_email(&conn)?;
git::add_crate(git_crate, emails, user.name, verified_email_address).enqueue(&conn)?;

// The `other` field on `PublishWarnings` was introduced to handle a temporary warning
// that is no longer needed. As such, crates.io currently does not return any `other`
Expand Down
70 changes: 62 additions & 8 deletions src/email.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ https://{}/confirm/{}",
token
);

self.send(email, subject, &body)
self.send(&[email], subject, &body)
}

/// Attempts to send an ownership invitation.
Expand All @@ -83,7 +83,45 @@ or go to https://{domain}/me/pending-invites to manage all of your crate ownersh
domain = crate::config::domain_name()
);

self.send(email, subject, &body)
self.send(&[email], subject, &body)
}

/// Attempts to send a new crate version published notification to crate owners.
pub fn notify_owners(
&self,
emails: &[&str],
crate_name: &str,
crate_version: &str,
publisher_name: Option<&str>,
publisher_email: &str,
) -> AppResult<()> {
let subject = format!(
"Crate {} ({}) published to {}",
crate_name,
crate_version,
crate::config::domain_name()
);
let body = format!(
"A crate you have publish access to has recently released a new version.
Crate: {} ({})
Published by: {} <{}>
Published at: {}
If this publish is expected, you do not need to take further action.
Only if this publish is unexpected, please take immediate steps to secure your account:
* If you suspect your GitHub account was compromised, change your password
* Revoke your API Token
* Yank the version of the crate reported in this email
* Report this incident to RustSec https://rustsec.org
To stop receiving these messages, update your email notification settings at https://{domain}/me",
crate_name,
crate_version,
publisher_name.unwrap_or("(unknown username)"),
publisher_email,
chrono::Utc::now().to_rfc2822(),
domain = crate::config::domain_name()
);

self.send(emails, &subject, &body)
}

/// This is supposed to be used only during tests, to retrieve the messages stored in the
Expand All @@ -96,9 +134,19 @@ or go to https://{domain}/me/pending-invites to manage all of your crate ownersh
}
}

fn send(&self, recipient: &str, subject: &str, body: &str) -> AppResult<()> {
let email = Message::builder()
.to(recipient.parse()?)
fn send(&self, recipients: &[&str], subject: &str, body: &str) -> AppResult<()> {
let mut recipients_iter = recipients.iter();

let mut builder = Message::builder();
let to = recipients_iter
.next()
.ok_or_else(|| server_error("Email has no recipients"))?;
builder = builder.to(to.parse()?);
for bcc in recipients_iter {
builder = builder.bcc(bcc.parse()?);
}

let email = builder
.from(self.sender_address().parse()?)
.subject(subject)
.body(body.to_string())?;
Expand All @@ -122,7 +170,12 @@ or go to https://{domain}/me/pending-invites to manage all of your crate ownersh
.map_err(|_| server_error("Email file could not be generated"))?;
}
EmailBackend::Memory { mails } => mails.lock().unwrap().push(StoredEmail {
to: recipient.into(),
to: to.to_string(),
bcc: recipients
.iter()
.skip(1)
.map(|address| address.to_string())
.collect(),
subject: subject.into(),
body: body.into(),
}),
Expand Down Expand Up @@ -176,6 +229,7 @@ impl std::fmt::Debug for EmailBackend {
#[derive(Debug, Clone)]
pub struct StoredEmail {
pub to: String,
pub bcc: Vec<String>,
pub subject: String,
pub body: String,
}
Expand All @@ -189,7 +243,7 @@ mod tests {
let emails = Emails::new_in_memory();

assert_err!(emails.send(
"String.Format(\"{0}.{1}@live.com\", FirstName, LastName)",
&["String.Format(\"{0}.{1}@live.com\", FirstName, LastName)"],
"test",
"test",
));
Expand All @@ -199,6 +253,6 @@ mod tests {
fn sending_to_valid_email_succeeds() {
let emails = Emails::new_in_memory();

assert_ok!(emails.send("[email protected]", "test", "test"));
assert_ok!(emails.send(&["[email protected]"], "test", "test"));
}
}
23 changes: 20 additions & 3 deletions src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,13 @@ impl Repository {
}

#[swirl::background_job]
pub fn add_crate(env: &Environment, krate: Crate) -> Result<(), PerformError> {
pub fn add_crate(
env: &Environment,
krate: Crate,
owners_emails: Vec<String>,
publisher_name: Option<String>,
publisher_email: String,
) -> Result<(), PerformError> {
use std::io::prelude::*;

let repo = env.lock_index()?;
Expand All @@ -279,8 +285,19 @@ pub fn add_crate(env: &Environment, krate: Crate) -> Result<(), PerformError> {
file.write_all(b"\n")?;

let message: String = format!("Updating crate `{}#{}`", krate.name, krate.vers);

repo.commit_and_push(&message, &repo.relative_index_file(&krate.name))
repo.commit_and_push(&message, &repo.relative_index_file(&krate.name))?;

// Notify crate owners of this new version being published
let emails = owners_emails.iter().map(AsRef::as_ref).collect::<Vec<_>>();
let _ = env.emails.notify_owners(
&emails,
&krate.name,
&krate.vers,
publisher_name.as_deref(),
&publisher_email,
);

Ok(())
}

/// Yanks or unyanks a crate version. This requires finding the index
Expand Down
11 changes: 11 additions & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,17 @@ impl Crate {
Ok(users.chain(teams).collect())
}

pub fn owners_with_notification_email(&self, conn: &PgConnection) -> QueryResult<Vec<String>> {
CrateOwner::by_owner_kind(OwnerKind::User)
.filter(crate_owners::crate_id.eq(self.id))
.filter(crate_owners::email_notifications.eq(true))
.inner_join(emails::table.on(crate_owners::owner_id.eq(emails::user_id)))
.filter(emails::verified.eq(true))
.select(emails::email)
.distinct()
.load(conn)
}

pub fn owner_add(
&self,
app: &App,
Expand Down
2 changes: 1 addition & 1 deletion src/tests/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ fn new_krate_records_verified_email() {
.select(versions_published_by::email)
.first(conn)
.unwrap();
assert_eq!(email, "[email protected]");
assert_eq!(email, "something+foo@example.com");
});
}

Expand Down
56 changes: 56 additions & 0 deletions src/tests/owners.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,25 @@ impl MockCookieUser {
fn list_invitations(&self) -> InvitationListResponse {
self.get("/api/v1/me/crate_owner_invitations").good()
}

fn set_email_notifications(&self, krate_id: i32, email_notifications: bool) {
let body = json!([
{
"id": krate_id,
"email_notifications": email_notifications,
}
]);

#[derive(Deserialize)]
struct Empty {}

let _: Empty = self
.put(
"/api/v1/me/email_notifications",
body.to_string().as_bytes(),
)
.good();
}
}

impl MockAnonymousUser {
Expand Down Expand Up @@ -510,3 +529,40 @@ fn extract_token_from_invite_email(emails: &Emails) -> String {
let after_pos = before_pos + (&body[before_pos..]).find(after_token).unwrap();
body[before_pos..after_pos].to_string()
}

#[test]
fn test_list_owners_with_notification_email() {
let (app, _, owner, owner_token) = TestApp::init().with_token();
let owner = owner.as_model();

let krate_name = "notification_crate";
let user_name = "notification_user";

let new_user = app.db_new_user(user_name);
let krate = app.db(|conn| CrateBuilder::new(krate_name, owner.id).expect_build(conn));

// crate author gets notified
let (owners_notification, email) = app.db(|conn| {
let owners_notification = krate.owners_with_notification_email(conn).unwrap();
let email = owner.verified_email(conn).unwrap().unwrap();
(owners_notification, email)
});
assert_eq!(owners_notification, [email.clone()]);

// crate author and the new crate owner get notified
owner_token.add_named_owner(krate_name, user_name).good();
new_user.accept_ownership_invitation(&krate.name, krate.id);

let (owners_notification, new_user_email) = app.db(|conn| {
let new_user_email = new_user.as_model().verified_email(conn).unwrap().unwrap();
let owners_notification = krate.owners_with_notification_email(conn).unwrap();
(owners_notification, new_user_email)
});
assert_eq!(owners_notification, [email.clone(), new_user_email]);

// crate owners who disabled notifications don't get notified
new_user.set_email_notifications(krate.id, false);

let owners_notification = app.db(|conn| krate.owners_with_notification_email(conn).unwrap());
assert_eq!(owners_notification, [email]);
}
2 changes: 1 addition & 1 deletion src/tests/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ fn test_email_get_and_put() {
let (_app, _anon, user) = TestApp::init().with_user();

let json = user.show_me();
assert_eq!(json.user.email.unwrap(), "[email protected]");
assert_eq!(json.user.email.unwrap(), "something+foo@example.com");

user.update_email("[email protected]");

Expand Down
3 changes: 2 additions & 1 deletion src/tests/util/test_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ impl TestApp {
use diesel::prelude::*;

let user = self.db(|conn| {
let email = "[email protected]";
let email = format!("something+{}@example.com", username);

let user = crate::new_user(username)
.create_or_update(None, &self.0.app.emails, conn)
Expand Down Expand Up @@ -194,6 +194,7 @@ impl TestAppBuilder {
index,
app.config.uploader.clone(),
app.http_client().clone(),
Emails::new_in_memory(),
);

Some(
Expand Down