From b297ec2231253dd3ad862983dd8b24ce4a7a17ca Mon Sep 17 00:00:00 2001 From: blaipr Date: Sun, 23 Aug 2026 02:40:00 +0200 Subject: [PATCH] Let the API create a token that carries a vault MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/v1/auth-tokens answered 500 "Error while retrieving master password from context" for every action a token carries a vault for — the five SECURED_ACTIONS and the three CAN_USE_SECURE_TOKEN_ACTIONS, so ACCOUNT_VIEW and ACCOUNT_CREATE among them — with or without a password, while the web created the same tokens without difficulty. Sealing a vault needs the master password on the context, and the API only loads that from the calling token's own vault, which AUTHTOKEN_CREATE did not have. Fixing it is a decision rather than a repair, which is why it was recorded first: AUTHTOKEN_CREATE and AUTHTOKEN_EDIT are now themselves on CAN_USE_SECURE_TOKEN_ACTIONS, so a token that can mint tokens also carries the master password. That is the authority the web already grants, since an administrator who can reach the tokens page has unlocked the vault with their own password — but on the API it is a bearer credential in somebody's script, so it is worth as much as the vault. Two things follow, both enforced in AuthTokenBase::prepareSecureToken(). The password on such a token is required rather than optional, because without it the vault is sealed with the empty string and nothing can open it: tokenPass is a required parameter and required refuses the empty string. And creating one needs a calling token that already carries a vault, so an AUTHTOKEN_CREATE token minted before this change gets a 401 until it is re-issued; the web can always issue the first one. The tests round-trip rather than only mint: an ACCOUNT_VIEW token created through the API is then used to read an account with customFields, which is the path that needs the vault. All eight vault-carrying actions are covered, a missing password is refused, and an action carrying no vault still needs none. AuthTokenHelp is unchanged: password stays optional there because it is conditional on the action, and ApiHelpMatchesControllersTest pins the declared flag against what the controller reads. --- CLAUDE.md | 47 ++++---- src/Application/Auth/Services/AuthToken.php | 12 ++ .../Controllers/AuthToken/AuthTokenBase.php | 35 ++++++ .../AuthToken/CreateController.php | 9 +- .../Controllers/AuthToken/EditController.php | 9 +- .../Controllers/AuthTokenRoundTripTest.php | 112 ++++++++++++++++++ 6 files changed, 200 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 27b703f20..c398c7b1b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -632,26 +632,33 @@ against the code as it stood, not reasoned about: where a claim needed a fact, t asserts each is *still* unrouted and *still* never handed out — so the list cannot silently excuse a new one. Everything not on it must resolve **and** satisfy the dispatch contract. -## Known gap — the API cannot create a token that carries a vault - -`POST /api/v1/auth-tokens` answers **500 "Error while retrieving master password from context"** for -every action a token carries a vault for — the five `SECURED_ACTIONS` and the three -`CAN_USE_SECURE_TOKEN_ACTIONS`, so `ACCOUNT_VIEW` and `ACCOUNT_CREATE` among them. With or without a -`password`. The help documents `actionId` with no restriction, and the web creates these tokens -fine, so this is an oversight rather than a policy. - -The cause is not in the controller. `AuthToken::injectSecureData()` needs the master password on the -context to seal the vault; the API only ever loads it from the *calling* token's own vault -(`Api::requireMasterPass()`), and `AUTHTOKEN_CREATE` is on neither list, so it has no vault to load -it from. Making this work therefore means putting `AUTHTOKEN_CREATE` on -`CAN_USE_SECURE_TOKEN_ACTIONS` — which is a decision, not a fix: it makes a token that can mint -tokens also a token that carries the master password. **That is why it is recorded here rather than -patched.** The alternative is to answer a clear refusal instead of a 500, which is smaller but -encodes "permanently unavailable" for `ACCOUNT_VIEW` tokens, and that looks wrong. - -Do not "fix" it by having `injectSecureData()` fall back to an empty key: that seals the vault with -the empty string, and `Api::getMasterPassFromVault()` requires a non-empty `tokenPass`, so the token -can never be opened. That exact failure is what the web form's password rule now prevents. +## A token that carries a vault, and what that costs + +Some actions need the master password, so a token issued for one carries a **vault**: the master +password sealed with the token's own password *and* the token itself. `AuthToken::needsSecureToken()` +is the single definition of which actions those are — `SECURED_ACTIONS` plus +`CAN_USE_SECURE_TOKEN_ACTIONS` — and both the web form and the API ask it. + +`POST /api/v1/auth-tokens` used to answer **500 "Error while retrieving master password from +context"** for every one of them, with or without a password, while the web created the same tokens +without difficulty: sealing a vault needs the master password on the context, and the API only ever +loads that from the *calling* token's own vault, which `AUTHTOKEN_CREATE` did not have. + +It works now, and the way it was made to work is a decision worth knowing rather than a detail: +**`AUTHTOKEN_CREATE` and `AUTHTOKEN_EDIT` are themselves on `CAN_USE_SECURE_TOKEN_ACTIONS`, so a +token that can mint tokens also carries the master password.** That is the authority the web already +grants — an administrator who can reach the tokens page has unlocked the vault with their own +password — but on the API it is a bearer credential sitting in somebody's script, so it is worth as +much as the vault. Two things follow, and both are enforced in +`AuthTokenBase::prepareSecureToken()`: + +- the password on such a token is **required**, not optional. Without it the vault is sealed with + the empty string and 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. Do not "fix" that by letting the empty key through; +- creating one requires a calling token that already carries a vault, so an `AUTHTOKEN_CREATE` + token minted before this existed has none and gets a 401 until it is re-issued. The web can + always issue the first one. ## Escaping: on output, never on input diff --git a/src/Application/Auth/Services/AuthToken.php b/src/Application/Auth/Services/AuthToken.php index ef89ca736..d90c1a541 100644 --- a/src/Application/Auth/Services/AuthToken.php +++ b/src/Application/Auth/Services/AuthToken.php @@ -76,6 +76,18 @@ final class AuthToken extends Service implements AuthTokenService AclActionsInterface::ACCOUNT_VIEW, AclActionsInterface::CATEGORY_VIEW, AclActionsInterface::CLIENT_VIEW, + // Sealing the master password into a new token needs the master password, and the API can + // only get it out of the calling token's own vault. Without these two, every action a + // token carries a vault for — ACCOUNT_VIEW and ACCOUNT_CREATE among them — could be + // created from the web and not from the API, which answered 500 for all of them. + // + // It follows that a token which can mint tokens also carries the master password. That is + // the same authority the web grants: an administrator who can reach the tokens page has + // already unlocked the vault with their own password. It is not free, though — such a + // token is worth as much as the vault, which is why the password protecting it is required + // rather than optional (`AuthTokenBase::prepareSecureToken()`). + AclActionsInterface::AUTHTOKEN_CREATE, + AclActionsInterface::AUTHTOKEN_EDIT, ]; /** diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/AuthTokenBase.php b/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/AuthTokenBase.php index 4e161155a..ae3cdff53 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/AuthTokenBase.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/AuthTokenBase.php @@ -8,9 +8,14 @@ use SP\Application\Auth\Ports\AuthTokenService; use SP\Domain\Auth\Models\AuthToken as AuthTokenModel; use SP\Domain\Core\Acl\AclInterface; +use SP\Domain\Core\Exceptions\ValidationException; +use SP\Domain\Common\Services\ServiceException; +use SP\Application\Auth\Services\AuthToken; use SP\Infrastructure\Adapter\In\Api\Controllers\ControllerBase; use SP\Infrastructure\Adapter\In\Api\Controllers\Help\AuthTokenHelp; +use function SP\__u; + abstract class AuthTokenBase extends ControllerBase { /** @@ -45,4 +50,34 @@ protected static function withoutSecrets(AuthTokenModel $authToken): array { return $authToken->toArray(null, AuthTokenModel::SECRET_COLS, true); } + + /** + * What a token that carries a vault needs before it can be written. + * + * A vault is the master password sealed with the token's own password and the token itself, so + * two things have to be true and neither was checked here. The password cannot be blank, or the + * vault is sealed with the empty string and nothing can open it again — `Api` reads `tokenPass` + * as a required parameter, and required refuses the empty string, so the one password that + * would work cannot be presented. And the master password has to be on the context to seal it + * there at all, which on the API means loading it from the calling token's own vault. + * + * Without the second, this endpoint answered 500 "Error while retrieving master password from + * context" for every action a token carries a vault for, with or without a password, while the + * web created the same tokens without difficulty. + * + * @throws ServiceException + * @throws ValidationException + */ + final protected function prepareSecureToken(int $actionId, ?string $password): void + { + if (!AuthToken::needsSecureToken($actionId)) { + return; + } + + if (empty($password)) { + throw ValidationException::error(__u('Password cannot be blank')); + } + + $this->apiService->requireMasterPass(); + } } diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/CreateController.php b/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/CreateController.php index 84d1f046e..5da2cbe5d 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/CreateController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/CreateController.php @@ -17,10 +17,15 @@ public function createAction(): ApiResponse { $this->setupApi(AclActionsInterface::AUTHTOKEN_CREATE); + $actionId = $this->apiService->getParamInt('actionId', true); + $password = $this->apiService->getParamRaw('password'); + + $this->prepareSecureToken($actionId, $password); + $tokenData = new AuthToken([ 'userId' => $this->apiService->getParamInt('userId', true), - 'actionId' => $this->apiService->getParamInt('actionId', true), - 'hash' => $this->apiService->getParamRaw('password'), + 'actionId' => $actionId, + 'hash' => $password, ]); $id = $this->authTokenService->create($tokenData); diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/EditController.php b/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/EditController.php index 360ffb958..9b6eb080f 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/EditController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/AuthToken/EditController.php @@ -17,11 +17,16 @@ public function editAction(): ApiResponse { $this->setupApi(AclActionsInterface::AUTHTOKEN_EDIT); + $actionId = $this->apiService->getParamInt('actionId', true); + $password = $this->apiService->getParamRaw('password'); + + $this->prepareSecureToken($actionId, $password); + $tokenData = new AuthToken([ 'id' => $this->apiService->getParamInt('id', true), 'userId' => $this->apiService->getParamInt('userId', true), - 'actionId' => $this->apiService->getParamInt('actionId', true), - 'hash' => $this->apiService->getParamRaw('password'), + 'actionId' => $actionId, + 'hash' => $password, ]); $this->authTokenService->update($tokenData); diff --git a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AuthTokenRoundTripTest.php b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AuthTokenRoundTripTest.php index 37566b59d..c2ccb6f24 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AuthTokenRoundTripTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AuthTokenRoundTripTest.php @@ -26,6 +26,8 @@ namespace SP\Tests\Integration\Infrastructure\Adapter\In\Api\Controllers; use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Test; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Tests\Integration\Infrastructure\Adapter\In\Api\ApiTestCase; use stdClass; @@ -89,6 +91,116 @@ public function testATokenThatWasNeverIssuedIsRefused(): void $this->assertObjectHasProperty('error', $r->body); } + /** + * A token for an action that carries a vault can be created through the API, and works. + * + * It could not be. `AuthToken::injectSecureData()` seals the master password into the new + * token, which needs the master password on the context, and the API only ever loads that from + * the *calling* token's own vault — which `AUTHTOKEN_CREATE` did not have. So every action a + * token carries a vault for, `ACCOUNT_VIEW` and `ACCOUNT_CREATE` among them, answered 500 + * "Error while retrieving master password from context", with or without a password, while the + * web created the same tokens without difficulty. + * + * The round trip is the point: minting it is only half of it, so the token it answers with is + * then used to read an account, custom fields and all, which is the path that needs the vault. + * + * @throws \Exception + */ + #[Test] + public function aTokenForAnActionThatCarriesAVaultCanBeCreatedAndUsed(): void + { + $token = $this->createTokenFor(AclActionsInterface::ACCOUNT_VIEW); + + $r = $this->callApiPath( + 'GET', + '/api/v1/accounts/1', + ['customFields' => '1', 'tokenPass' => self::AUTH_TOKEN_PASS], + 'Bearer ' . $token + ); + + $this->assertSame(200, $r->status, 'the token has to open the account it was issued for'); + $this->assertNotEmpty($r->body->data); + } + + /** + * Every one of them, so this is not a fix for the one action that happened to be tried. + * + * @throws \Exception + */ + #[Test] + #[DataProvider('vaultCarryingActionProvider')] + public function everyActionThatCarriesAVaultCanBeMinted(int $actionId): void + { + $created = $this->callApi( + AclActionsInterface::AUTHTOKEN_CREATE, + [ + 'userId' => self::ADMIN_USER_ID, + 'actionId' => $actionId, + 'password' => self::AUTH_TOKEN_PASS, + ] + ); + + $this->assertSame(201, $created->status, json_encode($created->body)); + $this->assertNotEmpty($created->body->data->token); + } + + /** + * @return array + */ + public static function vaultCarryingActionProvider(): array + { + return [ + 'ACCOUNT_VIEW_PASS' => [AclActionsInterface::ACCOUNT_VIEW_PASS], + 'ACCOUNT_EDIT_PASS' => [AclActionsInterface::ACCOUNT_EDIT_PASS], + 'ACCOUNT_CREATE' => [AclActionsInterface::ACCOUNT_CREATE], + 'PUBLICLINK_CREATE' => [AclActionsInterface::PUBLICLINK_CREATE], + 'PUBLICLINK_REFRESH' => [AclActionsInterface::PUBLICLINK_REFRESH], + 'ACCOUNT_VIEW' => [AclActionsInterface::ACCOUNT_VIEW], + 'CATEGORY_VIEW' => [AclActionsInterface::CATEGORY_VIEW], + 'CLIENT_VIEW' => [AclActionsInterface::CLIENT_VIEW], + ]; + } + + /** + * Without a password the vault would be sealed with the empty string, and nothing could open + * it again: `Api` reads `tokenPass` as a required parameter, and required refuses the empty + * string, so the one password that would work cannot be presented. The web form has refused + * this since #833; the API refuses it now too, rather than issuing a token that can never be + * used. + * + * @throws \Exception + */ + #[Test] + public function aTokenThatCarriesAVaultIsRefusedWithoutAPassword(): void + { + $created = $this->callApi( + AclActionsInterface::AUTHTOKEN_CREATE, + ['userId' => self::ADMIN_USER_ID, 'actionId' => AclActionsInterface::ACCOUNT_VIEW] + ); + + $this->assertInstanceOf(stdClass::class, $created->body->error ?? null); + $this->assertSame('Password cannot be blank', $created->body->error->message); + } + + /** + * The control: an action that carries no vault still needs no password, so the rule above did + * not become "every token needs one". + * + * @throws \Exception + */ + #[Test] + public function aTokenThatCarriesNoVaultStillNeedsNoPassword(): void + { + $created = $this->callApi( + AclActionsInterface::AUTHTOKEN_CREATE, + ['userId' => self::ADMIN_USER_ID, 'actionId' => AclActionsInterface::CATEGORY_SEARCH] + ); + + $this->assertSame(201, $created->status, json_encode($created->body)); + $this->assertNotEmpty($created->body->data->token); + } + + /** * Create a token through the API and hand back the bearer string it answered with. *