From c903a958e7600f4758997d82a0e2d5017a82df57 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Thu, 7 Sep 2023 11:44:48 +0200 Subject: [PATCH] cob: Implement action authorization Add `authorization` functions to `Patch` and `Issue` COBs. Co-authored-by: Arastoo Bozorgi --- radicle-cli/examples/workflow/3-issues.md | 48 +-- radicle-cob/src/history.rs | 13 +- radicle-cob/src/tests.rs | 25 +- radicle/src/cob/common.rs | 25 ++ radicle/src/cob/identity.rs | 9 +- radicle/src/cob/issue.rs | 107 +++++- radicle/src/cob/patch.rs | 439 +++++++++++++++++----- radicle/src/cob/store.rs | 73 ++-- radicle/src/cob/test.rs | 47 ++- radicle/src/cob/thread.rs | 117 +++--- 10 files changed, 641 insertions(+), 262 deletions(-) diff --git a/radicle-cli/examples/workflow/3-issues.md b/radicle-cli/examples/workflow/3-issues.md index 51c22655..3b57cf66 100644 --- a/radicle-cli/examples/workflow/3-issues.md +++ b/radicle-cli/examples/workflow/3-issues.md @@ -26,51 +26,11 @@ $ rad issue list ╰────────────────────────────────────────────────────────────────────────────────────────────────╯ ``` -Great! Now we've documented the issue for ourselves and others. - -Just like with other project management systems, the issue can be assigned to -others to work on. This is to ensure work is not duplicated. - -Let's assign this issue to ourself. - -``` -$ rad assign 2f6eb49efac492327f71437b6bfc01b49afa0981 --to did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk -``` - -It will now show in the list of issues assigned to us. - -``` -$ rad issue list --assigned -╭────────────────────────────────────────────────────────────────────────────────────────────────╮ -│ ● ID Title Author Labels Assignees Opened │ -├────────────────────────────────────────────────────────────────────────────────────────────────┤ -│ ● 2f6eb49 flux capacitor underpowered bob (you) bob [ .. ] │ -╰────────────────────────────────────────────────────────────────────────────────────────────────╯ -``` - -In addition, you can see that when you run `rad issue show` you are listed under the `Assignees`. - -``` -$ rad issue show 2f6eb49 -╭─────────────────────────────────────────────────────────╮ -│ Title flux capacitor underpowered │ -│ Issue 2f6eb49efac492327f71437b6bfc01b49afa0981 │ -│ Author bob (you) │ -│ Assignees z6Mkt67…v4N1tRk │ -│ Status open │ -│ │ -│ Flux capacitor power requirements exceed current supply │ -╰─────────────────────────────────────────────────────────╯ -``` - -Note: this can always be undone with the `unassign` subcommand. - -Great, now we have communicated to the world about our car's defect. - -But wait! We've found an important detail about the car's power requirements. -It will help whoever works on a fix. +Great! Now we've documented the issue for ourselves and others. But wait, we've +found an important detail about the car's power requirements. It will help +whoever works on a fix. ``` $ rad comment 2f6eb49efac492327f71437b6bfc01b49afa0981 --message 'The flux capacitor needs 1.21 Gigawatts' -24ab347afda760e77d565f9cb013c6db560f44fd +d2b50873009b93680698aef4f57f43f7e850b651 ``` diff --git a/radicle-cob/src/history.rs b/radicle-cob/src/history.rs index 04f051c8..eba86026 100644 --- a/radicle-cob/src/history.rs +++ b/radicle-cob/src/history.rs @@ -60,12 +60,11 @@ impl History { /// accumulator value of type `A`. However, unlike `fold` the function `f` /// may prune branches from the dependency graph by returning /// `ControlFlow::Break`. - pub fn traverse(&self, init: A, mut f: F) -> A + pub fn traverse(&self, init: A, roots: &[EntryId], mut f: F) -> A where F: for<'r> FnMut(A, &'r EntryId, &'r Entry) -> ControlFlow, { - self.graph - .fold(&[self.root], init, |acc, k, v| f(acc, k, v)) + self.graph.fold(roots, init, |acc, k, v| f(acc, k, v)) } /// Return a topologically-sorted list of history entries. @@ -114,4 +113,12 @@ impl History { .get(&self.root) .expect("History::root: the root entry must be present in the graph") } + + /// Get the children of the given entry. + pub fn children_of(&self, id: &EntryId) -> Vec { + self.graph + .get(id) + .map(|n| n.dependents.iter().cloned().collect()) + .unwrap_or_default() + } } diff --git a/radicle-cob/src/tests.rs b/radicle-cob/src/tests.rs index def62425..1a8d1908 100644 --- a/radicle-cob/src/tests.rs +++ b/radicle-cob/src/tests.rs @@ -211,21 +211,26 @@ fn traverse_cobs() { ) .unwrap(); + let root = object.history.root().id; // traverse over the history and filter by changes that were only authorized by terry - let contents = object.history().traverse(Vec::new(), |mut acc, _, entry| { - if entry.author() == terry_signer.public_key() { - acc.push(entry.contents().head.clone()); - } - ControlFlow::Continue(acc) - }); + let contents = object + .history() + .traverse(Vec::new(), &[root], |mut acc, _, entry| { + if entry.author() == terry_signer.public_key() { + acc.push(entry.contents().head.clone()); + } + ControlFlow::Continue(acc) + }); assert_eq!(contents, vec![b"issue 1".to_vec()]); // traverse over the history and filter by changes that were only authorized by neil - let contents = object.history().traverse(Vec::new(), |mut acc, _, entry| { - acc.push(entry.contents().head.clone()); - ControlFlow::Continue(acc) - }); + let contents = object + .history() + .traverse(Vec::new(), &[root], |mut acc, _, entry| { + acc.push(entry.contents().head.clone()); + ControlFlow::Continue(acc) + }); assert_eq!(contents, vec![b"issue 1".to_vec(), b"issue 2".to_vec()]); } diff --git a/radicle/src/cob/common.rs b/radicle/src/cob/common.rs index e2b25d23..8de83e17 100644 --- a/radicle/src/cob/common.rs +++ b/radicle/src/cob/common.rs @@ -24,6 +24,10 @@ impl Author { pub fn id(&self) -> &Did { &self.id } + + pub fn public_key(&self) -> &PublicKey { + self.id.as_key() + } } impl From for Author { @@ -278,6 +282,27 @@ impl std::str::FromStr for Uri { } } +/// The result of an authorization check on an COB action. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Authorization { + /// Action is allowed. + Allow, + /// Action is denied. + Deny, + /// Authorization cannot be determined due to missing object, eg. due to redaction. + Unknown, +} + +impl From for Authorization { + fn from(value: bool) -> Self { + if value { + Self::Allow + } else { + Self::Deny + } + } +} + #[cfg(test)] mod test { use super::*; diff --git a/radicle/src/cob/identity.rs b/radicle/src/cob/identity.rs index 06202685..ae73bc44 100644 --- a/radicle/src/cob/identity.rs +++ b/radicle/src/cob/identity.rs @@ -323,11 +323,10 @@ impl store::FromHistory for Proposal { &TYPENAME } - fn validate(&self) -> Result<(), Self::Error> { - if self.revisions.is_empty() { - return Err(ApplyError::Validate("no revisions found")); - } - Ok(()) + fn init(op: Op, repo: &R) -> Result { + let mut identity = Self::default(); + identity.apply(op, repo)?; + Ok(identity) } fn apply(&mut self, op: Op, _repo: &R) -> Result<(), Self::Error> { diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index 80cb884b..b9ef0f36 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -7,15 +7,16 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::cob; -use crate::cob::common::{Author, Label, Reaction, Timestamp, Uri}; +use crate::cob::common::{Author, Authorization, Label, Reaction, Timestamp, Uri}; use crate::cob::store::Transaction; use crate::cob::store::{FromHistory as _, HistoryAction}; use crate::cob::thread; -use crate::cob::thread::{CommentId, Thread}; +use crate::cob::thread::{Comment, CommentId, Thread}; use crate::cob::{store, ActorId, Embed, EntryId, ObjectId, TypeName}; use crate::crypto::Signer; use crate::git; -use crate::prelude::{Did, ReadRepository}; +use crate::identity::doc::{Doc, DocError}; +use crate::prelude::{Did, ReadRepository, Verified}; use crate::storage::WriteRepository; /// Issue operation. @@ -31,16 +32,23 @@ pub type IssueId = ObjectId; /// Error updating or creating issues. #[derive(Error, Debug)] pub enum Error { - #[error("apply failed")] - Apply, - #[error("validation failed: {0}")] - Validate(&'static str), + /// Error loading the identity document. + #[error("identity doc failed to load: {0}")] + Doc(#[from] DocError), #[error("description missing")] DescriptionMissing, #[error("thread apply failed: {0}")] Thread(#[from] thread::Error), #[error("store: {0}")] Store(#[from] store::Error), + #[error("history: {0}")] + History(Box), + /// Action not authorized. + #[error("{0} not authorized to apply {1:?}")] + NotAuthorized(ActorId, Action), + /// General error initializing an issue. + #[error("initialization failed: {0}")] + Init(&'static str), } /// Reason why an issue was closed. @@ -91,7 +99,7 @@ impl State { } /// Issue state. Accumulates [`Action`]. -#[derive(Debug, Clone, PartialEq, Eq, Default)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Issue { /// Actors assigned to this issue. pub(super) assignees: BTreeSet, @@ -113,25 +121,52 @@ impl store::FromHistory for Issue { &TYPENAME } - fn validate(&self) -> Result<(), Self::Error> { - if self.title.is_empty() { - return Err(Error::Validate("title is empty")); + fn init(op: Op, repo: &R) -> Result { + let mut actions = op.actions.into_iter(); + let Some(Action::Comment { body, reply_to: None, embeds }) = actions.next() else { + return Err(Error::Init("the first action must be of type `comment`")); + }; + let comment = Comment::new(op.author, body, None, None, embeds, op.timestamp); + let thread = Thread::new(op.id, comment); + let mut issue = Issue::new(thread); + + for action in actions { + issue.action(action, op.id, op.author, op.timestamp, op.identity, repo)?; } - if self.thread.validate().is_err() { - return Err(Error::Validate("invalid thread")); - } - Ok(()) + Ok(issue) } fn apply(&mut self, op: Op, repo: &R) -> Result<(), Error> { + let doc = repo.identity_doc_at(op.identity)?.verified()?; for action in op.actions { - self.action(action, op.id, op.author, op.timestamp, op.identity, repo)?; + match self.authorization(&action, &op.author, &doc) { + Authorization::Allow => { + self.action(action, op.id, op.author, op.timestamp, op.identity, repo)?; + } + Authorization::Deny => { + return Err(Error::NotAuthorized(op.author, action)); + } + Authorization::Unknown => { + continue; + } + } } Ok(()) } } impl Issue { + /// Construct a new issue. + pub fn new(thread: Thread) -> Self { + Self { + assignees: BTreeSet::default(), + title: String::default(), + state: State::default(), + labels: BTreeSet::default(), + thread, + } + } + pub fn assigned(&self) -> impl Iterator + '_ { self.assignees.iter() } @@ -180,6 +215,46 @@ impl Issue { pub fn comments(&self) -> impl Iterator { self.thread.comments() } + + /// Apply authorization rules on issue actions. + pub fn authorization( + &self, + action: &Action, + actor: &ActorId, + doc: &Doc, + ) -> Authorization { + if doc.is_delegate(actor) { + // A delegate is authorized to do all actions. + return Authorization::Allow; + } + let author: ActorId = *self.author().id().as_key(); + + match action { + // Only delegate can assign someone to an issue. + Action::Assign { .. } => Authorization::Deny, + // Issue authors can edit their own issues. + Action::Edit { .. } => Authorization::from(*actor == author), + // Issue authors can close or re-open their own issue. + Action::Lifecycle { state } => Authorization::from(match state { + State::Closed { .. } => *actor == author, + State::Open => *actor == author, + }), + // Only delegate can label an issue. + Action::Label { .. } => Authorization::Deny, + // All roles can comment on an issues + Action::Comment { .. } => Authorization::Allow, + // All roles can edit or redact their own comments. + Action::CommentEdit { id, .. } | Action::CommentRedact { id, .. } => { + if let Some(comment) = self.comment(id) { + Authorization::from(*actor == comment.author()) + } else { + Authorization::Unknown + } + } + // All roles can react to a comment on an issue. + Action::CommentReact { .. } => Authorization::Allow, + } + } } impl Issue { diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index bf856ff6..ed1f778e 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::cob; -use crate::cob::common::{Author, Label, Reaction, Timestamp}; +use crate::cob::common::{Author, Authorization, Label, Reaction, Timestamp}; use crate::cob::store::Transaction; use crate::cob::store::{FromHistory as _, HistoryAction}; use crate::cob::thread; @@ -105,18 +105,20 @@ pub enum Error { /// Error loading the document payload. #[error("payload failed to load: {0}")] Payload(#[from] PayloadError), - /// The merge operation is invalid. - #[error("invalid merge operation in {0}")] - InvalidMerge(EntryId), /// Git error. #[error("git: {0}")] Git(#[from] git::ext::Error), - /// Validation error. - #[error("validation failed: {0}")] - Validate(&'static str), /// Store error. #[error("store: {0}")] Store(#[from] store::Error), + #[error("history error: {0}")] + History(Box), + /// Action not authorized by the author + #[error("{0} not authorized to apply {1:?}")] + NotAuthorized(ActorId, Action), + /// Initialization failed. + #[error("initialization failed: {0}")] + Init(&'static str), } /// Patch operation. @@ -341,7 +343,7 @@ impl MergeTarget { } /// Patch state. -#[derive(Debug, Clone, PartialEq, Eq, Default)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Patch { /// Title of the patch. pub(super) title: String, @@ -374,6 +376,24 @@ pub struct Patch { } impl Patch { + /// Create a valid patch + pub fn new( + title: String, + target: MergeTarget, + (revision_id, revision): (RevisionId, Revision), + ) -> Self { + Self { + title, + state: State::default(), + target, + labels: BTreeSet::default(), + merges: BTreeMap::default(), + revisions: BTreeMap::from([(revision_id, Some(revision))]), + assignees: BTreeSet::default(), + timeline: vec![revision_id.into_inner()], + reviews: BTreeMap::default(), + } + } /// Title of the patch. pub fn title(&self) -> &str { self.title.as_str() @@ -523,6 +543,118 @@ impl Patch { pub fn is_draft(&self) -> bool { matches!(self.state(), State::Draft) } + + /// Apply authorization rules on patch actions. + pub fn authorization( + &self, + action: &Action, + actor: &ActorId, + doc: &Doc, + ) -> Result { + if doc.is_delegate(actor) { + // A delegate is authorized to do all actions. + return Ok(Authorization::Allow); + } + let author = self.author().id().as_key(); + let outcome = match action { + // The patch author can edit the patch and change its state. + Action::Edit { .. } => Authorization::from(actor == author), + Action::Lifecycle { state } => Authorization::from(match state { + Lifecycle::Open { .. } => actor == author, + Lifecycle::Draft { .. } => actor == author, + Lifecycle::Archived { .. } => actor == author, + }), + // Only delegates can carry out these actions. + Action::Label { .. } => Authorization::Deny, + Action::Assign { .. } => Authorization::Deny, + Action::Merge { .. } => match self.target() { + MergeTarget::Delegates => Authorization::Deny, + }, + // Anyone can submit a review. + Action::Review { .. } => Authorization::Allow, + // The review author can edit a review. + Action::ReviewEdit { review, .. } => { + if let Some((_, review)) = lookup::review(self, review)? { + Authorization::from(actor == review.author.public_key()) + } else { + // Redacted. + Authorization::Unknown + } + } + Action::ReviewRedact { review, .. } => { + if let Some((_, review)) = lookup::review(self, review)? { + Authorization::from(actor == review.author.public_key()) + } else { + // Redacted. + Authorization::Unknown + } + } + // Anyone can comment on a review. + Action::ReviewComment { .. } => Authorization::Allow, + // The comment author can edit and redact their own comment. + Action::ReviewCommentEdit { + review, comment, .. + } + | Action::ReviewCommentRedact { review, comment } => { + if let Some((_, review)) = lookup::review(self, review)? { + if let Some(comment) = review.comments.comment(comment) { + return Ok(Authorization::from(*actor == comment.author())); + } + } + // Redacted. + Authorization::Unknown + } + // Anyone can react to a review comment. + Action::ReviewCommentReact { .. } => Authorization::Allow, + // The reviewer, commenter or revision author can resolve and unresolve review comments. + Action::ReviewCommentResolve { review, comment } + | Action::ReviewCommentUnresolve { review, comment } => { + if let Some((revision, review)) = lookup::review(self, review)? { + if let Some(comment) = review.comments.comment(comment) { + return Ok(Authorization::from( + actor == &comment.author() + || actor == review.author.public_key() + || actor == revision.author.public_key(), + )); + } + } + // Redacted. + Authorization::Unknown + } + // Only patch authors can propose revisions. + Action::Revision { .. } => Authorization::from(actor == author), + // Only the revision author can edit or redact their revision. + Action::RevisionEdit { revision, .. } | Action::RevisionRedact { revision, .. } => { + if let Some(revision) = lookup::revision(self, revision)? { + Authorization::from(actor == revision.author.public_key()) + } else { + // Redacted. + Authorization::Unknown + } + } + // Anyone can react to or comment on a revision. + Action::RevisionReact { .. } => Authorization::Allow, + Action::RevisionComment { .. } => Authorization::Allow, + // Only the comment author can edit or redact their comment. + Action::RevisionCommentEdit { + revision, comment, .. + } + | Action::RevisionCommentRedact { + revision, comment, .. + } => { + if let Some(revision) = lookup::revision(self, revision)? { + if let Some(comment) = revision.discussion.comment(comment) { + return Ok(Authorization::from(actor == &comment.author())); + } + } + // Redacted. + Authorization::Unknown + } + // Anyone can react to a revision. + Action::RevisionCommentReact { .. } => Authorization::Allow, + }; + Ok(outcome) + } } impl Patch { @@ -533,7 +665,7 @@ impl Patch { entry: EntryId, author: ActorId, timestamp: Timestamp, - identity: git::Oid, + identity: &Doc, repo: &R, ) -> Result<(), Error> { match action { @@ -576,19 +708,18 @@ impl Patch { summary, verdict, } => { - let Some(Some((revision, author))) = - self.reviews.get(&review) else { - return Err(Error::Missing(review.into_inner())); - }; + let Some(Some((revision, author))) = self.reviews.get(&review) else { + return Err(Error::Missing(review.into_inner())); + }; let Some(rev) = self.revisions.get_mut(revision) else { - return Err(Error::Missing(revision.into_inner())); - }; + return Err(Error::Missing(revision.into_inner())); + }; // If the revision was redacted concurrently, there's nothing to do. // Likewise, if the review was redacted concurrently, there's nothing to do. if let Some(rev) = rev { let Some(review) = rev.reviews.get_mut(author) else { - return Err(Error::Missing(review.into_inner())); - }; + return Err(Error::Missing(review.into_inner())); + }; if let Some(review) = review { review.summary = summary; review.verdict = verdict; @@ -621,7 +752,7 @@ impl Patch { active, location, } => { - if let Some(revision) = lookup::revision(self, &revision)? { + if let Some(revision) = lookup::revision_mut(self, &revision)? { let key = (author, reaction); let reactions = revision.reactions.entry(location).or_default(); @@ -652,14 +783,20 @@ impl Patch { labels, } => { let Some(rev) = self.revisions.get_mut(&revision) else { - return Err(Error::Missing(revision.into_inner())); - }; + return Err(Error::Missing(revision.into_inner())); + }; if let Some(rev) = rev { // Nb. Applying two reviews by the same author is not allowed and // results in the review being redacted. rev.reviews.insert( author, - Some(Review::new(verdict, summary.to_owned(), labels, timestamp)), + Some(Review::new( + Author::new(author), + verdict, + summary.to_owned(), + labels, + timestamp, + )), ); // Update reviews index. self.reviews @@ -672,7 +809,7 @@ impl Patch { reaction, active, } => { - if let Some(review) = lookup::review(self, &review)? { + if let Some(review) = lookup::review_mut(self, &review)? { thread::react( &mut review.comments, entry, @@ -684,7 +821,7 @@ impl Patch { } } Action::ReviewCommentRedact { review, comment } => { - if let Some(review) = lookup::review(self, &review)? { + if let Some(review) = lookup::review_mut(self, &review)? { thread::redact(&mut review.comments, entry, comment)?; } } @@ -693,7 +830,7 @@ impl Patch { comment, body, } => { - if let Some(review) = lookup::review(self, &review)? { + if let Some(review) = lookup::review_mut(self, &review)? { thread::edit( &mut review.comments, entry, @@ -705,12 +842,12 @@ impl Patch { } } Action::ReviewCommentResolve { review, comment } => { - if let Some(review) = lookup::review(self, &review)? { + if let Some(review) = lookup::review_mut(self, &review)? { thread::resolve(&mut review.comments, entry, comment)?; } } Action::ReviewCommentUnresolve { review, comment } => { - if let Some(review) = lookup::review(self, &review)? { + if let Some(review) = lookup::review_mut(self, &review)? { thread::unresolve(&mut review.comments, entry, comment)?; } } @@ -720,7 +857,7 @@ impl Patch { location, reply_to, } => { - if let Some(review) = lookup::review(self, &review)? { + if let Some(review) = lookup::review_mut(self, &review)? { thread::comment( &mut review.comments, entry, @@ -736,41 +873,36 @@ impl Patch { Action::ReviewRedact { review } => { // Redactions must have observed a review to be valid. let Some(locator) = self.reviews.get_mut(&review) else { - return Err(Error::Missing(review.into_inner())); - }; + return Err(Error::Missing(review.into_inner())); + }; // If the review is already redacted, do nothing. let Some((revision, reviewer)) = locator else { - return Ok(()); - }; + return Ok(()); + }; // The revision must have existed at some point. let Some(redactable) = self.revisions.get_mut(revision) else { - return Err(Error::Missing(revision.into_inner())); - }; + return Err(Error::Missing(revision.into_inner())); + }; // But it could be redacted. let Some(revision) = redactable else { - return Ok(()); - }; + return Ok(()); + }; // The review must have existed as well. let Some(review) = revision.reviews.get_mut(reviewer) else { - return Err(Error::Missing(review.into_inner())); - }; + return Err(Error::Missing(review.into_inner())); + }; // Set both the review and the locator in the review index to redacted. *review = None; *locator = None; } Action::Merge { revision, commit } => { // If the revision was redacted before the merge, ignore the merge. - if lookup::revision(self, &revision)?.is_none() { + if lookup::revision_mut(self, &revision)?.is_none() { return Ok(()); }; - let doc = repo.identity_doc_at(identity)?.verified()?; - match self.target() { MergeTarget::Delegates => { - if !doc.is_delegate(&author) { - return Err(Error::InvalidMerge(entry)); - } - let proj = doc.project()?; + let proj = identity.project()?; let branch = git::refs::branch(proj.default_branch()); // Nb. We don't return an error in case the merge commit is not an @@ -804,7 +936,7 @@ impl Patch { }, ); // Discard revisions that weren't merged by a threshold of delegates. - merges.retain(|_, count| *count >= doc.threshold); + merges.retain(|_, count| *count >= identity.threshold); match merges.into_keys().collect::>().as_slice() { [] => { @@ -832,7 +964,7 @@ impl Patch { reply_to, .. } => { - if let Some(revision) = lookup::revision(self, &revision)? { + if let Some(revision) = lookup::revision_mut(self, &revision)? { thread::comment( &mut revision.discussion, entry, @@ -850,7 +982,7 @@ impl Patch { comment, body, } => { - if let Some(revision) = lookup::revision(self, &revision)? { + if let Some(revision) = lookup::revision_mut(self, &revision)? { thread::edit( &mut revision.discussion, entry, @@ -862,7 +994,7 @@ impl Patch { } } Action::RevisionCommentRedact { revision, comment } => { - if let Some(revision) = lookup::revision(self, &revision)? { + if let Some(revision) = lookup::revision_mut(self, &revision)? { thread::redact(&mut revision.discussion, entry, comment)?; } } @@ -872,7 +1004,7 @@ impl Patch { reaction, active, } => { - if let Some(revision) = lookup::revision(self, &revision)? { + if let Some(revision) = lookup::revision_mut(self, &revision)? { thread::react( &mut revision.discussion, entry, @@ -896,22 +1028,53 @@ impl store::FromHistory for Patch { &TYPENAME } - fn validate(&self) -> Result<(), Self::Error> { - if self.revisions.is_empty() { - return Err(Error::Validate("no revisions found")); + fn init(op: Op, repo: &R) -> Result { + let mut actions = op.actions.into_iter(); + let Some(Action::Revision { description, base, oid, resolves }) = actions.next() else { + return Err(Error::Init("the first action must be of type `revision`")); + }; + let Some(Action::Edit { title, target }) = actions.next() else { + return Err(Error::Init("the second action must be of type `edit`")); + }; + let revision = Revision::new( + op.author.into(), + description, + base, + oid, + op.timestamp, + resolves, + ); + let mut patch = Patch::new(title, target, (RevisionId(op.id), revision)); + let doc = repo.identity_doc_at(op.identity)?.verified()?; + + for action in actions { + patch.action(action, op.id, op.author, op.timestamp, &doc, repo)?; } - if self.title().is_empty() { - return Err(Error::Validate("empty title")); - } - Ok(()) + Ok(patch) } fn apply(&mut self, op: Op, repo: &R) -> Result<(), Error> { debug_assert!(!self.timeline.contains(&op.id)); self.timeline.push(op.id); + let doc = repo.identity_doc_at(op.identity)?.verified()?; + for action in op.actions { - self.action(action, op.id, op.author, op.timestamp, op.identity, repo)?; + match self.authorization(&action, &op.author, &doc)? { + Authorization::Allow => { + self.action(action, op.id, op.author, op.timestamp, &doc, repo)?; + } + Authorization::Deny => { + return Err(Error::NotAuthorized(op.author, action)); + } + Authorization::Unknown => { + // In this case, since there is not enough information to determine + // whether the action is authorized or not, we simply ignore it. + // It's likely that the target object was redacted, and we can't + // verify whether the action would have been allowed or not. + continue; + } + } } Ok(()) } @@ -921,6 +1084,19 @@ mod lookup { use super::*; pub fn revision<'a>( + patch: &'a Patch, + revision: &RevisionId, + ) -> Result, Error> { + match patch.revisions.get(revision) { + Some(Some(revision)) => Ok(Some(revision)), + // Redacted. + Some(None) => Ok(None), + // Missing. Causal error. + None => Err(Error::Missing(revision.into_inner())), + } + } + + pub fn revision_mut<'a>( patch: &'a mut Patch, revision: &RevisionId, ) -> Result, Error> { @@ -934,6 +1110,35 @@ mod lookup { } pub fn review<'a>( + patch: &'a Patch, + review: &ReviewId, + ) -> Result, Error> { + match patch.reviews.get(review) { + Some(Some((revision, author))) => { + match patch.revisions.get(revision) { + Some(Some(r)) => { + let Some(review) = r.reviews.get(author) else { + return Err(Error::Missing(review.into_inner())); + }; + Ok(review.as_ref().map(|review| (r, review))) + } + Some(None) => { + // If the revision was redacted concurrently, there's nothing to do. + // Likewise, if the review was redacted concurrently, there's nothing to do. + Ok(None) + } + None => Err(Error::Missing(revision.into_inner())), + } + } + Some(None) => { + // Redacted. + Ok(None) + } + None => Err(Error::Missing(review.into_inner())), + } + } + + pub fn review_mut<'a>( patch: &'a mut Patch, review: &ReviewId, ) -> Result, Error> { @@ -942,8 +1147,8 @@ mod lookup { match patch.revisions.get_mut(revision) { Some(Some(r)) => { let Some(review) = r.reviews.get_mut(author) else { - return Err(Error::Missing(review.into_inner())); - }; + return Err(Error::Missing(review.into_inner())); + }; Ok(review.as_mut()) } Some(None) => { @@ -1180,8 +1385,10 @@ pub struct CodeLocation { } /// A patch review on a revision. -#[derive(Debug, Default, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Review { + /// Review author. + pub(super) author: Author, /// Review verdict. /// /// The verdict cannot be changed, since revisions are immutable. @@ -1218,12 +1425,14 @@ impl Serialize for Review { impl Review { pub fn new( + author: Author, verdict: Option, summary: Option, labels: Vec