From 14000832c90c3e694f6eeb1e73f1c1ef620b68a1 Mon Sep 17 00:00:00 2001 From: Dom Dwyer Date: Sat, 10 Aug 2024 23:09:24 +0200 Subject: [PATCH 1/5] Implement token creation notifications When a new token is created for an account, send a notification email to the account owner. --- src/controllers/token.rs | 45 ++++++++++++++++++++++++++-- src/tests/routes/me/tokens/create.rs | 22 ++++++++++---- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/src/controllers/token.rs b/src/controllers/token.rs index df637d917ce..1f0527f5787 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -130,6 +130,8 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult> { .transpose() .map_err(|_err| bad_request("invalid endpoint scope"))?; + let recipient = user.email(conn)?; + let api_token = ApiToken::insert_with_scopes( conn, user.id, @@ -138,9 +140,26 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult> { endpoint_scopes, new.api_token.expired_at, )?; - let api_token = EncodableApiTokenWithToken::from(api_token); - Ok(Json(json!({ "api_token": api_token }))) + if let Some(recipient) = recipient { + // At this point the token has been created so failing to send the + // email should not cause an error response to be returned to the + // caller. + let email_ret = app.emails.send( + &recipient, + NewTokenEmail { + user_name: &user.gh_login, + domain: &app.emails.domain, + }, + ); + if let Err(e) = email_ret { + error!("Failed to send token creation email: {e}") + } + } + + Ok(Json( + json!({ "api_token": EncodableApiTokenWithToken::from(api_token) }), + )) }) .await } @@ -199,3 +218,25 @@ pub async fn revoke_current(app: AppState, req: Parts) -> AppResult { }) .await } + +struct NewTokenEmail<'a> { + user_name: &'a str, + domain: &'a str, +} + +impl<'a> crate::email::Email for NewTokenEmail<'a> { + const SUBJECT: &'static str = "New API token created"; + + fn body(&self) -> String { + format!( + "\ +Hello {user_name}! + +A new API token was recently added to your crates.io account. + +If this wasn't you, you should revoke the token immediately: https://{domain}/settings/tokens", + user_name = self.user_name, + domain = self.domain, + ) + } +} diff --git a/src/tests/routes/me/tokens/create.rs b/src/tests/routes/me/tokens/create.rs index 28e5b37ba3f..7d40d1038e0 100644 --- a/src/tests/routes/me/tokens/create.rs +++ b/src/tests/routes/me/tokens/create.rs @@ -19,7 +19,7 @@ async fn create_token_logged_out() { #[tokio::test(flavor = "multi_thread")] async fn create_token_invalid_request() { - let (_, _, user) = TestApp::init().with_user(); + let (app, _, user) = TestApp::init().with_user(); let invalid: &[u8] = br#"{ "name": "" }"#; let response = user.put::<()>("/api/v1/me/tokens", invalid).await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); @@ -27,11 +27,12 @@ async fn create_token_invalid_request() { response.json(), json!({ "errors": [{ "detail": "invalid new token request: Error(\"missing field `api_token`\", line: 1, column: 14)" }] }) ); + assert!(app.as_inner().emails.mails_in_memory().unwrap().is_empty()); } #[tokio::test(flavor = "multi_thread")] async fn create_token_no_name() { - let (_, _, user) = TestApp::init().with_user(); + let (app, _, user) = TestApp::init().with_user(); let empty_name: &[u8] = br#"{ "api_token": { "name": "" } }"#; let response = user.put::<()>("/api/v1/me/tokens", empty_name).await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); @@ -39,6 +40,7 @@ async fn create_token_no_name() { response.json(), json!({ "errors": [{ "detail": "name must have a value" }] }) ); + assert!(app.as_inner().emails.mails_in_memory().unwrap().is_empty()); } #[tokio::test(flavor = "multi_thread")] @@ -56,6 +58,7 @@ async fn create_token_exceeded_tokens_per_user() { response.json(), json!({ "errors": [{ "detail": "maximum tokens per user is: 500" }] }) ); + assert!(app.as_inner().emails.mails_in_memory().unwrap().is_empty()); } #[tokio::test(flavor = "multi_thread")] @@ -82,6 +85,7 @@ async fn create_token_success() { assert_eq!(tokens[0].last_used_at, None); assert_eq!(tokens[0].crate_scopes, None); assert_eq!(tokens[0].endpoint_scopes, None); + assert_eq!(app.as_inner().emails.mails_in_memory().unwrap().len(), 1); } #[tokio::test(flavor = "multi_thread")] @@ -107,7 +111,7 @@ async fn create_token_multiple_users_have_different_values() { #[tokio::test(flavor = "multi_thread")] async fn cannot_create_token_with_token() { - let (_, _, _, token) = TestApp::init().with_token(); + let (app, _, _, token) = TestApp::init().with_token(); let response = token .put::<()>( "/api/v1/me/tokens", @@ -119,6 +123,7 @@ async fn cannot_create_token_with_token() { response.json(), json!({ "errors": [{ "detail": "cannot use an API token to create a new API token" }] }) ); + assert!(app.as_inner().emails.mails_in_memory().unwrap().is_empty()); } #[tokio::test(flavor = "multi_thread")] @@ -164,6 +169,7 @@ async fn create_token_with_scopes() { tokens[0].endpoint_scopes, Some(vec![EndpointScope::PublishUpdate]) ); + assert_eq!(app.as_inner().emails.mails_in_memory().unwrap().len(), 1); } #[tokio::test(flavor = "multi_thread")] @@ -200,11 +206,12 @@ async fn create_token_with_null_scopes() { assert_eq!(tokens[0].last_used_at, None); assert_eq!(tokens[0].crate_scopes, None); assert_eq!(tokens[0].endpoint_scopes, None); + assert_eq!(app.as_inner().emails.mails_in_memory().unwrap().len(), 1); } #[tokio::test(flavor = "multi_thread")] async fn create_token_with_empty_crate_scope() { - let (_, _, user) = TestApp::init().with_user(); + let (app, _, user) = TestApp::init().with_user(); let json = json!({ "api_token": { @@ -222,11 +229,12 @@ async fn create_token_with_empty_crate_scope() { response.json(), json!({ "errors": [{ "detail": "invalid crate scope" }] }) ); + assert!(app.as_inner().emails.mails_in_memory().unwrap().is_empty()); } #[tokio::test(flavor = "multi_thread")] async fn create_token_with_invalid_endpoint_scope() { - let (_, _, user) = TestApp::init().with_user(); + let (app, _, user) = TestApp::init().with_user(); let json = json!({ "api_token": { @@ -244,11 +252,12 @@ async fn create_token_with_invalid_endpoint_scope() { response.json(), json!({ "errors": [{ "detail": "invalid endpoint scope" }] }) ); + assert!(app.as_inner().emails.mails_in_memory().unwrap().is_empty()); } #[tokio::test(flavor = "multi_thread")] async fn create_token_with_expiry_date() { - let (_app, _, user) = TestApp::init().with_user(); + let (app, _, user) = TestApp::init().with_user(); let json = json!({ "api_token": { @@ -269,4 +278,5 @@ async fn create_token_with_expiry_date() { ".api_token.last_used_at" => "[datetime]", ".api_token.token" => insta::api_token_redaction(), }); + assert_eq!(app.as_inner().emails.mails_in_memory().unwrap().len(), 1); } From 1d4687eab0adf7df35de7e30e2d16e7408e10abb Mon Sep 17 00:00:00 2001 From: Dom Dwyer Date: Sun, 11 Aug 2024 13:11:22 +0200 Subject: [PATCH 2/5] Use templated domain in email Use "{domain}" instead of "crates.io" when describing where the token was created. --- src/controllers/token.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 1f0527f5787..05bfb0b4aa3 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -232,7 +232,7 @@ impl<'a> crate::email::Email for NewTokenEmail<'a> { "\ Hello {user_name}! -A new API token was recently added to your crates.io account. +A new API token was recently added to your {domain} account. If this wasn't you, you should revoke the token immediately: https://{domain}/settings/tokens", user_name = self.user_name, From 2de7ef76eda1dd35b93a89d0d99a7917b7861612 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Fri, 16 Aug 2024 09:29:49 +0200 Subject: [PATCH 3/5] controllers/token: Extract `api_token` variable Reduces the diff of the pull request since this is an unrelated change ;) --- src/controllers/token.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 05bfb0b4aa3..00637bcd27f 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -157,9 +157,9 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult> { } } - Ok(Json( - json!({ "api_token": EncodableApiTokenWithToken::from(api_token) }), - )) + let api_token = EncodableApiTokenWithToken::from(api_token); + + Ok(Json(json!({ "api_token": api_token }))) }) .await } From 30f71a2d50e29e4d10631871936549051b3ac898 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Fri, 16 Aug 2024 09:33:50 +0200 Subject: [PATCH 4/5] controllers/token: Extract `email` variable A little easier on the eyes :) --- src/controllers/token.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 00637bcd27f..5db9693eaea 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -142,16 +142,15 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult> { )?; if let Some(recipient) = recipient { + let email = NewTokenEmail { + user_name: &user.gh_login, + domain: &app.emails.domain, + }; + // At this point the token has been created so failing to send the // email should not cause an error response to be returned to the // caller. - let email_ret = app.emails.send( - &recipient, - NewTokenEmail { - user_name: &user.gh_login, - domain: &app.emails.domain, - }, - ); + let email_ret = app.emails.send(&recipient, email); if let Err(e) = email_ret { error!("Failed to send token creation email: {e}") } From 4618f59a504759752ccee1fa61329251cf9ee55c Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Fri, 16 Aug 2024 09:36:37 +0200 Subject: [PATCH 5/5] controllers/token: Add API token name to the notification text --- src/controllers/token.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 5db9693eaea..7c98270e375 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -143,6 +143,7 @@ pub async fn new(app: AppState, req: BytesRequest) -> AppResult> { if let Some(recipient) = recipient { let email = NewTokenEmail { + token_name: name, user_name: &user.gh_login, domain: &app.emails.domain, }; @@ -219,6 +220,7 @@ pub async fn revoke_current(app: AppState, req: Parts) -> AppResult { } struct NewTokenEmail<'a> { + token_name: &'a str, user_name: &'a str, domain: &'a str, } @@ -231,9 +233,10 @@ impl<'a> crate::email::Email for NewTokenEmail<'a> { "\ Hello {user_name}! -A new API token was recently added to your {domain} account. +A new API token with the name \"{token_name}\" was recently added to your {domain} account. If this wasn't you, you should revoke the token immediately: https://{domain}/settings/tokens", + token_name = self.token_name, user_name = self.user_name, domain = self.domain, )