From bf7c114a6d508c85484f8a083a49b3b13bcc947d Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Wed, 14 Feb 2024 18:50:35 +0100 Subject: [PATCH 1/8] database migration, deprecations --- .../down.sql | 1 + .../up.sql | 7 +++++++ src/db/schema.rs | 16 ++++++++++++++++ tests/api/multiple_collections.rs | 4 ++-- 4 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 migrations/2024-02-14-165616_user_subscription_transitions/down.sql create mode 100644 migrations/2024-02-14-165616_user_subscription_transitions/up.sql diff --git a/migrations/2024-02-14-165616_user_subscription_transitions/down.sql b/migrations/2024-02-14-165616_user_subscription_transitions/down.sql new file mode 100644 index 00000000..8d1cfb2d --- /dev/null +++ b/migrations/2024-02-14-165616_user_subscription_transitions/down.sql @@ -0,0 +1 @@ +DROP TABLE user_subscription_transitions; diff --git a/migrations/2024-02-14-165616_user_subscription_transitions/up.sql b/migrations/2024-02-14-165616_user_subscription_transitions/up.sql new file mode 100644 index 00000000..057b7846 --- /dev/null +++ b/migrations/2024-02-14-165616_user_subscription_transitions/up.sql @@ -0,0 +1,7 @@ +CREATE TABLE user_subscription_transitions ( + id BIGSERIAL PRIMARY KEY, + user_id BIGSERIAL REFERENCES users (id) ON DELETE CASCADE, + old_subscription_type subscription_type, + new_subscription_type subscription_type, + created_at TIMESTAMP NOT NULL DEFAULT now() +); diff --git a/src/db/schema.rs b/src/db/schema.rs index 5580fe66..1442a84d 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -255,6 +255,20 @@ diesel::table! { } } +diesel::table! { + use diesel::sql_types::*; + use crate::db::types::*; + use super::sql_types::SubscriptionType; + + user_subscription_transitions (id) { + id -> Int8, + user_id -> Int8, + old_subscription_type -> Nullable, + new_subscription_type -> Nullable, + created_at -> Timestamp, + } +} + diesel::table! { use diesel::sql_types::*; use crate::db::types::*; @@ -307,6 +321,7 @@ diesel::joinable!(collection_items -> users (user_id)); diesel::joinable!(multiple_collections -> users (user_id)); diesel::joinable!(playground -> users (user_id)); diesel::joinable!(settings -> users (user_id)); +diesel::joinable!(user_subscription_transitions -> users (user_id)); diesel::allow_tables_to_appear_in_same_query!( activity_pings, @@ -324,6 +339,7 @@ diesel::allow_tables_to_appear_in_same_query!( playground, raw_webhook_events_tokens, settings, + user_subscription_transitions, users, webhook_events, ); diff --git a/tests/api/multiple_collections.rs b/tests/api/multiple_collections.rs index 98f37ac9..782c8113 100644 --- a/tests/api/multiple_collections.rs +++ b/tests/api/multiple_collections.rs @@ -905,10 +905,10 @@ async fn test_create_and_get_many_collections() -> Result<(), Error> { body.as_array().unwrap().iter().reduce(|acc, next| { let t1 = DateTime::parse_from_rfc3339(acc["created_at"].as_str().unwrap()) .unwrap() - .timestamp_nanos(); + .timestamp_nanos_opt(); let t2 = DateTime::parse_from_rfc3339(next["created_at"].as_str().unwrap()) .unwrap() - .timestamp_nanos(); + .timestamp_nanos_opt(); assert!(t1 < t2); next }); From 83beab0a1ccfb77c6c88b145392e1602fb79530a Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 15 Feb 2024 20:56:29 +0100 Subject: [PATCH 2/8] implement sub state transition recording, test scaffolding --- .../up.sql | 4 +- src/db/fxa_webhook.rs | 21 +++- src/db/model.rs | 19 ++++ src/db/schema.rs | 4 +- tests/api/fxa_webhooks.rs | 104 +++++++++++++++++- ...token_subscription_state_change_to_5m.json | 16 +++ ..._token_subscription_state_change_to_5m.txt | 1 + tests/helpers/app.rs | 12 ++ .../userinfo_core_subscription.json | 28 +++++ 9 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 tests/data/set_tokens/set_token_subscription_state_change_to_5m.json create mode 100644 tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt create mode 100644 tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json diff --git a/migrations/2024-02-14-165616_user_subscription_transitions/up.sql b/migrations/2024-02-14-165616_user_subscription_transitions/up.sql index 057b7846..8f8bd636 100644 --- a/migrations/2024-02-14-165616_user_subscription_transitions/up.sql +++ b/migrations/2024-02-14-165616_user_subscription_transitions/up.sql @@ -1,7 +1,7 @@ CREATE TABLE user_subscription_transitions ( id BIGSERIAL PRIMARY KEY, user_id BIGSERIAL REFERENCES users (id) ON DELETE CASCADE, - old_subscription_type subscription_type, - new_subscription_type subscription_type, + old_subscription_type subscription_type NOT NULL, + new_subscription_type subscription_type NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT now() ); diff --git a/src/db/fxa_webhook.rs b/src/db/fxa_webhook.rs index 4aa46ddc..df6ab64c 100644 --- a/src/db/fxa_webhook.rs +++ b/src/db/fxa_webhook.rs @@ -18,6 +18,7 @@ use diesel::prelude::*; use diesel::ExpressionMethods; use serde_json::json; +use super::model::SubscriptionChangeInsert; use super::types::{FxaEventStatus, Subscription}; pub fn log_failed_webhook_event( @@ -179,6 +180,7 @@ pub async fn update_subscription_state_from_webhook( .count() .first::(&mut conn)? != 0; + println!("ignore: {}", ignore); if !ignore { let id = insert_into(schema::webhook_events::table) .values(fxa_event) @@ -191,7 +193,7 @@ pub async fn update_subscription_state_from_webhook( }; if subscription == Subscription::Core { // drop permissions - if let Some(basket) = &**basket { + if let Some(basket) = basket.get_ref() { if let Err(e) = newsletter::unsubscribe(&mut conn, &user, basket).await { error!("error unsubscribing user: {}", e); } @@ -217,7 +219,7 @@ pub async fn update_subscription_state_from_webhook( ) .set(schema::webhook_events::status.eq(FxaEventStatus::Processed)) .execute(&mut conn)?; - return Ok(()); + // return Ok(()); } Err(e) => { diesel::update( @@ -228,6 +230,21 @@ pub async fn update_subscription_state_from_webhook( return Err(e.into()); } } + // Record the subscription state change in its table. + let old_subscription = user.get_subscription_type(); + if let Some(old_subscription) = old_subscription { + // We have the user id, the old and new subscription, store it. + let subscription_change = SubscriptionChangeInsert { + user_id: user.id, + old_subscription_type: old_subscription, + new_subscription_type: subscription, + created_at: update.change_time.naive_utc(), + }; + insert_into(schema::user_subscription_transitions::table) + .values(subscription_change) + .execute(&mut conn)?; + } + return Ok(()); } } diff --git a/src/db/model.rs b/src/db/model.rs index b7a9b070..a7e10456 100644 --- a/src/db/model.rs +++ b/src/db/model.rs @@ -294,3 +294,22 @@ pub struct AIHelpHistoryMessage { pub request: Value, pub response: Value, } + +#[derive(Insertable)] +#[diesel(table_name = user_subscription_transitions)] +pub struct SubscriptionChangeInsert { + pub user_id: i64, + pub old_subscription_type: Subscription, + pub new_subscription_type: Subscription, + pub created_at: NaiveDateTime, +} + +#[derive(Queryable, Debug)] +#[diesel(table_name = user_subscription_transitions)] +pub struct SubscriptionChangeTestQuery { + pub id: i64, + pub user_id: i64, + pub old_subscription_type: Subscription, + pub new_subscription_type: Subscription, + pub created_at: NaiveDateTime, +} diff --git a/src/db/schema.rs b/src/db/schema.rs index 1442a84d..7b2f4c0b 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -263,8 +263,8 @@ diesel::table! { user_subscription_transitions (id) { id -> Int8, user_id -> Int8, - old_subscription_type -> Nullable, - new_subscription_type -> Nullable, + old_subscription_type -> SubscriptionType, + new_subscription_type -> SubscriptionType, created_at -> Timestamp, } } diff --git a/tests/api/fxa_webhooks.rs b/tests/api/fxa_webhooks.rs index 20182b11..57bf1d2a 100644 --- a/tests/api/fxa_webhooks.rs +++ b/tests/api/fxa_webhooks.rs @@ -1,6 +1,4 @@ -use std::thread; -use std::time::Duration; - +use crate::helpers::app::drop_stubr; use crate::helpers::app::test_app_with_login; use crate::helpers::db::reset; use crate::helpers::http_client::PostPayload; @@ -11,13 +9,18 @@ use actix_http::StatusCode; use actix_web::test; use anyhow::anyhow; use anyhow::Error; +use chrono::NaiveDateTime; use diesel::prelude::*; +use rumba::db::model::SubscriptionChangeTestQuery; use rumba::db::model::WebHookEventQuery; use rumba::db::schema; use rumba::db::types::FxaEvent; use rumba::db::types::FxaEventStatus; +use rumba::db::types::Subscription; use rumba::db::Pool; use serde_json::json; +use std::thread; +use std::time::Duration; use stubr::{Config, Stubr}; const TEN_MS: std::time::Duration = Duration::from_millis(10); @@ -359,3 +362,98 @@ async fn change_profile_test() -> Result<(), Error> { drop(stubr); Ok(()) } + +#[actix_rt::test] +async fn record_subscription_state_transitions_test() -> Result<(), Error> { + let stubr = Stubr::start_blocking_with( + vec!["tests/stubs", "tests/test_specific_stubs/core_user"], + Config { + port: Some(4321), + latency: None, + global_delay: None, + verbose: true, + verify: false, + }, + ); + let pool = reset()?; + let app = test_app_with_login(&pool).await?; + let service = test::init_service(app).await; + let mut logged_in_client = TestHttpClient::new(service).await; + let whoami = logged_in_client + .get("/api/v1/whoami", Some(vec![("X-Appengine-Country", "IS")])) + .await; + assert!(whoami.response().status().is_success()); + let json = read_json(whoami).await; + assert_eq!(json["username"], "TEST_SUB"); + assert_eq!(json["subscription_type"], "core", "Subscription type wrong"); + + // verify there are no state transitions in the table + let mut conn = pool.get()?; + let count = schema::user_subscription_transitions::table + .count() + .first::(&mut conn)?; + assert_eq!(count, 0); + + // Create a transition to 5m and check if it is recorded. + { + let set_token = + include_str!("../data/set_tokens/set_token_subscription_state_change_to_5m.txt"); + let res = logged_in_client.trigger_webhook(set_token).await; + assert!(res.response().status().is_success()); + + // check the transition is recorded + let transitions = schema::user_subscription_transitions::table + .load::(&mut conn)?; + assert_eq!(transitions.len(), 1); + assert_eq!(transitions[0].old_subscription_type, Subscription::Core); + assert_eq!( + transitions[0].new_subscription_type, + Subscription::MdnPlus_5m + ); + assert_eq!(transitions[0].user_id, 1); + assert_eq!( + transitions[0].created_at, + NaiveDateTime::from_timestamp_opt(1654425317, 0).unwrap() + ); + } + + // Now create a transition to 10m and check the table again + { + // let set_token = + // include_str!("../data/set_tokens/set_token_subscription_state_change_to_10m.txt"); + // let res = logged_in_client.trigger_webhook(set_token).await; + // assert!(res.response().status().is_success()); + + // // check the transition is recorded + // let transitions = schema::user_subscription_transitions::table + // .load::(&mut conn)?; + // assert_eq!(transitions.len(), 2); + // assert_eq!(transitions[0].old_subscription_type, Subscription::Core); + // assert_eq!( + // transitions[0].new_subscription_type, + // Subscription::MdnPlus_5m + // ); + // assert_eq!(transitions[0].user_id, 1); + // assert_eq!( + // transitions[0].created_at, + // NaiveDateTime::from_timestamp_opt(1654425317, 0).unwrap() + // ); + } + + drop_stubr(stubr).await; + Ok(()) + + // let res = logged_in_client.trigger_webhook(set_token).await; + // assert!(res.response().status().is_success()); + + // // The second event must be ignored. + // assert_last_fxa_webhook( + // &pool, + // "TEST_SUB", + // FxaEvent::SubscriptionStateChange, + // FxaEventStatus::Ignored, + // )?; + + // drop(stubr); + // Ok(()) +} diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_5m.json b/tests/data/set_tokens/set_token_subscription_state_change_to_5m.json new file mode 100644 index 00000000..838b4bd4 --- /dev/null +++ b/tests/data/set_tokens/set_token_subscription_state_change_to_5m.json @@ -0,0 +1,16 @@ +{ + "aud": "ed18cbc69ec23491", + "events": { + "https://schemas.accounts.firefox.com/event/subscription-state-change": { + "capabilities": [ + "mdn_plus_5m" + ], + "changeTime": 1654425317000, + "isActive": true + } + }, + "iat": 1654425318.762, + "iss": "http://localhost:4321", + "jti": "eeb9d9ae-90d5-499f-8056-94eaf635e09b", + "sub": "TEST_SUB" +} diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt b/tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt new file mode 100644 index 00000000..d87932dd --- /dev/null +++ b/tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt @@ -0,0 +1 @@ +eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6IlRFU1RfS0VZIn0.eyJhdWQiOiJlZDE4Y2JjNjllYzIzNDkxIiwiZXZlbnRzIjp7Imh0dHBzOi8vc2NoZW1hcy5hY2NvdW50cy5maXJlZm94LmNvbS9ldmVudC9zdWJzY3JpcHRpb24tc3RhdGUtY2hhbmdlIjp7ImNhcGFiaWxpdGllcyI6WyJtZG5fcGx1c181bSJdLCJjaGFuZ2VUaW1lIjoxNjU0NDI1MzE3MDAwLCJpc0FjdGl2ZSI6dHJ1ZX19LCJpYXQiOjE2NTQ0MjUzMTguNzYyLCJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjQzMjEiLCJqdGkiOiJlZWI5ZDlhZS05MGQ1LTQ5OWYtODA1Ni05NGVhZjYzNWUwOWIiLCJzdWIiOiJURVNUX1NVQiJ9.KIsJjcOZ_Dgzu1VdWraaIxW2jYFiAJBA1PReVWit9Z5mNb_-LjsUjTFz4WamDHGQ5DqO6dIjo3oCm857fW9LxH6-o75l3an1tq5EJ0kGrsqC98tc-IlAAIEGDNgNSOH4-uqE1MEIIF6x6Ys2MnftZmGWiM8fbQZ9zJdmbmo1_66BFRy1_Zm1PF9UbAWidNI6daFRBmcGI5WfIsAhIAa4kn7bhXLJQYHIgTqeqyKk_EDSt7Xd-4A2i_YWb0fdcu5b46tWJGL8MRuCxrll9tteeZfYQXmm3i5mk85AOGPVuETHi3H04UWf9a4Q792FF3YhUzANXrM5tfoQEQPBwAy41A \ No newline at end of file diff --git a/tests/helpers/app.rs b/tests/helpers/app.rs index c8bcf829..f14b34b2 100644 --- a/tests/helpers/app.rs +++ b/tests/helpers/app.rs @@ -1,6 +1,7 @@ use actix_http::body::BoxBody; use actix_http::Request; use actix_identity::IdentityMiddleware; +use actix_rt::time::sleep; use actix_rt::Arbiter; use actix_session::storage::CookieSessionStore; use actix_session::SessionMiddleware; @@ -24,6 +25,7 @@ use rumba::db::{Pool, SupaPool}; use rumba::fxa::LoginManager; use rumba::settings::SETTINGS; use slog::{slog_o, Drain}; +use std::time::Duration; use stubr::{Config, Stubr}; use super::db::reset; @@ -160,3 +162,13 @@ pub async fn init_test( let logged_in_client = TestHttpClient::new(service).await; Ok((logged_in_client, stubr)) } + +// We drop the stubr instances explicitly and add a small +// delay to give it time to shutdown. This is necessary to fix +// flaky tests that fail because leftover stubr instances are still +// holding on the network port in some cases. +const STUBR_POST_DROP_SLEEP: std::time::Duration = Duration::from_millis(10); +pub async fn drop_stubr(stubr: Stubr) { + drop(stubr); + sleep(STUBR_POST_DROP_SLEEP).await; +} diff --git a/tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json b/tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json new file mode 100644 index 00000000..39e4eb35 --- /dev/null +++ b/tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json @@ -0,0 +1,28 @@ +{ + "uuid": "userinfo", + "priority": 2, + "request": { + "method": "GET", + "url": "/v1/profile" + }, + "response": { + "status": 200, + "headers": { + "Content-Type": "application/json" + }, + "jsonBody": { + "email": "test@test.com", + "locale": "en", + "displayName": "Test Mcface", + "avatar": "https://i1.sndcdn.com/avatars-000460644402-0iiiub-t500x500.jpg", + "avatarDefault": false, + "amrValues": [ + "pwd" + ], + "uid": "TEST_SUB", + "subscriptions": [ + "mdn_plus" + ] + } + } +} From 7fd70f23b81aeb1eb5b66fedb62be37bc0b4457b Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Thu, 15 Feb 2024 23:39:57 +0100 Subject: [PATCH 3/8] implement a helper function to encode webhook payloads --- src/db/fxa_webhook.rs | 1 - tests/helpers/mod.rs | 1 + tests/helpers/set_tokens.rs | 110 ++++++++++++++++++ .../userinfo_core_subscription.json | 28 ----- 4 files changed, 111 insertions(+), 29 deletions(-) create mode 100644 tests/helpers/set_tokens.rs delete mode 100644 tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json diff --git a/src/db/fxa_webhook.rs b/src/db/fxa_webhook.rs index df6ab64c..8a4f6293 100644 --- a/src/db/fxa_webhook.rs +++ b/src/db/fxa_webhook.rs @@ -180,7 +180,6 @@ pub async fn update_subscription_state_from_webhook( .count() .first::(&mut conn)? != 0; - println!("ignore: {}", ignore); if !ignore { let id = insert_into(schema::webhook_events::table) .values(fxa_event) diff --git a/tests/helpers/mod.rs b/tests/helpers/mod.rs index f83d4106..5a32922d 100644 --- a/tests/helpers/mod.rs +++ b/tests/helpers/mod.rs @@ -14,6 +14,7 @@ pub mod api_assertions; pub mod app; pub mod db; pub mod http_client; +pub mod set_tokens; pub type RumbaTestResponse = ServiceResponse>; diff --git a/tests/helpers/set_tokens.rs b/tests/helpers/set_tokens.rs new file mode 100644 index 00000000..b973eb46 --- /dev/null +++ b/tests/helpers/set_tokens.rs @@ -0,0 +1,110 @@ +use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; +use serde_json::Value; + +/// Returns a proper token from a json file path. +pub fn token_from_json_file(json_path: &str) -> String { + let json_str = std::fs::read_to_string(json_path).unwrap(); + token_from_json(&json_str) +} +/// Returns an invalid token from a json file path. +pub fn invalid_token_from_json_file(json_path: &str) -> String { + let json_str = std::fs::read_to_string(json_path).unwrap(); + invalid_token_from_json(&json_str) +} + +/// Returns a proper token from a json string. +pub fn token_from_json(json_str: &str) -> String { + token_from_json_and_pem(&json_str, Pem::Valid) +} +/// Returns an invalid token from a json string. +pub fn invalid_token_from_json(json_str: &str) -> String { + token_from_json_and_pem(&json_str, Pem::Invalid) +} + +enum Pem { + Valid, + Invalid, +} + +fn token_from_json_and_pem(json_str: &str, pem_variant: Pem) -> String { + let claims: Value = serde_json::from_str(&json_str).unwrap(); + let header = Header { + kid: Some("TEST_KEY".to_owned()), + alg: Algorithm::RS256, + ..Default::default() + }; + + let encoding_key = match pem_variant { + Pem::Valid => EncodingKey::from_rsa_pem(include_bytes!("../data/rumba-test.pem")).unwrap(), + Pem::Invalid => { + EncodingKey::from_rsa_pem(include_bytes!("../data/rumba-test-invalid.pem")).unwrap() + } + }; + + encode(&header, &claims, &encoding_key).unwrap() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + + #[test] + #[ignore] + fn verify() { + let proper = vec![ + ( + "tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt", + "tests/data/set_tokens/set_token_subscription_state_change_to_5m.json", + ), + ( + "tests/data/set_tokens/set_token_delete_user.txt", + "tests/data/set_tokens/set_token_delete_user.json", + ), + ( + "tests/data/set_tokens/set_token_profile_change.txt", + "tests/data/set_tokens/set_token_profile_change.json", + ), + ( + "tests/data/set_tokens/set_token_subscription_state_change_to_10m.txt", + "tests/data/set_tokens/set_token_subscription_state_change_to_10m.json", + ), + ( + "tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt", + "tests/data/set_tokens/set_token_subscription_state_change_to_5m.json", + ), + ( + "tests/data/set_tokens/set_token_subscription_state_change_to_core.txt", + "tests/data/set_tokens/set_token_subscription_state_change_to_core.json", + ), + ( + "tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.txt", + "tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.json", + ), + ]; + let invalid = vec![( + "tests/data/set_tokens/set_token_delete_user_invalid.txt", + "tests/data/set_tokens/set_token_delete_user.json", + )]; + println!(""); + for (set_token, json_file) in proper { + println!("set_token: {set_token}"); + let token = fs::read_to_string(set_token).unwrap(); + let dynamic = token_from_json_file(json_file); + // let len = token.len(); + // println!("{} ... {}", &token[..80], &token[(len - 80)..]); + // println!("{} ... {}", &dynamic[..80], &dynamic[(len - 80)..]); + assert_eq!(token, dynamic); + } + + for (set_token, json_file) in invalid { + println!("set_token: {set_token}"); + let token = fs::read_to_string(set_token).unwrap(); + let dynamic = invalid_token_from_json_file(json_file); + // let len = token.len(); + // println!("{} ... {}", &token[..80], &token[(len - 80)..]); + // println!("{} ... {}", &dynamic[..80], &dynamic[(len - 80)..]); + assert_eq!(token, dynamic); + } + } +} diff --git a/tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json b/tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json deleted file mode 100644 index 39e4eb35..00000000 --- a/tests/test_specific_stubs/fxa_webhooks/userinfo_core_subscription.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "uuid": "userinfo", - "priority": 2, - "request": { - "method": "GET", - "url": "/v1/profile" - }, - "response": { - "status": 200, - "headers": { - "Content-Type": "application/json" - }, - "jsonBody": { - "email": "test@test.com", - "locale": "en", - "displayName": "Test Mcface", - "avatar": "https://i1.sndcdn.com/avatars-000460644402-0iiiub-t500x500.jpg", - "avatarDefault": false, - "amrValues": [ - "pwd" - ], - "uid": "TEST_SUB", - "subscriptions": [ - "mdn_plus" - ] - } - } -} From ceaa5808e066502014d3f686cee73a68f6de7f32 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Fri, 16 Feb 2024 12:02:08 +0100 Subject: [PATCH 4/8] mores tests, moved fixtures around, amended docs for new set_token helper --- src/db/model.rs | 2 +- tests/api/fxa_webhooks.rs | 119 ++++++++++-------- tests/data/set_tokens/README.md | 5 +- ...oken_subscription_state_change_to_10m.json | 4 +- tests/data/set_tokens/test/README.md | 1 + .../{ => test}/set_token_delete_user.txt | 0 .../set_token_delete_user_invalid.txt | 0 .../{ => test}/set_token_profile_change.txt | 0 ...token_subscription_state_change_to_10m.txt | 0 ..._token_subscription_state_change_to_5m.txt | 0 ...oken_subscription_state_change_to_core.txt | 0 ...cription_state_change_to_core_inactive.txt | 0 tests/helpers/set_tokens.rs | 50 ++++---- 13 files changed, 101 insertions(+), 80 deletions(-) create mode 100644 tests/data/set_tokens/test/README.md rename tests/data/set_tokens/{ => test}/set_token_delete_user.txt (100%) rename tests/data/set_tokens/{ => test}/set_token_delete_user_invalid.txt (100%) rename tests/data/set_tokens/{ => test}/set_token_profile_change.txt (100%) rename tests/data/set_tokens/{ => test}/set_token_subscription_state_change_to_10m.txt (100%) rename tests/data/set_tokens/{ => test}/set_token_subscription_state_change_to_5m.txt (100%) rename tests/data/set_tokens/{ => test}/set_token_subscription_state_change_to_core.txt (100%) rename tests/data/set_tokens/{ => test}/set_token_subscription_state_change_to_core_inactive.txt (100%) diff --git a/src/db/model.rs b/src/db/model.rs index a7e10456..a3227e0a 100644 --- a/src/db/model.rs +++ b/src/db/model.rs @@ -306,7 +306,7 @@ pub struct SubscriptionChangeInsert { #[derive(Queryable, Debug)] #[diesel(table_name = user_subscription_transitions)] -pub struct SubscriptionChangeTestQuery { +pub struct SubscriptionChangeQuery { pub id: i64, pub user_id: i64, pub old_subscription_type: Subscription, diff --git a/tests/api/fxa_webhooks.rs b/tests/api/fxa_webhooks.rs index 57bf1d2a..0f6e4269 100644 --- a/tests/api/fxa_webhooks.rs +++ b/tests/api/fxa_webhooks.rs @@ -4,6 +4,9 @@ use crate::helpers::db::reset; use crate::helpers::http_client::PostPayload; use crate::helpers::http_client::TestHttpClient; use crate::helpers::read_json; +use crate::helpers::set_tokens::invalid_token_from_json_file; +use crate::helpers::set_tokens::token_from_claim; +use crate::helpers::set_tokens::token_from_file; use crate::helpers::wait_for_stubr; use actix_http::StatusCode; use actix_web::test; @@ -11,7 +14,7 @@ use anyhow::anyhow; use anyhow::Error; use chrono::NaiveDateTime; use diesel::prelude::*; -use rumba::db::model::SubscriptionChangeTestQuery; +use rumba::db::model::SubscriptionChangeQuery; use rumba::db::model::WebHookEventQuery; use rumba::db::schema; use rumba::db::types::FxaEvent; @@ -19,6 +22,7 @@ use rumba::db::types::FxaEventStatus; use rumba::db::types::Subscription; use rumba::db::Pool; use serde_json::json; +use serde_json::Value; use std::thread; use std::time::Duration; use stubr::{Config, Stubr}; @@ -69,7 +73,7 @@ fn assert_last_fxa_webhook_with_retry( #[stubr::mock(port = 4321)] async fn subscription_state_change_to_10m_test() -> Result<(), Error> { let set_token = - include_str!("../data/set_tokens/set_token_subscription_state_change_to_10m.txt"); + token_from_file("tests/data/set_tokens/set_token_subscription_state_change_to_10m.json"); let pool = reset()?; wait_for_stubr().await?; let app = test_app_with_login(&pool).await?; @@ -86,7 +90,7 @@ async fn subscription_state_change_to_10m_test() -> Result<(), Error> { "Subscription type wrong" ); - let res = logged_in_client.trigger_webhook(set_token).await; + let res = logged_in_client.trigger_webhook(&set_token).await; assert!(res.response().status().is_success()); let whoami = logged_in_client @@ -104,7 +108,7 @@ async fn subscription_state_change_to_10m_test() -> Result<(), Error> { FxaEventStatus::Processed, )?; - let res = logged_in_client.trigger_webhook(set_token).await; + let res = logged_in_client.trigger_webhook(&set_token).await; assert!(res.response().status().is_success()); // The second event must be ignored. @@ -123,16 +127,17 @@ async fn subscription_state_change_to_10m_test() -> Result<(), Error> { #[stubr::mock(port = 4321)] async fn subscription_state_change_to_core_test_empty_subscription() -> Result<(), Error> { let set_token = - include_str!("../data/set_tokens/set_token_subscription_state_change_to_core.txt"); - subscription_state_change_to_core_test(set_token).await + token_from_file("tests/data/set_tokens/set_token_subscription_state_change_to_core.json"); + subscription_state_change_to_core_test(&set_token).await } #[actix_rt::test] #[stubr::mock(port = 4321)] async fn subscription_state_change_to_core_test_inactive() -> Result<(), Error> { - let set_token = - include_str!("../data/set_tokens/set_token_subscription_state_change_to_core_inactive.txt"); - subscription_state_change_to_core_test(set_token).await + let set_token = token_from_file( + "tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.json", + ); + subscription_state_change_to_core_test(&set_token).await } async fn subscription_state_change_to_core_test(set_token: &str) -> Result<(), Error> { @@ -176,7 +181,7 @@ async fn subscription_state_change_to_core_test(set_token: &str) -> Result<(), E #[actix_rt::test] async fn delete_user_test() -> Result<(), Error> { - let set_token = include_str!("../data/set_tokens/set_token_delete_user.txt"); + let set_token = token_from_file("tests/data/set_tokens/set_token_delete_user.json"); let pool = reset()?; let stubr = Stubr::start_blocking_with( vec!["tests/stubs", "tests/test_specific_stubs/collections"], @@ -237,7 +242,7 @@ async fn delete_user_test() -> Result<(), Error> { .await; assert_eq!(res.response().status(), 201); - let res = logged_in_client.trigger_webhook(set_token).await; + let res = logged_in_client.trigger_webhook(&set_token).await; assert!(res.response().status().is_success()); let whoami = logged_in_client @@ -259,7 +264,8 @@ async fn delete_user_test() -> Result<(), Error> { #[actix_rt::test] #[stubr::mock(port = 4321)] async fn invalid_set_test() -> Result<(), Error> { - let set_token = include_str!("../data/set_tokens/set_token_delete_user_invalid.txt"); + let set_token = + invalid_token_from_json_file("tests/data/set_tokens/set_token_delete_user.json"); let pool = reset()?; wait_for_stubr().await?; let app = test_app_with_login(&pool).await?; @@ -274,7 +280,7 @@ async fn invalid_set_test() -> Result<(), Error> { assert_eq!(json["geo"]["country_iso"], "IS"); assert_eq!(json["is_authenticated"], true); - let res = logged_in_client.trigger_webhook(set_token).await; + let res = logged_in_client.trigger_webhook(&set_token).await; assert_eq!(res.response().status(), StatusCode::OK); @@ -301,7 +307,7 @@ async fn change_profile_test() -> Result<(), Error> { ); wait_for_stubr().await?; - let set_token = include_str!("../data/set_tokens/set_token_profile_change.txt"); + let set_token = token_from_file("tests/data/set_tokens/set_token_profile_change.json"); let pool = reset()?; let app = test_app_with_login(&pool).await?; let service = test::init_service(app).await; @@ -330,7 +336,7 @@ async fn change_profile_test() -> Result<(), Error> { thread::sleep(TEN_MS); - let res = logged_in_client.trigger_webhook(set_token).await; + let res = logged_in_client.trigger_webhook(&set_token).await; assert!(res.response().status().is_success()); let mut tries = 100; @@ -397,13 +403,13 @@ async fn record_subscription_state_transitions_test() -> Result<(), Error> { // Create a transition to 5m and check if it is recorded. { let set_token = - include_str!("../data/set_tokens/set_token_subscription_state_change_to_5m.txt"); - let res = logged_in_client.trigger_webhook(set_token).await; + token_from_file("tests/data/set_tokens/set_token_subscription_state_change_to_5m.json"); + let res = logged_in_client.trigger_webhook(&set_token).await; assert!(res.response().status().is_success()); // check the transition is recorded let transitions = schema::user_subscription_transitions::table - .load::(&mut conn)?; + .load::(&mut conn)?; assert_eq!(transitions.len(), 1); assert_eq!(transitions[0].old_subscription_type, Subscription::Core); assert_eq!( @@ -417,43 +423,52 @@ async fn record_subscription_state_transitions_test() -> Result<(), Error> { ); } - // Now create a transition to 10m and check the table again + // Now create a later transition to 10m and check the table again { - // let set_token = - // include_str!("../data/set_tokens/set_token_subscription_state_change_to_10m.txt"); - // let res = logged_in_client.trigger_webhook(set_token).await; - // assert!(res.response().status().is_success()); - - // // check the transition is recorded - // let transitions = schema::user_subscription_transitions::table - // .load::(&mut conn)?; - // assert_eq!(transitions.len(), 2); - // assert_eq!(transitions[0].old_subscription_type, Subscription::Core); - // assert_eq!( - // transitions[0].new_subscription_type, - // Subscription::MdnPlus_5m - // ); - // assert_eq!(transitions[0].user_id, 1); - // assert_eq!( - // transitions[0].created_at, - // NaiveDateTime::from_timestamp_opt(1654425317, 0).unwrap() - // ); + let json_str = std::fs::read_to_string( + "tests/data/set_tokens/set_token_subscription_state_change_to_10m.json", + ) + .unwrap(); + let mut claim: Value = serde_json::from_str(&json_str).unwrap(); + // add time to the event to be sure it is after the previous event + claim["iat"] = json!(1654425317000i64 + 300000); + claim["events"]["https://schemas.accounts.firefox.com/event/subscription-state-change"] + ["changeTime"] = json!(1654425317000i64 + 300000); + let set_token = token_from_claim(&claim); + + let res = logged_in_client.trigger_webhook(&set_token).await; + assert!(res.response().status().is_success()); + + // check the transition is recorded + let transitions = schema::user_subscription_transitions::table + .order(schema::user_subscription_transitions::created_at) + .load::(&mut conn)?; + assert_eq!(transitions.len(), 2); + assert_eq!(transitions[0].old_subscription_type, Subscription::Core); + assert_eq!( + transitions[0].new_subscription_type, + Subscription::MdnPlus_5m + ); + assert_eq!( + transitions[1].old_subscription_type, + Subscription::MdnPlus_5m + ); + assert_eq!( + transitions[1].new_subscription_type, + Subscription::MdnPlus_10m + ); + assert_eq!(transitions[0].user_id, 1); + assert_eq!(transitions[1].user_id, 1); + assert_eq!( + transitions[0].created_at, + NaiveDateTime::from_timestamp_opt(1654425317, 0).unwrap() + ); + assert_eq!( + transitions[1].created_at, + NaiveDateTime::from_timestamp_opt(1654425617, 0).unwrap() + ); } drop_stubr(stubr).await; Ok(()) - - // let res = logged_in_client.trigger_webhook(set_token).await; - // assert!(res.response().status().is_success()); - - // // The second event must be ignored. - // assert_last_fxa_webhook( - // &pool, - // "TEST_SUB", - // FxaEvent::SubscriptionStateChange, - // FxaEventStatus::Ignored, - // )?; - - // drop(stubr); - // Ok(()) } diff --git a/tests/data/set_tokens/README.md b/tests/data/set_tokens/README.md index e811e9aa..8f92f2e2 100644 --- a/tests/data/set_tokens/README.md +++ b/tests/data/set_tokens/README.md @@ -1,5 +1,9 @@ # [Security Event Token (SET)](https://tools.ietf.org/html/rfc8417) generation +## Note +Token generation has moved to helper functions in module `helpers::set_tokens`. For reference, the following steps were used to generate the token files on the command line and are kept here for reference. + +## Manual token generation Install [jq](https://stedolan.github.io/jq/) and [jwt](https://github.com/mike-engel/jwt-cli). Create a file e.g. `set_token_delete_user.json` and run: @@ -7,4 +11,3 @@ Create a file e.g. `set_token_delete_user.json` and run: ```sh jq -r tostring set_token_delete_user.json | jwt encode -S @../rumba-test.pem -A RS256 -k TEST_KEY - > set_token_delete_user.txt ``` - diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_10m.json b/tests/data/set_tokens/set_token_subscription_state_change_to_10m.json index 64371d0a..d2b893d6 100644 --- a/tests/data/set_tokens/set_token_subscription_state_change_to_10m.json +++ b/tests/data/set_tokens/set_token_subscription_state_change_to_10m.json @@ -2,7 +2,9 @@ "aud": "ed18cbc69ec23491", "events": { "https://schemas.accounts.firefox.com/event/subscription-state-change": { - "capabilities": ["mdn_plus_10m"], + "capabilities": [ + "mdn_plus_10m" + ], "changeTime": 1654425317000, "isActive": true } diff --git a/tests/data/set_tokens/test/README.md b/tests/data/set_tokens/test/README.md new file mode 100644 index 00000000..c1c3aed9 --- /dev/null +++ b/tests/data/set_tokens/test/README.md @@ -0,0 +1 @@ +The files in here are only used for testing token helper functions in `helpers::set_tokens`. diff --git a/tests/data/set_tokens/set_token_delete_user.txt b/tests/data/set_tokens/test/set_token_delete_user.txt similarity index 100% rename from tests/data/set_tokens/set_token_delete_user.txt rename to tests/data/set_tokens/test/set_token_delete_user.txt diff --git a/tests/data/set_tokens/set_token_delete_user_invalid.txt b/tests/data/set_tokens/test/set_token_delete_user_invalid.txt similarity index 100% rename from tests/data/set_tokens/set_token_delete_user_invalid.txt rename to tests/data/set_tokens/test/set_token_delete_user_invalid.txt diff --git a/tests/data/set_tokens/set_token_profile_change.txt b/tests/data/set_tokens/test/set_token_profile_change.txt similarity index 100% rename from tests/data/set_tokens/set_token_profile_change.txt rename to tests/data/set_tokens/test/set_token_profile_change.txt diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_10m.txt b/tests/data/set_tokens/test/set_token_subscription_state_change_to_10m.txt similarity index 100% rename from tests/data/set_tokens/set_token_subscription_state_change_to_10m.txt rename to tests/data/set_tokens/test/set_token_subscription_state_change_to_10m.txt diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt b/tests/data/set_tokens/test/set_token_subscription_state_change_to_5m.txt similarity index 100% rename from tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt rename to tests/data/set_tokens/test/set_token_subscription_state_change_to_5m.txt diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_core.txt b/tests/data/set_tokens/test/set_token_subscription_state_change_to_core.txt similarity index 100% rename from tests/data/set_tokens/set_token_subscription_state_change_to_core.txt rename to tests/data/set_tokens/test/set_token_subscription_state_change_to_core.txt diff --git a/tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.txt b/tests/data/set_tokens/test/set_token_subscription_state_change_to_core_inactive.txt similarity index 100% rename from tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.txt rename to tests/data/set_tokens/test/set_token_subscription_state_change_to_core_inactive.txt diff --git a/tests/helpers/set_tokens.rs b/tests/helpers/set_tokens.rs index b973eb46..dd83b923 100644 --- a/tests/helpers/set_tokens.rs +++ b/tests/helpers/set_tokens.rs @@ -2,10 +2,15 @@ use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; use serde_json::Value; /// Returns a proper token from a json file path. -pub fn token_from_json_file(json_path: &str) -> String { +pub fn token_from_file(json_path: &str) -> String { let json_str = std::fs::read_to_string(json_path).unwrap(); token_from_json(&json_str) } + +pub fn token_from_claim(claims: &Value) -> String { + token_from_claim_and_pem(claims, Pem::Valid) +} + /// Returns an invalid token from a json file path. pub fn invalid_token_from_json_file(json_path: &str) -> String { let json_str = std::fs::read_to_string(json_path).unwrap(); @@ -14,11 +19,11 @@ pub fn invalid_token_from_json_file(json_path: &str) -> String { /// Returns a proper token from a json string. pub fn token_from_json(json_str: &str) -> String { - token_from_json_and_pem(&json_str, Pem::Valid) + token_from_json_string_and_pem(&json_str, Pem::Valid) } /// Returns an invalid token from a json string. pub fn invalid_token_from_json(json_str: &str) -> String { - token_from_json_and_pem(&json_str, Pem::Invalid) + token_from_json_string_and_pem(&json_str, Pem::Invalid) } enum Pem { @@ -26,8 +31,12 @@ enum Pem { Invalid, } -fn token_from_json_and_pem(json_str: &str, pem_variant: Pem) -> String { +fn token_from_json_string_and_pem(json_str: &str, pem_variant: Pem) -> String { let claims: Value = serde_json::from_str(&json_str).unwrap(); + token_from_claim_and_pem(&claims, pem_variant) +} + +fn token_from_claim_and_pem(claims: &Value, pem_variant: Pem) -> String { let header = Header { kid: Some("TEST_KEY".to_owned()), alg: Algorithm::RS256, @@ -50,61 +59,52 @@ mod tests { use std::fs; #[test] - #[ignore] fn verify() { let proper = vec![ ( - "tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt", + "tests/data/set_tokens/test/set_token_subscription_state_change_to_5m.txt", "tests/data/set_tokens/set_token_subscription_state_change_to_5m.json", ), ( - "tests/data/set_tokens/set_token_delete_user.txt", + "tests/data/set_tokens/test/set_token_delete_user.txt", "tests/data/set_tokens/set_token_delete_user.json", ), ( - "tests/data/set_tokens/set_token_profile_change.txt", + "tests/data/set_tokens/test/set_token_profile_change.txt", "tests/data/set_tokens/set_token_profile_change.json", ), ( - "tests/data/set_tokens/set_token_subscription_state_change_to_10m.txt", + "tests/data/set_tokens/test/set_token_subscription_state_change_to_10m.txt", "tests/data/set_tokens/set_token_subscription_state_change_to_10m.json", ), ( - "tests/data/set_tokens/set_token_subscription_state_change_to_5m.txt", + "tests/data/set_tokens/test/set_token_subscription_state_change_to_5m.txt", "tests/data/set_tokens/set_token_subscription_state_change_to_5m.json", ), ( - "tests/data/set_tokens/set_token_subscription_state_change_to_core.txt", + "tests/data/set_tokens/test/set_token_subscription_state_change_to_core.txt", "tests/data/set_tokens/set_token_subscription_state_change_to_core.json", ), ( - "tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.txt", + "tests/data/set_tokens/test/set_token_subscription_state_change_to_core_inactive.txt", "tests/data/set_tokens/set_token_subscription_state_change_to_core_inactive.json", ), ]; let invalid = vec![( - "tests/data/set_tokens/set_token_delete_user_invalid.txt", + "tests/data/set_tokens/test/set_token_delete_user_invalid.txt", "tests/data/set_tokens/set_token_delete_user.json", )]; println!(""); for (set_token, json_file) in proper { - println!("set_token: {set_token}"); let token = fs::read_to_string(set_token).unwrap(); - let dynamic = token_from_json_file(json_file); - // let len = token.len(); - // println!("{} ... {}", &token[..80], &token[(len - 80)..]); - // println!("{} ... {}", &dynamic[..80], &dynamic[(len - 80)..]); - assert_eq!(token, dynamic); + let generated = token_from_file(json_file); + assert_eq!(token, generated); } for (set_token, json_file) in invalid { - println!("set_token: {set_token}"); let token = fs::read_to_string(set_token).unwrap(); - let dynamic = invalid_token_from_json_file(json_file); - // let len = token.len(); - // println!("{} ... {}", &token[..80], &token[(len - 80)..]); - // println!("{} ... {}", &dynamic[..80], &dynamic[(len - 80)..]); - assert_eq!(token, dynamic); + let generated = invalid_token_from_json_file(json_file); + assert_eq!(token, generated); } } } From a2bdbf02d87f08662d62de6daac6dd451c3b8336 Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Mon, 19 Feb 2024 19:21:31 +0100 Subject: [PATCH 5/8] tracking some master changes --- Cargo.lock | 1 - Cargo.toml | 22 +++++++++++++++++----- tests/api/fxa_webhooks.rs | 13 +++---------- tests/helpers/app.rs | 3 --- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3fc23e6b..b6d2deae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3736,7 +3736,6 @@ dependencies = [ "stubr-attributes", "thiserror", "tiktoken-rs", - "tokio", "url", "uuid", "validator", diff --git a/Cargo.toml b/Cargo.toml index b2f5b2e2..39755253 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,11 +27,20 @@ actix-session = { version = "0.8", features = ["cookie-session"] } actix-web-httpauth = "0.8" actix-web-lab = "0.19" -diesel = { version = "2", features = ["postgres", "uuid", "r2d2", "chrono", "serde_json"] } +diesel = { version = "2", features = [ + "postgres", + "uuid", + "r2d2", + "chrono", + "serde_json", +] } diesel_migrations = "2" diesel-derive-enum = { version = "2", features = ["postgres"] } pgvector = { version = "0.2", features = ["sqlx"] } -sqlx = { version = "0.7", features = [ "runtime-tokio-rustls", "postgres"], default-features = false } +sqlx = { version = "0.7", features = [ + "runtime-tokio-rustls", + "postgres", +], default-features = false } elasticsearch = "7.17.7-alpha.1" harsh = "0.2" @@ -43,7 +52,7 @@ jsonwebtoken = "8" serde = { version = "1", features = ["derive"] } serde_json = "1" -serde_with = { version = "3", features = ["base64"]} +serde_with = { version = "3", features = ["base64"] } serde_urlencoded = "0.7" form_urlencoded = "1" serde_path_to_error = "0.1" @@ -51,7 +60,11 @@ percent-encoding = "2" config = "0.13" hostname = "0.3" -slog = { version = "2", features = ["max_level_trace", "release_max_level_info", "dynamic-keys"] } +slog = { version = "2", features = [ + "max_level_trace", + "release_max_level_info", + "dynamic-keys", +] } slog-async = "2" slog-envlogger = "2" slog-mozlog-json = "0.1" @@ -90,4 +103,3 @@ sha2 = "0.10" stubr = "0.6" stubr-attributes = "0.6" assert-json-diff = "2" -tokio = { version = "1", features = ["io-util"] } diff --git a/tests/api/fxa_webhooks.rs b/tests/api/fxa_webhooks.rs index 8cb98790..2cce4765 100644 --- a/tests/api/fxa_webhooks.rs +++ b/tests/api/fxa_webhooks.rs @@ -7,8 +7,8 @@ use crate::helpers::read_json; use crate::helpers::set_tokens::invalid_token_from_json_file; use crate::helpers::set_tokens::token_from_claim; use crate::helpers::set_tokens::token_from_file; -use crate::helpers::wait_for_stubr; use actix_http::StatusCode; +use actix_rt::time::sleep; use actix_web::test; use anyhow::anyhow; use anyhow::Error; @@ -177,8 +177,6 @@ async fn subscription_state_change_to_core_test(set_token: &str) -> Result<(), E #[actix_rt::test] async fn delete_user_test() -> Result<(), Error> { - let set_token = token_from_file("tests/data/set_tokens/set_token_delete_user.json"); - let pool = reset()?; let stubr = Stubr::start_blocking_with( vec!["tests/stubs", "tests/test_specific_stubs/collections"], Config { @@ -189,8 +187,8 @@ async fn delete_user_test() -> Result<(), Error> { verify: false, }, ); - let set_token = include_str!("../data/set_tokens/set_token_delete_user.txt"); let pool = reset()?; + let set_token = token_from_file("tests/data/set_tokens/set_token_delete_user.json"); let app = test_app_with_login(&pool).await?; let service = test::init_service(app).await; @@ -300,16 +298,11 @@ async fn whoami_test() -> Result<(), Error> { verify: false, }, ); - wait_for_stubr().await?; - let set_token = token_from_file("tests/data/set_tokens/set_token_profile_change.json"); let pool = reset()?; let app = test_app_with_login(&pool).await?; let service = test::init_service(app).await; let mut logged_in_client = TestHttpClient::new(service).await; - let res = logged_in_client.trigger_webhook(set_token).await; - assert!(res.response().status().is_success()); - let whoami = logged_in_client .get("/api/v1/whoami", Some(vec![("X-Appengine-Country", "IS")])) .await; @@ -329,7 +322,7 @@ async fn whoami_test() -> Result<(), Error> { verify: false, }, ); - let set_token = include_str!("../data/set_tokens/set_token_profile_change.txt"); + let set_token = token_from_file("tests/data/set_tokens/set_token_profile_change.json"); let pool = reset()?; let app = test_app_with_login(&pool).await?; let service = test::init_service(app).await; diff --git a/tests/helpers/app.rs b/tests/helpers/app.rs index dc77744a..f14b34b2 100644 --- a/tests/helpers/app.rs +++ b/tests/helpers/app.rs @@ -1,5 +1,3 @@ -use std::time::Duration; - use actix_http::body::BoxBody; use actix_http::Request; use actix_identity::IdentityMiddleware; @@ -29,7 +27,6 @@ use rumba::settings::SETTINGS; use slog::{slog_o, Drain}; use std::time::Duration; use stubr::{Config, Stubr}; -use tokio::time::sleep; use super::db::reset; use super::http_client::TestHttpClient; From aa8f9e592d4f91ad277d4dda3ee57ee7797abccd Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Tue, 20 Feb 2024 13:58:53 +0100 Subject: [PATCH 6/8] Fix a problem with unknown capabilities getting passed in from the subscription change webhook --- src/db/fxa_webhook.rs | 15 ++++++++++++--- tests/api/fxa_webhooks.rs | 7 +++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/db/fxa_webhook.rs b/src/db/fxa_webhook.rs index 8a4f6293..c36ff046 100644 --- a/src/db/fxa_webhook.rs +++ b/src/db/fxa_webhook.rs @@ -8,7 +8,7 @@ use crate::db::settings::create_or_update_settings; use crate::db::types::FxaEvent; use crate::db::users::get_user_opt; use crate::db::{schema, Pool}; -use crate::fxa::LoginManager; +use crate::fxa::{self, LoginManager}; use actix_rt::ArbiterHandle; use actix_web::web; use basket::Basket; @@ -185,9 +185,18 @@ pub async fn update_subscription_state_from_webhook( .values(fxa_event) .returning(schema::webhook_events::id) .get_result::(&mut conn)?; - let subscription: Subscription = match (update.is_active, update.capabilities.first()) { + // Filter out any unknown subscription types we get passed in + // before we try to get the interesting first element of the + // capabilities array. + let filtered_capabilities = update + .capabilities + .into_iter() + .filter(|&c| c != fxa::types::Subscription::Unknown) + .collect::>(); + let subscription: Subscription = match (update.is_active, filtered_capabilities.first()) + { (false, _) => Subscription::Core, - (true, Some(c)) => Subscription::from(*c), + (true, Some(&c)) => Subscription::from(c), (true, None) => Subscription::Core, }; if subscription == Subscription::Core { diff --git a/tests/api/fxa_webhooks.rs b/tests/api/fxa_webhooks.rs index 2cce4765..7f3387fe 100644 --- a/tests/api/fxa_webhooks.rs +++ b/tests/api/fxa_webhooks.rs @@ -414,17 +414,20 @@ async fn record_subscription_state_transitions_test() -> Result<(), Error> { ); } - // Now create a later transition to 10m and check the table again + // Now create a later transition to 10m and check the table again. { let json_str = std::fs::read_to_string( "tests/data/set_tokens/set_token_subscription_state_change_to_10m.json", ) .unwrap(); let mut claim: Value = serde_json::from_str(&json_str).unwrap(); - // add time to the event to be sure it is after the previous event + // Add some time to the event to be sure it is after the previous event. claim["iat"] = json!(1654425317000i64 + 300000); claim["events"]["https://schemas.accounts.firefox.com/event/subscription-state-change"] ["changeTime"] = json!(1654425317000i64 + 300000); + // We also add some unknown capability to the event to check that they are ignored correctly. + claim["events"]["https://schemas.accounts.firefox.com/event/subscription-state-change"] + ["capabilities"] = json!(["something_unknown", "mdn_plus_10m"]); let set_token = token_from_claim(&claim); let res = logged_in_client.trigger_webhook(&set_token).await; From cad8d70728425f496ffdd8d73b1aaeb91cbe19ed Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Tue, 20 Feb 2024 14:20:54 +0100 Subject: [PATCH 7/8] Skip creating a new vector, get the first element directly from the iterator. --- src/db/fxa_webhook.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/db/fxa_webhook.rs b/src/db/fxa_webhook.rs index c36ff046..2ddb4152 100644 --- a/src/db/fxa_webhook.rs +++ b/src/db/fxa_webhook.rs @@ -188,15 +188,14 @@ pub async fn update_subscription_state_from_webhook( // Filter out any unknown subscription types we get passed in // before we try to get the interesting first element of the // capabilities array. - let filtered_capabilities = update + let capability = update .capabilities .into_iter() .filter(|&c| c != fxa::types::Subscription::Unknown) - .collect::>(); - let subscription: Subscription = match (update.is_active, filtered_capabilities.first()) - { + .next(); + let subscription: Subscription = match (update.is_active, capability) { (false, _) => Subscription::Core, - (true, Some(&c)) => Subscription::from(c), + (true, Some(c)) => Subscription::from(c), (true, None) => Subscription::Core, }; if subscription == Subscription::Core { From 1d51e282d58397e75d710876895bcc3c20bf8cde Mon Sep 17 00:00:00 2001 From: Andi Pieper Date: Tue, 20 Feb 2024 14:31:01 +0100 Subject: [PATCH 8/8] Follow Clippy's hints --- src/db/fxa_webhook.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/db/fxa_webhook.rs b/src/db/fxa_webhook.rs index 2ddb4152..99fba3d5 100644 --- a/src/db/fxa_webhook.rs +++ b/src/db/fxa_webhook.rs @@ -191,8 +191,7 @@ pub async fn update_subscription_state_from_webhook( let capability = update .capabilities .into_iter() - .filter(|&c| c != fxa::types::Subscription::Unknown) - .next(); + .find(|&c| c != fxa::types::Subscription::Unknown); let subscription: Subscription = match (update.is_active, capability) { (false, _) => Subscription::Core, (true, Some(c)) => Subscription::from(c),