feat: add checkPermission to all loyalty resolvers and define permiss#7622
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
📝 WalkthroughWalkthroughAdds an exported ChangesLoyalty Plugin Authorization System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🌗 Pull Request OverviewThis PR adds comprehensive permission checks to all loyalty plugin GraphQL resolvers and defines a complete permission configuration with modules, actions, and default groups (admin, user, viewer). Reviewed Changes Show a summary per file
📋 Review Findings📄
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.ts (1)
42-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
cpDonateCampaignsshould not gate on an admin permission.The
cpprefix in erxes conventionally identifies customer portal (clientportal) endpoints, which are served to unauthenticated customers/contacts — not to staff users with RBAC permissions. Applyingawait checkPermission('donateCampaignView')here will throw a "permission denied" error for every customer portal request, effectively disabling the feature for end-users.The admin permission guard should only be on
donateCampaignsanddonateCampaignDetail. ForcpDonateCampaigns, either remove the permission check entirely or replace it with a customer-portal authentication guard (e.g., checkingcontext.cpUser), consistent with how othercp*resolvers in the codebase are secured.🐛 Proposed fix
async cpDonateCampaigns( _root: undefined, _args: undefined, - { models, checkPermission }: IContext, + { models }: IContext, ) { - await checkPermission('donateCampaignView'); const now = new Date(); return models.DonateCampaigns.find({🤖 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 `@backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.ts` around lines 42 - 55, The cpDonateCampaigns resolver currently calls await checkPermission('donateCampaignView') which blocks unauthenticated customer-portal access; remove that admin RBAC guard from cpDonateCampaigns (or replace it with a customer-portal check such as verifying context.cpUser) so that the resolver simply queries models.DonateCampaigns for active campaigns (like donateCampaigns/donateCampaignDetail use admin guards but cp* endpoints use cpUser). Ensure any replacement uses the same cp authentication pattern as other cp* resolvers in the codebase rather than checkPermission.backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.ts (1)
51-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
cpSpinCampaignsmust not gate on an admin permission — this will break the customer portal.The
cpprefix in erxes marks a customer portal resolver called by end-customers, not admins. ApplyingcheckPermission('spinCampaignView')(an admin-level action) here fails all customer requests, making the spin campaign list inaccessible. This pattern is established across the codebase:cpForms,cpBmsOrders,cpDeals,cpPayments,cpWebPages, and many othercp*resolvers in frontline_api, sales_api, payment_api, and other plugins do not call permission checks. The sales_api plugin even includes an explicit comment: "Client portal – no permission check."Remove the permission check from
cpSpinCampaigns:Suggested diff
async cpSpinCampaigns( _root: undefined, _args: undefined, - { models, checkPermission }: IContext, + { models }: IContext, ) { - await checkPermission('spinCampaignView'); const now = new Date();Note: This issue also affects other
cp*resolvers in loyalty_api (cpVoucherCampaigns, cpDonateCampaigns, cpLotteryCampaigns, cpAssignmentCampaigns) which similarly gate on admin permissions and should be reviewed for consistency.🤖 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 `@backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.ts` around lines 51 - 64, The resolver cpSpinCampaigns is incorrectly enforcing an admin permission by calling checkPermission('spinCampaignView'); remove that permission check from the cpSpinCampaigns function so the customer portal can access active campaigns (keep the date filtering and sort logic intact in models.SpinCampaigns.find(...).sort(...)). Also audit and remove similar checkPermission calls in other customer-facing resolvers in this file/plugin (cpVoucherCampaigns, cpDonateCampaigns, cpLotteryCampaigns, cpAssignmentCampaigns) to match the established cp* pattern of no permission gating.backend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignment.ts (1)
59-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
checkAssignmentperforms write operations but is gated on a read-onlyassignmentViewpermission.
checkAssignmentis declared as a query but internally it:
- Calls
awardAssignmentCampaign()which creates Voucher and Assignment records (lines 75-89)- Issues a tRPC
updateOnemutation that writes to customercustomFieldsData(lines 164–182)Protecting these write side-effects behind only
assignmentViewviolates least-privilege: any user with view-only access can trigger actual data creation. The permission should beassignmentCreateto match the actual operation semantics, sinceassignmentCreatealready exists in the permission model and is used for other mutations in this module.- await checkPermission('assignmentView'); + await checkPermission('assignmentCreate');🤖 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 `@backend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignment.ts` around lines 59 - 207, The resolver checkAssignment (in this file) performs writes via models.AssignmentCampaigns.awardAssignmentCampaign and sendTRPCMessage(..., action: 'updateOne') but only checks checkPermission('assignmentView'); change the permission check to checkPermission('assignmentCreate') and also ensure the resolver is exposed as a mutation (not a read-only query) in your GraphQL schema/registration so its intent matches side effects (move or re-register checkAssignment from queries to mutations if applicable); keep all existing calls to awardAssignmentCampaign, sendTRPCMessage and generateFieldMaxValue unchanged.
🧹 Nitpick comments (3)
backend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucher.ts (1)
5-46: ⚡ Quick winInconsistent permission action naming across modules (
voucherEditvs*Updateelsewhere).The voucher mutations use
voucherEdit(Line 21) while sibling modules use the*Updatesuffix (e.g.,agentUpdate,pricingPlanUpdate). This inconsistency complicates permission management — admins assigning permissions must know which modules useEditvsUpdate.Consider standardising to either
voucherUpdateor align the other modules' permission strings to*Editwithin the centralpermissions.ts.🤖 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 `@backend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucher.ts` around lines 5 - 46, The voucherMutations file is using the permission string 'voucherEdit' in the vouchersEdit resolver (checked via checkPermission), which is inconsistent with the rest of the codebase that uses a '*Update' suffix; change the permission check in vouchersEdit from 'voucherEdit' to 'voucherUpdate' (i.e., update the argument passed to checkPermission in vouchersEdit) and then update the central permissions list in permissions.ts to include 'voucherUpdate' (remove or alias 'voucherEdit' if present) so permission names are consistent across modules.backend/plugins/loyalty_api/src/modules/agent/graphql/resolvers/mutations/agent.ts (1)
4-49: ⚡ Quick winThree pairs of fully duplicate resolver implementations — DRY violation.
createAgent/agentsAdd,updateAgent/agentsEdit, andremoveAgent/agentsRemoveeach share exactly the same permission check and model call. Any future change (added validation, error handling, permission rename) must be applied in both places, and having them diverge silently is a real maintenance risk.If both names must exist for backwards compatibility, delegate to a shared implementation:
♻️ Proposed refactor
export const agentMutations = { - async createAgent(_root: undefined, doc: IAgent, { models, checkPermission }: IContext) { - await checkPermission('agentCreate') - return models.Agents.createAgent(doc) - }, - - async updateAgent( - _root: undefined, - { _id, ...doc }: { _id: string } & IAgent, - { models, checkPermission }: IContext, - ) { - await checkPermission('agentUpdate') - return models.Agents.updateAgent(_id, doc) - }, - - async removeAgent( - _root: undefined, - { _id }: { _id: string }, - { models, checkPermission }: IContext, - ) { - await checkPermission('agentRemove') - return models.Agents.removeAgent(_id) - }, - async agentsAdd(_root: undefined, doc: IAgent, { models, checkPermission }: IContext) { await checkPermission('agentCreate') return models.Agents.createAgent(doc) }, + createAgent: agentMutations.agentsAdd, async agentsEdit( _root: undefined, { _id, ...doc }: { _id: string } & IAgent, { models, checkPermission }: IContext, ) { await checkPermission('agentUpdate') return models.Agents.updateAgent(_id, doc) }, + updateAgent: agentMutations.agentsEdit, async agentsRemove( _root: undefined, { _id }: { _id: string }, { models, checkPermission }: IContext, ) { await checkPermission('agentRemove') return models.Agents.removeAgent(_id) }, + + removeAgent: agentMutations.agentsRemove, }🤖 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 `@backend/plugins/loyalty_api/src/modules/agent/graphql/resolvers/mutations/agent.ts` around lines 4 - 49, The three resolver pairs (createAgent/agentsAdd, updateAgent/agentsEdit, removeAgent/agentsRemove) are exact duplicates causing a DRY violation; refactor by extracting a single implementation per operation (e.g., doCreateAgent, doUpdateAgent, doRemoveAgent or keep createAgent/updateAgent/removeAgent as the canonical functions) and have the duplicate names delegate to those implementations (agentsAdd -> createAgent, agentsEdit -> updateAgent, agentsRemove -> removeAgent) so permission checks and model calls live in one place (refer to createAgent, updateAgent, removeAgent, agentsAdd, agentsEdit, agentsRemove).backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.ts (1)
30-31: ⚡ Quick winUse the correct type narrowing for
voucherCampaignIdingenerateFilter.
voucherCampaignIdis defined onILotteryMainParams, but the code casts toILotteryParams, which is misleading and weakens type safety.Suggested fix
- if ((params as ILotteryParams).voucherCampaignId) { - filter.voucherCampaignId = (params as ILotteryParams).voucherCampaignId; + if ('voucherCampaignId' in params && params.voucherCampaignId) { + filter.voucherCampaignId = params.voucherCampaignId; }🤖 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 `@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.ts` around lines 30 - 31, The code in generateFilter incorrectly narrows params to ILotteryParams when checking voucherCampaignId; voucherCampaignId actually lives on ILotteryMainParams—change the type guard/cast to use ILotteryMainParams (or refine the params union check) so you read (params as ILotteryMainParams).voucherCampaignId (or perform an instanceof/type-predicate check) before assigning to filter.voucherCampaignId to restore correct type safety and avoid the misleading ILotteryParams cast.
🤖 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 `@backend/plugins/loyalty_api/src/meta/permissions.ts`:
- Around line 956-1022: The loyalty:viewer permission set is missing campaign-
and pricing-related read permissions, causing incomplete read-only access;
update the permissions array inside the object with id 'loyalty:viewer' to add
entries for modules voucherCampaign, lotteryCampaign, spinCampaign,
couponCampaign, donateCampaign, assignmentCampaign and pricing/pricingPlan each
granting their respective view actions (e.g. voucherCampaignView,
lotteryCampaignView, spinCampaignView, couponCampaignView, donateCampaignView,
assignmentCampaignView, pricingView/pricingPlanView) with scope 'all' so viewers
can see both entities and their campaign/configuration context.
- Around line 9-16: The permission entries for the donate and scoreLog modules
(and any other module configured with scope 'own' in defaultGroups such as the
loyalty:user group) currently have ownerFields: [] so the 'own' scope cannot
enforce ownership; update the module permission objects for "donate" and
"scoreLog" to set ownerFields: ['ownerId'] (and similarly populate
ownerFields:['ownerId'] for any other modules listed with scope: 'own' in
defaultGroups) so the permission framework can filter records by ownerId; locate
the module definitions named donate and scoreLog in permissions.ts and replace
their ownerFields arrays accordingly.
In
`@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.ts`:
- Around line 30-31: The resolver functions in lotteryCampaign.ts destructure
context as "{ models, checkPermission }: IContext" but omit subdomain; update
each resolver parameter to destructure subdomain as well (e.g., "{ models,
checkPermission, subdomain }: IContext") so tenant-scoped model access uses the
correct tenant context; ensure every occurrence mentioned (the resolver
parameter locations corresponding to lines around the existing "{ models,
checkPermission }: IContext" entries) is updated consistently and any downstream
calls that rely on context pass/consume subdomain as needed.
- Around line 13-15: The current code directly builds a RegExp from
params.searchValue and assigns it to filter.name, which risks ReDoS and
performance issues; change this to escape any regex metacharacters before
constructing the regex (e.g., implement or import an escapeRegExp function and
use it on params.searchValue) or avoid RegExp entirely by using a safe DB text
match (e.g., use a $regex value built from the escaped string with the 'i' flag
or use a case-insensitive index/search method). Update the assignment that sets
filter.name (where params.searchValue is read) to use the escaped value or safe
search approach so untrusted input cannot inject regex patterns.
In
`@backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spin.ts`:
- Around line 34-37: The resolver doSpin is receiving the entire GraphQL args
object as the spinId parameter, so models.Spins.doSpin gets { spinId: "..." }
instead of a string; update the doSpin signature to destructure the args object
to extract the spinId string (e.g., change the second parameter from spinId:
string to { spinId }: { spinId: string } or similar), keep the permission check
via checkPermission('spinDo'), and then call models.Spins.doSpin(spinId) with
the extracted string.
---
Outside diff comments:
In
`@backend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignment.ts`:
- Around line 59-207: The resolver checkAssignment (in this file) performs
writes via models.AssignmentCampaigns.awardAssignmentCampaign and
sendTRPCMessage(..., action: 'updateOne') but only checks
checkPermission('assignmentView'); change the permission check to
checkPermission('assignmentCreate') and also ensure the resolver is exposed as a
mutation (not a read-only query) in your GraphQL schema/registration so its
intent matches side effects (move or re-register checkAssignment from queries to
mutations if applicable); keep all existing calls to awardAssignmentCampaign,
sendTRPCMessage and generateFieldMaxValue unchanged.
In
`@backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.ts`:
- Around line 42-55: The cpDonateCampaigns resolver currently calls await
checkPermission('donateCampaignView') which blocks unauthenticated
customer-portal access; remove that admin RBAC guard from cpDonateCampaigns (or
replace it with a customer-portal check such as verifying context.cpUser) so
that the resolver simply queries models.DonateCampaigns for active campaigns
(like donateCampaigns/donateCampaignDetail use admin guards but cp* endpoints
use cpUser). Ensure any replacement uses the same cp authentication pattern as
other cp* resolvers in the codebase rather than checkPermission.
In
`@backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.ts`:
- Around line 51-64: The resolver cpSpinCampaigns is incorrectly enforcing an
admin permission by calling checkPermission('spinCampaignView'); remove that
permission check from the cpSpinCampaigns function so the customer portal can
access active campaigns (keep the date filtering and sort logic intact in
models.SpinCampaigns.find(...).sort(...)). Also audit and remove similar
checkPermission calls in other customer-facing resolvers in this file/plugin
(cpVoucherCampaigns, cpDonateCampaigns, cpLotteryCampaigns,
cpAssignmentCampaigns) to match the established cp* pattern of no permission
gating.
---
Nitpick comments:
In
`@backend/plugins/loyalty_api/src/modules/agent/graphql/resolvers/mutations/agent.ts`:
- Around line 4-49: The three resolver pairs (createAgent/agentsAdd,
updateAgent/agentsEdit, removeAgent/agentsRemove) are exact duplicates causing a
DRY violation; refactor by extracting a single implementation per operation
(e.g., doCreateAgent, doUpdateAgent, doRemoveAgent or keep
createAgent/updateAgent/removeAgent as the canonical functions) and have the
duplicate names delegate to those implementations (agentsAdd -> createAgent,
agentsEdit -> updateAgent, agentsRemove -> removeAgent) so permission checks and
model calls live in one place (refer to createAgent, updateAgent, removeAgent,
agentsAdd, agentsEdit, agentsRemove).
In
`@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.ts`:
- Around line 30-31: The code in generateFilter incorrectly narrows params to
ILotteryParams when checking voucherCampaignId; voucherCampaignId actually lives
on ILotteryMainParams—change the type guard/cast to use ILotteryMainParams (or
refine the params union check) so you read (params as
ILotteryMainParams).voucherCampaignId (or perform an instanceof/type-predicate
check) before assigning to filter.voucherCampaignId to restore correct type
safety and avoid the misleading ILotteryParams cast.
In
`@backend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucher.ts`:
- Around line 5-46: The voucherMutations file is using the permission string
'voucherEdit' in the vouchersEdit resolver (checked via checkPermission), which
is inconsistent with the rest of the codebase that uses a '*Update' suffix;
change the permission check in vouchersEdit from 'voucherEdit' to
'voucherUpdate' (i.e., update the argument passed to checkPermission in
vouchersEdit) and then update the central permissions list in permissions.ts to
include 'voucherUpdate' (remove or alias 'voucherEdit' if present) so permission
names are consistent across modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 088830bb-1623-43cd-917e-f9119492331f
📒 Files selected for processing (39)
backend/plugins/loyalty_api/src/meta/permissions.tsbackend/plugins/loyalty_api/src/modules/agent/graphql/resolvers/mutations/agent.tsbackend/plugins/loyalty_api/src/modules/agent/graphql/resolvers/queries/agent.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/mutations/assignment.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/mutations/assignmentCampaign.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignment.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignmentCampaign.tsbackend/plugins/loyalty_api/src/modules/config/graphql/mutations/config.tsbackend/plugins/loyalty_api/src/modules/config/graphql/mutations/loyalty.tsbackend/plugins/loyalty_api/src/modules/config/graphql/queries/config.tsbackend/plugins/loyalty_api/src/modules/config/graphql/queries/loyalty.tsbackend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/mutations/coupon.tsbackend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/mutations/couponCampaign.tsbackend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/queries/coupon.tsbackend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/queries/couponCampaign.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/mutations/donate.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/mutations/donateCampaign.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donate.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/queries/lottery.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/queries/lotteryCampaign.tsbackend/plugins/loyalty_api/src/modules/pricing/graphql/resolvers/mutations/pricing.tsbackend/plugins/loyalty_api/src/modules/pricing/graphql/resolvers/mutations/pricingPlan.tsbackend/plugins/loyalty_api/src/modules/pricing/graphql/resolvers/queries/pricing.tsbackend/plugins/loyalty_api/src/modules/pricing/graphql/resolvers/queries/pricingPlan.tsbackend/plugins/loyalty_api/src/modules/score/graphql/resolvers/mutations/scoreCampaign.tsbackend/plugins/loyalty_api/src/modules/score/graphql/resolvers/mutations/scoreLog.tsbackend/plugins/loyalty_api/src/modules/score/graphql/resolvers/queries/scoreCampaign.tsbackend/plugins/loyalty_api/src/modules/score/graphql/resolvers/queries/scoreLog.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spin.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spinCampaign.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spin.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.tsbackend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucher.tsbackend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucherCampaign.tsbackend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/queries/voucher.tsbackend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/queries/voucherCampaign.ts
Comment on lines
+30
to
31
| { models, checkPermission }: IContext, | ||
| ) { |
There was a problem hiding this comment.
Include subdomain in resolver context for tenant-isolated data access.
These resolvers access tenant-scoped models but omit subdomain in context destructuring, which violates the backend multi-tenancy rule.
As per coding guidelines, "backend/plugins//src/**/.ts: Always include subdomain in context for data access to maintain multi-tenancy isolation; models are automatically scoped to subdomain collections" and "backend/**/*.ts: Always use the subdomain context parameter for tenant-isolated data access in multi-tenant resolvers and services".
Also applies to: 45-46, 60-61, 69-70, 88-89
🤖 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
`@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.ts`
around lines 30 - 31, The resolver functions in lotteryCampaign.ts destructure
context as "{ models, checkPermission }: IContext" but omit subdomain; update
each resolver parameter to destructure subdomain as well (e.g., "{ models,
checkPermission, subdomain }: IContext") so tenant-scoped model access uses the
correct tenant context; ensure every occurrence mentioned (the resolver
parameter locations corresponding to lines around the existing "{ models,
checkPermission }: IContext" entries) is updated consistently and any downstream
calls that rely on context pass/consume subdomain as needed.
🌗 Pull Request OverviewThis PR adds permission checks ( Reviewed Changes Show a summary per file
📋 Review Findings📄
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.ts (1)
8-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
subdomainin context for multi-tenant data access.All resolvers in this file destructure
{ models, checkPermission }but omitsubdomain, which is required for tenant-isolated data access when callingmodels.LotteryCampaigns.*operations.As per coding guidelines, "backend/plugins//src/**/.ts: Always include subdomain in context for data access to maintain multi-tenancy isolation" and "backend/**/*.ts: Always use the
subdomaincontext parameter for tenant-isolated data access in multi-tenant resolvers and services".🔧 Proposed fix to include subdomain in all resolvers
async lotteryCampaignsAdd( _root: undefined, doc: ILotteryCampaign, - { models, checkPermission }: IContext, + { models, checkPermission, subdomain }: IContext, ) { async lotteryCampaignsEdit( _root: undefined, { _id, ...doc }: ILotteryCampaign & { _id: string }, - { models, checkPermission }: IContext, + { models, checkPermission, subdomain }: IContext, ) { async lotteryCampaignsRemove( _root: undefined, { _ids }: { _ids: string[] }, - { models, checkPermission }: IContext, + { models, checkPermission, subdomain }: IContext, ) { async doLottery( _root: undefined, params: { campaignId: string; awardId: string }, - { models, checkPermission }: IContext, + { models, checkPermission, subdomain }: IContext, ) { async doLotteryMultiple( _root: undefined, params: { campaignId: string; awardId: string; multiple: number }, - { models, checkPermission }: IContext, + { models, checkPermission, subdomain }: IContext, ) { async getNextChar( _root: undefined, params: { campaignId: string; awardId: string; prevChars: string }, - { models, checkPermission }: IContext, + { models, checkPermission, subdomain }: IContext, ) {Also applies to: 17-18, 26-27, 35-36, 44-45, 53-54
🤖 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 `@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.ts` around lines 8 - 9, Resolver functions currently destructure "{ models, checkPermission }" but omit "subdomain"; update each resolver (the mutation handlers that call models.LotteryCampaigns.*) to destructure "{ models, checkPermission, subdomain }" and pass that subdomain into the models.LotteryCampaigns method calls (e.g., create, update, find, delete) so tenant-isolated data access is used. Ensure every occurrence where models.LotteryCampaigns.* is invoked uses the subdomain parameter and update any function signatures or local variables accordingly.
🤖 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
`@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.ts`:
- Line 6: The resolver signatures (e.g., lotteriesAdd) destructure context but
omit subdomain; update each resolver in this file to include subdomain in the
context destructuring (for example change "{ models, checkPermission }:
IContext" to "{ models, checkPermission, subdomain }: IContext") and then pass
that subdomain into any tenant-isolated model calls (models.Lotteries.*) so the
database operations use the correct tenant; apply the same change to the other
resolvers referenced in this file.
---
Duplicate comments:
In
`@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.ts`:
- Around line 8-9: Resolver functions currently destructure "{ models,
checkPermission }" but omit "subdomain"; update each resolver (the mutation
handlers that call models.LotteryCampaigns.*) to destructure "{ models,
checkPermission, subdomain }" and pass that subdomain into the
models.LotteryCampaigns method calls (e.g., create, update, find, delete) so
tenant-isolated data access is used. Ensure every occurrence where
models.LotteryCampaigns.* is invoked uses the subdomain parameter and update any
function signatures or local variables accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc2cb107-023e-41fc-b325-2724a42fdac3
📒 Files selected for processing (8)
backend/plugins/loyalty_api/src/meta/permissions.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignmentCampaign.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/queries/lotteryCampaign.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spin.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/queries/lotteryCampaign.ts
- backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.ts
- backend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignmentCampaign.ts
- backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spin.ts
- backend/plugins/loyalty_api/src/meta/permissions.ts
|
|
||
| export const lotteryMutations = { | ||
| async lotteriesAdd(_root: undefined, doc: ILottery, { models }: IContext) { | ||
| async lotteriesAdd(_root: undefined, doc: ILottery, { models, checkPermission }: IContext) { |
There was a problem hiding this comment.
Include subdomain in context for multi-tenant data access.
All resolvers in this file destructure context but omit subdomain, which is required for tenant-isolated data access when calling models.Lotteries.* operations.
As per coding guidelines, "backend/plugins//src/**/.ts: Always include subdomain in context for data access to maintain multi-tenancy isolation" and "backend/**/*.ts: Always use the subdomain context parameter for tenant-isolated data access in multi-tenant resolvers and services".
🔧 Proposed fix to include subdomain in all resolvers
- async lotteriesAdd(_root: undefined, doc: ILottery, { models, checkPermission }: IContext) {
+ async lotteriesAdd(_root: undefined, doc: ILottery, { models, checkPermission, subdomain }: IContext) {
async lotteriesEdit(
_root: undefined,
{ _id, ...doc }: ILottery & { _id: string },
- { models, user, checkPermission }: IContext,
+ { models, user, checkPermission, subdomain }: IContext,
) {
async lotteriesRemove(
_root: undefined,
{ _ids }: { _ids: string[] },
- { models, checkPermission }: IContext,
+ { models, checkPermission, subdomain }: IContext,
) {
- async buyLottery(_root: undefined, param: IBuyParams, { models, checkPermission }: IContext) {
+ async buyLottery(_root: undefined, param: IBuyParams, { models, checkPermission, subdomain }: IContext) {Also applies to: 14-14, 23-23, 29-29
🤖 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
`@backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lottery.ts`
at line 6, The resolver signatures (e.g., lotteriesAdd) destructure context but
omit subdomain; update each resolver in this file to include subdomain in the
context destructuring (for example change "{ models, checkPermission }:
IContext" to "{ models, checkPermission, subdomain }: IContext") and then pass
that subdomain into any tenant-isolated model calls (models.Lotteries.*) so the
database operations use the correct tenant; apply the same change to the other
resolvers referenced in this file.
🌗 Pull Request OverviewThis PR adds comprehensive permission checks to all GraphQL resolvers in the loyalty plugin and defines a detailed permission configuration. It introduces role-based access control with three default groups (admin, user, viewer) and fixes a potential ReDoS vulnerability by escaping regex patterns. Reviewed Changes Show a summary per file
📋 Review Findings📄
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.ts (1)
42-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't gate the customer-portal campaign feed behind a staff permission.
cpDonateCampaignsonly returns active, date-bounded campaigns and doesn't accept any back-office filters, so this looks like the customer-facing feed. RequiringloyaltyCampaignViewhere will reject portal users who don't carry loyalty staff permissions and can break campaign discovery/redeem flows.🤖 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 `@backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.ts` around lines 42 - 54, The cpDonateCampaigns resolver is incorrectly requiring the staff permission checkPermission('loyaltyCampaignView'), which blocks customer-portal access; remove the await checkPermission('loyaltyCampaignView') call from the cpDonateCampaigns function so the resolver returns active, date-bounded campaigns to portal users as intended (leave the models.DonateCampaigns.find(...) and sorting logic unchanged).
🤖 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 `@backend/plugins/loyalty_api/src/meta/permissions.ts`:
- Around line 367-389: The loyalty:viewer role (id: 'loyalty:viewer') is missing
the config read permission; add a permission entry granting the config module's
read action to that role by appending { plugin: 'loyalty', module: 'config',
actions: ['loyaltyConfigView'], scope: 'all' } to the permissions array for the
'loyalty:viewer' role so it retains read-only access to loyalty settings when
resolvers enforce loyaltyConfigView.
---
Outside diff comments:
In
`@backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.ts`:
- Around line 42-54: The cpDonateCampaigns resolver is incorrectly requiring the
staff permission checkPermission('loyaltyCampaignView'), which blocks
customer-portal access; remove the await checkPermission('loyaltyCampaignView')
call from the cpDonateCampaigns function so the resolver returns active,
date-bounded campaigns to portal users as intended (leave the
models.DonateCampaigns.find(...) and sorting logic unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c3dbee9-a98b-4530-9c70-21d423a60df2
📒 Files selected for processing (15)
backend/plugins/loyalty_api/src/meta/permissions.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/mutations/assignmentCampaign.tsbackend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignmentCampaign.tsbackend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/mutations/couponCampaign.tsbackend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/queries/couponCampaign.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/mutations/donateCampaign.tsbackend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/queries/donateCampaign.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.tsbackend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/queries/lotteryCampaign.tsbackend/plugins/loyalty_api/src/modules/score/graphql/resolvers/mutations/scoreCampaign.tsbackend/plugins/loyalty_api/src/modules/score/graphql/resolvers/queries/scoreCampaign.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spinCampaign.tsbackend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/queries/spinCampaign.tsbackend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucherCampaign.tsbackend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/queries/voucherCampaign.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/plugins/loyalty_api/src/modules/donate/graphql/resolvers/mutations/donateCampaign.ts
- backend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/queries/assignmentCampaign.ts
- backend/plugins/loyalty_api/src/modules/voucher/graphql/resolvers/mutations/voucherCampaign.ts
- backend/plugins/loyalty_api/src/modules/coupon/graphql/resolvers/mutations/couponCampaign.ts
- backend/plugins/loyalty_api/src/modules/assignment/graphql/resolvers/mutations/assignmentCampaign.ts
- backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/queries/lotteryCampaign.ts
- backend/plugins/loyalty_api/src/modules/lottery/graphql/resolvers/mutations/lotteryCampaign.ts
- backend/plugins/loyalty_api/src/modules/spin/graphql/resolvers/mutations/spinCampaign.ts
🌗 Pull Request OverviewThis PR adds comprehensive permission checks ( Reviewed Changes Show a summary per file
📋 Review Findings📄
|
🌗 Pull Request OverviewThis PR adds comprehensive permission checks ( Reviewed Changes Show a summary per file
📋 Review Findings📄
|
|
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
2 participants
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.
…ions config
Summary by CodeRabbit
New Features
Refactor
Bug Fixes