Add a new "Forbidden" FxA error. (#7320)

This is distinct from Authentication in that there's no implication the account
state isn't good, it's just that you aren't allowed to do what you tried.
An example of this is asking for an access token you don't have the scopes for.

This has the side effect of fixing a bug in `get_access_token()` - when that
code calls `exchange_token_for_scope()`, the server responds with a http 403
response - which ends up getting re-thrown as "other error".

It arranges for `NoCachedToken` errors to turn into this (that was previously
`Authentication`) and for `NoSessionToken` to be `Authentication`. It also tweaks
`get_access_token()` to explicitly throw `NoSessionToken` when there's no session
token. That way `get_access_token()` continues to return `Authentication` when
not logged in, but otherwise will generally return `Forbidden`.

Technically a breaking change, but both Android and iOS seem fine with new
variants here.

Author: mhammond

components/fxa-client/src/error.rs

@@ -18,6 +18,12 @@ pub enum FxaError {
     /// or retry the operation with a freshly-generated token.
     #[error("authentication error")]
     Authentication,
+    /// Thrown when an authenticated account isn't allowed to perform some operation. Unlike
+    /// `Authentication`, there's no problem with the account status. In some cases it
+    /// might be possible to request additional scopes, and once granted, the operation
+    /// may succeed.
+    #[error("forbidden")]
+    Forbidden,
     /// Thrown if an operation fails due to network access problems.
     /// The application may retry at a later time once connectivity is restored.
     #[error("network error")]
@@ -70,9 +76,6 @@ pub enum Error {
     #[error("Multiple OAuth scopes requested")]
     MultipleScopesRequested,
 
-    #[error("No cached token for scope {0}")]
-    NoCachedToken(String),
-
     #[error("No cached scoped keys for scope {0}")]
     NoScopedKey(String),
 
@@ -112,6 +115,15 @@ pub enum Error {
     #[error("Remote key and local key mismatch")]
     MismatchedKeys,
 
+    // For example, we requested an access token, the server responded with a 200, but the response body
+    // had no token. This is a little bit vague, because in many cases you would get a JSON error if the
+    // shape of the payload was entirely wrong.
+    #[error("The response from the server, or the content in that reponse, was unexpected")]
+    UnexpectedServerResponse,
+
+    // This should probably be rolled up into `UnexpectedServerResponse`, but the way it is implemented
+    // and even exposed to consumers makes that a little tricky - but at the end of the day, this can
+    // only happen when the server gave us a confused response.
     #[error("The sync scoped key was missing in the server response")]
     SyncScopedKeyMissingInServerResponse,
 
@@ -209,10 +221,13 @@ impl GetErrorHandling for Error {
         match self {
             Error::RemoteError { code: 401, .. }
             | Error::NoRefreshToken
-            | Error::NoScopedKey(_)
-            | Error::NoCachedToken(_) => {
+            | Error::NoSessionToken
+            | Error::NoScopedKey(_) => {
                 ErrorHandling::convert(FxaError::Authentication).log_warning()
             }
+            Error::RemoteError { code: 403, .. } => {
+                ErrorHandling::convert(FxaError::Forbidden).log_warning()
+            }
             Error::RequestError(_) => ErrorHandling::convert(FxaError::Network).log_warning(),
             Error::SyncScopedKeyMissingInServerResponse => {
                 ErrorHandling::convert(FxaError::SyncScopedKeyMissingInServerResponse)

components/fxa-client/src/fxa_client.udl

@@ -54,6 +54,12 @@ enum FxaError {
   /// or retry the operation with a freshly-generated token.
   "Authentication",
 
+  /// Thrown when an authenticated account isn't allowed to perform some operation. Unlike
+  /// `Authentication`, there's no problem with the account status. In some cases it
+  /// might be possible to request additional scopes, and once granted, the operation
+  /// may succeed.
+  "Forbidden",
+
   /// Thrown if an operation fails due to network access problems.
   /// The application may retry at a later time once connectivity is restored.
   "Network",

components/fxa-client/src/internal/mod.rs

@@ -325,8 +325,8 @@ mod tests {
         let mut fxa = FirefoxAccount::with_config(config);
         // No current user -> Error.
         match fxa.get_manage_account_url("test").unwrap_err() {
-            Error::NoCachedToken(_) => {}
-            _ => panic!("error not NoCachedToken"),
+            Error::NoSessionToken => {}
+            _ => panic!("error not NoSessionToken"),
         };
         // With current user -> expected Url.
         fxa.add_cached_profile("123", "test@example.com");
@@ -361,8 +361,8 @@ mod tests {
         let mut fxa = FirefoxAccount::with_config(config);
         // No current user -> Error.
         match fxa.get_manage_devices_url("test").unwrap_err() {
-            Error::NoCachedToken(_) => {}
-            _ => panic!("error not NoCachedToken"),
+            Error::NoSessionToken => {}
+            _ => panic!("error not NoSessionToken"),
         };
         // With current user -> expected Url.
         fxa.add_cached_profile("123", "test@example.com");

components/fxa-client/src/internal/oauth.rs

@@ -12,7 +12,7 @@ use super::{
     util, FirefoxAccount,
 };
 use crate::auth::UserData;
-use crate::{warn, AuthorizationParameters, Error, FxaServer, Result, ScopedKey};
+use crate::{error, warn, AuthorizationParameters, Error, FxaServer, Result, ScopedKey};
 use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine};
 use jwcrypto::{EncryptionAlgorithm, EncryptionParameters};
 use rate_limiter::RateLimiter;
@@ -71,10 +71,16 @@ impl FirefoxAccount {
                             new_refresh_token,
                             exchange_resp.scope,
                         ));
+                    } else {
+                        // A request for a new token succeeding but without a new token is unexpected.
+                        error!("successful response for a new refresh token with additional scopes, but no token was delivered");
+                        // at this stage we are almost certainly still going to fail to get a token...
                     }
                     // Get the updated refresh token from state.
                     refresh_token = match self.state.refresh_token() {
-                        None => return Err(Error::NoCachedToken(scope.to_string())),
+                        // We had a refresh token, we must either still have the original or maybe a new one,
+                        // but it's impossible for us to not have one at this point.
+                        None => unreachable!("lost the refresh token"),
                         Some(token) => token,
                     };
                 }
@@ -86,7 +92,11 @@ impl FirefoxAccount {
                         &[scope],
                     )?
                 } else {
-                    return Err(Error::NoCachedToken(scope.to_string()));
+                    // This should be impossible - if we don't have the scope we would have entered
+                    // the block where we try and get it, that succeeded and we got a new refresh token,
+                    // but still don't have the scope.
+                    error!("New refresh token doesn't have the scope we requested: {scope}");
+                    return Err(Error::UnexpectedServerResponse);
                 }
             }
             None => match self.state.session_token() {
@@ -95,7 +105,7 @@ impl FirefoxAccount {
                     session_token,
                     &[scope],
                 )?,
-                None => return Err(Error::NoCachedToken(scope.to_string())),
+                None => return Err(Error::NoSessionToken),
             },
         };
         let since_epoch = SystemTime::now()