diff --git a/docs/design.md b/docs/design.md index 150e183..d0af6da 100644 --- a/docs/design.md +++ b/docs/design.md @@ -283,7 +283,12 @@ approved wherever the owner is with the same cleaned reason that arrive within 250 ms of the first join one batch of up to 8 queries (`FindBatches`, keyed by reason so an unrelated program's find never shares an approval or a Deny), asked on its own task so a caller that gives up cannot strand the rest; the batch is dropped once every caller - has. One prompt quotes every query; a companion answers `{"found_each": [...]}`, one `Found` per query in + has. `op find --also` sends each query as its own `/admin/op/find` request at once, so the batch joins them. + An approved batch's answers are kept for 10 minutes in `KeptValues::finds`, by batch key and query, and a + later find with both the same is answered from there without asking; a forget clears them, and a batch + approved while a forget ran keeps nothing. On a hub, batches and kept answers are keyed by the spoke's ID as + well as the reason (`FindAsk::scope`), so neither a local program nor another spoke can join a spoke's batch + or get its kept answers by claiming its reason One prompt quotes every query; a companion answers `{"found_each": [...]}`, one `Found` per query in order, and one from before batches, which ignores `also` and answers `found`, is reported as needing an update. The Mac lists items once per batch and opens an item two queries match once The Mac that answers runs `op item list --format json`, which holds no values, and scores items by how many query words diff --git a/docs/operations.md b/docs/operations.md index 0150ce8..4a56192 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -216,7 +216,11 @@ the prompt quotes the words and the reason. The phone cannot search 1Password, s reference picked afterwards asks again and names that item. Scripts can do the same with the `secrets` server's `find_references(query, reason)`. Finds with the same reason that arrive within 250 ms of each other, such as the calls a script starts together under `asyncio.gather`, share one prompt that quotes every query, up to 8 of them; -each call still gets its own matches. +each call still gets its own matches. One `op find` can do the same with `--also`: +`codemode-mcp op find apple signing --also pushover --reason "set up the release build"` asks once and prints a +JSON list with each query, its items and `more`. Once approved, the same query with the same reason is answered +again for 10 minutes without asking, so a program rerun after misreading the output does not prompt again; a +different query or reason asks, and `codemode-mcp service restart --fresh` forgets every kept answer. A file attached to an item, such as a signing certificate's `.p12`, is binary. Write it with `codemode-mcp op read "op://Personal/Signing/cert.p12" --out-file cert.p12`, which keeps its exact bytes and adds no diff --git a/src/admin.rs b/src/admin.rs index 624a32f..5bca772 100644 --- a/src/admin.rs +++ b/src/admin.rs @@ -1449,6 +1449,7 @@ async fn op_find( .find_for_program(&companion::FindAsk { query: request.query, reason: request.reason, + scope: None, }) .await; diff --git a/src/companion.rs b/src/companion.rs index fc84940..187dcd7 100644 --- a/src/companion.rs +++ b/src/companion.rs @@ -31,7 +31,9 @@ pub use identity::{IdentityError, Role, load_or_create}; pub use invite::{InviteStatus, MAX_INVITE_LIFETIME, OpenedInvite}; pub use join::{JoinError, Joined, Joining, join}; pub use pair::{PairError, Paired, Pairing, companion_id, pair}; -pub use program::{ProgramAsk, ProgramRead, ProgramReadError, ProgramTarget, ProgramValues}; +pub use program::{ + ProgramAsk, ProgramRead, ProgramReadError, ProgramTarget, ProgramValues, checked_query, +}; pub use server::{CompanionHandle, ServeError, serve}; pub use ticket::{ AnyTicket, INVITE_PREFIX, InviteSecret, InviteTicket, TICKET_PREFIX, Ticket, TicketError, diff --git a/src/companion/find.rs b/src/companion/find.rs index 56571ff..d421b8e 100644 --- a/src/companion/find.rs +++ b/src/companion/find.rs @@ -10,7 +10,13 @@ //! //! Finds with the same reason that arrive within [`FIND_BATCH_WINDOW`] of //! the first are asked as one, up to [`MAX_FIND_QUERIES`], so the searches a -//! script starts together under `asyncio.gather` need one approval +//! script starts together under `asyncio.gather`, or the queries of one +//! `op find`, need one approval +//! +//! An approved answer is kept for [`FIND_KEEP`], so the same query with the +//! same reason, such as a program run again after it misread the output, is +//! answered without asking. It names items and references, never a value, +//! that the owner already let a program with that reason see use std::{ collections::HashMap, @@ -46,6 +52,10 @@ const CANCEL_TITLE: &str = "Deny"; /// milliseconds const FIND_BATCH_WINDOW: Duration = Duration::from_millis(250); +/// How long an approved find answers the same query and reason again +/// without asking; long enough for a program to be fixed and rerun +const FIND_KEEP: Duration = Duration::from_secs(10 * 60); + /// What a program looks for, as the admin route and the `secrets` builtin /// server pass it on #[derive(Debug, Clone, Default)] @@ -54,10 +64,14 @@ pub struct FindAsk { pub query: String, /// Why, shown quoted as the program's claim pub reason: Option, + /// Whose finds these are, such as a spoke's ID, so one requester's find + /// never joins another's batch or gets its kept answer whatever reason + /// it claims; `None` for this machine + pub scope: Option, } /// The items a find matched and the Mac that searched -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ProgramFound { /// `this Mac`, or the name of the companion that answered pub source: String, @@ -65,7 +79,8 @@ pub struct ProgramFound { pub found: Found, } -/// The finds waiting to be asked together, by their cleaned reason +/// The finds waiting to be asked together, by their scope and cleaned +/// reason, as [`batch_key`] joins them /// /// Keying by reason keeps one script's searches together without letting an /// unrelated program's find ride on the same approval or fall with its Deny @@ -107,6 +122,12 @@ impl OnePassword { } let query = checked_query(&ask.query).map_err(ProgramReadError::Invalid)?; let reason = clean_reason(ask.reason.as_deref()); + let key = batch_key(ask.scope.as_deref(), reason.as_deref()); + if let Some(found) = self.kept_find(key.as_deref(), &query).await { + tracing::info!("find answered from one approved earlier"); + + return Ok(found); + } let (answer, answered) = oneshot::channel(); let waiting = Waiting { query, answer }; @@ -114,19 +135,19 @@ impl OnePassword { // one lock, so a batch cannot be taken for asking between the // check and the push let mut forming = self.find_batches().forming(); - match forming.get(&reason).cloned() { + match forming.get(&key).cloned() { Some(batch) => { let mut queued = batch.waiting(); queued.push(waiting); if queued.len() >= MAX_FIND_QUERIES { - forming.remove(&reason); + forming.remove(&key); } None } None => { let batch = Arc::new(Forming(Mutex::new(vec![waiting]))); - forming.insert(reason.clone(), Arc::clone(&batch)); + forming.insert(key.clone(), Arc::clone(&batch)); Some(batch) } @@ -136,7 +157,7 @@ impl OnePassword { // cannot strand the others that joined it if let Some(batch) = opened { let onepassword = self.clone(); - tokio::spawn(async move { onepassword.ask_batch(&batch, reason).await }); + tokio::spawn(async move { onepassword.ask_batch(&batch, key, reason).await }); } answered.await.unwrap_or_else(|_| { @@ -147,18 +168,18 @@ impl OnePassword { } /// Waits out the window, then asks once for every find that joined - /// `batch` and hands each its own items - async fn ask_batch(&self, batch: &Arc, reason: Option) { + /// `batch`, filed under `key`, and hands each its own items + async fn ask_batch(&self, batch: &Arc, key: Option, reason: Option) { tokio::time::sleep(FIND_BATCH_WINDOW).await; let mut waiting = { let mut forming = self.find_batches().forming(); // a full batch already left the map, and a newer one may hold // its place if forming - .get(&reason) + .get(&key) .is_some_and(|current| Arc::ptr_eq(current, batch)) { - forming.remove(&reason); + forming.remove(&key); } std::mem::take(&mut *batch.waiting()) @@ -169,6 +190,9 @@ impl OnePassword { let mut queries: Vec = waiting.iter().map(|find| find.query.clone()).collect(); queries.sort(); queries.dedup(); + // read before asking, so a forget while the prompt is up drops this + // answer instead of keeping it + let forgotten = self.forgotten().await; // every caller giving up takes the prompts down, as a lone find's did let all_gone = async { for find in &mut waiting { @@ -179,6 +203,18 @@ impl OnePassword { answer = self.find_each(&queries, reason.as_deref()) => answer, () = all_gone => return, }; + if let Ok((source, found)) = &answer { + let answers = queries.iter().zip(found).map(|(query, found)| { + let found = ProgramFound { + source: source.clone(), + found: found.clone(), + }; + + (query.clone(), found) + }); + self.keep_finds(key.as_deref(), answers, FIND_KEEP, forgotten) + .await; + } for find in waiting { let own = match &answer { @@ -309,6 +345,16 @@ impl OnePassword { } } +/// What finds are batched and kept under: the cleaned reason alone for this +/// machine, otherwise the scope and the reason on separate lines; a cleaned +/// reason holds no line break, so a scoped key never equals an unscoped one +fn batch_key(scope: Option<&str>, reason: Option<&str>) -> Option { + match scope { + Some(scope) => Some(format!("{scope}\n{}", reason.unwrap_or(""))), + None => reason.map(str::to_owned), + } +} + impl Companions { /// Asks the companion Mac `peer` to search for `find.query` and each of /// `find.also`, answering their items in that order @@ -368,3 +414,22 @@ impl Companions { } } } + +#[cfg(test)] +mod tests { + use super::{batch_key, clean_reason}; + + #[test] + fn no_reason_this_machine_claims_files_its_find_under_a_spokes_key() { + let spoke = batch_key(Some("spoke1"), clean_reason(Some("set up")).as_deref()); + for claimed in [ + "set up", + "spoke1\nset up", + "spoke1\r\nset up", + "spoke1: set up", + ] { + let local = batch_key(None, clean_reason(Some(claimed)).as_deref()); + assert_ne!(local, spoke, "{claimed:?}"); + } + } +} diff --git a/src/env_files.rs b/src/env_files.rs index 81a21f0..c668c66 100644 --- a/src/env_files.rs +++ b/src/env_files.rs @@ -340,7 +340,8 @@ impl SecretsServer { one you want to `write_env`, which asks the owner again naming that item. \ The owner approves the search with Touch ID on this Mac or a companion Mac; \ to look for several things, start one call for each together under \ - `asyncio.gather` with the same reason, and one prompt approves them all." + `asyncio.gather` with the same reason, and one prompt approves them all. \ + The same query and reason is answered again for 10 minutes without asking." )] async fn find_references( &self, @@ -350,6 +351,7 @@ impl SecretsServer { let ask = FindAsk { query: params.query, reason: Some(params.reason), + scope: None, }; let found = tokio::select! { found = self.0.onepassword.find_for_program(&ask) => found.map_err(failed)?, diff --git a/src/guidance.rs b/src/guidance.rs index 7e4505a..c9097e7 100644 --- a/src/guidance.rs +++ b/src/guidance.rs @@ -69,9 +69,13 @@ macro_rules! onepassword_rule { `codemode-mcp op find WORDS --reason WHY` (or the `secrets` server's `find_references` \ tool) lists matching items' op:// references and types, never values, after the user \ approves on a Mac; then use the reference you picked. A query matches items holding any of \ - its words, up to 80 characters and 10 items, so to look for several things, start one \ - `find_references` call for each together under `asyncio.gather` with the same `reason`: \ - the user approves them all with one prompt. Or pass a description in one step: \ + its words, up to 80 characters and 10 items, so put other names for the same thing in one \ + query. To look for several things, pass each extra query as `--also WORDS` to one `op \ + find`, or start one `find_references` call for each together under `asyncio.gather` with \ + the same `reason`: the user approves them all with one prompt. Never run separate finds \ + one after another, since each asks the user again. The same query and reason is answered \ + again for 10 minutes without asking, so rerun it rather than rewording it if you misread \ + the output. Or pass a description in one step: \ `codemode-mcp op read --describe \"Apple signing cert\" --out-file PATH` or `write_env`'s \ `describe`. If the user approves on a Mac it searches instead, writes nothing and answers the \ matches (`op read` exits 3), so run again with the exact reference; if they approve on their \ diff --git a/src/hub/server.rs b/src/hub/server.rs index fc41ce3..6e971bd 100644 --- a/src/hub/server.rs +++ b/src/hub/server.rs @@ -1014,6 +1014,8 @@ async fn op_find( .find_for_program(&FindAsk { query: find.query, reason, + // spokes that share a name never share a batch or kept answers + scope: Some(spoke.id.clone()), }) .await; diff --git a/src/main.rs b/src/main.rs index 8aa07e2..9c8c2ae 100644 --- a/src/main.rs +++ b/src/main.rs @@ -423,10 +423,21 @@ enum OpCommand { /// the JSON printed, then read it with `op read`, `op inject` or `op run`, /// which ask again and name that item. Words are matched in item titles, /// vaults, tags and categories, ignoring case; at most 10 items are listed + /// per query + /// + /// Each `--also` is another query searched under the same approval, and + /// the JSON printed is then a list of each query with its items. The same + /// query with the same reason is answered again for 10 minutes after an + /// approval without asking Find { - /// Words to look for, such as `apple signing` + /// Words to look for, such as `apple signing`; an item matching any + /// of them is listed #[arg(required = true, num_args = 1..)] query: Vec, + /// Another query to search under the same approval, such as `--also + /// "signing cert"`; up to 7 + #[arg(long, value_name = "WORDS")] + also: Vec, /// Why the program is looking; every prompt shows it #[arg(long)] reason: Option, @@ -2263,36 +2274,95 @@ async fn op_command(config: &Config, command: OpCommand) -> Result<()> { } } } - OpCommand::Find { query, reason } => { - let request = admin::OpFindRequest { - query: query.join(" "), - reason, - }; + OpCommand::Find { + query, + also, + reason, + } => { + let several = !also.is_empty(); + let queries: Vec = std::iter::once(query.join(" ")).chain(also).collect(); + if queries.len() > companion::MAX_FIND_QUERIES { + bail!( + "at most {} queries share one approval; pass the query and up to {} --also", + companion::MAX_FIND_QUERIES, + companion::MAX_FIND_QUERIES - 1 + ); + } + // checked before any is sent, so a malformed query cannot leave + // the others asking for nothing + for query in &queries { + companion::checked_query(query).map_err(|message| eyre!("refused: {message}"))?; + } + eprintln!( "Asking the owner to approve a 1Password search: Touch ID on this Mac or a \ companion Mac..." ); - let response: admin::OpFindResponse = - admin_post_as_owner(config, "/admin/op/find", &request).await?; - match response { - admin::OpFindResponse::Found { - source, - items, - more, - } => { - eprintln!("Approved on {source}"); - println!( - "{}", - serde_json::to_string_pretty(&serde_json::json!({ + // the daemon asks once for finds with the same reason that arrive + // together, so every query is sent at once + let mut finds = tokio::task::JoinSet::new(); + for (index, query) in queries.iter().enumerate() { + let config = config.clone(); + let request = admin::OpFindRequest { + query: query.clone(), + reason: reason.clone(), + }; + finds.spawn(async move { + let response: Result = + admin_post_as_owner(&config, "/admin/op/find", &request).await; + + (index, response) + }); + } + let mut responses: Vec> = std::iter::repeat_with(|| None) + .take(queries.len()) + .collect(); + while let Some(joined) = finds.join_next().await { + let (index, response) = joined?; + responses[index] = Some(response?); + } + + let mut sources = Vec::new(); + let mut answers = Vec::new(); + for (query, response) in queries.iter().zip(responses) { + match response { + Some(admin::OpFindResponse::Found { + source, + items, + more, + }) => { + if !sources.contains(&source) { + sources.push(source); + } + answers.push(serde_json::json!({ + "query": query, "items": items, "more": more, - }))? - ); + })); + } + Some(admin::OpFindResponse::Invalid { message }) => { + bail!("refused: {message}") + } + Some(admin::OpFindResponse::Denied { message }) => bail!("denied: {message}"), + Some(admin::OpFindResponse::Failed { message }) => { + bail!("no items: {message}") + } + None => bail!("the daemon did not answer the search for {query:?}"), } - admin::OpFindResponse::Invalid { message } => bail!("refused: {message}"), - admin::OpFindResponse::Denied { message } => bail!("denied: {message}"), - admin::OpFindResponse::Failed { message } => bail!("no items: {message}"), } + + eprintln!("Approved on {}", sources.join(", ")); + let printed = if several { + serde_json::Value::Array(answers) + } else { + let mut answer = answers.pop().unwrap_or_default(); + if let Some(answer) = answer.as_object_mut() { + answer.remove("query"); + } + + answer + }; + println!("{}", serde_json::to_string_pretty(&printed)?); } OpCommand::Inject { in_file, diff --git a/src/secrets.rs b/src/secrets.rs index f8dc127..e7cb713 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -26,7 +26,9 @@ use tokio::{sync::Mutex, task::JoinSet}; use self::approved::{Approved, Consumer}; use crate::{ - companion::{Companions, CreatedItem, DeviceTrust, FindBatches, NewItem, ProgramValue}, + companion::{ + Companions, CreatedItem, DeviceTrust, FindBatches, NewItem, ProgramFound, ProgramValue, + }, config::{Config, SecretsConfig}, hub::client::HubClient, mac::{OnMac, TouchId}, @@ -894,23 +896,27 @@ pub struct OnePassword { hub: Option>, } -/// The keep grants, and how many times they were forgotten, so a read -/// approved before a forget cannot keep its values after it +/// The keep grants and approved finds, and how many times they were +/// forgotten, so a read or find approved before a forget cannot be kept +/// after it #[derive(Default)] struct KeptValues { - grants: HashMap<(Option, String), Kept>, + grants: HashMap<(Option, String), Kept>, + /// Find answers by the key their batch was filed under, its scope and + /// cleaned reason, and query + finds: HashMap<(Option, String), Kept>, forgotten: u64, } -/// A value a program may get again without asking until both clocks pass -/// its end: `Instant` stops while the Mac sleeps, the wall clock can be set -struct Kept { - value: ProgramValue, +/// What a program may get again without asking until both clocks pass its +/// end: `Instant` stops while the Mac sleeps, the wall clock can be set +struct Kept { + value: T, until: std::time::Instant, until_wall: std::time::SystemTime, } -impl Kept { +impl Kept { fn live(&self) -> bool { std::time::Instant::now() < self.until && std::time::SystemTime::now() < self.until_wall } @@ -1056,18 +1062,23 @@ impl OnePassword { Ok(count) } - /// Forgets every value read, and every value kept for programs, so the - /// next read of each asks again, as after a rotated key; how many config - /// values were held + /// Forgets every value read, every value kept for programs and every + /// find answer kept, so the next read or find asks again, as after a + /// rotated key; how many config values were held pub async fn forget(&self) -> usize { - { - let mut kept = self.kept.lock().await; - kept.grants.clear(); - kept.forgotten += 1; - } - let mut known = self.known.lock().await; - let count = known.len(); - known.clear(); + let count = { + let mut known = self.known.lock().await; + let count = known.len(); + known.clear(); + + count + }; + // cleared last, after any wait for `known`, so nothing kept while + // this waited outlasts the forget + let mut kept = self.kept.lock().await; + kept.grants.clear(); + kept.finds.clear(); + kept.forgotten += 1; count } @@ -1104,6 +1115,7 @@ impl OnePassword { } /// How many times the grants were forgotten; pass it to [`Self::keep`] + /// or [`Self::keep_finds`] pub(crate) async fn forgotten(&self) -> u64 { self.kept.lock().await.forgotten } @@ -1138,6 +1150,46 @@ impl OnePassword { } } + /// The answer kept for a find of `query` filed under `key`, the scope + /// and cleaned reason of its batch, while it is live + pub(crate) async fn kept_find(&self, key: Option<&str>, query: &str) -> Option { + let mut kept = self.kept.lock().await; + kept.finds.retain(|_, entry| entry.live()); + + kept.finds + .get(&(key.map(str::to_owned), query.to_owned())) + .map(|entry| entry.value.clone()) + } + + /// Keeps each approved find answer of `answers`, by query, for finds + /// filed under the same `key` to get again without asking until `keep` + /// has passed; nothing is kept when the finds were forgotten since + /// `forgotten` was read + pub(crate) async fn keep_finds( + &self, + key: Option<&str>, + answers: impl IntoIterator, + keep: Duration, + forgotten: u64, + ) { + let until = std::time::Instant::now() + keep; + let until_wall = std::time::SystemTime::now() + keep; + let mut kept = self.kept.lock().await; + if kept.forgotten != forgotten { + return; + } + for (query, found) in answers { + kept.finds.insert( + (key.map(str::to_owned), query), + Kept { + value: found, + until, + until_wall, + }, + ); + } + } + /// Where the owner's presence is read and Touch ID asked, when this /// reader was set up with one pub fn on_mac(&self) -> Option<&OnMac> { diff --git a/tests/gateway/companion.rs b/tests/gateway/companion.rs index ee61915..32dfe06 100644 --- a/tests/gateway/companion.rs +++ b/tests/gateway/companion.rs @@ -1964,6 +1964,74 @@ async fn finds_a_script_starts_together_share_one_prompt() -> TestResult { rig.stop().await } +#[tokio::test(flavor = "multi_thread")] +async fn one_op_find_asks_once_for_every_query_and_a_rerun_asks_nothing() -> TestResult { + let Some(rig) = Rig::start(Options::default()).await? else { + return Ok(()); + }; + signing_items(&rig, "hunter2-signing-secret")?; + std::fs::write( + rig.companion_log.with_file_name("item-tok1.json"), + r#"{"id":"tok1","title":"Pushover","category":"API_CREDENTIAL", + "vault":{"id":"v1","name":"Personal"}, + "fields":[{"id":"credential","type":"CONCEALED","label":"credential", + "reference":"op://Personal/Pushover/credential"}]}"#, + )?; + let find_prompts = || { + rig.companion_mac + .shown() + .into_iter() + .filter(|prompt| prompt.contains("1Password items match")) + .count() + }; + let args = [ + "op", + "find", + "signing", + "--also", + "pushover", + "--reason", + "set up the release build", + ]; + + // an agent ran each of these as its own `op find` and got a prompt each + let (ok, stdout, stderr) = cli(&rig, &args).await?; + assert!(ok, "{stderr}"); + let found: Value = serde_json::from_str(&stdout)?; + let titles: Vec<(&str, &str)> = found + .as_array() + .into_iter() + .flatten() + .flat_map(|answer| { + let query = answer["query"].as_str().unwrap_or_default(); + answer["items"] + .as_array() + .into_iter() + .flatten() + .map(move |item| (query, item["title"].as_str().unwrap_or_default())) + }) + .collect(); + assert_eq!( + titles, + [("signing", "Apple Signing"), ("pushover", "Pushover")] + ); + assert_eq!(find_prompts(), 1); + + // the same searches again, as after misreading the output, are answered + // without asking; another reason is asked + rig.companion_mac.set_answer(Some(TouchId::Review)); + let (ok, again, stderr) = cli(&rig, &args).await?; + assert!(ok, "{stderr}"); + assert_eq!(again, stdout); + assert_eq!(find_prompts(), 1); + + let (ok, _, stderr) = cli(&rig, &["op", "find", "signing", "--reason", "other"]).await?; + assert!(!ok && stderr.contains("denied"), "{stderr}"); + assert_eq!(find_prompts(), 2); + + rig.stop().await +} + #[tokio::test(flavor = "multi_thread")] async fn a_description_approved_on_a_mac_answers_its_matches_and_writes_nothing() -> TestResult { let Some(rig) = Rig::start(Options {