WPB-26650 federate senderless adminless events - #5525
Conversation
0da53c2 to
437cd7d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new federation notification handlers trust alreadyPresentUsers from remote backends as notification recipients without filtering against locally-known conversation membership, enabling remote-triggered notification spam.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends Wire’s federation layer to support senderless/system-triggered preventAdminlessGroups notifications (member update, conversation delete, adminless reminder) with explicit capability detection for older remote backends.
Changes:
- Adds new Galley federation notification endpoints and payload types for system-triggered adminless-group events (version-gated from federation API v4).
- Introduces capability checks (
api-version+ version-range support) to decide whether to emit system-delete notifications to remote backends. - Updates adminless-group cleanup/autopromotion logic to emit the new federated system notifications and expands integration coverage accordingly.
File summaries
| File | Description |
|---|---|
| services/galley/src/Galley/API/Federation.hs | Wires the new federation notification endpoints into the Galley federation API sitemap. |
| libs/wire-subsystems/src/Wire/FederationAPIAccess.hs | Adds helper to check whether all remotes support a given notification (via api-version). |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs | Emits federated senderless system notifications; gates system delete on remote capability support. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Notify.hs | Adds enqueue/send helpers for the new system notification bundles. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs | Routes new federation notification operations to federation handlers. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs | Implements handlers for receiving the new system notifications from remote backends. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem.hs | Adds new effect constructors for the three new federation notification handlers. |
| libs/wire-api-federation/src/Wire/API/Federation/Version.hs | Introduces federation API version V4 plus a helper to test support against a version range. |
| libs/wire-api-federation/src/Wire/API/Federation/HasNotificationEndpoint.hs | Exposes helper to check if a given VersionInfo supports an endpoint’s version range. |
| libs/wire-api-federation/src/Wire/API/Federation/API/Util.hs | Adds bundle builders for the three new system notifications with drop-if-unsupported policy. |
| libs/wire-api-federation/src/Wire/API/Federation/API/Galley/Notifications.hs | Defines new notification tags, endpoint paths, version gating, and payload schemas. |
| libs/wire-api-federation/src/Wire/API/Federation/API.hs | Re-exports the new bundle helpers. |
| integration/test/Test/AdminlessGroups.hs | Updates/extends integration tests to cover senderless system delete/reminder/member-update with remote members and unsupported remotes. |
| integration/test/Notifications.hs | Adds helper matcher for system delete notifications. |
| changelog.d/2-features/WPB-26650 | Adds changelog entry for the new federated system notifications. |
Review details
Suppressed comments (2)
libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs:249
alreadyPresentUsersis trusted from the remote backend and used to delete/notify local users without verifying they are members of the remote conversation in our DB. For consistency with other federation notifications (and to prevent remote backends from spamming arbitrary locals), filter recipients viaConversationStore.selectRemoteMembersand only delete/notify the filtered set; also log a warning if the incoming list contains users that are not members.
let rconvId = toRemoteUnsafe requestingDomain e.conversation
localMembers = e.alreadyPresentUsers
E.deleteMembersInRemoteConversation rconvId localMembers
pushSystemEvent
libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs:270
alreadyPresentUserscomes from the remote backend and is used directly as recipients for the reminder. This should be filtered against local DB membership for the remote conversation (same spam-prevention reasoning as other federation notifications), and warnings should be logged when the incoming list contains non-members.
onSystemAdminlessReminder requestingDomain notification = do
let localMembers = notification.alreadyPresentUsers
pushSystemEvent
Nothing
( SystemEvent
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new sendSystem* notification helpers ignore the per-domain recipient buckets from enqueueNotificationsConcurrently, which makes it easy to accidentally send incorrect alreadyPresentUsers payloads across domains.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
libs/wire-subsystems/src/Wire/ConversationSubsystem/Notify.hs:151
- Same issue as
sendSystemMemberUpdate: the callback ignores the per-domain bucket (ruids) and therefore depends on the caller to provide a domain-correctalreadyPresentUserslist. OverridingalreadyPresentUsersfromruidsprevents accidental cross-domain payloads iftargetsspans multiple domains.
sendSystemDelete targets notification =
do
void $
enqueueNotificationsConcurrently Q.Persistent (toList targets) $ \_ ->
makeSystemDeleteBundle notification >>= sendBundle
libs/wire-subsystems/src/Wire/ConversationSubsystem/Notify.hs:164
- Same issue as the other
sendSystem*helpers: the callback ignoresruids, so correctness depends on the caller providing a domain-matchingalreadyPresentUsers. DerivealreadyPresentUsersfromruidsto keep payloads consistent with the recipient bucket.
sendSystemAdminlessReminder targets notification =
do
void $
enqueueNotificationsConcurrently Q.Persistent (toList targets) $ \_ ->
makeSystemAdminlessReminderBundle notification >>= sendBundle
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes cross-backend federation behavior and deletion flows in a way that is operationally sensitive and should get a final human review despite tests.
Review details
Suppressed comments (2)
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs:1526
- This per-domain grouping and
alreadyPresentUsersconstruction is redundant:Notify.sendSystemDeletebuckets recipients by domain internally and overwritesalreadyPresentUserswith the per-bucket user list. Passing the full remote-member set avoids duplicated grouping logic and removes unused work.
let remoteMembersByDomain =
Map.fromListWith
Set.union
[ (tDomain member.id_, Set.singleton member.id_)
| member <- conv.remoteMembers
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs:1579
- This per-domain grouping and
alreadyPresentUsersconstruction is redundant:Notify.sendSystemAdminlessReminderbuckets recipients by domain internally and overwritesalreadyPresentUserswith the per-bucket user list. Passing the full remote-member set reduces duplication and avoids constructing data that won't be used.
let remoteMembersByDomain =
Map.fromListWith
Set.union
[ (tDomain member.id_, Set.singleton member.id_)
| member <- conv.remoteMembers
]
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new version-range support check can crash at runtime when encountering unknown/future version integers due to a partial intToVersion implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs:1225
- logSkippedSystemAdminlessDeletion always logs the reason as "remote backend does not support system delete", but the capability check also returns False on federation errors (e.g. version lookup failures). This makes the reason misleading when the skip is due to an error rather than lack of support.
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new federation API version and new cross-backend notification flows whose correctness depends on multi-service interoperability and runtime federation behavior.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
VeryMilkyJoe
left a comment
There was a problem hiding this comment.
Looks good to me! Just some questions about implementation decisions and the code base :)
77183a1 to
3a694b7
Compare
https://wearezeta.atlassian.net/browse/WPB-26650
Checklist
changelog.d