-
Notifications
You must be signed in to change notification settings - Fork 246
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
fix(community)_: fix pure readonly channels not respecting the right roles #5513
fix(community)_: fix pure readonly channels not respecting the right roles #5513
Conversation
Jenkins Builds
|
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.
Good catch, thanks!
} | ||
|
||
_, err = s.bob.SendChatMessage(context.Background(), msg) | ||
s.Require().Error(err) |
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.
Would it be possible to check exact error here?
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.
That was a good tip, turns out I was passing the wrong chatID, so the error was unknown chat
🙈
I'm fixing it and making it test the error
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.
Fixed
FYI, I ran the test a 1000 times:
|
4841803
to
dd77220
Compare
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
Fixes status-im/status-desktop#15547
The problem was that we used
channelEncrypted
to know if we should use all the community members or the given members by the control node.However, pure read-only channel (for example announcements) are permissions, but not encrypted, since everyone still has access to read them.
The solution is to add
channelHasPermissions
and use that one instead in the hydrate and dehydrate functions.I also added a test to validate the behaviour. It seems like we never tested pure read-only channels before.