-
-
Notifications
You must be signed in to change notification settings - Fork 652
Import room key bundles received after invite. #5080
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
Import room key bundles received after invite. #5080
Conversation
richvdh
left a comment
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.
A few comments, generally looks good
- Remove `onReceiveToDeviceEvent` from `CryptoBackend`. - Copy old room key bundle importing logic to `preprocessToDeviceEvents`.
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.
a few more comments. Main thing is the inviter param to maybeAcceptKeyBundle.
richvdh
left a comment
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 otherwise. Feel free to merge, after addressing the comments
src/common-crypto/CryptoBackend.ts
Outdated
| * the inviter's ID. | ||
| * | ||
| * @param roomId - The room we were invited to, for which we did not receive a key bundle before accepting the invite. | ||
| * @param inviterId - The user who invited us to the room and is expected to have sent the room key bundle. |
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.
| * @param inviterId - The user who invited us to the room and is expected to have sent the room key bundle. | |
| * @param inviterId - The user who invited us to the room and is expected to send the room key bundle. |
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.
Addressed in f9ea673
src/rust-crypto/rust-crypto.ts
Outdated
|
|
||
| /** mapping of room ID -> inviter ID for rooms pending MSC4268 key bundles */ | ||
| private roomsPendingKeyBundles: Set<string> = new Set(); | ||
| private readonly roomsPendingKeyBundles: Record<string, string> = {}; |
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.
generally we try to use Map rather than Record, to avoid any danger of prototype pollution
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.
Addressed in f9ea673
| } | ||
| }, | ||
| (err) => { | ||
| this.logger.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.
suggest giving a bit more context here, so that the logs are more useful: error attempting to download key bundle for room ${roomId} or so
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.
Addressed in f9ea673
Partially fixes element-hq/element-web#30740. This currently does not work over a session reload, which I intend to introduce in a different PR.
Checklist
public/exportedsymbols have accurate TSDoc documentation.