Accept multiple scopes in authorize requests and let the user grant one, PG-5168 - #43
Conversation
Spec-bound MCP clients send every scope advertised by /.well-known/oauth-authorization-server in the authorize request, which the single-scope guard rejected outright, so no connection could be established. The consent screen now shows the selectable scopes as a radio group defaulting to the least privileged one, and the submitted scope is re-validated server side and narrowed onto the authorization request before the code is issued. A single selectable scope is still shown as before. Selectable scopes are the requested scopes the client may use and the signed in user has the access level to grant. A client without a scope restriction falls back to the globally allowed scopes, as ScopeRepository::finalizeScopes() already did. When the user cannot grant any of the requested scopes the error names them instead of reporting an invalid client scope mapping. Response status codes are sent through Common so they are recorded in test mode, which makes them assertable. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
checkDoesUserHasAccessAsPerScope() had no default case, so any scope outside the four known identifiers passed without a single access check. It is unreachable today because the selected scope is validated against the selectable scopes first, but a future caller that misses that step would silently skip authorisation, so the switch now throws instead. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The consent decision, the nonce and the selected scope were read through Request::fromRequest(), which builds its parameters from $_GET + $_POST and therefore lets a query string parameter take precedence over the value the user actually submitted in the consent form. They are now read from the POST body only, and the decision has to be exactly allow or deny rather than anything that is not "allow" being treated as a denial. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…atomo Common::$headersSentInTests only exists since Matomo 5.1.0, but the plugin supports 5.0.0 and up, so touching it unconditionally made every new test error out on the minimum_required_matomo build. Access to it is now guarded. The assertions that prove the granted scope and the recorded decision read the stored authorization code and the audit event instead of the redirect, so they still run on 5.0.x, and only the redirect and status line assertions are skipped there. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The locally generated images differed from the CI rendering, so the two new consent screen assertions failed on the UI build. Content is identical, only font rendering differs. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Core positions the alert icon at the top of the box and the plugin only nudged it down by a fixed margin, which left it visibly off centre next to the two lines of a single scope. The single scope box now carries a modifier class that centres the icon the way core's alert-icon-center-vertically mixin does. The multi scope box keeps the existing position, where the icon sits beside the first option. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fallback for status codes Common::sendResponseCode() does not know, such as the 405 the token and metadata endpoints send for a wrong request method, always emitted an HTTP/1.1 status line. Core sends a Status header on FastCGI and otherwise keeps the protocol of the request, so the fallback now derives the prefix the same way. The test no longer pins the protocol prefix, since it depends on the SAPI the tests run under. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two consent screenshots waited 250ms before capturing. They now wait for the elements they assert on, which is the readiness condition the fixed delay stood in for. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The event is consumed by Activity/AuthorizeClient and is available to other plugins, but the payload it carries was never described. Documented in place, including that the granted scopes hold the single scope the user selected rather than everything the client requested. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@mneudert Does this PR meet what you were trying to achieve with PG-5168? |
AltamashShaikh
left a comment
There was a problem hiding this comment.
@snake14 I was testing this locally and it seems sending multiple scope, still shows only 1 scope which is mapped in the UI, its will not throw any error for Claude MCP.
Test Link
https://localhost.matomo.com/index.php?module=OAuth2&action=authorize&response_type=code&client_id=xx&redirect_uri=http://localhost.demo.com/matomoOAuth2Callback.php&scope=matomo:read%20matomo:write%20matomo:admin%20matomo:superuser&state=abc123&code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM&code_challenge_method=S256
The API accepts a single scope per client, so intersecting the requested scopes with that one configured scope always left exactly one selectable scope and the consent screen never offered a choice in practice. Reported on the PR after testing a client mapped to one scope in the UI. The configured scope is now applied the way OAuth2_AdminScopeHelp already describes it, as the maximum access level the client may be granted, so a client configured with matomo:admin also permits matomo:write and matomo:read and the user selects among them. Expansion happens both when building the selectable scopes and in ScopeRepository::finalizeScopes(), otherwise a downgraded authorization code would be rejected when it is exchanged for a token. Scope levels move into OAuth2::getScopeAccessLevels() as the single source of truth, replacing the two duplicated level maps. The UI fixture now configures clients with a single scope, as the API does, instead of a scope list that no client can actually have. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Expanding the configured client scope for every grant type also changed the client credentials flow, where a client configured above read started receiving a read token for a request that named no scope instead of being rejected. That behaviour was never part of this change. The expansion now applies to the authorization code and refresh token grants only, so a user can still consent to a lower scope and the refresh token that follows keeps working, while client credentials clients keep getting exactly the scope they are configured for. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The UI fixture now configures clients with a single scope like the API does, so both clients were renamed after what they are configured for. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Thanks @AltamashShaikh . I think misunderstood the initiating request and expected the collection of scopes. It should be fixed now. |
AltamashShaikh
left a comment
There was a problem hiding this comment.
Works now as expected
@snake14 Can you check this ?
Medium-risk issues
- The branch changes the meaning of the OAuth2.authorize.decision.end payload without changing its version. In Controller.php:154, scopes is now documented and
emitted as only the user-selected scope. Previously the event carried the request scopes. For allowed decisions that may be acceptable, but for denied decisions
it is actively misleading: the event now reports one selected/defaulted scope even though no scope was granted and the client may have requested more. Any
downstream plugin or audit consumer using version: v1 can silently misinterpret this.
- The tests lock in that misleading denied-event behavior. tests/Integration/AuthorizeControllerTest.php:299 explicitly asserts that a denied multi-scope request
records only ['matomo:read']. If the user simply clicks Deny on the default selection, the audit trail will say they denied matomo:read, not that the client
requested matomo:read matomo:write. That weakens the branch’s own stated audit value.
Low-risk / polish
- The event docblock in Controller.php:141 says “the scopes that were granted” even though the same payload shape is emitted for denied decisions. That wording
should be split between “requested”, “selected”, and “granted” semantics, or the payload should carry separate fields
…iting The authorize activity reported the selected scope in `scopes`, which previously held everything the client asked for. On a denied decision that was doubly wrong: it named a scope although nothing was granted, and it dropped the rest of the request, so the audit trail no longer showed what the client had asked for. `scopes` goes back to the scopes the client requested, which an authorize request may now list several of, and the scope the user granted moves to a new `grantedScope` that is null when the request was denied. The activity has to whitelist the new key as well, otherwise it never reaches the log. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…side Reporting the requested scopes in `scopes` meant a listener that reads it as the effective permission, which it always was before this branch, could act on more access than the user granted. That fails in the unsafe direction, so `scopes` keeps its meaning and only ever holds what was granted, which is nothing at all when the request is denied. Everything the client asked for moves to a new `requestedScopes`, so a denied decision still records the full request rather than losing it. Refs PG-5168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AltamashShaikh Agreed, and fixed.
|
Description
Spec-bound MCP clients (Claude Web, Claude Code) send every scope advertised by
/.well-known/oauth-authorization-serverin the authorize request, which the single-scope guard rejected outright, so no connection could be established without manually overriding the client to a single scope.The consent screen now lists the selectable scopes as a radio group and the user grants exactly one, defaulting to the least privileged (read < write < admin < superuser). The chosen scope is re-validated server side and narrowed onto the authorization request before the code is issued, so tokens still carry exactly one scope, and it is recorded in the authorize audit activity for both allow and deny. Selectable scopes are limited to those the client may use and the signed-in user has the access level to grant, so a write-only user is never offered
matomo:admin. A single selectable scope is still shown as plain text.Also includes a hardening fix for the consent handling, tracked separately under #AS-621, kept in its own commit.
Behaviour changes worth a look during review:
ScopeRepository::finalizeScopes()already did at the token endpoint.allownordenyis rejected instead of being treated as a denial.Issue No
PG-5168
Steps to Replicate the Issue
?module=OAuth2&action=authorize&response_type=code&client_id=<clientId>&redirect_uri=<redirectUri>&scope=matomo:read%20matomo:write%20matomo:admin&state=testmatomo:readpreselected. Allow, exchange the code, and the token carries only the chosen scope. Deny returnserror=access_denied. Either way the ActivityLog entry records the selected scope.Checklist
🤖 Generated with Claude Code