radicle/refs: Better `SignedRefs` Encapsulation

Ensure that the public interface of signed references does not leak its
implementation details.

This allows the evolution of the interface in a safer manner, and does
not leak implementation details to the rest of the crate or any
dependents.
This commit is contained in:
Fintan Halpenny 2026-03-03 14:21:53 +00:00 committed by Lorenz Leutgeb
parent 39a58ded05
commit ba9c09facb
6 changed files with 27 additions and 16 deletions

View File

@ -517,7 +517,7 @@ impl ProtocolStage for DataRefs {
for (remote, loaded) in &self.remotes { for (remote, loaded) in &self.remotes {
wants_haves.add( wants_haves.add(
refdb, refdb,
loaded.refs.iter().filter_map(|(refname, tip)| { loaded.refs().iter().filter_map(|(refname, tip)| {
let refname = Qualified::from_refstr(refname) let refname = Qualified::from_refstr(refname)
.map(|refname| refname.with_namespace(Component::from(remote)))?; .map(|refname| refname.with_namespace(Component::from(remote)))?;
Some((refname, *tip)) Some((refname, *tip))
@ -537,7 +537,7 @@ impl ProtocolStage for DataRefs {
let mut updates = Updates::default(); let mut updates = Updates::default();
for (remote, refs) in &self.remotes { for (remote, refs) in &self.remotes {
let mut signed = HashSet::with_capacity(refs.refs.len()); let mut signed = HashSet::with_capacity(refs.refs().len());
for (name, tip) in refs.iter() { for (name, tip) in refs.iter() {
let tracking: Namespaced<'_> = Qualified::from_refstr(name) let tracking: Namespaced<'_> = Qualified::from_refstr(name)
.and_then(|q| refs::ReceivedRefname::remote(*remote, q).to_namespaced()) .and_then(|q| refs::ReceivedRefname::remote(*remote, q).to_namespaced())

View File

@ -733,7 +733,7 @@ where
let mut has_sigrefs = false; let mut has_sigrefs = false;
// Check all repository references, making sure they are present in the signed refs map. // Check all repository references, making sure they are present in the signed refs map.
for (refname, oid) in self.state.refs.references_of(&remote.id) { for (refname, oid) in self.state.refs.references_of(&remote.id()) {
// Skip validation of the signed refs branch, as it is not part of `Remote`. // Skip validation of the signed refs branch, as it is not part of `Remote`.
if refname == storage::refs::SIGREFS_BRANCH.to_ref_string() { if refname == storage::refs::SIGREFS_BRANCH.to_ref_string() {
has_sigrefs = true; has_sigrefs = true;
@ -753,7 +753,7 @@ where
} }
if !has_sigrefs { if !has_sigrefs {
validations.push(Validation::MissingRadSigRefs(remote.id)); validations.push(Validation::MissingRadSigRefs(remote.id()));
} }
// The refs that are left in the map, are ones that were signed, but are not // The refs that are left in the map, are ones that were signed, but are not
@ -761,7 +761,7 @@ where
for (name, _) in signed.into_iter() { for (name, _) in signed.into_iter() {
validations.push(Validation::MissingRef { validations.push(Validation::MissingRef {
refname: name, refname: name,
remote: remote.id, remote: remote.id(),
}); });
} }

View File

@ -384,7 +384,7 @@ impl Remote<Verified> {
} }
pub fn to_refspecs(&self) -> Vec<Refspec<PatternString, PatternString>> { pub fn to_refspecs(&self) -> Vec<Refspec<PatternString, PatternString>> {
let ns = self.id.to_namespace(); let ns = self.id().to_namespace();
// Nb. the references in Refs are expected to be Qualified // Nb. the references in Refs are expected to be Qualified
self.refs self.refs
.keys() .keys()

View File

@ -639,7 +639,7 @@ impl ValidateRepository for Repository {
let mut has_sigrefs = false; let mut has_sigrefs = false;
// Check all repository references, making sure they are present in the signed refs map. // Check all repository references, making sure they are present in the signed refs map.
for (refname, oid) in self.references_of(&remote.id)? { for (refname, oid) in self.references_of(&remote.id())? {
// Skip validation of the signed refs branch, as it is not part of `Remote`. // Skip validation of the signed refs branch, as it is not part of `Remote`.
if refname == refs::SIGREFS_BRANCH.to_ref_string() { if refname == refs::SIGREFS_BRANCH.to_ref_string() {
has_sigrefs = true; has_sigrefs = true;
@ -659,7 +659,7 @@ impl ValidateRepository for Repository {
} }
if !has_sigrefs { if !has_sigrefs {
failures.push(Validation::MissingRadSigRefs(remote.id)); failures.push(Validation::MissingRadSigRefs(remote.id()));
} }
// The refs that are left in the map, are ones that were signed, but are not // The refs that are left in the map, are ones that were signed, but are not
@ -667,7 +667,7 @@ impl ValidateRepository for Repository {
if let Some((name, _)) = signed.into_iter().next() { if let Some((name, _)) = signed.into_iter().next() {
failures.push(Validation::MissingRef { failures.push(Validation::MissingRef {
refname: name, refname: name,
remote: remote.id, remote: remote.id(),
}); });
} }

View File

@ -12,6 +12,7 @@ use std::str::FromStr;
use crypto::signature::Signer; use crypto::signature::Signer;
use crypto::{PublicKey, Signature, Unverified, Verified}; use crypto::{PublicKey, Signature, Unverified, Verified};
use radicle_core::NodeId;
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use thiserror::Error; use thiserror::Error;
@ -112,7 +113,7 @@ impl Refs {
} }
/// Create refs from a canonical representation. /// Create refs from a canonical representation.
pub fn from_canonical(bytes: &[u8]) -> Result<Self, canonical::Error> { fn from_canonical(bytes: &[u8]) -> Result<Self, canonical::Error> {
let reader = BufReader::new(bytes); let reader = BufReader::new(bytes);
let mut refs = BTreeMap::new(); let mut refs = BTreeMap::new();
@ -133,7 +134,7 @@ impl Refs {
Ok(Self(refs)) Ok(Self(refs))
} }
pub fn canonical(&self) -> Vec<u8> { fn canonical(&self) -> Vec<u8> {
let mut buf = String::new(); let mut buf = String::new();
for (name, oid) in self.iter() { for (name, oid) in self.iter() {
@ -196,12 +197,12 @@ impl DerefMut for Refs {
#[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
pub struct SignedRefs<V> { pub struct SignedRefs<V> {
/// The signed refs. /// The signed refs.
pub refs: Refs, refs: Refs,
/// The signature of the signer over the refs. /// The signature of the signer over the refs.
#[serde(skip)] #[serde(skip)]
pub signature: Signature, signature: Signature,
/// This is the remote under which these refs exist, and the public key of the signer. /// This is the remote under which these refs exist, and the public key of the signer.
pub id: PublicKey, id: PublicKey,
#[serde(skip)] #[serde(skip)]
_verified: PhantomData<V>, _verified: PhantomData<V>,
@ -263,6 +264,16 @@ impl SignedRefs<Unverified> {
} }
impl SignedRefs<Verified> { impl SignedRefs<Verified> {
/// Returns the [`NodeId`] of the [`SignedRefs`].
pub fn id(&self) -> NodeId {
self.id
}
/// Returns the [`Refs`] of the [`SignedRefs`].
pub fn refs(&self) -> &Refs {
&self.refs
}
pub fn load<S>(remote: RemoteId, repo: &S) -> Result<Self, Error> pub fn load<S>(remote: RemoteId, repo: &S) -> Result<Self, Error>
where where
S: ReadRepository, S: ReadRepository,

View File

@ -182,7 +182,7 @@ impl RemoteRepository for MockRepository {
.remotes .remotes
.values() .values()
.map(|s| refs::RefsAt { .map(|s| refs::RefsAt {
remote: s.id, remote: s.id(),
at: s.at, at: s.at,
}) })
.collect()) .collect())
@ -232,7 +232,7 @@ impl ReadRepository for MockRepository {
Ok(self Ok(self
.remotes .remotes
.values() .values()
.any(|sigrefs| sigrefs.at == oid || sigrefs.refs.values().any(|oid_| *oid_ == oid))) .any(|sigrefs| sigrefs.at == oid || sigrefs.values().any(|oid_| *oid_ == oid)))
} }
fn is_ancestor_of(&self, _ancestor: Oid, _head: Oid) -> Result<bool, crate::git::raw::Error> { fn is_ancestor_of(&self, _ancestor: Oid, _head: Oid) -> Result<bool, crate::git::raw::Error> {