Make spending a public link's view the thing that guards it

src/Application/Account/Ports/PublicLinkService.php

@@ -114,13 +114,17 @@ public function create(PublicLink $publicLink): int;
     public function getAll(): array;
 
     /**
-     * Increments the visit counter of a link
+     * Records a view of a link, if it still has one to give.
+     *
+     * The view limit and the expiry are conditions of the update itself, so this is the moment the
+     * link is spent — and the answer is whether it was. False means the link was exhausted or had
+     * expired, and the caller must not hand out the account.
      *
      * @throws NoSuchItemException
      * @throws ConstraintException
      * @throws QueryException
      */
-    public function addLinkView(PublicLink $publicLink): void;
+    public function addLinkView(PublicLink $publicLink): bool;
 
     /**
      * @throws SPException

src/Application/Account/Services/PublicLink.php

@@ -279,7 +279,7 @@ public function getAll(): array
      * @throws QueryException
      * @throws ServiceException
      */
-    public function addLinkView(PublicLinkModel $publicLink): void
+    public function addLinkView(PublicLinkModel $publicLink): bool
     {
         $useInfo = [];
 
@@ -297,7 +297,16 @@ public function addLinkView(PublicLinkModel $publicLink): void
 
         $useInfo[] = self::getUseInfo($publicLink->getHash(), $this->request);
 
-        $this->publicLinkRepository->addLinkView($publicLink->mutate(['useInfo' => Serde::serialize($useInfo)]));
+        // False when the link is exhausted or expired: the repository makes both conditions of the
+        // update, so this answers whether the view was actually recorded, and the caller must not
+        // hand out the account unless it was.
+        //
+        // The usage list itself is still assembled from the row as it was read, so two views
+        // landing together record one entry between them. The counter is exact — that is the one
+        // that decides access — while the list is a log.
+        return $this->publicLinkRepository->addLinkView(
+            $publicLink->mutate(['useInfo' => Serde::serialize($useInfo)])
+        );
     }
 
     /**

src/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkController.php

@@ -101,12 +101,13 @@ public function viewLinkAction(string $hash): ActionResponse
             logger(sprintf('Public link not found: %s', $hash));
         }
 
-        if ($publicLink !== null
-            && time() < $publicLink->getDateExpire()
-            && $publicLink->getCountViews() < $publicLink->getMaxCountViews()
-        ) {
-            $this->publicLinkService->addLinkView($publicLink);
-
+        // Spending the view is what decides whether to serve. The expiry and the view limit used
+        // to be tested here, against a row that had already been read, and the counter was then
+        // incremented — so two requests arriving together on a link with one view left both got
+        // past the test and both were served. A link issued to be followed once handed the
+        // account out twice. Both conditions now live in that update, and it reports whether it
+        // applied.
+        if ($publicLink !== null && $this->publicLinkService->addLinkView($publicLink)) {
             $this->accountService->incrementViewCounter($publicLink->getItemId());
             $this->accountService->incrementDecryptCounter($publicLink->getItemId());
 

src/Infrastructure/Adapter/Out/Account/Repositories/PublicLink.php

@@ -258,14 +258,24 @@ private function checkDuplicatedOnAdd(int $id): bool
      */
     public function addLinkView(PublicLinkModel $publicLinkData): bool
     {
+        // The limit and the expiry are conditions of the update, not something checked beforehand.
+        // They used to be tested in PHP against a row that had already been read, so two requests
+        // arriving together on a link with one view left both passed the check and both were
+        // served — a link issued for a single view handed the account out twice. Here the server
+        // decides, and a link that is exhausted or expired matches no row.
+        //
+        // COALESCE because `countViews` is nullable: a NULL would make the comparison NULL and
+        // refuse a link that has simply never been followed.
         $query = $this->queryFactory
             ->newUpdate()
             ->table('PublicLink')
             ->set('countViews', '(countViews + 1)')
             ->set('totalCountViews', '(totalCountViews + 1)')
             ->col('useInfo', $publicLinkData->getUseInfo())
             ->where('hash = :hash')
-            ->bindValues(['hash' => $publicLinkData->getHash()]);
+            ->where('COALESCE(countViews, 0) < maxCountViews')
+            ->where('dateExpire > :now')
+            ->bindValues(['hash' => $publicLinkData->getHash(), 'now' => time()]);
 
         $queryData = QueryData::build($query)->setOnErrorMessage(__u('Error while updating the link'));
 

tests/Integration/Application/Account/PublicLinkRoundTripTest.php

@@ -230,6 +230,88 @@ public function testALinkStopsYieldingTheAccountOnceItsViewLimitIsReached(): voi
         );
     }
 
+    /**
+     * A copy of the link read before it was spent cannot be used to spend it again.
+     *
+     * This is the concurrent case, made deterministic. Two requests arriving together each read
+     * the row before either has written to it, so each holds a copy saying the link still has
+     * views left — and the check used to be made against that copy:
+     *
+     * ```php
+     * && $publicLink->getCountViews() < $publicLink->getMaxCountViews()
+     * ```
+     *
+     * Both passed, both were served, and a link issued for a single view handed the account out
+     * twice. Holding a stale copy is exactly what the second request has, so exhausting the link
+     * and then presenting the copy reproduces it without needing two processes.
+     *
+     * The limit and the expiry are conditions of the update now, so the server is what refuses,
+     * and a copy of any age cannot talk it round.
+     */
+    public function testAStaleCopyOfALinkCannotSpendItPastItsLimit(): void
+    {
+        $publicLinkService = $this->dic->get(PublicLinkServiceInterface::class);
+
+        $suffix = bin2hex(random_bytes(4));
+        $accountId = $this->createAccount('race-' . $suffix, 'RacePass!' . bin2hex(random_bytes(6)));
+
+        $hash = $this->createLinkFor($accountId);
+
+        // The copy the second request would be holding: read before anything has been spent.
+        $staleCopy = $publicLinkService->getByHash($hash);
+
+        $maxCountViews = $staleCopy->getMaxCountViews();
+        self::assertGreaterThan(0, $maxCountViews, 'setup: the configured view limit must be positive');
+
+        for ($view = 1; $view <= $maxCountViews; $view++) {
+            self::assertNotNull($this->followLink($hash), sprintf('setup: view %d should be allowed', $view));
+        }
+
+        self::assertFalse(
+            $publicLinkService->addLinkView($staleCopy),
+            'a copy read before the link was exhausted must not be able to spend it again'
+        );
+
+        self::assertSame(
+            $maxCountViews,
+            $publicLinkService->getByHash($hash)->getCountViews(),
+            'the refused attempt must not have moved the counter past the limit'
+        );
+    }
+
+    /**
+     * The same for an expiry that passed while the request was in flight.
+     *
+     * The expiry was the other half of the same check, read from the same stale copy, so it is
+     * refused by the same update rather than by a comparison made before it.
+     */
+    public function testALinkThatExpiresBeforeTheViewIsRecordedIsRefused(): void
+    {
+        $publicLinkService = $this->dic->get(PublicLinkServiceInterface::class);
+
+        $suffix = bin2hex(random_bytes(4));
+        $accountId = $this->createAccount('exp-' . $suffix, 'ExpPass!' . bin2hex(random_bytes(6)));
+
+        $hash = $this->createLinkFor($accountId);
+        $publicLink = $publicLinkService->getByHash($hash);
+
+        // Expire it behind the copy the request is holding, which still says it is good.
+        $statement = getDbHandler()->getConnection()
+                                   ->prepare('UPDATE `PublicLink` SET `dateExpire` = :expired WHERE `hash` = :hash');
+        $statement->execute(['expired' => time() - 1, 'hash' => $hash]);
+
+        self::assertGreaterThan(
+            time(),
+            $publicLink->getDateExpire(),
+            'setup: the copy in hand must still believe the link is live'
+        );
+
+        self::assertFalse(
+            $publicLinkService->addLinkView($publicLink),
+            'a link that expired before the view was recorded must not be spent'
+        );
+    }
+
     /**
      * A link whose expiry has already passed does not yield the account either, even though it is
      * nowhere near its view limit. The expiry is moved into the past through
@@ -306,12 +388,13 @@ private function followLink(string $hash): ?string
 
         $publicLink = $publicLinkService->getByHash($hash);
 
-        if (time() >= $publicLink->getDateExpire() || $publicLink->getCountViews() >= $publicLink->getMaxCountViews()) {
+        // Spending the view is the guard, exactly as the controller now has it: the expiry and the
+        // limit are conditions of the update, so this answers whether there was a view left to
+        // take. It used to be a pair of comparisons here, against the row just read.
+        if (!$publicLinkService->addLinkView($publicLink)) {
             return null;
         }
 
-        $publicLinkService->addLinkView($publicLink);
-
         $accountService->incrementViewCounter($publicLink->getItemId());
         $accountService->incrementDecryptCounter($publicLink->getItemId());
 

tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkRefusalsTest.php

@@ -42,6 +42,7 @@
 use SP\Domain\Core\Exceptions\CryptException;
 use SP\Domain\Crypt\Vault;
 use SP\Infrastructure\Crypt\Crypt;
+use SP\Infrastructure\Database\QueryData;
 use SP\Infrastructure\Events\EventDispatcher;
 use SP\Tests\Support\BodyChecker;
 use SP\Tests\Support\Generators\AccountDataGenerator;
@@ -258,7 +259,31 @@ private function givenALink(array $properties): void
                                                  )
                                              );
 
-        $this->addDatabaseMapperResolver(PublicLink::class, new QueryResult([$publicLink]));
+        // Whether the link still has a view to give is decided by the update that spends it: its
+        // WHERE clause carries the limit and the expiry, so an exhausted or expired link matches
+        // no row and the update reports nothing affected. The double has to answer the way the
+        // server would, or a test for a refusal only shows that the fixture said "expired"
+        // somewhere — the guard it is aiming at moved out of PHP precisely so that two
+        // simultaneous requests cannot both pass it.
+        $spent = (int)($publicLink->getCountViews() ?? 0) >= (int)($publicLink->getMaxCountViews() ?? 0)
+                 || time() >= (int)$publicLink->getDateExpire();
+
+        // One resolver rather than a mapper resolver beside it: `databaseQueryResolver` is
+        // consulted first and short-circuits the mapper ones, so the read has to be answered here
+        // too. Not a static closure — the harness binds it with Closure::call().
+        $this->databaseQueryResolver = function (QueryData $queryData) use ($spent, $publicLink): QueryResult {
+            $statement = $queryData->getQuery()->getStatement();
+
+            if (str_contains($statement, 'UPDATE') && str_contains($statement, 'PublicLink')) {
+                return new QueryResult([], $spent ? 0 : 1);
+            }
+
+            if ($queryData->getMapClassName() === PublicLink::class) {
+                return new QueryResult([$publicLink]);
+            }
+
+            return new QueryResult([], 1, 100);
+        };
     }
 
     /**