This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
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.
Support MSC3814: Dehydrated Devices Part 2 #16010
Support MSC3814: Dehydrated Devices Part 2 #16010
Changes from 7 commits
ac4f3fb
ceed144
8a7db88
217e2eb
61818c8
ec98b6a
ef2099d
8ca4c1f
c3cb646
f14c51d
9e3de14
4357f5b
cdc9859
c5f4357
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.
Aren't we creating the dehydrated device now? How can it already have keys? Aren't devices unique per user/device 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.
This is creating/storing the dehydrated device - to your question of how it can already have keys I actually don't know - per the MSC the keys were to be uploaded over the
/keys/upload
endpoint, implying that they already exist: "After the dehydrated device is uploaded, the client will upload the encryptionkeys using
POST /keys/upload/{device_id}
, where thedevice_id
parameter isthe device ID given in the response to
PUT /dehydrated_device
"#15929 changed this so that the key upload was integrated into the
PUT
endpoint, and this PR is further refining the key upload such that storing the device and storing the keys are part of one single database transaction, addressing @poljar's concerns around storing the dehdrated device and it's keys being atomic.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.
Uploading the public keys at the time of the dehydrated device creation tries to address this race: matrix-org/matrix-spec-proposals#3814 (comment).
As to how can it already have keys, well if we remember what a dehydrated device is then it becomes quite clear. A dehydrated device are the private identity and one-time keys of a device, of course they are encrypted so the server can't access them.
It becomes quite natural, that we would like to upload the public parts of those same keys to the server at the same time, once we realize this. I think that doing it in a separate
POST /keys/upload/
call is a mistake and quite weird from an API point of view, which the above mentioned race condition showcases.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.
OK, but part of this function checks if we already have keys uploaded for this device (and bails if they're not exactly equal to the new keys). I still don't follow the order of events that would let you upload keys before the device is created.
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 could be me being overzealous here - I basically made sure that all the checks that happen in the
/keys/upload
pathway happen in this pathway, but maybe that's not necessary and we can just store the keys without checking them?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.
Part of it would be straightforward (Move
_set_e2e_device_keys_txn
to a method (instead of an inner function) and call it from both places.)Although it seems this entire function is just a copy of
upload_keys_for_user
-- can we just call that instead?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 if this is the case we can probably call the store methods that set the values directly and avoid the complicated logic of reusing 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.
so the reason I didn't do this was that my understanding of what was being asked was that the storing of the keys and the storing of the device needs to all happen in the same transaction - ie in
_store_dehydrated_device_txn
- wouldn't calling the store methods open a separate transaction?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.
They would need to be refactored to take a transaction as the first parameter and current callers would call that also.
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.
Right I think this is sorted now, sorry for the confusion.