Conversation
9e075d2 to
1e5d2a1
Compare
1e5d2a1 to
f8f3705
Compare
There was a problem hiding this comment.
Pull request overview
This PR prevents app users from creating/being part of user connections across teams by adding an explicit “no apps in connection creation” guard, and it adds an integration test covering cross-team (and optionally cross-domain) scenarios.
Changes:
- Add
ensureNoAppsguard and apply it to local + remote connection creation paths. - Move
ensureNotSameTeamintoBrig.API.Connection.Utiland reuse it fromBrig.API.Connection. - Add an integration test covering cross-team conversations involving apps; add a changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/brig/src/Brig/API/Connection/Util.hs | Introduces ensureNoApps (and exports ensureNotSameTeam) to enforce connection preconditions. |
| services/brig/src/Brig/API/Connection/Remote.hs | Applies ensureNoApps to remote connection creation and updates effect constraints/imports. |
| services/brig/src/Brig/API/Connection.hs | Removes inline ensureNotSameTeam and applies ensureNoApps to local connection creation. |
| integration/test/Test/Apps.hs | Adds a cross-team app conversation integration test. |
| changelog.d/3-bug-fixes/WPB-23995-app-visibility-accross-teams | Documents the behavior change in the changelog entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| createConnectionToRemoteUser self zcon other = do | ||
| ensureNotSameAndActivated self (tUntagged other) | ||
| ensureFederatesWith other | ||
| ensureNoApps self [tUntagged self, tUntagged other] | ||
| mconnection <- lift . wrapClient $ Data.lookupConnection self (tUntagged other) |
There was a problem hiding this comment.
createConnectionToRemoteUser already performs a federated get-users-by-ids in ensureFederatesWith. Calling ensureNoApps here triggers an additional getUserProfiles federation round-trip for the same remote user, increasing latency and adding another failure point on the hot path.
Consider reusing the profile(s) fetched in ensureFederatesWith (it already returns UserProfile with profileType) or restructuring so remote user type + team are checked from a single remote lookup.
| resp.json %. "user" | ||
|
|
||
| -- M1 tries to connect to app A2 from team B => should fail | ||
| -- Apps cannot create connections accross teams |
There was a problem hiding this comment.
Spelling: "accross" should be "across" in this comment.
| -- Apps cannot create connections accross teams | |
| -- Apps cannot create connections across teams |
| import qualified API.BrigInternal as BrigI | ||
| import API.Common | ||
| import API.Galley | ||
| import Control.Lens hiding ((.=)) |
There was a problem hiding this comment.
Control.Lens appears to be unused in this module (no lens operators/functions referenced). With -Wall enabled for integration tests, this will produce an unused-import warning; please remove the import if it isn't needed.
| import Control.Lens hiding ((.=)) |
| @@ -0,0 +1 @@ | |||
| Fix: apps cannot form connections accross teams. Integration test for cross-team conversations working with apps as expected. No newline at end of file | |||
There was a problem hiding this comment.
Spelling: "accross" should be "across". Also consider renaming the changelog file to avoid propagating the typo in tooling/output (depending on how changelog entries are aggregated).
| Fix: apps cannot form connections accross teams. Integration test for cross-team conversations working with apps as expected. | |
| Fix: apps cannot form connections across teams. Integration test for cross-team conversations working with apps as expected. |
blackheaven
left a comment
There was a problem hiding this comment.
copilot lints seem legit
Checklist
changelog.d