fix: propagate credential store errors instead of erasing them into false/absent

.cargo/config.toml

@@ -1,3 +1,9 @@
 [target.aarch64-unknown-linux-musl]
 linker = "aarch64-linux-musl-gcc"
 rustflags = ["-C", "target-feature=-crt-static"]
+
+# `cargo test` builds this crate as a standalone test binary, which cannot
+# resolve the `napi_*` symbols that the Node process provides at load time for
+# the cdylib. Deferring those symbols to load time lets unit tests link.
+[target.'cfg(target_os = "macos")']
+rustflags = ["-C", "link-arg=-Wl,-undefined,dynamic_lookup"]

__test__/index.spec.ts

@@ -8,6 +8,8 @@ const testPassword = 'napi.rs'
 const testService = 'keyring-node-test-service'
 const testUser = 'test-user'
 const testSecret = new Uint8Array([0x68, 0x65, 0x6c, 0x6c, 0x6f]) // "hello" in bytes
+const testDeleteUser = 'test-delete-user'
+const testMissingSecretUser = 'test-missing-secret-user'
 
 test('Should create and get password back', (t) => {
   const entry = new Entry(testService, testUser)
@@ -72,6 +74,45 @@ test('Should handle binary data correctly with setSecret/getSecret async', async
   await t.notThrowsAsync(() => entry.deleteCredential())
 })
 
+test('deleteCredential should report whether a credential was removed', (t) => {
+  const entry = new Entry(testService, testDeleteUser)
+  t.notThrows(() => entry.setPassword(testPassword))
+  t.true(entry.deleteCredential(), 'deleting an existing credential reports true')
+  t.false(entry.deleteCredential(), 'deleting a missing credential reports false')
+})
+
+test('deleteCredential should report whether a credential was removed async', async (t) => {
+  const entry = new AsyncEntry(testService, testDeleteUser)
+  await t.notThrowsAsync(() => entry.setPassword(testPassword))
+  t.true(await entry.deleteCredential(), 'deleting an existing credential resolves true')
+  t.false(await entry.deleteCredential(), 'deleting a missing credential resolves false')
+})
+
+test('deletePassword should behave like deleteCredential', (t) => {
+  const entry = new Entry(testService, testDeleteUser)
+  t.notThrows(() => entry.setPassword(testPassword))
+  t.true(entry.deletePassword())
+  t.false(entry.deletePassword())
+})
+
+test('deletePassword should behave like deleteCredential async', async (t) => {
+  const entry = new AsyncEntry(testService, testDeleteUser)
+  await t.notThrowsAsync(() => entry.setPassword(testPassword))
+  t.true(await entry.deletePassword())
+  t.false(await entry.deletePassword())
+})
+
+test('Should return no secret value when the entry is missing async', async (t) => {
+  const entry = new AsyncEntry(testService, testMissingSecretUser)
+  // These resolve `null` at runtime while being declared as `undefined`.
+  t.is((await entry.getSecret()) ?? null, null)
+})
+
+test('Should return no secret value when the entry is missing', (t) => {
+  const entry = new Entry(testService, testMissingSecretUser)
+  t.is(entry.getSecret(), null)
+})
+
 let testTarget: string | undefined
 
 const platform = os.platform()

index.d.ts

@@ -48,9 +48,12 @@ export declare class AsyncEntry {
   /**
    * Retrieve the secret saved for this entry.
    *
-   * Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+   * Returns no secret if there isn't one.
    *
-   * Can return an [Ambiguous](Error::Ambiguous) error
+   * Rejects if the credential store cannot be read, for example when it is
+   * locked or inaccessible.
+   *
+   * Can reject with an [Ambiguous](Error::Ambiguous) error
    * if there is more than one platform credential
    * that matches this entry.  This can only happen
    * on some platforms, and then only if a third-party
@@ -60,9 +63,15 @@ export declare class AsyncEntry {
   /**
    * Delete the underlying credential for this entry.
    *
-   * Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+   * Resolves `true` if a credential was deleted, and `false` if there was no
+   * credential to delete.
    *
-   * Can return an [Ambiguous](Error::Ambiguous) error
+   * Rejects if the credential exists but could not be deleted, for example
+   * when the store is locked or inaccessible. A failed deletion is never
+   * reported as `false`, so a `false` result always means the credential is
+   * absent from the store.
+   *
+   * Can reject with an [Ambiguous](Error::Ambiguous) error
    * if there is more than one platform credential
    * that matches this entry.  This can only happen
    * on some platforms, and then only if a third-party
@@ -74,7 +83,7 @@ export declare class AsyncEntry {
    */
   deleteCredential(signal?: AbortSignal | undefined | null): Promise<boolean>
   /** Alias for `deleteCredential` */
-  deletePassword(signal?: AbortSignal | undefined | null): Promise<unknown>
+  deletePassword(signal?: AbortSignal | undefined | null): Promise<boolean>
 }
 
 export declare class Entry {
@@ -125,9 +134,12 @@ export declare class Entry {
   /**
    * Retrieve the secret saved for this entry.
    *
-   * Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+   * Returns no secret if there isn't one.
    *
-   * Can return an [Ambiguous](Error::Ambiguous) error
+   * Throws if the credential store cannot be read, for example when it is
+   * locked or inaccessible.
+   *
+   * Can throw an [Ambiguous](Error::Ambiguous) error
    * if there is more than one platform credential
    * that matches this entry.  This can only happen
    * on some platforms, and then only if a third-party
@@ -137,9 +149,15 @@ export declare class Entry {
   /**
    * Delete the underlying credential for this entry.
    *
-   * Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+   * Returns `true` if a credential was deleted, and `false` if there was no
+   * credential to delete.
    *
-   * Can return an [Ambiguous](Error::Ambiguous) error
+   * Throws if the credential exists but could not be deleted, for example
+   * when the store is locked or inaccessible. A failed deletion is never
+   * reported as `false`, so a `false` result always means the credential is
+   * absent from the store.
+   *
+   * Can throw an [Ambiguous](Error::Ambiguous) error
    * if there is more than one platform credential
    * that matches this entry.  This can only happen
    * on some platforms, and then only if a third-party

src/async_entry.rs

@@ -3,9 +3,9 @@ use std::sync::Arc;
 use napi::bindgen_prelude::*;
 use napi_derive::napi;
 
-use crate::entry::into_optional_password;
 #[cfg(target_os = "linux")]
 use crate::linux_credential_builder::LinuxCredentialBuilder;
+use crate::result::{into_deleted, into_optional};
 
 #[napi]
 pub struct AsyncEntry {
@@ -181,9 +181,12 @@ impl AsyncEntry {
   #[napi(ts_return_type = "Promise<Uint8Array | undefined>")]
   /// Retrieve the secret saved for this entry.
   ///
-  /// Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+  /// Returns no secret if there isn't one.
   ///
-  /// Can return an [Ambiguous](Error::Ambiguous) error
+  /// Rejects if the credential store cannot be read, for example when it is
+  /// locked or inaccessible.
+  ///
+  /// Can reject with an [Ambiguous](Error::Ambiguous) error
   /// if there is more than one platform credential
   /// that matches this entry.  This can only happen
   /// on some platforms, and then only if a third-party
@@ -200,9 +203,15 @@ impl AsyncEntry {
   #[napi(ts_return_type = "Promise<boolean>")]
   /// Delete the underlying credential for this entry.
   ///
-  /// Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+  /// Resolves `true` if a credential was deleted, and `false` if there was no
+  /// credential to delete.
   ///
-  /// Can return an [Ambiguous](Error::Ambiguous) error
+  /// Rejects if the credential exists but could not be deleted, for example
+  /// when the store is locked or inaccessible. A failed deletion is never
+  /// reported as `false`, so a `false` result always means the credential is
+  /// absent from the store.
+  ///
+  /// Can reject with an [Ambiguous](Error::Ambiguous) error
   /// if there is more than one platform credential
   /// that matches this entry.  This can only happen
   /// on some platforms, and then only if a third-party
@@ -221,7 +230,7 @@ impl AsyncEntry {
     )
   }
 
-  #[napi]
+  #[napi(ts_return_type = "Promise<boolean>")]
   /// Alias for `deleteCredential`
   pub fn delete_password(&self, signal: Option<AbortSignal>) -> AsyncTask<EntryTask> {
     self.delete_credential(signal)
@@ -251,7 +260,7 @@ impl Task for PasswordTask {
   type JsValue = Option<String>;
 
   fn compute(&mut self) -> Result<Self::Output> {
-    into_optional_password(self.inner.get_password())
+    into_optional(self.inner.get_password())
   }
 
   fn resolve(&mut self, _env: Env, output: Self::Output) -> Result<Self::JsValue> {
@@ -270,7 +279,7 @@ impl Task for SecretTask {
   type JsValue = Option<Vec<u8>>;
 
   fn compute(&mut self) -> Result<Self::Output> {
-    Ok(self.inner.get_secret().ok())
+    into_optional(self.inner.get_secret())
   }
 
   fn resolve(&mut self, _env: Env, output: Self::Output) -> Result<Self::JsValue> {
@@ -286,7 +295,7 @@ impl Task for EntryTask {
 
   fn compute(&mut self) -> Result<Self::Output> {
     match self.kind {
-      TaskKind::DeleteCredential => Ok(Some(self.inner.delete_credential().is_ok())),
+      TaskKind::DeleteCredential => into_deleted(self.inner.delete_credential()).map(Some),
       TaskKind::SetPassword(ref password) => {
         self
           .inner

src/entry.rs

@@ -3,16 +3,7 @@ use napi_derive::napi;
 
 #[cfg(target_os = "linux")]
 use crate::linux_credential_builder::LinuxCredentialBuilder;
-
-pub(crate) fn into_optional_password(
-  result: keyring_core::Result<String>,
-) -> Result<Option<String>> {
-  match result {
-    Ok(password) => Ok(Some(password)),
-    Err(keyring_core::Error::NoEntry) => Ok(None),
-    Err(error) => Err(anyhow::Error::from(error).into()),
-  }
-}
+use crate::result::{into_deleted, into_optional};
 
 #[napi]
 pub struct Entry {
@@ -164,29 +155,38 @@ impl Entry {
   /// on some platforms, and then only if a third-party
   /// application wrote the ambiguous credential.
   pub fn get_password(&self) -> Result<Option<String>> {
-    into_optional_password(self.inner.get_password())
+    into_optional(self.inner.get_password())
   }
 
   #[napi]
   /// Retrieve the secret saved for this entry.
   ///
-  /// Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+  /// Returns no secret if there isn't one.
   ///
-  /// Can return an [Ambiguous](Error::Ambiguous) error
+  /// Throws if the credential store cannot be read, for example when it is
+  /// locked or inaccessible.
+  ///
+  /// Can throw an [Ambiguous](Error::Ambiguous) error
   /// if there is more than one platform credential
   /// that matches this entry.  This can only happen
   /// on some platforms, and then only if a third-party
   /// application wrote the ambiguous credential.
-  pub fn get_secret(&self) -> Option<Vec<u8>> {
-    self.inner.get_secret().ok()
+  pub fn get_secret(&self) -> Result<Option<Vec<u8>>> {
+    into_optional(self.inner.get_secret())
   }
 
   #[napi]
   /// Delete the underlying credential for this entry.
   ///
-  /// Returns a [NoEntry](Error::NoEntry) error if there isn't one.
+  /// Returns `true` if a credential was deleted, and `false` if there was no
+  /// credential to delete.
   ///
-  /// Can return an [Ambiguous](Error::Ambiguous) error
+  /// Throws if the credential exists but could not be deleted, for example
+  /// when the store is locked or inaccessible. A failed deletion is never
+  /// reported as `false`, so a `false` result always means the credential is
+  /// absent from the store.
+  ///
+  /// Can throw an [Ambiguous](Error::Ambiguous) error
   /// if there is more than one platform credential
   /// that matches this entry.  This can only happen
   /// on some platforms, and then only if a third-party
@@ -195,51 +195,17 @@ impl Entry {
   /// Note: This does _not_ affect the lifetime of the [Entry]
   /// structure, which is controlled by Rust.  It only
   /// affects the underlying credential store.
-  pub fn delete_credential(&self) -> bool {
-    self
-      .inner
-      .delete_credential()
-      .map_err(anyhow::Error::from)
-      .is_ok()
+  pub fn delete_credential(&self) -> Result<bool> {
+    into_deleted(self.inner.delete_credential())
   }
 
   #[napi]
   /// Alias for `deleteCredential`
-  pub fn delete_password(&self) -> bool {
+  pub fn delete_password(&self) -> Result<bool> {
     self.delete_credential()
   }
 }
 
-#[cfg(test)]
-mod tests {
-  use super::into_optional_password;
-
-  #[test]
-  fn returns_password_when_found() {
-    assert_eq!(
-      into_optional_password(Ok("password".to_string())).unwrap(),
-      Some("password".to_string())
-    );
-  }
-
-  #[test]
-  fn returns_none_when_password_is_missing() {
-    assert_eq!(
-      into_optional_password(Err(keyring_core::Error::NoEntry)).unwrap(),
-      None
-    );
-  }
-
-  #[test]
-  fn preserves_non_missing_errors() {
-    let result = into_optional_password(Err(keyring_core::Error::NoStorageAccess(Box::new(
-      std::io::Error::other("keychain is locked"),
-    ))));
-
-    assert!(result.is_err());
-  }
-}
-
 #[napi(object)]
 pub struct Credential {
   pub account: String,

src/lib.rs

@@ -5,3 +5,4 @@ pub mod entry;
 
 #[cfg(target_os = "linux")]
 mod linux_credential_builder;
+mod result;