Address @touilleMan's comments

libparsec/crates/client/src/certif/block_validate.rs

@@ -147,6 +147,8 @@ pub(super) async fn validate_block(
                 CertifValidateBlockError::InvalidCertificate(err)
             }
             CertifForReadWithRequirementsError::InvalidRequirements => {
+                // This shouldn't occur since `needed_realm_certificate_timestamp` is provided by the server along with the block to validate
+                // (and the server is expected to only provide us with valid requirements !).
                 CertifValidateBlockError::Internal(anyhow::anyhow!(
                     "Unexpected invalid requirements"
                 ))

libparsec/crates/client/src/certif/manifest_validate.rs

@@ -161,6 +161,8 @@ pub(super) async fn validate_user_manifest(
                 CertifValidateManifestError::InvalidCertificate(err)
             }
             CertifForReadWithRequirementsError::InvalidRequirements => {
+                // This shouldn't occur since `needed_realm_certificate_timestamp` is provided by the server along with the vlob to validate
+                // (and the server is expected to only provide us with valid requirements !).
                 CertifValidateManifestError::Internal(anyhow::anyhow!(
                     "Unexpected invalid requirements"
                 ))
@@ -275,6 +277,8 @@ pub(super) async fn validate_workspace_manifest(
                 CertifValidateManifestError::InvalidCertificate(err)
             }
             CertifForReadWithRequirementsError::InvalidRequirements => {
+                // This shouldn't occur since `needed_realm_certificate_timestamp` is provided by the server along with the vlob to validate
+                // (and the server is expected to only provide us with valid requirements !).
                 CertifValidateManifestError::Internal(anyhow::anyhow!(
                     "Unexpected invalid requirements"
                 ))
@@ -388,6 +392,8 @@ pub(super) async fn validate_child_manifest(
                 CertifValidateManifestError::InvalidCertificate(err)
             }
             CertifForReadWithRequirementsError::InvalidRequirements => {
+                // This shouldn't occur since `needed_realm_certificate_timestamp` is provided by the server along with the vlob to validate
+                // (and the server is expected to only provide us with valid requirements !).
                 CertifValidateManifestError::Internal(anyhow::anyhow!(
                     "Unexpected invalid requirements"
                 ))

libparsec/crates/client/src/certif/shamir_recovery_list.rs

@@ -416,6 +416,9 @@ pub async fn get_shamir_recovery_share_data(
                 .await?;
             let brief_certificate = match brief_certificate {
                 Some(brief_certificate) if brief_certificate.timestamp == shamir_recovery_created_on => Ok(brief_certificate),
+                // Only the last shamir recovery setup is meant to be active.
+                // If the provided timestamp doesn't match the last setup, then it either corresponds to a deleted setup or a non-existing one.
+                // But since we already checked for deletions, we can safely assume it's the latter.
                 _ => {
                     Err(CertifGetShamirRecoveryShareDataError::ShamirRecoveryBriefCertificateNotFound)
                 }
@@ -430,6 +433,9 @@ pub async fn get_shamir_recovery_share_data(
                 .await?;
             let share_certificate = match share_certificate {
                 Some(share_certificate) if share_certificate.timestamp == shamir_recovery_created_on => Ok(share_certificate),
+                // A similar check has been performed for the brief certificate, but we do it once more for the share certificate.
+                // This is justified in the case where the corresponding shamir recovery setup is for the current device.
+                // In this case, we will find a brief certificate but no share certificate.
                 _ => {
                     Err(CertifGetShamirRecoveryShareDataError::ShamirRecoveryShareCertificateNotFound)
                 }

libparsec/crates/client/src/client/start_invitation_greet.rs

@@ -91,7 +91,11 @@ impl From<CertifGetShamirRecoveryShareDataError> for ClientStartShamirRecoveryIn
                 ClientStartShamirRecoveryInvitationGreetError::InvalidCertificate(e)
             }
             CertifGetShamirRecoveryShareDataError::InvalidRequirements => {
-                ClientStartShamirRecoveryInvitationGreetError::ShamirRecoveryNotFound
+                // This shouldn't occur since the requirements timestamp is provided by the server along with the invitation list
+                // (and the server is expected to only provide us with valid requirements !).
+                ClientStartShamirRecoveryInvitationGreetError::Internal(anyhow::anyhow!(
+                    "Unexpected invalid requirements"
+                ))
             }
             CertifGetShamirRecoveryShareDataError::Internal(e) => {
                 ClientStartShamirRecoveryInvitationGreetError::Internal(e)

libparsec/crates/client/tests/unit/client/start_shamir_recovery_invitation_greet.rs

@@ -12,6 +12,7 @@ fn get_alice_token(env: &TestbedEnv) -> InvitationToken {
     env.template
         .events
         .iter()
+        .rev()
         .find_map(|e| match e {
             TestbedEvent::NewShamirRecoveryInvitation(event) if event.claimer == alice.user_id => {
                 Some(event.token)

libparsec/crates/protocol/tests/authenticated_cmds/v4/invite_list.rs

@@ -114,7 +114,7 @@ pub fn rep_ok() {
     //       type: 'SHAMIR_RECOVERY',
     //       claimer_user_id: ext(2, 0xa11cec00100000000000000000000000),
     //       created_on: ext(1, 946774800000000) i.e. 2000-01-02T02:00:00Z,
-    //       created_on: ext(1, 946688400000000) i.e. 2000-01-02T01:00:00Z,
+    //       shamir_recovery_created_on: ext(1, 946688400000000) i.e. 2000-01-02T01:00:00Z,
     //       status: 'IDLE',
     //       token: 0xd864b93ded264aae9ae583fd3d40c45a,
     //     },

server/parsec/components/memory/invite.py

@@ -52,7 +52,7 @@
 from parsec.events import EventInvitation
 
 
-def is_invitation_cancelled(org: MemoryOrganization, invitation: MemoryInvitation) -> bool:
+def _is_invitation_cancelled(org: MemoryOrganization, invitation: MemoryInvitation) -> bool:
     if invitation.is_cancelled:
         return True
     if invitation.type == InvitationType.SHAMIR_RECOVERY:
@@ -718,7 +718,7 @@ async def greeter_start_greeting_attempt(
             return InviteGreeterStartGreetingAttemptBadOutcome.INVITATION_NOT_FOUND
         if invitation.is_completed:
             return InviteGreeterStartGreetingAttemptBadOutcome.INVITATION_COMPLETED
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteGreeterStartGreetingAttemptBadOutcome.INVITATION_CANCELLED
 
         if not self.is_greeter_allowed(org, invitation, greeter_user):
@@ -757,7 +757,7 @@ async def claimer_start_greeting_attempt(
             return InviteClaimerStartGreetingAttemptBadOutcome.INVITATION_NOT_FOUND
         if invitation.is_completed:
             return InviteClaimerStartGreetingAttemptBadOutcome.INVITATION_COMPLETED
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteClaimerStartGreetingAttemptBadOutcome.INVITATION_CANCELLED
 
         if not self.is_greeter_allowed(org, invitation, greeter_user):
@@ -804,7 +804,7 @@ async def greeter_cancel_greeting_attempt(
 
         if invitation.is_completed:
             return InviteGreeterCancelGreetingAttemptBadOutcome.INVITATION_COMPLETED
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteGreeterCancelGreetingAttemptBadOutcome.INVITATION_CANCELLED
 
         if not self.is_greeter_allowed(org, invitation, greeter_user):
@@ -847,7 +847,7 @@ async def claimer_cancel_greeting_attempt(
 
         if invitation.is_completed:
             return InviteClaimerCancelGreetingAttemptBadOutcome.INVITATION_COMPLETED
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteClaimerCancelGreetingAttemptBadOutcome.INVITATION_CANCELLED
 
         if not self.is_greeter_allowed(org, invitation, greeter_user):
@@ -898,7 +898,7 @@ async def greeter_step(
 
         if invitation.is_completed:
             return InviteGreeterStepBadOutcome.INVITATION_COMPLETED
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteGreeterStepBadOutcome.INVITATION_CANCELLED
 
         if not self.is_greeter_allowed(org, invitation, greeter_user):
@@ -950,7 +950,7 @@ async def claimer_step(
 
         if invitation.is_completed:
             return InviteClaimerStepBadOutcome.INVITATION_COMPLETED
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteClaimerStepBadOutcome.INVITATION_CANCELLED
 
         if not self.is_greeter_allowed(org, invitation, greeter_user):
@@ -1003,7 +1003,7 @@ async def complete(
             invitation = org.invitations[token]
         except KeyError:
             return InviteCompleteBadOutcome.INVITATION_NOT_FOUND
-        if is_invitation_cancelled(org, invitation):
+        if _is_invitation_cancelled(org, invitation):
             return InviteCompleteBadOutcome.INVITATION_CANCELLED
         if invitation.is_completed:
             return InviteCompleteBadOutcome.INVITATION_ALREADY_COMPLETED

server/parsec/components/memory/shamir.py

@@ -87,7 +87,7 @@ async def setup(
 
         async with org.topics_lock(read=["common"], write=["shamir_recovery"]) as (
             common_topic_last_timestamp,
-            last_shamir_certificate_timestamp,
+            shamir_topic_last_timestamp,
         ):
             # Ensure all recipients exist and are not revoked
             for share_recipient in cooked_shares.keys():
@@ -109,14 +109,14 @@ async def setup(
                 # The user had already setup a shamir recovery... but it might be deleted
                 if not previous_shamir_setup.is_deleted:
                     return ShamirSetupAlreadyExistsBadOutcome(
-                        last_shamir_recovery_certificate_timestamp=last_shamir_certificate_timestamp
+                        last_shamir_recovery_certificate_timestamp=shamir_topic_last_timestamp
                     )
 
             # Ensure we are not breaking causality by adding a newer timestamp.
 
             last_certificate = max(
                 common_topic_last_timestamp,
-                last_shamir_certificate_timestamp,
+                shamir_topic_last_timestamp,
             )
             if last_certificate >= cooked_brief.timestamp:
                 return RequireGreaterTimestamp(strictly_greater_than=last_certificate)

server/parsec/components/postgresql/invite.py

@@ -79,7 +79,7 @@ class InvitationInfo:
     created_by_email: str
     created_by_label: str
     claimer_email: str | None
-    shamir_recovery_setup: int | None
+    shamir_recovery_setup_internal_id: int | None
     created_on: DateTime
     deleted_on: DateTime | None
     deleted_reason: InvitationStatus | None
@@ -129,7 +129,7 @@ def from_record(cls, record: Record) -> InvitationInfo:
             created_by_email=created_by_email,
             created_by_label=created_by_label,
             claimer_email=claimer_email,
-            shamir_recovery_setup=shamir_recovery_setup,
+            shamir_recovery_setup_internal_id=shamir_recovery_setup,
             created_on=created_on,
             deleted_on=deleted_on,
             deleted_reason=deleted_reason,
@@ -1267,9 +1267,9 @@ async def list(
                         status=status,
                     )
                 case InvitationType.SHAMIR_RECOVERY:
-                    assert invitation_info.shamir_recovery_setup is not None
+                    assert invitation_info.shamir_recovery_setup_internal_id is not None
                     shamir_recovery_info = await self._get_shamir_recovery_info(
-                        conn, invitation_info.shamir_recovery_setup
+                        conn, invitation_info.shamir_recovery_setup_internal_id
                     )
 
                     invitation = ShamirRecoveryInvitation(
@@ -1335,9 +1335,9 @@ async def _info_as_invited(
                 status=InvitationStatus.READY,
             )
         elif invitation_info.type == InvitationType.SHAMIR_RECOVERY:
-            assert invitation_info.shamir_recovery_setup is not None
+            assert invitation_info.shamir_recovery_setup_internal_id is not None
             shamir_recovery_info = await self._get_shamir_recovery_info(
-                conn, invitation_info.shamir_recovery_setup
+                conn, invitation_info.shamir_recovery_setup_internal_id
             )
             return ShamirRecoveryInvitation(
                 created_by_user_id=invitation_info.created_by_user_id,
@@ -1394,7 +1394,7 @@ async def shamir_recovery_reveal(
         row = await conn.fetchrow(
             *_q_retrieve_shamir_recovery_ciphered_data(
                 organization_id=organization_id.str,
-                shamir_recovery_setup_internal_id=invitation_info.shamir_recovery_setup,
+                shamir_recovery_setup_internal_id=invitation_info.shamir_recovery_setup_internal_id,
             )
         )
         assert row is not None
@@ -1473,9 +1473,9 @@ async def test_dump_all_invitations(
                         )
                     )
                 case InvitationType.SHAMIR_RECOVERY:
-                    assert invitation_info.shamir_recovery_setup is not None
+                    assert invitation_info.shamir_recovery_setup_internal_id is not None
                     shamir_recovery_info = await self._get_shamir_recovery_info(
-                        conn, invitation_info.shamir_recovery_setup
+                        conn, invitation_info.shamir_recovery_setup_internal_id
                     )
                     current_user_invitations.append(
                         ShamirRecoveryInvitation(
@@ -1619,7 +1619,7 @@ async def is_greeter_allowed(
         elif invitation_info.type == InvitationType.SHAMIR_RECOVERY:
             row = await conn.fetchrow(
                 *_q_is_greeter_in_recipients(
-                    shamir_recovery_setup_internal_id=invitation_info.shamir_recovery_setup,
+                    shamir_recovery_setup_internal_id=invitation_info.shamir_recovery_setup_internal_id,
                     greeter_id=greeter_id,
                 )
             )

server/parsec/components/postgresql/shamir_delete.py

@@ -226,7 +226,7 @@ async def shamir_delete(
 
     # 5) Mark the shamir recovery setup as deleted
 
-    row = await conn.fetchrow(
+    success = await conn.fetchval(
         *_q_mark_shamir_recovery_setup_as_deleted(
             organization_internal_id=organization_internal_id,
             shamir_recovery_setup_internal_id=shamir_recovery_setup_internal_id,
@@ -236,6 +236,6 @@ async def shamir_delete(
     )
 
     # Check that the update is successful
-    assert row is not None
+    assert success is True, success
 
     return cooked_deletion

server/parsec/components/postgresql/shamir_setup.py

@@ -294,6 +294,7 @@ async def shamir_setup(
         )
 
     # 2) Validate the shamir certificates
+
     match shamir_setup_validate(
         now,
         author,
@@ -308,8 +309,9 @@ async def shamir_setup(
             return error
 
     # 3) Check the recipients
-    recipient_internal_ids = []
-    for recipient_id in cooked_shares:
+
+    share_args = []
+    for recipient_id, (share_certificate, _) in cooked_shares.items():
         row = await conn.fetchrow(
             *_q_check_recipient(
                 organization_internal_id=organization_internal_id,
@@ -331,7 +333,8 @@ async def shamir_setup(
 
         match row["recipient_internal_id"]:
             case int() as recipient_internal_id:
-                recipient_internal_ids.append(recipient_internal_id)
+                number_of_shares = cooked_brief.per_recipient_shares[recipient_id]
+                share_args.append((recipient_internal_id, share_certificate, number_of_shares))
             case unknown:
                 assert False, repr(unknown)
 
@@ -372,15 +375,13 @@ async def shamir_setup(
             assert False, repr(unknown)
 
     def arg_gen():
-        for recipient_internal_id, (recipient_id, (share_certificate, _)) in zip(
-            recipient_internal_ids, cooked_shares.items()
-        ):
+        for recipient_internal_id, share_certificate, number_of_shares in share_args:
             yield _q_insert_shamir_recovery_share.arg_only(
                 organization_internal_id=organization_internal_id,
                 shamir_recovery_setup_internal_id=shamir_recovery_setup_internal_id,
                 recipient_internal_id=recipient_internal_id,
                 share_certificate=share_certificate,
-                shares=cooked_brief.per_recipient_shares[recipient_id],
+                shares=number_of_shares,
             )
 
     await conn.executemany(