chore: adjustments following reviews

Alenar · Alenar · commit a93a40f21ab0 · 2024-12-19T10:32:26.000+01:00
- Rename new event from `CertificateValidatedFromCache` to
  `CertificateFetchedFromCache`
- Better naming for the cache default duration in client (to make it
  explicit that the number is one week)
- Make some tests more clear with better naming and more assertion
- Change `ClientBuilder::with_certificate_verifier_cache` to take an
  option, allowing to disable existing cache and simplifying usage in
  the wasm library.
- Simplify local storage acquisition in `LocalStorageCertificateVerifierCache`
  by raising an error if it's missing instead of asking consumer to handle
  an option (it should not be used if not available anyway).
- Adjusted some comments and logs
diff --git a/examples/client-snapshot/src/main.rs b/examples/client-snapshot/src/main.rs
@@ -171,7 +171,7 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver {
                     progress_bar.inc(1);
                 }
             }
-            MithrilEvent::CertificateValidatedFromCache {
+            MithrilEvent::CertificateFetchedFromCache {
                 certificate_chain_validation_id: _,
                 certificate_hash,
             } => {
diff --git a/mithril-client-cli/src/utils/feedback_receiver.rs b/mithril-client-cli/src/utils/feedback_receiver.rs
@@ -92,7 +92,7 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver {
                     progress_bar.inc(1);
                 }
             }
-            MithrilEvent::CertificateValidatedFromCache {
+            MithrilEvent::CertificateFetchedFromCache {
                 certificate_chain_validation_id: _,
                 certificate_hash,
             } => {
diff --git a/mithril-client-wasm/src/certificate_verification_cache.rs b/mithril-client-wasm/src/certificate_verification_cache.rs
@@ -54,19 +54,18 @@ impl LocalStorageCertificateVerifierCache {
         previous_certificate_hash: &PreviousCertificateHash,
         expire_at: DateTime<Utc>,
     ) -> MithrilResult<()> {
-        if let Some(storage) = open_local_storage()? {
-            let key = self.cache_key(certificate_hash);
-            storage
-                .set_item(
-                    &key,
-                    &serde_json::to_string(&CachedCertificate::new(
-                        previous_certificate_hash,
-                        expire_at,
-                    ))
-                    .map_err(|err| anyhow!("Error serializing cache: {err:?}"))?,
-                )
-                .map_err(|err| anyhow!("Error storing key `{key}` in local storage: {err:?}"))?;
-        }
+        let key = self.cache_key(certificate_hash);
+        open_local_storage()?
+            .set_item(
+                &key,
+                &serde_json::to_string(&CachedCertificate::new(
+                    previous_certificate_hash,
+                    expire_at,
+                ))
+                .map_err(|err| anyhow!("Error serializing cache: {err:?}"))?,
+            )
+            .map_err(|err| anyhow!("Error storing key `{key}` in local storage: {err:?}"))?;
+
         Ok(())
     }
 
@@ -80,11 +79,12 @@ impl LocalStorageCertificateVerifierCache {
     }
 }
 
-fn open_local_storage() -> MithrilResult<Option<Storage>> {
+fn open_local_storage() -> MithrilResult<Storage> {
     let window = window()
         .with_context(|| "No window object")?
         .local_storage()
-        .map_err(|err| anyhow!("Error accessing local storage: {err:?}"))?;
+        .map_err(|err| anyhow!("Error accessing local storage: {err:?}"))?
+        .with_context(|| "No local storage object")?;
     Ok(window)
 }
 
@@ -108,50 +108,45 @@ impl CertificateVerifierCache for LocalStorageCertificateVerifierCache {
         &self,
         certificate_hash: &CertificateHash,
     ) -> MithrilResult<Option<String>> {
-        match open_local_storage()? {
-            None => return Ok(None),
-            Some(storage) => {
-                let key = self.cache_key(certificate_hash);
-                match storage.get_item(&key).map_err(|err| {
-                    anyhow!("Error accessing key `{key}` from local storage: {err:?}")
-                })? {
-                    Some(value) => {
-                        let cached = Self::parse_cached_certificate(value)?;
-                        if Utc::now() >= cached.expire_at {
-                            Ok(None)
-                        } else {
-                            Ok(Some(cached.previous_hash))
-                        }
-                    }
-                    None => Ok(None),
+        let key = self.cache_key(certificate_hash);
+        match open_local_storage()?
+            .get_item(&key)
+            .map_err(|err| anyhow!("Error accessing key `{key}` from local storage: {err:?}"))?
+        {
+            Some(value) => {
+                let cached = Self::parse_cached_certificate(value)?;
+                if Utc::now() >= cached.expire_at {
+                    Ok(None)
+                } else {
+                    Ok(Some(cached.previous_hash))
                 }
             }
+            None => Ok(None),
         }
     }
 
     async fn reset(&self) -> MithrilResult<()> {
-        if let Some(storage) = open_local_storage()? {
-            let len = storage
-                .length()
-                .map_err(|err| anyhow!("Error accessing local storage length: {err:?}"))?;
-            let mut key_to_remove = vec![];
-
-            for i in 0..len {
-                match storage.key(i).map_err(|err| {
-                    anyhow!("Error accessing key index `{i}` from local storage: {err:?}")
-                })? {
-                    Some(key) if key.starts_with(&self.cache_key_prefix) => key_to_remove.push(key),
-                    _ => continue,
-                }
-            }
+        let storage = open_local_storage()?;
+        let len = storage
+            .length()
+            .map_err(|err| anyhow!("Error accessing local storage length: {err:?}"))?;
+        let mut key_to_remove = vec![];
 
-            for key in key_to_remove {
-                storage.remove_item(&key).map_err(|err| {
-                    anyhow!("Error removing key `{key}` from local storage: {err:?}")
-                })?;
+        for i in 0..len {
+            match storage.key(i).map_err(|err| {
+                anyhow!("Error accessing key index `{i}` from local storage: {err:?}")
+            })? {
+                Some(key) if key.starts_with(&self.cache_key_prefix) => key_to_remove.push(key),
+                _ => continue,
             }
         }
 
+        for key in key_to_remove {
+            storage
+                .remove_item(&key)
+                .map_err(|err| anyhow!("Error removing key `{key}` from local storage: {err:?}"))?;
+        }
+
         Ok(())
     }
 }
@@ -164,7 +159,7 @@ pub(crate) mod test_tools {
 
     /// `Test only` Return the raw content of the local storage
     pub(crate) fn local_storage_content() -> HashMap<String, String> {
-        let storage = open_local_storage().unwrap().unwrap();
+        let storage = open_local_storage().unwrap();
         let len = storage.length().unwrap();
         let mut content = HashMap::new();
 
@@ -220,7 +215,7 @@ pub(crate) mod test_tools {
             certificate_hash: &CertificateHash,
             expire_at: DateTime<Utc>,
         ) {
-            let storage = open_local_storage().unwrap().unwrap();
+            let storage = open_local_storage().unwrap();
             let key = self.cache_key(certificate_hash);
             let existing_value = Self::parse_cached_certificate(
                 storage.get_item(&key).unwrap().expect("Key not found"),
@@ -243,7 +238,7 @@ pub(crate) mod test_tools {
             &self,
             certificate_hash: &CertificateHash,
         ) -> Option<CachedCertificate> {
-            let storage = open_local_storage().unwrap().unwrap();
+            let storage = open_local_storage().unwrap();
             storage
                 .get_item(&self.cache_key(certificate_hash))
                 .unwrap()
@@ -286,7 +281,7 @@ mod tests {
         #[wasm_bindgen_test]
         async fn store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay() {
             let expiration_delay = TimeDelta::hours(1);
-            let now = Utc::now();
+            let start_time = Utc::now();
             let cache = LocalStorageCertificateVerifierCache::new(
                 "store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay",
                 expiration_delay,
@@ -302,7 +297,7 @@ mod tests {
 
             assert_eq!(1, cache.len());
             assert_eq!("parent", cached.previous_hash);
-            assert!(cached.expire_at - now >= expiration_delay);
+            assert!(cached.expire_at - start_time >= expiration_delay);
         }
 
         #[wasm_bindgen_test]
@@ -333,31 +328,32 @@ mod tests {
         #[wasm_bindgen_test]
         async fn storing_same_hash_update_parent_hash_and_expiration_time() {
             let expiration_delay = TimeDelta::days(2);
-            let now = Utc::now();
+            let start_time = Utc::now();
             let cache = LocalStorageCertificateVerifierCache::new(
                 "storing_same_hash_update_parent_hash_and_expiration_time",
                 expiration_delay,
             )
             .with_items([("hash", "first_parent"), ("another_hash", "another_parent")]);
 
+            let initial_value = cache.get_cached_value("hash").unwrap();
+
             cache
                 .store_validated_certificate("hash", "updated_parent")
                 .await
                 .unwrap();
 
-            let updated_value = cache
-                .get_cached_value("hash")
-                .expect("Cache should have been populated");
+            let updated_value = cache.get_cached_value("hash").unwrap();
 
             assert_eq!(2, cache.len());
             assert_eq!(
                 Some("another_parent".to_string()),
                 cache.get_previous_hash("another_hash").await.unwrap(),
-                "Existing but not updated value should not have been altered, content: {:#?}, now: {:?}",
-                local_storage_content(), now,
+                "Existing but not updated value should not have been altered, content: {:#?}, start_time: {:?}",
+                local_storage_content(), start_time,
             );
             assert_eq!("updated_parent", updated_value.previous_hash);
-            assert!(updated_value.expire_at - now >= expiration_delay);
+            assert_ne!(initial_value, updated_value);
+            assert!(updated_value.expire_at - start_time >= expiration_delay);
         }
     }
 
@@ -424,7 +420,7 @@ mod tests {
                 TimeDelta::hours(1),
             )
             .with_items([("hash", "parent"), ("another_hash", "another_parent")]);
-            let storage = open_local_storage().unwrap().unwrap();
+            let storage = open_local_storage().unwrap();
             storage
                 .set_item("key_from_another_component", "another_value")
                 .unwrap();
diff --git a/mithril-client-wasm/src/client_wasm.rs b/mithril-client-wasm/src/client_wasm.rs
@@ -91,29 +91,24 @@ impl MithrilClient {
                 .map_err(|err| format!("Failed to parse options: {err:?}"))
                 .unwrap()
         };
-        let mut client_builder =
-            ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key)
-                .add_feedback_receiver(feedback_receiver)
-                .with_options(client_options.clone());
 
         let certificate_verifier_cache = if client_options.unstable
             && client_options.enable_certificate_chain_verification_cache
         {
-            let cache = Self::build_certifier_cache(
+            Self::build_certifier_cache(
                 aggregator_endpoint,
                 TimeDelta::seconds(
                     client_options.certificate_chain_verification_cache_duration_in_seconds as i64,
                 ),
-            );
-            if let Some(cache) = &cache {
-                client_builder = client_builder.with_certificate_verifier_cache(cache.clone());
-            }
-            cache
+            )
         } else {
             None
         };
 
-        let client = client_builder
+        let client = ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key)
+            .add_feedback_receiver(feedback_receiver)
+            .with_options(client_options.clone())
+            .with_certificate_verifier_cache(certificate_verifier_cache.clone())
             .build()
             .map_err(|err| format!("{err:?}"))
             .unwrap();
@@ -132,16 +127,15 @@ impl MithrilClient {
         if web_sys::window().is_none() {
             web_sys::console::warn_1(
                 &"Can't enable certificate chain verification cache: window object is not available\
-                    (are you running in a browser environment ?)"
+                    (are you running in a browser environment?)"
                     .into(),
             );
             return None;
         }
 
         web_sys::console::warn_1(
-            &"Certificate chain verification cache is enabled.\n\
-            This feature is experimental and will be heavily modified or removed in the \
-            future as its security implications are not fully understood."
+            &"Danger: the certificate chain verification cache is enabled.\n\
+            This feature is highly experimental and insecure, and it must not be used in production."
                 .into(),
         );
 
diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs
@@ -111,9 +111,9 @@ impl MithrilCertificateVerifier {
     ) -> MithrilResult<Option<CertificateToVerify>> {
         trace!(self.logger, "Validating certificate"; "hash" => certificate.hash(), "previous_hash" => certificate.hash());
         if let Some(previous_hash) = self.fetch_cached_previous_hash(certificate.hash()).await? {
-            trace!(self.logger, "Certificate validated from cache"; "hash" => certificate.hash(), "previous_hash" => &previous_hash);
+            trace!(self.logger, "Certificate fetched from cache"; "hash" => certificate.hash(), "previous_hash" => &previous_hash);
             self.feedback_sender
-                .send_event(MithrilEvent::CertificateValidatedFromCache {
+                .send_event(MithrilEvent::CertificateFetchedFromCache {
                     certificate_hash: certificate.hash().to_owned(),
                     certificate_chain_validation_id: certificate_chain_validation_id.to_string(),
                 })
diff --git a/mithril-client/src/certificate_client/verify_cache/memory_cache.rs b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs
@@ -212,7 +212,7 @@ mod tests {
         #[tokio::test]
         async fn store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay() {
             let expiration_delay = TimeDelta::hours(1);
-            let now = Utc::now();
+            let start_time = Utc::now();
             let cache = MemoryCertificateVerifierCache::new(expiration_delay);
             cache
                 .store_validated_certificate("hash", "parent")
@@ -226,7 +226,7 @@ mod tests {
 
             assert_eq!(1, cache.len().await);
             assert_eq!("parent", cached.previous_hash);
-            assert!(cached.expire_at - now >= expiration_delay);
+            assert!(cached.expire_at - start_time >= expiration_delay);
         }
 
         #[tokio::test]
@@ -253,28 +253,28 @@ mod tests {
         #[tokio::test]
         async fn storing_same_hash_update_parent_hash_and_expiration_time() {
             let expiration_delay = TimeDelta::days(2);
-            let now = Utc::now();
+            let start_time = Utc::now();
             let cache = MemoryCertificateVerifierCache::new(expiration_delay)
                 .with_items([("hash", "first_parent"), ("another_hash", "another_parent")]);
 
+            let initial_value = cache.get_cached_value("hash").await.unwrap();
+
             cache
                 .store_validated_certificate("hash", "updated_parent")
                 .await
                 .unwrap();
 
-            let updated_value = cache
-                .get_cached_value("hash")
-                .await
-                .expect("Cache should have been populated");
+            let updated_value = cache.get_cached_value("hash").await.unwrap();
 
             assert_eq!(2, cache.len().await);
             assert_eq!(
                 Some("another_parent".to_string()),
                 cache.get_previous_hash("another_hash").await.unwrap(),
                 "Existing but not updated value should not have been altered"
             );
+            assert_ne!(initial_value, updated_value);
             assert_eq!("updated_parent", updated_value.previous_hash);
-            assert!(updated_value.expire_at - now >= expiration_delay);
+            assert!(updated_value.expire_at - start_time >= expiration_delay);
         }
     }
 
diff --git a/mithril-client/src/client.rs b/mithril-client/src/client.rs
diff --git a/mithril-client/src/feedback.rs b/mithril-client/src/feedback.rs
diff --git a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js

Original file line number	Diff line number	Diff line change
`@@ -171,7 +171,7 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver {`
`171`	`171`	`progress_bar.inc(1);`
`172`	`172`	`}`
`173`	`173`	`}`
`174`		`- MithrilEvent::CertificateValidatedFromCache {`
	`174`	`+ MithrilEvent::CertificateFetchedFromCache {`
`175`	`175`	`certificate_chain_validation_id: _,`
`176`	`176`	`certificate_hash,`
`177`	`177`	`} => {`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver {`
`92`	`92`	`progress_bar.inc(1);`
`93`	`93`	`}`
`94`	`94`	`}`
`95`		`- MithrilEvent::CertificateValidatedFromCache {`
	`95`	`+ MithrilEvent::CertificateFetchedFromCache {`
`96`	`96`	`certificate_chain_validation_id: _,`
`97`	`97`	`certificate_hash,`
`98`	`98`	`} => {`