feat(auth): allow admin override of tenant/org #839
Conversation
… environment variables
WalkthroughReplace hardcoded tenant/organization header names with environment-configurable names, register those env vars, add admin-only header-override validation and token-data mutation in the authenticator, update controllers to read headers via the new constants, and add three English locale keys. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Auth as Authenticator
participant Env as Env (header names)
participant Headers
participant OrgQ as organizationQueries
participant TenantQ as tenantDomainQueries
participant Controller
Client->>Auth: HTTP request (auth token + optional headers)
Auth->>Auth: decode token -> decodedToken
alt decodedToken.data.role == "admin"
Auth->>Env: read ORG_ID/ORG_CODE/TENANT_CODE names
Auth->>Headers: read headers by those names (lowercase, trim)
alt any override header present
Auth->>Auth: require all three present else throw CLIENT_ERROR
Auth->>Auth: validate org_id is positive integer else throw CLIENT_ERROR
Auth->>OrgQ: query org by organization_code + tenant_code + active/not deleted
OrgQ-->>Auth: org (or not found)
Auth->>Auth: set decodedToken.data.{tenant_code,organization_id,organization_code}
else
Auth->>Auth: keep decodedToken.data as-is
end
else
Auth->>Auth: keep decodedToken.data as-is
end
alt public path / tenant resolution
Auth->>Env: read TENANT_CODE header name
Auth->>Headers: read tenant header or domain
Auth->>TenantQ: tenantDomainQueries.findOne(tenantFilter)
TenantQ-->>Auth: tenant (or not found)
Auth->>Auth: set req.body.tenant_code
end
Auth->>Controller: attach req.decodedToken and continue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 3
🧹 Nitpick comments (2)
src/envVariables.js (1)
461-475: Consider clarifying the message field.The message field states "Required X" while
optional: true, which could be confusing. This pattern exists throughout the file, but for new entries, consider using clearer messages like "Header name for organization ID (default: X-Org-Id)".src/middlewares/authenticator.js (1)
216-263: Consider adding audit logging for admin overrides.Admin tenant/organization overrides are a powerful capability that bypasses normal access controls. For security and compliance, consider logging when admins use this feature, including:
- Admin user ID and name
- Original vs overridden values
- Timestamp and endpoint accessed
- Result (success/failure)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/constants/common.js(1 hunks)src/envVariables.js(1 hunks)src/locales/en.json(1 hunks)src/middlewares/authenticator.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.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/middlewares/authenticator.js
🧬 Code graph analysis (2)
src/middlewares/authenticator.js (3)
src/services/organization-feature.js (3)
isAdmin(36-36)orgCode(38-38)tenantCode(39-39)src/controllers/v1/organization-feature.js (3)
isAdmin(27-27)orgCode(68-68)tenantCode(69-69)src/controllers/v1/public.js (1)
tenantCode(7-7)
src/constants/common.js (1)
src/database/models/index.js (1)
process(6-6)
🔇 Additional comments (3)
src/locales/en.json (1)
185-187: LGTM!The new localization keys for admin override functionality are clear and properly structured. The trailing comma addition on line 185 follows JSON best practices for maintainability.
src/constants/common.js (1)
105-106: LGTM!Migrating to environment-based configuration enables flexibility across deployments. The middleware reads these same environment variables directly (lines 219-221 in authenticator.js), maintaining consistency.
src/middlewares/authenticator.js (1)
162-162: LGTM!The early declaration of
decodedTokenis necessary for the updated control flow.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/middlewares/authenticator.js (1)
216-261: Add database validation for overridden organization and tenant.The admin override logic accepts any organization ID, code, and tenant code from headers without verifying:
- The tenant exists in the database
- The organization exists in the database
- The organization belongs to the specified tenant
This creates a security risk in multi-tenant architecture where admins could:
- Set non-existent tenant/org combinations leading to undefined behavior
- Potentially bypass tenant isolation boundaries
- Cause downstream errors when services expect valid references
The past review comment on lines 224-259 correctly identified this gap and remains unaddressed.
Consider adding async database lookups before setting the override values:
// Validate orgId is a valid positive integer const parsedOrgId = parseInt(orgId, 10) if (isNaN(parsedOrgId) || parsedOrgId <= 0) { throw responses.failureResponse({ message: 'INVALID_ORG_ID', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) } + + // Validate tenant exists + const tenant = await tenantQueries.findOne({ tenant_code: tenantCode }) + if (!tenant) { + throw responses.failureResponse({ + message: 'INVALID_TENANT', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + + // Validate organization exists and belongs to tenant + const organization = await organizationQueries.findOne({ + id: parsedOrgId, + code: orgCode, + }) + if (!organization) { + throw responses.failureResponse({ + message: 'INVALID_ORGANIZATION', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + + // Verify organization belongs to the specified tenant + if (organization.tenant_code !== tenantCode) { + throw responses.failureResponse({ + message: 'ORG_TENANT_MISMATCH', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } // Override the values from the token with sanitized header values decodedToken.data.tenant_code = tenantCode decodedToken.data.organization_id = parsedOrgId decodedToken.data.organization_code = orgCode }Note: You'll need to import the appropriate query modules and ensure error messages are added to the locale files.
Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/middlewares/authenticator.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.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/middlewares/authenticator.js
🧬 Code graph analysis (1)
src/middlewares/authenticator.js (2)
src/services/organization-feature.js (4)
isAdmin(36-36)common(14-14)orgCode(38-38)tenantCode(39-39)src/controllers/v1/organization-feature.js (4)
isAdmin(27-27)common(10-10)orgCode(68-68)tenantCode(69-69)
🔇 Additional comments (3)
src/middlewares/authenticator.js (3)
162-162: LGTM! Declaration supports admin override logic.Moving the
decodedTokendeclaration earlier enables the admin override mutations in the subsequent block while maintaining proper scope.
294-294: LGTM! Single assignment point for all users.The token assignment occurs once for all code paths (admin and non-admin), correctly resolving the previous redundancy issue.
219-226: Fix header name case handling.Lines 224-226 incorrectly call
.toLowerCase()on the environment variable values. HTTP header lookups in Express are already case-insensitive, so you should use the env variable values directly without modification.The current code will fail if an admin sets headers using the correct case (e.g.,
X-Tenant-Codewon't matchx-tenant-codeafter lowercasing the env variable).Apply this diff:
// Header names are configurable via environment variables with sensible defaults const orgIdHeaderName = process.env.ORG_ID_HEADER_NAME const orgCodeHeaderName = process.env.ORG_CODE_HEADER_NAME const tenantCodeHeaderName = process.env.TENANT_CODE_HEADER_NAME // Extract and sanitize header values (trim whitespace, case-insensitive header lookup) - const orgId = (req.headers[orgIdHeaderName.toLowerCase()] || '').trim() - const orgCode = (req.headers[orgCodeHeaderName.toLowerCase()] || '').trim() - const tenantCode = (req.headers[tenantCodeHeaderName.toLowerCase()] || '').trim() + const orgId = (req.headers[orgIdHeaderName] || '').trim() + const orgCode = (req.headers[orgCodeHeaderName] || '').trim() + const tenantCode = (req.headers[tenantCodeHeaderName] || '').trim()Note: Express's
req.headersalready handles case-insensitive lookups, so explicit lowercasing is unnecessary and breaks the functionality.⛔ Skipped due to learnings
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/controllers/v1/admin.js (1)
157-157: Same issue as Line 113.This has the same environment variable and case-sensitivity concerns as the header access on Line 113.
src/controllers/v1/public.js (1)
7-7: Same environment variable and case-sensitivity issues as in admin.js.This has identical concerns:
- If
process.env.TENANT_CODE_HEADER_NAMEis undefined,req?.headers?.[undefined]returnsundefinedinstead of the intended header- No case normalization (unlike
src/middlewares/authenticator.js:225which uses.toLowerCase())While the fallback to
getDomainFromRequestprovides resilience, relying on it due to an undefined environment variable would be an unintended code path.Apply the same fix as suggested for admin.js:
- let tenantCode = req?.headers?.[process.env.TENANT_CODE_HEADER_NAME] || null + let tenantCode = req?.headers?.[process.env.TENANT_CODE_HEADER_NAME?.toLowerCase()] || nullsrc/controllers/v1/form.js (1)
103-103: Same environment variable and case-sensitivity issues with good fallback structure.The fallback chain is well-designed (token → header → null), but shares the same concerns:
- Undefined
process.env.TENANT_CODE_HEADER_NAMEcauses silent failure- Missing case normalization
Apply the same fix:
- req?.decodedToken?.tenant_code || req?.headers?.[process.env.TENANT_CODE_HEADER_NAME] || null, + req?.decodedToken?.tenant_code || req?.headers?.[process.env.TENANT_CODE_HEADER_NAME?.toLowerCase()] || null,src/controllers/v1/tenant.js (1)
149-150: Same environment variable and case-sensitivity issues for both headers.Both lines share identical concerns:
- If
process.env.ORG_CODE_HEADER_NAMEorprocess.env.TENANT_CODE_HEADER_NAMEare undefined, the bracket notation accessesundefinedas a header name- No case normalization unlike the authenticator middleware
This is particularly important for bulk user creation operations where incorrect header resolution could affect multiple users.
Apply the same fix to both lines:
- req.headers?.[process.env.ORG_CODE_HEADER_NAME], - req.headers?.[process.env.TENANT_CODE_HEADER_NAME], + req.headers?.[process.env.ORG_CODE_HEADER_NAME?.toLowerCase()], + req.headers?.[process.env.TENANT_CODE_HEADER_NAME?.toLowerCase()],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/controllers/v1/admin.js(2 hunks)src/controllers/v1/form.js(1 hunks)src/controllers/v1/public.js(1 hunks)src/controllers/v1/tenant.js(1 hunks)src/envVariables.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/envVariables.js
🧰 Additional context used
📓 Path-based instructions (1)
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/form.jssrc/controllers/v1/admin.jssrc/controllers/v1/tenant.jssrc/controllers/v1/public.js
🧬 Code graph analysis (3)
src/controllers/v1/form.js (1)
src/database/models/index.js (1)
process(6-6)
src/controllers/v1/tenant.js (1)
src/database/models/index.js (1)
process(6-6)
src/controllers/v1/public.js (1)
src/middlewares/authenticator.js (1)
tenantCode(226-226)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/constants/common.js(1 hunks)src/controllers/v1/admin.js(2 hunks)src/controllers/v1/form.js(2 hunks)src/controllers/v1/public.js(1 hunks)src/controllers/v1/tenant.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/controllers/v1/form.js
- src/controllers/v1/admin.js
- src/controllers/v1/tenant.js
- src/controllers/v1/public.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/constants/common.js (1)
src/database/models/index.js (1)
process(6-6)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/middlewares/authenticator.js (1)
217-276: Approve admin override logic with robust validation.
- Verify tenant active status: code enforces
organization.status === ACTIVEbut does not validate the tenant’s status; confirm this is intentional or implement a tenant-status check.- Optional: Add audit logging after the override (e.g., at line 274) to record
admin_id,override_org_id,override_org_code,override_tenant_codeand a timestamp:logger.info('Admin override', { admin_id: decodedToken.data.id, override_org_id: parsedOrgId, override_org_code: orgCode, override_tenant_code: tenantCode, timestamp: new Date().toISOString() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/locales/en.json(1 hunks)src/middlewares/authenticator.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.js
🧠 Learnings (2)
📚 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/middlewares/authenticator.js
📚 Learning: 2025-08-19T07:40:30.106Z
Learnt from: MallanagoudaB
PR: ELEVATE-Project/user#800
File: src/envVariables.js:413-417
Timestamp: 2025-08-19T07:40:30.106Z
Learning: In the ELEVATE-Project/user service, environment variables are consistently marked as `optional: false` even when default values are provided, as this is their established pattern across the service.
Applied to files:
src/middlewares/authenticator.js
🧬 Code graph analysis (1)
src/middlewares/authenticator.js (2)
src/helpers/userInvite.js (2)
organizationQueries(15-15)orgCode(1081-1089)src/controllers/v1/public.js (1)
tenantCode(8-8)
🔇 Additional comments (3)
src/locales/en.json (1)
185-188: LGTM!The new locale keys are well-defined and align correctly with the middleware implementation. The interpolation placeholders in
ADD_ORG_HEADERmatch the parameters passed from the middleware, and the error messages are appropriately generic to avoid information disclosure while remaining helpful to admin users.src/middlewares/authenticator.js (2)
13-13: LGTM!The import change from
roleQueriestoorganizationQueriesis necessary for the new admin override validation logic. The organizational query at lines 256-262 properly validates the organization-tenant relationship.Also applies to: 21-21
163-163: LGTM!The early declaration of
decodedTokenat line 163 is necessary for the admin override logic to modifydecodedToken.datain place (lines 272-274). This structural change aligns well with the new admin override flow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/middlewares/authenticator.js (1)
214-214: LGTM!The strict equality check correctly prevents type coercion in the admin role validation, as addressed in previous review.
🧹 Nitpick comments (1)
src/middlewares/authenticator.js (1)
217-265: Consider extracting to helper function.The admin override block spans ~48 lines within the middleware. Extracting it to a separate helper function (e.g.,
applyAdminOverride(decodedToken, req)) would improve readability and make the logic easier to unit test in isolation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/locales/en.json(1 hunks)src/middlewares/authenticator.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/locales/en.json
🧰 Additional context used
📓 Path-based instructions (1)
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.js
🧠 Learnings (2)
📚 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/middlewares/authenticator.js
📚 Learning: 2025-08-19T07:40:30.106Z
Learnt from: MallanagoudaB
PR: ELEVATE-Project/user#800
File: src/envVariables.js:413-417
Timestamp: 2025-08-19T07:40:30.106Z
Learning: In the ELEVATE-Project/user service, environment variables are consistently marked as `optional: false` even when default values are provided, as this is their established pattern across the service.
Applied to files:
src/middlewares/authenticator.js
🧬 Code graph analysis (1)
src/middlewares/authenticator.js (5)
src/services/organization.js (5)
organizationQueries(3-3)role(72-77)role(373-373)common(2-2)tenantCode(281-281)src/services/organization-feature.js (6)
utilsHelper(12-12)organizationQueries(10-10)isAdmin(36-36)common(14-14)orgCode(38-38)tenantCode(39-39)src/services/admin.js (5)
organizationQueries(20-20)role(152-158)role(548-551)common(15-15)org(655-655)src/helpers/userInvite.js (2)
organizationQueries(15-15)orgCode(1081-1089)src/controllers/v1/public.js (1)
tenantCode(8-8)
🔇 Additional comments (4)
src/middlewares/authenticator.js (4)
21-21: LGTM!The import of
organizationQueriesis correctly added to support the admin override validation logic.
163-164: LGTM!Moving the
decodedTokendeclaration earlier is necessary to support the admin override logic that modifies the token data before the final assignment at line 298.
228-242: AI summary inconsistency about header count.The AI summary states "all three must be present" but the implementation correctly requires only two headers (orgCode and tenantCode). The organization ID is derived from the database query rather than from a header, which is the more secure approach.
217-265: LGTM! Admin override implementation is secure.The admin override logic correctly:
- Validates admin role with strict equality
- Requires both headers if any override is attempted
- Queries the database to validate the org/tenant relationship
- Uses the database org.id instead of accepting it from headers
- Checks for active status and soft-delete flag
- Provides appropriate error messages
The implementation addresses all security concerns raised in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/middlewares/authenticator.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.js
🧠 Learnings (2)
📚 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/middlewares/authenticator.js
📚 Learning: 2025-08-19T07:40:30.106Z
Learnt from: MallanagoudaB
PR: ELEVATE-Project/user#800
File: src/envVariables.js:413-417
Timestamp: 2025-08-19T07:40:30.106Z
Learning: In the ELEVATE-Project/user service, environment variables are consistently marked as `optional: false` even when default values are provided, as this is their established pattern across the service.
Applied to files:
src/middlewares/authenticator.js
🧬 Code graph analysis (1)
src/middlewares/authenticator.js (5)
src/services/organization.js (10)
require(12-12)require(14-14)require(15-15)require(18-18)require(22-22)organizationQueries(3-3)common(2-2)role(72-77)role(373-373)tenantCode(281-281)src/services/admin.js (5)
organizationQueries(20-20)common(15-15)role(152-158)role(548-551)org(655-655)src/controllers/v1/organization-feature.js (5)
utilsHelper(13-13)common(10-10)isAdmin(27-27)orgCode(68-68)tenantCode(69-69)src/helpers/userInvite.js (2)
organizationQueries(15-15)orgCode(1081-1089)src/controllers/v1/public.js (3)
domain(7-7)domain(25-25)tenantCode(8-8)
Summary by CodeRabbit
New Features
Bug Fixes
Chores