diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs index 735b81a4..46156639 100644 --- a/radicle-cli/tests/commands.rs +++ b/radicle-cli/tests/commands.rs @@ -612,7 +612,12 @@ fn test_cob_deletion() { let alice_issues = radicle::cob::issue::Issues::open(&alice_repo).unwrap(); alice_issues.remove(issue_id, &alice.signer).unwrap(); - bob.handle.fetch(rid, alice.id).unwrap(); + log::debug!(target: "test", "Removing issue.."); + + radicle::assert_matches!( + bob.handle.fetch(rid, alice.id).unwrap(), + radicle::node::FetchResult::Success { .. } + ); let bob_repo = bob.storage.repository(rid).unwrap(); let bob_issues = radicle::cob::issue::Issues::open(&bob_repo).unwrap(); assert!(bob_issues.get(issue_id).unwrap().is_none()); diff --git a/radicle-crypto/src/lib.rs b/radicle-crypto/src/lib.rs index a975f0b5..e41a5646 100644 --- a/radicle-crypto/src/lib.rs +++ b/radicle-crypto/src/lib.rs @@ -364,6 +364,13 @@ impl PublicKey { pub fn to_namespace(&self) -> git_ref_format::RefString { git_ref_format::refname!("refs/namespaces").join(git_ref_format::Component::from(self)) } + + #[cfg(feature = "git-ref-format")] + pub fn from_namespaced(refstr: &git_ref_format::Namespaced) -> Result { + let name = refstr.namespace().into_inner(); + + Self::from_str(name.deref().as_str()) + } } impl FromStr for PublicKey { diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs index 9ad37233..668f589a 100644 --- a/radicle-node/src/tests/e2e.rs +++ b/radicle-node/src/tests/e2e.rs @@ -736,7 +736,6 @@ fn test_connection_crossing() { } #[test] -#[should_panic] /// Alice is going to try to fetch outdated refs of Bob, from Eve. This is a non-fastfoward fetch /// on the sigrefs branch. fn test_non_fastforward_sigrefs() { @@ -791,7 +790,6 @@ fn test_non_fastforward_sigrefs() { } #[test] -#[should_panic] fn test_outdated_sigrefs() { logger::init(log::Level::Debug); @@ -850,6 +848,7 @@ fn test_outdated_sigrefs() { let eves_refs = repo.remote(&eve.id).unwrap().refs; // Get the current state of eve's refs in alice's storage + log::debug!(target: "test", "Alice fetches from Eve.."); assert_matches!( alice.handle.fetch(rid, eve.id).unwrap(), FetchResult::Success { .. } @@ -860,6 +859,8 @@ fn test_outdated_sigrefs() { assert_ne!(eves_refs_expected, old_refs); assert_eq!(eves_refs_expected, eves_refs); + log::debug!(target: "test", "Alice fetches from Bob.."); + alice .handle .track_node(bob.id, Some("bob".to_string())) @@ -869,7 +870,7 @@ fn test_outdated_sigrefs() { FetchResult::Success { .. } ); - // Ensure that bob's refs have not changed + // Ensure that Eve's refs have not changed after fetching the old refs from Bob. let repo = alice.storage.repository(rid).unwrap(); let eve_remote = repo.remote(&eve.id).unwrap(); let eves_refs = eve_remote.refs; diff --git a/radicle-node/src/worker.rs b/radicle-node/src/worker.rs index 8f3e6745..e6e34690 100644 --- a/radicle-node/src/worker.rs +++ b/radicle-node/src/worker.rs @@ -229,7 +229,8 @@ impl Worker { namespaces: &Namespaces, mut channels: Channels, ) -> Result<(Vec, HashSet), FetchError> { - let staging = fetch::StagingPhaseInitial::new(&self.storage, rid, namespaces.clone())?; + let staging = + fetch::StagingPhaseInitial::new(&self.storage, rid, self.nid, namespaces.clone())?; let refs = if staging.repo.is_cloning() { match self._fetch( &staging.repo, diff --git a/radicle-node/src/worker/fetch.rs b/radicle-node/src/worker/fetch.rs index de89d6fc..37cd3ab9 100644 --- a/radicle-node/src/worker/fetch.rs +++ b/radicle-node/src/worker/fetch.rs @@ -28,6 +28,8 @@ pub struct StagingPhaseInitial<'a> { pub(super) repo: StagedRepository, /// The original [`Storage`] we are finalising changes into. production: &'a Storage, + /// The local Node ID. + nid: NodeId, /// The `Namespaces` passed by the fetching caller. pub(super) namespaces: Namespaces, _tmp: tempfile::TempDir, @@ -101,6 +103,8 @@ pub struct StagingPhaseFinal<'a> { pub(super) repo: FinalStagedRepository, /// The original [`Storage`] we are finalising changes into. production: &'a Storage, + /// The local Node ID. + nid: NodeId, _tmp: tempfile::TempDir, } @@ -115,6 +119,7 @@ enum VerifiedRemote { /// Unsigned refs. unsigned: Vec, }, + UpToDate, } impl<'a> StagingPhaseInitial<'a> { @@ -123,6 +128,7 @@ impl<'a> StagingPhaseInitial<'a> { pub fn new( production: &'a Storage, rid: Id, + nid: NodeId, namespaces: Namespaces, ) -> Result { let tmp = tempfile::TempDir::new()?; @@ -131,6 +137,7 @@ impl<'a> StagingPhaseInitial<'a> { let repo = Self::repository(&staging, production, rid)?; Ok(Self { repo, + nid, production, namespaces, _tmp: tmp, @@ -194,6 +201,7 @@ impl<'a> StagingPhaseInitial<'a> { Ok(StagingPhaseFinal { repo, + nid: self.nid, production: self.production, _tmp: self._tmp, }) @@ -273,27 +281,45 @@ impl<'a> StagingPhaseFinal<'a> { /// All references that were updated are returned as a /// [`RefUpdate`]. pub fn transfer(self) -> Result<(Vec, HashSet), error::Transfer> { - let verifications = self.verify()?; - let production = match &self.repo { - FinalStagedRepository::Cloning { repo, .. } => self.production.create(repo.id)?, - FinalStagedRepository::Fetching { repo, .. } => self.production.repository(repo.id)?, + // Nb. we have to verify in a different order when fetching vs. cloning, due to needing + // access to the existing repository in the fetching case. + let (production, verifications) = match &self.repo { + FinalStagedRepository::Cloning { repo, .. } => { + let verifications = self.verify::(None)?; + let prod = self.production.create(repo.id)?; + + (prod, verifications) + } + FinalStagedRepository::Fetching { repo, .. } => { + let prod = self.production.repository(repo.id)?; + let verifications = self.verify(Some(&prod))?; + + (prod, verifications) + } }; let url = url::File::new(self.repo.path().to_path_buf()).to_string(); let mut remote = production.backend.remote_anonymous(&url)?; let mut updates = Vec::new(); let mut delete = HashSet::new(); + let mut skipped = HashSet::new(); let callbacks = ref_updates(&mut updates); - let remotes = { + let mut remotes = { let specs = verifications .into_iter() .flat_map(|(remote, verified)| match verified { + VerifiedRemote::UpToDate => { + log::debug!(target: "worker", "{remote} is up-to-date"); + skipped.insert(remote); + + vec![] + } VerifiedRemote::Failed { reason } => { // TODO: We should include the skipped remotes in the fetch result, // with the reason why they're skipped. log::warn!( target: "worker", - "{remote} failed to verify, will not fetch any further refs: {reason}", + "{remote} failed to verify, ignoring ref updates: {reason}", ); vec![] } @@ -356,11 +382,12 @@ impl<'a> StagingPhaseFinal<'a> { let (fetching, specs): (HashSet<_>, Vec<_>) = specs.into_iter().unzip(); - if !self - .repo - .delegates()? - .iter() - .all(|d| fetching.contains(d.as_key())) + if self.repo.is_cloning() + && !self + .repo + .delegates()? + .iter() + .all(|d| fetching.contains(d.as_key())) { return Err(error::Transfer::NoDelegates); } @@ -408,6 +435,10 @@ impl<'a> StagingPhaseFinal<'a> { production.id, ); + // Extend the list of remotes we attempted to fetch from with the skipped remotes. + // This confirms to the user that the remote was indeed tried. + remotes.extend(skipped); + Ok((updates, remotes)) } @@ -418,17 +449,68 @@ impl<'a> StagingPhaseFinal<'a> { .iter() .filter_map(|remote| self.repo.remote(remote).ok()), )), - FinalStagedRepository::Fetching { repo, .. } => Ok(Box::new( - repo.remotes()?.filter_map(|r| r.ok().map(|(_, r)| r)), - )), + FinalStagedRepository::Fetching { repo, refs } => { + // Only verify remotes we're fetching refs from. + let remotes = refs + .iter() + .filter_map(|r| NodeId::from_namespaced(r).ok()) + .collect::>(); + let remotes = remotes.into_iter().filter_map(|r| repo.remote(&r).ok()); + + Ok(Box::new(remotes)) + } } } - fn verify(&self) -> Result, git::raw::Error> { - Ok(self + fn verify( + &self, + local: Option<&R>, + ) -> Result, git::raw::Error> { + let result = self .remotes()? + .filter(|remote| remote.id != self.nid || self.repo.is_cloning()) .map(|remote| { let remote_id = remote.id; + + log::debug!(target: "worker", "Verifying remote {remote_id}.."); + + // If we have a local copy, ie. we're not cloning, we check that the signed refs + // are being fast-forwarded. + if let Some(local) = local { + if let (Ok(local), Ok(staging)) = ( + local.reference_oid(&remote_id, &git::refs::storage::SIGREFS_BRANCH), + self.repo.reference_oid(&remote_id, &git::refs::storage::SIGREFS_BRANCH), + ) { + if local != staging { + match self + .repo + .backend + .graph_descendant_of(staging.into(), local.into()) + { + Ok(true) => { + log::debug!(target: "worker", "Signed refs for {remote_id} fast-foward: {local} -> {staging}"); + } + Ok(false) => { + return ( + remote_id, + VerifiedRemote::Failed { + reason: "signed refs have diverged".to_owned() + } + ); + } + Err(e) => { + return ( + remote_id, + VerifiedRemote::Failed { reason: e.to_string() }, + ); + } + } + } else { + return (remote_id, VerifiedRemote::UpToDate); + } + } + } + let verification = match self.repo.identity_doc_of(&remote_id) { Ok(doc) => match self.repo.validate_remote(&remote) { Ok(unsigned) => VerifiedRemote::Success { @@ -446,7 +528,9 @@ impl<'a> StagingPhaseFinal<'a> { }; (remote_id, verification) }) - .collect()) + .collect(); + + Ok(result) } } diff --git a/radicle-node/src/worker/fetch/refspecs.rs b/radicle-node/src/worker/fetch/refspecs.rs index 7a9229af..6e68fd56 100644 --- a/radicle-node/src/worker/fetch/refspecs.rs +++ b/radicle-node/src/worker/fetch/refspecs.rs @@ -42,18 +42,15 @@ impl AsRefspecs for SpecialRefs { Namespaces::All => { let id = NAMESPACES_GLOB.join(&*IDENTITY_BRANCH); let sigrefs = NAMESPACES_GLOB.join(&*SIGREFS_BRANCH); - vec![ - Refspec { - src: id.clone(), - dst: id, - force: false, - }, - Refspec { - src: sigrefs.clone(), - dst: sigrefs, - force: false, - }, - ] + + [id, sigrefs] + .into_iter() + .map(|spec| Refspec { + src: spec.clone(), + dst: spec, + force: true, + }) + .collect() } Namespaces::Trusted(pks) => pks.iter().flat_map(rad_refs).collect(), } @@ -70,13 +67,13 @@ fn rad_refs(pk: &PublicKey) -> Vec vec![Refspec { src: (*storage::git::NAMESPACES_GLOB).clone(), dst: (*storage::git::NAMESPACES_GLOB).clone(), - force: false, + force: true, }], Namespaces::Trusted(pks) => pks .iter() @@ -113,7 +110,7 @@ impl AsRefspecs for Namespaces { Refspec { src: ns.clone(), dst: ns, - force: false, + force: true, } }) .collect(), @@ -163,19 +160,11 @@ impl AsRefspecs for Remote { impl<'a> AsRefspecs for BTreeSet> { fn as_refspecs(&self) -> Vec> { - let reserved = [(*IDENTITY_BRANCH).clone(), (*SIGREFS_BRANCH).clone()] - .into_iter() - .collect::>(); self.iter() - .map(|r| { - // Only force ordinary refs. - let suffix = r.strip_namespace(); - let force = !reserved.contains(&suffix); - Refspec { - src: r.clone().to_ref_string().into(), - dst: r.clone().to_ref_string().into(), - force, - } + .map(|r| Refspec { + src: r.clone().to_ref_string().into(), + dst: r.clone().to_ref_string().into(), + force: true, }) .collect() }