Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 23 additions & 24 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand Down
15 changes: 13 additions & 2 deletions src/utils/auth_token/org_auth_token.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions src/utils/auth_token/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand All @@ -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());

Expand Down
Original file line number Diff line number Diff line change
@@ -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
...

```
5 changes: 5 additions & 0 deletions tests/integration/org_tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Loading