Add support for shamir recovery invitation #8819

vxgmichel · 2024-10-30T10:51:34Z

Part of #6090

Tested with:

$ . ./cli/run_testenv.sh 
[...]
Alice & Bob devices (password: P@ssw0rd.):
- 624 // Alice
- 196 // Alice 2nd device
- 1f2 // Bob

$ echo P@ssw0rd. | target/debug/parsec-cli shared-recovery create --password-stdin -d 1f2
Shamir setup has been created

$ echo P@ssw0rd. | target/debug/parsec-cli invite shared-recovery --password-stdin -d 624 --email [email protected]
Invitation URL: parsec3://127.0.0.1:41997/Org?no_ssl=true&a=claim_shamir_recovery&p=xBCqEy9RIO8HPyycmlus9Qzg

$ echo P@ssw0rd. | target/debug/parsec-cli invite list --password-stdin -d 624
aa132f5120ef073f2c9c9a5bacf50ce0        idle    shamir recovery (Bob <[email protected]>)

$ echo P@ssw0rd. | target/debug/parsec-cli invite cancel --password-stdin -d 624 --token aa132f5120ef073f2c9c9a5bacf50ce0
Invitation deleted

$ echo P@ssw0rd. | target/debug/parsec-cli invite list --password-stdin -d 624 
aa132f5120ef073f2c9c9a5bacf50ce0        cancelled       shamir recovery (Bob <[email protected]>)

vxgmichel · 2024-11-01T16:18:54Z

Missing tests for:

invite_list with shamir recovery
invite_info for shamir recovery
certificates_get with shamir certificates

AureliaDolo · 2024-11-04T09:10:21Z

server/parsec/components/memory/datamodel.py

@@ -289,8 +289,12 @@ class MemoryInvitation:
    type: InvitationType
    created_by_user_id: UserID
    created_by_device_id: DeviceID
+


Suggested change

I grouped the attribute on purpose, to easily visualize those that are type dependent

server/parsec/components/memory/datamodel.py

AureliaDolo · 2024-11-04T09:28:44Z

server/parsec/components/memory/invite.py

+            if shamir_setup is None:
+                # Since the author only knows about a shamir recovery if they are part of it,
+                # we don't have a specific error for the case where the shamir setup doesn't exist
+                return InviteNewForShamirBadOutcome.AUTHOR_NOT_ALLOWED


Is there a specific reason to not define a SetupNotFound error ?

AFAIC we decided to not share the brief and removal certificates with users that are not part of the corresponding shamir. That means that users don't have a way to know whether another user has a shamir recovery setup when they are not part of it. For this reason, it would be weird to have the server give this information through an error status.

AureliaDolo · 2024-11-04T09:39:35Z

server/parsec/templates/invitation_mail.html

                        3. Start the invitation process from a device already part of the organization,
                        then follow the steps on the Parsec client.
+                        {% elif is_shamir_recovery_invitation %}
+                        3. Get in touch with <b>{{ greeter }}</b> and follow the next steps on the Parsec client.


There is one mail sent per greeter ? Does the order of greeter matter ?

In this context, the greeter is the user that created the invitation link. That user is part of the recipients so it's a good idea to include their name in the mail, although the claimer will have to contact some of the other recipients too.

cli/src/commands/invite/greet.rs

cli/src/commands/invite/claim.rs

cli/src/commands/invite/shared_recovery.rs

FirelightFlagboy · 2024-11-04T09:53:18Z

cli/src/commands/invite/shared_recovery.rs

+            InvitationEmailSentStatus::RecipientRefused => {
+                println!(
+                    "Invitation email not sent to {} because the recipient was refused",
+                    email
+                );
+            }
+            InvitationEmailSentStatus::ServerUnavailable => {
+                println!(
+                    "Invitation email not sent to {} because the server is unavailable",
+                    email
+                );
+            }


If the email was not sent, should the CLI fail with an error ?

It's expected that the message being print to stdout and not stderr ? (println vs eprintln)

I understand the email sent status as extra information, so maybe it could be a warning if the mail wasn't sent properly?

Also we should probably start a discussion about making sure that all commands use the same approach to error handling.

FirelightFlagboy · 2024-11-04T09:56:38Z

Missing multiple news fragments for the CLI change

vxgmichel · 2024-11-04T11:44:41Z

@FirelightFlagboy

Missing multiple news fragments for the CLI change

You mean for #8429 or for other shamir related commands?

I don't think it's worth adding newsfragments for the different shamir commands, a single newsfragment commenting the general feature at the end is enough IMO.

FirelightFlagboy · 2024-11-04T12:09:40Z

You would expect at least 2 newsfragment: one for the updated example in the CLI help message, and the other that could group the change related to the added/updated shamir command

vxgmichel · 2024-11-04T12:28:56Z

one for the updated example in the CLI help message,

Does that really deserve a newsfragment though? I'd say no, but I can add it if necessary.

and the other that could group the change related to the added/updated shamir command

This will come later once the feature is fully implemented.

cli/src/commands/invite/greet.rs

FirelightFlagboy · 2024-11-04T12:31:20Z

cli/src/commands/invite/shared_recovery.rs

+        #[arg(long)]
+        email: String,


What do you think of providing the email as a argument over an option ?

Does our clap_parser_with_shared_opts_builder allow to do that?

libparsec/crates/client/src/invite/claimer.rs

libparsec/crates/protocol/schema/authenticated_cmds/invite_list.json5

libparsec/crates/protocol/schema/invited_cmds/invite_info.json5

FirelightFlagboy · 2024-11-04T12:36:27Z

libparsec/crates/types/tests/unit/addr.rs

@@ -489,7 +489,8 @@ fn invitation_addr_bad_type(
                &url,
                AddrError::InvalidParamValue {
                    param: "a",
-                    help: "Expected `a=claim_user` or `a=claim_device`".to_string(),
+                    help: "Expected `a=claim_user`, `a=claim_device` or `a=claim_shamir_recovery`"


nitpick: Could we use constant here ?

Feel free to provide a suggested change

vxgmichel requested review from a team as code owners October 30, 2024 10:51

vxgmichel marked this pull request as draft October 30, 2024 10:51

vxgmichel force-pushed the issue-7359 branch 10 times, most recently from 2c19d47 to c63b236 Compare November 1, 2024 16:12

vxgmichel marked this pull request as ready for review November 1, 2024 16:12

vxgmichel requested review from a team as code owners November 1, 2024 16:12

vxgmichel force-pushed the issue-7359 branch 2 times, most recently from 3dcdb16 to 35ea62b Compare November 1, 2024 17:10

AureliaDolo reviewed Nov 4, 2024

View reviewed changes

FirelightFlagboy requested changes Nov 4, 2024

View reviewed changes

FirelightFlagboy linked an issue Nov 4, 2024 that may be closed by this pull request

[🐛 | Bug]: CLI claim invitation exemple at bad format #8429

Open

FirelightFlagboy requested changes Nov 4, 2024

View reviewed changes

vxgmichel added 2 commits November 8, 2024 11:32

Remove useless hooks

c219324

Add invite_new_shamir command

d94beaf

vxgmichel force-pushed the issue-7359 branch from af9b9f4 to db4f94d Compare November 8, 2024 10:33

vxgmichel added 8 commits November 8, 2024 11:44

Fix cspell

df63a38

Address @FirelightFlagboy's comments

d1e0308

Bump API version to 4.1

41b2563

Make listUserInvitations ignore shamir recovery invitations

506e3f4

Add shamir recovery invitation to invite_list tests

ff9247f

Remove unused ShamirRevealToken type hints

09d8178

Improve shamir_setup_create::ok test

7082f3d

Bump testbed server

1d426b0

vxgmichel force-pushed the issue-7359 branch from db4f94d to 1d426b0 Compare November 8, 2024 10:44

Address @FirelightFlagboy's comments

f934021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for shamir recovery invitation #8819

Add support for shamir recovery invitation #8819

vxgmichel commented Oct 30, 2024 •

edited

Loading

vxgmichel commented Nov 1, 2024 •

edited

Loading

AureliaDolo Nov 4, 2024

vxgmichel Nov 4, 2024 •

edited

Loading

AureliaDolo Nov 4, 2024

vxgmichel Nov 4, 2024 •

edited

Loading

AureliaDolo Nov 4, 2024

vxgmichel Nov 4, 2024

FirelightFlagboy Nov 4, 2024

vxgmichel Nov 4, 2024

FirelightFlagboy commented Nov 4, 2024 •

edited

Loading

vxgmichel commented Nov 4, 2024 •

edited

Loading

FirelightFlagboy commented Nov 4, 2024

vxgmichel commented Nov 4, 2024 •

edited

Loading

FirelightFlagboy Nov 4, 2024

vxgmichel Nov 8, 2024

FirelightFlagboy Nov 4, 2024

vxgmichel Nov 8, 2024

Add support for shamir recovery invitation #8819

Are you sure you want to change the base?

Add support for shamir recovery invitation #8819

Conversation

vxgmichel commented Oct 30, 2024 • edited Loading

vxgmichel commented Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

vxgmichel Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vxgmichel Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FirelightFlagboy commented Nov 4, 2024 • edited Loading

vxgmichel commented Nov 4, 2024 • edited Loading

FirelightFlagboy commented Nov 4, 2024

vxgmichel commented Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vxgmichel commented Oct 30, 2024 •

edited

Loading

vxgmichel commented Nov 1, 2024 •

edited

Loading

vxgmichel Nov 4, 2024 •

edited

Loading

vxgmichel Nov 4, 2024 •

edited

Loading

FirelightFlagboy commented Nov 4, 2024 •

edited

Loading

vxgmichel commented Nov 4, 2024 •

edited

Loading

vxgmichel commented Nov 4, 2024 •

edited

Loading