feat(auth-oidc,dashboard,auth,js-sdk,types,medusa): generic OIDC auth provider#16023
feat(auth-oidc,dashboard,auth,js-sdk,types,medusa): generic OIDC auth provider#16023NicolasGorga wants to merge 7 commits into
Conversation
🦋 Changeset detectedLatest commit: 5f4d722 The changes in this PR will be included in the next version bump. This PR includes changesets to release 80 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
NicolasGorga
changed the title
| this.config_.callback_url, | ||
| ] | ||
|
|
||
| if (!allowedCallbackUrls.includes(callbackUrl)) { |
There was a problem hiding this comment.
Isn't this already checked by the IdP? I guess a double line of defense is fine, and we prevent the user from even being redirected to Google. But it also means two places to maintain the allowlist. I don'd really know what the best practice is, I think I've always relied on the IdP, but maybe that wasn't ideal.
There was a problem hiding this comment.
maybe the best would be to not check if this.config_.allowed_callback_urls is not defined, so it's up to the user to decide if they want the double check.
| const body: Record<string, string> = req.body ?? {} | ||
|
|
||
| if (query.error) { | ||
| this.logger_?.error( |
There was a problem hiding this comment.
I think we shouldn't log an error here. This is the IdP telling us something went wrong in the auth, e.g. the user canceled, or access_denied. Neither is our app's fault, so an error log is noisy. As a minimum, I'd downgrade the log level, but I think it would be better to return the actual error instead of GENERIC_AUTH_ERROR and have a higher layer log the error and return an opaque message to the user.
There was a problem hiding this comment.
agree to lowering the level, but I don't think we should completely remove it. Would be useful for debugging.
| </div> | ||
| ) : ( | ||
| isEmailPassInstalled && ( | ||
| <Form {...form}> |
There was a problem hiding this comment.
not part of this PR, but it would be nice to extract the emailpass form to a component. It's quite deep now, while the SsoLogin component, which is at the same conceptual level is already extracted.
There was a problem hiding this comment.
nit: a lot of the auth logic here, especially related to MFA, is similar to the cloud auth and I imagine any auth we add in the future. Would be good to make it reusable where possible.
| // Fallback: process-local cache when no cache module is available | ||
| const now = Date.now() | ||
| const cached = discoveryCache.get(this.options_.issuer) | ||
| if (cached && cached.expiresAt > now) { | ||
| return cached.metadata | ||
| } | ||
|
|
||
| const metadata = (await Issuer.discover(this.options_.issuer)).metadata | ||
|
|
||
| discoveryCache.set(this.options_.issuer, { | ||
| metadata, | ||
| expiresAt: now + this.discoveryCacheTtlMs_, | ||
| }) | ||
|
|
||
| return metadata |
There was a problem hiding this comment.
q: is this necessary? feels like an over-defensive mechanism. I don't think this is a consistent pattern across our modules, but correct me if I'm wrong.
| const isLocalhost = | ||
| url.hostname === "localhost" || | ||
| url.hostname === "127.0.0.1" || | ||
| url.hostname === "::1" || | ||
| url.hostname === "[::1]" | ||
|
|
||
| if ( | ||
| url.protocol !== "https:" && | ||
| !(url.protocol === "http:" && isLocalhost) | ||
| ) { | ||
| throw new MedusaError( | ||
| MedusaError.Types.INVALID_DATA, | ||
| `OIDC '${label}' must use https (http is only allowed for localhost)` | ||
| ) | ||
| } |
There was a problem hiding this comment.
nit: shouldn't we allow localhost only in development?
| const body: Record<string, string> = req.body ?? {} | ||
|
|
||
| if (query.error) { | ||
| this.logger_?.error( |
There was a problem hiding this comment.
agree to lowering the level, but I don't think we should completely remove it. Would be useful for debugging.
| private static assertSecureIssuer_(value: string): void { | ||
| let url: URL | ||
| try { | ||
| url = new URL(value) | ||
| } catch (e) { | ||
| throw new MedusaError( | ||
| MedusaError.Types.INVALID_DATA, | ||
| "OIDC 'issuer' must be a valid URL" | ||
| ) | ||
| } | ||
|
|
||
| const isLocalhost = | ||
| url.hostname === "localhost" || | ||
| url.hostname === "127.0.0.1" || | ||
| url.hostname === "::1" || | ||
| url.hostname === "[::1]" | ||
|
|
||
| if ( | ||
| url.protocol !== "https:" && | ||
| !(url.protocol === "http:" && isLocalhost) | ||
| ) { | ||
| throw new MedusaError( | ||
| MedusaError.Types.INVALID_DATA, | ||
| "OIDC 'issuer' must use https (http is only allowed for localhost)" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
this logic is repeated here and in the engine, would be better to reuse it from the engine
There was a problem hiding this comment.
thought: we need to document a warning that enabling some providers like Google or GitHub for admin users would technically allow any user to register with this route as an admin user.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
medusa-docs-ui | 5f4d722 | Commit Preview URL Branch Preview URL |
Jul 20 2026, 02:02 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
medusa-docs-cloud | 5f4d722 | Commit Preview URL Branch Preview URL |
Jul 20 2026, 02:01 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
medusa-docs-user-guide | 5f4d722 | Commit Preview URL Branch Preview URL |
Jul 20 2026, 02:01 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
medusa-docs-resources | 5f4d722 | Commit Preview URL Branch Preview URL |
Jul 20 2026, 02:06 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
medusa-docs-book | 5f4d722 | Commit Preview URL Branch Preview URL |
Jul 20 2026, 02:01 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
medusa-docs-api-reference | 5f4d722 | Commit Preview URL Branch Preview URL |
Jul 20 2026, 02:00 PM |
3 participants
Summary
What — What changes are introduced in this PR?
Please provide answer here
Why — Why are these changes relevant or necessary?
Please provide answer here
How — How have these changes been implemented?
Please provide answer here
Testing — How have these changes been tested, or how can the reviewer test the feature?
Please provide answer here
Examples
Provide examples or code snippets that demonstrate how this feature works, or how it can be used in practice.
This helps with documentation and ensures maintainers can quickly understand and verify the change.
// Example usageChecklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
Add any additional context, related issues, or references that might help the reviewer understand this PR.