From eaf763dcef1433aa91c4f582338bc7db295ec8da Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Wed, 24 Jan 2024 13:55:13 +0100 Subject: [PATCH 1/2] Prefer `--url` argument over empty auth token URL --- src/config.rs | 43 ++++++++++--------- src/utils/auth_token/org_auth_token.rs | 15 ++++++- src/utils/auth_token/test.rs | 4 +- .../url-mismatch-empty-token.trycmd | 8 ++++ tests/integration/org_tokens.rs | 5 +++ 5 files changed, 51 insertions(+), 24 deletions(-) create mode 100644 tests/integration/_cases/org_tokens/url-mismatch-empty-token.trycmd diff --git a/src/config.rs b/src/config.rs index 893a4353a9..14400e45e6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -60,18 +60,20 @@ impl Config { _ => None, // get_default_auth never returns Auth::Token variant }; - let mut url = get_default_url(&ini); - - if let Some(token_url) = token_embedded_data.as_ref().and_then(|td| td.url.as_ref()) { - if url == DEFAULT_URL || url.is_empty() { - url = token_url.clone(); - } else if url != *token_url { - bail!( - "Two different url values supplied: `{}` (from token), `{url}`.", - token_url, - ); - } - } + let default_url = get_default_url(&ini); + let token_url = token_embedded_data + .as_ref() + .and_then(|td| Some(td.url.as_str())) + .unwrap_or_default(); + + let url = match (default_url.as_str(), token_url) { + (_, "") => default_url, + _ if default_url == token_url => default_url, + (DEFAULT_URL | "", _) => String::from(token_url), + _ => bail!( + "Two different url values supplied: `{token_url}` (from token), `{default_url}`." + ), + }; Ok(Config { filename, @@ -174,9 +176,9 @@ impl Config { if let Some(token_url) = self .cached_token_data .as_ref() - .and_then(|td| td.url.as_ref()) + .and_then(|td| Some(td.url.as_str())) { - self.cached_base_url = token_url.clone(); + self.cached_base_url = token_url.to_string(); } self.ini @@ -206,15 +208,16 @@ impl Config { /// Sets the URL pub fn set_base_url(&mut self, url: &str) -> Result<()> { - if let Some(token_url) = self + let token_url = self .cached_token_data .as_ref() - .and_then(|td| td.url.as_ref()) - { - if url != token_url { - bail!("Two different url values supplied: `{token_url}` (from token), `{url}`."); - } + .and_then(|td| Some(td.url.as_str())) + .unwrap_or_default(); + + if !token_url.is_empty() && url != token_url { + bail!("Two different url values supplied: `{token_url}` (from token), `{url}`."); } + self.cached_base_url = url.to_owned(); self.ini .set_to(Some("defaults"), "url".into(), self.cached_base_url.clone()); diff --git a/src/utils/auth_token/org_auth_token.rs b/src/utils/auth_token/org_auth_token.rs index 6623676243..b6ff0e7455 100644 --- a/src/utils/auth_token/org_auth_token.rs +++ b/src/utils/auth_token/org_auth_token.rs @@ -1,5 +1,5 @@ use super::{AuthTokenParseError, Result}; -use serde::Deserialize; +use serde::{Deserialize, Deserializer}; const ORG_AUTH_TOKEN_PREFIX: &str = "sntrys_"; const ORG_TOKEN_SECRET_BYTES: usize = 32; @@ -16,9 +16,20 @@ pub struct OrgAuthToken { #[allow(dead_code)] // Otherwise, we get a warning about unused fields pub struct AuthTokenPayload { iat: f64, - pub url: Option, // URL may be missing from some old auth tokens, see getsentry/sentry#57123 region_url: String, pub org: String, + + // URL may be missing from some old auth tokens, see getsentry/sentry#57123 + #[serde(deserialize_with = "url_deserializer")] + pub url: String, +} + +/// Deserializes a URL from a string, returning an empty string if the URL is missing or null. +fn url_deserializer<'de, D>(deserializer: D) -> std::result::Result +where + D: Deserializer<'de>, +{ + Option::deserialize(deserializer).map(|url| url.unwrap_or_default()) } impl OrgAuthToken { diff --git a/src/utils/auth_token/test.rs b/src/utils/auth_token/test.rs index cf748c881f..48c9c280bd 100644 --- a/src/utils/auth_token/test.rs +++ b/src/utils/auth_token/test.rs @@ -33,7 +33,7 @@ fn test_valid_org_auth_token() { let payload = token.payload().unwrap(); assert_eq!(payload.org, "sentry"); - assert_eq!(payload.url, Some(String::from("http://localhost:8000"))); + assert_eq!(payload.url, "http://localhost:8000"); assert_eq!(good_token, token.to_string()); @@ -56,7 +56,7 @@ fn test_valid_org_auth_token_missing_url() { let payload = token.payload().unwrap(); assert_eq!(payload.org, "sentry"); - assert!(payload.url.is_none()); + assert!(payload.url.is_empty()); assert_eq!(good_token, token.to_string()); diff --git a/tests/integration/_cases/org_tokens/url-mismatch-empty-token.trycmd b/tests/integration/_cases/org_tokens/url-mismatch-empty-token.trycmd new file mode 100644 index 0000000000..db83846b6c --- /dev/null +++ b/tests/integration/_cases/org_tokens/url-mismatch-empty-token.trycmd @@ -0,0 +1,8 @@ +This auth token has an empty URL. We expect the --url argument to take precedence here +``` +$ sentry-cli --auth-token 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 --url https://sentry.example.com info +? failed +Sentry Server: https://sentry.example.com +... + +``` diff --git a/tests/integration/org_tokens.rs b/tests/integration/org_tokens.rs index 1c05451004..41b70f4d1b 100644 --- a/tests/integration/org_tokens.rs +++ b/tests/integration/org_tokens.rs @@ -24,3 +24,8 @@ fn org_token_org_match() { fn org_token_url_works() { register_test("org_tokens/url-works.trycmd"); } + +#[test] +fn org_token_url_mismatch_empty_token() { + register_test("org_tokens/url-mismatch-empty-token.trycmd"); +} From a63b2c8b0c18c10f01c591d31ecf2d218b793a66 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Wed, 24 Jan 2024 14:19:22 +0100 Subject: [PATCH 2/2] Change `and_then` to `map` --- src/config.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/config.rs b/src/config.rs index 14400e45e6..f70d093a0d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -63,7 +63,7 @@ impl Config { let default_url = get_default_url(&ini); let token_url = token_embedded_data .as_ref() - .and_then(|td| Some(td.url.as_str())) + .map(|td| td.url.as_str()) .unwrap_or_default(); let url = match (default_url.as_str(), token_url) { @@ -173,11 +173,7 @@ impl Config { Some(Auth::Token(ref val)) => { self.cached_token_data = val.payload().cloned(); - if let Some(token_url) = self - .cached_token_data - .as_ref() - .and_then(|td| Some(td.url.as_str())) - { + if let Some(token_url) = self.cached_token_data.as_ref().map(|td| td.url.as_str()) { self.cached_base_url = token_url.to_string(); } @@ -211,7 +207,7 @@ impl Config { let token_url = self .cached_token_data .as_ref() - .and_then(|td| Some(td.url.as_str())) + .map(|td| td.url.as_str()) .unwrap_or_default(); if !token_url.is_empty() && url != token_url {