Fix external guest access url for unencrypted rooms #12345
Conversation
…compatible with unencrypted embedded calls) Signed-off-by: Timo K <[email protected]>
toger5
added
the
T-Defect
| @@ -279,7 +279,7 @@ export const useRoomCall = ( | |||
| url.pathname = "/room/"; | |||
| // Set params for the sharable url | |||
| url.searchParams.set("roomId", room.roomId); | |||
| url.searchParams.set("perParticipantE2EE", "true"); | |||
| if (room.hasEncryptionStateEvent()) url.searchParams.set("perParticipantE2EE", "true"); | |||
There was a problem hiding this comment.
This is different from the logic used for the widget URL: https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/models/Call.ts#L784 - the other one seems more correct. Shouldn't they just share a function to decide whether the call is encrypted, and/or some common code to generate the common parts of the URL?
There was a problem hiding this comment.
The other one says it's deprecated. Maybe updating both makes sense though.
There was a problem hiding this comment.
Mmm, but that says to use CryptoApi.isEncryptionEnabledInRoom instead. I think the problem with just checking for the presence of a state event is that it can be redacted, potentially by a nasty homeserver, but the clients should still treat the room as encrypted. This might let the HS force calls to be unencrypted when they ought to be.
There was a problem hiding this comment.
This is an sync fn however. that would trickle down quiet far. I would need to make Call.get async (because that needs the widget, which needs the widget url parameters that contain on the encryption flag. Is this what we want?
Its also super important, that both clients have the same understanding of the current encryption state of the room. Could CryptoApi remembering that state one one client but another client does not have this information bring this out of sync?
There was a problem hiding this comment.
I am doing the "use isEncryptionEnabledInRoom" change now.
There was a problem hiding this comment.
I think i found a fix for it: 6c03bb4
There was a problem hiding this comment.
Eugh, sorry - I didn't notice this was async, and yes, having Call.get be async is not ideal. Chatting to the crypto team suggests that actually the presence of the state event might be fine after all, so I think whichever you think do whatever you think is best.
There was a problem hiding this comment.
I updated back to not be async and use hasEncryptionStateEvent
There was a problem hiding this comment.
no worries. I think it was worth investigating if it can be done using the async fn.
Signed-off-by: Timo K <[email protected]>
toger5
force-pushed
the
toger5/fix-link-share-encryption
branch
from
d6e498c
to
74cf9cb
Compare
toger5
force-pushed
the
toger5/fix-link-share-encryption
branch
from
6c03bb4
to
a6754c3
Compare
Signed-off-by: Timo K <[email protected]>
Signed-off-by: Timo K <[email protected]>
The external guest url always contained perParticipantE2EE. We do not encrypt calls in unencrypted matrix rooms (if ther room does not have a megolm session there is no good way to get the key material). So the guest SPA should also not use encryption.
Signed-off-by: Timo K [email protected]
Checklist
public/exportedsymbols have accurate TSDoc documentation.