Repository: woodruffw/pyrage (language: python)

Issue:
Better/more precise exceptions
Right now, every error is a `ValueError`. For a stable release, we should have specific `EncryptError`, `DecryptError`, and so forth exception types.


Discussion / hints:

Fix patch:
diff --git a/src/lib.rs b/src/lib.rs
index e2cdde8..8ad563f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -2,10 +2,14 @@
 
 use std::io::{Read, Write};
 
-use age::{DecryptError, EncryptError, Encryptor, Identity, Recipient};
+use age::{
+    DecryptError as RageDecryptError, EncryptError as RageEncryptError, Encryptor, Identity,
+    Recipient,
+};
 use age_core::format::{FileKey, Stanza};
 use pyo3::{
-    exceptions::{PyTypeError, PyValueError},
+    create_exception,
+    exceptions::{PyException, PyTypeError},
     prelude::*,
     py_run,
     types::PyBytes,
@@ -15,6 +19,11 @@ mod passphrase;
 mod ssh;
 mod x25519;
 
+// These exceptions are raised by the `pyrage.ssh` and `pyrage.x25519` APIs,
+// where appropriate.
+create_exception!(pyrage, RecipientError, PyException);
+create_exception!(pyrage, IdentityError, PyException);
+
 // This is a wrapper trait for age's `Recipient`, providing trait downcasting.
 //
 // We need this so that we can pass multiple different types of recipients
@@ -39,7 +48,7 @@ macro_rules! recipient_traits {
     ($($t:ty),+) => {
         $(
             impl Recipient for $t {
-                fn wrap_file_key(&self, file_key: &FileKey) -> Result<Vec<Stanza>, EncryptError> {
+                fn wrap_file_key(&self, file_key: &FileKey) -> Result<Vec<Stanza>, RageEncryptError> {
                     self.0.wrap_file_key(file_key)
                 }
             }
@@ -63,7 +72,7 @@ macro_rules! identity_traits {
     ($($t:ty),+) => {
         $(
             impl Identity for $t {
-                fn unwrap_stanza(&self, stanza: &Stanza) -> Option<Result<FileKey, DecryptError>> {
+                fn unwrap_stanza(&self, stanza: &Stanza) -> Option<Result<FileKey, RageDecryptError>> {
                     self.0.unwrap_stanza(stanza)
                 }
             }
@@ -116,6 +125,8 @@ impl<'source> FromPyObject<'source> for Box<dyn PyrageIdentity> {
     }
 }
 
+create_exception!(pyrage, EncryptError, PyException);
+
 #[pyfunction]
 fn encrypt<'p>(
     py: Python<'p>,
@@ -126,23 +137,24 @@ fn encrypt<'p>(
     // is what the underlying `age` API expects.
     let recipients = recipients.into_iter().map(|pr| pr.as_recipient()).collect();
 
-    // TODO: More specific exceptions here, rather than ValueError for everything.
     let encryptor = Encryptor::with_recipients(recipients);
     let mut encrypted = vec![];
     let mut writer = encryptor
         .wrap_output(&mut encrypted)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| EncryptError::new_err(e.to_string()))?;
     writer
         .write_all(plaintext)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| EncryptError::new_err(e.to_string()))?;
     writer
         .finish()
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| EncryptError::new_err(e.to_string()))?;
 
     // TODO: Avoid this copy. Maybe PyBytes::new_with?
     Ok(PyBytes::new(py, &encrypted))
 }
 
+create_exception!(pyrage, DecryptError, PyException);
+
 #[pyfunction]
 fn decrypt<'p>(
     py: Python<'p>,
@@ -152,10 +164,10 @@ fn decrypt<'p>(
     let identities = identities.iter().map(|pi| pi.as_ref().as_identity());
 
     let decryptor =
-        match age::Decryptor::new(ciphertext).map_err(|e| PyValueError::new_err(e.to_string()))? {
+        match age::Decryptor::new(ciphertext).map_err(|e| DecryptError::new_err(e.to_string()))? {
             age::Decryptor::Recipients(d) => d,
             age::Decryptor::Passphrase(_) => {
-                return Err(PyValueError::new_err(
+                return Err(DecryptError::new_err(
                     "invalid ciphertext (encrypted with passphrase, not identities)",
                 ))
             }
@@ -164,10 +176,10 @@ fn decrypt<'p>(
     let mut decrypted = vec![];
     let mut reader = decryptor
         .decrypt(identities)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| DecryptError::new_err(e.to_string()))?;
     reader
         .read_to_end(&mut decrypted)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| DecryptError::new_err(e.to_string()))?;
 
     // TODO: Avoid this copy. Maybe PyBytes::new_with?
     Ok(PyBytes::new(py, &decrypted))
@@ -198,7 +210,12 @@ fn pyrage(py: Python, m: &PyModule) -> PyResult<()> {
     );
     m.add_submodule(passphrase)?;
 
+    m.add("IdentityError", py.get_type::<IdentityError>())?;
+    m.add("RecipientError", py.get_type::<RecipientError>())?;
+
+    m.add("EncryptError", py.get_type::<EncryptError>())?;
     m.add_wrapped(wrap_pyfunction!(encrypt))?;
+    m.add("DecryptError", py.get_type::<DecryptError>())?;
     m.add_wrapped(wrap_pyfunction!(decrypt))?;
 
     Ok(())
diff --git a/src/passphrase.rs b/src/passphrase.rs
index 157a5d5..49bfd71 100644
--- a/src/passphrase.rs
+++ b/src/passphrase.rs
@@ -1,7 +1,9 @@
 use std::io::{Read, Write};
 
 use age::{secrecy::Secret, Decryptor, Encryptor};
-use pyo3::{exceptions::PyValueError, prelude::*, types::PyBytes};
+use pyo3::{prelude::*, types::PyBytes};
+
+use crate::{DecryptError, EncryptError};
 
 #[pyfunction]
 fn encrypt<'p>(py: Python<'p>, plaintext: &[u8], passphrase: &str) -> PyResult<&'p PyBytes> {
@@ -9,13 +11,13 @@ fn encrypt<'p>(py: Python<'p>, plaintext: &[u8], passphrase: &str) -> PyResult<&
     let mut encrypted = vec![];
     let mut writer = encryptor
         .wrap_output(&mut encrypted)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| EncryptError::new_err(e.to_string()))?;
     writer
         .write_all(plaintext)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| EncryptError::new_err(e.to_string()))?;
     writer
         .finish()
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| EncryptError::new_err(e.to_string()))?;
 
     Ok(PyBytes::new(py, &encrypted))
 }
@@ -23,10 +25,10 @@ fn encrypt<'p>(py: Python<'p>, plaintext: &[u8], passphrase: &str) -> PyResult<&
 #[pyfunction]
 fn decrypt<'p>(py: Python<'p>, ciphertext: &[u8], passphrase: &str) -> PyResult<&'p PyBytes> {
     let decryptor =
-        match Decryptor::new(ciphertext).map_err(|e| PyValueError::new_err(e.to_string()))? {
+        match Decryptor::new(ciphertext).map_err(|e| DecryptError::new_err(e.to_string()))? {
             Decryptor::Passphrase(d) => d,
             _ => {
-                return Err(PyValueError::new_err(
+                return Err(DecryptError::new_err(
                     "invalid ciphertext (not passphrase encrypted)",
                 ))
             }
@@ -34,10 +36,10 @@ fn decrypt<'p>(py: Python<'p>, ciphertext: &[u8], passphrase: &str) -> PyResult<
     let mut decrypted = vec![];
     let mut reader = decryptor
         .decrypt(&Secret::new(passphrase.into()), None)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| DecryptError::new_err(e.to_string()))?;
     reader
         .read_to_end(&mut decrypted)
-        .map_err(|e| PyValueError::new_err(e.to_string()))?;
+        .map_err(|e| DecryptError::new_err(e.to_string()))?;
 
     Ok(PyBytes::new(py, &decrypted))
 }
diff --git a/src/ssh.rs b/src/ssh.rs
index 40cbbd6..18acbf8 100644
--- a/src/ssh.rs
+++ b/src/ssh.rs
@@ -1,6 +1,8 @@
 use std::str::FromStr;
 
-use pyo3::{exceptions::PyValueError, prelude::*, types::PyType};
+use pyo3::{prelude::*, types::PyType};
+
+use crate::{IdentityError, RecipientError};
 
 #[pyclass(module = "pyrage.ssh")]
 #[derive(Clone)]
@@ -11,7 +13,7 @@ impl Recipient {
     #[classmethod]
     fn from_str(_cls: &PyType, v: &str) -> PyResult<Self> {
         let recipient = age::ssh::Recipient::from_str(v)
-            .map_err(|e| PyValueError::new_err(format!("invalid public key: {:?}", e)))?;
+            .map_err(|e| RecipientError::new_err(format!("invalid public key: {:?}", e)))?;
 
         Ok(Self(recipient))
     }
@@ -26,16 +28,16 @@ impl Identity {
     #[classmethod]
     fn from_buffer(_cls: &PyType, buf: &[u8]) -> PyResult<Self> {
         let identity = age::ssh::Identity::from_buffer(buf, None)
-            .map_err(|e| PyValueError::new_err(e.to_string()))?;
+            .map_err(|e| IdentityError::new_err(e.to_string()))?;
 
         match identity {
             age::ssh::Identity::Unencrypted(_) => Ok(Self(identity)),
             age::ssh::Identity::Encrypted(_) => {
-                Err(PyValueError::new_err("ssh key must be decrypted first"))
+                Err(IdentityError::new_err("ssh key must be decrypted first"))
             }
             age::ssh::Identity::Unsupported(uk) => {
                 // Unsupported doesn't have a Display impl, only a hardcoded `display` function.
-                Err(PyValueError::new_err(format!("unsupported key: {:?}", uk)))
+                Err(IdentityError::new_err(format!("unsupported key: {:?}", uk)))
             }
         }
     }
diff --git a/src/x25519.rs b/src/x25519.rs
index f33dc52..7c7025d 100644
--- a/src/x25519.rs
+++ b/src/x25519.rs
@@ -1,7 +1,9 @@
 use std::str::FromStr;
 
 use age::secrecy::ExposeSecret;
-use pyo3::{exceptions::PyValueError, prelude::*, types::PyType};
+use pyo3::{prelude::*, types::PyType};
+
+use crate::{IdentityError, RecipientError};
 
 #[pyclass(module = "pyrage.x25519")]
 #[derive(Clone)]
@@ -13,7 +15,7 @@ impl Recipient {
     fn from_str(_cls: &PyType, v: &str) -> PyResult<Self> {
         age::x25519::Recipient::from_str(v)
             .map(Self)
-            .map_err(PyValueError::new_err)
+            .map_err(RecipientError::new_err)
     }
 
     fn __str__(&self) -> String {
@@ -34,8 +36,8 @@ impl Identity {
 
     #[classmethod]
     fn from_str(_cls: &PyType, v: &str) -> PyResult<Self> {
-        let identity =
-            age::x25519::Identity::from_str(v).map_err(|e| PyValueError::new_err(e.to_string()))?;
+        let identity = age::x25519::Identity::from_str(v)
+            .map_err(|e| IdentityError::new_err(e.to_string()))?;
 
         Ok(Self(identity))
     }


Test patch:
diff --git a/test/test_pyrage.py b/test/test_pyrage.py
index c2d43bf..cdd02c1 100644
--- a/test/test_pyrage.py
+++ b/test/test_pyrage.py
@@ -23,7 +23,7 @@ def test_decrypt_fails_wrong_recipient(self):
         encrypted = pyrage.encrypt(b"test", [alice.to_public()])
 
         # bob tries to decrypt and fails
-        with self.assertRaisesRegex(ValueError, "No matching keys found"):
+        with self.assertRaisesRegex(pyrage.DecryptError, "No matching keys found"):
             pyrage.decrypt(encrypted, [bob])
 
         # one key matches, so decryption succeeds
diff --git a/test/test_ssh.py b/test/test_ssh.py
index beff874..1433942 100644
--- a/test/test_ssh.py
+++ b/test/test_ssh.py
@@ -1,6 +1,6 @@
 import unittest
 
-from pyrage import ssh
+from pyrage import ssh, RecipientError
 
 from .utils import ssh_keypair
 
@@ -21,5 +21,5 @@ def test_from_str(self):
             self.assertIsInstance(recipient, ssh.Recipient)
 
     def test_from_str_invalid(self):
-        with self.assertRaisesRegex(ValueError, "invalid SSH recipient"):
+        with self.assertRaisesRegex(RecipientError, "invalid SSH recipient"):
             ssh.Recipient.from_str("invalid ssh pubkey")
diff --git a/test/test_x25519.py b/test/test_x25519.py
index 25ea1b7..6f208ac 100644
--- a/test/test_x25519.py
+++ b/test/test_x25519.py
@@ -1,6 +1,6 @@
 import unittest
 
-from pyrage import x25519
+from pyrage import x25519, IdentityError, RecipientError
 
 
 class TestIdentity(unittest.TestCase):
@@ -18,7 +18,7 @@ def test_from_str(self):
         self.assertIsInstance(parsed, x25519.Identity)
 
     def test_from_str_invalid(self):
-        with self.assertRaisesRegex(ValueError, "invalid Bech32 encoding"):
+        with self.assertRaisesRegex(IdentityError, "invalid Bech32 encoding"):
             x25519.Identity.from_str("BAD-PREFIX")
 
 
@@ -34,7 +34,7 @@ def test_from_str(self):
         )
 
     def test_from_str_invalid(self):
-        with self.assertRaisesRegex(ValueError, "invalid Bech32 encoding"):
+        with self.assertRaisesRegex(RecipientError, "invalid Bech32 encoding"):
             x25519.Recipient.from_str("badprefix")