rework the oauth2 code a bit and fix some bugs in it

1. Fix the check for expired token in default client. a. addded a helper for this: client_trait::is_token_expired_error() 2. (BREAKING) Change oauth2::Oauth2Type variants: a. Oauth2Type::AuthorizationCode is now a struct variant with a named field for the secret b. Oauth2Type::ImplicitGrant now does not take the client secret any more, since it was not ever actually needed or used 3. (BREAKING) Rename Authorization::from_access_token() to Authorization::from_long_lived_access_token() to reflect its more niche purpose, and has been marked deprecated. 4. Authorization::obtain_access_token() when currently holding a refresh token will retain the refresh token. There was previously a bug where it would switch to only being a short-lived token with no way to refresh. Thanks to @dennishall3 and Julian Aichholz (@rusty-jules) for their contributions to reporting and investigating these issues.
dropbox · Jan 12, 2024 · 2952f80 · 2952f80
1 parent f250ff1
commit 2952f80
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 47 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "dropbox-sdk"
-version = "0.17.0"
+version = "0.18.0"
 authors = ["Bill Fraser <wfraser@dropbox.com>"]
 edition = "2018"
 description = "Rust bindings to the Dropbox API, generated by Stone from the official spec."

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -1,6 +1,13 @@
-# v0.17.1
+# v0.18.0
 xxxx-yy-zz
 * MSRV raised to 1.65.0
+* oauth2 changes and bugfixes:
+    * Fix the check for expired token in the default client.
+    * Added a helper for the above: client_trait::is_token_expired_error()
+    * Authorize::obtain_access_token() when currently holding a refresh token will return an updated bearer token and retain the refresh token. There was previously a bug where it would switch to only being a short-lived token with no way to refresh.
+    * (breaking) Oauth2Type::AuthorizationCode is now a struct variant with a named field instead of a tuple variant.
+    * (breaking) Oauth2Type::ImplicitGrant now is a nullary variant
+    * (breaking) Renamed Authorization::from_access_token() to Authorization::from_long_lived_access_token() and marked it as deprecated
 
 # v0.17.0
 2023-11-10

diff --git a/src/client_helpers.rs b/src/client_helpers.rs
@@ -11,7 +11,7 @@ use serde::ser::Serialize;
 /// When Dropbox returns an error with HTTP 409 or 429, it uses an implicit JSON object with the
 /// following structure, which contains the actual error as a field.
 #[derive(Debug, Deserialize)]
-struct TopLevelError<T> {
+pub(crate) struct TopLevelError<T> {
     pub error: T,
 
     // It also has these fields, which we don't expose anywhere:

diff --git a/src/client_trait.rs b/src/client_trait.rs
@@ -4,6 +4,9 @@
 
 use std::io::Read;
 
+use crate::auth::AuthError;
+use crate::client_helpers::TopLevelError;
+
 /// The base HTTP client trait.
 pub trait HttpClient {
     /// Make a HTTP request.
@@ -152,3 +155,17 @@ impl TeamSelect {
         }
     }
 }
+
+/// Check whether a given [error](crate::Error) is due to using an expired access token. Clients
+/// receiving such a response should update their access token and retry.
+pub fn is_token_expired_error(e: &crate::Error) -> bool {
+    let crate::Error::UnexpectedHttpError { code: 401, status: _, json } = &e else {
+        return false;
+    };
+
+    let Ok(auth_err) = serde_json::from_str::<TopLevelError<AuthError>>(json) else {
+        return false;
+    };
+
+    auth_err.error == AuthError::ExpiredAccessToken
+}
diff --git a/src/default_client.rs b/src/default_client.rs
@@ -12,7 +12,6 @@
 //! This code (and its dependencies) are only built if you use the `default_client` Cargo feature.
 
 use crate::Error;
-use crate::auth::AuthError;
 use crate::client_trait::*;
 use crate::oauth2::{Authorization, TokenCache};
 use std::borrow::Cow;
@@ -60,11 +59,18 @@ macro_rules! forward_authed_request {
                 let result = $inner.request(endpoint, style, function, &params, params_type, body,
                     range_start, range_end, Some(&token), $path_root, $team_select);
 
+                // if-let chains will make this much less verbose...
+                // if !retried && let Err(e) = &result && is_token_expired_error(e) { ... }
+
                 if retried {
                     break result;
                 }
 
-                if let Err(crate::Error::Authentication(AuthError::ExpiredAccessToken)) = &result {
+                let Err(e) = &result else {
+                    break result;
+                };
+
+                if is_token_expired_error(e) {
                     info!("refreshing auth token");
                     let old_token = token;
                     token = $tokens.update_token(

diff --git a/src/oauth2.rs b/src/oauth2.rs
@@ -31,7 +31,10 @@ pub enum Oauth2Type {
     /// your app with the code (if it is a server-side app), or can be used without a redirect URI,
     /// in which case the authorization page displays the authorization code to the user and they
     /// must then input the code manually into the program.
-    AuthorizationCode(String /* client secret */),
+    AuthorizationCode {
+        /// Client secret
+        client_secret: String,
+    },
 
     /// The PKCE flow is an extension of the Authorization Code flow which uses dynamically
     /// generated codes instead of an app secret to perform the OAuth exchange. This both avoids
@@ -44,15 +47,15 @@ pub enum Oauth2Type {
     /// token is needed. This can ONLY be used with a redirect URI.
     ///
     /// This flow is considered "legacy" and is not as secure as the other flows.
-    ImplicitGrant(String /* client secret */),
+    ImplicitGrant,
 }
 
 impl Oauth2Type {
     /// The value to put in the "response_type" parameter to request the given token type.
     pub(crate) fn response_type_str(&self) -> &'static str {
         match self {
             Oauth2Type::AuthorizationCode { .. } | Oauth2Type::PKCE { .. } => "code",
-            Oauth2Type::ImplicitGrant { .. } => "token",
+            Oauth2Type::ImplicitGrant => "token",
         }
     }
 }
@@ -284,21 +287,23 @@ impl<'a> AuthorizeUrlBuilder<'a> {
 #[derive(Debug, Clone)]
 enum AuthorizationState {
     InitialAuth {
-        client_id: String,
         flow_type: Oauth2Type,
         auth_code: String,
         redirect_uri: Option<String>,
     },
     Refresh {
-        client_id: String,
         refresh_token: String,
     },
-    AccessToken(String),
+    AccessToken {
+        client_secret: Option<String>,
+        token: String,
+    },
 }
 
 /// Provides for continuing authorization of the app.
 #[derive(Debug, Clone)]
 pub struct Authorization {
+    client_id: String,
     state: AuthorizationState,
 }
 
@@ -316,8 +321,8 @@ impl Authorization {
         redirect_uri: Option<String>,
     ) -> Self {
         Self {
-            state: AuthorizationState::InitialAuth {
-                client_id, flow_type, auth_code, redirect_uri },
+            client_id,
+            state: AuthorizationState::InitialAuth { flow_type, auth_code, redirect_uri },
         }
     }
 
@@ -327,11 +332,14 @@ impl Authorization {
     /// token yet).
     pub fn save(&self) -> Option<String> {
         match &self.state {
-            AuthorizationState::InitialAuth { .. } => None,
-            AuthorizationState::AccessToken(access_token) =>
-                Some(format!("1&{}", access_token)),
-            AuthorizationState::Refresh { client_id: _, refresh_token } =>
-                Some(format!("2&{}", refresh_token)),
+            AuthorizationState::AccessToken { token, client_secret } if client_secret.is_none() => {
+                // Legacy long-lived access token.
+                Some(format!("1&{}", token))
+            },
+            AuthorizationState::Refresh { refresh_token, .. } => {
+                Some(format!("2&{}", refresh_token))
+            },
+            _ => None,
         }
     }
 
@@ -344,72 +352,86 @@ impl Authorization {
     /// [`Authentication`](crate::Error::Authentication) errors. In such a case you should also
     /// start the authorization procedure from scratch.
     pub fn load(client_id: String, saved: &str) -> Option<Self> {
-        let state = match saved.get(0..2) {
-            Some("1&") => AuthorizationState::AccessToken(saved[2..].to_owned()),
-            Some("2&") => AuthorizationState::Refresh {
-                client_id,
-                refresh_token: saved[2..].to_owned(),
+        Some(match saved.get(0..2) {
+            Some("1&") => {
+                #[allow(deprecated)]
+                Self::from_long_lived_access_token(saved[2..].to_owned())
             },
+            Some("2&") => Self::from_refresh_token(client_id, saved[2..].to_owned()),
             _ => {
                 error!("unrecognized saved Authorization representation: {:?}", saved);
                 return None;
             }
-        };
-        Some(Self { state })
+        })
     }
 
-    /// Recreate the authorization from an authorization code and refresh token.
+    /// Recreate the authorization from a refresh token.
     pub fn from_refresh_token(
         client_id: String,
         refresh_token: String,
     ) -> Self {
-        Self { state: AuthorizationState::Refresh { client_id, refresh_token } }
+        Self {
+            client_id,
+            state: AuthorizationState::Refresh { refresh_token },
+        }
     }
 
     /// Recreate the authorization from a long-lived access token. This token cannot be refreshed;
     /// any call to [`obtain_access_token`](Authorization::obtain_access_token) will simply return
-    /// the given token.
-    pub fn from_access_token(
+    /// the given token. Therefore this requires neither client ID or client secret.
+    ///
+    /// Long-lived tokens are deprecated and the ability to generate them will be removed in the
+    /// future.
+    #[deprecated]
+    pub fn from_long_lived_access_token(
         access_token: String,
     ) -> Self {
-        Self { state: AuthorizationState::AccessToken(access_token) }
+        Self {
+            client_id: String::new(),
+            state: AuthorizationState::AccessToken { token: access_token, client_secret: None },
+        }
     }
 
     /// Obtain an access token. Use this to complete the authorization process, or to obtain an
     /// updated token when a short-lived access token has expired.
     pub fn obtain_access_token(&mut self, client: impl NoauthClient) -> crate::Result<String> {
-        let client_id: String;
         let mut redirect_uri = None;
         let mut client_secret = None;
         let mut pkce_code = None;
         let mut refresh_token = None;
         let mut auth_code = None;
 
         match self.state.clone() {
-            AuthorizationState::AccessToken(token) => {
-                return Ok(token);
+            AuthorizationState::AccessToken { token, client_secret: secret } => {
+                match secret {
+                    None => {
+                        // Long-lived token which cannot be refreshed
+                        return Ok(token)
+                    },
+                    Some(secret) => {
+                        client_secret = Some(secret);
+                    }
+                }
             }
             AuthorizationState::InitialAuth {
-                client_id: id, flow_type, auth_code: code, redirect_uri: uri } =>
+                flow_type, auth_code: code, redirect_uri: uri } =>
             {
                 match flow_type {
-                    Oauth2Type::ImplicitGrant(_secret) => {
-                        self.state = AuthorizationState::AccessToken(code.clone());
+                    Oauth2Type::ImplicitGrant => {
+                        self.state = AuthorizationState::AccessToken { client_secret: None, token: code.clone() };
                         return Ok(code);
                     }
-                    Oauth2Type::AuthorizationCode(secret) => {
+                    Oauth2Type::AuthorizationCode { client_secret: secret } => {
                         client_secret = Some(secret);
                     }
                     Oauth2Type::PKCE(pkce) => {
-                        pkce_code = Some(pkce.code);
+                        pkce_code = Some(pkce.code.clone());
                     }
                 }
-                client_id = id;
                 auth_code = Some(code);
                 redirect_uri = uri;
             }
-            AuthorizationState::Refresh { client_id: id, refresh_token: refresh } => {
-                client_id = id;
+            AuthorizationState::Refresh { refresh_token: refresh } => {
                 refresh_token = Some(refresh);
             }
         }
@@ -424,15 +446,15 @@ impl Authorization {
             params.append_pair("code", &auth_code.unwrap());
         }
 
-        params.append_pair("client_id", &client_id);
+        params.append_pair("client_id", &self.client_id);
 
         if refresh_token.is_none() {
             if let Some(pkce) = pkce_code {
                 params.append_pair("code_verifier", &pkce);
             } else {
                 params.append_pair(
                     "client_secret",
-                    &client_secret.expect("need either PKCE code or client secret"));
+                    client_secret.as_ref().expect("need either PKCE code or client secret"));
             }
         }
 
@@ -477,11 +499,15 @@ impl Authorization {
 
         match refresh_token {
             Some(refresh) => {
-                self.state = AuthorizationState::Refresh { client_id, refresh_token: refresh };
+                self.state = AuthorizationState::Refresh { refresh_token: refresh };
             }
-            None => {
-                self.state = AuthorizationState::AccessToken(access_token.clone());
+            None if !matches!(self.state, AuthorizationState::Refresh {..}) => {
+                self.state = AuthorizationState::AccessToken {
+                    token: access_token.clone(),
+                    client_secret,
+                };
             }
+            _ => (),
         }
 
         Ok(access_token)
@@ -550,7 +576,8 @@ impl TokenCache {
 pub fn get_auth_from_env_or_prompt() -> Authorization {
     if let Ok(long_lived) = env::var("DBX_OAUTH_TOKEN") {
         // Used to provide a legacy long-lived token.
-        return Authorization::from_access_token(long_lived);
+        #[allow(deprecated)]
+        return Authorization::from_long_lived_access_token(long_lived);
     }
 
     if let (Ok(client_id), Ok(saved))