From b62cc242b99184be1d603aaefd764a647da8682b Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Sun, 6 Sep 2026 20:26:03 +1000 Subject: [PATCH 1/2] Let a message request be accepted without answering it Approval has only ever been a side effect of messages flowing, so a client whose UI offers an Accept button had nothing to call: accepting was answering, or it was nothing. DM::approve() records it outright, through the same helper the send path uses, because it is one fact however it was arrived at. The conversation leaves message_requests() for conversations(), and the Contacts config carries the approval to our other devices, exactly as an answer would. It also tells them, which nothing here did. Session's other clients send a MessageRequestResponse on that button, and short of a message from us it is the only thing that clears the other end's awaiting_approval, while accepting is precisely the choice not to send one. An arriving one is now read as well. Nothing looked at the field before, so a peer accepting on Desktop, Android or iOS left us waiting on somebody who had already said yes. Only the acceptance is taken from it, and only for an account already a contact of ours: approval has no reverse, and a response from someone we never wrote to answers a request we never made. --- include/session/client.hpp | 15 ++- include/session/client/conversation.hpp | 35 +++++-- src/client/client.cpp | 81 ++++++++++++++- src/client/conversation.cpp | 9 ++ tests/test_client/requests.cpp | 125 ++++++++++++++++++++++++ 5 files changed, 250 insertions(+), 15 deletions(-) diff --git a/include/session/client.hpp b/include/session/client.hpp index 36c1540a..45f9e4d4 100644 --- a/include/session/client.hpp +++ b/include/session/client.hpp @@ -61,6 +61,7 @@ namespace SessionProtos { class Content; class DataExtractionNotification; +class MessageRequestResponse; class UnsendRequest; } @@ -652,9 +653,10 @@ class Client { // accumulate in _early_status forever. std::unordered_map _sync_sends; // core send id -> client message id - // Sends whose outcome nobody is waiting for -- the media-saved notification is the only one so - // far. Tracked rather than left unregistered so that their statuses are dropped as they - // arrive, instead of accumulating in _early_status against ids that will never be claimed. + // Sends whose outcome nobody is waiting for: the messages Client sends about a conversation + // rather than in it. Tracked rather than left unregistered so that their statuses are dropped + // as they arrive, instead of accumulating in _early_status against ids that will never be + // claimed. std::unordered_set _quiet_sends; // The actual work, all of it assuming it is already on the loop thread. The public methods @@ -681,6 +683,13 @@ class Client { template void _set_conversation_setting(const ConversationId& id, std::string_view column, T value); void _set_blocked(const ConversationId& id, bool blocked); + void _approve(const ConversationId& id); + // A peer telling us they accepted a message request of ours. Nothing but the acceptance is + // read out of it: the profile it may carry says no more than the next message from them will, + // and unlike that one it is not attached to anything we would store. + void _on_message_request_response( + std::span sender, + const SessionProtos::MessageRequestResponse& res); void _clear_messages(const ConversationId& id); void _delete_conversation(const ConversationId& id, bool keep_messages); void _delete_contact(const ConversationId& id); diff --git a/include/session/client/conversation.hpp b/include/session/client/conversation.hpp index 00f72551..fb00908b 100644 --- a/include/session/client/conversation.hpp +++ b/include/session/client/conversation.hpp @@ -346,9 +346,9 @@ class DM : public Conversation { /// True while this is a message request rather than a conversation: someone we have never /// written to has written to us. /// - /// Approval is not something anyone sets — it is recorded by messages flowing. Writing to - /// someone approves them, so answering a request is what accepts it, and there is no way back: - /// what un-requests a conversation is deleting the contact, not clearing a flag. + /// Cleared by approving them, which `approve()` does outright and writing to them does on the + /// way past, an answer being an acceptance. Either way there is no way back: what un-requests + /// a conversation is deleting the contact, not clearing a flag. /// /// Requests are conversations in every other respect — they have history, an unread count and a /// name — which is why this is a property of one rather than a kind of its own. What differs @@ -361,10 +361,11 @@ class DM : public Conversation { /// The mirror of `request`: we have written to someone who has never written back, so they have /// us in *their* message requests and have not answered. /// - /// Set from the same evidence, read the other way round — nobody sends anything to say they - /// accepted, so the only thing that clears this is a message from them. A display showing - /// "waiting for them to accept" wants this; the conversation is otherwise ordinary and is in - /// `conversations()` like any other, because it is one we chose to start. + /// Set from the same evidence, read the other way round: what clears it is their approving us, + /// which reaches us either as the acceptance `approve()` sends or as a message from them, since + /// writing is approving. A display showing "waiting for them to accept" wants this; the + /// conversation is otherwise ordinary and is in `conversations()` like any other, because it is + /// one we chose to start. /// /// The two are mutually exclusive: their having written to us is exactly what makes this false /// and what can make `request` true. @@ -396,6 +397,26 @@ class DM : public Conversation { std::string name; std::string nickname; + /// Accepts a message request without answering it: records that we approved them, which moves + /// the conversation out of `message_requests()` and into `conversations()`, and tells them we + /// did. + /// + /// The other way to approve someone is to write to them, which is what an answer already does. + /// This is Session's Accept button: the same approval, arrived at without saying anything. + /// Synced like any other, so a request accepted here is not still waiting on another device. + /// + /// No way back, as there is none from answering: what un-requests a conversation is + /// `delete_contact`, not a second call clearing the flag. Approving somebody already approved + /// does nothing and tells them nothing. + /// + /// Telling them is best effort and is not waited on: there is nothing to acknowledge it, and + /// the acceptance is ours whether or not it arrives. Our first message to them says the same + /// thing again. + /// + /// @throws std::invalid_argument if the id is not a one-to-one conversation, or is our own. + void approve(failable_function cb); + void approve(wait_t); + /// Sets or clears the name we have given them, which is ours rather than theirs and follows us /// between devices. An empty nickname removes it, so `display_name` falls back to `name`. void set_nickname(std::string_view nickname, failable_function cb); diff --git a/src/client/client.cpp b/src/client/client.cpp index 3b3e2031..88ba128f 100644 --- a/src/client/client.cpp +++ b/src/client/client.cpp @@ -294,12 +294,12 @@ static bool ensure_contact(sqlite::Connection& c, int64_t account, bool approved approved ? 1 : 0) > 0; } -// Records that we have approved whoever an outgoing message is addressed to, and says whether that -// changed anything. Must be called inside the caller's transaction. +// Records that we have approved an account, and says whether that changed anything. Must be called +// inside the caller's transaction. // -// Writing to someone is what approving them is -- there is no separate accept -- so answering a -// message request is what takes it out of the requests list. Never for note to self, which is not -// a contact and cannot be a request. +// Reached from both directions, because approval is one fact however it was arrived at: writing to +// someone approves them on the way past, and accepting a request approves them outright. Never for +// note to self, which is not a contact and cannot be a request. static bool approve_recipient(sqlite::Connection& c, const ConversationId& id, Client& client) { if (client.is_me(id.session_id())) return false; @@ -1685,6 +1685,69 @@ void Client::_set_blocked(const ConversationId& id, bool blocked) { _touch(id); } +void Client::_approve(const ConversationId& id) { + bool approved = false; + { + auto c = core.database().conn(); + SQLite::Transaction tx{c.sql}; + approved = approve_recipient(c, id, *this); + tx.commit(); + } + + // Already approved, by an answer or by an earlier accept. The lists are where they belong and + // they have been told, so a second accept is not a second acceptance to announce. + if (!approved) + return; + + _sync_contact(id); + _emit_lists_replaced(); + + // Best effort, and deliberately not waited on: there is nothing to acknowledge it, and the + // acceptance is recorded whether or not it arrives. Session's other clients send this on the + // same button and read it the same way, and without it the only thing that would tell them is + // our first message -- which accepting is precisely the choice not to send. + auto now = clock_now_ms(); + SessionProtos::Content content; + content.set_sigtimestamp(static_cast(epoch_ms(now))); + content.mutable_messagerequestresponse()->set_isapproved(true); + + // Registered rather than fired blind: Core reports on every send, and a status for an id nobody + // claims would sit in _early_status for the life of the process. + _quiet_sends.insert(core.send_dm(id.session_id(), content, now)); +} + +void Client::_on_message_request_response( + std::span sender, const SessionProtos::MessageRequestResponse& res) { + // Only an acceptance is acted on. Approval has no reverse -- what a refusal does is delete + // the contact, which reaches us as the entry going from the Contacts config -- so the false + // form has nothing here to mean. + if (!res.isapproved()) + return; + + // Only for an account that is already a contact of ours, which writing to them made them: a + // response from someone we have never written to answers a request we never made, and there is + // no relationship of ours for it to be a fact about. + bool changed = false; + { + auto c = core.database().conn(); + changed = c.prepared_exec( + R"( + UPDATE contacts SET approved_me = 1 + WHERE account = (SELECT id FROM accounts WHERE session_id = ?) AND NOT approved_me + )", + sender) > 0; + } + + if (!changed) + return; + + // No _emit_lists_replaced: only our own approval moves a conversation between the lists. This + // is theirs, so the conversation stays where it was and what changed is a field of it. + auto id = ConversationId::dm(sender); + _sync_contact(id); + _touch(id); +} + void Client::_clear_messages(const ConversationId& id) { auto now = clock_now_ms(); bool emptied = false; @@ -4751,6 +4814,14 @@ void Client::_on_message_received(core::ReceivedMessage&& msg) { return; } + // Someone telling us they accepted a message request of ours. Not history either -- it says + // something about the relationship rather than adding to what was said in it -- so it is + // handled here and goes no further. + if (content.has_messagerequestresponse()) { + _on_message_request_response(msg.sender_session_id, content.messagerequestresponse()); + return; + } + // Receipts, typing indicators and call signalling are not conversation history; ignore them // rather than materialising an empty conversation for a stranger who is merely typing. if (!content.has_datamessage()) diff --git a/src/client/conversation.cpp b/src/client/conversation.cpp index e8f7ce9f..be598f28 100644 --- a/src/client/conversation.cpp +++ b/src/client/conversation.cpp @@ -197,6 +197,15 @@ void Conversation::delete_conversation(bool keep_messages, wait_t) { // -- One-to-one only ---------------------------------------------------------------------------- +void DM::approve(failable_function cb) { + _client->_require_contact("approve", id); + _client->_async([c = _client, id = id] { c->_approve(id); }, std::move(cb)); +} +void DM::approve(wait_t) { + _client->_require_contact("approve", id); + _client->loop.call_get([this] { _client->_approve(id); }); +} + void DM::set_blocked(bool blocked, failable_function cb) { _client->_require_contact("set_blocked", id); _client->_async([c = _client, id = id, blocked] { c->_set_blocked(id, blocked); }, diff --git a/tests/test_client/requests.cpp b/tests/test_client/requests.cpp index d42523a7..1f4e7a26 100644 --- a/tests/test_client/requests.cpp +++ b/tests/test_client/requests.cpp @@ -59,6 +59,131 @@ TEST_CASE("Client: answering a request accepts it", "[client][requests]") { CHECK(std::ranges::count(r.order, "requests") == 1); } +TEST_CASE("Client: accepting a request approves without answering it", "[client][requests]") { + Recorder r; + TempClient c{r.handlers()}; + auto* net = attach_mock_network(c->core); + SenderKeys sender; + auto id = ConversationId::dm(sender.session_id); + + deliver(*c, sender, "hello?", from_epoch_ms(5000), "h1"); + sync(*c); + REQUIRE(c->message_requests(wait).size() == 1); + r.order.clear(); + + // No PFS keys published for them, so the acceptance falls back to a v1 send. + TestHelper::seed_pfs_nak(c->core, sender.session_id); + c->dm(id, wait)->approve(wait); + sync(*c); + + // Accepted, and nothing was said: the history is still the one message they sent. + CHECK(c->message_requests(wait).empty()); + REQUIRE(c->conversations(wait).size() == 1); + CHECK_FALSE(c->conversations(wait)[0].dm()->request); + CHECK(c->conversation(id, wait)->messages(wait).size() == 1); + CHECK(c->core.configs.contacts().get(oxenc::to_hex(sender.session_id))->approved); + + // It left one list and joined the other, which is neither an addition nor a removal to either, + // so both are replaced. + CHECK(std::ranges::count(r.order, "replaced") == 1); + CHECK(std::ranges::count(r.order, "requests") == 1); + + // One store, to their swarm and only theirs: an acceptance is not a message, so there is no + // copy of it for our own devices -- what tells those is the Contacts config. + CHECK(stores(*net).size() == 1); + + // And a second accept is not a second acceptance: nothing moved, so nobody is told again. + r.order.clear(); + c->dm(id, wait)->approve(wait); + sync(*c); + CHECK(r.order.empty()); + CHECK(stores(*net).size() == 1); +} + +TEST_CASE("Client: their acceptance is what stops us awaiting it", "[client][requests]") { + TempClient us; + TempClient them; + auto* net = attach_mock_network(them->core); + auto us_id = ConversationId::dm(own_sid(*them)); + auto them_id = ConversationId::dm(own_sid(*us)); + + // We write first, which puts us in their requests and leaves us waiting on them. + us->send_message(us_id, {.body = "are you there?"}, wait); + sync(*us); + REQUIRE(us->conversations(wait).size() == 1); + REQUIRE(us->conversations(wait)[0].dm()->awaiting_approval); + + deliver(*them, self_keys(*us), "are you there?", from_epoch_ms(5000), "h1"); + REQUIRE(them->message_requests(wait).size() == 1); + + TestHelper::seed_pfs_nak(them->core, own_sid(*us)); + them->dm(them_id, wait)->approve(wait); + sync(*them); + + // Their acceptance as they actually sent it, read by the client it was addressed to. + auto sent = stores(*net); + REQUIRE(sent.size() == 1); + // Named: SwarmMessage::data is a span, so a temporary here would be read after it had gone. + auto payload = store_payload(*sent[0]); + core::SwarmMessage sm{payload, "h2", from_epoch_ms(6000), from_epoch_ms(1'000'000'000'000)}; + us->core.loop().call_get([&] { + us->core.receive_messages({&sm, 1}, config::Namespace::Default, true); + return 0; + }); + sync(*us); + + // Accepted, and still not a word from them: what changed is the flag, not the history. + REQUIRE(us->conversation(us_id, wait)); + CHECK_FALSE(us->conversation(us_id, wait)->dm()->awaiting_approval); + CHECK(us->conversation(us_id, wait)->messages(wait).size() == 1); + CHECK(us->core.configs.contacts().get(oxenc::to_hex(own_sid(*them)))->approved_me); + + // Theirs to approve us, not to move us: the conversation was ours from the moment we wrote it. + CHECK(us->conversations(wait).size() == 1); + CHECK(us->message_requests(wait).empty()); +} + +TEST_CASE("Client: a response that accepts nothing is not recorded", "[client][requests]") { + TempClient c; + + // Built here rather than sent by another client, because no client sends either of these. + auto respond = [&](const SenderKeys& from, bool approved, sys_ms at) { + SessionProtos::Content content; + content.set_sigtimestamp(static_cast(epoch_ms(at))); + content.mutable_messagerequestresponse()->set_isapproved(approved); + + auto plaintext = content.SerializeAsString(); + auto encoded = encode_dm_v1( + std::as_bytes(std::span{plaintext}), from.ed_sk, at, own_sid(*c), std::nullopt); + core::SwarmMessage sm{ + encoded, random::unique_id("h", 8), at, from_epoch_ms(1'000'000'000'000)}; + c->core.loop().call_get([&] { + c->core.receive_messages({&sm, 1}, config::Namespace::Default, true); + return 0; + }); + sync(*c); + }; + + // An acceptance from someone we never wrote to answers a request we never made, and there is no + // relationship of ours for it to be a fact about -- so it makes neither. + SenderKeys stranger; + respond(stranger, true, from_epoch_ms(5000)); + CHECK(c->conversations(wait).empty()); + CHECK(c->message_requests(wait).empty()); + CHECK_FALSE(c->core.configs.contacts().get(oxenc::to_hex(stranger.session_id))); + + // And a refusal from someone we did write to says nothing either: approval has no reverse, so + // this is not the un-approval it reads as. + SenderKeys them; + auto id = ConversationId::dm(them.session_id); + c->send_message(id, {.body = "are you there?"}, wait); + sync(*c); + + respond(them, false, from_epoch_ms(6000)); + CHECK(c->conversation(id, wait)->dm()->awaiting_approval); + CHECK_FALSE(c->core.configs.contacts().get(oxenc::to_hex(them.session_id))->approved_me); +} + TEST_CASE("Client: a linked device's answer accepts the request", "[client][requests]") { TempClient c; SenderKeys sender; From 382afaf36b72cb4e75593e4669aa4cb6d98fc090 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 7 Sep 2026 08:05:34 +1000 Subject: [PATCH 2/2] Say which of the two approvals _approve is Writing to someone already approves them, so the declaration is where to name the other path rather than leave a reader to find it in _send_message. --- include/session/client.hpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/session/client.hpp b/include/session/client.hpp index 45f9e4d4..2d508c1e 100644 --- a/include/session/client.hpp +++ b/include/session/client.hpp @@ -683,6 +683,10 @@ class Client { template void _set_conversation_setting(const ConversationId& id, std::string_view column, T value); void _set_blocked(const ConversationId& id, bool blocked); + // The second of the two ways a conversation gets approved. The first is _send_message, which + // approves the recipient on the way past because writing to someone is approving them; this one + // approves outright, and sends the message request response that says so in place of the + // message it did not send. void _approve(const ConversationId& id); // A peer telling us they accepted a message request of ours. Nothing but the acceptance is // read out of it: the profile it may carry says no more than the next message from them will,