feat: feature paywall with free/enterprise tier enforcement - #575
feat: feature paywall with free/enterprise tier enforcement#575Harrio-6 wants to merge 12 commits into
Conversation
|
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:
WalkthroughAdds org plan-tier storage and API support, enforces free-tier member limits, expands invitation cleanup, and wires the org UI to display and upgrade the current plan. ChangesPlan tier & upgrade feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 8
🤖 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 `@client/src/app/org/components/OrgMembers.tsx`:
- Around line 265-277: The `isFree` check in `OrgMembers` has the same
missing-default issue as `OrgOverview`: an undefined `planTier` is currently
treated as non-free, so the Add flow can bypass the upgrade gate. Update the
`isFree` logic in `OrgMembers` so unresolved tier data is handled as free/locked
by default, and ensure the Add button path continues to open `UpgradeModal`
until `planTier` is known rather than falling through to `AddUserDialog`.
In `@client/src/app/org/components/OrgOverview.tsx`:
- Around line 20-32: `OrgOverview` currently treats missing `planTier` as
non-free when computing `isFree`, which makes the plan label fallback and
upgrade gating inconsistent. Update the `isFree` derivation in `OrgOverview` to
use the same default as the displayed plan text (`free` when `planTier` is
undefined), and apply the same normalization in `OrgMembers` where the
`UpgradeModal` vs `AddUserDialog` choice is made. Keep the logic centralized or
mirrored so both components handle missing `planTier` consistently.
In `@client/src/app/org/components/UpgradeModal.tsx`:
- Around line 39-44: The external link opened from UpgradeModal’s Button click
handler should use safer window.open settings to prevent reverse-tabnabbing.
Update the onClick callback in UpgradeModal to open the Cal.com URL with the
same _blank target plus noopener,noreferrer in the window features, keeping the
existing Button behavior unchanged.
In `@server/routes/org_routes.py`:
- Around line 1203-1206: The downgrade cleanup in the org route is bypassing the
centralized token/secret lifecycle by deleting directly from user_tokens. Update
the org downgrade flow in the route handler to use the existing token management
helpers instead of ad-hoc SQL, so secret_ref cleanup and Vault secret handling
stay consistent. Locate the cleanup logic around the org downgrade path and
replace the direct DELETE with the centralized helper-based token removal path
used by store_tokens_in_db/get_token_data-related flows.
- Around line 1195-1202: The downgrade handling in org_routes should expire
every pending org invitation when moving from enterprise to free, not just those
matching existing non-creator users. Update the invitation cleanup logic around
the old_tier/new_tier check so the org_invitations update targets all pending
invites for the org, likely by filtering on pending status and org_id rather
than the current email subquery, while preserving the creator exclusion only
where needed. Use the existing org invitation expiration block in the route
handler to locate the change.
- Around line 1173-1232: The organization plan_tier update flow currently allows
any org-manage caller to set the tier to enterprise, which should not be
permitted from this route. Update the handler around the new_tier validation and
the old_tier/new_tier transition logic to reject enterprise upgrades here,
allowing only safe downgrades (for example enterprise to free) or gating
upgrades behind a trusted billing/internal path. Keep the existing downgrade
cleanup in the db_pool.get_admin_connection / organizations UPDATE path, but
prevent persisting enterprise from this endpoint.
In `@server/utils/db/db_utils.py`:
- Around line 3121-3123: `plan_tier` is currently added as a nullable, free-form
column, which lets NULL or unexpected values bypass the seat-limit enforcement
that only checks for `"free"`. Update the `ALTER TABLE organizations` migration
in `db_utils.py` to make `plan_tier` non-null and constrained to the allowed set
(at minimum including `"free"`), while keeping the default consistent. Make sure
the `plan_tier` schema change in the organizations table enforces this at the
database boundary so downstream hooks cannot receive invalid values.
In `@server/utils/hooks.py`:
- Around line 127-133: The default fallback in the hook dispatch is failing
open, which can bypass seat-limit enforcement when `before_add_member` errors.
Update the `server/utils/hooks.py` hook invocation path around
`_HOOK_REGISTRY[name]` so that if the default DB-backed check raises, it returns
a denial result instead of `(True, None)`, and keep the behavior specific to
access-control hooks like `before_add_member` while preserving the existing
exception logging.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 623e9f00-fa7d-45f4-98bc-1beedf257b0d
📒 Files selected for processing (9)
client/src/app/org/components/OrgMembers.tsxclient/src/app/org/components/OrgOverview.tsxclient/src/app/org/components/UpgradeModal.tsxclient/src/components/OrgSettings.tsxclient/src/components/SettingsModal.tsxserver/routes/admin_routes.pyserver/routes/org_routes.pyserver/utils/db/db_utils.pyserver/utils/hooks.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/src/app/org/components/UpgradeModal.tsx (1)
41-41: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
noreferrerstill missing fromwindow.open.Prior feedback asked for
noopener,noreferrer; onlynoopenerwas added.noopenercovers the main reverse-tabnabbing risk, so this is low severity, but consider addingnoreferrertoo for completeness.🔧 Suggested tweak
- onClick={() => window.open("https://cal.com/arvo-ai?ref=999998", "_blank", "noopener")} + onClick={() => window.open("https://cal.com/arvo-ai?ref=999998", "_blank", "noopener,noreferrer")}🤖 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 `@client/src/app/org/components/UpgradeModal.tsx` at line 41, The `UpgradeModal` link opener still only passes `noopener`, so update the `window.open` call in the click handler to include `noreferrer` as well, using the existing Cal.com URL and `_blank` target. Keep the change localized to the `onClick` handler in `UpgradeModal.tsx` so the popup behavior remains the same while adding the missing referrer protection.
🤖 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.
Duplicate comments:
In `@client/src/app/org/components/UpgradeModal.tsx`:
- Line 41: The `UpgradeModal` link opener still only passes `noopener`, so
update the `window.open` call in the click handler to include `noreferrer` as
well, using the existing Cal.com URL and `_blank` target. Keep the change
localized to the `onClick` handler in `UpgradeModal.tsx` so the popup behavior
remains the same while adding the missing referrer protection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38906f56-0df3-41fc-bd05-9a9a0dc7a634
📒 Files selected for processing (5)
client/src/app/org/components/OrgMembers.tsxclient/src/app/org/components/OrgOverview.tsxclient/src/app/org/components/UpgradeModal.tsxserver/routes/org_routes.pyserver/utils/hooks.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/org_routes.py (1)
1239-1252: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPost-commit side effects can mask a successful downgrade with a 500.
The transaction commits at Line 1237, but
record_audit_event(...)(Line 1246) runs inside the outertry. If audit logging raises, the handler falls through to theexceptand returns500, even though the plan change and destructive cleanup already committed. The client sees a failure for an operation that actually succeeded. Consider isolating post-commit work so it can't invert the response status.♻️ Isolate post-commit audit from the request result
record_audit_event(org_id, user_id, "update_plan", "organization", org_id, - {"old_tier": old_tier, "new_tier": new_tier}, request) + {"old_tier": old_tier, "new_tier": new_tier}, request)Wrap the audit call (and, as already done for Casbin, treat it as best-effort):
try: record_audit_event(org_id, user_id, "update_plan", "organization", org_id, {"old_tier": old_tier, "new_tier": new_tier}, request) except Exception as e: logger.warning("Failed to record plan-update audit event: %s", e)🤖 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/routes/org_routes.py` around lines 1239 - 1252, The plan update handler in org_routes.py is letting post-commit audit failures turn a successful downgrade into a 500 response. Move the record_audit_event call in the update_plan flow out of the outer failure path by wrapping it in its own best-effort try/except, similar to the existing Casbin cleanup around get_user_roles_in_org/remove_role_from_user, and log a warning instead of propagating so the committed plan change still returns success.
🤖 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.
Outside diff comments:
In `@server/routes/org_routes.py`:
- Around line 1239-1252: The plan update handler in org_routes.py is letting
post-commit audit failures turn a successful downgrade into a 500 response. Move
the record_audit_event call in the update_plan flow out of the outer failure
path by wrapping it in its own best-effort try/except, similar to the existing
Casbin cleanup around get_user_roles_in_org/remove_role_from_user, and log a
warning instead of propagating so the committed plan change still returns
success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e73d0887-9ab1-49e8-a80d-dfafb106214a
📒 Files selected for processing (4)
client/src/app/org/components/OrgMembers.tsxclient/src/app/org/components/OrgOverview.tsxclient/src/app/org/components/UpgradeModal.tsxserver/routes/org_routes.py
![arvo-ai-staging[bot]](https://avatars.githubusercontent.com/in/4088260?s=60&v=4){kind=small}
There was a problem hiding this comment.
Aurora Risk Review — Latest changes
Verdict: RISKY
Reviewed the latest changes. This single-file diff introduces a SQL UPDATE that references organizations.plan_tier, a column for which no migration exists anywhere in the repository — two independent code searches returned zero results. Deploying this to production will cause a column "plan_tier" does not exist PostgreSQL error on every call to GET /api/orgs/invitations, breaking invitation management for all organizations. A secondary concern is that this destructive write (bulk-expiring all pending invitations for free-tier orgs) fires on every read of the invitations list, not just on plan-change events.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | HIGH | server/routes/org_routes.py:663 |
SQL references `organizations.plan_tier` column that has no migration in the repo |
| 2 | MEDIUM | server/routes/org_routes.py:663 |
Destructive bulk-expiry of pending invitations fires on every GET /invitations read |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/routes/org_routes.py (2)
1247-1262: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPost-commit audit failure returns 500 for an already-applied change.
conn.commit()at Line 1247 persists the tier change, butrecord_audit_eventat Line 1256 runs unguarded inside the sametry. If it raises, the handler returns500 "Failed to update plan"(and logs an exception) even though the downgrade already succeeded, misleading the caller. Guard the audit call like the Casbin cleanup already is.Proposed fix
- record_audit_event(org_id, user_id, "update_plan", "organization", org_id, - {"old_tier": old_tier, "new_tier": new_tier}, request) + try: + record_audit_event(org_id, user_id, "update_plan", "organization", org_id, + {"old_tier": old_tier, "new_tier": new_tier}, request) + except Exception as e: + logger.warning("Failed to record plan-update audit event for org %s: %s", org_id, e)🤖 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/routes/org_routes.py` around lines 1247 - 1262, The plan update flow in update_plan currently treats record_audit_event as part of the main success path, so an audit failure after conn.commit() makes the endpoint return a 500 even though the tier change already succeeded. Move the audit call out of the main failure path or wrap it in its own try/except, similar to the Casbin cleanup loop, so the response still returns the updated planTier when the commit succeeds. Use the update_plan handler and record_audit_event call as the key places to adjust.
1176-1241: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict plan downgrades to the creator or preserve the caller
@require_permission("org", "manage")lets any org admin reach this path, but the cleanup only exemptscreator_id. A non-creator admin who downgrades the org will be removed from the org and have their tokens/connections purged, which effectively locks them out. Gate this onuser_id == creator_idor change the cleanup to preserve the actor instead of hardcoding the creator.🤖 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/routes/org_routes.py` around lines 1176 - 1241, The downgrade flow in update_plan currently preserves only creator_id, which can purge the acting admin if they are not the creator. Update the enterprise-to-free cleanup in org_routes.update_plan to either require user_id == creator_id before allowing the downgrade or treat the caller as the preserved account when deleting org data, and make sure the user/token/connection cleanup queries and the final role/org_id updates use that preserved actor consistently.
🤖 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.
Outside diff comments:
In `@server/routes/org_routes.py`:
- Around line 1247-1262: The plan update flow in update_plan currently treats
record_audit_event as part of the main success path, so an audit failure after
conn.commit() makes the endpoint return a 500 even though the tier change
already succeeded. Move the audit call out of the main failure path or wrap it
in its own try/except, similar to the Casbin cleanup loop, so the response still
returns the updated planTier when the commit succeeds. Use the update_plan
handler and record_audit_event call as the key places to adjust.
- Around line 1176-1241: The downgrade flow in update_plan currently preserves
only creator_id, which can purge the acting admin if they are not the creator.
Update the enterprise-to-free cleanup in org_routes.update_plan to either
require user_id == creator_id before allowing the downgrade or treat the caller
as the preserved account when deleting org data, and make sure the
user/token/connection cleanup queries and the final role/org_id updates use that
preserved actor consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26c213ca-acd2-46c6-8bf7-18d65cdd9203
📒 Files selected for processing (1)
server/routes/org_routes.py
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 `@server/routes/org_routes.py`:
- Around line 1183-1210: The downgrade cleanup in the org removal flow does not
handle a missing creator because the queries in the member-purge path use
creator_id from the created_by field, so NULL makes the filters skip all rows.
Update the org downgrade logic around the cleanup helper used by this route to
either reject the free-tier downgrade when organizations.created_by is absent or
fall back to purging all non-owner members, and make sure the DELETE/UPDATE
statements and _purge_vault_secrets handling still target the correct users.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e79ecc1-4fb7-4330-be50-96b45fef87ff
📒 Files selected for processing (1)
server/routes/org_routes.py
|
3 participants
Add plan_tier column to organizations (defaults to free)
Free tier limited to 1 seat (org creator only); enterprise unlimited
Invite button shows upgrade modal with cal.com booking link on free tier
Upgrade modal accessible from both Members tab and Overview settings
PATCH /api/orgs/plan route handles downgrade cleanup (revokes non-creator access, resets Casbin roles, expires invitations)
Paywall bypassed entirely when AURORA_ENV=dev
Summary by CodeRabbit
New Features
Bug Fixes