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.
MatrixRTC: Refactor | Introduce a new Encryption manager (used with experimental to device transport) #4799
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
base: develop
Are you sure you want to change the base?
MatrixRTC: Refactor | Introduce a new Encryption manager (used with experimental to device transport) #4799
Changes from 3 commits
0f4e370
b19c7a6
6411597
7e0cf4d
ecf3f82
c1b9e0f
8ad79ef
ec4c466
80bd66d
d79fc58
8e9af36
9b06920
a9413f9
be3c359
07af3d9
5e7043f
cf05a8f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 deprecation could already be added to
IEncryptionManager
(@deprecated
)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 don't use media in most places. And ensure does not imply that calling this will result in sending keys over the chosen transport. I also think the name should make clear that we generate a new key (for a new index).
I think we should include the following words in the name:
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 manager has changed a bit so this call is really ensuring that everyone has the keys it needs. So it will not rotate key anymore.
it will be no op if no action needed.
I have no strong opinion on the name to use, maybe ensureOutboundKey ?
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 make me wonder if we are mixing event emission and "callbacks in constructors" too much in the matrixrtc part of the js-sdk.
If onEncryptionKeysChanged does nothing else but emitting the key when this is called, It might be nicer to use the
reemitter
pattern like we do for the transport change event.IIRC the session will just emit the key change and the new key will be consumed by element-call/livekit.
So reemission (if there is not processing happening on the MatrixRTCSession level) sounds like it is easier to read. especially in line 144 this would make it super clear that this is where the key leaves the EncryptionManager.
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.
maybe we can sanity check other locations where we do "callbacks in constructors" and see what makes the most sense to makt it consistent.
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 are other things that can be cleaned in the constructor. I don't think we also need the
getMembership
(if we could just pass it in the join)