From 94dbf88809ecc6b83e69ef665f7bb171195674a7 Mon Sep 17 00:00:00 2001 From: Sammy Khamis Date: Tue, 19 May 2026 11:23:27 -1000 Subject: [PATCH 1/3] Let state machine handle CheckAuthorizationStatus --- components/fxa-client/src/internal/push.rs | 46 ++----------------- .../src/state_machine/transitions.rs | 29 ++++++++++++ 2 files changed, 32 insertions(+), 43 deletions(-) diff --git a/components/fxa-client/src/internal/push.rs b/components/fxa-client/src/internal/push.rs index 14f2352fe2..5eeb23fa03 100644 --- a/components/fxa-client/src/internal/push.rs +++ b/components/fxa-client/src/internal/push.rs @@ -67,15 +67,9 @@ impl FirefoxAccount { }) } PushPayload::PasswordChanged | PushPayload::PasswordReset => { - let status = self.check_authorization_status()?; - // clear any device or client data due to password change. self.clear_devices_and_attached_clients_cache(); - Ok(if !status.active { - AccountEvent::AccountAuthStateChanged - } else { - info!("Password change event, but no action required"); - AccountEvent::Unknown - }) + // Send the event here and let the state machine decide next steps + Ok(AccountEvent::AccountAuthStateChanged) } PushPayload::Unknown => { info!("Unknown Push command."); @@ -138,14 +132,8 @@ pub struct AccountDestroyedPushPayload { #[cfg(test)] mod tests { use super::*; - use crate::internal::http_client::IntrospectResponse; - use crate::internal::http_client::MockFxAClient; - use crate::internal::oauth::RefreshToken; use crate::internal::CachedResponse; use crate::internal::Config; - use mockall::predicate::always; - use mockall::predicate::eq; - use std::sync::Arc; #[test] fn test_deserialize_send_tab_command() { @@ -196,26 +184,12 @@ mod tests { fn test_push_password_reset() { let mut fxa = FirefoxAccount::with_config(Config::stable_dev("12345678", "https://foo.bar")); - let mut client = MockFxAClient::new(); - client - .expect_check_refresh_token_status() - .with(always(), eq("refresh_token")) - .times(1) - .returning(|_, _| Ok(IntrospectResponse { active: false })); - fxa.set_client(Arc::new(client)); - let refresh_token_scopes = std::collections::HashSet::new(); - fxa.state.force_refresh_token(RefreshToken { - token: "refresh_token".to_owned(), - scopes: refresh_token_scopes, - }); - fxa.state.force_current_device_id("my_id"); fxa.devices_cache = Some(CachedResponse { response: vec![], cached_at: 0, etag: "".to_string(), }); let json = "{\"version\":1,\"command\":\"fxaccounts:password_reset\"}"; - assert!(fxa.devices_cache.is_some()); let event = fxa.handle_push_message(json).unwrap(); assert!(matches!(event, AccountEvent::AccountAuthStateChanged)); assert!(fxa.devices_cache.is_none()); @@ -225,28 +199,14 @@ mod tests { fn test_push_password_change() { let mut fxa = FirefoxAccount::with_config(Config::stable_dev("12345678", "https://foo.bar")); - let mut client = MockFxAClient::new(); - client - .expect_check_refresh_token_status() - .with(always(), eq("refresh_token")) - .times(1) - .returning(|_, _| Ok(IntrospectResponse { active: true })); - fxa.set_client(Arc::new(client)); - let refresh_token_scopes = std::collections::HashSet::new(); - fxa.state.force_refresh_token(RefreshToken { - token: "refresh_token".to_owned(), - scopes: refresh_token_scopes, - }); - fxa.state.force_current_device_id("my_id"); fxa.devices_cache = Some(CachedResponse { response: vec![], cached_at: 0, etag: "".to_string(), }); let json = "{\"version\":1,\"command\":\"fxaccounts:password_changed\"}"; - assert!(fxa.devices_cache.is_some()); let event = fxa.handle_push_message(json).unwrap(); - assert!(matches!(event, AccountEvent::Unknown)); + assert!(matches!(event, AccountEvent::AccountAuthStateChanged)); assert!(fxa.devices_cache.is_none()); } #[test] diff --git a/components/fxa-client/src/state_machine/transitions.rs b/components/fxa-client/src/state_machine/transitions.rs index 26ffe03048..17a9018825 100644 --- a/components/fxa-client/src/state_machine/transitions.rs +++ b/components/fxa-client/src/state_machine/transitions.rs @@ -203,6 +203,14 @@ pub fn transition( account.disconnect(); Ok(S::Disconnected) } + (S::AuthIssues, FxaEvent::CheckAuthorizationStatus) => { + // Recovery path after a password-change swaps in a + // fresh refresh token. Stays in AuthIssues on introspect failure. + let active = account + .check_authorization_status() + .to_state_machine_err(|| S::AuthIssues)?; + Ok(if active { S::Connected } else { S::AuthIssues }) + } // ── Invalid (state, event) pair ───────────────────────────────── (state, event) => Err(StateMachineErr::Fatal(Box::new( @@ -305,4 +313,25 @@ mod tests { let result = transition(&mut wrapper, FxaState::Disconnected, FxaEvent::Disconnect); assert_fatal_invalid_transition(result); } + + #[test] + fn auth_issues_check_authorization_status_is_a_valid_transition() { + nss_as::ensure_initialized(); + let mut account = mock_account(); + let mut wrapper = RetryingAccount::new(&mut account); + let result = transition( + &mut wrapper, + FxaState::AuthIssues, + FxaEvent::CheckAuthorizationStatus, + ); + match result { + Err(StateMachineErr::Handled { target, .. }) => { + assert_eq!(target, FxaState::AuthIssues); + } + Err(StateMachineErr::Fatal(cause)) => { + panic!("expected Handled, got Fatal({cause:?})") + } + Ok(s) => panic!("expected Handled, got Ok({s:?})"), + } + } } From 404846c711b9dedf0ab0400add932650ef070963 Mon Sep 17 00:00:00 2001 From: Sammy Khamis Date: Thu, 21 May 2026 09:42:47 -1000 Subject: [PATCH 2/3] split webchannel vs push flows for pw changes --- components/fxa-client/src/auth.rs | 8 ++ components/fxa-client/src/fxa_client.udl | 1 + components/fxa-client/src/internal/push.rs | 4 +- .../fxa-client/src/state_machine/display.rs | 1 + .../fxa-client/src/state_machine/helpers.rs | 4 + .../src/state_machine/transitions.rs | 88 +++++++++++++++---- 6 files changed, 88 insertions(+), 18 deletions(-) diff --git a/components/fxa-client/src/auth.rs b/components/fxa-client/src/auth.rs index 21d3975ef2..b408197090 100644 --- a/components/fxa-client/src/auth.rs +++ b/components/fxa-client/src/auth.rs @@ -311,6 +311,14 @@ pub enum FxaEvent { /// /// This event is valid for the `Authenticating` state. CompleteOAuthFlow { code: String, state: String }, + /// Handle an `fxaccounts:change_password` WebChannel message on the device that just changed + /// its password. `json_payload` is the `data` object of that message and contains the new + /// session token. The state machine swaps the session token for a new refresh token and + /// re-initialises the device record. + /// + /// This event is valid for the `Connected` and `AuthIssues` states. In `Authenticating` it + /// is a no-op so the in-progress OAuth flow is not disrupted. + HandleWebChannelPasswordChange { json_payload: String }, /// Cancel an OAuth flow. /// /// Use this to cancel an in-progress OAuth, returning to [FxaState::Disconnected] so the diff --git a/components/fxa-client/src/fxa_client.udl b/components/fxa-client/src/fxa_client.udl index eaadf4cd65..1542477cb8 100644 --- a/components/fxa-client/src/fxa_client.udl +++ b/components/fxa-client/src/fxa_client.udl @@ -992,6 +992,7 @@ interface FxaEvent { CompleteOAuthFlow(string code, string state); CancelOAuthFlow(); CheckAuthorizationStatus(); + HandleWebChannelPasswordChange(string json_payload); Disconnect(); CallGetProfile(); }; diff --git a/components/fxa-client/src/internal/push.rs b/components/fxa-client/src/internal/push.rs index 5eeb23fa03..6d7b8bc8ff 100644 --- a/components/fxa-client/src/internal/push.rs +++ b/components/fxa-client/src/internal/push.rs @@ -68,7 +68,9 @@ impl FirefoxAccount { } PushPayload::PasswordChanged | PushPayload::PasswordReset => { self.clear_devices_and_attached_clients_cache(); - // Send the event here and let the state machine decide next steps + // Emit the event and let the + // CheckAuthorizationStatus arm verify with the + // server before committing to AuthIssues. Ok(AccountEvent::AccountAuthStateChanged) } PushPayload::Unknown => { diff --git a/components/fxa-client/src/state_machine/display.rs b/components/fxa-client/src/state_machine/display.rs index 8e92c5770f..626d61b6d8 100644 --- a/components/fxa-client/src/state_machine/display.rs +++ b/components/fxa-client/src/state_machine/display.rs @@ -35,6 +35,7 @@ impl fmt::Display for FxaEvent { Self::CompleteOAuthFlow { .. } => "CompleteOAthFlow", Self::CancelOAuthFlow => "CancelOAthFlow", Self::CheckAuthorizationStatus => "CheckAuthorizationStatus", + Self::HandleWebChannelPasswordChange { .. } => "HandleWebChannelPwdChange", Self::Disconnect => "Disconnect", Self::CallGetProfile => "CallGetProfile", }; diff --git a/components/fxa-client/src/state_machine/helpers.rs b/components/fxa-client/src/state_machine/helpers.rs index cec99905f4..b165c51260 100644 --- a/components/fxa-client/src/state_machine/helpers.rs +++ b/components/fxa-client/src/state_machine/helpers.rs @@ -114,6 +114,10 @@ impl<'a> RetryingAccount<'a> { self.with_auth_recovery(|a| a.complete_oauth_flow(code, state)) } + pub fn handle_web_channel_password_change(&mut self, json_payload: &str) -> Result<()> { + self.with_auth_recovery(|a| a.handle_web_channel_password_change(json_payload)) + } + /// Cancels any existing OAuth flow before starting a new one. pub fn begin_oauth_flow( &mut self, diff --git a/components/fxa-client/src/state_machine/transitions.rs b/components/fxa-client/src/state_machine/transitions.rs index 17a9018825..c83bccbdfb 100644 --- a/components/fxa-client/src/state_machine/transitions.rs +++ b/components/fxa-client/src/state_machine/transitions.rs @@ -142,6 +142,9 @@ pub fn transition( initial_state, }) } + // A WebChannel password change while an OAuth flow is in progress + // is a no-op; let the flow finish. + (s @ S::Authenticating { .. }, FxaEvent::HandleWebChannelPasswordChange { .. }) => Ok(s), // ── From Connected ────────────────────────────────────────────── (S::Connected, FxaEvent::Disconnect) => { @@ -180,6 +183,17 @@ pub fn transition( initial_state: FxaRustAuthState::Connected, }) } + (S::Connected, FxaEvent::HandleWebChannelPasswordChange { json_payload }) => { + account + .handle_web_channel_password_change(&json_payload) + .to_state_machine_err(|| S::AuthIssues)?; + // Token swap succeeded; auth is valid. + let dc = account.device_config().clone(); + if let Err(e) = account.initialize_device(&dc.name, dc.device_type, &dc.capabilities) { + crate::warn!("initialize_device failed after password change; device record may be stale: {e}"); + } + Ok(S::Connected) + } // ── From AuthIssues ───────────────────────────────────────────── ( @@ -203,13 +217,17 @@ pub fn transition( account.disconnect(); Ok(S::Disconnected) } - (S::AuthIssues, FxaEvent::CheckAuthorizationStatus) => { - // Recovery path after a password-change swaps in a - // fresh refresh token. Stays in AuthIssues on introspect failure. - let active = account - .check_authorization_status() + (S::AuthIssues, FxaEvent::HandleWebChannelPasswordChange { json_payload }) => { + // A concurrent sync/401 may have pushed us here + // before the webchannel ran. The new session token still recovers us. + account + .handle_web_channel_password_change(&json_payload) .to_state_machine_err(|| S::AuthIssues)?; - Ok(if active { S::Connected } else { S::AuthIssues }) + let dc = account.device_config().clone(); + if let Err(e) = account.initialize_device(&dc.name, dc.device_type, &dc.capabilities) { + crate::warn!("initialize_device failed after password change; device record may be stale: {e}"); + } + Ok(S::Connected) } // ── Invalid (state, event) pair ───────────────────────────────── @@ -314,24 +332,60 @@ mod tests { assert_fatal_invalid_transition(result); } + fn assert_handled_lands_at( + result: std::result::Result, + expected: FxaState, + ) { + match result { + Err(StateMachineErr::Handled { target, .. }) => assert_eq!(target, expected), + Err(StateMachineErr::Fatal(cause)) => panic!("expected Handled, got Fatal({cause:?})"), + Ok(s) => panic!("expected Handled, got Ok({s:?})"), + } + } + + #[test] + fn connected_handle_web_channel_password_change_is_valid_transition() { + nss_as::ensure_initialized(); + let mut account = mock_account(); + let mut wrapper = RetryingAccount::new(&mut account); + let result = transition( + &mut wrapper, + FxaState::Connected, + FxaEvent::HandleWebChannelPasswordChange { + json_payload: "{}".to_owned(), + }, + ); + assert_handled_lands_at(result, FxaState::AuthIssues); + } + #[test] - fn auth_issues_check_authorization_status_is_a_valid_transition() { + fn auth_issues_handle_web_channel_password_change_is_valid_transition() { nss_as::ensure_initialized(); let mut account = mock_account(); let mut wrapper = RetryingAccount::new(&mut account); let result = transition( &mut wrapper, FxaState::AuthIssues, - FxaEvent::CheckAuthorizationStatus, + FxaEvent::HandleWebChannelPasswordChange { + json_payload: "{}".to_owned(), + }, ); - match result { - Err(StateMachineErr::Handled { target, .. }) => { - assert_eq!(target, FxaState::AuthIssues); - } - Err(StateMachineErr::Fatal(cause)) => { - panic!("expected Handled, got Fatal({cause:?})") - } - Ok(s) => panic!("expected Handled, got Ok({s:?})"), - } + assert_handled_lands_at(result, FxaState::AuthIssues); + } + + #[test] + fn authenticating_handle_web_channel_password_change_stays_in_authenticating() { + nss_as::ensure_initialized(); + let mut account = mock_account(); + let mut wrapper = RetryingAccount::new(&mut account); + let from = authenticating_from(FxaRustAuthState::Connected); + let result = transition( + &mut wrapper, + from.clone(), + FxaEvent::HandleWebChannelPasswordChange { + json_payload: "{}".to_owned(), + }, + ); + assert_eq!(result.unwrap(), from); } } From 32ad8fcd815c74f9d98796a80a41ee55e7dac913 Mon Sep 17 00:00:00 2001 From: Sammy Khamis Date: Fri, 22 May 2026 10:47:11 -1000 Subject: [PATCH 3/3] Renamed webchannel func, review nits --- components/fxa-client/src/auth.rs | 4 ++-- components/fxa-client/src/fxa_client.udl | 2 +- .../fxa-client/src/state_machine/display.rs | 2 +- .../src/state_machine/transitions.rs | 23 +++++++++++-------- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/components/fxa-client/src/auth.rs b/components/fxa-client/src/auth.rs index b408197090..5d039344fe 100644 --- a/components/fxa-client/src/auth.rs +++ b/components/fxa-client/src/auth.rs @@ -311,14 +311,14 @@ pub enum FxaEvent { /// /// This event is valid for the `Authenticating` state. CompleteOAuthFlow { code: String, state: String }, - /// Handle an `fxaccounts:change_password` WebChannel message on the device that just changed + /// An `fxaccounts:change_password` WebChannel message arrived on the device that just changed /// its password. `json_payload` is the `data` object of that message and contains the new /// session token. The state machine swaps the session token for a new refresh token and /// re-initialises the device record. /// /// This event is valid for the `Connected` and `AuthIssues` states. In `Authenticating` it /// is a no-op so the in-progress OAuth flow is not disrupted. - HandleWebChannelPasswordChange { json_payload: String }, + WebChannelPasswordChange { json_payload: String }, /// Cancel an OAuth flow. /// /// Use this to cancel an in-progress OAuth, returning to [FxaState::Disconnected] so the diff --git a/components/fxa-client/src/fxa_client.udl b/components/fxa-client/src/fxa_client.udl index 1542477cb8..feabe98af3 100644 --- a/components/fxa-client/src/fxa_client.udl +++ b/components/fxa-client/src/fxa_client.udl @@ -992,7 +992,7 @@ interface FxaEvent { CompleteOAuthFlow(string code, string state); CancelOAuthFlow(); CheckAuthorizationStatus(); - HandleWebChannelPasswordChange(string json_payload); + WebChannelPasswordChange(string json_payload); Disconnect(); CallGetProfile(); }; diff --git a/components/fxa-client/src/state_machine/display.rs b/components/fxa-client/src/state_machine/display.rs index 626d61b6d8..f5d06a9332 100644 --- a/components/fxa-client/src/state_machine/display.rs +++ b/components/fxa-client/src/state_machine/display.rs @@ -35,7 +35,7 @@ impl fmt::Display for FxaEvent { Self::CompleteOAuthFlow { .. } => "CompleteOAthFlow", Self::CancelOAuthFlow => "CancelOAthFlow", Self::CheckAuthorizationStatus => "CheckAuthorizationStatus", - Self::HandleWebChannelPasswordChange { .. } => "HandleWebChannelPwdChange", + Self::WebChannelPasswordChange { .. } => "WebChannelPwdChange", Self::Disconnect => "Disconnect", Self::CallGetProfile => "CallGetProfile", }; diff --git a/components/fxa-client/src/state_machine/transitions.rs b/components/fxa-client/src/state_machine/transitions.rs index c83bccbdfb..792853f04c 100644 --- a/components/fxa-client/src/state_machine/transitions.rs +++ b/components/fxa-client/src/state_machine/transitions.rs @@ -143,8 +143,11 @@ pub fn transition( }) } // A WebChannel password change while an OAuth flow is in progress - // is a no-op; let the flow finish. - (s @ S::Authenticating { .. }, FxaEvent::HandleWebChannelPasswordChange { .. }) => Ok(s), + // is a no-op; let the flow finish. Should be rare in practice. + (s @ S::Authenticating { .. }, FxaEvent::WebChannelPasswordChange { .. }) => { + crate::warn!("WebChannel password change received while Authenticating; ignoring"); + Ok(s) + } // ── From Connected ────────────────────────────────────────────── (S::Connected, FxaEvent::Disconnect) => { @@ -183,7 +186,7 @@ pub fn transition( initial_state: FxaRustAuthState::Connected, }) } - (S::Connected, FxaEvent::HandleWebChannelPasswordChange { json_payload }) => { + (S::Connected, FxaEvent::WebChannelPasswordChange { json_payload }) => { account .handle_web_channel_password_change(&json_payload) .to_state_machine_err(|| S::AuthIssues)?; @@ -217,7 +220,7 @@ pub fn transition( account.disconnect(); Ok(S::Disconnected) } - (S::AuthIssues, FxaEvent::HandleWebChannelPasswordChange { json_payload }) => { + (S::AuthIssues, FxaEvent::WebChannelPasswordChange { json_payload }) => { // A concurrent sync/401 may have pushed us here // before the webchannel ran. The new session token still recovers us. account @@ -344,14 +347,14 @@ mod tests { } #[test] - fn connected_handle_web_channel_password_change_is_valid_transition() { + fn connected_web_channel_password_change_is_valid_transition() { nss_as::ensure_initialized(); let mut account = mock_account(); let mut wrapper = RetryingAccount::new(&mut account); let result = transition( &mut wrapper, FxaState::Connected, - FxaEvent::HandleWebChannelPasswordChange { + FxaEvent::WebChannelPasswordChange { json_payload: "{}".to_owned(), }, ); @@ -359,14 +362,14 @@ mod tests { } #[test] - fn auth_issues_handle_web_channel_password_change_is_valid_transition() { + fn auth_issues_web_channel_password_change_is_valid_transition() { nss_as::ensure_initialized(); let mut account = mock_account(); let mut wrapper = RetryingAccount::new(&mut account); let result = transition( &mut wrapper, FxaState::AuthIssues, - FxaEvent::HandleWebChannelPasswordChange { + FxaEvent::WebChannelPasswordChange { json_payload: "{}".to_owned(), }, ); @@ -374,7 +377,7 @@ mod tests { } #[test] - fn authenticating_handle_web_channel_password_change_stays_in_authenticating() { + fn authenticating_web_channel_password_change_stays_in_authenticating() { nss_as::ensure_initialized(); let mut account = mock_account(); let mut wrapper = RetryingAccount::new(&mut account); @@ -382,7 +385,7 @@ mod tests { let result = transition( &mut wrapper, from.clone(), - FxaEvent::HandleWebChannelPasswordChange { + FxaEvent::WebChannelPasswordChange { json_payload: "{}".to_owned(), }, );