Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for executing multiple policies in sequence by allowing the policy parameter to be an array of policy names. The implementation enables chaining multiple authorization/validation checks, where all policies must accept for the controller to execute.
Key changes:
- Added
normalizePoliciesmethod to handle both single policies and arrays of policies - Refactored policy execution logic to iterate through multiple policies with per-policy acceptor callbacks
- Added
rejectedflag to prevent controller execution after any policy rejects
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| classes/policy.js | Core implementation: added array policy support with normalizePolicies, refactored restrict to loop through policies, updated acceptor/rejector with rejection tracking |
| tests/policy.js | Added two test cases verifying multiple policy execution and rejection behavior |
| tests/mocks/policies.js | Added secondPolicy and rejectingPolicy mocks to support new test cases, with tracking via req.ranPolicies |
| package.json | Version bump to 1.2.1 and removed unused doc generation scripts and dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (let i = 0; i < this.policyList.length; i++) { | ||
| const name = this.policyList[i]; | ||
|
|
||
| this.debug('Policy: lookup %s > %s', name, req.id); | ||
|
|
||
| if (!this.policies[name]) { | ||
| this.debug('Policy: %s not found [error] %s', name, req.id); | ||
| chalk.error(`Policy Error: ${req.id || ''} The policy "${name}" does not exist, therefore the request was denied`); | ||
| reject(); | ||
| break; | ||
| } | ||
|
|
||
| const accept = this.acceptor(req, res, i); | ||
| this.debug('Policy: apply %s > %s', name, req.id); | ||
| this.policies[name](req, res, accept, reject); | ||
| } |
There was a problem hiding this comment.
All policies in the array are invoked synchronously in the loop, even if an earlier policy calls reject(). The rejected flag only prevents the controller from being executed, but subsequent policies will still run.
This means if correctPolicy accepts and rejectingPolicy rejects, secondPolicy (if present after it) will still be invoked and execute its logic, even though the request has already been rejected.
Consider checking this.rejected status before invoking each policy, or breaking out of the loop when a policy rejects:
if (this.rejected) {
break;
}There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
|
|
||
| if (Array.isArray(policy)) { | ||
| return policy.filter(Boolean); |
There was a problem hiding this comment.
Inconsistent handling of 0 as a policy identifier. Line 20 explicitly allows 0 as a valid policy when passed as a single value (policy !== 0), but line 25 filters it out when it's in an array since filter(Boolean) removes all falsy values including 0.
If 0 should be a valid policy identifier, consider using a more explicit filter:
return policy.filter(p => p !== null && p !== undefined && p !== false && p !== '');Or if 0 shouldn't be valid, remove the special case check on line 20.
| return policy.filter(Boolean); | |
| return policy.filter(p => p !== null && p !== undefined && p !== false && p !== ''); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@charliemitchell I've opened a new pull request, #33, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@charliemitchell I've opened a new pull request, #34, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@charliemitchell I've opened a new pull request, #35, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@charliemitchell I've opened a new pull request, #36, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.