Add tenant-aware admin login with multi-identifier and session limits #795
Conversation
WalkthroughReplaces the admin login with a tenant-aware multi-identifier flow (email/phone/username), adds bcrypt password verification, admin-role and session-limit checks, session creation and Redis sync, richer token payloads, and sanitization. Also updates registration email template usage, domain creation payload, and refresh_token assignment logic. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as AdminService (src/services/admin.js)
participant DB as Database
participant B as bcryptjs
participant S as SessionService / Redis
participant T as TokenService
C->>A: login(identifier, password, deviceInfo)
A->>A: Normalize identifier -> build tenant-scoped query
A->>DB: Query user + organizations + roles
DB-->>A: User record + orgs/roles
A->>B: Verify password
B-->>A: Match / Fail
A->>A: Verify admin role
A->>S: Check/enforce active-session limit
A->>S: Create session
S-->>A: Session details
A->>A: Build enriched token payload (org ids/codes, organizations, tenant_code)
A->>T: Generate access & refresh tokens
T-->>A: Tokens
A->>A: Sanitize user, resolve image URL, attach identifier
A->>S: Persist session data in Redis
S-->>A: Stored
A-->>C: Return { tokens, user payload }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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
changed the title
![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 (3)
src/services/admin.js (3)
281-287: Consider extracting the failure helper as a module-level functionThe
failurehelper function is defined inside theloginmethod, but it could be reused across other methods in this class. Consider extracting it as a private static method or a module-level utility to improve code reusability and maintainability.+ /** + * Helper for consistent failure responses + * @private + */ + static #failure(message, status = httpStatusCode.bad_request) { + return responses.failureResponse({ + message, + statusCode: status, + responseCode: 'CLIENT_ERROR', + }) + } + static async login(bodyData, deviceInformation) { try { - // helper for consistent failure responses - const failure = (message, status = httpStatusCode.bad_request) => - responses.failureResponse({ - message, - statusCode: status, - responseCode: 'CLIENT_ERROR', - })Then update all
failure()calls toAdminHelper.#failure().
294-297: Consider using established validation libraries for identifier type checkingThe regex patterns for email, phone, and username validation are defined inline. These patterns may not cover all edge cases and international formats. Consider using well-tested validation libraries like
validator.jsfor more robust validation.Example with validator.js:
const validator = require('validator') // Email validation const isEmail = (str) => validator.isEmail(str) // Phone validation with international format support const isPhone = (str) => validator.isMobilePhone(str, 'any') // Username validation can remain custom or use validator's isAlphanumeric with length check const isUsername = (str) => validator.isAlphanumeric(str, 'en-US', { ignore: '_' }) && str.length >= 3 && str.length <= 30
408-409: Improve error logging for productionUsing
console.logfor error logging in production is not ideal. Consider using a proper logging framework with appropriate log levels and structured logging.} catch (error) { - console.log(error) + // Use a proper logger instead of console.log + // logger.error('Login failed', { error: error.message, stack: error.stack }) throw error }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/admin.js(3 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/admin.js
🔇 Additional comments (5)
src/services/admin.js (5)
325-329: Good use of bcrypt for secure password comparisonThe implementation correctly uses bcrypt's async
comparemethod for password verification, which is secure and prevents timing attacks. The error message appropriately doesn't reveal whether the identifier or password was incorrect.
331-338: Admin role check logic looks secureThe nested check for admin role properly validates through the user's organizations and their associated roles, ensuring only users with the
ADMIN_ROLEtitle can proceed. The generic error message maintains security by not revealing the specific reason for failure.
386-389: Good security practice: removing sensitive dataThe code properly removes the password field before returning the user object and handles the image URL conversion for downloadable links. This is a good security practice.
392-392: Consider sanitizing the identifier before returningThe original identifier is being returned directly to the user. While this is likely safe since it was already validated, consider if any additional sanitization is needed for security purposes.
Is there any risk in returning the original identifier without additional sanitization? The identifier has already been validated against specific patterns, but double-check if there are any XSS or injection concerns.
366-369: Ensureuser.organizationsIs Always DefinedThe mappings here assume
user.organizationsis a non-null array. Without a guaranteed default, calling.mapmay throw at runtime.• Location:
src/services/admin.js(lines 366–369)
• Add a fallback (e.g.user.organizations = user.organizations || []) in your transform/DTO or API parsing layer.
• If you have aUserTransformDTO.transform(or equivalent) method, ensure it initializesorganizationsto an empty array when absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/services/organization.js (1)
215-221: Mirror tenant_code and updated_by in the update flow to avoid inconsistent domain rowsWhen adding domains during update(), new rows currently lack tenant_code and updated_by. This creates inconsistent domain records versus the create() path and can break tenant scoping or audit assumptions downstream.
Apply this diff to include these fields (orgDetailsBeforeUpdate is already available in scope):
- domains.map(async (domain) => { - let domainCreationData = { - domain: domain, - organization_id: id, - created_by: loggedInUserId, - } - await orgDomainQueries.create(domainCreationData) - }) + domains.map(async (domain) => { + const domainCreationData = { + domain: domain, + organization_id: id, + created_by: loggedInUserId, + updated_by: loggedInUserId, + tenant_code: orgDetailsBeforeUpdate?.tenant_code, + } + await orgDomainQueries.create(domainCreationData) + })
🧹 Nitpick comments (3)
src/services/organization.js (1)
54-65: Normalize and dedupe input domains to prevent case/whitespace duplicatesWhile create() assumes a new organization (so duplicates are unlikely), it can still receive duplicate or mixed-case domains. Consider lowercasing and trimming domains and deduping before insert to avoid unique constraint violations or duplicate rows.
Example approach within this block (pseudocode):
- Transform bodyData.domains into a set of normalized domains: domainsNormalized = [...new Set(bodyData.domains.map(d => d.trim().toLowerCase()))]
- Iterate over domainsNormalized for creation.
src/services/user-sessions.js (2)
21-28: Document tenantCode in the JSDoc for createUserSessionThe signature includes tenantCode, but the parameter is missing in the JSDoc. Add it to keep docs aligned with the API.
/** * Create a user session. * @param {number} userId - The ID of the user. * @param {string} [refreshToken=''] - Optional. The refresh token associated with the session. * @param {string} [accessToken=''] - Optional. The access token associated with the session. * @param {Object} deviceInfo - Information about the device used for the session. + * @param {string} tenantCode - Tenant code associated with the session. * @returns {Promise<Object>} - A promise that resolves to a success response with the created session details. * @throws {Error} - Throws an error if any issue occurs during the process. */
30-46: Optionally mirror the trimming check for accessToken for consistencyIf you adopt trimming for refresh_token, consider the same for access token to avoid persisting whitespace-only tokens.
Minimal change:
- if (accessToken !== '') { - userSessionDetails.token = accessToken - } + if (typeof accessToken === 'string' && accessToken.trim() !== '') { + userSessionDetails.token = accessToken.trim() + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/services/account.js(0 hunks)src/services/organization.js(1 hunks)src/services/user-sessions.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/services/account.js
🧰 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.jssrc/services/user-sessions.js
🧬 Code Graph Analysis (2)
src/services/organization.js (1)
src/services/org-admin.js (1)
bodyData(435-435)
src/services/user-sessions.js (2)
src/services/admin.js (1)
refreshToken(379-383)src/services/account.js (3)
refreshToken(527-531)refreshToken(772-776)refreshToken(1456-1460)
🔇 Additional comments (2)
src/services/organization.js (1)
59-61: Adds audit and tenant context for domain provisioning — good changeIncluding updated_by and tenant_code in domainCreationData improves auditability and ensures domain rows are tenant-scoped at insert time. This aligns well with the tenant-aware login/session work.
src/services/user-sessions.js (1)
30-46: No null/undefined passed as refreshToken at call sites
A repository-wide search confirms everycreateUserSessioninvocation supplies an empty string ('') rather thannullorundefined. No further changes are needed here.
| if (refreshToken !== '') { | ||
| userSessionDetails.refresh_token = refreshToken | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null/whitespace refresh tokens before persisting
The new check prevents empty strings, but null/undefined or whitespace strings could still slip through from some call sites. Persist only non-empty, trimmed strings to keep DB clean and consistent.
Apply this diff:
- if (refreshToken !== '') {
- userSessionDetails.refresh_token = refreshToken
- }
+ if (typeof refreshToken === 'string' && refreshToken.trim() !== '') {
+ userSessionDetails.refresh_token = refreshToken.trim()
+ }📝 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.
| if (refreshToken !== '') { | |
| userSessionDetails.refresh_token = refreshToken | |
| } | |
| if (typeof refreshToken === 'string' && refreshToken.trim() !== '') { | |
| userSessionDetails.refresh_token = refreshToken.trim() | |
| } |
🤖 Prompt for AI Agents
In src/services/user-sessions.js around lines 44 to 46, the current check only
blocks the empty string but allows null/undefined or whitespace-only tokens;
update the guard to first verify refreshToken is not null/undefined, trim it,
and persist only if the trimmed value has length > 0 (e.g., const token = typeof
refreshToken === 'string' ? refreshToken.trim() : ''; if (token)
userSessionDetails.refresh_token = token). This ensures only non-empty, trimmed
strings are saved.
Summary by CodeRabbit
New Features
Improvements
Changes
Documentation