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. *