feat(oidc): infer dynamic OIDC issuer parameters#200
Conversation
halvaradop
added
the
oauth
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughOIDC provider issuers can now include ChangesOIDC dynamic issuer parameters
Sequence Diagram(s)sequenceDiagram
participant defineOpenIDProviderConfig
participant setDynamicParams
participant resolveOpenIDProvider
participant discoveryMetadata
participant fetch
defineOpenIDProviderConfig->>setDynamicParams: resolve config.issuer placeholders
resolveOpenIDProvider->>setDynamicParams: resolve provider.oidc.issuer placeholders
resolveOpenIDProvider->>discoveryMetadata: request discovery metadata
discoveryMetadata->>fetch: GET /.well-known/openid-configuration
fetch-->>discoveryMetadata: JSON metadata
discoveryMetadata-->>resolveOpenIDProvider: issuer metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/core/test/actions/oidc/resolve-provider.test.ts (1)
73-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit assertions.
This test only awaits
resolveOpenIDProvider(placeholder)and asserts nothing; it passes as long as nothing throws. The dynamic-param substitution is validated only indirectly via the discovery issuer-match. Assert the substituted URL was used (e.g.expect(fetchMock).toHaveBeenCalledWith("https://app.com/issuer/1/apps/2/.well-known/openid-configuration", ...)) and/or the resolvedoidc.issuer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/actions/oidc/resolve-provider.test.ts` around lines 73 - 93, The dynamic-params OIDC test in resolveOpenIDProvider does not assert anything, so add explicit expectations to verify the placeholder substitution actually happened. Use fetchMock and/or the resolved provider from resolveOpenIDProvider(placeholder) to assert the discovery URL was called with https://app.com/issuer/1/apps/2/.well-known/openid-configuration and that the resolved issuer matches the substituted value.packages/core/src/actions/oidc/resolve-provider.ts (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-resolving an already-resolved issuer here is fragile.
provideris aRuntimeOAuthProviderplaceholder whoseoidc.issuerwas already substituted increateOpenIDPlaceholder, and the runtime provider does not carry the original:paramkeys (teamId/appId). This call is a no-op only because the issuer is already concrete; if any unresolved:paramever reached here, it would throwOIDC_INVALID_ISSUER_PARAMSsince the values aren't present onprovider. Consider resolving issuer params in exactly one place to avoid relying on idempotency. Related to the double-resolution noted inpackages/core/src/oauth/index.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/actions/oidc/resolve-provider.ts` around lines 20 - 24, The issuer in resolveProvider is being resolved twice, which is fragile because the RuntimeOAuthProvider no longer contains the original dynamic param keys. Update the flow so issuer params are resolved in exactly one place, and in this function just read and validate provider.oidc.issuer without calling setDynamicParams again. Keep the existing missing-issuer check and make sure the resolution logic is centralized with createOpenIDPlaceholder and the related OAuth provider handling in oauth/index.ts.packages/core/src/oauth/index.ts (1)
104-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant re-resolution and mutation of the caller's
config.Line 110 mutates the input
config.issuerin place (an observable side effect on the caller's object), andcreateOpenIDPlaceholder(line 111) already callssetDynamicParams(config.issuer, config)internally, so substitution runs twice. The second pass is a no-op only because resolution happens to be idempotent on an already-resolved URL. Prefer resolving once without mutating the argument.♻️ Suggested change
- const envConfig = !config.clientId || !config.clientSecret ? defineOAuthEnvironment(config.id) : undefined - config.issuer = setDynamicParams(config.issuer, config) - return createOpenIDPlaceholder(config, { + const envConfig = !config.clientId || !config.clientSecret ? defineOAuthEnvironment(config.id) : undefined + return createOpenIDPlaceholder(config, { clientId: config.clientId || envConfig!.clientId, clientSecret: config.clientSecret || envConfig!.clientSecret, })(
createOpenIDPlaceholderalready appliessetDynamicParamstoconfig.issuer.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/oauth/index.ts` around lines 104 - 114, The OpenID provider setup in defineOpenIDProviderConfig is resolving the issuer twice and mutating the caller’s config object in place. Remove the direct assignment to config.issuer, and instead pass an untouched config through to createOpenIDPlaceholder, which already applies setDynamicParams to config.issuer internally. Keep the clientId/clientSecret fallback logic using envConfig, but avoid any observable side effects on the input OpenIDProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/oauth/index.ts`:
- Around line 94-102: The issuer templating logic is incorrectly treating URL
authority ports as placeholder parameters, causing valid issuer URLs with
explicit ports to fail. Update setDynamicParams() so it only substitutes real
template segments and does not match the host:port portion of an issuer URL,
then adjust defineOpenIDProviderConfig() and createOpenIDPlaceholder() to work
from a derived issuer value instead of mutating config.issuer and resolving it
twice.
In `@packages/core/src/shared/errors.ts`:
- Around line 802-808: The OIDC_INVALID_ISSUER_PARAMS error definition in
errors.ts currently has empty message and userMessage fields, which causes blank
output through the AuraAuthError flow. Update that error entry with a clear
internal message and a user-facing message, keeping the existing OIDC validation
mapping and 502 statusCode intact so callers like AuraAuthError receive
meaningful content.
---
Nitpick comments:
In `@packages/core/src/actions/oidc/resolve-provider.ts`:
- Around line 20-24: The issuer in resolveProvider is being resolved twice,
which is fragile because the RuntimeOAuthProvider no longer contains the
original dynamic param keys. Update the flow so issuer params are resolved in
exactly one place, and in this function just read and validate
provider.oidc.issuer without calling setDynamicParams again. Keep the existing
missing-issuer check and make sure the resolution logic is centralized with
createOpenIDPlaceholder and the related OAuth provider handling in
oauth/index.ts.
In `@packages/core/src/oauth/index.ts`:
- Around line 104-114: The OpenID provider setup in defineOpenIDProviderConfig
is resolving the issuer twice and mutating the caller’s config object in place.
Remove the direct assignment to config.issuer, and instead pass an untouched
config through to createOpenIDPlaceholder, which already applies
setDynamicParams to config.issuer internally. Keep the clientId/clientSecret
fallback logic using envConfig, but avoid any observable side effects on the
input OpenIDProvider.
In `@packages/core/test/actions/oidc/resolve-provider.test.ts`:
- Around line 73-93: The dynamic-params OIDC test in resolveOpenIDProvider does
not assert anything, so add explicit expectations to verify the placeholder
substitution actually happened. Use fetchMock and/or the resolved provider from
resolveOpenIDProvider(placeholder) to assert the discovery URL was called with
https://app.com/issuer/1/apps/2/.well-known/openid-configuration and that the
resolved issuer matches the substituted value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59c444d0-fd6b-44e8-a361-5e3b5dd39517
📒 Files selected for processing (8)
packages/core/CHANGELOG.mdpackages/core/src/@types/oidc.tspackages/core/src/actions/oidc/resolve-provider.tspackages/core/src/oauth/index.tspackages/core/src/shared/errors.tspackages/core/test/actions/oidc/discovery.test.tspackages/core/test/actions/oidc/resolve-provider.test.tspackages/core/test/oauth.test.ts
halvaradop
changed the title
1 participant
Description
This pull request adds TypeScript inference for dynamic parameters in OpenID Connect (OIDC) provider issuers.
Any issuer URL containing dynamic segments (path or host segments prefixed with
:) is now automatically analyzed, and the corresponding parameters become required when configuring the provider. This improves type safety by ensuring that all dynamic issuer parameters are provided at compile time.Usage
@coderabbitai ignore