1139053 - Implement Firefox Accounts login state machine

Status: RESOLVED FIXED

(Firefox for iOS :: Firefox Accounts, defect)

Description

[Nick Alexander :nalexander [he/him]]

Including updating the old diagram of the account states.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/251

rnewman: over to you.  I haven't tried to rebase your branch on this -- I expect the changes to be minor -- and I don't have time to continue on this tonight.

This is not perfect:

* I'm not 100% that we ignore certain error classes correctly.  For example, a 503 should keep the account in the same state.

* The expiration delays are hard coded right now; they should be extracted to FxALoginClient and be per-client.

* The logging is there, but not yet exposed.

I have a WIP commit to allow testers to update the state manually; we may be able to leap frog this directly to "test Synced tabs".

Comment 2

[Richard Newman [:rnewman]]

Thanks, Nick.

I'll try to get to this tonight. Part of the test is likely to be rebasing my branch and seeing if my tests pass! :D

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #2)
> Thanks, Nick.
> 
> I'll try to get to this tonight. Part of the test is likely to be rebasing
> my branch and seeing if my tests pass! :D

I realized I may have made a tactical error in this PR.  I changed the main advance signature from

-> Deferred<Result<FxAState>>

to

-> Deferred<FxAState>

My reasoning is that you should *always* be able to advance a state -- you might just get the same state back.  However, this hides transient errors (such as broken connectivity, or a server 503), which consumers will almost certainly themselves witness shortly after the login machine does.  This could cost us a connection round trip to error out higher in the stack.  I wonder if it might be better to have the signature be

-> Deferred<NSError?, FxAState>

or even have the caller possibly provide error storage, like

-> ErrorPointer -> Deferred<FxAState>

This will become clear when I implement the abstraction I have planned for the Token Server token caching layer; but keep it in my mind if you have a strong opinion.

Comment 4

[Richard Newman [:rnewman]]

I guess the question is: does the caller care about the error, and does the error transition result in a useful state?

That leaves us with three approaches:

1. The caller doesn't care about the error, and only cares about a successful advancement. Return `Deferred<FxAState?>`.

2. The caller always expects a state (even if it's the same one), and also wants to know if an error occurred. Return `Deferred<FxAState, NSError?>`.

3. The caller doesn't expect a state if an error occurs (e.g., because it's the same one), 'real' failures are modeled as a particular state (Doghouse), and the caller cares about errors. Return `Deferred<Result<FxAState>>`.


I would be happy with any of these, assuming the choice matches the need. E.g., if you know that you'll either advance to an error state or experience a transient error, #1 works fine.


I expect this to be mostly encapsulated from my POV -- that is, for a sync to run in any capacity we need:

  * From the token server:
    * A server assignment
    * A token (expired or not)
    * A way to fetch a new token and new server assignment if necessary (which errors)

  * From the account itself:
    * The user's keys
    * Client state to use when fetching a token.

These things can be bundled up in an object that can expose its own API.

We don't really care about the account state itself, only about the capabilities that we derive from an account in good state. Even the syncing rules -- "can I sync? should I try again later?" -- should be abstracted somewhat.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8581208 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/251

Reviewed on GH.

Comment 6

[Nick Alexander :nalexander [he/him]]

https://github.com/mozilla/firefox-ios/commit/26ea91172f18c5237723402655e0ab0f5e97f50b

Comment 7

[Nick Alexander :nalexander [he/him]]

I pushed this code basically as it was, deferring the "return error" API decision to the next ticket, which I am about to file.