Add missing shared recovery cli commands #9100

AureliaDolo · 2024-12-03T14:55:02Z

TODO

End to end test scenario:

assert alice's shared recovery info is never setup echo P@ssw0rd. | target/debug/parsec-cli shared-recovery info --password-stdin -d a79
setup shared recovery for alice, bob and toto, with the two other users as recipient, with threshold = 1 echo P@ssw0rd. | target/debug/parsec-cli shared-recovery create --password-stdin -d 932 -t 1 -r [email protected] [email protected]

in the current test_env organization, toto is external, so he can't access alice's and bob's informations, so he needs to rely on the default shamir recipients (ie all admins = just alice)
assert alice's shared recovery info is setup
assert alice's shared recovery list has bob's and toto's setup listed echo P@ssw0rd. | target/debug/parsec-cli shared-recovery list --password-stdin -d a79
remove alice's setup echo P@ssw0rd. | target/debug/parsec-cli shared-recovery delete --password-stdin -d a79
assert alice's shared recovery info is deleted
assert bob's shared recovery list has toto's setup setup and alice's deleted
setup shared recovery for alice with the same args as before
assert alice's shared recovery info is setup
revoke toto
assert alice's shared recovery info is warning that a recipient is revoked
assert bob's shared recovery list has alice's warning that a recipient is revoked
revoke bob
assert alice's shared recovery info is warning that a recipient is unusable
assert bob's shared recovery list has alice's setup unusable

how to revoke

This section describe how to test revocation for now, as the cli command for revocation is not exposed (see #9143). Make sure to close/kill EVERY instance of parsec and testbed that's laying around before starting.

run a testbed server in a separated terminal
in a separated terminal run the testenv for the cli
in a separated terminal open the GUI (this part will become unnecessary)
- export the variable in the output of the cli testenv (so this parsec will use the test organization, not your real data)
- cd client
- npm install
- cd electron
- npm install
- npx electron . --no-sandbox
check that both the gui and the cli connect to the same organization (by creating a workspace on one side and checking it exists on the other side

Note that to connect to the gui, you're using one device, so if you don't want to close the gui to use the cli, you'll need to use alice's other device
cli : setup alice's shared recovery
gui: revoke toto
cli: check alice's shared recovery

cli/tests/integration/shared_recovery.rs

cli/src/commands/shared_recovery/delete.rs

cli/src/commands/shared_recovery/info.rs

cli/src/commands/shared_recovery/list.rs

FirelightFlagboy

The command shared-recovery create is often called in the tests,
Could its logic be extracted in a helpers that could be used without using the CLI ?

FirelightFlagboy · 2024-12-06T15:30:27Z

cli/tests/integration/shared_recovery/delete.rs

+
+#[rstest::rstest]
+#[tokio::test]
+async fn remove_shared_recovery_ok(tmp_path: TmpPath) {


Try to limit call to the cli, each call add 2sec (on my machine) to the test duration.

Here some call could be removed since they're tested elsewhere (I think of the 2 info command)

I just noticed the cli tests do not use the testbed pre-configured organization. Those would solve the slowness you are mentioning. I suggest we:

factor test initialization into a fixture (i.e. the creation of shamir recoveries)

create an issue to migrate the cli tests to the testbed pre-configured organization

I created the corresponding issue: #9144

FirelightFlagboy · 2024-12-06T15:36:41Z

cli/tests/integration/shared_recovery/list.rs

+    crate::assert_cmd_success!(
+        with_password = DEFAULT_DEVICE_PASSWORD,
+        "shared-recovery",
+        "list",
+        "--device",
+        &bob.device_id.hex()
+    )
+    .stdout(predicates::str::contains("No shared recovery found"));
+


Suggested change

crate::assert_cmd_success!(

with_password = DEFAULT_DEVICE_PASSWORD,

"shared-recovery",

"list",

"--device",

&bob.device_id.hex()

)

.stdout(predicates::str::contains("No shared recovery found"));

I don't get why this chink should be delted

#9100 (comment)

vxgmichel

A couple of suggestions, LGTM otherwise 👍

cli/src/commands/shared_recovery/create.rs

cli/src/commands/shared_recovery/delete.rs

cli/src/commands/shared_recovery/info.rs

cli/src/commands/shared_recovery/list.rs

* Cli shared recovery create - ask for threshold if not provided. * CLI shared recovery create - add test for interactive threshold * [CLI] rework spinners and display for shared recovery.

FirelightFlagboy · 2024-12-11T09:05:54Z

cli/tests/integration/shared_recovery/mod.rs

+    crate::assert_cmd_success!(
+        with_password = DEFAULT_DEVICE_PASSWORD,
+        "shared-recovery",
+        "info",
+        "--device",
+        &alice.device_id.hex()
+    )
+    .stdout(predicates::str::contains(format!(
+        "Shared recovery {GREEN}set up{RESET}"
+    )));


Could this be done without using the CLI ?

@FirelightFlagboy

Could this be done without using the CLI ?

Personally I'd rather have more CLI testing than less, even though this test is meant to test another command than shared-recovery info.

I understand this is a trade-off between unit tests being more "unit" and more test coverage, and I tend to choose the latter.

vxgmichel

Looks much better, thanks 👍

cli/src/commands/shared_recovery/list.rs

cli/tests/integration/shared_recovery/mod.rs

vxgmichel · 2024-12-11T13:03:17Z

cli/tests/integration/shared_recovery/mod.rs

+    crate::assert_cmd_success!(
+        with_password = DEFAULT_DEVICE_PASSWORD,
+        "shared-recovery",
+        "info",
+        "--device",
+        &alice.device_id.hex()
+    )
+    .stdout(predicates::str::contains(format!(
+        "Shared recovery {GREEN}set up{RESET}"
+    )));


@FirelightFlagboy

Could this be done without using the CLI ?

Personally I'd rather have more CLI testing than less, even though this test is meant to test another command than shared-recovery info.

I understand this is a trade-off between unit tests being more "unit" and more test coverage, and I tend to choose the latter.

AureliaDolo marked this pull request as ready for review December 4, 2024 10:49

AureliaDolo requested a review from a team as a code owner December 4, 2024 10:49

FirelightFlagboy requested changes Dec 4, 2024

View reviewed changes

cli/tests/integration/shared_recovery.rs Outdated Show resolved Hide resolved

FirelightFlagboy reviewed Dec 4, 2024

View reviewed changes

cli/src/commands/shared_recovery/list.rs Outdated Show resolved Hide resolved

cli/src/commands/shared_recovery/list.rs Outdated Show resolved Hide resolved

AureliaDolo requested review from FirelightFlagboy and touilleMan December 5, 2024 14:48

FirelightFlagboy reviewed Dec 6, 2024

View reviewed changes

vxgmichel approved these changes Dec 9, 2024

View reviewed changes

AureliaDolo and others added 8 commits December 10, 2024 17:26

Implement list shamred recovery cli command.

d9a6aa8

Add cli remove shared recovery setup.

e0d41f9

Add info shared recovery device cli command.

0f4c0d3

Fixup shared recovery creation cli command test.

159778a

Add tests for shared recovery cli commands.

038bf69

split cli shared recovery tests.

d99e3da

CLI shared recovery format displayed infos

a29ca71

Cli shared recovery create - ask for threshold if not provided. (#9127)

08c10f6

* Cli shared recovery create - ask for threshold if not provided. * CLI shared recovery create - add test for interactive threshold * [CLI] rework spinners and display for shared recovery.

AureliaDolo force-pushed the aurelia/cli/shamir branch from 982ec52 to 08c10f6 Compare December 10, 2024 16:32

AureliaDolo requested a review from FirelightFlagboy December 10, 2024 16:47

AureliaDolo added 2 commits December 10, 2024 18:05

Fix test. cli shared recovery

d098840

CLI factorize client start up for shared recovery.

e4fecc0

FirelightFlagboy reviewed Dec 11, 2024

View reviewed changes

vxgmichel approved these changes Dec 11, 2024

View reviewed changes

AureliaDolo added 2 commits December 11, 2024 16:47

CLI shared recovery fixup

915edb2

Add user human handles ifor shared recovery cli list and info.

1d225c2

AureliaDolo requested a review from FirelightFlagboy December 11, 2024 16:26

Fix test cli shared recovery list.

e77c235

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing shared recovery cli commands #9100

Add missing shared recovery cli commands #9100

AureliaDolo commented Dec 3, 2024 •

edited

Loading

FirelightFlagboy left a comment

FirelightFlagboy Dec 6, 2024

vxgmichel Dec 9, 2024

vxgmichel Dec 9, 2024

FirelightFlagboy Dec 6, 2024

AureliaDolo Dec 10, 2024

FirelightFlagboy Dec 11, 2024

vxgmichel left a comment

FirelightFlagboy Dec 11, 2024

vxgmichel Dec 11, 2024 •

edited

Loading

vxgmichel left a comment

vxgmichel Dec 11, 2024 •

edited

Loading

Add missing shared recovery cli commands #9100

Are you sure you want to change the base?

Add missing shared recovery cli commands #9100

Conversation

AureliaDolo commented Dec 3, 2024 • edited Loading

how to revoke

FirelightFlagboy left a comment

Choose a reason for hiding this comment

FirelightFlagboy Dec 6, 2024

Choose a reason for hiding this comment

vxgmichel Dec 9, 2024

Choose a reason for hiding this comment

vxgmichel Dec 9, 2024

Choose a reason for hiding this comment

FirelightFlagboy Dec 6, 2024

Choose a reason for hiding this comment

AureliaDolo Dec 10, 2024

Choose a reason for hiding this comment

FirelightFlagboy Dec 11, 2024

Choose a reason for hiding this comment

vxgmichel left a comment

Choose a reason for hiding this comment

FirelightFlagboy Dec 11, 2024

Choose a reason for hiding this comment

vxgmichel Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

vxgmichel left a comment

Choose a reason for hiding this comment

vxgmichel Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

AureliaDolo commented Dec 3, 2024 •

edited

Loading

vxgmichel Dec 11, 2024 •

edited

Loading

vxgmichel Dec 11, 2024 •

edited

Loading