Skip to content
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

feat: Add device message about outgoing undecryptable messages (#5164) #5176

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Jan 12, 2024

See commit messages.

Close #5164

@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from 4b601d8 to 9d273a4 Compare January 13, 2024 03:32
@iequidoo iequidoo changed the title feat: Add StockMessage::CantDecryptOutgoingMsg (#5164) feat: Add device message about outgoing undecryptable messages (#5164) Jan 13, 2024
@iequidoo iequidoo marked this pull request as ready for review January 13, 2024 04:00
@iequidoo iequidoo requested review from link2xt and r10s January 13, 2024 04:00
@iequidoo
Copy link
Collaborator Author

Currently undecryptable outgoing messages don't go to Trash, i.e. i don't change this logic. Just not sure we can do that safely. Also i'm not sure that adding a notification once per day is sufficient, e.g. if the user tried to fix their multi-device setup, but did smth wrong, they will know about that only on the next day.

@iequidoo
Copy link
Collaborator Author

Maybe add a device message for each received outgoing undecryptable message, but remove also the previous if it's of the same kind? Then we don't need to store the last notification time in the db and don't need this magic 1-day period.

@link2xt
Copy link
Collaborator

link2xt commented Jan 13, 2024

e.g. if the user tried to fix their multi-device setup, but did smth wrong, they will know about that only on the next day

Fixing multi-device setup generally means transferring a backup from the other device, so the counter associated to incorrectly set up account will be removed.

@link2xt
Copy link
Collaborator

link2xt commented Jan 14, 2024

Maybe add a device message for each received outgoing undecryptable message, but remove also the previous if it's of the same kind? Then we don't need to store the last notification time in the db and don't need this magic 1-day period.

Intention of 1-day period is to notify/annoy user at most once a day if they keep sending messages outside interchangeably from two devices. If a new message is added and previous one removed each time like this, user will be notified every time, or never notified if the message is actually replaced.

src/mimeparser.rs Show resolved Hide resolved
src/receive_imf.rs Outdated Show resolved Hide resolved
src/receive_imf.rs Outdated Show resolved Hide resolved
src/receive_imf.rs Outdated Show resolved Hide resolved
src/stock_str.rs Outdated Show resolved Hide resolved
@link2xt
Copy link
Collaborator

link2xt commented Jan 14, 2024

Currently undecryptable outgoing messages don't go to Trash, i.e. i don't change this logic. Just not sure we can do that safely.

What may go wrong if we do this? I can only see that chat assignment may get broken, but this outgoing message would not be assigned to the correct chat as well. If this is a new from-scratch setup (user simply logged in), user likely does not see any existing groups anyway. Old setup may assign messages from new setup, but these messages from new setup are likely not group messages, but simply messages to self or to some manually typed in contacts.

@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch 2 times, most recently from 6576aaa to 7c78bb0 Compare January 14, 2024 19:00
@iequidoo
Copy link
Collaborator Author

What may go wrong if we do this? I can only see that chat assignment may get broken, but this outgoing message would not be assigned to the correct chat as well. If this is a new from-scratch setup (user simply logged in), user likely does not see any existing groups anyway. Old setup may assign messages from new setup, but these messages from new setup are likely not group messages, but simply messages to self or to some manually typed in contacts.

I agree, it's unlikely that subsequent messages chat assignment may get broken, now i send outgoing undecryptables to the Trash. But as for incoming, i left the current logic.

@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from 7c78bb0 to f881b20 Compare January 14, 2024 19:45
@iequidoo iequidoo marked this pull request as draft January 14, 2024 19:46
@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from f881b20 to 0e81709 Compare January 14, 2024 23:56
@link2xt
Copy link
Collaborator

link2xt commented Jan 15, 2024

But as for incoming, i left the current logic.

Yes, the plan is only for outgoing because it is a clear sign that two setups with different keys are present right now (assuming all devices are online) and this should definitely be fixed.

For incoming messages some improvement is needed because current state is that an encrypted group turns into a lot of contact request 1:1 chats as you get messages from all participants, but this is not designed yet.

@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch 2 times, most recently from 18d0fa2 to 0e81709 Compare January 15, 2024 19:20
@iequidoo
Copy link
Collaborator Author

There is a question what to do if an undecryptable outgoing message has unencrypted Message-ID or In-Reply-To that allow assigning it to the proper chat. Would it be useful? F.e. for assigning subsequent messages to the chat. Now test_verified_group_member_added_recovery fails because such messages still go to the group chat. @link2xt

@link2xt
Copy link
Collaborator

link2xt commented Jan 16, 2024

How test_verified_group_member_added_recovery is affected by this? ac2 resetups the device at one point, but then old device is gone so new device should not be able to receive undecryptable outgoing message I guess.

Are you sure the test is broken by this change? It looks like this test somehow got flaky on main already.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jan 16, 2024

Indeed, it's not broken by this change, i just saw that [This message was encrypted for another setup.] messages go to the group chat, but they are incoming, not outgoing. Anyway, if we see an outgoing message that can be assigned to the proper chat by its references in the unencrypted part, maybe keep the current behaviour?

EDIT: At least the user would see that there was some message from their another device, this looks good to me

@link2xt
Copy link
Collaborator

link2xt commented Jan 16, 2024

Anyway, if we see an outgoing message that can be assigned to the proper chat by its references in the unencrypted part, maybe keep the current behaviour?

The goal is to quickly push user to remove this newely set up account and resetup using backup transfer, so assigning square bracket messages to chats does not help and it is anyway unlikely that user will even see these group chats on a newely set up account.

EDIT: At least the user would see that there was some message from their another device, this looks good to me

If there is a new outgoing message it means the user just used this device. Also this would be one more message to decide on and translate in addition to device chat message, because current square bracket message does not fit for outgoing messages.

@iequidoo
Copy link
Collaborator Author

If there is a new outgoing message it means the user just used this device. Also this would be one more message to decide on and translate in addition to device chat message, because current square bracket message does not fit for outgoing messages.

Then a device message should be added, but the original one just may be made hidden. This would be closer to what other users have, because the message doesn't go to Trash for them.

@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from e18cdda to 15d3940 Compare January 16, 2024 19:04
@iequidoo iequidoo marked this pull request as ready for review January 16, 2024 19:15
@iequidoo iequidoo requested a review from link2xt January 16, 2024 23:45
@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch 2 times, most recently from 2c0d487 to e129334 Compare January 26, 2024 18:01
src/stock_str.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from e129334 to 957b274 Compare January 27, 2024 01:00
@iequidoo iequidoo requested a review from r10s January 27, 2024 01:08
@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from 957b274 to b63e71f Compare January 31, 2024 03:39
@iequidoo

This comment was marked as resolved.

@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from b63e71f to 91f52c6 Compare February 8, 2024 02:54
Currently when a user sets up another device by logging in, a new key is created. If a message is
sent from either device outside, it cannot be decrypted by the other device.

The message is replaced with square bracket error like this:
```
<string name="systemmsg_cannot_decrypt">This message cannot be decrypted.\n\n• It might already help to simply reply to this message and ask the sender to send the message again.\n\n• If you just re-installed Delta Chat then it is best if you re-setup Delta Chat now and choose "Add as second device" or import a backup.</string>
```
(taken from Android repo `res/values/strings.xml`)

If the message is outgoing, it does not help to "simply reply to this message". Instead, we should
add a translatable device message of a special type so UI can link to the FAQ entry about second
device. But let's limit such notifications to 1 per day. And as for the undecryptable message
itself, let it go to Trash if it can't be assigned to a chat by its references.
@iequidoo iequidoo force-pushed the iequidoo/outgoing-undecryptable branch from 91f52c6 to 066f832 Compare February 12, 2024 01:30
@iequidoo
Copy link
Collaborator Author

@link2xt do you have an idea why this check hung?

WIP Expected — Waiting for status to be reported

Btw, in #5158 too

@link2xt
Copy link
Collaborator

link2xt commented Feb 12, 2024

@link2xt do you have an idea why this check hung?

WIP Expected — Waiting for status to be reported

Btw, in #5158 too

No idea, but I removed it from required checks now. Maybe someone has added it while in fact it should not be part of required checks because it is not reported when there is no WIP in title.

@iequidoo iequidoo merged commit ba35e83 into main Feb 12, 2024
37 checks passed
@iequidoo iequidoo deleted the iequidoo/outgoing-undecryptable branch February 12, 2024 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add device chat message when a self-sent undecryptable message is sent
3 participants