feat: Conversation MLS verification status updating (WPB-3872) #2035
Conversation
|
|
Datadog ReportAll test runs ❌ 2 Total Test Services: 1 Failed, 0 with New Flaky, 1 Passed Test Services
❌ Failed Tests (12)
|
|
@borichellow do you want feedback on this? If so add a description of what you've done. |
…creating a new for each observer
borichellow
requested review from
MohamadJaara,
typfel,
vitorhugods and
mchenani
Codecov Report
@@ Coverage Diff @@
## develop #2035 +/- ##
=============================================
- Coverage 57.94% 57.88% -0.06%
Complexity 24 24
=============================================
Files 1016 1014 -2
Lines 38173 38215 +42
Branches 3472 3469 -3
=============================================
+ Hits 22119 22121 +2
- Misses 14562 14604 +42
+ Partials 1492 1490 -2
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
So checking that've understood the implementation correctly We have the existing ObserveConversationDetailsUseCase which is the entry point which starts the ConversationVerificationStatusHandler. ConversationVerificationStatusHandler does: observes the epoch flow -> Now some questions:
For example, given clients are added add and then removed again: verified Wouldn't we fail to report that the conversation was unverified for time period? |
Yes, you understood the flow perfectly.
Well, observing these statuses for ALL the users conversations may have tragical app performance decreasing.
True, user will lose "not verified" status in that scenario, but I believe it's not so important, as user was not texting anything to that conversation while those changes happened, so verification status is not so important for him for that period of time. The main goal is to inform user when the conversation he's going to text/call is not secure anymore. |
| @@ -39,7 +39,7 @@ class ProtocolInfoMapperImpl( | |||
| Conversation.ProtocolInfo.MLS.GroupState.valueOf(protocolInfo.groupState.name), | |||
| protocolInfo.epoch, | |||
| protocolInfo.keyingMaterialLastUpdate, | |||
| Conversation.CipherSuite.fromTag(protocolInfo.cipherSuite.cipherSuiteTag) | |||
| Conversation.CipherSuite.fromTag(protocolInfo.cipherSuite.cipherSuiteTag), | |||
There was a problem hiding this comment.
| Conversation.CipherSuite.fromTag(protocolInfo.cipherSuite.cipherSuiteTag), | |
| Conversation.CipherSuite.fromTag(protocolInfo.cipherSuite.cipherSuiteTag) |
| .distinctUntilChanged() | ||
| .onlyRight() | ||
| .mapLatest { newStatus -> | ||
| val currentStatus = conversationRepository.getConversationVerificationStatus(conversationId) |
There was a problem hiding this comment.
question: do we need to call CC anymore? since we update/fetch the state based on the epoch changes? here simply can't we only observe the value in the db?
There was a problem hiding this comment.
I'm not sure if I got your question.
We observe epoch changes, but it only triggers us that something was changed and we need to check if the Verification Status changed (by calling CC).
| @@ -76,7 +78,7 @@ data class ConversationEntity( | |||
| enum class MutedStatus { ALL_ALLOWED, ONLY_MENTIONS_AND_REPLIES_ALLOWED, MENTIONS_MUTED, ALL_MUTED } | |||
|
|
|||
| sealed class ProtocolInfo { | |||
| object Proteus : ProtocolInfo() | |||
| data object Proteus : ProtocolInfo() | |||
There was a problem hiding this comment.
| data object Proteus : ProtocolInfo() | |
| object Proteus : ProtocolInfo() |
But we will never a web-socket event since this all driven by the clients without any knowledge from the server, no? Why do think this would have such a big performance impact? Commits are not arriving constantly in conversations. We don't need a a separate observer for each conversation we can one observe for all conversations.
Ok if that's the only thing we are concerned about. But for proteus verification we also tried to insert the system message at the "right" point time so that the user can see which messages were sent or received when the conversation was in verified/degraded/unverified state. |
|
@typfel Updated the PR, no it run observation in |
| if ((newStatus == VerificationStatus.NOT_VERIFIED && currentStatus == VerificationStatus.DEGRADED) || | ||
| newStatus == currentStatus | ||
| ) { | ||
| return@collect | ||
| } | ||
|
|
||
| if (newStatus == VerificationStatus.NOT_VERIFIED && currentStatus == VerificationStatus.VERIFIED) { | ||
| conversationRepository.updateVerificationStatus(VerificationStatus.DEGRADED, conversation.conversation.id) | ||
| notifyUserAboutStateChanges(conversation.conversation.id, VerificationStatus.DEGRADED) | ||
| } else { | ||
| conversationRepository.updateVerificationStatus(newStatus, conversation.conversation.id) | ||
| } |
There was a problem hiding this comment.
suggestion: extract into private function
borichellow
deleted the
feat/conversation_mls_verification_status_updating
branch
What's new in this PR?
Implementing the machine for calculating the verification state of Conversation.
Issues
CoreCrypto can only "say" if MLS conversation is verified or not, while we need also to indicate when the conversation verification state was degraded (was verified but became not_verified).
Solutions
ConversationVerificationStatusHandlerand call it (start observing the state changes) every time ConversationDetails are observed (by observing that flow inObserveConversationDetailsUseCase)