chore(web): Improved security settings#1303
Merged
Merged
Conversation
Contributor
|
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:
WalkthroughThis PR migrates org auth/access flags into DB fields, adds shared helpers, init sync, server actions and UI for security settings, updates auth/provider wiring and entitlements, refactors onboarding and verification flows, and removes legacy metadata helpers and components. ChangesOrg-Level Auth Settings & Access Control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This comment has been minimized.
This comment has been minimized.
brendan-kellam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/app/onboard/page.tsx (1)
69-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp and validate
stepbefore indexingsteps.
parseInt()returnsNaNfor inputs like?step=abc, and the authenticated branch never caps large values. That makessteps[currentStep]undefined and crashes the render a few lines later whencurrentStepData.title/subtitleare read.🐛 Suggested fix
- const stepParam = searchParams?.step ? parseInt(searchParams.step) : 0; - const currentStep = session?.user ? Math.max(2, stepParam) : Math.max(0, Math.min(stepParam, 1)); + const parsedStep = searchParams?.step ? Number.parseInt(searchParams.step, 10) : 0; + const requestedStep = Number.isFinite(parsedStep) ? parsedStep : 0; @@ - const currentStepData = steps[currentStep] + const unclampedStep = session?.user + ? Math.max(2, requestedStep) + : Math.max(0, Math.min(requestedStep, 1)) + const currentStep = Math.min(unclampedStep, steps.length - 1) + const currentStepData = steps[currentStep]Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/onboard/page.tsx` around lines 69 - 71, Validate and clamp the parsed step value before using it to index steps: replace raw parseInt usage with a safe integer parse that treats NaN as 0 for stepParam (e.g., const stepParam = Number.isInteger(parsed) ? parsed : 0), then compute currentStep by clamping into the valid index range of the steps array (0 to steps.length - 1); update both occurrences that set currentStep (the unauthenticated and authenticated branches) so the authenticated branch still enforces a minimum (e.g., Math.max(2, clampedStep)) but also applies Math.min against steps.length - 1 to avoid out-of-bounds access when reading steps[currentStep] (reference symbols: stepParam, currentStep, steps).
🧹 Nitpick comments (3)
packages/web/src/lib/entitlements.ts (1)
44-61: ⚡ Quick winLet callers pass a preloaded org into
isAnonymousAccessEnabled().This helper now always re-queries
org, which adds a second DB read on paths likewithOptionalAuth()that already loaded the org ingetAuthContext(). An overload or a small pure resolver that acceptsOrgwould keep the env/license logic centralized without adding extra I/O to public-request hot paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/lib/entitlements.ts` around lines 44 - 61, Modify isAnonymousAccessEnabled to accept an optional preloaded Org object (e.g., add a parameter org?: Org or create an overload) and use that Org if provided instead of always re-querying the database; only call __unsafePrisma.org.findUnique when the optional org argument is undefined. Preserve the existing env.FORCE_ENABLE_ANONYMOUS_ACCESS and isAnonymousAccessAvailable() checks and return semantics, and update hot-path callers like getAuthContext()/withOptionalAuth to pass their loaded org into isAnonymousAccessEnabled to avoid the extra DB read.packages/web/src/app/onboard/page.tsx (1)
168-179: ⚡ Quick winUse
cn()for these conditional class lists.These branches still build Tailwind class strings with template literal interpolation. Please move them to
cn(...)to match the repo TSX rule and keep the variants easier to scan.♻️ Suggested cleanup
+import { cn } from "`@/lib/utils`"; @@ - <div - className={`absolute top-10 left-1/2 transform -translate-x-1/2 w-0.5 h-8 transition-all duration-300 ${index < currentStep ? "bg-primary" : "bg-border" - }`} - /> + <div + className={cn( + "absolute top-10 left-1/2 transform -translate-x-1/2 w-0.5 h-8 transition-all duration-300", + index < currentStep ? "bg-primary" : "bg-border" + )} + /> @@ - <div - className={`relative z-10 w-10 h-10 rounded-full border-2 flex items-center justify-center font-semibold text-sm transition-all duration-300 ${index < currentStep - ? "bg-primary border-primary text-primary-foreground" - : index === currentStep - ? "bg-primary border-primary text-primary-foreground scale-110 shadow-lg" - : "bg-background border-border text-muted-foreground" - }`} - > + <div + className={cn( + "relative z-10 w-10 h-10 rounded-full border-2 flex items-center justify-center font-semibold text-sm transition-all duration-300", + index < currentStep + ? "bg-primary border-primary text-primary-foreground" + : index === currentStep + ? "bg-primary border-primary text-primary-foreground scale-110 shadow-lg" + : "bg-background border-border text-muted-foreground" + )} + > @@ - <div className={`font-medium text-sm transition-all duration-200 ${index <= currentStep ? "text-foreground" : "text-muted-foreground" - }`}> + <div + className={cn( + "font-medium text-sm transition-all duration-200", + index <= currentStep ? "text-foreground" : "text-muted-foreground" + )} + >As per coding guidelines, use
cn()from@/lib/utilsfor conditional classNames instead of template literal interpolation.Also applies to: 191-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/onboard/page.tsx` around lines 168 - 179, Replace the template-literal Tailwind class strings for the vertical line and the circle div's className with calls to the cn(...) helper from '`@/lib/utils`' (e.g., the className prop building inside the element rendering the absolute top-10 left-1/2 w-0.5 h-8 line and the div that renders the circle with relative z-10 w-10 h-10 ...). Import cn if missing, convert the conditional expressions (index < currentStep, index === currentStep) into boolean arguments or ternary branches inside cn so each variant becomes a separate class argument rather than a template string, and make the same change for the other occurrences mentioned around the 191-192 area.Source: Coding guidelines
packages/web/src/app/(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsx (1)
53-59: ⚡ Quick winAlign this TSX with the repo style rules.
Line 53 still uses a single-line
ifwithout braces, and Line 96 uses CSS variable syntax for a color class. Please switch that guard to a braced block and use the matching Tailwind token class instead.♻️ Suggested cleanup
const handleCopy = async () => { - if (!inviteLink) return + if (!inviteLink) { + return + } try { await navigator.clipboard.writeText(inviteLink) setCopied(true) setTimeout(() => setCopied(false), 2000) @@ > {copied ? ( - <Check className="h-4 w-4 text-[var(--chart-2)]" /> + <Check className="h-4 w-4 text-chart-2" /> ) : ( <Copy className="h-4 w-4" /> )} </Button>As per coding guidelines, always use curly braces for
ifstatements, with the body on a new line, and use Tailwind color classes directly instead of CSS variable syntax.Also applies to: 95-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/`(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsx around lines 53 - 59, Change the single-line guard "if (!inviteLink) return" in inviteLinkEnabledSettingsCard.tsx to a braced block with the body on a new line (e.g., if (!inviteLink) { return; }) so the early-return follows repo style; also replace the CSS variable color class used in the component's className (the token using CSS var(...) around the color) with the corresponding Tailwind token class (e.g., the repo's standard text-<token> utility) so the color uses Tailwind tokens instead of CSS variable syntax; ensure this touches the same JSX that calls navigator.clipboard.writeText and setCopied so formatting remains consistent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/configuration/auth/providers.mdx`:
- Line 14: Update the navigation text in the documentation: replace the phrase
"Settings → Access" with "Settings → Security" in the sentence that currently
reads "You can toggle it from **Settings → Access** using the **Email login**
setting" so the doc matches the renamed menu entry.
In `@packages/web/src/app/`(app)/settings/security/actions.ts:
- Around line 49-59: The guard that prevents disabling email login only checks
configured providers via getProviders() and hasAlternativeLoginMethod, but
ignores per-organization flags org.isEmailCodeLoginEnabled and
org.isCredentialsLoginEnabled; update the logic in the same block (and the
analogous block around lines 85-95) to treat a provider as "available" only if
it is both configured and enabled for the org—i.e., when computing
hasAlternativeLoginMethod include checks for org.isCredentialsLoginEnabled and
org.isEmailCodeLoginEnabled (and any provider.type !== "credentials SSO" logic)
so the function that currently uses getProviders() and hasAlternativeLoginMethod
refuses to disable the last actually-enabled non-SSO login path for that org.
In
`@packages/web/src/app/`(app)/settings/security/components/emailCodeLoginEnabledSettingsCard.tsx:
- Around line 73-76: The Switch is currently disabled whenever
isEmailServiceConfigured is false, which prevents turning off email-code login
when it's already enabled; update the disabled prop on the Switch (the JSX using
checked={enabled} and onCheckedChange={handleToggle}) to only disable when
loading or when the service is not configured AND the feature is not currently
enabled (e.g. disabled={isLoading || (!isEmailServiceConfigured && !enabled)}),
so users can still toggle off an already-enabled setting.
In
`@packages/web/src/app/`(app)/settings/security/components/identityProviderSettingsCard.tsx:
- Around line 20-25: The Image usage inside identityProviderSettingsCard is
missing intrinsic dimensions so Next.js can't infer size from
providerInfo.icon.src; update the Image call (the JSX that renders when
providerInfo.icon is truthy) to include explicit width and height props (or
switch to layout="fill"/fill with a positioned parent) based on the known icon
dimensions on AuthProviderInfo.icon, e.g., pass numeric width and height or use
fill and ensure the container has relative positioning; modify the JSX where
providerInfo.icon, Image and name are referenced to add these props.
In
`@packages/web/src/app/`(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsx:
- Around line 88-100: The copy Button in inviteLinkEnabledSettingsCard.tsx is an
icon-only control and needs an accessible name; update the Button element (the
one with onClick={handleCopy}, disabled={!inviteLink} and showing copied ?
<Check/> : <Copy/>) to include an aria-label (e.g., aria-label="Copy invite
link") or add visually hidden text; ensure the label reflects its action and
still works when disabled and when the copied state changes.
In `@packages/web/src/auth.ts`:
- Around line 75-79: The code calls __unsafePrisma.org.findUnique and then
passes org! into helpers (e.g., isEmailCodeLoginEnabled) inside getProviders(),
which crashes when the single-tenant org is missing; guard the missing-org path
before evaluating those helpers: after retrieving org from
__unsafePrisma.org.findUnique, check for null and if null use the schema
defaults (credentials = true, email-code = false) or respect env overrides
(e.g., EMAIL_FROM_ADDRESS, getSMTPConnectionURL) instead of dereferencing org;
update all places in getProviders() that use org! (and the other call site that
passes org into isEmailCodeLoginEnabled) to branch on org presence and only call
org-reading helpers when org is non-null so provider resolution falls back
safely.
---
Outside diff comments:
In `@packages/web/src/app/onboard/page.tsx`:
- Around line 69-71: Validate and clamp the parsed step value before using it to
index steps: replace raw parseInt usage with a safe integer parse that treats
NaN as 0 for stepParam (e.g., const stepParam = Number.isInteger(parsed) ?
parsed : 0), then compute currentStep by clamping into the valid index range of
the steps array (0 to steps.length - 1); update both occurrences that set
currentStep (the unauthenticated and authenticated branches) so the
authenticated branch still enforces a minimum (e.g., Math.max(2, clampedStep))
but also applies Math.min against steps.length - 1 to avoid out-of-bounds access
when reading steps[currentStep] (reference symbols: stepParam, currentStep,
steps).
---
Nitpick comments:
In
`@packages/web/src/app/`(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsx:
- Around line 53-59: Change the single-line guard "if (!inviteLink) return" in
inviteLinkEnabledSettingsCard.tsx to a braced block with the body on a new line
(e.g., if (!inviteLink) { return; }) so the early-return follows repo style;
also replace the CSS variable color class used in the component's className (the
token using CSS var(...) around the color) with the corresponding Tailwind token
class (e.g., the repo's standard text-<token> utility) so the color uses
Tailwind tokens instead of CSS variable syntax; ensure this touches the same JSX
that calls navigator.clipboard.writeText and setCopied so formatting remains
consistent.
In `@packages/web/src/app/onboard/page.tsx`:
- Around line 168-179: Replace the template-literal Tailwind class strings for
the vertical line and the circle div's className with calls to the cn(...)
helper from '`@/lib/utils`' (e.g., the className prop building inside the element
rendering the absolute top-10 left-1/2 w-0.5 h-8 line and the div that renders
the circle with relative z-10 w-10 h-10 ...). Import cn if missing, convert the
conditional expressions (index < currentStep, index === currentStep) into
boolean arguments or ternary branches inside cn so each variant becomes a
separate class argument rather than a template string, and make the same change
for the other occurrences mentioned around the 191-192 area.
In `@packages/web/src/lib/entitlements.ts`:
- Around line 44-61: Modify isAnonymousAccessEnabled to accept an optional
preloaded Org object (e.g., add a parameter org?: Org or create an overload) and
use that Org if provided instead of always re-querying the database; only call
__unsafePrisma.org.findUnique when the optional org argument is undefined.
Preserve the existing env.FORCE_ENABLE_ANONYMOUS_ACCESS and
isAnonymousAccessAvailable() checks and return semantics, and update hot-path
callers like getAuthContext()/withOptionalAuth to pass their loaded org into
isAnonymousAccessEnabled to avoid the extra DB read.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f28f52d-3369-4ba3-a39e-6a722d084eeb
📒 Files selected for processing (45)
.env.developmentdocs/docs/configuration/auth/providers.mdxdocs/docs/configuration/config-file.mdxdocs/docs/configuration/environment-variables.mdxpackages/db/prisma/migrations/20260611195453_add_org_access_settings/migration.sqlpackages/db/prisma/schema.prismapackages/shared/src/env.server.tspackages/shared/src/index.server.tspackages/shared/src/utils.tspackages/web/src/__mocks__/prisma.tspackages/web/src/actions.tspackages/web/src/app/(app)/@sidebar/components/upgradeBadge.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/access/page.tsxpackages/web/src/app/(app)/settings/components/settingsCard.tsxpackages/web/src/app/(app)/settings/layout.tsxpackages/web/src/app/(app)/settings/security/actions.tspackages/web/src/app/(app)/settings/security/components/anonymousAccessEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/credentialsLoginEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/emailCodeLoginEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/identityProviderSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/identityProviderUpsellCard.tsxpackages/web/src/app/(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/memberApprovalRequiredSettingsCard.tsxpackages/web/src/app/(app)/settings/security/page.tsxpackages/web/src/app/components/anonymousAccessToggle.tsxpackages/web/src/app/components/inviteLinkToggle.tsxpackages/web/src/app/components/memberApprovalRequiredToggle.tsxpackages/web/src/app/components/organizationAccessSettings.tsxpackages/web/src/app/components/organizationAccessSettingsWrapper.tsxpackages/web/src/app/invite/actions.tspackages/web/src/app/onboard/components/accessSettingsStep.tsxpackages/web/src/app/onboard/components/ownerSignupStep.tsxpackages/web/src/app/onboard/components/welcomeStep.tsxpackages/web/src/app/onboard/page.tsxpackages/web/src/auth.tspackages/web/src/initialize.tspackages/web/src/lib/entitlements.test.tspackages/web/src/lib/entitlements.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/posthogEvents.tspackages/web/src/lib/utils.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.tspackages/web/src/types.ts
💤 Files with no reviewable changes (10)
- packages/web/src/app/components/anonymousAccessToggle.tsx
- docs/docs/configuration/config-file.mdx
- packages/web/src/app/components/inviteLinkToggle.tsx
- packages/web/src/app/(app)/settings/access/page.tsx
- packages/web/src/app/components/organizationAccessSettingsWrapper.tsx
- packages/web/src/app/components/organizationAccessSettings.tsx
- packages/web/src/app/components/memberApprovalRequiredToggle.tsx
- .env.development
- packages/web/src/types.ts
- docs/docs/configuration/environment-variables.mdx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/app/login/verify/verifyForm.tsx (1)
3-6: 💤 Low valueConsolidate imports from the same module.
These four imports can be combined into a single import statement for better readability.
Suggested consolidation
-import { InputOTPSeparator } from "`@/components/ui/input-otp`" -import { InputOTPGroup } from "`@/components/ui/input-otp`" -import { InputOTPSlot } from "`@/components/ui/input-otp`" -import { InputOTP } from "`@/components/ui/input-otp`" +import { InputOTP, InputOTPGroup, InputOTPSeparator, InputOTPSlot } from "`@/components/ui/input-otp`"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/login/verify/verifyForm.tsx` around lines 3 - 6, Consolidate the four separate imports from the same module into a single import statement: replace the individual imports of InputOTPSeparator, InputOTPGroup, InputOTPSlot, and InputOTP with one combined import from "`@/components/ui/input-otp`" (referencing the symbols InputOTPSeparator, InputOTPGroup, InputOTPSlot, InputOTP) so the file imports are cleaner and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/app/login/verify/verifyForm.tsx`:
- Around line 49-54: The captureEvent call is causing a side effect during
render; remove the synchronous captureEvent("wa_login_verify_page_no_email", {})
from the render return path in verifyForm.tsx and instead invoke it inside a
useEffect that runs when email is falsy (e.g., useEffect(() => { if (!email)
captureEvent(...); }, [email])); keep returning the <Redirect to="/login" />
from render when !email, but ensure the useEffect dependency prevents duplicate
calls (and optionally guard for client-only execution if needed).
---
Nitpick comments:
In `@packages/web/src/app/login/verify/verifyForm.tsx`:
- Around line 3-6: Consolidate the four separate imports from the same module
into a single import statement: replace the individual imports of
InputOTPSeparator, InputOTPGroup, InputOTPSlot, and InputOTP with one combined
import from "`@/components/ui/input-otp`" (referencing the symbols
InputOTPSeparator, InputOTPGroup, InputOTPSlot, InputOTP) so the file imports
are cleaner and easier to maintain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41c1cc01-0e50-4fef-b65c-1972840c8f9d
📒 Files selected for processing (4)
packages/web/src/app/login/verify/page.tsxpackages/web/src/app/login/verify/verificationFailed.tsxpackages/web/src/app/login/verify/verifyForm.tsxpackages/web/src/auth.ts
💤 Files with no reviewable changes (1)
- packages/web/src/app/login/verify/verificationFailed.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR does a couple of things:
Primarily, it redesigns the security settings page (previously the access page) to include some new settings & views, namely:
a. setting to configure if email & password login is enabled.
b. setting to configure if email code login is enabled.
c. list to view configured SSO providers.
In order to achieve (a) and (b), I've added
isCredentialsLoginEnabledandisEmailCodeLoginEnabledfields to the database. Additionally, I moved theisAnonymousAccessEnabledsetting from theorg.metadataJSON blob into a dedicated field. The migration handles back-compat.The 4 environment variables that can control these settings (
REQUIRE_APPROVAL_NEW_MEMBERS,AUTH_CREDENTIALS_LOGIN_ENABLED,AUTH_EMAIL_CODE_LOGIN_ENABLED, andFORCE_ENABLE_ANONYMOUS_ACCESS) have now been marked deprecated, but will still be respected for the current major version.TODO:
metadatafield from Org (?)Screenshots
Security page (w/o entitlement):
Security page (w/ entitlement):
SSO (w/o configured providers):
SSO (w/ configured providers):
Email login (w/o configured SMTP server):
Summary by CodeRabbit
New Features
Documentation
Deprecations
Other