fix: can_send(): Return false for protected 1:1 chat if contact isn't forward verified (#6222) #6240
Conversation
… forward verified (#6222) Otherwise if the user tries to send a message, that would just fail. `Chat::can_send()` returning false makes the chat input field unavailable. A protected 1:1 chat with a not forward verified contact may be a result of AEAP, the user should use the 1:1 chat with the new contact then.
…n't changed when removing it
iequidoo
force-pushed
the
iequidoo/cant_send-NoForwardVerification
branch
from
1411105 to
7e6e085
Compare
| } else if self.typ == Chattype::Single | ||
| && self.is_protected() | ||
| && match get_chat_contacts(context, self.id).await?.pop() { | ||
| Some(contact_id) => { | ||
| !Contact::get_by_id(context, contact_id) | ||
| .await? | ||
| .is_forward_verified(context) | ||
| .await? | ||
| } | ||
| None => false, | ||
| } | ||
| { | ||
| Some(NoForwardVerification) |
There was a problem hiding this comment.
Performance: This is adding two database calls for can_send() in protected 1:1 chats which is called at least everytime a chat is opened, depending on the UI implementation it might also be multiple times. I didn't measure the performance impact of this, but I assume that it's too big that we would want to accept it.
#6222 is a super niche bug, and it would be even fine not to fix it at all - no one complained during the two years we have AEAP (probably because no one is using it) and AEAP is bound to be replaced soon. We shouldn't sacrifice performance for that. Personally, thinking about it, I tend to just close #6222 and move it to project resurrection, or do a very minimal PR that just makes things work somewhat nicely - e.g. replace the error message with something the user can make sense of (it's fine if it's not translated, many error messages are not).
|
Agreeing with @r10s's suggestion of moving to project resurrection at #6222 (comment) |
Agree, but other two commits (the test and additional check before "unverifying" the old contact) are still useful, will move them to separate PRs |
See commit messages.
Fix #6222
NB: We don't need to move
verified_keytopublic_keyfor the old peerstate, seePeerstate::peek_key()-- the verified key is never used for encrypting in unverified chats, so even ifpublic_keydoesn't exist in the peerstate for some reason, just removingverified_keychanges nothing.