diff --git a/CHANGELOG.md b/CHANGELOG.md index 76ddb06..383afb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,11 @@ commands, `--json` output and exit codes. Each release's notes start with its se `portfolio` and `credential_scopes` fields, null when the API cannot be read; the exit code does not depend on that call. ([#48](https://github.com/Hodeitek/hodeishield-cli/issues/48)) +- `whoami` shows the signed-in user (`Name `, in the `User` line and the `--json` `user` + string) from `GET /v1/me` when the API provides it for a sign-in, with no extra call. The + authorization server's userinfo endpoint stays as the fallback; a key still shows no user, and + nothing changes while the API does not send one. + ([#182](https://github.com/Hodeitek/hodeishield-cli/issues/182)) ### Fixed diff --git a/README.md b/README.md index 585f1a7..e2bc4ee 100644 --- a/README.md +++ b/README.md @@ -302,7 +302,8 @@ When `HODEISHIELD_API_KEY` is set it is used for every call, whatever else is co your CI's secret store; the CLI never prints it. `whoami` asks the API who the credential is and shows the tenant it reads (name and id), its -organisation and, for a key, its scopes; for a portfolio key, the MSSP and how many client tenants it +organisation and, for a key, its scopes; for a sign-in, also the signed-in user when the service +provides it; for a portfolio key, the MSSP and how many client tenants it can select. With `--json` these are the `tenant`, `organization`, `portfolio` and `credential_scopes` fields. If the API cannot be reached, `whoami` still shows the local settings, leaves those fields null and says why on standard error. diff --git a/crates/hodeishield-cli/src/commands/session.rs b/crates/hodeishield-cli/src/commands/session.rs index 9d4928c..1a133f8 100644 --- a/crates/hodeishield-cli/src/commands/session.rs +++ b/crates/hodeishield-cli/src/commands/session.rs @@ -221,11 +221,11 @@ fn note_api_key() { pub fn whoami(ctx: &Context, out: &mut dyn Write) -> Result<()> { let settings = ctx.settings()?; // `GET /v1/me` describes the credential itself: it takes no tenant selection. - let me = || -> std::result::Result { + let me = || -> std::result::Result { let (client, source) = ctx.api(&settings, None).map_err(|f| f.message)?; client .who_am_i() - .map(|answer| answer.data.data) + .map(|answer| Me::from_answer(answer.data.data, &answer.raw)) .map_err(|e| { let mut call = ctx.call(source, None); call.tenant_selected = false; @@ -238,12 +238,78 @@ pub fn whoami(ctx: &Context, out: &mut dyn Write) -> Result<()> { ctx.store.as_ref(), ctx.json, &me, + &userinfo_label, out, ) } -/// `whoami` with the settings, whether an API key is set and the call to `GET /v1/me` given, so it -/// can be tested without the process environment. With a key, the store is not read. The call is +/// What `GET /v1/me` says about the credential: the generated type, and the signed-in user, which +/// the document of the API does not describe yet and so is read from the body as received. +struct Me { + who: WhoAmI, + user: Option, +} + +impl Me { + fn from_answer(who: WhoAmI, raw: &Value) -> Self { + Self { + who, + user: MeUser::from_raw(&raw["data"]["user"]), + } + } +} + +/// The user of a sign-in, as `GET /v1/me` gives it. Read leniently: anything that is not an object +/// with at least one text field is no user, never an error. +struct MeUser { + id: Option, + name: Option, + email: Option, +} + +impl MeUser { + fn from_raw(raw: &Value) -> Option { + let object = raw.as_object()?; + let text = |key: &str| object.get(key).and_then(Value::as_str).map(str::to_owned); + let user = Self { + id: text("id"), + name: text("name"), + email: text("email"), + }; + (user.id.is_some() || user.name.is_some() || user.email.is_some()).then_some(user) + } + + /// Labelled like the user of the userinfo endpoint: `Name `, whichever of them there is, + /// or the id. + fn label(&self) -> Option { + let mut info = Map::new(); + for (key, value) in [ + ("name", &self.name), + ("email", &self.email), + ("sub", &self.id), + ] { + if let Some(value) = value { + info.insert(key.into(), json!(value)); + } + } + user_label(&info) + } +} + +/// The user from the authorization server's userinfo endpoint: the fallback for an API that does +/// not say who the user is. Nothing when the app has no such endpoint or cannot be reached. +fn userinfo_label(token: &StoredToken) -> Option { + let issuer = auth::issuer_url(token).ok()?; + let http = oauth::http_client(&issuer).ok()?; + let metadata = oauth::discover(&http, &issuer).ok()?; + let info = oauth::userinfo(&http, &metadata, &token.access_token) + .ok() + .flatten()?; + user_label(&info) +} + +/// `whoami` with the settings, whether an API key is set, the call to `GET /v1/me` and the userinfo +/// fallback given, so it can be tested without the process environment. With a key, the store is not read. The call is /// made only when there is a credential; when it fails, the local lines are still printed, the /// API's lines are left out (null in `--json`) and a warning says why. fn whoami_with( @@ -251,7 +317,8 @@ fn whoami_with( has_api_key: bool, store: &dyn auth::TokenStore, json: bool, - me: &dyn Fn() -> std::result::Result, + me: &dyn Fn() -> std::result::Result, + userinfo: &dyn Fn(&StoredToken) -> Option, out: &mut dyn Write, ) -> Result<()> { let mut info = Map::new(); @@ -259,6 +326,7 @@ fn whoami_with( info.insert("api_url".into(), json!(settings.api_url.as_str())); info.insert("app_url".into(), json!(settings.app_url.as_str())); + let mut signed_in_token = None; let source = if has_api_key { Some(CredentialSource::ApiKey) } else { @@ -273,16 +341,9 @@ fn whoami_with( .map_or(Value::Null, |t| json!(unix_to_rfc3339(t))), ); info.insert("scope".into(), json!(token.scope)); - let user = auth::issuer_url(&token) - .and_then(|issuer| { - let http = oauth::http_client(&issuer)?; - let metadata = oauth::discover(&http, &issuer)?; - oauth::userinfo(&http, &metadata, &token.access_token) - }) - .ok() - .flatten() - .and_then(|u| user_label(&u)); - info.insert("user".into(), json!(user)); + // Filled in below, once the API has been asked who the user is. + info.insert("user".into(), Value::Null); + signed_in_token = Some(token); Some(CredentialSource::OAuth) } None => None, @@ -307,21 +368,29 @@ fn whoami_with( } None => None, }; + if let Some(token) = &signed_in_token { + // The API first; the userinfo endpoint only when it gives no user. A key has no user, + // whatever the API sends. + let user = me + .as_ref() + .and_then(|me| me.user.as_ref()) + .and_then(MeUser::label) + .or_else(|| userinfo(token)); + info.insert("user".into(), json!(user)); + } + let who = me.as_ref().map(|me| &me.who); info.insert( "tenant".into(), - json!(me.as_ref().and_then(|me| me.tenant.as_ref())), - ); - info.insert( - "organization".into(), - json!(me.as_ref().map(|me| &me.organization)), + json!(who.and_then(|me| me.tenant.as_ref())), ); + info.insert("organization".into(), json!(who.map(|me| &me.organization))); info.insert( "portfolio".into(), - json!(me.as_ref().and_then(|me| me.portfolio.as_ref())), + json!(who.and_then(|me| me.portfolio.as_ref())), ); info.insert( "credential_scopes".into(), - json!(me.as_ref().map(|me| &me.credential.scopes)), + json!(who.map(|me| &me.credential.scopes)), ); if json { @@ -349,7 +418,7 @@ fn whoami_with( rows.push((label, clean(value))); } } - if let Some(me) = &me { + if let Some(me) = who { rows.extend(me_rows(me, source == Some(CredentialSource::ApiKey))); } for (label, value) in rows { @@ -791,6 +860,7 @@ mod tests { &UnreachableStore, false, &no_call, + &no_userinfo, &mut out, ) .expect_err("no credential"); @@ -817,7 +887,16 @@ mod tests { let settings = settings_for(&api, &app); let mut out = Vec::new(); let me = || Err("offline".to_owned()); - whoami_with(&settings, true, &UnreachableStore, true, &me, &mut out).expect("key set"); + whoami_with( + &settings, + true, + &UnreachableStore, + true, + &me, + &no_userinfo, + &mut out, + ) + .expect("key set"); let printed: Value = serde_json::from_slice(&out).expect("json"); assert_eq!(printed["credential"], "api_key"); // The API could not be read: its fields are there, null. @@ -826,12 +905,150 @@ mod tests { } } - fn no_call() -> std::result::Result { + fn no_call() -> std::result::Result { panic!("GET /v1/me is not called without a credential") } - fn who(json: &str) -> WhoAmI { - serde_json::from_str(json).expect("WhoAmI") + fn no_userinfo(_: &StoredToken) -> Option { + panic!("the userinfo endpoint is not asked here") + } + + /// The `data` of a `GET /v1/me` answer, read the way the client reads it. + fn who(json: &str) -> Me { + let data: Value = serde_json::from_str(json).expect("json"); + let typed = serde_json::from_value(data.clone()).expect("WhoAmI"); + Me::from_answer(typed, &json!({ "data": data })) + } + + /// A `data` for a sign-in, with `user` spliced in as written (nothing when `None`). + fn sign_in_me(user: Option<&str>) -> Me { + let user = user.map_or_else(String::new, |u| format!(r#","user":{u}"#)); + who(&format!( + r#"{{"credential":{{"type":"oauth","prefix":null,"scopes":["supply_risk:read"]}}, + "tenant":{{"id":"6f1c2d3e-4b5a-4c6d-8e7f-0a1b2c3d4e5f","name":"Acme S.L.","type":"direct"}}, + "organization":{{"id":"7a1c2d3e-4b5a-4c6d-8e7f-0a1b2c3d4e5f","name":"Acme Group","type":"direct"}}, + "portfolio":null{user}}}"# + )) + } + + /// `whoami` of a sign-in whose `GET /v1/me` answers with `user` (as JSON text), and the + /// userinfo fallback gives `fallback`; the table and the JSON are returned. + fn whoami_of_a_sign_in(user: Option<&str>, fallback: Option<&str>) -> (String, Value) { + let (api, app) = (mockito::Server::new(), mockito::Server::new()); + let settings = settings_for(&api, &app); + let store = MemoryStore::default(); + store.save("default", &signed_in(&app, None)).expect("save"); + let me = || Ok(sign_in_me(user)); + let userinfo = |_: &StoredToken| fallback.map(str::to_owned); + let mut table = Vec::new(); + whoami_with(&settings, false, &store, false, &me, &userinfo, &mut table) + .expect("signed in"); + let mut json = Vec::new(); + whoami_with(&settings, false, &store, true, &me, &userinfo, &mut json).expect("signed in"); + ( + String::from_utf8(table).expect("utf8"), + serde_json::from_slice(&json).expect("json"), + ) + } + + #[test] + fn whoami_shows_the_user_that_the_api_gives_for_a_sign_in() { + let (text, printed) = whoami_of_a_sign_in( + Some(r#"{"id":"u-1","name":"Ana\u001b[31m Ruiz","email":"ana@acme.example"}"#), + None, + ); + assert!( + text.contains("User Ana\u{fffd}[31m Ruiz \n"), + "{text}" + ); + assert!(!text.contains('\u{1b}'), "the name is cleaned: {text:?}"); + // The same text and type as before: a string. + assert_eq!(printed["user"], "Ana\u{fffd}[31m Ruiz "); + assert!(!printed["user"].as_str().expect("string").contains('\u{1b}')); + let keys: Vec<&String> = printed.as_object().expect("object").keys().collect(); + let position = |k: &str| keys.iter().position(|key| *key == k).expect(k); + assert!(position("scope") < position("user") && position("user") < position("credential")); + } + + #[test] + fn whoami_prefers_the_api_over_the_userinfo_endpoint() { + let (text, printed) = whoami_of_a_sign_in( + Some(r#"{"id":"u-1","name":"Ana","email":"ana@acme.example"}"#), + Some("Someone Else"), + ); + assert!( + text.contains("User Ana \n"), + "{text}" + ); + assert_eq!(printed["user"], "Ana "); + } + + #[test] + fn whoami_without_a_user_from_the_api_falls_back_to_userinfo() { + // Today's API (no `user` field), `user: null`, and a malformed `user`. + for user in [None, Some("null"), Some(r#""x""#), Some("[]"), Some("{}")] { + let (text, printed) = whoami_of_a_sign_in(user, Some("Old Way ")); + assert!( + text.contains("User Old Way \n"), + "{user:?}: {text}" + ); + assert_eq!(printed["user"], "Old Way ", "{user:?}"); + } + } + + #[test] + fn whoami_without_any_user_has_no_user_line_and_does_not_fail() { + for user in [None, Some("null"), Some(r#""x""#), Some("42"), Some("{}")] { + let (text, printed) = whoami_of_a_sign_in(user, None); + assert!(!text.contains("User"), "{user:?}: {text}"); + assert_eq!(printed["user"], Value::Null, "{user:?}"); + // The rest of the answer is still shown. + assert!(text.contains("Org Acme Group (direct)\n"), "{text}"); + assert_eq!(printed["organization"]["name"], "Acme Group"); + } + } + + #[test] + fn whoami_uses_the_id_when_the_api_gives_no_name_or_email() { + let (text, _) = whoami_of_a_sign_in(Some(r#"{"id":"u-1","name":null}"#), None); + assert!(text.contains("User u-1\n"), "{text}"); + } + + #[test] + fn whoami_ignores_the_user_the_api_sends_for_a_key() { + let (api, app) = (mockito::Server::new(), mockito::Server::new()); + let settings = settings_for(&api, &app); + let me = || { + Ok(sign_in_me(Some( + r#"{"id":"u-1","name":"Ana","email":"a@b.example"}"#, + ))) + }; + let mut out = Vec::new(); + whoami_with( + &settings, + true, + &UnreachableStore, + false, + &me, + &no_userinfo, + &mut out, + ) + .expect("key set"); + let text = String::from_utf8(out).expect("utf8"); + assert!(!text.contains("User") && !text.contains("Ana"), "{text}"); + let mut out = Vec::new(); + whoami_with( + &settings, + true, + &UnreachableStore, + true, + &me, + &no_userinfo, + &mut out, + ) + .expect("key set"); + let printed: Value = serde_json::from_slice(&out).expect("json"); + assert!(printed.get("user").is_none(), "{printed}"); } #[test] @@ -847,7 +1064,16 @@ mod tests { )) }; let mut out = Vec::new(); - whoami_with(&settings, true, &UnreachableStore, false, &me, &mut out).expect("key set"); + whoami_with( + &settings, + true, + &UnreachableStore, + false, + &me, + &no_userinfo, + &mut out, + ) + .expect("key set"); let text = String::from_utf8(out).expect("utf8"); assert!( text.contains( @@ -864,7 +1090,16 @@ mod tests { assert!(!text.contains("Tenants"), "{text}"); let mut out = Vec::new(); - whoami_with(&settings, true, &UnreachableStore, true, &me, &mut out).expect("key set"); + whoami_with( + &settings, + true, + &UnreachableStore, + true, + &me, + &no_userinfo, + &mut out, + ) + .expect("key set"); let printed: Value = serde_json::from_slice(&out).expect("json"); assert_eq!( printed["tenant"]["id"], @@ -889,7 +1124,16 @@ mod tests { )) }; let mut out = Vec::new(); - whoami_with(&settings, true, &UnreachableStore, false, &me, &mut out).expect("key set"); + whoami_with( + &settings, + true, + &UnreachableStore, + false, + &me, + &no_userinfo, + &mut out, + ) + .expect("key set"); let text = String::from_utf8(out).expect("utf8"); assert!(!text.contains("Tenant "), "{text}"); assert!(text.contains("Org - (mssp)\n"), "{text}"); diff --git a/crates/hodeishield-cli/tests/contract.rs b/crates/hodeishield-cli/tests/contract.rs index ef72df6..3f2974b 100644 --- a/crates/hodeishield-cli/tests/contract.rs +++ b/crates/hodeishield-cli/tests/contract.rs @@ -528,6 +528,33 @@ fn json_whoami_fields() { me.assert(); } +/// A key has no user: whatever `user` the API sends in `GET /v1/me`, neither the table nor `--json` +/// shows one, and the fields stay as they were. +#[test] +fn whoami_with_a_key_ignores_a_user_in_the_answer() { + let env = Env::new(); + let mut server = mockito::Server::new(); + server + .mock("GET", "/v1/me") + .with_body( + r#"{"data":{"credential":{"type":"tenant","prefix":null,"scopes":["supply_risk:read"]}, + "tenant":{"id":"6f1c2d3e-4b5a-4c6d-8e7f-0a1b2c3d4e5f","name":"Acme S.L.","type":"direct"}, + "organization":{"id":"7a1c2d3e-4b5a-4c6d-8e7f-0a1b2c3d4e5f","name":"Acme Group","type":"direct"}, + "portfolio":null, + "user":{"id":"u-1","name":"Ana Ruiz","email":"ana@acme.example"}}}"#, + ) + .expect(2) + .create(); + let output = env.api(&server.url(), &["--json", "whoami"]); + assert_eq!(output.status.code(), Some(0), "{output:?}"); + let printed: Value = serde_json::from_str(&out(&output)).expect("json"); + assert!(printed.get("user").is_none(), "{printed}"); + assert!(!out(&output).contains("Ana Ruiz")); + let output = env.api(&server.url(), &["whoami"]); + assert_eq!(output.status.code(), Some(0), "{output:?}"); + assert!(!out(&output).contains("User") && !out(&output).contains("Ana")); +} + /// When `GET /v1/me` fails, `whoami` still succeeds with the local fields; the API's are null and /// standard error says why. #[test]