From 2952f80fdb6831d59ab83c9bb4c63fe261874d56 Mon Sep 17 00:00:00 2001 From: Bill Fraser Date: Sun, 31 Dec 2023 22:01:49 -0800 Subject: [PATCH] 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. --- Cargo.toml | 2 +- RELEASE_NOTES.md | 9 +++- src/client_helpers.rs | 2 +- src/client_trait.rs | 17 +++++++ src/default_client.rs | 10 +++- src/oauth2.rs | 111 ++++++++++++++++++++++++++---------------- 6 files changed, 104 insertions(+), 47 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bd4c0a0..08b37ce 100644 --- 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 "] 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 index 350a718..036dfd1 100644 --- 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 index 7b5994c..adc6429 100644 --- 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 { +pub(crate) struct TopLevelError { 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 index da5f771..b9d4f29 100644 --- 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::>(json) else { + return false; + }; + + auth_err.error == AuthError::ExpiredAccessToken +} diff --git a/src/default_client.rs b/src/default_client.rs index f0f6d82..716be97 100644 --- 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, ¶ms, 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 index 32fb3e2..ce08883 100644 --- 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,7 +47,7 @@ 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 { @@ -52,7 +55,7 @@ impl Oauth2Type { 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, }, Refresh { - client_id: String, refresh_token: String, }, - AccessToken(String), + AccessToken { + client_secret: Option, + 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, ) -> 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 { 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,41 +352,49 @@ 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 { - 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 { - let client_id: String; let mut redirect_uri = None; let mut client_secret = None; let mut pkce_code = None; @@ -386,30 +402,36 @@ impl Authorization { 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,7 +446,7 @@ 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 { @@ -432,7 +454,7 @@ impl Authorization { } 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))