Skip to content

Commit 95d0c52

Browse files
committed
refactor(cli): share the confirmation prompt and the private-file writer
Both had reached a third copy, which is where the review asked for them to be folded together. Deleting an OIDC provider, running a replication check that writes to every configured target, and clearing somebody's second factor each carried their own confirmation. They differed only in three strings, and agreed on the parts that matter: `--yes` skips the question, a run with nobody to ask fails rather than assuming consent, and anything other than `y`/`yes` is a decline. Those are exactly the rules a fourth copy would get subtly wrong, so they now live in `crate::confirm` and each caller supplies its own wording. `admin config export` and the recovery-code output both create a file that only its owner may read. The mechanics — created rather than opened, `0600` set in the open flags so it is never briefly readable by anyone else, never a silent overwrite — move to `crate::private_file`. How the refusal is read stays with the callers, because it genuinely differs: an export that will not clobber a file is an ordinary I/O failure, while an occupied recovery-code path is a conflict the operator has to resolve before there is anywhere to put the only copy of a set the server has already issued. `config.rs` has a test pinning the first of those, and this change keeps it passing. No behaviour changes. The refactor left three `std::io` imports unused, which `-D warnings` rejects, and one `rc_core::Error` import used only from a test module; that assertion now uses the fully qualified path its neighbours in the same file already use.
1 parent d0432c6 commit 95d0c52

9 files changed

Lines changed: 252 additions & 131 deletions

File tree

crates/cli/src/commands/admin/account.rs

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use serde::Serialize;
1919
use super::get_admin_client;
2020
use crate::exit_code::ExitCode;
2121
use crate::output::{Formatter, qr};
22+
use crate::private_file::write_private_file;
2223
use crate::secret_input::{SecretSource, can_prompt, read_code_interactive};
2324
use rc_core::admin::{
2425
AccountApi, AccountInfo, AccountMfaApi, CredentialsSource, IdentityType, MfaEnrollment,
@@ -858,35 +859,24 @@ fn print_recovery_codes(codes: &RecoveryCodes, formatter: &Formatter, activated:
858859
}
859860

860861
/// Write recovery codes to a new file with owner-only permissions.
862+
///
863+
/// The file mechanics are shared with `admin config export`; only the reading of
864+
/// the failure is local. An occupied path is a `Conflict` here rather than a
865+
/// plain I/O error, because what is in the way may be the only copy of a
866+
/// previous set and the operator has to decide what happens to it.
861867
fn write_recovery_codes(path: &std::path::Path, codes: &[String]) -> Result<()> {
862-
use std::io::Write as _;
863-
864-
let mut options = std::fs::OpenOptions::new();
865-
// `create_new` so an existing file is never silently overwritten: it may
866-
// hold the only copy of a previous set.
867-
options.write(true).create_new(true);
868-
#[cfg(unix)]
869-
{
870-
use std::os::unix::fs::OpenOptionsExt as _;
871-
options.mode(0o600);
872-
}
873-
874-
let mut file = options.open(path).map_err(|error| {
875-
if error.kind() == std::io::ErrorKind::AlreadyExists {
876-
Error::Conflict(format!(
877-
"{} already exists; choose another path",
878-
path.display()
879-
))
880-
} else {
881-
Error::Io(error)
882-
}
883-
})?;
884-
868+
let mut contents = String::new();
885869
for code in codes {
886-
writeln!(file, "{code}").map_err(Error::Io)?;
870+
contents.push_str(code);
871+
contents.push('\n');
887872
}
888-
file.flush().map_err(Error::Io)?;
889-
Ok(())
873+
874+
write_private_file(path, contents.as_bytes()).map_err(|error| match error {
875+
Error::Io(io) if io.kind() == std::io::ErrorKind::AlreadyExists => Error::Conflict(
876+
format!("{} already exists; choose another path", path.display()),
877+
),
878+
other => other,
879+
})
890880
}
891881

892882
/// Report a failed operation and return its exit code.

crates/cli/src/commands/admin/config.rs

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ use zeroize::Zeroizing;
1717
use super::get_admin_client;
1818
use crate::exit_code::ExitCode;
1919
use crate::output::Formatter;
20+
use crate::private_file::write_private_file;
2021

2122
#[derive(Subcommand, Debug)]
2223
#[command(disable_help_subcommand = true)]
@@ -839,20 +840,6 @@ fn read_protected_value_file(path: &Path) -> rc_core::Result<Zeroizing<String>>
839840
Ok(Zeroizing::new(value.to_string()))
840841
}
841842

842-
fn write_private_file(path: &Path, contents: &[u8]) -> rc_core::Result<()> {
843-
let mut options = std::fs::OpenOptions::new();
844-
options.write(true).create_new(true);
845-
#[cfg(unix)]
846-
{
847-
use std::os::unix::fs::OpenOptionsExt;
848-
options.mode(0o600);
849-
}
850-
let mut file = options.open(path)?;
851-
use std::io::Write;
852-
file.write_all(contents)?;
853-
Ok(())
854-
}
855-
856843
fn emit_error(error: &Error, formatter: &Formatter) -> ExitCode {
857844
let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError);
858845
if formatter.is_json() {

crates/cli/src/commands/admin/idp.rs

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,12 @@ use rc_core::{Error, Result};
99
use serde::Serialize;
1010
use serde_json::Value;
1111
use std::fs::File;
12-
use std::io::{BufRead, IsTerminal, Read, Write};
12+
use std::io::Read;
1313
use std::path::{Path, PathBuf};
1414
use zeroize::Zeroizing;
1515

1616
use super::{emit_observability_error, get_admin_client};
17+
use crate::confirm::{Confirmation, confirm};
1718
use crate::exit_code::ExitCode;
1819
use crate::output::Formatter;
1920

@@ -554,35 +555,19 @@ async fn prepare_and_delete(
554555
}
555556

556557
fn confirm_delete(provider: &OidcProvider, yes: bool, formatter: &Formatter) -> Result<()> {
557-
if yes {
558-
return Ok(());
559-
}
560-
if formatter.is_json() || !std::io::stdin().is_terminal() {
561-
return Err(Error::InvalidPath(
562-
"OIDC provider deletion requires --yes in non-interactive or JSON mode".to_string(),
563-
));
564-
}
565-
566-
let mut stderr = std::io::stderr().lock();
567-
write!(
568-
stderr,
569-
"Delete OIDC provider '{}'? [y/N] ",
558+
let prompt = format!(
559+
"Delete OIDC provider '{}'? [y/N]",
570560
safe(&provider.provider_id, formatter)
561+
);
562+
confirm(
563+
&Confirmation {
564+
prompt: &prompt,
565+
requires_yes: "OIDC provider deletion requires --yes in non-interactive or JSON mode",
566+
declined: "OIDC provider deletion was declined",
567+
},
568+
yes,
569+
formatter,
571570
)
572-
.map_err(Error::Io)?;
573-
stderr.flush().map_err(Error::Io)?;
574-
let mut answer = String::new();
575-
std::io::stdin()
576-
.lock()
577-
.read_line(&mut answer)
578-
.map_err(Error::Io)?;
579-
if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") {
580-
Ok(())
581-
} else {
582-
Err(Error::Interrupted(
583-
"OIDC provider deletion was declined".to_string(),
584-
))
585-
}
586571
}
587572

588573
async fn prepare_and_apply_mutation(

crates/cli/src/commands/admin/user.rs

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ use clap::Subcommand;
88
use serde::Serialize;
99

1010
use super::get_admin_client;
11+
use crate::confirm::{Confirmation, confirm};
1112
use crate::exit_code::ExitCode;
1213
use crate::output::Formatter;
1314
use crate::secret_input::{SecretSource, can_prompt};
14-
use rc_core::Error;
1515
use rc_core::admin::{AdminApi, SecretValue, User, UserCredentialApi, UserStatus};
1616

1717
const ADD_USER_AFTER_HELP: &str = "\
@@ -398,40 +398,19 @@ async fn execute_user_mfa_reset(args: UserMfaResetArgs, formatter: &Formatter) -
398398
/// — the user has to enrol again — so a non-interactive run must say `--yes`
399399
/// rather than have the confirmation silently skipped.
400400
fn confirm_mfa_reset(access_key: &str, yes: bool, formatter: &Formatter) -> rc_core::Result<()> {
401-
use std::io::{BufRead as _, IsTerminal as _, Write as _};
402-
403-
if yes {
404-
return Ok(());
405-
}
406-
if formatter.is_json() || !std::io::stdin().is_terminal() {
407-
return Err(Error::InvalidPath(
408-
"Clearing a user's second factor requires --yes in non-interactive or JSON mode"
409-
.to_string(),
410-
));
411-
}
412-
413-
let mut stderr = std::io::stderr().lock();
414-
write!(
415-
stderr,
416-
"Clear two-factor authentication for '{}'? The account will be protected by its password alone. [y/N] ",
401+
let prompt = format!(
402+
"Clear two-factor authentication for '{}'? The account will be protected by its password alone. [y/N]",
417403
formatter.sanitize_text(access_key)
404+
);
405+
confirm(
406+
&Confirmation {
407+
prompt: &prompt,
408+
requires_yes: "Clearing a user's second factor requires --yes in non-interactive or JSON mode",
409+
declined: "Clearing two-factor authentication was declined",
410+
},
411+
yes,
412+
formatter,
418413
)
419-
.map_err(Error::Io)?;
420-
stderr.flush().map_err(Error::Io)?;
421-
422-
let mut answer = String::new();
423-
std::io::stdin()
424-
.lock()
425-
.read_line(&mut answer)
426-
.map_err(Error::Io)?;
427-
428-
if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") {
429-
Ok(())
430-
} else {
431-
Err(Error::Interrupted(
432-
"Clearing two-factor authentication was declined".to_string(),
433-
))
434-
}
435414
}
436415

437416
async fn execute_list(args: ListArgs, formatter: &Formatter) -> ExitCode {
@@ -733,7 +712,7 @@ mod tests {
733712
});
734713

735714
let error = confirm_mfa_reset("analyst", false, &formatter).expect_err("must refuse");
736-
assert!(matches!(error, Error::InvalidPath(_)), "{error:?}");
715+
assert!(matches!(error, rc_core::Error::InvalidPath(_)), "{error:?}");
737716
assert!(error.to_string().contains("--yes"), "{error}");
738717
}
739718

crates/cli/src/commands/replicate.rs

Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@ use rc_core::{AliasManager, Error, ObjectStore as _};
2020
use rc_s3::{AdminClient, S3Client};
2121
use serde::{Deserialize, Serialize};
2222
use std::collections::{BTreeMap, BTreeSet, HashMap};
23-
use std::io::{BufRead as _, IsTerminal as _, Write as _};
2423
use std::path::{Path, PathBuf};
2524

25+
use crate::confirm::{Confirmation, confirm};
2626
use crate::exit_code::ExitCode;
2727
use crate::output::{Formatter, OutputConfig};
2828

@@ -1988,35 +1988,15 @@ async fn execute_check(args: CheckArgs, output_config: OutputConfig) -> ExitCode
19881988
}
19891989

19901990
fn confirm_replication_check(yes: bool, formatter: &Formatter) -> rc_core::Result<()> {
1991-
if yes {
1992-
return Ok(());
1993-
}
1994-
if formatter.is_json() || !std::io::stdin().is_terminal() {
1995-
return Err(Error::InvalidPath(
1996-
"Replication check performs temporary remote writes and deletes; pass --yes in non-interactive or JSON mode"
1997-
.to_string(),
1998-
));
1999-
}
2000-
2001-
let mut stderr = std::io::stderr().lock();
2002-
write!(
2003-
stderr,
2004-
"Replication check writes and deletes a temporary object on every configured target. Continue? [y/N] "
1991+
confirm(
1992+
&Confirmation {
1993+
prompt: "Replication check writes and deletes a temporary object on every configured target. Continue? [y/N]",
1994+
requires_yes: "Replication check performs temporary remote writes and deletes; pass --yes in non-interactive or JSON mode",
1995+
declined: "Replication check was declined",
1996+
},
1997+
yes,
1998+
formatter,
20051999
)
2006-
.map_err(Error::Io)?;
2007-
stderr.flush().map_err(Error::Io)?;
2008-
let mut answer = String::new();
2009-
std::io::stdin()
2010-
.lock()
2011-
.read_line(&mut answer)
2012-
.map_err(Error::Io)?;
2013-
if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") {
2014-
Ok(())
2015-
} else {
2016-
Err(Error::Interrupted(
2017-
"Replication check was declined".to_string(),
2018-
))
2019-
}
20202000
}
20212001

20222002
fn output_replication_check(

crates/cli/src/confirm.rs

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
//! The yes/no prompt for an action worth asking about twice.
2+
//!
3+
//! Three commands had grown their own copy of this: deleting an OIDC provider,
4+
//! running a replication check that writes to every target, and clearing
5+
//! somebody's second factor. They differed only in their three strings, while
6+
//! agreeing on the parts that matter — that `--yes` skips the question, that a
7+
//! run with nobody to ask fails instead of assuming consent, and that anything
8+
//! other than `y`/`yes` is a decline rather than a default.
9+
//!
10+
//! Those are the rules a fourth copy would be most likely to get subtly wrong,
11+
//! so they live here once.
12+
13+
use std::io::{BufRead as _, IsTerminal as _, Write as _};
14+
15+
use rc_core::{Error, Result};
16+
17+
use crate::output::Formatter;
18+
19+
/// What to say while confirming one particular action.
20+
pub(crate) struct Confirmation<'a> {
21+
/// The question, ending in `[y/N]`.
22+
///
23+
/// Callers that interpolate a name are responsible for passing it through
24+
/// [`Formatter::sanitize_text`] first: it reaches a terminal from here with
25+
/// no further escaping.
26+
pub(crate) prompt: &'a str,
27+
/// Why `--yes` is required when there is no terminal to ask on.
28+
pub(crate) requires_yes: &'a str,
29+
/// Reported when the answer is anything but yes.
30+
pub(crate) declined: &'a str,
31+
}
32+
33+
/// Ask, unless `yes` was passed.
34+
///
35+
/// `Ok(())` means go ahead. A refusal is [`Error::Interrupted`]; a run that
36+
/// could not ask at all is [`Error::InvalidPath`], which is a usage problem and
37+
/// exits as one.
38+
pub(crate) fn confirm(request: &Confirmation<'_>, yes: bool, formatter: &Formatter) -> Result<()> {
39+
if yes {
40+
return Ok(());
41+
}
42+
// Refuse rather than proceed: a machine-readable run has nobody to answer,
43+
// and treating silence as consent is how a destructive command becomes a
44+
// surprise in someone's CI log.
45+
if formatter.is_json() || !std::io::stdin().is_terminal() {
46+
return Err(Error::InvalidPath(request.requires_yes.to_string()));
47+
}
48+
49+
// The question goes to stderr so stdout stays usable in a pipeline.
50+
let mut stderr = std::io::stderr().lock();
51+
write!(stderr, "{} ", request.prompt).map_err(Error::Io)?;
52+
stderr.flush().map_err(Error::Io)?;
53+
54+
let mut answer = String::new();
55+
std::io::stdin()
56+
.lock()
57+
.read_line(&mut answer)
58+
.map_err(Error::Io)?;
59+
60+
if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") {
61+
Ok(())
62+
} else {
63+
Err(Error::Interrupted(request.declined.to_string()))
64+
}
65+
}
66+
67+
#[cfg(test)]
68+
mod tests {
69+
use super::*;
70+
use crate::output::OutputConfig;
71+
72+
fn request() -> Confirmation<'static> {
73+
Confirmation {
74+
prompt: "Delete everything? [y/N]",
75+
requires_yes: "Deleting everything requires --yes in non-interactive or JSON mode",
76+
declined: "Deleting everything was declined",
77+
}
78+
}
79+
80+
fn formatter(json: bool) -> Formatter {
81+
Formatter::new(OutputConfig {
82+
json,
83+
no_color: true,
84+
..Default::default()
85+
})
86+
}
87+
88+
#[test]
89+
fn yes_skips_the_question_entirely() {
90+
// True even in JSON mode, where there would be nobody to ask.
91+
confirm(&request(), true, &formatter(true)).expect("--yes must be honoured");
92+
}
93+
94+
#[test]
95+
fn json_mode_refuses_instead_of_assuming_consent() {
96+
let error = confirm(&request(), false, &formatter(true)).expect_err("must refuse");
97+
98+
assert!(matches!(error, Error::InvalidPath(_)), "{error:?}");
99+
assert_eq!(error.exit_code(), 2, "a missing --yes is a usage error");
100+
assert!(error.to_string().contains("--yes"), "{error}");
101+
}
102+
103+
#[test]
104+
fn a_declined_answer_is_reported_as_an_interruption() {
105+
// Not asserted through the prompt, which needs a terminal: this pins the
106+
// contract the callers rely on for their exit code.
107+
let error = Error::Interrupted(request().declined.to_string());
108+
assert_eq!(error.exit_code(), 130);
109+
}
110+
}

crates/cli/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
//! This module exports the CLI components for use in integration tests.
44
55
pub mod commands;
6+
pub(crate) mod confirm;
67
pub mod exit_code;
78
pub mod output;
9+
pub(crate) mod private_file;
810
pub(crate) mod secret_input;

0 commit comments

Comments
 (0)