Remove pws set subcommand

This patch removes the pws set subcommand as part of the PWS interface
refactoring. Users should use pws add (for writing to new slots) or pws
update (for writing to existing slots) instead. This makes it less
likely to accidentally overwrite PWS data.

CHANGELOG.md

@@ -4,6 +4,7 @@ Unreleased
 - Changed error reporting format to make up only a single line
 - Added the `pws add` subcommand to write to a new slot
 - Added the `pws update` subcommand to update an existing PWS slot
+- Removed the `pws set` subcommand
 - Added the `--only-aes-key` option to the `reset` command to build a new AES
  key without performing a factory reset
 - Added `NITROCLI_RESOLVED_USB_PATH` environment variable to be used by

doc/nitrocli.1

@@ -1,4 +1,4 @@
-.TH NITROCLI 1 2021-04-18
+.TH NITROCLI 1 2021-04-20
 .SH NAME
 nitrocli \- access Nitrokey devices
 .SH SYNOPSIS
@@ -273,12 +273,6 @@ The fields are printed together with a label.
 Use the \fB\-\-quiet\fR option to suppress the labels and to only output the
 values stored in the PWS slot.
 .TP
-\fBnitrocli pws set \fIslot name login password\fR
-Set the content of a PWS slot.
-\fIslot\fR is the number of the slot to write.
-\fIname\fR, \fIlogin\fR, and \fIpassword\fR represent the data to write to the
-slot.
-.TP
 \fBnitrocli pws add \fR[\fB\-s\fR|\fB\-\-slot \fIslot\fR] \
 \fIname login password\fR
 Add a new PWS slot.
@@ -540,7 +534,8 @@ Change the configuration:
 
 .SS Password safe
 Configure a PWS slot:
- $ \fBnitrocli pws set 0 example.org john.doe passw0rd\fR
+ $ \fBnitrocli pws add example.org john.doe passw0rd\fR
+ Added PWS slot 0
 
 Get the data from a slot:
  $ \fBnitrocli pws get 0\fR

src/args.rs

@@ -394,10 +394,6 @@ Command! {PwsCommand, [
  Get(PwsGetArgs) => |ctx, args: PwsGetArgs| {
  crate::commands::pws_get(ctx, args.slot, args.name, args.login, args.password, args.quiet)
  },
- /// Writes a password safe slot
- Set(PwsSetArgs) => |ctx, args: PwsSetArgs| {
- crate::commands::pws_set(ctx, args.slot, &args.name, &args.login, &args.password)
- },
  /// Adds a new password safe slot
  Add(PwsAddArgs) => |ctx, args: PwsAddArgs| {
  crate::commands::pws_add(ctx, &args.name, &args.login, &args.password, args.slot)
@@ -440,18 +436,6 @@ pub struct PwsGetArgs {
  pub slot: u8,
 }
 
-#[derive(Debug, PartialEq, structopt::StructOpt)]
-pub struct PwsSetArgs {
- /// The PWS slot to write
- pub slot: u8,
- /// The name to store on the slot
- pub name: String,
- /// The login to store on the slot
- pub login: String,
- /// The password to store on the slot
- pub password: String,
-}
-
 #[derive(Debug, PartialEq, structopt::StructOpt)]
 pub struct PwsAddArgs {
  /// The name to store on the slot

src/commands.rs

@@ -1032,21 +1032,6 @@ pub fn pws_get(
  })
 }
 
-/// Write a PWS slot.
-pub fn pws_set(
- ctx: &mut Context<'_>,
- slot: u8,
- name: &str,
- login: &str,
- password: &str,
-) -> anyhow::Result<()> {
- with_password_safe(ctx, |_ctx, mut pws| {
- pws
- .write_slot(slot, name, login, password)
- .context("Failed to write PWS slot")
- })
-}
-
 /// Add a new PWS slot.
 pub fn pws_add(
  ctx: &mut Context<'_>,

src/tests/pws.rs

@@ -3,8 +3,26 @@
 // Copyright (C) 2019-2021 The Nitrocli Developers
 // SPDX-License-Identifier: GPL-3.0-or-later
 
+use nitrokey::GetPasswordSafe as _;
+
 use super::*;
 
+fn clear_pws(model: nitrokey::Model) -> anyhow::Result<()> {
+ let mut manager = nitrokey::force_take()?;
+ let mut device = manager.connect_model(model)?;
+ let mut pws = device.get_password_safe(nitrokey::DEFAULT_USER_PIN)?;
+ let slots_to_clear = pws
+ .get_slots()?
+ .into_iter()
+ .flatten()
+ .map(|s| s.index())
+ .collect::<Vec<_>>();
+ for slot in slots_to_clear {
+ pws.erase_slot(slot)?;
+ }
+ Ok(())
+}
+
 fn assert_slot(
  model: nitrokey::Model,
  slot: u8,
@@ -18,17 +36,6 @@ fn assert_slot(
  Ok(())
 }
 
-#[test_device]
-fn set_invalid_slot(model: nitrokey::Model) {
- let err = Nitrocli::new()
- .model(model)
- .handle(&["pws", "set", "100", "name", "login", "1234"])
- .unwrap_err()
- .to_string();
-
- assert_eq!(err, "Failed to write PWS slot");
-}
-
 #[test_device]
 fn add_invalid_slot(model: nitrokey::Model) {
  let err = Nitrocli::new()
@@ -48,24 +55,28 @@ fn status(model: nitrokey::Model) -> anyhow::Result<()> {
  )
  .unwrap();
 
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
  // Make sure that we have at least something to display by ensuring
  // that there are there is one slot programmed.
- let _ = ncli.handle(&["pws", "set", "0", "the-name", "the-login", "123456"])?;
+ let _ = ncli.handle(&["pws", "add", "the-name", "the-login", "123456"])?;
 
  let out = ncli.handle(&["pws", "status"])?;
  assert!(re.is_match(&out), "{}", out);
  Ok(())
 }
 
 #[test_device]
-fn set_get(model: nitrokey::Model) -> anyhow::Result<()> {
+fn add_get(model: nitrokey::Model) -> anyhow::Result<()> {
  const NAME: &str = "dropbox";
  const LOGIN: &str = "d-e-s-o";
  const PASSWORD: &str = "my-secret-password";
 
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
- let _ = ncli.handle(&["pws", "set", "1", &NAME, &LOGIN, &PASSWORD])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "1", &NAME, &LOGIN, &PASSWORD])?;
 
  let out = ncli.handle(&["pws", "get", "1", "--quiet", "--name"])?;
  assert_eq!(out, format!("{}\n", NAME));
@@ -90,9 +101,11 @@ fn set_get(model: nitrokey::Model) -> anyhow::Result<()> {
 }
 
 #[test_device]
-fn set_empty(model: nitrokey::Model) -> anyhow::Result<()> {
+fn add_empty(model: nitrokey::Model) -> anyhow::Result<()> {
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
- let _ = ncli.handle(&["pws", "set", "1", "", "", ""])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "1", "", "", ""])?;
 
  let out = ncli.handle(&["pws", "get", "1", "--quiet", "--name"])?;
  assert_eq!(out, "\n");
@@ -111,13 +124,15 @@ fn set_empty(model: nitrokey::Model) -> anyhow::Result<()> {
 }
 
 #[test_device]
-fn set_reset_get(model: nitrokey::Model) -> anyhow::Result<()> {
+fn add_reset_get(model: nitrokey::Model) -> anyhow::Result<()> {
  const NAME: &str = "some/svc";
  const LOGIN: &str = "a\\user";
  const PASSWORD: &str = "!@&-)*(&+%^@";
 
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
- let _ = ncli.handle(&["pws", "set", "2", &NAME, &LOGIN, &PASSWORD])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "2", &NAME, &LOGIN, &PASSWORD])?;
 
  let out = ncli.handle(&["reset"])?;
  assert_eq!(out, "");
@@ -130,8 +145,19 @@ fn set_reset_get(model: nitrokey::Model) -> anyhow::Result<()> {
 
 #[test_device]
 fn clear(model: nitrokey::Model) -> anyhow::Result<()> {
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
- let _ = ncli.handle(&["pws", "set", "10", "clear-test", "some-login", "abcdef"])?;
+ let _ = ncli.handle(&["pws", "clear", "10"])?;
+ let _ = ncli.handle(&[
+ "pws",
+ "add",
+ "--slot",
+ "10",
+ "clear-test",
+ "some-login",
+ "abcdef",
+ ])?;
  let _ = ncli.handle(&["pws", "clear", "10"])?;
  let res = ncli.handle(&["pws", "get", "10"]);
 
@@ -142,9 +168,9 @@ fn clear(model: nitrokey::Model) -> anyhow::Result<()> {
 
 #[test_device]
 fn update_unprogrammed(model: nitrokey::Model) -> anyhow::Result<()> {
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
- let _ = ncli.handle(&["pws", "set", "10", "clear-test", "some-login", "abcdef"])?;
- let _ = ncli.handle(&["pws", "clear", "10"])?;
  let res = ncli.handle(&["pws", "update", "10", "--name", "test"]);
 
  let err = res.unwrap_err().to_string();
@@ -174,10 +200,13 @@ fn update(model: nitrokey::Model) -> anyhow::Result<()> {
  const PASSWORD_BEFORE: &str = "password-before";
  const PASSWORD_AFTER: &str = "password-after";
 
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
  let _ = ncli.handle(&[
  "pws",
- "set",
+ "add",
+ "--slot",
  "10",
  NAME_BEFORE,
  LOGIN_BEFORE,
@@ -213,20 +242,17 @@ fn update(model: nitrokey::Model) -> anyhow::Result<()> {
 
 #[test_device]
 fn add_full(model: nitrokey::Model) -> anyhow::Result<()> {
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
 
  // Fill all PWS slots
- for slot in u8::MIN..=u8::MAX {
- let res = ncli.handle(&["pws", "set", &slot.to_string(), "name", "login", "passw0rd"]);
- match res {
- Ok(_) => {}
- Err(err) => match err.downcast::<nitrokey::Error>() {
- Ok(err) => match err {
- nitrokey::Error::LibraryError(nitrokey::LibraryError::InvalidSlot) => break,
- err => anyhow::bail!(err),
- },
- Err(err) => anyhow::bail!(err),
- },
+ {
+ let mut manager = nitrokey::force_take()?;
+ let mut device = manager.connect_model(model)?;
+ let mut pws = device.get_password_safe(nitrokey::DEFAULT_USER_PIN)?;
+ for slot in 0..pws.get_slot_count() {
+ pws.write_slot(slot, "name", "login", "passw0rd")?;
  }
  }
 
@@ -240,10 +266,12 @@ fn add_full(model: nitrokey::Model) -> anyhow::Result<()> {
 
 #[test_device]
 fn add_existing(model: nitrokey::Model) -> anyhow::Result<()> {
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
 
  // Fill slot 0
- let _ = ncli.handle(&["pws", "set", "0", "name0", "login0", "pass0rd"])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "0", "name0", "login0", "pass0rd"])?;
 
  // Try to add slot 0
  let res = ncli.handle(&["pws", "add", "--slot", "0", "name", "login", "passw0rd"]);
@@ -255,14 +283,13 @@ fn add_existing(model: nitrokey::Model) -> anyhow::Result<()> {
 
 #[test_device]
 fn add_slot(model: nitrokey::Model) -> anyhow::Result<()> {
+ clear_pws(model)?;
+
  let mut ncli = Nitrocli::new().model(model);
 
  // Fill slots 0 and 5
- let _ = ncli.handle(&["pws", "set", "0", "name0", "login0", "passw0rd"])?;
- let _ = ncli.handle(&["pws", "set", "5", "name5", "login5", "passw5rd"])?;
-
- // Clear slot 1 (in case it was written to by other slots)
- let _ = ncli.handle(&["pws", "clear", "1"])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "0", "name0", "login0", "passw0rd"])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "5", "name5", "login5", "passw5rd"])?;
 
  // Try to add slot 1
  let out = ncli.handle(&["pws", "add", "--slot", "1", "name1", "login1", "passw1rd"])?;
@@ -277,26 +304,13 @@ fn add_slot(model: nitrokey::Model) -> anyhow::Result<()> {
 
 #[test_device]
 fn add(model: nitrokey::Model) -> anyhow::Result<()> {
- let mut ncli = Nitrocli::new().model(model);
+ clear_pws(model)?;
 
- // Clear all PWS slots
- for slot in u8::MIN..=u8::MAX {
- let res = ncli.handle(&["pws", "clear", &slot.to_string()]);
- match res {
- Ok(_) => {}
- Err(err) => match err.downcast::<nitrokey::Error>() {
- Ok(err) => match err {
- nitrokey::Error::LibraryError(nitrokey::LibraryError::InvalidSlot) => break,
- err => anyhow::bail!(err),
- },
- Err(err) => anyhow::bail!(err),
- },
- }
- }
+ let mut ncli = Nitrocli::new().model(model);
 
  // Fill slots 0 and 5
- let _ = ncli.handle(&["pws", "set", "0", "name0", "login0", "pass0rd"])?;
- let _ = ncli.handle(&["pws", "set", "5", "name5", "login5", "pass5rd"])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "0", "name0", "login0", "pass0rd"])?;
+ let _ = ncli.handle(&["pws", "add", "--slot", "5", "name5", "login5", "pass5rd"])?;
 
  // Try to add another one
  let out = ncli.handle(&["pws", "add", "name1", "login1", "passw1rd"])?;

src/tests/run.rs

@@ -75,7 +75,8 @@ fn help_options() {
  test(&["pws"]);
  test(&["pws", "clear"]);
  test(&["pws", "get"]);
- test(&["pws", "set"]);
+ test(&["pws", "add"]);
+ test(&["pws", "update"]);
  test(&["pws", "status"]);
  test(&["reset"]);
  test(&["status"]);