-
Notifications
You must be signed in to change notification settings - Fork 25
fix: Do not allow guest users add member - WPB-20582 #4197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix: Do not allow guest users add member - WPB-20582 #4197
Conversation
Test Results 2 files 543 suites 6m 23s ⏱️ For more details on these failures, see this check. Results for commit de27f5f. ♻️ This comment has been updated with latest results. Summary: workflow run #21437654621 |
netbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM;)
David-Henner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're using the wrong value to check isGuest
| public var isGuest: Bool { | ||
| accessRoles.contains(.guest) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accessRoles defines which user roles are allowed in the conversation. To me, the isGuest naming is confusing. A conversation itself cannot be a guest, whereas a user could be a guest in a conversation.
I think a better name for this would be areGuestsAllowed. But even then, I don't think this is the correct value to look at, I think you should rather verify wether the user is a guest
| let isPublicChannelAccessNoGuest = conversation.isChannel | ||
| && conversation.privateChannelPermission == .everyone | ||
| && !conversation.isGuest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checks wether:
- The conversation is a channel
- The permissions are set to everyone
- Guests aren't allowed in the conversation <- wrong check
Here I think we should verify if the user is a guest
…hub.com:wireapp/wire-ios into fix/do-no-allow-add-members-when-guest_WPB20582
| public var areGuestsAllowed: Bool { | ||
| accessRoles.contains(.guest) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry about the misunderstanding, but I meant that you shouldn't use this
|
|
||
| let isPublicChannelAccessNoGuest = conversation.isChannel | ||
| && conversation.privateChannelPermission == .everyone | ||
| && !conversation.areGuestsAllowed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, we should verify that the user isn't a guest, not that the conversation doesn't allow guests
…hub.com:wireapp/wire-ios into fix/do-no-allow-add-members-when-guest_WPB20582
Issue
Federated guest users can add members to a conversation to which they have been invited.
Testing (attached video: BeforeFix_WPB20582)
PROBLEM: The '+ Add Participants' option appears. (at bottom-left)
EXPECTED: The option must not appear, when user is guest.
Evidences
Cause
The system does not validate that a Guest user is restricted from adding members. Specification: https://wearezeta.atlassian.net/wiki/spaces/ENGINEERIN/pages/18416694/Team#Guests
Solution
Add an extra check for validating that Guest users cannot add new members.
Testing
Unit test 'testCanAddParticipantsInChannelIfAddParticipantsIsAdminAndMemberButGuest' validates that button can be added when user conversation is not guest, and cannot otherwise.
Manual test
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: