From 4634913b66bfc5bc82f354ced8ac059808d45615 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Fri, 23 Jun 2023 15:06:49 +0200 Subject: [PATCH] radicle: Split out `SignRepository` trait This new trait is used when only signing refs is needed, and not general write access to a repository. We also implement `ReadRepository` and `SignRepository` for `DraftStore`. --- radicle-node/src/test/environment.rs | 2 +- radicle-remote-helper/src/push.rs | 4 +- radicle-tools/src/rad-push.rs | 5 +- radicle/src/cob/patch.rs | 135 ++++++++++++------------ radicle/src/cob/store.rs | 56 +++++----- radicle/src/lib.rs | 4 +- radicle/src/rad.rs | 2 +- radicle/src/storage.rs | 10 +- radicle/src/storage/git.rs | 15 +-- radicle/src/storage/git/cob.rs | 152 +++++++++++++++++++++++++-- radicle/src/test/storage.rs | 10 +- 11 files changed, 279 insertions(+), 116 deletions(-) diff --git a/radicle-node/src/test/environment.rs b/radicle-node/src/test/environment.rs index c8962549..c17153ec 100644 --- a/radicle-node/src/test/environment.rs +++ b/radicle-node/src/test/environment.rs @@ -27,7 +27,7 @@ use radicle::profile; use radicle::profile::Home; use radicle::profile::Profile; use radicle::rad; -use radicle::storage::{ReadRepository, ReadStorage as _, WriteRepository}; +use radicle::storage::{ReadRepository, ReadStorage as _, SignRepository as _}; use radicle::test::fixtures; use radicle::Storage; diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index 68040ff9..81b3e97c 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -11,10 +11,10 @@ use radicle::crypto::{PublicKey, Signer}; use radicle::node; use radicle::node::{Handle, NodeId}; use radicle::prelude::Id; +use radicle::storage; use radicle::storage::git::cob::object::ParseObjectId; use radicle::storage::git::transport::local::Url; -use radicle::storage::WriteRepository; -use radicle::storage::{self, ReadRepository}; +use radicle::storage::{ReadRepository, SignRepository as _, WriteRepository}; use radicle::Profile; use radicle::{git, rad}; use radicle_cli::terminal as cli; diff --git a/radicle-tools/src/rad-push.rs b/radicle-tools/src/rad-push.rs index a50e4d45..3f0e7eb6 100644 --- a/radicle-tools/src/rad-push.rs +++ b/radicle-tools/src/rad-push.rs @@ -1,6 +1,9 @@ use std::path::Path; -use radicle::{node::Handle, storage::ReadStorage, storage::WriteRepository}; +use radicle::{ + node::Handle, + storage::{ReadStorage, SignRepository, WriteRepository}, +}; fn main() -> anyhow::Result<()> { let cwd = Path::new(".").canonicalize()?; diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index e6205bb3..4523a238 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -1152,7 +1152,7 @@ pub struct PatchMut<'a, 'g, R> { impl<'a, 'g, R> PatchMut<'a, 'g, R> where - R: WriteRepository + cob::Store, + R: ReadRepository + SignRepository + cob::Store, { pub fn new( id: ObjectId, @@ -1392,7 +1392,7 @@ impl<'a, R> Deref for Patches<'a, R> { impl<'a, R> Patches<'a, R> where - R: WriteRepository + cob::Store, + R: ReadRepository + cob::Store, { /// Open an patches store. pub fn open(repository: &'a R) -> Result { @@ -1401,6 +1401,74 @@ where Ok(Self { raw }) } + /// Patches count by state. + pub fn counts(&self) -> Result { + let all = self.all()?; + let state_groups = + all.filter_map(|s| s.ok()) + .fold(PatchCounts::default(), |mut state, (_, p, _)| { + match p.state() { + State::Draft => state.draft += 1, + State::Open { .. } => state.open += 1, + State::Archived => state.archived += 1, + State::Merged { .. } => state.merged += 1, + } + state + }); + + Ok(state_groups) + } + + /// Find the `Patch` containing the given `Revision`. + pub fn find_by_revision( + &self, + id: &RevisionId, + ) -> Result, Error> { + // Revision may be the patch's first, making it have the same ID. + let p_id = ObjectId::from(id); + if let Some(p) = self.get(&p_id)? { + return Ok(p.revision(id).map(|r| (p_id, p.clone(), r.clone()))); + } + + let result = self + .all()? + .filter_map(|result| result.ok()) + .find_map(|(p_id, p, _)| p.revision(id).map(|r| (p_id, p.clone(), r.clone()))); + Ok(result) + } + + /// Get a patch. + pub fn get(&self, id: &ObjectId) -> Result, store::Error> { + self.raw.get(id).map(|r| r.map(|(p, _)| p)) + } + + /// Get proposed patches. + pub fn proposed( + &self, + ) -> Result + '_, Error> { + let all = self.all()?; + + Ok(all + .into_iter() + .filter_map(|result| result.ok()) + .filter(|(_, p, _)| p.is_open())) + } + + /// Get patches proposed by the given key. + pub fn proposed_by<'b>( + &'b self, + who: &'b Did, + ) -> Result + '_, Error> { + Ok(self + .proposed()? + .filter(move |(_, p, _)| p.author().id() == who)) + } +} + +impl<'a, R> Patches<'a, R> +where + R: ReadRepository + SignRepository + cob::Store, +{ /// Open a new patch. pub fn create<'g, G: Signer>( &'g mut self, @@ -1447,47 +1515,6 @@ where ) } - /// Patches count by state. - pub fn counts(&self) -> Result { - let all = self.all()?; - let state_groups = - all.filter_map(|s| s.ok()) - .fold(PatchCounts::default(), |mut state, (_, p, _)| { - match p.state() { - State::Draft => state.draft += 1, - State::Open { .. } => state.open += 1, - State::Archived => state.archived += 1, - State::Merged { .. } => state.merged += 1, - } - state - }); - - Ok(state_groups) - } - - /// Find the `Patch` containing the given `Revision`. - pub fn find_by_revision( - &self, - id: &RevisionId, - ) -> Result, Error> { - // Revision may be the patch's first, making it have the same ID. - let p_id = ObjectId::from(id); - if let Some(p) = self.get(&p_id)? { - return Ok(p.revision(id).map(|r| (p_id, p.clone(), r.clone()))); - } - - let result = self - .all()? - .filter_map(|result| result.ok()) - .find_map(|(p_id, p, _)| p.revision(id).map(|r| (p_id, p.clone(), r.clone()))); - Ok(result) - } - - /// Get a patch. - pub fn get(&self, id: &ObjectId) -> Result, store::Error> { - self.raw.get(id).map(|r| r.map(|(p, _)| p)) - } - /// Get a patch mutably. pub fn get_mut<'g>(&'g mut self, id: &ObjectId) -> Result, store::Error> { let (patch, clock) = self @@ -1503,28 +1530,6 @@ where }) } - /// Get proposed patches. - pub fn proposed( - &self, - ) -> Result + 'a, Error> { - let all = self.all()?; - - Ok(all - .into_iter() - .filter_map(|result| result.ok()) - .filter(|(_, p, _)| p.is_open())) - } - - /// Get patches proposed by the given key. - pub fn proposed_by<'b>( - &'b self, - who: &'b Did, - ) -> Result + '_, Error> { - Ok(self - .proposed()? - .filter(move |(_, p, _)| p.author().id() == who)) - } - /// Create a patch. This is an internal function used by `create` and `draft`. fn _create<'g, G: Signer>( &'g mut self, diff --git a/radicle/src/cob/store.rs b/radicle/src/cob/store.rs index 54227d1a..db4e76fe 100644 --- a/radicle/src/cob/store.rs +++ b/radicle/src/cob/store.rs @@ -14,6 +14,7 @@ use crate::cob::{ActorId, Create, EntryId, History, ObjectId, TypeName, Update, use crate::git; use crate::prelude::*; use crate::storage::git as storage; +use crate::storage::SignRepository; use crate::{cob, identity}; /// History type for standard radicle COBs. @@ -155,7 +156,7 @@ impl<'a, T, R: ReadRepository> Store<'a, T, R> { impl<'a, T, R> Store<'a, T, R> where - R: WriteRepository + cob::Store, + R: ReadRepository + SignRepository + cob::Store, T: FromHistory, T::Action: Serialize, { @@ -220,6 +221,34 @@ where Ok((*cob.id(), object, clock)) } + /// Remove an object. + pub fn remove(&self, id: &ObjectId, signer: &G) -> Result<(), Error> { + let name = git::refs::storage::cob(signer.public_key(), T::type_name(), id); + match self + .repo + .reference_oid(signer.public_key(), &name.strip_namespace()) + { + Ok(_) => { + cob::remove(self.repo, signer.public_key(), T::type_name(), id)?; + self.repo.sign_refs(signer).map_err(Error::SignRefs)?; + Ok(()) + } + Err(git::Error::NotFound(_)) => Ok(()), + Err(git::Error::Git(err)) if err.code() == git::raw::ErrorCode::NotFound => Ok(()), + Err(err) => Err(Error::RefLookup { + name: name.to_ref_string(), + err, + }), + } + } +} + +impl<'a, T, R> Store<'a, T, R> +where + R: ReadRepository + cob::Store, + T: FromHistory, + T::Action: Serialize, +{ /// Get an object. pub fn get(&self, id: &ObjectId) -> Result, Error> { let cob = cob::get(self.repo, T::type_name(), id)?; @@ -259,27 +288,6 @@ where Ok(raw.len()) } - - /// Remove an object. - pub fn remove(&self, id: &ObjectId, signer: &G) -> Result<(), Error> { - let name = git::refs::storage::cob(signer.public_key(), T::type_name(), id); - match self - .repo - .reference_oid(signer.public_key(), &name.strip_namespace()) - { - Ok(_) => { - cob::remove(self.repo, signer.public_key(), T::type_name(), id)?; - self.repo.sign_refs(signer).map_err(Error::SignRefs)?; - Ok(()) - } - Err(git::Error::NotFound(_)) => Ok(()), - Err(git::Error::Git(err)) if err.code() == git::raw::ErrorCode::NotFound => Ok(()), - Err(err) => Err(Error::RefLookup { - name: name.to_ref_string(), - err, - }), - } - } } /// Allows operations to be batched atomically. @@ -310,7 +318,7 @@ impl Transaction { where G: Signer, F: FnOnce(&mut Self) -> Result<(), Error>, - R: WriteRepository + cob::Store, + R: ReadRepository + SignRepository + cob::Store, T::Action: Serialize + Clone, { let actor = *signer.public_key(); @@ -350,7 +358,7 @@ impl Transaction { signer: &G, ) -> Result<(Vec>, Lamport, EntryId), Error> where - R: WriteRepository + cob::Store, + R: ReadRepository + SignRepository + cob::Store, T::Action: Serialize + Clone, { let actions = NonEmpty::from_vec(self.actions) diff --git a/radicle/src/lib.rs b/radicle/src/lib.rs index f1c62423..5bfcdefd 100644 --- a/radicle/src/lib.rs +++ b/radicle/src/lib.rs @@ -33,7 +33,9 @@ pub mod prelude { pub use identity::{project::Project, Did, Doc, Id}; pub use node::{Alias, NodeId, Timestamp}; pub use profile::Profile; - pub use storage::{BranchName, ReadRepository, ReadStorage, WriteRepository, WriteStorage}; + pub use storage::{ + BranchName, ReadRepository, ReadStorage, SignRepository, WriteRepository, WriteStorage, + }; } pub mod env { diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index 8de07480..4f7f8b30 100644 --- a/radicle/src/rad.rs +++ b/radicle/src/rad.rs @@ -15,7 +15,7 @@ use crate::identity::{doc, IdentityError}; use crate::storage::git::transport; use crate::storage::git::Repository; use crate::storage::refs::SignedRefs; -use crate::storage::{BranchName, ReadRepository as _, RemoteId}; +use crate::storage::{BranchName, ReadRepository as _, RemoteId, SignRepository as _}; use crate::storage::{WriteRepository, WriteStorage}; use crate::{identity, storage}; diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs index aedf66ba..fa92f968 100644 --- a/radicle/src/storage.rs +++ b/radicle/src/storage.rs @@ -450,18 +450,22 @@ pub trait ReadRepository: Sized { } /// Allows read-write access to a repository. -pub trait WriteRepository: ReadRepository { +pub trait WriteRepository: ReadRepository + SignRepository { /// Set the repository head to the canonical branch. /// This computes the head based on the delegate set. fn set_head(&self) -> Result; /// Set the repository 'rad/id' to the canonical commit, agreed by quorum. fn set_identity_head(&self) -> Result; - /// Sign the repository's refs under the `refs/rad/sigrefs` branch. - fn sign_refs(&self, signer: &G) -> Result, Error>; /// Get the underlying git repository. fn raw(&self) -> &git2::Repository; } +/// Allows signing refs. +pub trait SignRepository { + /// Sign the repository's refs under the `refs/rad/sigrefs` branch. + fn sign_refs(&self, signer: &G) -> Result, Error>; +} + impl ReadStorage for T where T: Deref, diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 2807c12b..d35ee44b 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -16,7 +16,8 @@ use crate::identity::{Identity, IdentityError, Project}; use crate::storage::refs; use crate::storage::refs::{Refs, SignedRefs}; use crate::storage::{ - Inventory, ReadRepository, ReadStorage, Remote, Remotes, WriteRepository, WriteStorage, + Inventory, ReadRepository, ReadStorage, Remote, Remotes, SignRepository, WriteRepository, + WriteStorage, }; pub use crate::git::*; @@ -628,6 +629,12 @@ impl WriteRepository for Repository { Ok(head) } + fn raw(&self) -> &git2::Repository { + &self.backend + } +} + +impl SignRepository for Repository { fn sign_refs(&self, signer: &G) -> Result, Error> { let remote = signer.public_key(); let refs = self.references_of(remote)?; @@ -637,10 +644,6 @@ impl WriteRepository for Repository { Ok(signed) } - - fn raw(&self) -> &git2::Repository { - &self.backend - } } pub mod trailers { @@ -703,7 +706,7 @@ mod tests { use super::*; use crate::git; use crate::storage::refs::SIGREFS_BRANCH; - use crate::storage::{ReadRepository, ReadStorage, WriteRepository}; + use crate::storage::{ReadRepository, ReadStorage}; use crate::test::arbitrary; use crate::test::fixtures; diff --git a/radicle/src/storage/git/cob.rs b/radicle/src/storage/git/cob.rs index 7e2ba5e0..b5af2f12 100644 --- a/radicle/src/storage/git/cob.rs +++ b/radicle/src/storage/git/cob.rs @@ -4,10 +4,18 @@ use std::collections::BTreeMap; use cob::object::Objects; use radicle_cob as cob; use radicle_cob::change; +use storage::SignRepository; -use crate::git; +use crate::storage; use crate::storage::Error; -use crate::storage::ReadRepository; +use crate::storage::{ + git::{Remote, Remotes, VerifyError}, + ReadRepository, Verified, +}; +use crate::{ + git, identity, + identity::{doc::DocError, IdentityError}, +}; pub use crate::git::*; pub use cob::*; @@ -165,14 +173,24 @@ impl cob::object::Storage for Repository { } /// Stores draft collaborative objects. -pub struct DraftStore { +/// +// This storage backend for COBs stores changes in a `draft/cobs/*` namespace, +// which allows for some of the features needed for code review. For +// example, users can draft comments and later decide to publish them. +pub struct DraftStore<'a> { remote: RemoteId, - repo: Repository, + repo: &'a Repository, } -impl cob::Store for DraftStore {} +impl<'a> DraftStore<'a> { + pub fn new(remote: RemoteId, repo: &'a Repository) -> Self { + Self { remote, repo } + } +} -impl change::Storage for DraftStore { +impl<'a> cob::Store for DraftStore<'a> {} + +impl<'a> change::Storage for DraftStore<'a> { type StoreError = ::StoreError; type LoadError = ::LoadError; @@ -202,7 +220,125 @@ impl change::Storage for DraftStore { } } -impl cob::object::Storage for DraftStore { +impl<'a> SignRepository for DraftStore<'a> { + fn sign_refs( + &self, + signer: &G, + ) -> Result, Error> { + self.repo.sign_refs(signer) + } +} + +impl<'a> ReadRepository for DraftStore<'a> { + fn id(&self) -> identity::Id { + self.repo.id() + } + + fn is_empty(&self) -> Result { + self.repo.is_empty() + } + + fn head(&self) -> Result<(fmt::Qualified, Oid), identity::IdentityError> { + self.repo.head() + } + + fn canonical_head(&self) -> Result<(fmt::Qualified, Oid), identity::IdentityError> { + self.repo.canonical_head() + } + + fn validate_remote( + &self, + remote: &Remote, + ) -> Result, VerifyError> { + 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) + } + + fn revwalk(&self, head: Oid) -> Result { + self.repo.revwalk(head) + } + + fn is_ancestor_of(&self, ancestor: Oid, head: Oid) -> Result { + self.repo.is_ancestor_of(ancestor, head) + } + + fn blob_at<'b>( + &'b self, + oid: git_ext::Oid, + path: &'b std::path::Path, + ) -> Result, git_ext::Error> { + self.repo.blob_at(oid, path) + } + + fn reference( + &self, + remote: &RemoteId, + reference: &git::Qualified, + ) -> Result { + self.repo.reference(remote, reference) + } + + fn reference_oid( + &self, + remote: &RemoteId, + reference: &git::Qualified, + ) -> Result { + self.repo.reference_oid(remote, reference) + } + + fn references_of(&self, remote: &RemoteId) -> Result { + self.repo.references_of(remote) + } + + fn references_glob( + &self, + pattern: &git::PatternStr, + ) -> Result, git::ext::Error> { + self.repo.references_glob(pattern) + } + + fn identity_doc( + &self, + ) -> Result<(Oid, crate::identity::Doc), IdentityError> { + self.repo.identity_doc() + } + + fn identity_doc_at( + &self, + head: Oid, + ) -> Result, DocError> { + self.repo.identity_doc_at(head) + } + + fn identity_head(&self) -> Result { + self.repo.identity_head() + } + + fn canonical_identity_head(&self) -> Result { + self.repo.canonical_identity_head() + } + + fn merge_base(&self, left: &Oid, right: &Oid) -> Result { + self.repo.merge_base(left, right) + } +} + +impl<'a> cob::object::Storage for DraftStore<'a> { type ObjectsError = ObjectsError; type TypesError = git::ext::Error; type UpdateError = git2::Error; @@ -276,6 +412,6 @@ impl cob::object::Storage for DraftStore { git::refs::storage::draft::cob(identifier, typename, object_id).as_str(), )?; - reference.delete().map_err(Self::RemoveError::from) + reference.delete() } } diff --git a/radicle/src/test/storage.rs b/radicle/src/test/storage.rs index 1af656e4..586f391a 100644 --- a/radicle/src/test/storage.rs +++ b/radicle/src/test/storage.rs @@ -248,14 +248,16 @@ impl WriteRepository for MockRepository { todo!() } + fn set_identity_head(&self) -> Result { + todo!() + } +} + +impl SignRepository for MockRepository { fn sign_refs( &self, _signer: &G, ) -> Result, Error> { todo!() } - - fn set_identity_head(&self) -> Result { - todo!() - } }