diff --git a/Activity/AuthorizeClient.php b/Activity/AuthorizeClient.php index fdce616..bb8a051 100644 --- a/Activity/AuthorizeClient.php +++ b/Activity/AuthorizeClient.php @@ -31,7 +31,9 @@ public function extractParams($eventData) 'version' => 'v1', 'client' => $client, 'userLogin' => $activityData['userLogin'] ?? null, - 'scopes' => array_values((array) ($activityData['scopes'] ?? [])), + // the scopes that were granted, empty when the request was denied + 'scopes' => $decision === 'allowed' ? array_values((array) ($activityData['scopes'] ?? [])) : [], + 'requestedScopes' => array_values((array) ($activityData['requestedScopes'] ?? [])), 'decision' => $decision, ]; } diff --git a/CHANGELOG.md b/CHANGELOG.md index cede501..746db89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ ## Changelog +5.3.0 - 2026-08-03 +- Authorize requests may now carry multiple scopes; the consent screen shows a radio group and the user grants exactly one scope (defaulting to the least privileged). Issued tokens still carry a single scope. +- Scopes offered on the consent screen are limited to those the authorizing user can actually grant. If the user's access level is too low for every requested scope, the error now says so and names the scopes, instead of reporting an invalid client scope mapping. +- Clients with an empty scope list now fall back to the globally allowed scopes during authorization (previously rejected), matching the token endpoint behaviour. +- The scope configured for a client is now applied as the maximum access level a user can grant it, as the setting has always been documented. A client configured with `matomo:admin` can therefore be granted `matomo:write` or `matomo:read` as well, which is what lets the user choose a scope on the consent screen. Clients can never be granted more than their configured scope. The client credentials grant is unchanged and still requires the exact configured scope, as no user is involved to choose a lower one. +- Added code to harden the consent decision handling on the authorize endpoint +- The `OAuth2.authorize.decision.end` event and the authorize activity keep reporting the granted scope in `scopes`, which is now empty when the request was denied, and add `requestedScopes` with everything the client asked for. + 5.2.4 - 2026-07-27 - Added code to warn users if scope is downgraded diff --git a/Controller.php b/Controller.php index 60bc4f7..159e0a1 100644 --- a/Controller.php +++ b/Controller.php @@ -28,6 +28,7 @@ use Piwik\Request; use Piwik\Url; use Matomo\Dependencies\OAuth2\Psr\Http\Message\ResponseInterface; +use Matomo\Dependencies\OAuth2\League\OAuth2\Server\Entities\ClientEntityInterface; use Matomo\Dependencies\OAuth2\League\OAuth2\Server\Exception\OAuthServerException; class Controller extends ControllerAdmin @@ -94,31 +95,78 @@ public function authorize() return $scope->getIdentifier(); }, $authRequest->getScopes()); - $client = $authRequest->getClient(); - $clientScopes = []; - if ($client instanceof ClientEntity) { - $clientScopes = array_values($client->getAllowedScopes()); - } - $scopes = array_values($scopes); + $requestedClientScopes = $this->getScopesAllowedForClient(array_values($scopes), $authRequest->getClient()); - if (count($scopes) !== 1 || count($clientScopes) !== 1 || $clientScopes[0] !== $scopes[0]) { + if (empty($requestedClientScopes)) { return $this->renderUnauthorized(Piwik::translate('OAuth2_InvalidClientScope')); } - $this->checkDoesUserHasAccessAsPerScope($scopes[0]); + $selectableScopes = $this->getScopesUserCanGrant($requestedClientScopes); + + if (empty($selectableScopes)) { + return $this->renderUnauthorized(Piwik::translate( + 'OAuth2_NoAccessForRequestedScopes', + implode(', ', $requestedClientScopes) + )); + } if ($this->isPostRequest()) { - $decision = Request::fromRequest()->getStringParameter('decision', ''); - $nonce = Request::fromRequest()->getStringParameter('nonce', ''); + // the consent form is submitted as POST, and Request::fromRequest() lets a query string + // parameter of the same name win, so the decision must be read from the POST body only + $post = Request::fromPost(); + $decision = $post->getStringParameter('decision', ''); + $nonce = $post->getStringParameter('nonce', ''); if (!Nonce::verifyNonce('Oauth2.authorize', $nonce)) { return $this->renderUnauthorized(Piwik::translate('OAuth2_InvalidAuthorizationRequest')); } + if (!in_array($decision, ['allow', 'deny'], true)) { + return $this->renderUnauthorized(Piwik::translate('OAuth2_InvalidAuthorizationRequest')); + } + + $selectedScope = $post->getStringParameter('selected_scope', ''); + + if (!in_array($selectedScope, $selectableScopes, true)) { + return $this->renderUnauthorized(Piwik::translate('OAuth2_InvalidScopeValue')); + } + + $this->checkDoesUserHasAccessAsPerScope($selectedScope); + + $authRequest->setScopes([$this->scopeRepository->getScopeEntityByIdentifier($selectedScope)]); + $isApproved = $decision === 'allow'; $authRequest->setAuthorizationApproved($isApproved); + + /** + * Triggered after a user allowed or denied an OAuth 2.0 authorization request on the + * consent screen, before the authorization code is issued. + * + * Used by the plugin itself to record the decision in the activity log, and available + * to other plugins that need to audit or react to granted and denied access. + * + * @param array $activityData Details of the decision: + * + * - `version`: the payload version, currently `v1`. + * - `client`: the OAuth client, with `id` and `name`, plus + * `type` and `active` for clients of this plugin. + * - `userLogin`: the login of the user who decided. + * - `scopes`: the scopes that were granted. The user grants + * exactly one, so this holds a single scope, and it is + * empty when the request was denied. + * - `requestedScopes`: everything the client asked for. An + * authorize request may name several scopes even though + * only one of them can be granted. + * - `decision`: either `allowed` or `denied`. + */ Piwik::postEvent('OAuth2.authorize.decision.end', [ - $this->buildAuthorizationActivityData($authRequest->getClient(), $login, $scopes, $isApproved), + $this->buildAuthorizationActivityData( + $authRequest->getClient(), + $login, + array_values($scopes), + $selectedScope, + $isApproved + ), ]); try { @@ -150,7 +198,7 @@ public function authorize() 'clientId' => $client->getIdentifier(), 'userLogin' => $login, 'userEmail' => $user['email'] ?? '', - 'scopes' => $scopes, + 'scopes' => $selectableScopes, 'scopeDescriptions' => $this->scopeRepository->describeScopes(), 'nonce' => Nonce::getNonce('Oauth2.authorize'), 'termsAndCondition' => $termsAndConditionUrl, @@ -238,7 +286,7 @@ private function createServerRequest() private function emitResponse(ResponseInterface $response) { - http_response_code($response->getStatusCode()); + $this->sendResponseCode($response->getStatusCode(), $response->getReasonPhrase()); foreach ($response->getHeaders() as $name => $values) { foreach ($values as $value) { Common::sendHeader($name . ': ' . $value); @@ -255,12 +303,49 @@ private function isPostRequest(): bool private function renderUnauthorized(string $message) { - http_response_code(400); + $this->sendResponseCode(400); return $message; } - private function buildAuthorizationActivityData($client, string $login, array $scopes, bool $isApproved): array + private function sendResponseCode(int $statusCode, string $reasonPhrase = ''): void + { + try { + Common::sendResponseCode($statusCode); + return; + } catch (\Exception $e) { + // Common::sendResponseCode() only knows a fixed set of status codes and rejects the + // others, such as the 405 the token and metadata endpoints send for a wrong method. + } + + Common::sendHeader(rtrim($this->getStatusHeaderPrefix() . ' ' . $statusCode . ' ' . $reasonPhrase)); + } + + /** + * Same prefix Common::sendResponseCode() uses, as FastCGI needs a Status header instead of a + * status line and the response should keep the protocol the request was made with. + */ + private function getStatusHeaderPrefix(): string { + if (strpos(PHP_SAPI, '-fcgi') !== false) { + return 'Status:'; + } + + $protocol = $_SERVER['SERVER_PROTOCOL'] ?? ''; + + if (strlen($protocol) > 1 && strlen($protocol) < 15) { + return $protocol; + } + + return 'HTTP/1.1'; + } + + private function buildAuthorizationActivityData( + $client, + string $login, + array $requestedScopes, + string $selectedScope, + bool $isApproved + ): array { $clientData = [ 'id' => method_exists($client, 'getIdentifier') ? $client->getIdentifier() : null, 'name' => method_exists($client, 'getName') ? $client->getName() : null, @@ -275,7 +360,10 @@ private function buildAuthorizationActivityData($client, string $login, array $s 'version' => 'v1', 'client' => $clientData, 'userLogin' => $login, - 'scopes' => array_values($scopes), + // keeps reporting the effective permission, which is nothing at all when denied, so a + // listener reading only this field can never read more access than was granted + 'scopes' => $isApproved ? [$selectedScope] : [], + 'requestedScopes' => array_values($requestedScopes), 'decision' => $isApproved ? 'allowed' : 'denied', ]; } @@ -305,6 +393,54 @@ private function checkDoesUserHasAccessAsPerScope(string $scope): void case 'matomo:superuser': Piwik::checkUserHasSuperUserAccess(); break; + default: + // never reached, the scope is validated against the selectable scopes beforehand + throw new \Exception(Piwik::translate('OAuth2_InvalidScopeValue')); + } + } + + /** + * Returns the requested scopes the client is allowed to use, in ascending privilege order + * (read < write < admin < superuser). + */ + private function getScopesAllowedForClient(array $requestedScopes, ClientEntityInterface $client): array + { + $allowed = $this->scopeRepository->getAllowedScopeIds(); + + if ($client instanceof ClientEntity && !empty($client->getAllowedScopes())) { + // an empty client scope list means no client specific restriction, as in ScopeRepository::finalizeScopes() + $allowed = array_values(array_intersect( + $allowed, + ScopeRepository::expandScopes($client->getAllowedScopes()) + )); + } + + return array_values(array_intersect($allowed, $requestedScopes)); + } + + /** + * Returns the scopes the current user has a high enough access level to grant. + */ + private function getScopesUserCanGrant(array $scopes): array + { + return array_values(array_filter($scopes, function (string $scope) { + return $this->canUserGrantScope($scope); + })); + } + + private function canUserGrantScope(string $scope): bool + { + switch ($scope) { + case 'matomo:read': + return Piwik::isUserHasSomeViewAccess(); + case 'matomo:write': + return Piwik::isUserHasSomeWriteAccess(); + case 'matomo:admin': + return Piwik::isUserHasSomeAdminAccess(); + case 'matomo:superuser': + return Piwik::hasUserSuperUserAccess(); + default: + return false; } } } diff --git a/OAuth2.php b/OAuth2.php index 68cd1a5..789d346 100644 --- a/OAuth2.php +++ b/OAuth2.php @@ -40,6 +40,51 @@ public static function getScopeDescriptions(): array ]; } + /** + * Maps each scope to the access role it confines a token to, and to its privilege level. + * The level is what orders the scopes, a higher level includes every lower one. + * + * @return array + */ + public static function getScopeAccessLevels(): array + { + return [ + 'matomo:read' => ['role' => 'view', 'level' => 1], + 'matomo:write' => ['role' => 'write', 'level' => 2], + 'matomo:admin' => ['role' => 'admin', 'level' => 3], + 'matomo:superuser' => ['role' => 'superuser', 'level' => 4], + ]; + } + + /** + * Returns the given scope together with every less privileged scope, in ascending order. + * + * The scope configured for a client is the maximum access level its tokens may get, as + * described by OAuth2_AdminScopeHelp, so a client allowed `matomo:admin` may also be + * granted `matomo:write` or `matomo:read`. Unknown scopes expand to nothing. + * + * @return string[] + */ + public static function expandScopeHierarchically(string $scope): array + { + $levels = self::getScopeAccessLevels(); + + if (!isset($levels[$scope])) { + return []; + } + + $maximumLevel = $levels[$scope]['level']; + $expanded = []; + + foreach ($levels as $identifier => $level) { + if ($level['level'] <= $maximumLevel) { + $expanded[] = $identifier; + } + } + + return $expanded; + } + public function registerEvents() { return [ @@ -387,24 +432,29 @@ public static function getAuthorizationHeader(): ?string private function scopeGrantsAtLeastWrite(?string $scope): bool { - $scopeToLevel = ['matomo:read' => 1, 'matomo:write' => 2, 'matomo:admin' => 3, 'matomo:superuser' => 4]; + $scopeLevels = self::getScopeAccessLevels(); // Unknown/empty scopes are treated as below write. - $level = $scopeToLevel[$scope] ?? 0; + $level = $scopeLevels[$scope]['level'] ?? 0; - return $level >= $scopeToLevel['matomo:write']; + return $level >= $scopeLevels['matomo:write']['level']; } private function modifyAccessBasedOnScope(?array $idSitesAccess, ?string $scope): array { - $levels = ['view' => 1, 'write' => 2, 'admin' => 3, 'superuser' => 4]; - $scopeToLevelMapping = ['matomo:read' => 'view', 'matomo:write' => 'write', 'matomo:admin' => 'admin', 'matomo:superuser' => 'superuser']; - if (empty($scopeToLevelMapping[$scope])) { + $scopeLevels = self::getScopeAccessLevels(); + + if (!isset($scopeLevels[$scope])) { return []; } - $target = $scopeToLevelMapping[$scope]; - $targetLevel = $levels[$target]; + $levels = []; + foreach ($scopeLevels as $scopeLevel) { + $levels[$scopeLevel['role']] = $scopeLevel['level']; + } + + $target = $scopeLevels[$scope]['role']; + $targetLevel = $scopeLevels[$scope]['level']; // Capture the subject's super user sites before the demotion loop empties them, // so the capability reconciliation below can grant a super user every capability. diff --git a/README.md b/README.md index edae4b7..13687ae 100644 --- a/README.md +++ b/README.md @@ -230,7 +230,7 @@ https://matomo.example.com/index.php?module=OAuth2&action=authorize The user will: 1. Log in to Matomo -2. Review requested permissions +2. Review requested permissions (if the request lists multiple space-separated scopes, pick exactly one to grant — the least privileged scope is preselected) 3. Click **Allow** Matomo will redirect back: @@ -241,7 +241,7 @@ https://example-app.com/oauth/callback?code=AUTHORIZATION_CODE&state=abc123 ## Exchange Authorization Code for Tokens -**Note:** The scope should be same as client scope and only one scope is allowed at the moment. +**Note:** The authorize request may list multiple space-separated scopes (e.g. `scope=matomo:read matomo:write`). The user grants exactly one of them on the consent screen, and the issued tokens always carry that single scope. The scope configured for a client is the maximum a user can grant it, so a client configured with `matomo:admin` can also be granted `matomo:write` or `matomo:read`. Tokens obtained with the client credentials grant always use the exact configured scope. ### PKCE Token Request diff --git a/Repositories/ScopeRepository.php b/Repositories/ScopeRepository.php index 1e8ccce..b71cc80 100644 --- a/Repositories/ScopeRepository.php +++ b/Repositories/ScopeRepository.php @@ -58,7 +58,16 @@ public function finalizeScopes( $allowed = $this->getAllowedScopeIds(); if ($clientEntity instanceof ClientEntity && !empty($clientEntity->getAllowedScopes())) { - $allowed = array_values(array_intersect($allowed, $clientEntity->getAllowedScopes())); + $clientScopes = $clientEntity->getAllowedScopes(); + + // A user can consent to less than the client is configured for, so codes and the + // refresh tokens that follow them may carry a lower scope. Client credentials have no + // user to make that choice, so those clients keep getting exactly what is configured. + if (in_array($grantType, ['authorization_code', 'refresh_token'], true)) { + $clientScopes = self::expandScopes($clientScopes); + } + + $allowed = array_values(array_intersect($allowed, $clientScopes)); } if (empty($scopes)) { @@ -82,6 +91,24 @@ public function finalizeScopes( return $final; } + /** + * Expands configured client scopes to every scope they permit, as a client scope is the + * maximum access level its tokens may get rather than the only one it can be granted. + * + * @param string[] $scopes + * @return string[] + */ + public static function expandScopes(array $scopes): array + { + $expanded = []; + + foreach ($scopes as $scope) { + $expanded = array_merge($expanded, OAuth2::expandScopeHierarchically($scope)); + } + + return array_values(array_unique($expanded)); + } + public function getAllowedScopeIds(): array { $configured = $this->settings->defaultScopes->getValue(); diff --git a/lang/en.json b/lang/en.json index 50d988b..3aeec9c 100644 --- a/lang/en.json +++ b/lang/en.json @@ -6,6 +6,7 @@ "AuthorizeTextTitle": "is requesting access to your Matomo account", "AuthorizeHelpText": "You can revoke access at any time from your Matomo account settings.", "RequestedScopes": "Requested scopes", + "SelectScopeToGrant": "Select the scope to grant", "Allow": "Allow", "Deny": "Deny", "AdminHeading": "OAuth 2.0 Clients", @@ -84,6 +85,7 @@ "InvalidRedirectUri": "Invalid redirect_uri", "InvalidGrantTypes": "Unsupported grant_types", "MultipleScopesNotAllowed": "Multiple scopes are not allowed.", + "NoAccessForRequestedScopes": "Your Matomo user does not have the access level required for any of the requested scopes (%1$s). Sign in with a user that has the required access, or ask an administrator to grant it to you.", "InvalidClientToRotateSecretExceptionMessage": "Invalid client, please check your client type.", "ServerError": "Server error", "SystemSettingOauthTitle": "OAuth 2.0", diff --git a/plugin.json b/plugin.json index 85febd3..9c4df3e 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "name": "OAuth2", "description": "Provide secure access to the Matomo API using scoped permissions. No static credentials.", - "version": "5.2.4", + "version": "5.3.0", "theme": false, "require": { "php": ">=8.1.0", diff --git a/stylesheets/oauth2.less b/stylesheets/oauth2.less index 6567ac0..58582ee 100644 --- a/stylesheets/oauth2.less +++ b/stylesheets/oauth2.less @@ -108,12 +108,38 @@ font-weight: 400; line-height: normal; } + + .scope-option { + display: block; + + & + .scope-option { + margin-top: 12px; + } + + // undo the fixed single-line sizing materialize applies to radio label spans + [type="radio"] + .scope-option-label { + height: auto; + line-height: normal; + } + + .scope, + .scope-help { + display: block; + } + } } .alert-warning::before { margin-top: 0.5rem; } + // core leaves the alert icon at the top of the box, which looks off next to a single scope. + // 12px is half of the icon font size core sets, as in its alert-icon-center-vertically mixin. + .alert-warning.single-scope::before { + top: calc(~'50% - 12px'); + margin-top: 0; + } + .form-help { display: flex; padding: 12px 16px; diff --git a/templates/authorize.twig b/templates/authorize.twig index dffb29e..de5ec87 100644 --- a/templates/authorize.twig +++ b/templates/authorize.twig @@ -25,19 +25,30 @@ {% if userEmail %}
{{ userEmail }}
{% endif %} -
- {{ 'OAuth2_RequestedScopes'|translate }} -
-
- {% for scope in scopes %} -
{{ scope }}
- {% if scopeDescriptions[scope] %}
{{ scopeDescriptions[scope] }}
{% endif %} - {% endfor %} -
- -
{{ 'OAuth2_AuthorizeHelpText'|translate }}
-
+
+ {% if scopes|length > 1 %}{{ 'OAuth2_SelectScopeToGrant'|translate }}{% else %}{{ 'OAuth2_RequestedScopes'|translate }}{% endif %} +
+
+ {% if scopes|length > 1 %} + {% for scope in scopes %} + + {% endfor %} + {% else %} +
{{ scopes[0] }}
+ {% if scopeDescriptions[scopes[0]] %}
{{ scopeDescriptions[scopes[0]] }}
{% endif %} + + {% endif %} +
+ +
{{ 'OAuth2_AuthorizeHelpText'|translate }}
+ diff --git a/tests/Fixtures/OAuth2ConsentFixture.php b/tests/Fixtures/OAuth2ConsentFixture.php new file mode 100644 index 0000000..d943077 --- /dev/null +++ b/tests/Fixtures/OAuth2ConsentFixture.php @@ -0,0 +1,54 @@ +createClient(self::ADMIN_SCOPE_CLIENT_ID, 'Admin scope UI client', ['matomo:admin']); + $this->createClient(self::READ_SCOPE_CLIENT_ID, 'Read scope UI client', ['matomo:read']); + } + + private function createClient(string $clientId, string $name, array $scopes): void + { + if (!empty(StaticContainer::get(ClientModel::class)->find($clientId))) { + return; + } + + StaticContainer::get(ClientManager::class)->create([ + 'client_id' => $clientId, + 'name' => $name, + 'description' => $name . ' for consent screen UI tests', + 'redirect_uris' => [self::REDIRECT_URI], + 'grant_types' => ['authorization_code'], + 'scopes' => $scopes, + 'type' => 'confidential', + 'active' => true, + ], Fixture::ADMIN_USER_LOGIN); + } +} diff --git a/tests/Integration/AuthorizeControllerTest.php b/tests/Integration/AuthorizeControllerTest.php new file mode 100644 index 0000000..2851fcb --- /dev/null +++ b/tests/Integration/AuthorizeControllerTest.php @@ -0,0 +1,589 @@ +backupGet = $_GET; + $this->backupPost = $_POST; + $this->backupRequest = $_REQUEST; + $this->backupRequestMethod = $_SERVER['REQUEST_METHOD'] ?? null; + + $this->controller = StaticContainer::get(Controller::class); + $this->clientManager = StaticContainer::get(ClientManager::class); + } + + public function tearDown(): void + { + $_GET = $this->backupGet; + $_POST = $this->backupPost; + $_REQUEST = $this->backupRequest; + + if ($this->backupRequestMethod === null) { + unset($_SERVER['REQUEST_METHOD']); + } else { + $_SERVER['REQUEST_METHOD'] = $this->backupRequestMethod; + } + + if ($this->recordsSentHeaders()) { + Common::$headersSentInTests = []; + } + + parent::tearDown(); + } + + public function test_get_showsRadioGroupWithLeastPrivilegedScopePreselected() + { + $client = $this->createClient(['matomo:admin']); + + $html = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:admin matomo:write matomo:read')); + + $this->assertSame(3, substr_count($html, 'name="selected_scope"')); + $this->assertStringContainsString('value="matomo:read" checked', $html); + $this->assertStringContainsString(Piwik::translate('OAuth2_SelectScopeToGrant'), $html); + + // ascending privilege order regardless of the order in the request + $this->assertLessThan(strpos($html, 'value="matomo:write"'), strpos($html, 'value="matomo:read"')); + $this->assertLessThan(strpos($html, 'value="matomo:admin"'), strpos($html, 'value="matomo:write"')); + } + + public function test_get_showsSingleScopeWithoutRadiosWhenClientAllowsOnlyOne() + { + $client = $this->createClient(['matomo:read']); + + $html = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin')); + + $this->assertStringNotContainsString('type="radio"', $html); + $this->assertStringContainsString('type="hidden" name="selected_scope" value="matomo:read"', $html); + $this->assertStringContainsString(Piwik::translate('OAuth2_RequestedScopes'), $html); + } + + public function test_get_fallsBackToGloballyAllowedScopesForClientWithoutScopeRestriction() + { + $client = $this->createClient([]); + + $html = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin')); + + $this->assertSame(3, substr_count($html, 'name="selected_scope"')); + } + + public function test_get_hidesScopesTheUserCannotGrant() + { + $client = $this->createClient(['matomo:admin']); + FakeAccess::clearAccess(false, [], [1], 'writeUserLogin', [1]); + + $html = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin')); + + $this->assertSame(2, substr_count($html, 'name="selected_scope"')); + $this->assertStringContainsString('value="matomo:read" checked', $html); + $this->assertStringContainsString('value="matomo:write"', $html); + $this->assertStringNotContainsString('matomo:admin', $html); + } + + public function test_get_offersEveryScopeUpToTheClientMaximumOnly() + { + // the configured scope is a maximum, so a write client also offers read but never admin + $client = $this->createClient(['matomo:write']); + + $html = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin')); + + $this->assertSame(2, substr_count($html, 'name="selected_scope"')); + $this->assertStringContainsString('value="matomo:read" checked', $html); + $this->assertStringContainsString('value="matomo:write"', $html); + $this->assertStringNotContainsString('matomo:admin', $html); + } + + /** + * @dataProvider getUserAuthorizedGrantTypes + */ + public function test_finalizeScopes_acceptsAScopeBelowTheClientMaximumForUserAuthorizedGrants(string $grantType) + { + // the consent screen may grant less than the client is configured for, so the token + // endpoint has to accept the downgraded scope when the code is exchanged, and again + // when the resulting refresh token is used + $clientEntity = new ClientEntity(); + $clientEntity->allowedScopes = ['matomo:admin']; + + $scopeRepository = StaticContainer::get(ScopeRepository::class); + + $finalized = $scopeRepository->finalizeScopes( + [$scopeRepository->getScopeEntityByIdentifier('matomo:read')], + $grantType, + $clientEntity, + Fixture::ADMIN_USER_LOGIN + ); + + $this->assertSame(['matomo:read'], array_map(static function ($scope) { + return $scope->getIdentifier(); + }, $finalized)); + } + + public function getUserAuthorizedGrantTypes(): array + { + return [['authorization_code'], ['refresh_token']]; + } + + public function test_finalizeScopes_requiresTheExactClientScopeForClientCredentials() + { + // there is no user to pick a lower scope, so the configured scope stays a requirement + $clientEntity = new ClientEntity(); + $clientEntity->allowedScopes = ['matomo:admin']; + + $scopeRepository = StaticContainer::get(ScopeRepository::class); + + $this->expectException(OAuthServerException::class); + + $scopeRepository->finalizeScopes( + [$scopeRepository->getScopeEntityByIdentifier('matomo:read')], + 'client_credentials', + $clientEntity, + Fixture::ADMIN_USER_LOGIN + ); + } + + public function test_get_rejectsRequestWhenTheClientAllowsNoneOfTheRequestedScopes() + { + $client = $this->createClient(['matomo:read']); + + $response = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:write matomo:admin')); + + $this->assertSame(Piwik::translate('OAuth2_InvalidClientScope'), $response); + $this->assertResponseCodeSent(400, 'Bad Request'); + } + + public function test_get_rejectsRequestWithADistinctMessageWhenTheUserCannotGrantAnyScope() + { + $client = $this->createClient(['matomo:admin']); + FakeAccess::clearAccess(false, [], [], 'noAccessLogin'); + + $response = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:write matomo:admin')); + + $this->assertSame( + Piwik::translate('OAuth2_NoAccessForRequestedScopes', 'matomo:write, matomo:admin'), + $response + ); + $this->assertResponseCodeSent(400, 'Bad Request'); + // the scope mapping is fine here, so the message must not blame the client configuration + $this->assertStringNotContainsString(Piwik::translate('OAuth2_InvalidClientScope'), $response); + } + + public function test_get_deduplicatesRequestedScopes() + { + $client = $this->createClient(['matomo:admin']); + + $html = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:read matomo:read matomo:write')); + + $this->assertSame(2, substr_count($html, 'name="selected_scope"')); + $this->assertSame(1, substr_count($html, 'value="matomo:read"')); + } + + public function test_post_allow_issuesAuthCodeNarrowedToSelectedScope() + { + $client = $this->createClient(['matomo:admin']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + $this->requestAuthorize( + $this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin'), + ['decision' => 'allow', 'selected_scope' => 'matomo:write', 'nonce' => Nonce::getNonce('Oauth2.authorize')] + ); + + // the issued code carries only the selected scope + $this->assertSame(['matomo:write'], $this->storedAuthCodeScopes($client['client']['client_id'])); + $this->assertCount(1, $capturedEvents); + // the audit records the granted scope and, separately, everything that was requested + $this->assertSame(['matomo:write'], $capturedEvents[0]['scopes']); + $this->assertSame(['matomo:read', 'matomo:write', 'matomo:admin'], $capturedEvents[0]['requestedScopes']); + $this->assertSame('allowed', $capturedEvents[0]['decision']); + + $redirectParams = $this->parseRedirectOrSkip(); + $this->assertNotEmpty($redirectParams['code']); + $this->assertSame('test-state', $redirectParams['state']); + $this->assertResponseCodeSent(302, 'Found'); + + // the narrowed code passes finalizeScopes at the token endpoint and yields a matomo:write token + $tokenPayload = $this->exchangeCodeForTokens($client, $redirectParams['code']); + $this->assertNotEmpty($tokenPayload['access_token']); + + $storedToken = Db::fetchRow( + 'SELECT * FROM ' . Common::prefixTable('oauth2_access_token') . ' ORDER BY created_at DESC LIMIT 1' + ); + $this->assertSame(['matomo:write'], json_decode($storedToken['scopes'], true)); + } + + public function test_post_allow_rejectsScopeOutsideTheSelectableSet() + { + $client = $this->createClient(['matomo:write']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + $response = $this->requestAuthorize( + $this->authorizeQuery($client, 'matomo:read matomo:write'), + ['decision' => 'allow', 'selected_scope' => 'matomo:superuser', 'nonce' => Nonce::getNonce('Oauth2.authorize')] + ); + + $this->assertSame(Piwik::translate('OAuth2_InvalidScopeValue'), $response); + $this->assertResponseCodeSent(400, 'Bad Request'); + $this->assertCount(0, $capturedEvents); + $this->assertSame(0, $this->countAuthCodes($client['client']['client_id'])); + } + + public function test_post_allow_rejectsMissingSelectedScope() + { + $client = $this->createClient(['matomo:write']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + $response = $this->requestAuthorize( + $this->authorizeQuery($client, 'matomo:read matomo:write'), + ['decision' => 'allow', 'nonce' => Nonce::getNonce('Oauth2.authorize')] + ); + + $this->assertSame(Piwik::translate('OAuth2_InvalidScopeValue'), $response); + $this->assertResponseCodeSent(400, 'Bad Request'); + $this->assertCount(0, $capturedEvents); + $this->assertSame(0, $this->countAuthCodes($client['client']['client_id'])); + } + + public function test_post_deny_recordsTheRequestedScopesAndNoGrantedScope() + { + $client = $this->createClient(['matomo:write']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + $this->requestAuthorize( + $this->authorizeQuery($client, 'matomo:read matomo:write'), + ['decision' => 'deny', 'selected_scope' => 'matomo:read', 'nonce' => Nonce::getNonce('Oauth2.authorize')] + ); + + $this->assertCount(1, $capturedEvents); + // denying grants nothing, so no scope is reported as granted, while the request the user + // refused is still recorded in full + $this->assertSame([], $capturedEvents[0]['scopes']); + $this->assertSame(['matomo:read', 'matomo:write'], $capturedEvents[0]['requestedScopes']); + $this->assertSame('denied', $capturedEvents[0]['decision']); + $this->assertSame(0, $this->countAuthCodes($client['client']['client_id'])); + + $redirectParams = $this->parseRedirectOrSkip(); + $this->assertSame('access_denied', $redirectParams['error']); + } + + public function test_post_readsTheDecisionFromThePostBodyOnly() + { + $client = $this->createClient(['matomo:write']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + // a query string parameter must not override the decision the user submitted + $query = $this->authorizeQuery($client, 'matomo:read matomo:write'); + $query['decision'] = 'allow'; + + $this->requestAuthorize($query, [ + 'decision' => 'deny', + 'selected_scope' => 'matomo:read', + 'nonce' => Nonce::getNonce('Oauth2.authorize'), + ]); + + $this->assertSame('denied', $capturedEvents[0]['decision']); + $this->assertSame([], $capturedEvents[0]['scopes']); + $this->assertSame(0, $this->countAuthCodes($client['client']['client_id'])); + + $redirectParams = $this->parseRedirectOrSkip(); + $this->assertSame('access_denied', $redirectParams['error']); + $this->assertArrayNotHasKey('code', $redirectParams); + } + + public function test_post_readsTheSelectedScopeFromThePostBodyOnly() + { + $client = $this->createClient(['matomo:admin']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + // a query string parameter must not widen the scope the user picked on the consent screen + $query = $this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin'); + $query['selected_scope'] = 'matomo:admin'; + + $this->requestAuthorize($query, [ + 'decision' => 'allow', + 'selected_scope' => 'matomo:read', + 'nonce' => Nonce::getNonce('Oauth2.authorize'), + ]); + + $this->assertSame(['matomo:read'], $this->storedAuthCodeScopes($client['client']['client_id'])); + $this->assertSame(['matomo:read'], $capturedEvents[0]['scopes']); + + $redirectParams = $this->parseRedirectOrSkip(); + $this->assertNotEmpty($redirectParams['code']); + + $this->exchangeCodeForTokens($client, $redirectParams['code']); + $storedToken = Db::fetchRow( + 'SELECT * FROM ' . Common::prefixTable('oauth2_access_token') . ' ORDER BY created_at DESC LIMIT 1' + ); + $this->assertSame(['matomo:read'], json_decode($storedToken['scopes'], true)); + } + + public function test_post_rejectsADecisionValueThatIsNeitherAllowNorDeny() + { + $client = $this->createClient(['matomo:read']); + $capturedEvents = $this->captureAuthorizeDecisionEvents(); + + $response = $this->requestAuthorize($this->authorizeQuery($client, 'matomo:read'), [ + 'decision' => 'something else', + 'selected_scope' => 'matomo:read', + 'nonce' => Nonce::getNonce('Oauth2.authorize'), + ]); + + $this->assertSame(Piwik::translate('OAuth2_InvalidAuthorizationRequest'), $response); + $this->assertResponseCodeSent(400, 'Bad Request'); + $this->assertCount(0, $capturedEvents); + $this->assertSame(0, $this->countAuthCodes($client['client']['client_id'])); + } + + public function test_post_allow_singleScopeClientStillIssuesCode() + { + $client = $this->createClient(['matomo:read']); + + $this->requestAuthorize( + $this->authorizeQuery($client, 'matomo:read matomo:write matomo:admin'), + ['decision' => 'allow', 'selected_scope' => 'matomo:read', 'nonce' => Nonce::getNonce('Oauth2.authorize')] + ); + + $this->assertSame(['matomo:read'], $this->storedAuthCodeScopes($client['client']['client_id'])); + + $redirectParams = $this->parseRedirectOrSkip(); + $this->assertNotEmpty($redirectParams['code']); + + $tokenPayload = $this->exchangeCodeForTokens($client, $redirectParams['code']); + $this->assertNotEmpty($tokenPayload['access_token']); + } + + public function test_emitResponse_sendsStatusCodesCoreHasNoReasonPhraseFor() + { + if (!$this->recordsSentHeaders()) { + $this->markTestSkipped('Asserting the sent status line requires Matomo 5.1.0 or higher'); + } + + // the token endpoint answers a GET with 405, which Common::sendResponseCode() does not know + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_GET = ['module' => 'OAuth2', 'action' => 'token']; + $_POST = []; + $_REQUEST = $_GET; + + ob_start(); + try { + $this->controller->token(); + } finally { + $body = ob_get_clean(); + } + + $this->assertSame(Piwik::translate('OAuth2_TokenEndpointException'), $body); + $this->assertResponseCodeSent(405, 'Method Not Allowed'); + } + + public function test_checkDoesUserHasAccessAsPerScope_failsClosedForAnUnknownScope() + { + // the selectable scope validation makes this unreachable, the check must still not pass silently + $method = new \ReflectionMethod(Controller::class, 'checkDoesUserHasAccessAsPerScope'); + $method->setAccessible(true); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage(Piwik::translate('OAuth2_InvalidScopeValue')); + + $method->invoke($this->controller, 'matomo:unknown'); + } + + private function createClient(array $scopes): array + { + return $this->clientManager->create([ + 'name' => 'Consent test client', + 'description' => 'Authorize controller test client', + 'redirect_uris' => [self::REDIRECT_URI], + 'grant_types' => ['authorization_code'], + 'scopes' => $scopes, + 'type' => 'confidential', + 'active' => true, + ], Fixture::ADMIN_USER_LOGIN); + } + + private function authorizeQuery(array $client, string $scope): array + { + return [ + 'module' => 'OAuth2', + 'action' => 'authorize', + 'response_type' => 'code', + 'client_id' => $client['client']['client_id'], + 'redirect_uri' => self::REDIRECT_URI, + 'scope' => $scope, + 'state' => 'test-state', + ]; + } + + private function requestAuthorize(array $query, ?array $post = null): string + { + $_SERVER['REQUEST_METHOD'] = $post === null ? 'GET' : 'POST'; + $_GET = $query; + $_POST = $post ?? []; + $_REQUEST = array_merge($_POST, $_GET); + + ob_start(); + try { + $result = $this->controller->authorize(); + } finally { + $echoed = ob_get_clean(); + } + + return (string) ($result ?? $echoed); + } + + private function captureAuthorizeDecisionEvents(): \ArrayObject + { + $capturedEvents = new \ArrayObject(); + Piwik::addAction('OAuth2.authorize.decision.end', function ($activityData) use ($capturedEvents) { + $capturedEvents[] = $activityData; + }); + + return $capturedEvents; + } + + /** + * Core only records sent headers in test mode since Matomo 5.1.0, while the plugin supports + * 5.0.0 and up, so assertions on the status line and the redirect have to be conditional. + */ + private function recordsSentHeaders(): bool + { + return property_exists(Common::class, 'headersSentInTests'); + } + + private function assertResponseCodeSent(int $statusCode, string $reasonPhrase): void + { + if (!$this->recordsSentHeaders()) { + return; + } + + // the status line is sent as a header without a colon, so it is recorded as a key in test + // mode, prefixed with the request protocol or with Status: on FastCGI + $statusLines = array_filter(array_keys(Common::$headersSentInTests), function (string $header) use ($statusCode, $reasonPhrase) { + return substr($header, -strlen($statusCode . ' ' . $reasonPhrase)) === $statusCode . ' ' . $reasonPhrase; + }); + + $this->assertCount(1, $statusLines, 'expected a sent status line for ' . $statusCode . ' ' . $reasonPhrase); + } + + private function parseRedirectOrSkip(): array + { + if (!$this->recordsSentHeaders()) { + $this->markTestSkipped('Reading the redirect requires Matomo 5.1.0 or higher'); + } + + $location = trim(Common::$headersSentInTests['Location'] ?? ''); + $this->assertNotEmpty($location, 'expected authorize() to respond with a redirect'); + parse_str((string) parse_url($location, PHP_URL_QUERY), $redirectParams); + + return $redirectParams; + } + + private function storedAuthCodeScopes(string $clientId): array + { + $scopes = Db::fetchOne( + 'SELECT scopes FROM ' . Common::prefixTable('oauth2_auth_code') + . ' WHERE client_id = ? ORDER BY created_at DESC LIMIT 1', + [$clientId] + ); + + return json_decode((string) $scopes, true) ?: []; + } + + private function countAuthCodes(string $clientId): int + { + return (int) Db::fetchOne( + 'SELECT COUNT(*) FROM ' . Common::prefixTable('oauth2_auth_code') . ' WHERE client_id = ?', + [$clientId] + ); + } + + private function exchangeCodeForTokens(array $client, string $code): array + { + $serverFactory = new ServerFactory( + StaticContainer::get(ClientRepository::class), + StaticContainer::get(AccessTokenRepository::class), + StaticContainer::get(ScopeRepository::class), + StaticContainer::get(AuthCodeRepository::class), + StaticContainer::get(RefreshTokenRepository::class), + new SystemSettings() + ); + + $tokenResponse = $serverFactory->makeAuthorizationServer()->respondToAccessTokenRequest( + (new ServerRequest('POST', 'https://matomo.example/token'))->withParsedBody([ + 'grant_type' => 'authorization_code', + 'client_id' => $client['client']['client_id'], + 'client_secret' => $client['secret'], + 'code' => $code, + 'redirect_uri' => self::REDIRECT_URI, + ]), + new Response() + ); + + $this->assertSame(200, $tokenResponse->getStatusCode()); + + return json_decode((string) $tokenResponse->getBody(), true); + } + + public function provideContainerConfig() + { + return [ + 'Piwik\Access' => new FakeAccess(), + ]; + } +} + +AuthorizeControllerTest::$fixture = new OAuth2Fixture(); diff --git a/tests/Integration/OAuthFlowTest.php b/tests/Integration/OAuthFlowTest.php index d4b23b5..b261860 100644 --- a/tests/Integration/OAuthFlowTest.php +++ b/tests/Integration/OAuthFlowTest.php @@ -317,6 +317,8 @@ public function test_clientCredentialsFlow_returnsAccessTokenForConfidentialClie public function test_clientCredentialsFlow_withoutScope_rejectsClientWhenReadScopeIsNotAllowed() { + // the client scope is only treated as a maximum for scopes a user consented to, so a + // client credentials client still has to be configured for the scope it receives $client = $this->api->createClient( 'Write only machine client', ['client_credentials'], diff --git a/tests/UI/OAuth2Consent_spec.js b/tests/UI/OAuth2Consent_spec.js new file mode 100644 index 0000000..d8b04d1 --- /dev/null +++ b/tests/UI/OAuth2Consent_spec.js @@ -0,0 +1,71 @@ +/*! + * Matomo - free/libre analytics platform + * + * Screenshot integration tests for the OAuth2 consent screen. + * + * @link https://matomo.org + * @license https://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later + */ + +describe("OAuth2Consent", function () { + this.fixture = "Piwik\\Plugins\\OAuth2\\tests\\Fixtures\\OAuth2ConsentFixture"; + this.optionsOverride = { + 'persist-fixture-data': false + }; + + // must match the constants in OAuth2ConsentFixture + const adminScopeClientId = '11111111111111111111111111111111'; + const readScopeClientId = '22222222222222222222222222222222'; + const redirectUri = 'https://client.example/callback'; + + before(function () { + testEnvironment.pluginsToLoad = ['OAuth2']; + testEnvironment.save(); + }); + + function authorizeUrl(clientId, scope) + { + return '?module=OAuth2&action=authorize&response_type=code' + + '&client_id=' + clientId + + '&redirect_uri=' + encodeURIComponent(redirectUri) + + '&scope=' + encodeURIComponent(scope) + + '&state=uitest'; + } + + it('should show a scope radio group with the least privileged scope preselected', async function () { + await page.goto(authorizeUrl(adminScopeClientId, 'matomo:read matomo:write matomo:admin')); + await page.waitForSelector('.card-authorize', { visible: true }); + await page.waitForSelector('input[name="selected_scope"][value="matomo:admin"]', { visible: true }); + await page.waitForNetworkIdle(); + + const radioValues = await page.$$eval('input[name="selected_scope"]', function (inputs) { + return inputs.map(function (input) { return input.value; }); + }); + expect(radioValues).to.deep.equal(['matomo:read', 'matomo:write', 'matomo:admin']); + + const readIsChecked = await page.$eval('input[name="selected_scope"][value="matomo:read"]', function (input) { + return input.checked; + }); + expect(readIsChecked).to.equal(true); + + expect(await page.screenshotSelector('.card-authorize')).to.matchImage('consent_screen_multiple_scopes'); + }); + + it('should show a single selectable scope without radio buttons', async function () { + await page.goto(authorizeUrl(readScopeClientId, 'matomo:read matomo:write matomo:admin')); + await page.waitForSelector('.card-authorize', { visible: true }); + // the only scope input is hidden here, so wait for the rendered scope instead + await page.waitForSelector('.alert-warning .scope', { visible: true }); + await page.waitForNetworkIdle(); + + const radios = await page.$$('.card-authorize input[type="radio"]'); + expect(radios.length).to.equal(0); + + const selectedScope = await page.$eval('input[name="selected_scope"]', function (input) { + return input.value; + }); + expect(selectedScope).to.equal('matomo:read'); + + expect(await page.screenshotSelector('.card-authorize')).to.matchImage('consent_screen_single_scope'); + }); +}); diff --git a/tests/UI/expected-ui-screenshots/OAuth2Consent_consent_screen_multiple_scopes.png b/tests/UI/expected-ui-screenshots/OAuth2Consent_consent_screen_multiple_scopes.png new file mode 100644 index 0000000..af12993 Binary files /dev/null and b/tests/UI/expected-ui-screenshots/OAuth2Consent_consent_screen_multiple_scopes.png differ diff --git a/tests/UI/expected-ui-screenshots/OAuth2Consent_consent_screen_single_scope.png b/tests/UI/expected-ui-screenshots/OAuth2Consent_consent_screen_single_scope.png new file mode 100644 index 0000000..93c1e4c Binary files /dev/null and b/tests/UI/expected-ui-screenshots/OAuth2Consent_consent_screen_single_scope.png differ