Add OIDC/OAuth SSO support (rebased from #719)#916
Conversation
|
@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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughServer discovers OIDC and social providers from environment variables and exposes them plus an internal-auth toggle via /api/config; the client consumes these configs to render dynamic OIDC/social buttons, register generic OAuth clients, gate email/password UI, and support SSO sign-in/auto-redirect flows. 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: GET /login
Client->>Server: GET /api/config (useConfigs)
Server->>Server: read env -> getOIDCProviders(), getSocialProviders(), INTERNAL_AUTHENTICATION_ENABLED
Server-->>Client: { enabledOIDCProviders, enabledSocialProviders, internalAuthEnabled }
Client->>Browser: render UI (email/password OR SSO buttons) based on configs
Note right of Client: User selects SSO
Browser->>Client: click OIDC/social button
Client->>AuthClient: signIn.oauth2({ providerId, callbackURL, ... })
AuthClient->>OIDC: redirect to authorize
OIDC->>AuthClient: callback with tokens
AuthClient-->>Client: complete sign-in (session established)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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.
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 { 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. |
…iple languages, ensuring consistency in terminology for custom events and team management. Added missing translations for various terms and improved user experience by maintaining uniformity in the user interface language.
This commit adds the ability to disable email/password login and prioritize SSO/OIDC authentication methods for organizations.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/components/auth/SocialButtons.tsx (2)
81-85:⚠️ Potential issue | 🟡 Minor"Or continue with email" divider renders even when internal auth is disabled.
The divider is unconditionally rendered whenever there is at least one OIDC/social provider, but in
client/src/app/login/page.tsxandclient/src/app/signup/components/AccountStep.tsxthe email/password form is now gated onconfigs.internalAuthEnabled. When internal auth is disabled, users see an "Or continue with email" separator pointing to empty space below the SSO buttons.Consider gating the divider on
configs.internalAuthEnabled, or lifting it out ofSocialButtonsentirely and rendering it from the parent only when the email form will be shown.🤖 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 81 - 85, The "Or continue with email" divider inside SocialButtons is always rendered causing an empty separator when internal auth is disabled; update SocialButtons to only render that divider when internal auth will actually be shown—either read configs.internalAuthEnabled inside SocialButtons and gate the divider on that flag, or add a boolean prop (e.g., showEmailDivider) to SocialButtons and have the callers in login/page.tsx and AccountStep.tsx pass configs.internalAuthEnabled so the divider is only rendered when internal auth is enabled.
31-55:⚠️ Potential issue | 🟡 MinorInconsistent
callbackURLhandling between OIDC and social flows.
handleOIDCAuthsetscallbackURLonly whenmode !== "signup", whilehandleSocialAuthsetscallbackURLunconditionally (even in signup mode). In practice, for the signup flow (callbackURL="/signup?step=2",mode="signup"):
- OIDC: only
newUserCallbackURLis set → existing users land on the default redirect.- Social (Google/GitHub): both
callbackURLandnewUserCallbackURLare set to/signup?step=2→ existing users also get pushed through the signup step flow.Please confirm the intended behavior and align the two handlers. If the OIDC version is correct,
handleSocialAuthshould also skipcallbackURLwhenmode === "signup".🔧 Proposed alignment
const handleSocialAuth = async (provider: string) => { try { await authClient.signIn.social({ provider, - ...(callbackURL ? { callbackURL } : {}), - // For signup flow, new users should also be redirected to the callbackURL + ...(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)); } };🤖 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 31 - 55, handleOIDCAuth and handleSocialAuth disagree on passing callbackURL in signup mode; update handleSocialAuth so it matches handleOIDCAuth by only including callbackURL when callbackURL exists AND mode !== "signup" (but still include newUserCallbackURL when mode === "signup"); modify the authClient.signIn.social call inside handleSocialAuth to conditionally spread { callbackURL } only if callbackURL && mode !== "signup", keeping the existing newUserCallbackURL behavior for mode === "signup".client/src/app/login/page.tsx (1)
99-153:⚠️ Potential issue | 🟡 MinorDead-end UI when internal auth is disabled and no SSO providers are configured.
If
configs.internalAuthEnabled === falseandconfigs.enabledOIDCProviders/enabledSocialProvidersare both empty,SocialButtonsreturnsnull(line 21–29 ofSocialButtons.tsx) and the form on lines 103–153 is hidden. The user would be left with a blank page and only the "Sign up" link. Consider surfacing a friendly error/empty state for this misconfiguration, or prevent the server from reportinginternalAuthEnabled: falseunless at least one SSO provider is registered.🤖 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 99 - 153, The login page currently renders nothing when internal auth is disabled and SocialButtons returns null (because configs.enabledOIDCProviders and configs.enabledSocialProviders are empty), causing a dead-end; update the login page (page.tsx) to detect this misconfiguration (configs.internalAuthEnabled === false && configs.enabledOIDCProviders?.length === 0 && configs.enabledSocialProviders?.length === 0) and render a friendly empty-state/error panel (e.g., a message with guidance and a contact/admin link) in place of the form/SocialButtons, or alternatively show a fallback CTA (e.g., "No sign-in methods configured") so users are not left on a blank page; reference SocialButtons, configs.internalAuthEnabled, enabledOIDCProviders, enabledSocialProviders, and the Login page render logic to implement this check and UI fallback.
♻️ Duplicate comments (1)
client/src/app/login/page.tsx (1)
76-87:⚠️ Potential issue | 🟠 MajorUnsafe access to
enabledOIDCProvidersand missing redirect-in-progress UI.Two concerns with the new auto-redirect effect:
- Potential
TypeErrorat line 78:configs.enabledOIDCProviders.lengthwill throw if the server response omits the field (e.g., during a rolling deploy where the client is newer than the server). Guard with optional chaining:configs.enabledOIDCProviders?.length === 1.- No visual feedback during redirect: Between the effect firing and the browser navigating away, the user sees the bare login UI (which currently renders nothing else when
internalAuthEnabledis false). Consider anisAutoRedirectingstate that swaps the form area for a spinner/"Redirecting to SSO…" message, and set it back tofalsein the.catchso errors are actionable.🤖 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 76 - 87, Guard access to configs.enabledOIDCProviders with optional chaining and add a redirecting UI state: update the useEffect that auto-redirects (useEffect) to check configs.enabledOIDCProviders?.length === 1 instead of accessing .length directly, and introduce an isAutoRedirecting state (with setter setIsAutoRedirecting) that you set to true immediately before calling authClient.signIn.oauth2({ providerId: provider.providerId, callbackURL: "/" }) and set back to false in the .catch handler (and/or .finally) where you already call setError(String(err)); render a spinner/"Redirecting to SSO…" when isAutoRedirecting is true so the user sees feedback while the redirect is in progress.
🧹 Nitpick comments (3)
client/src/app/settings/account/page.tsx (1)
16-21: Usingflexon the description<p>may hurt wrapping/RTL behavior.Turning this paragraph into a flex container makes the translated sentence and the
ExternalLinktwo independent flex items rather than inline text. On narrow viewports or with longer translations (e.g., many non-English locales), the leading text won't wrap around the link the way inline flow would, anditems-center gap-1can produce odd vertical alignment once the sentence wraps internally.Consider keeping the paragraph as regular inline flow and just adding a space before the link, matching the pattern used in
NoData.tsxandTraitsExplorer.tsx.♻️ Suggested change
- <p className="text-neutral-500 dark:text-neutral-400 flex items-center gap-1"> - {t("Manage your personal account settings")} - <ExternalLink href="https://www.rybbit.com/docs/account-settings"> - {t("Learn more")} - </ExternalLink> - </p> + <p className="text-neutral-500 dark:text-neutral-400"> + {t("Manage your personal account settings")}{" "} + <ExternalLink href="https://www.rybbit.com/docs/account-settings"> + {t("Learn more")} + </ExternalLink> + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/settings/account/page.tsx` around lines 16 - 21, The paragraph currently renders as a flex container which breaks inline wrapping and RTL behavior; remove the flex-specific classes ("flex items-center gap-1") from the <p> (leave the text color classes) so it stays in normal inline flow, and ensure there's an explicit space between the translated sentence and the ExternalLink (e.g., by adding a literal space or {" "} before <ExternalLink>) to match the pattern used in NoData.tsx and TraitsExplorer.tsx.server/src/lib/const.ts (1)
633-653: Simplify env scan — one loop per key instead of four regex matches.Since every branch does the same thing (add capture group 1 to
providerIds), a single combined regex avoids redundant work and is easier to maintain.♻️ Proposed refactor
- 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]); - } - } + const oidcKeyRegex = /^OIDC_([A-Z0-9_]+)_(?:CLIENT_ID|CLIENT_SECRET|DISCOVERY_URL|NAME)$/; + const providerIds = new Set<string>(); + for (const [key, value] of Object.entries(process.env)) { + const match = key.match(oidcKeyRegex); + if (match && value) providerIds.add(match[1]); + }🤖 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 633 - 653, The loop that scans process.env performs four separate regex matches for the same outcome; replace it with a single combined regex (or sequential test using a single matching step) that captures the provider id in group 1 to avoid redundant checks. Update the loop that currently references oidcClientIdRegex, oidcClientSecretRegex, oidcDiscoveryUrlRegex, and oidcNameRegex to use a combined pattern (or one match call) and then, if matched and process.env[key] is truthy, add the captured id to providerIds (preserving the existing providerIds.add(match[1]) behavior).client/src/components/auth/SocialButtons.tsx (1)
4-4:SiGoogleis imported but never used.The Google button now uses
<Image src="/crawlers/Google.svg" />instead ofSiGoogle. Remove the unused import to keep the module clean.-import { SiGoogle, SiGithub, SiOpenid } from "@icons-pack/react-simple-icons"; +import { SiGithub, SiOpenid } from "@icons-pack/react-simple-icons";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/auth/SocialButtons.tsx` at line 4, Remove the unused SiGoogle import from the SocialButtons module: locate the import line importing SiGoogle, SiGithub, SiOpenid (symbol: SiGoogle) in SocialButtons.tsx and delete SiGoogle from that import so only used icons (e.g., SiGithub, SiOpenid) remain; ensure the file still compiles and no other references to SiGoogle exist.
🤖 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/messages/cs.json`:
- Line 870: The Czech translation key "Ix3wt3" is empty; add an appropriate
Czech string value (for example "Zjisti více o týmech") for the "Ix3wt3" key so
the inline link shown by t("Learn more about teams") in the teams settings
layout renders non-empty; update the cs.json entry for "Ix3wt3" with the chosen
Czech text.
In `@client/messages/de.json`:
- Line 870: The translation key "Ix3wt3" is empty in client/messages/de.json but
is used by t("Learn more about teams") in
client/src/app/settings/teams/layout.tsx; add an appropriate German translation
(e.g., "Erfahren Sie mehr über Teams") as the value for "Ix3wt3" in
client/messages/de.json so the Teams settings UI renders correctly for German
users, then rebuild/localize assets if your workflow requires it.
In `@client/messages/it.json`:
- Line 870: The translation key "Ix3wt3" is empty in the Italian locale and
should mirror the English "Learn more about teams"; update the value for
"Ix3wt3" in the Italian messages to an appropriate Italian string (e.g., "Scopri
di più sui team" or "Ulteriori informazioni sui team") so the teams settings
link displays correctly for Italian users.
In `@client/messages/ja.json`:
- Line 870: The JSON key "Ix3wt3" in the Japanese locale is an empty string
causing the "Learn more about teams" message to render blank; update the ja.json
entry for "Ix3wt3" to a proper Japanese translation of "Learn more about teams"
(or delete the "Ix3wt3" key so the i18n fallback to the default locale is used)
and commit that change so next-intl will display the intended text.
In `@client/messages/pl.json`:
- Line 870: Add the missing Polish translation for the message key "Ix3wt3"
(which corresponds to "Learn more about teams" in en.json and is used as the
visible text for the ExternalLink in settings/teams/layout.tsx) by replacing the
empty string value with an appropriate Polish translation (e.g., "Dowiedz się
więcej o zespołach") so the link is visible to Polish users.
In `@client/src/app/signup/components/AccountStep.tsx`:
- Around line 38-45: AccountStep currently defaults internalAuthEnabled to true
but when configs?.internalAuthEnabled === false and no external providers are
configured the UI shows only the heading and "Log in" link with no signup path;
update the AccountStep component to detect this misconfiguration by checking
configs?.internalAuthEnabled and whether SocialButtons would render any
providers (use the same provider-detection logic as SocialButtons or expose a
prop/utility), and when internal auth is disabled AND no external providers
exist render an informational/misconfiguration message (matching the
style/resolution used in login/page.tsx) that explains signup is unavailable and
directs the user/admin to correct the auth provider configuration or contact
support. Include references to internalAuthEnabled, configs, SocialButtons and
AccountStep to locate and implement the change.
In `@client/src/components/auth/SocialButtons.tsx`:
- Around line 25-29: The code in SocialButtons.tsx assumes
configs.enabledOIDCProviders and configs.enabledSocialProviders are always
arrays and uses .length (and elsewhere may use .includes), which will throw if
the API omits those keys; make the check defensive by treating those fields as
optional arrays and defaulting to an empty array (e.g., use optional chaining
and/or fallback to [] when evaluating hasProviders and any .includes usages) so
the component returns null safely when no providers exist even if the server
omits those properties.
In `@server/src/lib/const.ts`:
- Line 9: Document the default-on behavior of INTERNAL_AUTHENTICATION_ENABLED in
server/.env.example and server/README.md noting that only the exact string
"false" disables it, and add a runtime safeguard in server/src/lib/auth.ts to
prevent complete lockout: check INTERNAL_AUTHENTICATION_ENABLED and the
configured external auth providers and, if INTERNAL_AUTHENTICATION_ENABLED ===
"false" and no OIDC/social providers or other auth methods are configured, throw
an explicit error or revert to a safe default; update the validation to run at
startup (e.g., in the same init path that uses emailOTP and emailAndPassword)
and reference INTERNAL_AUTHENTICATION_ENABLED, emailOTP, and emailAndPassword so
maintainers can find and adjust the logic.
---
Outside diff comments:
In `@client/src/app/login/page.tsx`:
- Around line 99-153: The login page currently renders nothing when internal
auth is disabled and SocialButtons returns null (because
configs.enabledOIDCProviders and configs.enabledSocialProviders are empty),
causing a dead-end; update the login page (page.tsx) to detect this
misconfiguration (configs.internalAuthEnabled === false &&
configs.enabledOIDCProviders?.length === 0 &&
configs.enabledSocialProviders?.length === 0) and render a friendly
empty-state/error panel (e.g., a message with guidance and a contact/admin link)
in place of the form/SocialButtons, or alternatively show a fallback CTA (e.g.,
"No sign-in methods configured") so users are not left on a blank page;
reference SocialButtons, configs.internalAuthEnabled, enabledOIDCProviders,
enabledSocialProviders, and the Login page render logic to implement this check
and UI fallback.
In `@client/src/components/auth/SocialButtons.tsx`:
- Around line 81-85: The "Or continue with email" divider inside SocialButtons
is always rendered causing an empty separator when internal auth is disabled;
update SocialButtons to only render that divider when internal auth will
actually be shown—either read configs.internalAuthEnabled inside SocialButtons
and gate the divider on that flag, or add a boolean prop (e.g.,
showEmailDivider) to SocialButtons and have the callers in login/page.tsx and
AccountStep.tsx pass configs.internalAuthEnabled so the divider is only rendered
when internal auth is enabled.
- Around line 31-55: handleOIDCAuth and handleSocialAuth disagree on passing
callbackURL in signup mode; update handleSocialAuth so it matches handleOIDCAuth
by only including callbackURL when callbackURL exists AND mode !== "signup" (but
still include newUserCallbackURL when mode === "signup"); modify the
authClient.signIn.social call inside handleSocialAuth to conditionally spread {
callbackURL } only if callbackURL && mode !== "signup", keeping the existing
newUserCallbackURL behavior for mode === "signup".
---
Duplicate comments:
In `@client/src/app/login/page.tsx`:
- Around line 76-87: Guard access to configs.enabledOIDCProviders with optional
chaining and add a redirecting UI state: update the useEffect that
auto-redirects (useEffect) to check configs.enabledOIDCProviders?.length === 1
instead of accessing .length directly, and introduce an isAutoRedirecting state
(with setter setIsAutoRedirecting) that you set to true immediately before
calling authClient.signIn.oauth2({ providerId: provider.providerId, callbackURL:
"/" }) and set back to false in the .catch handler (and/or .finally) where you
already call setError(String(err)); render a spinner/"Redirecting to SSO…" when
isAutoRedirecting is true so the user sees feedback while the redirect is in
progress.
---
Nitpick comments:
In `@client/src/app/settings/account/page.tsx`:
- Around line 16-21: The paragraph currently renders as a flex container which
breaks inline wrapping and RTL behavior; remove the flex-specific classes ("flex
items-center gap-1") from the <p> (leave the text color classes) so it stays in
normal inline flow, and ensure there's an explicit space between the translated
sentence and the ExternalLink (e.g., by adding a literal space or {" "} before
<ExternalLink>) to match the pattern used in NoData.tsx and TraitsExplorer.tsx.
In `@client/src/components/auth/SocialButtons.tsx`:
- Line 4: Remove the unused SiGoogle import from the SocialButtons module:
locate the import line importing SiGoogle, SiGithub, SiOpenid (symbol: SiGoogle)
in SocialButtons.tsx and delete SiGoogle from that import so only used icons
(e.g., SiGithub, SiOpenid) remain; ensure the file still compiles and no other
references to SiGoogle exist.
In `@server/src/lib/const.ts`:
- Around line 633-653: The loop that scans process.env performs four separate
regex matches for the same outcome; replace it with a single combined regex (or
sequential test using a single matching step) that captures the provider id in
group 1 to avoid redundant checks. Update the loop that currently references
oidcClientIdRegex, oidcClientSecretRegex, oidcDiscoveryUrlRegex, and
oidcNameRegex to use a combined pattern (or one match call) and then, if matched
and process.env[key] is truthy, add the captured id to providerIds (preserving
the existing providerIds.add(match[1]) behavior).
🪄 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: 694a3006-7372-42ee-9752-c1bd6b7c0f58
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
README.mdclient/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/zh.jsonclient/src/app/[site]/components/Header/NoData.tsxclient/src/app/[site]/funnels/page.tsxclient/src/app/[site]/goals/page.tsxclient/src/app/[site]/users/components/TraitsExplorer.tsxclient/src/app/invitation/components/login.tsxclient/src/app/login/page.tsxclient/src/app/settings/account/page.tsxclient/src/app/settings/teams/layout.tsxclient/src/app/signup/components/AccountStep.tsxclient/src/app/signup/page.tsxclient/src/components/ExternalLink.tsxclient/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
✅ Files skipped from review due to trivial changes (13)
- README.md
- client/src/app/[site]/goals/page.tsx
- client/src/app/settings/teams/layout.tsx
- docker-compose.yml
- client/messages/ko.json
- client/messages/zh.json
- client/messages/es.json
- client/messages/fr.json
- client/messages/en.json
- client/messages/pt.json
- docker-compose.cloud.yml
- client/src/lib/configs.ts
- client/src/app/invitation/components/login.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- client/src/lib/auth.ts
- server/src/api/getConfig.ts
- server/src/lib/auth.ts
| "+WnNru": "", | ||
| "DNeuzb": "", | ||
| "f7uwif": "", | ||
| "Ix3wt3": "", |
There was a problem hiding this comment.
Czech translation missing for Ix3wt3.
Like several other keys in this file, Ix3wt3 has no translation and will render as empty in the Teams settings page (used by t("Learn more about teams") in client/src/app/settings/teams/layout.tsx). Consider providing a Czech value, e.g. "Zjisti více o týmech", to avoid a blank inline link label.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/messages/cs.json` at line 870, The Czech translation key "Ix3wt3" is
empty; add an appropriate Czech string value (for example "Zjisti více o
týmech") for the "Ix3wt3" key so the inline link shown by t("Learn more about
teams") in the teams settings layout renders non-empty; update the cs.json entry
for "Ix3wt3" with the chosen Czech text.
| "+WnNru": "Sind Sie sicher, dass Sie das Team \"{name}\" löschen möchten? Websites, die diesem Team zugewiesen sind, werden nicht mehr zugewiesen und für alle Mitglieder sichtbar.", | ||
| "DNeuzb": "Teams", | ||
| "f7uwif": "Organisieren Sie Websites in Teams, um zu steuern, welche Mitglieder darauf zugreifen können.", | ||
| "Ix3wt3": "", |
There was a problem hiding this comment.
Missing German translation for actively used key Ix3wt3.
This key is referenced by t("Learn more about teams") in client/src/app/settings/teams/layout.tsx (line 27) and will render as an empty string in the Teams settings UI for German users, breaking the sentence that introduces the "Learn more about teams" link.
🛠️ Suggested fix
- "Ix3wt3": "",
+ "Ix3wt3": "Mehr über Teams erfahren",📝 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.
| "Ix3wt3": "", | |
| "Ix3wt3": "Mehr über Teams erfahren", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/messages/de.json` at line 870, The translation key "Ix3wt3" is empty
in client/messages/de.json but is used by t("Learn more about teams") in
client/src/app/settings/teams/layout.tsx; add an appropriate German translation
(e.g., "Erfahren Sie mehr über Teams") as the value for "Ix3wt3" in
client/messages/de.json so the Teams settings UI renders correctly for German
users, then rebuild/localize assets if your workflow requires it.
| "+WnNru": "Sei sicuro di voler eliminare il team \"{name}\"? I siti assegnati a questo team diventeranno non assegnati e visibili a tutti i membri.", | ||
| "DNeuzb": "Team", | ||
| "f7uwif": "Organizza i siti in team per controllare quali membri possono accedervi.", | ||
| "Ix3wt3": "", |
There was a problem hiding this comment.
Missing Italian translation for Ix3wt3.
The key Ix3wt3 maps to "Learn more about teams" in client/messages/en.json, but here it's an empty string. Italian users will see a blank link in the teams settings page.
🌐 Proposed translation
- "Ix3wt3": "",
+ "Ix3wt3": "Scopri di più sui team",📝 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.
| "Ix3wt3": "", | |
| "Ix3wt3": "Scopri di più sui team", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/messages/it.json` at line 870, The translation key "Ix3wt3" is empty
in the Italian locale and should mirror the English "Learn more about teams";
update the value for "Ix3wt3" in the Italian messages to an appropriate Italian
string (e.g., "Scopri di più sui team" or "Ulteriori informazioni sui team") so
the teams settings link displays correctly for Italian users.
| "+WnNru": "チーム\"{name}\"を削除してよろしいですか？このチームに割り当てられたサイトは未割り当てになり、すべてのメンバーに表示されます。", | ||
| "DNeuzb": "チーム", | ||
| "f7uwif": "サイトをチームに整理して、どのメンバーがアクセスできるかを制御します。", | ||
| "Ix3wt3": "", |
There was a problem hiding this comment.
Empty placeholder for Ix3wt3 — confirm this is intentional.
"Ix3wt3": "" leaves the "Learn more about teams" string untranslated for Japanese users (and will likely render as empty text in the UI rather than falling back to English, depending on how next-intl resolves missing values). If a Japanese translation isn't ready, consider removing the key so the default/fallback kicks in, or adding a translation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/messages/ja.json` at line 870, The JSON key "Ix3wt3" in the Japanese
locale is an empty string causing the "Learn more about teams" message to render
blank; update the ja.json entry for "Ix3wt3" to a proper Japanese translation of
"Learn more about teams" (or delete the "Ix3wt3" key so the i18n fallback to the
default locale is used) and commit that change so next-intl will display the
intended text.
| "+WnNru": "Czy na pewno chcesz usunąć zespół \"{name}\"? Witryny przypisane do tego zespołu staną się nieprzypisane i widoczne dla wszystkich członków.", | ||
| "DNeuzb": "Zespoły", | ||
| "f7uwif": "Organizuj witryny w zespoły, aby kontrolować, którzy członkowie mogą do nich uzyskać dostęp.", | ||
| "Ix3wt3": "", |
There was a problem hiding this comment.
Missing Polish translation for Ix3wt3 results in empty link text.
The key Ix3wt3 maps to "Learn more about teams" in client/messages/en.json and is rendered as the visible text of the ExternalLink in client/src/app/settings/teams/layout.tsx (line 27). With an empty string here, Polish users will see a blank/invisible link next to the teams description.
🌐 Proposed translation
- "Ix3wt3": "",
+ "Ix3wt3": "Dowiedz się więcej o zespołach",📝 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.
| "Ix3wt3": "", | |
| "Ix3wt3": "Dowiedz się więcej o zespołach", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/messages/pl.json` at line 870, Add the missing Polish translation for
the message key "Ix3wt3" (which corresponds to "Learn more about teams" in
en.json and is used as the visible text for the ExternalLink in
settings/teams/layout.tsx) by replacing the empty string value with an
appropriate Polish translation (e.g., "Dowiedz się więcej o zespołach") so the
link is visible to Polish users.
| const internalAuthEnabled = configs?.internalAuthEnabled ?? true; | ||
|
|
||
| return ( | ||
| <div> | ||
| <h2 className="text-2xl font-semibold mb-4">{t("Signup")}</h2> | ||
| <div className="space-y-4"> | ||
| <SocialButtons onError={setError} callbackURL="/signup?step=2" mode="signup" /> | ||
| <AuthInput | ||
| id="email" | ||
| label={t("Email")} | ||
| type="email" | ||
| placeholder="email@example.com" | ||
| required | ||
| value={email} | ||
| onChange={e => setEmail(e.target.value)} | ||
| /> | ||
| <AuthInput | ||
| id="password" | ||
| label={t("Password")} | ||
| type="password" | ||
| placeholder="••••••••" | ||
| required | ||
| value={password} | ||
| onChange={e => setPassword(e.target.value)} | ||
| /> | ||
| {IS_CLOUD && ( | ||
| <Turnstile | ||
| onSuccess={token => setTurnstileToken(token)} | ||
| onError={() => setTurnstileToken("")} | ||
| onExpire={() => setTurnstileToken("")} | ||
| className="flex justify-center" | ||
| /> | ||
| {internalAuthEnabled && ( |
There was a problem hiding this comment.
Default to true is a sensible fallback, but consider the no-provider edge case.
internalAuthEnabled ?? true correctly treats "configs still loading / field missing" as enabled. However, when configs?.internalAuthEnabled === false and no OIDC/social providers are configured, this step renders only the heading and the "Log in" link — no way to sign up. Consider rendering an informational message for that misconfiguration, matching whatever resolution is chosen in login/page.tsx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/signup/components/AccountStep.tsx` around lines 38 - 45,
AccountStep currently defaults internalAuthEnabled to true but when
configs?.internalAuthEnabled === false and no external providers are configured
the UI shows only the heading and "Log in" link with no signup path; update the
AccountStep component to detect this misconfiguration by checking
configs?.internalAuthEnabled and whether SocialButtons would render any
providers (use the same provider-detection logic as SocialButtons or expose a
prop/utility), and when internal auth is disabled AND no external providers
exist render an informational/misconfiguration message (matching the
style/resolution used in login/page.tsx) that explains signup is unavailable and
directs the user/admin to correct the auth provider configuration or contact
support. Include references to internalAuthEnabled, configs, SocialButtons and
AccountStep to locate and implement the change.
| const hasProviders = configs.enabledOIDCProviders.length > 0 || configs.enabledSocialProviders.length > 0; | ||
|
|
||
| if (!hasProviders) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Defensive null check for API-sourced arrays.
configs.enabledOIDCProviders and configs.enabledSocialProviders come from the server /api/config response. If the backend ever omits them (e.g., older server version against newer client during rolling deploy), .length/.includes will throw. Consider optional chaining or a default.
- const hasProviders = configs.enabledOIDCProviders.length > 0 || configs.enabledSocialProviders.length > 0;
+ const hasProviders =
+ (configs.enabledOIDCProviders?.length ?? 0) > 0 ||
+ (configs.enabledSocialProviders?.length ?? 0) > 0;📝 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.
| const hasProviders = configs.enabledOIDCProviders.length > 0 || configs.enabledSocialProviders.length > 0; | |
| if (!hasProviders) { | |
| return null; | |
| } | |
| const hasProviders = | |
| (configs.enabledOIDCProviders?.length ?? 0) > 0 || | |
| (configs.enabledSocialProviders?.length ?? 0) > 0; | |
| if (!hasProviders) { | |
| return null; | |
| } |
🤖 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 25 - 29, The code
in SocialButtons.tsx assumes configs.enabledOIDCProviders and
configs.enabledSocialProviders are always arrays and uses .length (and elsewhere
may use .includes), which will throw if the API omits those keys; make the check
defensive by treating those fields as optional arrays and defaulting to an empty
array (e.g., use optional chaining and/or fallback to [] when evaluating
hasProviders and any .includes usages) so the component returns null safely when
no providers exist even if the server omits those properties.
| export const IS_CLOUD = process.env.CLOUD === "true"; | ||
| export const DISABLE_SIGNUP = process.env.DISABLE_SIGNUP === "true"; | ||
| export const DISABLE_TELEMETRY = process.env.DISABLE_TELEMETRY === "true"; | ||
| export const INTERNAL_AUTHENTICATION_ENABLED = process.env.INTERNAL_AUTHENTICATION_ENABLED !== "false"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\bINTERNAL_AUTHENTICATION_ENABLED\b'Repository: rybbit-io/rybbit
Length of output: 3256
🏁 Script executed:
fd -type f -name '.env*' -o -name 'README*' | head -20Repository: rybbit-io/rybbit
Length of output: 230
🏁 Script executed:
wc -l server/src/lib/auth.tsRepository: rybbit-io/rybbit
Length of output: 86
🏁 Script executed:
fd -type file \( -name '.env*' -o -name 'README*' \)Repository: rybbit-io/rybbit
Length of output: 230
🏁 Script executed:
cat -n server/src/lib/auth.tsRepository: rybbit-io/rybbit
Length of output: 11474
🏁 Script executed:
find . -maxdepth 2 -type f \( -name '.env*' -o -name 'README*' \)Repository: rybbit-io/rybbit
Length of output: 218
🏁 Script executed:
rg -n 'getOIDCProviders|getSocialProviders' server/src/lib/const.ts -A 10Repository: rybbit-io/rybbit
Length of output: 1153
🏁 Script executed:
cat server/.env.exampleRepository: rybbit-io/rybbit
Length of output: 837
🏁 Script executed:
cat server/README.mdRepository: rybbit-io/rybbit
Length of output: 1276
Document INTERNAL_AUTHENTICATION_ENABLED default behavior and verify no lockout risk.
The default-on behavior using !== "false" is reasonable for backward compatibility, but needs attention:
-
Missing documentation:
INTERNAL_AUTHENTICATION_ENABLEDis not documented inserver/.env.exampleorserver/README.md. Add it with clarification that only the exact string"false"disables it. -
Lockout risk confirmed: In
server/src/lib/auth.ts, whenINTERNAL_AUTHENTICATION_ENABLEDisfalse, bothemailAndPassword(line 98) andemailOTP(lines 70–75) are disabled. If no OIDC providers or social credentials are configured, operators can lock themselves out with zero authentication methods available. Add a validation or safeguard to prevent this scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/lib/const.ts` at line 9, Document the default-on behavior of
INTERNAL_AUTHENTICATION_ENABLED in server/.env.example and server/README.md
noting that only the exact string "false" disables it, and add a runtime
safeguard in server/src/lib/auth.ts to prevent complete lockout: check
INTERNAL_AUTHENTICATION_ENABLED and the configured external auth providers and,
if INTERNAL_AUTHENTICATION_ENABLED === "false" and no OIDC/social providers or
other auth methods are configured, throw an explicit error or revert to a safe
default; update the validation to run at startup (e.g., in the same init path
that uses emailOTP and emailAndPassword) and reference
INTERNAL_AUTHENTICATION_ENABLED, emailOTP, and emailAndPassword so maintainers
can find and adjust the logic.
Summary
Based on #719 by @acvigue, updated for compatibility with current main.
Changes from original PR
INTERNAL_AUTHENTICATION_ENABLED)Configuration