[OAuth] Multi-application OAuth: public contracts, audience binding, scope catalogue - #2022
Draft
fashxp wants to merge 22 commits into
Draft
[OAuth] Multi-application OAuth: public contracts, audience binding, scope catalogue#2022fashxp wants to merge 22 commits into
fashxp wants to merge 22 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Exposes five OAuth resource-server contracts as public API by removing @internal annotations and clarifying URI handling.
Changes:
- Publishes token validation contracts and result DTOs.
- Publishes protected-resource registry contracts and DTOs.
- Documents registry URI canonicalisation.
Review Contract
- Claim: Public resource-server API without behavior changes.
- Root cause: Not fully addressed; audience validation is still absent (
EmbeddedTokenValidator.php:92-94). - Call sites: Existing callers were reviewed; no missed references found.
- Boundary: Audience enforcement belongs in the validator implementation.
- Compatibility: No immediate BC break, but this creates a new compatibility commitment.
- Tests: A wrong-audience regression test is required before publication.
- Docs: Runtime registration does not fully honor the new canonicalisation documentation (
ConfigProtectedResourceRegistry.php:57-60). - Risk: Resource servers could accept tokens issued for another audience; maintainer security review is recommended.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/OAuth/Dto/ResolvedAccess.php |
Publishes the validated-access result DTO. |
src/OAuth/Dto/ProtectedResourceMetadata.php |
Publishes the metadata result DTO. |
src/OAuth/Dto/ProtectedResource.php |
Publishes resource registration data and revises URI guidance. |
src/OAuth/Contract/TokenValidatorInterface.php |
Publishes token validation integration contract. |
src/OAuth/Contract/ResourceRegistryInterface.php |
Publishes protected-resource registration contract. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+26
to
+28
| * Public API. Bundles that expose their own OAuth-protected endpoints implement | ||
| * an authenticator against this contract rather than duplicating token parsing, | ||
| * signature and revocation checks. |
Comment on lines
+20
to
+21
| * `$canonicalUri` need not be pre-canonicalised: the registry canonicalises on | ||
| * registration and on every lookup. |
fashxp
changed the title
fashxp
changed the title
wwidergoldpimcore
force-pushed
the
feature/mcp-1309-server-config-management
branch
from
September 1, 2026 14:54
590203a to
5d4fe87
Compare
First step of #1309: a first-class, config-managed "MCP server" model, backend only (no UI, no endpoint yet). - McpServerDefinition / McpServerAccess value objects, with fromArray()/toArray() as the single (de)serialization boundary so the shipped symfony-config seed and the settings-store JSON map onto one shape. Access mirrors the SavedSearch/Grid sharing model (owner + shareGlobal + sharedUsers[] + sharedRoles[]), with users and roles in separate lists so a shared id is never ambiguous. - McpServerConfigRepository over Pimcore's LocationAwareConfigRepository, mirroring PerspectiveConfigRepository: shipped defaults from the new studio_mcp_servers node, runtime servers from the configured write target (settings-store or symfony-config, deployer-switchable via config_location.studio_mcp_servers). - Config tree + config_location node + prependCustomConfig wiring; the repository is registered and its config/storage args are set in the extension. Inert by default: the node defaults to an empty map and nothing consumes the repository yet. Tool registration, the per-URL endpoint, access enforcement and OAuth discovery follow in later steps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…istry Second step of #1309: the mechanism by which tools become available to assign to a server. No server/endpoint yet. - McpToolInterface: a tool describes itself via getDefinition() (name, title, description, MCP annotations, JSON schemas) and runs via execute(). Native contract — the bundle does not depend on the MCP SDK; the per-server endpoint (later step) maps these onto the wire types. - Implementing the interface auto-applies the McpToolRegistry::TAG (via registerForAutoconfiguration in the bundle), and McpToolRegistry collects the tagged tools through an #[AutowireIterator], name-keyed, rejecting duplicates. McpToolPass guards against a hand-written tag on a non-tool service. - McpToolDefinition::requiredScope() derives the OAuth scope from the tool's readOnly annotation (read-only -> mcp:read, else mcp:write; unannotated defaults to write, fail-safe) — the basis for the operation-level scope enforcement tracked for a later step. Mirrors the agent bundle's PR #118 ToolAnnotations, authored and enforced server-side. - PingTool: a built-in, dependency-free read-only tool, so a server can be exercised end-to-end without the agent bundle. Inert by default: the registry is populated but nothing consumes it yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nd discovery
Makes step-1 testable end to end without a UI: a configured server is reachable
over MCP at /pimcore-mcp/studio/{server} under the shared pimcore_mcp firewall,
so it accepts the OAuth bearer.
- Adds the mcp/sdk dependency (^0.7, matching the agent bundle) and McpServerFactory,
which assembles a tools-only Mcp\Server per definition. Each assigned tool is
resolved from the registry and bridged onto the SDK: the native execute(array)
is wrapped in a handler that reads the call arguments (via RequestContext /
CallToolRequest) and maps the result to CallToolResult, so tools stay SDK-agnostic.
- McpServerController resolves the definition by URL slug, enforces per-server access
(McpServerAccessResolver — admin/global/owner/user/role, mirroring the bundle's
config sharing), and runs the streamable-HTTP transport with an explicit middleware
stack (dropping the SDK's Dns-rebinding middleware, incompatible with a proxy). The
route is namespaced under /studio/ and declared explicitly so it is neither swept
under the Studio API prefix nor colliding with other bundles' /pimcore-mcp/ routes.
- The extension advertises each enabled server as an RFC 9728 protected resource
(derived from the issuer), so the per-server 401 challenge and discovery resolve.
- A dedicated MCP session cache pool keeps sessions isolated from other bundles.
Verified live: unauthenticated -> 401 + WWW-Authenticate with the per-server
resource_metadata + scope; the resource metadata resolves 200; an authenticated
initialize -> tools/list -> tools/call ping returns "pong".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expose the location-aware MCP server configuration over the Studio API so a UI can manage servers without touching symfony-config or the settings store: - Servers: GET/POST/PUT/DELETE under /pimcore-studio/api/mcp/servers, one action per controller, guarded by a new `mcp_servers` user permission. - Tools: GET /pimcore-studio/api/mcp/tools returns the registry's tool catalogue (name, title, description, required scope, read-only/destructive hints) for assignment to a server. The service derives a server's advertised OAuth scopes from its tools' required scope, preserves the owner across updates, locks the url slug to the id, and builds the serving URL from the OAuth issuer. Response DTOs flatten the access model (owner/shareGlobal/sharedUsers/sharedRoles) and each carries a pre-response event. Adds the MCP OpenAPI tag and translation keys. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g installs The mcp_servers permission is created by the installer on a fresh install, but that runs only once — instances updating from an earlier version never get the row. Without it the Studio permission voter cannot resolve the attribute and denies every user (admins included, since the admin bypass lives inside the vote which never runs for an unsupported attribute), so the MCP server management endpoints return 403. The migration inserts the definition idempotently (guarded on existence, so it is a no-op on a fresh install or a forward-merge replay) and drops the cached permission-key list in postUp so the change takes effect without a separate cache clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lists Rework the per-server access model on the agent bundle's run/update pattern, but kept deny-by-default and keyed by id: - Two levels (McpServerPermission Read/Write, write implies read). Read = see the server, view its config, copy the URL, connect a client at runtime; Write = read plus edit, re-share and delete. - Access is a grid: owner (implicit write) + global read flag + user/role share entries, each carrying a level (McpServerAccessEntry). The stored/submitted shapes tolerate the earlier flat id lists, reading them as read grants. - The resolver answers a requested level: admin, then owner, then an authoritative direct-user entry, then a granting role, then global-read, else deny. It backs both the Studio API and the runtime serving endpoint (which asks for Read). Endpoint gating follows the single-permission path: mcp_servers now gates only create and the tool catalogue. List/get are ungated and filtered/asserted by read access, so a user a server is shared with — with no manage permission — still sees it and copies its URL; update/delete assert write. The response carries the caller's resolved permissions (currentUserPermissions) plus the grid, so the UI can mirror the agent-bundle sharing editor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a setup/configuration page for the opt-in OAuth 2.1 authorization server under Installation and Configuration (enabling, key generation, exposing the root-level endpoints, the auth-code + PKCE flow, client onboarding, and a full config reference), and link it from the install guide. Also update the MCP Server Infrastructure doc's authenticator-chain table, which was stale: the pimcore_mcp firewall now runs OAuthAccessTokenAuthenticator (JWT bearers) between the internal token and PAT authenticators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two docs for the runtime MCP server feature: - Extending/Providing MCP Tools: the McpToolInterface contract, the auto-applied pimcore.studio_backend.mcp_tool tag, annotations-to-scope mapping, and McpToolResult. - Development Details/MCP Server Management: the mcp_servers permission, the settings-store write target, the Studio API surface, the read/write sharing model (deny-by-default, resolution order), and the Studio master/detail management UI including its read-only behavior. Both are cross-linked with the existing MCP infrastructure and OAuth docs and registered in their section indexes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the bundle's parallel MCP tool framework in favour of the mcp/sdk types the agent bundle already uses, so studio tools and agent tools share one contract: - Delete McpToolInterface, McpToolDefinition, McpToolAnnotations, McpToolResult (and the now-dead DuplicateMcpToolException). A tool is now a plain service with an #[McpTool] method returning a CallToolResult. - Tools opt in explicitly with the pimcore.studio_backend.mcp_tool tag (no auto-tag-by-interface); McpToolPass reflects the attribute into the registry and builds a service locator. Tool names must be unique. - McpToolRegistry hands out McpToolReference descriptors (SDK Tool metadata + class/method); McpServerFactory registers them straight onto the SDK builder's addTool([class, method], ...) with a generated + normalized input schema — the former bridge closure and result mapping are gone. - The only Pimcore-specific concern kept is the OAuth scope, now a one-line McpScopes::forReadOnly() helper over the tool's readOnlyHint. - PingTool becomes the reference #[McpTool] example; the tool-authoring doc is rewritten around #[McpTool]/#[Schema]/CallToolResult. Server management, sharing, the mcp_servers permission, the API and routes are unchanged. Full cross-bundle unification of registries/routes is out of scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Configurations are meant to be portable across instances where the same user or role carries a different numeric id, so the sharing model now identifies users and roles by their (unique) name — matching how the agent bundle does it: - McpServerAccess.owner and McpServerAccessEntry are name-based; the stored shape and the API (McpServer.owner, McpServerAccessGrant.name) use names. - McpServerAccessResolver matches owner/user entries on the current user's name and resolves the user's role ids to names via RoleResolverInterface (the same id->name resolution the agent bundle performs). - The service stamps the owner from getCurrentUser()->getName(). The feature is experimental and unreleased, so no id->name back-compat is kept; the tolerant deserializer now reads a bare string as a read grant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… OAuth page Match the OAuth server doc to the removal of the pre-registered `clients` config and the client_credentials machine path (that change lives on the OAuth branch): onboarding is DCR/CIMD only, and non-interactive access uses the PAT authenticator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the single read/write level with three independent capabilities on an
MCP server, matching the two-checkbox sharing the UI wants:
- A share entry is now { name, canAccess, canEdit }; being listed at all grants
a read-only view. canAccess = connect a client at runtime; canEdit = change
the config. They no longer imply each other.
- The resolver returns { view, access, edit } (union of the user's own and role
entries): view = admin OR public OR listed; access = public OR a granting
entry (admins do NOT get access implicitly); edit = admin OR a granting entry.
- shareGlobal is the "public" flag: any authenticated user may view and use
(not edit) the server.
- The owner (creator) is auto-listed with full capabilities on save, so they
keep view/use/edit of their own server.
- The Studio API gates get on view, put/delete on edit; the runtime serving
endpoint requires access. The server list is filtered to viewable servers.
currentUserPermissions is now { canView, canAccess, canEdit }.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… page Re-document pre-registered clients on the embedded OAuth 2.1 server page, matching the restored public-only support: an onboarding option alongside DCR and CIMD, each entry a client_id with a redirect_uris allow-list, no secret / confidential / service_user and no Client Credentials grant (machine access uses the PAT authenticator). Adds the clients config-reference row and a note to prefer pre-registration over open DCR when clients are known up front. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Studio-API write path moved MCP server access to name-based, two-capability
grants, but the symfony-config `studio_mcp_servers` tree still typed identities
as integers — so a YAML-configured server with a string `owner` failed at
container compile with `Expected "int", but got "string"`, and shared_users /
shared_roles could not carry the {name, can_access, can_edit} grid at all.
Align the file-config `access` node with the settings-store shape the repository
feeds into McpServerAccess::fromArray:
- owner is a scalar username (was integerNode)
- shared_users / shared_roles are grants of { name, can_access, can_edit }, and a
bare string is accepted as a view-only grant (was integerPrototype)
Regression test processes the node and the full tree with a string owner and the
capability grid.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
wwidergoldpimcore
force-pushed
the
feature/mcp-1309-server-config-management
branch
from
September 2, 2026 09:54
5d4fe87 to
e529a61
Compare
Per the refined permission model (#1452), the owner is now symmetric with an admin: implicit Config Read + Config Edit, but MCP Server Access must be granted explicitly — so nobody, not even the owner or an admin, has default access to a server's runtime. - McpServerAccessResolver: the owner resolves to view + edit (not access), like an admin. Access stays public-or-explicit-entry only. - McpServerConfigurationService: stop seeding the owner into the sharing grid with full capabilities; the owner's read/edit is implicit, and they add themselves to the user list to grant access. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The MCP Server Management page still described the original two-level read/write sharing (owner with implicit write). Rewrite it to the current model: three independent capabilities — Config Read, Config Edit, MCP Server Access — where the owner and admins have implicit read+edit but must be granted access explicitly, and a public server grants read+access (not edit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bundles that expose their own OAuth-protected endpoints need to validate tokens and register their endpoints as protected resources, which @internal forbade. No behaviour change: docblocks only. Co-Authored-By: Claude <noreply@anthropic.com>
Adds an OAuth-Protected Applications page: the two roles, the public contracts, and a blueprint for adding an application, with Pimcore MCP and Data Hub Simple REST as the first two. Also corrects the MCP page, which claimed the authenticator validates the audience; it does not. Co-Authored-By: Claude <noreply@anthropic.com>
register() stored the caller's URI verbatim while the newly public docblock said the registry canonicalises, so the RFC 9728 document could echo a non-canonical resource. Unreadable key material now returns null instead of escaping as a 500. Corrects the blueprint doc, which described a firewall its stated reference implementation does not use. Co-Authored-By: Claude <noreply@anthropic.com>
…ible Implements RFC 8707: a client names the resource it wants a token for, the server validates it, stamps it as `aud` and refuses the token elsewhere. Without this any token opened every protected resource, which only became exploitable once a second one existed. Tokens with no audience stay valid everywhere, so clients that do not ask are unaffected. Scopes are now contributed by tagged providers, so a bundle can ship its own namespace instead of reusing `mcp:*`. Co-Authored-By: Claude <noreply@anthropic.com>
Enforcing `aud` exposed a mismatch: resources are registered as <issuer>/pimcore-mcp/studio/<slug> but the authenticator validated against <host>/pimcore-mcp, so a token a client correctly requested for the server it discovered was refused at that very server. Also stamps the canonical form of the resource, and makes a dynamic registration that omits `scope` deterministic rather than depending on bundle registration order. Co-Authored-By: Claude <noreply@anthropic.com>
The resource is now mandatory: a control the client can decline is not a control, and an unbound token was accepted by every protected resource of this server. Replaces two mirrored league methods with a resource column on the token record. completeAuthorizationRequest and BearerTokenResponse::generateHttpResponse were duplicated wholesale to smuggle one field through league's payloads; the binding is now written when the code and refresh token are issued and read back by id, so both mirrors are gone and the audience is queryable. Recovery also moves out of validateClient, which was the wrong layer. Co-Authored-By: Claude <noreply@anthropic.com>
fashxp
force-pushed
the
feature/oauth-multi-application
branch
from
September 2, 2026 13:09
0c8eca6 to
09d86e0
Compare
|
wwidergoldpimcore
force-pushed
the
feature/mcp-1309-server-config-management
branch
from
September 3, 2026 14:40
20053c3 to
969bbb2
Compare
This was referenced Sep 3, 2026
fashxp
marked this pull request as draft
September 3, 2026 21:33
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes in this pull request
Makes the embedded OAuth 2.1 server usable by more than one application, and closes the gap that created: with a second protected resource in play, an unbound token would have opened all of them.
1. Public contracts
Drops
@internalfrom the types a bundle needs to act as a resource server, and adds two for scopes.TokenValidatorInterfaceResolvedAccessResourceRegistryInterfaceProtectedResource/ProtectedResourceMetadataScopeProviderInterface/ScopeRegistryInterfaceCanonicalUristays internal: the registry and the validator canonicalise on both sides, so callers never need it.2. Audience binding (RFC 8707), with
resourcerequiredA client names the resource it wants a token for; the server validates it against the registry, refuses an unknown one, stamps the canonical form as
aud, andTokenValidatorInterface::validate()refuses a token whose audience does not name the resource it is presented at. The binding survives a refresh.resourceis mandatory. An authorization request that omits it is refused withinvalid_request. The alternative — accepting an audience-less token everywhere — leaves exactly the cross-application hole this PR exists to close, and there is no OAuth surface in the wild yet whose compatibility is worth preserving.The practical consequence is that clients which do not implement RFC 8707 stop working, and that is deliberate. Two were found and fixed on the application side rather than by relaxing the rule: Swagger UI (passes the resource via
additionalQueryStringParams) and MCP Inspector, which sendsresourceonly when it can discover protected-resource metadata from the endpoint it was given.Why now rather than later. It was deferred on the reasoning that per-application admission covers access control. That holds within an application; it does not hold between them. Once
data-hub-simple-restbecame a second resource, a token consented to for one application authenticated against the other as the same Pimcore user. Adding the second resource is what made the pre-existing gap exploitable.3. The binding is carried in the token record, not in a forked league
An earlier revision mirrored two
league/oauth2-serverinternals to move the resource through the flow. Both are gone. A nullableresourcecolumn on the token record carries it instead:src/OAuth/Server/ResponseType/ResourceBearerTokenResponse.phpis deleted.LoopbackAuthCodeGrant::completeAuthorizationRequest()is reduced to a six-line hook that captures the resource and delegates toparent::, rather than reproducing league's body.The commit deletes 185 lines against 282 added (the additions are mostly the migration, the record plumbing and tests). The point is not the count but that no upstream body is reproduced any more, so the flow no longer breaks silently when league changes internally.
4. Scope catalogue
Scopes were hardcoded in four places. They now come from tagged
ScopeProviderInterfaceservices, so a bundle ships its own namespace (datahub:read) instead of reusingmcp:*, and discovery advertises the union. A dynamic registration that omitsscopenow yields no scope rather than "whichever provider loaded first", which depended on bundle registration order.5. A functional break this surfaced
Enforcing
audexposed a mismatch that was invisible while nothing compared it: MCP resources register as<issuer>/pimcore-mcp/studio/<slug>, but the authenticator validated against<host>/pimcore-mcp. A client that discovered a server and correctly requested a token for it was then refused at that very server. The authenticator now derives the per-server URI the same way registration does. Verified live: a token bound to…/studio/<slug>authenticates there, and the same token is refused at a different resource.6. Documentation
New OAuth-Protected Applications page: the authorization-server / resource-server split, the public contracts, the anatomy of an application, and a blueprint covering both integration shapes actually in use (a firewall, as the MCP servers do; an existing request pipeline, as Data Hub Simple REST does). The surrounding pages are reframed so OAuth reads as a platform capability rather than an MCP feature, and MCP server management states it is a configuration topic independent of how a caller authenticated.
Two corrections where docs promised more than the code delivered: the MCP page claimed the authenticator "validates the token's signature and audience" when it did not, and resource URIs were described as an isolation boundary before they were one. The new page has an explicit "what the platform does not do yet" section, which still names scope enforcement.
Known gaps, stated plainly
datahub:read) as the raw string, because the label map instudio-ui-bundlelists onlymcp:*.Verification
Unit suite green: 871 tests, 1967 assertions, including new coverage for resource validation, canonicalisation, and a request that names no resource.
Manually end-to-end against a running stack: the real authorization-code + PKCE flow, a token carrying
aud, accepted at its own resource and refused at another; an authorization request withoutresourcerefused withinvalid_request; and the binding intact across a refresh. Discovery verified through the 401 challenge and the metadata document, for the Studio MCP servers and for both Data Hub Simple REST surfaces.php-cs-fixer clean on the changed paths. PHPStan not run locally (no dev dependencies in the parent project); note the
static-analysisfailure on this PR is atDependencyInjection/Configuration.php:867, a file this branch does not touch, inherited from the base branch.