feat: Added autosignuponlogin #9873
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds a new server option Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant UR as UsersRouter
participant AU as Auth Service
participant UC as User Create (REST)
participant SS as Sessions
C->>UR: POST /login {username/email, password}
UR->>AU: Authenticate
AU-->>UR: Error OBJECT_NOT_FOUND
alt autoSignupOnLogin enabled and credentials present
UR->>UC: Create user (auto-signup)
UC-->>UR: {userId, tempSessionToken}
note over UR: store temp signup session token
UR->>AU: Authenticate (retry)
AU-->>UR: {sessionToken, user}
UR->>SS: Revoke tempSessionToken
SS-->>UR: OK / NOT_FOUND
UR-->>C: 200 {sessionToken, user}
else disabled or invalid credentials
UR-->>C: Error OBJECT_NOT_FOUND
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (4)
spec/ParseUser.spec.js (1)
189-213: Always restore server config; use try/finally to prevent leakageIf an assertion throws before the last lines,
autoSignupOnLoginmay remain enabled and affect other tests.Apply try/finally so logout and config reset always run.
it('auto signs up user on login when enabled', async () => { - await reconfigureServer({ autoSignupOnLogin: true }); - const username = 'autoLoginUser'; - const password = 'autoLoginPass'; - const response = await request({ - method: 'POST', - url: 'http://localhost:8378/1/login', - headers: { - 'X-Parse-Application-Id': Parse.applicationId, - 'X-Parse-REST-API-Key': 'rest', - 'Content-Type': 'application/json', - }, - body: { - username, - password, - }, - }); - expect(response.data.username).toBe(username); - expect(response.data.sessionToken).toBeDefined(); - - const user = await Parse.User.logIn(username, password); - expect(user).toBeDefined(); - await Parse.User.logOut(); - await reconfigureServer({ autoSignupOnLogin: false }); + await reconfigureServer({ autoSignupOnLogin: true }); + const username = 'autoLoginUser'; + const password = 'autoLoginPass'; + try { + const response = await request({ + method: 'POST', + url: 'http://localhost:8378/1/login', + headers: { + 'X-Parse-Application-Id': Parse.applicationId, + 'X-Parse-REST-API-Key': 'rest', + 'Content-Type': 'application/json', + }, + body: { username, password }, + }); + expect(response.data.username).toBe(username); + expect(response.data.sessionToken).toBeDefined(); + const user = await Parse.User.logIn(username, password); + expect(user).toBeDefined(); + } finally { + await Parse.User.logOut().catch(() => {}); + await reconfigureServer({ autoSignupOnLogin: false }); + } });src/Routers/UsersRouter.js (3)
366-393: Gate auto-signup strictly and align normalization
- Don’t auto-signup if a user with provided username/email exists; avoids enumeration on wrong password.
- Avoid trimming here unless authentication does the same; differing normalization can create unintended accounts.
Apply the following changes (make function async, add existence check, remove trim):
- _prepareAutoSignupCredentials(req, error) { + async _prepareAutoSignupCredentials(req, error) { if (!req.config.autoSignupOnLogin) { return null; } if (!(error instanceof Parse.Error) || error.code !== Parse.Error.OBJECT_NOT_FOUND) { return null; } if (req.body && req.body.authData) { return null; } const payload = this._getLoginPayload(req); - const rawUsername = typeof payload.username === 'string' ? payload.username.trim() : ''; - const rawEmail = typeof payload.email === 'string' ? payload.email.trim() : ''; - const password = payload.password; - const hasUsername = rawUsername.length > 0; - const hasEmail = rawEmail.length > 0; + const rawUsername = typeof payload.username === 'string' ? payload.username : ''; + const rawEmail = typeof payload.email === 'string' ? payload.email : ''; + const password = payload.password; + const hasUsername = rawUsername.length > 0; + const hasEmail = rawEmail.length > 0; if (!hasUsername && !hasEmail) { return null; } if (typeof password !== 'string') { return null; } + // Only auto-signup when there is no existing user for supplied identifier(s) + let query; + if (hasEmail && hasUsername) { + query = { email: rawEmail, username: rawUsername }; + } else if (hasEmail) { + query = { email: rawEmail }; + } else { + query = { $or: [{ username: rawUsername }, { email: rawUsername }] }; + } + const existing = await req.config.database.find('_User', query, {}, Auth.maintenance(req.config)); + if (existing && existing.length) { + return null; + } return { username: hasUsername ? rawUsername : rawEmail, email: hasEmail ? rawEmail : undefined, password, }; }
395-422: Handle duplicate username/email errors gracefully to avoid race-induced failuresIf another request created the user between your existence check and create, rest.create will throw USERNAME_TAKEN / EMAIL_TAKEN. Swallow these and continue to re-authenticate.
Apply:
async _autoSignupOnLogin(req, credentials) { const userData = { username: credentials.username, password: credentials.password, }; if (credentials.email !== undefined) { userData.email = credentials.email; } @@ - const result = await rest.create( - req.config, - req.auth, - '_User', - userData, - req.info.clientSDK, - req.info.context - ); - const user = result?.response; - if (!user) { - throw new Parse.Error( - Parse.Error.INTERNAL_SERVER_ERROR, - 'Unable to automatically sign up user.' - ); - } - return user.sessionToken; + try { + const result = await rest.create( + req.config, + req.auth, + '_User', + userData, + req.info.clientSDK, + req.info.context + ); + const user = result?.response; + if (!user) { + throw new Parse.Error( + Parse.Error.INTERNAL_SERVER_ERROR, + 'Unable to automatically sign up user.' + ); + } + return user.sessionToken; + } catch (e) { + if (e && (e.code === Parse.Error.USERNAME_TAKEN || e.code === Parse.Error.EMAIL_TAKEN)) { + // Likely created concurrently; let caller re-attempt authentication + return null; + } + throw e; + } }
377-382: Normalization mismatch (trim) may create unintended accountsLogin authentication doesn’t trim username/email, but auto-signup does. This can auto-create “[email protected]” when the user typed “ [email protected] ”.
Remove trim here (or apply consistent normalization across both paths). See suggested diff in the comment above for Lines 366-393.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
spec/ParseUser.spec.js(1 hunks)src/Config.js(3 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/Routers/UsersRouter.js(3 hunks)types/Options/index.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
spec/ParseUser.spec.js (1)
spec/helper.js (2)
reconfigureServer(171-205)Parse(4-4)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 197-197: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🔇 Additional comments (4)
src/Options/index.js (1)
193-197: Option surfaced correctly; verify Biome parse error on Line 197The field and docs look consistent with the surrounding Flow types. However, static analysis flagged “Expected a statement but instead found ‘?’” at Line 197; likely a parser/config hiccup or stray char.
- Please re-run lint/format locally and ensure Biome/ESLint parse this file cleanly.
- If needed, regenerate options or normalize the line to remove hidden chars.
types/Options/index.d.ts (1)
80-80: Types aligned
autoSignupOnLogin?: booleanmatches the public option surface.src/Options/docs.js (1)
89-89: Docs added and consistentJSDoc for
autoSignupOnLoginmatches Definitions/help and default.src/Options/Definitions.js (1)
485-491: Definition looks goodEnv, help, parser, and default for
autoSignupOnLoginare correct and aligned with the option surface.
feat: Auto signup (followed suggestions partially)
|
any more changes needed? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Routers/UsersRouter.js (1)
365-378: Consider extracting shared payload logic.This method duplicates the query/body fallback logic from
_authenticateUserFromRequest(lines 71-77). While the duplication is minimal, extracting this shared logic into_getLoginPayloadand calling it from both methods would improve maintainability.Optional refactor:
Reuse
_getLoginPayloadin_authenticateUserFromRequest:_authenticateUserFromRequest(req) { return new Promise((resolve, reject) => { - // Use query parameters instead if provided in url - let payload = req.body || {}; - if ( - (!payload.username && req.query && req.query.username) || - (!payload.email && req.query && req.query.email) - ) { - payload = req.query; - } - const { username, email, password, ignoreEmailVerification } = payload; + const payload = this._getLoginPayload(req); + const { username, email, password } = payload; + const ignoreEmailVerification = (req.body || req.query)?.ignoreEmailVerification;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Routers/UsersRouter.js(4 hunks)
🔇 Additional comments (4)
src/Routers/UsersRouter.js (4)
203-245: Implementation addresses race condition and enumeration concerns.The current implementation successfully handles the concerns raised in previous reviews:
- Race condition (lines 217-228): Catches duplicate-user errors during signup and re-authenticates, allowing concurrent login requests to succeed when another request creates the user first.
- User enumeration: By re-authenticating on
USERNAME_TAKEN/EMAIL_TAKENerrors, wrong-password scenarios correctly returnOBJECT_NOT_FOUND(matching standard login behavior) rather than leaking account existence.- Session cleanup (lines 229-239): Properly destroys the temporary signup session token with appropriate error handling.
The implementation aligns well with the discussion in past reviews.
383-410: LGTM!The guard conditions and credential preparation logic are well-structured:
- Comprehensive checks ensure auto-signup only proceeds when appropriate (feature enabled, OBJECT_NOT_FOUND error, no authData, valid credentials).
- Proper trimming and validation of username and email fields.
- Correctly handles the case where only email is provided by using it as the username (line 406), ensuring the username field is always populated for user creation.
412-439: LGTM!The implementation correctly delegates to
rest.create, ensuring all existing validation, triggers, schema checks, and side effects are executed. The error handling properly covers the edge case where user creation might succeed but not return a user object.
744-744: LGTM!Standardizing error serialization with
JSON.stringifyimproves consistency in error logging.
3 participants
Pull Request
Issue
Closes: #9560
Approach
autoSignupOnLoginboolean flag for Parse-server configurationTasks
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests