Prefer --url argument over empty auth token URL (#1914)

With this change, the CLI will treat an org auth token that specifies an empty URL the same way as an org auth token that is not specifying a URL at all.

We implemented this change in behavior by changing the type of the url in the AuthTokenPayload from an Option<String> to a String. In cases where the url previously would have been set to None, the url will now be set to an empty string. Uses of the AuthTokenPayload's url field have been updated to treat empty strings the way they previously treated None values.

Fixes GH-1913

Author: szokeasaurusrex

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()
+            .map(|td| 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,
@@ -171,12 +173,8 @@ 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| td.url.as_ref())
-                {
-                    self.cached_base_url = token_url.clone();
+                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();
                 }
 
                 self.ini
@@ -206,15 +204,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}`.");
-            }
+            .map(|td| 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());

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<String>, // 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<String, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    Option::deserialize(deserializer).map(|url| url.unwrap_or_default())
 }
 
 impl OrgAuthToken {

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());
 

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
+...
+
+```

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");
+}