Remove NK_lock_device call from PasswordSafe::drop

When enabled, the password safe can be used without authentication.  The
lock device can be used to lock the password safe.  Currently,
PasswordSafe::drop calls this command to make sure that other
applications cannot access the password safe without authentication.

On the Nitrokey Storage, locking the device may also disable the
encrypted or hidden volume.  As using the password safe should not have
side effects on the storage volumes, this patch removes the call to the
lock device command from the Drop implementation.  Instead, the user
should call this method after making sure that it does not have side
effects.

A feature request for a command that only locks the password safe
without side effects is submitted to the Nitrokey Storage firmware
repository:
	https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65

3 files changed, 39 insertions, 14 deletions

diff --git a/TODO.md b/TODO.md
index e2fb3fe..b666ce3 100644
--- a/TODO.md
+++ b/TODO.md
@@ -35,6 +35,9 @@
 - Differentiate empty strings and errors (see `result_from_string`).
 - Check integer conversions.
 - Consider implementing `Into<CommandError>` for `(Device, CommandError)`
-- Check error handling in PasswordSafe::drop().
+- Lock password safe in `PasswordSafe::drop()` (see [nitrokey-storage-firmware
+  issue 65][]).
 - Disable creation of multiple password safes at the same time.
 - Check timing in Storage tests.
+
+[nitrokey-storage-firmware issue 65]: https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65
diff --git a/src/pws.rs b/src/pws.rs
index 85726c5..903ecef 100644
--- a/src/pws.rs
+++ b/src/pws.rs
@@ -17,27 +17,39 @@ pub const SLOT_COUNT: u8 = 16;
 /// the [`GetPasswordSafe`][] trait.  Note that the device must live at least as long as the
 /// password safe.
 ///
+/// Once the password safe has been unlocked, it can be accessed without a password.  Therefore it
+/// is mandatory to call [`lock`][] on the corresponding device after the password store is used.
+/// As this command may have side effects on the Nitrokey Storage, it cannot be called
+/// automatically once the password safe is destroyed.
+///
 /// # Examples
 ///
 /// Open a password safe and access a password:
 ///
 /// ```no_run
-/// use nitrokey::GetPasswordSafe;
+/// use nitrokey::{Device, GetPasswordSafe, PasswordSafe};
 /// # use nitrokey::CommandError;
 ///
+/// fn use_password_safe(pws: &PasswordSafe) -> Result<(), CommandError> {
+///     let name = pws.get_slot_name(0)?;
+///     let login = pws.get_slot_login(0)?;
+///     let password = pws.get_slot_login(0)?;
+///     println!("Credentials for {}: login {}, password {}", name, login, password);
+///     Ok(())
+/// }
+///
 /// # fn try_main() -> Result<(), CommandError> {
 /// let device = nitrokey::connect()?;
 /// let pws = device.get_password_safe("123456")?;
-/// let name = pws.get_slot_name(0)?;
-/// let login = pws.get_slot_login(0)?;
-/// let password = pws.get_slot_login(0)?;
-/// println!("Credentials for {}: login {}, password {}", name, login, password);
+/// use_password_safe(&pws);
+/// device.lock();
 /// #     Ok(())
 /// # }
 /// ```
 ///
 /// [`SLOT_COUNT`]: constant.SLOT_COUNT.html
 /// [`get_password_safe`]: trait.GetPasswordSafe.html#method.get_password_safe
+/// [`lock`]: trait.Device.html#method.lock
 /// [`GetPasswordSafe`]: trait.GetPasswordSafe.html
 pub struct PasswordSafe<'a> {
     _device: &'a Device,
@@ -50,9 +62,12 @@ pub struct PasswordSafe<'a> {
 ///
 /// [`PasswordSafe`]: struct.PasswordSafe.html
 pub trait GetPasswordSafe {
-    /// Enables and returns the password safe.  This method consumes the device.  You can go back
-    /// to the device using the [`device`][] method of the returned password safe.  If the method
-    /// fails, the current device will be returned as part of the error.
+    /// Enables and returns the password safe.
+    ///
+    /// The underlying device must always live at least as long as a password safe retrieved from
+    /// it.  It is mandatory to lock the underlying device using [`lock`][] after the password safe
+    /// has been used.  Otherwise, other applications can access the password store without
+    /// authentication.
     ///
     /// # Errors
     ///
@@ -62,7 +77,7 @@ pub trait GetPasswordSafe {
     /// # Example
     ///
     /// ```no_run
-    /// use nitrokey::{GetPasswordSafe, PasswordSafe};
+    /// use nitrokey::{Device, GetPasswordSafe, PasswordSafe};
     /// # use nitrokey::CommandError;
     ///
     /// fn use_password_safe(pws: &PasswordSafe) {}
@@ -70,7 +85,10 @@ pub trait GetPasswordSafe {
     /// # fn try_main() -> Result<(), CommandError> {
     /// let device = nitrokey::connect()?;
     /// match device.get_password_safe("123456") {
-    ///     Ok(pws) => use_password_safe(&pws),
+    ///     Ok(pws) => {
+    ///         use_password_safe(&pws);
+    ///         device.lock();
+    ///     },
     ///     Err(err) => println!("Could not open the password safe: {:?}", err),
     /// };
     /// #     Ok(())
@@ -78,6 +96,7 @@ pub trait GetPasswordSafe {
     /// ```
     ///
     /// [`device`]: struct.PasswordSafe.html#method.device
+    /// [`lock`]: trait.Device.html#method.lock
     /// [`InvalidString`]: enum.CommandError.html#variant.InvalidString
     /// [`WrongPassword`]: enum.CommandError.html#variant.WrongPassword
     fn get_password_safe(&self, user_pin: &str) -> Result<PasswordSafe, CommandError>;
@@ -317,9 +336,8 @@ impl<'a> PasswordSafe<'a> {
 
 impl<'a> Drop for PasswordSafe<'a> {
     fn drop(&mut self) {
-        unsafe {
-            nitrokey_sys::NK_lock_device();
-        }
+        // TODO: disable the password safe -- NK_lock_device has side effects on the Nitrokey
+        // Storage, see https://github.com/Nitrokey/nitrokey-storage-firmware/issues/65
     }
 }
 
diff --git a/src/tests/pws.rs b/src/tests/pws.rs
index d6125a9..02e33cd 100644
--- a/src/tests/pws.rs
+++ b/src/tests/pws.rs
@@ -1,3 +1,4 @@
+use device::Device;
 use nitrokey_sys;
 use pws::{GetPasswordSafe, PasswordSafe, SLOT_COUNT};
 use tests::util::{Target, ADMIN_PASSWORD, USER_PASSWORD};
@@ -36,6 +37,9 @@ fn drop() {
         assert_eq!(Ok(String::from("name")), result);
     }
     let result = result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_name(1) });
+    assert_eq!(Ok(String::from("name")), result);
+    device.lock();
+    let result = result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_name(1) });
     assert_eq!(Err(CommandError::NotAuthorized), result);
 }