Fix Direct Message shims. Fixes #7441

app/Http/Controllers/Api/ApiV1Controller.php

@@ -3370,7 +3370,7 @@ class ApiV1Controller extends Controller
         abort_unless($request->user()->tokenCan('write'), 403);
 
         $service = app(DirectMessageService::class);
-        $found = is_numeric($id) ? $service->conversationFor($id, $request->user()->profile_id) : null;
+        $found = is_numeric($id) ? $service->conversationForMastodonId($id, $request->user()->profile_id) : null;
         abort_if(! $found, 404);
 
         $service->setHidden($found[1], true);
@@ -3390,7 +3390,7 @@ class ApiV1Controller extends Controller
         $payloads = app(DirectMessagePayloadService::class);
         $pid = $request->user()->profile_id;
 
-        $found = is_numeric($id) ? $service->conversationFor($id, $pid) : null;
+        $found = is_numeric($id) ? $service->conversationForMastodonId($id, $pid) : null;
         abort_if(! $found, 404);
 
         [$conversation, $participant] = $found;
@@ -3425,6 +3425,14 @@ class ApiV1Controller extends Controller
 
         $res = $request->has(self::PF_API_ENTITY_KEY) ? StatusService::get($id, false) : StatusService::getMastodon($id, false);
         if (! $res || ! isset($res['visibility'])) {
+            // Direct messages are no longer statuses, but clients still take
+            // the id they got from /api/v1/conversations to this endpoint
+            $direct = app(DirectMessagePayloadService::class)->mastodonStatusById($id, $pid);
+
+            if ($direct) {
+                return $this->json($direct);
+            }
+
             abort(404);
         }
 
@@ -3478,6 +3486,12 @@ class ApiV1Controller extends Controller
         );
 
         if (! $status || ! isset($status['account'])) {
+            $direct = app(DirectMessagePayloadService::class)->mastodonContext($id, $pid);
+
+            if ($direct) {
+                return $this->json($direct);
+            }
+
             return response('', 404);
         }
 
@@ -4040,8 +4054,24 @@ class ApiV1Controller extends Controller
         abort_unless($request->user()->tokenCan('write'), 403);
 
         AccountService::setLastActive($request->user()->id);
-        $status = Status::whereProfileId($request->user()->profile->id)
-            ->findOrFail($id);
+        $pid = $request->user()->profile_id;
+        $status = Status::whereProfileId($pid)->find($id);
+
+        if (! $status) {
+            $message = DmMessage::where('profile_id', $pid)->find($id);
+            abort_if(! $message, 404);
+
+            $payloads = app(DirectMessagePayloadService::class);
+            $res = $payloads->mastodonStatusById($message->id, $pid);
+            abort_if(! $res, 404);
+
+            app(DirectMessageService::class)->deleteMessage($message);
+
+            $res['text'] = $res['content_text'];
+            unset($res['content']);
+
+            return $this->json($res);
+        }
 
         $resource = new Fractal\Resource\Item($status, new StatusTransformer);
 

app/Services/DirectMessagePayloadService.php

@@ -186,10 +186,13 @@ class DirectMessagePayloadService
      */
     public function mastodonConversation(DmConversation $conversation, DmConversationParticipant $viewer, Collection $members, ?DmMessage $last, int $viewerId): ?array
     {
-        $accounts = $members
-            ->filter(fn ($member) => (int) $member->profile_id !== $viewerId)
+        $everyone = $members
             ->map(fn ($member) => AccountService::getMastodon($member->profile_id, true))
             ->filter(fn ($account) => $account && isset($account['id']))
+            ->values();
+
+        $accounts = $everyone
+            ->filter(fn ($account) => (int) $account['id'] !== $viewerId)
             ->values()
             ->all();
 
@@ -197,22 +200,108 @@ class DirectMessagePayloadService
             return null;
         }
 
+        // The id is the viewer's participant row, not the conversation. It
+        // is an autoincrement, and the old endpoint handed out one of those
+        // too, so clients that store this as a 32-bit int (Pixelix does)
+        // keep working. It is only used for the DELETE and read calls, where
+        // it resolves back to the conversation for this viewer.
         return [
-            'id' => (string) $conversation->id,
+            'id' => (string) $viewer->id,
             'unread' => $viewer->unread_count > 0,
             'accounts' => $accounts,
-            'last_status' => $this->mastodonStatus($last, $accounts, $viewerId),
+            'last_status' => $this->mastodonStatus($last, $everyone->all(), $viewerId),
         ];
     }
 
+    /**
+     * The message as a status entity, for a viewer who is in its
+     * conversation. Mastodon clients take the `last_status` id from
+     * /api/v1/conversations straight to /api/v1/statuses/{id}.
+     *
+     * @return array<string, mixed>|null
+     */
+    public function mastodonStatusById(int|string $messageId, int $viewerId): ?array
+    {
+        $message = DmMessage::with('media')->find($messageId);
+
+        if (! $message) {
+            return null;
+        }
+
+        $found = app(DirectMessageService::class)->conversationFor($message->conversation_id, $viewerId);
+
+        if (! $found || in_array((int) $message->profile_id, $this->blockedIds($viewerId), true)) {
+            return null;
+        }
+
+        return $this->mastodonStatus($message, $this->mastodonParticipants($found[0]), $viewerId);
+    }
+
+    /**
+     * The rest of the conversation around a message, in the shape of
+     * /api/v1/statuses/{id}/context: what came before as ancestors and what
+     * came after as descendants.
+     *
+     * @return array{ancestors: array<int, array<string, mixed>>, descendants: array<int, array<string, mixed>>}|null
+     */
+    public function mastodonContext(int|string $messageId, int $viewerId, int $limit = 40): ?array
+    {
+        $message = DmMessage::find($messageId);
+
+        if (! $message) {
+            return null;
+        }
+
+        $found = app(DirectMessageService::class)->conversationFor($message->conversation_id, $viewerId);
+
+        if (! $found) {
+            return null;
+        }
+
+        $participants = $this->mastodonParticipants($found[0]);
+        $blocked = $this->blockedIds($viewerId) ?: [0];
+
+        $query = fn () => DmMessage::with('media')
+            ->where('conversation_id', $message->conversation_id)
+            ->whereNotIn('profile_id', $blocked);
+
+        $ancestors = $query()->where('id', '<', $message->id)->orderByDesc('id')->limit($limit)->get()->reverse();
+        $descendants = $query()->where('id', '>', $message->id)->orderBy('id')->limit($limit)->get();
+
+        $toStatus = fn (DmMessage $m) => $this->mastodonStatus($m, $participants, $viewerId);
+
+        return [
+            'ancestors' => $ancestors->map($toStatus)->values()->all(),
+            'descendants' => $descendants->map($toStatus)->values()->all(),
+        ];
+    }
+
+    /**
+     * Everyone in the conversation, as Mastodon account entities.
+     *
+     * @return array<int, array<string, mixed>>
+     */
+    public function mastodonParticipants(DmConversation $conversation): array
+    {
+        return DmConversationParticipant::where('conversation_id', $conversation->id)
+            ->orderBy('id')
+            ->pluck('profile_id')
+            ->map(fn ($id) => AccountService::getMastodon($id, true))
+            ->filter(fn ($account) => $account && isset($account['id']))
+            ->values()
+            ->all();
+    }
+
     /**
      * Messages are not statuses any more, but Mastodon clients expect one as
-     * `last_status`, so this builds the entity from the message.
+     * `last_status`, so this builds the entity from the message. Everyone in
+     * the conversation other than the author is a mention, the viewer
+     * included: that is how a client tells the message was addressed to them.
      *
-     * @param  array<int, array<string, mixed>>  $accounts
+     * @param  array<int, array<string, mixed>>  $participants  Everyone in the conversation, as account entities
      * @return array<string, mixed>
      */
-    public function mastodonStatus(DmMessage $message, array $accounts, int $viewerId): array
+    public function mastodonStatus(DmMessage $message, array $participants, int $viewerId): array
     {
         $media = collect($this->media($message))->map(function (array $item) {
             $mime = $item['mime'] ?? null;
@@ -259,7 +348,7 @@ class DirectMessagePayloadService
             'visibility' => 'direct',
             'application' => null,
             'language' => null,
-            'mentions' => collect($accounts)
+            'mentions' => collect($participants)
                 ->filter(fn ($account) => (string) $account['id'] !== (string) $message->profile_id)
                 ->map(fn ($account) => [
                     'id' => (string) $account['id'],

app/Services/DirectMessageService.php

@@ -232,6 +232,28 @@ class DirectMessageService
         return $conversation ? [$conversation, $participant] : null;
     }
 
+    /**
+     * The conversation behind an id from /api/v1/conversations, which is the
+     * viewer's participant row. A conversation id is accepted too.
+     *
+     * @return array{0: DmConversation, 1: DmConversationParticipant}|null
+     */
+    public function conversationForMastodonId(int|string $id, int $profileId): ?array
+    {
+        $participant = DmConversationParticipant::where('id', $id)
+            ->where('profile_id', $profileId)
+            ->where('state', '!=', DmConversationParticipant::STATE_LEFT)
+            ->first();
+
+        if ($participant) {
+            $conversation = DmConversation::find($participant->conversation_id);
+
+            return $conversation ? [$conversation, $participant] : null;
+        }
+
+        return $this->conversationFor($id, $profileId);
+    }
+
     /**
      * Profile ids of everyone in the conversation.
      *

tests/Feature/DirectMessage/DirectMessageLegacyApiTest.php

@@ -15,7 +15,7 @@ use Illuminate\Support\Facades\Redis;
 use Illuminate\Support\Facades\Storage;
 use Laravel\Passport\Passport;
 
-require_once __DIR__.'/helpers.php';
+require_once __DIR__ . '/helpers.php';
 
 uses(LazilyRefreshDatabase::class);
 
@@ -35,13 +35,19 @@ beforeEach(function () {
     Queue::fake();
     Http::fake();
 
+    // Ids minted in the same millisecond only sort by creation order when the
+    // worker bits are fixed. Left unset they are random for every id.
+    config(['snowflake.datacenter_id' => 1, 'snowflake.worker_id' => 1]);
+
     $this->withoutMiddleware(ThrottleRequests::class);
 
+    // Ids minted in the same millisecond only sort by creation order when the
+    // worker bits are fixed. Left unset they are random for every id.
+    config(['snowflake.datacenter_id' => 1, 'snowflake.worker_id' => 1]);
+
     config([
         'instance.enable_cc' => false,
         'federation.activitypub.enabled' => true,
-        'snowflake.datacenter_id' => 1,
-        'snowflake.worker_id' => 1,
     ]);
 });
 
@@ -68,7 +74,7 @@ describe('thread endpoints', function () {
 
         Passport::actingAs($bob, ['read', 'write']);
 
-        $thread = $this->getJson('/api/v1.1/direct/thread?pid='.$alice->profile_id);
+        $thread = $this->getJson('/api/v1.1/direct/thread?pid=' . $alice->profile_id);
 
         $thread->assertOk()
             ->assertJsonPath('id', (string) $alice->profile_id)
@@ -86,7 +92,7 @@ describe('thread endpoints', function () {
 
         Passport::actingAs($alice, ['read', 'write']);
 
-        $this->getJson('/api/v1.1/direct/thread?pid='.$bob->profile_id)
+        $this->getJson('/api/v1.1/direct/thread?pid=' . $bob->profile_id)
             ->assertOk()
             ->assertJsonCount(0, 'messages')
             ->assertJsonPath('conversation_id', null);
@@ -115,12 +121,12 @@ describe('thread endpoints', function () {
 
         Passport::actingAs($bob, ['read', 'write']);
 
-        $this->getJson('/api/v1.1/direct/thread?pid='.$alice->profile_id)
+        $this->getJson('/api/v1.1/direct/thread?pid=' . $alice->profile_id)
             ->assertJsonPath('messages.0.text', 'my cat')
             ->assertJsonPath('messages.0.type', 'photo')
             ->assertJsonCount(1, 'messages.0.carousel');
 
-        expect($this->getJson('/api/v1.1/direct/thread?pid='.$alice->profile_id)->json('messages.0.media'))->not->toBeNull();
+        expect($this->getJson('/api/v1.1/direct/thread?pid=' . $alice->profile_id)->json('messages.0.media'))->not->toBeNull();
     });
 
     it('marks a thread read and reports it as seen to the sender', function () {
@@ -138,7 +144,7 @@ describe('thread endpoints', function () {
         expect(DmConversationParticipant::where('profile_id', $bob->profile_id)->value('unread_count'))->toBe(0);
 
         Passport::actingAs($alice, ['read', 'write']);
-        $this->getJson('/api/v1.1/direct/thread?pid='.$bob->profile_id)->assertJsonPath('messages.0.seen', true);
+        $this->getJson('/api/v1.1/direct/thread?pid=' . $bob->profile_id)->assertJsonPath('messages.0.seen', true);
     });
 
     it('deletes by the id the thread handed out', function () {
@@ -195,22 +201,50 @@ describe('GET /api/v1/conversations', function () {
 
         Passport::actingAs($bob, ['read', 'write']);
 
-        $this->getJson('/api/v1/conversations')
+        $response = $this->getJson('/api/v1/conversations')
             ->assertOk()
             ->assertJsonCount(1)
-            ->assertJsonPath('0.id', (string) $conversation->id)
             ->assertJsonPath('0.unread', true)
             ->assertJsonPath('0.accounts.0.id', (string) $alice->profile_id)
             ->assertJsonPath('0.last_status.id', (string) $message->id)
             ->assertJsonPath('0.last_status.visibility', 'direct')
             ->assertJsonPath('0.last_status.content', '<p>hello</p>')
-            ->assertJsonPath('0.last_status.account.id', (string) $alice->profile_id);
+            ->assertJsonPath('0.last_status.account.id', (string) $alice->profile_id)
+            ->assertJsonPath('0.last_status.mentions.0.id', (string) $bob->profile_id);
+
+        // Pixelix stores this id as a 32-bit int, as the old endpoint allowed
+        $id = $response->json('0.id');
+        expect((int) $id)->toBeLessThan(2 ** 31)
+            ->and((int) $id)->toBeGreaterThan(0)
+            ->and((int) $id)->toBe(DmConversationParticipant::where('profile_id', $bob->profile_id)->value('id'));
+
+        $this->postJson("/api/v1/conversations/{$id}/read")->assertOk()->assertJsonPath('unread', false);
 
-        $this->postJson("/api/v1/conversations/{$conversation->id}/read")->assertOk()->assertJsonPath('unread', false);
-        $this->deleteJson("/api/v1/conversations/{$conversation->id}")->assertOk();
+        // The real conversation id still works for clients written against it
+        $this->postJson("/api/v1/conversations/{$conversation->id}/read")->assertOk();
+
+        $this->deleteJson("/api/v1/conversations/{$id}")->assertOk();
         $this->getJson('/api/v1/conversations')->assertJsonCount(0);
     });
 
+    it('does not let one person use another persons conversation id', function () {
+        $alice = dmLocalUser();
+        $bob = dmLocalUser();
+        $eve = dmLocalUser();
+
+        $service = app(DirectMessageService::class);
+        $conversation = $service->findOrCreateDm(dmProfile($alice), dmProfile($bob));
+        $service->sendMessage($conversation, dmProfile($alice), ['body' => 'hello']);
+
+        Passport::actingAs($bob, ['read', 'write']);
+        $id = $this->getJson('/api/v1/conversations')->json('0.id');
+
+        Passport::actingAs($eve, ['read', 'write']);
+        $this->postJson("/api/v1/conversations/{$id}/read")->assertNotFound();
+        $this->deleteJson("/api/v1/conversations/{$id}")->assertNotFound();
+        $this->deleteJson("/api/v1/conversations/{$conversation->id}")->assertNotFound();
+    });
+
     it('only includes groups when asked', function () {
         $alice = dmLocalUser();
         $bob = dmLocalUser();
@@ -243,6 +277,78 @@ describe('GET /api/v1/conversations', function () {
     });
 });
 
+describe('mastodon status endpoints', function () {
+    it('serves a message as a status to people in the conversation and nobody else', function () {
+        $alice = dmLocalUser();
+        $bob = dmLocalUser();
+        $eve = dmLocalUser();
+
+        $service = app(DirectMessageService::class);
+        $conversation = $service->findOrCreateDm(dmProfile($alice), dmProfile($bob));
+        $message = $service->sendMessage($conversation, dmProfile($alice), ['body' => 'hello']);
+
+        Passport::actingAs($bob, ['read', 'write']);
+
+        $this->getJson("/api/v1/statuses/{$message->id}")
+            ->assertOk()
+            ->assertJsonPath('id', (string) $message->id)
+            ->assertJsonPath('visibility', 'direct')
+            ->assertJsonPath('content', '<p>hello</p>')
+            ->assertJsonPath('account.id', (string) $alice->profile_id)
+            ->assertJsonPath('mentions.0.id', (string) $bob->profile_id);
+
+        Passport::actingAs($eve, ['read', 'write']);
+
+        $this->getJson("/api/v1/statuses/{$message->id}")->assertNotFound();
+    });
+
+    it('returns the rest of the conversation as the context of a message', function () {
+        $alice = dmLocalUser();
+        $bob = dmLocalUser();
+
+        $service = app(DirectMessageService::class);
+        $conversation = $service->findOrCreateDm(dmProfile($alice), dmProfile($bob));
+        $one = $service->sendMessage($conversation, dmProfile($alice), ['body' => 'one']);
+        $two = $service->sendMessage($conversation, dmProfile($bob), ['body' => 'two']);
+        $three = $service->sendMessage($conversation, dmProfile($alice), ['body' => 'three']);
+
+        Passport::actingAs($bob, ['read', 'write']);
+
+        $this->getJson("/api/v1/statuses/{$two->id}/context")
+            ->assertOk()
+            ->assertJsonCount(1, 'ancestors')
+            ->assertJsonPath('ancestors.0.id', (string) $one->id)
+            ->assertJsonCount(1, 'descendants')
+            ->assertJsonPath('descendants.0.id', (string) $three->id)
+            ->assertJsonPath('descendants.0.visibility', 'direct');
+
+        // The usual client flow: open the conversation from its last status
+        $this->getJson("/api/v1/statuses/{$three->id}/context")
+            ->assertOk()
+            ->assertJsonCount(2, 'ancestors')
+            ->assertJsonCount(0, 'descendants');
+    });
+
+    it('lets the author delete a message through the status endpoint', function () {
+        $alice = dmLocalUser();
+        $bob = dmLocalUser();
+
+        $service = app(DirectMessageService::class);
+        $conversation = $service->findOrCreateDm(dmProfile($alice), dmProfile($bob));
+        $message = $service->sendMessage($conversation, dmProfile($alice), ['body' => 'oops']);
+
+        Passport::actingAs($bob, ['read', 'write']);
+        $this->deleteJson("/api/v1/statuses/{$message->id}")->assertNotFound();
+
+        Passport::actingAs($alice, ['read', 'write']);
+        $this->deleteJson("/api/v1/statuses/{$message->id}")
+            ->assertOk()
+            ->assertJsonPath('text', 'oops');
+
+        expect(DmMessage::count())->toBe(0);
+    });
+});
+
 describe('media housekeeping', function () {
     it('does not let direct message media be attached to a post', function () {
         $alice = dmLocalUser();