diff --git a/src/actions.rs b/src/actions.rs index 79d304d4..c08f2fc8 100644 --- a/src/actions.rs +++ b/src/actions.rs @@ -137,7 +137,7 @@ pub async fn sync( access_token: &str, refresh_token: &str, ) -> Result<( - Option, + Option, ( String, String, @@ -175,7 +175,7 @@ pub fn add( data: &crate::db::EntryData, notes: Option<&str>, folder_id: Option<&str>, -) -> Result<(Option, ())> { +) -> Result<(Option, ())> { with_exchange_refresh_token(access_token, refresh_token, |access_token| { add_once(access_token, name, data, notes, folder_id) }) @@ -204,7 +204,7 @@ pub fn edit( notes: Option<&str>, folder_uuid: Option<&str>, history: &[crate::db::HistoryEntry], -) -> Result<(Option, ())> { +) -> Result<(Option, ())> { with_exchange_refresh_token(access_token, refresh_token, |access_token| { edit_once( access_token, @@ -250,7 +250,7 @@ pub fn remove( access_token: &str, refresh_token: &str, id: &str, -) -> Result<(Option, ())> { +) -> Result<(Option, ())> { with_exchange_refresh_token(access_token, refresh_token, |access_token| { remove_once(access_token, id) }) @@ -265,7 +265,7 @@ fn remove_once(access_token: &str, id: &str) -> Result<()> { pub fn list_folders( access_token: &str, refresh_token: &str, -) -> Result<(Option, Vec<(String, String)>)> { +) -> Result<(Option, Vec<(String, String)>)> { with_exchange_refresh_token(access_token, refresh_token, |access_token| { list_folders_once(access_token) }) @@ -280,7 +280,7 @@ pub fn create_folder( access_token: &str, refresh_token: &str, name: &str, -) -> Result<(Option, String)> { +) -> Result<(Option, String)> { with_exchange_refresh_token(access_token, refresh_token, |access_token| { create_folder_once(access_token, name) }) @@ -291,20 +291,30 @@ fn create_folder_once(access_token: &str, name: &str) -> Result { client.create_folder(access_token, name) } +/// Tokens returned by a successful refresh. +/// +/// `refresh_token` is `None` against servers that do not rotate it, in which +/// case the stored one stays valid and should be left alone. +#[derive(Debug, Clone)] +pub struct RefreshedTokens { + pub access_token: String, + pub refresh_token: Option, +} + fn with_exchange_refresh_token( access_token: &str, refresh_token: &str, f: F, -) -> Result<(Option, T)> +) -> Result<(Option, T)> where F: Fn(&str) -> Result, { match f(access_token) { Ok(t) => Ok((None, t)), Err(Error::RequestUnauthorized) => { - let access_token = exchange_refresh_token(refresh_token)?; - let t = f(&access_token)?; - Ok((Some(access_token), t)) + let tokens = exchange_refresh_token(refresh_token)?; + let t = f(&tokens.access_token)?; + Ok((Some(tokens), t)) } Err(e) => Err(e), } @@ -314,7 +324,7 @@ async fn with_exchange_refresh_token_async( access_token: &str, refresh_token: &str, f: F, -) -> Result<(Option, T)> +) -> Result<(Option, T)> where F: Fn( &str, @@ -327,23 +337,34 @@ where match f(access_token).await { Ok(t) => Ok((None, t)), Err(Error::RequestUnauthorized) => { - let access_token = - exchange_refresh_token_async(refresh_token).await?; - let t = f(&access_token).await?; - Ok((Some(access_token), t)) + let tokens = exchange_refresh_token_async(refresh_token).await?; + let t = f(&tokens.access_token).await?; + Ok((Some(tokens), t)) } Err(e) => Err(e), } } -fn exchange_refresh_token(refresh_token: &str) -> Result { +fn exchange_refresh_token(refresh_token: &str) -> Result { let (client, _) = api_client()?; - client.exchange_refresh_token(refresh_token) + let (access_token, refresh_token) = + client.exchange_refresh_token(refresh_token)?; + Ok(RefreshedTokens { + access_token, + refresh_token, + }) } -async fn exchange_refresh_token_async(refresh_token: &str) -> Result { +async fn exchange_refresh_token_async( + refresh_token: &str, +) -> Result { let (client, _) = api_client()?; - client.exchange_refresh_token_async(refresh_token).await + let (access_token, refresh_token) = + client.exchange_refresh_token_async(refresh_token).await?; + Ok(RefreshedTokens { + access_token, + refresh_token, + }) } fn api_client() -> Result<(crate::api::Client, crate::config::Config)> { diff --git a/src/api.rs b/src/api.rs index a817fb26..dd10d576 100644 --- a/src/api.rs +++ b/src/api.rs @@ -372,6 +372,12 @@ struct ConnectRefreshTokenReq { #[derive(serde::Deserialize, Debug)] struct ConnectRefreshTokenRes { access_token: String, + // Bitwarden's own identity server returns long-lived refresh tokens and + // does not rotate them, so this is absent there. A Vaultwarden backed by an + // external identity provider relays a rotating token, and the previous one + // is consumed by the exchange -- reusing it fails every subsequent refresh. + #[serde(default)] + refresh_token: Option, } #[derive(serde::Serialize, Debug)] @@ -1545,7 +1551,7 @@ impl Client { pub fn exchange_refresh_token( &self, refresh_token: &str, - ) -> Result { + ) -> Result<(String, Option)> { let connect_req = ConnectRefreshTokenReq { grant_type: "refresh_token".to_string(), client_id: "cli".to_string(), @@ -1557,14 +1563,20 @@ impl Client { .form(&connect_req) .send() .map_err(|source| Error::Reqwest { source })?; + if !res.status().is_success() { + return Err(classify_refresh_error_res( + res.status().as_u16(), + res.text(), + )); + } let connect_res: ConnectRefreshTokenRes = res.json_with_path()?; - Ok(connect_res.access_token) + Ok((connect_res.access_token, connect_res.refresh_token)) } pub async fn exchange_refresh_token_async( &self, refresh_token: &str, - ) -> Result { + ) -> Result<(String, Option)> { let connect_req = ConnectRefreshTokenReq { grant_type: "refresh_token".to_string(), client_id: "cli".to_string(), @@ -1577,9 +1589,13 @@ impl Client { .send() .await .map_err(|source| Error::Reqwest { source })?; + if !res.status().is_success() { + let code = res.status().as_u16(); + return Err(classify_refresh_error_res(code, res.text().await)); + } let connect_res: ConnectRefreshTokenRes = res.json_with_path().await?; - Ok(connect_res.access_token) + Ok((connect_res.access_token, connect_res.refresh_token)) } fn api_url(&self, path: &str) -> String { @@ -1707,6 +1723,34 @@ fn sso_query_code( Ok(sso_code.clone()) } +/// Classify a failed response from the `refresh_token` grant. +/// +/// `invalid_grant` is unambiguous here in a way it is not during login: no +/// password or second factor is involved, so the only thing the server can be +/// rejecting is the refresh token itself. It will never become valid again, so +/// callers should stop retrying and require a fresh login. +fn classify_refresh_error_res( + code: u16, + body: std::result::Result, +) -> Error { + let Ok(body) = body else { + return Error::RequestFailed { status: code }; + }; + match body.clone().json_with_path::() { + Ok(json) => { + if json.error == "invalid_grant" { + Error::RefreshTokenInvalid + } else { + classify_login_error(&json, code) + } + } + Err(e) => { + log::warn!("{e}: {body}"); + Error::RequestFailed { status: code } + } + } +} + fn classify_login_error(error_res: &ConnectErrorRes, code: u16) -> Error { let error_desc = error_res.error_description.clone(); let error_desc = error_desc.as_deref(); diff --git a/src/bin/rbw-agent/actions.rs b/src/bin/rbw-agent/actions.rs index 9ddd2ad9..aef70080 100644 --- a/src/bin/rbw-agent/actions.rs +++ b/src/bin/rbw-agent/actions.rs @@ -522,14 +522,42 @@ pub async fn sync( return Err(anyhow::anyhow!("failed to find refresh token in db")); }; let ( - access_token, + tokens, (protected_key, protected_private_key, protected_org_keys, entries), - ) = rbw::actions::sync(&access_token, &refresh_token) - .await - .context("failed to sync database from server")?; + ) = match rbw::actions::sync(&access_token, &refresh_token).await { + Ok(res) => res, + // The server will never accept this refresh token again, so retrying + // on the sync interval only repeats the same rejection forever. Drop + // the tokens instead: that makes db.needs_login() true, so the next + // command the user runs goes through the ordinary login flow, and the + // syncs in between fail locally without touching the network. + Err(rbw::error::Error::RefreshTokenInvalid) => { + // Re-read rather than trusting the copy loaded above: a concurrent + // sync may have rotated the token while this request was in + // flight, in which case the server is rejecting a token that has + // already been superseded and the stored replacement is valid. + // Only clear if what we sent is still what is stored. + let mut current = load_db().await?; + if current.refresh_token.as_deref() + == Some(refresh_token.as_str()) + { + current.access_token = None; + current.refresh_token = None; + save_db(¤t).await?; + } + return Err(rbw::error::Error::RefreshTokenInvalid) + .context("failed to sync database from server"); + } + Err(e) => { + return Err(e).context("failed to sync database from server") + } + }; state.lock().await.set_master_password_reprompt(&entries); - if let Some(access_token) = access_token { - db.access_token = Some(access_token); + if let Some(tokens) = tokens { + db.access_token = Some(tokens.access_token); + if let Some(refresh_token) = tokens.refresh_token { + db.refresh_token = Some(refresh_token); + } } db.protected_key = Some(protected_key); db.protected_private_key = Some(protected_private_key); diff --git a/src/bin/rbw/commands.rs b/src/bin/rbw/commands.rs index bddf0efe..4e7a476b 100644 --- a/src/bin/rbw/commands.rs +++ b/src/bin/rbw/commands.rs @@ -1567,7 +1567,7 @@ pub fn add( // unwrap is safe here because the call to unlock above is guaranteed to // populate these or error let mut access_token = db.access_token.as_ref().unwrap().clone(); - let refresh_token = db.refresh_token.as_ref().unwrap(); + let mut refresh_token = db.refresh_token.as_ref().unwrap().clone(); let name = crate::actions::encrypt(name, None)?; @@ -1596,11 +1596,15 @@ pub fn add( let mut folder_id = None; if let Some(folder_name) = folder { - let (new_access_token, folders) = - rbw::actions::list_folders(&access_token, refresh_token)?; - if let Some(new_access_token) = new_access_token { - access_token.clone_from(&new_access_token); - db.access_token = Some(new_access_token); + let (tokens, folders) = + rbw::actions::list_folders(&access_token, &refresh_token)?; + if let Some(tokens) = tokens { + access_token = tokens.access_token; + db.access_token = Some(access_token.clone()); + if let Some(new_refresh_token) = tokens.refresh_token { + refresh_token = new_refresh_token; + db.refresh_token = Some(refresh_token.clone()); + } save_db(&db)?; } @@ -1618,23 +1622,27 @@ pub fn add( } } if folder_id.is_none() { - let (new_access_token, id) = rbw::actions::create_folder( + let (tokens, id) = rbw::actions::create_folder( &access_token, - refresh_token, + &refresh_token, &crate::actions::encrypt(folder_name, None)?, )?; - if let Some(new_access_token) = new_access_token { - access_token.clone_from(&new_access_token); - db.access_token = Some(new_access_token); + if let Some(tokens) = tokens { + access_token = tokens.access_token; + db.access_token = Some(access_token.clone()); + if let Some(new_refresh_token) = tokens.refresh_token { + refresh_token = new_refresh_token; + db.refresh_token = Some(refresh_token.clone()); + } save_db(&db)?; } folder_id = Some(id); } } - if let (Some(access_token), ()) = rbw::actions::add( + if let (Some(tokens), ()) = rbw::actions::add( &access_token, - refresh_token, + &refresh_token, &name, &rbw::db::EntryData::Login { username, @@ -1645,7 +1653,10 @@ pub fn add( notes.as_deref(), folder_id.as_deref(), )? { - db.access_token = Some(access_token); + db.access_token = Some(tokens.access_token); + if let Some(refresh_token) = tokens.refresh_token { + db.refresh_token = Some(refresh_token); + } save_db(&db)?; } @@ -1672,7 +1683,7 @@ pub fn generate( // unwrap is safe here because the call to unlock above is guaranteed // to populate these or error let mut access_token = db.access_token.as_ref().unwrap().clone(); - let refresh_token = db.refresh_token.as_ref().unwrap(); + let mut refresh_token = db.refresh_token.as_ref().unwrap().clone(); let name = crate::actions::encrypt(name, None)?; let username = username @@ -1691,11 +1702,15 @@ pub fn generate( let mut folder_id = None; if let Some(folder_name) = folder { - let (new_access_token, folders) = - rbw::actions::list_folders(&access_token, refresh_token)?; - if let Some(new_access_token) = new_access_token { - access_token.clone_from(&new_access_token); - db.access_token = Some(new_access_token); + let (tokens, folders) = + rbw::actions::list_folders(&access_token, &refresh_token)?; + if let Some(tokens) = tokens { + access_token = tokens.access_token; + db.access_token = Some(access_token.clone()); + if let Some(new_refresh_token) = tokens.refresh_token { + refresh_token = new_refresh_token; + db.refresh_token = Some(refresh_token.clone()); + } save_db(&db)?; } @@ -1713,23 +1728,27 @@ pub fn generate( } } if folder_id.is_none() { - let (new_access_token, id) = rbw::actions::create_folder( + let (tokens, id) = rbw::actions::create_folder( &access_token, - refresh_token, + &refresh_token, &crate::actions::encrypt(folder_name, None)?, )?; - if let Some(new_access_token) = new_access_token { - access_token.clone_from(&new_access_token); - db.access_token = Some(new_access_token); + if let Some(tokens) = tokens { + access_token = tokens.access_token; + db.access_token = Some(access_token.clone()); + if let Some(new_refresh_token) = tokens.refresh_token { + refresh_token = new_refresh_token; + db.refresh_token = Some(refresh_token.clone()); + } save_db(&db)?; } folder_id = Some(id); } } - if let (Some(access_token), ()) = rbw::actions::add( + if let (Some(tokens), ()) = rbw::actions::add( &access_token, - refresh_token, + &refresh_token, &name, &rbw::db::EntryData::Login { username, @@ -1740,7 +1759,10 @@ pub fn generate( None, folder_id.as_deref(), )? { - db.access_token = Some(access_token); + db.access_token = Some(tokens.access_token); + if let Some(refresh_token) = tokens.refresh_token { + db.refresh_token = Some(refresh_token); + } save_db(&db)?; } @@ -1855,7 +1877,7 @@ pub fn edit( } }; - if let (Some(access_token), ()) = rbw::actions::edit( + if let (Some(tokens), ()) = rbw::actions::edit( access_token, refresh_token, &entry.id, @@ -1867,7 +1889,10 @@ pub fn edit( entry.folder_id.as_deref(), &history, )? { - db.access_token = Some(access_token); + db.access_token = Some(tokens.access_token); + if let Some(refresh_token) = tokens.refresh_token { + db.refresh_token = Some(refresh_token); + } save_db(&db)?; } @@ -1896,10 +1921,13 @@ pub fn remove( let (entry, _) = find_entry(&db, name, username, folder, ignore_case) .with_context(|| format!("couldn't find entry for '{desc}'"))?; - if let (Some(access_token), ()) = + if let (Some(tokens), ()) = rbw::actions::remove(access_token, refresh_token, &entry.id)? { - db.access_token = Some(access_token); + db.access_token = Some(tokens.access_token); + if let Some(refresh_token) = tokens.refresh_token { + db.refresh_token = Some(refresh_token); + } save_db(&db)?; } diff --git a/src/error.rs b/src/error.rs index b7789a92..49ec1b37 100644 --- a/src/error.rs +++ b/src/error.rs @@ -172,6 +172,9 @@ pub enum Error { #[error("This device has not yet been registered with the Bitwarden server. Run `rbw register` first, and then try again.")] RegistrationRequired, + #[error("refresh token is no longer valid, run `rbw login`")] + RefreshTokenInvalid, + #[error("failed to remove db at {}", .file.display())] RemoveDb { source: std::io::Error,