Conversation
…ded URLs Remove custom getUserInfo from okta(), auth0(), and keycloak() provider helpers. The hardcoded userinfo URLs caused failures when the provider's actual endpoint differed from the assumed path (e.g., Okta custom authorization servers duplicating the /oauth2/ segment). The genericOAuth framework already resolves userinfo_endpoint from the OIDC discovery document, making the custom implementations redundant. Closes #8179
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
@better-auth/api-key
better-auth
auth
@better-auth/core
@better-auth/drizzle-adapter
@better-auth/electron
@better-auth/expo
@better-auth/i18n
@better-auth/kysely-adapter
@better-auth/memory-adapter
@better-auth/mongo-adapter
@better-auth/oauth-provider
@better-auth/passkey
@better-auth/prisma-adapter
@better-auth/redis-storage
@better-auth/scim
@better-auth/sso
@better-auth/stripe
@better-auth/telemetry
@better-auth/test-utils
commit: |
There was a problem hiding this comment.
Pull request overview
This PR updates the generic-oauth provider helpers (Okta/Auth0/Keycloak) to stop hardcoding userinfo URLs and instead rely on the plugin’s existing OIDC discovery-based resolution of userinfo_endpoint, fixing incorrect URLs for Okta custom authorization servers (e.g., duplicated /oauth2/).
Changes:
- Removed provider-specific
getUserInfoimplementations fromokta(),auth0(), andkeycloak(). - Removed now-unused profile typings and fetch imports from those provider helper modules.
- Updated
generic-oauth.test.tsexpectations to reflect that these helpers no longer return agetUserInfofunction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/better-auth/src/plugins/generic-oauth/providers/okta.ts | Removes hardcoded userinfo fetch logic so userinfo is resolved via discovery. |
| packages/better-auth/src/plugins/generic-oauth/providers/auth0.ts | Removes hardcoded userinfo fetch logic so userinfo is resolved via discovery. |
| packages/better-auth/src/plugins/generic-oauth/providers/keycloak.ts | Removes hardcoded userinfo fetch logic so userinfo is resolved via discovery. |
| packages/better-auth/src/plugins/generic-oauth/generic-oauth.test.ts | Updates helper unit tests to assert getUserInfo is no longer provided by these helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Ensure issuer ends without trailing slash for proper discovery URL construction | ||
| const issuer = options.issuer.replace(/\/$/, ""); | ||
| const discoveryUrl = `${issuer}/.well-known/openid-configuration`; | ||
|
|
||
| const getUserInfo = async ( | ||
| tokens: OAuth2Tokens, | ||
| ): Promise<OAuth2UserInfo | null> => { | ||
| const userInfoUrl = `${issuer}/oauth2/v1/userinfo`; | ||
|
|
||
| const { data: profile, error } = await betterFetch<OktaProfile>( | ||
| userInfoUrl, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${tokens.accessToken}`, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (error || !profile) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| id: profile.sub, | ||
| name: profile.name ?? profile.preferred_username ?? undefined, | ||
| email: profile.email ?? undefined, | ||
| image: profile.picture, | ||
| emailVerified: profile.email_verified ?? false, | ||
| }; | ||
| }; | ||
|
|
||
| return { | ||
| providerId: "okta", | ||
| discoveryUrl, | ||
| clientId: options.clientId, | ||
| clientSecret: options.clientSecret, | ||
| scopes: options.scopes ?? defaultScopes, | ||
| redirectURI: options.redirectURI, | ||
| pkce: options.pkce, | ||
| disableImplicitSignUp: options.disableImplicitSignUp, | ||
| disableSignUp: options.disableSignUp, | ||
| overrideUserInfo: options.overrideUserInfo, | ||
| getUserInfo, | ||
| }; |
There was a problem hiding this comment.
By removing the provider-specific getUserInfo, this helper now relies entirely on the generic fallback. The current fallback (routes.getUserInfo) returns a non-null object even when the userinfo request fails (e.g., betterFetch returns error / no data), which can yield an empty id and misleading downstream errors. Consider updating the shared fallback to return null when betterFetch.error is set or data is missing, so failures are handled consistently (and so callers can surface a clear user_info_is_missing error).
| // Ensure domain doesn't have protocol prefix | ||
| const domain = options.domain.replace(/^https?:\/\//, ""); | ||
| const discoveryUrl = `https://${domain}/.well-known/openid-configuration`; | ||
|
|
||
| const getUserInfo = async ( | ||
| tokens: OAuth2Tokens, | ||
| ): Promise<OAuth2UserInfo | null> => { | ||
| const userInfoUrl = `https://${domain}/userinfo`; | ||
|
|
||
| const { data: profile, error } = await betterFetch<Auth0Profile>( | ||
| userInfoUrl, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${tokens.accessToken}`, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (error || !profile) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| id: profile.sub, | ||
| name: profile.name ?? profile.nickname ?? undefined, | ||
| email: profile.email ?? undefined, | ||
| image: profile.picture, | ||
| emailVerified: profile.email_verified ?? false, | ||
| }; | ||
| }; | ||
|
|
||
| return { | ||
| providerId: "auth0", | ||
| discoveryUrl, | ||
| clientId: options.clientId, | ||
| clientSecret: options.clientSecret, | ||
| scopes: options.scopes ?? defaultScopes, | ||
| redirectURI: options.redirectURI, | ||
| pkce: options.pkce, | ||
| disableImplicitSignUp: options.disableImplicitSignUp, | ||
| disableSignUp: options.disableSignUp, | ||
| overrideUserInfo: options.overrideUserInfo, | ||
| getUserInfo, | ||
| }; |
There was a problem hiding this comment.
Same as Okta: now that auth0() no longer provides a custom getUserInfo, any userinfo fetch failures will go through the generic fallback. That fallback currently doesn't check betterFetch.error / empty data and may return an object with missing fields instead of null, which changes error handling behavior. Recommend hardening the shared fallback to return null on fetch errors/missing payload.
| // Ensure issuer ends without trailing slash for proper discovery URL construction | ||
| const issuer = options.issuer.replace(/\/$/, ""); | ||
| const discoveryUrl = `${issuer}/.well-known/openid-configuration`; | ||
|
|
||
| const getUserInfo = async ( | ||
| tokens: OAuth2Tokens, | ||
| ): Promise<OAuth2UserInfo | null> => { | ||
| // Construct userinfo URL from issuer | ||
| const userInfoUrl = `${issuer}/protocol/openid-connect/userinfo`; | ||
|
|
||
| const { data: profile, error } = await betterFetch<KeycloakProfile>( | ||
| userInfoUrl, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${tokens.accessToken}`, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (error || !profile) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| id: profile.sub, | ||
| name: profile.name ?? profile.preferred_username ?? undefined, | ||
| email: profile.email ?? undefined, | ||
| image: profile.picture, | ||
| // Keycloak provides email_verified per OIDC standard, but availability depends on configuration. | ||
| // We default to false when not provided or not configured. | ||
| emailVerified: profile.email_verified ?? false, | ||
| }; | ||
| }; | ||
|
|
||
| return { | ||
| providerId: "keycloak", | ||
| discoveryUrl, | ||
| clientId: options.clientId, | ||
| clientSecret: options.clientSecret, | ||
| scopes: options.scopes ?? defaultScopes, | ||
| redirectURI: options.redirectURI, | ||
| pkce: options.pkce, | ||
| disableImplicitSignUp: options.disableImplicitSignUp, | ||
| disableSignUp: options.disableSignUp, | ||
| overrideUserInfo: options.overrideUserInfo, | ||
| getUserInfo, | ||
| }; |
There was a problem hiding this comment.
Now that keycloak() relies on the generic userinfo fallback, userinfo request errors/missing payload currently won't produce null (the fallback doesn't check betterFetch.error / missing data). This can result in empty IDs or misleading downstream errors. Suggest updating the shared fallback to return null on fetch error or empty response to keep behavior consistent with the removed provider-specific implementation.
| describe("Okta Provider Helper", () => { | ||
| it("should return correct GenericOAuthConfig", () => { | ||
| const oktaConfig = okta({ | ||
| clientId: "okta-client-id", | ||
| clientSecret: "okta-client-secret", | ||
| issuer: "https://dev-12345.okta.com/oauth2/default", | ||
| }); | ||
|
|
||
| expect(oktaConfig.providerId).toBe("okta"); | ||
| expect(oktaConfig.discoveryUrl).toBe( | ||
| "https://dev-12345.okta.com/oauth2/default/.well-known/openid-configuration", | ||
| ); | ||
| expect(oktaConfig.scopes).toEqual(["openid", "profile", "email"]); | ||
| expect(oktaConfig.clientId).toBe("okta-client-id"); | ||
| expect(oktaConfig.clientSecret).toBe("okta-client-secret"); | ||
| expect(oktaConfig.getUserInfo).toBeDefined(); | ||
| expect(typeof oktaConfig.getUserInfo).toBe("function"); | ||
| expect(oktaConfig.getUserInfo).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
These helper tests now assert getUserInfo is undefined, but there isn’t a regression test that the sign-in flow actually uses userinfo_endpoint from discovery for these providers (the core fix for #8179). Consider adding a test that stubs discovery to return a non-default userinfo_endpoint and asserts the userinfo request hits that URL (and not a hardcoded path), especially for Okta issuers that include /oauth2/<authServerId>.
Summary
getUserInfofromokta(),auth0(), andkeycloak()provider helpersuserinfo_endpointfrom the OIDC discovery document, making the custom implementations redundant and error-prone/oauth2/segment was duplicated (e.g.,https://xxx.okta.com/oauth2/default/oauth2/v1/userinfo)Closes #8179
Test plan
generic-oauth.test.tstests pass