From 2cd9697bc914e85008bdc994d17cb219bf14cad1 Mon Sep 17 00:00:00 2001 From: blaipr Date: Fri, 21 Aug 2026 00:19:32 +0200 Subject: [PATCH] Ask for a token password whenever a vault will be built MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A token's vault is the master password, sealed with the token's own password and the token itself. Two rules decided when a token has one, and they were not the same rule: AuthTokenForm::checkCommon() demanded a password for isSecuredAction(), while AuthToken::injectSecureData() builds a vault for isSecuredAction() || canUseSecureTokenAction() — the three view actions, which need the master password when a caller asks for custom fields. So a token for ACCOUNT_VIEW, CATEGORY_VIEW or CLIENT_VIEW could be created with the password field left blank, and its vault was then sealed with the empty string. Nothing can open it: Api::getMasterPassFromVault() reads tokenPass as a required parameter, and required refuses the empty string, so the one password that would work cannot be presented. Against the real dispatch such a token answers 400 for an empty tokenPass and 401 for any other, while the same token created with a password answers 200. It was issued, reported as created, and permanently unable to do the thing that needed the vault. One predicate instead of two: needsSecureToken() is what "this token carries a vault" means, injectSecureData() uses it, and the form asks for the password exactly when it is true, or on a refresh, which re-seals the vault whatever the action. There is no second copy left to drift from. AuthTokenFormTest is new — the form had no test. Every vault-carrying action is refused without a password and accepted with one, with three controls so the rule cannot collapse into "always demand a password": an action carrying no vault still needs none, a refresh still always does, and the user and action checks that run first are pinned so a refusal cannot be the form rejecting the fixture for another reason. Restoring isSecuredAction() in the form fails exactly the three CAN_USE_SECURE_TOKEN_ACTIONS cases. UserFormTest kept its own copy of the demo admin id with a comment saying the real one was unreachable; it is public now, so the test uses it. --- src/Application/Auth/Services/AuthToken.php | 21 ++- .../Adapter/In/Web/Forms/AuthTokenForm.php | 2 +- .../In/Web/Forms/AuthTokenFormTest.php | 147 ++++++++++++++++++ .../Adapter/In/Web/Forms/UserFormTest.php | 10 +- 4 files changed, 170 insertions(+), 10 deletions(-) create mode 100644 tests/Unit/Infrastructure/Adapter/In/Web/Forms/AuthTokenFormTest.php diff --git a/src/Application/Auth/Services/AuthToken.php b/src/Application/Auth/Services/AuthToken.php index ebf03cb5e..ef89ca736 100644 --- a/src/Application/Auth/Services/AuthToken.php +++ b/src/Application/Auth/Services/AuthToken.php @@ -170,9 +170,7 @@ public function create(AuthTokenModel $authToken): int */ private function injectSecureData(AuthTokenModel $authToken, string $token): AuthTokenModel { - if (self::isSecuredAction($authToken->getActionId()) - || self::canUseSecureTokenAction($authToken->getActionId()) - ) { + if (self::needsSecureToken($authToken->getActionId())) { $properties = [ 'vault' => $this->getSecureData($token, $authToken->getHash() ?? '')->getSerialized(), 'hash' => Hash::hashKey($authToken->getHash() ?? '') @@ -189,6 +187,23 @@ private function injectSecureData(AuthTokenModel $authToken, string $token): Aut return $authToken->mutate($properties); } + /** + * Whether a token for this action carries a vault: the master password, sealed with the + * token's own password and the token itself. + * + * Both lists mean that, and anything asking the administrator for a token password has to ask + * exactly when one will be built. `AuthTokenForm` asked only for `isSecuredAction()`, so a + * token for one of the three `CAN_USE_SECURE_TOKEN_ACTIONS` could be created with the field + * left blank — and the vault was then sealed with the empty string. Nothing can open it: + * `Api::getMasterPassFromVault()` reads `tokenPass` as a required parameter, which refuses the + * empty string, so the one password that would work cannot be presented. The token was issued, + * reported as created, and permanently unable to do the thing it was issued for. + */ + public static function needsSecureToken(int $action): bool + { + return self::isSecuredAction($action) || self::canUseSecureTokenAction($action); + } + public static function isSecuredAction(int $action): bool { return in_array($action, self::SECURED_ACTIONS, true); diff --git a/src/Infrastructure/Adapter/In/Web/Forms/AuthTokenForm.php b/src/Infrastructure/Adapter/In/Web/Forms/AuthTokenForm.php index 51076e36e..070eed69f 100644 --- a/src/Infrastructure/Adapter/In/Web/Forms/AuthTokenForm.php +++ b/src/Infrastructure/Adapter/In/Web/Forms/AuthTokenForm.php @@ -103,7 +103,7 @@ protected function checkCommon(): void } if (empty($this->authTokenData->getHash()) - && (AuthTokenService::isSecuredAction($this->authTokenData->getActionId()) + && (AuthTokenService::needsSecureToken($this->authTokenData->getActionId()) || $this->isRefresh()) ) { throw new ValidationException(__u('Password cannot be blank')); diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AuthTokenFormTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AuthTokenFormTest.php new file mode 100644 index 000000000..7f3b7e2ec --- /dev/null +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Forms/AuthTokenFormTest.php @@ -0,0 +1,147 @@ +request = $this->createMock(RequestService::class); + } + + /** + * Every action a vault gets built for. The first three are the ones the form used to let + * through; the last two it always refused, and must go on refusing. + * + * @return array + */ + public static function actionsThatCarryAVaultProvider(): array + { + return [ + 'ACCOUNT_VIEW' => [AclActionsInterface::ACCOUNT_VIEW], + 'CATEGORY_VIEW' => [AclActionsInterface::CATEGORY_VIEW], + 'CLIENT_VIEW' => [AclActionsInterface::CLIENT_VIEW], + 'ACCOUNT_VIEW_PASS' => [AclActionsInterface::ACCOUNT_VIEW_PASS], + 'ACCOUNT_CREATE' => [AclActionsInterface::ACCOUNT_CREATE], + ]; + } + + #[Test] + #[DataProvider('actionsThatCarryAVaultProvider')] + public function aTokenThatWillCarryAVaultCannotBeCreatedWithoutAPassword(int $actionId): void + { + $this->givenARequestFor($actionId, password: ''); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Password cannot be blank'); + + $this->buildForm()->validateFor(AclActionsInterface::AUTHTOKEN_CREATE); + } + + #[Test] + #[DataProvider('actionsThatCarryAVaultProvider')] + public function theSameTokenIsAcceptedWithOne(int $actionId): void + { + $this->givenARequestFor($actionId, password: 'a-token-password'); + + $form = $this->buildForm()->validateFor(AclActionsInterface::AUTHTOKEN_CREATE); + + self::assertSame('a-token-password', $form->getItemData()->getHash()); + self::assertSame($actionId, $form->getItemData()->getActionId()); + } + + /** + * The control, and the reason this is not simply "always demand a password": most actions + * carry no vault at all, and a token for one of them is meant to be usable without one. + */ + #[Test] + public function aTokenThatCarriesNoVaultStillNeedsNoPassword(): void + { + $this->givenARequestFor(AclActionsInterface::CATEGORY_SEARCH, password: ''); + + $form = $this->buildForm()->validateFor(AclActionsInterface::AUTHTOKEN_CREATE); + + self::assertSame(AclActionsInterface::CATEGORY_SEARCH, $form->getItemData()->getActionId()); + } + + /** + * Refreshing re-seals the vault whatever the action, so it has always needed the password — + * unchanged here, and asserted so the shared predicate cannot quietly drop it. + */ + #[Test] + public function refreshingAlwaysNeedsThePassword(): void + { + $this->givenARequestFor(AclActionsInterface::CATEGORY_SEARCH, password: '', refresh: true); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Password cannot be blank'); + + $this->buildForm()->validateFor(AclActionsInterface::AUTHTOKEN_CREATE); + } + + /** + * The two checks that come before the password one, so a refusal above cannot be the form + * rejecting the fixture for some other reason. + */ + #[Test] + public function theUserAndTheActionAreBothRequired(): void + { + $this->givenARequestFor(0, password: 'p'); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Action not set'); + + $this->buildForm()->validateFor(AclActionsInterface::AUTHTOKEN_CREATE); + } + + private function givenARequestFor(int $actionId, string $password, bool $refresh = false): void + { + $this->request->method('analyzeBool')->willReturn($refresh); + $this->request->method('analyzeInt')->willReturnMap( + [ + ['users', null, self::A_USER], + ['actions', null, $actionId], + ] + ); + $this->request->method('analyzeEncrypted')->willReturn($password); + } + + private function buildForm(): AuthTokenForm + { + return new AuthTokenForm($this->application, $this->request); + } +} diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Forms/UserFormTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Forms/UserFormTest.php index 23333f01f..2fb4bb223 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Forms/UserFormTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Forms/UserFormTest.php @@ -244,7 +244,7 @@ public function testEditCommonDemoAdminIsRejected(): void $this->expectException(ValidationException::class); $this->expectExceptionMessage('Ey, this is a DEMO!!'); - $form->validateFor(AclActionsInterface::USER_EDIT, self::DEMO_ADMIN_USER_ID); + $form->validateFor(AclActionsInterface::USER_EDIT, User::DEMO_ADMIN_ID); } /** @@ -310,7 +310,7 @@ public function testEditPassDemoAdminIsRejected(): void $this->expectException(ValidationException::class); $this->expectExceptionMessage('Ey, this is a DEMO!!'); - $form->validateFor(AclActionsInterface::USER_EDIT_PASS, self::DEMO_ADMIN_USER_ID); + $form->validateFor(AclActionsInterface::USER_EDIT_PASS, User::DEMO_ADMIN_ID); } /** @@ -326,7 +326,7 @@ public function testDeleteDemoAdminIsRejected(): void $this->expectException(ValidationException::class); $this->expectExceptionMessage('Ey, this is a DEMO!!'); - $form->validateFor(AclActionsInterface::USER_DELETE, self::DEMO_ADMIN_USER_ID); + $form->validateFor(AclActionsInterface::USER_DELETE, User::DEMO_ADMIN_ID); } /** @@ -347,8 +347,6 @@ public function testGetItemDataThrowsWhenUserDataNotSet(): void // ── helpers ────────────────────────────────────────────────────────────── - /** Mirrors UserForm::DEMO_ADMIN_USER_ID (private const, not reachable from the test). */ - private const int DEMO_ADMIN_USER_ID = 2; private function buildForm(?int $itemId = null): UserForm { @@ -358,7 +356,7 @@ private function buildForm(?int $itemId = null): UserForm /** * Builds an Application whose config has the demo mode enabled and whose * actor is an app-admin — the two preconditions UserForm::isDemo() needs - * besides the target itemId matching DEMO_ADMIN_USER_ID. + * besides the target itemId matching User::DEMO_ADMIN_ID. */ private function buildDemoAdminApplication(): Application {