[WPB-22549] fix logic around members with user type app and no user identity#5029
Merged
[WPB-22549] fix logic around members with user type app and no user identity#5029
Conversation
fisx
force-pushed
the
WPB-22549-events-debugging-2
branch
from
b150ea4 to
f7459ee
Compare
zebot
added
the
ok-to-test
fisx
force-pushed
the
WPB-22549-events-debugging-2
branch
2 times, most recently
from
60f0725 to
3bbfc76
Compare
Contributor
Author
|
one2one conversations work! here is a shell script that may reproduce just the "member opens conv with app" case (not thoroughly tested): |
Contributor
Author
|
regular user creating a team group conv with an app does not work. 403 instead of success when sending the MLS add commit. again, shell script for the "regular contacts app" case: Channels have a similar problem. |
fisx
force-pushed
the
WPB-22549-events-debugging-2
branch
from
59d450a to
92bd4ec
Compare
... even though they have no identity.
fisx
force-pushed
the
WPB-22549-events-debugging-2
branch
from
4782849 to
c1a8a2f
Compare
fisx
changed the title
fisx
commented
Feb 19, 2026
| -- these tests are structurally very similar, and we could reduce the | ||
| -- redundancy further. | ||
| -- | ||
| -- TODO: what about federation? |
Contributor
Author
There was a problem hiding this comment.
can we keep these? i really want to address them in the next PR.
Member
There was a problem hiding this comment.
Why next PR?
Would you remove these if your next PR gets delayed due to something else?
Contributor
Author
There was a problem hiding this comment.
client devs are waiting for this PR to land. i changed it into FUTUREWORK and will either get to it or not, ok?
Contributor
Author
There was a problem hiding this comment.
This is the relevant change in this PR. It allows apps to be found even though they have no identity. I also reworded the whole thing in the vaining hope it's now more readable.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where app users (which don't have a userIdentity) were incorrectly filtered out by the getAccountsByImpl function when includeUsersWithoutIdentity = False. The fix refactors the filtering logic to explicitly allow users with userType == UserTypeApp regardless of the includeUsersWithoutIdentity flag, since apps are valid users that should always be included.
Changes:
- Refactored user filtering logic in
getAccountsByImplto correctly handle app users without identities - Expanded test coverage for conversations involving app users across multiple conversation types (Proteus, one-to-one, team, and channel)
- Removed redundant app testing code from
Test/Apps.hsthat was moved to more comprehensive tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs |
Core fix: refactored filter logic to ensure app users are included regardless of includeUsersWithoutIdentity flag |
libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs |
Added FUTUREWORK comment questioning necessity of getUsersByVariousKeys function |
integration/test/Test/Conversation.hs |
Significantly expanded test coverage with structured tests for apps in different conversation types; added ConvType enum and parameterized test function |
integration/test/Test/Apps.hs |
Removed duplicate test code for one2one conversations and key package claiming that's now covered in Test/Conversation.hs |
integration/test/MLS/Util.hs |
Added Eq and Show derivations to MessagePackage for better test support |
changelog.d/3-bug-fixes/WPB-22549-fix-brig-lookup-users |
Added changelog entry documenting the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
blackheaven
requested changes
Feb 19, 2026
fisx
commented
Feb 19, 2026
| resp.status `shouldMatchInt` 404 | ||
| resp.json %. "label" `shouldMatch` "not-found" | ||
| resp.status `shouldMatchInt` 403 | ||
| resp.json %. "label" `shouldMatch` "same-binding-team-users" |
Contributor
Author
There was a problem hiding this comment.
not sure where the 404 comes from, i think it's a git artefact from an earlier, wrong version of this test.
Member
There was a problem hiding this comment.
Its an artefact of apps getting skipped when using UserSubsystem.getAccountsBy.
Contributor
Author
There was a problem hiding this comment.
No, that changes in this PR: getAccountsBy now returns apps even if they have no identity.
4 tasks
akshaymankar
previously requested changes
Feb 19, 2026
| resp.status `shouldMatchInt` 404 | ||
| resp.json %. "label" `shouldMatch` "not-found" | ||
| resp.status `shouldMatchInt` 403 | ||
| resp.json %. "label" `shouldMatch` "same-binding-team-users" |
Member
There was a problem hiding this comment.
Its an artefact of apps getting skipped when using UserSubsystem.getAccountsBy.
| -- these tests are structurally very similar, and we could reduce the | ||
| -- redundancy further. | ||
| -- | ||
| -- TODO: what about federation? |
Member
There was a problem hiding this comment.
Why next PR?
Would you remove these if your next PR gets delayed due to something else?
Comment on lines
+52
to
+53
| testConversationWithAppOwnTeam :: (HasCallStack) => ConvType -> App () | ||
| testConversationWithAppOwnTeam ConvTypeProteus = do |
Member
There was a problem hiding this comment.
Why create this ConvType thing and write four different cases for this function, instead of just writing 4 functions?
Contributor
Author
There was a problem hiding this comment.
short answer it just happened :)
i wrote a comment explaining this, but if that comment isn't compelling i can change it.
Comment on lines
+1007
to
+1017
| weWant :: User -> Bool | ||
| weWant user = | ||
| not dropBecauseInvitationPending | ||
| && ( isJust user.userIdentity | ||
| || includeUsersWithoutIdentity | ||
| || user.userType == UserTypeApp | ||
| ) | ||
| where | ||
| dropBecauseInvitationPending = | ||
| includePendingInvitations == NoPendingInvitations | ||
| && user.userStatus == PendingInvitation |
Member
There was a problem hiding this comment.
Suggested change
| weWant :: User -> Bool | |
| weWant user = | |
| not dropBecauseInvitationPending | |
| && ( isJust user.userIdentity | |
| || includeUsersWithoutIdentity | |
| || user.userType == UserTypeApp | |
| ) | |
| where | |
| dropBecauseInvitationPending = | |
| includePendingInvitations == NoPendingInvitations | |
| && user.userStatus == PendingInvitation | |
| applyPendingInvitationsFilter :: User -> Bool | |
| applyPendingInvitationsFilter user = | |
| case (includePendingInvitations, user.userStatus) of | |
| (NoPendingInvitations, PendingInvitation) -> False | |
| _ -> True | |
| applyIdentityFilter :: User -> Bool | |
| applyIdentityFilter user = | |
| case (includeUsersWithoutIdentity, user.userType, user.userIdentity) of | |
| (False, UserTypeRegular, Nothing) -> False | |
| _ -> True |
IMO this is easier to understand. Please feel free to change the names of the funtions.
Co-authored-by: Gautier DI FOLCO <gautier.difolco@wire.com>
Co-authored-by: Akshay Mankar <akshay@wire.com>
- s/TODO/FUTUREWORK/ - write 4 test cases, not one with four possible argument values.
…bugging-2' into WPB-22549-events-debugging-2
blackheaven
approved these changes
Feb 20, 2026
Contributor
blackheaven
left a comment
blackheaven
left a comment
There was a problem hiding this comment.
I have paired on it :)
fisx
dismissed
akshaymankar’s stale review
i think i've handled all your feedback, let me know if i missed anything.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
changelog.d