Bug 2060541 - Recheck auth status on startup, weekly by bytesized · Pull Request #7564 · mozilla/application-services

bytesized · 2026-08-26T23:04:13Z

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes
Quality: This PR builds and tests run cleanly
- cargo test succeeds locally.
Tests: This PR includes thorough tests or an explanation of why it does not
- No automated testing, but this adds the cargo fxa force-auth-issues command to the example CLI client in order to be able to test this manually.
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- This change is small enough that it doesn't seem like it warrants a changelog entry.
Dependencies: This PR follows our dependency management guidelines
- No new dependencies added.

bytesized · 2026-08-31T16:44:59Z

I have been thinking a fair amount about the part of this patch that I'm the least sure about: the part in components/fxa-client/src/state_machine/mod.rs where we avoid calling on_auth_issues() on the Uninitialized -> AuthIssues transition. When I wrote it, I felt reasonably sure that this change wouldn't have any negative effects because that state transition should only happen after we already transitioned to AuthIssues and because the effects of on_auth_issues should persist through the next initialization. But having thought about it a bit more, I believe that there are ways of having the Uninitialized -> AuthIssues transition without having previously been in the AuthIssues state. So I think I may have to undo that change. But I'm not sure of the correct way to address the issue that it always runs on_auth_issues when we start up in the AuthIssues state, resetting the last_auth_recheck_time timer. I'm going to have to think about this.

bytesized · 2026-09-02T22:05:39Z

I ended up not putting the logic to reset the auth timer in on_auth_issues() because of the issue I discussed above. To summarize, we always call on_auth_issues() on the Uninitialized to AuthIssues transition which causes the recheck to never happen since we never see an expired timer. But this means that I've basically littered the transition() function with account.reset_auth_recheck_timer() calls. Which doesn't feel great since it makes maintenance of this function kinda nasty. I'm not sure what a better option is but I'm open to ideas.

bendk

This is looking very good. Thanks for updating the example client.

I had a couple suggestions, but the only blocker is figuring out how we should store the refresh token when we're in the auth-issues state.

bendk · 2026-09-03T15:32:22Z

            last_seen_profile: None,
            access_token_cache: HashMap::new(),
            logged_out_from_auth_issues: false,
+            last_auth_recheck_time: None,


I see your point about resetting this timer and the Unititialized -> AuthIssues transition. My one idea here is to rename this to last_auth_check_time and set it in whenever check_authorization_status() is called (or maybe just whenever it fails?). I think that might give you the timer you need without the having to call reset_auth_recheck_timer in so many places. However, I'm not sure and I'm okay with the repetition if that's what we need to do. However you want to handle this is fine with me.

My concern with this solution is that there are other ways that we can get into the AuthIssues state. Here, for example. With the proposed design, that line (and several others) would put us into the AuthIssues state and then we would re-check on the very next initialization. I assume that this isn't really the behavior that we want.

I suspect that the those cases are buggy and we should fix those. For example, I think we should just get rid of the CallGetProfile event. Can you make a list of the other cases where you think there could be an issue? I'd love to go through them.

In the meantime, maybe we should just leave this code as-is. I don't want block this PR on figuring all that out.

Aside from the one listed above, there is:

This one where we initialize into the Connected state, but account.finish_initialize fails.

This one where we get the WebChannelPasswordChange event from the Connected state and handle_web_channel_password_change fails.

This one where we are in the AuthIssues state, get the BeginOAuthFlow event, and begin_oauth_flow fails.

This one where we are in the AuthIssues state, get the WebChannelPasswordChange event, and handle_web_channel_password_change fails.

Another possibility here is that we could just accept the behavior of doing one check on the very next initialization after the AuthIssues transition. That would simplify the code a bit since we would only reset the timer in one place. And if we did somehow end up in a situation where someone was frequently getting into that state, it would fix itself quickly instead of fixing itself once a week.

I think an extra check wouldn't be the worst if we had to do it, but hopefully we can avoid it. I made https://bugzilla.mozilla.org/show_bug.cgi?id=2070265 and added some comments there.

The pull request has been modified, dismissing previous reviews.

mhammond · 2026-09-08T05:47:28Z

+                // This probably indicates that the user is not authorized but there are various
+                // corner cases where we might have gotten something wrong. For example, a bug in an
+                // older browser version that we've since fixed or an FxA server bug.
+                // Because of this, we will recheck the authorization status from time to time.


This patch looks nice to me!

But I think the comment about an fxa server bug is wishful thinking (eg, desktop has never done this and there's never been evidence it should), and even "old browser versions" is slightly wishful thinking - we fixed one such case just weeks ago :) I'm really more worried about remaining bugs!

ie, tbh I don't see this patch as an "improvement", but more as a "mitigation" - I fully expect this to never work once we've nailed the bugs. So I think it makes sense to have another issue be opened for recording telemetry here so we can witness it hit zero times it actually recovered and then remove this code? Happy to leave the formal review to Ben though.

Good point. I think a lot of the bugs had to do with the way the old fxa code was structured, so there's good reason to believe that we'll actually fix the bugs for good at some point. That said, I don't see much downside to keeping it around either. I'm happy to go either way on this one.

yeah, that's a future consideration, but I do like the idea of telemetry in the shorter term?

bendk

The changes look great. The last part is updating the methods I noted so that they continue to throw a NoRefreshToken error if logged_out_from_auth_issues=true.

bendk · 2026-09-08T16:37:58Z

    pub(crate) server_local_device_info: Option<LocalDevice>,
    #[serde(default)]
    pub(crate) logged_out_from_auth_issues: bool,
+    pub(crate) last_auth_recheck_time: Option<u64>,


Nano-nit: should this just be called last_auth_check_time? I think it's set on the initial auth check failure, not on the first recheck failure.

bendk · 2026-09-08T16:48:34Z

+        if self.persisted_state.logged_out_from_auth_issues {
            FxaRustAuthState::AuthIssues
+        } else if self.persisted_state.refresh_token.is_some() {
+            FxaRustAuthState::Connected


This change looks good, the other places that may need to be changed are functions that we expose to the consumer that read the refresh token. If the application calls those, we'll now use the old refresh token even though it may be bad, whereas before they would fail with a NoRefreshToken error. I noticed these methods:

has_scope()

update_device()

fetch_and_parse_commands()

invoke_command()

get_command_for_index()

I'm not sure if we should change those methods or not. However, I think it's better to keep the behavior similar to how it was before. I guess NoRefreshToken is not the right error anymore, but it seems okay to continue to use it since it will end up as an Authentication error by the time it gets to the application which seems right. We can fix this in a follow-up.

Like I said, I'm not totally sure about this, what do you think @bytesized @mhammond ?

nice catch - I think NoRefreshToken seems fine even if now it means NoValidRefreshToken :) I think the simplest would be to have pub fn refresh_token(&self) -> Option<&RefreshToken> check the auth status and return None if in that state?

(hrm, which might mean we need a new method for check_authorization_status() to use, but given a suitable long name that seems ok)

bytesized force-pushed the 5386 branch from 01aca7c to 8e10c22 Compare September 2, 2026 21:44

bendk self-requested a review September 2, 2026 22:08

bendk previously requested changes Sep 3, 2026

View reviewed changes

bytesized force-pushed the 5386 branch from 8e10c22 to 0430a40 Compare September 4, 2026 19:32

Bug 2060541 - Recheck auth status on startup, weekly

7833171

bytesized force-pushed the 5386 branch from 0430a40 to 7833171 Compare September 4, 2026 19:37

mhammond reviewed Sep 8, 2026

View reviewed changes

bendk requested changes Sep 8, 2026

View reviewed changes

Uh oh!

Conversation

bytesized commented Aug 26, 2026

Pull Request checklist

Uh oh!

bytesized commented Aug 31, 2026

Uh oh!

bytesized commented Sep 2, 2026

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants