Add offline use case for `migrateFromSessionToken` #2481

vladikoff · 2020-01-16T15:13:10Z

Pull Request checklist

Quality: This PR builds and tests run cleanly
- cargo test --all produces no test failures
- cargo clippy --all --all-targets --all-features runs without emitting any warnings
- cargo fmt does not produce any changes to the code
- ./gradlew ktlint detekt runs without emitting any warnings
- swiftformat --swiftversion 4 megazords components/*/ios && swiftlint runs without emitting any warnings or producing changes
- Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.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 dilligence applied in selecting them.

vladikoff · 2020-01-16T15:15:18Z

components/fxa-client/src/lib.rs

+                if self.state.in_flight_migration.is_some() {
+                    // if there's a pending migration state, then try to provision a refresh_token
+                    // by exchanging the session token for the refresh token
+                    self.helper_migration_network_methods();


@rfk @eoger any smart ideas here how to deal with the mutable here?

error[E0596]: cannot borrow `*self` as mutable, as it is behind a `&` reference --> components/fxa-client/src/device.rs:24:29 | 23 | pub fn get_devices(&self) -> Result<Vec<Device>> { | ----- help: consider changing this to be a mutable reference: `&mut self` 24 | let refresh_token = self.get_refresh_token()?; | ^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable error[E0596]: cannot borrow `*self` as mutable, as it is behind a `&` reference --> components/fxa-client/src/device.rs:104:29 | 99 | &self, | ----- help: consider changing this to be a mutable reference: `&mut self` ... 104 | let refresh_token = self.get_refresh_token()?; | ^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable error[E0502]: cannot borrow `self.client` as immutable because it is also borrowed as mutable --> components/fxa-client/src/device.rs:146:13 | 144 | let refresh_token = self.get_refresh_token()?; | ---- mutable borrow occurs here 145 | let pending_commands = 146 | self.client | ^^^^^^^^^^^ immutable borrow occurs here 147 | .pending_commands(&self.state.config, refresh_token, index, limit)?; | ------------- mutable borrow later used here error[E0502]: cannot borrow `self.state.config` as immutable because it is also borrowed as mutable --> components/fxa-client/src/device.rs:147:35 | 144 | let refresh_token = self.get_refresh_token()?; | ---- mutable borrow occurs here ... 147 | .pending_commands(&self.state.config, refresh_token, index, limit)?; | ^^^^^^^^^^^^^^^^^^ ------------- mutable borrow later used here | | | immutable borrow occurs here error[E0596]: cannot borrow `*self` as mutable, as it is behind a `&` reference --> components/fxa-client/src/device.rs:264:29 | 263 | fn update_device(&self, update: DeviceUpdateRequest<'_>) -> Result<UpdateDeviceResponse> { | ----- help: consider changing this to be a mutable reference: `&mut self` 264 | let refresh_token = self.get_refresh_token()?; | ^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable

Or do we need to rewrite all the cases where the refresh_token is used:

No way around it, we are modifying the state.
You also need to keep in mind that these methods calling refresh_token() can now potentially mutate the persisted state, which means you need to call tryPersistState in the Kotlin/Swift wrappers after calling these. (also please add the /// **💾 This method may alter the persisted account state.** doc)

eoger

Thanks, this is more or less what I was expecting to happen in this PR, however you may have found an important thing regarding the persisted state, so it's important to fix :)

eoger · 2020-01-16T16:02:31Z

CHANGES_UNRELEASED.md

+### Features
+
+* `migrateFromSessionToken` now handles offline use cases. It caches the data the consumers originally provide.
+If there's no network connectivity then the migration.


then the migration? 🤣

eoger · 2020-01-16T16:06:32Z

components/fxa-client/src/lib.rs

+                if self.state.in_flight_migration.is_some() {
+                    // if there's a pending migration state, then try to provision a refresh_token
+                    // by exchanging the session token for the refresh token
+                    self.helper_migration_network_methods();


Is this wrong to assume we always want to do an in-flight-migration? I think we should do the if is_some() migrate before the match self.state.refresh_token {

yeap there's a if self.state.in_flight_migration.is_some() {

Sorry I wasn't really clear, I meant we could execute that block before the rest of the function.

eoger · 2020-01-16T16:08:34Z

components/fxa-client/src/lib.rs

+                if self.state.in_flight_migration.is_some() {
+                    // if there's a pending migration state, then try to provision a refresh_token
+                    // by exchanging the session token for the refresh token
+                    self.helper_migration_network_methods();


No way around it, we are modifying the state.
You also need to keep in mind that these methods calling refresh_token() can now potentially mutate the persisted state, which means you need to call tryPersistState in the Kotlin/Swift wrappers after calling these. (also please add the /// **💾 This method may alter the persisted account state.** doc)

components/fxa-client/src/migrator.rs

eoger · 2020-01-16T16:11:17Z

components/fxa-client/src/migrator.rs

+        }
+    }
+
+    pub fn helper_migration_network_methods(&mut self) -> Result<()> {


Nit: how about do_migrate or something else? helper_migration_network_methods was a bad temporary name I came up with

Fixes #2396

vladikoff · 2020-01-16T22:29:02Z

components/fxa-client/src/device.rs

        let refresh_token = self.get_refresh_token()?;
-        self.client.devices(&self.state.config, &refresh_token)
+        client.devices(config, &refresh_token)


@thomcc do you think it's the right fix for https://github.com/mozilla/application-services/pull/2481/files/1b95d9e91f653a30e00ea6aeed354594523af57f#diff-1bde00663b7e4a063fc914419e896b59 ? (we are cloning clients and config everywhere)

hm i think we figured it out! the refresh_token was a ref

rfk

Looks like a good start!

As noted below, I think we need more nuanced handling around the type of error, because there are things other than network errors that we might run in to here.

I'd also like to see some explicit testcases to cover at least the following scenarios:

that we correctly handle a network error, and subsequent operations will complete the migration.
that we correctly handle an invalid session token, and subsequent operations don't keep retrying the migration
that we correctly handle a network error during migration, and an invalid sessionToken discovered only after retrying it

rfk · 2020-01-17T01:22:47Z

components/fxa-client/src/migrator.rs

-        let k_xcs = hex::decode(k_xcs)?;
-        let k_xcs = base64::encode_config(&k_xcs, base64::URL_SAFE_NO_PAD);
-        let scoped_key_data = self.client.scoped_key_data(
+        // Gather the scope key data


nit "scope" => "scoped">

rfk · 2020-01-17T02:02:12Z

CHANGES_UNRELEASED.md

+### Features
+
+* `migrateFromSessionToken` now handles offline use cases. It caches the data the consumers originally provide.
+If there's no network connectivity then the migration could be retried using the new `retryMigration` method.


I don't see a retryMigration method, is this changelog entry from an older version of the code?

rfk · 2020-01-17T02:04:51Z

components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FirefoxAccount.kt

     * This performs network requests, and should not be used on the main thread.
     */
-    fun migrateFromSessionToken(sessionToken: String, kSync: String, kXCS: String) {
+    fun migrateFromSessionToken(sessionToken: String, kSync: String, kXCS: String): Boolean {


I'm not opposed to adding it, but I'm also not sure what the caller is supposed to do with the information in the return value here. Are they supposed to change behaviour based on whether it's true or false, or is it for including in migration-related metrics? If the later than a json blob of metrics data may be a little clearer for consumers, by analogy with the other migration-related components.

rfk · 2020-01-17T02:07:27Z

components/fxa-client/src/device.rs

@@ -96,14 +98,16 @@ impl FirefoxAccount {
    }

    pub(crate) fn invoke_command(
-        &self,
+        &mut self,


So just to check that I understand...all this mutable-self is because these methods might now alter the persisted account state by completing the migration, right?

I recall a bug a while ago where Fenix was deserializing multiple copies of the FirefoxAccount object from JSON, one in the main app and then others in background worker threads. Do we still have any risk of that here? If so then I wonder if we might have some strange behaviours lurking where we try to complete the migration multiple times, possibly causing the FxA server's device-registration logic to get confused.

/cc @grigoryk

So just to check that I understand...all this mutable-self is because these methods might now alter the persisted account state by completing the migration, right?

Yes it makes me nervous too. I almost wonder if we should go with a different approach:

return boolean on fn migrate

let a-c call the same method later if we returned false.

rfk · 2020-01-17T02:12:28Z

components/fxa-client/src/lib.rs

-            None => Err(ErrorKind::NoRefreshToken.into()),
+            None => {
+                log::info!("No refresh_token, attempting to recover via migration state");
+                // if we have no refresh token, check if this is the case of a failed migration


nit "the case of" -> "a case of" or "due to" or similar?

rfk · 2020-01-17T02:13:05Z

components/fxa-client/src/lib.rs

+                // if we have no refresh token, check if this is the case of a failed migration
+                if self.state.in_flight_migration.is_some() {
+                    // if there's a pending migration state, then try to provision a refresh_token
+                    // by exchanging the session token for the refresh token


nit: this is not an "exchange" because you keep the sessionToken as well, you are using the sessionToken to grant the refresh token.

rfk · 2020-01-17T02:13:56Z

components/fxa-client/src/lib.rs

@@ -257,7 +287,7 @@ impl FirefoxAccount {
    ///
    /// **💾 This method alters the persisted account state.**
    pub fn disconnect(&mut self) {
-        if let Some(ref refresh_token) = self.state.refresh_token {
+        if let Some(ref refresh_token) = self.state.refresh_token.clone() {


Out of curiosity, why did this clone become necessary?

rfk · 2020-01-17T02:14:59Z

components/fxa-client/src/migrator.rs

+            }
+            Err(err) => {
+                log::info!("Failed to perform network requests: {}", err);
+                // don't throw hard error here, let the consumers try again


This comment may be out of date, since it's not the consumer that tries again, we try again ourselves internally.

rfk · 2020-01-17T02:22:14Z

components/fxa-client/src/migrator.rs

+        }
+    }
+
+    pub fn helper_migration_network_methods(&mut self, client: Arc<dyn FxAClient>) -> Result<()> {


There are two ways that this could fail, and I think they need to be treated differently.

In one case, it might fail due to some sort of transient network error. That's fine, trying again later will probably resolve the issue.

In the other case, it might fail due to some sort of permanently-fatal error. For example the provided sessionToken might be invalid, and we don't find that out until we hit the network and get an auth error back from the server. We must avoid getting stuck in an endless retry loop in the case of such permanent errors.

I think it's fine to re-throw the error in both cases, but you're going to need to clear the in-flight migration state in the case of a permanent error.

vladikoff · 2020-01-20T20:22:27Z

Rewrite in #2492

vladikoff requested review from rfk and eoger January 16, 2020 15:13

vladikoff commented Jan 16, 2020

View reviewed changes

eoger reviewed Jan 16, 2020

View reviewed changes

vladikoff added 2 commits January 16, 2020 16:11

Add offline use case for migrateFromSessionToken

b851364

Fixes #2396

wip

dc89194

vladikoff force-pushed the migration-offline-v3 branch from 1b95d9e to dc89194 Compare January 16, 2020 22:27

vladikoff commented Jan 16, 2020

View reviewed changes

rfk suggested changes Jan 17, 2020

View reviewed changes

vladikoff closed this Jan 20, 2020

rfk mentioned this pull request Jan 21, 2020

Add offline use case for 'migrateFromSessionToken' #2492

Merged

4 tasks

eoger deleted the migration-offline-v3 branch April 21, 2020 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add offline use case for `migrateFromSessionToken` #2481

Add offline use case for `migrateFromSessionToken` #2481

vladikoff commented Jan 16, 2020

vladikoff Jan 16, 2020

eoger Jan 16, 2020

eoger left a comment

eoger Jan 16, 2020

eoger Jan 16, 2020

vladikoff Jan 16, 2020

eoger Jan 16, 2020

eoger Jan 16, 2020

eoger Jan 16, 2020

vladikoff Jan 16, 2020

vladikoff Jan 16, 2020

rfk left a comment

rfk Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

eoger Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

rfk Jan 17, 2020

vladikoff commented Jan 20, 2020

Add offline use case for migrateFromSessionToken #2481

Add offline use case for migrateFromSessionToken #2481

Conversation

vladikoff commented Jan 16, 2020

Pull Request checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff commented Jan 20, 2020

Add offline use case for `migrateFromSessionToken` #2481

Add offline use case for `migrateFromSessionToken` #2481