Repository navigation
feat(op): configurable response and grant types - #1009
choksi2212 wants to merge 2 commits into
Conversation
Add Config.SupportedResponseTypes and Config.SupportedGrantTypes so a self-hosted provider can publish metadata that matches its deployment policy. Previously the discovery document always advertised code, id_token and id_token token response types, and there was no way to refuse an id_token response type at the provider level, even when every registered client had it disabled. Both fields are optional: when unset, the defaults reproduce the behavior of previous versions. The authorization endpoint and the discovery document read the same configuration, so the advertised capabilities and the enforced ones cannot drift. The authorization code grant cannot be turned off: OpenID Connect Core section 2 requires an OP to support it and the provider callback is built around it. Closes zitadel#924 Signed-off-by: Manas Ajaykumar Choksi <choksi2212@users.noreply.github.com> Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Decorated providers can bypass response restrictions, and some configurations advertise capabilities inconsistently or accept unsupported response values.
3 open findings
What changed in this PR
Adds provider-level configuration for advertised and accepted response/grant types, addressing #924.
Changes:
- Adds configurable response and grant type policies.
- Enforces policies across authorization and token paths.
- Adds discovery and endpoint integration tests.
| File | Description |
|---|---|
pkg/op/server_legacy.go |
Enforces policies in legacy server paths. |
pkg/op/op.go |
Adds configuration fields and capability checks. |
pkg/op/discovery.go |
Generates configurable discovery metadata. |
pkg/op/discovery_config_test.go |
Tests configuration behavior. |
pkg/op/auth_request.go |
Validates provider-supported response types. |
pkg/oidc/error.go |
Adds unsupported_response_type. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| responseTypes := DefaultSupportedResponseTypes | ||
| if provider, ok := c.(*Provider); ok { | ||
| responseTypes = provider.SupportedResponseTypes() | ||
| } | ||
| strings := make([]string, len(responseTypes)) | ||
| for i, responseType := range responseTypes { | ||
| strings[i] = string(responseType) | ||
| } | ||
| return strings | ||
| } | ||
|
|
||
| // SupportedResponseType reports whether the provider accepts the response type | ||
| // at the authorization endpoint and advertises it in its discovery document. | ||
| // Both read the same [Provider.SupportedResponseTypes], so the two cannot | ||
| // drift. Configurations that do not implement the method, or did not set | ||
| // [Config.SupportedResponseTypes], get the defaults. | ||
| func SupportedResponseType(c Configuration, responseType oidc.ResponseType) bool { | ||
| provider, ok := c.(*Provider) | ||
| if !ok { | ||
| return slices.Contains(DefaultSupportedResponseTypes, responseType) | ||
| } | ||
| return slices.Contains(provider.SupportedResponseTypes(), responseType) |
| if o.config.SupportedResponseTypes == nil { | ||
| return slices.Clone(DefaultSupportedResponseTypes) | ||
| } | ||
| return slices.Clone(o.config.SupportedResponseTypes) |
| // the response types the authorization endpoint accepts. Discovery keeps | ||
| // advertising it, because OIDC Core requires it; the PR body documents this. |
Copilot review follow-up. Discovery and the authorization-endpoint gate asserted *Provider, so an OpenIDProvider decorator that embeds *Provider (e.g. to add an AuthorizeValidator) fell through to the defaults: its operator configuration was silently dropped, and the identical assertion in SupportedResponseType allowed response types disabled in the embedded provider. - ResponseTypes, SupportedResponseType, GrantTypes and grantTypeConfigured now assert optional method interfaces (SupportedResponseTypes / ConfiguredGrantTypes) that both *Provider and any wrapper embedding it satisfy through promotion. - Provider.ConfiguredGrantTypes exposes the operator grant list for that assertion. Also fail fast on operator response-type lists containing a value the authorization endpoint cannot actually serve (anything other than code, id_token, id_token token would fall through to the implicit token path and produce tokens under the wrong response type): NewProvider now rejects such lists with an error naming the offending value. And the TestAuthorizeRejectsResponseTypeCodeToo comment no longer claims discovery keeps advertising the code response type; the invariant that is actually preserved is the authorization_code grant. Tests: TestDiscoveryConfiguredThroughDecorator pins that a wrapping provider's configuration reaches both the gate and the discovery document, and TestNewProviderRejectsUnservableResponseTypes pins the construction-time refusal. Both fail on the previous commit (verified via stash) and pass here. Full pkg/... suite green except the pre-existing TestRoutes/TestServerRoutes expires_in timing flake, reproduced identically on pristine main. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
|
All three findings addressed in 4cbc187:
|
|
Hi, gentle nudge on this one as well — it has been open for about five days now with no review activity. Quick recap of the change: this closes #924 by making the response and grant types that a provider advertises configurable, instead of the previous fixed set. Highlights:
Happy to restructure any part of it if a different shape would make review easier — for example, splitting the decorator-bypass hardening into its own commit or PR if you would prefer to review that separately. |
Description
pkg/op/discovery.gohardcoded the provider's capabilities:ResponseTypes()always returnedcode,id_tokenandid_token token(with a// TODO: ok for now, check later if dynamic needed), and there was no provider-level way to refuse anid_tokenresponse type at the authorization endpoint. A self-hosted deployment that disables implicit and ID-token responses on every client still advertises them in its discovery document, and a client that is accidentally configured to allow them gets one served.This adds two optional
Configfields following the existingSupportedScopes/SupportedClaimsoverride idiom:SupportedResponseTypes []oidc.ResponseType— drives both the discovery document and a new provider-level check at the authorization endpoint.SupportedGrantTypes []oidc.GrantType— restricts which grants the token endpoint serves and the discovery document advertises; theGrantType*Supported()methods consult it, so all dispatch paths (legacyExchange, v2tokensHandler, and theLegacyServermethods) answer consistently.Both fields are optional: when unset, the behavior of previous versions is reproduced exactly (pinned by
TestDiscoveryResponseAndGrantTypesDefaults, and the byte-identical discovery row inTestRoutes).Closes #924
Behavior
When the operator restricts response types to
code:response_types_supported: ["code"]andgrant_types_supportedwithoutimplicit.response_type=id_tokenorid_token tokenis refused withunsupported_response_type(newoidc.ErrUnsupportedResponseType, the RFC 6749 §4.1.2.1 error code for exactly this), delivered to the client's redirect_uri withstate— even if the client allows it.Providerrouter and theop.Serverv2 path) enforce it, proven by driving both over the same provider inTestAuthorizeRejectsDisabledResponseType.TestAuthorizeAcceptsConfiguredResponseType).unsupported_grant_typeand absent from discovery (TestGrantTypesOverrideRejectsAtTokenEndpoint).The authorization code grant cannot be turned off: OIDC Core section 2 requires an OP to support it and the provider callback and token endpoint are built around it, so discovery always includes it. The list is still authoritative for what the authorization endpoint accepts, which
TestAuthorizeRejectsResponseTypeCodeToopins.The provider-level check runs after the client-level one, after the redirect_uri has been validated, so the refusal is delivered to the client rather than rendered at the endpoint, mirroring the order
ValidateAuthRequestClientestablishes. In theProviderpath it sits outside the validation closure, so a customAuthorizeValidatorcannot skip the deployment-wide policy.Reproduced
On pristine
main(5ac478e), a web client (which allows every response type) sendingresponse_type=id_token:There is no provider-level refusal point:
ValidateAuthReqResponseTypeonly checks the client, so the request is stored and the user is sent to login for an implicit flow the operator may have intended to disable. (Probe kept as the negative control for this PR; it hands the request to login on main and is refused withunsupported_response_typeon this branch.)Tests
TestDiscoveryResponseAndGrantTypesDefaults— defaults byte-identical to previous versions (also covered by the unchanged exact-JSON discovery row inTestRoutes).TestDiscoveryCodeOnly— code-only provider advertises exactly that;implicitgone from grants;authorization_codestill present.TestAuthorizeRejectsDisabledResponseType— id_token refused on both /authorize paths, delivered to redirect_uri with state.TestAuthorizeRejectsResponseTypeCodeToo— the list is authoritative even forcode.TestAuthorizeAcceptsConfiguredResponseType— listed response types still reach login end-to-end.TestGrantTypesOverrideRejectsAtTokenEndpoint— refresh_token grant removed via the list: refusedunsupported_grant_typeat the token endpoint, absent from discovery.Negative control: on pristine main (fresh worktree of 5ac478e),
TestAuthorizeRejectsDisabledResponseType/TestGrantTypesOverrideRejectsAtTokenEndpointcannot even be expressed — the fields do not exist — and the behavioral probe above shows the id_token request proceeding to login.Verification on this branch (Windows, go1.26):
go test ./pkg/...— all packages green except the pre-existingTestRoutes/TestServerRoutessubtestsToken_exchange,Client_credentials_exchange,refresh_token(andTestServerRoutes/JWT_authorization), which fail identically on pristine main in a fresh worktree on this machine (deterministic, 3/3 runs both sides;expires_in299-vs-300 wall-clock expectation in the test table).go vet ./pkg/...clean;gofmtclean on all touched files.go test ./pkg/op/ -race -count=3green on all new tests.