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

ref(crons): Prefer DSN based auth #1545

Merged
merged 3 commits into from
Mar 24, 2023
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
40 changes: 33 additions & 7 deletions src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use parking_lot::{Mutex, RwLock};
use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS};
use regex::{Captures, Regex};
use sentry::protocol::{Exception, Values};
use sentry::types::Dsn;
use serde::de::{DeserializeOwned, Deserializer};
use serde::{Deserialize, Serialize};
use sha1_smol::Digest;
Expand Down Expand Up @@ -433,6 +434,24 @@ impl Api {
ApiRequest::create(handle, &method, &url, auth, env, headers)
}

/// Convenience method that performs a request using DSN as authentication method.
pub fn request_with_dsn_auth(
&self,
method: Method,
url: &str,
dsn: Dsn,
) -> ApiResult<ApiResponse> {
// We resolve an absolute URL to skip default authentication flow.
let url = self
.config
.get_api_endpoint(url)
.map_err(|err| ApiError::with_source(ApiErrorKind::BadApiUrl, err))?;

self.request(method, &url)?
.with_header("Authorization", &format!("DSN {dsn}"))?
.send()
}

/// Convenience method that performs a `GET` request.
pub fn get(&self, path: &str) -> ApiResult<ApiResponse> {
self.request(Method::Get, path)?.send()
Expand Down Expand Up @@ -1442,11 +1461,16 @@ impl Api {
/// Create a new checkin for a monitor
pub fn create_monitor_checkin(
&self,
dsn: Option<Dsn>,
monitor_slug: &String,
checkin: &CreateMonitorCheckIn,
) -> ApiResult<MonitorCheckIn> {
let path = &format!("/monitors/{}/checkins/", PathArg(monitor_slug),);
let resp = self.post(path, checkin)?;
let resp = if let Some(dsn) = dsn {
self.request_with_dsn_auth(Method::Post, path, dsn)?
} else {
self.post(path, checkin)?
};
if resp.status() == 404 {
return Err(ApiErrorKind::ResourceNotFound.into());
}
Expand All @@ -1456,6 +1480,7 @@ impl Api {
/// Update a checkin for a monitor
pub fn update_monitor_checkin(
&self,
dsn: Option<Dsn>,
monitor_slug: &String,
checkin_id: &Uuid,
checkin: &UpdateMonitorCheckIn,
Expand All @@ -1465,7 +1490,12 @@ impl Api {
PathArg(monitor_slug),
PathArg(checkin_id),
);
let resp = self.put(path, checkin)?;
let resp = if let Some(dsn) = dsn {
self.request_with_dsn_auth(Method::Put, path, dsn)?
} else {
self.put(path, checkin)?
};

if resp.status() == 404 {
return Err(ApiErrorKind::ResourceNotFound.into());
}
Expand Down Expand Up @@ -1812,10 +1842,6 @@ impl ApiRequest {
debug!("using token authentication");
self.with_header("Authorization", &format!("Bearer {token}"))
}
Auth::Dsn(ref public_key) => {
debug!("using dsn authentication");
self.with_header("Authorization", &format!("DSN {public_key}"))
}
}
}

Expand Down Expand Up @@ -2463,7 +2489,7 @@ pub enum MonitorCheckinStatus {
#[derive(Debug, Deserialize)]
pub struct MonitorCheckIn {
pub id: Uuid,
pub status: MonitorCheckinStatus,
pub status: Option<MonitorCheckinStatus>,
pub duration: Option<u64>,
}

Expand Down
2 changes: 0 additions & 2 deletions src/commands/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ fn describe_auth(auth: Option<&Auth>) -> &str {
None => "Unauthorized",
Some(&Auth::Token(_)) => "Auth Token",
Some(&Auth::Key(_)) => "API Key",
Some(&Auth::Dsn(_)) => "DSN",
}
}

Expand All @@ -76,7 +75,6 @@ fn get_config_status_json() -> Result<()> {
rv.auth.auth_type = config.get_auth().map(|val| match val {
Auth::Token(_) => "token".into(),
Auth::Key(_) => "api_key".into(),
Auth::Dsn(_) => "dsn".into(),
});
rv.auth.successful = config.get_auth().is_some() && Api::current().get_auth_info().is_ok();
rv.have_dsn = config.get_dsn().is_ok();
Expand Down
12 changes: 12 additions & 0 deletions src/commands/monitors/run.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use log::warn;
use std::process;
use std::time::Instant;

Expand All @@ -6,6 +7,7 @@ use clap::{Arg, ArgAction, ArgMatches, Command};
use console::style;

use crate::api::{Api, CreateMonitorCheckIn, MonitorCheckinStatus, UpdateMonitorCheckIn};
use crate::config::Config;
use crate::utils::system::{print_error, QuietExit};

pub fn make_command(command: Command) -> Command {
Expand Down Expand Up @@ -35,13 +37,22 @@ pub fn make_command(command: Command) -> Command {

pub fn execute(matches: &ArgMatches) -> Result<()> {
let api = Api::current();
let config = Config::current();
let dsn = config.get_dsn().ok();

// Token based auth is deprecated, prefer DSN style auth for monitor checkins.
// Using token based auth *DOES NOT WORK* when using slugs.
if dsn.is_none() {
warn!("Token auth is deprecated for cron monitor checkins. Please use DSN auth.");
}

let monitor_slug = matches.get_one::<String>("monitor_slug").unwrap();

let allow_failure = matches.get_flag("allow_failure");
let args: Vec<_> = matches.get_many::<String>("args").unwrap().collect();

let monitor_checkin = api.create_monitor_checkin(
dsn.clone(),
monitor_slug,
&CreateMonitorCheckIn {
status: MonitorCheckinStatus::InProgress,
Expand Down Expand Up @@ -69,6 +80,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
match monitor_checkin {
Ok(checkin) => {
api.update_monitor_checkin(
dsn,
monitor_slug,
&checkin.id,
&UpdateMonitorCheckIn {
Expand Down
8 changes: 0 additions & 8 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use crate::utils::http::is_absolute_url;
pub enum Auth {
Key(String),
Token(String),
Dsn(String),
}

lazy_static! {
Expand Down Expand Up @@ -154,9 +153,6 @@ impl Config {
self.ini
.set_to(Some("auth"), "api_key".into(), val.to_string());
}
Some(Auth::Dsn(ref val)) => {
self.ini.set_to(Some("auth"), "dsn".into(), val.to_string());
}
None => {}
}
}
Expand Down Expand Up @@ -588,14 +584,10 @@ fn get_default_auth(ini: &Ini) -> Option<Auth> {
Some(Auth::Token(val))
} else if let Ok(val) = env::var("SENTRY_API_KEY") {
Some(Auth::Key(val))
} else if let Ok(val) = env::var("SENTRY_DSN") {
Some(Auth::Dsn(val))
} else if let Some(val) = ini.get_from(Some("auth"), "token") {
Some(Auth::Token(val.to_owned()))
} else if let Some(val) = ini.get_from(Some("auth"), "api_key") {
Some(Auth::Key(val.to_owned()))
} else if let Some(val) = ini.get_from(Some("auth"), "dsn") {
Some(Auth::Dsn(val.to_owned()))
} else {
None
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```
$ sentry-cli monitors run foo-monitor -- cmd.exe /C echo 123
? success
WARN [..] Token auth is deprecated for cron monitor checkins. Please use DSN auth.
123

```
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```
$ sentry-cli monitors run foo-monitor -- echo 123
? success
WARN [..] Token auth is deprecated for cron monitor checkins. Please use DSN auth.
123

```
14 changes: 14 additions & 0 deletions tests/integration/monitors/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,27 @@ fn command_monitors_run() {
EndpointOptions::new("POST", "/api/0/monitors/foo-monitor/checkins/", 200)
.with_response_file("monitors/post-monitors.json"),
);

if cfg!(windows) {
register_test("monitors/monitors-run-win.trycmd");
} else {
register_test("monitors/monitors-run.trycmd");
}
}

#[test]
fn command_monitors_run_token_auth() {
let _server = mock_endpoint(
EndpointOptions::new("POST", "/api/0/monitors/foo-monitor/checkins/", 200)
.with_response_file("monitors/post-monitors.json"),
);
if cfg!(windows) {
register_test("monitors/monitors-run-token-auth-win.trycmd").env("SENTRY_DSN", "");
Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I was trying to clear it out like this but that wasn't working since even having a blank value was reading it :-)

There must be some logic somewhere that treats empty as None in the new code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's here -

sentry-cli/src/config.rs

Lines 407 to 416 in ac6532f

/// Return the DSN
pub fn get_dsn(&self) -> Result<Dsn> {
if let Ok(val) = env::var("SENTRY_DSN") {
Ok(val.parse()?)
} else if let Some(val) = self.ini.get_from(Some("auth"), "dsn") {
Ok(val.parse()?)
} else {
bail!("No DSN provided");
}
}

Empty string fails to .parse() as Dsn type.

Copy link
Member

Choose a reason for hiding this comment

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

Nice

} else {
register_test("monitors/monitors-run-token-auth.trycmd").env("SENTRY_DSN", "");
}
}

#[test]
fn command_monitors_run_env() {
let _server = mock_endpoint(
Expand Down