feat: enforce demo account isolation and organization access restrictions#204
Conversation
…ions - Added checks to prevent demo accounts from modifying organization memberships or settings in the auth API. - Implemented restrictions for demo accounts to join or request access to other organizations. - Enhanced organization search API to return an empty response for demo accounts. - Introduced utility functions to manage demo account email configuration and validation. - Updated relevant API handlers to enforce demo account rules. - Added unit tests to ensure demo account isolation and access restrictions are enforced correctly.
|
🚅 Deployed to the reqcore-pr-204 environment in applirank
|
|
Warning Review limit reached
More reviews will be available in 34 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds demo account organization isolation: new helpers in ChangesDemo Account Organization Isolation
Dashboard Organization Settings Error Handling
CI Workflow and Dependency Updates
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthHandler as server/api/auth/[...all].ts
participant demoOrg as server/utils/demoOrg.ts
participant BetterAuth as auth.handler
Client->>AuthHandler: Request to /api/auth/*
AuthHandler->>demoOrg: getSession(), getDemoAccountOrgIds()
demoOrg-->>AuthHandler: demo org IDs + normalized activeOrganizationId
AuthHandler->>AuthHandler: persist activeOrganizationId to session
alt forbidden org mutation + demo account
AuthHandler-->>Client: 403 Forbidden
else normal request
AuthHandler->>BetterAuth: auth.handler(event)
BetterAuth-->>AuthHandler: response
alt org list response + demo account
AuthHandler->>demoOrg: getDemoAccountOrgIds()
AuthHandler->>AuthHandler: filter organizations to allowed set
end
AuthHandler-->>Client: (filtered) response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
🧹 Nitpick comments (2)
tests/unit/demo-account-isolation.test.ts (1)
8-29: 💤 Low valuePattern-based tests are fragile but serve as safety nets.
These tests verify code structure via regex rather than behavior. While this approach is brittle (formatting changes or refactoring could break tests without affecting functionality), it serves as a useful safety net to ensure demo isolation guards aren't accidentally removed during future refactoring.
Consider supplementing with integration tests that actually verify demo accounts receive 403 responses when attempting blocked operations, which would test behavior rather than implementation.
🤖 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 `@tests/unit/demo-account-isolation.test.ts` around lines 8 - 29, The pattern-based tests in the describe block use regex matching to verify that demo isolation guards exist in the codebase, which is brittle to formatting changes. Supplement these structural verification tests by adding integration tests that verify the actual behavioral guarantees: test that demo accounts receive 403 responses when attempting to modify non-demo organizations, when accessing organization routes like set-active and list, and when trying to join or discover real organizations. This ensures that the security guarantees are enforced through actual API behavior testing rather than relying on code pattern matching.server/utils/demoOrg.ts (1)
65-69: 💤 Low valuePotential duplicate slug addition.
getDemoAccountSlugs()unconditionally addsenv.DEMO_ORG_SLUG || DEFAULT_PREVIEW_DEMO_ORG_SLUG, butgetConfiguredDemoSlugs()already addsenv.DEMO_ORG_SLUGwhen it's set (line 33). WhenDEMO_ORG_SLUGis configured, it will appear twice in the Set (deduplicated) but the fallback logic is confusing: ifDEMO_ORG_SLUGis unset, both the preview slug from line 38 (conditionally) and line 67 (unconditionally) could addDEFAULT_PREVIEW_DEMO_ORG_SLUG.Consider clarifying the intent—if the demo account should always have access to the preview slug regardless of environment:
♻️ Suggested clarification
export function getDemoAccountSlugs(): string[] { const slugs = new Set(getConfiguredDemoSlugs().slugs) - slugs.add(env.DEMO_ORG_SLUG || DEFAULT_PREVIEW_DEMO_ORG_SLUG) + // Always include the preview demo slug for demo account isolation + slugs.add(DEFAULT_PREVIEW_DEMO_ORG_SLUG) + if (env.DEMO_ORG_SLUG) { + slugs.add(env.DEMO_ORG_SLUG) + } return [...slugs] }🤖 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 `@server/utils/demoOrg.ts` around lines 65 - 69, The getDemoAccountSlugs() function is redundantly handling env.DEMO_ORG_SLUG, which is already included by getConfiguredDemoSlugs(). The unconditional addition of env.DEMO_ORG_SLUG or DEFAULT_PREVIEW_DEMO_ORG_SLUG creates confusing fallback logic where the preview slug could be added multiple times. Remove the redundant slug.add() call in getDemoAccountSlugs() and rely solely on getConfiguredDemoSlugs() to provide all the necessary slugs, ensuring the fallback logic is centralized and clear.
🤖 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.
Nitpick comments:
In `@server/utils/demoOrg.ts`:
- Around line 65-69: The getDemoAccountSlugs() function is redundantly handling
env.DEMO_ORG_SLUG, which is already included by getConfiguredDemoSlugs(). The
unconditional addition of env.DEMO_ORG_SLUG or DEFAULT_PREVIEW_DEMO_ORG_SLUG
creates confusing fallback logic where the preview slug could be added multiple
times. Remove the redundant slug.add() call in getDemoAccountSlugs() and rely
solely on getConfiguredDemoSlugs() to provide all the necessary slugs, ensuring
the fallback logic is centralized and clear.
In `@tests/unit/demo-account-isolation.test.ts`:
- Around line 8-29: The pattern-based tests in the describe block use regex
matching to verify that demo isolation guards exist in the codebase, which is
brittle to formatting changes. Supplement these structural verification tests by
adding integration tests that verify the actual behavioral guarantees: test that
demo accounts receive 403 responses when attempting to modify non-demo
organizations, when accessing organization routes like set-active and list, and
when trying to join or discover real organizations. This ensures that the
security guarantees are enforced through actual API behavior testing rather than
relying on code pattern matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b90978bc-df31-48a4-bcd9-f7cd0bc1b4d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/pr-validation.ymlpackage.jsonserver/api/auth/[...all].tsserver/api/invite-links/accept.post.tsserver/api/join-requests/index.post.tsserver/api/org-search/index.get.tsserver/utils/demoOrg.tsserver/utils/requireAuth.tsserver/utils/requirePermission.tstests/unit/demo-account-isolation.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/unit/org-settings-auth-errors.test.ts`:
- Around line 20-27: The assertions at lines 25-27 checking the ordering of
patterns in settingsPage can produce false positives because indexOf returns -1
when a pattern is not found, and -1 is still less than any valid index. To fix
this, explicitly assert that each critical pattern exists in the settingsPage
string before comparing their positions using toBeLessThan. First verify that
the string 'if (result.error)' exists within the settingsPage and that 'track'
appears in the settingsPage, then verify their relative ordering. This ensures
that both patterns are actually present in the code before the ordering
assertion runs, preventing the test from passing when the error handling guard
is missing.
🪄 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: 368a9fc9-691b-4ab2-a613-3455fdd59097
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
app/pages/dashboard/settings/index.vuetests/unit/org-settings-auth-errors.test.ts
1 participant
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores