From 0bc35fd1eb2a2554e46d049b9b140d2c11017a9f Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:58:56 +0000 Subject: [PATCH] directory: identify registration owner by member id to fix incorrect de-listing (#7362) * directory: identify registration owner by member id to fix incorrect de-listing The leave/removed/role-changed handlers identified the registration owner by contact id. A non-owner member can be associated with the owner's contact (via the contact/member merge), so its departure incorrectly de-listed the group. Compare by group member id (owner_member_id) instead, falling back to contact id for registrations recorded before owner_member_id existed. * fix test, always compare contact ID in owner check --------- Co-authored-by: Evgeny Poberezkin --- .../src/Directory/Events.hs | 12 ++--- .../src/Directory/Service.hs | 33 +++++++----- tests/Bots/DirectoryTests.hs | 53 ++++++++++++++++++- 3 files changed, 79 insertions(+), 19 deletions(-) diff --git a/apps/simplex-directory-service/src/Directory/Events.hs b/apps/simplex-directory-service/src/Directory/Events.hs index 3bff611a28..bc84ab86e1 100644 --- a/apps/simplex-directory-service/src/Directory/Events.hs +++ b/apps/simplex-directory-service/src/Directory/Events.hs @@ -54,10 +54,10 @@ data DirectoryEvent | DEPendingMember GroupInfo GroupMember | DEPendingMemberMsg GroupInfo GroupMember ChatItemId Text | DEGroupItemProhibited GroupInfo GroupMember ChatItemId GroupFeature -- a member posted content prohibited by the group's settings - | DEContactRoleChanged GroupInfo ContactId GroupMemberRole -- contactId here is the contact whose role changed + | DEContactRoleChanged GroupInfo ContactId GroupMemberId GroupMemberRole -- contactId/memberId here identify the member whose role changed | DEServiceRoleChanged GroupInfo GroupMemberRole - | DEContactRemovedFromGroup ContactId GroupInfo - | DEContactLeftGroup ContactId GroupInfo + | DEContactRemovedFromGroup ContactId GroupMemberId GroupInfo + | DEContactLeftGroup ContactId GroupMemberId GroupInfo | DEServiceRemovedFromGroup GroupInfo | DEGroupDeleted GroupInfo | DEChatLinkReceived {contact :: Contact, chatItemId :: ChatItemId, chatLink :: MsgChatLink, ownerSig :: Maybe LinkOwnerSig} @@ -93,9 +93,9 @@ crDirectoryEvent_ = \case _ -> Nothing CEvtMemberRole {groupInfo, member, toRole} | groupMemberId' member == groupMemberId' (membership groupInfo) -> Just $ DEServiceRoleChanged groupInfo toRole - | otherwise -> (\ctId -> DEContactRoleChanged groupInfo ctId toRole) <$> memberContactId member - CEvtDeletedMember {groupInfo, deletedMember} -> (`DEContactRemovedFromGroup` groupInfo) <$> memberContactId deletedMember - CEvtLeftMember {groupInfo, member} -> (`DEContactLeftGroup` groupInfo) <$> memberContactId member + | otherwise -> (\ctId -> DEContactRoleChanged groupInfo ctId (groupMemberId' member) toRole) <$> memberContactId member + CEvtDeletedMember {groupInfo, deletedMember} -> (\ctId -> DEContactRemovedFromGroup ctId (groupMemberId' deletedMember) groupInfo) <$> memberContactId deletedMember + CEvtLeftMember {groupInfo, member} -> (\ctId -> DEContactLeftGroup ctId (groupMemberId' member) groupInfo) <$> memberContactId member CEvtDeletedMemberUser {groupInfo} -> Just $ DEServiceRemovedFromGroup groupInfo CEvtGroupDeleted {groupInfo} -> Just $ DEGroupDeleted groupInfo CEvtUnknownMemberAnnounced {groupInfo, unknownMember, announcedMember} -> Just $ DEMemberUpdated {groupInfo, fromMember = unknownMember, toMember = announcedMember} diff --git a/apps/simplex-directory-service/src/Directory/Service.hs b/apps/simplex-directory-service/src/Directory/Service.hs index 81cb35954b..fe57489f4d 100644 --- a/apps/simplex-directory-service/src/Directory/Service.hs +++ b/apps/simplex-directory-service/src/Directory/Service.hs @@ -323,10 +323,10 @@ directoryServiceEvent opts@DirectoryOpts {adminUsers, superUsers, serviceName, o DEPendingMember g m -> dePendingMember g m DEPendingMemberMsg g m ciId t -> dePendingMemberMsg g m ciId t DEGroupItemProhibited g m ciId gf -> when prohibitedToObserver $ deGroupItemProhibited g m ciId gf - DEContactRoleChanged g ctId role -> deContactRoleChanged g ctId role + DEContactRoleChanged g ctId gmId role -> deContactRoleChanged g ctId gmId role DEServiceRoleChanged g role -> deServiceRoleChanged g role - DEContactRemovedFromGroup ctId g -> deContactRemovedFromGroup ctId g - DEContactLeftGroup ctId g -> deContactLeftGroup ctId g + DEContactRemovedFromGroup ctId gmId g -> deContactRemovedFromGroup ctId gmId g + DEContactLeftGroup ctId gmId g -> deContactLeftGroup ctId gmId g DEServiceRemovedFromGroup g -> deServiceRemovedFromGroup g DEGroupDeleted g -> deGroupDeleted g DEChatLinkReceived {contact = ct, chatLink, ownerSig} -> deChatLinkReceived ct chatLink ownerSig @@ -350,6 +350,15 @@ directoryServiceEvent opts@DirectoryOpts {adminUsers, superUsers, serviceName, o notifyAdminUsers s = withAdminUsers $ \contactId -> sendMessage' cc contactId s notifyOwner = sendMessage' cc . dbContactId ctId `isOwner` GroupReg {dbContactId} = ctId == dbContactId + -- Whether the leaving/removed/role-changed member is the registration owner. + -- Comparing by member id (not contact id) is required because a non-owner + -- member can be associated with the owner's contact by the probe-and-merge + -- mechanism, which would otherwise make its departure de-list the group. + -- Registrations recorded before owner_member_id existed keep the contact-id + -- comparison. + isOwnerMember :: GroupReg -> GroupMemberId -> ContactId -> Bool + isOwnerMember GroupReg {dbContactId, dbOwnerMemberId} gmId ctId = + ctId == dbContactId && maybe True (gmId ==) dbOwnerMemberId withGroupReg :: GroupInfo -> Text -> (GroupReg -> IO ()) -> IO () withGroupReg GroupInfo {groupId, localDisplayName} err action = getGroupReg cc groupId >>= \case @@ -806,13 +815,13 @@ directoryServiceEvent opts@DirectoryOpts {adminUsers, superUsers, serviceName, o sendToApprove g' gr (n + 1) _ -> pure () - deContactRoleChanged :: GroupInfo -> ContactId -> GroupMemberRole -> IO () - deContactRoleChanged g@GroupInfo {groupId, membership = GroupMember {memberRole = serviceRole}} ctId contactRole = do + deContactRoleChanged :: GroupInfo -> ContactId -> GroupMemberId -> GroupMemberRole -> IO () + deContactRoleChanged g@GroupInfo {groupId, membership = GroupMember {memberRole = serviceRole}} ctId gmId contactRole = do logInfo $ "contact ID " <> tshow ctId <> " role changed in group " <> viewGroupName g <> " to " <> tshow contactRole withGroupReg g "contact role changed" $ \gr@GroupReg {groupRegStatus} -> do let userGroupRef = userGroupReference gr g uCtRole = "Your role in the group " <> userGroupRef <> " is changed to " <> ctRole - when (ctId `isOwner` gr) $ + when (isOwnerMember gr gmId ctId) $ case groupRegStatus of GRSSuspendedBadRoles | rStatus == GRSOk -> setGroupStatus notifyAdminUsers env cc groupId GRSActive $ \gr' -> do @@ -861,23 +870,23 @@ directoryServiceEvent opts@DirectoryOpts {adminUsers, superUsers, serviceName, o getOwnerGroupMember groupId gr >>= mapM_ (\cm@GroupMember {memberRole} -> when (memberRole == GROwner && memberActive cm) action) - deContactRemovedFromGroup :: ContactId -> GroupInfo -> IO () - deContactRemovedFromGroup ctId g@GroupInfo {groupId, groupProfile = GroupProfile {publicGroup = pg_}} = do + deContactRemovedFromGroup :: ContactId -> GroupMemberId -> GroupInfo -> IO () + deContactRemovedFromGroup ctId gmId g@GroupInfo {groupId, groupProfile = GroupProfile {publicGroup = pg_}} = do let gt = maybe "group" groupTypeStr' pg_ logInfo $ "contact ID " <> tshow ctId <> " removed from group " <> viewGroupName g withGroupReg g "contact removed" $ \gr -> - when (ctId `isOwner` gr) $ + when (isOwnerMember gr gmId ctId) $ setGroupStatus notifyAdminUsers env cc groupId GRSRemoved $ \gr' -> do notifyOwner gr' $ "You are removed from the " <> gt <> " " <> userGroupReference gr' g <> ".\n\nThe " <> gt <> " is no longer listed in the directory." notifyAdminUsers $ "The " <> gt <> " " <> groupReference g <> " is de-listed (" <> gt <> " owner is removed)." when (isJust pg_) $ leavePublicGroup g - deContactLeftGroup :: ContactId -> GroupInfo -> IO () - deContactLeftGroup ctId g@GroupInfo {groupId, groupProfile = GroupProfile {publicGroup = pg_}} = do + deContactLeftGroup :: ContactId -> GroupMemberId -> GroupInfo -> IO () + deContactLeftGroup ctId gmId g@GroupInfo {groupId, groupProfile = GroupProfile {publicGroup = pg_}} = do let gt = maybe "group" groupTypeStr' pg_ logInfo $ "contact ID " <> tshow ctId <> " left group " <> viewGroupName g withGroupReg g "contact left" $ \gr -> - when (ctId `isOwner` gr) $ + when (isOwnerMember gr gmId ctId) $ setGroupStatus notifyAdminUsers env cc groupId GRSRemoved $ \gr' -> do notifyOwner gr' $ "You left the " <> gt <> " " <> userGroupReference gr' g <> ".\n\nThe " <> gt <> " is no longer listed in the directory." notifyAdminUsers $ "The " <> gt <> " " <> groupReference g <> " is de-listed (" <> gt <> " owner left)." diff --git a/tests/Bots/DirectoryTests.hs b/tests/Bots/DirectoryTests.hs index 4fe275a0dd..9c627df90d 100644 --- a/tests/Bots/DirectoryTests.hs +++ b/tests/Bots/DirectoryTests.hs @@ -12,7 +12,7 @@ import ChatTests.Groups (memberJoinChannel, prepareChannel1Relay) import ChatTests.Utils import Control.Concurrent (forkIO, killThread, threadDelay) import Control.Exception (finally) -import Control.Monad (forM_, void, when) +import Control.Monad (forM_, when, void) import qualified Data.Aeson as J import qualified Data.Text as T import Directory.Captcha @@ -52,6 +52,7 @@ directoryServiceTests = do it "should de-list if owner is removed from the group" testDelistedOwnerRemoved it "should NOT de-list if another member leaves the group" testNotDelistedMemberLeaves it "should NOT de-list if another member is removed from the group" testNotDelistedMemberRemoved + it "should NOT de-list if the owner rejoins via the group link and leaves the second membership" testNotDelistedOwnerRejoinsViaLink it "should de-list if service is removed from the group" testDelistedServiceRemoved it "should de-list if group is deleted" testDelistedGroupDeleted it "should de-list/re-list when service/owner roles change" testDelistedRoleChanges @@ -697,6 +698,56 @@ testNotDelistedMemberRemoved ps = cath #> "@'SimpleX Directory_1' privacy" groupFoundN_ "_1" Nothing 2 cath "privacy" +-- Reproduces the de-listing bug where a non-owner member associated with the +-- registration owner's contact (via the probe-and-merge mechanism) de-lists the +-- group when it leaves. The owner joins the directory-managed link a second time +-- (a single client owning both connection ends completes the merge with no +-- modified client), then leaves that second membership while remaining the owner. +testNotDelistedOwnerRejoinsViaLink :: HasCallStack => TestParams -> IO () +testNotDelistedOwnerRejoinsViaLink ps = + withDirectoryService ps $ \superUser dsLink -> + withNewTestChat ps "bob" bobProfile $ \bob -> do + bob `connectVia` dsLink + submitGroup bob "privacy" "Privacy" + groupAccepted bob "privacy" 1 + welcomeWithLink <- completeRegistration superUser bob "privacy" "Privacy" 1 + let groupLink = dropStrPrefix "Link to join the group privacy: " welcomeWithLink + -- turn off the captcha filter so the owner's re-join is not screened + bob #> "@'SimpleX Directory' /filter 1 off" + bob <# "'SimpleX Directory'> > /filter 1 off" + bob <## " Spam filter settings for group privacy set to:" + bob <## "- reject long/inappropriate names: disabled" + bob <## "- pass captcha to join: disabled" + bob <## "" + bob <## "/'filter 1 name' - enable name filter" + bob <## "/'filter 1 captcha' - enable captcha challenge" + bob <## "/'filter 1 name captcha' - enable both" + -- the registration owner connects to the directory-managed link again, + -- creating a second membership that the probe-and-merge mechanism + -- associates with the owner's own contact on the directory service + bob ##> ("/c " <> groupLink) + bob <## "connection request sent!" + bob <## "#privacy_1: joining the group..." + bob <## "#privacy_1: you joined the group" + bob + <### [ "#privacy: 'SimpleX Directory' added bob_1 (Bob) to the group (connecting...)", + "contact and member are merged: 'SimpleX Directory', #privacy_1 'SimpleX Directory_1'", + "use @'SimpleX Directory' to send messages", + "#privacy_1: member bob_2 (Bob) is connected", + "#privacy: new member bob_1 is connected" + ] + -- allow the directory service to complete the contact/member merge that + -- associates the second membership (bob_1) with bob's contact + threadDelay 3000000 + -- owner leaves the second membership, which is not the owner member + bob ##> "/l privacy_1" + bob <## "#privacy_1: you left the group" + bob <## "use /d #privacy_1 to delete the group" + bob <## "#privacy: bob_1 left the group" + -- the group must remain listed: the leaving member is not the owner member + (superUser TestParams -> IO () testDelistedServiceRemoved ps = withDirectoryService ps $ \superUser dsLink ->