From 4833d804e0a121141fb236e17530a2ac2cb60f14 Mon Sep 17 00:00:00 2001 From: Vincent Michel Date: Mon, 27 Jan 2025 18:40:22 +0100 Subject: [PATCH] Address @touilleMan's comments --- .../authenticated_cmds/v5/invite_list.rs | 1 - libparsec/src/invite.rs | 3 +- server/parsec/components/invite.py | 8 +-- server/parsec/components/postgresql/invite.py | 60 +++++++++---------- .../tests/api_v5/invited/test_invite_info.py | 11 +++- server/tests/migrations/0009_after.sql | 19 +++++- server/tests/test_sse.py | 2 + 7 files changed, 59 insertions(+), 45 deletions(-) diff --git a/libparsec/crates/protocol/tests/authenticated_cmds/v5/invite_list.rs b/libparsec/crates/protocol/tests/authenticated_cmds/v5/invite_list.rs index f796ea4516b..5f99ceeb7d8 100644 --- a/libparsec/crates/protocol/tests/authenticated_cmds/v5/invite_list.rs +++ b/libparsec/crates/protocol/tests/authenticated_cmds/v5/invite_list.rs @@ -8,7 +8,6 @@ use super::authenticated_cmds; use libparsec_types::prelude::*; use libparsec_tests_lite::{hex, p_assert_eq}; -use libparsec_types::{InvitationStatus, InvitationToken}; // Request diff --git a/libparsec/src/invite.rs b/libparsec/src/invite.rs index ba76e53f1b4..e5f01bf56cd 100644 --- a/libparsec/src/invite.rs +++ b/libparsec/src/invite.rs @@ -12,9 +12,8 @@ pub use libparsec_client::{ ShamirRecoveryClaimRecoverDeviceError, }; pub use libparsec_protocol::authenticated_cmds::latest::invite_list::InvitationCreatedBy as InviteListInvitationCreatedBy; -pub use libparsec_protocol::invited_cmds::latest::invite_info::InvitationCreatedBy as InviteInfoInvitationCreatedBy; pub use libparsec_protocol::invited_cmds::latest::invite_info::{ - ShamirRecoveryRecipient, UserOnlineStatus, + InvitationCreatedBy as InviteInfoInvitationCreatedBy, ShamirRecoveryRecipient, UserOnlineStatus, }; pub use libparsec_types::prelude::*; diff --git a/server/parsec/components/invite.py b/server/parsec/components/invite.py index 1d165c4a709..412e532b3be 100644 --- a/server/parsec/components/invite.py +++ b/server/parsec/components/invite.py @@ -57,7 +57,7 @@ @dataclass(slots=True) -class _InvitationCreatedBy: +class BaseInvitationCreatedBy: def for_invite_info(self) -> InviteInfoInvitationCreatedBy: match self: case InvitationCreatedByUser(user_id, human_handle): @@ -86,17 +86,17 @@ def for_invite_list(self) -> InviteListInvitationCreatedBy: @dataclass(slots=True) -class InvitationCreatedByUser(_InvitationCreatedBy): +class InvitationCreatedByUser(BaseInvitationCreatedBy): user_id: UserID human_handle: HumanHandle @dataclass(slots=True) -class InvitationCreatedByExternalService(_InvitationCreatedBy): +class InvitationCreatedByExternalService(BaseInvitationCreatedBy): service_label: str -InvitationCreatedBy: TypeAlias = InvitationCreatedByUser | InvitationCreatedByExternalService +type InvitationCreatedBy = InvitationCreatedByUser | InvitationCreatedByExternalService @dataclass(slots=True) diff --git a/server/parsec/components/postgresql/invite.py b/server/parsec/components/postgresql/invite.py index 81c10420c51..58849720558 100644 --- a/server/parsec/components/postgresql/invite.py +++ b/server/parsec/components/postgresql/invite.py @@ -110,7 +110,7 @@ class ShamirRecoveryInvitationInfo(BaseInvitationInfo): shamir_recovery_deleted_on: DateTime | None -InvitationInfo: TypeAlias = UserInvitationInfo | DeviceInvitationInfo | ShamirRecoveryInvitationInfo +type InvitationInfo = UserInvitationInfo | DeviceInvitationInfo | ShamirRecoveryInvitationInfo def invitation_info_from_record(record: Record) -> InvitationInfo: @@ -132,16 +132,10 @@ def invitation_info_from_record(record: Record) -> InvitationInfo: case unknown: assert False, repr(unknown) - match record["created_by_user_id"]: - case None: - match record["created_by_service_label"]: - case str() as created_by_service_label: - created_by = InvitationCreatedByExternalService( - service_label=created_by_service_label - ) - case unknown: - assert False, repr(unknown) - case str() as created_by_user_id_str: + match (record["created_by_user_id"], record["created_by_service_label"]): + case (None, str() as created_by_service_label): + created_by = InvitationCreatedByExternalService(service_label=created_by_service_label) + case (str() as created_by_user_id_str, None): match (record["created_by_email"], record["created_by_label"]): case (str() as created_by_email, str() as created_by_label): created_by = InvitationCreatedByUser( @@ -431,7 +425,7 @@ def from_record(cls, record: Record) -> GreetingAttemptInfo: WHERE organization.organization_id = $organization_id AND type = 'USER' - AND user_.user_id = $user_id + AND user_.user_id = $invitation_creator_user_id AND user_invitation_claimer_email = $user_invitation_claimer_email AND deleted_on IS NULL LIMIT 1 @@ -529,9 +523,9 @@ def from_record(cls, record: Record) -> GreetingAttemptInfo: DISTINCT invitation._id AS invitation_internal_id, invitation.token, invitation.type, - user_.user_id AS created_by_user_id, - human.email AS created_by_email, - human.label AS created_by_label, + created_by_user.user_id AS created_by_user_id, + created_by_human.email AS created_by_email, + created_by_human.label AS created_by_label, invitation.created_by_service_label, invitation.user_invitation_claimer_email, device_invitation_claimer.user_id AS device_invitation_claimer_user_id, @@ -548,9 +542,9 @@ def from_record(cls, record: Record) -> GreetingAttemptInfo: invitation.deleted_on, invitation.deleted_reason FROM invitation -LEFT JOIN device ON invitation.created_by_device = device._id -LEFT JOIN user_ ON device.user_ = user_._id -LEFT JOIN human ON human._id = user_.human +LEFT JOIN device AS created_by_device ON invitation.created_by_device = created_by_device._id +LEFT JOIN user_ AS created_by_user ON created_by_device.user_ = created_by_user._id +LEFT JOIN human AS created_by_human ON created_by_human._id = created_by_user.human LEFT JOIN user_ AS device_invitation_claimer ON invitation.device_invitation_claimer = device_invitation_claimer._id LEFT JOIN human AS device_invitation_claimer_human ON device_invitation_claimer.human = device_invitation_claimer_human._id LEFT JOIN shamir_recovery_setup ON invitation.shamir_recovery = shamir_recovery_setup._id @@ -562,8 +556,8 @@ def from_record(cls, record: Record) -> GreetingAttemptInfo: invitation.organization = { q_organization_internal_id("$organization_id") } -- Different invitation types have different filtering rules AND ( - (invitation.type = 'USER' AND user_.user_id = $user_id) - OR (invitation.type = 'DEVICE' AND user_.user_id = $user_id) + (invitation.type = 'USER' AND created_by_user.user_id = $user_id) + OR (invitation.type = 'DEVICE' AND device_invitation_claimer.user_id = $user_id) OR (invitation.type = 'SHAMIR_RECOVERY' AND recipient_user_.user_id = $user_id) ) ORDER BY created_on @@ -576,9 +570,9 @@ def from_record(cls, record: Record) -> GreetingAttemptInfo: invitation._id AS invitation_internal_id, invitation.token, invitation.type, - user_.user_id AS created_by_user_id, - human.email AS created_by_email, - human.label AS created_by_label, + created_by_user.user_id AS created_by_user_id, + created_by_human.email AS created_by_email, + created_by_human.label AS created_by_label, invitation.created_by_service_label, invitation.user_invitation_claimer_email, device_invitation_claimer.user_id AS device_invitation_claimer_user_id, @@ -595,9 +589,9 @@ def from_record(cls, record: Record) -> GreetingAttemptInfo: invitation.deleted_on, invitation.deleted_reason FROM invitation -LEFT JOIN device ON invitation.created_by_device = device._id -LEFT JOIN user_ ON device.user_ = user_._id -LEFT JOIN human ON human._id = user_.human +LEFT JOIN device AS created_by_device ON invitation.created_by_device = created_by_device._id +LEFT JOIN user_ AS created_by_user ON created_by_device.user_ = created_by_user._id +LEFT JOIN human AS created_by_human ON created_by_human._id = created_by_user.human LEFT JOIN user_ AS device_invitation_claimer ON invitation.device_invitation_claimer = device_invitation_claimer._id LEFT JOIN human AS device_invitation_claimer_human ON device_invitation_claimer.human = device_invitation_claimer_human._id LEFT JOIN shamir_recovery_setup ON invitation.shamir_recovery = shamir_recovery_setup._id @@ -642,9 +636,9 @@ def make_q_info_invitation( invitation._id AS invitation_internal_id, invitation.token, invitation.type, - user_.user_id AS created_by_user_id, - human.email AS created_by_email, - human.label AS created_by_label, + created_by_user.user_id AS created_by_user_id, + created_by_human.email AS created_by_email, + created_by_human.label AS created_by_label, invitation.created_by_service_label, invitation.user_invitation_claimer_email, device_invitation_claimer.user_id AS device_invitation_claimer_user_id, @@ -662,9 +656,9 @@ def make_q_info_invitation( invitation.deleted_reason FROM invitation INNER JOIN selected_invitation ON invitation._id = selected_invitation.invitation_internal_id - LEFT JOIN device ON invitation.created_by_device = device._id - LEFT JOIN user_ ON device.user_ = user_._id - LEFT JOIN human ON human._id = user_.human + LEFT JOIN device AS created_by_device ON invitation.created_by_device = created_by_device._id + LEFT JOIN user_ AS created_by_user ON created_by_device.user_ = created_by_user._id + LEFT JOIN human AS created_by_human ON created_by_human._id = created_by_user.human LEFT JOIN user_ AS device_invitation_claimer ON invitation.device_invitation_claimer = device_invitation_claimer._id LEFT JOIN human AS device_invitation_claimer_human ON device_invitation_claimer.human = device_invitation_claimer_human._id LEFT JOIN shamir_recovery_setup ON invitation.shamir_recovery = shamir_recovery_setup._id @@ -997,7 +991,7 @@ async def _do_new_invitation( # TODO: Update this when implementing https://github.com/Scille/parsec-cloud/issues/9413 q = _q_retrieve_compatible_user_invitation( organization_id=organization_id.str, - user_id=author_user_id, + invitation_creator_user_id=author_user_id, user_invitation_claimer_email=user_invitation_claimer_email, ) case InvitationType.DEVICE: diff --git a/server/tests/api_v5/invited/test_invite_info.py b/server/tests/api_v5/invited/test_invite_info.py index b7da6f5cc7a..fc3e6533208 100644 --- a/server/tests/api_v5/invited/test_invite_info.py +++ b/server/tests/api_v5/invited/test_invite_info.py @@ -8,8 +8,13 @@ UserGreetingAdministrator, UserOnlineStatus, ) -from tests.common import Backend, CoolorgRpcClients, HttpCommonErrorsTester, ShamirOrgRpcClients -from tests.common.data import bob_becomes_admin +from tests.common import ( + Backend, + CoolorgRpcClients, + HttpCommonErrorsTester, + ShamirOrgRpcClients, + bob_becomes_admin, +) @pytest.mark.parametrize("user_or_device", ("user", "device")) @@ -177,7 +182,7 @@ async def test_invited_invite_info_for_user_with_multiple_admins( ) ) - # Alice starts greeting attempt for zack + # Alice re-starts greeting attempt for zack t3 = DateTime.now() rep = await backend.invite.greeter_start_greeting_attempt( t3, diff --git a/server/tests/migrations/0009_after.sql b/server/tests/migrations/0009_after.sql index df585cf3a6c..5abffb94f83 100644 --- a/server/tests/migrations/0009_after.sql +++ b/server/tests/migrations/0009_after.sql @@ -3,8 +3,8 @@ DO $$ DECLARE organization_internal_id integer; - user_current_profile text; device_invitation_claimer_human_email text; + user_invitation_claimer_email text; BEGIN SELECT _id INTO organization_internal_id @@ -21,7 +21,22 @@ BEGIN ON user_.human = human._id WHERE invitation.organization = organization_internal_id - AND invitation.token = 'e0000000000000000000000000000002'; + AND invitation.token = 'e0000000000000000000000000000002' + AND invitation.user_invitation_claimer_email IS NULL; ASSERT device_invitation_claimer_human_email = 'alice@example.com', FORMAT('Bad invitation migration: `%s`', device_invitation_claimer_human_email); + + -- Ensure that the `device_invitation_claimer` column is NULL for the user invitations + SELECT invitation.user_invitation_claimer_email + INTO user_invitation_claimer_email + FROM invitation + WHERE + invitation.organization = organization_internal_id + AND invitation.token = 'e0000000000000000000000000000001' + AND invitation.device_invitation_claimer IS NULL; + + ASSERT user_invitation_claimer_email = 'zack@example.invalid', FORMAT('Bad invitation migration: `%s`', user_invitation_claimer_email); + + + END$$; diff --git a/server/tests/test_sse.py b/server/tests/test_sse.py index 0a2dd84cf3a..650b84eb517 100644 --- a/server/tests/test_sse.py +++ b/server/tests/test_sse.py @@ -208,12 +208,14 @@ def get_local_port(host: str) -> int: "app": app, "host": HOST, "port": PORT, + "proxy_trusted_addresses": None, }, ) proc.start() yield (HOST, PORT) proc.terminate() + proc.join() @pytest.mark.timeout(2)