From fc7dddbeda7ccd53f99d951ba0cda0cbd645acbf Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Tue, 11 Aug 2026 09:17:55 +0000 Subject: [PATCH] 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. --- .../src/Directory/Events.hs | 12 ++--- .../src/Directory/Service.hs | 33 +++++++----- tests/Bots/DirectoryTests.hs | 53 +++++++++++++++++++ 3 files changed, 80 insertions(+), 18 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 a3af872ac3..7dcc32ee73 100644 --- a/apps/simplex-directory-service/src/Directory/Service.hs +++ b/apps/simplex-directory-service/src/Directory/Service.hs @@ -327,10 +327,10 @@ directoryServiceEvent st opts@DirectoryOpts {adminUsers, superUsers, serviceName 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 @@ -354,6 +354,15 @@ directoryServiceEvent st opts@DirectoryOpts {adminUsers, superUsers, serviceName 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 {dbOwnerMemberId, dbContactId} gmId ctId = + maybe (ctId == dbContactId) (== gmId) dbOwnerMemberId withGroupReg :: GroupInfo -> Text -> (GroupReg -> IO ()) -> IO () withGroupReg GroupInfo {groupId, localDisplayName} err action = getGroupReg cc groupId >>= \case @@ -868,13 +877,13 @@ directoryServiceEvent st opts@DirectoryOpts {adminUsers, superUsers, serviceName 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 st env cc groupId GRSActive $ \gr' -> do @@ -923,23 +932,23 @@ directoryServiceEvent st opts@DirectoryOpts {adminUsers, superUsers, serviceName 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 st 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 st 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 15f713cd1c..48c114152e 100644 --- a/tests/Bots/DirectoryTests.hs +++ b/tests/Bots/DirectoryTests.hs @@ -13,6 +13,7 @@ import ChatTests.Utils import Control.Concurrent (forkIO, killThread, threadDelay) import Control.Exception (finally) import Control.Monad (forM_, when) +import Data.List (isInfixOf) import qualified Data.Aeson as J import qualified Data.Text as T import Directory.Captcha @@ -53,6 +54,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 @@ -702,6 +704,57 @@ 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" + welcomeWithLink <- groupAccepted bob "privacy" 1 + completeRegistration superUser bob "privacy" "Privacy" welcomeWithLink 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", + Predicate ("Link to join the group privacy" `isInfixOf`), + "#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 ->