mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(gmail): provider-id lookups no longer reach other users' mail
gmail_id and graph_id are unique per account only, yet the batch handler and the Graph delta syncer looked rows up by the bare id across the whole install, so a collision enriched, relabelled, flagged or erased another user's message. Graph lookups now carry the account; the Gmail one is scoped to the owning user, because a Gmail message may be attributed to a sibling account of the same user. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
98030c6f4b
commit
ba426fc6d8
5 files changed
+57
-8
No files matched your search
@@ -19,7 +19,7 @@ use Doctrine\ORM\Mapping as ORM;
|
||||
// batch handlers dedup in PHP before inserting, but that check is a read on
|
||||
// stale data — this is the guard that actually holds when batches overlap
|
||||
// across runs or retries. Provider id leads the column list so the indexes
|
||||
// also serve the id-only lookups (findOneBy(['gmailId'|'graphId'])).
|
||||
// also serve the provider-id lookups (graph by account, gmail by owning user).
|
||||
#[ORM\UniqueConstraint(name: 'uniq_message_gmail_id_account', columns: ['gmail_id', 'account_id'])]
|
||||
#[ORM\UniqueConstraint(name: 'uniq_message_graph_id_account', columns: ['graph_id', 'account_id'])]
|
||||
// IMAP UIDs are unique within a mailbox, so this is the guard for the sync
|
||||
|
||||
@@ -183,7 +183,7 @@ final readonly class SyncGmailMessageBatchHandler
|
||||
// redelivered batch — a worker restart, a retry after a partial
|
||||
// failure — used to build a second row for every message in it.
|
||||
if ('' !== $gmailId) {
|
||||
$known = $this->messageRepository->findOneBy(['gmailId' => $gmailId]);
|
||||
$known = $this->messageRepository->findOneByGmailIdForUser($account->usr, $gmailId);
|
||||
|
||||
if (null !== $known) {
|
||||
// A native Gmail row: Gmail is the authority on its flags,
|
||||
|
||||
@@ -1537,6 +1537,30 @@ class MessageRepository extends ServiceEntityRepository
|
||||
->getResult();
|
||||
}
|
||||
|
||||
/**
|
||||
* The one row for a Gmail id among the owner's accounts, for the batch
|
||||
* handler's "already stored?" check.
|
||||
*
|
||||
* Scoped through the user and not the account, for the reason given on
|
||||
* findByGmailIdsForUser(): a message fetched by one Gmail account may be
|
||||
* attributed to a sibling. It used to be a bare findOneBy(['gmailId']),
|
||||
* which searched every user on the install — a gmailId is only unique per
|
||||
* account, so another user's row could be found and enriched with this
|
||||
* user's labels. QueryBuilder because the owner is behind an association.
|
||||
*/
|
||||
public function findOneByGmailIdForUser(User $user, string $gmailId): ?Message
|
||||
{
|
||||
return $this->createQueryBuilder('m')
|
||||
->innerJoin('m.account', 'a')
|
||||
->where('a.usr = :usr')
|
||||
->andWhere('m.gmailId = :gmailId')
|
||||
->setParameter('usr', $user)
|
||||
->setParameter('gmailId', $gmailId)
|
||||
->setMaxResults(1)
|
||||
->getQuery()
|
||||
->getOneOrNullResult();
|
||||
}
|
||||
|
||||
/**
|
||||
* QueryBuilder on two counts: the owner is reached through the account
|
||||
* association, which findBy() cannot traverse, and only one column comes
|
||||
|
||||
@@ -217,7 +217,7 @@ final class GraphApiSyncer
|
||||
// Cast for the same reason, and here the symptom would be silent:
|
||||
// an int compared against a stored string id matches nothing, so
|
||||
// the message is simply not erased and nothing is raised.
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => (string) $graphId]);
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => (string) $graphId, 'account' => $account]);
|
||||
|
||||
if (null === $message) {
|
||||
continue;
|
||||
@@ -294,7 +294,7 @@ final class GraphApiSyncer
|
||||
// phone stayed unread here, exactly as it did on IMAP, and for the
|
||||
// same reason — nothing ever re-read the state of a row it already
|
||||
// had.
|
||||
$state = $this->flagStateOf($graphId, $item);
|
||||
$state = $this->flagStateOf($account, $graphId, $item);
|
||||
|
||||
if (null !== $state) {
|
||||
$flagStates[] = $state;
|
||||
@@ -323,7 +323,7 @@ final class GraphApiSyncer
|
||||
*
|
||||
* @param array<string,mixed> $item
|
||||
*/
|
||||
private function flagStateOf(string $graphId, array $item): ?RemoteFlagState
|
||||
private function flagStateOf(Account $account, string $graphId, array $item): ?RemoteFlagState
|
||||
{
|
||||
if (false === array_key_exists('isRead', $item)) {
|
||||
// A delta entry that does not mention read state is not asserting
|
||||
@@ -332,7 +332,7 @@ final class GraphApiSyncer
|
||||
return null;
|
||||
}
|
||||
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => $graphId]);
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => $graphId, 'account' => $account]);
|
||||
|
||||
if (null === $message) {
|
||||
return null;
|
||||
@@ -355,7 +355,7 @@ final class GraphApiSyncer
|
||||
|
||||
private function attachFolderLabel(Account $account, string $graphId, string $folderId): void
|
||||
{
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => $graphId]);
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => $graphId, 'account' => $account]);
|
||||
|
||||
if (null === $message) {
|
||||
return;
|
||||
@@ -404,7 +404,7 @@ final class GraphApiSyncer
|
||||
|
||||
private function detachFolderLabel(Account $account, string $graphId, string $folderId): void
|
||||
{
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => $graphId]);
|
||||
$message = $this->messageRepository->findOneBy(['graphId' => $graphId, 'account' => $account]);
|
||||
|
||||
if (null === $message) {
|
||||
return;
|
||||
|
||||
@@ -170,6 +170,31 @@ final class GmailInboundReadStateTest extends KernelTestCase
|
||||
self::assertNotNull($message->seenAt, 'the local change is still in flight');
|
||||
}
|
||||
|
||||
/**
|
||||
* A gmailId is only unique per account. The "already stored?" lookup was
|
||||
* a bare findOneBy(['gmailId']) across the whole install, so another
|
||||
* user's row with the same id was found and rewritten with this user's
|
||||
* labels, and this user's message was never stored at all.
|
||||
*/
|
||||
public function testAnotherUsersRowWithTheSameGmailIdIsLeftAlone(): void
|
||||
{
|
||||
$mine = $this->account;
|
||||
$this->user = $this->seedUser();
|
||||
$this->account = $this->seedAccount();
|
||||
$theirs = $this->seedMessage(seen: false);
|
||||
$this->account = $mine;
|
||||
|
||||
$this->handle(['INBOX']);
|
||||
|
||||
$this->em->refresh($theirs);
|
||||
|
||||
self::assertNull($theirs->seenAt, 'the other user\'s row must not be touched');
|
||||
self::assertNotNull(
|
||||
$this->em->getRepository(Message::class)->findOneBy(['gmailId' => self::GMAIL_ID, 'account' => $mine]),
|
||||
'the message is stored for the account that fetched it',
|
||||
);
|
||||
}
|
||||
|
||||
// ── helpers ───────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
|
||||
Reference in new issue
Block a user