Let a message request be accepted without answering it

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<int64_t, int64_t> _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<int64_t> _quiet_sends;
 
     // The actual work, all of it assuming it is already on the loop thread.  The public methods
@@ -681,6 +683,17 @@ class Client {
     template <typename T>
     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,
+    // and unlike that one it is not attached to anything we would store.
+    void _on_message_request_response(
+            std::span<const std::byte, 33> 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);

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<void()> 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<void()> cb);

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<uint64_t>(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<const std::byte, 33> 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())

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<void()> 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<void()> cb) {
     _client->_require_contact("set_blocked", id);
     _client->_async([c = _client, id = id, blocked] { c->_set_blocked(id, blocked); },

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<uint64_t>(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;