diff --git a/API.php b/API.php index 98de732..95fffb8 100644 --- a/API.php +++ b/API.php @@ -79,6 +79,7 @@ public function mcp(): ResponseInterface } try { + $this->checkUserIsNotAnonymous(); $this->checkUserHasSomeViewAccess(); } catch (\Throwable $e) { if ($this->isUnauthorizedLike($e)) { @@ -126,6 +127,11 @@ protected function checkUserHasSomeViewAccess(): void Piwik::checkUserHasSomeViewAccess(); } + protected function checkUserIsNotAnonymous(): void + { + Piwik::checkUserIsNotAnonymous(); + } + protected function createUnauthorizedResponse(string|int $requestId = ''): ResponseInterface { return $this->jsonRpcErrorResponseFactory->create( diff --git a/CHANGELOG.md b/CHANGELOG.md index f2103a5..a8de556 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### Unreleased +- Disabled anonymous access to the MCP API endpoint and connect guidance page. - Added support for exposing subtable reports through the MCP report tools. - Aligned `matomo_report_processed` so empty `resolvedReport.apiParameters` values serialize as `{}` rather than `[]`, matching the declared MCP output schema. - Updated `matomo_report_processed` and `matomo_report_metadata` to accept `apiParameters: []` as the empty-input compatibility form. diff --git a/Controller.php b/Controller.php index 1a48cae..0accdba 100644 --- a/Controller.php +++ b/Controller.php @@ -25,6 +25,7 @@ public function __construct(private SystemSettings $systemSettings) public function connect(): string { + Piwik::checkUserIsNotAnonymous(); Piwik::checkUserHasSomeViewAccess(); $view = new View('@McpServer/connect'); diff --git a/Menu.php b/Menu.php index c62201e..e7d407d 100644 --- a/Menu.php +++ b/Menu.php @@ -18,7 +18,7 @@ class Menu extends \Piwik\Plugin\Menu { public function configureAdminMenu(MenuAdmin $menu): void { - if (Piwik::isUserHasSomeViewAccess()) { + if (!Piwik::isUserIsAnonymous() && Piwik::isUserHasSomeViewAccess()) { $menu->addPlatformItem('McpServer_PlatformMenu', $this->urlForAction('connect'), 42); } } diff --git a/tests/Integration/ControllerTest.php b/tests/Integration/ControllerTest.php new file mode 100644 index 0000000..ebb4979 --- /dev/null +++ b/tests/Integration/ControllerTest.php @@ -0,0 +1,111 @@ +idSite = Fixture::createWebsite( + '2010-01-01 00:00:00', + 0, + 'MCP Controller Test Site', + 'https://mcp-controller.test', + ); + } + + public function tearDown(): void + { + $this->restoreAnonymousAccessForSite($this->idSite); + Access::getInstance()->setSuperUserAccess(false); + + parent::tearDown(); + } + + public function testConnectRejectsAnonymousWithViewAccess(): void + { + $this->setAnonymousAccessForSite($this->idSite, 'view'); + $originalTokenAuth = McpAuthTestHelper::captureCurrentTokenAuth(); + + try { + McpAuthTestHelper::switchToAnonymous(); + + $this->expectException(NoAccessException::class); + + (new Controller(StaticContainer::get(SystemSettings::class)))->connect(); + } finally { + McpAuthTestHelper::restoreAuth($originalTokenAuth); + } + } + + private function setAnonymousAccessForSite(int $idSite, string $access): void + { + $model = new UsersManagerModel(); + if (!$model->userExists('anonymous')) { + $model->addUser('anonymous', 'not_a_hash', 'anonymous@example.com', Date::now()->getDatetime()); + $this->createdAnonymousUser = true; + } + + if ($this->anonymousAccessBackup === null) { + $usersAccess = $model->getUsersAccessFromSite($idSite); + $this->anonymousAccessBackup = $usersAccess['anonymous'] ?? 'noaccess'; + } + + $model->deleteUserAccess('anonymous', [$idSite]); + if ($access !== 'noaccess') { + $model->addUserAccess('anonymous', $access, [$idSite]); + } + } + + private function restoreAnonymousAccessForSite(int $idSite): void + { + if ($this->anonymousAccessBackup === null) { + return; + } + + $model = new UsersManagerModel(); + $model->deleteUserAccess('anonymous', [$idSite]); + if ($this->anonymousAccessBackup !== 'noaccess') { + $model->addUserAccess('anonymous', $this->anonymousAccessBackup, [$idSite]); + } + + if ($this->createdAnonymousUser) { + $model->deleteUser('anonymous'); + $this->createdAnonymousUser = false; + } + + $this->anonymousAccessBackup = null; + } +} diff --git a/tests/Integration/McpApiEndpointBoundaryTest.php b/tests/Integration/McpApiEndpointBoundaryTest.php index becfeb0..a1c687c 100644 --- a/tests/Integration/McpApiEndpointBoundaryTest.php +++ b/tests/Integration/McpApiEndpointBoundaryTest.php @@ -17,6 +17,7 @@ use Piwik\Access; use Piwik\API\Request as ApiRequest; use Piwik\Container\StaticContainer; +use Piwik\Date; use Piwik\FrontController; use Piwik\Plugins\McpServer\API; use Piwik\Plugins\McpServer\McpServerFactory; @@ -28,6 +29,7 @@ use Piwik\Plugins\McpServer\SystemSettings; use Piwik\Plugins\McpServer\tests\Framework\McpAuthTestHelper; use Piwik\Plugins\McpServer\tests\Framework\McpTestHelper; +use Piwik\Plugins\UsersManager\Model as UsersManagerModel; use Piwik\Tests\Framework\Fixture; use Piwik\Tests\Framework\TestCase\IntegrationTestCase; @@ -48,6 +50,10 @@ class McpApiEndpointBoundaryTest extends IntegrationTestCase private string $originalMaximumAllowedMcpAccessLevel = McpAccessLevel::UNLIMITED; + private ?string $anonymousAccessBackup = null; + + private bool $createdAnonymousUser = false; + private int $idSite = 0; private int $idSiteOther = 0; @@ -75,6 +81,7 @@ public function tearDown(): void $_GET = $this->originalGet; $this->setMcpEnabled($this->originalEnableMcpValue); $this->setMaximumAllowedMcpAccessLevel($this->originalMaximumAllowedMcpAccessLevel); + $this->restoreAnonymousAccessForSite($this->idSite); $this->setNestedApiInvocationCount($this->originalNestedApiInvocationCount); ApiRequest::setIsRootRequestApiRequest($this->originalRootApiMethod); Access::getInstance()->setSuperUserAccess(false); @@ -223,6 +230,35 @@ public function testDisabledMcpAndUnauthenticatedReturnsUnauthorizedChallenge(): self::assertSame('disabled-auth-1', $error->id); } + public function testAnonymousWithViewAccessIsRejectedBeforeMcpEnabledCheck(): void + { + $this->setMcpEnabled(true); + $this->setAnonymousAccessForSite($this->idSite, 'view'); + $originalTokenAuth = McpAuthTestHelper::captureCurrentTokenAuth(); + + $_GET['module'] = 'API'; + $_GET['method'] = 'McpServer.mcp'; + $_GET['format'] = 'mcp'; + + try { + McpAuthTestHelper::switchToAnonymous(); + + $api = $this->createApiWithRequest( + $this->createRequest(McpTestHelper::makeInitializeRequest('anonymous-view-1')), + ); + $response = $api->mcp(); + $error = McpTestHelper::decodeError($response); + + self::assertSame(401, $response->getStatusCode()); + self::assertSame('Bearer realm="mcp"', $response->getHeaderLine('WWW-Authenticate')); + self::assertSame(JsonRpcError::INVALID_REQUEST, $error->code); + self::assertSame(McpEndpointSpec::UNAUTHORIZED_ERROR, $error->message); + self::assertSame('anonymous-view-1', $error->id); + } finally { + McpAuthTestHelper::restoreAuth($originalTokenAuth); + } + } + public function testDisabledMcpReturnsForbiddenWithEmptyBodyWhenTopLevelIdMissing(): void { $this->setMcpEnabled(false); @@ -461,4 +497,43 @@ private function setMaximumAllowedMcpAccessLevel(string $maximumAllowedMcpAccess Access::getInstance()->setSuperUserAccess($hadSuperUserAccess); } } + + private function setAnonymousAccessForSite(int $idSite, string $access): void + { + $model = new UsersManagerModel(); + if (!$model->userExists('anonymous')) { + $model->addUser('anonymous', 'not_a_hash', 'anonymous@example.com', Date::now()->getDatetime()); + $this->createdAnonymousUser = true; + } + + if ($this->anonymousAccessBackup === null) { + $usersAccess = $model->getUsersAccessFromSite($idSite); + $this->anonymousAccessBackup = $usersAccess['anonymous'] ?? 'noaccess'; + } + + $model->deleteUserAccess('anonymous', [$idSite]); + if ($access !== 'noaccess') { + $model->addUserAccess('anonymous', $access, [$idSite]); + } + } + + private function restoreAnonymousAccessForSite(int $idSite): void + { + if ($this->anonymousAccessBackup === null) { + return; + } + + $model = new UsersManagerModel(); + $model->deleteUserAccess('anonymous', [$idSite]); + if ($this->anonymousAccessBackup !== 'noaccess') { + $model->addUserAccess('anonymous', $this->anonymousAccessBackup, [$idSite]); + } + + if ($this->createdAnonymousUser) { + $model->deleteUser('anonymous'); + $this->createdAnonymousUser = false; + } + + $this->anonymousAccessBackup = null; + } } diff --git a/tests/Unit/APITest.php b/tests/Unit/APITest.php index 2e5de3d..505ef3f 100644 --- a/tests/Unit/APITest.php +++ b/tests/Unit/APITest.php @@ -165,6 +165,7 @@ public function testMcpReturnsUnauthorizedChallengeWhenNoViewAccess(): void 'createRequestFromGlobals', 'isCurrentApiRequestRoot', 'getRootApiRequestMethod', + 'checkUserIsNotAnonymous', 'checkUserHasSomeViewAccess', ]) ->getMock(); @@ -211,6 +212,7 @@ public function testMcpReturnsUnauthorizedChallengeWhenNoAccessExceptionIsWrappe 'createRequestFromGlobals', 'isCurrentApiRequestRoot', 'getRootApiRequestMethod', + 'checkUserIsNotAnonymous', 'checkUserHasSomeViewAccess', ]) ->getMock(); @@ -236,6 +238,52 @@ public function testMcpReturnsUnauthorizedChallengeWhenNoAccessExceptionIsWrappe self::assertSame('init-1', $message->id); } + public function testMcpReturnsUnauthorizedChallengeWhenUserIsAnonymous(): void + { + $_GET['module'] = 'API'; + $_GET['method'] = 'McpServer.mcp'; + $_GET['format'] = 'mcp'; + + $factory = $this->createFactory(); + + $api = $this + ->getMockBuilder(API::class) + ->setConstructorArgs([ + $factory, + new McpEndpointGuard(), + new JsonRpcErrorResponseFactory(), + new JsonRpcRequestIdExtractor(), + $this->createMock(SystemSettings::class), + ]) + ->onlyMethods([ + 'createRequestFromGlobals', + 'isCurrentApiRequestRoot', + 'getRootApiRequestMethod', + 'checkUserIsNotAnonymous', + 'checkUserHasSomeViewAccess', + ]) + ->getMock(); + + $api->method('createRequestFromGlobals') + ->willReturn($this->createRequest()); + $api->method('isCurrentApiRequestRoot') + ->willReturn(true); + $api->method('getRootApiRequestMethod') + ->willReturn('McpServer.mcp'); + $api->method('checkUserIsNotAnonymous') + ->willThrowException(new \Piwik\NoAccessException('You must be logged in.')); + $api->expects(self::never())->method('checkUserHasSomeViewAccess'); + + $result = $api->mcp(); + + self::assertSame(401, $result->getStatusCode()); + self::assertSame('Bearer realm="mcp"', $result->getHeaderLine('WWW-Authenticate')); + $message = McpTestHelper::decodeError($result); + self::assertSame(JsonRpcError::INVALID_REQUEST, $message->code); + self::assertSame('Authentication required.', $message->message); + self::assertSame('init-1', $message->id); + } + public function testMcpReturnsForbiddenErrorWhenMcpIsDisabledAndTopLevelIdExists(): void { Access::getInstance()->setSuperUserAccess(true);