diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php b/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php index 3881c11e3..7572ec6ea 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php @@ -53,9 +53,9 @@ public static function create(): array { return [ self::getItem('itemId', __('Account Id'), true), - self::getItem('notify', __('Notify when the link is used')), - self::getItem('dateExpire', __('Expiry date')), - self::getItem('maxCountViews', __('Maximum views')) + self::getItem('notify', __('Notify when the link is used')) + // No expiry or view limit: both come from the configuration, which is where the + // maximum lives, and a caller who could set them could outlive it. ]; } diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php b/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php index 8868fdbdf..8c1f236cf 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php @@ -24,12 +24,25 @@ public function createAction(): ApiResponse // call fail on the insert. 'typeId' => PublicLinkType::Account->value, 'notify' => (bool) $this->apiService->getParamInt('notify'), - 'dateExpire' => $this->apiService->getParamInt('dateExpire'), - 'maxCountViews' => $this->apiService->getParamInt('maxCountViews'), ]); $id = $this->publicLinkService->create($linkData); - $linkData = $linkData->mutate(['id' => $id]); + + // When the link expires and how often it may be opened are the administrator's + // configuration — `getPublinksMaxTime()` is a maximum, and neither web path lets anyone + // choose either. The service sets both, so the `dateExpire` and `maxCountViews` this + // endpoint used to accept were discarded on the way to the database. What made that + // costly rather than merely useless is that the response echoed the request back, so a + // caller was told a link would expire when they asked, and it did not. + $stored = $this->publicLinkService->getById($id); + + $linkData = $linkData->mutate( + [ + 'id' => $id, + 'dateExpire' => $stored->getDateExpire(), + 'maxCountViews' => $stored->getMaxCountViews(), + ] + ); $this->eventDispatcher->notify(new Event( 'create.publicLink', diff --git a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php index 1b9d46d97..7e97964d5 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php @@ -26,11 +26,14 @@ namespace SP\Tests\Integration\Infrastructure\Adapter\In\Api\Controllers; +use PDO; use PHPUnit\Framework\Attributes\Group; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Tests\Integration\Infrastructure\Adapter\In\Api\ApiTestCase; use stdClass; +use function SP\Tests\getDbHandler; + /** * Covers the public-link endpoints over the REST API. None of them had tests. * @@ -59,6 +62,67 @@ public function testCreateAction(): void $this->assertGreaterThan(0, $r->body->itemId); } + /** + * A caller cannot choose when a link expires, or how many times it may be opened. + * + * `getPublinksMaxTime()` and `getPublinksMaxViews()` are the administrator's configuration — + * a maximum — and neither web path offers a way to set either. The endpoint nonetheless + * accepted `dateExpire` and `maxCountViews`, advertised them in its own help, and discarded + * them on the way to the database. + * + * What made that costly rather than merely useless is the response: it echoed the request + * back, so a caller asking for a link that expired at a particular time was told it would, + * while the stored link expired at whatever the configuration said. For a feature that hands + * out credentials, being wrong about the expiry in the safe direction is luck, not design. + */ + public function testTheExpiryAndViewLimitComeFromTheConfigurationNotTheCaller(): void + { + $wanted = time() + 12345; + + $r = $this->callApi(AclActionsInterface::PUBLICLINK_CREATE, [ + 'itemId' => $this->accountId(), + 'dateExpire' => $wanted, + 'maxCountViews' => 7, + ]); + + $this->assertSame(201, $r->status); + + [$storedExpiry, $storedViews] = $this->storedLimitsFor($r->body->itemId); + + $this->assertNotSame($wanted, $storedExpiry, 'the caller must not be able to set the expiry'); + $this->assertNotSame(7, $storedViews, 'nor the view limit'); + + // The half that actually misled anyone: the answer has to describe the link that exists. + $this->assertSame( + $storedExpiry, + $r->body->data->dateExpire, + 'the response reported an expiry the link does not have' + ); + $this->assertSame( + $storedViews, + $r->body->data->maxCountViews, + 'the response reported a view limit the link does not have' + ); + } + + /** + * The expiry and the view limit as the row holds them. + * + * @return array{int, int} + */ + private function storedLimitsFor(int $id): array + { + $statement = getDbHandler()->getConnection() + ->prepare('SELECT `dateExpire`, `maxCountViews` FROM `PublicLink` WHERE `id` = :id'); + $statement->execute(['id' => $id]); + + $row = $statement->fetch(PDO::FETCH_ASSOC); + + $this->assertNotFalse($row, sprintf('No PublicLink row with id %d', $id)); + + return [(int)$row['dateExpire'], (int)$row['maxCountViews']]; + } + /** * A link with no account behind it would hand out nothing, so the account is required. */