Changes done to handle login with otp and auto registration flow based on configuration#888
Changes done to handle login with otp and auto registration flow based on configuration#888Vinod-V3 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds tenant auth configuration defaults and persistence, exposes tenant configuration through tenant APIs, updates account validation and OTP handling for email, phone, and username flows, and adds a migration for ChangesTenant auth configuration and account flow refactor
Entity type backfill migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountService
participant TenantDB
participant UserDB
participant Redis
Client->>AccountService: registrationOtp / login / create
AccountService->>TenantDB: read tenant.configuration.allowed_auth_mode
TenantDB-->>AccountService: configuration
AccountService->>UserDB: lookup user by email / phone / username
UserDB-->>AccountService: user or null
AccountService->>Redis: get / set purpose-qualified OTP
Redis-->>AccountService: OTP payload
AccountService->>AccountService: enforce auth mode and identifier rules
AccountService->>Redis: delete used OTP key
AccountService-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/validators/v1/tenant.js (1)
56-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate path still allows unvalidated
configurationpayloads.In the
req.params.idbranch (Line 58 onward),configurationis normalized but not validated withisValidTenantConfiguration(same fortheming/meta). Invalid objects can pass through update and be persisted.✅ Suggested fix (mirror create-path checks in update branch)
if (req.params.id) { req.body = filterRequestBody(req.body, tenant.update) normalizeTenantConfiguration(req) req.checkParams('id') .trim() .notEmpty() .withMessage('code param is empty') .matches(/^[a-zA-Z0-9_]+$/) .withMessage('Code must contain only letters, numbers, and underscores') + + req.checkBody('theming') + .optional({ checkFalsy: true }) + .custom(isValidObject) + .withMessage('Theming must be a valid object or a JSON string representing an object') + + req.checkBody('configuration') + .optional({ checkFalsy: true }) + .custom(isValidTenantConfiguration) + .withMessage('Configuration must include allowed_auth_mode, and auto_register') + + req.checkBody('meta') + .optional({ checkFalsy: true }) + .custom(isValidObject) + .withMessage('Meta must be a valid object or a JSON string representing an object') } else {As per path instructions,
src/validators/**: "Validate all incoming data thoroughly. Check for missing or incomplete validation 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 56 - 69, The update method is missing validation checks for configuration, theming, and meta fields in the req.params.id branch after normalizeTenantConfiguration is called. Add the same validation checks that are present in the create branch (mirror the isValidTenantConfiguration and related validations) to the update branch, ensuring that configuration, theming, and meta objects are validated using req.checkBody before they can be persisted. These validation checks should be added after the normalizeTenantConfiguration call but before the update operation completes.Source: Path instructions
🤖 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/migrations/20260621090000-make-user-name-and-password-nullable.js`:
- Around line 21-35: The down migration function attempts to restore allowNull:
false constraints on the name column in users table, password column in users
table, and password column in users_credentials table, but does not handle null
values that may have been created while the up migration was active. Before
calling changeColumn to set allowNull: false for each of these columns, add
update statements in the down function to replace any existing null values with
a safe default value such as an empty string. This ensures the rollback can
execute successfully without constraint violations.
In `@src/services/account.js`:
- Around line 1440-1444: The current check using includes('OK') on
redisSetResults only verifies that at least one Redis write succeeded, but in
multi-identifier flows you need to ensure ALL OTP writes completed successfully.
Replace the includes('OK') check in the redisSetResults validation with a check
that verifies every element in the redisSetResults array is 'OK', such as using
the every() method, to guarantee all OTP values were stored in Redis before
returning success.
- Around line 1335-1456: In the code section that sends SMS notification,
replace the condition that checks `bodyData.phone_code` with a check against the
`phoneCode` variable instead. The `phoneCode` variable is properly hydrated from
the user record in the username branch of the conditional, so using it ensures
SMS notifications are sent when a user has a phone number on file, even if the
request body doesn't contain phone_code. This fix applies to the SMS delivery
gate logic that mirrors the email notification check shown in the diff.
In `@src/validators/v1/account.js`:
- Around line 146-159: Both the password and otp fields in the account validator
are independently optional, which allows requests with neither credential to
pass validation. Add a cross-field validation check using a custom validator
that runs after the individual field validations to ensure at least one of
password or otp is provided in the request body. This should be done at the
request level to reject payloads that contain neither credential during
validation time rather than failing later in the service layer.
---
Outside diff comments:
In `@src/validators/v1/tenant.js`:
- Around line 56-69: The update method is missing validation checks for
configuration, theming, and meta fields in the req.params.id branch after
normalizeTenantConfiguration is called. Add the same validation checks that are
present in the create branch (mirror the isValidTenantConfiguration and related
validations) to the update branch, ensuring that configuration, theming, and
meta objects are validated using req.checkBody before they can be persisted.
These validation checks should be added after the normalizeTenantConfiguration
call but before the update operation completes.
🪄 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: 7d446886-5465-44cd-bfb0-18d496de229d
📒 Files selected for processing (12)
src/constants/common.jssrc/database/migrations/20260618090000-add-configuration-to-tenants.jssrc/database/migrations/20260621090000-make-user-name-and-password-nullable.jssrc/database/models/Tenant.jssrc/database/models/users.jssrc/dtos/tenantDTO.jssrc/locales/en.jsonsrc/locales/hi.jsonsrc/services/account.jssrc/services/tenant.jssrc/validators/v1/account.jssrc/validators/v1/tenant.js
| let phoneCode = bodyData.phone_code | ||
| let notificationName = bodyData.name | ||
| let redisIdentifiers = [] | ||
| let purpose = OTP_PURPOSES.SIGNUP | ||
|
|
||
| if (criteria.length === 0) { | ||
| return // Skip if no criteria | ||
| } | ||
| if (username) { | ||
| const user = await userQueries.findOne( | ||
| { | ||
| username, | ||
| tenant_code: tenantDetail.code, | ||
| status: common.ACTIVE_STATUS, | ||
| }, | ||
| { | ||
| attributes: ['email', 'phone', 'phone_code', 'name', 'username'], | ||
| } | ||
| ) | ||
|
|
||
| // Check if user already exists with email or phone or username | ||
| let user = await userQueries.findOne( | ||
| { | ||
| [Op.or]: criteria, | ||
| password: { [Op.ne]: null }, | ||
| tenant_code: tenantDetail.code, | ||
| }, | ||
| { | ||
| attributes: ['id'], | ||
| if (!user) { | ||
| return responses.failureResponse({ | ||
| message: 'ACCOUNT_NOT_FOUND', | ||
| statusCode: httpStatusCode.not_found, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
| ) | ||
|
|
||
| // Return error if user already exists | ||
| if (user) { | ||
| return responses.failureResponse({ | ||
| message: 'USER_ALREADY_EXISTS', | ||
| statusCode: httpStatusCode.not_acceptable, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
|
|
||
| // Generate or retrieve OTP | ||
| let emailUserData = encryptedEmailId ? await utilsHelper.redisGet(encryptedEmailId) : null | ||
| let phoneUserData = | ||
| encryptedPhoneNumber && bodyData.phone_code | ||
| ? await utilsHelper.redisGet(bodyData.phone_code + encryptedPhoneNumber) | ||
| : null | ||
| purpose = OTP_PURPOSES.LOGIN | ||
|
|
||
| // Use existing OTP if already in progress, otherwise generate a new one | ||
| const userData = emailUserData || phoneUserData | ||
| // Usage: [generateSecureOTP(), true] | ||
| if (!allowedAuthMode.includes(AUTH_MODES.OTP)) { | ||
| return failureResponse('AUTH_MODE_NOT_ALLOWED') | ||
| } | ||
|
|
||
| const [otp, isNew] = | ||
| userData && userData.action === 'signup' ? [userData.otp, false] : [utils.generateSecureOTP(), true] | ||
| encryptedEmailId = user.email || null | ||
| encryptedPhoneNumber = user.phone || null | ||
| phoneCode = user.phone_code || null | ||
| plaintextEmailId = encryptedEmailId ? emailEncryption.decrypt(encryptedEmailId) : null | ||
| plaintextPhoneNumber = encryptedPhoneNumber ? emailEncryption.decrypt(encryptedPhoneNumber) : null | ||
| notificationName = bodyData.name || user.name || 'User' | ||
|
|
||
| // Store OTP data in redis if it's new | ||
| if (isNew) { | ||
| const redisData = { | ||
| verify: encryptedEmailId || encryptedPhoneNumber, | ||
| action: 'signup', | ||
| otp, | ||
| if (!plaintextEmailId && !plaintextPhoneNumber) { | ||
| return failureResponse('USER_CONTACT_NOT_AVAILABLE') | ||
| } | ||
|
|
||
| let redisSetResults = [] | ||
|
|
||
| // Store OTP for email if provided | ||
| if (encryptedEmailId) { | ||
| const emailResult = await utilsHelper.redisSet(encryptedEmailId, redisData, common.otpExpirationTime) | ||
| redisSetResults.push(emailResult) | ||
| redisIdentifiers = [username] | ||
| if (encryptedEmailId) redisIdentifiers.push(encryptedEmailId) | ||
| if (encryptedPhoneNumber && phoneCode) redisIdentifiers.push(phoneCode + encryptedPhoneNumber) | ||
| } else { | ||
| if (bodyData.email) { | ||
| plaintextEmailId = bodyData.email.toLowerCase() | ||
| encryptedEmailId = emailEncryption.encrypt(plaintextEmailId) | ||
| } | ||
|
|
||
| // Store OTP for phone if provided | ||
| if (encryptedPhoneNumber && bodyData.phone_code) { | ||
| const phoneResult = await utilsHelper.redisSet( | ||
| bodyData.phone_code + encryptedPhoneNumber, | ||
| redisData, | ||
| common.otpExpirationTime | ||
| ) | ||
| redisSetResults.push(phoneResult) | ||
| if (bodyData.phone && bodyData.phone_code) { | ||
| plaintextPhoneNumber = bodyData.phone | ||
| encryptedPhoneNumber = emailEncryption.encrypt(plaintextPhoneNumber) | ||
| } | ||
|
|
||
| // Check if storing in Redis was successful | ||
| if (!redisSetResults.includes('OK')) { | ||
| const criteria = [] | ||
| if (encryptedEmailId) criteria.push({ email: encryptedEmailId }) | ||
| if (encryptedPhoneNumber) criteria.push({ phone: encryptedPhoneNumber }) | ||
|
|
||
| const existingUser = | ||
| criteria.length > 0 | ||
| ? await userQueries.findOne( | ||
| { | ||
| [Op.or]: criteria, | ||
| tenant_code: tenantDetail.code, | ||
| }, | ||
| { attributes: ['id'] } | ||
| ) | ||
| : null | ||
|
|
||
| const userExists = Boolean(existingUser) | ||
| purpose = resolveOtpPurpose({ | ||
| username: null, | ||
| registrationCode: bodyData.registration_code, | ||
| userExists, | ||
| }) | ||
|
|
||
| if (bodyData.registration_code && userExists) { | ||
| return responses.failureResponse({ | ||
| message: 'UNABLE_TO_SEND_OTP', | ||
| statusCode: httpStatusCode.internal_server_error, | ||
| responseCode: 'SERVER_ERROR', | ||
| message: 'USER_ALREADY_EXISTS', | ||
| statusCode: httpStatusCode.not_acceptable, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
|
|
||
| if (purpose === OTP_PURPOSES.LOGIN && !allowedAuthMode.includes(AUTH_MODES.OTP)) { | ||
| return failureResponse('AUTH_MODE_NOT_ALLOWED') | ||
| } | ||
|
|
||
| if (encryptedEmailId) redisIdentifiers.push(encryptedEmailId) | ||
| if (encryptedPhoneNumber && phoneCode) redisIdentifiers.push(phoneCode + encryptedPhoneNumber) | ||
| } | ||
|
|
||
| const existingOtpData = await getExistingOtpForPurpose(purpose, redisIdentifiers) | ||
|
|
||
| const otp = existingOtpData ? existingOtpData.otp : utils.generateSecureOTP() | ||
| const isNew = !existingOtpData | ||
|
|
||
| const redisData = { | ||
| verify: username || encryptedEmailId || encryptedPhoneNumber, | ||
| action: purpose, | ||
| otp, | ||
| } | ||
|
|
||
| const redisSetResults = await setOtpForPurpose(purpose, redisIdentifiers, redisData) | ||
|
|
||
| // Check if storing in Redis was successful | ||
| if (!redisSetResults.includes('OK')) { | ||
| return responses.failureResponse({ | ||
| message: 'UNABLE_TO_SEND_OTP', | ||
| statusCode: httpStatusCode.internal_server_error, | ||
| responseCode: 'SERVER_ERROR', | ||
| }) | ||
| } | ||
|
|
||
| // Send email notification with OTP if email is provided | ||
| if (plaintextEmailId) { | ||
| notificationUtils.sendEmailNotification({ | ||
| emailId: plaintextEmailId, | ||
| templateCode: process.env.REGISTRATION_OTP_EMAIL_TEMPLATE_CODE, | ||
| variables: { name: bodyData.name || plaintextEmailId, otp }, | ||
| variables: { name: notificationName || plaintextEmailId, otp }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use phoneCode (not bodyData.phone_code) when deciding SMS delivery in username OTP flow.
In the username branch you hydrate phoneCode from the user record, but Line 1463 still gates SMS on bodyData.phone_code. Username-only requests usually don’t include that field, so OTP can be generated/stored without being delivered via SMS.
Suggested patch
- if (plaintextPhoneNumber && bodyData.phone_code) {
+ if (plaintextPhoneNumber && phoneCode) {
notificationUtils.sendSMSNotification({
phoneNumber: plaintextPhoneNumber,
templateCode: process.env.REGISTRATION_OTP_EMAIL_TEMPLATE_CODE,
variables: { app_name: tenantDetail.name, otp },
tenantCode: tenantDetail.code,
organization_code: process.env.DEFAULT_ORGANISATION_CODE || null,
})
}As per path instructions, src/services/**: This is core business logic. Please check for correctness, efficiency, and potential edge cases.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 1439-1439: Avoid using the initial state variable in setState
Context: setOtpForPurpose(purpose, redisIdentifiers, redisData)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 1335 - 1456, In the code section that
sends SMS notification, replace the condition that checks `bodyData.phone_code`
with a check against the `phoneCode` variable instead. The `phoneCode` variable
is properly hydrated from the user record in the username branch of the
conditional, so using it ensures SMS notifications are sent when a user has a
phone number on file, even if the request body doesn't contain phone_code. This
fix applies to the SMS delivery gate logic that mirrors the email notification
check shown in the diff.
Source: Path instructions
| const redisSetResults = await setOtpForPurpose(purpose, redisIdentifiers, redisData) | ||
|
|
||
| // Check if storing in Redis was successful | ||
| if (!redisSetResults.includes('OK')) { | ||
| return responses.failureResponse({ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require all OTP Redis writes to succeed before returning success.
includes('OK') passes even when only one key write succeeds. For multi-identifier flows, that can acknowledge OTP delivery while one login path has no stored OTP.
Suggested patch
const redisSetResults = await setOtpForPurpose(purpose, redisIdentifiers, redisData)
// Check if storing in Redis was successful
- if (!redisSetResults.includes('OK')) {
+ const allRedisWritesSucceeded =
+ redisIdentifiers.length > 0 &&
+ redisSetResults.length === redisIdentifiers.length &&
+ redisSetResults.every((status) => status === 'OK')
+ if (!allRedisWritesSucceeded) {
return responses.failureResponse({
message: 'UNABLE_TO_SEND_OTP',
statusCode: httpStatusCode.internal_server_error,
responseCode: 'SERVER_ERROR',
})
}As per path instructions, src/services/**: This is core business logic. Please check 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.
| const redisSetResults = await setOtpForPurpose(purpose, redisIdentifiers, redisData) | |
| // Check if storing in Redis was successful | |
| if (!redisSetResults.includes('OK')) { | |
| return responses.failureResponse({ | |
| const redisSetResults = await setOtpForPurpose(purpose, redisIdentifiers, redisData) | |
| // Check if storing in Redis was successful | |
| const allRedisWritesSucceeded = | |
| redisIdentifiers.length > 0 && | |
| redisSetResults.length === redisIdentifiers.length && | |
| redisSetResults.every((status) => status === 'OK') | |
| if (!allRedisWritesSucceeded) { | |
| return responses.failureResponse({ |
🤖 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 1440 - 1444, The current check using
includes('OK') on redisSetResults only verifies that at least one Redis write
succeeded, but in multi-identifier flows you need to ensure ALL OTP writes
completed successfully. Replace the includes('OK') check in the redisSetResults
validation with a check that verifies every element in the redisSetResults array
is 'OK', such as using the every() method, to guarantee all OTP values were
stored in Redis before returning success.
Source: Path instructions
| req.checkBody('password') | ||
| .optional({ checkFalsy: true }) | ||
| .trim() | ||
| .matches(process.env.PASSWORD_POLICY_REGEX) | ||
| .withMessage(process.env.PASSWORD_POLICY_MESSAGE) | ||
| .custom((value) => !/\s/.test(value)) | ||
| .withMessage('Password cannot contain spaces') | ||
|
|
||
| req.checkBody('otp') | ||
| .optional({ checkFalsy: true }) | ||
| .matches(/^[0-9]+$/) | ||
| .withMessage('otp should be number') | ||
| .isLength({ min: 6, max: 6 }) | ||
| .withMessage('Otp is invalid') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a cross-field check so login rejects requests with neither password nor OTP at validation time.
Right now both fields are optional, so payloads with no credential pass validation and fail later in service. Add a validator-level guard for completeness.
Suggested patch
req.checkBody('otp')
.optional({ checkFalsy: true })
.matches(/^[0-9]+$/)
.withMessage('otp should be number')
.isLength({ min: 6, max: 6 })
.withMessage('Otp is invalid')
+
+ req.checkBody(['password', 'otp']).custom(() => {
+ if (!req.body.password && !req.body.otp) {
+ throw new Error('Password or OTP is required')
+ }
+ return true
+ })As per path instructions, src/validators/**: Validate all incoming data thoroughly. Check for missing or incomplete validation 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 146 - 159, Both the password and
otp fields in the account validator are independently optional, which allows
requests with neither credential to pass validation. Add a cross-field
validation check using a custom validator that runs after the individual field
validations to ensure at least one of password or otp is provided in the request
body. This should be done at the request level to reject payloads that contain
neither credential during validation time rather than failing later in the
service layer.
Source: Path instructions
| allowed_auth_mode: ['otp', 'password'], | ||
| auto_register: false, |
There was a problem hiding this comment.
Can we take these from the env? We can store these in the envVariabels.js
DEFAULT_TENANT_CONFIGURATION: {
allowed_auth_mode: process.env.DEFAULT_ALLOWED_AUTH_MODES
? process.env.DEFAULT_ALLOWED_AUTH_MODES.split(',')
: ['otp', 'password'],
auto_register: process.env.DEFAULT_AUTO_REGISTER === 'true' || false,
},| created_at: tenantCreateResponse?.created_at || new Date(), | ||
| updated_at: tenantCreateResponse?.updated_at || new Date(), | ||
| meta: tenantCreateResponse?.meta || {}, | ||
| configuration: tenantCreateResponse?.configuration || common.DEFAULT_TENANT_CONFIGURATION, |
There was a problem hiding this comment.
Can we remove|| common.DEFAULT_TENANT_CONFIGURATION, we should only return back what is in the database, and since we are handling online no. 54, this is redundant.
| const parseObject = (value) => { | ||
| if (typeof value === 'string') { | ||
| try { | ||
| return JSON.parse(value) | ||
| } catch (e) { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return value | ||
| } | ||
|
|
||
| const isPlainObject = (value) => { | ||
| return typeof value === 'object' && value !== null && !Array.isArray(value) | ||
| } | ||
|
|
||
| const isValidObject = (value) => { | ||
| return isPlainObject(parseObject(value)) | ||
| } | ||
|
|
||
| const isValidTenantConfiguration = (value) => { | ||
| const configuration = parseObject(value) | ||
| const allowedAuthModes = common.DEFAULT_TENANT_CONFIGURATION.allowed_auth_mode | ||
|
|
||
| return ( | ||
| isPlainObject(configuration) && | ||
| Array.isArray(configuration.allowed_auth_mode) && | ||
| configuration.allowed_auth_mode.length > 0 && | ||
| configuration.allowed_auth_mode.every((authMode) => allowedAuthModes.includes(authMode)) && | ||
| typeof configuration.auto_register === 'boolean' | ||
| ) | ||
| } | ||
|
|
||
| const normalizeTenantConfiguration = (req) => { | ||
| if (req.body.configuration === '') { | ||
| delete req.body.configuration | ||
| return | ||
| } | ||
|
|
||
| if (typeof req.body.configuration === 'string') { | ||
| const configuration = parseObject(req.body.configuration) | ||
| if (configuration) req.body.configuration = configuration | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Great refactoring! The deduplication of the object validation logic is a solid improvement.
One small suggestion: a few of the extracted functions might be doing more than necessary. Specifically:
isPlainObject()is only used internally as a helper—could be inlined intoisValidObject()for simplicityisValidTenantConfiguration()is only used once for the configuration field. Since there's no duplication to eliminate here, extracting it adds a layer of indirection without much benefit
I'd suggest keeping just parseObject() and isValidObject() as extracted functions (they eliminate real duplication), and keeping the configuration validation logic inline:
req.checkBody('configuration')
.optional({ checkFalsy: true })
.custom((value) => {
const configuration = parseObject(value)
const allowedAuthModes = common.DEFAULT_TENANT_CONFIGURATION.allowed_auth_mode
return (
isValidObject(configuration) &&
Array.isArray(configuration.allowed_auth_mode) &&
configuration.allowed_auth_mode.length > 0 &&
configuration.allowed_auth_mode.every((authMode) => allowedAuthModes.includes(authMode)) &&
typeof configuration.auto_register === 'boolean'
)
})This keeps the helper extraction minimal while still removing the duplication. Otherwise looks good! 👍
There was a problem hiding this comment.
This change might not be required
| const AUTH_MODES = common.AUTH_MODES | ||
| const OTP_PURPOSES = common.OTP_PURPOSES | ||
|
|
||
| function tenantConfiguration(tenantDetail) { | ||
| return { | ||
| ...common.DEFAULT_TENANT_CONFIGURATION, | ||
| ...(tenantDetail?.configuration || {}), | ||
| } | ||
| } | ||
|
|
||
| function failureResponse(message, statusCode = httpStatusCode.bad_request, responseCode = 'CLIENT_ERROR') { | ||
| return responses.failureResponse({ | ||
| message, | ||
| statusCode, | ||
| responseCode, | ||
| }) | ||
| } | ||
|
|
||
| function identifierType(identifier) { | ||
| if (/^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/.test(identifier)) return 'email' | ||
| if (/^\+?[1-9]\d{1,14}$/.test(identifier)) return 'phone' | ||
| if (/^[a-zA-Z0-9_-]{3,30}$/.test(identifier)) return 'username' | ||
| return null | ||
| } | ||
|
|
||
| function otpRedisKey(purpose, encryptedIdentifier) { | ||
| return `${purpose}:${encryptedIdentifier}` | ||
| } | ||
|
|
||
| function resolveOtpPurpose({ username, registrationCode, userExists }) { | ||
| if (username) return OTP_PURPOSES.LOGIN | ||
| if (registrationCode) return OTP_PURPOSES.SIGNUP | ||
| if (userExists) return OTP_PURPOSES.LOGIN | ||
| return OTP_PURPOSES.SIGNUP | ||
| } | ||
|
|
||
| async function getExistingOtpForPurpose(purpose, identifiers) { | ||
| for (const identifier of identifiers) { | ||
| const data = await utilsHelper.redisGet(otpRedisKey(purpose, identifier)) | ||
| if (data && data.action === purpose) { | ||
| return data | ||
| } | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| async function setOtpForPurpose(purpose, identifiers, redisData) { | ||
| const redisSetResults = [] | ||
| for (const identifier of identifiers) { | ||
| const result = await utilsHelper.redisSet(otpRedisKey(purpose, identifier), redisData, common.otpExpirationTime) | ||
| redisSetResults.push(result) | ||
| } | ||
| return redisSetResults | ||
| } | ||
|
|
There was a problem hiding this comment.
Double-check this helper funcs and move the ones we discussed to helper/utils
There was a problem hiding this comment.
Made the changes, the functions for otp handling is still kept others have been moved
|
|
||
| // OTP validation | ||
| if (process.env.ENABLE_EMAIL_OTP_VERIFICATION === 'true') { | ||
| if (process.env.ENABLE_EMAIL_OTP_VERIFICATION === 'true' && allowedAuthMode.includes(AUTH_MODES.OTP)) { |
There was a problem hiding this comment.
Remove && allowedAuthMode.includes(AUTH_MODES.OTP)) as we need to verify the identity all the time
| if (bodyData.password && !passwordAuthAllowed) { | ||
| return failureResponse('AUTH_MODE_NOT_ALLOWED') | ||
| } | ||
|
|
||
| if (passwordAuthAllowed && !bodyData.password) { | ||
| return failureResponse('PASSWORD_REQUIRED') | ||
| } |
There was a problem hiding this comment.
Can we move this to the validator?
There was a problem hiding this comment.
Currently its better to keep it here, inorder to move it to validator it will require few changes as configurations are fetched from db
Will note this and pick it up later if thats fine
There was a problem hiding this comment.
Ok, can we add a "#TODO:" comment so we can keep track of this?
| bodyData.username = await generateUniqueUsername( | ||
| bodyData.name || plaintextEmailId || plaintextPhoneNumber | ||
| ) |
There was a problem hiding this comment.
can we not use plaintextEmailId || plaintextPhoneNumber since its PII
| if (type === 'email') { | ||
| encryptedIdentifier = emailEncryption.encrypt(identifier) | ||
| query[Op.or].push({ email: encryptedIdentifier }) | ||
| } else if (type === 'phone') { | ||
| encryptedIdentifier = emailEncryption.encrypt(identifier) | ||
| query[Op.or].push({ phone: encryptedIdentifier, phone_code: bodyData.phone_code }) |
There was a problem hiding this comment.
Do we need to hardcode the 'email and phone' in line 799 and 804
| if (bodyData.password && !passwordAuthAllowed) { | ||
| return failureResponse('AUTH_MODE_NOT_ALLOWED') | ||
| } | ||
|
|
||
| if (passwordAuthAllowed && !bodyData.password) { | ||
| return failureResponse('PASSWORD_REQUIRED') | ||
| } |
There was a problem hiding this comment.
Ok, can we add a "#TODO:" comment so we can keep track of this?
| const existingOtpData = await getExistingOtpForPurpose(purpose, redisIdentifiers) | ||
|
|
||
| const otp = existingOtpData ? existingOtpData.otp : utils.generateSecureOTP() | ||
| const isNew = !existingOtpData |
There was a problem hiding this comment.
isNew is computed but we always write to Redis regardless. Every resend call resets the OTP expiry, so it never actually expires. Should only write when it's a new OTP.
if (isNew) {
const redisSetResults = await setOtpForPurpose(purpose, redisIdentifiers, redisData)
if (!redisSetResults.includes('OK')) { ... }
}| refreshToken | ||
| ) | ||
|
|
||
| if (hasOtp) await utilsHelper.redisDel(otpRedisKey(OTP_PURPOSES.LOGIN, redisIdentifier)) |
There was a problem hiding this comment.
purpose is already in scope here, no need to hardcode OTP_PURPOSES.LOGIN. Minor but keeps it consistent if the logic above ever changes.
if (hasOtp) await utilsHelper.redisDel(otpRedisKey(purpose, redisIdentifier))There was a problem hiding this comment.
Incomplete OTP Redis key cleanup after OTP-based login leaves reusable keys
When a user logs in using OTP, only a single Redis key is deleted at src/services/account.js:951. However, registrationOtp stores the OTP at multiple Redis keys when the OTP is requested via username (e.g., login:<username>, login:<encryptedEmail>, login:<phoneCode+encryptedPhone> at lines 1360-1362). After a successful login using one identifier (e.g., username), the OTP remains valid under the other keys until TTL expiry. This allows the same OTP to be reused to create additional sessions for the same account via a different identifier within the TTL window.
There was a problem hiding this comment.
Changes done for purpose
For the otp deletion, this scenario might happen only for login via username
If its fine for now will make a note of it and take it up in the next release
| // Validate that at least one contact method is provided | ||
| if (!bodyData.email && !bodyData.phone) { | ||
| const config = tenantDetail.configuration || {} | ||
| const allowedAuthMode = config.allowed_auth_mode || [] |
There was a problem hiding this comment.
If this comes back as a non-array (bad config, migration issue), .includes() will throw. Small guard to be safe:
const allowedAuthMode = Array.isArray(config.allowed_auth_mode)
? config.allowed_auth_mode
: []Make the same changes in other occurrences as well
| if (!isOtpValid) { | ||
| return badRequestResponse('OTP_INVALID') | ||
| } |
There was a problem hiding this comment.
If an account gets created between OTP request and login (e.g. admin creates the user), the OTP is stored under SIGNUP but login looks under LOGIN — user gets OTP_INVALID with no way to recover except re-requesting. Worth documenting at minimum.
No code fix needed unless you want to handle it-in which case, check both keys on login:
const redisData =
await utilsHelper.redisGet(otpRedisKey(OTP_PURPOSES.LOGIN, redisIdentifier)) ||
await utilsHelper.redisGet(otpRedisKey(OTP_PURPOSES.SIGNUP, redisIdentifier))There was a problem hiding this comment.
Currently not making this change, but have added a TODO comment to take it up for the next release
| function checkIdentifierType(identifier) { | ||
| if (/^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/.test(identifier)) return common.EMAIL | ||
| if (/^\+?[1-9]\d{1,14}$/.test(identifier)) return common.PHONE | ||
| if (/^[a-zA-Z0-9_-]{3,30}$/.test(identifier)) return common.USER_NAME |
There was a problem hiding this comment.
Can we remove - since it wasn't present in the original regex?
There was a problem hiding this comment.
Keeping this as system generated name might have -
| } | ||
|
|
||
| // Send SMS notification with OTP if phone is provided | ||
| if (plaintextPhoneNumber && bodyData.phone_code) { |
There was a problem hiding this comment.
SMS notification skipped in username-based OTP flow due to using bodyData.phone_code instead of local phoneCode
In registrationOtp, when a user requests an OTP by providing their username, the SMS notification check at line 1450 uses bodyData.phone_code (from the request body) instead of the local phoneCode variable. The local phoneCode is correctly populated from the user's database record at src/services/account.js:1347, but the notification guard condition references bodyData.phone_code which will be undefined when only username is provided in the request (the validator makes phone_code optional). This causes the SMS notification to be silently skipped even when the user has a phone number on their account.
| refreshToken | ||
| ) | ||
|
|
||
| if (hasOtp) await utilsHelper.redisDel(otpRedisKey(OTP_PURPOSES.LOGIN, redisIdentifier)) |
There was a problem hiding this comment.
Incomplete OTP Redis key cleanup after OTP-based login leaves reusable keys
When a user logs in using OTP, only a single Redis key is deleted at src/services/account.js:951. However, registrationOtp stores the OTP at multiple Redis keys when the OTP is requested via username (e.g., login:<username>, login:<encryptedEmail>, login:<phoneCode+encryptedPhone> at lines 1360-1362). After a successful login using one identifier (e.g., username), the OTP remains valid under the other keys until TTL expiry. This allows the same OTP to be reused to create additional sessions for the same account via a different identifier within the TTL window.
There was a problem hiding this comment.
We should get the list of tenants instead of orgs; a new entity type should be inserted for only the default org (process.env....) of the available tenants since default org data is available to other orgs.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Data & Model Updates
Validation & API Changes
Localization