From a69397386a9b14021f9859229eac2b83aade0139 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Thu, 19 Jun 2025 14:08:49 +0200 Subject: [PATCH] cli: extract document update logic The business logic of modifying the identity document should not live in the CLI code for updating said document. Instead, we extract out that logic into separate update functions and these are using from the CLI instead. --- crates/radicle-cli/examples/rad-id-private.md | 3 +- crates/radicle-cli/src/commands/id.rs | 192 ++++---------- crates/radicle/src/identity/doc.rs | 2 + crates/radicle/src/identity/doc/update.rs | 235 ++++++++++++++++++ .../radicle/src/identity/doc/update/error.rs | 37 +++ 5 files changed, 318 insertions(+), 151 deletions(-) create mode 100644 crates/radicle/src/identity/doc/update.rs create mode 100644 crates/radicle/src/identity/doc/update/error.rs diff --git a/crates/radicle-cli/examples/rad-id-private.md b/crates/radicle-cli/examples/rad-id-private.md index 63998838..711ee25e 100644 --- a/crates/radicle-cli/examples/rad-id-private.md +++ b/crates/radicle-cli/examples/rad-id-private.md @@ -79,7 +79,8 @@ then the command will fail since there is no longer an allow list to work with: ``` (fails) $ rad id update --visibility public --allow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk -✗ Error: `--allow` and `--disallow` cannot be used with `--visibility public` +✗ Error: `--allow` and `--disallow` should only be used for private repositories +✗ Hint: use `--visibility private` to make the repository private, or perhaps you meant to use `--delegate`/`--rescind` ``` Let's change the repository to `public`: diff --git a/crates/radicle-cli/src/commands/id.rs b/crates/radicle-cli/src/commands/id.rs index 3a0f4dc5..2690454e 100644 --- a/crates/radicle-cli/src/commands/id.rs +++ b/crates/radicle-cli/src/commands/id.rs @@ -1,16 +1,16 @@ use std::collections::BTreeSet; -use std::str::FromStr; use std::{ffi::OsString, io}; use anyhow::{anyhow, Context}; use radicle::cob::identity::{self, IdentityMut, Revision, RevisionId}; -use radicle::identity::{doc, Doc, Identity, PayloadError, RawDoc, Visibility}; +use radicle::identity::doc::update; +use radicle::identity::doc::update::EditVisibility; +use radicle::identity::{doc, Doc, Identity, RawDoc}; use radicle::node::device::Device; use radicle::node::NodeId; use radicle::prelude::{Did, RepoId}; -use radicle::storage::refs; -use radicle::storage::{ReadRepository, ReadStorage as _, WriteRepository}; +use radicle::storage::{ReadStorage as _, WriteRepository}; use radicle::{cob, crypto, Profile}; use radicle_surf::diff::Diff; use radicle_term::Element; @@ -88,29 +88,6 @@ pub enum Operation { ListRevisions, } -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] -pub enum EditVisibility { - #[default] - Public, - Private, -} - -#[derive(thiserror::Error, Debug)] -#[error("'{0}' is not a valid visibility type")] -pub struct EditVisibilityParseError(String); - -impl FromStr for EditVisibility { - type Err = EditVisibilityParseError; - - fn from_str(s: &str) -> Result { - match s { - "public" => Ok(EditVisibility::Public), - "private" => Ok(EditVisibility::Private), - _ => Err(EditVisibilityParseError(s.to_owned())), - } - } -} - #[derive(Default, PartialEq, Eq)] pub enum OperationName { Accept, @@ -398,76 +375,38 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { let mut proposal = current.doc.clone().edit(); proposal.threshold = threshold.unwrap_or(proposal.threshold); - if !allow.is_disjoint(&disallow) { - let overlap = allow - .intersection(&disallow) - .map(Did::to_string) - .collect::>(); - anyhow::bail!("`--allow` and `--disallow` must not overlap: {overlap:?}") - } - - match (&mut proposal.visibility, visibility) { - (Visibility::Public, None | Some(EditVisibility::Public)) if !allow.is_empty() || !disallow.is_empty() => { - return Err(Error::WithHint { + let proposal = match visibility { + Some(edit) => update::visibility(proposal, edit), + None => proposal, + }; + let proposal = match update::privacy_allow_list(proposal, allow, disallow) { + Ok(proposal) => proposal, + Err(e) => match e { + update::error::PrivacyAllowList::Overlapping(overlap) => anyhow::bail!("`--allow` and `--disallow` must not overlap: {overlap:?}"), + update::error::PrivacyAllowList::PublicVisibility => return Err(Error::WithHint { err: anyhow!("`--allow` and `--disallow` should only be used for private repositories"), hint: "use `--visibility private` to make the repository private, or perhaps you meant to use `--delegate`/`--rescind`", }.into()) } - (Visibility::Public, None | Some(EditVisibility::Public)) => { /* no-op */ }, - (Visibility::Private { allow: existing }, None | Some(EditVisibility::Private)) => { - for did in allow { - existing.insert(did); - } - for did in disallow { - existing.remove(&did); + }; + let threshold = proposal.threshold; + let proposal = match update::delegates(proposal, delegates, rescind, &repo)? { + Ok(proposal) => proposal, + Err(errs) => { + term::error(format!("failed to verify delegates for {rid}")); + term::error(format!( + "the threshold of {} delegates cannot be met..", + threshold + )); + for e in errs { + print_delegate_verification_error(&e); } + anyhow::bail!("fatal: refusing to update identity document"); } - (Visibility::Public, Some(EditVisibility::Private)) => { - // We ignore disallow since only allowing matters and the sets are disjoint. - proposal.visibility = Visibility::Private { allow }; - } - (Visibility::Private { .. }, Some(EditVisibility::Public)) if !allow.is_empty() || !disallow.is_empty() => { - anyhow::bail!("`--allow` and `--disallow` cannot be used with `--visibility public`") - } - (Visibility::Private { .. }, Some(EditVisibility::Public)) => { - proposal.visibility = Visibility::Public; - } - } - proposal.delegates = proposal - .delegates - .into_iter() - .chain(delegates) - .filter(|d| !rescind.contains(d)) - .collect::>(); - if let Some(errs) = verify_delegates(&proposal, &repo)? { - term::error(format!("failed to verify delegates for {rid}")); - term::error(format!( - "the threshold of {} delegates cannot be met..", - proposal.threshold - )); - for e in errs { - e.print(); - } - anyhow::bail!("fatal: refusing to update identity document"); - } + }; - for (id, key, val) in payload { - if let Some(ref mut payload) = proposal.payload.get_mut(&id) { - if let Some(obj) = payload.as_object_mut() { - if val.is_null() { - obj.remove(&key); - } else { - obj.insert(key, val); - } - } else { - anyhow::bail!("payload `{id}` is not a map"); - } - } else { - anyhow::bail!("payload `{id}` not found in identity document"); - } - } - proposal + update::payload(proposal, payload)? }; // If `--edit` is specified, the document can also be edited via a text edit. @@ -489,11 +428,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { proposal }; - // Verify that the project payload can still be parsed into the `Project` type. - if let Err(PayloadError::Json(e)) = proposal.project() { - anyhow::bail!("failed to verify `xyz.radicle.project`, {e}"); - } - let proposal = proposal.verified()?; + let proposal = update::verify(proposal)?; if proposal == current.doc { if !options.quiet { term::print(term::format::italic( @@ -793,62 +728,19 @@ fn print_diff( Ok(()) } -#[derive(Clone)] -enum VerificationError { - MissingDefaultBranch { - branch: radicle::git::RefString, - did: Did, - }, - MissingDelegate { - did: Did, - }, -} - -impl VerificationError { - fn print(&self) { - match self { - VerificationError::MissingDefaultBranch { branch, did } => term::error(format!( - "missing {} for {} in local storage", - term::format::secondary(branch), - term::format::did(did) - )), - VerificationError::MissingDelegate { did } => { - term::error(format!("the delegate {did} is missing")); - term::hint(format!( - "run `rad follow {did}` to follow this missing peer" - )); - } +fn print_delegate_verification_error(err: &update::error::DelegateVerification) { + use update::error::DelegateVerification::*; + match err { + MissingDefaultBranch { branch, did } => term::error(format!( + "missing {} for {} in local storage", + term::format::secondary(branch), + term::format::did(did) + )), + MissingDelegate { did } => { + term::error(format!("the delegate {did} is missing")); + term::hint(format!( + "run `rad follow {did}` to follow this missing peer" + )); } } } - -fn verify_delegates( - proposal: &RawDoc, - repo: &S, -) -> anyhow::Result>> -where - S: ReadRepository, -{ - let dids = &proposal.delegates; - let threshold = proposal.threshold; - let (canonical, _) = repo.canonical_head()?; - let mut missing = Vec::with_capacity(dids.len()); - - for did in dids { - match refs::SignedRefsAt::load((*did).into(), repo)? { - None => { - missing.push(VerificationError::MissingDelegate { did: *did }); - } - Some(refs::SignedRefsAt { sigrefs, .. }) => { - if sigrefs.get(&canonical).is_none() { - missing.push(VerificationError::MissingDefaultBranch { - branch: canonical.to_ref_string(), - did: *did, - }); - } - } - } - } - - Ok((dids.len() - missing.len() < threshold).then_some(missing)) -} diff --git a/crates/radicle/src/identity/doc.rs b/crates/radicle/src/identity/doc.rs index b3f4c1b3..169876ae 100644 --- a/crates/radicle/src/identity/doc.rs +++ b/crates/radicle/src/identity/doc.rs @@ -1,3 +1,5 @@ +pub mod update; + mod id; use std::collections::{BTreeMap, BTreeSet}; diff --git a/crates/radicle/src/identity/doc/update.rs b/crates/radicle/src/identity/doc/update.rs new file mode 100644 index 00000000..14662319 --- /dev/null +++ b/crates/radicle/src/identity/doc/update.rs @@ -0,0 +1,235 @@ +pub mod error; + +use std::{collections::BTreeSet, str::FromStr}; + +use serde_json as json; + +use crate::{ + identity::crefs::GetCanonicalRefs as _, + prelude::Did, + storage::{refs, ReadRepository, RepositoryError}, +}; + +use super::{Doc, PayloadId, RawDoc, Visibility}; + +/// [`EditVisibility`] allows the visibility of a [`RawDoc`] to be edited using +/// the [`visibility`] function. +/// +/// Note that this differs from [`Visibility`] since the +/// [`EditVisibility::Private`] variant does not hold the allowed set of +/// [`Did`]s. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum EditVisibility { + #[default] + Public, + Private, +} + +impl FromStr for EditVisibility { + type Err = error::ParseEditVisibility; + + fn from_str(s: &str) -> Result { + match s { + "public" => Ok(EditVisibility::Public), + "private" => Ok(EditVisibility::Private), + _ => Err(error::ParseEditVisibility(s.to_owned())), + } + } +} + +/// Change the visibility of the [`RawDoc`], using the provided +/// [`EditVisibility`]. +pub fn visibility(mut raw: RawDoc, edit: EditVisibility) -> RawDoc { + match (&mut raw.visibility, edit) { + (Visibility::Public, EditVisibility::Public) => raw, + (Visibility::Private { .. }, EditVisibility::Private) => raw, + (Visibility::Public, EditVisibility::Private) => { + raw.visibility = Visibility::private([]); + raw + } + (Visibility::Private { .. }, EditVisibility::Public) => { + raw.visibility = Visibility::Public; + raw + } + } +} + +/// Change the `allow` set of a document if the visibility is set to +/// [`Visibility::Private`]. +/// +/// All `Did`s in the `allow` set are added to the set, while all `Did`s in the +/// `disallow` set are removed from the set. +/// +/// # Errors +/// +/// This will fail when `allow` and `disallow` are not disjoint, i.e. they +/// contain at least share one `Did`. +/// +/// This will fail when the [`Visibility`] of the document is +/// [`Visibility::Public`]. +pub fn privacy_allow_list( + mut raw: RawDoc, + allow: BTreeSet, + disallow: BTreeSet, +) -> Result { + if allow.is_empty() && disallow.is_empty() { + return Ok(raw); + } + + if !allow.is_disjoint(&disallow) { + let overlap = allow + .intersection(&disallow) + .map(Did::to_string) + .collect::>(); + return Err(error::PrivacyAllowList::Overlapping(overlap)); + } + + match &mut raw.visibility { + Visibility::Public => Err(error::PrivacyAllowList::PublicVisibility), + Visibility::Private { allow: existing } => { + for did in allow { + existing.insert(did); + } + for did in disallow { + existing.remove(&did); + } + Ok(raw) + } + } +} + +/// Change the delegates of the document and perform some verification based on +/// the new set of delegates. +/// +/// The set of `additions` are added to the delegates, while the set to +/// `removals` are removed from the delegates. Note that `removals` will take +/// precedence over the additions, i.e. if an addition and removal overlap, then +/// the [`Did`] will not be in the final set. +/// +/// The result is either the updated [`RawDoc`] or a set of +/// [`error::DelegateVerification`] errors – which may be reported by the caller +/// to provide better error messaging. +/// +/// # Errors +/// +/// This will fail if an operation using the repository fails. +pub fn delegates( + mut raw: RawDoc, + additions: Vec, + removals: Vec, + repo: &S, +) -> Result>, RepositoryError> +where + S: ReadRepository, +{ + if additions.is_empty() && removals.is_empty() { + return Ok(Ok(raw)); + } + + raw.delegates = raw + .delegates + .into_iter() + .chain(additions) + .filter(|d| !removals.contains(d)) + .collect::>(); + match verify_delegates(&raw, repo)? { + Some(errors) => Ok(Err(errors)), + None => Ok(Ok(raw)), + } +} + +// TODO(finto): I think this API would likely be much nicer if we use [JSON Patch][patch] and [JSON Merge Patch][merge] +// +// [patch]: https://datatracker.ietf.org/doc/html/rfc6902 +// [merge]: https://datatracker.ietf.org/doc/html/rfc7396 +/// Change the payload of the document, using the set of triples: +/// +/// - [`PayloadId`]: the identifier for the document [`Payload`] +/// - [`String`]: the key within the [`Payload`] that is being updated +/// - [`json::Value`]: the value to update the [`Payload`] +/// +/// # Errors +/// +/// This fails if one of the [`PayloadId`]s does not point to a JSON object as +/// its value. +/// +/// [`Payload`]: super::Payload +pub fn payload( + mut raw: RawDoc, + payload: Vec<(PayloadId, String, json::Value)>, +) -> Result { + for (id, key, val) in payload { + if let Some(ref mut payload) = raw.payload.get_mut(&id) { + if let Some(obj) = payload.as_object_mut() { + if val.is_null() { + obj.remove(&key); + } else { + obj.insert(key, val); + } + } else { + return Err(error::PayloadError::ExpectedObject { id }); + } + } else { + raw.payload + .insert(id, serde_json::json!({ key: val }).into()); + } + } + Ok(raw) +} + +/// Verify the document. +/// +/// This ensures performs the verification of the [`RawDoc`] into the [`Doc`], +/// while also checking the [`Project`] and [`CanonicalRefs`] will also +/// deserialize correctly. +/// +/// [`Project`]: crate::identity::Project +/// [`CanonicalRefs`]: crate::identity::CanonicalRefs +pub fn verify(raw: RawDoc) -> Result { + let proposal = raw.verified()?; + // Verify that the payloads can still be parsed into the correct types. + if let Err(super::PayloadError::Json(e)) = proposal.project() { + return Err(error::DocVerification::PayloadJson { + id: PayloadId::project(), + err: e, + }); + } + if let Err(super::PayloadError::Json(e)) = proposal.canonical_refs() { + return Err(error::DocVerification::PayloadJson { + id: PayloadId::canonical_refs(), + err: e, + }); + } + Ok(proposal) +} + +fn verify_delegates( + proposal: &RawDoc, + repo: &S, +) -> Result>, RepositoryError> +where + S: ReadRepository, +{ + let dids = &proposal.delegates; + let threshold = proposal.threshold; + let (canonical, _) = repo.canonical_head()?; + let mut missing = Vec::with_capacity(dids.len()); + + for did in dids { + match refs::SignedRefsAt::load((*did).into(), repo)? { + None => { + missing.push(error::DelegateVerification::MissingDelegate { did: *did }); + } + Some(refs::SignedRefsAt { sigrefs, .. }) => { + if sigrefs.get(&canonical).is_none() { + missing.push(error::DelegateVerification::MissingDefaultBranch { + branch: canonical.to_ref_string(), + did: *did, + }); + } + } + } + } + + Ok((dids.len() - missing.len() < threshold).then_some(missing)) +} diff --git a/crates/radicle/src/identity/doc/update/error.rs b/crates/radicle/src/identity/doc/update/error.rs new file mode 100644 index 00000000..02ebd910 --- /dev/null +++ b/crates/radicle/src/identity/doc/update/error.rs @@ -0,0 +1,37 @@ +use serde_json as json; +use thiserror::Error; + +use crate::git::RefString; +use crate::identity::{doc::PayloadId, Did, DocError}; + +#[derive(Debug, Error)] +#[error("'{0}' is not a valid visibility type")] +pub struct ParseEditVisibility(pub(super) String); + +#[derive(Debug, Error)] +pub enum PrivacyAllowList { + #[error("overlapping allow and disallow of DIDs {0:?}")] + Overlapping(Vec), + #[error("the visibility of the document is public")] + PublicVisibility, +} + +#[derive(Debug, Error)] +pub enum PayloadError { + #[error("payload found under `{id}` is expected to be a map")] + ExpectedObject { id: PayloadId }, +} + +#[derive(Debug, Error)] +pub enum DocVerification { + #[error("failed to verify `{id}`, {err}")] + PayloadJson { id: PayloadId, err: json::Error }, + #[error(transparent)] + Doc(#[from] DocError), +} + +#[derive(Clone, Debug)] +pub enum DelegateVerification { + MissingDefaultBranch { branch: RefString, did: Did }, + MissingDelegate { did: Did }, +}