From 75b52e28b2445472d283daed405c141e77c85883 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 13 Sep 2023 15:32:11 +0100 Subject: [PATCH] radicle: separate repository validation Make the validation logic customisable by extracting it out into a separate trait that can be defined by other crates. The Repository validation logic stays the same. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-cli/src/commands/clone.rs | 1 + radicle-cli/src/commands/patch/list.rs | 1 + radicle-cli/src/commands/publish.rs | 2 +- radicle-cli/tests/commands.rs | 2 +- radicle-httpd/src/api/json.rs | 2 +- radicle-httpd/src/api/v1/projects.rs | 2 +- radicle-node/src/service.rs | 2 +- radicle-node/src/service/message.rs | 2 +- radicle-node/src/test/environment.rs | 2 +- radicle-node/src/test/peer.rs | 2 +- radicle-node/src/tests.rs | 2 +- radicle-node/src/tests/e2e.rs | 5 +- radicle-node/src/worker/fetch.rs | 5 +- radicle/src/rad.rs | 2 +- radicle/src/storage.rs | 52 +++++++++------- radicle/src/storage/git.rs | 86 ++++++++++++++------------ radicle/src/storage/git/cob.rs | 30 +++++---- radicle/src/test/storage.rs | 42 +++++++------ 18 files changed, 136 insertions(+), 106 deletions(-) diff --git a/radicle-cli/src/commands/clone.rs b/radicle-cli/src/commands/clone.rs index a855e913..d937e6ef 100644 --- a/radicle-cli/src/commands/clone.rs +++ b/radicle-cli/src/commands/clone.rs @@ -18,6 +18,7 @@ use radicle::prelude::*; use radicle::rad; use radicle::storage; use radicle::storage::git::Storage; +use radicle::storage::RemoteRepository as _; use radicle::storage::RepositoryError; use crate::commands::rad_checkout as checkout; diff --git a/radicle-cli/src/commands/patch/list.rs b/radicle-cli/src/commands/patch/list.rs index 08f6966e..ee28cd52 100644 --- a/radicle-cli/src/commands/patch/list.rs +++ b/radicle-cli/src/commands/patch/list.rs @@ -3,6 +3,7 @@ use radicle::cob::patch::{Patch, PatchId, Patches, Verdict}; use radicle::prelude::*; use radicle::profile::Profile; use radicle::storage::git::Repository; +use radicle::storage::RemoteRepository as _; use crate::terminal as term; use term::format::Author; diff --git a/radicle-cli/src/commands/publish.rs b/radicle-cli/src/commands/publish.rs index ac45af76..818c364f 100644 --- a/radicle-cli/src/commands/publish.rs +++ b/radicle-cli/src/commands/publish.rs @@ -5,7 +5,7 @@ use anyhow::{anyhow, Context as _}; use radicle::identity::{Identity, Visibility}; use radicle::node::Handle as _; use radicle::prelude::Id; -use radicle::storage::{ReadRepository, SignRepository, WriteRepository, WriteStorage}; +use radicle::storage::{SignRepository, ValidateRepository, WriteRepository, WriteStorage}; use crate::terminal as term; use crate::terminal::args::{Args, Error, Help}; diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs index fca4ca8a..10fdd0dc 100644 --- a/radicle-cli/tests/commands.rs +++ b/radicle-cli/tests/commands.rs @@ -10,7 +10,7 @@ use radicle::node::Handle as _; use radicle::node::{Alias, DEFAULT_TIMEOUT}; use radicle::prelude::Id; use radicle::profile::Home; -use radicle::storage::{ReadRepository, ReadStorage}; +use radicle::storage::{ReadStorage, RemoteRepository}; use radicle::test::fixtures; use radicle_cli_test::TestFormula; diff --git a/radicle-httpd/src/api/json.rs b/radicle-httpd/src/api/json.rs index cde680a1..9e19b33a 100644 --- a/radicle-httpd/src/api/json.rs +++ b/radicle-httpd/src/api/json.rs @@ -17,7 +17,7 @@ use radicle::cob::{ActorId, Author, Embed, Reaction, Timestamp, Uri}; use radicle::git::RefString; use radicle::node::{Alias, AliasStore}; use radicle::prelude::NodeId; -use radicle::storage::{git, refs, ReadRepository}; +use radicle::storage::{git, refs, RemoteRepository}; use radicle_surf::blob::Blob; use radicle_surf::tree::Tree; use radicle_surf::{Commit, Oid, Stats}; diff --git a/radicle-httpd/src/api/v1/projects.rs b/radicle-httpd/src/api/v1/projects.rs index 69e45efc..829408d7 100644 --- a/radicle-httpd/src/api/v1/projects.rs +++ b/radicle-httpd/src/api/v1/projects.rs @@ -19,7 +19,7 @@ use radicle::node::routing::Store; use radicle::node::AliasStore; use radicle::node::NodeId; use radicle::storage::git::paths; -use radicle::storage::{ReadRepository, ReadStorage, WriteRepository}; +use radicle::storage::{ReadRepository, ReadStorage, RemoteRepository, WriteRepository}; use radicle_surf::{diff, Glob, Oid, Repository}; use crate::api::error::Error; diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index 419ef675..4fe180ad 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -39,7 +39,7 @@ use crate::service::message::{NodeAnnouncement, RefsAnnouncement}; use crate::service::tracking::{store::Write, Scope}; use crate::storage; use crate::storage::{Namespaces, ReadStorage}; -use crate::storage::{ReadRepository, RefUpdate}; +use crate::storage::{ReadRepository, RefUpdate, RemoteRepository as _}; use crate::worker::FetchError; use crate::Link; diff --git a/radicle-node/src/service/message.rs b/radicle-node/src/service/message.rs index c242508b..6f621bbe 100644 --- a/radicle-node/src/service/message.rs +++ b/radicle-node/src/service/message.rs @@ -10,7 +10,7 @@ use crate::service::filter::Filter; use crate::service::{Link, NodeId, Timestamp}; use crate::storage; use crate::storage::refs::SignedRefs; -use crate::storage::{ReadRepository, ReadStorage}; +use crate::storage::{ReadStorage, RemoteRepository as _}; use crate::wire; /// Maximum number of addresses which can be announced to other nodes. diff --git a/radicle-node/src/test/environment.rs b/radicle-node/src/test/environment.rs index f4ffe8c8..871997fb 100644 --- a/radicle-node/src/test/environment.rs +++ b/radicle-node/src/test/environment.rs @@ -29,7 +29,7 @@ use radicle::profile; use radicle::profile::Home; use radicle::profile::Profile; use radicle::rad; -use radicle::storage::{ReadRepository, ReadStorage as _, SignRepository as _}; +use radicle::storage::{ReadStorage as _, RemoteRepository as _, SignRepository as _}; use radicle::test::fixtures; use radicle::Storage; diff --git a/radicle-node/src/test/peer.rs b/radicle-node/src/test/peer.rs index 3e4dbf16..0db729a9 100644 --- a/radicle-node/src/test/peer.rs +++ b/radicle-node/src/test/peer.rs @@ -10,7 +10,7 @@ use radicle::identity::Visibility; use radicle::node::address::Store; use radicle::node::{address, Alias, ConnectOptions}; use radicle::rad; -use radicle::storage::ReadRepository; +use radicle::storage::{ReadRepository, RemoteRepository}; use radicle::Storage; use crate::crypto::test::signer::MockSigner; diff --git a/radicle-node/src/tests.rs b/radicle-node/src/tests.rs index f5dc6e37..70e82f17 100644 --- a/radicle-node/src/tests.rs +++ b/radicle-node/src/tests.rs @@ -11,7 +11,7 @@ use netservices::Direction as Link; use radicle::identity::Visibility; use radicle::node::routing::Store as _; use radicle::node::{ConnectOptions, DEFAULT_TIMEOUT}; -use radicle::storage::ReadRepository; +use radicle::storage::RemoteRepository as _; use crate::collections::{RandomMap, RandomSet}; use crate::crypto::test::signer::MockSigner; diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs index fc9d9c60..bc3547e4 100644 --- a/radicle-node/src/tests/e2e.rs +++ b/radicle-node/src/tests/e2e.rs @@ -3,7 +3,10 @@ use std::{collections::HashSet, thread, time}; use radicle::crypto::{test::signer::MockSigner, Signer}; use radicle::git; use radicle::node::{Alias, FetchResult, Handle as _, DEFAULT_TIMEOUT}; -use radicle::storage::{ReadRepository, ReadStorage, WriteRepository, WriteStorage}; +use radicle::storage::{ + ReadRepository, ReadStorage, RemoteRepository, ValidateRepository, WriteRepository, + WriteStorage, +}; use radicle::test::fixtures; use radicle::{assert_matches, rad}; diff --git a/radicle-node/src/worker/fetch.rs b/radicle-node/src/worker/fetch.rs index fcefd2cc..9067d96c 100644 --- a/radicle-node/src/worker/fetch.rs +++ b/radicle-node/src/worker/fetch.rs @@ -13,7 +13,10 @@ use radicle::prelude::{Doc, Id, NodeId}; use radicle::storage::git::Repository; use radicle::storage::refs::IDENTITY_BRANCH; use radicle::storage::{Namespaces, RefUpdate, Remote, RemoteId, Validation, Validations}; -use radicle::storage::{ReadRepository, ReadStorage, WriteRepository, WriteStorage}; +use radicle::storage::{ + ReadRepository, ReadStorage, RemoteRepository, ValidateRepository, WriteRepository, + WriteStorage, +}; use radicle::{git, Storage}; pub type Refspec = refspec::Refspec; diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index 92af9d39..c7fc0e31 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -351,7 +351,7 @@ mod tests { use crate::identity::Did; use crate::storage::git::transport; use crate::storage::git::Storage; - use crate::storage::ReadStorage; + use crate::storage::{ReadStorage, RemoteRepository as _}; use crate::test::fixtures; use super::*; diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index 581da9d0..ef32e5d0 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -345,7 +345,7 @@ pub trait WriteStorage: ReadStorage { } /// Allows read-only access to a repository. -pub trait ReadRepository: Sized { +pub trait ReadRepository: Sized + ValidateRepository { /// Return the repository id. fn id(&self) -> Id; @@ -361,21 +361,6 @@ pub trait ReadRepository: Sized { /// Get a blob in this repository, given its id. fn blob(&self, oid: Oid) -> Result; - /// Validate all remotes with [`ReadRepository::validate_remote`]. - fn validate(&self) -> Result { - let mut failures = Validations::default(); - for (_, remote) in self.remotes()? { - failures.append(&mut self.validate_remote(&remote)?); - } - Ok(failures) - } - - /// Validates a remote's signed refs and identity. - /// - /// Returns any ref found under that remote that isn't signed. - /// If a signed ref is missing from the repository, an error is returned. - fn validate_remote(&self, remote: &Remote) -> Result; - /// Get the head of this repository. /// /// Returns the reference pointed to by `HEAD` if it is set. Otherwise, computes the canonical @@ -467,12 +452,6 @@ pub trait ReadRepository: Sized { pattern: &git::PatternStr, ) -> Result, git::ext::Error>; - /// Get the given remote. - fn remote(&self, remote: &RemoteId) -> Result, refs::Error>; - - /// Get all remotes. - fn remotes(&self) -> Result, refs::Error>; - /// Get repository delegates. fn delegates(&self) -> Result, RepositoryError> { let doc: Doc<_> = self.identity_doc()?.into(); @@ -495,6 +474,35 @@ pub trait ReadRepository: Sized { fn merge_base(&self, left: &Oid, right: &Oid) -> Result; } +/// Access the remotes of a repository. +pub trait RemoteRepository { + /// Get the given remote. + fn remote(&self, remote: &RemoteId) -> Result, refs::Error>; + + /// Get all remotes. + fn remotes(&self) -> Result, refs::Error>; +} + +pub trait ValidateRepository +where + Self: RemoteRepository, +{ + /// Validate all remotes with [`ValidateRepository::validate_remote`]. + fn validate(&self) -> Result { + let mut failures = Validations::default(); + for (_, remote) in self.remotes()? { + failures.append(&mut self.validate_remote(&remote)?); + } + Ok(failures) + } + + /// Validates a remote's signed refs and identity. + /// + /// Returns any ref found under that remote that isn't signed. + /// If a signed ref is missing from the repository, an error is returned. + fn validate_remote(&self, remote: &Remote) -> Result; +} + /// Allows read-write access to a repository. pub trait WriteRepository: ReadRepository + SignRepository { /// Set the repository head to the canonical branch. diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index c77a9fb6..1539ad63 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -25,7 +25,7 @@ use crate::storage::{ pub use crate::git::*; pub use crate::storage::Error; -use super::RemoteId; +use super::{RemoteId, RemoteRepository, ValidateRepository}; pub static NAMESPACES_GLOB: Lazy = Lazy::new(|| git::refspec::pattern!("refs/namespaces/*")); @@ -394,37 +394,22 @@ impl Repository { } } -impl ReadRepository for Repository { - fn id(&self) -> Id { - self.id +impl RemoteRepository for Repository { + fn remotes(&self) -> Result, refs::Error> { + let mut remotes = Vec::new(); + for remote in Repository::remotes(self)? { + remotes.push(remote?); + } + Ok(Remotes::from_iter(remotes)) } - fn is_empty(&self) -> Result { - Ok(self.remotes()?.next().is_none()) - } - - fn path(&self) -> &Path { - self.backend.path() - } - - fn blob_at>(&self, commit: Oid, path: P) -> Result { - let commit = self.backend.find_commit(*commit)?; - let tree = commit.tree()?; - let entry = tree.get_path(path.as_ref())?; - let obj = entry.to_object(&self.backend)?; - let blob = obj.into_blob().map_err(|_| { - git::Error::NotFound(git::NotFound::NoSuchBlob( - path.as_ref().display().to_string(), - )) - })?; - - Ok(blob) - } - - fn blob(&self, oid: Oid) -> Result { - self.backend.find_blob(oid.into()).map_err(git::Error::from) + fn remote(&self, remote: &RemoteId) -> Result, refs::Error> { + let refs = SignedRefs::load(*remote, self)?; + Ok(Remote::::new(refs)) } +} +impl ValidateRepository for Repository { fn validate_remote(&self, remote: &Remote) -> Result { // Contains a copy of the signed refs of this remote. let mut signed = BTreeMap::from((*remote.refs).clone()); @@ -469,6 +454,38 @@ impl ReadRepository for Repository { Ok(failures) } +} + +impl ReadRepository for Repository { + fn id(&self) -> Id { + self.id + } + + fn is_empty(&self) -> Result { + Ok(self.remotes()?.next().is_none()) + } + + fn path(&self) -> &Path { + self.backend.path() + } + + fn blob_at>(&self, commit: Oid, path: P) -> Result { + let commit = self.backend.find_commit(*commit)?; + let tree = commit.tree()?; + let entry = tree.get_path(path.as_ref())?; + let obj = entry.to_object(&self.backend)?; + let blob = obj.into_blob().map_err(|_| { + git::Error::NotFound(git::NotFound::NoSuchBlob( + path.as_ref().display().to_string(), + )) + })?; + + Ok(blob) + } + + fn blob(&self, oid: Oid) -> Result { + self.backend.find_blob(oid.into()).map_err(git::Error::from) + } fn reference( &self, @@ -509,11 +526,6 @@ impl ReadRepository for Repository { .map_err(git::Error::from) } - fn remote(&self, remote: &RemoteId) -> Result, refs::Error> { - let refs = SignedRefs::load(*remote, self)?; - Ok(Remote::::new(refs)) - } - fn references_of(&self, remote: &RemoteId) -> Result { let entries = self .backend @@ -563,14 +575,6 @@ impl ReadRepository for Repository { Ok(refs) } - fn remotes(&self) -> Result, refs::Error> { - let mut remotes = Vec::new(); - for remote in Repository::remotes(self)? { - remotes.push(remote?); - } - Ok(Remotes::from_iter(remotes)) - } - fn identity_doc_at(&self, head: Oid) -> Result { Doc::::load_at(head, self) } diff --git a/radicle/src/storage/git/cob.rs b/radicle/src/storage/git/cob.rs index abeb20e9..b7275a3d 100644 --- a/radicle/src/storage/git/cob.rs +++ b/radicle/src/storage/git/cob.rs @@ -5,8 +5,10 @@ use std::path::Path; use cob::object::Objects; use radicle_cob as cob; use radicle_cob::change; +use storage::RemoteRepository; use storage::RepositoryError; use storage::SignRepository; +use storage::ValidateRepository; use crate::git::*; use crate::storage; @@ -231,6 +233,22 @@ impl<'a, R: storage::ReadRepository> SignRepository for DraftStore<'a, R> { } } +impl<'a, R: storage::RemoteRepository> RemoteRepository for DraftStore<'a, R> { + fn remote(&self, id: &RemoteId) -> Result, storage::refs::Error> { + self.repo.remote(id) + } + + fn remotes(&self) -> Result, storage::refs::Error> { + RemoteRepository::remotes(self.repo) + } +} + +impl<'a, R: storage::ValidateRepository> ValidateRepository for DraftStore<'a, R> { + fn validate_remote(&self, remote: &Remote) -> Result { + self.repo.validate_remote(remote) + } +} + impl<'a, R: storage::ReadRepository> ReadRepository for DraftStore<'a, R> { fn id(&self) -> identity::Id { self.repo.id() @@ -248,22 +266,10 @@ impl<'a, R: storage::ReadRepository> ReadRepository for DraftStore<'a, R> { self.repo.canonical_head() } - fn validate_remote(&self, remote: &Remote) -> Result { - self.repo.validate_remote(remote) - } - fn path(&self) -> &std::path::Path { self.repo.path() } - fn remote(&self, id: &RemoteId) -> Result, storage::refs::Error> { - self.repo.remote(id) - } - - fn remotes(&self) -> Result, storage::refs::Error> { - ReadRepository::remotes(self.repo) - } - fn commit(&self, oid: Oid) -> Result { self.repo.commit(oid) } diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index 253313dd..e04a6e15 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -124,6 +124,29 @@ impl MockRepository { } } +impl RemoteRepository for MockRepository { + fn remote(&self, id: &RemoteId) -> Result, refs::Error> { + self.remotes + .get(id) + .map(|refs| Remote { refs: refs.clone() }) + .ok_or(refs::Error::InvalidRef) + } + + fn remotes(&self) -> Result, refs::Error> { + Ok(self + .remotes + .iter() + .map(|(id, refs)| (*id, Remote { refs: refs.clone() })) + .collect()) + } +} + +impl ValidateRepository for MockRepository { + fn validate_remote(&self, _remote: &Remote) -> Result { + Ok(Validations::default()) + } +} + impl ReadRepository for MockRepository { fn id(&self) -> Id { self.id @@ -141,29 +164,10 @@ impl ReadRepository for MockRepository { todo!() } - fn validate_remote(&self, _remote: &Remote) -> Result { - Ok(Validations::default()) - } - fn path(&self) -> &std::path::Path { todo!() } - fn remote(&self, id: &RemoteId) -> Result, refs::Error> { - self.remotes - .get(id) - .map(|refs| Remote { refs: refs.clone() }) - .ok_or(refs::Error::InvalidRef) - } - - fn remotes(&self) -> Result, refs::Error> { - Ok(self - .remotes - .iter() - .map(|(id, refs)| (*id, Remote { refs: refs.clone() })) - .collect()) - } - fn commit(&self, _oid: Oid) -> Result { todo!() }