refactor(rust/signed-doc): Move validate_id_and_ver as separate rules IdRule and VerRule (#497)

* feat: ver

* chore: check version

* feat: id

* finalize

* feat: apply rules

* chore: fmtfix

* refactor: move to rules

* chore: minor doc

* move unit tests for IdRule and VerRule

* update VerRule validation

* fix tests and spelling

* wip

* chore: cspellfix

* fix clippy

---------

Co-authored-by: Alex Pozhylenkov <[email protected]>

Author: apskhem

rust/signed_doc/src/providers.rs

@@ -2,7 +2,7 @@
 
 use std::{future::Future, time::Duration};
 
-use catalyst_types::catalyst_id::CatalystId;
+use catalyst_types::{catalyst_id::CatalystId, uuid::UuidV7};
 use ed25519_dalek::VerifyingKey;
 
 use crate::{CatalystSignedDocument, DocumentRef};
@@ -18,12 +18,19 @@ pub trait VerifyingKeyProvider {
 
 /// `CatalystSignedDocument` Provider trait
 pub trait CatalystSignedDocumentProvider: Send + Sync {
-    /// Try to get `CatalystSignedDocument`from document reference
+    /// Try to get `CatalystSignedDocument` from document reference
     fn try_get_doc(
         &self,
         doc_ref: &DocumentRef,
     ) -> impl Future<Output = anyhow::Result<Option<CatalystSignedDocument>>> + Send;
 
+    /// Try to get the last known version of the `CatalystSignedDocument`, same
+    /// `id` and the highest known `ver`.
+    fn try_get_last_doc(
+        &self,
+        id: UuidV7,
+    ) -> impl Future<Output = anyhow::Result<Option<CatalystSignedDocument>>> + Send;
+
     /// Returns a future threshold value, which is used in the validation of the `ver`
     /// field that it is not too far in the future.
     /// If `None` is returned, skips "too far in the future" validation.
@@ -48,7 +55,6 @@ pub mod tests {
 
     ///  Simple testing implementation of `CatalystSignedDocumentProvider`
     #[derive(Default, Debug)]
-
     pub struct TestCatalystSignedDocumentProvider(HashMap<DocumentRef, CatalystSignedDocument>);
 
     impl TestCatalystSignedDocumentProvider {
@@ -82,6 +88,18 @@ pub mod tests {
             Ok(self.0.get(doc_ref).cloned())
         }
 
+        async fn try_get_last_doc(
+            &self,
+            id: catalyst_types::uuid::UuidV7,
+        ) -> anyhow::Result<Option<CatalystSignedDocument>> {
+            Ok(self
+                .0
+                .iter()
+                .filter(|(doc_ref, _)| doc_ref.id() == &id)
+                .max_by_key(|(doc_ref, _)| doc_ref.ver().uuid())
+                .map(|(_, doc)| doc.clone()))
+        }
+
         fn future_threshold(&self) -> Option<std::time::Duration> {
             Some(Duration::from_secs(5))
         }

rust/signed_doc/src/validator/mod.rs

@@ -6,14 +6,13 @@ pub(crate) mod rules;
 use std::{
     collections::HashMap,
     sync::{Arc, LazyLock},
-    time::{Duration, SystemTime},
 };
 
 use anyhow::Context;
 use catalyst_types::{catalyst_id::role_index::RoleId, problem_report::ProblemReport};
 use rules::{
-    ContentEncodingRule, ContentRule, ContentSchema, ContentTypeRule, ParametersRule, RefRule,
-    ReplyRule, Rules, SectionRule, SignatureKidRule,
+    ContentEncodingRule, ContentRule, ContentSchema, ContentTypeRule, IdRule, ParametersRule,
+    RefRule, ReplyRule, Rules, SectionRule, SignatureKidRule, VerRule,
 };
 
 use crate::{
@@ -41,6 +40,8 @@ fn proposal_rule() -> Rules {
         CATEGORY_PARAMETERS.clone(),
     ];
     Rules {
+        id: IdRule,
+        ver: VerRule,
         content_type: ContentTypeRule {
             exp: ContentType::Json,
         },
@@ -75,6 +76,8 @@ fn proposal_comment_rule() -> Rules {
         CATEGORY_PARAMETERS.clone(),
     ];
     Rules {
+        id: IdRule,
+        ver: VerRule,
         content_type: ContentTypeRule {
             exp: ContentType::Json,
         },
@@ -126,6 +129,8 @@ fn proposal_submission_action_rule() -> Rules {
             .expect("Must be a valid json scheme file");
 
     Rules {
+        id: IdRule,
+        ver: VerRule,
         content_type: ContentTypeRule {
             exp: ContentType::Json,
         },
@@ -190,10 +195,6 @@ where
         return Ok(false);
     };
 
-    if !validate_id_and_ver(doc, provider)? {
-        return Ok(false);
-    }
-
     let Some(rules) = DOCUMENT_RULES.get(doc_type) else {
         doc.report().invalid_value(
             "`type`",
@@ -206,106 +207,6 @@ where
     rules.check(doc, provider).await
 }
 
-/// Validates document `id` and `ver` fields on the timestamps:
-/// 1. document `ver` cannot be smaller than document id field
-/// 2. If `provider.future_threshold()` not `None`, document `id` cannot be too far in the
-///    future (`future_threshold` arg) from `SystemTime::now()` based on the provide
-///    threshold
-/// 3. If `provider.future_threshold()` not `None`, document `id` cannot be too far behind
-///    (`past_threshold` arg) from `SystemTime::now()` based on the provide threshold
-fn validate_id_and_ver<Provider>(
-    doc: &CatalystSignedDocument,
-    provider: &Provider,
-) -> anyhow::Result<bool>
-where
-    Provider: CatalystSignedDocumentProvider,
-{
-    let id = doc.doc_id().ok();
-    let ver = doc.doc_ver().ok();
-    if id.is_none() {
-        doc.report().missing_field(
-            "id",
-            "Can't get a document id during the validation process",
-        );
-    }
-    if ver.is_none() {
-        doc.report().missing_field(
-            "ver",
-            "Can't get a document ver during the validation process",
-        );
-    }
-    match (id, ver) {
-        (Some(id), Some(ver)) => {
-            let mut is_valid = true;
-            if ver < id {
-                doc.report().invalid_value(
-                    "ver",
-                    &ver.to_string(),
-                    "ver < id",
-                    &format!("Document Version {ver} cannot be smaller than Document ID {id}"),
-                );
-                is_valid = false;
-            }
-
-            let (ver_time_secs, ver_time_nanos) = ver
-                .uuid()
-                .get_timestamp()
-                .ok_or(anyhow::anyhow!("Document ver field must be a UUIDv7"))?
-                .to_unix();
-
-            let Some(ver_time) =
-                SystemTime::UNIX_EPOCH.checked_add(Duration::new(ver_time_secs, ver_time_nanos))
-            else {
-                doc.report().invalid_value(
-                    "ver",
-                    &ver.to_string(),
-                    "Must a valid duration since `UNIX_EPOCH`",
-                    "Cannot instantiate a valid `SystemTime` value from the provided `ver` field timestamp.",
-                );
-                return Ok(false);
-            };
-
-            let now = SystemTime::now();
-
-            if let Ok(version_age) = ver_time.duration_since(now) {
-                // `now` is earlier than `ver_time`
-                if let Some(future_threshold) = provider.future_threshold() {
-                    if version_age > future_threshold {
-                        doc.report().invalid_value(
-                        "ver",
-                        &ver.to_string(),
-                        "ver < now + future_threshold",
-                        &format!("Document Version timestamp {id} cannot be too far in future (threshold: {future_threshold:?}) from now: {now:?}"),
-                    );
-                        is_valid = false;
-                    }
-                }
-            } else {
-                // `ver_time` is earlier than `now`
-                let version_age = now
-                    .duration_since(ver_time)
-                    .context("BUG! `ver_time` must be earlier than `now` at this place")?;
-
-                if let Some(past_threshold) = provider.past_threshold() {
-                    if version_age > past_threshold {
-                        doc.report().invalid_value(
-                        "ver",
-                        &ver.to_string(),
-                        "ver > now - past_threshold",
-                        &format!("Document Version timestamp {id} cannot be too far behind (threshold: {past_threshold:?}) from now: {now:?}",),
-                    );
-                        is_valid = false;
-                    }
-                }
-            }
-
-            Ok(is_valid)
-        },
-
-        _ => Ok(false),
-    }
-}
-
 /// Verify document signatures.
 /// Return true if all signatures are valid, otherwise return false.
 ///
@@ -384,82 +285,7 @@ where
 
 #[cfg(test)]
 mod tests {
-    use std::time::SystemTime;
-
-    use uuid::{Timestamp, Uuid};
-
-    use crate::{
-        builder::tests::Builder,
-        metadata::SupportedField,
-        providers::{tests::TestCatalystSignedDocumentProvider, CatalystSignedDocumentProvider},
-        validator::{document_rules_init, validate_id_and_ver},
-        UuidV7,
-    };
-
-    #[test]
-    fn document_id_and_ver_test() {
-        let provider = TestCatalystSignedDocumentProvider::default();
-        let now = SystemTime::now()
-            .duration_since(SystemTime::UNIX_EPOCH)
-            .unwrap()
-            .as_secs();
-
-        let uuid_v7 = UuidV7::new();
-        let doc = Builder::new()
-            .with_metadata_field(SupportedField::Id(uuid_v7))
-            .with_metadata_field(SupportedField::Ver(uuid_v7))
-            .build();
-
-        let is_valid = validate_id_and_ver(&doc, &provider).unwrap();
-        assert!(is_valid);
-
-        let ver = Uuid::new_v7(Timestamp::from_unix_time(now - 1, 0, 0, 0))
-            .try_into()
-            .unwrap();
-        let id = Uuid::new_v7(Timestamp::from_unix_time(now + 1, 0, 0, 0))
-            .try_into()
-            .unwrap();
-        assert!(ver < id);
-        let doc = Builder::new()
-            .with_metadata_field(SupportedField::Id(id))
-            .with_metadata_field(SupportedField::Ver(ver))
-            .build();
-
-        let is_valid = validate_id_and_ver(&doc, &provider).unwrap();
-        assert!(!is_valid);
-
-        let to_far_in_past = Uuid::new_v7(Timestamp::from_unix_time(
-            now - provider.past_threshold().unwrap().as_secs() - 1,
-            0,
-            0,
-            0,
-        ))
-        .try_into()
-        .unwrap();
-        let doc = Builder::new()
-            .with_metadata_field(SupportedField::Id(to_far_in_past))
-            .with_metadata_field(SupportedField::Ver(to_far_in_past))
-            .build();
-
-        let is_valid = validate_id_and_ver(&doc, &provider).unwrap();
-        assert!(!is_valid);
-
-        let to_far_in_future = Uuid::new_v7(Timestamp::from_unix_time(
-            now + provider.future_threshold().unwrap().as_secs() + 1,
-            0,
-            0,
-            0,
-        ))
-        .try_into()
-        .unwrap();
-        let doc = Builder::new()
-            .with_metadata_field(SupportedField::Id(to_far_in_future))
-            .with_metadata_field(SupportedField::Ver(to_far_in_future))
-            .build();
-
-        let is_valid = validate_id_and_ver(&doc, &provider).unwrap();
-        assert!(!is_valid);
-    }
+    use crate::validator::document_rules_init;
 
     #[test]
     fn document_rules_init_test() {