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.

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}

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)."

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' <message> 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 </)
+      groupFound bob "privacy"
+
 testDelistedServiceRemoved :: HasCallStack => TestParams -> IO ()
 testDelistedServiceRemoved ps =
   withDirectoryService ps $ \superUser dsLink ->