Bug 2059169 - Allow CheckAuthorizationStatus from more states

[bendk]

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, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[skhamis]

Nice!

[mhammond]

What's the motivation here? Do we ever expect AuthIssues to magically resolve itself? And shouldn't someone trying to check the auth status when, eg, Disconnected make more noise as it seems to strongly imply a confused client? ie, I don't think this is "wrong", but not sure it's "helpful" or something :)

[mhammond]

Do we ever expect AuthIssues to magically resolve itself?

ie, if it ever did resolve itself, it implies there was a bug which caused us to get into AuthIssues in the first place?

[mhammond]

oops, I see the info in the bug. It's a little sad, but I think this is fine.

[bendk]

What's the motivation here? Do we ever expect AuthIssues to magically resolve itself?

Not really. Should it be a no-op as well?

oops, I see the info in the bug. It's a little sad, but I think this is fine.

Agreed. It's kind of weird but I don't think the current Android behavior is too unreasonable.

[mhammond]

Not really. Should it be a no-op as well?

This seems fine I guess. I'd like to better understand the Android weirdness (but not enough to actually dig into it :) but given it exists, this isn't going to hurt.

[mhammond]

just noticed we do record some telemetry here - "Account Manager recovered a broken FxA auth state, without direct user involvement.". It would be interesting to see that - I don't think it would have great UX

[bendk]

just noticed we do record some telemetry here - "Account Manager recovered a broken FxA auth state, without direct user involvement.". It would be interesting to see that - I don't think it would have great UX

Oh. Given that the defined that telemetry, I'm going to keep the check from the AuthIssues state. One scenario I could imagine is they see a 401 error for normal reasons, maybe they tried to use an expired auth token or something, then the first auth check fails with a network error so they move to AuthIssues. At some point later they do another auth check and it works.

I'd like to better understand the Android weirdness

Other than the above weirdness, the other part is here. Whenever they see an auth error, they queue an operation that ends with sending us CheckAuthorizationStatus. Sometimes they see multiple auth errors at once, queue a bunch of those events, then the ones after the first can lead to the invalid state transition error.

[mhammond]

One scenario I could imagine is they see a 401 error for normal reasons, maybe they tried to use an expired auth token or something, then the first auth check fails with a network error so they move to AuthIssues. At some point later they do another auth check and it works.

Yeah, exactly - I'd call it a bug that a network error caused this in the first place, so if we can get evidence of that we should track it down. I'm kinda thinking that all these extra users who suddenly show up in all our stats might be something like this - we end up kinda permanently leaving them in a needs-reauth state. You patch seems fine, but isn't going to be a panacea - it still relies on the app trying the transition, which I don't think it expects to do in normal cases - once we are in an authissues state I expect we just stay there and don't check the auth status again in the usual cases?

[bendk]

I'd call it a bug that a network error caused this in the first place, so if we can get evidence of that we should track it down.

The hypothetical scenario is that there's a real, but recoverable, auth error. However, when we try to recover we get the network error. In that case, I feel like AuthIssues is the correct state, but there is a case to be made that they should stay inConnected. Or maybe AuthIssues should get a flag tracking this somehow. Consumers could check that flag and re-check the auth state periodically.

[mhammond]

The hypothetical scenario is that there's a real, but recoverable, auth error.

I don't think that exists.

However, when we try to recover we get the network error. In that case, I feel like AuthIssues is the correct state

The thing is, we aren't trying to recover - we think there's a possibility we are in a bad auth state. Failing to check that should not put us in one.

Conceptually, this is no different from startup. If we startup without a network connection you couldn't argue we should go to AuthIssues, even though there's quite a good chance we are disconnected as it's been some time since we last checked.

[mhammond]

("recoverable" implies some action we take, but there's never any action we do take here, right? ie, I'm saying the only thing we do that qualifies as "recovery" is to prompt for a password)

[bendk]

The hypothetical scenario is that there's a real, but recoverable, auth error.

I don't think that exists.

Hmm. At one point I thought it was common, but I can't think of a realistic scenario anymore. The best one I can come up with is you messed with your clock and have an expired auth token, but our code can't detect it.

However, when we try to recover we get the network error. In that case, I feel like AuthIssues is the correct state

The thing is, we aren't trying to recover - we think there's a possibility we are in a bad auth state. Failing to check that should not put us in one.

Conceptually, this is no different from startup. If we startup without a network connection you couldn't argue we should go to AuthIssues, even though there's quite a good chance we are disconnected as it's been some time since we last checked.

That's a good point. Maybe we should never move to AuthIssues unless we see a concrete failure from checking the auth status. If we see a network error, then it seems okay to move back to Connected. Like you say, the worst thing that can happen is a user with network issues is in the Connected state when they should be in AuthIssues , but that can happen in many ways.

[mhammond]

The best one I can come up with is you messed with your clock and have an expired auth token, but our code can't detect it.

The session and refresh tokens never expire.

That's a good point. Maybe we should never move to AuthIssues unless we see a concrete failure from checking the auth status.

Yep, that's exactly what I think should happen.

If we see a network error, then it seems okay to move back to Connected

subtle but important point here - there's no move "back to Connected" - it's that we never leave connected just due to a network error

[mhammond]

And to further clarify:

concrete failure from checking the auth status.

This reads better as "concrete affirmation that the auth status is bad" or something - the term "failure" is somewhat misleading here - the request to check the auth status must work with a result that tells us the status is bad - there's no "failure" here.

[bendk]

That logic make sense to me. I made https://bugzilla.mozilla.org/show_bug.cgi?id=2060539 for updating the state machine.

I also made https://bugzilla.mozilla.org/show_bug.cgi?id=2060541, for trying to recover every so often since we have some current users that are in AuthIssues when they don't need to be.