Radish alpha
h
heartwood
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
Patches › ed97c94f
cli: improve rad id allow list UX
Merged fintohaps opened 1 year ago

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.

Activity Changes +206 -9
3 files changed +206 -9 6f91f2fb → f26be552
added 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 have different DIDs: ["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`
+ 
```
+
modified 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 have different DIDs: {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
modified radicle-cli/tests/commands.rs
@@ -717,6 +717,30 @@ fn rad_id_unknown_field() {
}



#[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();

    let alice = environment.node(Config::test(Alias::new("alice")));