From b138d486e491a62da8ba92a21f65f5ecf588c909 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 19 Feb 2020 13:29:58 +0100 Subject: [PATCH 01/73] two ideas to tackle group consistency --- draft/group-sync.txt | 96 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 draft/group-sync.txt diff --git a/draft/group-sync.txt b/draft/group-sync.txt new file mode 100644 index 000000000..a2773264e --- /dev/null +++ b/draft/group-sync.txt @@ -0,0 +1,96 @@ + +Problem: missing eventual group consistency +-------------------------------------------- + +If group members are concurrently adding new members, +the new members will miss each other's additions, example: + +- Alice and Bob are in a two-member group + +- Alice adds Carol, concurrently Bob adds Doris + +- Carol will see a three-member group (Alice, Bob, Carol), + Doris will see a different three-member group (Alice, Bob, Doris), + and only Alice and Bob will have all four members. + +Note that for verified groups any mitigation mechanism likely +needs to make all clients to know who originally added a member. + +solution: memorize+attach (possible encrypted) chat-meta mime messages +---------------------------------------------------------------------- + +- All Chat-Group-Member-Added/Removed messages are recorded in their + full raw (signed and encrypted) mime-format in the DB + +- If an incoming member-add/member-delete messages has a member list + which is, apart from the added/removed member, not consistent + with our own view, send a "Chat-Group-Member-Correction" message out, + attaching the original added/removed mime-message for all mismatching contacts. + +- When receiving added/removed attachments don't do the + check_consistency+correction message dance. This avoids recursion problems + and hard-to-reason-about chatter. + +Notes: + +- mechanism works for both encrypted and unencrypted add/del messages + +- we already have a "mime_headers" column in the DB for each incoming message. + We could extend it to also include the payload and store mime unconditionally + for member-added/removed messages. + +- multiple member-added/removed messages can be attached in a single message + +- is minimal on the number of overall messages to reach group consistency + (best-case: no extra messages, the ABCD case above: max two extra messages) + +- somewhat backward compatible: older clients will probably ignore + messages which are signed by someone who is not the outer From-address. + +- we can quite easily extend the mechanism to also provide the group-avatar or + other meta-information. + + + + +solution2: repeat member-added/removed messages +--------------------------------------------------- + +Introduce a new Chat-Group-Member-Changed header and deprecate Chat-Group-Member-Added/Removed +but keep sending out the old headers until the new protocol is sufficiently deployed. + +The new Chat-Group-Member-Changed header contains a Time-to-Live number (TTL) +which controls repetition of the signed "add/del e-mail address" payload. + +Example:: + + Chat-Group-Member-Changed: TTL add "somedisplayname" someone@example.org + owEBYQGe/pANAwACAY47A6J5t3LWAcsxYgBeTQypYWRkICJzb21lZGlzcGxheW5h + bWUiIHNvbWVvbmVAZXhhbXBsZS5vcmcgCokBHAQAAQIABgUCXk0MqQAKCRCOOwOi + ebdy1hfRB/wJ74tgFQulicthcv9n+ZsqzwOtBKMEVIHqJCzzDB/Hg/2z8ogYoZNR + iUKKrv3Y1XuFvdKyOC+wC/unXAWKFHYzY6Tv6qDp6r+amt+ad+8Z02q53h9E55IP + FUBdq2rbS8hLGjQB+mVRowYrUACrOqGgNbXMZjQfuV7fSc7y813OsCQgi3tjstup + b+uduVzxCp3PChGhcZPs3iOGCnQvSB8VAaLGMWE2d7nTo/yMQ0Jx69x5qwfXogTk + mTt5rOJyrosbtf09TMKFzGgtqBcEqHLp3+mQpZQ+WHUKAbsRa8Jc9DOUOSKJ8SNM + clKdskprY+4LY0EBwLD3SQ7dPkTITCRD + =P6GG + +TTL is set to "2" on an initial Chat-Group-Member-Changed add/del message. +Receivers will apply the add/del change to the group-membership, +decrease the TTL by 1, and if TTL>0 re-sent the header. + +The "add|del e-mail address" payload is pgp-signed and repeated verbatim. +This allows to propagate, in a cryptographically secured way, +who added a member. This is particularly important for allowing +to show in verified groups who added a member (planned). + +Disadvantage to solution 1: + +- requires to specify encoding and precise rules for what/how is signed. + +- causes O(N^2) extra messages + +- Not easily extendable for other things (without introducing a new + header / encoding) + + From 702c7382a7bfdd42d03ea36f3524d4ad64785b33 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 19 Feb 2020 13:35:07 +0100 Subject: [PATCH 02/73] move --- draft/{group-sync.txt => group-sync.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename draft/{group-sync.txt => group-sync.rst} (100%) diff --git a/draft/group-sync.txt b/draft/group-sync.rst similarity index 100% rename from draft/group-sync.txt rename to draft/group-sync.rst From ffe3c84e7c1d74a45558eff6198b6dc468611be5 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 19 Feb 2020 13:39:33 +0100 Subject: [PATCH 03/73] small refinements --- draft/group-sync.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/draft/group-sync.rst b/draft/group-sync.rst index a2773264e..c43de2ae0 100644 --- a/draft/group-sync.rst +++ b/draft/group-sync.rst @@ -26,8 +26,9 @@ solution: memorize+attach (possible encrypted) chat-meta mime messages which is, apart from the added/removed member, not consistent with our own view, send a "Chat-Group-Member-Correction" message out, attaching the original added/removed mime-message for all mismatching contacts. + If we have no relevant add/del information, don't send a correction message out. -- When receiving added/removed attachments don't do the +- Upong receiving added/removed attachments we don't do the check_consistency+correction message dance. This avoids recursion problems and hard-to-reason-about chatter. @@ -39,9 +40,10 @@ Notes: We could extend it to also include the payload and store mime unconditionally for member-added/removed messages. -- multiple member-added/removed messages can be attached in a single message +- multiple member-added/removed messages can be attached in a single + correction message -- is minimal on the number of overall messages to reach group consistency +- it is minimal on the number of overall messages to reach group consistency (best-case: no extra messages, the ABCD case above: max two extra messages) - somewhat backward compatible: older clients will probably ignore From d1a26e66a7138b67fb74058f70a703438597dd7d Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 19 Feb 2020 13:48:05 +0100 Subject: [PATCH 04/73] update --- draft/group-sync.rst | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/draft/group-sync.rst b/draft/group-sync.rst index c43de2ae0..968272667 100644 --- a/draft/group-sync.rst +++ b/draft/group-sync.rst @@ -16,9 +16,13 @@ the new members will miss each other's additions, example: Note that for verified groups any mitigation mechanism likely needs to make all clients to know who originally added a member. + solution: memorize+attach (possible encrypted) chat-meta mime messages ---------------------------------------------------------------------- +For reference, please see https://github.com/deltachat/deltachat-core-rust/blob/master/spec.md#add-and-remove-members how MemberAdded/Removed messages are shaped. + + - All Chat-Group-Member-Added/Removed messages are recorded in their full raw (signed and encrypted) mime-format in the DB @@ -29,8 +33,8 @@ solution: memorize+attach (possible encrypted) chat-meta mime messages If we have no relevant add/del information, don't send a correction message out. - Upong receiving added/removed attachments we don't do the - check_consistency+correction message dance. This avoids recursion problems - and hard-to-reason-about chatter. + check_consistency+correction message dance. + This avoids recursion problems and hard-to-reason-about chatter. Notes: From 13e361aabc1921f03d276e1f34cc2aa0bf18bebf Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 19 Feb 2020 14:20:07 +0100 Subject: [PATCH 05/73] add two variants --- draft/group-sync.rst | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/draft/group-sync.rst b/draft/group-sync.rst index 968272667..286d19cb6 100644 --- a/draft/group-sync.rst +++ b/draft/group-sync.rst @@ -28,9 +28,10 @@ For reference, please see https://github.com/deltachat/deltachat-core-rust/blob/ - If an incoming member-add/member-delete messages has a member list which is, apart from the added/removed member, not consistent - with our own view, send a "Chat-Group-Member-Correction" message out, - attaching the original added/removed mime-message for all mismatching contacts. - If we have no relevant add/del information, don't send a correction message out. + with our own view, broadcast a "Chat-Group-Member-Correction" message to + all members, attaching the original added/removed mime-message for all mismatching + contacts. If we have no relevant add/del information, don't send a + correction message out. - Upong receiving added/removed attachments we don't do the check_consistency+correction message dance. @@ -53,10 +54,33 @@ Notes: - somewhat backward compatible: older clients will probably ignore messages which are signed by someone who is not the outer From-address. +- the correction-protocol also helps with dropped messages. If a member + did not see a member-added/removed message, the next member add/removed + message in the group will likely heal group consistency for this member. + - we can quite easily extend the mechanism to also provide the group-avatar or other meta-information. +Discussions of variants +++++++++++++++++++++++++ +- instead of acting on MemberAdded/Removed message we could send + corrections for any received message that addresses inconsistent group members but + a) this would delay group-membership healing + b) could lead to a lot of members sending corrections + +- instead of broadcasting correction messages we could only send it to + the sender of the inconsistent member-added/removed message. + A receiver of such a correction message would then need to forward + the message to the members it thinks also have an inconsistent view. + This sounds complicated and error-prone. Concretely, if Alice + receives Bob's "Member-added: Doris" message, then Alice + broadcasting the correction message with "Member-added: Carol" + would reach all four members, healing group consistency in one step. + If Bob meanwhile receives Alice's "Member-Added: Carol" message, + Bob would broadcast a correction message to all four members as well. + (Imagine a situation where Alice/Bob added Carol/Doris + while both being in an offline or bad-connection situation). solution2: repeat member-added/removed messages From 7c39bb66597f32c5a6b9ea0410b97c57a710fadd Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Thu, 27 Feb 2020 13:37:46 +0100 Subject: [PATCH 06/73] ignore handshake messages seen from another device moreover, prepare for marking peers as verified accross devices, see detailed comment for observe_securejoin_on_other_device() --- src/dc_receive_imf.rs | 32 +++++++++++++++++++++++++------- src/securejoin.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index d65107540..539aa5224 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -17,7 +17,7 @@ use crate::message::{self, MessageState, MessengerMessage, MsgId}; use crate::mimeparser::*; use crate::param::*; use crate::peerstate::*; -use crate::securejoin::{self, handle_securejoin_handshake}; +use crate::securejoin::{self, handle_securejoin_handshake, observe_securejoin_on_other_device}; use crate::sql; use crate::stock::StockMessage; use crate::{contact, location}; @@ -340,11 +340,9 @@ fn add_parts( }; to_id = DC_CONTACT_ID_SELF; - // handshake messages must be processed _before_ chats are created - // (eg. contacs may be marked as verified) + // handshake may mark contacts as verified and must be processed before chats are created if mime_parser.get(HeaderDef::SecureJoin).is_some() { - // avoid discarding by show_emails setting - msgrmsg = MessengerMessage::Yes; + msgrmsg = MessengerMessage::Yes; // avoid discarding by show_emails setting *chat_id = ChatId::new(0); allow_creation = true; match handle_securejoin_handshake(context, mime_parser, from_id) { @@ -358,8 +356,7 @@ fn add_parts( state = MessageState::InSeen; } Ok(securejoin::HandshakeMessage::Propagate) => { - // Message will still be processed as "member - // added" or similar system message. + // process messages as "member added" normally } Err(err) => { *hidden = true; @@ -472,6 +469,27 @@ fn add_parts( // We cannot recreate other states (read, error). state = MessageState::OutDelivered; to_id = to_ids.get_index(0).cloned().unwrap_or_default(); + + // handshake may mark contacts as verified and must be processed before chats are created + if mime_parser.get(HeaderDef::SecureJoin).is_some() { + msgrmsg = MessengerMessage::Yes; // avoid discarding by show_emails setting + *chat_id = ChatId::new(0); + allow_creation = true; + match observe_securejoin_on_other_device(context, mime_parser, to_id) { + Ok(securejoin::HandshakeMessage::Done) + | Ok(securejoin::HandshakeMessage::Ignore) => { + *hidden = true; + } + Ok(securejoin::HandshakeMessage::Propagate) => { + // process messages as "member added" normally + } + Err(err) => { + *hidden = true; + error!(context, "Error in Secure-Join watching: {}", err); + } + } + } + if !to_ids.is_empty() { if chat_id.is_unset() { let (new_chat_id, new_chat_id_blocked) = create_or_lookup_group( diff --git a/src/securejoin.rs b/src/securejoin.rs index 28a8f0f7f..d6fce24a7 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -770,6 +770,38 @@ pub(crate) fn handle_securejoin_handshake( } } +/// observe_securejoin_on_other_device() must be called when a self-sent securejoin message is seen. +/// currently, the message is only ignored, in the future, +/// we may mark peers as verified accross devices: +/// +/// in a multi-device-setup, there may be other devices that "see" the handshake messages. +/// if the seen messages seen are self-sent messages encrypted+signed correctly with our key, +/// we can make some conclusions of it: +/// +/// - if we see the self-sent-message vg-member-added/vc-contact-confirm, +/// we know that we're an inviter-observer. +/// the inviting device has marked a peer as verified on vg-request-with-auth/vc-request-with-auth +/// before sending vg-member-added/vc-contact-confirm - so, if we observe vg-member-added/vc-contact-confirm, +/// we can mark the peer as verified as well. +/// +/// - if we see the self-sent-message vg-member-added-received +/// we know that we're an joiner-observer. +/// the joining device has marked the peer as verified on vg-member-added/vc-contact-confirm +/// before sending vg-member-added-received - so, if we observe vg-member-added-received, +/// we can mark the peer as verified as well. +/// +/// to make this work, (a) some messages must not be deleted, +/// (b) we need a vc-contact-confirm-received message if bcc_self is set, +/// (c) we should make sure, we do not only rely on the unencrypted To:-header for identifying the peer +/// (in handle_securejoin_handshake() we have the oob information for that) +pub(crate) fn observe_securejoin_on_other_device( + _context: &Context, + _mime_message: &MimeMessage, + _contact_id: u32, +) -> Result { + Ok(HandshakeMessage::Ignore) +} + fn secure_connection_established(context: &Context, contact_chat_id: ChatId) { let contact_id: u32 = chat_id_2_contact_id(context, contact_chat_id); let contact = Contact::get_by_id(context, contact_id); From 818c20e0cbe25e7180aa580be929e9cd43e87a59 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Fri, 27 Mar 2020 19:56:26 +0100 Subject: [PATCH 07/73] switch to ecc keys after fixing some issues wrt ecc keys, see #1319, and waiting some time (three core releases, two ios/android/desktop releases), it is now the time to switch again to ecc keys again, after the first attempt was stopped in #1319 --- src/pgp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pgp.rs b/src/pgp.rs index e4f1534f9..64d64e095 100644 --- a/src/pgp.rs +++ b/src/pgp.rs @@ -153,8 +153,8 @@ pub(crate) fn create_keypair( keygen_type: KeyGenType, ) -> std::result::Result { let (secret_key_type, public_key_type) = match keygen_type { - KeyGenType::Rsa2048 | KeyGenType::Default => (PgpKeyType::Rsa(2048), PgpKeyType::Rsa(2048)), - KeyGenType::Ed25519 => (PgpKeyType::EdDSA, PgpKeyType::ECDH), + KeyGenType::Rsa2048 => (PgpKeyType::Rsa(2048), PgpKeyType::Rsa(2048)), + KeyGenType::Ed25519 | KeyGenType::Default => (PgpKeyType::EdDSA, PgpKeyType::ECDH), }; let user_id = format!("<{}>", addr); From a7afbf85ad09ca58046fe095d142e12adc55c03d Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Fri, 28 Feb 2020 05:17:40 +0300 Subject: [PATCH 08/73] Replace test Alice key with ed25519 key Autocrypt Setup Message does not contain "==" anymore since key length has changed. --- src/imex.rs | 1 - test-data/key/alice-public.asc | 2 +- test-data/key/alice-secret.asc | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/imex.rs b/src/imex.rs index ce603961e..f27219e94 100644 --- a/src/imex.rs +++ b/src/imex.rs @@ -756,7 +756,6 @@ mod tests { assert!(msg.contains("-----BEGIN PGP MESSAGE-----\r\n")); assert!(msg.contains("Passphrase-Format: numeric9x4\r\n")); assert!(msg.contains("Passphrase-Begin: he\n")); - assert!(msg.contains("==\n")); assert!(msg.contains("-----END PGP MESSAGE-----\n")); } diff --git a/test-data/key/alice-public.asc b/test-data/key/alice-public.asc index 7ee921bc8..dd9998ad7 100644 --- a/test-data/key/alice-public.asc +++ b/test-data/key/alice-public.asc @@ -1 +1 @@ -xsBNBF425W8BCADLIbltPzG1vk/V2ov2+eBeJJJnRu1kJHdo6e3oNB+HTIxVde5+7Uq8tTEDZB1O7m9NBUFrXr7UYQsA/86G2jmsyWKTzIu1O/t5kdcNDqsNcTVZAhBu2ixYsYVc3ws6kJONjpXLtD2u3P7vEXU3INiOb2JrBQDT8/ubEm1xas/UirYnP5DMaH068IHRdVEYs9ULFaD5scw1m/94buXYZ1CRt/2hT8iRrtBi6ki8kArnhsZC2Xr0+jRQNMUnG5k7Bwi6saCqVmd7IlqSM6MbfYank30Gi/UyDmyIrOk7daTg6WIqgiVOTHav65EK/aUvvjlr+awM+C+u35rQytzyTitZABEBAAHNEzxhbGljZUBleGFtcGxlLmNvbT7CwIkEEAEIADMCGQEFAl425ZQCGwMECwkIBwYVCAkKCwIDFgIBFiEEsBJRVVptIGB7DRLzYuJiDHjRb8EACgkQYuJiDHjRb8HiZQf+PLDxzWchkHAdQFbxxtoXj66aiknofjlRWHDWvUG4nULZ15tjDjnv3z22Meldr8kSV4r1+ejhLFHou9gTzAYk7eAxiybDd8AJOdK+ZgK/Nn7xjdO+HTZLhNdi+R7EektDyf8WDNktEaS8pZc74VKu4984ESi4PoqVxqGHRiSisH4cw4b2pQYxp32BkIdil7sWnqRUEoCpMoKdw2h0N7/lm+rS7/JR9cdjXaVzy1dYTqAVsTL1FTGy4osOKGOyQbkP+Cm6uNq7kC/Bt+fefsb+c2JycmI1uwdvnG7PoFslKv3lRnfkNSmrcIYlJHUl5z0yAgliophr5fqMfzQpO4zMc87ATQReNuVvAQgAuNjE1i+g4v25UNDPIMgXODU4WztE30074gQs5sZa0DQnDUMsdWc2g1o060YZDojMYJQAtBjlW1Dz8FEE7WsLNohGtRyUWmIgNxE5CpodjpwIZ0MdO4Aji0YM+g+WsOSS8kiHMs+dMFfQJuNKjujGFaMIciSaMMrUmPtzkQ/o8NEJs2Aftw90fpVR+M7Mue3++rcEX09ntbgqkgm8SV6OIrOY2kfILudtybocgYkCTeNVqz5VFXuxrnT4ceyFQ64JkwsZxb+X/pCm4V5Q2TbKRwtdonU8HfAz0nAd5tsNeGmf/dPLOKBCxlNEme399YmzWrT+kJBp7CIH5jlWQKyuLwARAQABwsB2BBgBCAAgBQJeNuWUAhsMFiEEsBJRVVptIGB7DRLzYuJiDHjRb8EACgkQYuJiDHjRb8HrEgf/Xu8eRPPdskwtyd98y64teidBpkHuIjuZKJpNyy2HhdGXQwYbNIzwINg0EJ+u2nkreNF/h2Lu+/saqI8Dai02dpYXjvxJIlCgP2os7sNhVaZSaS4XmmJjkHCfZuIKblZypKDJVc5AceZxrtvUbgG+94+H3zeRWVAA30S5ep6YPvxigvhmQah/sdzY7708/jd9uXcCbkP47PBaXCpuPiYLb3t7z8mOteJb7LOZUmSI1efiLDLTGj7ofkdDfA7E6/nF/1+nq+UIDWqljwiUzeNIJsFlZRa/9/uDEjcQbaDe9/knBs7k9pEDZX5u8SSwSED75L+OvRpFWenp4SSKvd2BUw== \ No newline at end of file +mDMEXlh13RYJKwYBBAHaRw8BAQdAzfVIAleCXMJrq8VeLlEVof6ITCviMktKjmcBKAu4m5C0GUFsaWNlIDxhbGljZUBleGFtcGxlLm9yZz6IkAQTFggAOBYhBC5vossjtTLXKGNLWGSwj2Gp7ZRDBQJeWHXdAhsDBQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEGSwj2Gp7ZRDE3oA/i4MCyDMTsjWqDZoQwX/A/GoTO2/V0wKPhjJJy/8m2pMAPkBjOnGOtx2SZpQvJGTa9h804RY6iDrRuI8A/8tEEXAA7g4BF5Ydd0SCisGAQQBl1UBBQEBB0AG7cjWy2SFAU8KnltlubVW67rFiyfp01JrRe6Xqy22HQMBCAeIeAQYFggAIBYhBC5vossjtTLXKGNLWGSwj2Gp7ZRDBQJeWHXdAhsMAAoJEGSwj2Gp7ZRDLo8BAObE8GnsGVwKzNqCvHeWgJsqhjS3C6gvSlV3tEm9XmF6AQDXucIyVfoBwoyMh2h6cSn/ATn5QJb35pgo+ivp3jsMAg== \ No newline at end of file diff --git a/test-data/key/alice-secret.asc b/test-data/key/alice-secret.asc index b09800bc5..ccc7e0f64 100644 --- a/test-data/key/alice-secret.asc +++ b/test-data/key/alice-secret.asc @@ -1 +1 @@ -xcLYBF425W8BCADLIbltPzG1vk/V2ov2+eBeJJJnRu1kJHdo6e3oNB+HTIxVde5+7Uq8tTEDZB1O7m9NBUFrXr7UYQsA/86G2jmsyWKTzIu1O/t5kdcNDqsNcTVZAhBu2ixYsYVc3ws6kJONjpXLtD2u3P7vEXU3INiOb2JrBQDT8/ubEm1xas/UirYnP5DMaH068IHRdVEYs9ULFaD5scw1m/94buXYZ1CRt/2hT8iRrtBi6ki8kArnhsZC2Xr0+jRQNMUnG5k7Bwi6saCqVmd7IlqSM6MbfYank30Gi/UyDmyIrOk7daTg6WIqgiVOTHav65EK/aUvvjlr+awM+C+u35rQytzyTitZABEBAAEAB/oDQFnwdrd7+jza5nGhFWTS/PDe+FKqbK8AneXx9ouepcoFQCr+Gxw8IwZS0JJrhgOADxp59n1FdvwvGukaXXnY2yxZw0dlMj2XN49ipR51y58X+qF6tMFK9iR1VRif6lqCRIr/RLZMCzuFZhkjNcJhnUTNA7p8qgYX+FaKHzSOaVat/v0kIUHUcZDkREWPUESYDmc1Nv6FXhB0WBiTsBglF+fq5Rm7UWPSmA59Cr7BrW8DctbzTh0+6bkzum2xdOcZ59nuTZa+IKcReI1+kVne5JPNFNJ2tP2f9GSSlL7u+NBtx3zRxZgAotXcJK9cVNIWtegqf+2hoLvm7m2CkWKRBADglpC7TpjV+8wJH+KuyGQ7jepqzf5EHwMrK2i6lPnnmoi0nkKvkklvtdcC7FoFGtLCDJ7vwlUdeN+itDxPlP8bbbUabcy0lLuzyGOVt5NwYXgIuPicpdt2ZTJgvChd9oWi1DG8pVpm+EMJZPyYVEpvDGl6q95oktrytbqjASZbBQQA54roJnwBcptLMTrttDrglULX7ciSKY5HXN1c0rqZn1dTKB1nPYB26hNbu6lZ8ixSOyZm3KwpeDUNW7A3hyzXOfoGFPaddH6WMSFFsGGC/orRVxnuPZLr3UJ3uFX7J0JOav90n/6A4YmS7uImRAG4/vTrAbEfmlBl5msHVUaYh0UD/jSX22JLenO1o8pNU04JQl3lQ4mWY6MvgTyCvpchTzDDva+wdOBTUeVUmb/KqYkYBq98tXl1VnGnNpeEymUISSi60RjaXDhbg7a3ELV0yvvWcBN9zreyyINuCU5OmNefPRvPt4Co12KtIxPACByFNTevzPKbrXd1cyhHOxAuqfzLRbDNEzxhbGljZUBleGFtcGxlLmNvbT7CwIkEEAEIADMCGQEFAl425ZQCGwMECwkIBwYVCAkKCwIDFgIBFiEEsBJRVVptIGB7DRLzYuJiDHjRb8EACgkQYuJiDHjRb8HiZQf+PLDxzWchkHAdQFbxxtoXj66aiknofjlRWHDWvUG4nULZ15tjDjnv3z22Meldr8kSV4r1+ejhLFHou9gTzAYk7eAxiybDd8AJOdK+ZgK/Nn7xjdO+HTZLhNdi+R7EektDyf8WDNktEaS8pZc74VKu4984ESi4PoqVxqGHRiSisH4cw4b2pQYxp32BkIdil7sWnqRUEoCpMoKdw2h0N7/lm+rS7/JR9cdjXaVzy1dYTqAVsTL1FTGy4osOKGOyQbkP+Cm6uNq7kC/Bt+fefsb+c2JycmI1uwdvnG7PoFslKv3lRnfkNSmrcIYlJHUl5z0yAgliophr5fqMfzQpO4zMc8fC2AReNuVvAQgAuNjE1i+g4v25UNDPIMgXODU4WztE30074gQs5sZa0DQnDUMsdWc2g1o060YZDojMYJQAtBjlW1Dz8FEE7WsLNohGtRyUWmIgNxE5CpodjpwIZ0MdO4Aji0YM+g+WsOSS8kiHMs+dMFfQJuNKjujGFaMIciSaMMrUmPtzkQ/o8NEJs2Aftw90fpVR+M7Mue3++rcEX09ntbgqkgm8SV6OIrOY2kfILudtybocgYkCTeNVqz5VFXuxrnT4ceyFQ64JkwsZxb+X/pCm4V5Q2TbKRwtdonU8HfAz0nAd5tsNeGmf/dPLOKBCxlNEme399YmzWrT+kJBp7CIH5jlWQKyuLwARAQABAAf/YmpfWp5fLZvjJ8kVDqIZ4r5LNB+5Sp7nbC3G7lPblBDAXgpOyG9ckdDcbguTWa6yChWizkCXFOhkCKZKVlHw1Wb3JoSB5CFsf4U29pMZe41N2BTeoohV5Fg2nojgNWxtZHwDJ6VsTonidGH9l1sN5AU6gPNF+QZ07MKsRCbRYi0yMgX064gwZXRtkm8AECz8ay1wDzoBy14ALe9aDClafVwfxdYUcxDBqtvjLhGeTWX5lMMAQ1Ix8D0Gp4r0Zvtl+oxlTSZFAt9m6sbRBbJf4LJjRQh07aWF2gUOiyIyz7YymYdwsyFnCPn2Aj84uRdqYCekAUfzBeNTBukUQq1DYQQA3BeH38pnr34m0UyD/tCrTvrX60MOJVvFuaTQw+IgY4XmT9UiiiqYMaoLfzxeevdMCQ9EtMdXUTjI27/II3dR5Obg6J0QTybj78IKPbH8Vdlg0etllRjC3bV/M4a5UcXPKG6W5CvB0UJg7eqn/8wUqwiL9x+hZoLy+nU5rCAjzZEEANcBK8Vy9eBxkKmEfH/mChDSKE82ua0xdQuZiTvvGedUYG3ucH4rAlkZaZcZrtJTod1BKhAhDBrjxk/yLCjK3z5JDsacdDGGfaqga3zdPBJubWE7f4mg6uYVs04Uf90YVY7t0LEQAh7i9QYiIqUOJDy3L8y3+bNgNz2r1p8pFd+/A/0YoYE0YDgABbLKmBQFoWjF3Op7P+k6Z4ENK4Me1fkNSAU451QX7ZduI7i3pGTM06bXG2umhTI1lg48ZveMRk1vBezHU+ThnciEkuhYafnq7NRdkEtI20MyFmN7dZF8LQ/joYKsJbeSG5svj8f1ue2eHkiIIlTtDqVUTizDU3ddlzUZwsB2BBgBCAAgBQJeNuWUAhsMFiEEsBJRVVptIGB7DRLzYuJiDHjRb8EACgkQYuJiDHjRb8HrEgf/Xu8eRPPdskwtyd98y64teidBpkHuIjuZKJpNyy2HhdGXQwYbNIzwINg0EJ+u2nkreNF/h2Lu+/saqI8Dai02dpYXjvxJIlCgP2os7sNhVaZSaS4XmmJjkHCfZuIKblZypKDJVc5AceZxrtvUbgG+94+H3zeRWVAA30S5ep6YPvxigvhmQah/sdzY7708/jd9uXcCbkP47PBaXCpuPiYLb3t7z8mOteJb7LOZUmSI1efiLDLTGj7ofkdDfA7E6/nF/1+nq+UIDWqljwiUzeNIJsFlZRa/9/uDEjcQbaDe9/knBs7k9pEDZX5u8SSwSED75L+OvRpFWenp4SSKvd2BUw== \ No newline at end of file +lFgEXlh13RYJKwYBBAHaRw8BAQdAzfVIAleCXMJrq8VeLlEVof6ITCviMktKjmcBKAu4m5AAAQDMpCY4sD5/DUR0jRjGC5WstwShz1q+5Vofo5mY9+XRXRA3tBlBbGljZSA8YWxpY2VAZXhhbXBsZS5vcmc+iJAEExYIADgWIQQub6LLI7Uy1yhjS1hksI9hqe2UQwUCXlh13QIbAwULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAKCRBksI9hqe2UQxN6AP4uDAsgzE7I1qg2aEMF/wPxqEztv1dMCj4YyScv/JtqTAD5AYzpxjrcdkmaULyRk2vYfNOEWOog60biPAP/LRBFwAOcXQReWHXdEgorBgEEAZdVAQUBAQdABu3I1stkhQFPCp5bZbm1Vuu6xYsn6dNSa0Xul6stth0DAQgHAAD/X9y9I/JFBeArkgR3U363cWXXxMCWftS+BDwM9zE4PrgQb4h4BBgWCAAgFiEELm+iyyO1MtcoY0tYZLCPYantlEMFAl5Ydd0CGwwACgkQZLCPYantlEMujwEA5sTwaewZXArM2oK8d5aAmyqGNLcLqC9KVXe0Sb1eYXoBANe5wjJV+gHCjIyHaHpxKf8BOflAlvfmmCj6K+neOwwC \ No newline at end of file From 8aa4ceb5703101bda2cdf485b78b0ed07f002784 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 30 Mar 2020 15:59:07 +0200 Subject: [PATCH 09/73] hide the device-chat from default forward-to-lists --- src/chatlist.rs | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/chatlist.rs b/src/chatlist.rs index 84491ce2d..cc58cb5f1 100644 --- a/src/chatlist.rs +++ b/src/chatlist.rs @@ -74,7 +74,8 @@ impl Chatlist { /// if DC_GCL_ARCHIVED_ONLY is not set, only unarchived chats are returned and /// the pseudo-chat DC_CHAT_ID_ARCHIVED_LINK is added if there are *any* archived /// chats - /// - the flag DC_GCL_FOR_FORWARDING sorts "Saved messages" to the top of the chatlist, + /// - the flag DC_GCL_FOR_FORWARDING sorts "Saved messages" to the top of the chatlist + /// and hides the device-chat, // typically used on forwarding, may be combined with DC_GCL_NO_SPECIALS /// - if the flag DC_GCL_NO_SPECIALS is set, deaddrop and archive link are not added /// to the list (may be used eg. for selecting chats on forwarding, the flag is @@ -104,6 +105,14 @@ impl Chatlist { .map_err(Into::into) }; + let skip_id = if 0 != listflags & DC_GCL_FOR_FORWARDING { + chat::lookup_by_contact_id(context, DC_CONTACT_ID_DEVICE) + .unwrap_or_default() + .0 + } else { + ChatId::new(0) + }; + // select with left join and minimum: // // - the inner select must use `hidden` and _not_ `m.hidden` @@ -142,6 +151,9 @@ impl Chatlist { )? } else if 0 != listflags & DC_GCL_ARCHIVED_ONLY { // show archived chats + // (this includes the archived device-chat; we could skip it, + // however, then the number of archived chats do not match, which might be even more irritating. + // and adapting the number requires larger refactorings and seems not to be worth the effort) context.sql.query_map( "SELECT c.id, m.id FROM chats c @@ -181,13 +193,13 @@ impl Chatlist { SELECT MAX(timestamp) FROM msgs WHERE chat_id=c.id - AND (hidden=0 OR state=?)) - WHERE c.id>9 + AND (hidden=0 OR state=?1)) + WHERE c.id>9 AND c.id!=?2 AND c.blocked=0 - AND c.name LIKE ? + AND c.name LIKE ?3 GROUP BY c.id ORDER BY IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", - params![MessageState::OutDraft, str_like_cmd], + params![MessageState::OutDraft, skip_id, str_like_cmd], process_row, process_rows, )? @@ -210,12 +222,12 @@ impl Chatlist { FROM msgs WHERE chat_id=c.id AND (hidden=0 OR state=?1)) - WHERE c.id>9 + WHERE c.id>9 AND c.id!=?2 AND c.blocked=0 - AND NOT c.archived=?2 + AND NOT c.archived=?3 GROUP BY c.id - ORDER BY c.id=?3 DESC, c.archived=?4 DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", - params![MessageState::OutDraft, ChatVisibility::Archived, sort_id_up, ChatVisibility::Pinned], + ORDER BY c.id=?4 DESC, c.archived=?5 DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", + params![MessageState::OutDraft, skip_id, ChatVisibility::Archived, sort_id_up, ChatVisibility::Pinned], process_row, process_rows, )?; @@ -415,13 +427,16 @@ mod tests { fn test_sort_self_talk_up_on_forward() { let t = dummy_context(); t.ctx.update_device_chats().unwrap(); + create_group_chat(&t.ctx, VerifiedStatus::Unverified, "a chat").unwrap(); let chats = Chatlist::try_load(&t.ctx, 0, None, None).unwrap(); - assert!(Chat::load_from_db(&t.ctx, chats.get_chat_id(0)) + assert!(chats.len() == 3); + assert!(!Chat::load_from_db(&t.ctx, chats.get_chat_id(0)) .unwrap() - .is_device_talk()); + .is_self_talk()); let chats = Chatlist::try_load(&t.ctx, DC_GCL_FOR_FORWARDING, None, None).unwrap(); + assert!(chats.len() == 2); // device chat cannot be written and is skipped on forwarding assert!(Chat::load_from_db(&t.ctx, chats.get_chat_id(0)) .unwrap() .is_self_talk()); From 50569f12f537fbfeb55d1a962844fdc18ba94846 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sun, 29 Mar 2020 19:53:51 +0200 Subject: [PATCH 10/73] Remove unsafe CString::yolo from ffi CString::yolo was still used in the ffi, this was an unsafe transitional thing. To remove it there were two choices: 1. make errors in creating CStrings hard errors or 2. try and be as lenient as possible. Given the to_string_lossy() convention adopted in the ffi this choose the lenient option and simply skips over embedded null bytes, leaving the rest of the strings intact. Thus now CString::new_lossy(). It's only used for .strdup() however so no longer a public trait. This also cleans up the public visibility of things in the strings.rs file: - Rename StrExt/OptStrExt traits to what they actually do: provide .strdup() -> Strdup/OptStrdup. - dc_strdup() should be an implementation detail, replace all usages with Strdup.strdup() method. - Only allow visibility inside the crate for all things. - Reduce visibility to only the module for things not used in lib.rs. --- deltachat-ffi/src/lib.rs | 27 +++++++------- deltachat-ffi/src/string.rs | 74 ++++++++++++++++++++++--------------- tests/stress.rs | 4 +- 3 files changed, 60 insertions(+), 45 deletions(-) diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index d6cbd09b1..a7ccca1be 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -433,7 +433,7 @@ pub unsafe extern "C" fn dc_set_config_from_qr( pub unsafe extern "C" fn dc_get_info(context: *mut dc_context_t) -> *mut libc::c_char { if context.is_null() { eprintln!("ignoring careless call to dc_get_info()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_context = &*context; let guard = ffi_context.inner.read().unwrap(); @@ -1455,7 +1455,7 @@ pub unsafe extern "C" fn dc_get_msg_info( ) -> *mut libc::c_char { if context.is_null() { eprintln!("ignoring careless call to dc_get_msg_info()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_context = &*context; ffi_context @@ -2406,7 +2406,7 @@ pub unsafe extern "C" fn dc_chat_get_type(chat: *mut dc_chat_t) -> libc::c_int { pub unsafe extern "C" fn dc_chat_get_name(chat: *mut dc_chat_t) -> *mut libc::c_char { if chat.is_null() { eprintln!("ignoring careless call to dc_chat_get_name()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_chat = &*chat; ffi_chat.chat.get_name().strdup() @@ -2728,7 +2728,7 @@ pub unsafe extern "C" fn dc_msg_get_sort_timestamp(msg: *mut dc_msg_t) -> i64 { pub unsafe extern "C" fn dc_msg_get_text(msg: *mut dc_msg_t) -> *mut libc::c_char { if msg.is_null() { eprintln!("ignoring careless call to dc_msg_get_text()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_msg = &*msg; ffi_msg.message.get_text().unwrap_or_default().strdup() @@ -2747,8 +2747,7 @@ pub unsafe extern "C" fn dc_msg_get_file(msg: *mut dc_msg_t) -> *mut libc::c_cha ffi_msg .message .get_file(ctx) - .and_then(|p| p.to_c_string().ok()) - .map(|cs| dc_strdup(cs.as_ptr())) + .map(|p| p.strdup()) .unwrap_or_else(|| "".strdup()) }) .unwrap_or_else(|_| "".strdup()) @@ -2758,7 +2757,7 @@ pub unsafe extern "C" fn dc_msg_get_file(msg: *mut dc_msg_t) -> *mut libc::c_cha pub unsafe extern "C" fn dc_msg_get_filename(msg: *mut dc_msg_t) -> *mut libc::c_char { if msg.is_null() { eprintln!("ignoring careless call to dc_msg_get_filename()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_msg = &*msg; ffi_msg.message.get_filename().unwrap_or_default().strdup() @@ -2768,13 +2767,13 @@ pub unsafe extern "C" fn dc_msg_get_filename(msg: *mut dc_msg_t) -> *mut libc::c pub unsafe extern "C" fn dc_msg_get_filemime(msg: *mut dc_msg_t) -> *mut libc::c_char { if msg.is_null() { eprintln!("ignoring careless call to dc_msg_get_filemime()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_msg = &*msg; if let Some(x) = ffi_msg.message.get_filemime() { x.strdup() } else { - dc_strdup(ptr::null()) + "".strdup() } } @@ -3098,7 +3097,7 @@ pub unsafe extern "C" fn dc_contact_get_id(contact: *mut dc_contact_t) -> u32 { pub unsafe extern "C" fn dc_contact_get_addr(contact: *mut dc_contact_t) -> *mut libc::c_char { if contact.is_null() { eprintln!("ignoring careless call to dc_contact_get_addr()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_contact = &*contact; ffi_contact.contact.get_addr().strdup() @@ -3108,7 +3107,7 @@ pub unsafe extern "C" fn dc_contact_get_addr(contact: *mut dc_contact_t) -> *mut pub unsafe extern "C" fn dc_contact_get_name(contact: *mut dc_contact_t) -> *mut libc::c_char { if contact.is_null() { eprintln!("ignoring careless call to dc_contact_get_name()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_contact = &*contact; ffi_contact.contact.get_name().strdup() @@ -3120,7 +3119,7 @@ pub unsafe extern "C" fn dc_contact_get_display_name( ) -> *mut libc::c_char { if contact.is_null() { eprintln!("ignoring careless call to dc_contact_get_display_name()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_contact = &*contact; ffi_contact.contact.get_display_name().strdup() @@ -3132,7 +3131,7 @@ pub unsafe extern "C" fn dc_contact_get_name_n_addr( ) -> *mut libc::c_char { if contact.is_null() { eprintln!("ignoring careless call to dc_contact_get_name_n_addr()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_contact = &*contact; ffi_contact.contact.get_name_n_addr().strdup() @@ -3144,7 +3143,7 @@ pub unsafe extern "C" fn dc_contact_get_first_name( ) -> *mut libc::c_char { if contact.is_null() { eprintln!("ignoring careless call to dc_contact_get_first_name()"); - return dc_strdup(ptr::null()); + return "".strdup(); } let ffi_contact = &*contact; ffi_contact.contact.get_first_name().strdup() diff --git a/deltachat-ffi/src/string.rs b/deltachat-ffi/src/string.rs index 7cfd1c357..256b110b0 100644 --- a/deltachat-ffi/src/string.rs +++ b/deltachat-ffi/src/string.rs @@ -9,7 +9,7 @@ use std::ptr; /// # Examples /// /// ```rust,norun -/// use deltachat::dc_tools::{dc_strdup, to_string_lossy}; +/// use crate::string::{dc_strdup, to_string_lossy}; /// unsafe { /// let str_a = b"foobar\x00" as *const u8 as *const libc::c_char; /// let str_a_copy = dc_strdup(str_a); @@ -17,7 +17,7 @@ use std::ptr; /// assert_ne!(str_a, str_a_copy); /// } /// ``` -pub unsafe fn dc_strdup(s: *const libc::c_char) -> *mut libc::c_char { +unsafe fn dc_strdup(s: *const libc::c_char) -> *mut libc::c_char { let ret: *mut libc::c_char; if !s.is_null() { ret = libc::strdup(s); @@ -32,7 +32,7 @@ pub unsafe fn dc_strdup(s: *const libc::c_char) -> *mut libc::c_char { /// Error type for the [OsStrExt] trait #[derive(Debug, Fail, PartialEq)] -pub enum CStringError { +pub(crate) enum CStringError { /// The string contains an interior null byte #[fail(display = "String contains an interior null byte")] InteriorNullByte, @@ -66,7 +66,7 @@ pub enum CStringError { /// let mut c_ptr: *mut libc::c_char = dc_strdup(path_c.as_ptr()); /// } /// ``` -pub trait OsStrExt { +pub(crate) trait OsStrExt { /// Convert a [std::ffi::OsStr] to an [std::ffi::CString] /// /// This is useful to convert e.g. a [std::path::Path] to @@ -131,15 +131,16 @@ fn os_str_to_c_string_unicode( } /// Convenience methods/associated functions for working with [CString] -/// -/// This is helps transitioning from unsafe code. -pub trait CStringExt { - /// Create a new [CString], yolo style +trait CStringExt { + /// Create a new [CString], best effort /// - /// This unwrap the result, panicking when there are embedded NULL - /// bytes. - fn yolo>>(t: T) -> CString { - CString::new(t).expect("String contains null byte, can not be CString") + /// Like the [to_string_lossy] this doesn't give up in the face of + /// bad input (embedded null bytes in this case) instead it does + /// the best it can by stripping the embedded null bytes. + fn new_lossy>>(t: T) -> CString { + let mut s = t.into(); + s.retain(|&c| c != 0); + CString::new(s).unwrap_or_default() } } @@ -151,7 +152,7 @@ impl CStringExt for CString {} /// Rust strings to raw C strings. This can be clumsy to do correctly /// and the compiler sometimes allows it in an unsafe way. These /// methods make it more succinct and help you get it right. -pub trait StrExt { +pub(crate) trait Strdup { /// Allocate a new raw C `*char` version of this string. /// /// This allocates a new raw C string which must be freed using @@ -168,35 +169,44 @@ pub trait StrExt { unsafe fn strdup(&self) -> *mut libc::c_char; } -impl> StrExt for T { +impl> Strdup for T { unsafe fn strdup(&self) -> *mut libc::c_char { - let tmp = CString::yolo(self.as_ref()); + let tmp = CString::new_lossy(self.as_ref()); + dc_strdup(tmp.as_ptr()) + } +} + +// We can not implement for AsRef because we already implement +// AsRev and this conflicts. So implement for Path directly. +impl Strdup for std::path::Path { + unsafe fn strdup(&self) -> *mut libc::c_char { + let tmp = self.to_c_string().unwrap_or_else(|_| CString::default()); dc_strdup(tmp.as_ptr()) } } /// Convenience methods to turn optional strings into C strings. /// -/// This is the same as the [StrExt] trait but a different trait name -/// to work around the type system not allowing to implement [StrExt] -/// for `Option` When we already have an [StrExt] impl +/// This is the same as the [Strdup] trait but a different trait name +/// to work around the type system not allowing to implement [Strdup] +/// for `Option` When we already have an [Strdup] impl /// for `AsRef<&str>`. /// /// When the [Option] is [Option::Some] this behaves just like -/// [StrExt::strdup], when it is [Option::None] a null pointer is +/// [Strdup::strdup], when it is [Option::None] a null pointer is /// returned. -pub trait OptStrExt { +pub(crate) trait OptStrdup { /// Allocate a new raw C `*char` version of this string, or NULL. /// - /// See [StrExt::strdup] for details. + /// See [Strdup::strdup] for details. unsafe fn strdup(&self) -> *mut libc::c_char; } -impl> OptStrExt for Option { +impl> OptStrdup for Option { unsafe fn strdup(&self) -> *mut libc::c_char { match self { Some(s) => { - let tmp = CString::yolo(s.as_ref()); + let tmp = CString::new_lossy(s.as_ref()); dc_strdup(tmp.as_ptr()) } None => ptr::null_mut(), @@ -204,7 +214,7 @@ impl> OptStrExt for Option { } } -pub fn to_string_lossy(s: *const libc::c_char) -> String { +pub(crate) fn to_string_lossy(s: *const libc::c_char) -> String { if s.is_null() { return "".into(); } @@ -214,7 +224,7 @@ pub fn to_string_lossy(s: *const libc::c_char) -> String { cstr.to_string_lossy().to_string() } -pub fn to_opt_string_lossy(s: *const libc::c_char) -> Option { +pub(crate) fn to_opt_string_lossy(s: *const libc::c_char) -> Option { if s.is_null() { return None; } @@ -235,7 +245,7 @@ pub fn to_opt_string_lossy(s: *const libc::c_char) -> Option { /// /// [Path]: std::path::Path #[cfg(not(target_os = "windows"))] -pub fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path { +pub(crate) fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path { assert!(!s.is_null(), "cannot be used on null pointers"); use std::os::unix::ffi::OsStrExt; unsafe { @@ -247,7 +257,7 @@ pub fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path { // as_path() implementation for windows, documented above. #[cfg(target_os = "windows")] -pub fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path { +pub(crate) fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path { as_path_unicode(s) } @@ -354,8 +364,14 @@ mod tests { } #[test] - fn test_cstring_yolo() { - assert_eq!(CString::new("hello").unwrap(), CString::yolo("hello")); + fn test_cstring_new_lossy() { + assert!(CString::new("hel\x00lo").is_err()); + assert!(CString::new(String::from("hel\x00o")).is_err()); + let r = CString::new("hello").unwrap(); + assert_eq!(CString::new_lossy("hello"), r); + assert_eq!(CString::new_lossy("hel\x00lo"), r); + assert_eq!(CString::new_lossy(String::from("hello")), r); + assert_eq!(CString::new_lossy(String::from("hel\x00lo")), r); } #[test] diff --git a/tests/stress.rs b/tests/stress.rs index 55e9e35a8..ed562ee77 100644 --- a/tests/stress.rs +++ b/tests/stress.rs @@ -44,9 +44,9 @@ fn stress_functions(context: &Context) { // assert!(dc_is_configured(context) != 0, "Missing configured context"); // let setupcode = dc_create_setup_code(context); - // let setupcode_c = CString::yolo(setupcode.clone()); + // let setupcode_c = CString::new(setupcode.clone()).unwrap(); // let setupfile = dc_render_setup_file(context, &setupcode).unwrap(); - // let setupfile_c = CString::yolo(setupfile); + // let setupfile_c = CString::new(setupfile).unwrap(); // let mut headerline_2: *const libc::c_char = ptr::null(); // let payload = dc_decrypt_setup_file(context, setupcode_c.as_ptr(), setupfile_c.as_ptr()); From 958802a233071e444ad994bdef6031a38941d055 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 31 Mar 2020 00:41:07 +0300 Subject: [PATCH 11/73] Add system message only if contact was removed successfully --- src/chat.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index f0f3d0e6d..11c391097 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2063,7 +2063,7 @@ pub fn remove_contact_from_chat( "Cannot remove contact from chat; self not in group.".into() ) ); - } else { + } else if remove_from_chat_contacts_table(context, chat_id, contact_id) { /* we should respect this - whatever we send to the group, it gets discarded anyway! */ if let Ok(contact) = Contact::get_by_id(context, contact_id) { if chat.is_promoted() { @@ -2093,10 +2093,9 @@ pub fn remove_contact_from_chat( }); } } - if remove_from_chat_contacts_table(context, chat_id, contact_id) { - context.call_cb(Event::ChatModified(chat_id)); - success = true; - } + + context.call_cb(Event::ChatModified(chat_id)); + success = true; } } } From d78ea882c8ba43e7facb53dc06e519a10ddd919f Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 22 Mar 2020 19:51:59 +0300 Subject: [PATCH 12/73] Update mailparse to 0.12 --- Cargo.lock | 6 ++--- Cargo.toml | 2 +- src/aheader.rs | 2 +- src/e2ee.rs | 2 +- src/headerdef.rs | 17 +++++------- src/imap/mod.rs | 25 ++++++++--------- src/mimeparser.rs | 69 +++++++++++++++-------------------------------- 7 files changed, 45 insertions(+), 78 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ae331de3..c4bcf1ccf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -651,7 +651,7 @@ dependencies = [ "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "lettre_email 0.9.2 (git+https://github.com/deltachat/lettre)", "libc 0.2.67 (registry+https://github.com/rust-lang/crates.io-index)", - "mailparse 0.10.4 (registry+https://github.com/rust-lang/crates.io-index)", + "mailparse 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)", "native-tls 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "num-derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1473,7 +1473,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "mailparse" -version = "0.10.4" +version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "base64 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3299,7 +3299,7 @@ dependencies = [ "checksum log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "14b6052be84e6b71ab17edffc2eeabf5c2c3ae1fdb464aae35ac50c67a44e1f7" "checksum lru-cache 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "31e24f1ad8321ca0e8a1e0ac13f23cb668e6f5466c2c57319f6a5cf1cc8e3b1c" "checksum lzw 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)" = "7d947cbb889ed21c2a84be6ffbaebf5b4e0f4340638cba0444907e38b56be084" -"checksum mailparse 0.10.4 (registry+https://github.com/rust-lang/crates.io-index)" = "6c03df7fe4bab038aaa2f313baae7600de0afd606f8244860801c46f53babdd8" +"checksum mailparse 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)" = "7181507a68fef921f011b0c0f143efca20871da5ab3963bdc064537278469cd2" "checksum matches 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "7ffc5c5338469d4d3ea17d269fa8ea3512ad247247c30bd2df69e68309ed0a08" "checksum maybe-uninit 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "60302e4db3a61da70c0cb7991976248362f30319e88850c487b9b95bbf059e00" "checksum md-5 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "a18af3dcaf2b0219366cdb4e2af65a6101457b415c3d1a5c71dd9c2b7c77b9c8" diff --git a/Cargo.toml b/Cargo.toml index d54e42468..72cb02b89 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,7 +53,7 @@ bitflags = "1.1.0" debug_stub_derive = "0.3.0" sanitize-filename = "0.2.1" stop-token = { version = "0.1.1", features = ["unstable"] } -mailparse = "0.10.2" +mailparse = "0.12.0" encoded-words = { git = "https://github.com/async-email/encoded-words", branch="master" } native-tls = "0.2.3" image = { version = "0.22.4", default-features=false, features = ["gif_codec", "jpeg", "ico", "png_codec", "pnm", "webp", "bmp"] } diff --git a/src/aheader.rs b/src/aheader.rs index a87f7be65..628de89f7 100644 --- a/src/aheader.rs +++ b/src/aheader.rs @@ -75,7 +75,7 @@ impl Aheader { wanted_from: &str, headers: &[mailparse::MailHeader<'_>], ) -> Option { - if let Ok(Some(value)) = headers.get_header_value(HeaderDef::Autocrypt) { + if let Some(value) = headers.get_header_value(HeaderDef::Autocrypt) { match Self::from_str(&value) { Ok(header) => { if addr_cmp(&header.addr, wanted_from) { diff --git a/src/e2ee.rs b/src/e2ee.rs index 55baf2acb..4ae91e9c6 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -126,7 +126,7 @@ pub fn try_decrypt( ) -> Result<(Option>, HashSet)> { let from = mail .headers - .get_header_value(HeaderDef::From_)? + .get_header_value(HeaderDef::From_) .and_then(|from_addr| mailparse::addrparse(&from_addr).ok()) .and_then(|from| from.extract_single_info()) .map(|from| from.addr) diff --git a/src/headerdef.rs b/src/headerdef.rs index df75adec7..af8343949 100644 --- a/src/headerdef.rs +++ b/src/headerdef.rs @@ -1,5 +1,5 @@ use crate::strum::AsStaticRef; -use mailparse::{MailHeader, MailHeaderMap, MailParseError}; +use mailparse::{MailHeader, MailHeaderMap}; #[derive(Debug, Display, Clone, PartialEq, Eq, EnumVariantNames, AsStaticStr)] #[strum(serialize_all = "kebab_case")] @@ -52,11 +52,11 @@ impl HeaderDef { } pub trait HeaderDefMap { - fn get_header_value(&self, headerdef: HeaderDef) -> Result, MailParseError>; + fn get_header_value(&self, headerdef: HeaderDef) -> Option; } impl HeaderDefMap for [MailHeader<'_>] { - fn get_header_value(&self, headerdef: HeaderDef) -> Result, MailParseError> { + fn get_header_value(&self, headerdef: HeaderDef) -> Option { self.get_first_value(headerdef.get_headername()) } } @@ -79,18 +79,13 @@ mod tests { let (headers, _) = mailparse::parse_headers(b"fRoM: Bob\naUtoCryPt-SeTup-MessAge: v99").unwrap(); assert_eq!( - headers - .get_header_value(HeaderDef::AutocryptSetupMessage) - .unwrap(), + headers.get_header_value(HeaderDef::AutocryptSetupMessage), Some("v99".to_string()) ); assert_eq!( - headers.get_header_value(HeaderDef::From_).unwrap(), + headers.get_header_value(HeaderDef::From_), Some("Bob".to_string()) ); - assert_eq!( - headers.get_header_value(HeaderDef::Autocrypt).unwrap(), - None - ); + assert_eq!(headers.get_header_value(HeaderDef::Autocrypt), None); } } diff --git a/src/imap/mod.rs b/src/imap/mod.rs index 7f1eaaf0e..3eb0c6dc2 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1338,30 +1338,27 @@ fn get_fetch_headers(prefetch_msg: &Fetch) -> Result> } fn prefetch_get_message_id(headers: &[mailparse::MailHeader]) -> Result { - if let Some(message_id) = headers.get_header_value(HeaderDef::MessageId)? { + if let Some(message_id) = headers.get_header_value(HeaderDef::MessageId) { Ok(crate::mimeparser::parse_message_id(&message_id)?) } else { Err(Error::Other("prefetch: No message ID found".to_string())) } } -fn prefetch_is_reply_to_chat_message( - context: &Context, - headers: &[mailparse::MailHeader], -) -> Result { - if let Some(value) = headers.get_header_value(HeaderDef::InReplyTo)? { +fn prefetch_is_reply_to_chat_message(context: &Context, headers: &[mailparse::MailHeader]) -> bool { + if let Some(value) = headers.get_header_value(HeaderDef::InReplyTo) { if is_msgrmsg_rfc724_mid_in_list(context, &value) { - return Ok(true); + return true; } } - if let Some(value) = headers.get_header_value(HeaderDef::References)? { + if let Some(value) = headers.get_header_value(HeaderDef::References) { if is_msgrmsg_rfc724_mid_in_list(context, &value) { - return Ok(true); + return true; } } - Ok(false) + false } fn prefetch_should_download( @@ -1369,16 +1366,16 @@ fn prefetch_should_download( headers: &[mailparse::MailHeader], show_emails: ShowEmails, ) -> Result { - let is_chat_message = headers.get_header_value(HeaderDef::ChatVersion)?.is_some(); - let is_reply_to_chat_message = prefetch_is_reply_to_chat_message(context, &headers)?; + let is_chat_message = headers.get_header_value(HeaderDef::ChatVersion).is_some(); + let is_reply_to_chat_message = prefetch_is_reply_to_chat_message(context, &headers); // Autocrypt Setup Message should be shown even if it is from non-chat client. let is_autocrypt_setup_message = headers - .get_header_value(HeaderDef::AutocryptSetupMessage)? + .get_header_value(HeaderDef::AutocryptSetupMessage) .is_some(); let from_field = headers - .get_header_value(HeaderDef::From_)? + .get_header_value(HeaderDef::From_) .unwrap_or_default(); let (_contact_id, blocked_contact, origin) = from_field_to_contact_id(context, &from_field)?; diff --git a/src/mimeparser.rs b/src/mimeparser.rs index e4ab0817c..597b5ef48 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -85,7 +85,7 @@ impl MimeMessage { let message_time = mail .headers - .get_header_value(HeaderDef::Date)? + .get_header_value(HeaderDef::Date) .and_then(|v| mailparse::dateparse(&v).ok()) .unwrap_or_default(); @@ -111,8 +111,7 @@ impl MimeMessage { // Handle any gossip headers if the mail was encrypted. See section // "3.6 Key Gossip" of https://autocrypt.org/autocrypt-spec-1.1.0.pdf - let gossip_headers = - decrypted_mail.headers.get_all_values("Autocrypt-Gossip")?; + let gossip_headers = decrypted_mail.headers.get_all_values("Autocrypt-Gossip"); gossipped_addr = update_gossip_peerstates(context, message_time, &mail, gossip_headers)?; @@ -746,16 +745,13 @@ impl MimeMessage { fn merge_headers(headers: &mut HashMap, fields: &[mailparse::MailHeader<'_>]) { for field in fields { - if let Ok(key) = field.get_key() { - // lowercasing all headers is technically not correct, but makes things work better - let key = key.to_lowercase(); - if !headers.contains_key(&key) || // key already exists, only overwrite known types (protected headers) + // lowercasing all headers is technically not correct, but makes things work better + let key = field.get_key().to_lowercase(); + if !headers.contains_key(&key) || // key already exists, only overwrite known types (protected headers) is_known(&key) || key.starts_with("chat-") - { - if let Ok(value) = field.get_value() { - headers.insert(key, value); - } - } + { + let value = field.get_value(); + headers.insert(key.to_string(), value); } } } @@ -770,21 +766,13 @@ impl MimeMessage { let (report_fields, _) = mailparse::parse_headers(&report_body)?; // must be present - if let Some(_disposition) = report_fields - .get_header_value(HeaderDef::Disposition) - .ok() - .flatten() - { + if let Some(_disposition) = report_fields.get_header_value(HeaderDef::Disposition) { if let Some(original_message_id) = report_fields .get_header_value(HeaderDef::OriginalMessageId) - .ok() - .flatten() .and_then(|v| parse_message_id(&v).ok()) { let additional_message_ids = report_fields .get_header_value(HeaderDef::AdditionalMessageIds) - .ok() - .flatten() .map_or_else(Vec::new, |v| { v.split(' ') .filter_map(|s| parse_message_id(s).ok()) @@ -800,7 +788,7 @@ impl MimeMessage { warn!( context, "ignoring unknown disposition-notification, Message-Id: {:?}", - report_fields.get_header_value(HeaderDef::MessageId).ok() + report_fields.get_header_value(HeaderDef::MessageId) ); Ok(None) @@ -860,14 +848,9 @@ fn update_gossip_peerstates( if let Ok(ref header) = gossip_header { if recipients.is_none() { - recipients = Some(get_recipients(mail.headers.iter().filter_map(|v| { - let key = v.get_key(); - let value = v.get_value(); - if key.is_err() || value.is_err() { - return None; - } - Some((v.get_key().unwrap(), v.get_value().unwrap())) - }))); + recipients = Some(get_recipients( + mail.headers.iter().map(|v| (v.get_key(), v.get_value())), + )); } if recipients @@ -915,13 +898,8 @@ pub(crate) fn parse_message_id(value: &str) -> crate::error::Result { let ids = mailparse::msgidparse(value) .map_err(|err| format_err!("failed to parse message id {:?}", err))?; - if ids.len() == 1 { - let id = &ids[0]; - if id.starts_with('<') && id.ends_with('>') { - Ok(id.chars().skip(1).take(id.len() - 2).collect()) - } else { - bail!("message-ID {} is not enclosed in < and >", value); - } + if let Some(id) = ids.first() { + Ok(id.to_string()) } else { bail!("could not parse message_id: {}", value); } @@ -987,15 +965,12 @@ fn get_mime_type(mail: &mailparse::ParsedMail<'_>) -> Result<(Mime, Viewtype)> { } fn is_attachment_disposition(mail: &mailparse::ParsedMail<'_>) -> bool { - if let Ok(ct) = mail.get_content_disposition() { - return ct.disposition == DispositionType::Attachment - && ct - .params - .iter() - .any(|(key, _value)| key.starts_with("filename")); - } - - false + let ct = mail.get_content_disposition(); + ct.disposition == DispositionType::Attachment + && ct + .params + .iter() + .any(|(key, _value)| key.starts_with("filename")) } /// Tries to get attachment filename. @@ -1010,7 +985,7 @@ fn get_attachment_filename(mail: &mailparse::ParsedMail) -> Result = ct .params From 6ffe54d68f61527c25233c26c02913b11f4a3273 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Thu, 27 Feb 2020 16:16:00 +0100 Subject: [PATCH 13/73] do no longer ignore keypair generation test, due to the ecc-move, it is no longer expensive --- src/e2ee.rs | 2 -- src/pgp.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/src/e2ee.rs b/src/e2ee.rs index 4ae91e9c6..0f40b3d1f 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -417,7 +417,6 @@ Sent with my Delta Chat Messenger: https://delta.chat"; } #[test] - #[ignore] // generating keys is expensive fn test_generate() { let t = dummy_context(); let addr = "alice@example.org"; @@ -429,7 +428,6 @@ Sent with my Delta Chat Messenger: https://delta.chat"; } #[test] - #[ignore] fn test_generate_concurrent() { use std::sync::Arc; use std::thread; diff --git a/src/pgp.rs b/src/pgp.rs index 64d64e095..1806b6fcb 100644 --- a/src/pgp.rs +++ b/src/pgp.rs @@ -394,7 +394,6 @@ mod tests { } #[test] - #[ignore] // is too expensive fn test_create_keypair() { let keypair0 = create_keypair( EmailAddress::new("foo@bar.de").unwrap(), From 76fc84be375bec9a8d9037a9ee1151ee4b003e55 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 23 Feb 2020 03:37:18 +0300 Subject: [PATCH 14/73] job: document DeleteMsgOnImap --- src/job.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/job.rs b/src/job.rs index 23405ea41..04a8af69d 100644 --- a/src/job.rs +++ b/src/job.rs @@ -437,6 +437,14 @@ impl Job { } } + /// Deletes a message on the server. + /// + /// foreign_id is a MsgId pointing to a message in the trash chat + /// or a hidden message. + /// + /// This job removes the database record. If there are no more + /// records pointing to the same message on the server, the job + /// also removes the message on the server. #[allow(non_snake_case)] fn DeleteMsgOnImap(&mut self, context: &Context) -> Status { let imap_inbox = &context.inbox_thread.read().unwrap().imap; From d64e55c66f3e596928268b249d34c33c25850cc8 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 23 Feb 2020 06:30:19 +0300 Subject: [PATCH 15/73] delete_msgs: remove explicit .iter() --- src/message.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/message.rs b/src/message.rs index 67d8c2d23..a06f2a0c2 100644 --- a/src/message.rs +++ b/src/message.rs @@ -951,7 +951,7 @@ pub fn get_mime_headers(context: &Context, msg_id: MsgId) -> Option { } pub fn delete_msgs(context: &Context, msg_ids: &[MsgId]) { - for msg_id in msg_ids.iter() { + for msg_id in msg_ids { if let Ok(msg) = Message::load_from_db(context, *msg_id) { if msg.location_id > 0 { delete_poi_location(context, msg.location_id); From c1c769ceb03c8feffbe8e875895344f094c5cfd5 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 23 Feb 2020 18:18:38 +0300 Subject: [PATCH 16/73] Add MsgId.trash() and use it to delete messages locally In addition to moving the message into trash chat, this function removes message text to make sure the message does not remain in the database. Only the information necessary to delete message from the server and avoid redownloading it should be kept, such as Message-Id and IMAP UID. --- src/message.rs | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/message.rs b/src/message.rs index a06f2a0c2..359e399a3 100644 --- a/src/message.rs +++ b/src/message.rs @@ -85,6 +85,20 @@ impl MsgId { self.0 == DC_MSG_ID_DAYMARKER } + /// Put message into trash chat and delete message text. + /// + /// It means the message is deleted locally, but not on the server + /// yet. + pub fn trash(self, context: &Context) -> crate::sql::Result<()> { + let chat_id = ChatId::new(DC_CHAT_ID_TRASH); + sql::execute( + context, + &context.sql, + "UPDATE msgs SET chat_id=?, txt='', txt_raw='' WHERE id=?", + params![chat_id, self], + ) + } + /// Bad evil escape hatch. /// /// Avoid using this, eventually types should be cleaned up enough @@ -957,7 +971,9 @@ pub fn delete_msgs(context: &Context, msg_ids: &[MsgId]) { delete_poi_location(context, msg.location_id); } } - update_msg_chat_id(context, *msg_id, ChatId::new(DC_CHAT_ID_TRASH)); + if let Err(err) = msg_id.trash(context) { + warn!(context, "Unable to trash message {}: {}", msg_id, err); + } job_add( context, Action::DeleteMsgOnImap, @@ -977,16 +993,6 @@ pub fn delete_msgs(context: &Context, msg_ids: &[MsgId]) { }; } -fn update_msg_chat_id(context: &Context, msg_id: MsgId, chat_id: ChatId) -> bool { - sql::execute( - context, - &context.sql, - "UPDATE msgs SET chat_id=? WHERE id=?;", - params![chat_id, msg_id], - ) - .is_ok() -} - fn delete_poi_location(context: &Context, location_id: u32) -> bool { sql::execute( context, From 98bd64621ae6f54439b6b7c0651566192b4bce5a Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 00:29:46 +0300 Subject: [PATCH 17/73] Refactor Imap.delete_msg() error handling Mutable UID reference is removed. Instead, ImapActionResult is returned immediately. If message has changed and UID does not correspond to expected message, ImapActionResult::Failed is returned. Temprorary IMAP errors result in ImapActionResult::RetryLater. On the job side, ImapActionResult::RetryLater does not result in immediate job retry anymore. Instead, job is retried with a backoff. --- src/imap/mod.rs | 12 ++++++------ src/job.rs | 15 ++++++++++----- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index 3eb0c6dc2..be919b5c4 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -994,10 +994,10 @@ impl Imap { context: &Context, message_id: &str, folder: &str, - uid: &mut u32, + uid: u32, ) -> ImapActionResult { task::block_on(async move { - if let Some(imapresult) = self.prepare_imap_operation_on_msg(context, folder, *uid) { + if let Some(imapresult) = self.prepare_imap_operation_on_msg(context, folder, uid) { return imapresult; } // we are connected, and the folder is selected @@ -1034,7 +1034,7 @@ impl Imap { remote_message_id, message_id, ); - *uid = 0; + return ImapActionResult::Failed; } } Err(err) => { @@ -1042,18 +1042,18 @@ impl Imap { context, "Cannot delete {} on IMAP: {}", display_imap_id, err ); - *uid = 0; + return ImapActionResult::RetryLater; } } } // mark the message for deletion - if !self.add_flag_finalized(context, *uid, "\\Deleted").await { + if !self.add_flag_finalized(context, uid, "\\Deleted").await { warn!( context, "Cannot mark message {} as \"Deleted\".", display_imap_id ); - ImapActionResult::Failed + ImapActionResult::RetryLater } else { emit_event!( context, diff --git a/src/job.rs b/src/job.rs index 04a8af69d..684457823 100644 --- a/src/job.rs +++ b/src/job.rs @@ -449,7 +449,7 @@ impl Job { fn DeleteMsgOnImap(&mut self, context: &Context) -> Status { let imap_inbox = &context.inbox_thread.read().unwrap().imap; - let mut msg = job_try!(Message::load_from_db(context, MsgId::new(self.foreign_id))); + let msg = job_try!(Message::load_from_db(context, MsgId::new(self.foreign_id))); if !msg.rfc724_mid.is_empty() { if message::rfc724_mid_cnt(context, &msg.rfc724_mid) > 1 { @@ -462,10 +462,15 @@ impl Job { we delete the message from the server */ let mid = msg.rfc724_mid; let server_folder = msg.server_folder.as_ref().unwrap(); - let res = imap_inbox.delete_msg(context, &mid, server_folder, &mut msg.server_uid); - if res == ImapActionResult::RetryLater { - // XXX RetryLater is converted to RetryNow here - return Status::RetryNow; + let res = imap_inbox.delete_msg(context, &mid, server_folder, msg.server_uid); + match res { + ImapActionResult::RetryLater => { + return Status::RetryLater; + } + ImapActionResult::AlreadyDone | ImapActionResult::Success => {} + ImapActionResult::Failed => { + return Status::Finished(Err(format_err!("Message deletion failed"))); + } } } Message::delete_from_db(context, msg.id); From 4452cab9873bea999e05724623d1e01dac165d52 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 01:21:28 +0300 Subject: [PATCH 18/73] Turn Message::Delete_from_db into MsgId method --- src/chat.rs | 2 +- src/job.rs | 2 +- src/message.rs | 37 ++++++++++++++++++------------------- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 11c391097..80b70904c 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -287,7 +287,7 @@ impl ChatId { fn maybe_delete_draft(self, context: &Context) -> bool { match self.get_draft_msg_id(context) { Some(msg_id) => { - Message::delete_from_db(context, msg_id); + msg_id.delete_from_db(context); true } None => false, diff --git a/src/job.rs b/src/job.rs index 684457823..2fb2174bc 100644 --- a/src/job.rs +++ b/src/job.rs @@ -473,7 +473,7 @@ impl Job { } } } - Message::delete_from_db(context, msg.id); + msg.id.delete_from_db(context); Status::Finished(Ok(())) } else { /* eg. device messages have no Message-ID */ diff --git a/src/message.rs b/src/message.rs index 359e399a3..6dbe05665 100644 --- a/src/message.rs +++ b/src/message.rs @@ -99,6 +99,24 @@ impl MsgId { ) } + /// Deletes a message and corresponding MDNs from the database. + pub fn delete_from_db(self, context: &Context) { + sql::execute( + context, + &context.sql, + "DELETE FROM msgs WHERE id=?;", + params![self], + ) + .ok(); + sql::execute( + context, + &context.sql, + "DELETE FROM msgs_mdns WHERE msg_id=?;", + params![self], + ) + .ok(); + } + /// Bad evil escape hatch. /// /// Avoid using this, eventually types should be cleaned up enough @@ -319,25 +337,6 @@ impl Message { .map_err(Into::into) } - pub fn delete_from_db(context: &Context, msg_id: MsgId) { - if let Ok(msg) = Message::load_from_db(context, msg_id) { - sql::execute( - context, - &context.sql, - "DELETE FROM msgs WHERE id=?;", - params![msg.id], - ) - .ok(); - sql::execute( - context, - &context.sql, - "DELETE FROM msgs_mdns WHERE msg_id=?;", - params![msg.id], - ) - .ok(); - } - } - pub fn get_filemime(&self) -> Option { if let Some(m) = self.param.get(Param::MimeType) { return Some(m.to_string()); From 7879952fde0c0a1f482af5f2ac4f2811b23ea9c9 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 01:22:28 +0300 Subject: [PATCH 19/73] Delete MDNs first in MsgId.delete_from_db() --- src/message.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/message.rs b/src/message.rs index 6dbe05665..bd9038fbc 100644 --- a/src/message.rs +++ b/src/message.rs @@ -101,17 +101,19 @@ impl MsgId { /// Deletes a message and corresponding MDNs from the database. pub fn delete_from_db(self, context: &Context) { + // We don't use transactions yet, so remove MDNs first to make + // sure they are not left while the message is deleted. sql::execute( context, &context.sql, - "DELETE FROM msgs WHERE id=?;", + "DELETE FROM msgs_mdns WHERE msg_id=?;", params![self], ) .ok(); sql::execute( context, &context.sql, - "DELETE FROM msgs_mdns WHERE msg_id=?;", + "DELETE FROM msgs WHERE id=?;", params![self], ) .ok(); From cc0f977d6f05230fe1f871f0963b725e03ca2799 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 02:35:44 +0300 Subject: [PATCH 20/73] Document rfc724_mid_cnt --- src/message.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/message.rs b/src/message.rs index bd9038fbc..7a656e99c 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1354,6 +1354,8 @@ pub fn get_deaddrop_msg_cnt(context: &Context) -> usize { } } +/// Counts number of database records pointing to specified +/// Message-ID. pub fn rfc724_mid_cnt(context: &Context, rfc724_mid: &str) -> i32 { // check the number of messages with the same rfc724_mid match context.sql.query_row( From 2cf9c68040afd5e71e1849abf2f78adbf24d8a59 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 02:36:18 +0300 Subject: [PATCH 21/73] Implement MsgId.unlink() and use it in DeleteMsgOnImap Currently only trashed or hidden messages are deleted by DeleteMsgOnImap, so it is safe to remove database records. It is planned to delete messages on IMAP server after user-configurable time to cleanup the server even for messages displayed in chats. For such messages, we unlink them from the Message-ID, but keep the database record to display them. --- src/job.rs | 29 +++++++++++++++++++++++++++-- src/message.rs | 17 +++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/job.rs b/src/job.rs index 2fb2174bc..521553f6b 100644 --- a/src/job.rs +++ b/src/job.rs @@ -462,7 +462,12 @@ impl Job { we delete the message from the server */ let mid = msg.rfc724_mid; let server_folder = msg.server_folder.as_ref().unwrap(); - let res = imap_inbox.delete_msg(context, &mid, server_folder, msg.server_uid); + let res = if msg.server_uid == 0 { + // Message is already deleted on IMAP server. + ImapActionResult::AlreadyDone + } else { + imap_inbox.delete_msg(context, &mid, server_folder, msg.server_uid) + }; match res { ImapActionResult::RetryLater => { return Status::RetryLater; @@ -473,7 +478,27 @@ impl Job { } } } - msg.id.delete_from_db(context); + if msg.chat_id.is_trash() || msg.hidden { + // Messages are stored in trash chat only to keep + // their server UID and Message-ID. Once message is + // deleted from the server, database record can be + // removed as well. + // + // Hidden messages are similar to trashed, but are + // related to some chat. We also delete their + // database records. + msg.id.delete_from_db(context); + } else { + // Remove server UID from the database record. + // + // We have either just removed the message from the + // server, in which case UID is not valid anymore, or + // we have more refernces to the same server UID, so + // we remove UID to reduce the number of messages + // pointing to the corresponding UID. Once the counter + // reaches zero, we will remove the message. + job_try!(msg.id.unlink(context)); + } Status::Finished(Ok(())) } else { /* eg. device messages have no Message-ID */ diff --git a/src/message.rs b/src/message.rs index 7a656e99c..9a0d5553c 100644 --- a/src/message.rs +++ b/src/message.rs @@ -119,6 +119,23 @@ impl MsgId { .ok(); } + /// Removes Message-ID, IMAP server UID, folder from the database + /// record. + /// + /// It is used to avoid trying to remove the message from the + /// server multiple times when there are multiple message records + /// pointing to the same server UID. + pub(crate) fn unlink(self, context: &Context) -> sql::Result<()> { + sql::execute( + context, + &context.sql, + "UPDATE msgs \ + SET rfc724_mid='', server_folder='', server_uid=0 \ + WHERE id=?", + params![self], + ) + } + /// Bad evil escape hatch. /// /// Avoid using this, eventually types should be cleaned up enough From 4f4241ba3a4cca86c31926cc16f6210ae1e83807 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 16:45:52 +0300 Subject: [PATCH 22/73] dc_receive_imf: delete all message parts if message should be deleted DeleteMsgOnImap deletes files from the server only when the last part is deleted. Removing only the first part of the hidden or trashed message does not result in message deletion. --- src/dc_receive_imf.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 3dca2329a..7bff625ae 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -194,13 +194,15 @@ pub fn dc_receive_imf( // if we delete we don't need to try moving messages if needs_delete_job && !created_db_entries.is_empty() { - job_add( - context, - Action::DeleteMsgOnImap, - created_db_entries[0].1.to_u32() as i32, - Params::new(), - 0, - ); + for db_entry in &created_db_entries { + job_add( + context, + Action::DeleteMsgOnImap, + db_entry.1.to_u32() as i32, + Params::new(), + 0, + ); + } } else { context.do_heuristics_moves(server_folder.as_ref(), insert_msg_id); } From a653e469f21da7ddecd84f111516f9bb21380b2f Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 17:05:26 +0300 Subject: [PATCH 23/73] Add user-configurable option "delete_server_after" The option sets timer in seconds after which all parts of the message are deleted from the server. --- src/config.rs | 8 ++++++++ src/dc_receive_imf.rs | 25 +++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index a0ef749ff..2e4afd6a6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -65,6 +65,14 @@ pub enum Config { #[strum(props(default = "0"))] KeyGenType, + /// Timer in seconds after which the message is deleted from the + /// server. + /// + /// Equals to -1 by default, which means the message is never + /// deleted. + #[strum(props(default = "-1"))] + DeleteServerAfter, + SaveMimeHeaders, ConfiguredAddr, ConfiguredMailServer, diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 7bff625ae..d6f62bb0b 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -192,7 +192,6 @@ pub fn dc_receive_imf( }; } - // if we delete we don't need to try moving messages if needs_delete_job && !created_db_entries.is_empty() { for db_entry in &created_db_entries { job_add( @@ -204,7 +203,29 @@ pub fn dc_receive_imf( ); } } else { - context.do_heuristics_moves(server_folder.as_ref(), insert_msg_id); + // Get user-configured server deletion + let delete_server_after = context.get_config_int(Config::DeleteServerAfter); + + if delete_server_after != 0 { + // Move message if we don't delete it immediately. + context.do_heuristics_moves(server_folder.as_ref(), insert_msg_id); + } + + if delete_server_after >= 0 { + info!( + context, + "Scheduling message deletion in {} seconds", delete_server_after + ); + for db_entry in &created_db_entries { + job_add( + context, + Action::DeleteMsgOnImap, + db_entry.1.to_u32() as i32, + Params::new(), + delete_server_after as i64, + ); + } + } } info!( From b2f1d9f376264d503f85402b67ff7977745424b9 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 24 Feb 2020 20:26:55 +0300 Subject: [PATCH 24/73] Do not remove rfc724_mid for unlinked messages Message-ID is used to send read receipts. Instead, add a separate "unlinked" column. --- src/message.rs | 12 +++++++----- src/sql.rs | 8 ++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/message.rs b/src/message.rs index 9a0d5553c..607ba602b 100644 --- a/src/message.rs +++ b/src/message.rs @@ -119,8 +119,7 @@ impl MsgId { .ok(); } - /// Removes Message-ID, IMAP server UID, folder from the database - /// record. + /// Removes IMAP server UID and folder from the database record. /// /// It is used to avoid trying to remove the message from the /// server multiple times when there are multiple message records @@ -130,7 +129,7 @@ impl MsgId { context, &context.sql, "UPDATE msgs \ - SET rfc724_mid='', server_folder='', server_uid=0 \ + SET unlinked=1, server_folder='', server_uid=0 \ WHERE id=?", params![self], ) @@ -1373,10 +1372,12 @@ pub fn get_deaddrop_msg_cnt(context: &Context) -> usize { /// Counts number of database records pointing to specified /// Message-ID. +/// +/// Unlinked messages are excluded. pub fn rfc724_mid_cnt(context: &Context, rfc724_mid: &str) -> i32 { // check the number of messages with the same rfc724_mid match context.sql.query_row( - "SELECT COUNT(*) FROM msgs WHERE rfc724_mid=?;", + "SELECT COUNT(*) FROM msgs WHERE rfc724_mid=? AND NOT unlinked", &[rfc724_mid], |row| row.get(0), ) { @@ -1417,7 +1418,8 @@ pub fn update_server_uid( server_uid: u32, ) { match context.sql.execute( - "UPDATE msgs SET server_folder=?, server_uid=? WHERE rfc724_mid=?;", + "UPDATE msgs SET server_folder=?, server_uid=? \ + WHERE rfc724_mid=? AND NOT unlinked", params![server_folder.as_ref(), server_uid, rfc724_mid], ) { Ok(_) => {} diff --git a/src/sql.rs b/src/sql.rs index 8e15d1fdd..d1992683f 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -898,6 +898,14 @@ fn open( sql.execute("UPDATE chats SET grpid='' WHERE type=100", NO_PARAMS)?; sql.set_raw_config_int(context, "dbversion", 63)?; } + if dbversion < 64 { + info!(context, "[migration] v64"); + sql.execute( + "ALTER TABLE msgs ADD COLUMN unlinked INTEGER DEFAULT 0", + NO_PARAMS, + )?; + sql.set_raw_config_int(context, "dbversion", 64)?; + } // (2) updates that require high-level objects // (the structure is complete now and all objects are usable) From 6d216af5072e832dd53daa11319e416c1c7cb60e Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 25 Feb 2020 02:43:28 +0300 Subject: [PATCH 25/73] Delete BCC-self messages after "delete_server_after" time --- src/imap/mod.rs | 50 +++++++++++++++++++++++++++++++++++++++++-------- src/message.rs | 22 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index be919b5c4..a0e699942 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1307,14 +1307,48 @@ fn precheck_imf(context: &Context, rfc724_mid: &str, server_folder: &str, server { if old_server_folder.is_empty() && old_server_uid == 0 { info!(context, "[move] detected bcc-self {}", rfc724_mid,); - context.do_heuristics_moves(server_folder.as_ref(), msg_id); - job_add( - context, - Action::MarkseenMsgOnImap, - msg_id.to_u32() as i32, - Params::new(), - 0, - ); + + let delete_server_after = context.get_config_int(Config::DeleteServerAfter); + + if delete_server_after != 0 { + context.do_heuristics_moves(server_folder.as_ref(), msg_id); + job_add( + context, + Action::MarkseenMsgOnImap, + msg_id.to_u32() as i32, + Params::new(), + 0, + ); + } + + if delete_server_after >= 0 { + info!( + context, + "Scheduling BCC-self deletion in {} seconds", delete_server_after + ); + + let msg_ids = match message::get_all_by_rfc724_mid(context, &rfc724_mid) { + Err(err) => { + warn!( + context, + "Cannot get all database records for Message-ID {}: {}", + &rfc724_mid, + err + ); + vec![msg_id] // Remove at least the MsgId we know about + } + Ok(msg_ids) => msg_ids, + }; + for part_msg_id in msg_ids { + job_add( + context, + Action::DeleteMsgOnImap, + part_msg_id.to_u32() as i32, + Params::new(), + delete_server_after as i64, + ); + } + } } else if old_server_folder != server_folder { info!(context, "[move] detected moved message {}", rfc724_mid,); } diff --git a/src/message.rs b/src/message.rs index 607ba602b..03ceb1646 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1411,6 +1411,28 @@ pub(crate) fn rfc724_mid_exists( .map_err(Into::into) } +/// Returns all MsgIds corresponding to Message-ID +pub(crate) fn get_all_by_rfc724_mid( + context: &Context, + rfc724_mid: &str, +) -> Result, Error> { + ensure!(!rfc724_mid.is_empty(), "empty rfc724_mid"); + + let msg_ids = context.sql.query_map( + "SELECT id FROM msgs WHERE rfc724_mid=?", + params![rfc724_mid], + |row| row.get::<_, MsgId>("id"), + |ids| { + let mut ret: Vec = Vec::new(); + for id in ids { + ret.push(id?); + } + Ok(ret) + }, + )?; + Ok(msg_ids) +} + pub fn update_server_uid( context: &Context, rfc724_mid: &str, From bc06b9e0518116f94f51fe337deb69b0f22f4c63 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 25 Feb 2020 03:14:06 +0300 Subject: [PATCH 26/73] Do not send BCC-Self copy if we are going to remove it immediately --- src/job.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/job.rs b/src/job.rs index 521553f6b..3e3f36dcd 100644 --- a/src/job.rs +++ b/src/job.rs @@ -860,6 +860,7 @@ pub fn job_send_msg(context: &Context, msg_id: MsgId) -> Result<()> { .unwrap_or_default(); let lowercase_from = from.to_lowercase(); if context.get_config_bool(Config::BccSelf) + && context.get_config_int(Config::DeleteServerAfter) != 0 && !recipients .iter() .any(|x| x.to_lowercase() == lowercase_from) From f2aa17c9d0d608d0208e659d8f7333f5a3137881 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 25 Feb 2020 22:47:59 +0300 Subject: [PATCH 27/73] Resultify MsgId.delete_from_db() --- src/chat.rs | 5 +---- src/job.rs | 2 +- src/message.rs | 9 ++++----- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 80b70904c..921a83181 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -286,10 +286,7 @@ impl ChatId { /// Returns `true`, if message was deleted, `false` otherwise. fn maybe_delete_draft(self, context: &Context) -> bool { match self.get_draft_msg_id(context) { - Some(msg_id) => { - msg_id.delete_from_db(context); - true - } + Some(msg_id) => msg_id.delete_from_db(context).is_ok(), None => false, } } diff --git a/src/job.rs b/src/job.rs index 3e3f36dcd..c8d75e901 100644 --- a/src/job.rs +++ b/src/job.rs @@ -487,7 +487,7 @@ impl Job { // Hidden messages are similar to trashed, but are // related to some chat. We also delete their // database records. - msg.id.delete_from_db(context); + job_try!(msg.id.delete_from_db(context)) } else { // Remove server UID from the database record. // diff --git a/src/message.rs b/src/message.rs index 03ceb1646..c2fe89c17 100644 --- a/src/message.rs +++ b/src/message.rs @@ -100,7 +100,7 @@ impl MsgId { } /// Deletes a message and corresponding MDNs from the database. - pub fn delete_from_db(self, context: &Context) { + pub fn delete_from_db(self, context: &Context) -> crate::sql::Result<()> { // We don't use transactions yet, so remove MDNs first to make // sure they are not left while the message is deleted. sql::execute( @@ -108,15 +108,14 @@ impl MsgId { &context.sql, "DELETE FROM msgs_mdns WHERE msg_id=?;", params![self], - ) - .ok(); + )?; sql::execute( context, &context.sql, "DELETE FROM msgs WHERE id=?;", params![self], - ) - .ok(); + )?; + Ok(()) } /// Removes IMAP server UID and folder from the database record. From 5b3bec1aac5eb28ccbb281db73bc8bad94b10673 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Wed, 26 Feb 2020 01:08:00 +0300 Subject: [PATCH 28/73] Create entries in msgs table for MDNs At least one entry is required for DeleteMsgOnImap job. Additionally, adding a hidden entry makes it possible to avoid redownloading the message if it gets a new UID on the server. --- src/dc_receive_imf.rs | 9 ++++++--- src/mimeparser.rs | 16 +++++++++++++--- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index d6f62bb0b..3cd71cfbf 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -624,10 +624,13 @@ fn add_parts( let subject = mime_parser.get_subject().unwrap_or_default(); for part in mime_parser.parts.iter_mut() { - if mime_parser.location_kml.is_some() + let is_mdn = !mime_parser.reports.is_empty(); + + let is_location_kml = mime_parser.location_kml.is_some() && icnt == 1 - && (part.msg == "-location-" || part.msg.is_empty()) - { + && (part.msg == "-location-" || part.msg.is_empty()); + + if is_mdn || is_location_kml { *hidden = true; if state == MessageState::InFresh { state = MessageState::InNoticed; diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 597b5ef48..376a7d9ad 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -544,6 +544,16 @@ impl MimeMessage { if let Some(report) = self.process_report(context, mail)? { self.reports.push(report); } + + // Add MDN part so we can track it, avoid + // downloading the message again and + // delete if automatic message deletion is + // enabled. + let mut part = Part::default(); + part.typ = Viewtype::Unknown; + self.parts.push(part); + + any_part_added = true; } else { /* eg. `report-type=delivery-status`; maybe we should show them as a little error icon */ @@ -1344,7 +1354,7 @@ Disposition: manual-action/MDN-sent-automatically; displayed\n\ Some("Chat: Message opened".to_string()) ); - assert_eq!(message.parts.len(), 0); + assert_eq!(message.parts.len(), 1); assert_eq!(message.reports.len(), 1); } @@ -1422,7 +1432,7 @@ Disposition: manual-action/MDN-sent-automatically; displayed\n\ Some("Chat: Message opened".to_string()) ); - assert_eq!(message.parts.len(), 0); + assert_eq!(message.parts.len(), 2); assert_eq!(message.reports.len(), 2); } @@ -1467,7 +1477,7 @@ Additional-Message-IDs: \n\ Some("Chat: Message opened".to_string()) ); - assert_eq!(message.parts.len(), 0); + assert_eq!(message.parts.len(), 1); assert_eq!(message.reports.len(), 1); assert_eq!(message.reports[0].original_message_id, "foo@example.org"); assert_eq!( From 8f7a456a39ef420570f331a503b669a75ce5bc66 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 1 Mar 2020 19:22:29 +0300 Subject: [PATCH 29/73] Use 0 value for "delete_server_after" default. Now 0 means "never delete", 1 means "delete at once" and other values indicate the number of seconds after which them message should be deleted from the server. Configuration value interpretation is moved into Context.get_config_delete_server_after() function. --- src/config.rs | 19 +++++++++++++++++-- src/dc_receive_imf.rs | 6 +++--- src/imap/mod.rs | 6 +++--- src/job.rs | 5 ++++- 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/config.rs b/src/config.rs index 2e4afd6a6..beea4d46b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -68,9 +68,12 @@ pub enum Config { /// Timer in seconds after which the message is deleted from the /// server. /// - /// Equals to -1 by default, which means the message is never + /// Equals to 0 by default, which means the message is never /// deleted. - #[strum(props(default = "-1"))] + /// + /// Value 1 is treated as "delete at once": messages are deleted + /// immediately, without moving to DeltaChat folder. + #[strum(props(default = "0"))] DeleteServerAfter, SaveMimeHeaders, @@ -136,6 +139,18 @@ impl Context { self.get_config_int(key) != 0 } + /// Gets configured "delete_server_after" value. + /// + /// `None` means never delete the message, `Some(0)` means delete + /// at once, `Some(x)` means delete after `x` seconds. + pub fn get_config_delete_server_after(&self) -> Option { + match self.get_config_int(Config::DeleteServerAfter) { + 0 => None, + 1 => Some(0), + x => Some(x as i64), + } + } + /// Set the given config key. /// If `None` is passed as a value the value is cleared and set to the default if there is one. pub fn set_config(&self, key: Config, value: Option<&str>) -> crate::sql::Result<()> { diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 3cd71cfbf..8a517d2ae 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -204,14 +204,14 @@ pub fn dc_receive_imf( } } else { // Get user-configured server deletion - let delete_server_after = context.get_config_int(Config::DeleteServerAfter); + let delete_server_after = context.get_config_delete_server_after(); - if delete_server_after != 0 { + if delete_server_after != Some(0) { // Move message if we don't delete it immediately. context.do_heuristics_moves(server_folder.as_ref(), insert_msg_id); } - if delete_server_after >= 0 { + if let Some(delete_server_after) = delete_server_after { info!( context, "Scheduling message deletion in {} seconds", delete_server_after diff --git a/src/imap/mod.rs b/src/imap/mod.rs index a0e699942..c0c118da0 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1308,9 +1308,9 @@ fn precheck_imf(context: &Context, rfc724_mid: &str, server_folder: &str, server if old_server_folder.is_empty() && old_server_uid == 0 { info!(context, "[move] detected bcc-self {}", rfc724_mid,); - let delete_server_after = context.get_config_int(Config::DeleteServerAfter); + let delete_server_after = context.get_config_delete_server_after(); - if delete_server_after != 0 { + if delete_server_after != Some(0) { context.do_heuristics_moves(server_folder.as_ref(), msg_id); job_add( context, @@ -1321,7 +1321,7 @@ fn precheck_imf(context: &Context, rfc724_mid: &str, server_folder: &str, server ); } - if delete_server_after >= 0 { + if let Some(delete_server_after) = delete_server_after { info!( context, "Scheduling BCC-self deletion in {} seconds", delete_server_after diff --git a/src/job.rs b/src/job.rs index c8d75e901..160401d85 100644 --- a/src/job.rs +++ b/src/job.rs @@ -859,8 +859,11 @@ pub fn job_send_msg(context: &Context, msg_id: MsgId) -> Result<()> { .get_config(Config::ConfiguredAddr) .unwrap_or_default(); let lowercase_from = from.to_lowercase(); + + // Send BCC to self if it is enabled and we are not going to + // delete it immediately. if context.get_config_bool(Config::BccSelf) - && context.get_config_int(Config::DeleteServerAfter) != 0 + && context.get_config_delete_server_after() != Some(0) && !recipients .iter() .any(|x| x.to_lowercase() == lowercase_from) From 28af919b098f90f4e1a833f2761b2452babfad08 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 7 Mar 2020 01:28:45 +0300 Subject: [PATCH 30/73] Create DeleteMsgOnImap jobs before performing IMAP jobs When "delete_server_after" setting is configured, postponed DeleteMsgOnImap jobs are created for incoming messages. This commit adds job::add_imap_deletion_jobs function which creates DeleteMsgOnImap jobs right before performing IMAP jobs. This way even messages that expired when the setting was disabled are going to be deleted. Job creation on message reception is unnecessary now, and even harmful because it will create jobs with an expiration time which may later be reduced. It is planned to be removed in following commits. --- src/job.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/job.rs b/src/job.rs index 160401d85..a84ca30dc 100644 --- a/src/job.rs +++ b/src/job.rs @@ -939,6 +939,40 @@ pub fn job_send_msg(context: &Context, msg_id: MsgId) -> Result<()> { Ok(()) } +fn add_imap_deletion_jobs(context: &Context) -> sql::Result<()> { + if let Some(delete_server_after) = context.get_config_delete_server_after() { + let threshold_timestamp = time() - delete_server_after; + + // Select all expired messages which don't have a + // corresponding message deletion job yet. + let msg_ids = context.sql.query_map( + "SELECT id FROM msgs \ + WHERE timestamp < ? \ + AND server_uid != 0 \ + AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id)", + params![threshold_timestamp], + |row| row.get::<_, MsgId>(0), + |ids| { + ids.collect::, _>>() + .map_err(Into::into) + }, + )?; + + // Schedule IMAP deletion for expired messages. + for msg_id in msg_ids { + job_add( + context, + Action::DeleteMsgOnImap, + msg_id.to_u32() as i32, + Params::new(), + 0, + ) + } + } + + Ok(()) +} + pub fn perform_inbox_jobs(context: &Context) { info!(context, "dc_perform_inbox_jobs starting.",); @@ -946,6 +980,9 @@ pub fn perform_inbox_jobs(context: &Context) { *context.probe_imap_network.write().unwrap() = false; *context.perform_inbox_jobs_needed.write().unwrap() = false; + if let Err(err) = add_imap_deletion_jobs(context) { + warn!(context, "Can't add IMAP message deletion jobs: {}", err); + } job_perform(context, Thread::Imap, probe_imap_network); info!(context, "dc_perform_inbox_jobs ended.",); } From 6db03356b51199d1de9c4335a301d855f2a4ab91 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 7 Mar 2020 02:51:31 +0300 Subject: [PATCH 31/73] Return AlreadyDone from Imap.delete_msg if message is gone This way DeleteMsgOnImap will remove invalid server_uid from the database. --- src/imap/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index c0c118da0..b7de59b5c 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1019,7 +1019,7 @@ impl Imap { display_imap_id, message_id, ); - return ImapActionResult::Failed; + return ImapActionResult::AlreadyDone; }; let remote_message_id = get_fetch_headers(fetch) From 314c3d5e7818ddb7901164ca208dfb1d6eccc4f7 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 7 Mar 2020 18:57:17 +0300 Subject: [PATCH 32/73] add_imap_deletion_jobs: check only for DeleteMsgOnImap jobs Other jobs may have different meaning for foreign_id. Also removed " \" at the end of lines. --- src/job.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/job.rs b/src/job.rs index a84ca30dc..4d07057c2 100644 --- a/src/job.rs +++ b/src/job.rs @@ -946,11 +946,12 @@ fn add_imap_deletion_jobs(context: &Context) -> sql::Result<()> { // Select all expired messages which don't have a // corresponding message deletion job yet. let msg_ids = context.sql.query_map( - "SELECT id FROM msgs \ - WHERE timestamp < ? \ - AND server_uid != 0 \ - AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id)", - params![threshold_timestamp], + "SELECT id FROM msgs + WHERE timestamp < ? + AND server_uid != 0 + AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id + AND action = ?)", + params![threshold_timestamp, Action::DeleteMsgOnImap], |row| row.get::<_, MsgId>(0), |ids| { ids.collect::, _>>() From 055aba189c9668bc2b7e74369edd6c1c46c853f6 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 7 Mar 2020 19:10:13 +0300 Subject: [PATCH 33/73] Automatically delete messages in 2 weeks window only This is to avoid creating thousands of jobs when user enables "delete_server_after" setting for the first time. If device is offline for more than 2 weeks, some messages may not be deleted. --- src/job.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/job.rs b/src/job.rs index 4d07057c2..4ad83b762 100644 --- a/src/job.rs +++ b/src/job.rs @@ -943,15 +943,25 @@ fn add_imap_deletion_jobs(context: &Context) -> sql::Result<()> { if let Some(delete_server_after) = context.get_config_delete_server_after() { let threshold_timestamp = time() - delete_server_after; + // Only try to delete messages in 2 weeks window + // (oldest_timestamp, threshold_timestamp). Otherwise we may + // waste a lot of traffic attempting to delete all the + // messages since the beginning of Delta Chat usage. + let oldest_timestamp = threshold_timestamp - 2 * 7 * 24 * 60 * 60; + // Select all expired messages which don't have a // corresponding message deletion job yet. let msg_ids = context.sql.query_map( "SELECT id FROM msgs - WHERE timestamp < ? + WHERE timestamp < ? AND timestamp > ? AND server_uid != 0 AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id AND action = ?)", - params![threshold_timestamp, Action::DeleteMsgOnImap], + params![ + threshold_timestamp, + oldest_timestamp, + Action::DeleteMsgOnImap + ], |row| row.get::<_, MsgId>(0), |ids| { ids.collect::, _>>() From c4677190be584bb6501a0ea7e359b8dea128526d Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 7 Mar 2020 20:33:56 +0300 Subject: [PATCH 34/73] Postpone DeleteMsgOnImap on error If job returns Status::Finished, it will be deleted. Then add_imap_deletion_jobs will recreate it immediately if the message is expired. To actually backoff the job, we should postpone it instead of removing. --- src/job.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/job.rs b/src/job.rs index 4ad83b762..1fc75336c 100644 --- a/src/job.rs +++ b/src/job.rs @@ -469,12 +469,16 @@ impl Job { imap_inbox.delete_msg(context, &mid, server_folder, msg.server_uid) }; match res { - ImapActionResult::RetryLater => { - return Status::RetryLater; - } ImapActionResult::AlreadyDone | ImapActionResult::Success => {} - ImapActionResult::Failed => { - return Status::Finished(Err(format_err!("Message deletion failed"))); + ImapActionResult::RetryLater | ImapActionResult::Failed => { + // If job has failed, for example due to some + // IMAP bug, we postpone it instead of failing + // immediately. This will prevent adding it + // immediately again if user has enabled + // automatic message deletion. Without this, + // we might waste a lot of traffic constantly + // retrying message deletion. + return Status::RetryLater; } } } From bdb2a47743c048377c439044a108e2cc20471894 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 7 Mar 2020 20:48:04 +0300 Subject: [PATCH 35/73] Revert "Automatically delete messages in 2 weeks window only" This may result in messages not being deleted. Optimization and traffic-saving is postponed for later, one idea is to optimize message deletion to avoid checking if Message-ID on the server matches Message-ID in the database. --- src/job.rs | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/job.rs b/src/job.rs index 1fc75336c..9a55ba22c 100644 --- a/src/job.rs +++ b/src/job.rs @@ -947,25 +947,15 @@ fn add_imap_deletion_jobs(context: &Context) -> sql::Result<()> { if let Some(delete_server_after) = context.get_config_delete_server_after() { let threshold_timestamp = time() - delete_server_after; - // Only try to delete messages in 2 weeks window - // (oldest_timestamp, threshold_timestamp). Otherwise we may - // waste a lot of traffic attempting to delete all the - // messages since the beginning of Delta Chat usage. - let oldest_timestamp = threshold_timestamp - 2 * 7 * 24 * 60 * 60; - // Select all expired messages which don't have a // corresponding message deletion job yet. let msg_ids = context.sql.query_map( "SELECT id FROM msgs - WHERE timestamp < ? AND timestamp > ? + WHERE timestamp < ? AND server_uid != 0 AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id AND action = ?)", - params![ - threshold_timestamp, - oldest_timestamp, - Action::DeleteMsgOnImap - ], + params![threshold_timestamp, Action::DeleteMsgOnImap], |row| row.get::<_, MsgId>(0), |ids| { ids.collect::, _>>() From efb7280e99926a18b4cf1f69fe1887198040a0ef Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 01:24:17 +0300 Subject: [PATCH 36/73] Do not schedule delayed DeleteMsgOnImap jobs All IMAP deletion jobs are scheduled either immediately, or later by job::add_imap_deletion_jobs(). --- src/dc_receive_imf.rs | 32 ++++++++------------------------ src/imap/mod.rs | 29 ----------------------------- src/message.rs | 22 ---------------------- 3 files changed, 8 insertions(+), 75 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 8a517d2ae..a99a14c55 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -192,39 +192,23 @@ pub fn dc_receive_imf( }; } - if needs_delete_job && !created_db_entries.is_empty() { - for db_entry in &created_db_entries { - job_add( - context, - Action::DeleteMsgOnImap, - db_entry.1.to_u32() as i32, - Params::new(), - 0, - ); - } - } else { - // Get user-configured server deletion - let delete_server_after = context.get_config_delete_server_after(); + // Get user-configured server deletion + let delete_server_after = context.get_config_delete_server_after(); - if delete_server_after != Some(0) { - // Move message if we don't delete it immediately. - context.do_heuristics_moves(server_folder.as_ref(), insert_msg_id); - } - - if let Some(delete_server_after) = delete_server_after { - info!( - context, - "Scheduling message deletion in {} seconds", delete_server_after - ); + if !created_db_entries.is_empty() { + if needs_delete_job || delete_server_after == Some(0) { for db_entry in &created_db_entries { job_add( context, Action::DeleteMsgOnImap, db_entry.1.to_u32() as i32, Params::new(), - delete_server_after as i64, + 0, ); } + } else { + // Move message if we don't delete it immediately. + context.do_heuristics_moves(server_folder.as_ref(), insert_msg_id); } } diff --git a/src/imap/mod.rs b/src/imap/mod.rs index b7de59b5c..554fd8e1c 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1320,35 +1320,6 @@ fn precheck_imf(context: &Context, rfc724_mid: &str, server_folder: &str, server 0, ); } - - if let Some(delete_server_after) = delete_server_after { - info!( - context, - "Scheduling BCC-self deletion in {} seconds", delete_server_after - ); - - let msg_ids = match message::get_all_by_rfc724_mid(context, &rfc724_mid) { - Err(err) => { - warn!( - context, - "Cannot get all database records for Message-ID {}: {}", - &rfc724_mid, - err - ); - vec![msg_id] // Remove at least the MsgId we know about - } - Ok(msg_ids) => msg_ids, - }; - for part_msg_id in msg_ids { - job_add( - context, - Action::DeleteMsgOnImap, - part_msg_id.to_u32() as i32, - Params::new(), - delete_server_after as i64, - ); - } - } } else if old_server_folder != server_folder { info!(context, "[move] detected moved message {}", rfc724_mid,); } diff --git a/src/message.rs b/src/message.rs index c2fe89c17..58a8893e6 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1410,28 +1410,6 @@ pub(crate) fn rfc724_mid_exists( .map_err(Into::into) } -/// Returns all MsgIds corresponding to Message-ID -pub(crate) fn get_all_by_rfc724_mid( - context: &Context, - rfc724_mid: &str, -) -> Result, Error> { - ensure!(!rfc724_mid.is_empty(), "empty rfc724_mid"); - - let msg_ids = context.sql.query_map( - "SELECT id FROM msgs WHERE rfc724_mid=?", - params![rfc724_mid], - |row| row.get::<_, MsgId>("id"), - |ids| { - let mut ret: Vec = Vec::new(); - for id in ids { - ret.push(id?); - } - Ok(ret) - }, - )?; - Ok(msg_ids) -} - pub fn update_server_uid( context: &Context, rfc724_mid: &str, From 62097765a67d9eef3859357ee055437d690df165 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 05:51:18 +0300 Subject: [PATCH 37/73] update_server_uid: set server_uid even for unlinked messages Sometimes message deletion job marks message as unlinked without actually deleting it. It is possible if the message was already moved into another folder, possibly by second device, but not detected there yet. It should be detected later in the other folder, and the server_uid in the database should be set. Since introduction of add_imap_deletion_jobs() any expired message record will be marked as unlinked eventually, because another message deletion job will be scheduled even if update_server_uid() resurrects the message once. --- src/message.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/message.rs b/src/message.rs index 58a8893e6..3ed158274 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1417,8 +1417,8 @@ pub fn update_server_uid( server_uid: u32, ) { match context.sql.execute( - "UPDATE msgs SET server_folder=?, server_uid=? \ - WHERE rfc724_mid=? AND NOT unlinked", + "UPDATE msgs SET server_folder=?, server_uid=?, unlinked=0 \ + WHERE rfc724_mid=?", params![server_folder.as_ref(), server_uid, rfc724_mid], ) { Ok(_) => {} From ad53678c19326a271a8f81cfe767fe29eb3a1c7f Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 06:11:37 +0300 Subject: [PATCH 38/73] Remove msgs.unlinked column It is not used anymore. Database version 64 migration introducing this column is also removed. --- src/message.rs | 6 +++--- src/sql.rs | 8 -------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/message.rs b/src/message.rs index 3ed158274..32ef942b5 100644 --- a/src/message.rs +++ b/src/message.rs @@ -128,7 +128,7 @@ impl MsgId { context, &context.sql, "UPDATE msgs \ - SET unlinked=1, server_folder='', server_uid=0 \ + SET server_folder='', server_uid=0 \ WHERE id=?", params![self], ) @@ -1376,7 +1376,7 @@ pub fn get_deaddrop_msg_cnt(context: &Context) -> usize { pub fn rfc724_mid_cnt(context: &Context, rfc724_mid: &str) -> i32 { // check the number of messages with the same rfc724_mid match context.sql.query_row( - "SELECT COUNT(*) FROM msgs WHERE rfc724_mid=? AND NOT unlinked", + "SELECT COUNT(*) FROM msgs WHERE rfc724_mid=? AND NOT server_uid = 0", &[rfc724_mid], |row| row.get(0), ) { @@ -1417,7 +1417,7 @@ pub fn update_server_uid( server_uid: u32, ) { match context.sql.execute( - "UPDATE msgs SET server_folder=?, server_uid=?, unlinked=0 \ + "UPDATE msgs SET server_folder=?, server_uid=? \ WHERE rfc724_mid=?", params![server_folder.as_ref(), server_uid, rfc724_mid], ) { diff --git a/src/sql.rs b/src/sql.rs index d1992683f..8e15d1fdd 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -898,14 +898,6 @@ fn open( sql.execute("UPDATE chats SET grpid='' WHERE type=100", NO_PARAMS)?; sql.set_raw_config_int(context, "dbversion", 63)?; } - if dbversion < 64 { - info!(context, "[migration] v64"); - sql.execute( - "ALTER TABLE msgs ADD COLUMN unlinked INTEGER DEFAULT 0", - NO_PARAMS, - )?; - sql.set_raw_config_int(context, "dbversion", 64)?; - } // (2) updates that require high-level objects // (the structure is complete now and all objects are usable) From cb0c00bc6df11284ec9be09555c6c6684cd42104 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 18:08:35 +0300 Subject: [PATCH 39/73] Log rfc724_mid in DeleteMsgOnImap --- src/job.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/job.rs b/src/job.rs index 9a55ba22c..8731fdb7d 100644 --- a/src/job.rs +++ b/src/job.rs @@ -452,7 +452,14 @@ impl Job { let msg = job_try!(Message::load_from_db(context, MsgId::new(self.foreign_id))); if !msg.rfc724_mid.is_empty() { - if message::rfc724_mid_cnt(context, &msg.rfc724_mid) > 1 { + let cnt = message::rfc724_mid_cnt(context, &msg.rfc724_mid); + info!( + context, + "Running delete job for message {} which has {} entries in the database", + &msg.rfc724_mid, + cnt + ); + if cnt > 1 { info!( context, "The message is deleted from the server when all parts are deleted.", From 65fdfac86689376082c4316e21727876a08ee6dc Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 21:24:03 +0300 Subject: [PATCH 40/73] Remove unused dest_uid argument from Imap.mv() It was always set to 0 because we don't know the destination UID. --- src/imap/mod.rs | 5 ----- src/job.rs | 19 ++++++++----------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index 554fd8e1c..6209f5e10 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -794,7 +794,6 @@ impl Imap { folder: &str, uid: u32, dest_folder: &str, - dest_uid: &mut u32, ) -> ImapActionResult { task::block_on(async move { if folder == dest_folder { @@ -811,10 +810,6 @@ impl Imap { return imapresult; } // we are connected, and the folder is selected - - // XXX Rust-Imap provides no target uid on mv, so just set it to 0 - *dest_uid = 0; - let set = format!("{}", uid); let display_folder_id = format!("{}/{}", folder, uid); diff --git a/src/job.rs b/src/job.rs index 8731fdb7d..31933686b 100644 --- a/src/job.rs +++ b/src/job.rs @@ -413,18 +413,12 @@ impl Job { if let Some(dest_folder) = dest_folder { let server_folder = msg.server_folder.as_ref().unwrap(); - let mut dest_uid = 0; - match imap_inbox.mv( - context, - server_folder, - msg.server_uid, - &dest_folder, - &mut dest_uid, - ) { + match imap_inbox.mv(context, server_folder, msg.server_uid, &dest_folder) { ImapActionResult::RetryLater => Status::RetryLater, ImapActionResult::Success => { - message::update_server_uid(context, &msg.rfc724_mid, &dest_folder, dest_uid); + // XXX Rust-Imap provides no target uid on mv, so just set it to 0 + message::update_server_uid(context, &msg.rfc724_mid, &dest_folder, 0); Status::Finished(Ok(())) } ImapActionResult::Failed => { @@ -583,12 +577,15 @@ impl Job { .sql .get_raw_config(context, "configured_mvbox_folder"); if let Some(dest_folder) = dest_folder { - let mut dest_uid = 0; if ImapActionResult::RetryLater - == imap_inbox.mv(context, &folder, uid, &dest_folder, &mut dest_uid) + == imap_inbox.mv(context, &folder, uid, &dest_folder) { Status::RetryLater } else { + // FIXME: server UID should be updated for all + // hidden MDN entries, but it does not happen + // because we don't know Message-ID here and can't + // find corresponding MsgId. Status::Finished(Ok(())) } } else { From 41f776763b549faa8cd2db9605e5a49dd296238a Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 21:43:05 +0300 Subject: [PATCH 41/73] Remove MarkseenMdnOnImap MarkseenMdnOnImap stored server folder and UID which are never updated by update_server_uid. Now hidden entries are created for MDNs, so they should be handled as ordinary messages. --- src/dc_receive_imf.rs | 2 +- src/job.rs | 43 ------------------------------------------- src/mimeparser.rs | 23 +---------------------- 3 files changed, 2 insertions(+), 66 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index a99a14c55..b50a66323 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -219,7 +219,7 @@ pub fn dc_receive_imf( cleanup(context, &create_event_to_send, created_db_entries); - mime_parser.handle_reports(context, from_id, sent_timestamp, &server_folder, server_uid); + mime_parser.handle_reports(context, from_id, sent_timestamp); Ok(()) } diff --git a/src/job.rs b/src/job.rs index 31933686b..c0fdfad49 100644 --- a/src/job.rs +++ b/src/job.rs @@ -81,7 +81,6 @@ pub enum Action { Housekeeping = 105, // low priority ... EmptyServer = 107, DeleteMsgOnImap = 110, - MarkseenMdnOnImap = 120, MarkseenMsgOnImap = 130, MoveMsg = 200, ConfigureImap = 900, @@ -110,7 +109,6 @@ impl From for Thread { Housekeeping => Thread::Imap, DeleteMsgOnImap => Thread::Imap, EmptyServer => Thread::Imap, - MarkseenMdnOnImap => Thread::Imap, MarkseenMsgOnImap => Thread::Imap, MoveMsg => Thread::Imap, ConfigureImap => Thread::Imap, @@ -555,46 +553,6 @@ impl Job { } } } - - #[allow(non_snake_case)] - fn MarkseenMdnOnImap(&mut self, context: &Context) -> Status { - let folder = self - .param - .get(Param::ServerFolder) - .unwrap_or_default() - .to_string(); - let uid = self.param.get_int(Param::ServerUid).unwrap_or_default() as u32; - let imap_inbox = &context.inbox_thread.read().unwrap().imap; - if imap_inbox.set_seen(context, &folder, uid) == ImapActionResult::RetryLater { - return Status::RetryLater; - } - if self.param.get_bool(Param::AlsoMove).unwrap_or_default() { - if let Err(err) = imap_inbox.ensure_configured_folders(context, true) { - warn!(context, "configuring folders failed: {:?}", err); - return Status::RetryLater; - } - let dest_folder = context - .sql - .get_raw_config(context, "configured_mvbox_folder"); - if let Some(dest_folder) = dest_folder { - if ImapActionResult::RetryLater - == imap_inbox.mv(context, &folder, uid, &dest_folder) - { - Status::RetryLater - } else { - // FIXME: server UID should be updated for all - // hidden MDN entries, but it does not happen - // because we don't know Message-ID here and can't - // find corresponding MsgId. - Status::Finished(Ok(())) - } - } else { - Status::Finished(Err(format_err!("MVBOX is not configured"))) - } - } else { - Status::Finished(Ok(())) - } - } } /* delete all pending jobs with the given action */ @@ -1135,7 +1093,6 @@ fn perform_job_action(context: &Context, mut job: &mut Job, thread: Thread, trie Action::EmptyServer => job.EmptyServer(context), Action::DeleteMsgOnImap => job.DeleteMsgOnImap(context), Action::MarkseenMsgOnImap => job.MarkseenMsgOnImap(context), - Action::MarkseenMdnOnImap => job.MarkseenMdnOnImap(context), Action::MoveMsg => job.MoveMsg(context), Action::SendMdn => job.SendMdn(context), Action::ConfigureImap => JobConfigureImap(context), diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 376a7d9ad..b1c14bd22 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -7,7 +7,6 @@ use mailparse::{DispositionType, MailAddr, MailHeaderMap}; use crate::aheader::Aheader; use crate::bail; use crate::blob::BlobObject; -use crate::config::Config; use crate::constants::Viewtype; use crate::contact::*; use crate::context::Context; @@ -17,7 +16,6 @@ use crate::e2ee; use crate::error::Result; use crate::events::Event; use crate::headerdef::{HeaderDef, HeaderDefMap}; -use crate::job::{job_add, Action}; use crate::location; use crate::message; use crate::param::*; @@ -805,19 +803,11 @@ impl MimeMessage { } /// Handle reports (only MDNs for now) - pub fn handle_reports( - &self, - context: &Context, - from_id: u32, - sent_timestamp: i64, - server_folder: impl AsRef, - server_uid: u32, - ) { + pub fn handle_reports(&self, context: &Context, from_id: u32, sent_timestamp: i64) { if self.reports.is_empty() { return; } - let mut mdn_recognized = false; for report in &self.reports { for original_message_id in std::iter::once(&report.original_message_id).chain(&report.additional_message_ids) @@ -826,20 +816,9 @@ impl MimeMessage { message::mdn_from_ext(context, from_id, original_message_id, sent_timestamp) { context.call_cb(Event::MsgRead { chat_id, msg_id }); - mdn_recognized = true; } } } - - if self.has_chat_version() || mdn_recognized { - let mut param = Params::new(); - param.set(Param::ServerFolder, server_folder.as_ref()); - param.set_int(Param::ServerUid, server_uid as i32); - if self.has_chat_version() && context.get_config_bool(Config::MvboxMove) { - param.set_int(Param::AlsoMove, 1); - } - job_add(context, Action::MarkseenMdnOnImap, 0, param, 0); - } } } From 491f83c86d98f27d43aabc202344d6785ddfd4ca Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 22:13:31 +0300 Subject: [PATCH 42/73] Remove ServerFolder and ServerUid job parameters They were used by MarkseenMdnOnImap --- src/param.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/param.rs b/src/param.rs index 48df7ff82..03f53bd10 100644 --- a/src/param.rs +++ b/src/param.rs @@ -88,12 +88,6 @@ pub enum Param { /// For Jobs SetLongitude = b'n', - /// For Jobs - ServerFolder = b'Z', - - /// For Jobs - ServerUid = b'z', - /// For Jobs AlsoMove = b'M', From 33150615a186423bd4309388044af17458257eb3 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 8 Mar 2020 22:41:47 +0300 Subject: [PATCH 43/73] Improve logging for server UID updates in precheck_imf() Log all folders and UIDs and warn about UID changes without folder change. --- src/imap/mod.rs | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index 6209f5e10..c9f90a2bd 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1301,7 +1301,10 @@ fn precheck_imf(context: &Context, rfc724_mid: &str, server_folder: &str, server message::rfc724_mid_exists(context, &rfc724_mid) { if old_server_folder.is_empty() && old_server_uid == 0 { - info!(context, "[move] detected bcc-self {}", rfc724_mid,); + info!( + context, + "[move] detected bcc-self {} as {}/{}", rfc724_mid, server_folder, server_uid + ); let delete_server_after = context.get_config_delete_server_after(); @@ -1316,7 +1319,34 @@ fn precheck_imf(context: &Context, rfc724_mid: &str, server_folder: &str, server ); } } else if old_server_folder != server_folder { - info!(context, "[move] detected moved message {}", rfc724_mid,); + info!( + context, + "[move] detected message {} moved by other device from {}/{} to {}/{}", + rfc724_mid, + old_server_folder, + old_server_uid, + server_folder, + server_uid + ); + } else if old_server_uid == 0 { + info!( + context, + "[move] detected message {} moved by us from {}/{} to {}/{}", + rfc724_mid, + old_server_folder, + old_server_uid, + server_folder, + server_uid + ); + } else if old_server_uid != server_uid { + warn!( + context, + "UID for message {} in folder {} changed from {} to {}", + rfc724_mid, + server_folder, + old_server_uid, + server_uid + ); } if old_server_folder != server_folder || old_server_uid != server_uid { From 4f73812673915b28153cf3a7425d5eead72fc73d Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 9 Mar 2020 01:31:46 +0300 Subject: [PATCH 44/73] Prioritize message deletion over message moving If Delta Chat goes online and gets an expired message is in the Inbox, it should delete it instead of moving and then deleting. --- src/job.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/job.rs b/src/job.rs index c0fdfad49..6784c6bf2 100644 --- a/src/job.rs +++ b/src/job.rs @@ -80,9 +80,14 @@ pub enum Action { // Jobs in the INBOX-thread, range from DC_IMAP_THREAD..DC_IMAP_THREAD+999 Housekeeping = 105, // low priority ... EmptyServer = 107, - DeleteMsgOnImap = 110, + OldDeleteMsgOnImap = 110, MarkseenMsgOnImap = 130, + + // Moving message is prioritized lower than deletion so we don't + // bother moving message if it is already scheduled for deletion. MoveMsg = 200, + + DeleteMsgOnImap = 210, ConfigureImap = 900, ImexImap = 910, // ... high priority @@ -107,6 +112,7 @@ impl From for Thread { Unknown => Thread::Unknown, Housekeeping => Thread::Imap, + OldDeleteMsgOnImap => Thread::Imap, DeleteMsgOnImap => Thread::Imap, EmptyServer => Thread::Imap, MarkseenMsgOnImap => Thread::Imap, @@ -1091,6 +1097,7 @@ fn perform_job_action(context: &Context, mut job: &mut Job, thread: Thread, trie Action::Unknown => Status::Finished(Err(format_err!("Unknown job id found"))), Action::SendMsgToSmtp => job.SendMsgToSmtp(context), Action::EmptyServer => job.EmptyServer(context), + Action::OldDeleteMsgOnImap => job.DeleteMsgOnImap(context), Action::DeleteMsgOnImap => job.DeleteMsgOnImap(context), Action::MarkseenMsgOnImap => job.MarkseenMsgOnImap(context), Action::MoveMsg => job.MoveMsg(context), From d1a4c82937acef02a71df082950d1852df0a1818 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 9 Mar 2020 00:06:51 +0100 Subject: [PATCH 45/73] prototype ffi for ephemeral messages --- deltachat-ffi/deltachat.h | 50 +++++++++++++++++++++------------------ deltachat-ffi/src/lib.rs | 19 +++++++++++++++ src/message.rs | 8 +++++++ 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index afa5770c2..0a8875d2c 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -373,6 +373,14 @@ char* dc_get_blobdir (const dc_context_t* context); * - `save_mime_headers` = 1=save mime headers * and make dc_get_mime_headers() work for subsequent calls, * 0=do not save mime headers (default) + * - `delete_device_after` = 0=do not delete messages from device automatically (default), + * >=1=seconds, after which messages are deleted automatically from the device. + * Messages in the "saved messages" chat (see dc_chat_is_self_talk()) are skipped. + * - `delete_server_after` = 0=do not delete messages from server automatically (default), + * >=1=seconds, after which messages are deleted automatically from the server. + * Also emails matching the `show_emails` settings above are deleted from the server, + * the UI should clearly point that out. + * Messages in the "saved messages" chat (see dc_chat_is_self_talk()) are skipped. * * If you want to retrieve a value, use dc_get_config(). * @@ -1295,6 +1303,21 @@ int dc_get_msg_cnt (dc_context_t* context, uint32_t ch int dc_get_fresh_msg_cnt (dc_context_t* context, uint32_t chat_id); + +/** + * Estimte the number of messages that will be deleted + * by the dc_set_config()-options `delete_device_after` or `delete_server_after`. + * This is typically used to show the estimated impact to the user before actually enabling ephemeral messages. + * + * @param context The context object as returned from dc_context_new(). + * @param from_server 1=Estimate deletion count for server, 0=Estimate deletion count for device + * @param seconds Count messages older than the given number of seconds. + * @return Number of messages that are older than the given number of seconds. + * This includes emails downloaded due to the `show_emails` option. + * Messages in the "saved messages" folder are not counted as they will not be deleted automatically. + */ +int dc_estimate_deletion_cnt (dc_context_t* context, int from_server, int64_t seconds); + /** * Returns the message IDs of all _fresh_ messages of any chat. * Typically used for implementing notification summaries. @@ -1658,8 +1681,9 @@ char* dc_get_mime_headers (dc_context_t* context, uint32_t ms */ void dc_delete_msgs (dc_context_t* context, const uint32_t* msg_ids, int msg_cnt); -/** +/* * Empty IMAP server folder: delete all messages. + * Deprecated, use dc_set_config() with the key "delete_server_after" instead. * * @memberof dc_context_t * @param context The context object as created by dc_context_new() @@ -4127,28 +4151,8 @@ int64_t dc_lot_get_timestamp (const dc_lot_t* lot); */ -/** - * @defgroup DC_EMPTY DC_EMPTY - * - * These constants configure emptying imap folders with dc_empty_server() - * - * @addtogroup DC_EMPTY - * @{ - */ - -/** - * Clear all mvbox messages. - */ -#define DC_EMPTY_MVBOX 0x01 - -/** - * Clear all INBOX messages. - */ -#define DC_EMPTY_INBOX 0x02 - -/** - * @} - */ +#define DC_EMPTY_MVBOX 0x01 // Deprecated, flag for dc_empty_server(): Clear all mvbox messages +#define DC_EMPTY_INBOX 0x02 // Deprecated, flag for dc_empty_server(): Clear all INBOX messages /** diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index a7ccca1be..bf07d9f68 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -1045,6 +1045,25 @@ pub unsafe extern "C" fn dc_get_fresh_msg_cnt( .unwrap_or(0) } +#[no_mangle] +pub unsafe extern "C" fn dc_estimate_deletion_cnt( + context: *mut dc_context_t, + from_server: libc::c_int, + seconds: i64, +) -> libc::c_int { + if context.is_null() || seconds < 0 { + eprintln!("ignoring careless call to dc_estimate_deletion_cnt()"); + return 0; + } + let ffi_context = &*context; + ffi_context + .with_inner(|ctx| { + message::estimate_deletion_cnt(ctx, from_server as bool, seconds).unwrap_or(0) + as libc::c_int + }) + .unwrap_or(0) +} + #[no_mangle] pub unsafe extern "C" fn dc_get_fresh_msgs( context: *mut dc_context_t, diff --git a/src/message.rs b/src/message.rs index 32ef942b5..ee4a2e84a 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1369,6 +1369,14 @@ pub fn get_deaddrop_msg_cnt(context: &Context) -> usize { } } +pub fn estimate_deletion_cnt( + _context: &Context, + _from_server: bool, + _seconds: i64, +) -> Result { + Ok(0) +} + /// Counts number of database records pointing to specified /// Message-ID. /// From aea8a32ba57260b2a8227d704fa58650aacbb9f4 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 9 Mar 2020 00:28:37 +0100 Subject: [PATCH 46/73] add 'estimatedeletion' to repl tool --- examples/repl/cmdline.rs | 11 +++++++++++ examples/repl/main.rs | 13 +++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/examples/repl/cmdline.rs b/examples/repl/cmdline.rs index b065f5845..926287e8f 100644 --- a/examples/repl/cmdline.rs +++ b/examples/repl/cmdline.rs @@ -397,6 +397,7 @@ pub fn dc_cmdline(context: &Context, line: &str) -> Result<(), failure::Error> { providerinfo \n\ event \n\ fileinfo \n\ + estimatedeletion \n\ emptyserver (1=MVBOX 2=INBOX)\n\ clear -- clear screen\n\ exit or quit\n\ @@ -1028,6 +1029,16 @@ pub fn dc_cmdline(context: &Context, line: &str) -> Result<(), failure::Error> { bail!("Command failed."); } } + "estimatedeletion" => { + ensure!(!arg1.is_empty(), "Argument missing"); + let seconds = arg1.parse()?; + let device_cnt = message::estimate_deletion_cnt(context, false, seconds)?; + let server_cnt = message::estimate_deletion_cnt(context, true, seconds)?; + println!( + "estimated count of messages older than {} seconds:\non device: {}\non server: {}", + seconds, device_cnt, server_cnt + ); + } "emptyserver" => { ensure!(!arg1.is_empty(), "Argument missing"); diff --git a/examples/repl/main.rs b/examples/repl/main.rs index c1873829c..e34deadd8 100644 --- a/examples/repl/main.rs +++ b/examples/repl/main.rs @@ -308,8 +308,17 @@ const CONTACT_COMMANDS: [&str; 6] = [ "delcontact", "cleanupcontacts", ]; -const MISC_COMMANDS: [&str; 9] = [ - "getqr", "getbadqr", "checkqr", "event", "fileinfo", "clear", "exit", "quit", "help", +const MISC_COMMANDS: [&str; 10] = [ + "getqr", + "getbadqr", + "checkqr", + "event", + "fileinfo", + "clear", + "exit", + "quit", + "help", + "estimatedeletion", ]; impl Hinter for DcHelper { From 9f19d20344f05d9c7a6a7d116357d99b845aac80 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 9 Mar 2020 01:55:15 +0100 Subject: [PATCH 47/73] implement message estimating --- src/message.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/src/message.rs b/src/message.rs index ee4a2e84a..cdc0f525e 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1370,11 +1370,55 @@ pub fn get_deaddrop_msg_cnt(context: &Context) -> usize { } pub fn estimate_deletion_cnt( - _context: &Context, - _from_server: bool, - _seconds: i64, + context: &Context, + from_server: bool, + seconds: i64, ) -> Result { - Ok(0) + let self_chat_id = chat::lookup_by_contact_id(context, DC_CONTACT_ID_SELF) + .unwrap_or_default() + .0; + let threshold_timestamp = time() - seconds; + + let cnt: isize; + if from_server { + cnt = context.sql.query_row( + "SELECT COUNT(*) + FROM msgs m + WHERE m.id > ? + AND (state = ? OR state >= ?) + AND chat_id != ? + AND timestamp < ? + AND server_uid != 0;", + params![ + DC_MSG_ID_LAST_SPECIAL, + MessageState::InSeen, + MessageState::OutFailed, + self_chat_id, + threshold_timestamp + ], + |row| row.get(0), + )?; + } else { + cnt = context.sql.query_row( + "SELECT COUNT(*) + FROM msgs m + WHERE m.id > ? + AND (state = ? OR state >= ?) + AND chat_id != ? + AND timestamp < ? + AND chat_id != ?;", + params![ + DC_MSG_ID_LAST_SPECIAL, + MessageState::InSeen, + MessageState::OutFailed, + self_chat_id, + threshold_timestamp, + ChatId::new(DC_CHAT_ID_TRASH) + ], + |row| row.get(0), + )?; + } + Ok(cnt as usize) } /// Counts number of database records pointing to specified From be0afdebfdb1b754e4af68d8e0e2c70c906af1d5 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 9 Mar 2020 12:11:01 +0100 Subject: [PATCH 48/73] target comments of @link2xt, fix ci --- deltachat-ffi/deltachat.h | 5 +++-- deltachat-ffi/src/lib.rs | 2 +- src/message.rs | 20 +++++--------------- 3 files changed, 9 insertions(+), 18 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index 0a8875d2c..a2f5976b0 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -376,11 +376,12 @@ char* dc_get_blobdir (const dc_context_t* context); * - `delete_device_after` = 0=do not delete messages from device automatically (default), * >=1=seconds, after which messages are deleted automatically from the device. * Messages in the "saved messages" chat (see dc_chat_is_self_talk()) are skipped. + * Messages are deleted whether they were seen or not, the UI should clearly point that out. * - `delete_server_after` = 0=do not delete messages from server automatically (default), * >=1=seconds, after which messages are deleted automatically from the server. + * Messages in the "saved messages" chat (see dc_chat_is_self_talk()) are skipped. * Also emails matching the `show_emails` settings above are deleted from the server, * the UI should clearly point that out. - * Messages in the "saved messages" chat (see dc_chat_is_self_talk()) are skipped. * * If you want to retrieve a value, use dc_get_config(). * @@ -1305,7 +1306,7 @@ int dc_get_fresh_msg_cnt (dc_context_t* context, uint32_t ch /** - * Estimte the number of messages that will be deleted + * Estimate the number of messages that will be deleted * by the dc_set_config()-options `delete_device_after` or `delete_server_after`. * This is typically used to show the estimated impact to the user before actually enabling ephemeral messages. * diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index bf07d9f68..f1019650b 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -1058,7 +1058,7 @@ pub unsafe extern "C" fn dc_estimate_deletion_cnt( let ffi_context = &*context; ffi_context .with_inner(|ctx| { - message::estimate_deletion_cnt(ctx, from_server as bool, seconds).unwrap_or(0) + message::estimate_deletion_cnt(ctx, from_server != 0, seconds).unwrap_or(0) as libc::c_int }) .unwrap_or(0) diff --git a/src/message.rs b/src/message.rs index cdc0f525e..b3aa67d00 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1385,17 +1385,10 @@ pub fn estimate_deletion_cnt( "SELECT COUNT(*) FROM msgs m WHERE m.id > ? - AND (state = ? OR state >= ?) - AND chat_id != ? AND timestamp < ? + AND chat_id != ? AND server_uid != 0;", - params![ - DC_MSG_ID_LAST_SPECIAL, - MessageState::InSeen, - MessageState::OutFailed, - self_chat_id, - threshold_timestamp - ], + params![DC_MSG_ID_LAST_SPECIAL, threshold_timestamp, self_chat_id], |row| row.get(0), )?; } else { @@ -1403,16 +1396,13 @@ pub fn estimate_deletion_cnt( "SELECT COUNT(*) FROM msgs m WHERE m.id > ? - AND (state = ? OR state >= ?) - AND chat_id != ? AND timestamp < ? - AND chat_id != ?;", + AND chat_id != ? + AND chat_id != ? AND hidden = 0;", params![ DC_MSG_ID_LAST_SPECIAL, - MessageState::InSeen, - MessageState::OutFailed, - self_chat_id, threshold_timestamp, + self_chat_id, ChatId::new(DC_CHAT_ID_TRASH) ], |row| row.get(0), From 248e6ea5e7801f1529ef8a581c683d4e535f3fe0 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 9 Mar 2020 12:27:55 +0100 Subject: [PATCH 49/73] make clippy happy --- src/message.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/message.rs b/src/message.rs index b3aa67d00..703411469 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1379,9 +1379,8 @@ pub fn estimate_deletion_cnt( .0; let threshold_timestamp = time() - seconds; - let cnt: isize; - if from_server { - cnt = context.sql.query_row( + let cnt: isize = if from_server { + context.sql.query_row( "SELECT COUNT(*) FROM msgs m WHERE m.id > ? @@ -1390,9 +1389,9 @@ pub fn estimate_deletion_cnt( AND server_uid != 0;", params![DC_MSG_ID_LAST_SPECIAL, threshold_timestamp, self_chat_id], |row| row.get(0), - )?; + )? } else { - cnt = context.sql.query_row( + context.sql.query_row( "SELECT COUNT(*) FROM msgs m WHERE m.id > ? @@ -1406,8 +1405,8 @@ pub fn estimate_deletion_cnt( ChatId::new(DC_CHAT_ID_TRASH) ], |row| row.get(0), - )?; - } + )? + }; Ok(cnt as usize) } From ff8b249cc65b486663c914e22d8ee0e096e0aff8 Mon Sep 17 00:00:00 2001 From: "B. Petersen" Date: Mon, 9 Mar 2020 16:40:53 +0100 Subject: [PATCH 50/73] For now, 'Saved messages' are auto-deleted from the server, but not from device --- deltachat-ffi/deltachat.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index a2f5976b0..632600d6d 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -379,9 +379,8 @@ char* dc_get_blobdir (const dc_context_t* context); * Messages are deleted whether they were seen or not, the UI should clearly point that out. * - `delete_server_after` = 0=do not delete messages from server automatically (default), * >=1=seconds, after which messages are deleted automatically from the server. - * Messages in the "saved messages" chat (see dc_chat_is_self_talk()) are skipped. - * Also emails matching the `show_emails` settings above are deleted from the server, - * the UI should clearly point that out. + * "Saved messages" are deleted from the server as well as + * emails matching the `show_emails` settings above, the UI should clearly point that out. * * If you want to retrieve a value, use dc_get_config(). * From 9d03d441e1cac523c132ebe26094f4c51b3140d4 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 9 Mar 2020 20:49:56 +0300 Subject: [PATCH 51/73] Add Config::DeleteDeviceAfter option --- src/config.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/config.rs b/src/config.rs index beea4d46b..44f281cba 100644 --- a/src/config.rs +++ b/src/config.rs @@ -76,6 +76,14 @@ pub enum Config { #[strum(props(default = "0"))] DeleteServerAfter, + /// Timer in seconds after which the message is deleted from the + /// device. + /// + /// Equals to 0 by default, which means the message is never + /// deleted. + #[strum(props(default = "0"))] + DeleteDeviceAfter, + SaveMimeHeaders, ConfiguredAddr, ConfiguredMailServer, From 4b742c220cad6dc4019ebe3faa9d6a1487895599 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Mon, 9 Mar 2020 21:15:02 +0300 Subject: [PATCH 52/73] Add Context.get_config_delete_device_after() method In contrast to get_config_delete_server_after(), value 1 does not mean "delete at once", because it does not make sense to delete messages immediately after receivning them. --- src/config.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/config.rs b/src/config.rs index 44f281cba..ce7259c30 100644 --- a/src/config.rs +++ b/src/config.rs @@ -159,6 +159,17 @@ impl Context { } } + /// Gets configured "delete_device_after" value. + /// + /// `None` means never delete the message, `Some(x)` means delete + /// after `x` seconds. + pub fn get_config_delete_device_after(&self) -> Option { + match self.get_config_int(Config::DeleteDeviceAfter) { + 0 => None, + x => Some(x as i64), + } + } + /// Set the given config key. /// If `None` is passed as a value the value is cleared and set to the default if there is one. pub fn set_config(&self, key: Config, value: Option<&str>) -> crate::sql::Result<()> { From 9febc762dababa73ecdb45c9e5c17bacd528b27e Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 10 Mar 2020 04:04:14 +0300 Subject: [PATCH 53/73] Add chat::delete_device_expired_messages() function --- src/chat.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/chat.rs b/src/chat.rs index 921a83181..d184e3ad2 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2508,6 +2508,31 @@ pub(crate) fn add_info_msg(context: &Context, chat_id: ChatId, text: impl AsRef< }); } +/// Hides or deletes messages which are expired according to +/// "delete_device_after" setting. +pub fn delete_device_expired_messages(context: &Context) -> sql::Result<()> { + if let Some(delete_device_after) = context.get_config_delete_device_after() { + let threshold_timestamp = time() - delete_device_after; + + // Hide expired messages + context.sql.execute( + "UPDATE msgs \ + SET txt = '', hidden = 1 \ + WHERE timestamp < ?", + params![threshold_timestamp], + )?; + + // Delete hidden messages that are removed from the server. + context.sql.execute( + "DELETE FROM msgs \ + WHERE (chat_id = ? OR hidden) \ + AND server_uid = 0", + params![DC_CHAT_ID_TRASH], + )?; + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; From 9eb672ea178ce0bdf86177cbdf22e9df2aae4607 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 22 Mar 2020 07:59:43 +0300 Subject: [PATCH 54/73] Move removal of chat message tombstones to a separate function DELETE operation may be slow compared to UPDATE. It is better to do in a separate job. --- src/chat.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index d184e3ad2..ab3e9af98 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2521,18 +2521,22 @@ pub fn delete_device_expired_messages(context: &Context) -> sql::Result<()> { WHERE timestamp < ?", params![threshold_timestamp], )?; - - // Delete hidden messages that are removed from the server. - context.sql.execute( - "DELETE FROM msgs \ - WHERE (chat_id = ? OR hidden) \ - AND server_uid = 0", - params![DC_CHAT_ID_TRASH], - )?; } Ok(()) } +/// Removes from the database locally deleted messages that also don't +/// have a server UID. +pub fn prune_tombstones(context: &Context) -> sql::Result<()> { + context.sql.execute( + "DELETE FROM msgs \ + WHERE (chat_id = ? OR hidden) \ + AND server_uid = 0", + params![DC_CHAT_ID_TRASH], + )?; + Ok(()) +} + #[cfg(test)] mod tests { use super::*; From 25f8a735a9867eca681aede6116d439bb39f522d Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 22 Mar 2020 08:08:05 +0300 Subject: [PATCH 55/73] get_chat_msgs: remove locally expired messages Expired messages are hidden right before retrieving messages from the database, so expired messages are not shown to the user. --- src/chat.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/chat.rs b/src/chat.rs index ab3e9af98..75a236525 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1447,6 +1447,10 @@ pub fn get_chat_msgs( flags: u32, marker1before: Option, ) -> Vec { + if let Err(err) = delete_device_expired_messages(context) { + warn!(context, "Failed to delete expired messages: {}", err); + } + let process_row = |row: &rusqlite::Row| Ok((row.get::<_, MsgId>("id")?, row.get::<_, i64>("timestamp")?)); let process_rows = |rows: rusqlite::MappedRows<_>| { From 270d18a88ae2f6839484acb57655fde1a14a7333 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 24 Mar 2020 22:51:58 +0300 Subject: [PATCH 56/73] Move prune_tombstones() to sql:: and call from housekeeping() --- src/chat.rs | 12 ------------ src/sql.rs | 21 ++++++++++++++++++++- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 75a236525..eba16047a 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2529,18 +2529,6 @@ pub fn delete_device_expired_messages(context: &Context) -> sql::Result<()> { Ok(()) } -/// Removes from the database locally deleted messages that also don't -/// have a server UID. -pub fn prune_tombstones(context: &Context) -> sql::Result<()> { - context.sql.execute( - "DELETE FROM msgs \ - WHERE (chat_id = ? OR hidden) \ - AND server_uid = 0", - params![DC_CHAT_ID_TRASH], - )?; - Ok(()) -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/sql.rs b/src/sql.rs index 8e15d1fdd..51ee65262 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -8,7 +8,7 @@ use rusqlite::{Connection, OpenFlags, Statement, NO_PARAMS}; use thread_local_object::ThreadLocal; use crate::chat::{update_device_icon, update_saved_messages_icon}; -use crate::constants::ShowEmails; +use crate::constants::{ShowEmails, DC_CHAT_ID_TRASH}; use crate::context::Context; use crate::dc_tools::*; use crate::param::*; @@ -1053,6 +1053,18 @@ pub fn get_rowid2_with_conn( } } +/// Removes from the database locally deleted messages that also don't +/// have a server UID. +fn prune_tombstones(context: &Context) -> Result<()> { + context.sql.execute( + "DELETE FROM msgs \ + WHERE (chat_id = ? OR hidden) \ + AND server_uid = 0", + params![DC_CHAT_ID_TRASH], + )?; + Ok(()) +} + pub fn housekeeping(context: &Context) { let mut files_in_use = HashSet::new(); let mut unreferenced_count = 0; @@ -1165,6 +1177,13 @@ pub fn housekeeping(context: &Context) { } } + if let Err(err) = prune_tombstones(context) { + warn!( + context, + "Houskeeping: Cannot prune message tombstones: {}", err + ); + } + info!(context, "Housekeeping done.",); } From 7e67b2cbb363979857019f27853cc88128b6dc93 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Wed, 25 Mar 2020 00:32:42 +0300 Subject: [PATCH 57/73] Do not delete messages from special chats --- src/chat.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index eba16047a..6adca60b9 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2522,8 +2522,9 @@ pub fn delete_device_expired_messages(context: &Context) -> sql::Result<()> { context.sql.execute( "UPDATE msgs \ SET txt = '', hidden = 1 \ - WHERE timestamp < ?", - params![threshold_timestamp], + WHERE timestamp < ? \ + AND chat_id > ?", + params![threshold_timestamp, DC_CHAT_ID_LAST_SPECIAL], )?; } Ok(()) From 4daa57c98e5d7fc3835554c45e5565658d798ce5 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 16:22:59 +0300 Subject: [PATCH 58/73] delete_device_expired_messages: set text to DELETED for hidden messages This makes debugging easier: DELETED messages should never be shown. --- src/chat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chat.rs b/src/chat.rs index 6adca60b9..f219e47d8 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2521,7 +2521,7 @@ pub fn delete_device_expired_messages(context: &Context) -> sql::Result<()> { // Hide expired messages context.sql.execute( "UPDATE msgs \ - SET txt = '', hidden = 1 \ + SET txt = 'DELETED', hidden = 1 \ WHERE timestamp < ? \ AND chat_id > ?", params![threshold_timestamp, DC_CHAT_ID_LAST_SPECIAL], From adaa1e856c04d7af17f444bde5c5ab19d77ec717 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 17:28:49 +0300 Subject: [PATCH 59/73] chat: add ChatId.is_self_talk() and ChatId.is_device_talk() --- src/chat.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/chat.rs b/src/chat.rs index f219e47d8..657f4d7af 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -359,6 +359,25 @@ impl ChatId { .unwrap_or_default() as usize } + pub(crate) fn get_param(self, context: &Context) -> Result { + let res: Option = context + .sql + .query_get_value_result::<_, _>("SELECT param FROM chats WHERE id=?", params![self])?; + Ok(res + .map(|s| s.parse().unwrap_or_default()) + .unwrap_or_default()) + } + + // Returns true if chat is a saved messages chat. + pub fn is_self_talk(self, context: &Context) -> Result { + Ok(self.get_param(context)?.exists(Param::Selftalk)) + } + + /// Returns true if chat is a device chat. + pub fn is_device_talk(self, context: &Context) -> Result { + Ok(self.get_param(context)?.exists(Param::Devicetalk)) + } + /// Bad evil escape hatch. /// /// Avoid using this, eventually types should be cleaned up enough From 051d80b2f33febe906b6e71d05bc50aa5149e3de Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 17:30:05 +0300 Subject: [PATCH 60/73] Move delete_device_expired_messages() to ChatId This allows to check if chat is a self-talk or device chat. Messages are deleted only in viewed chats now. --- src/chat.rs | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 657f4d7af..990638a2e 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -378,6 +378,28 @@ impl ChatId { Ok(self.get_param(context)?.exists(Param::Devicetalk)) } + /// Hides or deletes messages which are expired according to + /// "delete_device_after" setting. + pub fn delete_device_expired_messages(self, context: &Context) -> Result<(), Error> { + if self.is_special() || self.is_self_talk(context)? || self.is_device_talk(context)? { + return Ok(()); + } + + if let Some(delete_device_after) = context.get_config_delete_device_after() { + let threshold_timestamp = time() - delete_device_after; + + // Hide expired messages + context.sql.execute( + "UPDATE msgs \ + SET txt = 'DELETED', hidden = 1 \ + WHERE timestamp < ? \ + AND chat_id == ?", + params![threshold_timestamp, self], + )?; + } + Ok(()) + } + /// Bad evil escape hatch. /// /// Avoid using this, eventually types should be cleaned up enough @@ -1466,7 +1488,7 @@ pub fn get_chat_msgs( flags: u32, marker1before: Option, ) -> Vec { - if let Err(err) = delete_device_expired_messages(context) { + if let Err(err) = chat_id.delete_device_expired_messages(context) { warn!(context, "Failed to delete expired messages: {}", err); } @@ -2531,24 +2553,6 @@ pub(crate) fn add_info_msg(context: &Context, chat_id: ChatId, text: impl AsRef< }); } -/// Hides or deletes messages which are expired according to -/// "delete_device_after" setting. -pub fn delete_device_expired_messages(context: &Context) -> sql::Result<()> { - if let Some(delete_device_after) = context.get_config_delete_device_after() { - let threshold_timestamp = time() - delete_device_after; - - // Hide expired messages - context.sql.execute( - "UPDATE msgs \ - SET txt = 'DELETED', hidden = 1 \ - WHERE timestamp < ? \ - AND chat_id > ?", - params![threshold_timestamp, DC_CHAT_ID_LAST_SPECIAL], - )?; - } - Ok(()) -} - #[cfg(test)] mod tests { use super::*; From 2bf4c5d7e7781007bfc65edc51cd95a570025fad Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 20:13:17 +0300 Subject: [PATCH 61/73] Emit "chat modified" event when messages expire --- src/chat.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/chat.rs b/src/chat.rs index 990638a2e..69b8ca0ad 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -389,13 +389,17 @@ impl ChatId { let threshold_timestamp = time() - delete_device_after; // Hide expired messages - context.sql.execute( + let rows_modified = context.sql.execute( "UPDATE msgs \ SET txt = 'DELETED', hidden = 1 \ WHERE timestamp < ? \ AND chat_id == ?", params![threshold_timestamp, self], )?; + + if rows_modified > 0 { + context.call_cb(Event::ChatModified(self)); + } } Ok(()) } From 3686048ab6aac1bff30ee1f5b6923684500bd3cd Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 21:58:46 +0300 Subject: [PATCH 62/73] chatlist: hide all expired messages before loading --- src/chat.rs | 22 ++++++++++++++++++++++ src/chatlist.rs | 2 ++ 2 files changed, 24 insertions(+) diff --git a/src/chat.rs b/src/chat.rs index 69b8ca0ad..eb022d3d5 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1630,6 +1630,28 @@ pub fn marknoticed_all_chats(context: &Context) -> Result<(), Error> { Ok(()) } +pub fn delete_device_expired_messages_all_chats(context: &Context) -> Result<(), Error> { + let chat_ids = context.sql.query_map( + "SELECT id FROM chats WHERE id > 9", + params![], + |row| row.get::<_, ChatId>(0), + |ids| { + let mut ret = Vec::new(); + for id in ids { + if let Ok(chat_id) = id { + ret.push(chat_id) + } + } + Ok(ret) + }, + )?; + + for chat_id in chat_ids { + chat_id.delete_device_expired_messages(context)?; + } + Ok(()) +} + pub fn get_chat_media( context: &Context, chat_id: ChatId, diff --git a/src/chatlist.rs b/src/chatlist.rs index cc58cb5f1..427f44977 100644 --- a/src/chatlist.rs +++ b/src/chatlist.rs @@ -92,6 +92,8 @@ impl Chatlist { query: Option<&str>, query_contact_id: Option, ) -> Result { + delete_device_expired_messages_all_chats(context)?; + let mut add_archived_link_item = false; let process_row = |row: &rusqlite::Row| { From 237dabb90758747c5a781b945bd018280bf700f3 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 22:09:06 +0300 Subject: [PATCH 63/73] Do not hide hidden messages in ChatId.delete_device_expired_messages() This prevents infinite event loop, when chatlist is reloaded in response to event, and event is emitted by hiding messages before chatlist reload. --- src/chat.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/chat.rs b/src/chat.rs index eb022d3d5..11f365a05 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -389,11 +389,15 @@ impl ChatId { let threshold_timestamp = time() - delete_device_after; // Hide expired messages + // + // Only update the rows that have to be updated, to avoid emitting + // unnecessary "chat modified" events. let rows_modified = context.sql.execute( "UPDATE msgs \ SET txt = 'DELETED', hidden = 1 \ WHERE timestamp < ? \ - AND chat_id == ?", + AND chat_id == ? \ + AND NOT hidden", params![threshold_timestamp, self], )?; From 7522fec0446f88862b13f751350bd3c0b72368f2 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 22:15:47 +0300 Subject: [PATCH 64/73] Do not emit "chat modified" events when loading chatlist Otherwise chatlist will be loaded twice, once right after hiding expired messages, and once in response to event emitted by try_load(). --- src/chat.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 11f365a05..5ad898c40 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -380,9 +380,12 @@ impl ChatId { /// Hides or deletes messages which are expired according to /// "delete_device_after" setting. - pub fn delete_device_expired_messages(self, context: &Context) -> Result<(), Error> { + /// + /// Returns true if any message is hidden, so event can be emitted. If + /// nothing has been hidden, returns false. + pub fn delete_device_expired_messages(self, context: &Context) -> Result { if self.is_special() || self.is_self_talk(context)? || self.is_device_talk(context)? { - return Ok(()); + return Ok(false); } if let Some(delete_device_after) = context.get_config_delete_device_after() { @@ -401,11 +404,10 @@ impl ChatId { params![threshold_timestamp, self], )?; - if rows_modified > 0 { - context.call_cb(Event::ChatModified(self)); - } + Ok(rows_modified > 0) + } else { + Ok(false) } - Ok(()) } /// Bad evil escape hatch. @@ -1496,8 +1498,13 @@ pub fn get_chat_msgs( flags: u32, marker1before: Option, ) -> Vec { - if let Err(err) = chat_id.delete_device_expired_messages(context) { - warn!(context, "Failed to delete expired messages: {}", err); + match chat_id.delete_device_expired_messages(context) { + Err(err) => warn!(context, "Failed to delete expired messages: {}", err), + Ok(messages_deleted) => { + if messages_deleted { + context.call_cb(Event::ChatModified(chat_id)); + } + } } let process_row = From fc57cbfb499c40112496f8d542de008e02f7a4bd Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sat, 28 Mar 2020 23:29:15 +0300 Subject: [PATCH 65/73] Refactor hiding of expired messages Now there is only one function: hide_device_expired_messages(). If any messages are hidden event DC_EVENT_MSGS_CHANGED(0,0) is emitted now, which is more correct than DC_EVENT_CHAT_MODIFIED and also triggers chatlist reload. --- src/chat.rs | 90 +++++++++++++++++++++---------------------------- src/chatlist.rs | 4 ++- 2 files changed, 41 insertions(+), 53 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 5ad898c40..8be63194d 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -378,38 +378,6 @@ impl ChatId { Ok(self.get_param(context)?.exists(Param::Devicetalk)) } - /// Hides or deletes messages which are expired according to - /// "delete_device_after" setting. - /// - /// Returns true if any message is hidden, so event can be emitted. If - /// nothing has been hidden, returns false. - pub fn delete_device_expired_messages(self, context: &Context) -> Result { - if self.is_special() || self.is_self_talk(context)? || self.is_device_talk(context)? { - return Ok(false); - } - - if let Some(delete_device_after) = context.get_config_delete_device_after() { - let threshold_timestamp = time() - delete_device_after; - - // Hide expired messages - // - // Only update the rows that have to be updated, to avoid emitting - // unnecessary "chat modified" events. - let rows_modified = context.sql.execute( - "UPDATE msgs \ - SET txt = 'DELETED', hidden = 1 \ - WHERE timestamp < ? \ - AND chat_id == ? \ - AND NOT hidden", - params![threshold_timestamp, self], - )?; - - Ok(rows_modified > 0) - } else { - Ok(false) - } - } - /// Bad evil escape hatch. /// /// Avoid using this, eventually types should be cleaned up enough @@ -1498,11 +1466,14 @@ pub fn get_chat_msgs( flags: u32, marker1before: Option, ) -> Vec { - match chat_id.delete_device_expired_messages(context) { + match hide_device_expired_messages(context) { Err(err) => warn!(context, "Failed to delete expired messages: {}", err), Ok(messages_deleted) => { if messages_deleted { - context.call_cb(Event::ChatModified(chat_id)); + context.call_cb(Event::MsgsChanged { + msg_id: MsgId::new(0), + chat_id: ChatId::new(0), + }) } } } @@ -1641,26 +1612,41 @@ pub fn marknoticed_all_chats(context: &Context) -> Result<(), Error> { Ok(()) } -pub fn delete_device_expired_messages_all_chats(context: &Context) -> Result<(), Error> { - let chat_ids = context.sql.query_map( - "SELECT id FROM chats WHERE id > 9", - params![], - |row| row.get::<_, ChatId>(0), - |ids| { - let mut ret = Vec::new(); - for id in ids { - if let Ok(chat_id) = id { - ret.push(chat_id) - } - } - Ok(ret) - }, - )?; +/// Hides messages which are expired according to "delete_device_after" setting. +/// +/// Returns true if any message is hidden, so event can be emitted. If nothing +/// has been hidden, returns false. +pub fn hide_device_expired_messages(context: &Context) -> Result { + if let Some(delete_device_after) = context.get_config_delete_device_after() { + let threshold_timestamp = time() - delete_device_after; - for chat_id in chat_ids { - chat_id.delete_device_expired_messages(context)?; + let self_chat_id = lookup_by_contact_id(context, DC_CONTACT_ID_SELF)?.0; + let device_chat_id = lookup_by_contact_id(context, DC_CONTACT_ID_DEVICE)?.0; + + // Hide expired messages + // + // Only update the rows that have to be updated, to avoid emitting + // unnecessary "chat modified" events. + let rows_modified = context.sql.execute( + "UPDATE msgs \ + SET txt = 'DELETED', hidden = 1 \ + WHERE timestamp < ? \ + AND chat_id > ? \ + AND chat_id != ? \ + AND chat_id != ? \ + AND NOT hidden", + params![ + threshold_timestamp, + DC_CHAT_ID_LAST_SPECIAL, + self_chat_id, + device_chat_id + ], + )?; + + Ok(rows_modified > 0) + } else { + Ok(false) } - Ok(()) } pub fn get_chat_media( diff --git a/src/chatlist.rs b/src/chatlist.rs index 427f44977..174c1a252 100644 --- a/src/chatlist.rs +++ b/src/chatlist.rs @@ -92,7 +92,9 @@ impl Chatlist { query: Option<&str>, query_contact_id: Option, ) -> Result { - delete_device_expired_messages_all_chats(context)?; + // Note that we do not emit DC_EVENT_MSGS_MODIFIED here even if some + // messages get hidden to avoid reloading the same chatlist. + hide_device_expired_messages(context)?; let mut add_archived_link_item = false; From e8cc739fbd35e50f42fbc804b63a16bd4e20f722 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Sun, 29 Mar 2020 00:49:59 +0300 Subject: [PATCH 66/73] Reload chatlist when "delete_device_after" is set --- src/config.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/config.rs b/src/config.rs index ce7259c30..61f58ee90 100644 --- a/src/config.rs +++ b/src/config.rs @@ -4,10 +4,13 @@ use strum::{EnumProperty, IntoEnumIterator}; use strum_macros::{AsRefStr, Display, EnumIter, EnumProperty, EnumString}; use crate::blob::BlobObject; +use crate::chat::ChatId; use crate::constants::DC_VERSION_STR; use crate::context::Context; use crate::dc_tools::*; +use crate::events::Event; use crate::job::*; +use crate::message::MsgId; use crate::mimefactory::RECOMMENDED_FILE_SIZE; use crate::stock::StockMessage; use rusqlite::NO_PARAMS; @@ -213,6 +216,15 @@ impl Context { self.sql.set_raw_config(self, key, val) } + Config::DeleteDeviceAfter => { + let ret = self.sql.set_raw_config(self, key, value); + // Force chatlist reload to delete old messages immediately. + self.call_cb(Event::MsgsChanged { + msg_id: MsgId::new(0), + chat_id: ChatId::new(0), + }); + ret + } _ => self.sql.set_raw_config(self, key, value), } } From 4e0a08106d0a2ff8d06cfab0c893f1285a676972 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 31 Mar 2020 02:31:38 +0300 Subject: [PATCH 67/73] ChatId.get_param: remove unnecessary type annotation --- src/chat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chat.rs b/src/chat.rs index 8be63194d..23c5a7725 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -362,7 +362,7 @@ impl ChatId { pub(crate) fn get_param(self, context: &Context) -> Result { let res: Option = context .sql - .query_get_value_result::<_, _>("SELECT param FROM chats WHERE id=?", params![self])?; + .query_get_value_result("SELECT param FROM chats WHERE id=?", params![self])?; Ok(res .map(|s| s.parse().unwrap_or_default()) .unwrap_or_default()) From 1b815a7d96f22631931d878f15a75e8005269ea4 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 31 Mar 2020 02:42:24 +0300 Subject: [PATCH 68/73] Display an error if message cannot be trashed --- src/message.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/message.rs b/src/message.rs index 703411469..1dbcf02d4 100644 --- a/src/message.rs +++ b/src/message.rs @@ -988,7 +988,7 @@ pub fn delete_msgs(context: &Context, msg_ids: &[MsgId]) { } } if let Err(err) = msg_id.trash(context) { - warn!(context, "Unable to trash message {}: {}", msg_id, err); + error!(context, "Unable to trash message {}: {}", msg_id, err); } job_add( context, From 1934181b522058bf76d04c2b9743714958cf67d9 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Tue, 31 Mar 2020 02:44:02 +0300 Subject: [PATCH 69/73] Terminate new SQL statement lines with \ This way rustfmt can change indentation. However, care should be taken to keep a space before each \ --- src/job.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/job.rs b/src/job.rs index 6784c6bf2..7d6f08946 100644 --- a/src/job.rs +++ b/src/job.rs @@ -918,11 +918,11 @@ fn add_imap_deletion_jobs(context: &Context) -> sql::Result<()> { // Select all expired messages which don't have a // corresponding message deletion job yet. let msg_ids = context.sql.query_map( - "SELECT id FROM msgs - WHERE timestamp < ? - AND server_uid != 0 - AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id - AND action = ?)", + "SELECT id FROM msgs \ + WHERE timestamp < ? \ + AND server_uid != 0 \ + AND NOT EXISTS (SELECT 1 FROM jobs WHERE foreign_id = msgs.id \ + AND action = ?)", params![threshold_timestamp, Action::DeleteMsgOnImap], |row| row.get::<_, MsgId>(0), |ids| { From 3163ef87c6ee406abd17b64dec61c051130af365 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Wed, 1 Apr 2020 23:27:15 +0300 Subject: [PATCH 70/73] imap: display errors with {}, not {:?} --- src/imap/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index c9f90a2bd..f3ba8d90d 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -1210,13 +1210,13 @@ impl Imap { return; } if let Err(err) = self.setup_handle_if_needed(context).await { - error!(context, "could not setup imap connection: {:?}", err); + error!(context, "could not setup imap connection: {}", err); return; } if let Err(err) = self.select_folder(context, Some(&folder)).await { error!( context, - "Could not select {} for expunging: {:?}", folder, err + "Could not select {} for expunging: {}", folder, err ); return; } @@ -1236,7 +1236,7 @@ impl Imap { emit_event!(context, Event::ImapFolderEmptied(folder.to_string())); } Err(err) => { - error!(context, "expunge failed {}: {:?}", folder, err); + error!(context, "expunge failed {}: {}", folder, err); } } From 916fab7d4b3555a3f649d8794d9a9e13ef2c6406 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Fri, 3 Apr 2020 12:12:56 +0300 Subject: [PATCH 71/73] hide_device_expired_messages: allow missing self or device chat --- src/chat.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 23c5a7725..e89a0561e 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1620,8 +1620,12 @@ pub fn hide_device_expired_messages(context: &Context) -> Result { if let Some(delete_device_after) = context.get_config_delete_device_after() { let threshold_timestamp = time() - delete_device_after; - let self_chat_id = lookup_by_contact_id(context, DC_CONTACT_ID_SELF)?.0; - let device_chat_id = lookup_by_contact_id(context, DC_CONTACT_ID_DEVICE)?.0; + let self_chat_id = lookup_by_contact_id(context, DC_CONTACT_ID_SELF) + .unwrap_or_default() + .0; + let device_chat_id = lookup_by_contact_id(context, DC_CONTACT_ID_DEVICE) + .unwrap_or_default() + .0; // Hide expired messages // From 9c2a3b8a82bbd54de77809192e3897c2164d2fbb Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Fri, 3 Apr 2020 12:13:42 +0300 Subject: [PATCH 72/73] Chatlist::try_load: make hide_device_expired_messages errors non-fatal --- src/chatlist.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/chatlist.rs b/src/chatlist.rs index 174c1a252..00380ed7f 100644 --- a/src/chatlist.rs +++ b/src/chatlist.rs @@ -94,7 +94,9 @@ impl Chatlist { ) -> Result { // Note that we do not emit DC_EVENT_MSGS_MODIFIED here even if some // messages get hidden to avoid reloading the same chatlist. - hide_device_expired_messages(context)?; + if let Err(err) = hide_device_expired_messages(context) { + warn!(context, "Failed to hide expired messages: {}", err); + } let mut add_archived_link_item = false; From d31265895d2206870cdb0b347a66e7deaad9d7f6 Mon Sep 17 00:00:00 2001 From: Alexander Krotov Date: Thu, 2 Apr 2020 22:42:25 +0300 Subject: [PATCH 73/73] Update rPGP to 0.5.2 --- Cargo.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c4bcf1ccf..f1c9513eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -656,7 +656,7 @@ dependencies = [ "num-derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", "percent-encoding 2.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "pgp 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "pgp 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)", "pretty_assertions 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)", "pretty_env_logger 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "proptest 0.9.5 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1814,7 +1814,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "pgp" -version = "0.5.1" +version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "aes 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3336,7 +3336,7 @@ dependencies = [ "checksum parking_lot 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)" = "92e98c49ab0b7ce5b222f2cc9193fc4efe11c6d0bd4f648e374684a6857b1cfc" "checksum parking_lot_core 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)" = "7582838484df45743c8434fbff785e8edf260c28748353d44bc0da32e0ceabf1" "checksum percent-encoding 2.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "d4fd5641d01c8f18a23da7b6fe29298ff4b55afcccdf78973b24cf3175fee32e" -"checksum pgp 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "96c659fb6141cb4b6bd2c50af03869b82789291770f0b035f36ab92eba5d8663" +"checksum pgp 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)" = "8172973101790c866e66966002bf1028d0df27bf6b3b29be86a6fd440d8a4285" "checksum pin-project 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "7804a463a8d9572f13453c516a5faea534a2403d7ced2f0c7e100eeff072772c" "checksum pin-project-internal 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "385322a45f2ecf3410c68d2a549a4a2685e8051d0f278e39743ff4e451cb9b3f" "checksum pin-project-lite 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)" = "237844750cfbb86f67afe27eee600dfbbcb6188d734139b534cbfbf4f96792ae"