diff --git a/.env b/.env index 87985b5f..ebdecfb0 100644 --- a/.env +++ b/.env @@ -85,8 +85,9 @@ APP_CMS_API_BASE_URL="https://www.ep-reisen.de/" # TYPO3 API key (X-Api-Key), sent by the typo3.client scoped client and by app:content:sync; # the real value per stage goes in .env.local APP_CMS_API_KEY= -# Static bearer token n8n sends to the chatbot MCP endpoint (/mcp); empty refuses every request -APP_MCP_TOKEN= +# Bearer tokens for the chatbot MCP endpoint (/mcp), one per client: "name:token,name:token" +# (e.g. "n8n:…,claude-desktop:…"); empty refuses every request +APP_MCP_TOKENS= APP_WEBSITE_BASE_URL="https://www.ep-reisen.de/" APP_BASE_URL="https://my.ep-reisen.de" APP_CUSTOMER_SERVICE_EMAIL="kundenservice@ep-reisen.de" diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 5130987c..f40708a9 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -17,7 +17,7 @@ framework: log: true rate_limiter: - # Per MCP token. A chatbot turn makes a few tool calls; this catches runaway loops. + # Per MCP client. A chatbot turn makes a few tool calls; this catches runaway loops. chatbot_mcp: policy: sliding_window limit: 60 diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 1a5e968c..7f3fd61d 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -15,11 +15,11 @@ security: mailjet: password: '%env(MAILJET_WEBHOOK_BASIC_PASSWORD)%' roles: [ ROLE_MAILJET_WEBHOOK ] + # Never consulted: McpAccessTokenHandler builds the user from the client name its + # token belongs to. Named only because a firewall must have a provider. mcp_provider: memory: - users: - n8n: - roles: [ ROLE_CHATBOT_MCP ] + users: [] role_hierarchy: # ROLE_CUSTOMER_EXPERT gates the booking-draft and log surfaces, which used to be diff --git a/config/services.yaml b/config/services.yaml index 4a0a3877..2186f6db 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -340,7 +340,7 @@ services: App\Security\McpAccessTokenHandler: arguments: - $token: '%env(APP_MCP_TOKEN)%' + $tokens: '%env(APP_MCP_TOKENS)%' App\Service\CmsDataProvider: arguments: diff --git a/docs/chatbot-mcp.md b/docs/chatbot-mcp.md index 5c0416de..9c99ef48 100644 --- a/docs/chatbot-mcp.md +++ b/docs/chatbot-mcp.md @@ -15,19 +15,24 @@ TYPO3 sends no dates or prices, and nothing flows back to TYPO3. `POST /mcp` (Streamable HTTP; `GET`/`DELETE` are part of the transport), served by `symfony/mcp-bundle` from `config/packages/mcp.yaml`. Tools only, no resources or prompts. -Authentication is a static bearer token, `Authorization: Bearer `, checked by -`App\Security\McpAccessTokenHandler` on the `mcp` firewall. If `APP_MCP_TOKEN` is empty, every request is -refused. It is not the OAuth2 client-credentials flow of `/api`: n8n's MCP Client Tool node sends a fixed +Authentication is a static bearer token per client, `Authorization: Bearer `, checked by +`App\Security\McpAccessTokenHandler` on the `mcp` firewall. `APP_MCP_TOKENS` lists the clients as +`name:token` pairs separated by commas, e.g. `APP_MCP_TOKENS="n8n:3f9c…,claude-desktop:a71e…"`. Give every +system its own token, so one can be revoked by removing its entry without touching the others. Use tokens +without commas, e.g. `openssl rand -hex 32`. A malformed list (no name, a duplicate name, a token shared by +two clients) fails loudly with a 500 on `/mcp` rather than locking a client out silently. If the list is +empty, every request is refused. It is not the OAuth2 client-credentials flow of `/api`: n8n's MCP Client Tool node sends a fixed header and cannot renew a token every 10 minutes. -Tool calls are rate-limited per token (`chatbot_mcp` limiter, 60 per minute). The limit is there to stop +The client name becomes the authenticated user. Tool calls are rate-limited per client (`chatbot_mcp` +limiter, 60 per minute), so testing from Claude Desktop does not eat into n8n's budget. The limit is there to stop runaway agent loops. Each call is logged on the `chatbot` channel (`var/log/.chatbot.log`, file -only) with the tool, arguments, result count and duration. Nothing identifies the person chatting. +only) with the client, tool, arguments, result count and duration. Nothing identifies the person chatting. ### n8n MCP Client Tool node: endpoint `https:///mcp`, transport *HTTP Streamable*, authentication *Bearer* -with the stage's `APP_MCP_TOKEN`. The agent prompt should tell the bot to cite each answer's `url`, and +with n8n's own token from the stage's `APP_MCP_TOKENS`. The agent prompt should tell the bot to cite each answer's `url`, and that prices and availability are a snapshot (`dataAsOf`) and only the booking page is binding. ## Tools @@ -109,4 +114,4 @@ After a complete run that changed something, the codes the trip join cannot plac |---|---| | `APP_CMS_API_BASE_URL` | TYPO3 base URL per stage (also used by `CmsDataProvider`) | | `APP_CMS_API_KEY` | TYPO3 `X-Api-Key`: the content feed and its `contentUrl` pages, and `typo3.client` (`CmsDataProvider`) | -| `APP_MCP_TOKEN` | Bearer token n8n sends to `/mcp` | +| `APP_MCP_TOKENS` | Bearer tokens for `/mcp`, one per client: `name:token,name:token` | diff --git a/src/Chatbot/Mcp/ToolCallRunner.php b/src/Chatbot/Mcp/ToolCallRunner.php index d27f9b16..cc0bc3d5 100644 --- a/src/Chatbot/Mcp/ToolCallRunner.php +++ b/src/Chatbot/Mcp/ToolCallRunner.php @@ -12,12 +12,13 @@ use Symfony\Bundle\SecurityBundle\Security; use Symfony\Component\RateLimiter\RateLimiterFactory; /** - * What every chatbot tool call goes through: the per-token rate limit, the call log, and + * What every chatbot tool call goes through: the per-client rate limit, the call log, and * the answer as compact JSON. * * The SDK would pretty-print an array result and repeat it as structured content; both * cost the model tokens and neither helps n8n. The log records tool, arguments, result - * count and duration, never anything about the person chatting, which n8n does not send. + * count, duration and the calling client (n8n, claude-desktop, …), never anything about + * the person chatting, which n8n does not send. */ class ToolCallRunner { @@ -37,10 +38,11 @@ class ToolCallRunner public function run(string $tool, array $arguments, callable $call): CallToolResult { $arguments = array_filter($arguments, static fn (mixed $value): bool => null !== $value); - $limit = $this->chatbotMcpLimiter->create($this->security->getUser()?->getUserIdentifier() ?? 'anonymous')->consume(); + $client = $this->security->getUser()?->getUserIdentifier() ?? 'anonymous'; + $limit = $this->chatbotMcpLimiter->create($client)->consume(); if (!$limit->isAccepted()) { - $this->chatbotLogger->warning('MCP tool call rate-limited', ['tool' => $tool, 'arguments' => $arguments]); + $this->chatbotLogger->warning('MCP tool call rate-limited', ['client' => $client, 'tool' => $tool, 'arguments' => $arguments]); throw new ToolCallException(sprintf('Too many tool calls. Retry after %s.', $limit->getRetryAfter()->format('H:i:s'))); } @@ -50,16 +52,17 @@ class ToolCallRunner try { $result = $call(); } catch (ToolCallException $e) { - $this->chatbotLogger->info('MCP tool call rejected', ['tool' => $tool, 'arguments' => $arguments, 'error' => $e->getMessage()]); + $this->chatbotLogger->info('MCP tool call rejected', ['client' => $client, 'tool' => $tool, 'arguments' => $arguments, 'error' => $e->getMessage()]); throw $e; } catch (\Throwable $e) { - $this->chatbotLogger->error('MCP tool call failed', ['tool' => $tool, 'arguments' => $arguments, 'exception' => $e]); + $this->chatbotLogger->error('MCP tool call failed', ['client' => $client, 'tool' => $tool, 'arguments' => $arguments, 'exception' => $e]); throw $e; } $this->chatbotLogger->info('MCP tool call', [ + 'client' => $client, 'tool' => $tool, 'arguments' => $arguments, 'resultCount' => is_array($result['results'] ?? null) ? count($result['results']) : 1, diff --git a/src/Security/McpAccessTokenHandler.php b/src/Security/McpAccessTokenHandler.php index 24e3c588..ef4ebdfb 100644 --- a/src/Security/McpAccessTokenHandler.php +++ b/src/Security/McpAccessTokenHandler.php @@ -5,33 +5,86 @@ declare(strict_types=1); namespace App\Security; use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; /** - * Checks the static bearer token n8n sends to the chatbot MCP endpoint. + * Checks the static bearer tokens of the systems calling the chatbot MCP endpoint. * * The data behind it is public, but the endpoint must not become a free LLM-tooling API - * for whoever finds it. A static token rather than the OAuth2 client-credentials grant of + * for whoever finds it. Static tokens rather than the OAuth2 client-credentials grant of * /api: n8n's MCP Client Tool node sends a fixed header and cannot refresh a 10-minute token. - * An unconfigured token refuses every request. + * + * Every client has its own token, configured as `name:token` pairs separated by commas + * (APP_MCP_TOKENS="n8n:3f9c…,claude-desktop:a71e…"), so one can be revoked without the + * others. The name becomes the authenticated user: each client gets its own rate-limit + * bucket and is named in the tool-call log. An empty list refuses every request. */ final class McpAccessTokenHandler implements AccessTokenHandlerInterface { - public const USER = 'n8n'; + public const ROLE = 'ROLE_CHATBOT_MCP'; + /** @var array token => client name */ + private readonly array $clients; + + /** + * @throws \InvalidArgumentException for a malformed list, so a typo fails loudly instead of locking a client out + */ public function __construct( #[\SensitiveParameter] - private readonly string $token, + string $tokens, ) { + $this->clients = self::parse($tokens); } public function getUserBadgeFrom(#[\SensitiveParameter] string $accessToken): UserBadge { - if ('' === $this->token || !hash_equals($this->token, $accessToken)) { + $client = null; + + // Every token is compared, so the time taken does not tell which one came close. + foreach ($this->clients as $token => $name) { + if (hash_equals($token, $accessToken)) { + $client = $name; + } + } + + if (null === $client) { throw new BadCredentialsException('Invalid MCP token.'); } - return new UserBadge(self::USER); + return new UserBadge($client, static fn (string $name): InMemoryUser => new InMemoryUser($name, null, [self::ROLE])); + } + + /** + * @return array token => client name + */ + private static function parse(string $tokens): array + { + $clients = []; + + foreach (explode(',', $tokens) as $entry) { + if ('' === trim($entry)) { + continue; + } + + [$name, $token] = array_map('trim', explode(':', $entry, 2)) + [1 => '']; + + if (1 !== preg_match('/^[a-z0-9_-]+$/i', $name) || '' === $token) { + throw new \InvalidArgumentException('APP_MCP_TOKENS entries must look like "name:token", with a name of letters, digits, "-" and "_".'); + } + + if (in_array($name, $clients, true)) { + throw new \InvalidArgumentException(sprintf('APP_MCP_TOKENS names client "%s" twice.', $name)); + } + + if (isset($clients[$token])) { + throw new \InvalidArgumentException(sprintf('APP_MCP_TOKENS gives clients "%s" and "%s" the same token.', $clients[$token], $name)); + } + + $clients[$token] = $name; + } + + return $clients; } } diff --git a/tests/Security/McpAccessTokenHandlerTest.php b/tests/Security/McpAccessTokenHandlerTest.php index 1f35b6f5..d80cbefb 100644 --- a/tests/Security/McpAccessTokenHandlerTest.php +++ b/tests/Security/McpAccessTokenHandlerTest.php @@ -5,27 +5,66 @@ declare(strict_types=1); namespace App\Tests\Security; use App\Security\McpAccessTokenHandler; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Exception\BadCredentialsException; class McpAccessTokenHandlerTest extends TestCase { - public function testTheConfiguredTokenAuthenticatesTheN8nUser(): void + public function testEachTokenAuthenticatesItsOwnClient(): void { - self::assertSame(McpAccessTokenHandler::USER, (new McpAccessTokenHandler('s3cret'))->getUserBadgeFrom('s3cret')->getUserIdentifier()); + $handler = new McpAccessTokenHandler('n8n:token-a, claude-desktop:token-b'); + + $badge = $handler->getUserBadgeFrom('token-b'); + + self::assertSame('claude-desktop', $badge->getUserIdentifier()); + self::assertSame(['ROLE_CHATBOT_MCP'], $badge->getUser()->getRoles()); + self::assertSame('n8n', $handler->getUserBadgeFrom('token-a')->getUserIdentifier()); } - public function testAWrongTokenIsRejected(): void + public function testATokenMayContainColons(): void + { + self::assertSame('n8n', (new McpAccessTokenHandler('n8n:a:b:c'))->getUserBadgeFrom('a:b:c')->getUserIdentifier()); + } + + public function testAnUnknownTokenIsRejected(): void { $this->expectException(BadCredentialsException::class); - (new McpAccessTokenHandler('s3cret'))->getUserBadgeFrom('guess'); + (new McpAccessTokenHandler('n8n:token-a'))->getUserBadgeFrom('token-b'); } - public function testAnUnconfiguredTokenRejectsEverything(): void + public function testAClientNameIsNotAToken(): void { $this->expectException(BadCredentialsException::class); - (new McpAccessTokenHandler(''))->getUserBadgeFrom(''); + (new McpAccessTokenHandler('n8n:token-a'))->getUserBadgeFrom('n8n'); + } + + public function testAnEmptyListRejectsEverything(): void + { + $this->expectException(BadCredentialsException::class); + + (new McpAccessTokenHandler(' '))->getUserBadgeFrom(''); + } + + /** + * @return iterable + */ + public static function malformedLists(): iterable + { + yield 'token without name' => ['token-a']; + yield 'empty token' => ['n8n:']; + yield 'invalid name' => ['n 8 n:token-a']; + yield 'duplicate name' => ['n8n:token-a,n8n:token-b']; + yield 'shared token' => ['n8n:token-a,claude-desktop:token-a']; + } + + #[DataProvider('malformedLists')] + public function testAMalformedListFailsLoudly(string $tokens): void + { + $this->expectException(\InvalidArgumentException::class); + + new McpAccessTokenHandler($tokens); } }