-
-
Notifications
You must be signed in to change notification settings - Fork 588
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
Process received room key requests #449
Conversation
Doesn't actually do any of the crypto magic yet.
* Determine if we have the keys necessary to respond to a room key request | ||
* | ||
* @param {module:crypto#RoomKeyRequest} keyRequest | ||
* @return {boolean} true if we have the keys and could (theoretically) share |
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.
You'll probably be querying indexeddb to get the answer right? Probably want this to be a Promise
then?
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.
well, not currently - the relevant info is in localstorage. Admittedly we'd probably like it to move to indexeddb, but that's a whole separate project
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.
If it's in local storage, won't we hit size limits again? It's pretty easy to convert this to be q(bool)
right? I would prefer if we did an async design up-front, as it's often a pain to turn a synchronous API into an async one retrospectively.
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 relevant info is already in localstorage.
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.
// list of IncomingRoomKeyRequests/IncomingRoomKeyRequestCancellations | ||
// we received in the current sync. | ||
this._receivedRoomKeyRequests = []; | ||
this._receivedRoomKeyRequestCancellations = []; |
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.
Storing them like this means we lose ordering information from /sync
. Is this a problem?
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.
E.g if I send a key request then immediately cancel it, currently we will process the key request due to how _processReceivedRoomKeyRequests
is currently implemented.
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.
well now.
AFAIK there is no guarantee in the protocol that the ordering of to-device messages is preserved anyway, so this whole thing is a steaming pile of fragility.
That said - in the case you mention, we will process the key request, and then process the cancellation. Which is considerably better than processing (and, presumably, ignoring) the cancellation and then the request.
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.
we will process the key request, and then process the cancellation.
Doesn't that race? We should at least mention the ordering concerns and decisions / things we've punted on for now somewhere.
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.
yes, of course it races:
AFAIK there is no guarantee in the protocol that the ordering of to-device messages is preserved anyway, so this whole thing is a steaming pile of fragility.
the order in which requests and cancellations order are, tbh, the least of my concerns here. I want to ship it and iterate it rather than think through the edge cases any more. This work has taken weeks already.
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.
src/crypto/index.js
Outdated
continue; | ||
} | ||
|
||
req._shareCallback = () => { |
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 don't like that Crypto
is clobbering an _
variable on a different class (IncomingRoomKeyRequest
) like this. Please either make a public function to do this, or don't pretend that this is private. Just because both classes are in the same 1300 line file is not a good enough reason to gut wrench.
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. Hopefully fixed in 1664312.
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.
WFM!
Otherwise LGTM. |
Avoid gut-wrenching properties on IncomingRoomKeyRequest.
LGTM |
port the matrix-org/matrix-js-sdk#449 code
Doesn't actually do any of the crypto magic yet.