Skip to content

Commit

Permalink
Added check for missing scoped keys in auth tokens.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bendk committed Oct 3, 2023
1 parent 886463d commit eff00c4
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 9 deletions.
11 changes: 10 additions & 1 deletion components/fxa-client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ pub enum FxaError {
/// **Note:** This error is currently only thrown in the Swift language bindings.
#[error("the requested authentication flow was not active")]
WrongAuthFlow,
/// A required scoped key was missing in the server response
#[error("A required scoped key was missing")]
ScopedKeyMissing,
/// Thrown if there is a panic in the underlying Rust code.
///
/// **Note:** This error is currently only thrown in the Kotlin language bindings.
Expand Down Expand Up @@ -103,6 +106,9 @@ pub enum Error {
#[error("Remote key and local key mismatch")]
MismatchedKeys,

#[error("A required scoped key was missing")]
ScopedKeyMissing,

#[error("Client: {0} is not allowed to request scope: {1}")]
ScopeNotAllowed(String, String),

Expand Down Expand Up @@ -193,7 +199,10 @@ impl GetErrorHandling for Error {
Error::RequestError(_) => {
ErrorHandling::convert(crate::FxaError::Network).log_warning()
}
_ => ErrorHandling::convert(crate::FxaError::Other).log_warning(),
Error::ScopedKeyMissing => ErrorHandling::convert(crate::FxaError::ScopedKeyMissing)
.report_error("fxa-client-scoped-key-missing"),
_ => ErrorHandling::convert(crate::FxaError::Other)
.report_error("fxa-client-other-error"),
}
}
}
8 changes: 6 additions & 2 deletions components/fxa-client/src/fxa_client.udl
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ enum FxaError {
// **Note:** This error is currently only thrown in the Swift language bindings.
"WrongAuthFlow",

// A required scoped key was missing in the server response
"ScopedKeyMissing",

// Thrown if there is a panic in the underlying Rust code.
//
// **Note:** This error is currently only thrown in the Kotlin language bindings.
Expand Down Expand Up @@ -579,6 +582,8 @@ interface FirefoxAccount {
// - This must be one of the scopes requested during the signin flow.
// - Only a single scope is supported; for multiple scopes request multiple tokens.
// - `ttl` - optionally, the time for which the token should be valid, in seconds.
// - `require_scoped_key` - optionally, throw [FxaError.ScopedKeyMissing] if the scoped_key
// is not present in the server response.
//
// # Notes
//
Expand All @@ -587,8 +592,7 @@ interface FirefoxAccount {
// before requesting a fresh token.
//
[Throws=FxaError]
AccessTokenInfo get_access_token([ByRef] string scope, i64? ttl );

AccessTokenInfo get_access_token([ByRef] string scope, optional i64? ttl = null, optional boolean require_scoped_key = false);

// Get the session token for the user's account, if one is available.
//
Expand Down
25 changes: 24 additions & 1 deletion components/fxa-client/src/internal/oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,24 @@ impl FirefoxAccount {
///
/// * `scopes` - Space-separated list of requested scopes.
/// * `ttl` - the ttl in seconds of the token requested from the server.
/// * `require_scpoed_key` - return [Error.ScopedKeyMissing] if the scoped_key is not present
/// in the server response.
///
/// **💾 This method may alter the persisted account state.**
pub fn get_access_token(&mut self, scope: &str, ttl: Option<u64>) -> Result<AccessTokenInfo> {
pub fn get_access_token(
&mut self,
scope: &str,
ttl: Option<u64>,
require_scoped_key: bool,
) -> Result<AccessTokenInfo> {
if scope.contains(' ') {
return Err(Error::MultipleScopesRequested);
}
if let Some(oauth_info) = self.state.get_cached_access_token(scope) {
if oauth_info.expires_at > util::now_secs() + OAUTH_MIN_TIME_LEFT {
if require_scoped_key {
oauth_info.check_scoped_key_present()?
}
return Ok(oauth_info.clone());
}
}
Expand Down Expand Up @@ -83,6 +93,9 @@ impl FirefoxAccount {
};
self.state
.add_cached_access_token(scope, token_info.clone());
if require_scoped_key {
token_info.check_scoped_key_present()?
}
Ok(token_info)
}

Expand Down Expand Up @@ -550,6 +563,16 @@ pub struct AccessTokenInfo {
pub expires_at: u64, // seconds since epoch
}

impl AccessTokenInfo {
pub fn check_scoped_key_present(&self) -> Result<()> {
if self.key.is_none() {
Err(Error::ScopedKeyMissing)
} else {
Ok(())
}
}
}

impl TryFrom<AccessTokenInfo> for crate::AccessTokenInfo {
type Error = Error;
fn try_from(info: AccessTokenInfo) -> Result<Self> {
Expand Down
2 changes: 1 addition & 1 deletion components/fxa-client/src/internal/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl FirefoxAccount {
}
etag = Some(cached_profile.etag.clone());
}
let profile_access_token = self.get_access_token(scopes::PROFILE, None)?.token;
let profile_access_token = self.get_access_token(scopes::PROFILE, None, false)?.token;
match self
.client
.get_profile(self.state.config(), &profile_access_token, etag)?
Expand Down
11 changes: 9 additions & 2 deletions components/fxa-client/src/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,26 @@ impl FirefoxAccount {
/// - This must be one of the scopes requested during the signin flow.
/// - Only a single scope is supported; for multiple scopes request multiple tokens.
/// - `ttl` - optionally, the time for which the token should be valid, in seconds.
/// - `require_scoped_key` - optionally, throw [FxaError.ScopedKeyMissing] if the scoped_key
/// is not present in the server response.
///
/// # Notes
///
/// - If the application receives an authorization error when trying to use the resulting
/// token, it should call [`clear_access_token_cache`](FirefoxAccount::clear_access_token_cache)
/// before requesting a fresh token.
#[handle_error(Error)]
pub fn get_access_token(&self, scope: &str, ttl: Option<i64>) -> ApiResult<AccessTokenInfo> {
pub fn get_access_token(
&self,
scope: &str,
ttl: Option<i64>,
requre_scoped_key: bool,
) -> ApiResult<AccessTokenInfo> {
// Signedness converstion for Kotlin compatibility :-/
let ttl = ttl.map(|ttl| u64::try_from(ttl).unwrap_or_default());
self.internal
.lock()
.get_access_token(scope, ttl)?
.get_access_token(scope, ttl, requre_scoped_key)?
.try_into()
}

Expand Down
4 changes: 2 additions & 2 deletions examples/cli-support/src/fxa_creds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,15 @@ pub fn get_account_and_token(
// TODO: we should probably set a persist callback on acct?
let mut acct = load_or_create_fxa_creds(cred_file, config.clone())?;
// `scope` could be a param, but I can't see it changing.
match acct.get_access_token(SYNC_SCOPE, None) {
match acct.get_access_token(SYNC_SCOPE, None, false) {
Ok(t) => Ok((acct, t)),
Err(e) => {
match e {
// We can retry an auth error.
FxaError::Authentication => {
println!("Saw an auth error using stored credentials - recreating them...");
acct = create_fxa_creds(cred_file, config)?;
let token = acct.get_access_token(SYNC_SCOPE, None)?;
let token = acct.get_access_token(SYNC_SCOPE, None, false)?;
Ok((acct, token))
}
_ => Err(e.into()),
Expand Down

0 comments on commit eff00c4

Please sign in to comment.