-
Notifications
You must be signed in to change notification settings - Fork 96
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
Make E2EEConfig required #1891
Make E2EEConfig required #1891
Conversation
Previously it could be either undefined or type None which meant the same thing: no need to have both, just make it required. This also means we can move the line to set e2ee enabled into a more sensible place rather than in the ActiveCall de-nulling wrapper.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## livekit #1891 +/- ##
========================================
Coverage 30.14% 30.14%
========================================
Files 47 47
Lines 1874 1874
Branches 327 327
========================================
Hits 565 565
Misses 1264 1264
Partials 45 45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
looks good but I think in this context we should decide what we dot with enableE2EE
https://github.com/vector-im/element-call/blob/04d41b8be1ac3b291db71afbf4fe7152293d1ba5/src/UrlParams.ts#L214-L215
It is somewhat redundant if missing perpartispant and missing shared key implies enableE2EE
= false
} else { | ||
return { mode: E2eeType.NONE }; |
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 also have the e2eeEnabled prop. maybe a check if enableE2EE=false
should overrride this?
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.
ah, so this is a throwback from full mesh and passes the flag to the js-sdk to disable encryption of the events for calling, so not relevant for livekit. In fact, we should probably remove it altogether from the livekit branch as it won't do anything. This is probably not something for this PR though as it's not particularly dependant or related.
Previously it could be either undefined or type None which meant the same thing: no need to have both, just make it required.
This also means we can move the line to set e2ee enabled into a more sensible place rather than in the ActiveCall de-nulling wrapper.