diff --git a/CHANGELOG.md b/CHANGELOG.md index b1063b6..1e42430 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +- The telemetry event of a command that uses no token, such as `--version`, + now reports the token kind and account an API command would use. Output + and exit codes are unchanged. - `mapbox usage --dimensions browsers,countries,hosts` asks the Statistics API for the per-browser, per-country and per-host breakdown, which it now sends only on request. Without the flag the request is unchanged, and each diff --git a/README.md b/README.md index 0bd6d97..0f42482 100644 --- a/README.md +++ b/README.md @@ -301,6 +301,7 @@ Each run also sends one event to Mapbox: after an upgrade, and whether it ran in CI or under a detected AI coding agent; - which kind of token the command used and the Mapbox account it belongs to. + A command that uses none reports the one an API command would use. It never includes a file path or free text you typed. It carries a random ID that is replaced on the first run a day after it was created. It is sent in @@ -692,8 +693,9 @@ tools are used and how to improve our products. file paths or free text), the global options it used (such as `--output`, `--dry-run` or whether a named profile was used), how it ended and how long it took, how many requests it made with their HTTP status and sizes, - the kind of token it used and the Mapbox account that token belongs to, - the CLI's own public token the event itself was sent with, a random + the kind of token it used and the Mapbox account that token belongs to + (for a command that uses none, the one an API command would use), the + CLI's own public token the event itself was sent with, a random identifier that is replaced every day, which run started this one when it ran inside another, CLI version, how the CLI was installed, the version it was updated from and any newer one it offered, OS/architecture, diff --git a/src/auth.rs b/src/auth.rs index c16f754..3157e77 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1092,6 +1092,10 @@ pub fn logout(profile: Option<&str>, mode: Mode) -> Result<()> { // Wait for another run's login refresh to finish, so it can't write // the login back after it is removed. let _lock = CredentialLock::acquire(profile).ok(); + // Read before it is gone: the run's account is the one logged out. + if let Some(creds) = load_credentials_readonly(profile) { + crate::run_record::set_token(TokenSource::Login, &creds.access_token); + } crate::run_record::set_auth_step("remove_credentials"); std::fs::remove_file(&path)?; } @@ -1166,6 +1170,21 @@ pub(crate) fn resolve_source<'a>( stored.map(|token| (TokenSource::Login, token)) } +/// [`resolve_source`] for a parsed command line: the token a service command +/// sends, and where it came from. `get_one` folds in the +/// `MAPBOX_ACCESS_TOKEN` fallback declared on the arg, which `--use-login` +/// asks to skip; a typed flag outranks it either way. +pub(crate) fn resolve_token( + matches: &clap::ArgMatches, + use_login: bool, + stored: Option<&str>, +) -> Option<(TokenSource, String)> { + let flag = typed_token(matches); + let environment = matches.get_one::("token").map(String::as_str); + resolve_source(flag.as_deref(), environment, stored, use_login) + .map(|(source, token)| (source, token.to_string())) +} + /// A token's kind, from its prefix: `pk` public, `sk` secret, `tk` temporary. /// /// Read off the prefix rather than the payload because that is where the API diff --git a/src/main.rs b/src/main.rs index 36c147f..ff4c78f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -954,6 +954,7 @@ fn cli() -> u8 { if let Err(e) = commands::check_query(&matches) { run_record::set_unparsed(&app, &raw_argv, e.kind()); + run_record::set_token_candidates(&matches); return report_parse_result(e, &raw_argv); } @@ -1466,18 +1467,18 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - // placeholders, a request body, and `--dry-run` — this GET always refreshes. Some((account_usage::COMMAND, usage_matches)) => { let stored_creds = auth::load_fresh_credentials(debug, profile); - let token: Option = if use_login { - auth::typed_token(matches) - } else { - matches.get_one::("token").cloned() - } - .or_else(|| stored_creds.as_ref().map(|c| c.access_token.clone())); + let resolved = auth::resolve_token( + matches, + use_login, + stored_creds.as_ref().map(|c| c.access_token.as_str()), + ); + let token = resolved.as_ref().map(|(_, token)| token.clone()); if use_login && token.is_none() { return Err(no_stored_credentials(profile)); } - if let Some(token) = &token { - run_record::set_resolved_token(matches, use_login, token); + if let Some((source, token)) = &resolved { + run_record::set_token(*source, token); } account_usage::run( @@ -1621,7 +1622,10 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - mode, ); match attempt { - CliAttempt::Done(result) => return result, + CliAttempt::Done(result) => { + run_record::set_sent_with_cli_token(); + return result; + } CliAttempt::Unusable { error, why, sent } => { // The user already confirmed the request that was // refused; sending it again with their own token is @@ -1640,15 +1644,12 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - } else { auth::load_fresh_credentials(debug, profile) }; - // `get_one` folds in the `MAPBOX_ACCESS_TOKEN` fallback declared on - // the arg; `--use-login` asks for that fallback to be skipped, so - // only a typed flag may still outrank the stored credentials. - let token: Option = if use_login { - auth::typed_token(matches) - } else { - matches.get_one::("token").cloned() - } - .or_else(|| stored_creds.as_ref().map(|c| c.access_token.clone())); + let resolved = auth::resolve_token( + matches, + use_login, + stored_creds.as_ref().map(|c| c.access_token.as_str()), + ); + let token = resolved.as_ref().map(|(_, token)| token.clone()); if let Some((failure, why)) = cli_failure { if let Some(note) = @@ -1660,8 +1661,8 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - if use_login && token.is_none() { return Err(no_stored_credentials(profile)); } - if let Some(token) = &token { - run_record::set_resolved_token(matches, use_login, token); + if let Some((source, token)) = &resolved { + run_record::set_token(*source, token); } let username: Option = matches .get_one::("username") diff --git a/src/run_record.rs b/src/run_record.rs index 8d41c14..48754b2 100644 --- a/src/run_record.rs +++ b/src/run_record.rs @@ -88,6 +88,18 @@ pub(crate) struct Token { pub account: Option, } +/// What a service command would choose its token from, kept for a run that +/// resolved none itself. Raw, like `argv`. +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct TokenCandidates { + pub flag: Option, + /// `MAPBOX_ACCESS_TOKEN`, the one variable a service command reads. May + /// also hold the typed flag, which `flag` already outranks it with. + pub environment: Option, + pub use_login: bool, + pub profile: Option, +} + #[derive(Debug, Clone, PartialEq)] pub(crate) struct Request { pub method: String, @@ -130,6 +142,7 @@ pub(crate) struct Record { pub body_fields: Vec, pub options: Option, pub token: Option, + pub token_candidates: Option, pub requests: Vec, pub more_pages: bool, pub error: Option, @@ -153,6 +166,7 @@ static RECORD: Mutex = Mutex::new(Record { body_fields: Vec::new(), options: None, token: None, + token_candidates: None, requests: Vec::new(), more_pages: false, error: None, @@ -234,6 +248,7 @@ pub fn set_parsed( fields = body_fields(specs, &path); } let options = options(matches, leaf_matches); + let candidates = parsed_token_candidates(matches); with_record(|record| { record.command = path; record.invocation = Some(invocation); @@ -241,6 +256,7 @@ pub fn set_parsed( record.args = args; record.body_fields = fields; record.options = Some(options); + record.token_candidates = Some(candidates); }); } @@ -250,8 +266,10 @@ pub fn set_parsed( pub fn set_unparsed(app: &Command, argv: &[OsString], kind: clap::error::ErrorKind) { use clap::error::ErrorKind; let path = command_from_argv(app, argv); + let candidates = unparsed_token_candidates(argv); with_record(|record| { record.command = path; + record.token_candidates = candidates; match kind { ErrorKind::DisplayHelp | ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand => { record.invocation = Some(Invocation::Help); @@ -272,6 +290,12 @@ pub fn set_token(source: auth::TokenSource, token: &str) { with_record(|record| record.token = Some(token)); } +/// The request went out with the CLI's own token, not the user's, so the +/// run names no token: standing in the user's would say it sent theirs. +pub fn set_sent_with_cli_token() { + with_record(|record| record.token_candidates = None); +} + fn token_fact(source: auth::TokenSource, token: &str) -> Token { let kind = match token.split('.').next() { Some("pk") => "pk", @@ -286,17 +310,65 @@ fn token_fact(source: auth::TokenSource, token: &str) -> Token { } } -/// [`set_token`] for the service arms' resolution: a typed `--token`, -/// then the environment unless `--use-login`, then the stored login. -pub fn set_resolved_token(matches: &ArgMatches, use_login: bool, token: &str) { - let source = if auth::typed_token(matches).is_some() { - auth::TokenSource::Flag - } else if !use_login && matches.get_one::("token").is_some() { - auth::TokenSource::Environment - } else { - auth::TokenSource::Login +fn parsed_token_candidates(matches: &ArgMatches) -> TokenCandidates { + TokenCandidates { + flag: auth::typed_token(matches), + // `get_one` is the typed flag or, failing that, the variable clap + // binds it to; the typed one already outranks it as `flag`. + environment: matches.get_one::("token").cloned(), + use_login: matches.get_flag("use-login"), + profile: matches.get_one::("profile").cloned(), + } +} + +/// For a run [`set_unparsed`] recorded although clap did parse it, so its +/// token is read from the answer rather than guessed from argv. +pub fn set_token_candidates(matches: &ArgMatches) { + let candidates = parsed_token_candidates(matches); + with_record(|record| record.token_candidates = Some(candidates)); +} + +/// Without clap's answer, only a command line that names none of the +/// options that choose a token can be read the way a service command would +/// read it; for any other, a guess could name the wrong account. +fn unparsed_token_candidates(argv: &[OsString]) -> Option { + let chooses_token = |word: &str| { + ["--token", "--profile", "--use-login"].iter().any(|long| { + word.strip_prefix(long) + .is_some_and(|rest| rest.is_empty() || rest.starts_with('=')) + }) || (word.starts_with('-') && !word.starts_with("--") && word.contains('t')) }; - set_token(source, token); + if argv + .iter() + .skip(1) + .any(|word| chooses_token(&word.to_string_lossy())) + { + return None; + } + Some(TokenCandidates { + flag: None, + environment: std::env::var(auth::CLAP_TOKEN_ENV) + .ok() + .filter(|value| !value.is_empty()), + use_login: false, + profile: None, + }) +} + +/// The token a service command would have sent, for a run that resolved +/// none: so that every event of a logged-in user names their account, not +/// only the ones that called an API. Reads the stored login without +/// refreshing it or taking its lock — a refresh never changes the account. +pub(crate) fn would_use_token(record: &Record) -> Option { + let candidates = record.token_candidates.as_ref()?; + let stored = auth::load_credentials_readonly(candidates.profile.as_deref()); + let (source, token) = auth::resolve_source( + candidates.flag.as_deref(), + candidates.environment.as_deref(), + stored.as_ref().map(|c| c.access_token.as_str()), + candidates.use_login, + )?; + Some(token_fact(source, token)) } /// The error the run ended with, as it was reported to the user. diff --git a/src/telemetry_event.rs b/src/telemetry_event.rs index 3da6e16..6a8b4b3 100644 --- a/src/telemetry_event.rs +++ b/src/telemetry_event.rs @@ -347,13 +347,22 @@ pub(crate) fn deliver(record: &Record) { { return; } - let event = build(record); + // Only here, past the opt-out: an event nobody sends is no reason to + // read the login. + let would_use = match record.token { + Some(_) => None, + None => run_record::would_use_token(record), + }; + let event = build(record, would_use.as_ref()); if let Ok(line) = serde_json::to_string(&event) { telemetry_sink::deliver(&line); } } -fn build(record: &Record) -> Event { +/// `would_use` stands in for a run that resolved no token: what a service +/// command would have sent, so `auth.account` names a logged-in user on +/// every event rather than only on those that called an API. +fn build(record: &Record, would_use: Option<&run_record::Token>) -> Event { let options = record.options.as_ref(); Event { event: EVENT, @@ -383,7 +392,7 @@ fn build(record: &Record) -> Event { Some(_) => "named", }), timeout_seconds: options.and_then(|o| o.timeout).map(|t| t.as_secs_f64()), - auth: record.token.as_ref().map(|token| Auth { + auth: record.token.as_ref().or(would_use).map(|token| Auth { source: token.source.as_str(), kind: token.kind, account: token.account.as_deref().map(|a| clip(a, MAX_NAME)), diff --git a/tests/telemetry_events.rs b/tests/telemetry_events.rs index 50c858a..a8508a1 100644 --- a/tests/telemetry_events.rs +++ b/tests/telemetry_events.rs @@ -639,6 +639,136 @@ fn log_in(home: &Path, token: &str) { .expect("write a login"); } +/// Tokens whose payloads claim the accounts in their names. +const LOGIN_TOKEN: &str = "tk.eyJ1IjoibG9naW4tdXNlciIsImEiOiJ4In0.SIG"; +const ENV_TOKEN: &str = "pk.eyJ1IjoiZW52LXVzZXIiLCJhIjoieCJ9.SIG"; +const WORK_TOKEN: &str = "sk.eyJ1Ijoid29yay11c2VyIiwiYSI6IngifQ.SIG"; + +/// `auth` as a run's event reported it. +fn auth_of(cmd: &mut Command) -> Value { + sent(cmd).1["auth"].clone() +} + +fn auth(source: &str, kind: &str, account: &str) -> Value { + serde_json::json!({ "source": source, "type": kind, "account": account }) +} + +/// A run that sends no request names the token a service command would +/// have sent, chosen in the same order, so every event of a logged-in user +/// names their account. +#[test] +fn a_run_without_a_request_names_the_token_an_api_call_would_use() { + let home = scratch("would-use-token"); + log_in(&home, LOGIN_TOKEN); + std::fs::write( + config_dir(&home).join("credentials-work.json"), + format!(r#"{{"access_token":"{WORK_TOKEN}"}}"#), + ) + .expect("write a second login"); + + let login = auth("login", "tk", "login-user"); + let env = auth("environment", "pk", "env-user"); + let cases: [(&[&str], Option<&str>, Value); 7] = [ + (&["config", "list"], None, login.clone()), + (&["--version"], None, login.clone()), + (&["styles", "--help"], None, login.clone()), + // The environment outranks a login, as it does for an API call… + (&["config", "list"], Some(ENV_TOKEN), env.clone()), + (&["--version"], Some(ENV_TOKEN), env), + // …unless `--use-login` skips it. + (&["config", "list", "--use-login"], Some(ENV_TOKEN), login), + ( + &["config", "list", "--profile", "work"], + None, + auth("login", "sk", "work-user"), + ), + ]; + for (args, environment, expected) in cases { + let mut cmd = command(&home); + cmd.args(args); + if let Some(token) = environment { + cmd.env("MAPBOX_ACCESS_TOKEN", token); + } + assert_eq!(auth_of(&mut cmd), expected, "{args:?} {environment:?}"); + } +} + +/// A query `commands search` refuses after clap parsed it still reads its +/// token from the parse, so `--profile` doesn't cost it the account. +#[test] +fn a_refused_search_query_still_names_the_profiles_account() { + let home = scratch("would-use-search"); + std::fs::write( + config_dir(&home).join("credentials-work.json"), + format!(r#"{{"access_token":"{WORK_TOKEN}"}}"#), + ) + .expect("write a login"); + let (out, event) = run_sent(&home, &["--profile", "work", "commands", "search", "?!"]); + assert_eq!(out.status.code(), Some(2), "the query should be refused"); + assert_eq!(event["usageError"], "InvalidValue"); + assert_eq!(event["auth"], auth("login", "sk", "work-user")); +} + +/// `logout` reports the account it logged out, read before the login is +/// removed. +#[test] +fn logout_names_the_account_it_logged_out() { + let home = scratch("would-use-logout"); + log_in(&home, LOGIN_TOKEN); + let (out, event) = run_sent(&home, &["auth", "logout"]); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(!config_dir(&home).join("credentials.json").exists()); + assert_eq!(event["auth"], auth("login", "tk", "login-user")); +} + +/// `report` goes with the CLI's own token when it can; a login standing in +/// for it would say the run sent the user's. +#[test] +fn a_run_sent_with_the_clis_token_reports_no_auth() { + let home = scratch("would-use-cli-token"); + log_in(&home, LOGIN_TOKEN); + let (out, event) = run_sent(&home, &["-o", "json", "report", "--dry-run", "A test"]); + let plan: Value = serde_json::from_slice(&out.stdout).expect("a JSON plan"); + assert_eq!( + plan["token"], "cli", + "the run should have used the CLI's token" + ); + assert_eq!(event["auth"], Value::Null); +} + +#[test] +fn no_token_anywhere_reports_no_auth() { + let home = scratch("would-use-none"); + for args in [&["config", "list"][..], &["--version"]] { + assert_eq!(auth_of(command(&home).args(args)), Value::Null, "{args:?}"); + } + // A profile that was never logged in, as an API call would find too. + log_in(&home, LOGIN_TOKEN); + assert_eq!( + auth_of(command(&home).args(["config", "list", "--profile", "work"])), + Value::Null + ); +} + +/// Help clap answers itself is not parsed, so a typed `--token` or +/// `--profile` can't be read; guessing past it could name the wrong account. +#[test] +fn an_unparsed_run_that_chooses_its_token_reports_no_auth() { + let home = scratch("would-use-unparsed"); + log_in(&home, LOGIN_TOKEN); + for args in [ + &["styles", "--help", "--token", ENV_TOKEN][..], + &["styles", "--help", "-t", ENV_TOKEN], + &["--profile=work", "--version"], + ] { + assert_eq!(auth_of(command(&home).args(args)), Value::Null, "{args:?}"); + } +} + /// The `access_token` the loopback server's one request carried. fn sent_with(received: &mpsc::Receiver) -> String { let request = received @@ -652,8 +782,9 @@ fn sent_with(received: &mpsc::Receiver) -> String { .to_string() } -/// Telemetry never reads the saved login, so sending creates no lock file -/// and doesn't change the directory's permissions. +/// Telemetry reads the saved login only for its account, never through the +/// lock, so sending creates no lock file and doesn't change the directory's +/// permissions. #[cfg(unix)] #[test] fn the_send_leaves_a_login_and_its_directory_alone() {