diff --git a/app/Federation/Handlers/DirectMessageHandler.php b/app/Federation/Handlers/DirectMessageHandler.php index 2f6a171da..c429e5742 100644 --- a/app/Federation/Handlers/DirectMessageHandler.php +++ b/app/Federation/Handlers/DirectMessageHandler.php @@ -74,9 +74,10 @@ class DirectMessageHandler $left = DmConversationParticipant::where('conversation_id', $existing->id) ->where('state', DmConversationParticipant::STATE_LEFT) ->pluck('profile_id') + ->map(fn ($id) => (int) $id) ->all(); - $readers = $readers->reject(fn (Profile $profile) => in_array($profile->id, $left)); + $readers = $readers->reject(fn (Profile $profile) => in_array((int) $profile->id, $left, true)); if ($readers->isEmpty() || $this->requestLimitReached($existing, $actor, $readers)) { return null; diff --git a/app/Models/DmConversation.php b/app/Models/DmConversation.php index 5cf84a775..1af3015c0 100644 --- a/app/Models/DmConversation.php +++ b/app/Models/DmConversation.php @@ -49,7 +49,7 @@ class DmConversation extends Model public static function participantsHash(array $profileIds): string { $ids = array_values(array_unique(array_map('intval', $profileIds))); - sort($ids, SORT_NUMERIC); + sort($ids); return hash('sha256', implode(':', $ids)); } diff --git a/tests/Feature/DirectMessage/DirectMessageInboundTest.php b/tests/Feature/DirectMessage/DirectMessageInboundTest.php index e5b000594..10c4b572f 100644 --- a/tests/Feature/DirectMessage/DirectMessageInboundTest.php +++ b/tests/Feature/DirectMessage/DirectMessageInboundTest.php @@ -11,6 +11,7 @@ use App\Models\Notification; use App\Models\Status; use App\Models\UserDomainBlock; use App\Models\UserFilter; +use App\Services\DirectMessageService; use Illuminate\Foundation\Testing\LazilyRefreshDatabase; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Http; @@ -439,7 +440,7 @@ describe('groups', function () { it('drops a message when the only person it could reach here has left the group', function () { $bob = dmProfile(dmLocalUser()); $alice = dmRemoteProfile(); - $carol = dmRemoteProfile('carol', 'other.example'); + $carol = dmNeighbour($alice, dmRemoteProfile('carol', 'other.example')); dmSeedHosts(); dmDeliver($alice, dmNote($alice, '1', [$bob, $carol])); @@ -464,6 +465,30 @@ describe('groups', function () { }); }); +describe('conversation identity', function () { + it('hashes the same people to the same conversation whatever order they come in', function () { + $a = 1007571621538590723; + $b = $a + 1; + $c = $a + 2; + expect(DmConversation::dmHash($a, $b))->toBe(DmConversation::dmHash($b, $a))->and(DmConversation::participantsHash([$a, $b, $c]))->toBe(DmConversation::participantsHash([$c, $a, $b]))->and(DmConversation::participantsHash([$a, $b, $c]))->toBe(DmConversation::participantsHash([(string) $b, $c, $a, $a]))->and(DmConversation::dmHash($a, $b))->not->toBe(DmConversation::dmHash($a, $c)); + }); + + it('keeps one conversation when two participants have neighbouring ids', function () { + $bob = dmProfile(dmLocalUser()); + $alice = dmRemoteProfile(); + $carol = dmNeighbour($alice, dmRemoteProfile('carol', 'other.example')); + dmSeedHosts(); + + dmDeliver($carol, dmNote($carol, '1', [$alice, $bob])); + dmDeliver($alice, dmNote($alice, '2', [$bob, $carol])); + + $mine = app(DirectMessageService::class)->findOrCreateConversation($bob, collect([$alice, $carol])); + + expect(DmConversation::count())->toBe(1) + ->and(DmMessage::where('conversation_id', $mine->id)->count())->toBe(2); + }); +}); + describe('audience', function () { it('does not treat public, unlisted or followers-only notes as direct', function () { $alice = dmRemoteProfile(); diff --git a/tests/Feature/DirectMessage/helpers.php b/tests/Feature/DirectMessage/helpers.php index 4c3a33a34..a886c6ad0 100644 --- a/tests/Feature/DirectMessage/helpers.php +++ b/tests/Feature/DirectMessage/helpers.php @@ -6,6 +6,7 @@ use App\Models\User; use App\Models\UserSetting; use App\Util\ActivityPub\Inbox; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; if (! function_exists('dmLocalUser')) { /** @@ -42,6 +43,18 @@ if (! function_exists('dmLocalUser')) { ]); } + /** + * Give $b the id right after $a, the way two profiles created in the same + * millisecond end up. Ids that close are equal once compared as floats, + * so anything that orders or matches ids has to cope with it. + */ + function dmNeighbour(Profile $a, Profile $b): Profile + { + DB::table('profiles')->where('id', $b->id)->update(['id' => $a->id + 1]); + + return Profile::findOrFail($a->id + 1); + } + /** * Seed the DNS and banned-domain caches so URL validation passes without * a network lookup. Call after factories, the lazy refresh can flush the