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.
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
Sliding Sync: Add E2EE extension (MSC3884) #17454
Sliding Sync: Add E2EE extension (MSC3884) #17454
Changes from 4 commits
97f12a7
5642b9f
3aef282
43bca3f
1aee3a7
6cb8131
81dfb72
3d5972f
1822a33
63f3287
9a4e2ed
4704f44
94390ff
2cfecae
9043180
d48fab2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
As an aside to this PR: Why is
device_one_time_keys_count
/device_unused_fallback_key_types
information necessary here?If we assume the device uploading the keys, is the same device specified for the keys, then we could assume they already know about their own keys. I think this is the case because the spec says "The ID of the device these keys belong to. Must match the device ID used when logging in.". And they get the count, etc in the upload response.
If not, it seems like
notifier.wait_for_events(user_id)
should trigger when weupload_keys_for_user(user_id, device_id)
because this would affect thedevice_one_time_keys_count
/device_unused_fallback_key_types
fields in the sync response.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.
For one time keys and fallback keys other devices can take/use them, and so the requesting device needs to upload more keys in that case. See https://spec.matrix.org/v1.11/client-server-api/#one-time-and-fallback-keys
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 not really seeing where other devices upload keys for another device. Can you point out the specific language and endpoint that someone would do that?
If that's the case, it seems like we need to update
notifier.wait_for_events(user_id)
to inform when new keys are uploaded.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 flow is: 1) your device uploads, say, 50 OTKs (one-time-keys), 2) another device takes one or more, 3) your device gets told that some of the keys have been taken, 4) your device uploads more OTKs.
So this mechanism in sync is to inform your device when another device has claimed some keys, so that you can upload 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.
Thanks for explaining!
I've created #17474 to track updating
notifier.wait_for_events(user_id)
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.
Can we add a test for this? i.e. one where there is data to send down and we set a non-zero timeout?
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.
Added tests for this behavior for the E2EE and To-Device extensions ✅
I also added it to the main Sliding sync response but the
test_wait_for_new_data_timeout
variant isn't supported there yet. Since the response currently hasops
(which are slated to be removed), we can't accurately detect empty. I can tackle this in a follow-up PR.