From f26be552c20e8db05ce9e0aad9d61ab6e4e2bf48 Mon Sep 17 00:00:00 2001
From: Fintan Halpenny <fintan.halpenny@gmail.com>
Date: Mon, 20 May 2024 17:10:43 +0100
Subject: [PATCH] cli: improve rad id allow list UX

The previous implementation of using the `--allow` option in `rad id
update` would be an absolute list, rather than additive. That is, if
there were existing DIDs not passed in via `--allow` then they would
be remove.

Change this behaviour to check the existing value of the document's
allow list, and add to it if `--allow` is specified.

To enable the user to also remove a DID, the `--disallow` option is
also added.

The correct usage of these options is improved by checking the
validity of the `--visibility` option being used, the current state of
the repository's privacy, and the use of the `--allow` and
`--disallow` options.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
---
 radicle-cli/examples/rad-id-private.md | 101 +++++++++++++++++++++++++
 radicle-cli/src/commands/id.rs         |  90 +++++++++++++++++++---
 radicle-cli/tests/commands.rs          |  24 ++++++
 3 files changed, 206 insertions(+), 9 deletions(-)
 create mode 100644 radicle-cli/examples/rad-id-private.md

diff --git a/radicle-cli/examples/rad-id-private.md b/radicle-cli/examples/rad-id-private.md
new file mode 100644
index 00000000..b09a34a8
--- /dev/null
+++ b/radicle-cli/examples/rad-id-private.md
@@ -0,0 +1,101 @@
+When we are working with a private repository, we can modify the list of peers
+we allow by using the `rad id` command with its `--allow` and `--disallow`
+options. Both options can be specified multiple times in the same command line call:
+
+Here we will add Bob and Eve's DIDs to the `allow`list:
+
+```
+$ rad id update --title "Allow Bob & Eve" --description "" --allow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --allow did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z -q
+...
+$ rad inspect --identity
+{
+  "payload": {
+    "xyz.radicle.project": {
+      "defaultBranch": "master",
+      "description": "radicle heartwood protocol & stack",
+      "name": "heartwood"
+    }
+  },
+  "delegates": [
+    "did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi"
+  ],
+  "threshold": 1,
+  "visibility": {
+    "type": "private",
+    "allow": [
+      "did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk",
+      "did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z"
+    ]
+  }
+}
+```
+
+To remove a peer's DID, we can use the `--disallow` option. Let's remove both of them again:
+
+```
+$ rad id update --title "Remove allow list" --description "" --disallow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --disallow did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z
+...
+$ rad inspect --identity
+{
+  "payload": {
+    "xyz.radicle.project": {
+      "defaultBranch": "master",
+      "description": "radicle heartwood protocol & stack",
+      "name": "heartwood"
+    }
+  },
+  "delegates": [
+    "did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi"
+  ],
+  "threshold": 1,
+  "visibility": {
+    "type": "private"
+  }
+}
+```
+
+Note that using both `--disallow` and `--allow` with the same DID will result in
+an error:
+
+``` (fails)
+$ rad id update --title "Remove allow list" --description "" --allow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --disallow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk
+✗ Error: `--allow` and `--disallow` must not overlap: ["did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk"]
+```
+
+Allowing or disallowing the same peer twice will result in an error the second
+call, since there is no update specified:
+
+```
+$ rad id update --title "Allow Bob" --description "" --allow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk -q
+...
+```
+``` (fails)
+$ rad id update --title "Allow Bob" --description "" --allow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk -q
+✗ Error: no update specified
+✗ Hint: an update to the identity must be specified, run `rad id update -h` to see the available options
+```
+
+If we attempt to change the list while also changing the repository to `public`,
+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`
+```
+
+Let's change the repository to `public`:
+
+```
+$ rad id update --title "IPO" --description "" --visibility public -q
+...
+```
+
+Now, if we attempt to change the `allow` list we also get an error with a
+helpful hint:
+
+``` (fails)
+$ rad id update --allow did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk
+✗ 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`
+```
+
diff --git a/radicle-cli/src/commands/id.rs b/radicle-cli/src/commands/id.rs
index 77729bfe..215b2f84 100644
--- a/radicle-cli/src/commands/id.rs
+++ b/radicle-cli/src/commands/id.rs
@@ -1,3 +1,5 @@
+use std::collections::BTreeSet;
+use std::str::FromStr;
 use std::{ffi::OsString, io};
 
 use anyhow::{anyhow, Context};
@@ -32,7 +34,8 @@ Usage
     rad id update [--title <string>] [--description <string>]
                   [--delegate <did>] [--rescind <did>]
                   [--threshold <num>] [--visibility <private | public>]
-                  [--allow <did>] [--no-confirm] [--payload <id> <key> <val>...]
+                  [--allow <did>] [--disallow <did>]
+                  [--no-confirm] [--payload <id> <key> <val>...]
                   [<option>...]
     rad id edit <revision-id> [--title <string>] [--description <string>] [<option>...]
     rad id show <revision-id> [<option>...]
@@ -59,7 +62,9 @@ pub enum Operation {
         delegate: Vec<Did>,
         rescind: Vec<Did>,
         threshold: Option<usize>,
-        visibility: Option<Visibility>,
+        visibility: Option<EditVisibility>,
+        allow: BTreeSet<Did>,
+        disallow: BTreeSet<Did>,
         payload: Vec<(doc::PayloadId, String, json::Value)>,
     },
     AcceptRevision {
@@ -83,6 +88,29 @@ 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<Self, Self::Err> {
+        match s {
+            "public" => Ok(EditVisibility::Public),
+            "private" => Ok(EditVisibility::Private),
+            _ => Err(EditVisibilityParseError(s.to_owned())),
+        }
+    }
+}
+
 #[derive(Default, PartialEq, Eq)]
 pub enum OperationName {
     Accept,
@@ -114,7 +142,9 @@ impl Args for Options {
         let mut description: Option<String> = None;
         let mut delegate: Vec<Did> = Vec::new();
         let mut rescind: Vec<Did> = Vec::new();
-        let mut visibility: Option<Visibility> = None;
+        let mut visibility: Option<EditVisibility> = None;
+        let mut allow: BTreeSet<Did> = BTreeSet::new();
+        let mut disallow: BTreeSet<Did> = BTreeSet::new();
         let mut threshold: Option<usize> = None;
         let mut interactive = Interactive::new(io::stdout());
         let mut payload = Vec::new();
@@ -172,11 +202,12 @@ impl Args for Options {
                 Long("allow") => {
                     let value = parser.value()?;
                     let did = term::args::did(&value)?;
-                    if let Some(Visibility::Private { allow }) = &mut visibility {
-                        allow.insert(did);
-                    } else {
-                        visibility = Some(Visibility::private([did]));
-                    }
+                    allow.insert(did);
+                }
+                Long("disallow") => {
+                    let value = parser.value()?;
+                    let did = term::args::did(&value)?;
+                    disallow.insert(did);
                 }
                 Long("visibility") => {
                     let value = parser.value()?;
@@ -244,6 +275,8 @@ impl Args for Options {
                 rescind,
                 threshold,
                 visibility,
+                allow,
+                disallow,
                 payload,
             },
         };
@@ -349,12 +382,51 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
             rescind,
             threshold,
             visibility,
+            allow,
+            disallow,
             payload,
         } => {
             let proposal = {
                 let mut proposal = current.doc.clone();
                 proposal.threshold = threshold.unwrap_or(proposal.threshold);
-                proposal.visibility = visibility.unwrap_or(proposal.visibility);
+
+                if !allow.is_disjoint(&disallow) {
+                    let overlap = allow
+                        .intersection(&disallow)
+                        .map(Did::to_string)
+                        .collect::<Vec<_>>();
+                    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 {
+                            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);
+                        }
+                    }
+                    (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 = NonEmpty::from_vec(
                     proposal
                         .delegates
diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs
index e9a555e8..61ee44d5 100644
--- a/radicle-cli/tests/commands.rs
+++ b/radicle-cli/tests/commands.rs
@@ -716,6 +716,30 @@ fn rad_id_unknown_field() {
     .unwrap();
 }
 
+#[test]
+fn rad_id_private() {
+    let mut environment = Environment::new();
+    let alice = environment.node(Config::test(Alias::new("alice")));
+    let working = environment.tmp().join("working");
+
+    fixtures::repository(working.join("alice"));
+
+    test(
+        "examples/rad-init-private.md",
+        working.join("alice"),
+        Some(&alice.home),
+        [],
+    )
+    .unwrap();
+    test(
+        "examples/rad-id-private.md",
+        working.join("alice"),
+        Some(&alice.home),
+        [],
+    )
+    .unwrap();
+}
+
 #[test]
 fn rad_node_connect() {
     let mut environment = Environment::new();