Issue#261206 Fix: While updateUser from Admin then phone and email store encrypted#898
Issue#261206 Fix: While updateUser from Admin then phone and email store encrypted#898Sachintechjoomla wants to merge 52 commits into
Conversation
…rties of undefined (reading 'outputFilePath')
…crypting phone and passing correct meta in account search
Issue #000 fix: added new endpoint to create account in teanat and decrypting phone and passing correct meta in account search
Added AWS secret access key exports for deployment.
Updated environment variable export syntax for AWS credentials.
Issue #000 fix: serach Users with one or more meta params
…into Issue#253287
Issue#253287 Feat: User account search API > Add status filter
…into Issue#252552
Issue#252552 Feat: User bulk upload with all entity types
Profile read while creating project giving error
Issue#254093 Feat: Update > LC update participant address
Issue #000 fix: update user profile added
Issue #000 fix: allow supervisors to login through Admin flow- i.e. a…
Issue#258700 Fix: User Not able to Import via bulkImport
WalkthroughAdds BRAC development and QA deployment workflows, tenant account creation, shared validation, organization-admin profile updates, encrypted contact handling, metadata-aware search, dynamic invitation entity mapping, and tenant-admin authorization. ChangesBRAC deployment workflows
Tenant account and user platform changes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
.github/workflows/brac-qa-deplyment.yaml (3)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the typo in the workflow filename.
The file is named
brac-qa-deplyment.yaml, which has a typo indeployment. Consider renaming the file tobrac-qa-deployment.yamlfor 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 @.github/workflows/brac-qa-deplyment.yaml at line 1, Rename the workflow file from brac-qa-deplyment.yaml to brac-qa-deployment.yaml, preserving the existing workflow contents and configuration.
87-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winQuote the tag argument to prevent code injection or word splitting.
The
${{ env.TAG }}expression is substituted directly into the shell script. Wrapping it in double quotes helps prevent command injection and word-splitting issues if the tag name were ever to contain spaces or special characters.♻️ Proposed fix
- ./deploy.sh ${{ env.TAG }} + ./deploy.sh "${{ env.TAG }}"🤖 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 @.github/workflows/brac-qa-deplyment.yaml at line 87, Quote the env.TAG argument in the deploy.sh invocation by wrapping the substituted value in double quotes, preserving it as a single shell argument and preventing special characters from being interpreted.Source: Linters/SAST tools
65-68: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winQuote exported variables to prevent word splitting and potential injection.
Unquoted GitHub Actions expressions can cause syntax errors or unintended shell expansion if the secrets or environment variables contain spaces or special characters.
♻️ Proposed fix
- export AWS_ACCESS_KEY_ID=${{ secrets.AWS_ACCESS_KEY_ID }} - export AWS_SECRET_ACCESS_KEY=${{ secrets.AWS_SECRET_ACCESS_KEY }} - export AWS_REGION=${{ env.AWS_REGION }} + export AWS_ACCESS_KEY_ID="${{ secrets.AWS_ACCESS_KEY_ID }}" + export AWS_SECRET_ACCESS_KEY="${{ secrets.AWS_SECRET_ACCESS_KEY }}" + export AWS_REGION="${{ env.AWS_REGION }}"🤖 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 @.github/workflows/brac-qa-deplyment.yaml around lines 65 - 68, Quote the assigned values in the AWS credential and region exports within the deployment workflow: AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_REGION. Preserve the existing GitHub Actions expressions while ensuring each shell assignment safely handles spaces and special characters.Source: Linters/SAST tools
.github/workflows/brac-dev-deployment.yaml (2)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin GitHub Actions to a specific version instead of
@master.Using
@masterfor GitHub Actions can lead to unexpected pipeline failures if the author introduces backward-incompatible changes. It is recommended to pin to a specific version tag (e.g.,@v1.0.3or@v1).
.github/workflows/brac-dev-deployment.yaml#L54-L55: changeappleboy/ssh-action@masterto a specific version tag likeappleboy/ssh-action@v1.0.3..github/workflows/brac-qa-deplyment.yaml#L56-L57: changeappleboy/ssh-action@masterto a specific version tag likeappleboy/ssh-action@v1.0.3.🤖 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 @.github/workflows/brac-dev-deployment.yaml around lines 54 - 55, Pin the Deploy Stack action to a specific stable version instead of the moving `@master` reference: update appleboy/ssh-action in .github/workflows/brac-dev-deployment.yaml lines 54-55 and .github/workflows/brac-qa-deplyment.yaml lines 56-57 to the approved version tag, such as `@v1.0.3`.
21-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falsefor checkout actions.To prevent the GitHub token from persisting in the runner environment and potentially being exposed to subsequent steps, set
persist-credentials: falseunless explicitly needed.
.github/workflows/brac-dev-deployment.yaml#L21-L22: addwith: persist-credentials: falseto theactions/checkout@v4step..github/workflows/brac-qa-deplyment.yaml#L22-L23: addwith: persist-credentials: falseto theactions/checkout@v4step.🤖 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 @.github/workflows/brac-dev-deployment.yaml around lines 21 - 22, Configure both actions/checkout@v4 steps with persist-credentials set to false: update .github/workflows/brac-dev-deployment.yaml lines 21-22 and .github/workflows/brac-qa-deplyment.yaml lines 22-23 by adding the checkout step’s with configuration. No other workflow behavior needs to change.Source: Linters/SAST tools
src/generics/utils.js (1)
451-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up commented-out code.
If preserving the key in
metais intentional, remove the commenteddeletestatement entirely to reduce clutter.🧹 Proposed fix
// Move the key from responseBody.meta to responseBody root level responseBody[entityTypeValue] = responseBody.meta[entityTypeValue] - // Delete the key from responseBody.meta - //delete responseBody.meta[entityTypeValue] } else { const externalBaseUrl =🤖 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 `@src/generics/utils.js` around lines 451 - 452, Remove the commented-out delete statement near the responseBody.meta handling in the relevant utility flow, while leaving the intentional key-preservation behavior unchanged.src/controllers/v1/tenant.js (1)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the misleading inline comment.
The code explicitly sets
registerWithLogin = false, but the inline comment statesset registerWithLogin to true. Update the comment to reflect the actual behavior (e.g.,// Pass false to bypass login session creation).💡 Proposed fix
let registerWithLogin = false const usersRes = await accountService.create( req.body, {}, '', - registerWithLogin, //set registerWithLogin to true, this will create user with login + registerWithLogin, // bypass user login session creation req )🤖 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 `@src/controllers/v1/tenant.js` around lines 172 - 177, Update the inline comment at the accountService.create call in the tenant controller to accurately describe that registerWithLogin is passed as false and login session creation is bypassed; do not change the existing registerWithLogin value or create-call behavior.
🤖 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 `@src/database/queries/users.js`:
- Around line 436-444: The generic metaFilters handling in the user query
currently uses a per-key JSONB text extraction predicate; update this logic to
use a parameterized JSONB containment predicate on User.meta, while preserving
key validation and filtering of null, undefined, and empty values. Add or verify
a GIN index on users.meta so these equality lookups are indexable.
In `@src/helpers/userInvite.js`:
- Around line 367-374: Update the ARRAY mapping in the row field-processing
logic to fall back to the trimmed original value when externalEntityNameIdMap
does not contain a matching ID. Preserve the existing resolved _id behavior for
mapped external values and keep filter(Boolean) for genuinely empty values,
matching the single-value handling below.
In `@src/services/account.js`:
- Around line 1979-1989: Update the metadata processing flow around userOrg,
findUserEntityTypesAndEntities, and removeDefaultOrgEntityTypes so it does not
rely on user.user_organizations[0]. Use the requested organization_id to select
the matching user organization and filter entity types accordingly, or include
all applicable organization codes when multiple organizations are valid;
preserve correct pruning and processDbResponse behavior.
- Around line 1970-1990: Batch the entity metadata lookup before the
user-processing loop in the surrounding account service method: call
getModelName() once, collect distinct organization codes from users.data, and
call findUserEntityTypesAndEntities() once with all codes plus
defaultOrganizationCode. Inside the loop, reuse the shared validation data while
retaining each user’s organization-specific removeDefaultOrgEntityTypes and
processDbResponse flow.
- Line 42: Remove the unused i18next.use binding at
src/services/account.js:42-42 and the unused blacklist user import at
src/validators/v1/user.js:9-9. At src/services/account.js:1730-1730, handle the
promise rejection with .catch(() => {}) or log the caught error.
- Around line 78-84: Update the tenant-resolution logic before
tenantQueries.findOne to return a client error when neither domain nor the
TENANT_CODE_HEADER is available, avoiding tenantDomain.tenant_code
dereferencing. In the header branch, resolve the complete tenant-domain record
by tenant code, including its domain/portal URL, so downstream registration
notifications receive a populated record.
- Line 325: Update the roles value used by Op.in in the account service to
safely normalize an optional req.body.roles only when req.body exists and the
value is a string or array; split strings into role entries, preserve arrays,
and otherwise fall back to process.env.DEFAULT_ROLE split into an array. Ensure
callers omitting req or body no longer throw and avoid dereferencing or
splitting unvalidated values.
In `@src/services/admin.js`:
- Around line 478-483: Update the login token construction in the admin service
so tokenDetail.data includes the user’s roles array alongside organizations.
Reuse the roles data represented by the admin-role check near hasAdminRole,
ensuring authenticator.js can read decodedToken.data.roles and preserve admin
and tenant-admin status on subsequent requests.
In `@src/services/org-admin.js`:
- Around line 621-674: The role assignment flow in the organization-admin update
must restore target membership validation and tenant scoping. Before processing
roles, verify userId belongs to tokenInformation.organization_code; constrain
roleQueries.findAll and userQueries.updateUser with the appropriate tenant and
organization fields, and validate the update result’s affected-row count,
returning the existing failure response when no membership is updated.
In `@src/services/user.js`:
- Around line 120-128: Update the existingPhoneUser query in the user update
flow to include bodyData.phone_code alongside phone, tenant_code, and the
excluded id. Preserve the existing duplicate handling while ensuring uniqueness
is checked against the combined phone and phone_code values.
In `@src/validators/v1/account.js`:
- Around line 29-43: Update validatePhoneWithCode to enforce server-side bounds
and formats for both phone and phone_code before encryption or storage: require
phone to contain only digits within the supported minimum and maximum length,
and validate phone_code as a numeric country code with its supported bounded
format. Preserve the conditional requirement that phone_code is required
whenever phone is provided, while ensuring optional absent fields remain valid.
In `@src/validators/v1/tenant.js`:
- Around line 143-150: Replace the accountCreate implementation’s delegation to
accountValidators.update with a dedicated create validator schema for this
route. Require and validate password before accountService.create hashes it, and
include creation-specific validation for name, identifier, username, role, plus
strict request-body allowlisting; preserve the route’s accepted dotted-username
format while covering all incoming fields.
---
Nitpick comments:
In @.github/workflows/brac-dev-deployment.yaml:
- Around line 54-55: Pin the Deploy Stack action to a specific stable version
instead of the moving `@master` reference: update appleboy/ssh-action in
.github/workflows/brac-dev-deployment.yaml lines 54-55 and
.github/workflows/brac-qa-deplyment.yaml lines 56-57 to the approved version
tag, such as `@v1.0.3`.
- Around line 21-22: Configure both actions/checkout@v4 steps with
persist-credentials set to false: update
.github/workflows/brac-dev-deployment.yaml lines 21-22 and
.github/workflows/brac-qa-deplyment.yaml lines 22-23 by adding the checkout
step’s with configuration. No other workflow behavior needs to change.
In @.github/workflows/brac-qa-deplyment.yaml:
- Line 1: Rename the workflow file from brac-qa-deplyment.yaml to
brac-qa-deployment.yaml, preserving the existing workflow contents and
configuration.
- Line 87: Quote the env.TAG argument in the deploy.sh invocation by wrapping
the substituted value in double quotes, preserving it as a single shell argument
and preventing special characters from being interpreted.
- Around line 65-68: Quote the assigned values in the AWS credential and region
exports within the deployment workflow: AWS_ACCESS_KEY_ID,
AWS_SECRET_ACCESS_KEY, and AWS_REGION. Preserve the existing GitHub Actions
expressions while ensuring each shell assignment safely handles spaces and
special characters.
In `@src/controllers/v1/tenant.js`:
- Around line 172-177: Update the inline comment at the accountService.create
call in the tenant controller to accurately describe that registerWithLogin is
passed as false and login session creation is bypassed; do not change the
existing registerWithLogin value or create-call behavior.
In `@src/generics/utils.js`:
- Around line 451-452: Remove the commented-out delete statement near the
responseBody.meta handling in the relevant utility flow, while leaving the
intentional key-preservation behavior 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c3c3e79-9279-48ba-8794-092907282fcb
⛔ Files ignored due to path filters (1)
src/api-doc/bulkUser.mdis excluded by!src/api-doc/**,!**/*.md
📒 Files selected for processing (16)
.github/workflows/brac-dev-deployment.yaml.github/workflows/brac-qa-deplyment.yamlsrc/controllers/v1/tenant.jssrc/database/queries/users.jssrc/generics/utils.jssrc/helpers/userInvite.jssrc/locales/en.jsonsrc/services/account.jssrc/services/admin.jssrc/services/org-admin.jssrc/services/user.jssrc/utils/emailEncryption.jssrc/validators/v1/account.jssrc/validators/v1/org-admin.jssrc/validators/v1/tenant.jssrc/validators/v1/user.js
| // Filter by meta fields (generic - supports any meta field) | ||
| if (metaFilters && typeof metaFilters === 'object' && Object.keys(metaFilters).length > 0) { | ||
| userWhere[Op.and] = userWhere[Op.and] || [] | ||
| for (const [key, value] of Object.entries(metaFilters)) { | ||
| if (value !== null && value !== undefined && value !== '') { | ||
| // Use Sequelize.where with JSONB operator for safe parameterized queries | ||
| // The key is validated to be alphanumeric/underscore only to prevent SQL injection | ||
| if (/^[a-zA-Z0-9_]+$/.test(key)) { | ||
| userWhere[Op.and].push(Sequelize.where(Sequelize.literal(`"User".meta->>'${key}'`), value)) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect metadata predicates and any model/migration indexes covering User.meta.
rg -n -C3 'meta->>|Op\.contains|searchUsersWithOrganization' src/database/queries/users.js
fd -t f . |
rg '(^|/)(migrations?|models?)/' |
xargs -r rg -n -C3 'jsonb_path_ops|using.*gin|GIN|["'\'']meta["'\'']'Repository: ELEVATE-Project/user
Length of output: 6687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the User model and migrations for meta column type and any indexes.
files=(
src/database/models/user.js
src/database/models/users.js
src/database/migrations/*user*.js
src/database/migrations/*users*.js
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "=== $f ==="
sed -n '1,240p' "$f" | rg -n 'meta|index|using|JSONB|JSON|addIndex|indexes|createTable|alterTable'
echo
fi
done
# Search specifically for users table meta/index definitions.
rg -n -C3 'meta|addIndex|using.*gin|jsonb_path_ops|gin\(' src/database/models src/database/migrations | rg 'users?|User'Repository: ELEVATE-Project/user
Length of output: 626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the users model and its table migration with line numbers around meta/indexes.
sed -n '1,180p' src/database/models/users.js | cat -n
printf '\n---\n'
sed -n '1,180p' src/database/migrations/20230531105847-create-users-table.js | cat -n
printf '\n---\n'
# Search all migrations for indexes on users or meta specifically.
rg -n -C4 'addIndex|indexes|using\s*:\s*["'\'']gin["'\'']|using.*gin|jsonb_path_ops|meta' src/database/migrations src/database/models | rg 'users?|users\.|User|meta'Repository: ELEVATE-Project/user
Length of output: 18474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the surrounding query logic for searchUsersWithOrganization.
sed -n '394,505p' src/database/queries/users.js | cat -n
# Show indexes or associations used by the query on users / user_organizations / user_roles.
rg -n -C2 'addIndex|indexes:|belongsToMany|hasMany|organization_id|tenant_code|roleIds|metaFilters' src/database/{models,migrations} | rg 'users|user_organizations|user_roles|searchUsersWithOrganization'Repository: ELEVATE-Project/user
Length of output: 17734
Use an indexable JSONB predicate for metaFilters.
"User".meta->>'${key}' = value only benefits from a per-key expression index; without one, this filter will scan users. For equality lookups, switch to a JSONB containment predicate (meta @> ...) and add/verify a GIN index on users.meta.
🤖 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 `@src/database/queries/users.js` around lines 436 - 444, The generic
metaFilters handling in the user query currently uses a per-key JSONB text
extraction predicate; update this logic to use a parameterized JSONB containment
predicate on User.meta, while preserving key validation and filtering of null,
undefined, and empty values. Add or verify a GIN index on users.meta so these
equality lookups are indexable.
Source: Path instructions
| row.meta[field] = row[field] | ||
| .split(',') | ||
| .map((val) => { | ||
| const cleanVal = val.trim().replaceAll(/\s+/g, '').toLowerCase() | ||
| const lookupKey = `${cleanVal}${cleanField}` | ||
| return externalEntityNameIdMap?.[lookupKey]?._id | ||
| }) | ||
| .filter(Boolean) // Removes null/undefined if an ID isn't found |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing fallback causes data loss for non-external ARRAY fields.
When mapping ARRAY types, the code exclusively attempts to resolve external IDs via externalEntityNameIdMap. If the field is a standard/non-external entity (or if the external mapping is missing), the lookup evaluates to undefined, which .filter(Boolean) then drops entirely. This silently wipes out valid incoming array data.
Align this array processing with the single-value logic below it by returning the original string value when an external ID is not found.
🐛 Proposed fix
row.meta[field] = row[field]
.split(',')
.map((val) => {
const cleanVal = val.trim().replaceAll(/\s+/g, '').toLowerCase()
const lookupKey = `${cleanVal}${cleanField}`
- return externalEntityNameIdMap?.[lookupKey]?._id
+ const matchedId = externalEntityNameIdMap?.[lookupKey]?._id
+ return matchedId !== undefined && matchedId !== null ? matchedId : val.trim()
})
.filter(Boolean) // Removes null/undefined if an ID isn't found📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| row.meta[field] = row[field] | |
| .split(',') | |
| .map((val) => { | |
| const cleanVal = val.trim().replaceAll(/\s+/g, '').toLowerCase() | |
| const lookupKey = `${cleanVal}${cleanField}` | |
| return externalEntityNameIdMap?.[lookupKey]?._id | |
| }) | |
| .filter(Boolean) // Removes null/undefined if an ID isn't found | |
| row.meta[field] = row[field] | |
| .split(',') | |
| .map((val) => { | |
| const cleanVal = val.trim().replaceAll(/\s+/g, '').toLowerCase() | |
| const lookupKey = `${cleanVal}${cleanField}` | |
| const matchedId = externalEntityNameIdMap?.[lookupKey]?._id | |
| return matchedId !== undefined && matchedId !== null ? matchedId : val.trim() | |
| }) | |
| .filter(Boolean) // Removes null/undefined if an ID isn't found |
🤖 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 `@src/helpers/userInvite.js` around lines 367 - 374, Update the ARRAY mapping
in the row field-processing logic to fall back to the trimmed original value
when externalEntityNameIdMap does not contain a matching ID. Preserve the
existing resolved _id behavior for mapped external values and keep
filter(Boolean) for genuinely empty values, matching the single-value handling
below.
| const notificationUtils = require('@utils/notification') | ||
| const userHelper = require('@helpers/userHelper') | ||
| const { broadcastEvent } = require('@helpers/eventBroadcasterMain') | ||
| const { use } = require('i18next') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the newly introduced unused bindings. These three sites fail the reported ESLint no-unused-vars rule.
src/services/account.js#L42-L42: remove the unusedi18next.useimport.src/services/account.js#L1730-L1730: use.catch(() => {})or log the caught error.src/validators/v1/user.js#L9-L9: remove the unused blacklistuserimport.
🧰 Tools
🪛 ESLint
[error] 42-42: 'use' is assigned a value but never used.
(no-unused-vars)
📍 Affects 2 files
src/services/account.js#L42-L42(this comment)src/services/account.js#L1730-L1730src/validators/v1/user.js#L9-L9
🤖 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 `@src/services/account.js` at line 42, Remove the unused i18next.use binding at
src/services/account.js:42-42 and the unused blacklist user import at
src/validators/v1/user.js:9-9. At src/services/account.js:1730-1730, handle the
promise rejection with .catch(() => {}) or log the caught error.
Source: Linters/SAST tools
| } else if (req.headers?.[common.TENANT_CODE_HEADER]) { | ||
| tenantId = req.headers?.[common.TENANT_CODE_HEADER] | ||
| tenantDomain = { tenant_code: tenantId } | ||
| } | ||
|
|
||
| const tenantDetail = await tenantQueries.findOne({ | ||
| code: tenantDomain.tenant_code, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle a missing tenant header and resolve the complete domain record.
When both domain and the tenant header are absent, Line 84 dereferences tenantDomain and returns a 500. The header branch also creates an object without domain, so registration notifications later receive an undefined portal URL. Return a client error for missing tenant context and fetch the actual tenant-domain record by tenant code.
As per path instructions, src/services/** must be checked for correctness, efficiency, and potential edge cases.
🤖 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 `@src/services/account.js` around lines 78 - 84, Update the tenant-resolution
logic before tenantQueries.findOne to return a client error when neither domain
nor the TENANT_CODE_HEADER is available, avoiding tenantDomain.tenant_code
dereferencing. In the header branch, resolve the complete tenant-domain record
by tenant code, including its domain/portal URL, so downstream registration
notifications receive a populated record.
Source: Path instructions
| { | ||
| title: { | ||
| [Op.in]: process.env.DEFAULT_ROLE.split(','), | ||
| [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Do not dereference or split unvalidated req.body.roles.
req defaults to {}, so existing callers that omit it crash on req.body.roles. An array or other non-string roles value also crashes on .split(). Normalize an optional string/array before building Op.in, otherwise use the default roles.
Proposed fix
+const requestedRoles = req.body?.roles
+const requestedRoleTitles = Array.isArray(requestedRoles)
+ ? requestedRoles
+ : typeof requestedRoles === 'string'
+ ? requestedRoles.split(',')
+ : process.env.DEFAULT_ROLE.split(',')
+
role = await roleQueries.findAll({
title: {
- [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','),
+ [Op.in]: requestedRoleTitles,
},As per path instructions, src/services/** must be checked for correctness, efficiency, and potential edge cases.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','), | |
| const requestedRoles = req.body?.roles | |
| const requestedRoleTitles = Array.isArray(requestedRoles) | |
| ? requestedRoles | |
| : typeof requestedRoles === 'string' | |
| ? requestedRoles.split(',') | |
| : process.env.DEFAULT_ROLE.split(',') | |
| role = await roleQueries.findAll({ | |
| title: { | |
| [Op.in]: requestedRoleTitles, | |
| }, |
🤖 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 `@src/services/account.js` at line 325, Update the roles value used by Op.in in
the account service to safely normalize an optional req.body.roles only when
req.body exists and the value is a string or array; split strings into role
entries, preserve arrays, and otherwise fall back to process.env.DEFAULT_ROLE
split into an array. Ensure callers omitting req or body no longer throw and
avoid dereferencing or splitting unvalidated values.
Source: Path instructions
| // Check for admin role or tenant admin role | ||
| const hasAdminRole = user.user_organizations?.some((org) => | ||
| org.roles?.some((r) => r.role?.title?.toLowerCase() === common.ADMIN_ROLE) | ||
| org.roles?.some((r) => { | ||
| const roleTitle = r.role?.title?.toLowerCase() | ||
| return roleTitle === common.ADMIN_ROLE || roleTitle === common.TENANT_ADMIN_ROLE | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'tokenDetail|decodedToken\.data\.roles|generateToken|UserTransformDTO\.transform' \
src/services/admin.js src/middlewares/authenticator.js srcRepository: ELEVATE-Project/user
Length of output: 27001
🏁 Script executed:
#!/bin/bash
sed -n '450,545p' src/services/admin.js
printf '\n---- authenticator ----\n'
sed -n '200,340p' src/middlewares/authenticator.js
printf '\n---- user transform ----\n'
rg -n -C3 'class UserTransformDTO|transform\(' src -g '*UserTransformDTO*' -g '*user*transform*' -g '*transform*.js'Repository: ELEVATE-Project/user
Length of output: 7928
🏁 Script executed:
#!/bin/bash
fd -i 'UserTransformDTO' srcRepository: ELEVATE-Project/user
Length of output: 158
🏁 Script executed:
#!/bin/bash
sed -n '450,545p' src/services/admin.js
printf '\n---- authenticator ----\n'
sed -n '200,340p' src/middlewares/authenticator.js
printf '\n---- user transform ----\n'
fd -i 'UserTransformDTO' srcRepository: ELEVATE-Project/user
Length of output: 7928
🏁 Script executed:
#!/bin/bash
rg -n -C3 'class UserTransformDTO|transform\(' srcRepository: ELEVATE-Project/user
Length of output: 3329
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' src/dtos/userDTO.jsRepository: ELEVATE-Project/user
Length of output: 4083
Add roles to the admin JWT payload. src/middlewares/authenticator.js reads decodedToken.data.roles to set isAdmin and isTenantAdmin, but this login path only puts organizations in the token. Include the roles array in tokenDetail.data, or tenant-admin/admin requests will be treated as non-admin on the next hop.
🤖 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 `@src/services/admin.js` around lines 478 - 483, Update the login token
construction in the admin service so tokenDetail.data includes the user’s roles
array alongside organizations. Reuse the roles data represented by the
admin-role check near hasAdminRole, ensuring authenticator.js can read
decodedToken.data.roles and preserve admin and tenant-admin status on subsequent
requests.
Source: Path instructions
| if (isAdmin && bodyData.roles && Array.isArray(bodyData.roles) && bodyData.roles.length > 0) { | ||
| let roles = _.without(bodyData.roles, common.ADMIN_ROLE) | ||
| let getRoleIds = await roleQueries.findAll({ title: roles }, { attributes: ['id'] }) | ||
| if (!getRoleIds) { | ||
| if (!getRoleIds || getRoleIds.length === 0) { | ||
| return responses.failureResponse({ | ||
| message: 'INVALID_ROLE_ASSIGNMENTS', | ||
| statusCode: httpStatusCode.bad_request, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
| let roleIds = getRoleIds.map((roleId) => roleId.id) | ||
| let checkUser = await userQueries.findOne({ id: userId }) | ||
| if (!checkUser) { | ||
| return responses.failureResponse({ | ||
| responseCode: 'CLIENT_ERROR', | ||
| statusCode: httpStatusCode.bad_request, | ||
| message: 'USER_NOT_FOUND', | ||
| }) | ||
| updateData.roles = roleIds | ||
| hasRoleUpdate = true | ||
| } | ||
|
|
||
| // Handle profile updates | ||
| // Extract profile fields (everything except organization_id and roles) | ||
| let profileFields = { ...bodyData } | ||
| delete profileFields.organization_id | ||
| delete profileFields.roles | ||
|
|
||
| if (Object.keys(profileFields).length > 0) { | ||
| // Use userService.update logic for profile updates | ||
| const userService = require('@services/user') | ||
|
|
||
| const orgCode = tokenInformation.organization_code | ||
| const tenantCode = tokenInformation.tenant_code | ||
|
|
||
| // For PATCH operations from org-admin, skip required field validation | ||
| // Only update the fields that are sent (province, site, location) | ||
| const skipRequiredValidation = true | ||
|
|
||
| // Call userService.update to handle profile validation and update | ||
| const profileUpdateResult = await userService.update( | ||
| profileFields, | ||
| userId, | ||
| orgCode, | ||
| tenantCode, | ||
| skipRequiredValidation | ||
| ) | ||
|
|
||
| if ( | ||
| profileUpdateResult.responseCode !== 'OK' && | ||
| profileUpdateResult.statusCode !== httpStatusCode.ok.status | ||
| ) { | ||
| return profileUpdateResult | ||
| } | ||
| await userQueries.updateUser({ id: userId }, { roles: roleIds }) | ||
| return responses.successResponse({ | ||
| statusCode: httpStatusCode.ok, | ||
| message: 'USER_ROLE_UPDATE_SUCCESSFUL', | ||
| }) | ||
| } else { | ||
| return responses.failureResponse({ | ||
| responseCode: 'CLIENT_ERROR', | ||
| statusCode: httpStatusCode.bad_request, | ||
| message: 'YOU_DONT_HAVE_ACCESS_TO_UPDATE_ROLES', | ||
| }) | ||
| hasProfileUpdate = true | ||
| } | ||
|
|
||
| // Update roles if provided (only for admin) | ||
| if (hasRoleUpdate) { | ||
| await userQueries.updateUser({ id: userId }, updateData) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Restore tenant and organization scoping before updating roles.
Both roleQueries.findAll({ title: roles }) and updateUser({ id: userId }) omit tenant scope, while the target membership check was removed. An authorized admin can therefore resolve another tenant’s roles or mutate an arbitrary user ID, and zero affected rows still produce success. Verify target membership in tokenInformation.organization_code, scope role lookup and update by tenant, and check affected rows.
As per path instructions, src/services/** must be checked for correctness and potential edge cases.
🤖 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 `@src/services/org-admin.js` around lines 621 - 674, The role assignment flow
in the organization-admin update must restore target membership validation and
tenant scoping. Before processing roles, verify userId belongs to
tokenInformation.organization_code; constrain roleQueries.findAll and
userQueries.updateUser with the appropriate tenant and organization fields, and
validate the update result’s affected-row count, returning the existing failure
response when no membership is updated.
Source: Path instructions
| const existingPhoneUser = await userQueries.findOne( | ||
| { | ||
| phone: bodyData.phone, | ||
| tenant_code: tenantCode, | ||
| id: { [Op.ne]: id }, | ||
| }, | ||
| { attributes: ['id'] } | ||
| ) | ||
| if (existingPhoneUser) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include phone_code in the phone uniqueness check.
Querying for existing users using only the phone field creates a false-positive edge case. If another user in the tenant shares the same phone number but has a different phone_code (country code), this check will incorrectly block the update with PHONE_ALREADY_EXISTS.
To accurately determine duplicates (and align with duplicate-checking logic seen in userInvite.js), include phone_code in the query constraint.
🛠️ Proposed fix
const existingPhoneUser = await userQueries.findOne(
{
phone: bodyData.phone,
+ phone_code: bodyData.phone_code || user.phone_code,
tenant_code: tenantCode,
id: { [Op.ne]: id },
},
{ attributes: ['id'] }
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const existingPhoneUser = await userQueries.findOne( | |
| { | |
| phone: bodyData.phone, | |
| tenant_code: tenantCode, | |
| id: { [Op.ne]: id }, | |
| }, | |
| { attributes: ['id'] } | |
| ) | |
| if (existingPhoneUser) { | |
| const existingPhoneUser = await userQueries.findOne( | |
| { | |
| phone: bodyData.phone, | |
| phone_code: bodyData.phone_code || user.phone_code, | |
| tenant_code: tenantCode, | |
| id: { [Op.ne]: id }, | |
| }, | |
| { attributes: ['id'] } | |
| ) | |
| if (existingPhoneUser) { |
🤖 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 `@src/services/user.js` around lines 120 - 128, Update the existingPhoneUser
query in the user update flow to include bodyData.phone_code alongside phone,
tenant_code, and the excluded id. Preserve the existing duplicate handling while
ensuring uniqueness is checked against the combined phone and phone_code values.
Source: Path instructions
| const validatePhoneWithCode = (req) => { | ||
| // Numbers only, no length restriction (digit-count enforcement left to the UI) | ||
| req.checkBody('phone') | ||
| .optional() | ||
| .trim() | ||
| .matches(/^[0-9]+$/) | ||
| .withMessage('phone must contain only numbers') | ||
|
|
||
| // phone is only ever encrypted/stored together with phone_code | ||
| req.checkBody(['phone', 'phone_code']).custom(() => { | ||
| if (req.body.phone && !req.body.phone_code) { | ||
| throw new Error('phone_code is required when phone is provided') | ||
| } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce server-side phone and phone-code constraints.
Numeric-only validation permits empty-scale or arbitrarily long phone data, while phone_code receives no type or format validation. UI enforcement is bypassable; validate both fields with bounded lengths and the supported country-code format before encryption/storage.
As per path instructions, src/validators/** must validate all incoming data thoroughly and check for incomplete rules.
🤖 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 `@src/validators/v1/account.js` around lines 29 - 43, Update
validatePhoneWithCode to enforce server-side bounds and formats for both phone
and phone_code before encryption or storage: require phone to contain only
digits within the supported minimum and maximum length, and validate phone_code
as a numeric country code with its supported bounded format. Preserve the
conditional requirement that phone_code is required whenever phone is provided,
while ensuring optional absent fields remain valid.
Source: Path instructions
| accountCreate: (req) => { | ||
| // This route calls the same accountService.create() as account.js's create(), which | ||
| // previously had no validator entry at all. Reuses account.js's shared update() | ||
| // validation (name + phone/phone_code) rather than create() itself, since create()'s | ||
| // username/email/password rules don't match what this route actually receives (e.g. | ||
| // dotted usernames like "first.last2" are valid here but rejected by create()'s regex). | ||
| accountValidators.update(req) | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Use a dedicated create validator instead of partial-update validation.
This route reaches accountService.create, which hashes password unconditionally, but accountValidators.update neither requires nor applies the password policy. It also omits creation-specific name, identifier, username, role, and request-body allowlisting rules. Add an accountCreate schema tailored to this route rather than reusing PATCH validation.
As per path instructions, src/validators/** must validate all incoming data thoroughly and check for missing rules.
🤖 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 `@src/validators/v1/tenant.js` around lines 143 - 150, Replace the
accountCreate implementation’s delegation to accountValidators.update with a
dedicated create validator schema for this route. Require and validate password
before accountService.create hashes it, and include creation-specific validation
for name, identifier, username, role, plus strict request-body allowlisting;
preserve the route’s accepted dotted-username format while covering all incoming
fields.
Source: Path instructions
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Deployment