fix(rust/signed-doc): Fixing collaborators metadata field and DocumentOwnershipRule validation rule

docs/src/architecture/08_concepts/signed_doc/cddl/collaborators.cddl

@@ -2,7 +2,7 @@
 
 
 ; Allowed Collaborators on the next subsequent version of a document.
-collaborators = [ * catalyst_id_kid ]
+collaborators = [ + catalyst_id_kid ]
 
 ; UTF8 Catalyst ID URI encoded as a bytes string.
 catalyst_id_kid = bytes

docs/src/architecture/08_concepts/signed_doc/cddl/signed_document.cddl

@@ -116,7 +116,7 @@ section_ref = json_pointer
 json_pointer = text
 
 ; Allowed Collaborators on the next subsequent version of a document.
-collaborators = [ * catalyst_id_kid ]
+collaborators = [ + catalyst_id_kid ]
 
 ; UTF8 Catalyst ID URI encoded as a bytes string.
 catalyst_id_kid = bytes

rust/signed_doc/src/lib.rs

@@ -239,6 +239,21 @@ impl CatalystSignedDocument {
     pub fn into_builder(&self) -> anyhow::Result<SignaturesBuilder> {
         self.try_into()
     }
+
+    /// Returns CBOR bytes.
+    pub fn to_bytes(&self) -> anyhow::Result<Vec<u8>> {
+        let mut e = minicbor::Encoder::new(Vec::new());
+        self.encode(&mut e, &mut ())?;
+        Ok(e.into_writer())
+    }
+
+    /// Build `CatalystSignedDoc` instance from CBOR bytes.
+    pub fn from_bytes(
+        bytes: &[u8],
+        mut policy: CompatibilityPolicy,
+    ) -> anyhow::Result<Self> {
+        Ok(minicbor::decode_with(bytes, &mut policy)?)
+    }
 }
 
 impl Decode<'_, CompatibilityPolicy> for CatalystSignedDocument {
@@ -344,17 +359,14 @@ impl TryFrom<&[u8]> for CatalystSignedDocument {
     type Error = anyhow::Error;
 
     fn try_from(value: &[u8]) -> Result<Self, Self::Error> {
-        Ok(minicbor::decode_with(
-            value,
-            &mut CompatibilityPolicy::Accept,
-        )?)
+        Self::from_bytes(value, CompatibilityPolicy::Accept)
     }
 }
 
-impl TryFrom<CatalystSignedDocument> for Vec<u8> {
+impl TryFrom<&CatalystSignedDocument> for Vec<u8> {
     type Error = anyhow::Error;
 
-    fn try_from(value: CatalystSignedDocument) -> Result<Self, Self::Error> {
-        Ok(minicbor::to_vec(value)?)
+    fn try_from(value: &CatalystSignedDocument) -> Result<Self, Self::Error> {
+        value.to_bytes()
     }
 }

rust/signed_doc/src/metadata/collaborators.rs

@@ -29,16 +29,14 @@ impl minicbor::Encode<()> for Collaborators {
         e: &mut minicbor::Encoder<W>,
         _ctx: &mut (),
     ) -> Result<(), minicbor::encode::Error<W::Error>> {
-        if !self.0.is_empty() {
-            e.array(
-                self.0
-                    .len()
-                    .try_into()
-                    .map_err(minicbor::encode::Error::message)?,
-            )?;
-            for c in &self.0 {
-                e.bytes(&c.to_string().into_bytes())?;
-            }
+        e.array(
+            self.0
+                .len()
+                .try_into()
+                .map_err(minicbor::encode::Error::message)?,
+        )?;
+        for c in &self.0 {
+            e.bytes(&c.to_string().into_bytes())?;
         }
         Ok(())
     }
@@ -49,15 +47,35 @@ impl minicbor::Decode<'_, ()> for Collaborators {
         d: &mut minicbor::Decoder<'_>,
         _ctx: &mut (),
     ) -> Result<Self, minicbor::decode::Error> {
-        Array::decode(d, &mut DecodeCtx::Deterministic)?
+        Array::decode(d, &mut DecodeCtx::Deterministic)
+            .and_then(|arr| {
+                if arr.is_empty() {
+                    Err(minicbor::decode::Error::message(
+                        "collaborators array must have at least one element",
+                    ))
+                } else {
+                    Ok(arr)
+                }
+            })?
             .iter()
             .map(|item| minicbor::Decoder::new(item).bytes())
             .collect::<Result<Vec<_>, _>>()?
             .into_iter()
-            .map(CatalystId::try_from)
+            .map(|id| {
+                CatalystId::try_from(id)
+                    .map_err(minicbor::decode::Error::custom)
+                    .and_then(|id| {
+                        if id.is_uri() {
+                            Ok(id)
+                        } else {
+                            Err(minicbor::decode::Error::message(format!(
+                            "provided CatalystId {id} must in ID format for collaborators field"
+                        )))
+                        }
+                    })
+            })
             .collect::<Result<_, _>>()
             .map(Self)
-            .map_err(minicbor::decode::Error::custom)
     }
 }
 
@@ -66,10 +84,21 @@ impl<'de> serde::Deserialize<'de> for Collaborators {
     where D: serde::Deserializer<'de> {
         Vec::<String>::deserialize(deserializer)?
             .into_iter()
-            .map(|id| CatalystId::from_str(&id))
+            .map(|id| {
+                CatalystId::from_str(&id)
+                    .map_err(serde::de::Error::custom)
+                    .and_then(|id| {
+                        if id.is_uri() {
+                            Ok(id)
+                        } else {
+                            Err(serde::de::Error::custom(format!(
+                            "provided CatalystId {id} must in ID format for collaborators field"
+                        )))
+                        }
+                    })
+            })
             .collect::<Result<_, _>>()
             .map(Self)
-            .map_err(serde::de::Error::custom)
     }
 }
 
@@ -85,3 +114,42 @@ impl serde::Serialize for Collaborators {
         serializer.collect_seq(iter)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use minicbor::{Decode, Decoder, Encoder};
+    use test_case::test_case;
+
+    use super::*;
+
+    #[test_case(
+        {
+            Encoder::new(Vec::new())
+        } ;
+        "Invalid empty CBOR bytes"
+    )]
+    #[test_case(
+        {
+            let mut e = Encoder::new(Vec::new());
+            e.array(0).unwrap();
+            e
+        } ;
+        "Empty CBOR array"
+    )]
+    #[test_case(
+        {
+            let mut e = Encoder::new(Vec::new());
+            e.array(1).unwrap();
+            /* cspell:disable */
+            e.bytes(b"preprod.cardano/FftxFnOrj2qmTuB2oZG2v0YEWJfKvQ9Gg8AgNAhDsKE/7/3").unwrap();
+            /* cspell:enable */
+            e
+        } ;
+        "CatalystId not in ID form"
+    )]
+    fn test_invalid_cbor_decode(e: Encoder<Vec<u8>>) {
+        assert!(
+            Collaborators::decode(&mut Decoder::new(e.into_writer().as_slice()), &mut ()).is_err()
+        );
+    }
+}

rust/signed_doc/src/validator/rules/ownership/mod.rs

@@ -14,18 +14,6 @@ use crate::{providers::CatalystSignedDocumentProvider, CatalystSignedDocument};
 /// Context for the validation problem report.
 const REPORT_CONTEXT: &str = "Document ownership validation";
 
-/// Returns `true` if the document has a single author.
-///
-/// If not, it adds to the document's problem report.
-fn single_author_check(doc: &CatalystSignedDocument) -> bool {
-    let is_valid = doc.authors().len() == 1;
-    if !is_valid {
-        doc.report()
-            .functional_validation("Document must only be signed by one author", REPORT_CONTEXT);
-    }
-    is_valid
-}
-
 /// Document Ownership Validation Rule
 #[derive(Debug)]
 pub(crate) struct DocumentOwnershipRule {
@@ -53,10 +41,17 @@ impl DocumentOwnershipRule {
         Provider: CatalystSignedDocumentProvider,
     {
         let doc_id = doc.doc_id()?;
-        let first_doc_opt = provider.try_get_first_doc(doc_id).await?;
 
-        if self.allow_collaborators {
-            if let Some(first_doc) = first_doc_opt {
+        if doc_id == doc.doc_ver()? && doc.authors().len() != 1 {
+            doc.report().functional_validation(
+                "Document must only be signed by one author",
+                REPORT_CONTEXT,
+            );
+            return Ok(false);
+        }
+
+        if let Some(first_doc) = provider.try_get_first_doc(doc_id).await? {
+            if self.allow_collaborators {
                 // This a new version of an existing `doc_id`
                 let Some(last_doc) = provider.try_get_last_doc(doc_id).await? else {
                     anyhow::bail!(
@@ -72,51 +67,45 @@ impl DocumentOwnershipRule {
                 let mut allowed_authors = first_doc
                     .authors()
                     .into_iter()
-                    .map(CatalystId::as_uri)
                     .collect::<HashSet<CatalystId>>();
                 allowed_authors.extend(
                     last_doc
                         .doc_meta()
                         .collaborators()
                         .iter()
-                        .cloned()
-                        .map(CatalystId::as_uri),
+                        .map(CatalystId::as_short_id),
                 );
-                let doc_authors = doc
-                    .authors()
-                    .into_iter()
-                    .map(CatalystId::as_uri)
-                    .collect::<HashSet<_>>();
+                let doc_authors = doc.authors().into_iter().collect::<HashSet<_>>();
 
-                let is_valid = allowed_authors.intersection(&doc_authors).count() > 0;
+                // all elements of the `doc_authors` should be intersecting with the
+                // `allowed_authors`
+                let is_valid =
+                    allowed_authors.intersection(&doc_authors).count() == doc_authors.len();
 
                 if !is_valid {
                     doc.report().functional_validation(
-                        "Document must only be signed by original author and/or by collaborators defined in the previous version",
+                         &format!(
+                            "Document must only be signed by original author and/or by collaborators defined in the previous version. Allowed signers: {:?}, Document signers: {:?}",
+                            allowed_authors.iter().map(|v| v.to_string()).collect::<Vec<_>>(),
+                            doc_authors.iter().map(|v| v.to_string()).collect::<Vec<_>>()
+                        ),
+                        REPORT_CONTEXT,
+                    );
+                }
+                return Ok(is_valid);
+            } else {
+                // No collaborators are allowed
+                let is_valid = first_doc.authors() == doc.authors();
+                if !is_valid {
+                    doc.report().functional_validation(
+                        "Document authors must match the author from the first version",
                         REPORT_CONTEXT,
                     );
                 }
                 return Ok(is_valid);
             }
-
-            // This is a first version of the doc
-            return Ok(single_author_check(doc));
-        }
-
-        // No collaborators are allowed
-        if let Some(first_doc) = first_doc_opt {
-            // This a new version of an existing `doc_id`
-            let is_valid = first_doc.authors() == doc.authors();
-            if !is_valid {
-                doc.report().functional_validation(
-                    "Document authors must match the author from the first version",
-                    REPORT_CONTEXT,
-                );
-            }
-            return Ok(is_valid);
         }
 
-        // This is a first version of the doc
-        Ok(single_author_check(doc))
+        Ok(true)
     }
 }