feat(admin): add assignRole method for validating user and role IDs#857
feat(admin): add assignRole method for validating user and role IDs#857nevil-mathew merged 2 commits intodevelopfrom
Conversation
WalkthroughAdded a new public validator method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validators/v1/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/admin.js
🧬 Code graph analysis (1)
src/validators/v1/admin.js (2)
src/validators/v1/user-role.js (1)
filterRequestBody(1-1)src/constants/blacklistConfig.js (1)
admin(136-208)
🔇 Additional comments (1)
src/validators/v1/admin.js (1)
188-198: Remove tenant-id header validation suggestion; improve ID validation instead.The original review incorrectly suggests adding
tenant-idheader validation. The controller implementation shows thatassignRoleextractstenant_codefromreq.decodedToken(JWT), not from headers—unlike other methods likeaddOrgAdminanddeactivateOrg.However, the suggestion to use stricter ID validation is valid. Replace
isNumeric()withisInt({ min: 1 })to reject decimals and negative numbers:req.checkBody('user_id') .notEmpty() .withMessage('user_id field is empty') - .isNumeric() - .withMessage('user_id must be a number') + .isInt({ min: 1 }) + .withMessage('user_id must be a positive integer') req.checkBody('role_id') .notEmpty() .withMessage('role_id field is empty') - .isNumeric() - .withMessage('role_id must be a number') + .isInt({ min: 1 }) + .withMessage('role_id must be a positive integer')Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/validators/v1/admin.js (1)
185-189: Consider adding.trim()for consistency.For consistency with other validators in this file (e.g., lines 17, 25, 56, 84, 110), consider adding
.trim()before validation. While numeric validation handles whitespace, explicit trimming makes the intent clearer and follows the established pattern.assignRole: (req) => { req.checkBody('user_id') + .trim() .notEmpty() .withMessage('user_id field is empty') .isInt({ min: 1 }) .withMessage('user_id must be a positive integer') req.checkBody('role_id') + .trim() .notEmpty() .withMessage('role_id field is empty') .isInt({ min: 1 }) .withMessage('role_id must be a positive integer') },Also applies to: 191-195
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validators/v1/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/admin.js
🧠 Learnings (1)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/validators/v1/admin.js
🔇 Additional comments (1)
src/validators/v1/admin.js (1)
184-196: ****The
assignRolevalidator correctly omits tenant-id validation because the endpoint uses a different architectural pattern thanaddOrgAdmin. WhileaddOrgAdminexplicitly accepts and passes the tenant-id header to the service (requiring validation),assignRoleextracts tenant-id solely from the authenticated JWT token viareq.decodedToken.tenant_codeand never reads request headers for this value. Since tenant-id is not incoming data forassignRole, there is no validation needed in the validator—the tenant scope is already enforced through JWT verification at the middleware level.Likely an incorrect or invalid review comment.
| assignRole: (req) => { | ||
| req.checkBody('user_id') | ||
| .notEmpty() | ||
| .withMessage('user_id field is empty') | ||
| .isNumeric() | ||
| .withMessage('user_id must be a number') | ||
|
|
||
| req.checkBody('role_id') | ||
| .notEmpty() | ||
| .withMessage('role_id field is empty') | ||
| .isNumeric() | ||
| .withMessage('role_id must be a number') | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that missing filterRequestBody doesn't introduce security risks.
Unlike other methods in this file (create, login, addOrgAdmin, deactivateUser), assignRole doesn't call filterRequestBody to sanitize the request body. While you mentioned that only IDs are used, it's important to verify that the controller and service layers don't inadvertently process additional fields, which could introduce security vulnerabilities.
Run the following script to verify that the controller only uses user_id and role_id from the request body:
🏁 Script executed:
#!/bin/bash
# Find the assignRole implementation and check what fields it accesses from req.body
rg -n -A20 "assignRole.*req.*res" --type=js -g '**/controllers/**/admin.js'
# Also check the service layer
rg -n -A20 "assignRole" --type=js -g '**/services/**/admin.js'Length of output: 2492
Validate organization_id and add filterRequestBody call—critical validator completeness issue.
The service layer at line 112 extracts THREE fields from the request body: user_id, role_id, and organization_id. However, the validator only validates two. The organization_id is used directly in the service at line 125 to locate the organization within the user's organizations array and must be validated. Additionally, filterRequestBody is missing, which allows extra fields to pass through.
Add validation for organization_id (should be numeric and required) and call filterRequestBody after all validation checks, consistent with other methods in this file:
assignRole: (req) => {
req.checkBody('user_id')
.notEmpty()
.withMessage('user_id field is empty')
.isNumeric()
.withMessage('user_id must be a number')
req.checkBody('role_id')
.notEmpty()
.withMessage('role_id field is empty')
.isNumeric()
.withMessage('role_id must be a number')
req.checkBody('organization_id')
.notEmpty()
.withMessage('organization_id field is empty')
.isNumeric()
.withMessage('organization_id must be a number')
filterRequestBody(req, ['user_id', 'role_id', 'organization_id'])
},🤖 Prompt for AI Agents
In src/validators/v1/admin.js around lines 184 to 196, the assignRole validator
only checks user_id and role_id but omits organization_id and does not call
filterRequestBody; add a req.checkBody('organization_id') block mirroring the
others (.notEmpty().withMessage('organization_id field is
empty').isNumeric().withMessage('organization_id must be a number')) and then
call filterRequestBody(req, ['user_id','role_id','organization_id']) after all
checks to ensure the body is validated and filtered.
| req.checkBody('user_id') | ||
| .notEmpty() | ||
| .withMessage('user_id field is empty') | ||
| .isNumeric() | ||
| .withMessage('user_id must be a number') |
There was a problem hiding this comment.
Use .isInt() instead of .isNumeric() for ID validation.
Database IDs should be integers. The .isNumeric() validator accepts decimal strings like "1.5", which are invalid for ID fields. Use .isInt({ min: 1 }) to ensure both fields are positive integers.
Apply this diff to fix the validation:
assignRole: (req) => {
req.checkBody('user_id')
.notEmpty()
.withMessage('user_id field is empty')
- .isNumeric()
- .withMessage('user_id must be a number')
+ .isInt({ min: 1 })
+ .withMessage('user_id must be a positive integer')
req.checkBody('role_id')
.notEmpty()
.withMessage('role_id field is empty')
- .isNumeric()
- .withMessage('role_id must be a number')
+ .isInt({ min: 1 })
+ .withMessage('role_id must be a positive integer')
},Also applies to: 191-195
🤖 Prompt for AI Agents
In src/validators/v1/admin.js around lines 185-189 (and similarly update lines
191-195), the validator uses .isNumeric() which allows decimals; change it to
.isInt({ min: 1 }) to ensure the user_id (and the other ID field at 191-195) are
positive integers, keeping the preceding .notEmpty() and updating the error
message to reflect "must be a positive integer" or similar.
1 participant
Summary by CodeRabbit