Bug 1856729 - Switch to new app-services FxaClient

[bendk]

Use the new functionality from app-services to replace code in service-firefox-accounts

General:

• Updated code to use FxaConfig class rather than Config. Config wrapped FxaConfig, but provided little value.
• Updated handleFxaExceptions and removed all other Utils functions
• Use FxaClient's CoroutineContext to run all fxa-related jobs.
• Many FxaClient methods no longer directly return a result, but instead input an optional CompletableDeferred<T> that they complete when the operation is finish. See the app-services PR for details.

FirefoxAccount:

• Don't wrap FxaClient calls in withContext, this is handled in app-services now.
• Updated FirefoxAccount docs to describe its relationship to OAuthAccount.
• Make AccountStorage return a FirefoxAccount directly instead of an OAuthAccount

FxaDeviceConstellation:

• Simplified device finalization
• Let FxaClient handle auth errors when sending device commands
• Updating FxaDeviceSettingsCache is now handled in the FxaAccountManager.onFxaEvent method

FxaAccountManager:

• Replaced state machine code with the new queueAction() + onFxaEvent functions instead.

Tests:

• Removed a good deal of tests, the app-services tests should replace these.

• There were several tests that dealt with state persistence plus oauth. I think these should be convered by the regular state change tests, were they testing something else?

• Auth recovery is handled by several tests starting with FxaActionProcessor calls checkAuthorizationStatus after auth errors

• The main tests I tried to keep were the ones focused on observer notification and other integrations with the larger code base. We're mostly just forwarding the events that FxaClient sends us to handle this.

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

1. Click on Checks at the top of the PR page.
2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
4. Click on View task in Taskcluster in the new DETAILS section.
5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1856729

[bendk]

Lots of code changes here, but hopefully the comments make some sense.

Here's the corresponding app-services PR. I set up a branch build in that PR, if it works you should be able to get an APK for testing from the branch-build-firefox-android taskcluster job.

One thing that I tried to do was improve the manual testing situation. Now if you run adb logcat | grep fxa_client you should see all logging from the app-services client and there should be some decent info on what's going on under the hood. I also added several items to the "sync debug menu", that appears on top of the settings page when you click the FF icon 5 times in a row (preferably in front of a bathroom mirror with one lit candle. The bottom of FirefoxAccountManager.kt has some notes on what's going on under the hood there.

Here's the log results from my manual testing. The first one is just the login process, the next ones are what happened when I pushed those debug buttons. Note: I don't recommend sharing these log files in general, since they could contain auth tokens. I only shared them because I was using a fake account. To me it looks like the client is doing the right thing in those logs:

• When there's a network error, it retries the action once, recovers and proceeds on.
• A similar sequence of events happens when the auth token is invalid.
• When the refresh token is invalid, it retries the action, fails, then puts the user in a disconnected-from-auth-issues state (IIRC, the FF menu item was yellow and said "Authentication problems")

[jonalmeida]

cc @MatthewTighe as this might effect the SyncStore work.

[github-actions]

🚧 Commit message is using the wrong format: _Switch to new app-services FxaClient

Use the new functionality from app-services to replace code in
service-firefox-accounts

General:

• Updated code to use FxaConfig class rather than Config. Config
  wrapped FxaConfig, but provided little value.
• Updated handleFxaExceptions and removed all other Utils functions
• Use FxaClient's CoroutineContext to run all fxa-related jobs.
• Many FxaClient methods no longer directly return a result, but instead
  input an optional CompletableDeferred<T> that they complete when the
  operation is finish. See the app-services PR for details.

FirefoxAccount:

• Don't wrap FxaClient calls in withContext, this is handled in
  app-services now.
• Updated FirefoxAccount docs to describe its relationship to
  OAuthAccount.
• Make AccountStorage return a FirefoxAccount directly instead of an
  OAuthAccount

FxaDeviceConstellation:

• Let FxaClient handle auth errors when sending device commands
• Updating FxaDeviceSettingsCache is now handled in the
  FxaAccountManager.onFxaEvent method

FxaAccountManager:

• Replaced state machine code with the new queueAction() +
  onFxaEvent functions instead.

Tests:

• Removed a good deal of tests, the app-services tests should replace
  these.

• There were several tests that dealt with state persistence plus
  oauth. I think these should be convered by the regular state change
  tests, were they testing something else?

• Auth recovery is handled by several tests starting with
  FxaActionProcessor calls checkAuthorizationStatus after auth errors

• The main tests I tried to keep were the ones focused on observer
  notification and other integrations with the larger code base.
  We're mostly just forwarding the events that FxaClient sends us to
  handle this._

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

[bendk]

I made some updates to the app-services side of things based on review of that PR. The main change is that FxaAuthState is now "flat", all subclasses are objects with no fields.

That required some changes to the android-components code, but nothing too big. It brings things closer to the old system, which seems good.

[bendk]

I've been thinking on how to best land this and about the risk in general. Maybe we could try to mitigate the risk by a) adding metrics and b) introducing the changes in steps. I think the process could go something like this:

• Before anything, we add metrics for each Fxa operation that track the success/error rate and group errors into a couple buckets (maybe network, auth, other). We currently do something similar for logins and places (dashbord)
• Watch the metrics for a bit to get a baseline.
• Introduce the app-services changes but only make minor/mechanical changes to android-components, like switching to the new FxaConfig class. android-components would keep using it's current state-machine code.
• Watch the metrics to see if there are significant changes. We can also watch the new sentry reports for things like fxa-client-scoped-key-missing.
• Switch android-components over to user the app-services state-machine code. This is going to be a large change and I don't see any way to break it down into smaller parts. We could consider rolling this out as an experiment, but that would not be trivial and may introduce its own set of risks.
• Watch the metrics to see if there are significant changes.

Zooming out a bit, I think the risk falls into 2 categories:

• There are a lot of known corner cases, like password resets, flakey network, etc. These should be well tested by QA. The new code has fairly good logging, so hopefully that will help debug any issues that come up.
• There's a lot of unknown corner cases that may be captured in all those unit tests that I removed. I tried to capture those cases in the new unit tests, but it's hard to tell. I don't think we can fully eliminate all risk here, but adding metrics and introducing the changes piecemeal can help.

[mergify]

This pull request has conflicts when rebasing. Could you fix it @bendk? 🙏

[MatthewTighe]

Hey there! I see that there hasn't been much other activity here outside yourself since this was posted. I was wondering if you were in contact with anyone from the Android team that was generally prepared to help with this, or if you were still looking for assistance. I think breaking it down a bit would definitely help ease ingesting it, and the steps you outlined above look like a good starting place. Also, love to see this:

[mavduevskiy]

@MatthewTighe
Ben reached out to me recently to help him lending it, but I am still in the process of digesting it and struggle to find enough time to focus solely on it last couple of days. I am committed to do it. But if you are interested in chiming in, feel free to do so :)

[MatthewTighe]

@mavduevskiy Just wanted to make sure we weren't leaving anyone hanging 🙂 I'm happy to get out of the way on this one, but please don't hesitate to reach out if I can help!

[jonalmeida]

Folks, if anyone wants to pair on this review, happy to pair on it. 🙂

[bendk]

I think we're going to be going forward with tracking these metrics. I just opened a data review issue and a PR to implement them in app-services. I really appreciate the effort here, sorry to drop such a large patch on you all. If it would help, I could definitely jump on a call and talk through some of the changes.

[mavduevskiy]

Talked with @bendk yesterday: we want to wait until the A-S change lands before approving/reviewing this thoroughly.

[bendk]

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.

[bendk]

I think everyone agrees with the slower, step-by-step, approach. Let's close this one and start with a much smaller one: #4228.