From a16300348944e8e75639d6be13e0952450cac5d8 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Thu, 27 Feb 2025 22:05:08 +0000 Subject: [PATCH 1/3] [TRUNK-14370] Do not error on forbidden/unauthorized Changes our exit logic to not use error reporting on forbidden/unauthorized errors. While I was there, added a nudge to check your token. --- cli-tests/src/upload.rs | 81 +++++++++++++++++++++++++++++++++++++++++ cli/src/main.rs | 33 ++++++++++++----- 2 files changed, 104 insertions(+), 10 deletions(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index d8c5288f..81882c13 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -6,6 +6,7 @@ use api::message::{ GetQuarantineConfigResponse, }; use assert_matches::assert_matches; +use axum::http::StatusCode; use axum::{extract::State, Json}; use bundle::{BundleMeta, FileSetType}; use codeowners::CodeOwners; @@ -601,3 +602,83 @@ async fn quarantines_tests_regardless_of_upload() { *CREATE_BUNDLE_RESPONSE.lock().unwrap() = CreateBundleResponse::Success; command.assert().success(); } +#[tokio::test(flavor = "multi_thread")] +async fn is_ok_on_unauthorized() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + generate_mock_codeowners(&temp_dir); + + let mut mock_server_builder = MockServerBuilder::new(); + + mock_server_builder.set_get_quarantining_config_handler( + |Json(_): Json| async { + Err::, StatusCode>(StatusCode::UNAUTHORIZED) + }, + ); + + mock_server_builder.set_create_bundle_handler( + |State(_): State, _: Json| async { + Err::, StatusCode>(StatusCode::UNAUTHORIZED) + }, + ); + let state = mock_server_builder.spawn_mock_server().await; + + let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); + + command.assert().success(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn is_ok_on_forbidden() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + generate_mock_codeowners(&temp_dir); + + let mut mock_server_builder = MockServerBuilder::new(); + + mock_server_builder.set_get_quarantining_config_handler( + |Json(_): Json| async { + Err::, StatusCode>(StatusCode::FORBIDDEN) + }, + ); + + mock_server_builder.set_create_bundle_handler( + |State(_): State, _: Json| async { + Err::, StatusCode>(StatusCode::FORBIDDEN) + }, + ); + let state = mock_server_builder.spawn_mock_server().await; + + let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); + + command.assert().success(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn is_not_ok_on_bad_request() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + generate_mock_codeowners(&temp_dir); + + let mut mock_server_builder = MockServerBuilder::new(); + + mock_server_builder.set_get_quarantining_config_handler( + |Json(_): Json| async { + Err::, StatusCode>(StatusCode::BAD_REQUEST) + }, + ); + + mock_server_builder.set_create_bundle_handler( + |State(_): State, _: Json| async { + Err::, StatusCode>(StatusCode::BAD_REQUEST) + }, + ); + let state = mock_server_builder.spawn_mock_server().await; + + let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); + + command.assert().failure(); +} diff --git a/cli/src/main.rs b/cli/src/main.rs index 25f0942f..3050f0d8 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -2,6 +2,7 @@ use std::env; use clap::{Parser, Subcommand}; use clap_verbosity_flag::{log::LevelFilter, InfoLevel, Verbosity}; +use http::StatusCode; use third_party::sentry; use tracing_subscriber::prelude::*; use trunk_analytics_cli::{ @@ -74,18 +75,30 @@ fn main() -> anyhow::Result<()> { ); match run(cli).await { Ok(exit_code) => std::process::exit(exit_code), - Err(e) => match (*(e.root_cause())).downcast_ref::() { - Some(io_error) if io_error.kind() == std::io::ErrorKind::ConnectionRefused => { - tracing::warn!("Could not connect to trunk's server: {:?}", e); - guard.flush(None); - std::process::exit(exitcode::OK); + Err(error) => { + let root_cause = error.root_cause(); + if let Some(io_error) = root_cause.downcast_ref::() { + if io_error.kind() == std::io::ErrorKind::ConnectionRefused { + tracing::warn!("Could not connect to trunk's server: {:?}", error); + guard.flush(None); + std::process::exit(exitcode::OK); + } } - _ => { - tracing::error!("Error: {:?}", e); - guard.flush(None); - std::process::exit(exitcode::SOFTWARE); + + if let Some(reqwest_error) = root_cause.downcast_ref::() { + if let Some(status) = reqwest_error.status() { + if status == StatusCode::UNAUTHORIZED || status == StatusCode::FORBIDDEN { + tracing::warn!("Unauthorized to access trunk, are you sure your token is correct? {:?}", error); + guard.flush(None); + std::process::exit(exitcode::OK); + } + } } - }, + + tracing::error!("Error: {:?}", error); + guard.flush(None); + std::process::exit(exitcode::SOFTWARE); + } } }) } From 469e88e03a44e8d1c90346eadaf2dcf5d357e0ee Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Fri, 28 Feb 2025 15:10:56 +0000 Subject: [PATCH 2/3] Change exit code on unauthorized to not report OK --- cli/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 3050f0d8..d8602f81 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -90,7 +90,7 @@ fn main() -> anyhow::Result<()> { if status == StatusCode::UNAUTHORIZED || status == StatusCode::FORBIDDEN { tracing::warn!("Unauthorized to access trunk, are you sure your token is correct? {:?}", error); guard.flush(None); - std::process::exit(exitcode::OK); + std::process::exit(exitcode::SOFTWARE); } } } From ff8374aae2925510056e979bd6f0ed3c3fc5ad22 Mon Sep 17 00:00:00 2001 From: Christian Millar Date: Fri, 28 Feb 2025 15:56:27 +0000 Subject: [PATCH 3/3] Fix failing tests --- cli-tests/src/upload.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/cli-tests/src/upload.rs b/cli-tests/src/upload.rs index d3ae800a..6daff25d 100644 --- a/cli-tests/src/upload.rs +++ b/cli-tests/src/upload.rs @@ -631,7 +631,10 @@ async fn is_ok_on_unauthorized() { let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); - command.assert().success(); + command + .assert() + .failure() + .stdout(predicate::str::contains("Error: ").not()); } #[tokio::test(flavor = "multi_thread")] @@ -658,7 +661,10 @@ async fn is_ok_on_forbidden() { let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); - command.assert().success(); + command + .assert() + .failure() + .stdout(predicate::str::contains("Error: ").not()); } #[tokio::test(flavor = "multi_thread")] @@ -685,7 +691,10 @@ async fn is_not_ok_on_bad_request() { let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone()).command(); - command.assert().failure(); + command + .assert() + .failure() + .stdout(predicate::str::contains("Error: ")); } #[tokio::test(flavor = "multi_thread")]