Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add offline use case for 'migrateFromSessionToken' #2492

Merged
merged 1 commit into from Jan 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion CHANGES_UNRELEASED.md
Expand Up @@ -4,14 +4,21 @@

[Full Changelog](https://github.com/mozilla/application-services/compare/v0.48.3...master)


## FxA Client

### What's New

- `FirefoxAccount` is now deprecated
- Introducing `FxAccountManager` which provides a higher-level interface to Firefox Accounts. Among other things, this class handles (and can recover from) authentication errors, exposes device-related account methods, handles its own keychain storage and fires observer notifications for important account events.

- `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 `retryMigrateFromSessionToken` method.
Consumers may also use the `isInMigrationState` method to check if there's a migration in progress.
([#2492](https://github.com/mozilla/application-services/pull/2492))

### Breaking changes

- `FirefoxAccount.fromJSON(json: String)` has been replaced by the `FirefoxAccount(fromJsonState: String)` constructor.

- `migrateFromSessionToken` now returns a metrics JSON object if the migration succeeded.
([#2492](https://github.com/mozilla/application-services/pull/2492))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest rewording this for clarity, to focus on the things that consumers need to know in the order they need to know them. Along the lines of:

  • migrateFromSessionToken now has special handling for transient failures such as network errors. If the migration fails due to a transient error, then the provided credentials will be cached so that the migration can be retried later. Consumers can call the isInMigrationState method to check if there is a cached migration in progress, and retryMigrateFromSessionToken to retry it.

1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Expand Up @@ -13,6 +13,7 @@ import mozilla.appservices.fxaclient.rust.RustError
import mozilla.appservices.support.native.toNioDirectBuffer
import java.nio.ByteBuffer
import java.util.concurrent.atomic.AtomicLong
import org.json.JSONObject

/**
* FirefoxAccount represents the authentication state of a client.
Expand Down Expand Up @@ -317,14 +318,28 @@ class FirefoxAccount(handle: FxaHandle, persistCallback: PersistCallback?) : Aut
* @param sessionToken 64 character string of hex-encoded bytes
* @param kSync 128 character string of hex-encoded bytes
* @param kXCS 32 character string of hex-encoded bytes
* @return JSONObject JSON object with the result of the migration
* This performs network requests, and should not be used on the main thread.
*/
fun migrateFromSessionToken(sessionToken: String, kSync: String, kXCS: String) {
rustCallWithLock { e ->
LibFxAFFI.INSTANCE.fxa_migrate_from_session_token(this.handle.get(), sessionToken, kSync, kXCS,
false, e)
}
fun migrateFromSessionToken(sessionToken: String, kSync: String, kXCS: String): JSONObject {
val json = rustCallWithLock { e ->
LibFxAFFI.INSTANCE.fxa_migrate_from_session_token(this.handle.get(), sessionToken, kSync, kXCS, false, e)
}.getAndConsumeRustString()

this.tryPersistState()
return JSONObject(json)
}

/**
* Migrate from a logged-in Firefox Account, takes ownership of the provided session token.
*
* @return bool Returns a boolean if we are in a migration state
*/
fun isInMigrationState(): Boolean {
rustCall { e ->
val state = LibFxAFFI.INSTANCE.fxa_is_in_migration_state(this.handle.get(), e)
return state.toInt() != 0
}
}

/**
Expand All @@ -334,14 +349,32 @@ class FirefoxAccount(handle: FxaHandle, persistCallback: PersistCallback?) : Aut
* @param sessionToken 64 character string of hex-encoded bytes
* @param kSync 128 character string of hex-encoded bytes
* @param kXCS 32 character string of hex-encoded bytes
* @return JSONObject JSON object with the result of the migration
* This performs network requests, and should not be used on the main thread.
*/
fun copyFromSessionToken(sessionToken: String, kSync: String, kXCS: String) {
rustCallWithLock { e ->
LibFxAFFI.INSTANCE.fxa_migrate_from_session_token(this.handle.get(), sessionToken, kSync, kXCS,
true, e)
}
fun copyFromSessionToken(sessionToken: String, kSync: String, kXCS: String): JSONObject {
val json = rustCallWithLock { e ->
LibFxAFFI.INSTANCE.fxa_migrate_from_session_token(this.handle.get(), sessionToken, kSync, kXCS, true, e)
}.getAndConsumeRustString()

this.tryPersistState()
return JSONObject(json)
}

/**
* Retry migration from a logged-in Firefox Account.
*
* Modifies the FirefoxAccount state.
* @return JSONObject JSON object with the result of the migration
* This performs network requests, and should not be used on the main thread.
*/
fun retryMigrateFromSessionToken(): JSONObject {
val json = rustCallWithLock { e ->
LibFxAFFI.INSTANCE.fxa_retry_migrate_from_session_token(this.handle.get(), e)
}.getAndConsumeRustString()

this.tryPersistState()
return JSONObject(json)
}

/**
Expand Down
Expand Up @@ -92,7 +92,14 @@ internal interface LibFxAFFI : Library {
kXCS: String,
copySessionToken: Boolean,
e: RustError.ByReference
)
): Pointer?

fun fxa_is_in_migration_state(
fxa: FxaHandle,
e: RustError.ByReference
): Byte

fun fxa_retry_migrate_from_session_token(fxa: FxaHandle, e: RustError.ByReference): Pointer?

fun fxa_str_free(string: Pointer)
fun fxa_bytebuffer_free(buffer: RustBuffer.ByValue)
Expand Down
27 changes: 20 additions & 7 deletions components/fxa-client/examples/migration.rs
@@ -1,11 +1,10 @@
use cli_support::prompt::prompt_string;
use fxa_client::FirefoxAccount;
use std::{thread, time};

static CLIENT_ID: &str = "3c49430b43dfba77";
//static CONTENT_SERVER: &str = "https://latest.dev.lcip.org";
static CONTENT_SERVER: &str = "http://127.0.0.1:3030";
//static REDIRECT_URI: &str = "https://latest.dev.lcip.org/oauth/success/3c49430b43dfba77";
static REDIRECT_URI: &str = "http://127.0.0.1:3030/oauth/success/3c49430b43dfba77";
static CONTENT_SERVER: &str = "https://accounts.firefox.com";
static REDIRECT_URI: &str = "https://accounts.firefox.com/oauth/success/3c49430b43dfba77";

fn main() {
let mut fxa = FirefoxAccount::new(CONTENT_SERVER, CLIENT_ID, REDIRECT_URI);
Expand All @@ -15,8 +14,22 @@ fn main() {
let k_sync: String = prompt_string("k_sync").unwrap();
println!("Enter kXCS (hex-string):");
let k_xcs: String = prompt_string("k_xcs").unwrap();
fxa.migrate_from_session_token(&session_token, &k_sync, &k_xcs, true)
.unwrap();
println!("WOW! You've been migrated.");
let migration_result =
match fxa.migrate_from_session_token(&session_token, &k_sync, &k_xcs, true) {
Ok(migration_result) => migration_result,
Err(err) => {
println!("Error: {}", err);
// example for offline behaviour
loop {
thread::sleep(time::Duration::from_millis(5000));
let retry = fxa.try_migration();
match retry {
Ok(result) => break result,
Err(_) => println!("Retrying... Are you connected to the internet?"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing discussion from the previous PR, each migration attempt could fail for either both fatal or non-fatal reasons. It would be useful for this example to show how to handle the two different cases. For example right now, if I enter an invalid sessionToken into this example script, then IIUC it will loop forever asking me whether I'm connected to the internet. We don't want to get anyone's migrated Fennec stuck in such a loop in practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'll also note that "not connected to the internet" is not necessarily the only transient error; 500-level server errors should probably also be treated as transient and retried).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's only for testing purposes, but I'd like to see this example use the same logic as we expect consumers to use in practice, which IIUC is basically:

match migrate_from_session_token(...) {
  Ok(result) => result,
  Err(...) => {
    while is_in_migration_state() {
       sleep(...)
       match retry_migrate_from_session_token() {
         Ok(result) => result,
         Err(...) => continue,
        }
    }
  }
}

}
}
}
};
println!("WOW! You've been migrated in {:?}.", migration_result);
println!("JSON: {}", fxa.to_json().unwrap());
}
1 change: 1 addition & 0 deletions components/fxa-client/ffi/Cargo.toml
Expand Up @@ -12,6 +12,7 @@ crate-type = ["lib"]
[dependencies]
ffi-support = { path = "../../support/ffi" }
log = "0.4.8"
serde_json = "1.0.44"
lazy_static = "1.4.0"
url = "2.1.1"
prost = "0.6.1"
Expand Down
34 changes: 30 additions & 4 deletions components/fxa-client/ffi/src/lib.rs
Expand Up @@ -239,14 +239,40 @@ pub extern "C" fn fxa_migrate_from_session_token(
k_xcs: FfiStr<'_>,
copy_session_token: bool,
error: &mut ExternError,
) {
) -> *mut c_char {
log::debug!("fxa_migrate_from_session_token");
ACCOUNTS.call_with_result_mut(error, handle, |fxa| {
ACCOUNTS.call_with_result_mut(error, handle, |fxa| -> fxa_client::Result<String> {
let session_token = session_token.as_str();
let k_sync = k_sync.as_str();
let k_xcs = k_xcs.as_str();
fxa.migrate_from_session_token(session_token, k_sync, k_xcs, copy_session_token)
});
let migration_metrics =
fxa.migrate_from_session_token(session_token, k_sync, k_xcs, copy_session_token)?;
let result = serde_json::to_string(&migration_metrics)?;
Ok(result)
})
}

/// Check if there is migration state.
#[no_mangle]
pub extern "C" fn fxa_is_in_migration_state(handle: u64, error: &mut ExternError) -> u8 {
log::debug!("fxa_is_in_migration_state");
ACCOUNTS.call_with_result(error, handle, |fxa| -> fxa_client::Result<u8> {
Ok(fxa.is_in_migration_state() as u8)
})
}

/// Retry the migration attempt using the stored migration state.
#[no_mangle]
pub extern "C" fn fxa_retry_migrate_from_session_token(
handle: u64,
error: &mut ExternError,
) -> *mut c_char {
log::debug!("fxa_retry_migrate_from_session_token");
ACCOUNTS.call_with_result_mut(error, handle, |fxa| -> fxa_client::Result<String> {
let migration_metrics = fxa.try_migration()?;
let result = serde_json::to_string(&migration_metrics)?;
Ok(result)
})
}

/// Try to get an access token.
Expand Down
3 changes: 3 additions & 0 deletions components/fxa-client/src/error.rs
Expand Up @@ -32,6 +32,9 @@ pub enum ErrorKind {
#[fail(display = "No stored session token")]
NoSessionToken,

#[fail(display = "No stored migration data")]
NoMigrationData,

#[fail(display = "No stored current device id")]
NoCurrentDeviceId,

Expand Down
11 changes: 11 additions & 0 deletions components/fxa-client/src/lib.rs
Expand Up @@ -57,6 +57,14 @@ pub struct FirefoxAccount {
flow_store: HashMap<String, OAuthFlow>,
}

#[derive(Clone, Serialize, Deserialize)]
pub struct MigrationData {
k_xcs: String,
k_sync: String,
copy_session_token: bool,
session_token: String,
}

// If this structure is modified, please:
// 1. Check if a migration needs to be done, as
// these fields are persisted as a JSON string
Expand All @@ -79,6 +87,7 @@ pub(crate) struct StateV2 {
access_token_cache: HashMap<String, AccessTokenInfo>,
session_token: Option<String>, // Hex-formatted string.
last_seen_profile: Option<CachedResponse<Profile>>,
in_flight_migration: Option<MigrationData>,
}

impl StateV2 {
Expand All @@ -97,6 +106,7 @@ impl StateV2 {
access_token_cache: HashMap::new(),
device_capabilities: HashSet::new(),
session_token: None,
in_flight_migration: None,
}
}
}
Expand Down Expand Up @@ -125,6 +135,7 @@ impl FirefoxAccount {
current_device_id: None,
last_seen_profile: None,
access_token_cache: HashMap::new(),
in_flight_migration: None,
})
}

Expand Down
81 changes: 74 additions & 7 deletions components/fxa-client/src/migrator.rs
Expand Up @@ -2,7 +2,14 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use crate::{error::*, scoped_keys::ScopedKey, scopes, FirefoxAccount};
use crate::{error::*, scoped_keys::ScopedKey, scopes, FirefoxAccount, MigrationData};
use serde_derive::*;
use std::time::Instant;

#[derive(Serialize, Deserialize, PartialEq, Debug, Clone, Default)]
pub struct FxAMigrationResult {
pub total_duration: u128,
}

impl FirefoxAccount {
/// Migrate from a logged-in with a sessionToken Firefox Account.
Expand All @@ -24,20 +31,79 @@ impl FirefoxAccount {
k_sync: &str,
k_xcs: &str,
copy_session_token: bool,
) -> Result<()> {
) -> Result<FxAMigrationResult> {
// if there is already a session token on account, we error out.
if self.state.session_token.is_some() {
return Err(ErrorKind::IllegalState("Session Token is already set.").into());
}

let migration_session_token = if copy_session_token {
self.state.in_flight_migration = Some(MigrationData {
k_sync: k_sync.to_string(),
k_xcs: k_xcs.to_string(),
copy_session_token,
session_token: session_token.to_string(),
});

self.try_migration()
}

/// Check if the client is in a pending migration state
pub fn is_in_migration_state(&self) -> bool {
self.state.in_flight_migration.is_some()
}

pub fn try_migration(&mut self) -> Result<FxAMigrationResult> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming nit: I think the overall structure of the code would be a bit clearer if this were called retry_migrate_from_session_token for symmetry with what it's called in the higher layers.

let import_start = Instant::now();

match self.network_migration() {
Ok(_) => {}
Err(err) => {
match err.kind() {
ErrorKind::RemoteError {
code: 500..=599, ..
}
| ErrorKind::RemoteError { code: 429, .. }
| ErrorKind::RequestError(_) => {
// network errors that will allow hopefully migrate later
log::warn!("Network error: {:?}", err);
return Err(err);
}
_ => {
// probably will not recover

self.state.in_flight_migration = None;

return Err(err);
}
};
}
}

self.state.in_flight_migration = None;

let metrics = FxAMigrationResult {
total_duration: import_start.elapsed().as_millis(),
};

Ok(metrics)
}

fn network_migration(&mut self) -> Result<()> {
let migration_data = match self.state.in_flight_migration {
Some(ref data) => data.clone(),
None => {
return Err(ErrorKind::NoMigrationData.into());
}
};

let migration_session_token = if migration_data.copy_session_token {
let duplicate_session = self
.client
.duplicate_session(&self.state.config, &session_token)?;
.duplicate_session(&self.state.config, &migration_data.session_token)?;

duplicate_session.session_token
} else {
session_token.to_string()
migration_data.session_token.to_string()
};

// Trade our session token for a refresh token.
Expand All @@ -49,9 +115,9 @@ impl FirefoxAccount {
self.handle_oauth_response(oauth_response, None)?;

// Synthesize a scoped key from our kSync.
let k_sync = hex::decode(k_sync)?;
let k_sync = hex::decode(&migration_data.k_sync)?;
let k_sync = base64::encode_config(&k_sync, base64::URL_SAFE_NO_PAD);
let k_xcs = hex::decode(k_xcs)?;
let k_xcs = hex::decode(&migration_data.k_xcs)?;
let k_xcs = base64::encode_config(&k_xcs, base64::URL_SAFE_NO_PAD);
let scoped_key_data = self.client.scoped_key_data(
&self.state.config,
Expand All @@ -72,6 +138,7 @@ impl FirefoxAccount {
self.state
.scoped_keys
.insert(scopes::OLD_SYNC.to_string(), k_sync_scoped_key);

Ok(())
}
}
1 change: 1 addition & 0 deletions components/fxa-client/src/state_persistence.rs
Expand Up @@ -90,6 +90,7 @@ impl From<StateV1> for Result<StateV2> {
session_token: None,
current_device_id: None,
last_seen_profile: None,
in_flight_migration: None,
access_token_cache: HashMap::new(),
})
}
Expand Down