Changes done to handle login with otp and auto registration flow based on configuration#888
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughAdds tenant auth configuration storage and validation, propagates tenant configuration through service and DTO paths, updates account and admin identifier handling for email, phone, and username, and adds a transactional migration for ChangesTenant auth configuration and account flow refactor
Entity type backfill migration
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)
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
| 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
| bodyData.username = await generateUniqueUsername( | ||
| bodyData.name || plaintextEmailId || plaintextPhoneNumber | ||
| ) |
There was a problem hiding this comment.
can we not use plaintextEmailId || plaintextPhoneNumber since its PII
| 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 -
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.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/database/migrations/20260623124000-add-name-entity-type.js (1)
54-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope the rollback to rows created by this migration.
downdeleting everyentity_typesrow withvalue = 'name'is broader thanup, which only inserts default-org rows. Rolling this back would also remove pre-existing or later-creatednameentity types for other orgs/tenants. Please delete with the same org/tenant scope used inup(or by IDs captured from the inserted set).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/migrations/20260623124000-add-name-entity-type.js` around lines 54 - 71, The rollback in the migration’s down method is too broad because it deletes every entity_types row with value 'name' instead of only the rows inserted by up. Update the bulkDelete in the down path to use the same org/tenant scoping as the insert logic in add-name-entity-type.js, or delete by the specific IDs created in up, and keep the transaction handling in down unchanged.src/services/admin.js (1)
453-461: 🩺 Stability & Availability | 🟠 MajorDigit-only usernames are misclassified as phone numbers.
checkIdentifierType()checksPHONEbeforeUSER_NAME, and the admin login validator accepts numeric usernames, so valid username logins can get routed into the phone branch and requirephone_codeunexpectedly. Reorder the checks or disallow numeric-only usernames.src/generics/utils.js:1138-1141🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/admin.js` around lines 453 - 461, The identifier handling in admin login is misclassifying digit-only usernames as PHONE because checkIdentifierType() returns PHONE before USER_NAME, which sends valid numeric usernames into the phone branch and requires phone_code unexpectedly. Update the logic in checkIdentifierType() and the admin login flow in admin.js to either prioritize USER_NAME checks before PHONE or explicitly reject numeric-only usernames, and make sure the query-building branch keyed by common.PHONE only runs for true phone identifiers.Source: Path instructions
src/services/account.js (1)
807-813: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLet valid passwords succeed when password and OTP are both supplied.
This returns
OTP_INVALIDbefore password verification runs, but Line 860 treats password and OTP as alternative credentials. A correct password plus an expired OTP currently fails login.Suggested patch
const redisData = await utilsHelper.redisGet(otpRedisKey(purpose, redisIdentifier)) isOtpValid = Boolean(redisData && redisData.otp === bodyData.otp) - if (!isOtpValid) { + if (!isOtpValid && !hasPassword) { return badRequestResponse('OTP_INVALID') }As per path instructions,
src/services/**: This is core business logic. Please check for correctness, efficiency, and potential edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/account.js` around lines 807 - 813, The OTP validation in the account login flow blocks valid password-based authentication when both credentials are supplied. Update the logic around hasOtp in account.js so OTP failure does not immediately return OTP_INVALID if a password is also present; instead, defer rejection until after the password check in the login path (near the later password/OTP credential handling) and only fail when neither credential is valid. Keep the behavior aligned with the existing alternative-credentials logic in the same flow and preserve the current redis-based OTP lookup via otpRedisKey and utilsHelper.redisGet.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/services/account.js`:
- Around line 1430-1431: The OTP reuse path in account.js only writes Redis keys
when isNew is true, so reused OTPs can miss newly requested aliases like a
username login key. Update the helper flow around setOtpForPurpose and the isNew
check to detect which redisIdentifiers are missing for the current request and
write only those absent keys even when reusing an existing OTP. Preserve the
existing OTP value and remaining TTL when adding the missing aliases, and keep
the logic in the existing account service flow correct for all alias
combinations.
In `@src/services/admin.js`:
- Line 441: The identifier classification in checkIdentifierType is sending
all-digit values down the PHONE path before USER_NAME, which causes numeric
usernames to skip username lookup. Update the matching flow in admin.js so
username handling is checked before phone matching in checkIdentifierType, or
tighten the username validators to reject numeric-only usernames, and ensure the
lookup logic in the affected admin service path still resolves usernames like
123456 correctly.
---
Outside diff comments:
In `@src/database/migrations/20260623124000-add-name-entity-type.js`:
- Around line 54-71: The rollback in the migration’s down method is too broad
because it deletes every entity_types row with value 'name' instead of only the
rows inserted by up. Update the bulkDelete in the down path to use the same
org/tenant scoping as the insert logic in add-name-entity-type.js, or delete by
the specific IDs created in up, and keep the transaction handling in down
unchanged.
In `@src/services/account.js`:
- Around line 807-813: The OTP validation in the account login flow blocks valid
password-based authentication when both credentials are supplied. Update the
logic around hasOtp in account.js so OTP failure does not immediately return
OTP_INVALID if a password is also present; instead, defer rejection until after
the password check in the login path (near the later password/OTP credential
handling) and only fail when neither credential is valid. Keep the behavior
aligned with the existing alternative-credentials logic in the same flow and
preserve the current redis-based OTP lookup via otpRedisKey and
utilsHelper.redisGet.
In `@src/services/admin.js`:
- Around line 453-461: The identifier handling in admin login is misclassifying
digit-only usernames as PHONE because checkIdentifierType() returns PHONE before
USER_NAME, which sends valid numeric usernames into the phone branch and
requires phone_code unexpectedly. Update the logic in checkIdentifierType() and
the admin login flow in admin.js to either prioritize USER_NAME checks before
PHONE or explicitly reject numeric-only usernames, and make sure the
query-building branch keyed by common.PHONE only runs for true phone
identifiers.
🪄 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: 3b91e979-f3f4-41b7-88a2-b370fca28157
📒 Files selected for processing (3)
src/database/migrations/20260623124000-add-name-entity-type.jssrc/services/account.jssrc/services/admin.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
||
| req.checkBody('configuration') | ||
| .optional({ checkFalsy: true }) | ||
| .custom(isValidTenantConfiguration) |
There was a problem hiding this comment.
Dont we need this in the udpate flow as well?
…gurations validator for tenant update
2 participants
Summary by CodeRabbit
Release Notes
configurationwith sensible defaults.nameandpasswordare now allowed to be nullable.