New fxa client [firefox-android: bendk/new-fxa-client] #5851
Conversation
bendk
changed the title
bendk
force-pushed
the
new-fxa-client
branch
2 times, most recently
from
0ef0a96 to
c4df096
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5851 +/- ##
==========================================
- Coverage 36.67% 36.55% -0.12%
==========================================
Files 348 348
Lines 33607 33710 +103
==========================================
Hits 12324 12324
- Misses 21283 21386 +103
☔ View full report in Codecov by Sentry. |
| * This is public so applications can use the same context to run their jobs in. For example | ||
| * the android-components FxaManager uses it for its startup tasks. | ||
| */ | ||
| public val coroutineContext = coroutineContext + SupervisorJob() |
There was a problem hiding this comment.
Someone with Kotlin experience please double check that I'm doing the correct thing here.
There was a problem hiding this comment.
If the intention is to allow children tasks to fail independently while still being able cancelling then all at once, you are doing the correct thing!
I would suggest communicating your intention more directly by keeping a reference to a SupervisorJob and cancelling the job, rather than the courutineContext itself, specially given that coroutineContext is public. It's quite a common practice, and FirefoxAccount is doing exactly that :)
| } else { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
I like this class because it isolates the retry logic, but I don't really know what the correct logic is.
The old android-components code performed 3 network retries for every operation, which seemed quite a lot to me. Once every 30 seconds seems reasonable, but I'm open to any suggestions.
Auth checking had a complicated system that boiled down to don't do it too much and definitely don't get stuck in an infinite loop. Infinite loops shouldn't be an issue anymore since we don't queue a new action to handle the auth check. More than one auth check per minute felt like too much to me, but I'm happy to follow someone else's lead here too.
There was a problem hiding this comment.
Is there any evidence the old strategy was harmful? This is a significant change - how can we be sure this change isn't going to be harmful? On a very flakey network I can see this being worse - eg, (1) request fails, (2) retried request passes, (3) next request fails - the old strategy would retry (3) but this would not. On a flakey network trying to log in, isn't there a risk that this strategy will never complete all the requests needed to complete a login whereas the old would?
(FWIW, I'm not trying to say the old strategy was ideal, but it's not at all clear to me that this will not be worse in edge-cases, even if better in some?)
There was a problem hiding this comment.
I think that makes sense, I'm going to change things to use the old logic.
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaClient.kt
Show resolved
Hide resolved
| */ | ||
| interface FxaPersistCallback { | ||
| fun persist(data: String) | ||
| } |
There was a problem hiding this comment.
Do we have a naming system for our components? I tended to prefix everything with Fxa, since you don't ever import modules in Kotlin. I'm open to any suggestions though.
|
|
||
| private suspend fun handleBeginOAuthFlow(currentState: FxaAuthState.Disconnected, action: FxaAction.BeginOAuthFlow) { | ||
| handleBeginEitherOAuthFlow(currentState, action.result) { | ||
| inner.beginOauthFlow(action.scopes.toList(), action.entrypoint, MetricsParams(mapOf())) |
There was a problem hiding this comment.
Am I handling MetricsParams correctly? I don't really understand how it's supposed to work.
There was a problem hiding this comment.
It gets passed in from the caller - it includes query parameters that the client wants to append to the URL, eg: all the utm_* params in https://mozilla.github.io/ecosystem-platform/relying-parties/reference/query-parameters#generic-parameters
There was a problem hiding this comment.
Ahh okay. I don't think Android uses this, but I added this as an optional parameter for the begin oauth actions.
| private val tryPersistState: () -> Unit, | ||
| initialState: FxaAuthState = FxaAuthState.fromRust(inner.getAuthState()), | ||
| ) { | ||
| private val channel = Channel<FxaAction>(Channel.UNLIMITED) |
There was a problem hiding this comment.
Thinking out loud, I wonder if actually adding a bound might help us diagnose other problems, such as the processor stalling/failing? OTOH though, we probably the median number of events per session to be zero, right, so that might not be effective in practice, unless it was a very low number, which itself has risks.
There was a problem hiding this comment.
I think a relatively high bound could be good here and would help diagnose the processor stalling. I don't think that should happen, but if it did I would expect that the actions would start piling up and even a bound of 100 or so would result in crash reports.
There was a problem hiding this comment.
I initially thought that too, but when writing the comment I also thought that we'd probably never expect to end up with 100 "actions" in total for a single session, right? ie, would a bound of 100 actually catch this stalling in practice?
| for (action in channel) { | ||
| Log.d(LOG_TAG, "Processing action: $action") | ||
| processAction(action) | ||
| Log.d(LOG_TAG, "Action processed: $action") |
There was a problem hiding this comment.
should there be an exception handler here? I note many/most of the "handle" methods do have them, but I wonder if it makes sense to move them up to here so that it's impossible for a future handler to forget to catch exceptions or fail to catch them all?
There was a problem hiding this comment.
There's a generic handler one level up in runActionProcessorManager that restarts the entire loop. I figured that maybe could catch some extra exceptions that a handler here wouldn't.
There was a problem hiding this comment.
fair enough - maybe a very short comment to that end?
| } else { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there any evidence the old strategy was harmful? This is a significant change - how can we be sure this change isn't going to be harmful? On a very flakey network I can see this being worse - eg, (1) request fails, (2) retried request passes, (3) next request fails - the old strategy would retry (3) but this would not. On a flakey network trying to log in, isn't there a risk that this strategy will never complete all the requests needed to complete a login whereas the old would?
(FWIW, I'm not trying to say the old strategy was ideal, but it's not at all clear to me that this will not be worse in edge-cases, even if better in some?)
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaActionProcessor.kt
Outdated
Show resolved
Hide resolved
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaClient.kt
Show resolved
Hide resolved
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaClient.kt
Outdated
Show resolved
Hide resolved
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaClient.kt
Outdated
Show resolved
Hide resolved
| ) : FxaAction() | ||
|
|
||
| /** | ||
| * Disconnect from the FxA server and destroy our device record. |
There was a problem hiding this comment.
we chatted a little about this on slack, but do we really need to destroy our device record on auth error? Indeed, I'm a little surprised we'd do anything called "disconnect" on auth error, because to my mind the act of "disconnecting" your account is significantly different than your account needing to be reauth'd.
(It's very possible I'm missing the point though - what does Disconnected {fromAuthIssues = false} mean? Is that the same as "never attempted to connect"?)
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/StateTypes.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| data class Disconnected( | ||
| val fromAuthIssues: Boolean = false, | ||
| val connecting: Boolean = false, |
There was a problem hiding this comment.
I guess I was expecting a few more states here:
Disconnected -> not trying to connect, may never have connected.
Authenticating -> actually trying
Authenticated -> completed trying and it worked.
AuthenticationFailed -> completed trying and it failed.
which would kill both booleans from here and make things a little clearer?
There was a problem hiding this comment.
I was attracted to the idea that the FxaAuthState subclass defined which actions were possible and the fields were details about the state. So if your state is Disconnected then you can begin an oauth flow, if it's Connected you can't.
You're point above about the difference between disconnected and having auth issues has convinced me that I shouldn't have done this. I'll change this to a flat enum.
There was a problem hiding this comment.
OTOH, if you want to keep the current mechanism, maybe "not connected" would be a better name - it's a subtle difference, but it avoids the baggage the term "disconnect" carries. OTOH, maybe that's just my baggage :)(although I think it comes from desktop really)
(I'm still not sure though - needing to examine 2 bools in one variant to determine the difference between "user has never even tried to connect an account" and "user needs to reenter their password" doesn't really seem ideal.
There was a problem hiding this comment.
I had a first pass but I will need another - thanks a lot Ben!
Like @mhammond said this is fairly large and introduces quite a few changes more than just moving A-C into here (may not be a bad thing, just means it's harder to review properly)
I had a few thoughts that I wrote down and my high-level thought is that I'm wondering if we can take this opportunity to write out a short artifact (a doc? diagram?) that's accessible to read about all the state transitions this PR moves over from A-C and introduces here. That will help me reason about it easier than hopping around Kotlin files (and I'm not too familiar with Kotlin so it introduces a possibility for us to miss things)
components/fxa-client/src/error.rs
Outdated
| #[error("A required scoped key was missing")] | ||
| ScopedKeyMissing, |
There was a problem hiding this comment.
How do we expect a consumer to recover from this? They'd probably need to re-auth, which we might be able to abstract away the error by re-using the Authentication variant..
Additionally, Just from the description of the error, the consumer might conflate "scoped key should be cached locally but is missing" and "The server did not respond with the scoped key", what do you think of making the error message a bit more explicit where the key was missing?
There was a problem hiding this comment.
Android-components handles each of these separately. For Authentication, it calls queueAction(CheckAuthorization). For ScopedKeyMissing it goes straight to logoutFromAuthIssues. I think that eventually the clients shouldn't need to deal with this distinction and we should be doing all the auth-checking for them, but I'd rather handle that as a follow up.
I'll fix the name and I also realized that I was making that same mistake in the code. If we have a cached auth info with the key missing, I think we should retry fetching it before giving up and returning the error.
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaActionProcessor.kt
Outdated
Show resolved
Hide resolved
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaActionProcessor.kt
Outdated
Show resolved
Hide resolved
|
|
||
| private suspend fun handleBeginOAuthFlow(currentState: FxaAuthState.Disconnected, action: FxaAction.BeginOAuthFlow) { | ||
| handleBeginEitherOAuthFlow(currentState, action.result) { | ||
| inner.beginOauthFlow(action.scopes.toList(), action.entrypoint, MetricsParams(mapOf())) |
There was a problem hiding this comment.
It gets passed in from the caller - it includes query parameters that the client wants to append to the URL, eg: all the utm_* params in https://mozilla.github.io/ecosystem-platform/relying-parties/reference/query-parameters#generic-parameters
| val url = withRetry { operation() } | ||
| persistState() | ||
| changeState(currentState.copy(connecting = true), FxaAuthStateTransition.OAUTH_STARTED) | ||
| sendEvent(FxaEvent.BeginOAuthFlow(url)) |
There was a problem hiding this comment.
Just double-checking that the consumer truly doesn't care which flow it was (i.e there is not a distinction here between pairing and regular flow)
I can see it from the correctness that this would be true, all the consumer has to do is navigate to the URL, and handle whichever webchannel messages are triggered but just verifying existing behaviour doesn't make further distinctions (in telemetry or otherwise)
There was a problem hiding this comment.
Good point, I'll add an extra param here. If a consumer doesn't care than they can just ignore it.
There was a problem hiding this comment.
Actually, I'm not so sure yet. I still think this is a good point, but I think it applies more to the FxaEvent.AuthEvent (formally StateChanged). I was considering adding field for the OAuth flow kind there, but it's not so easy since there can be multiple flows.
Firefox android doesn't need this yet, so I think we should wait on implementing it, maybe alongside a requirement that only one OAuth flow is in progress at any time.
components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaActionProcessor.kt
Outdated
Show resolved
Hide resolved
|
@mhammond Thanks for the review, I rebased in changes based on most of your suggestions. The changes to |
|
Pushed another commit to address @tarikeshaq's comments. I think we're passed the point of rebasing nicely into the individual commits, but I'll squash things up at the end of the review. I've been fairly aggressively marking comments resolved, feel fere to open any back up that I didn't address properly. |
bendk
force-pushed
the
new-fxa-client
branch
3 times, most recently
from
3bbc3d1 to
11df727
Compare
There was a problem hiding this comment.
I definitely welcome and appreciate the new API for interracting with FirefoxAccount :) But the PR is a bit on the bigger side.
- Changes affect both inner logic of Rust components and kotlin wrappers.
- The kotlin change aims to move some logic from A-C while simultaneously refactoring it (portion of the logic from
FxaAccountManageris being moved intoFxaClient). - There are improvements to address older problems.
- Some commit comments are more detailed than a bigger than average PR we have on fenix.
It looks like just a bit too much for a single thing to land – or even review thoroughly. Given there are 2600 lines of code added, my guess would be we might reach close to 500 hundred comments before landing it. Would you consider landing it in bits @bendk? It would definitely give reviewers more confidence.
Regarding the kotlin side change, I would agree with @tarikeshaq that some form of document (an RFC) describing the approach and the change would be nice. It's easier to grasp the intention behind the change rather than trying to make sense of all the changes. But overall, it does look good.
Also, the change to kotlin wrapper aims to move logic from A-C while simultaneously refactoring it. It's harder to review and to land – it have to land the A-S change first, and then start using it. Did you consider introducing all new wrappers in A-C layer, then moving them into A-S? It could make testing and reviewing it easier.
Just wanted to praise the work you have done. The scope is huge, and the change is very very much welcomed. Just trying to find a good way to land it. Given it's related to account domain, I would exercise extra cation.
| * This is public so applications can use the same context to run their jobs in. For example | ||
| * the android-components FxaManager uses it for its startup tasks. | ||
| */ | ||
| public val coroutineContext = coroutineContext + SupervisorJob() |
There was a problem hiding this comment.
If the intention is to allow children tasks to fail independently while still being able cancelling then all at once, you are doing the correct thing!
I would suggest communicating your intention more directly by keeping a reference to a SupervisorJob and cancelling the job, rather than the courutineContext itself, specially given that coroutineContext is public. It's quite a common practice, and FirefoxAccount is doing exactly that :)
| * Disconnect being executed at the same time from different threads. | ||
| * - Application events are also sent serially. If one action causes an [AUTH_CHECK_STARTED] | ||
| * state transition change, then the next causes [FxaAuthState.Disconnected], the | ||
| * [FxaEventHandler.onStateChange] callback for the second change won't be called until |
There was a problem hiding this comment.
I get the overall message, but still this point isn't exactly clear to me, especially about the return. Were you talking about [FxaEventHandler.onFxaEvent] suspend function?
There was a problem hiding this comment.
If I understand it correctly, we want to wait until the FxaEventHandler finishes processing the event. It puzzles me a bit, since usually the state machine would just report an event – and continue working. Is this to mimic this part of the FxaAccountManager?
There was a problem hiding this comment.
- I think you're understanding everything correctly. Agreed that talking about a change returning is weird, I'll update the comment to reference the method instead.
- It is indeed intended to mimic that functionality from the current
FxaAccountManager. The issue I want to avoid is the consumer code handling events out-of-order. For example:- We emit
onStateChangewhen the auth check starts and continue on - The auth check succeeds and we emit
onStateChangefor the successful check - We may now have two
onStateChangehandler functions running and there's no guarantee which finishes first. It's possible the user end up seeing an "auth check in progress" message at the end of everything.
- We emit
| class FxaClient private constructor( | ||
| private val inner: FirefoxAccount, | ||
| private val persistCallback: FxaPersistCallback, | ||
| // Note that this creates a reference cycle between FxaAccountManager, FxaClient, and FxaHandler |
There was a problem hiding this comment.
This comment is referencing FxaAccountManager which is not part of the project. It would create a rather fragile dependency and would imply knowledge of the client of the fxaclient module. I wonder if it should be out of scope on this level? Looks like creating a rather fragile dependency (for ex, a name change will be invisible to the FxaClient)
| /** | ||
| * Firefox account client | ||
| */ | ||
| class FxaClient private constructor( |
There was a problem hiding this comment.
What's the intention behind the private constructor? I think there is a way to combine this and the second constructor on line 50.
There was a problem hiding this comment.
There's 2 public constructors: one inputs the Config and the other inputs a persisted JSON string instead. You can't define one in terms of the other. It's a bit weird, but not something that I thought needed fixing.
| /** | ||
| * Restores a perisisted FxaClient | ||
| * | ||
| * @param json JSON data sent to FxaEventHandler.persistData |
There was a problem hiding this comment.
looks like the description doesn't match the FxaEventHandler implementation any more
| } | ||
|
|
||
| // This handles queueAction for us | ||
| private val actionProcessor = FxaActionProcessor(inner, eventHandler, { persistState() }) |
There was a problem hiding this comment.
Why wouldn't we put class variables into the top of the file?
| @@ -22,6 +22,7 @@ buildscript { | |||
| ktlint_version = '0.50.0' | |||
| gradle_download_task_version = '5.2.1' | |||
| junit_version = '4.13.2' | |||
| mockk_version = '1.13.8' | |||
There was a problem hiding this comment.
Drive-by for now 🚗 : The code moving here doesn't require mockk, and I would not recommend using it widely in the codebase given the troubles that we've had with it in fenix. We are now stuck with it and do not have an opportunity to easily remove it.
There was a problem hiding this comment.
Is mockito recommended over mockk or should I be avoiding mocking libraries altogether?
There was a problem hiding this comment.
mockito is preferable and we've used it in AC without issues for many years now. There were useful methods in the mockito-kotlin library that were helpful as well but to avoid adding more dependencies, we imported only those specific convenience functions.
If you can avoid a mocking library that would be ideal, but that is more of a philosophy than a good reason. 🙂
Added the `get_auth_state()` method which gives a high-level view of the auth state. Added a flag to track why the client was disconnected. Did the user manually disconnect or did we realize we had a bad refresh token and needed to start another OAuth flow? As noted, the Rust code doesn't track all the data that we want yet, and will need some help from the Kotlin wrapper.
This will replace a bunch of android-components code to workaround mozilla-mobile/android-components#8527. This also means we will be responsible for monitoring the error rate. While we're at it, let's also monitor the "Other" errors.
- Defined the `LocalDevice` struct, which contains the info about the client's device. - Many device methods now return a LocalDevice value constructed from the server response. - Use the last `LocalDevice` returned from the server to cache the capabilities. This improves on the previous system, since it differentiates between no known info and the empty list. - One drawback is that this will always be unset when users migrate, so they may hit the server an extra time when `ensure_capabilities` is first called.
|
@mavduevskiy thanks for the review! Short-term, I'm going to try to update this PR to incorporate your comments. Long-term, I like the idea of breaking things up more and definitely don't like doing this all in one pass. Making the changes in A-C first, then moving everything over to A-S sounds great to me. I think I can break up the changes to the state machine into smaller commits. However, before I do that, I'd like to get agreement that something like this code would be a good end-state. My guess is that those small refactor commits won't make sense without an end-goal in mind. Maybe this is where your suggestion of an RFC would come into play. I could put together a high-level description of the changes and we could discuss there. One of the reasons I bundled everything together is to reduce the number of reviews. But I can see why reviewing a number of smaller commits would actually be easier. |
Renamed the class from `PersistedFirefoxAccount` to `FxaClient` because class is now adding more functionality than just persistance. The main reason I chose `FxaClient` was that it doesn't conflict with any of the other class names. Added a state machine, similar to the android-components one but more lightweight. Applications send FxaActions the client sends them back FxaEvents. Everything happens in a single async task that reads actions from a channel and processing things serially. Well, that was the goal at least, there's a few places where we need break this model to make things work with the existing firefox-android code. See StateTypes.kt for details. Most getter functions are now async, using `withContext` plus a application-supplied `CoroutineContext`. Some getter functions are advertised as blocking because they don't make network requests. I don't think this is completely accurate (mozilla#5819), but I made this sync functions since that's what the application expects. Implemented a simple form of network retry / automatic auth checking. I'm not sure if this is the correct logic, but it should be simple to modify. Added functions to manually test this, the plan is to hook them up to the secret debug menu an Android. Updated sync_local_device_info to accept a None value for display name. This updates the device type/commands, then returns a LocalDevice record with the display name filled in from the server response. This is what we firefox-android needs for its startup. Switched to using FxaConfig and FxaServer directly. Don't persist the state on startup with a Config. This doesn't work on firefox-android since the state persistence callback is not setup on the layer above us. It also doesn't make much sense to save the state before any action has been taken -- you can always reproduce that state by starting with a new config.
Rust:
- Made AuthIssues it's own state and not just a "sub-state" of Disconnected.
- Split up the [StateV2.start_over] into [StateManager.disconnect] and
[StateManager.logout_from_auth_issues].
Kotlin:
- Flattened out FxaAuthState so that each possible state is just an
object with no fields
- Renamed `AuthStateChanged` to `AuthEvent`. I think this is a better
name because the auth state doesn't always change.
- Added support for the AUTH_ISSUES state and logoutFromAuthIssues
- Updated the unit tests to test all possible actions from all
possible states.
- Handled some corner cases better in FxaActionProcessor, for example
if one OAuth flow is in-progress and a second BeginOAuthFlow action
fails, we now stay in the AUTHENTICATING state rather than go back
to DISCONNECTED.
- Define constants for magic values - Go back to the old network retry logic - Removed the require_scoped_key param. Scoped keys are always required for the `oldsync` scope and never for other scopes. - Added back the `StateV2.device_capabilities` field. This tracks something different than `server_device_info`. Instead of tracking what the server thinks about our device, this tracks the capabilities that we know we have. It's a subtle difference, but `device_capabilities` is what we want to use for `reregister_capabilities`. - Added optional metrics parameter for BeginOAuthFlow and BeginPairingFlow
|
I'm really liking the idea of using an RFC to agree on the big picture, and splitting the commits into smaller pieces. I wrote up an initial draft here: https://docs.google.com/document/d/16UUdfx1yvK4W-jBvczL5tPUaJ-R-TFIRiLckAS7BJmM/edit Please take a look when you can, any feedback is appreciated. No rush though, we should wait for Tarik to get back next week before taking any action. |
|
I think everyone agrees with the slower, step-by-step approach. Let's close this one. I think the first step is going to be merging these PRs: |
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.