Bug 2060541 - Recheck auth status on startup, weekly

[bytesized]

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]

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]

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]

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.

[bytesized]

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.

[bendk]

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.

[bytesized]

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.

[bytesized]

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.

The pull request has been modified, dismissing previous reviews.