feat: replace string-based role checks with CASL ability system#519
Conversation
Greptile SummaryReplaces scattered string-based role checks with a centralized CASL ability system. Creates Key improvements:
Issue found:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| services/platform/lib/permissions/ability.ts | Defines CASL ability system with role-based permissions; logic issue with developer role |
| services/platform/lib/shared/schemas/users.ts | Adds strict type-safe role constants and Zod schema |
| services/platform/app/routes/dashboard/$id.tsx | Provides CASL ability via context, stable across auth refreshes using ref |
| services/platform/app/hooks/use-navigation-items.ts | Replaces roles array with typed can tuples; removes hasRequiredRole helper |
| services/platform/app/components/ui/navigation/navigation.tsx | Uses useAbility() for permission checks; removes role prop |
| services/platform/app/components/ui/navigation/tab-navigation.tsx | Replaces role-based filtering with ability checks; removes userRole prop and inline hasRequiredRole |
| services/platform/app/routes/dashboard/$id/custom-agents.tsx | Replaces inline role checks with ability.cannot('write', 'customAgents') |
| services/platform/app/routes/dashboard/$id/automations/index.tsx | Replaces role string comparisons with ability.cannot('write', 'wfDefinitions') |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User loads dashboard] --> B[Dashboard Layout]
B --> C[Get memberContext.role from auth]
C --> D[defineAbilityFor role]
D --> E[Store in abilityRef]
E --> F[Provide via AbilityContext]
F --> G[Navigation Components]
F --> H[Route Guards]
G --> G1[useAbility hook]
G1 --> G2{Check item.can?}
G2 -->|Yes| G3[ability.can action, subject]
G2 -->|No| G4[Show item]
G3 -->|true| G4
G3 -->|false| G5[Hide item]
H --> H1[useAbility hook]
H1 --> H2[ability.cannot action, subject]
H2 -->|true| H3[Show AccessDenied]
H2 -->|false| H4[Render page content]
style D fill:#e1f5ff
style F fill:#e1f5ff
style G3 fill:#fff4e6
style H2 fill:#fff4e6
Last reviewed commit: 2221f35
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case 'developer': { | ||
| can('read', 'all'); | ||
| can('write', 'all'); | ||
| can('read', 'developerSettings'); | ||
| can('write', 'knowledgeWrite'); | ||
| // developers cannot manage org settings or members | ||
| cannot('read', 'orgSettings'); | ||
| cannot('write', 'members'); | ||
| break; |
There was a problem hiding this comment.
contradictory permissions for developer role — can('write', 'all') on line 75 grants write access to all resources, but cannot('read', 'orgSettings') on line 79 attempts to revoke read access to org settings. CASL processes rules in order, so the explicit cannot should work, but the can('write', 'all') is semantically incorrect since developers shouldn't have write access to org settings resources
| case 'developer': { | |
| can('read', 'all'); | |
| can('write', 'all'); | |
| can('read', 'developerSettings'); | |
| can('write', 'knowledgeWrite'); | |
| // developers cannot manage org settings or members | |
| cannot('read', 'orgSettings'); | |
| cannot('write', 'members'); | |
| break; | |
| case 'developer': { | |
| can('read', 'all'); | |
| can('write', 'all'); | |
| can('read', 'developerSettings'); | |
| can('write', 'knowledgeWrite'); | |
| // developers cannot manage org settings or members | |
| cannot('read', 'orgSettings'); | |
| cannot('write', 'orgSettings'); | |
| cannot('write', 'members'); | |
| break; | |
| } |
📝 WalkthroughWalkthroughThis PR migrates the platform's authorization system from role-based prop-passing to a CASL ability-based context model. It introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/platform/convex/users/types.ts (1)
3-36: 🧹 Nitpick | 🔵 Trivial
role?: stringin the interfaces could usePlatformRolefor compile-time safety.Now that
PlatformRoleis the canonical role type (a string literal union), using it inCreateUserWithoutSessionArgs,CreateMemberArgs, andAddMemberInternalArgswould catch invalid role strings at call sites without any runtime cost.♻️ Proposed refactor
+import type { PlatformRole } from '../../lib/shared/schemas/users'; + export type { PlatformRole as Role } from '../../lib/shared/schemas/users'; export interface CreateUserWithoutSessionArgs { email: string; password: string; name?: string; organizationId: string; - role?: string; + role?: PlatformRole; displayName?: string; } ... export interface CreateMemberArgs { organizationId: string; email: string; password: string; displayName?: string; - role?: string; + role?: PlatformRole; } ... export interface AddMemberInternalArgs { organizationId: string; email: string; identityId: string; - role?: string; + role?: PlatformRole; displayName?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/convex/users/types.ts` around lines 3 - 36, Replace the loose string-typed role fields with the canonical PlatformRole type: update role?: string to role?: PlatformRole in the interfaces CreateUserWithoutSessionArgs, CreateMemberArgs, and AddMemberInternalArgs, and add the appropriate import for the PlatformRole type at the top of the file so the compiler enforces the string-literal union for role values.services/platform/app/components/ui/navigation/tab-navigation.stories.tsx (2)
40-46:⚠️ Potential issue | 🟡 MinorStale documentation: "Role-based visibility" should be updated.
Line 42 still says "Role-based visibility" but the feature has been migrated to ability-based checks. Consider updating to "Ability-based visibility" or "Permission-based visibility".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/components/ui/navigation/tab-navigation.stories.tsx` around lines 40 - 46, Update the stale feature text in the stories documentation: locate the features list in tab-navigation.stories.tsx (the template literal that currently includes "Role-based visibility") and replace that line with the updated wording such as "Ability-based visibility" (or "Permission-based visibility") so the documentation reflects the migrated ability-based checks used by the TabNavigation component.
133-150:⚠️ Potential issue | 🔴 Critical
WithMemberAbilitystory description will not match actual behavior.The description claims "The org settings tab is hidden for members," but
defineAbilityFor('member')grantscan('read', 'all'). In CASL,'all'matches every subject — soability.can('read', 'orgSettings')returnstruefor members. The Admin tab will actually be visible, contradicting the story. See the related comment onability.tsfor the root cause and fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/components/ui/navigation/tab-navigation.stories.tsx` around lines 133 - 150, The story text for WithMemberAbility is inaccurate: defineAbilityFor('member') currently grants can('read','all') so org settings will be visible; either update the story description to state that the org settings/Admin tab is visible for members (reference WithMemberAbility, defineAbilityFor, itemsWithAbilityCheck) or change the ability logic in ability.ts so a 'member' does not receive read 'all' (so ability.can('read','orgSettings') returns false) depending on intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/context/ability-context.tsx`:
- Line 1: Remove the top-level "'use client';" directive from the AbilityContext
module since `@vitejs/plugin-react` doesn't treat RSC boundary directives and the
string is a no-op; if you actually intend this file to be a client-only React
Server Component boundary instead, add/configure the appropriate RSC-supporting
plugin for Vite rather than leaving the bare "'use client';" expression in
ability-context.tsx.
- Line 7: The default AbilityContext currently uses
createContext<AppAbility>(defineAbilityFor(null)) which coerces null to 'member'
and grants broad read-all permissions; change the default to a disabled sentinel
by using defineAbilityFor('disabled') so the context fails closed if
useAbility() is called outside an AbilityContext.Provider (update the
AbilityContext initializer to call defineAbilityFor('disabled') and verify any
tests/components using AbilityContext.Provider still supply real roles).
In `@services/platform/app/routes/dashboard/`$id.tsx:
- Around line 39-48: The current code calls defineAbilityFor on every render (in
the useRef initializer and again when memberContext?.role is truthy), causing a
new ability object each render and forcing AbilityContext.Provider to propagate
changes; fix this by deriving the ability only when the role actually changes:
compute the ability for memberContext?.role once (e.g., via useMemo or in an
effect that compares previous role) and only assign abilityRef.current when the
role value differs from the last assigned role so abilityRef.current remains
referentially stable across unrelated re-renders; target symbols:
defineAbilityFor, abilityRef, memberContext?.role, and AbilityContext.Provider.
In `@services/platform/lib/permissions/ability.ts`:
- Around line 64-71: In the admin role case inside the ability building logic
(the case 'admin' block) you have redundant rules: can('read','orgSettings'),
can('read','developerSettings'), can('write','knowledgeWrite'),
can('write','members') are subsumed by can('read','all') and can('write','all');
either remove these redundant can(...) calls and add a short comment next to
can('read','all')/can('write','all') noting that 'all' covers all subjects, or
replace can('read'/'write','all') with an explicit list of subjects
(orgSettings, developerSettings, knowledgeWrite, members, etc.) to make
permissions explicit; update the case 'admin' block accordingly and keep the
intent clear in a one-line comment.
- Line 9: Remove the unused re-export of PlatformRole from this module: delete
the line exporting PlatformRole from
services/platform/lib/permissions/ability.ts (export type { PlatformRole };) so
the module no longer exposes an unused symbol, or alternatively if this module
should be the canonical permissions surface, update all consumers to import
PlatformRole from this module instead of from "@/lib/shared/schemas/users" and
then keep the export; ensure references are changed to the ability.ts export if
you choose the latter.
- Around line 57-61: The current fallback in defineAbilityFor uses (role ??
'member').toLowerCase() which causes unauthenticated or loading states to be
treated as 'member' (and get can('read','all')); change the fallback to an empty
or unknown string so the normalized role falls through to the
default/least-privilege branch — e.g., replace the initialization of normalized
in defineAbilityFor to use (role ?? '').toLowerCase() (or another explicit
'unknown' token) so unauthenticated contexts do not receive member permissions
and hit the existing default case that grants no permissions.
- Around line 119-122: The 'member' branch currently calls can('read', 'all')
which unintentionally grants access to org/developer settings and related tabs;
update the 'member' case in ability.ts to keep can('read', 'all') but explicitly
deny sensitive types by calling cannot('read', 'orgSettings'), cannot('read',
'developerSettings') and also cannot('read' , 'teams'), cannot('read',
'branding'), cannot('read', 'integrations'), cannot('read', 'apiKeys') (use the
same CASL API as the existing 'developer' case) so members cannot view those
sections; also remove the unused PlatformRole re-export symbol to stop
re-exporting PlatformRole (consumers import it from `@/lib/shared/schemas/users`).
In `@services/platform/lib/shared/schemas/users.ts`:
- Around line 3-9: PLATFORM_ROLES is missing the 'owner' role which causes
roleSchema.safeParse('owner') to fail; update the PLATFORM_ROLES const array
(symbol PLATFORM_ROLES in services/platform/lib/shared/schemas/users.ts) to
include 'owner' alongside 'admin','developer','editor','member','disabled' so
validation and authorization checks (e.g., code paths that call
roleSchema.safeParse and references in team_members/mutations.ts and
set_member_password.ts) accept existing owner roles.
In `@services/platform/package.json`:
- Line 40: Remove the unused `@casl/react` dependency from package.json (the line
containing "@casl/react": "^5.0.1",) because the codebase uses a homegrown
AbilityContext and useAbility hook; leave `@casl/ability` if present, run the
package manager to update lockfile (npm/yarn/pnpm install) and ensure no imports
reference `@casl/react` (search for "Can" or "useAbility" from `@casl/react`) before
committing.
---
Outside diff comments:
In `@services/platform/app/components/ui/navigation/tab-navigation.stories.tsx`:
- Around line 40-46: Update the stale feature text in the stories documentation:
locate the features list in tab-navigation.stories.tsx (the template literal
that currently includes "Role-based visibility") and replace that line with the
updated wording such as "Ability-based visibility" (or "Permission-based
visibility") so the documentation reflects the migrated ability-based checks
used by the TabNavigation component.
- Around line 133-150: The story text for WithMemberAbility is inaccurate:
defineAbilityFor('member') currently grants can('read','all') so org settings
will be visible; either update the story description to state that the org
settings/Admin tab is visible for members (reference WithMemberAbility,
defineAbilityFor, itemsWithAbilityCheck) or change the ability logic in
ability.ts so a 'member' does not receive read 'all' (so
ability.can('read','orgSettings') returns false) depending on intended behavior.
In `@services/platform/convex/users/types.ts`:
- Around line 3-36: Replace the loose string-typed role fields with the
canonical PlatformRole type: update role?: string to role?: PlatformRole in the
interfaces CreateUserWithoutSessionArgs, CreateMemberArgs, and
AddMemberInternalArgs, and add the appropriate import for the PlatformRole type
at the top of the file so the compiler enforces the string-literal union for
role values.
| @@ -0,0 +1,7 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
'use client' is a no-op in this Vite + TanStack Router project.
@vitejs/plugin-react does not process RSC boundary directives; the string is evaluated as a bare expression statement and has no effect. If RSC support is planned, add the relevant plugin; otherwise remove the directive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/context/ability-context.tsx` at line 1, Remove the
top-level "'use client';" directive from the AbilityContext module since
`@vitejs/plugin-react` doesn't treat RSC boundary directives and the string is a
no-op; if you actually intend this file to be a client-only React Server
Component boundary instead, add/configure the appropriate RSC-supporting plugin
for Vite rather than leaving the bare "'use client';" expression in
ability-context.tsx.
|
|
||
| import { defineAbilityFor, type AppAbility } from '@/lib/permissions/ability'; | ||
|
|
||
| export const AbilityContext = createContext<AppAbility>(defineAbilityFor(null)); |
There was a problem hiding this comment.
Fail-open default: defineAbilityFor(null) silently grants member-level read-all permissions.
Inside defineAbilityFor, null is coerced to 'member' via role ?? 'member', which grants can('read', 'all'). Any component that calls useAbility() outside the AbilityContext.Provider (e.g., a shared component rendered in a non-dashboard context, a missing provider during future refactors) will silently pass every ability.cannot('read', X) check and never show AccessDenied.
The context default should fail closed — defineAbilityFor('disabled') hits the default branch and grants zero permissions, which is the correct sentinel for "no ability set yet."
🛡️ Proposed fix
-export const AbilityContext = createContext<AppAbility>(defineAbilityFor(null));
+export const AbilityContext = createContext<AppAbility>(defineAbilityFor('disabled'));📝 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.
| export const AbilityContext = createContext<AppAbility>(defineAbilityFor(null)); | |
| export const AbilityContext = createContext<AppAbility>(defineAbilityFor('disabled')); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/context/ability-context.tsx` at line 7, The default
AbilityContext currently uses createContext<AppAbility>(defineAbilityFor(null))
which coerces null to 'member' and grants broad read-all permissions; change the
default to a disabled sentinel by using defineAbilityFor('disabled') so the
context fails closed if useAbility() is called outside an
AbilityContext.Provider (update the AbilityContext initializer to call
defineAbilityFor('disabled') and verify any tests/components using
AbilityContext.Provider still supply real roles).
| // Preserve the last known ability across auth token refreshes / WebSocket reconnections. | ||
| // When convexQuery args change to 'skip', the queryKey changes and data becomes undefined, | ||
| // which would cause role-gated nav items to briefly disappear. | ||
| const roleRef = useRef(memberContext?.role); | ||
| const abilityRef = useRef<AppAbility>(defineAbilityFor(memberContext?.role)); | ||
| if (memberContext?.role) { | ||
| roleRef.current = memberContext.role; | ||
| abilityRef.current = defineAbilityFor(memberContext.role); | ||
| } | ||
|
|
||
| return ( | ||
| <BrandingProvider | ||
| organizationId={organizationId} | ||
| skip={isAuthLoading || !isAuthenticated} | ||
| > | ||
| <TeamFilterProvider organizationId={organizationId}> | ||
| <AdaptiveHeaderProvider> | ||
| <div className="flex size-full flex-col overflow-hidden md:flex-row"> | ||
| <div className="bg-background flex h-[--nav-size] items-center gap-2 p-2 md:hidden"> | ||
| <MobileNavigation | ||
| organizationId={organizationId} | ||
| role={roleRef.current} | ||
| /> | ||
| <AdaptiveHeaderSlot /> | ||
| </div> | ||
| <AbilityContext.Provider value={abilityRef.current}> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
defineAbilityFor is called on every render — consider memoizing the role-to-ability derivation.
Lines 42 and 44 both call defineAbilityFor every render. The useRef initializer argument is always evaluated (but only used on mount), and the conditional on line 43–44 rebuilds on every render when role is truthy. Each call produces a new object reference, so the <AbilityContext.Provider value={…}> will signal a context change to all useAbility() consumers on every parent re-render, even if the role hasn't changed.
You can preserve the "keep last known ability" intent while avoiding unnecessary context propagation:
♻️ Suggested refactor
- const abilityRef = useRef<AppAbility>(defineAbilityFor(memberContext?.role));
- if (memberContext?.role) {
- abilityRef.current = defineAbilityFor(memberContext.role);
- }
+ const lastRoleRef = useRef<string | undefined>(undefined);
+ const abilityRef = useRef<AppAbility>(defineAbilityFor(null));
+ if (memberContext?.role && memberContext.role !== lastRoleRef.current) {
+ lastRoleRef.current = memberContext.role;
+ abilityRef.current = defineAbilityFor(memberContext.role);
+ }This skips the rebuild when the role is unchanged, keeping abilityRef.current referentially stable and avoiding cascading re-renders across every useAbility() consumer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/routes/dashboard/`$id.tsx around lines 39 - 48, The
current code calls defineAbilityFor on every render (in the useRef initializer
and again when memberContext?.role is truthy), causing a new ability object each
render and forcing AbilityContext.Provider to propagate changes; fix this by
deriving the ability only when the role actually changes: compute the ability
for memberContext?.role once (e.g., via useMemo or in an effect that compares
previous role) and only assign abilityRef.current when the role value differs
from the last assigned role so abilityRef.current remains referentially stable
across unrelated re-renders; target symbols: defineAbilityFor, abilityRef,
memberContext?.role, and AbilityContext.Provider.
|
|
||
| import { type PlatformRole } from '@/lib/shared/schemas/users'; | ||
|
|
||
| export type { PlatformRole }; |
There was a problem hiding this comment.
Pipeline failure: unused re-export of PlatformRole.
Knip reports this export as unused. Consumers import PlatformRole directly from @/lib/shared/schemas/users rather than from this module. Either remove the re-export or update consumers to import from here if this module is intended to be the canonical permissions API surface.
Option A: Remove the re-export
-export type { PlatformRole };
-📝 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.
| export type { PlatformRole }; |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 9-9: Knip: Unused exported type 'PlatformRole' detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/permissions/ability.ts` at line 9, Remove the unused
re-export of PlatformRole from this module: delete the line exporting
PlatformRole from services/platform/lib/permissions/ability.ts (export type {
PlatformRole };) so the module no longer exposes an unused symbol, or
alternatively if this module should be the canonical permissions surface, update
all consumers to import PlatformRole from this module instead of from
"@/lib/shared/schemas/users" and then keep the export; ensure references are
changed to the ability.ts export if you choose the latter.
| export function defineAbilityFor(role?: string | null): AppAbility { | ||
| const { can, cannot, build } = new AbilityBuilder<AppAbility>( | ||
| createMongoAbility, | ||
| ); | ||
| const normalized = (role ?? 'member').toLowerCase(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "defineAbilityFor" --type=ts -C3Repository: tale-project/tale
Length of output: 5017
🏁 Script executed:
cat -n services/platform/lib/permissions/ability.tsRepository: tale-project/tale
Length of output: 4451
Default role 'member' grants full read access — consider defensive fallback.
Confirmed: (role ?? 'member').toLowerCase() defaults to the 'member' role, which grants can('read', 'all'). This means unauthenticated or loading states (e.g., AbilityContext initialization at line 7 of ability-context.tsx) get full read access to all platform resources.
If the defensive posture is preferred, default to an unknown/empty role instead to fall through to the default case (lines 123–126), which grants zero permissions. This prevents unauth/loading states from reading sensitive data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/permissions/ability.ts` around lines 57 - 61, The
current fallback in defineAbilityFor uses (role ?? 'member').toLowerCase() which
causes unauthenticated or loading states to be treated as 'member' (and get
can('read','all')); change the fallback to an empty or unknown string so the
normalized role falls through to the default/least-privilege branch — e.g.,
replace the initialization of normalized in defineAbilityFor to use (role ??
'').toLowerCase() (or another explicit 'unknown' token) so unauthenticated
contexts do not receive member permissions and hit the existing default case
that grants no permissions.
| case 'admin': { | ||
| can('read', 'all'); | ||
| can('write', 'all'); | ||
| can('read', 'orgSettings'); | ||
| can('read', 'developerSettings'); | ||
| can('write', 'knowledgeWrite'); | ||
| can('write', 'members'); | ||
| break; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Admin case has redundant can rules after can('read'/'write', 'all').
Lines 67–70 grant specific permissions that are already covered by can('read', 'all') and can('write', 'all') on lines 65–66. While not a bug, this is misleading — it suggests 'all' doesn't cover these subjects, when in CASL it does. Consider either removing the redundant rules (with a comment explaining 'all' covers everything) or switching away from 'all' to explicit subject lists for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/permissions/ability.ts` around lines 64 - 71, In the
admin role case inside the ability building logic (the case 'admin' block) you
have redundant rules: can('read','orgSettings'),
can('read','developerSettings'), can('write','knowledgeWrite'),
can('write','members') are subsumed by can('read','all') and can('write','all');
either remove these redundant can(...) calls and add a short comment next to
can('read','all')/can('write','all') noting that 'all' covers all subjects, or
replace can('read'/'write','all') with an explicit list of subjects
(orgSettings, developerSettings, knowledgeWrite, members, etc.) to make
permissions explicit; update the case 'admin' block accordingly and keep the
intent clear in a one-line comment.
| case 'member': { | ||
| can('read', 'all'); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CASL ability "can read all" does it match specific string subjects
💡 Result:
In CASL, can('read', 'all') is a special wildcard rule: the subject type 'all' means “any subject type”, so it will allow reads on any specific subject string (e.g., 'Post', 'User', etc.) unless a more specific cannot overrides it. [1][2]
It does not do pattern/substring matching on subject strings (e.g., it won’t match only subjects that “contain” some text). Subject types are treated as explicit identifiers (typically exact strings/classes), and 'all' is just the built-in “match everything” subject type. [1][3]
Examples
can('read', 'all')⇒ability.can('read', 'Post')is true. [1]can('read', 'all'); cannot('read', 'Post')⇒ability.can('read', 'Post')is false (override). [2]
Sources:
[1] Glimpse API Wiki – CASL Integration (subject types, built-in all)
[2] @casl/ability changelog (examples showing read all and overrides)
[3] Glasswork CASL abilities docs (example can('manage','all') and checking other subjects)
🏁 Script executed:
# Locate and read the ability.ts file
find . -type f -name "ability.ts" | head -5Repository: tale-project/tale
Length of output: 107
🏁 Script executed:
# Search for settings-navigation.tsx and related UI components checking permissions
fd "settings-navigation" -o ".*settings.*tsx" | head -10Repository: tale-project/tale
Length of output: 210
🏁 Script executed:
# Search for how member permissions are tested or validated
rg "member.*read.*orgSettings\|orgSettings.*member" -t ts -t tsx -A 2 -B 2Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Read the ability.ts file
cat -n ./services/platform/lib/permissions/ability.tsRepository: tale-project/tale
Length of output: 4451
🏁 Script executed:
# Search for settings navigation file
fd settings-navigationRepository: tale-project/tale
Length of output: 135
🏁 Script executed:
# Search for tests related to member permissions
rg "member.*permission\|case.*member" -t ts --max-count=20Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Read settings-navigation component
cat -n ./services/platform/app/features/settings/components/settings-navigation.tsxRepository: tale-project/tale
Length of output: 2571
🏁 Script executed:
# Search for usage of PlatformRole import from ability.ts
rg "from.*ability.*PlatformRole\|import.*PlatformRole.*ability" -t ts -t tsxRepository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Check for tests of ability permissions
fd "*ability*test\|*test*ability" -o "*ability*.spec.*"Repository: tale-project/tale
Length of output: 212
🏁 Script executed:
# Search for all PlatformRole imports across the repo
rg "PlatformRole" -t ts -B 1 -A 1 --max-count=30Repository: tale-project/tale
Length of output: 12147
🏁 Script executed:
# Search for ability tests or permission-related test files
fd "test\|spec" | grep -i "ability\|permission" | head -10Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Search for TabNavigation to see how the 'can' property is validated
rg "TabNavigation\|can:.*read" -t ts -t tsx -B 2 -A 2 | head -50Repository: tale-project/tale
Length of output: 88
Critical: member role unintentionally grants access to org/developer settings.
In CASL, can('read', 'all') makes ability.can('read', 'orgSettings') and ability.can('read', 'developerSettings') return true. The developer case correctly uses cannot('read', 'orgSettings') to override, but member has no such restrictions. This means members will see organization settings, teams, branding, integrations, and API keys tabs — contradicting the intended permission model.
Add restriction rules to the member case:
case 'member': {
can('read', 'all');
+ cannot('read', 'orgSettings');
+ cannot('read', 'developerSettings');
+ cannot('write', 'all');
break;
}Additionally, remove the unused PlatformRole re-export at line 9; all consumers import it directly from @/lib/shared/schemas/users.
📝 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.
| case 'member': { | |
| can('read', 'all'); | |
| break; | |
| } | |
| case 'member': { | |
| can('read', 'all'); | |
| cannot('read', 'orgSettings'); | |
| cannot('read', 'developerSettings'); | |
| cannot('write', 'all'); | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/permissions/ability.ts` around lines 119 - 122, The
'member' branch currently calls can('read', 'all') which unintentionally grants
access to org/developer settings and related tabs; update the 'member' case in
ability.ts to keep can('read', 'all') but explicitly deny sensitive types by
calling cannot('read', 'orgSettings'), cannot('read', 'developerSettings') and
also cannot('read' , 'teams'), cannot('read', 'branding'), cannot('read',
'integrations'), cannot('read', 'apiKeys') (use the same CASL API as the
existing 'developer' case) so members cannot view those sections; also remove
the unused PlatformRole re-export symbol to stop re-exporting PlatformRole
(consumers import it from `@/lib/shared/schemas/users`).
| export const PLATFORM_ROLES = [ | ||
| 'admin', | ||
| 'developer', | ||
| 'editor', | ||
| 'member', | ||
| 'disabled', | ||
| ] as const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to 'owner' role in authorization/role contexts
echo "=== Searching for owner role in mutations and queries ==="
rg -n "owner" --type=ts --type=tsx -C 2 | grep -iE "(role|owner|admin|auth|remove)" | head -50
echo ""
echo "=== Checking team_members mutations specifically ==="
rg -n "owner" services/platform/convex/team_members/ --type=ts
echo ""
echo "=== Searching for role enums or type definitions ==="
rg -n "type.*Role|enum.*Role" --type=ts --type=tsx -A 3 | head -40
echo ""
echo "=== Checking all PLATFORM_ROLES usages ==="
rg -n "PLATFORM_ROLES" --type=ts --type=tsx -B 2 -A 2 | head -50Repository: tale-project/tale
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Get context around the owner role checks in mutations.ts
echo "=== Context around line 26 (removeMember check) ==="
sed -n '20,35n' services/platform/convex/team_members/mutations.ts
echo ""
echo "=== Context around line 119 (another check) ==="
sed -n '110,125n' services/platform/convex/team_members/mutations.ts
echo ""
echo "=== Check what roleSchema is used for ==="
rg -n "roleSchema" --type=ts services/platform/ -B 2 -A 3Repository: tale-project/tale
Length of output: 3123
🏁 Script executed:
#!/bin/bash
# Get the mutations file and search for owner role context
echo "=== Full mutations.ts file (check total lines) ==="
wc -l services/platform/convex/team_members/mutations.ts
echo ""
echo "=== Search for all owner references with context ==="
rg -n "'owner'" services/platform/convex/team_members/mutations.ts -B 5 -A 3
echo ""
echo "=== Check if there's an old PLATFORM_ROLES or role definitions ==="
find services/platform -name "*.ts" -o -name "*.tsx" | xargs grep -l "owner.*admin" | head -10Repository: tale-project/tale
Length of output: 910
🏁 Script executed:
#!/bin/bash
# Check rls/types.ts for role definitions
echo "=== rls/types.ts content ==="
cat services/platform/convex/lib/rls/types.ts
echo ""
echo "=== Check if owner role is used anywhere else ==="
rg -n "owner" services/platform/convex/ --type=ts -i | grep -i "role\|auth\|member"Repository: tale-project/tale
Length of output: 2338
Add 'owner' to the PLATFORM_ROLES array.
The codebase actively references the 'owner' role in authorization checks (team_members/mutations.ts lines 26 and 119, set_member_password.ts line 70), and it's documented in rls/types.ts as a valid Better Auth organization role. Omitting it from PLATFORM_ROLES causes roleSchema.safeParse('owner') to fail, breaking validation for existing members with the owner role. Add 'owner' to the array to align with the schema validation and authorization logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/shared/schemas/users.ts` around lines 3 - 9,
PLATFORM_ROLES is missing the 'owner' role which causes
roleSchema.safeParse('owner') to fail; update the PLATFORM_ROLES const array
(symbol PLATFORM_ROLES in services/platform/lib/shared/schemas/users.ts) to
include 'owner' alongside 'admin','developer','editor','member','disabled' so
validation and authorization checks (e.g., code paths that call
roleSchema.safeParse and references in team_members/mutations.ts and
set_member_password.ts) accept existing owner roles.
| "@ai-sdk/openai": "2.0.89", | ||
| "@ai-sdk/provider-utils": "3.0.20", | ||
| "@casl/ability": "^6.8.0", | ||
| "@casl/react": "^5.0.1", |
There was a problem hiding this comment.
Remove @casl/react — unused dependency causing pipeline failure.
The Knip lint pipeline flags @casl/react as unused. The codebase implements its own AbilityContext and useAbility hook via React's native createContext / useContext, so the @casl/react React integration package (Can component, CASL's own useAbility) is never consumed. Only @casl/ability is actually needed.
🐛 Proposed fix
- "@casl/react": "^5.0.1",📝 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.
| "@casl/react": "^5.0.1", |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 40-40: Knip: Unused dependency '@casl/react' detected in dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/package.json` at line 40, Remove the unused `@casl/react`
dependency from package.json (the line containing "@casl/react": "^5.0.1",)
because the codebase uses a homegrown AbilityContext and useAbility hook; leave
`@casl/ability` if present, run the package manager to update lockfile
(npm/yarn/pnpm install) and ensure no imports reference `@casl/react` (search for
"Can" or "useAbility" from `@casl/react`) before committing.
Introduces a unified CASL permission layer that replaces scattered inline role comparisons, duplicated hasRequiredRole helpers, and raw role string prop-drilling throughout the dashboard. - Add lib/permissions/ability.ts with defineAbilityFor(role) mirroring the Better Auth permission matrix; exposes PlatformResource, UiSubject, AppAction, AppAbility types - Add AbilityContext + useAbility() hook; provided by the dashboard layout via abilityRef (stable across auth refreshes) - Tighten lib/shared/schemas/users.ts: PLATFORM_ROLES const array, PlatformRole type, strict z.enum roleSchema - Replace NavItem.roles/TabNavigationItem.roles string arrays with typed can: [AppAction, AppSubject] tuples; remove hasRequiredRole - Navigation, MobileNavigation, TabNavigation use useAbility() internally; no longer accept role/userRole props - SettingsNavigation and KnowledgeNavigation drop userRole prop; settings items use can: ['read', 'orgSettings|developerSettings'] - All route guards (custom-agents, automations, knowledge, settings/*) replaced with ability.cannot() checks — no more per-route useCurrentMemberContext role reads - member-edit-dialog uses shared PlatformRole/roleSchema instead of duplicated inline type and isValidRole function
yannickmonney
force-pushed
the
feat/casl-permissions
branch
from
2221f35 to
ae97595
Compare
1 participant
Introduces a unified CASL permission layer that replaces scattered inline role comparisons, duplicated hasRequiredRole helpers, and raw role string prop-drilling throughout the dashboard.
Summary by CodeRabbit
Release Notes