fix missing authorization on multiple mutations#7240
Open
Amartuvshins0404 wants to merge 2 commits intoerxes:mainfrom
Open
fix missing authorization on multiple mutations#7240Amartuvshins0404 wants to merge 2 commits intoerxes:mainfrom
Amartuvshins0404 wants to merge 2 commits intoerxes:mainfrom
Conversation
Addresses erxes#7032: 1. Accounting mutations: add checkPermission calls to accountsAdd, accountsEdit, accountsRemove, accountsMerge with new permissions meta registration (admin/viewer default groups). 2. dealsRemove: extract user from context and add canEditMemberIds stage-level permission check, matching the existing editDeal pattern. 3. cpUsersRemove: add checkPermission('clientPortalManage') to enforce authorization before deleting client portal users. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Reviewer's GuideAdds missing authorization and permission checks to several GraphQL mutations across accounting, sales, and core-api client portal modules, and introduces a permissions configuration for the accounting plugin. Sequence diagram for accounting account mutations authorizationsequenceDiagram
actor Client
participant GraphQLServer
participant AuthService
participant Models
participant AccountsModel
Client->>GraphQLServer: accountsAdd(doc)
GraphQLServer->>AuthService: checkPermission(manageAccounts)
AuthService-->>GraphQLServer: ok or error
alt permission granted
GraphQLServer->>AccountsModel: createAccount(doc)
AccountsModel-->>GraphQLServer: account
GraphQLServer-->>Client: account
else permission denied
GraphQLServer-->>Client: error Permission required
end
Client->>GraphQLServer: accountsEdit(_id, doc)
GraphQLServer->>AuthService: checkPermission(manageAccounts)
AuthService-->>GraphQLServer: ok or error
alt permission granted
GraphQLServer->>AccountsModel: getAccount(_id)
AccountsModel-->>GraphQLServer: existingAccount
GraphQLServer->>AccountsModel: updateAccount(_id, doc)
AccountsModel-->>GraphQLServer: updatedAccount
GraphQLServer-->>Client: updatedAccount
else permission denied
GraphQLServer-->>Client: error Permission required
end
Client->>GraphQLServer: accountsRemove(accountIds)
GraphQLServer->>AuthService: checkPermission(removeAccounts)
AuthService-->>GraphQLServer: ok or error
alt permission granted
GraphQLServer->>AccountsModel: removeAccounts(accountIds)
AccountsModel-->>GraphQLServer: removeResult
GraphQLServer-->>Client: removeResult
else permission denied
GraphQLServer-->>Client: error Permission required
end
Client->>GraphQLServer: accountsMerge(accountIds, accountFields)
GraphQLServer->>AuthService: checkPermission(accountsMerge)
AuthService-->>GraphQLServer: ok or error
alt permission granted
GraphQLServer->>AccountsModel: mergeAccounts(accountIds, accountFields)
AccountsModel-->>GraphQLServer: mergedAccount
GraphQLServer-->>Client: mergedAccount
else permission denied
GraphQLServer-->>Client: error Permission required
end
Sequence diagram for dealsRemove authorization based on stage permissionssequenceDiagram
actor User
participant GraphQLServer
participant Models
participant DealsModel
participant StagesModel
participant SubscriptionSystem
User->>GraphQLServer: dealsRemove(_id)
GraphQLServer->>DealsModel: findOne({_id})
DealsModel-->>GraphQLServer: deal or null
alt deal not found
GraphQLServer-->>User: error Deal not found
else deal found
GraphQLServer->>StagesModel: getStage(deal.stageId)
StagesModel-->>GraphQLServer: stage
GraphQLServer->>GraphQLServer: read stage.canEditMemberIds
alt canEditMemberIds is set and does not include user._id
GraphQLServer-->>User: error Permission denied
else user allowed to edit stage
GraphQLServer->>DealsModel: removeDeals([deal._id])
DealsModel-->>GraphQLServer: removedDeals
GraphQLServer->>SubscriptionSystem: subscriptionWrapper(models, payload)
SubscriptionSystem-->>GraphQLServer: ok
GraphQLServer-->>User: removedDeals
end
end
Sequence diagram for cpUsersRemove client portal management permissionsequenceDiagram
actor TeamMember
participant GraphQLServer
participant AuthService
participant Models
participant CPUserModel
TeamMember->>GraphQLServer: cpUsersRemove(_id)
GraphQLServer->>AuthService: checkPermission(clientPortalManage)
AuthService-->>GraphQLServer: ok or error
alt permission granted
GraphQLServer->>CPUserModel: removeUser(_id, models)
CPUserModel-->>GraphQLServer: void
GraphQLServer-->>TeamMember: {_id}
else permission denied
GraphQLServer-->>TeamMember: error Permission required
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Mar 21, 2026 5:39p.m. | Review ↗ |
| Docker | |
Mar 21, 2026 5:39p.m. | Review ↗ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded explicit authorization checks and a new permissions config: client-portal user removal now awaits permission validation; accounting plugin exposes a permissions manifest and its account mutations enforce scoped checks; deals removal enforces stage-level member edit restrictions before deletion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 Tip Migrating from UI to YAML configuration.Use the |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
dealsRemove, consider handling the case wheremodels.Stages.getStage(item.stageId)returnsnull/undefinedbefore destructuringcanEditMemberIdsto avoid a runtime error when a deal has an invalid or missing stage. - The
canEditMemberIdspermission logic indealsRemoveduplicates whatdealsEditalready does; consider extracting this check into a shared helper to keep the authorization rules consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `dealsRemove`, consider handling the case where `models.Stages.getStage(item.stageId)` returns `null`/`undefined` before destructuring `canEditMemberIds` to avoid a runtime error when a deal has an invalid or missing stage.
- The `canEditMemberIds` permission logic in `dealsRemove` duplicates what `dealsEdit` already does; consider extracting this check into a shared helper to keep the authorization rules consistent and easier to maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/plugins/sales_api/src/modules/sales/graphql/resolvers/mutations/deals.ts (1)
154-157: Consider consistency:dealsCopylacks stage-level authorization check.The
dealsCopymutation creates a copy of a deal but doesn't enforce thecanEditMemberIdscheck thatdealsRemoveandeditDealnow use. If copying a deal should be restricted to authorized stage members, consider adding the same check here in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/plugins/sales_api/src/modules/sales/graphql/resolvers/mutations/deals.ts` around lines 154 - 157, The dealsCopy mutation is missing the same stage-level authorization check used in dealsRemove and editDeal; update the dealsCopy resolver to fetch the deal's stage (via the deal record or models/stages) and invoke the existing authorization helper (canEditMemberIds or the project's equivalent) with the current user and that stage before proceeding, returning/throwing an authorization error when the check fails so only authorized stage members can copy deals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/plugins/sales_api/src/modules/sales/graphql/resolvers/mutations/deals.ts`:
- Around line 154-157: The dealsCopy mutation is missing the same stage-level
authorization check used in dealsRemove and editDeal; update the dealsCopy
resolver to fetch the deal's stage (via the deal record or models/stages) and
invoke the existing authorization helper (canEditMemberIds or the project's
equivalent) with the current user and that stage before proceeding,
returning/throwing an authorization error when the check fails so only
authorized stage members can copy deals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 774adc84-aeaa-49e9-94cf-95ad7f3ba417
📒 Files selected for processing (5)
backend/core-api/src/modules/clientportal/graphql/resolvers/mutations/cpUser/admin.tsbackend/plugins/accounting_api/src/main.tsbackend/plugins/accounting_api/src/meta/permissions.tsbackend/plugins/accounting_api/src/modules/accounting/graphql/resolvers/mutations/accounts.tsbackend/plugins/sales_api/src/modules/sales/graphql/resolvers/mutations/deals.ts
Author
Amartuvshins0404
left a comment
Amartuvshins0404
left a comment
There was a problem hiding this comment.
getStage is a getter that throws if the stage doesn't exist, so it won't return null - the deal would fail with "stage not found" before reaching the destructure.
on the shared helper - it's 5 lines in two places. extracting it adds abstraction for something that doesn't need it yet. if the pattern shows up more, happy to pull it out then. for a security fix i'd rather keep it small and obvious.
extract action names into constants and derive defaultGroups from them instead of repeating the strings.
|
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
1 participant
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.
three mutations had no permission checks. any logged-in user could call them regardless of role.
accounting plugin had nothing - no checkPermission calls on accountsAdd, accountsEdit, accountsRemove, or accountsMerge, and no permissions meta registered at all. added a permissions.ts with action definitions and admin/viewer default groups, then wired checkPermission into all four mutations.
dealsRemove in sales didn't even extract user from context. added user and the same canEditMemberIds stage check that dealsEdit already does.
cpUsersRemove in core-api let any authenticated team member delete client portal users. added checkPermission('clientPortalManage').
the issue also mentions the trpc task:tag procedure having no auth. skipped that one because the trpc context across the whole codebase doesn't carry user or checkPermission - it's an architectural thing, not a single-file fix.
tested with an owner user and an unprivileged user. owner passes through, unprivileged gets "permission required" on all three.
closes #7032
Summary by Sourcery
Enforce proper authorization on accounting, sales deal removal, and client portal user removal mutations.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes