refactor(policy): decentralize subject resolution to modules#3382
Merged
PierreBrisorgueil merged 7 commits intomasterfrom Apr 4, 2026
Merged
refactor(policy): decentralize subject resolution to modules#3382PierreBrisorgueil merged 7 commits intomasterfrom
PierreBrisorgueil merged 7 commits intomasterfrom
Conversation
Replace hardcoded resolveSubject() and deriveSubjectType() in lib/middlewares/policy.js with a registry pattern. Each module policy file now exports a *SubjectRegistration() function that registers its own document-level and path-level CASL subjects during discoverPolicies(). Adds lib/helpers/authorize.js as a simple route-level middleware helper. Closes #3381
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughSwitches policy subject resolution from hardcoded logic to registry-driven resolvers populated by module-exported subject-registration hooks; adds Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Startup
participant Policy as PolicyRegistry
participant Module as ModulePolicy
participant Client as Client
participant Server as Server
participant Controller as Controller
Startup->>Module: discoverPolicies()
Module-->>Policy: call <module>SubjectRegistration({ registerDocumentSubject, registerPathSubject })
Policy-->>Policy: store registry entries
Client->>Server: HTTP request -> route
Server->>Policy: deriveSubjectType(routePath) (iterate path registry)
Policy-->>Server: subjectType
Server->>Server: defineAbilityFor(req.user, subjectType) -> req.ability
Server->>Policy: resolveSubject(req) (iterate document registry)
Policy-->>Server: { subjectType, document }
Server->>Server: authorize(action, subject) middleware checks req.ability.can(action, subject)
alt allowed
Server->>Controller: proceed to controller
else forbidden
Server-->>Client: 403 Forbidden JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3382 +/- ##
==========================================
+ Coverage 84.84% 84.95% +0.10%
==========================================
Files 110 111 +1
Lines 2811 2851 +40
Branches 789 778 -11
==========================================
+ Hits 2385 2422 +37
- Misses 337 339 +2
- Partials 89 90 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors CASL subject resolution to be registry-based and populated by modules at startup, reducing centralized coupling in lib/middlewares/policy.js.
Changes:
- Replaced hardcoded document/path subject resolution in
lib/middlewares/policy.jswith module-populated registries and*SubjectRegistration()hooks invoked duringdiscoverPolicies(). - Updated module policy files to export subject registration functions for document-level and path-level authorization.
- Added
lib/helpers/authorize.jsas a lightweight route-level authorization middleware helper and documented the migration inMIGRATIONS.md.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/middlewares/policy.js |
Adds subject registries + registration helpers; updates resolveSubject()/deriveSubjectType(); invokes *SubjectRegistration() during policy discovery. |
lib/helpers/authorize.js |
Introduces a small route-level authorization middleware helper. |
modules/users/policies/users.policy.js |
Registers /api/users* path subjects for account/self-service routes. |
modules/users/policies/users.admin.policy.js |
Registers admin user document/path subjects for /api/admin/users*. |
modules/tasks/policies/tasks.policy.js |
Registers task document/path subjects for /api/tasks*. |
modules/uploads/policies/uploads.policy.js |
Registers upload document/path subjects for /api/uploads*. |
modules/organizations/policies/organizations.policy.js |
Registers organization/membership document subjects and organization/membership path matchers. |
modules/home/policies/home.policy.js |
Registers home/readiness path subjects. |
modules/billing/policies/billing.policy.js |
Registers exact billing route → subject mappings. |
modules/audit/policies/audit.policy.js |
Registers audit route → subject mapping. |
MIGRATIONS.md |
Adds upgrade notes describing the new registry-based subject resolution and optional authorize() usage. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/helpers/authorize.js`:
- Around line 9-11: The authorize middleware (authorize) currently relies on
req.ability which is never set; add a simple middleware (e.g., computeAbility or
attachAbility) that calls the existing defineAbilityFor(user) helper (or the
same logic used in isAllowed) and assigns the result to req.ability before route
handlers, then ensure routes that use authorize run the new middleware first;
alternatively, change authorize to accept an ability factory
(authorize(abilityOrFactory, action, subject)) or import defineAbilityFor
directly and compute ability inside authorize—pick one approach and update the
middleware name references (authorize, isAllowed, defineAbilityFor,
computeAbility/attachAbility) accordingly so req.ability is initialized before
authorize runs.
In `@lib/middlewares/policy.js`:
- Around line 37-58: Add missing `@returns` to the JSDoc for both registration
helpers: for registerDocumentSubject(documentSubjectRegistry.push...) and
registerPathSubject(pathSubjectRegistry.push...) document that the functions
return {void} (or nothing) and optionally note the side effect of registering an
entry in documentSubjectRegistry or pathSubjectRegistry; ensure the JSDoc block
for registerDocumentSubject includes `@param` for reqProperty, subjectType, guard
and `@returns` {void}, and the JSDoc block for registerPathSubject includes `@param`
for routeMatch, subjectType and `@returns` {void}.
In `@MIGRATIONS.md`:
- Around line 21-24: Update the MIGRATIONS.md section to document the
deprecation window for policy.isAllowed and instruct downstreams to migrate to
authorize(action, subject); specifically mention that policy.isAllowed will be
supported for one release cycle only, recommend replacing middleware usage with
authorize(action, subject) in routes, and remind custom modules to keep their
*SubjectRegistration() export pattern (e.g.,
modules/tasks/policies/tasks.policy.js) while they migrate; include a brief
example phrase showing the migration path and the target release where
policy.isAllowed will be removed.
In `@modules/billing/policies/billing.policy.js`:
- Around line 11-16: The billingSubjectRegistration currently registers five
endpoints but misses the webhook path; add a registerPathSubject call for
'/api/billing/webhook' (e.g., registerPathSubject('/api/billing/webhook',
'BillingWebhook')) in billingSubjectRegistration so policy.isAllowed finds a
subject for that route, or alternatively update modules/billing/preroute.js to
bypass CASL for the webhook route (ensure the webhook handler is exempted from
policy.isAllowed) so external deliveries aren’t blocked.
- Around line 5-10: The JSDoc for the new function billingSubjectRegistration is
missing a `@returns` tag; update the JSDoc block above billingSubjectRegistration
to include "@returns {void}" so the function's return value is documented per
project guidelines, keeping the existing `@param` entries intact.
In `@modules/organizations/policies/organizations.policy.js`:
- Around line 22-23: The current registerPathSubject predicates use loose
substring checks (registerPathSubject((p) => p.includes('/requests'),
'Membership') and similarly for '/members') which can overmatch; replace them
with predicates that assert '/requests' and '/members' are full path segments
(e.g. use a regex test like (p) => /(^|\/)requests(\/|$)/.test(p) and (p) =>
/(^|\/)members(\/|$)/.test(p)) so only routes where those words appear as path
segments are matched by registerPathSubject.
🪄 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: d776ba22-3f9c-4cfc-b8c1-a03518651e00
📒 Files selected for processing (11)
MIGRATIONS.mdlib/helpers/authorize.jslib/middlewares/policy.jsmodules/audit/policies/audit.policy.jsmodules/billing/policies/billing.policy.jsmodules/home/policies/home.policy.jsmodules/organizations/policies/organizations.policy.jsmodules/tasks/policies/tasks.policy.jsmodules/uploads/policies/uploads.policy.jsmodules/users/policies/users.admin.policy.jsmodules/users/policies/users.policy.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/feature/SKILL.md:
- Around line 18-19: Update the SKILL.md guidance to remove the contradictory
blanket prohibition on editing config/ and instead exempt or explicitly allow
adding module-specific templates under config/templates/; replace the current
bullet that forbids "config/" with a clarified line like "No modifications to
shared core files (`lib/middlewares/`, `lib/services/`) — modules may add their
own config files such as email templates in `config/templates/` and register
them via exports (policies, subjects, config)" so reviewers know adding
templates is allowed while shared core config remains off-limits.
🪄 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: 89140ded-93fe-43a9-9516-dcb7f5299167
📒 Files selected for processing (3)
.claude/skills/create-module/SKILL.md.claude/skills/feature/SKILL.md.claude/skills/verify/SKILL.md
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
MIGRATIONS.md (1)
23-26:⚠️ Potential issue | 🟡 MinorClarify the deprecation stance for
policy.isAllowed.The PR objectives state
policy.isAllowedshould be "deprecated/backward-compatible for one release cycle," but line 26 says "No deprecation timeline is set — both approaches coexist." This inconsistency may confuse downstream maintainers about whether they should plan to migrate.Consider aligning the documentation with the intended deprecation strategy:
-3. `policy.isAllowed` continues to work unchanged — no route file modifications needed -4. Optional: use `authorize(action, subject)` from `lib/helpers/authorize.js` for simple route guards +3. `policy.isAllowed` is **deprecated** but remains available for this release cycle +4. Migrate route-level checks to `authorize(action, subject)` from `lib/helpers/authorize.js` -> **Deprecation**: The existing `policy.isAllowed` middleware continues to work unchanged. The new `authorize()` helper is optional and recommended for new routes. No deprecation timeline is set — both approaches coexist. +> **Deprecation**: `policy.isAllowed` remains available for backward compatibility but is deprecated. New routes should use `authorize()`. Plan to migrate existing routes before the next major release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MIGRATIONS.md` around lines 23 - 26, Update the MIGRATIONS.md text to remove the conflicting statement by clearly declaring that policy.isAllowed will be deprecated but remain backward-compatible for one release cycle and that maintainers should migrate to the new authorize(action, subject) helper; replace the sentence "No deprecation timeline is set — both approaches coexist." with a concise deprecation timeline and migration recommendation referencing policy.isAllowed and authorize(action, subject) so readers know to plan for a one-release transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/feature/SKILL.md:
- Line 19: Update the line "Module registers its own capabilities via exports
(policies, subjects, config)" in SKILL.md to clarify that only policies and
subjects are registered via exports, while config files are auto-discovered by
filepath pattern; replace or append the phrase to read something like "policies
and subjects via exports; config files are auto-discovered by filepath pattern
(modules/*/config/*.config.js)" so readers understand the different discovery
mechanisms.
- Line 18: Update the SKILL.md policy to explicitly state whether modifications
to lib/helpers/ are allowed or prohibited (the current list names
lib/middlewares/, lib/services/, config/ but omits lib/helpers/), and include a
short rule covering lib/helpers/ usage and justification requirements; mention
the specific example file lib/helpers/authorize.js and clarify whether adding
helper modules requires the same "explicit justification" as changes to
lib/services/ or is permitted for shared code, and ensure the allowed/prohibited
list includes lib/middlewares/, lib/services/, lib/helpers/, and
config/templates/ so readers cannot be ambiguous.
In `@modules/organizations/policies/organizations.policy.js`:
- Around line 5-25: Add a `@returns` {void} tag to the JSDoc for the
organizationSubjectRegistration function so the header documents the return
type; locate the JSDoc immediately above the exported function
organizationSubjectRegistration and add the `@returns` annotation (keeping
existing `@param` lines for registry, registerDocumentSubject,
registerPathSubject) to satisfy the project guideline that every function has
both `@param` and `@returns` documentation.
In `@modules/users/policies/users.admin.policy.js`:
- Around line 7-16: Update the JSDoc for the exported function
adminSubjectRegistration to include an explicit `@returns` {void} tag; locate the
JSDoc block above adminSubjectRegistration (which documents
registerDocumentSubject and registerPathSubject) and append a line documenting
the return type as void so it conforms to the required /** `@param` ... */ and /**
`@returns` {void} */ guideline.
---
Duplicate comments:
In `@MIGRATIONS.md`:
- Around line 23-26: Update the MIGRATIONS.md text to remove the conflicting
statement by clearly declaring that policy.isAllowed will be deprecated but
remain backward-compatible for one release cycle and that maintainers should
migrate to the new authorize(action, subject) helper; replace the sentence "No
deprecation timeline is set — both approaches coexist." with a concise
deprecation timeline and migration recommendation referencing policy.isAllowed
and authorize(action, subject) so readers know to plan for a one-release
transition.
🪄 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: d7d0d40f-ccc8-4830-ad6e-5d3bd9a64e16
📒 Files selected for processing (6)
.claude/skills/feature/SKILL.mdMIGRATIONS.mdlib/middlewares/policy.jsmodules/billing/policies/billing.policy.jsmodules/organizations/policies/organizations.policy.jsmodules/users/policies/users.admin.policy.js
…feature skill boundaries
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/middlewares/policy.js (1)
257-263:⚠️ Potential issue | 🟠 MajorThe legacy
invokeRolesPolicies()fallback is broken.This branch still claims to support old policy modules, but
policy.jsno longer exposesregisterRulesor any route-rule pipeline. A downstream module that still doesinvokeRolesPolicies() { policy.registerRules(...) }will now fail during bootstrap instead of staying backward-compatible.📝 Safer fallback
- } else if (mod.default && typeof mod.default.invokeRolesPolicies === 'function') { - // Legacy fallback — call old-style registration - mod.default.invokeRolesPolicies(); + } else if (mod.default && typeof mod.default.invokeRolesPolicies === 'function') { + throw new Error( + `${policyPath} still uses invokeRolesPolicies(); migrate it to named *Abilities/*GuestAbilities exports before enabling registry-based subject resolution.` + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/middlewares/policy.js` around lines 257 - 263, The legacy fallback calling mod.default.invokeRolesPolicies() breaks because policy.registerRules/route-rule API is gone; modify the fallback so that if a legacy module implements invokeRolesPolicies, you invoke it with a small adapter object that provides registerRules (or registerRule) which forwards calls to the new registerAbilities function: detect mod.default.invokeRolesPolicies, then call it as mod.default.invokeRolesPolicies({ registerRules: (...args) => registerAbilities(...args) }) (or similarly map registerRule/registerGuestRules to registerAbilities/registerGuestAbilities) so old modules keep working without restoring the old API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/feature/SKILL.md:
- Around line 18-19: Update the bullet that currently reads "No modifications to
shared core files (`lib/middlewares/`, `lib/services/`, `lib/helpers/`,
`config/`) — except `config/templates/` for new email templates; `lib/helpers/`
additions require explicit justification" so it allows justified changes to both
`lib/helpers/` and `lib/services/`; specifically change the wording to permit
modifications to `lib/services/` with the same justification requirement as
`lib/helpers/` (e.g., "No modifications to shared core files ... except
`config/templates/` for new email templates; additions to `lib/helpers/` or
`lib/services/` require explicit justification"), and update any matching
bullets (the other occurrence noted) to the same phrasing to keep policy
consistent.
In `@lib/middlewares/policy.js`:
- Around line 190-192: The current middleware sets req.ability only when
policy.isAllowed runs, so helper authorize(action, subject) (in
lib/helpers/authorize.js) will 403 if req.ability is missing; fix by making
authorize lazily initialize the ability: when req.ability is falsy, call
defineAbilityFor(req.user, req.membership || null) to populate req.ability
before performing the check, or alternatively move that
defineAbilityFor(req.user, req.membership || null) call into a shared middleware
that runs earlier for all requests; update authorize() to use the new lazy-init
or remove the redundant creation if you centralize it.
In `@modules/users/policies/users.admin.policy.js`:
- Around line 15-16: The current registerDocumentSubject('model', 'UserAdmin')
call registers the model subject globally; change it to register the 'model'
subject with a route predicate that only accepts requests whose path starts with
'/api/admin/users' (similar to the existing registerPathSubject usage) so
req.model is only resolved as UserAdmin for admin-user routes; update the
registerDocumentSubject invocation to accept a predicate function (e.g., p =>
p.startsWith('/api/admin/users') or req.path-based predicate) so the
registration is defensively scoped and prevents non-admin routes from being
resolved as UserAdmin.
---
Outside diff comments:
In `@lib/middlewares/policy.js`:
- Around line 257-263: The legacy fallback calling
mod.default.invokeRolesPolicies() breaks because policy.registerRules/route-rule
API is gone; modify the fallback so that if a legacy module implements
invokeRolesPolicies, you invoke it with a small adapter object that provides
registerRules (or registerRule) which forwards calls to the new
registerAbilities function: detect mod.default.invokeRolesPolicies, then call it
as mod.default.invokeRolesPolicies({ registerRules: (...args) =>
registerAbilities(...args) }) (or similarly map registerRule/registerGuestRules
to registerAbilities/registerGuestAbilities) so old modules keep working without
restoring the old API surface.
🪄 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: e795cb3d-cd21-45a3-aa67-a9d541c55cd9
📒 Files selected for processing (5)
.claude/skills/feature/SKILL.mdMIGRATIONS.mdlib/middlewares/policy.jsmodules/organizations/policies/organizations.policy.jsmodules/users/policies/users.admin.policy.js
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…ers/ in feature boundaries
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
modules/users/policies/users.admin.policy.js (1)
14-16:⚠️ Potential issue | 🟡 MinorScope the
modelregistration to/api/admin/users*.
registerDocumentSubject('model', 'UserAdmin')is still global, so any future non-admin request that populatesreq.modelwill resolve asUserAdminbefore the path-based fallback runs. Keep this document registration behind the same admin-route predicate as the path registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/users/policies/users.admin.policy.js` around lines 14 - 16, The document subject registration is currently global: in adminSubjectRegistration you must make the 'model' registration use the same admin-route predicate as the path-based registration instead of registering it unconditionally; replace the call to registerDocumentSubject('model', 'UserAdmin') with a predicate-based registration that only returns 'UserAdmin' when the request path matches the admin prefix (the same predicate used with registerPathSubject, e.g. p => p.startsWith('/api/admin/users')), so both registerDocumentSubject and registerPathSubject are scoped to admin routes only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/helpers/authorize.js`:
- Around line 2-3: Update the example in the authorize helper comment to use an
unconditional subject (e.g., authorize('read', 'all') or a note that the subject
must be unconditional) instead of authorize('read', 'Task'); change the usage
line in the comment inside lib/helpers/authorize.js and add a brief note that
authorize() is only safe for collection-level checks when no per-document
conditions are required (reference the authorize function and the
policy.isAllowed collection-vs-document caveat).
- Around line 9-11: The authorize middleware returns a raw 403 JSON which breaks
the standard response envelope used elsewhere; update the authorize function to
call the shared response helper used by policy.isAllowed (use the existing
responses helper instead of res.status(...).json(...)) so the 403 is emitted
with the same { type, message, data } envelope (e.g., use responses.* helper to
produce the Forbidden response) and keep the rest of the middleware flow the
same.
In `@modules/audit/policies/audit.policy.js`:
- Around line 5-10: The JSDoc for auditSubjectRegistration is missing a return
annotation; update the function's JSDoc block above export function
auditSubjectRegistration({ registerPathSubject }) to include "@returns {void}"
(indicating it returns nothing) so the header contains both `@param` and `@returns`
per the project's JS doc guideline.
In `@modules/home/policies/home.policy.js`:
- Around line 5-10: The JSDoc for the exported function homeSubjectRegistration
is missing the required return annotation; update the comment block above export
function homeSubjectRegistration({ registerPathSubject }) to include "@returns
{void}" (indicating it returns nothing) alongside the existing `@param` entries so
the JSDoc meets the repository guideline requiring `@param` and `@returns` for every
function.
In `@modules/tasks/policies/tasks.policy.js`:
- Around line 5-12: The JSDoc for taskSubjectRegistration is missing a `@returns`
annotation; update the comment block above the exported function
taskSubjectRegistration to include "@returns {void}" (documenting that the
function returns nothing) alongside the existing `@param` entries for
registry/registerDocumentSubject/registerPathSubject so the JSDoc meets the
project's rule requiring `@param` and `@returns` for every function.
In `@modules/uploads/policies/uploads.policy.js`:
- Around line 5-11: The JSDoc for uploadSubjectRegistration is missing a return
annotation—update the comment block above function uploadSubjectRegistration to
include "@returns {void}" (since it only performs registrations via
registerDocumentSubject and registerPathSubject and does not return a value),
ensuring the JSDoc now documents `@param` entries and the `@returns` {void} per
project guidelines.
In `@modules/users/policies/users.policy.js`:
- Around line 7-12: The JSDoc for the function that registers user
account-related path subjects (the function taking the registry param and
calling registry.registerPathSubject) is missing a `@returns` tag; update the
function's JSDoc block to add "@returns {void}" so it documents that the
function returns nothing, keeping the existing `@param` entries intact.
---
Duplicate comments:
In `@modules/users/policies/users.admin.policy.js`:
- Around line 14-16: The document subject registration is currently global: in
adminSubjectRegistration you must make the 'model' registration use the same
admin-route predicate as the path-based registration instead of registering it
unconditionally; replace the call to registerDocumentSubject('model',
'UserAdmin') with a predicate-based registration that only returns 'UserAdmin'
when the request path matches the admin prefix (the same predicate used with
registerPathSubject, e.g. p => p.startsWith('/api/admin/users')), so both
registerDocumentSubject and registerPathSubject are scoped to admin routes only.
🪄 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: 100e9e57-a0f5-42e0-9ffa-79e509451a6f
📒 Files selected for processing (14)
.claude/skills/create-module/SKILL.md.claude/skills/feature/SKILL.md.claude/skills/verify/SKILL.mdMIGRATIONS.mdlib/helpers/authorize.jslib/middlewares/policy.jsmodules/audit/policies/audit.policy.jsmodules/billing/policies/billing.policy.jsmodules/home/policies/home.policy.jsmodules/organizations/policies/organizations.policy.jsmodules/tasks/policies/tasks.policy.jsmodules/uploads/policies/uploads.policy.jsmodules/users/policies/users.admin.policy.jsmodules/users/policies/users.policy.js
… on all policy registrations
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
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.
Summary
resolveSubject()andderiveSubjectType()inlib/middlewares/policy.jswith document/path subject registries that modules populate at startup*SubjectRegistration()function called automatically bydiscoverPolicies()lib/helpers/authorize.js— simple middleware for route-level CASL checkspolicy.isAllowedremains fully backward-compatible; no route file changes neededTest plan
core.unit.tests.js)/update-stackpicks up registration exports correctlyCloses #3381
Summary by CodeRabbit
Refactor
New Features
Chores
Documentation