WPB-26650 prevent fed state drift on prevent adminless groups actions

changelog.d/2-features/WPB-26650

@@ -0,0 +1 @@
+Skip senderless prevent-adminless deletion for federated conversations with remote members to prevent remote state drift.

docs/src/developer/reference/config-options.md

@@ -362,6 +362,21 @@ The settings mean:
 - `deletionTimeoutDuration`: how long to keep an adminless conversation before it is deleted.
 - `reminderTimeoutDurations`: when before deletion reminder notifications should be sent.
 
+In federated conversations, automatic senderless deletion is skipped when the
+conversation contains remote members because the corresponding system delete
+event cannot yet be sent safely to the remote backend. This applies both when
+the feature is enabled and existing conversations are scanned without an
+origin user, and when a previously scheduled senderless deletion job runs.
+Reminders for a skipped deletion are also skipped because they would be
+misleading. The skipped deletion is logged at info level.
+
+Autopromotion still runs because the conversation-owning backend stores the
+authoritative member roles. Remote clients may miss the immediate senderless
+member-update notification, but a subsequent conversation fetch obtains the
+current role from the owning backend. Member updates and deletions with an
+origin user continue to use the existing ordinary federation events and are
+not skipped.
+
 Durations are strings with a number and a unit suffix. Supported units are `us`, `ms`, `s`, `m`, `h`, `d`, and `w`. It is **not** recommended or supported to set these below a day in production environments.
 
 Feature responses, including `GET /feature-configs`, `GET /teams/:tid/features`, and `GET /teams/:tid/features/preventAdminlessGroups`, include the duration fields:

integration/test/Test/AdminlessGroups.hs

@@ -21,6 +21,7 @@ import API.Brig
 import API.Galley
 import API.GalleyInternal hiding (getConversation)
 import qualified API.GalleyInternal as GalleyI
+import Control.Concurrent (threadDelay)
 import MLS.Util
 import Notifications
 import SetupHelpers hiding (deleteUser)
@@ -325,6 +326,129 @@ testAdminlessSetupMemberUpdateAfterAdminLeaves = do
       resp.status `shouldMatchInt` 200
       resp.json %. "members.self.conversation_role" `shouldMatch` "wire_admin"
 
+testAdminlessSetupDeletesWithOriginAndRemoteMembers :: (HasCallStack) => App ()
+testAdminlessSetupDeletesWithOriginAndRemoteMembers = do
+  (alice, tid, _) <- createTeam OwnDomain 1
+  remoteUser <- randomUser OtherDomain def
+  connectTwoUsers alice remoteUser
+
+  setTeamFeatureLockStatus OwnDomain tid "preventAdminlessGroups" "unlocked"
+  patchTeamFeature OwnDomain tid "preventAdminlessGroups" (object ["status" .= "disabled"]) >>= assertSuccess
+
+  conv <-
+    postConversation
+      alice
+      (defProteus {team = Just tid, qualifiedUsers = [remoteUser], newUsersRole = "wire_member"})
+      >>= getJSON 201
+  convQid <- objQidObject conv
+
+  removeMember alice conv alice >>= assertSuccess
+
+  eventually $ bindResponse (listConversationIds remoteUser def) $ \resp -> do
+    resp.status `shouldMatchInt` 200
+    conversationIds <- resp.json %. "qualified_conversations" & asList
+    conversationIds `shouldContain` [convQid]
+
+  withWebSockets [remoteUser] $ \[wsRemoteUser] -> do
+    setTeamFeatureConfigVersioned (ExplicitVersion 17) alice tid "preventAdminlessGroups" (mkAdminlessFeature "enabled" "1s" []) >>= assertSuccess
+
+    deleteNotif <- awaitMatchFor 20 isConvDeleteNotif wsRemoteUser
+    deleteNotif %. "payload.0.qualified_from" `shouldMatch` objQidObject alice
+
+    eventually $ bindResponse (listConversationIds remoteUser def) $ \resp -> do
+      resp.status `shouldMatchInt` 200
+      conversationIds <- resp.json %. "qualified_conversations" & asList
+      conversationIds `shouldNotContain` [convQid]
+
+testAdminlessSetupSkipsDeletionForRemoteMembers :: (HasCallStack) => App ()
+testAdminlessSetupSkipsDeletionForRemoteMembers = do
+  -- Senderless deletion is skipped when remote members are present because
+  -- remote backends do not support the system delete event yet.
+  (alice, tid, _) <- createTeam OwnDomain 1
+  remoteUser <- randomUser OtherDomain def
+  connectTwoUsers alice remoteUser
+
+  configureAdminlessGroupsFeature OwnDomain tid "disabled" "1s" []
+
+  alice1 <- createMLSClient def alice
+  remoteUser1 <- createMLSClient def remoteUser
+  traverse_ (uploadNewKeyPackage def) [alice1, remoteUser1]
+
+  conv <- createTeamMLSConversation alice tid alice1 [remoteUser]
+
+  -- Create an adminless conversation while the feature is disabled. Enabling
+  -- the feature later exercises the system-triggered setup path.
+  removeMember alice conv alice >>= assertSuccess
+
+  configureAdminlessGroupsFeature OwnDomain tid "enabled" "1s" ["1s"]
+
+  -- The setup job must not schedule deletion or reminders for this
+  -- conversation because it contains a remote member.
+  liftIO $ threadDelay 2_000_000
+  bindResponse (GalleyI.getConversation conv) $ \resp -> do
+    resp.status `shouldMatchInt` 200
+
+testAdminlessSetupSkipsReminderForRemoteMembers :: (HasCallStack) => App ()
+testAdminlessSetupSkipsReminderForRemoteMembers = do
+  -- A remote member prevents senderless deletion. The remaining local app is
+  -- not eligible for promotion, but would receive a system reminder if one
+  -- were emitted.
+  (alice, tid, _) <- createTeam OwnDomain 1
+  remoteUser <- randomUser OtherDomain def
+  connectTwoUsers alice remoteUser
+
+  configureAdminlessGroupsFeature OwnDomain tid "disabled" "5s" ["4s"]
+
+  alice1 <- createMLSClient def alice
+  remoteUser1 <- createMLSClient def remoteUser
+  traverse_ (uploadNewKeyPackage def) [alice1, remoteUser1]
+
+  conv <- createTeamMLSConversation alice tid alice1 [remoteUser]
+  let newApp = def {name = "adminless-federated-reminder-app", description = "not eligible for promotion"}
+  (app, _) <- createAndAddAppMember alice tid alice1 conv newApp
+
+  -- Create an adminless conversation while the feature is disabled. Enabling
+  -- it through the internal path runs senderless setup cleanup.
+  removeMember alice conv alice >>= assertSuccess
+
+  withWebSockets [app] $ \[wsApp] -> do
+    configureAdminlessGroupsFeature OwnDomain tid "enabled" "2s" ["1s"]
+
+    reminderResult <- awaitNMatchesResultFor 5 1 isConvSystemAdminlessReminderNotif wsApp
+    reminderResult.success `shouldMatch` False
+
+    bindResponse (GalleyI.getConversation conv) $ \resp -> do
+      resp.status `shouldMatchInt` 200
+
+testAdminlessSetupAutopromotesWithRemoteMembers :: (HasCallStack) => App ()
+testAdminlessSetupAutopromotesWithRemoteMembers = do
+  -- Autopromotion is safe with remote members because the owning backend is
+  -- authoritative for roles, even though remote clients do not receive the
+  -- senderless system member-update event yet.
+  (alice, tid, [bob]) <- createTeam OwnDomain 2
+  remoteUser <- randomUser OtherDomain def
+  connectTwoUsers alice remoteUser
+
+  configureAdminlessGroupsFeature OwnDomain tid "disabled" "1s" []
+
+  alice1 <- createMLSClient def alice
+  bob1 <- createMLSClient def bob
+  remoteUser1 <- createMLSClient def remoteUser
+  traverse_ (uploadNewKeyPackage def) [alice1, bob1, remoteUser1]
+
+  conv <- createTeamMLSConversation alice tid alice1 [bob, remoteUser]
+
+  -- Create an adminless conversation while the feature is disabled. Enabling
+  -- the feature later promotes Bob through a system action.
+  removeMember alice conv alice >>= assertSuccess
+
+  configureAdminlessGroupsFeature OwnDomain tid "enabled" "1s" []
+
+  liftIO $ threadDelay 2_000_000
+  bindResponse (getConversation bob conv) $ \resp -> do
+    resp.status `shouldMatchInt` 200
+    resp.json %. "members.self.conversation_role" `shouldMatch` "wire_admin"
+
 testAdminlessJobsCancelledOnFeatureDisable :: (HasCallStack) => App ()
 testAdminlessJobsCancelledOnFeatureDisable = do
   (alice, tid, _) <- createTeam OwnDomain 1

libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs

@@ -97,6 +97,7 @@ import Polysemy
 import Polysemy.Error
 import Polysemy.Input
 import Polysemy.TinyLog
+import System.Logger qualified as Log
 import Wire.API.Bot hiding (addBot)
 import Wire.API.Conversation hiding (Member)
 import Wire.API.Conversation.Action
@@ -1195,6 +1196,18 @@ isAdminlessCheckCandidate conv =
   conv.metadata.cnvmType == RegularConv
     && maybe True (== GroupConversation) conv.metadata.cnvmGroupConvType
 
+shouldSkipSystemAdminlessDeletion :: Maybe (Local UserId) -> StoredConversation -> Bool
+shouldSkipSystemAdminlessDeletion mlusr conv =
+  isNothing mlusr
+    && not (null conv.remoteMembers)
+
+logSkippedSystemAdminlessDeletion :: (Member TinyLog r) => Text -> StoredConversation -> Sem r ()
+logSkippedSystemAdminlessDeletion action conv =
+  info $
+    Log.msg (Log.val "Skipping senderless adminless deletion for conversation with remote members")
+      . Log.field "conversation_id" (idToText conv.id_)
+      . Log.field "action" action
+
 setupAdminlessGroupsCleanup ::
   ( Member ConversationStore r,
     Member (ErrorS 'ConvNotFound) r,
@@ -1207,7 +1220,8 @@ setupAdminlessGroupsCleanup ::
     Member BackendNotificationQueueAccess r,
     Member FeaturesConfigSubsystem r,
     Member (Input (Local ())) r,
-    Member JobSubsystem r
+    Member JobSubsystem r,
+    Member TinyLog r
   ) =>
   Maybe (Local UserId) ->
   TeamId ->
@@ -1216,7 +1230,10 @@ setupAdminlessGroupsCleanup mUsr tid = do
   teamConvIds <- E.getTeamConversations tid
   for_ teamConvIds $ \cnv -> do
     lcnv <- qualifyLocal cnv
-    adminlessTryAutopromote mUsr lcnv $ \_ feature _ -> scheduleDeletion lcnv mUsr tid feature
+    adminlessTryAutopromote mUsr lcnv $ \conv feature _ ->
+      if shouldSkipSystemAdminlessDeletion mUsr conv
+        then logSkippedSystemAdminlessDeletion "schedule_for_deletion" conv
+        else scheduleDeletion lcnv mUsr tid feature
 
 guardPreventAdminlessGroups ::
   ( Member ConversationStore r,
@@ -1419,33 +1436,37 @@ adminlessAutopromoteOrDelete ::
     Member BackendNotificationQueueAccess r,
     Member FeaturesConfigSubsystem r,
     Member ProposalStore r,
-    Member CodeStore r
+    Member CodeStore r,
+    Member TinyLog r
   ) =>
   Maybe (Local UserId) ->
   Local ConvId ->
   Sem r ()
 adminlessAutopromoteOrDelete mlusr lcnv = adminlessTryAutopromote mlusr lcnv orAlternativelyDeleteConv
   where
-    orAlternativelyDeleteConv conv _ _ = do
-      removeConversation (qualifyAs lcnv conv)
-      case mlusr of
-        Just lusr ->
-          void $
-            sendConversationActionNotifications
-              (sing @'ConversationDeleteTag)
-              (tUntagged lusr)
-              False
-              Nothing
-              (qualifyAs lcnv conv)
-              (convBotsAndMembers conv)
-              ()
-              def
-        Nothing -> do
-          now <- Now.get
-          Notify.pushSystemEvent
-            Nothing
-            (SystemEvent (tUntagged lcnv) Nothing now conv.metadata.cnvmTeam EdSystemConvDelete)
-            (Set.fromList (map (.id_) conv.localMembers))
+    orAlternativelyDeleteConv conv _ _ =
+      if shouldSkipSystemAdminlessDeletion mlusr conv
+        then logSkippedSystemAdminlessDeletion "deletion" conv
+        else do
+          removeConversation (qualifyAs lcnv conv)
+          case mlusr of
+            Just lusr ->
+              void $
+                sendConversationActionNotifications
+                  (sing @'ConversationDeleteTag)
+                  (tUntagged lusr)
+                  False
+                  Nothing
+                  (qualifyAs lcnv conv)
+                  (convBotsAndMembers conv)
+                  ()
+                  def
+            Nothing -> do
+              now <- Now.get
+              Notify.pushSystemEvent
+                Nothing
+                (SystemEvent (tUntagged lcnv) Nothing now conv.metadata.cnvmTeam EdSystemConvDelete)
+                (Set.fromList (map (.id_) conv.localMembers))
 
 adminlessAutopromoteOrSendReminder ::
   ( Member ConversationStore r,
@@ -1457,38 +1478,42 @@ adminlessAutopromoteOrSendReminder ::
     Member Now r,
     Member E.ExternalAccess r,
     Member BackendNotificationQueueAccess r,
-    Member FeaturesConfigSubsystem r
+    Member FeaturesConfigSubsystem r,
+    Member TinyLog r
   ) =>
   Maybe (Local UserId) ->
   Local ConvId ->
   UTCTimeMillis ->
   Sem r ()
 adminlessAutopromoteOrSendReminder mlusr lcnv deletionScheduledFor = adminlessTryAutopromote mlusr lcnv orAlternativelySendReminder
   where
-    orAlternativelySendReminder conv _ _ = do
-      now <- Now.get
-      case mlusr of
-        Just lusr -> do
-          let event =
-                Event
-                  (tUntagged lcnv)
-                  Nothing
-                  (EventFromUser (tUntagged lusr))
-                  now
-                  (conv.metadata.cnvmTeam)
-                  (EdAdminlessReminder (AdminlessReminder deletionScheduledFor))
-          pushConversationEvent Nothing conv event (qualifyAs lcnv (map (.id_) conv.localMembers)) []
-        Nothing ->
-          Notify.pushSystemEvent
-            Nothing
-            ( SystemEvent
-                (tUntagged lcnv)
+    orAlternativelySendReminder conv _ _ =
+      if shouldSkipSystemAdminlessDeletion mlusr conv
+        then logSkippedSystemAdminlessDeletion "reminder" conv
+        else do
+          now <- Now.get
+          case mlusr of
+            Just lusr -> do
+              let event =
+                    Event
+                      (tUntagged lcnv)
+                      Nothing
+                      (EventFromUser (tUntagged lusr))
+                      now
+                      (conv.metadata.cnvmTeam)
+                      (EdAdminlessReminder (AdminlessReminder deletionScheduledFor))
+              pushConversationEvent Nothing conv event (qualifyAs lcnv (map (.id_) conv.localMembers)) []
+            Nothing ->
+              Notify.pushSystemEvent
                 Nothing
-                now
-                conv.metadata.cnvmTeam
-                (EdSystemAdminlessReminder (AdminlessReminder deletionScheduledFor))
-            )
-            (Set.fromList (map (.id_) conv.localMembers))
+                ( SystemEvent
+                    (tUntagged lcnv)
+                    Nothing
+                    now
+                    conv.metadata.cnvmTeam
+                    (EdSystemAdminlessReminder (AdminlessReminder deletionScheduledFor))
+                )
+                (Set.fromList (map (.id_) conv.localMembers))
 
 -- Use eight random bytes and fold them into a big-endian Word64. This keeps
 -- the helper small, deterministic under tests, and free of extra Random API.