From 7c79051891a68fd9a67809c1315ba799f158fe7a Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 19 May 2022 08:42:06 -0700 Subject: [PATCH 01/12] bug: Fix GCM handling Closes: #308 --- autoendpoint/src/error.rs | 10 ++++++++++ autoendpoint/src/extractors/user.rs | 2 +- autoendpoint/src/routes/webpush.rs | 22 +++++++++++++++++++--- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index 36c80a890..b2184391f 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -152,6 +152,16 @@ impl ApiErrorKind { } } + /// Should the User record be removed? + pub fn user_still_valid(&self) -> bool { + match self { + ApiErrorKind::NoUser | ApiErrorKind::NoSubscription | ApiErrorKind::InvalidToken => { + false + } + _ => true, + } + } + /// Specify the label to use for metrics reporting. pub fn metric_label(&self) -> &'static str { match self { diff --git a/autoendpoint/src/extractors/user.rs b/autoendpoint/src/extractors/user.rs index 2be2e5212..ea4ea8037 100644 --- a/autoendpoint/src/extractors/user.rs +++ b/autoendpoint/src/extractors/user.rs @@ -69,7 +69,7 @@ async fn validate_webpush_user( } /// Drop a user and increment associated metric -async fn drop_user(uaid: Uuid, ddb: &dyn DbClient, metrics: &StatsdClient) -> ApiResult<()> { +pub async fn drop_user(uaid: Uuid, ddb: &dyn DbClient, metrics: &StatsdClient) -> ApiResult<()> { metrics .incr_with_tags("updates.drop_user") .with_tag("errno", "102") diff --git a/autoendpoint/src/routes/webpush.rs b/autoendpoint/src/routes/webpush.rs index e59637aab..06b3c6017 100644 --- a/autoendpoint/src/routes/webpush.rs +++ b/autoendpoint/src/routes/webpush.rs @@ -4,6 +4,7 @@ use crate::error::{ApiErrorKind, ApiResult}; use crate::extractors::message_id::MessageId; use crate::extractors::notification::Notification; use crate::extractors::routers::{RouterType, Routers}; +use crate::extractors::user::drop_user; use crate::server::ServerState; use actix_web::web::Data; use actix_web::HttpResponse; @@ -12,15 +13,30 @@ use actix_web::HttpResponse; pub async fn webpush_route( notification: Notification, routers: Routers, + state: Data, ) -> ApiResult { let router = routers.get( RouterType::from_str(¬ification.subscription.user.router_type) .map_err(|_| ApiErrorKind::InvalidRouterType)?, ); - let response = router.route_notification(¬ification).await?; - - Ok(response.into()) + match router.route_notification(¬ification).await { + Ok(v) => Ok(v.into()), + Err(e) => { + if !e.kind.user_still_valid() { + // The user record is no longer valid. We should remove it so that the client + // can attempt to recover. We don't care if this succeeds or fails at this point. + if let Ok(_) = drop_user( + notification.subscription.user.uaid, + state.ddb.as_ref(), + &state.metrics, + ) + .await + {}; + } + Err(e) + } + } } /// Handle the `DELETE /m/{message_id}` route From 05d35b87383b0e4c6505964d261618c244ac8541 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 19 May 2022 09:15:39 -0700 Subject: [PATCH 02/12] f fmt --- autoendpoint/src/routes/webpush.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/autoendpoint/src/routes/webpush.rs b/autoendpoint/src/routes/webpush.rs index 06b3c6017..16238fe88 100644 --- a/autoendpoint/src/routes/webpush.rs +++ b/autoendpoint/src/routes/webpush.rs @@ -26,12 +26,13 @@ pub async fn webpush_route( if !e.kind.user_still_valid() { // The user record is no longer valid. We should remove it so that the client // can attempt to recover. We don't care if this succeeds or fails at this point. - if let Ok(_) = drop_user( + if drop_user( notification.subscription.user.uaid, state.ddb.as_ref(), &state.metrics, ) .await + .is_ok() {}; } Err(e) From 5dcf603d2ca9aaf69fb9c5a51e434b6f1bcce7eb Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 20 May 2022 15:01:49 -0700 Subject: [PATCH 03/12] f exclude GCM auth messages from sentry reporting We can't fix them, and they could overflow our quota. DEPLOY NOTE: Remove the sentry.io filter for "Bridge authentication error". --- autoendpoint/src/error.rs | 3 +++ autoendpoint/src/routers/common.rs | 11 +++++++++++ autoendpoint/src/routers/fcm/client.rs | 2 +- autoendpoint/src/routers/mod.rs | 6 ++++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index b2184391f..1754aed0a 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -203,6 +203,9 @@ impl ApiErrorKind { self, // Ignore common webpush errors ApiErrorKind::NoTTL | ApiErrorKind::InvalidEncryption(_) | + // Ignore GCM router authentication errors otherwise + // we could blow through our quota. + ApiErrorKind::Router(RouterError::GCMAuthentication) | // Ignore common VAPID erros ApiErrorKind::VapidError(_) | ApiErrorKind::Jwt(_) diff --git a/autoendpoint/src/routers/common.rs b/autoendpoint/src/routers/common.rs index f56b4563d..92f5ccde0 100644 --- a/autoendpoint/src/routers/common.rs +++ b/autoendpoint/src/routers/common.rs @@ -57,6 +57,17 @@ pub async fn handle_error( error.errno(), ); } + RouterError::GCMAuthentication => { + // Don't record the GCM auth error. + incr_error_metric( + metrics, + platform, + app_id, + "gcm authentication", + error.status(), + error.errno(), + ); + } RouterError::RequestTimeout => { warn!("Bridge timeout"); incr_error_metric( diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index 2c6232c8d..ee51badb7 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -145,7 +145,7 @@ impl FcmClient { .map_err(FcmError::DeserializeResponse)?; return Err(match (status, data.error) { - (StatusCode::UNAUTHORIZED, _) => RouterError::Authentication, + (StatusCode::UNAUTHORIZED, _) => RouterError::GCMAuthentication, (StatusCode::NOT_FOUND, _) => RouterError::NotFound, (_, Some(error)) => RouterError::Upstream { status: error.status, diff --git a/autoendpoint/src/routers/mod.rs b/autoendpoint/src/routers/mod.rs index 1cc5900bc..19ae07145 100644 --- a/autoendpoint/src/routers/mod.rs +++ b/autoendpoint/src/routers/mod.rs @@ -95,6 +95,9 @@ pub enum RouterError { #[error("Bridge authentication error")] Authentication, + #[error("GCM Bridge authentication error")] + GCMAuthentication, + #[error("Bridge request timeout")] RequestTimeout, @@ -118,6 +121,7 @@ impl RouterError { RouterError::SaveDb(_) => StatusCode::SERVICE_UNAVAILABLE, + RouterError::GCMAuthentication | // Treat GCM auth failures as special, we reset the user. RouterError::UserWasDeleted | RouterError::NotFound => StatusCode::GONE, RouterError::TooMuchData(_) => StatusCode::PAYLOAD_TOO_LARGE, @@ -150,6 +154,8 @@ impl RouterError { RouterError::RequestTimeout => Some(903), + RouterError::GCMAuthentication => Some(904), + RouterError::Upstream { .. } => None, } } From 193e4eacd20509a83510ddd44798b563378f0c77 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 20 May 2022 16:13:48 -0700 Subject: [PATCH 04/12] =?UTF-8?q?f=20specify=20gcm=20path=20=F0=9F=A4=A6?= =?UTF-8?q?=F0=9F=8F=BB=E2=80=8D=E2=99=82=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- autoendpoint/src/routers/fcm/client.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index ee51badb7..0e0334e94 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -17,6 +17,7 @@ const OAUTH_SCOPES: &[&str] = &["https://www.googleapis.com/auth/firebase.messag /// handles sending notifications to Firebase. pub struct FcmClient { endpoint: Url, + gcm_endpoint: Url, timeout: Duration, max_data: usize, fcm_authenticator: Option, @@ -75,6 +76,10 @@ impl FcmClient { server_credential.project_id )) .expect("Project ID is not URL-safe"), + gcm_endpoint: settings + .base_url + .join("gcm/send") + .expect("GCM Project ID is not URL-safe"), timeout: Duration::from_secs(settings.timeout as u64), max_data: settings.max_data, fcm_authenticator: auth, @@ -121,7 +126,7 @@ impl FcmClient { let server_access_token = &self.server_credential.server_access_token; let response = self .http_client - .post(self.endpoint.clone()) + .post(self.gcm_endpoint.clone()) .header("Authorization", format!("key={}", server_access_token)) .header("Content-Type", "application/json") .json(&message) From 3188cb75016589746348b2bd4b06d08698c7cf11 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 20 May 2022 17:34:16 -0700 Subject: [PATCH 05/12] f r's --- autoendpoint/src/error.rs | 4 ++-- autoendpoint/src/routers/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index 1754aed0a..fa6076b6e 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -203,8 +203,8 @@ impl ApiErrorKind { self, // Ignore common webpush errors ApiErrorKind::NoTTL | ApiErrorKind::InvalidEncryption(_) | - // Ignore GCM router authentication errors otherwise - // we could blow through our quota. + // Do not report GCMAuthentication failures to Sentry since + // they're not really actionable and could flood our quota ApiErrorKind::Router(RouterError::GCMAuthentication) | // Ignore common VAPID erros ApiErrorKind::VapidError(_) diff --git a/autoendpoint/src/routers/mod.rs b/autoendpoint/src/routers/mod.rs index 19ae07145..76e43ceff 100644 --- a/autoendpoint/src/routers/mod.rs +++ b/autoendpoint/src/routers/mod.rs @@ -121,12 +121,12 @@ impl RouterError { RouterError::SaveDb(_) => StatusCode::SERVICE_UNAVAILABLE, - RouterError::GCMAuthentication | // Treat GCM auth failures as special, we reset the user. RouterError::UserWasDeleted | RouterError::NotFound => StatusCode::GONE, RouterError::TooMuchData(_) => StatusCode::PAYLOAD_TOO_LARGE, RouterError::Authentication + | RouterError::GCMAuthentication | RouterError::RequestTimeout | RouterError::Connect(_) | RouterError::Upstream { .. } => StatusCode::BAD_GATEWAY, From 219401d0d8bed36695a4619203febb65473f1d21 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 20 May 2022 18:35:59 -0700 Subject: [PATCH 06/12] f r's --- autoendpoint/src/error.rs | 7 +---- autoendpoint/src/routers/fcm/client.rs | 42 ++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index fa6076b6e..e762f7bad 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -154,12 +154,7 @@ impl ApiErrorKind { /// Should the User record be removed? pub fn user_still_valid(&self) -> bool { - match self { - ApiErrorKind::NoUser | ApiErrorKind::NoSubscription | ApiErrorKind::InvalidToken => { - false - } - _ => true, - } + !matches!(self, ApiErrorKind::InvalidToken) } /// Specify the label to use for metrics reporting. diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index 0e0334e94..b023597b0 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -220,18 +220,26 @@ impl FcmClient { // Handle error let status = response.status(); if status.is_client_error() || status.is_server_error() { - let data: FcmResponse = response + let data: GcmResponse = response .json() .await .map_err(FcmError::DeserializeResponse)?; - return Err(match (status, data.error) { + // we only ever send one. + return Err(match (status, &data.results[0].error) { (StatusCode::UNAUTHORIZED, _) => RouterError::Authentication, (StatusCode::NOT_FOUND, _) => RouterError::NotFound, - (_, Some(error)) => RouterError::Upstream { - status: error.status, - message: error.message, - }, + (_, Some(error)) => { + if *error == "NotRegistered" { + // other values? "Unavailable", "InvalidRegistration" + RouterError::NotFound + } else { + RouterError::Upstream { + status: status.to_string(), + message: error.clone(), + } + } + } (status, None) => RouterError::Upstream { status: status.to_string(), message: "Unknown reason".to_string(), @@ -254,6 +262,28 @@ struct FcmErrorResponse { message: String, } +#[derive(Deserialize)] +struct GcmResult { + error: Option, + #[serde(rename = "message_id")] + _message_id: Option, + #[serde(rename = "registration_id")] + _registration_id: Option, +} + +#[derive(Deserialize)] +struct GcmResponse { + results: Vec, + #[serde(rename = "multicast_id")] + _multicast_id: u32, + #[serde(rename = "success")] + _success: u32, + #[serde(rename = "failure")] + _failure: u32, + #[serde(rename = "canonical_ids")] + _canonical_ids: u32, +} + #[cfg(test)] pub mod tests { use crate::routers::fcm::client::FcmClient; From 8c2957d63d8a2cd0ae4e4a74a878cdd535c13757 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 20 May 2022 22:00:01 -0700 Subject: [PATCH 07/12] f fix tests --- autoendpoint/src/routers/common.rs | 2 +- autoendpoint/src/routers/fcm/client.rs | 81 +++++++++++++++++++------- autoendpoint/src/routers/fcm/router.rs | 8 +-- 3 files changed, 64 insertions(+), 27 deletions(-) diff --git a/autoendpoint/src/routers/common.rs b/autoendpoint/src/routers/common.rs index 92f5ccde0..477a7c1c3 100644 --- a/autoendpoint/src/routers/common.rs +++ b/autoendpoint/src/routers/common.rs @@ -58,7 +58,7 @@ pub async fn handle_error( ); } RouterError::GCMAuthentication => { - // Don't record the GCM auth error. + warn!("GCM Authentication error"); incr_error_metric( metrics, platform, diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index b023597b0..b41cc7070 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -144,18 +144,25 @@ impl FcmClient { // Handle error let status = response.status(); if status.is_client_error() || status.is_server_error() { - let data: FcmResponse = response + let data: GcmResponse = response .json() .await .map_err(FcmError::DeserializeResponse)?; - return Err(match (status, data.error) { + return Err(match (status, &data.results[0].error) { (StatusCode::UNAUTHORIZED, _) => RouterError::GCMAuthentication, (StatusCode::NOT_FOUND, _) => RouterError::NotFound, - (_, Some(error)) => RouterError::Upstream { - status: error.status, - message: error.message, - }, + (_, Some(error)) => { + if error == "NotRegistered" { + // other values? "Unavailable", "InvalidRegistration" + RouterError::NotFound + } else { + RouterError::Upstream { + status: status.to_string(), + message: error.clone(), + } + } + } (status, None) => RouterError::Upstream { status: status.to_string(), message: "Unknown reason".to_string(), @@ -220,26 +227,19 @@ impl FcmClient { // Handle error let status = response.status(); if status.is_client_error() || status.is_server_error() { - let data: GcmResponse = response + let data: FcmResponse = response .json() .await .map_err(FcmError::DeserializeResponse)?; // we only ever send one. - return Err(match (status, &data.results[0].error) { + return Err(match (status, data.error) { (StatusCode::UNAUTHORIZED, _) => RouterError::Authentication, (StatusCode::NOT_FOUND, _) => RouterError::NotFound, - (_, Some(error)) => { - if *error == "NotRegistered" { - // other values? "Unavailable", "InvalidRegistration" - RouterError::NotFound - } else { - RouterError::Upstream { - status: status.to_string(), - message: error.clone(), - } - } - } + (_, Some(error)) => RouterError::Upstream { + status: error.status, + message: error.message, + }, (status, None) => RouterError::Upstream { status: status.to_string(), message: "Unknown reason".to_string(), @@ -335,6 +335,10 @@ pub mod tests { ) } + pub fn mock_gcm_endpoint_builder() -> mockito::Mock { + mockito::mock("POST", "/fcm/send") + } + /// Make a FcmClient from the service auth data async fn make_client(credential: FcmServerCredential) -> FcmClient { FcmClient::new( @@ -386,9 +390,8 @@ pub mod tests { server_access_token: registration_id.to_owned(), }) .await; - let _token_mock = mock_token_endpoint(); let body = format!("{{\"data\":{{\"is_test\":\"true\"}},\"delay_while_idle\":false,\"registration_ids\":[\"{}\"],\"time_to_live\":42}}", ®istration_id); - let fcm_mock = mock_fcm_endpoint_builder(project_id) + let gcm_mock = mock_gcm_endpoint_builder() .match_header("Authorization", format!("key={}", registration_id).as_str()) .match_header("Content-Type", "application/json") .match_body(body.as_str()) @@ -397,7 +400,7 @@ pub mod tests { data.insert("is_test", "true".to_string()); let result = client.send_gcm(data, registration_id.to_owned(), 42).await; assert!(result.is_ok(), "result={:?}", result); - fcm_mock.assert(); + gcm_mock.assert(); } /// Authorization errors are handled @@ -425,6 +428,40 @@ pub mod tests { ); } + /// Authorization errors are handled + #[tokio::test] + async fn gcm_unauthorized() { + let client = make_client(FcmServerCredential { + project_id: PROJECT_ID.to_owned(), + server_access_token: make_service_key(), + }) + .await; + let _token_mock = mock_token_endpoint(); + let _gcm_mock = mock_gcm_endpoint_builder() + .with_status(401) + .with_body( + r#"{ "multicast_id": 216, + "success": 0, + "failure": 1, + "canonical_ids": 0, + "results": [ + { "error": "NotRegistered"} + ] + }"#, + ) + .create(); + + let result = client + .send_gcm(HashMap::new(), "test-token".to_string(), 42) + .await; + assert!(result.is_err()); + assert!( + matches!(result.as_ref().unwrap_err(), RouterError::GCMAuthentication), + "result = {:?}", + result + ); + } + /// 404 errors are handled #[tokio::test] async fn not_found() { diff --git a/autoendpoint/src/routers/fcm/router.rs b/autoendpoint/src/routers/fcm/router.rs index 6842c670b..6255f248b 100644 --- a/autoendpoint/src/routers/fcm/router.rs +++ b/autoendpoint/src/routers/fcm/router.rs @@ -229,8 +229,8 @@ mod tests { use crate::extractors::routers::RouterType; use crate::routers::common::tests::{make_notification, CHANNEL_ID}; use crate::routers::fcm::client::tests::{ - make_service_key, mock_fcm_endpoint_builder, mock_token_endpoint, GCM_PROJECT_ID, - PROJECT_ID, + make_service_key, mock_fcm_endpoint_builder, mock_gcm_endpoint_builder, + mock_token_endpoint, GCM_PROJECT_ID, PROJECT_ID, }; use crate::routers::fcm::error::FcmError; use crate::routers::fcm::router::FcmRouter; @@ -342,7 +342,7 @@ mod tests { async fn successful_gcm_fallback() { let auth_key = "AIzaSyB0ecSrqnEDXQ7yjLXqVc0CUGOeSlq9BsM"; // this is a nonce value used only for testing. let registration_id = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - let project_id = GCM_PROJECT_ID; + // let project_id = GCM_PROJECT_ID; let ddb = MockDbClient::new().into_boxed_arc(); let router = make_router(make_service_key(), auth_key.to_owned(), ddb).await; assert!(router.active()); @@ -357,7 +357,7 @@ mod tests { }) .to_string(); let _token_mock = mock_token_endpoint(); - let fcm_mock = mock_fcm_endpoint_builder(project_id) + let fcm_mock = mock_gcm_endpoint_builder() .match_header("Authorization", format!("key={}", &auth_key).as_str()) .match_header("Content-Type", "application/json") .match_body(body.as_str()) From 946afcf3c99792bb47b9db752a691ccf1c5e82e9 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 20 May 2022 22:05:54 -0700 Subject: [PATCH 08/12] f restore err --- autoendpoint/src/error.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index e762f7bad..833a46aa2 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -198,9 +198,6 @@ impl ApiErrorKind { self, // Ignore common webpush errors ApiErrorKind::NoTTL | ApiErrorKind::InvalidEncryption(_) | - // Do not report GCMAuthentication failures to Sentry since - // they're not really actionable and could flood our quota - ApiErrorKind::Router(RouterError::GCMAuthentication) | // Ignore common VAPID erros ApiErrorKind::VapidError(_) | ApiErrorKind::Jwt(_) From 275c8f1985709f750546b81273245d7c6192961b Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 23 May 2022 09:08:48 -0700 Subject: [PATCH 09/12] f add GCM comments tests --- autoendpoint/src/routers/fcm/client.rs | 110 +++++++++++++++---------- autoendpoint/src/routers/fcm/router.rs | 3 + 2 files changed, 68 insertions(+), 45 deletions(-) diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index b41cc7070..1def8b07f 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -89,6 +89,17 @@ impl FcmClient { } /// Send the message as GCM + /// Note: GCM is a beyond deprecated protocol, however older clients + /// may still use it (which includes about half of our traffic at + /// the time this comment was written). GCM documentation was also + /// removed from Google's site. It still persists in obscure corners + /// of the internet, but I have no idea for how long. + /// + /// + /// There is no way to generate GCM credentials. (They are disabled + /// on Google's server side.) There is no way to test GCM compatibility + /// other than monitoring errors. Users really should be migrated to the + /// new FCM endpoints, but we don't have full control over that. pub async fn send_gcm( &self, data: HashMap<&'static str, String>, @@ -114,9 +125,9 @@ impl FcmClient { // You can also turn on internal debugging, see // https://docs.rs/mockito/latest/mockito/#debug dbg!(self.endpoint.clone().as_str()); - let rr = ureq::post(self.endpoint.clone().as_str()) + let rr = ureq::post(self.gcm_endpoint.clone().as_str()) .set("Authorization", - format!("key={}", self.credential.credential.as_str()).as_str()) + format!("key={}", self.server_credential.server_access_token.as_str()).as_str()) .set("Content-Type", "application/json") .send_json(&message).unwrap(); dbg!(rr.status(), rr.status_text()); @@ -143,26 +154,25 @@ impl FcmClient { // Handle error let status = response.status(); - if status.is_client_error() || status.is_server_error() { - let data: GcmResponse = response - .json() - .await - .map_err(FcmError::DeserializeResponse)?; - + let data: GcmResponse = response + .json() + .await + .map_err(FcmError::DeserializeResponse)?; + if status.is_client_error() || status.is_server_error() || data.failure > 0 { return Err(match (status, &data.results[0].error) { (StatusCode::UNAUTHORIZED, _) => RouterError::GCMAuthentication, (StatusCode::NOT_FOUND, _) => RouterError::NotFound, - (_, Some(error)) => { - if error == "NotRegistered" { - // other values? "Unavailable", "InvalidRegistration" - RouterError::NotFound - } else { - RouterError::Upstream { - status: status.to_string(), - message: error.clone(), - } - } - } + (_, Some(error)) => match error.as_str() { + "NotRegistered" | "InvalidRegistration" => RouterError::NotFound, + "Unavailable" => RouterError::Upstream { + status: "USER UNAVAILABLE".to_owned(), + message: "User Unavailable. Try again later.".to_owned(), + }, + _ => RouterError::Upstream { + status: StatusCode::BAD_GATEWAY.to_string(), + message: format!("Unexpected error: {}", error), + }, + }, (status, None) => RouterError::Upstream { status: status.to_string(), message: "Unknown reason".to_string(), @@ -262,26 +272,29 @@ struct FcmErrorResponse { message: String, } -#[derive(Deserialize)] +/// This is a joint structure that would reflect the status of each delivered +/// message. (We only send one at a time.) +#[derive(Deserialize, Debug)] struct GcmResult { - error: Option, + error: Option, // Optional, standardized error message #[serde(rename = "message_id")] - _message_id: Option, + _message_id: Option, // Optional identifier for a successful send #[serde(rename = "registration_id")] - _registration_id: Option, + _registration_id: Option, // Optional replacement registration ID } -#[derive(Deserialize)] +// The expected GCM response message. (Being explicit here because +// the offical documentation has been removed) +#[derive(Deserialize, Debug)] struct GcmResponse { results: Vec, #[serde(rename = "multicast_id")] - _multicast_id: u32, + _multicast_id: u32, // ID for this set of messages/results #[serde(rename = "success")] - _success: u32, - #[serde(rename = "failure")] - _failure: u32, + _success: u32, // Number of messages succeeding. + failure: u32, // number of messages failing. #[serde(rename = "canonical_ids")] - _canonical_ids: u32, + _canonical_ids: u32, // number of IDs that are reassigned. } #[cfg(test)] @@ -390,10 +403,14 @@ pub mod tests { server_access_token: registration_id.to_owned(), }) .await; - let body = format!("{{\"data\":{{\"is_test\":\"true\"}},\"delay_while_idle\":false,\"registration_ids\":[\"{}\"],\"time_to_live\":42}}", ®istration_id); + let body = format!( + r#"{{"data":{{"is_test":"true"}},"delay_while_idle":false,"registration_ids":["{}"],"time_to_live":42}}"#, + ®istration_id + ); let gcm_mock = mock_gcm_endpoint_builder() .match_header("Authorization", format!("key={}", registration_id).as_str()) .match_header("Content-Type", "application/json") + .with_body(r#"{"multicast_id":216,"success":1,"failure":0,"canonical_ids":0,"results":[{"message_id":"1:02"}]}"#,) .match_body(body.as_str()) .create(); let mut data = HashMap::new(); @@ -428,35 +445,38 @@ pub mod tests { ); } - /// Authorization errors are handled + /// GCM errors are handled #[tokio::test] - async fn gcm_unauthorized() { + async fn gcm_unavailable() { + let token = make_service_key(); let client = make_client(FcmServerCredential { project_id: PROJECT_ID.to_owned(), - server_access_token: make_service_key(), + server_access_token: token.clone(), }) .await; let _token_mock = mock_token_endpoint(); + // Other potential GCM errors: + // "Unavailable" => client unavailable, retry sending. + // "InvalidRegistration" => registration corrupted, remove. let _gcm_mock = mock_gcm_endpoint_builder() - .with_status(401) - .with_body( - r#"{ "multicast_id": 216, - "success": 0, - "failure": 1, - "canonical_ids": 0, - "results": [ - { "error": "NotRegistered"} - ] - }"#, + .match_body(r#"{"data":{"is_test":"true"},"delay_while_idle":false,"registration_ids":["test-token"],"time_to_live":42}"#) + .match_header("Authorization", format!("key={}", token).as_str()) + .match_header("Content-Type", "application/json") + .with_status(200) + .with_body(r#"{"multicast_id":216,"success":0,"failure":1,"canonical_ids":0,"results":[{"error":"NotRegistered"}]}"#, ) .create(); let result = client - .send_gcm(HashMap::new(), "test-token".to_string(), 42) + .send_gcm( + HashMap::from([("is_test", "true".to_owned())]), + "test-token".to_string(), + 42, + ) .await; assert!(result.is_err()); assert!( - matches!(result.as_ref().unwrap_err(), RouterError::GCMAuthentication), + matches!(result.as_ref().unwrap_err(), RouterError::NotFound), "result = {:?}", result ); diff --git a/autoendpoint/src/routers/fcm/router.rs b/autoendpoint/src/routers/fcm/router.rs index 6255f248b..3bc4ea9fe 100644 --- a/autoendpoint/src/routers/fcm/router.rs +++ b/autoendpoint/src/routers/fcm/router.rs @@ -360,6 +360,9 @@ mod tests { let fcm_mock = mock_gcm_endpoint_builder() .match_header("Authorization", format!("key={}", &auth_key).as_str()) .match_header("Content-Type", "application/json") + .with_body( + r#"{ "multicast_id": 216,"success":1,"failure":0,"canonical_ids":0,"results":[{"message_id":"1:02"}]}"#, + ) .match_body(body.as_str()) .create(); let notification = make_notification( From cd41720115d56abfd1414ab5642da6d5ef0cb9c1 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 23 May 2022 16:39:57 -0700 Subject: [PATCH 10/12] f r's --- autoendpoint/src/routers/fcm/client.rs | 50 ++++++++++++++++---------- autoendpoint/src/routes/webpush.rs | 22 ++---------- 2 files changed, 33 insertions(+), 39 deletions(-) diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index 1def8b07f..943ead83f 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -159,25 +159,28 @@ impl FcmClient { .await .map_err(FcmError::DeserializeResponse)?; if status.is_client_error() || status.is_server_error() || data.failure > 0 { - return Err(match (status, &data.results[0].error) { - (StatusCode::UNAUTHORIZED, _) => RouterError::GCMAuthentication, - (StatusCode::NOT_FOUND, _) => RouterError::NotFound, - (_, Some(error)) => match error.as_str() { - "NotRegistered" | "InvalidRegistration" => RouterError::NotFound, - "Unavailable" => RouterError::Upstream { - status: "USER UNAVAILABLE".to_owned(), - message: "User Unavailable. Try again later.".to_owned(), + let invalid = GcmResult::invalid(); + return Err( + match (status, &data.results.get(0).unwrap_or(&invalid).error) { + (StatusCode::UNAUTHORIZED, _) => RouterError::GCMAuthentication, + (StatusCode::NOT_FOUND, _) => RouterError::NotFound, + (_, Some(error)) => match error.as_str() { + "NotRegistered" | "InvalidRegistration" => RouterError::NotFound, + "Unavailable" => RouterError::Upstream { + status: "USER UNAVAILABLE".to_owned(), + message: "User Unavailable. Try again later.".to_owned(), + }, + _ => RouterError::Upstream { + status: StatusCode::BAD_GATEWAY.to_string(), + message: format!("Unexpected error: {}", error), + }, }, - _ => RouterError::Upstream { - status: StatusCode::BAD_GATEWAY.to_string(), - message: format!("Unexpected error: {}", error), + (status, None) => RouterError::Upstream { + status: status.to_string(), + message: "Unknown reason".to_string(), }, }, - (status, None) => RouterError::Upstream { - status: status.to_string(), - message: "Unknown reason".to_string(), - }, - }); + ); } Ok(()) @@ -274,7 +277,7 @@ struct FcmErrorResponse { /// This is a joint structure that would reflect the status of each delivered /// message. (We only send one at a time.) -#[derive(Deserialize, Debug)] +#[derive(Clone, Deserialize, Debug, Default)] struct GcmResult { error: Option, // Optional, standardized error message #[serde(rename = "message_id")] @@ -283,13 +286,22 @@ struct GcmResult { _registration_id: Option, // Optional replacement registration ID } +impl GcmResult { + pub fn invalid() -> GcmResult { + Self { + error: Some("Invalid GCM Response".to_string()), + ..Default::default() + } + } +} + // The expected GCM response message. (Being explicit here because // the offical documentation has been removed) -#[derive(Deserialize, Debug)] +#[derive(Clone, Deserialize, Debug, Default)] struct GcmResponse { results: Vec, #[serde(rename = "multicast_id")] - _multicast_id: u32, // ID for this set of messages/results + _multicast_id: u64, // ID for this set of messages/results #[serde(rename = "success")] _success: u32, // Number of messages succeeding. failure: u32, // number of messages failing. diff --git a/autoendpoint/src/routes/webpush.rs b/autoendpoint/src/routes/webpush.rs index 16238fe88..cba9ee434 100644 --- a/autoendpoint/src/routes/webpush.rs +++ b/autoendpoint/src/routes/webpush.rs @@ -4,7 +4,6 @@ use crate::error::{ApiErrorKind, ApiResult}; use crate::extractors::message_id::MessageId; use crate::extractors::notification::Notification; use crate::extractors::routers::{RouterType, Routers}; -use crate::extractors::user::drop_user; use crate::server::ServerState; use actix_web::web::Data; use actix_web::HttpResponse; @@ -13,31 +12,14 @@ use actix_web::HttpResponse; pub async fn webpush_route( notification: Notification, routers: Routers, - state: Data, + _state: Data, ) -> ApiResult { let router = routers.get( RouterType::from_str(¬ification.subscription.user.router_type) .map_err(|_| ApiErrorKind::InvalidRouterType)?, ); - match router.route_notification(¬ification).await { - Ok(v) => Ok(v.into()), - Err(e) => { - if !e.kind.user_still_valid() { - // The user record is no longer valid. We should remove it so that the client - // can attempt to recover. We don't care if this succeeds or fails at this point. - if drop_user( - notification.subscription.user.uaid, - state.ddb.as_ref(), - &state.metrics, - ) - .await - .is_ok() - {}; - } - Err(e) - } - } + Ok(router.route_notification(¬ification).await?.into()) } /// Handle the `DELETE /m/{message_id}` route From 86b5d9b5a8cf35c0bc8c4a85b4a8ae0ecc3b5768 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 23 May 2022 16:52:41 -0700 Subject: [PATCH 11/12] f r's --- autoendpoint/Cargo.toml | 2 +- autoendpoint/src/routers/fcm/client.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/autoendpoint/Cargo.toml b/autoendpoint/Cargo.toml index 8775fbbe4..c8d7a3da1 100644 --- a/autoendpoint/Cargo.toml +++ b/autoendpoint/Cargo.toml @@ -58,7 +58,7 @@ validator_derive = "0.14" yup-oauth2 = "4.1.2" # 5.0+ requires tokio 1.1+ # For mockito test debugging -# ureq={ version="2.4", features=["json"] } +#ureq={ version="2.4", features=["json"] } [dev-dependencies] mockall = "0.8.3" # 0.9+ requires reworking tests diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs index 943ead83f..ee5a0af4e 100644 --- a/autoendpoint/src/routers/fcm/client.rs +++ b/autoendpoint/src/routers/fcm/client.rs @@ -78,7 +78,7 @@ impl FcmClient { .expect("Project ID is not URL-safe"), gcm_endpoint: settings .base_url - .join("gcm/send") + .join("fcm/send") .expect("GCM Project ID is not URL-safe"), timeout: Duration::from_secs(settings.timeout as u64), max_data: settings.max_data, From 79f83c4e2730d0623da944e2041adf1b1d000cc9 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 23 May 2022 21:36:20 -0700 Subject: [PATCH 12/12] f r's --- autoendpoint/src/error.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index 833a46aa2..36c80a890 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -152,11 +152,6 @@ impl ApiErrorKind { } } - /// Should the User record be removed? - pub fn user_still_valid(&self) -> bool { - !matches!(self, ApiErrorKind::InvalidToken) - } - /// Specify the label to use for metrics reporting. pub fn metric_label(&self) -> &'static str { match self {