-
Notifications
You must be signed in to change notification settings - Fork 326
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
[FS-705] Fix Repeated MLS Public Keys #2501
Conversation
- The test is extended so only one of the three clients gets an MLS public key, but due to a buggy implementation all of the three clients currently get the key. A fix in the application code is due.
- This test arguably does not belong to this PR, but given that we had an issue with looking up multiple clients, I wanted to also make sure looking up one client without MLS public keys works as expected.
I just added a related test that checks that in presence of a client with MLS public keys, a non-MLS client still has no MLS public keys when looked up via |
@@ -308,10 +309,15 @@ testListClients brig = do | |||
let (pk1, lk1) = (somePrekeys !! 0, (someLastPrekeys !! 0)) |
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.
hlint will soon complain about not using head
here. Whilst the consistent argument is a solid one, I don't think we want to disable the rule globally. I know this was already here, but just something to consider.
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'm in favor of not applying that hlint rule in tests. In tests we don't have to strive to have total functions and head
instead of (!!)
.
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.
Fair point. I'll look into disabling the rule for test targets. 👍
c3 <- responseJsonError =<< addClient brig uid (defNewClient TemporaryClientType [pk3] lk3) | ||
|
||
let pks = Map.fromList [(Ed25519, "random")] | ||
void $ putClient brig uid (clientId c1) pks |
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.
Could you clarify this for me? From what I understood, this test checks that listing the clients work, but here we're updating something.
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.
Sure. An error is in fetching the list of clients, where we would accidentally associate an MLS public key from one client with the rest of clients, potentially non-MLS clients. This test sets an MLS public key for one of the three clients and asserts that GET /clients
gets them as expected: the two unaffected clients should be the same as before, and the MLS client should have the public key listed due to the update.
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.
Ah, I thought that was what the new test did, but this one as well?
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.
The new test testMLSClient
in essence does the same, but for a different endpoint: GET /clients/:client
. I thought I'd add that test while at it (and actually my hunch was that the test would fail, but it turns out we got looking up one client right).
void $ putClient brig uid (clientId c1) pks | ||
|
||
-- Assert that adding MLS public keys to one client does not affect the other | ||
-- client |
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.
🚀
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 didn't expect I'd make that many comments, or I would've made a batch review. Sorry about that.
This PR fixes a bug where all clients, including non-MLS clients, would be associated with the same MLS public key in the response to
GET /clients
.Tracked by https://wearezeta.atlassian.net/browse/FS-705.
Checklist
changelog.d
.