From 11f4175df2630ae4c47dddf8c4e0187df1fbc9c5 Mon Sep 17 00:00:00 2001
From: xhon-pelushi <xhon@pelushi.com>
Date: Sun, 30 Aug 2026 13:15:37 -0400
Subject: [PATCH] Address review: move validate_ssh_key_data, simplify it and
 the tests

The function had been inserted between the third and fourth lines of the doc
comment on enforce_personal_ownership_policy, splitting it in two. Moved it
below that function so the comment reads as one block again.

Simplified the check to the same idiom Cipher::to_json already uses for these
exact three fields (as_str().is_none_or(str::is_empty)), and noted the
relationship between the two in the doc comment: to_json discards the type-data
of stored SSH ciphers whose fields are missing or empty, while this rejects
them before they are written.

Collapsed the four test functions into one table of cases, and added two the
originals did not cover: a non-string field, and type-data with no fields at
all. Each case carries a label that is printed on failure, so a broken case
still says which one it was.
---
 src/api/core/ciphers.rs | 74 +++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 47 deletions(-)

diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs
index cf5c493e..bb0a8eef 100644
--- a/src/api/core/ciphers.rs
+++ b/src/api/core/ciphers.rs
@@ -377,17 +377,6 @@ async fn post_ciphers(data: Json<CipherData>, headers: Headers, conn: DbConn, nt
 /// Enforces the personal ownership policy on user-owned ciphers, if applicable.
 /// A non-owner/admin user belonging to an org with the personal ownership policy
 /// enabled isn't allowed to create new user-owned ciphers or modify existing ones
-/// Ensure SSH key type-data has the required non-empty string fields.
-fn validate_ssh_key_data(type_data: &Value) -> EmptyResult {
-    for field in ["privateKey", "publicKey", "keyFingerprint"] {
-        match type_data.get(field).and_then(Value::as_str) {
-            Some(value) if !value.is_empty() => {}
-            _ => err!(format!("SSH key field '{field}' must be a non-empty string")),
-        }
-    }
-    Ok(())
-}
-
 /// (that were created before the policy was applicable to the user). The user is
 /// allowed to delete or share such ciphers to an org, however.
 ///
@@ -403,6 +392,20 @@ async fn enforce_personal_ownership_policy(data: Option<&CipherData>, headers: &
     Ok(())
 }
 
+/// Ensure SSH key type-data carries the mandatory non-empty string fields.
+///
+/// `Cipher::to_json` already discards the type-data of stored SSH ciphers whose
+/// fields are missing or empty; rejecting them here keeps such ciphers from
+/// being written in the first place.
+fn validate_ssh_key_data(type_data: &Value) -> EmptyResult {
+    for field in ["privateKey", "publicKey", "keyFingerprint"] {
+        if type_data[field].as_str().is_none_or(str::is_empty) {
+            err!(format!("SSH key field '{field}' must be a non-empty string"))
+        }
+    }
+    Ok(())
+}
+
 pub async fn update_cipher_from_data(
     cipher: &mut Cipher,
     data: CipherData,
@@ -2246,41 +2249,18 @@ mod ssh_key_validation_tests {
     use serde_json::json;
 
     #[test]
-    fn accepts_non_empty_required_fields() {
-        let data = json!({
-            "privateKey": "priv",
-            "publicKey": "pub",
-            "keyFingerprint": "fp"
-        });
-        assert!(validate_ssh_key_data(&data).is_ok());
-    }
-
-    #[test]
-    fn rejects_null_private_key() {
-        let data = json!({
-            "privateKey": null,
-            "publicKey": "pub",
-            "keyFingerprint": "fp"
-        });
-        assert!(validate_ssh_key_data(&data).is_err());
-    }
-
-    #[test]
-    fn rejects_empty_public_key() {
-        let data = json!({
-            "privateKey": "priv",
-            "publicKey": "",
-            "keyFingerprint": "fp"
-        });
-        assert!(validate_ssh_key_data(&data).is_err());
-    }
-
-    #[test]
-    fn rejects_missing_fingerprint() {
-        let data = json!({
-            "privateKey": "priv",
-            "publicKey": "pub"
-        });
-        assert!(validate_ssh_key_data(&data).is_err());
+    fn validate_ssh_key_data_required_fields() {
+        let cases = [
+            ("all fields present", json!({"privateKey": "priv", "publicKey": "pub", "keyFingerprint": "fp"}), true),
+            ("null private key", json!({"privateKey": null, "publicKey": "pub", "keyFingerprint": "fp"}), false),
+            ("empty public key", json!({"privateKey": "priv", "publicKey": "", "keyFingerprint": "fp"}), false),
+            ("missing fingerprint", json!({"privateKey": "priv", "publicKey": "pub"}), false),
+            ("non-string fingerprint", json!({"privateKey": "priv", "publicKey": "pub", "keyFingerprint": 42}), false),
+            ("no fields at all", json!({}), false),
+        ];
+
+        for (case, type_data, expected_ok) in cases {
+            assert_eq!(validate_ssh_key_data(&type_data).is_ok(), expected_ok, "case: {case}");
+        }
     }
 }