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

fix(hc): Adds support for region URL overrides in CLI #2003

Closed
Show file tree
Hide file tree
Changes from 5 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
25 changes: 18 additions & 7 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,17 @@ impl Config {
.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),
let region_url = token_embedded_data
.as_ref()
.map(|td| td.region_url.as_str())
.unwrap_or_default();

let url = match (default_url.as_str(), token_url, region_url) {
(_, "", "") => default_url,
_ if default_url == token_url || default_url == region_url => default_url,
(DEFAULT_URL | "", _, _) => String::from(token_url),
_ => bail!(
"Two different url values supplied: `{token_url}` (from token), `{default_url}`."
"URL must match one of the provided URLs on the authentication token: `{token_url}` or `{region_url}`, received: `{default_url}`."
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably use some better wording, but it feels appropriate to include the region URL here as an option for the user to select it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is a little quirky. If the region_url and token_url match, you'll essentially get a message with the same expected URL twice. I can make this exhaustive at the expense of adding multiple distinct error message cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we should add the distinct error message cases; might be confusing if we list the same URL twice in the same error message

),
};

Expand Down Expand Up @@ -210,8 +215,14 @@ impl Config {
.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}`.");
let region_url = self
.cached_token_data
.as_ref()
.map(|td| td.region_url.as_str())
.unwrap_or_default();

if !token_url.is_empty() && url != token_url && url != region_url {
bail!("URL must match one of the provided URLs on the authentication token: `{token_url}` or `{region_url}`, received: `{url}`.")
}

self.cached_base_url = url.to_owned();
Expand Down
2 changes: 1 addition & 1 deletion src/utils/auth_token/org_auth_token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub struct OrgAuthToken {
#[allow(dead_code)] // Otherwise, we get a warning about unused fields
pub struct AuthTokenPayload {
iat: f64,
region_url: String,
pub region_url: String,
pub org: String,

// URL may be missing from some old auth tokens, see getsentry/sentry#57123
Expand Down
10 changes: 4 additions & 6 deletions tests/integration/_cases/org_tokens/url-match.trycmd
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
```
$ sentry-cli --auth-token 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 --url http://localhost:8000 sourcemaps upload
$ sentry-cli --auth-token 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 --url http://localhost:8000 sourcemaps upload --org nomatch test_path
? failed
error: the following required arguments were not provided:
<PATHS>...
error: Two different org values supplied: `sentry` (from token), `nomatch`.

Usage: sentry-cli[EXE] sourcemaps upload <PATHS>...

For more information, try '--help'.
Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Comment on lines +2 to +6
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test wasn't actually asserting the URL matches correctly. It appears that the upload <paths> check occurs before the URL checking logic is run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

Please attach the full debug log to all bug reports.

```
4 changes: 2 additions & 2 deletions tests/integration/_cases/org_tokens/url-mismatch.trycmd
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
```
$ sentry-cli --auth-token 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 --url http://example.com sourcemaps upload test_path
$ sentry-cli --auth-token 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 --url http://example.com sourcemaps upload test_path
? failed
error: Two different url values supplied: `http://localhost:8000` (from token), `http://example.com`.
error: URL must match one of the provided URLs on the authentication token: `http://localhost:8000` or `http://region.localhost:8000`, received: `http://example.com`.

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.
Expand Down
9 changes: 9 additions & 0 deletions tests/integration/_cases/org_tokens/url-region-match.trycmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
```
$ sentry-cli --auth-token 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 --url http://region.localhost:8000 sourcemaps upload --org nomatch test_path
? failed
error: Two different org values supplied: `sentry` (from token), `nomatch`.

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.

```
5 changes: 5 additions & 0 deletions tests/integration/org_tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ fn org_token_url_match() {
register_test("org_tokens/url-match.trycmd");
}

#[test]
fn org_token_region_url_match() {
register_test("org_tokens/url-region-match.trycmd");
}

#[test]
fn org_token_org_match() {
register_test("org_tokens/org-match.trycmd");
Expand Down
Loading