diff --git a/changelog.d/2-features/WPB-26650 b/changelog.d/2-features/WPB-26650 new file mode 100644 index 0000000000..d033ec48bd --- /dev/null +++ b/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. diff --git a/docs/src/developer/reference/config-options.md b/docs/src/developer/reference/config-options.md index 066637dbb0..7ae112d851 100644 --- a/docs/src/developer/reference/config-options.md +++ b/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: diff --git a/integration/test/Test/AdminlessGroups.hs b/integration/test/Test/AdminlessGroups.hs index 05ef119f48..d6c18413f3 100644 --- a/integration/test/Test/AdminlessGroups.hs +++ b/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 diff --git a/libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs b/libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs index cc840b1783..78474964d8 100644 --- a/libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs +++ b/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,7 +1478,8 @@ 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 -> @@ -1465,30 +1487,33 @@ adminlessAutopromoteOrSendReminder :: 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.