Skip to content

fix: emit ChatModified event when can_send changes during secure join #7735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Simon-Laux wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: emit ChatModified event when can_send changes during secure join #7735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef309b5
fix: emit `ChatModified` event when `can_send` changes during secure
Simon-Laux Jan 16, 2026
f01b1d0
don't emit changed on joining broadcast
Simon-Laux Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/securejoin/bob.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,11 @@ pub(super) async fn handle_auth_required(
.await?;

match invite {
QrInvite::Contact { .. } | QrInvite::Broadcast { .. } => {}
QrInvite::Broadcast { .. } => {}
QrInvite::Contact { .. } => {
// This is to tell UI that Chat.can_send has changed
context.emit_event(EventType::ChatModified(chat_id));
}
QrInvite::Group { .. } => {
// The message reads "Alice replied, waiting to be added to the group…",
// so only show it when joining a group and not for a 1:1 chat or broadcast channel.
Expand Down
106 changes: 106 additions & 0 deletions src/securejoin/securejoin_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1370,3 +1370,109 @@ gU6dGXsFMe/RpRHrIAkMAaM5xkxMDRuRJDxiUdS/X+Y8

Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_secure_join_group_can_send_and_event() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = tcm.alice().await;
let bob = tcm.bob().await;

let alice_chatid = chat::create_group(&alice, "the chat").await?;

tcm.section("Step 1: Generate QR-code, secure-join implied by chatid");
Copy link
Collaborator

@Hocuri Hocuri Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tcm.section("Step 1: Generate QR-code, secure-join implied by chatid");
tcm.section("Step 1: Generate QR-code");

If the reader doesn't already know what the comment means, it is more confusing than helpful. And if you want to know, you can just go to get_securejoin_qr() and check what the second argument does.

let qr = get_securejoin_qr(&alice, Some(alice_chatid)).await.unwrap();

tcm.section("Step 2: Bob scans QR-code, sends vg-request");
let bob_chatid = join_securejoin(&bob, &qr).await?;
assert_eq!(
Chat::load_from_db(&bob, bob_chatid)
.await?
.can_send(&bob)
.await?,
false
);

let sent = bob.pop_sent_msg().await;
let _ = alice.parse_msg(&sent).await;

tcm.section("Step 3: Alice receives vg-request, sends vg-auth-required");
alice.recv_msg_trash(&sent).await;
let sent = alice.pop_sent_msg().await;
let _ = bob.parse_msg(&sent).await;

tcm.section("Step 4: Bob receives vg-auth-required, sends vg-request-with-auth");
bob.recv_msg_trash(&sent).await;
let sent = bob.pop_sent_msg().await;

tcm.section("Step 5+6: Alice receives vg-request-with-auth, sends vg-member-added");
alice.recv_msg_trash(&sent).await;
let sent = alice.pop_sent_msg().await;

bob.evtracker.clear_events();
assert_eq!(
Chat::load_from_db(&bob, bob_chatid)
.await?
.can_send(&bob)
Copy link
Collaborator

@iequidoo iequidoo Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use why_cant_send() to check the reason

.await?,
false
);

tcm.section("Step 7: Bob receives vg-member-added");
bob.recv_msg(&sent).await;

bob.evtracker
.get_matching(|typ| typ == &EventType::ChatModified(bob_chatid))
.await;

assert_eq!(
Chat::load_from_db(&bob, bob_chatid)
.await?
.can_send(&bob)
.await?,
true
);

Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_secure_join_contact_can_send_and_event() -> Result<()> {
Copy link
Collaborator

@iequidoo iequidoo Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already test_setup_contact_ex(), maybe add new checks there so that there's less code to maintain? It's not a new scenario anyway. And for SecureJoin to a group there's test_secure_join(). Or does awaiting for ChatModified somehow intersect with SecurejoinJoinerProgress so it's complicated to test both events?

Copy link
Member Author

@Simon-Laux Simon-Laux Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The used way of awaiting events drops all other events that are emitted. So when order of events is stable it could work to use the existing method. I thought a new test, which only tests for the event is cleaner and has less risk of breaking an existing test.

Also in this particular test there is this line

core/src/securejoin/securejoin_tests.rs

Line 139 in 659d21a

let msg = bob.parse_msg(&sent).await;

which already seemed to change can_send, because it already ingests alice's key.

maybe add new checks there so that there's less code to maintain?

Thought about it, but since that test is already hard to understand for me, I did not want to make it even more complex and more importantly I don't want to accidentally break an important test by editing it to test something less important.

Copy link
Collaborator

@iequidoo iequidoo Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall i agree with adding new test code to not intersect with checks for EventType::SecurejoinInviterProgress.

which already seemed to change can_send, because it already ingests alice's key.

parse_msg() shouldn't change bob's context state this or another significant way because it's used in the tests. If it does, this should be fixed in a separate PR

let mut tcm = TestContextManager::new();
let alice = tcm.alice().await;
let bob = tcm.bob().await;

tcm.section("Step 1: Generate QR-code, ChatId(0) indicates setup-contact");
Copy link
Collaborator

@Hocuri Hocuri Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tcm.section("Step 1: Generate QR-code, ChatId(0) indicates setup-contact");
tcm.section("Step 1: Generate QR-code");

let qr = get_securejoin_qr(&alice, None).await?;

tcm.section("Step 2: Bob scans QR-code, sends vc-request");
let bob_chat_id = join_securejoin(&bob.ctx, &qr).await?;

let bob_chat = Chat::load_from_db(&bob, bob_chat_id).await?;
assert_eq!(
bob_chat.why_cant_send(&bob).await?,
Some(CantSendReason::MissingKey)
);

let sent = bob.pop_sent_msg().await;

tcm.section("Step 3: Alice receives vc-request, sends vc-auth-required");
alice.recv_msg_trash(&sent).await;

let sent = alice.pop_sent_msg().await;
let bob_chat = bob.get_chat(&alice).await;
assert_eq!(bob_chat.can_send(&bob).await?, false);
Copy link
Collaborator

@iequidoo iequidoo Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why checking again here? Bob hasn't received any message from Alice since the previous check. I think that the previous check should just be moved here, it's better because it also checks the reason

bob.evtracker.clear_events();

tcm.section("Step 4: Bob receives vc-auth-required, sends vc-request-with-auth");
bob.recv_msg_trash(&sent).await;

bob.evtracker
.get_matching(|typ| typ == &EventType::ChatModified(bob_chat_id))
.await;

let bob_chat = bob.get_chat(&alice).await;
assert_eq!(bob_chat.can_send(&bob).await?, true);
Copy link
Collaborator

@iequidoo iequidoo Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: Bob already can send, but if vc-request-with-auth won't be delivered or reordered, Bob's message must be accepted and shown to Alice, but Bob shouldn't be verified yet. We need a separate test/PR for this, but it's better to be done as a separate SetupContactCase variant (testing for ChatModified isn't necessary in this case). Otherwise there's a risk that Alice just drops Bob's message to trash, this shalln't happen if we already allow to send messages.


// Rest of the steps (4-7) are irrelevant for this test
Ok(())
}