fix: resolve OIDC endpoints via discovery instead of hardcoded paths - #86
Merged
Conversation
The hub and MCP middleware hardcoded the userinfo URL to
{issuer}/oauth/v2/userinfo, which 404s on current Zitadel (the
discovery document advertises /oidc/v1/userinfo). The failure was
silently swallowed, so username/email enrichment never happened and
users were persisted with empty username/email forever.
- shared/auth/oidc: add FetchDiscovery/ResolveEndpoints reading
jwks_uri and userinfo_endpoint from the OIDC discovery document,
with fallback to the legacy Zitadel paths so a transient discovery
failure at startup does not block boot
- hub wire.go + mcp main.go: resolve endpoints via discovery, log a
warning when discovery fails; hub supports OIDC_JWKS_URL and
OIDC_USERINFO_URL overrides
- middleware: log a rate-limited warning when userinfo enrichment
fails instead of swallowing the error
6 tasks
Owner
Author
|
@review-agent please have a look |
There was a problem hiding this comment.
Review Summary
Overall: Approve
Clean, targeted fix with good fallback behaviour and adequate test coverage.
Blockers
None.
Suggestions
shared/auth/oidc/discovery.go:43— Observation:FetchDiscoveryvalidates thatjwks_uriis present but does not requireuserinfo_endpoint. If a provider omits it in an otherwise valid discovery document, the hub will pass an emptyUserInfoURLto the middleware and silently skip userinfo enrichment. Suggestion: consider logging a warning whenuserinfo_endpointis missing so operators know enrichment is disabled.shared/auth/oidc/discovery.go:64— The fallback paths (/oauth/v2/keys,/oauth/v2/userinfo) are Zitadel-legacy constants. A brief comment on thefallbackJWKSPath/fallbackUserInfoPathconstants noting they are Zitadel-specific legacy defaults would help future readers.services/mcp/internal/auth/middleware.go— The empty-string fallback logic duplicates the fallback already handled byResolveEndpoints. That is fine for resilience, but be aware it creates two places to update if the legacy paths ever change.
Nits
services/hub/cmd/hub/wire.gouses the importedslogpackage directly, whileservices/mcp/cmd/server/main.gouses alogalias (log "log/slog"). Picking one convention would be slightly nicer.
What's Done Well
- Self-contained
shared/auth/oidcdiscovery package with clear fallback semantics. - Discovery failure does not block boot — good operational choice.
- Rate-limited warning replaces the previously silent swallowing of userinfo errors.
- Env overrides (
OIDC_JWKS_URL,OIDC_USERINFO_URL) are useful escape hatches. - Unit tests cover success, 404, invalid JSON, missing
jwks_uri, and fallback paths.
Testing
New discovery_test.go covers the discovery happy path and several error cases. CI (hub + MCP) is passing.
|
@DominikPinsel LGTM — approved. |
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.
Problem
Users logging in via OIDC are persisted with empty
usernameandemail(verified in theuserstable). Root cause: the hub and MCP middleware hardcoded the userinfo URL to{issuer}/oauth/v2/userinfo, which 404s on current Zitadel — the discovery document advertisesuserinfo_endpoint: /oidc/v1/userinfo.The failure was silently swallowed (
// If userinfo fails, continue with JWT-only user data), and since Zitadel access JWTs don't carrypreferred_username/name/emailclaims, enrichment never happened andUpsertUser(sub, "", "")never wrote anything.Fix
shared/auth/oidc: newFetchDiscovery/ResolveEndpoints— readjwks_urianduserinfo_endpointfrom{issuer}/.well-known/openid-configuration. Works with any compliant IdP (Zitadel, Keycloak, Entra ID, Authelia, Okta), not just Zitadel. Falls back to the legacy Zitadel paths if discovery is unreachable at startup (with a warning), so boot is not blocked.services/hub/cmd/hub/wire.goandservices/mcp: resolve endpoints via discovery at startup; log when discovery fails. Hub supportsOIDC_JWKS_URL/OIDC_USERINFO_URLenv overrides as escape hatches.middleware.go: userinfo enrichment failures now log a rate-limited warning (max once per 5 min) instead of being swallowed.Notes
services/mcp/internal/auth/proxy.goalready used the correct/oidc/v1/userinfo; its other/oauth/v2/*paths match the current Zitadel discovery document and are left unchanged.Verification
GET /oauth/v2/userinfo→ 404,GET /oidc/v1/userinfo→ 401;userstable shows empty username/email for all OIDC-created users.go build,go vet,golangci-lint runclean forshared/auth/oidc,services/hub,services/mcp; fullgo test ./...passes for all three.