Encrypted phone, admin create checks, validator updates, unified login#811
Encrypted phone, admin create checks, validator updates, unified login#811nevil-mathew merged 5 commits intodevelopfrom
Conversation
WalkthroughAdds optional email/username/phone inputs, encrypts phone and email when present, and replaces single-field duplicate checks with a transactional OR-based existence query across encrypted email, encrypted phone (with phone_code), and username; validators updated for multi-identifier create/login and cross-field rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant V as Validator (`v1/admin`)
participant S as AdminService (`AdminHelper.create`)
participant DB as Database
rect rgba(230,245,255,0.6)
C->>V: POST /admin (email?/phone?/username?, password, phone_code?)
V->>V: Validate fields and cross-field rules
V-->>C: 400 on validation error
end
V->>S: AdminHelper.create(bodyData)
S->>S: normalize email lowercase (if present)
S->>S: encrypt email (if present) and phone (if present)
S->>DB: SELECT id WHERE tenant AND [Op.or]: {email}, {phone + phone_code}, {username} (txn)
alt existing user found
S-->>C: Error: ADMIN_USER_ALREADY_EXISTS
else
S->>S: set bodyData.phone = encryptedPhone (if any)
S->>DB: INSERT admin (txn)
DB-->>S: Created
S-->>C: 201 Created
end
sequenceDiagram
autonumber
participant C as Client
participant V as Validator (`v1/admin`)
note over C,V: Login with unified identifier
C->>V: POST /admin/login (identifier, password, [phone_code])
V->>V: Determine identifier type:
V->>V: - email → normalize & validate
V->>V: - phone → validate digits/length & require phone_code
V->>V: - username → validate pattern
V-->>C: 400 on invalid format/missing deps
V-->>C: pass to auth handler on success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/admin.js (2)
444-446: Duplicate validation helper functionsThe same validation helper functions (
isEmail,isPhone,isUsername) are defined multiple times in the file (lines 311-313 and 444-446). This violates the DRY principle.Extract these validation helpers to a shared utility module:
// Create a new file: src/utils/identifierValidation.js module.exports = { isEmail: (str) => /^[\w-.]+@([\w-]+\.)+[\w-]{2,4}$/.test(str), isPhone: (str) => /^\+?[1-9]\d{1,14}$/.test(str), isUsername: (str) => /^[a-zA-Z0-9_]{3,30}$/.test(str) }Then import and use them in the service file:
+const { isEmail, isPhone, isUsername } = require('@utils/identifierValidation') // Remove the duplicate definitions at lines 311-313 and 444-446 -const isEmail = (str) => /^[\w-.]+@([\w-]+\.)+[\w-]{2,4}$/.test(str) -const isPhone = (str) => /^\+?[1-9]\d{1,14}$/.test(str) -const isUsername = (str) => /^[a-zA-Z0-9_]{3,30}$/.test(str)
310-313: Align phone validation patterns across services and validators
Service helpers use/^\+?[1-9]\d{1,14}$/(e.g. src/services/admin.js:312, src/services/account.js:674), while validators use/^[0-9]{7,15}$/and^\d{6,15}$(e.g. src/validators/v1/admin.js:45,88; src/validators/v1/account.js:69,120). Standardize on a single regex (or E.164 format) and extract it into a shared helper.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/services/admin.js(3 hunks)src/validators/v1/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/admin.js
🧬 Code graph analysis (2)
src/services/admin.js (2)
src/services/account.js (8)
encryptedPhoneNumber(117-117)encryptedPhoneNumber(1185-1185)emailEncryption(29-29)criteria(126-126)criteria(1194-1194)encryptedEmailId(108-108)encryptedEmailId(1176-1176)encryptedEmailId(1792-1792)src/services/organization.js (2)
emailEncryption(17-17)encryptedEmailId(70-70)
src/validators/v1/admin.js (3)
src/constants/blacklistConfig.js (1)
admin(136-208)src/services/admin.js (6)
isEmail(311-311)isEmail(444-444)isPhone(312-312)isPhone(445-445)isUsername(313-313)isUsername(446-446)src/services/account.js (9)
isEmail(673-673)isEmail(1022-1022)isEmail(1352-1352)isPhone(674-674)isPhone(1023-1023)isPhone(1353-1353)isUsername(675-675)isUsername(1024-1024)isUsername(1354-1354)
🔇 Additional comments (3)
src/services/admin.js (1)
136-146: Transaction handling within existence checkThe transaction is passed to the query but not used consistently throughout the function. This could lead to data inconsistency if other operations fail after the check.
Good use of transaction for the existence check to ensure consistency.
src/validators/v1/admin.js (2)
64-78: Cross-field validation logic is correctThe validation ensures that at least one identifier (email or phone) is provided and that phone_code is required when phone is present. This is a good implementation of dependent field validation.
98-99: Conditional email normalization implementationGood implementation of conditional email normalization - it only normalizes when the identifier is detected as an email format.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/admin.js (2)
195-205: Return plaintext phone (or omit) instead of ciphertext in API response
processedUserreplaces email with plaintext but leavesphoneencrypted, leaking ciphertext and creating an inconsistent API surface.Apply:
- const processedUser = { ...user, email: plaintextEmailId } + const processedUser = { + ...user, + email: plaintextEmailId, + phone: user?.phone ? emailEncryption.decrypt(user.phone) : null, + }
319-329: Require or omitphone_codein phone-login query
In the login query (src/services/admin.js 319–329),isPhoneuses/^\+?[1-9]\d{1,14}$/so it matches both E.164 (+…) and local numbers, yet you always includephone_code(often undefined for+-prefixed values), causing Sequelize to generatephone_code IS NULLclauses and ambiguous matches (github.com). The validator only enforcesphone_codeon pure-digit identifiers, so for E.164 you should drop it and for local numbers enforce it. Update this block accordingly:} else if (isPhone(identifier)) { - // expects bodyData.phone_code when phone login is used - query[Op.or].push({ - phone: emailEncryption.encrypt(identifier), - phone_code: bodyData.phone_code, - }) + if (identifier.startsWith('+')) { + query[Op.or].push({ phone: emailEncryption.encrypt(identifier) }) + } else { + if (!bodyData.phone_code) + return failure('PHONE_CODE_REQUIRED', httpStatusCode.bad_request) + query[Op.or].push({ + phone: emailEncryption.encrypt(identifier), + phone_code: bodyData.phone_code, + }) + }
♻️ Duplicate comments (1)
src/services/admin.js (1)
131-142: OK to skip empty-criteria guard given validator guaranteeGiven validator-enforced presence of at least one identifier, calling
findOnewith a non-empty[Op.or]is safe. Removing the deadcriteria.length === 0block was the right call.If helpful, verify the validator rule exists:
#!/bin/bash # Expect to find rules enforcing at least one of email/phone/username for admin.create rg -nC2 -i 'admin.*create|at least one|identifier|required|phone_code' --type=js src/validators
🧹 Nitpick comments (3)
src/services/admin.js (3)
115-117: Normalize phone before encryption; avoid mismatches across flowsConfirm that validators normalize phone to a canonical E.164 string before this encryption. Otherwise, dedup/login may miss matches.
301-310: Username lowercasing: confirm storage/compare semanticsLowercasing the entire identifier makes username login case-insensitive. Confirm usernames are normalized to lowercase at creation or that DB collation is case-insensitive; otherwise logins may fail.
Example tweak (only lowercase when email):
- const identifier = rawIdentifier.toLowerCase() + const identifier = isEmail(rawIdentifier) ? rawIdentifier.toLowerCase() : rawIdentifier
126-135: Add DB indexes to keep multi-criteria lookups fastEnsure indexes on
(tenant_code, email),(tenant_code, phone, phone_code)(or(tenant_code, phone)if E.164), and(tenant_code, username)to support the new OR query without regressions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/services/admin.js(3 hunks)src/validators/v1/admin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/validators/v1/admin.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/admin.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#811
File: src/services/admin.js:126-133
Timestamp: 2025-09-02T14:15:52.372Z
Learning: In the admin service create method, validation of required fields (email, phone, username) is handled at the validator layer, making the criteria.length === 0 check unreachable code that should be removed.
📚 Learning: 2025-09-02T14:15:52.372Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#811
File: src/services/admin.js:126-133
Timestamp: 2025-09-02T14:15:52.372Z
Learning: In the admin service create method, validation of required fields (email, phone, username) is handled at the validator layer, making the criteria.length === 0 check unreachable code that should be removed.
Applied to files:
src/services/admin.js
🧬 Code graph analysis (1)
src/services/admin.js (5)
src/services/org-admin.js (6)
plaintextEmailId(669-669)bodyData(435-435)emailEncryption(27-27)userQueries(14-14)userQueries(45-48)userQueries(117-117)src/services/organization.js (3)
plaintextEmailId(69-69)encryptedEmailId(70-70)emailEncryption(17-17)src/services/account.js (12)
plaintextEmailId(109-109)plaintextEmailId(1177-1177)plaintextEmailId(1791-1791)encryptedEmailId(108-108)encryptedEmailId(1176-1176)encryptedEmailId(1792-1792)emailEncryption(29-29)encryptedPhoneNumber(117-117)encryptedPhoneNumber(1185-1185)criteria(126-126)criteria(1194-1194)userQueries(17-17)src/services/user.js (2)
emailEncryption(21-21)userQueries(11-11)src/database/queries/users.js (1)
emailEncryption(6-6)
🔇 Additional comments (1)
src/services/admin.js (1)
159-160: LGTM: encrypted-at-rest propagationPersisting the encrypted phone aligns with the new policy. Ensure
phone_codeis also persisted (if provided) for consistent lookups.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes