Organization list fix for SCP #793
Conversation
WalkthroughOrganizationsHelper.list now builds a dynamic filter including tenant_code and ACTIVE status, and optionally adds a code IN condition from normalized organization_codes; the ORM call uses findAll(filter, options). A new validator list(req) validates optional organization_codes array and its elements. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant V as Validator.list
participant H as OrganizationsHelper.list
participant DB as ORM/DB
C->>V: list(req)
V-->>C: validated params
C->>H: list(params, options)
H->>H: Build filter { tenant_code, status: ACTIVE }
alt organization_codes provided
H->>H: Normalize codes (trim, lowercase) and add code IN [...]
end
H->>DB: findAll(filter, options)
DB-->>H: results
H-->>C: results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🧹 Nitpick comments (1)
src/services/organization.js (1)
257-260: Consider an index to support this hot pathFilter is consistently on tenant_code + status, optionally code. Add/ensure a composite index on
(tenant_code, status, code)or at least(tenant_code, status)in the organizations table to avoid full scans.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/organization.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit Configuration File
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/organization.js
🧬 Code Graph Analysis (1)
src/services/organization.js (7)
src/services/org-admin.js (2)
filter(490-494)common(10-10)src/controllers/v1/account.js (1)
filter(336-336)src/database/queries/notificationTemplate.js (2)
filter(30-41)filter(64-75)src/services/user.js (7)
filter(63-70)filter(234-237)filter(269-269)filter(300-300)filter(427-427)filter(536-542)common(10-10)src/services/notification.js (2)
filter(55-58)filter(96-96)src/middlewares/authenticator.js (2)
filter(37-37)common(11-11)src/services/form.js (3)
filter(58-58)filter(127-129)common(2-2)
🔇 Additional comments (1)
src/services/organization.js (1)
267-267: LGTM: Using the constructed filter with findAll is correctSwitching to
findAll(filter, options)matches surrounding patterns and keeps concerns separated.
| let filter = { | ||
| tenant_code: params?.query?.tenantCode, | ||
| status: common.ACTIVE_STATUS, | ||
| } | ||
| if (params.body && params.body.organizationCodes) { | ||
| filter.code = { | ||
| [Op.in]: params.body.organizationCodes.map((code) => code.toString().toLowerCase().trim()), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against empty/malformed organizationCodes and align input handling with organizationIds
Current logic will generate IN [] when organizationCodes is an empty array, which can lead to invalid SQL or always-false conditions depending on the dialect. It also assumes organizationCodes is always an array, unlike organizationIds which supports JSON strings. Be defensive and normalize/dedupe inputs, and short-circuit on empty.
Apply this diff:
- let filter = {
- tenant_code: params?.query?.tenantCode,
- status: common.ACTIVE_STATUS,
- }
- if (params.body && params.body.organizationCodes) {
- filter.code = {
- [Op.in]: params.body.organizationCodes.map((code) => code.toString().toLowerCase().trim()),
- }
- }
+ let filter = {
+ tenant_code: params?.query?.tenantCode,
+ status: common.ACTIVE_STATUS,
+ }
+
+ const orgCodesInput = params?.body?.organizationCodes
+ if (orgCodesInput != null) {
+ // Coerce to array when organizationCodes comes as a JSON string or a single value
+ const arr =
+ Array.isArray(orgCodesInput)
+ ? orgCodesInput
+ : (typeof orgCodesInput === 'string' &&
+ orgCodesInput.trim().startsWith('[') &&
+ orgCodesInput.trim().endsWith(']'))
+ ? JSON.parse(orgCodesInput)
+ : [orgCodesInput]
+
+ const cleanedCodes = [...new Set(
+ arr
+ .map((c) => (c ?? '').toString().trim())
+ .filter((c) => c.length > 0)
+ .map((c) => c.toLowerCase())
+ )]
+
+ // Avoid generating an invalid IN () clause
+ if (cleanedCodes.length === 0) {
+ return responses.successResponse({
+ statusCode: httpStatusCode.ok,
+ message: 'ORGANIZATION_FETCHED_SUCCESSFULLY',
+ result: [],
+ })
+ }
+
+ filter.code = { [Op.in]: cleanedCodes }
+ }📝 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.
| let filter = { | |
| tenant_code: params?.query?.tenantCode, | |
| status: common.ACTIVE_STATUS, | |
| } | |
| if (params.body && params.body.organizationCodes) { | |
| filter.code = { | |
| [Op.in]: params.body.organizationCodes.map((code) => code.toString().toLowerCase().trim()), | |
| } | |
| } | |
| let filter = { | |
| tenant_code: params?.query?.tenantCode, | |
| status: common.ACTIVE_STATUS, | |
| } | |
| const orgCodesInput = params?.body?.organizationCodes | |
| if (orgCodesInput != null) { | |
| // Coerce to array when organizationCodes comes as a JSON string or a single value | |
| const arr = | |
| Array.isArray(orgCodesInput) | |
| ? orgCodesInput | |
| : (typeof orgCodesInput === 'string' && | |
| orgCodesInput.trim().startsWith('[') && | |
| orgCodesInput.trim().endsWith(']')) | |
| ? JSON.parse(orgCodesInput) | |
| : [orgCodesInput] | |
| const cleanedCodes = [...new Set( | |
| arr | |
| .map((c) => (c ?? '').toString().trim()) | |
| .filter((c) => c.length > 0) | |
| .map((c) => c.toLowerCase()) | |
| )] | |
| // Avoid generating an invalid IN () clause | |
| if (cleanedCodes.length === 0) { | |
| return responses.successResponse({ | |
| statusCode: httpStatusCode.ok, | |
| message: 'ORGANIZATION_FETCHED_SUCCESSFULLY', | |
| result: [], | |
| }) | |
| } | |
| filter.code = { [Op.in]: cleanedCodes } | |
| } |
🤖 Prompt for AI Agents
In src/services/organization.js around lines 257 to 266, the code blindly builds
an IN clause from params.body.organizationCodes which can be empty or non-array;
instead, defensively normalize and validate the input (like organizationIds): if
params.body.organizationCodes is a string attempt JSON.parse, if it's not an
array treat as missing; map each entry to toString().toLowerCase().trim(),
filter out empty/falsy values and deduplicate the list; only when the resulting
array has length > 0 set filter.code = { [Op.in]: normalizedArray }; otherwise
do not add filter.code (short-circuit) to avoid generating IN [] or bad SQL.
src/services/organization.js
Outdated
| [Op.in]: params.body.organizationCodes.map((code) => code.toString().toLowerCase().trim()), | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Lowercasing codes may miss matches unless the DB stores codes normalized or uses case-insensitive comparison
Mapping to toLowerCase() assumes organization codes are stored lowercase or compared case-insensitively. If the DB stores mixed/upper case and uses case-sensitive collation, this will return no rows.
Options:
- Standardize on lowercase at write time (create/update) for
organizations.code, and keep this query as-is. - Or, switch to a case-insensitive predicate, e.g.,
WHERE LOWER(code) IN (...)via Sequelize helpers.
Case-insensitive option (requires importing fn and col):
const { Op, fn, col } = require('sequelize')
// ...
filter[Op.and] = [
// remove filter.code assignment
where(fn('LOWER', col('code')), { [Op.in]: cleanedCodes })
]Verify whether normalization already exists in the codebase. Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Where do we create/update organizations?"
rg -n -A2 -B2 "organizationQueries\.(create|update)\("
echo "Any evidence of lowercasing organization 'code' at write time?"
rg -n -A3 -B3 "code:\s*.*toLowerCase\("
echo "Any case-insensitive code comparisons?"
rg -n -A2 -B2 "fn\(['\"]lower['\"]|LOWER\(|iLike"
echo "Look for organization model definition to check 'code' handling:"
fd -t f "(organization|organizations).*\.js" | xargs rg -n -A3 -B3 "(sequelize\.define|Model\.init|code:)"Length of output: 49195
Ensure consistent casing for organization codes
The current query lowercases incoming codes but there’s no runtime normalization of Organization.code on create/update—only a one-off migration. If the DB ever contains mixed- or uppercase codes (or uses a case-sensitive collation), this filter will miss matches.
Please address at src/services/organization.js:263–264:
- Either enforce lowercase on write (e.g. add a setter in the Organization model or call
bodyData.code = bodyData.code.toLowerCase().trim()beforeorganizationQueries.create/update) - Or switch this lookup to a truly case-insensitive predicate:
- filter.code = { [Op.in]: params.body.organizationCodes.map(code =>
- code.toString().toLowerCase().trim()
- ) }
+ const cleanedCodes = params.body.organizationCodes.map(code =>
+ code.toString().toLowerCase().trim()
+ )
+ filter[Op.and] = [
+ where(fn('LOWER', col('code')), { [Op.in]: cleanedCodes })
+ ]Committable suggestion skipped: line range outside the PR's diff.
|
Body needs to be in snake_case for consistency. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/services/organization.js (2)
263-264: Case sensitivity: LOWER() vs. assuming lowercase storageLowercasing input assumes
organizations.codeis stored lowercase or compared case-insensitively. If DB collation is case-sensitive and legacy rows contain mixed case, this may silently miss matches. Either enforce lowercase on write everywhere or do a case-insensitive predicate here.Optional case-insensitive lookup (requires extra imports):
- const { Op } = require('sequelize') + const { Op, fn, col, where } = require('sequelize')Within the block on Line 261–265, replace the assignment with:
// Instead of: filter.code = { [Op.in]: cleanedCodes } filter[Op.and] = [where(fn('LOWER', col('code')), { [Op.in]: cleanedCodes })]Please confirm whether the model or write-path already enforces lowercase codes; if yes, keeping the current approach is fine.
261-265: Prevent IN [] and normalize organization_codes (align with organizationIds)If
params.body.organization_codesis an empty array, this buildsIN []which is invalid on some dialects or always-false on others. Also, unlikeorganizationIds(Line 275 onward), this code doesn’t accept JSON string input or dedupe values.Refactor to defensively normalize, dedupe, and short-circuit on empty. This mirrors the robustness already present for
organizationIds.Apply this diff:
- if (params.body && params.body.organization_codes) { - filter.code = { - [Op.in]: params.body.organization_codes.map((code) => code.toString().toLowerCase().trim()), - } - } + const orgCodesInput = params?.body?.organization_codes + if (orgCodesInput != null) { + // Coerce to array when organization_codes comes as a JSON string or a single value + const arr = + Array.isArray(orgCodesInput) + ? orgCodesInput + : (typeof orgCodesInput === 'string' && + orgCodesInput.trim().startsWith('[') && + orgCodesInput.trim().endsWith(']')) + ? JSON.parse(orgCodesInput) + : [orgCodesInput] + + const cleanedCodes = [...new Set( + arr + .map((c) => (c ?? '').toString().trim()) + .filter((c) => c.length > 0) + .map((c) => c.toLowerCase()) + )] + + // Avoid generating an invalid/pointless IN () clause + if (cleanedCodes.length === 0) { + return responses.successResponse({ + statusCode: httpStatusCode.ok, + message: 'ORGANIZATION_FETCHED_SUCCESSFULLY', + result: [], + }) + } + + filter.code = { [Op.in]: cleanedCodes } + }src/validators/v1/organization.js (1)
146-153: Confirm input shape: array vs. JSON string (align with organizationIds handling)Service code for
organizationIdsaccepts JSON strings;organization_codescurrently requires an Array only. If clients may send JSON strings (e.g.,'["a","b"]'), either:
- Allow string JSON in the service normalization (preferred; see suggested service refactor on Line 261–265), or
- Update API docs to mandate arrays and ensure clients conform.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/organization.js(1 hunks)src/validators/v1/organization.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/validators/**
⚙️ CodeRabbit Configuration File
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/organization.js
src/services/**
⚙️ CodeRabbit Configuration File
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/organization.js
🧠 Learnings (1)
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/services/organization.js
🧬 Code Graph Analysis (1)
src/services/organization.js (3)
src/services/user.js (9)
filter(63-70)filter(234-237)filter(269-269)filter(300-300)filter(427-427)filter(536-542)common(10-10)options(311-315)options(432-436)src/middlewares/authenticator.js (2)
filter(37-37)common(11-11)src/database/queries/organization.js (1)
common(4-4)
🔇 Additional comments (2)
src/services/organization.js (1)
267-267: LGTM: Using the dynamic filter with findAll(filter, options)Switching to
findAll(filter, options)keeps the call consistent with other query helpers and enables optional code filtering without altering callers.src/validators/v1/organization.js (1)
145-158: Validator added with snake_case input; overall looks goodGood addition for
organization_codesvalidation and consistent snake_case naming.
| tenant_code: params?.query?.tenantCode, | ||
| status: common.ACTIVE_STATUS, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify tenantCode trust boundary
You’re trusting params.query.tenantCode directly to scope results. Per retrieved learnings, tenant/organization identifiers are typically sourced from req.decodedToken and guaranteed after token validation. Allowing a query override might enable cross-tenant data access unless the controller gates this route to privileged roles.
To verify call-sites and access control, run:
If this endpoint is not strictly admin-scoped, prefer deriving tenant from the token instead of a query param, or ignore the query value when a token is present.
🏁 Script executed:
#!/bin/bash
# Find where OrganizationsHelper.list is invoked and inspect how tenantCode is sourced/gated
rg -n -A4 -B4 "OrganizationsHelper\.list\("
# Also check any controllers/middlewares that handle 'tenantCode' for list endpoints
rg -n -A3 -B3 "tenantCode" | rg -n -A3 -B3 "list|organization"Length of output: 70421
🏁 Script executed:
#!/bin/bash
# Identify how the organization service is imported in the v1 controller
rg -n "require.*services/organization" src/controllers/v1/organization.js
# Locate where the list method is invoked in the organization controller
rg -n "await .*\.list" src/controllers/v1/organization.js
rg -n "\.list(" -n src/controllers/v1/organization.jsLength of output: 361
Remove direct query override for tenantCode in organization.list
The orgService.list method is currently trusting req.query.tenantCode, but the controller computes a safe tenantCode from the token (and body for admins) without forwarding it. As a result, anyone can supply ?tenantCode=… on the URL and scope results to another tenant.
• In src/controllers/v1/organization.js around line 119, replace:
const result = await orgService.list(req)with something like:
// forward only the vetted tenantCode into the service
const params = {
query: { tenantCode }, // use the controller’s computed tenantCode
body: req.body,
decodedToken: req.decodedToken
}
const result = await orgService.list(params)• In src/services/organization.js (lines 253–260), change the tenant filter to use the token-sourced value instead of the raw query:
- tenant_code: params?.query?.tenantCode,
+ tenant_code: params?.query?.tenantCode || params?.decodedToken?.tenant_code,—or remove the params.query branch entirely and rely solely on params.decodedToken.tenant_code (with an explicit admin override if needed).
This ensures the tenantCode always comes from the validated token (or an explicit, authorized override) rather than an untrusted query parameter.
🤖 Prompt for AI Agents
In src/services/organization.js around lines 258 to 260, the tenant filter is
using params?.query?.tenantCode which trusts an unvalidated query param; change
it to use the vetted tenant from the token (params.decodedToken.tenant_code) or
remove the params.query branch entirely and only accept an explicit, authorized
override via decodedToken (i.e., ignore req.query.tenantCode and derive
tenantCode from params.decodedToken or an admin-authorized field before applying
the filter).
| req.checkBody('organization_codes') | ||
| .optional({ checkFalsy: true }) | ||
| .custom((value) => { | ||
| if (!Array.isArray(value)) { | ||
| throw new Error('organization_codes must be an array') | ||
| } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cap the size of organization_codes to prevent large IN queries
Add an upper bound (e.g., 100 items) to avoid excessive IN (...) lists that can degrade DB performance or be abused.
Apply this diff:
- req.checkBody('organization_codes')
- .optional({ checkFalsy: true })
- .custom((value) => {
- if (!Array.isArray(value)) {
- throw new Error('organization_codes must be an array')
- }
- return true
- })
+ req.checkBody('organization_codes')
+ .optional({ checkFalsy: true })
+ .custom((value) => {
+ if (!Array.isArray(value)) {
+ throw new Error('organization_codes must be an array')
+ }
+ if (value.length > 100) {
+ throw new Error('organization_codes cannot exceed 100 entries')
+ }
+ return true
+ })📝 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.
| req.checkBody('organization_codes') | |
| .optional({ checkFalsy: true }) | |
| .custom((value) => { | |
| if (!Array.isArray(value)) { | |
| throw new Error('organization_codes must be an array') | |
| } | |
| return true | |
| }) | |
| req.checkBody('organization_codes') | |
| .optional({ checkFalsy: true }) | |
| .custom((value) => { | |
| if (!Array.isArray(value)) { | |
| throw new Error('organization_codes must be an array') | |
| } | |
| if (value.length > 100) { | |
| throw new Error('organization_codes cannot exceed 100 entries') | |
| } | |
| return true | |
| }) |
🤖 Prompt for AI Agents
In src/validators/v1/organization.js around lines 146 to 153, the custom
validator for organization_codes currently only ensures the value is an array;
add an upper bound check to prevent very large IN queries by validating that
value.length is <= 100 and throw a clear Error (e.g., 'organization_codes cannot
contain more than 100 items') if exceeded; implement this inside the same custom
validator so requests with more than 100 items fail validation early.
2 participants
Summary by CodeRabbit