fix: Fix info-message orderings of verified 1:1 chats (#4545)

Correctly handle messages with old timestamps for verified chats:

 * They must not be sorted over a protection-changed info message

 * If they change the protection, then they must not be sorted over existing other messages, because then the protection-changed info message would also be above these existing messages.


This PR fixes this:

 1. Even seen messages can't be sorted into already-noticed messages anymore. **This also changes DC's behavior in the absence of verified 1:1 chats**. Before this PR, messages that are marked as seen when they are downloaded will always be sorted by their timestamp, even if it's very old.

 2. protection-changed info messages are always sorted to the bottom.

    **Edit:**

 3. There is an exception to rule 1: Outgoing messages are still allowed to be sorted purely by their timestamp, and don't influence old messages. This is to the problem described at [*].


Together, these rules also make sure that the protection-changed info message is always right above the message causing the change.

[*] If we receive messages from two different folders, e.g. `Sent` and `Inbox`, then this will lead to wrong message ordering in many cases. I need to think about this more, or maybe someone else has an idea. One new idea that came to my mind is:

 * Always sort noticed messages under the newest info message (this PR sorts them under the newest noticed message, master sorts them purely by their sent timestamp)

 * Always sort unnoticed messages under the newest noticed message (that's the same behavior as in this PR and on master)

 * Always sort protection-changed info messages to the bottom (as in this PR)


However, after a talk with @link2xt we instead decided to add rule 3. (see above) because it seemed a little bit easier.

src/receive_imf.rs

@@ -754,8 +754,12 @@ async fn add_parts(
                             new_protection = ProtectionStatus::ProtectionBroken;
                         }
 
+                        // The message itself will be sorted under the device message since the device
+                        // message is `MessageState::InNoticed`, which means that all following
+                        // messages are sorted under it.
                         let sort_timestamp =
-                            calc_sort_timestamp(context, sent_timestamp, chat_id, true).await?;
+                            calc_sort_timestamp(context, sent_timestamp, chat_id, true, incoming)
+                                .await?;
                         chat_id
                             .set_protection(context, new_protection, sort_timestamp, Some(from_id))
                             .await?;
@@ -948,7 +952,8 @@ async fn add_parts(
     };
 
     let in_fresh = state == MessageState::InFresh;
-    let sort_timestamp = calc_sort_timestamp(context, sent_timestamp, chat_id, in_fresh).await?;
+    let sort_timestamp =
+        calc_sort_timestamp(context, sent_timestamp, chat_id, false, incoming).await?;
 
     // Apply ephemeral timer changes to the chat.
     //
@@ -1376,27 +1381,43 @@ async fn calc_sort_timestamp(
     context: &Context,
     message_timestamp: i64,
     chat_id: ChatId,
-    is_fresh_msg: bool,
+    always_sort_to_bottom: bool,
+    incoming: bool,
 ) -> Result<i64> {
     let mut sort_timestamp = message_timestamp;
 
-    // get newest non fresh message for this chat
+    let last_msg_time: Option<i64> = if always_sort_to_bottom {
-    // update sort_timestamp if less than that
+        // get newest message for this chat
-    if is_fresh_msg {
+        context
-        let last_msg_time: Option<i64> = context
             .sql
             .query_get_value(
-                "SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND state>?",
+                "SELECT MAX(timestamp) FROM msgs WHERE chat_id=?",
-                (chat_id, MessageState::InFresh),
+                (chat_id,),
             )
-            .await?;
+            .await?
+    } else if incoming {
+        // get newest incoming non fresh message for this chat.
+
+        // If a user hasn't been online for some time, the Inbox is
+        // fetched first and then the Sentbox. In order for Inbox
+        // and Sent messages to be allowed to mingle,
+        // outgoing messages are purely sorted by their sent timestamp.
+        context
+            .sql
+            .query_get_value(
+                "SELECT MAX(timestamp) FROM msgs WHERE chat_id=? AND state>? AND from_id!=?",
+                (chat_id, MessageState::InFresh, ContactId::SELF),
+            )
+            .await?
+    } else {
+        None
+    };
 
     if let Some(last_msg_time) = last_msg_time {
         if last_msg_time > sort_timestamp {
             sort_timestamp = last_msg_time;
         }
     }
-    }
 
     Ok(min(sort_timestamp, smeared_time(context)))
 }

src/tests/verified_chats.rs

@@ -313,6 +313,173 @@ async fn test_verified_oneonone_chat_enable_disable() -> Result<()> {
     Ok(())
 }
 
+/// Messages with old timestamps are difficult for verified chats:
+/// - They must not be sorted over a protection-changed info message.
+///   That's what `test_old_message_2` tests
+/// - If they change the protection, then they must not be sorted over existing other messages,
+///   because then the protection-changed info message would also be above these existing messages.
+///   That's what `test_old_message_3` tests.
+///
+/// `test_old_message_1` tests the case where both the old and the new message
+/// change verification
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_old_message_1() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = tcm.alice().await;
+    let bob = tcm.bob().await;
+    enable_verified_oneonone_chats(&[&alice, &bob]).await;
+
+    mark_as_verified(&alice, &bob).await;
+
+    let chat = alice.create_chat(&bob).await; // This creates a protection-changed info message
+    assert!(chat.is_protected());
+
+    // This creates protection-changed info message #2;
+    // even though the date is old, info message and email must be sorted below the original info message.
+    receive_imf(
+        &alice,
+        b"From: Bob <bob@example.net>\n\
+          To: alice@example.org\n\
+          Message-ID: <1234-2-3@example.org>\n\
+          Date: Sat, 07 Dec 2019 19:00:27 +0000\n\
+          \n\
+          Message from Thunderbird\n",
+        true,
+    )
+    .await?;
+
+    alice.golden_test_chat(chat.id, "test_old_message_1").await;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_old_message_2() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = tcm.alice().await;
+    let bob = tcm.bob().await;
+    enable_verified_oneonone_chats(&[&alice, &bob]).await;
+
+    mark_as_verified(&alice, &bob).await;
+
+    // This creates protection-changed info message #1:
+    let chat = alice.create_chat(&bob).await;
+    assert!(chat.is_protected());
+    let protection_msg = alice.get_last_msg().await;
+    assert_eq!(
+        protection_msg.param.get_cmd(),
+        SystemMessage::ChatProtectionEnabled
+    );
+
+    // This creates protection-changed info message #2.
+    let first_email = receive_imf(
+        &alice,
+        b"From: Bob <bob@example.net>\n\
+          To: alice@example.org\n\
+          Message-ID: <1234-2-3@example.org>\n\
+          Date: Sun, 08 Dec 2019 19:00:27 +0000\n\
+          \n\
+          Somewhat old message\n",
+        false,
+    )
+    .await?
+    .unwrap();
+
+    // Both messages will get the same timestamp as the protection-changed
+    // message, so this one will be sorted under the previous one
+    // even though it has an older timestamp.
+    let second_email = receive_imf(
+        &alice,
+        b"From: Bob <bob@example.net>\n\
+          To: alice@example.org\n\
+          Message-ID: <2319-2-3@example.org>\n\
+          Date: Sat, 07 Dec 2019 19:00:27 +0000\n\
+          \n\
+          Even older message, that must NOT be shown before the info message\n",
+        true,
+    )
+    .await?
+    .unwrap();
+
+    assert_eq!(first_email.sort_timestamp, second_email.sort_timestamp);
+    assert_eq!(first_email.sort_timestamp, protection_msg.timestamp_sort);
+
+    alice.golden_test_chat(chat.id, "test_old_message_2").await;
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_old_message_3() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = tcm.alice().await;
+    let bob = tcm.bob().await;
+    enable_verified_oneonone_chats(&[&alice, &bob]).await;
+
+    mark_as_verified(&alice, &bob).await;
+    mark_as_verified(&bob, &alice).await;
+
+    tcm.send_recv_accept(&bob, &alice, "Heyho from my verified device!")
+        .await;
+
+    // This unverified message must not be sorted over the message sent in the previous line:
+    receive_imf(
+        &alice,
+        b"From: Bob <bob@example.net>\n\
+          To: alice@example.org\n\
+          Message-ID: <1234-2-3@example.org>\n\
+          Date: Sat, 07 Dec 2019 19:00:27 +0000\n\
+          \n\
+          Old, unverified message\n",
+        true,
+    )
+    .await?;
+
+    alice
+        .golden_test_chat(alice.get_chat(&bob).await.unwrap().id, "test_old_message_3")
+        .await;
+
+    Ok(())
+}
+
+/// Alice is offline for some time.
+/// When she comes online, first her inbox is synced and then her sentbox.
+/// This test tests that the messages are still in the right order.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_old_message_4() -> Result<()> {
+    let alice = TestContext::new_alice().await;
+    let msg_incoming = receive_imf(
+        &alice,
+        b"From: Bob <bob@example.net>\n\
+          To: alice@example.org\n\
+          Message-ID: <1234-2-3@example.org>\n\
+          Date: Sun, 08 Dec 2019 19:00:27 +0000\n\
+          \n\
+          Thanks, Alice!\n",
+        true,
+    )
+    .await?
+    .unwrap();
+
+    let msg_sent = receive_imf(
+        &alice,
+        b"From: alice@example.org\n\
+          To: Bob <bob@example.net>\n\
+          Message-ID: <1234-2-4@example.org>\n\
+          Date: Sat, 07 Dec 2019 19:00:27 +0000\n\
+          \n\
+          Happy birthday, Bob!\n",
+        true,
+    )
+    .await?
+    .unwrap();
+
+    // The "Happy birthday" message should be shown first, and then the "Thanks" message
+    assert!(msg_sent.sort_timestamp < msg_incoming.sort_timestamp);
+
+    Ok(())
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_mdn_doesnt_disable_verification() -> Result<()> {
     let mut tcm = TestContextManager::new();

test-data/golden/test_old_message_1

@@ -0,0 +1,6 @@
+Single#Chat#10: Bob [bob@example.net] 
+--------------------------------------------------------------------------------
+Msg#10: info (Contact#Contact#Info): Messages are guaranteed to be end-to-end encrypted from now on. [NOTICED][INFO 🛡️]
+Msg#11: info (Contact#Contact#Info): Bob sent a message from another device. [NOTICED][INFO 🛡️❌]
+Msg#12:  (Contact#Contact#10): Message from Thunderbird [SEEN]
+--------------------------------------------------------------------------------

test-data/golden/test_old_message_2

@@ -0,0 +1,7 @@
+Single#Chat#10: Bob [bob@example.net] 
+--------------------------------------------------------------------------------
+Msg#10: info (Contact#Contact#Info): Messages are guaranteed to be end-to-end encrypted from now on. [NOTICED][INFO 🛡️]
+Msg#11: info (Contact#Contact#Info): Bob sent a message from another device. [NOTICED][INFO 🛡️❌]
+Msg#12:  (Contact#Contact#10): Somewhat old message [FRESH]
+Msg#13:  (Contact#Contact#10): Even older message, that must NOT be shown before the info message [SEEN]
+--------------------------------------------------------------------------------

test-data/golden/test_old_message_3

@@ -0,0 +1,7 @@
+Single#Chat#10: Bob [bob@example.net] 
+--------------------------------------------------------------------------------
+Msg#10: info (Contact#Contact#Info): Messages are guaranteed to be end-to-end encrypted from now on. [NOTICED][INFO 🛡️]
+Msg#11🔒:  (Contact#Contact#10): Heyho from my verified device! [FRESH]
+Msg#12: info (Contact#Contact#Info): Bob sent a message from another device. [NOTICED][INFO 🛡️❌]
+Msg#13:  (Contact#Contact#10): Old, unverified message [SEEN]
+--------------------------------------------------------------------------------