Add OIDC/OAuth SSO support (rebased from #719)#916
Add OIDC/OAuth SSO support (rebased from #719)#916reazndev wants to merge 8 commits intorybbit-io:masterfrom
Conversation
…urrent main - Resolved merge conflicts with main branch - Kept OIDC/genericOAuth features from original PR - Restored main's auth-utils, email, and schema changes - Built shared package dependency Based on work by @acvigue
|
@reazndev is attempting to deploy a commit to the goldflag's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughServer discovers enabled OIDC and social providers from env vars and exposes them via the config API; client consumes these configs to render dynamic OIDC/social auth buttons and invoke unified OAuth flows alongside gated internal email/password UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Client as Client (UI)
participant Server as Server / API
participant AuthClient as Auth Client
participant OIDC as OIDC Provider
Browser->>Client: mount / visit login
Client->>Server: GET /api/config (useConfigs)
Server->>Server: read env -> getOIDCProviders(), getSocialProviders(), INTERNAL_AUTHENTICATION_ENABLED
Server-->>Client: { enabledOIDCProviders, enabledSocialProviders, internalAuthEnabled }
Client->>Browser: render dynamic buttons (OIDC + social) and/or internal form based on configs
Browser->>Client: user clicks OIDC/social button
Client->>AuthClient: signIn.oauth2({ providerId, callbackURL, ... })
AuthClient->>OIDC: redirect / auth request
OIDC->>AuthClient: callback with tokens
AuthClient-->>Client: complete sign-in (session established)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
🧹 Nitpick comments (3)
client/src/components/auth/SocialButtons.tsx (1)
30-54: Narrow error types in catch blocks.
String(error)loses useful context and violates the specific-error guideline. Preferinstanceof Errorhandling with a fallback.♻️ Suggested error handling
- } catch (error) { - onError(String(error)); - } + } catch (error) { + if (error instanceof Error) { + onError(error.message); + } else { + onError("Unknown error"); + } + } } const handleSocialAuth = async (provider: string) => { try { await authClient.signIn.social({ @@ - } catch (error) { - onError(String(error)); - } + } catch (error) { + if (error instanceof Error) { + onError(error.message); + } else { + onError("Unknown error"); + } + } };As per coding guidelines: "Error handling: Use try/catch blocks with specific error types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/auth/SocialButtons.tsx` around lines 30 - 54, The catch blocks in handleOIDCAuth and handleSocialAuth convert errors with String(error) which discards type/context—update both to check if (error instanceof Error) and call onError(error.message) (or use error.stack if available), otherwise call onError(String(error)) as a fallback; ensure you import or reference Error only and preserve existing behavior for non-Error values.client/src/lib/configs.ts (1)
7-11: Add runtime validation for the new config fields.Since
/configis external data, please update the runtime validation to includeenabledOIDCProvidersandenabledSocialProvidersso malformed responses fail fast.♻️ Suggested update with Zod parsing
-import { useQuery } from "@tanstack/react-query"; +import { useQuery } from "@tanstack/react-query"; +import { z } from "zod"; import { authedFetch } from "../api/utils"; -interface Configs { - disableSignup: boolean; - mapboxToken: string; - enabledOIDCProviders: Array<{ - providerId: string; - name: string; - }>; - enabledSocialProviders: string[]; -} +const OidcProviderSchema = z.object({ + providerId: z.string(), + name: z.string(), +}); + +const ConfigsSchema = z.object({ + disableSignup: z.boolean(), + mapboxToken: z.string(), + enabledOIDCProviders: z.array(OidcProviderSchema), + enabledSocialProviders: z.array(z.string()), +}); + +type Configs = z.infer<typeof ConfigsSchema>; export function useConfigs() { const { data, isLoading, error } = useQuery<Configs>({ queryKey: ["configs"], - queryFn: () => authedFetch<Configs>("/config"), + queryFn: async () => ConfigsSchema.parse(await authedFetch<Configs>("/config")), });As per coding guidelines: "Use TypeScript strict mode throughout; use Zod for runtime validation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/lib/configs.ts` around lines 7 - 11, Add runtime Zod validation for the new config fields by updating the config schema and parse logic to include enabledOIDCProviders (array of objects with providerId:string and name:string) and enabledSocialProviders (string array). Locate the existing Zod schema (e.g., configSchema or similar) and extend it with z.array(z.object({ providerId: z.string(), name: z.string() })). Also update the runtime parsing/validation call (e.g., parseConfig, validateConfig, or wherever response from /config is consumed) to run the new schema.parse or safeParse so malformed responses throw or are rejected immediately. Ensure TypeScript types are inferred from the Zod schema so enabledOIDCProviders and enabledSocialProviders are available throughout the codebase.server/src/lib/const.ts (1)
501-502: Missing explicit return type annotation ongetOIDCProviders.The inline annotation on
providerscaptures the element shape but the function itself lacks an explicit return type, which weakens TypeScript's strict-mode guarantees at call sites. As per coding guidelines, TypeScript strict typing should be used throughout.♻️ Proposed refactor
-export const getOIDCProviders = () => { - const providers: Array<{ providerId: string; clientId: string; clientSecret: string; discoveryUrl: string; scopes: string[]; name: string }> = []; +type OIDCProviderConfig = { + providerId: string; + clientId: string; + clientSecret: string; + discoveryUrl: string; + scopes: string[]; + name: string; +}; + +export const getOIDCProviders = (): OIDCProviderConfig[] => { + const providers: OIDCProviderConfig[] = [];As per coding guidelines: "Use TypeScript with strict typing throughout both client and server."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/lib/const.ts` around lines 501 - 502, The function getOIDCProviders is missing an explicit return type; update its signature to declare a strict return type (e.g., an array of the provider shape) rather than relying solely on the inline providers variable annotation—either add a named type/interface (e.g., OIDCProvider) and use getOIDCProviders(): OIDCProvider[] or annotate directly as getOIDCProviders(): Array<{ providerId: string; clientId: string; clientSecret: string; discoveryUrl: string; scopes: string[]; name: string }>[]; ensure the declared return type matches the providers variable and update any imports/types as needed so callers receive a fully typed return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/lib/auth.ts`:
- Around line 4-11: The hard-coded socialProviders array in the createAuthClient
call (authClient / createAuthClient) will drift from server config; replace it
with a small injectable approach: export a factory function (e.g.,
createAuthClientWithProviders) or an initializer that accepts a providers array
and uses that value for the socialProviders option, and update any import sites
to call the factory with the server-provided list (or read from a
bootstrap/global config injected at app startup) so the client list is driven
from configuration rather than a TODO and static array.
In `@server/src/lib/const.ts`:
- Around line 532-554: The loop validates discoveryUrl before confirming
required env vars, causing partially-configured providers to be silently skipped
and making the later "&& discoveryUrl" check redundant; change the order in the
loop so you first check required fields (clientId, clientSecret, discoveryUrl)
and log/warn when any are missing (use the providerId to identify which), then
attempt URL validation with new URL(discoveryUrl) inside try/catch and continue
on invalid URLs; finally remove the redundant "&& discoveryUrl" from the
providers.push condition and only rely on the validated
clientId/clientSecret/discoveryUrl checks before pushing to providers.
- Around line 501-557: Add an explicit return type to getOIDCProviders: define
an interface/type named OIDCProviderConfig matching the object shape ({
providerId: string; clientId: string; clientSecret: string; discoveryUrl:
string; scopes: string[]; name: string }) and annotate the function as
getOIDCProviders(): OIDCProviderConfig[]; update any exports if needed so the
type is available where required (e.g., getConfig.ts) and ensure the
implementation still returns that array shape.
---
Nitpick comments:
In `@client/src/components/auth/SocialButtons.tsx`:
- Around line 30-54: The catch blocks in handleOIDCAuth and handleSocialAuth
convert errors with String(error) which discards type/context—update both to
check if (error instanceof Error) and call onError(error.message) (or use
error.stack if available), otherwise call onError(String(error)) as a fallback;
ensure you import or reference Error only and preserve existing behavior for
non-Error values.
In `@client/src/lib/configs.ts`:
- Around line 7-11: Add runtime Zod validation for the new config fields by
updating the config schema and parse logic to include enabledOIDCProviders
(array of objects with providerId:string and name:string) and
enabledSocialProviders (string array). Locate the existing Zod schema (e.g.,
configSchema or similar) and extend it with z.array(z.object({ providerId:
z.string(), name: z.string() })). Also update the runtime parsing/validation
call (e.g., parseConfig, validateConfig, or wherever response from /config is
consumed) to run the new schema.parse or safeParse so malformed responses throw
or are rejected immediately. Ensure TypeScript types are inferred from the Zod
schema so enabledOIDCProviders and enabledSocialProviders are available
throughout the codebase.
In `@server/src/lib/const.ts`:
- Around line 501-502: The function getOIDCProviders is missing an explicit
return type; update its signature to declare a strict return type (e.g., an
array of the provider shape) rather than relying solely on the inline providers
variable annotation—either add a named type/interface (e.g., OIDCProvider) and
use getOIDCProviders(): OIDCProvider[] or annotate directly as
getOIDCProviders(): Array<{ providerId: string; clientId: string; clientSecret:
string; discoveryUrl: string; scopes: string[]; name: string }>[]; ensure the
declared return type matches the providers variable and update any imports/types
as needed so callers receive a fully typed return.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.mdclient/src/components/auth/SocialButtons.tsxclient/src/lib/auth.tsclient/src/lib/configs.tsdocker-compose.cloud.ymldocker-compose.ymlserver/src/api/getConfig.tsserver/src/lib/auth.tsserver/src/lib/const.ts
| //TODO: Load socialProviders from configs, but we can't use hooks here | ||
| export const authClient = createAuthClient({ | ||
| baseURL: process.env.NEXT_PUBLIC_BACKEND_URL, | ||
| plugins: [adminClient(), organizationClient(), emailOTPClient(), apiKeyClient()], | ||
| plugins: [adminClient(), organizationClient(), emailOTPClient(), apiKeyClient(), genericOAuthClient()], | ||
| fetchOptions: { | ||
| credentials: "include", | ||
| }, | ||
| socialProviders: ["google", "github", "twitter"], | ||
| socialProviders: ['google', 'github'], |
There was a problem hiding this comment.
Track the TODO for dynamic social provider loading.
Leaving this TODO can cause long-term drift between server-configured providers and the client list. Consider a follow-up that injects enabled social providers into this module (e.g., via a factory or config bootstrap).
Would you like me to propose a small refactor to wire the provider list without hooks?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/lib/auth.ts` around lines 4 - 11, The hard-coded socialProviders
array in the createAuthClient call (authClient / createAuthClient) will drift
from server config; replace it with a small injectable approach: export a
factory function (e.g., createAuthClientWithProviders) or an initializer that
accepts a providers array and uses that value for the socialProviders option,
and update any import sites to call the factory with the server-provided list
(or read from a bootstrap/global config injected at app startup) so the client
list is driven from configuration rather than a TODO and static array.
| export const getOIDCProviders = () => { | ||
| const providers: Array<{ providerId: string; clientId: string; clientSecret: string; discoveryUrl: string; scopes: string[]; name: string }> = []; | ||
| const oidcClientIdRegex = /^OIDC_([A-Z0-9_]+)_CLIENT_ID$/; | ||
| const oidcClientSecretRegex = /^OIDC_([A-Z0-9_]+)_CLIENT_SECRET$/; | ||
| const oidcDiscoveryUrlRegex = /^OIDC_([A-Z0-9_]+)_DISCOVERY_URL$/; | ||
| const oidcNameRegex = /^OIDC_([A-Z0-9_]+)_NAME$/; | ||
|
|
||
| const providerIds = new Set<string>(); | ||
|
|
||
| for (const key in process.env) { | ||
| const clientIdMatch = key.match(oidcClientIdRegex); | ||
| if (clientIdMatch && process.env[key]) { | ||
| providerIds.add(clientIdMatch[1]); | ||
| } | ||
|
|
||
| const clientSecretMatch = key.match(oidcClientSecretRegex); | ||
| if (clientSecretMatch && process.env[key]) { | ||
| providerIds.add(clientSecretMatch[1]); | ||
| } | ||
|
|
||
| const discoveryUrlMatch = key.match(oidcDiscoveryUrlRegex); | ||
| if (discoveryUrlMatch && process.env[key]) { | ||
| providerIds.add(discoveryUrlMatch[1]); | ||
| } | ||
|
|
||
| const nameMatch = key.match(oidcNameRegex); | ||
| if (nameMatch && process.env[key]) { | ||
| providerIds.add(nameMatch[1]); | ||
| } | ||
| } | ||
|
|
||
| for (const providerId of providerIds) { | ||
| const clientId = process.env[`OIDC_${providerId}_CLIENT_ID`]; | ||
| const clientSecret = process.env[`OIDC_${providerId}_CLIENT_SECRET`]; | ||
| const discoveryUrl = process.env[`OIDC_${providerId}_DISCOVERY_URL`]; | ||
| const name = process.env[`OIDC_${providerId}_NAME`] || `SSO (${providerId.toLowerCase()})`; | ||
|
|
||
| try { | ||
| new URL(discoveryUrl || ""); | ||
| } catch { | ||
| continue; // Skip invalid URLs | ||
| } | ||
|
|
||
| if (clientId && clientSecret && discoveryUrl) { | ||
| providers.push({ | ||
| providerId: providerId.toLowerCase(), | ||
| clientId, | ||
| clientSecret, | ||
| discoveryUrl, | ||
| scopes: ["openid", "profile", "email"], | ||
| name | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return providers; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "getConfig.ts" -o -name "getConfig.js"Repository: rybbit-io/rybbit
Length of output: 89
🏁 Script executed:
grep -r "getOIDCProviders" --include="*.ts" --include="*.tsx" -lRepository: rybbit-io/rybbit
Length of output: 134
🏁 Script executed:
grep -r "from.*better-auth" --include="*.ts" --include="*.tsx" -A 2 | head -30Repository: rybbit-io/rybbit
Length of output: 1405
🏁 Script executed:
cat server/src/api/getConfig.tsRepository: rybbit-io/rybbit
Length of output: 869
🏁 Script executed:
# Check the SocialProviders import validity and the const.ts file structure
head -20 server/src/lib/const.tsRepository: rybbit-io/rybbit
Length of output: 857
🏁 Script executed:
# Verify the return type of getOIDCProviders is annotated
grep -A 2 "export const getOIDCProviders" server/src/lib/const.tsRepository: rybbit-io/rybbit
Length of output: 309
🏁 Script executed:
# Double-check that getSocialProviders() is also properly used
grep -A 10 "export const getSocialProviders" server/src/lib/const.tsRepository: rybbit-io/rybbit
Length of output: 488
Add explicit return type annotation to getOIDCProviders() per TypeScript strict typing guidelines.
getOIDCProviders() lacks an explicit return type. It should be annotated as (): OIDCProviderConfig[] (after defining the type) to comply with strict TypeScript requirements.
Note: The code in getConfig.ts already properly sanitizes the response by mapping the result to only { providerId, name }, so clientSecret is never exposed to the client. The current implementation is secure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/lib/const.ts` around lines 501 - 557, Add an explicit return type
to getOIDCProviders: define an interface/type named OIDCProviderConfig matching
the object shape ({ providerId: string; clientId: string; clientSecret: string;
discoveryUrl: string; scopes: string[]; name: string }) and annotate the
function as getOIDCProviders(): OIDCProviderConfig[]; update any exports if
needed so the type is available where required (e.g., getConfig.ts) and ensure
the implementation still returns that array shape.
| for (const providerId of providerIds) { | ||
| const clientId = process.env[`OIDC_${providerId}_CLIENT_ID`]; | ||
| const clientSecret = process.env[`OIDC_${providerId}_CLIENT_SECRET`]; | ||
| const discoveryUrl = process.env[`OIDC_${providerId}_DISCOVERY_URL`]; | ||
| const name = process.env[`OIDC_${providerId}_NAME`] || `SSO (${providerId.toLowerCase()})`; | ||
|
|
||
| try { | ||
| new URL(discoveryUrl || ""); | ||
| } catch { | ||
| continue; // Skip invalid URLs | ||
| } | ||
|
|
||
| if (clientId && clientSecret && discoveryUrl) { | ||
| providers.push({ | ||
| providerId: providerId.toLowerCase(), | ||
| clientId, | ||
| clientSecret, | ||
| discoveryUrl, | ||
| scopes: ["openid", "profile", "email"], | ||
| name | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
URL validation fires before required-field check, causing silent provider drops; guard at line 544 has a redundant discoveryUrl check.
Two related issues:
- A partially-configured provider (e.g., only
OIDC_FOO_NAMEset) is silently dropped becausenew URL("" )throws and thecontinueruns before any diagnostic. Operators get zero feedback when misconfigured. - After the
try/catch,discoveryUrlis guaranteed to be a non-empty, valid URL string (any falsy value already triggeredcontinue). The&& discoveryUrlterm in theifat line 544 is therefore unreachable dead code.
🔧 Proposed fix: guard first, warn on incomplete config
for (const providerId of providerIds) {
const clientId = process.env[`OIDC_${providerId}_CLIENT_ID`];
const clientSecret = process.env[`OIDC_${providerId}_CLIENT_SECRET`];
const discoveryUrl = process.env[`OIDC_${providerId}_DISCOVERY_URL`];
const name = process.env[`OIDC_${providerId}_NAME`] || `SSO (${providerId.toLowerCase()})`;
+ if (!clientId || !clientSecret || !discoveryUrl) {
+ console.warn(`[OIDC] Provider "${providerId}" is missing required env vars; skipping.`);
+ continue;
+ }
+
try {
new URL(discoveryUrl || "");
} catch {
+ console.warn(`[OIDC] Provider "${providerId}" has an invalid DISCOVERY_URL; skipping.`);
continue; // Skip invalid URLs
}
- if (clientId && clientSecret && discoveryUrl) {
+ {
providers.push({
providerId: providerId.toLowerCase(),
clientId,
clientSecret,
- discoveryUrl,
+ discoveryUrl, // already validated above
scopes: ["openid", "profile", "email"],
name
});
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/lib/const.ts` around lines 532 - 554, The loop validates
discoveryUrl before confirming required env vars, causing partially-configured
providers to be silently skipped and making the later "&& discoveryUrl" check
redundant; change the order in the loop so you first check required fields
(clientId, clientSecret, discoveryUrl) and log/warn when any are missing (use
the providerId to identify which), then attempt URL validation with new
URL(discoveryUrl) inside try/catch and continue on invalid URLs; finally remove
the redundant "&& discoveryUrl" from the providers.push condition and only rely
on the validated clientId/clientSecret/discoveryUrl checks before pushing to
providers.
There was a problem hiding this comment.
Pull request overview
This pull request adds OpenID Connect (OIDC) and OAuth single sign-on (SSO) support for self-hosted instances, rebased from PR #719. It enables dynamic configuration of multiple authentication providers through environment variables and updates both backend and frontend to support these providers alongside existing social authentication.
Changes:
- Added OIDC provider discovery and configuration through environment variables (OIDC_{PROVIDER}_*)
- Introduced INTERNAL_AUTHENTICATION_ENABLED flag to optionally disable email/password authentication
- Updated UI to dynamically display available OIDC and social authentication providers based on server configuration
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/lib/const.ts | Adds getOIDCProviders() function to dynamically discover OIDC providers from environment variables and getSocialProviders() to conditionally configure Google/GitHub OAuth |
| server/src/lib/auth.ts | Integrates genericOAuth plugin, makes emailOTP conditional based on INTERNAL_AUTHENTICATION_ENABLED flag, and uses dynamic social provider configuration |
| server/src/api/getConfig.ts | Exposes enabled OIDC and social providers to the client via config API endpoint |
| docker-compose.yml | Documents OIDC provider environment variable configuration format in comments |
| docker-compose.cloud.yml | Documents OIDC provider environment variable configuration format in comments |
| client/src/lib/configs.ts | Adds TypeScript interfaces for OIDC and social provider configurations returned from server |
| client/src/lib/auth.ts | Adds genericOAuthClient plugin and updates hardcoded social provider list |
| client/src/components/auth/SocialButtons.tsx | Refactors to dynamically render OIDC and social provider buttons based on server configuration instead of IS_CLOUD check |
| README.md | Updates feature comparison table to indicate SSO/OpenID Connect support |
Comments suppressed due to low confidence (2)
client/src/components/auth/SocialButtons.tsx:84
- The "Or continue with email" divider is always displayed even when internal authentication (email/password login) is disabled via INTERNAL_AUTHENTICATION_ENABLED. This can be confusing to users when only SSO/OAuth providers are available. Consider adding INTERNAL_AUTHENTICATION_ENABLED to the config API response and conditionally rendering this divider only when internal authentication is enabled.
<div className="relative flex items-center text-xs uppercase">
<div className="flex-1 border-t border-neutral-200 dark:border-neutral-800" />
<span className="px-3 text-muted-foreground">{t("Or continue with email")}</span>
<div className="flex-1 border-t border-neutral-200 dark:border-neutral-800" />
</div>
client/src/components/auth/SocialButtons.tsx:47
- Inconsistent callbackURL handling between OIDC and social authentication. For OIDC (line 34), callbackURL is excluded when mode is "signup", but for social auth (line 47), callbackURL is always included regardless of mode. This inconsistency could lead to different redirect behaviors. Both should handle callbackURL the same way, or the difference should be documented with a comment explaining why they differ.
...(callbackURL && mode !== "signup" ? { callbackURL } : {}),
// For signup flow, new users should be redirected to the same callbackURL
...(mode === "signup" && callbackURL ? { newUserCallbackURL: callbackURL } : {}),
});
} catch (error) {
onError(String(error));
}
}
const handleSocialAuth = async (provider: string) => {
try {
await authClient.signIn.social({
provider,
...(callbackURL ? { callbackURL } : {}),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # - OIDC_{PROVIDER}_NAME="SSO Provider" | ||
| # - OIDC_{PROVIDER}_CLIENT_ID=${OIDC_PROVIDER_CLIENT_ID} | ||
| # - OIDC_{PROVIDER}_CLIENT_SECRET=${OIDC_PROVIDER_CLIENT_SECRET} | ||
| # - OIDC_{PROVIDER}_DISCOVERY_URL=${OIDC_PROVIDER_DISCOVERY_URL} # e.g. https://accounts.google.com/.well-known/openid-configuration |
There was a problem hiding this comment.
The OIDC configuration comments are missing documentation for the INTERNAL_AUTHENTICATION_ENABLED environment variable, which controls whether email/password authentication is available. This is an important configuration option when using SSO-only authentication. Consider adding a comment like "# - INTERNAL_AUTHENTICATION_ENABLED=false # Set to false to disable email/password login and use SSO only" to help users understand how to configure SSO-only authentication.
| # - OIDC_{PROVIDER}_DISCOVERY_URL=${OIDC_PROVIDER_DISCOVERY_URL} # e.g. https://accounts.google.com/.well-known/openid-configuration | |
| # - OIDC_{PROVIDER}_DISCOVERY_URL=${OIDC_PROVIDER_DISCOVERY_URL} # e.g. https://accounts.google.com/.well-known/openid-configuration | |
| # - INTERNAL_AUTHENTICATION_ENABLED=false # Set to false to disable email/password login and use SSO only |
| // OIDC back-redirect will only work if backend and frontend are running on the same port. | ||
| // For development, it will redirect back to the backend, and you will manually need to go back to the frontend. | ||
| // | ||
| // This is fine, because there's really no need for yet another seperate env variable for frontend URL just for dev. |
There was a problem hiding this comment.
Spelling error: "seperate" should be "separate".
| // This is fine, because there's really no need for yet another seperate env variable for frontend URL just for dev. | |
| // This is fine, because there's really no need for yet another separate env variable for frontend URL just for dev. |
| credentials: "include", | ||
| }, | ||
| socialProviders: ["google", "github", "twitter"], | ||
| socialProviders: ['google', 'github'], |
There was a problem hiding this comment.
The socialProviders array is hardcoded to ['google', 'github'] even though the server dynamically determines which providers are available based on environment variables. This could cause issues if a self-hosted instance doesn't configure Google or GitHub credentials but configures other social providers. The TODO comment acknowledges this limitation, but it creates a mismatch between what the client expects and what the server provides. Consider moving the authClient creation into a provider component where configs can be accessed, or pass social providers as a configuration parameter that can be set at runtime.
| {t("Continue with GitHub")} | ||
| </Button> | ||
| {configs?.enabledOIDCProviders.map((provider) => ( | ||
| <Button key={provider.providerId} type="button" onClick={() => handleOIDCAuth(provider.providerId)}> |
There was a problem hiding this comment.
The OIDC provider buttons are missing the height class (h-11) that was present in the original social buttons, which could lead to inconsistent button sizing. Add className="h-11" to maintain consistent button heights across all authentication options.
| <Button key={provider.providerId} type="button" onClick={() => handleOIDCAuth(provider.providerId)}> | |
| <Button | |
| key={provider.providerId} | |
| type="button" | |
| className="h-11" | |
| onClick={() => handleOIDCAuth(provider.providerId)} | |
| > |
| <Button type="button" onClick={() => handleSocialAuth("google")}> | ||
| <SiGoogle /> | ||
| </Button> | ||
| )} | ||
|
|
||
| {configs?.enabledSocialProviders.includes("github") && ( | ||
| <Button type="button" onClick={() => handleSocialAuth("github")}> | ||
| <SiGithub /> | ||
| GitHub | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
The social provider buttons (Google and GitHub) are missing the height class (h-11) that was present in the original implementation, which could lead to inconsistent button sizing. Add className="h-11" to maintain consistent button heights across all authentication options.
| # - OIDC_{PROVIDER}_NAME="SSO Provider" | ||
| # - OIDC_{PROVIDER}_CLIENT_ID=${OIDC_PROVIDER_CLIENT_ID} | ||
| # - OIDC_{PROVIDER}_CLIENT_SECRET=${OIDC_PROVIDER_CLIENT_SECRET} | ||
| # - OIDC_{PROVIDER}_DISCOVERY_URL=${OIDC_PROVIDER_DISCOVERY_URL} # e.g. https://accounts.google.com/.well-known/openid-configuration |
There was a problem hiding this comment.
The OIDC configuration comments are missing documentation for the INTERNAL_AUTHENTICATION_ENABLED environment variable, which controls whether email/password authentication is available. This is an important configuration option when using SSO-only authentication. Consider adding a comment like "# - INTERNAL_AUTHENTICATION_ENABLED=false # Set to false to disable email/password login and use SSO only" to help users understand how to configure SSO-only authentication.
| # - OIDC_{PROVIDER}_DISCOVERY_URL=${OIDC_PROVIDER_DISCOVERY_URL} # e.g. https://accounts.google.com/.well-known/openid-configuration | |
| # - OIDC_{PROVIDER}_DISCOVERY_URL=${OIDC_PROVIDER_DISCOVERY_URL} # e.g. https://accounts.google.com/.well-known/openid-configuration | |
| # - INTERNAL_AUTHENTICATION_ENABLED=false # Set to false to disable email/password login and use SSO only |
| for (const providerId of providerIds) { | ||
| const clientId = process.env[`OIDC_${providerId}_CLIENT_ID`]; | ||
| const clientSecret = process.env[`OIDC_${providerId}_CLIENT_SECRET`]; | ||
| const discoveryUrl = process.env[`OIDC_${providerId}_DISCOVERY_URL`]; | ||
| const name = process.env[`OIDC_${providerId}_NAME`] || `SSO (${providerId.toLowerCase()})`; | ||
|
|
||
| try { | ||
| new URL(discoveryUrl || ""); | ||
| } catch { | ||
| continue; // Skip invalid URLs | ||
| } | ||
|
|
||
| if (clientId && clientSecret && discoveryUrl) { | ||
| providers.push({ | ||
| providerId: providerId.toLowerCase(), | ||
| clientId, | ||
| clientSecret, | ||
| discoveryUrl, | ||
| scopes: ["openid", "profile", "email"], | ||
| name | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The getOIDCProviders function does not validate that all required environment variables are present before adding a provider to the list. If clientId, clientSecret, or discoveryUrl is missing (after the URL validation passes), a provider with undefined values could be added. While line 544 checks if all three exist, a provider could be partially configured if one variable is set but others are missing. Consider adding explicit validation and logging warnings when providers are skipped due to incomplete configuration to aid debugging.
| credentials: "include", | ||
| }, | ||
| socialProviders: ["google", "github", "twitter"], | ||
| socialProviders: ['google', 'github'], |
There was a problem hiding this comment.
Inconsistent quote style: The array uses single quotes while the rest of the codebase consistently uses double quotes for strings. Change ['google', 'github'] to ["google", "github"] to maintain consistency.
| socialProviders: ['google', 'github'], | |
| socialProviders: ["google", "github"], |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/invitation/components/login.tsx`:
- Line 26: The code access configs?.enabledOIDCProviders.length can throw if
enabledOIDCProviders is nullish; change both occurrences to safely handle
null/undefined (e.g. use optional chaining or a fallback array) by replacing
configs?.enabledOIDCProviders.length with (configs?.enabledOIDCProviders ??
[]).length or configs?.enabledOIDCProviders?.length, ensuring you update both
the check at the top of the Login component and the repeated check around the
later conditional so enabledOIDCProviders is never dereferenced when
null/undefined.
- Around line 95-101: The SSO button currently hardcodes isLoading={false} and
uses a raw string; add a dedicated SSO loading state (e.g., const [isSSOLoading,
setIsSSOLoading] = useState(false)), update handleSSOLogin to
setIsSSOLoading(true) at start and setIsSSOLoading(false) on any early
returns/errors (or allow redirect to proceed without resetting), and pass
isLoading={isSSOLoading} and disabled={isSSOLoading} to <AuthButton>; replace
the raw "Login with SSO" string with t('auth.loginWithSSO') using
useTranslations(), and add that key to all messages/*.json translation files.
- Around line 25-37: Remove the redundant SSO flow: delete the handleSSOLogin
function (which calls authClient.signIn.oauth2 and sets setError) and remove the
"Login with SSO" button JSX so that SocialButtons handles OIDC providers; ensure
SocialButtons (which maps over enabledOIDCProviders) remains imported and
rendered, and remove any now-unused references to handleSSOLogin or the SSO
button text while keeping existing authClient and setError usages elsewhere
intact.
| const handleSSOLogin = async () => { | ||
| if (!configs?.enabledOIDCProviders.length) return; | ||
|
|
||
| const provider = configs.enabledOIDCProviders[0]; | ||
| try { | ||
| await authClient.signIn.oauth2({ | ||
| providerId: provider.providerId, | ||
| callbackURL, | ||
| }); | ||
| } catch (err) { | ||
| setError(String(err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the login.tsx file to understand the full context
cat -n client/src/app/invitation/components/login.tsx | head -120Repository: rybbit-io/rybbit
Length of output: 3757
🏁 Script executed:
# Search for how SocialButtons renders multiple buttons per provider
rg -n "SocialButtons\|providerId\|enabledOIDCProviders" --type=ts --type=tsx -B 2 -A 5 client/Repository: rybbit-io/rybbit
Length of output: 87
🏁 Script executed:
# Check the PR or commit message to understand the claimed feature
git log --oneline --all -n 50 2>/dev/null | head -20
git show --stat 2>/dev/null | head -100Repository: rybbit-io/rybbit
Length of output: 7829
🏁 Script executed:
# Find and examine the SocialButtons component
find client -name "*SocialButtons*" -o -name "*social*" | head -20Repository: rybbit-io/rybbit
Length of output: 104
🏁 Script executed:
# Search for SocialButtons implementation
rg -l "SocialButtons" client/ --type tsRepository: rybbit-io/rybbit
Length of output: 260
🏁 Script executed:
# Check the auth components directory
ls -la client/components/auth/ 2>/dev/null || echo "Not found, searching..."
fd -t f "SocialButtons" client/Repository: rybbit-io/rybbit
Length of output: 128
🏁 Script executed:
# Search for how providers are rendered or selected
rg "enabledOIDCProviders|SocialButtons" --type ts -B 3 -A 8 client/Repository: rybbit-io/rybbit
Length of output: 13437
🏁 Script executed:
# Check git commit message for PR 719 to see claimed features
git log --grep="OIDC\|oauth\|provider" --oneline -n 20 2>/dev/null || git log --all --oneline | grep -i "oidc\|oauth\|provider" | head -10Repository: rybbit-io/rybbit
Length of output: 42
🏁 Script executed:
# Get full SocialButtons implementation
cat -n client/src/components/auth/SocialButtons.tsxRepository: rybbit-io/rybbit
Length of output: 3482
🏁 Script executed:
# Check if invitation login.tsx already has SocialButtons rendering on line 71
sed -n '60,110p' client/src/app/invitation/components/login.tsxRepository: rybbit-io/rybbit
Length of output: 1305
Remove the redundant handleSSOLogin and "Login with SSO" button — SocialButtons already renders all OIDC providers correctly
The SocialButtons component (imported on line 11 and rendered on line 71) already properly handles multiple OIDC providers by mapping over enabledOIDCProviders and rendering one button per provider with the provider's name (see SocialButtons.tsx:59-64).
The separate handleSSOLogin function (lines 25-37) and its associated button (lines 93-102) are redundant—they only use the first provider and create an inconsistent experience. Delete handleSSOLogin and the "Login with SSO" button; let SocialButtons handle all OIDC authentication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/invitation/components/login.tsx` around lines 25 - 37, Remove
the redundant SSO flow: delete the handleSSOLogin function (which calls
authClient.signIn.oauth2 and sets setError) and remove the "Login with SSO"
button JSX so that SocialButtons handles OIDC providers; ensure SocialButtons
(which maps over enabledOIDCProviders) remains imported and rendered, and remove
any now-unused references to handleSSOLogin or the SSO button text while keeping
existing authClient and setError usages elsewhere intact.
| const { configs } = useConfigs(); | ||
|
|
||
| const handleSSOLogin = async () => { | ||
| if (!configs?.enabledOIDCProviders.length) return; |
There was a problem hiding this comment.
Unsafe property access — enabledOIDCProviders.length can throw if the property is nullish
configs?.enabledOIDCProviders.length short-circuits to undefined when configs itself is nullish, but if configs is defined while enabledOIDCProviders is null or undefined (e.g. during an in-flight config fetch or a partial response), the unguarded .length access throws a TypeError. The same pattern is repeated on line 93.
🛡️ Proposed fix
- if (!configs?.enabledOIDCProviders.length) return;
+ if (!configs?.enabledOIDCProviders?.length) return;- {configs?.enabledOIDCProviders.length ? (
+ {configs?.enabledOIDCProviders?.length ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/invitation/components/login.tsx` at line 26, The code access
configs?.enabledOIDCProviders.length can throw if enabledOIDCProviders is
nullish; change both occurrences to safely handle null/undefined (e.g. use
optional chaining or a fallback array) by replacing
configs?.enabledOIDCProviders.length with (configs?.enabledOIDCProviders ??
[]).length or configs?.enabledOIDCProviders?.length, ensuring you update both
the check at the top of the Login component and the repeated check around the
later conditional so enabledOIDCProviders is never dereferenced when
null/undefined.
| isLoading={false} | ||
| type="button" | ||
| variant="default" | ||
| onClick={handleSSOLogin} | ||
| > | ||
| Login with SSO | ||
| </AuthButton> |
There was a problem hiding this comment.
Missing SSO-specific loading state and hardcoded UI string violates i18n
Two issues in the SSO button:
isLoading={false}is hardcoded —handleSSOLoginisasyncand the OAuth2 redirect can take a noticeable moment. There is no loading feedback to the user, and nothing prevents double-clicks."Login with SSO"is a raw English string that bypassest(), violating the project's i18n requirement.
As per coding guidelines: "Use next-intl's 'useTranslations()' hook for i18n".
♻️ Proposed fix
Add a dedicated loading state for SSO:
const [isLoading, setIsLoading] = useState(false);
+ const [isSSOLoading, setIsSSOLoading] = useState(false);Update handleSSOLogin:
const handleSSOLogin = async () => {
if (!configs?.enabledOIDCProviders?.length) return;
const provider = configs.enabledOIDCProviders[0];
+ setIsSSOLoading(true);
try {
await authClient.signIn.oauth2({
providerId: provider.providerId,
callbackURL,
});
} catch (err) {
setError(String(err));
+ } finally {
+ setIsSSOLoading(false);
}
};Update the button:
- <AuthButton
- isLoading={false}
- type="button"
- variant="default"
- onClick={handleSSOLogin}
- >
- Login with SSO
- </AuthButton>
+ <AuthButton
+ isLoading={isSSOLoading}
+ type="button"
+ variant="default"
+ onClick={handleSSOLogin}
+ >
+ {t("Login with SSO")}
+ </AuthButton>Remember to add the key to all translation files (messages/en.json, messages/de.json, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/invitation/components/login.tsx` around lines 95 - 101, The
SSO button currently hardcodes isLoading={false} and uses a raw string; add a
dedicated SSO loading state (e.g., const [isSSOLoading, setIsSSOLoading] =
useState(false)), update handleSSOLogin to setIsSSOLoading(true) at start and
setIsSSOLoading(false) on any early returns/errors (or allow redirect to proceed
without resetting), and pass isLoading={isSSOLoading} and
disabled={isSSOLoading} to <AuthButton>; replace the raw "Login with SSO" string
with t('auth.loginWithSSO') using useTranslations(), and add that key to all
messages/*.json translation files.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/signup/page.tsx (1)
196-251:⚠️ Potential issue | 🟡 MinorNo actionable UI when
internalAuthEnabledisfalseand no providers are configuredWhen
configs.internalAuthEnabledisfalseandSocialButtonsrenders nothing (zero OIDC and social providers), step 1 displays only the "Already have an account?" link with no way to proceed. Consider a fallback message, or guard this page-level to surface a clear configuration error.💡 Suggested guard
+ {!configs?.internalAuthEnabled && + !configs?.enabledOIDCProviders?.length && + !configs?.enabledSocialProviders?.length && ( + <p className="text-sm text-muted-foreground text-center"> + {t("No authentication methods are currently configured.")} + </p> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/signup/page.tsx` around lines 196 - 251, The page can become unusable when configs.internalAuthEnabled is false and SocialButtons renders no providers; update the signup page render logic to detect "no auth methods available" (check configs.internalAuthEnabled and whether SocialButtons would render any providers/OIDC) and show a clear fallback UI or error message instead of the empty form area; locate the relevant JSX around SocialButtons, configs?.internalAuthEnabled, and the "Already have an account?" link and either render a configuration warning/CTA (e.g., "No authentication providers configured — contact admin" with a disabled Continue) or redirect/guard the route so users aren’t stuck on step 1.
♻️ Duplicate comments (3)
client/src/app/invitation/components/login.tsx (3)
97-106: HardcodedisLoading={false}and untranslated"Login with SSO"string
handleSSOLoginisasyncbut its button never reflects a loading state, allowing double-clicks."Login with SSO"bypassest(), violating the project's i18n requirement.As per coding guidelines: "Use next-intl's 'useTranslations()' hook for i18n".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/invitation/components/login.tsx` around lines 97 - 106, The SSO button uses a hardcoded isLoading={false} and a hardcoded label; update the AuthButton rendering that checks configs?.enabledOIDCProviders to track the async state of handleSSOLogin and use next-intl translations: add a local loading boolean (e.g., ssoLoading) that is set true before await in handleSSOLogin and false in finally, pass isLoading={ssoLoading} to the AuthButton, and replace the literal "Login with SSO" with t('loginWithSSO') (using useTranslations() at component top) so the label is localized.
25-37: RedundanthandleSSOLogin—SocialButtonsalready handles all OIDC providers
SocialButtons(imported at line 11, rendered at line 71) already maps overenabledOIDCProvidersand renders one button per provider.handleSSOLoginhard-codes only the first provider, so when multiple providers exist, the first one gets two buttons (one fromSocialButtonsand one from the explicit SSO button).Delete
handleSSOLogin(lines 25–37) and the correspondingAuthButtonblock (lines 97–106) entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/invitation/components/login.tsx` around lines 25 - 37, The handleSSOLogin function duplicates SocialButtons by hardcoding the first OIDC provider and causing duplicate buttons; remove the handleSSOLogin declaration and also remove the AuthButton usage that invokes it (the AuthButton block rendering the SSO button) so SocialButtons is the sole source of provider buttons; search for the handleSSOLogin identifier and the AuthButton rendering that references it and delete both to avoid the duplicate-first-provider behavior.
26-26: UnsafeenabledOIDCProviders.length— missing optional chain on the array
configs?.enabledOIDCProviders.lengthshort-circuitsconfigstoundefinedwhen nullish, but ifconfigsis defined whileenabledOIDCProvidersisundefined(absent from the API response), the unguarded.lengththrows aTypeError. Same pattern on line 97.- if (!configs?.enabledOIDCProviders.length) return; + if (!configs?.enabledOIDCProviders?.length) return;- {configs?.enabledOIDCProviders.length ? ( + {configs?.enabledOIDCProviders?.length ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/invitation/components/login.tsx` at line 26, The code reads configs?.enabledOIDCProviders.length which will throw if configs exists but enabledOIDCProviders is undefined; change these checks to safely handle a missing array by using either optional chaining on the array (e.g., configs?.enabledOIDCProviders?.length) or an explicit array check (e.g., Array.isArray(configs.enabledOIDCProviders) && configs.enabledOIDCProviders.length) wherever you reference enabledOIDCProviders (the configs variable and enabledOIDCProviders identifier appear in this file, including the second occurrence around the later conditional).
🧹 Nitpick comments (1)
client/src/app/login/page.tsx (1)
77-87: React 19 Strict Mode fires this effect twice in developmentReact 19 Strict Mode simulates mount→unmount→remount for every
useEffect. BecauseauthClient.signIn.oauth2()initiates a browser-level OAuth redirect (external navigation), a double-invocation will open the provider's authorization page twice in development, which is disorienting. Add a ref guard or anexecutedvariable to prevent the second call:🛡️ Proposed guard
+ const redirectInitiated = useRef(false); useEffect(() => { if ( !isLoadingConfigs && configs && !configs.internalAuthEnabled && - configs.enabledOIDCProviders?.length === 1 + configs.enabledOIDCProviders?.length === 1 && + !redirectInitiated.current ) { + redirectInitiated.current = true; const provider = configs.enabledOIDCProviders[0]; authClient.signIn.oauth2({ providerId: provider.providerId, callbackURL: "/" }) .catch(err => setError(String(err))); } }, [configs, isLoadingConfigs]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/login/page.tsx` around lines 77 - 87, The useEffect in page.tsx triggers authClient.signIn.oauth2() twice under React 19 Strict Mode; add a persistent ref guard (e.g., executedRef) or an `executed` variable to short-circuit the effect after the first invocation so the redirect is only initiated once. Update the effect that checks isLoadingConfigs/configs/internalAuthEnabled/enabledOIDCProviders and, before calling authClient.signIn.oauth2({ providerId, callbackURL: "/" }), check and set executedRef.current (or equivalent) so subsequent mount/remounts skip the oauth2 call; keep the existing catch handler that calls setError(String(err)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/login/page.tsx`:
- Around line 77-87: Guard the unsafe access to configs.enabledOIDCProviders by
checking configs.enabledOIDCProviders && configs.enabledOIDCProviders.length (or
use optional chaining configs.enabledOIDCProviders?.length) inside the useEffect
before reading .length to avoid a TypeError; add a new state flag (e.g.,
isAutoRedirecting) that you set to true immediately before calling
authClient.signIn.oauth2(...) and set to false in the catch handler (or finally)
so the UI can show a loading/spinner; update the component render to display a
spinner and "Redirecting to SSO…" message when isAutoRedirecting is true so
users see feedback during the redirect.
In `@client/src/lib/configs.ts`:
- Around line 8-12: Make enabledOIDCProviders and enabledSocialProviders
optional in the configs type (change their declarations to be optional) and
update all consumers to guard against undefined by using double
optional-chaining or nullish coalescing; specifically adjust usages in
login/page.tsx and invitation/components/login.tsx (where .length is accessed)
to use configs?.enabledOIDCProviders?.length or (configs?.enabledOIDCProviders
?? []).length and similarly for enabledSocialProviders so runtime undefined
responses from the HTTP fetch no longer throw.
---
Outside diff comments:
In `@client/src/app/signup/page.tsx`:
- Around line 196-251: The page can become unusable when
configs.internalAuthEnabled is false and SocialButtons renders no providers;
update the signup page render logic to detect "no auth methods available" (check
configs.internalAuthEnabled and whether SocialButtons would render any
providers/OIDC) and show a clear fallback UI or error message instead of the
empty form area; locate the relevant JSX around SocialButtons,
configs?.internalAuthEnabled, and the "Already have an account?" link and either
render a configuration warning/CTA (e.g., "No authentication providers
configured — contact admin" with a disabled Continue) or redirect/guard the
route so users aren’t stuck on step 1.
---
Duplicate comments:
In `@client/src/app/invitation/components/login.tsx`:
- Around line 97-106: The SSO button uses a hardcoded isLoading={false} and a
hardcoded label; update the AuthButton rendering that checks
configs?.enabledOIDCProviders to track the async state of handleSSOLogin and use
next-intl translations: add a local loading boolean (e.g., ssoLoading) that is
set true before await in handleSSOLogin and false in finally, pass
isLoading={ssoLoading} to the AuthButton, and replace the literal "Login with
SSO" with t('loginWithSSO') (using useTranslations() at component top) so the
label is localized.
- Around line 25-37: The handleSSOLogin function duplicates SocialButtons by
hardcoding the first OIDC provider and causing duplicate buttons; remove the
handleSSOLogin declaration and also remove the AuthButton usage that invokes it
(the AuthButton block rendering the SSO button) so SocialButtons is the sole
source of provider buttons; search for the handleSSOLogin identifier and the
AuthButton rendering that references it and delete both to avoid the
duplicate-first-provider behavior.
- Line 26: The code reads configs?.enabledOIDCProviders.length which will throw
if configs exists but enabledOIDCProviders is undefined; change these checks to
safely handle a missing array by using either optional chaining on the array
(e.g., configs?.enabledOIDCProviders?.length) or an explicit array check (e.g.,
Array.isArray(configs.enabledOIDCProviders) &&
configs.enabledOIDCProviders.length) wherever you reference enabledOIDCProviders
(the configs variable and enabledOIDCProviders identifier appear in this file,
including the second occurrence around the later conditional).
---
Nitpick comments:
In `@client/src/app/login/page.tsx`:
- Around line 77-87: The useEffect in page.tsx triggers
authClient.signIn.oauth2() twice under React 19 Strict Mode; add a persistent
ref guard (e.g., executedRef) or an `executed` variable to short-circuit the
effect after the first invocation so the redirect is only initiated once. Update
the effect that checks
isLoadingConfigs/configs/internalAuthEnabled/enabledOIDCProviders and, before
calling authClient.signIn.oauth2({ providerId, callbackURL: "/" }), check and
set executedRef.current (or equivalent) so subsequent mount/remounts skip the
oauth2 call; keep the existing catch handler that calls setError(String(err)).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/src/app/invitation/components/login.tsxclient/src/app/login/page.tsxclient/src/app/signup/page.tsxclient/src/lib/configs.tsserver/src/api/getConfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/api/getConfig.ts
| useEffect(() => { | ||
| if (!isLoadingConfigs && configs && !configs.internalAuthEnabled && configs.enabledOIDCProviders.length === 1) { | ||
| const provider = configs.enabledOIDCProviders[0]; | ||
| authClient.signIn.oauth2({ | ||
| providerId: provider.providerId, | ||
| callbackURL: "/", | ||
| }).catch(err => { | ||
| setError(String(err)); | ||
| }); | ||
| } | ||
| }, [configs, isLoadingConfigs]); |
There was a problem hiding this comment.
Unsafe enabledOIDCProviders.length access and missing loading indicator in auto-redirect effect
Two issues in this useEffect:
-
Potential
TypeError(line 78): Even thoughconfigsis confirmed truthy,configs.enabledOIDCProviderscan beundefinedif the server omits the field (see theConfigsinterface — the field is declared required but is API-sourced). The unguarded.lengthwill throw at runtime in that case. -
No loading/spinner state during redirect: Once the condition is met,
authClient.signIn.oauth2()is called silently. The user sees the otherwise-empty login page (onlySocialButtonsand the signup link are visible) with no indication that a redirect is in progress. This creates a confusing UX gap.
🛡️ Proposed fix
+ const [isAutoRedirecting, setIsAutoRedirecting] = useState(false);
useEffect(() => {
- if (!isLoadingConfigs && configs && !configs.internalAuthEnabled && configs.enabledOIDCProviders.length === 1) {
+ if (!isLoadingConfigs && configs && !configs.internalAuthEnabled && configs.enabledOIDCProviders?.length === 1) {
const provider = configs.enabledOIDCProviders[0];
+ setIsAutoRedirecting(true);
authClient.signIn.oauth2({
providerId: provider.providerId,
callbackURL: "/",
}).catch(err => {
setError(String(err));
+ setIsAutoRedirecting(false);
});
}
}, [configs, isLoadingConfigs]);Then render a loading indicator when isAutoRedirecting is true (e.g., replace the form area with a spinner and a "Redirecting to SSO…" message).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!isLoadingConfigs && configs && !configs.internalAuthEnabled && configs.enabledOIDCProviders.length === 1) { | |
| const provider = configs.enabledOIDCProviders[0]; | |
| authClient.signIn.oauth2({ | |
| providerId: provider.providerId, | |
| callbackURL: "/", | |
| }).catch(err => { | |
| setError(String(err)); | |
| }); | |
| } | |
| }, [configs, isLoadingConfigs]); | |
| const [isAutoRedirecting, setIsAutoRedirecting] = useState(false); | |
| useEffect(() => { | |
| if (!isLoadingConfigs && configs && !configs.internalAuthEnabled && configs.enabledOIDCProviders?.length === 1) { | |
| const provider = configs.enabledOIDCProviders[0]; | |
| setIsAutoRedirecting(true); | |
| authClient.signIn.oauth2({ | |
| providerId: provider.providerId, | |
| callbackURL: "/", | |
| }).catch(err => { | |
| setError(String(err)); | |
| setIsAutoRedirecting(false); | |
| }); | |
| } | |
| }, [configs, isLoadingConfigs]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/login/page.tsx` around lines 77 - 87, Guard the unsafe access
to configs.enabledOIDCProviders by checking configs.enabledOIDCProviders &&
configs.enabledOIDCProviders.length (or use optional chaining
configs.enabledOIDCProviders?.length) inside the useEffect before reading
.length to avoid a TypeError; add a new state flag (e.g., isAutoRedirecting)
that you set to true immediately before calling authClient.signIn.oauth2(...)
and set to false in the catch handler (or finally) so the UI can show a
loading/spinner; update the component render to display a spinner and
"Redirecting to SSO…" message when isAutoRedirecting is true so users see
feedback during the redirect.
| enabledOIDCProviders: Array<{ | ||
| providerId: string; | ||
| name: string; | ||
| }>; | ||
| enabledSocialProviders: string[]; |
There was a problem hiding this comment.
enabledOIDCProviders and enabledSocialProviders should be optional to match the API contract
Both are declared as required non-nullable fields, but they are populated from an HTTP fetch. Any response that omits these fields (e.g., older server, partial response, network truncation) will cause configs.enabledOIDCProviders to be undefined at runtime while TypeScript believes it is a defined array. Consumers in login/page.tsx (line 78) and invitation/components/login.tsx (lines 26, 97) then call .length on the field without the second optional-chain (enabledOIDCProviders?.length), which will throw a TypeError.
🛡️ Proposed fix
interface Configs {
disableSignup: boolean;
internalAuthEnabled: boolean;
mapboxToken: string;
- enabledOIDCProviders: Array<{
+ enabledOIDCProviders?: Array<{
providerId: string;
name: string;
}>;
- enabledSocialProviders: string[];
+ enabledSocialProviders?: string[];
}All consumers must then use configs?.enabledOIDCProviders?.length (double optional-chain) or a nullish-coalescing fallback.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enabledOIDCProviders: Array<{ | |
| providerId: string; | |
| name: string; | |
| }>; | |
| enabledSocialProviders: string[]; | |
| interface Configs { | |
| disableSignup: boolean; | |
| internalAuthEnabled: boolean; | |
| mapboxToken: string; | |
| enabledOIDCProviders?: Array<{ | |
| providerId: string; | |
| name: string; | |
| }>; | |
| enabledSocialProviders?: string[]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/lib/configs.ts` around lines 8 - 12, Make enabledOIDCProviders and
enabledSocialProviders optional in the configs type (change their declarations
to be optional) and update all consumers to guard against undefined by using
double optional-chaining or nullish coalescing; specifically adjust usages in
login/page.tsx and invitation/components/login.tsx (where .length is accessed)
to use configs?.enabledOIDCProviders?.length or (configs?.enabledOIDCProviders
?? []).length and similarly for enabledSocialProviders so runtime undefined
responses from the HTTP fetch no longer throw.
|
thank you for doing this, it looks great. I will close my PR here in a bit, as it seems there's a common theme of no one ever reviewing anything. |
The maintainer said in one GH issue that he does not plan on adding oauth2 to Rybbit so I doubt it would ever get approved so I also didn't care for writing any docs that would be needed for a merge. |
Summary
Based on #719 by @acvigue, updated for compatibility with current main.
Changes from original PR
INTERNAL_AUTHENTICATION_ENABLED)Configuration