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 AuthService
participant UC as UserCreate
participant SS as Sessions
C->>UR: POST /login {username/email, password}
UR->>AU: authenticate(credentials)
AU-->>UR: Error OBJECT_NOT_FOUND
alt autoSignupOnLogin enabled & username/password present
UR->>UC: createUser(minimalCredentials)
UC-->>UR: {userId, tempSessionToken}
UR->>AU: authenticate(credentials) -- retry after signup
AU-->>UR: {sessionToken, user}
UR->>SS: revoke(tempSessionToken) -- best-effort cleanup
SS-->>UR: OK / NOT_FOUND
UR-->>C: 200 {sessionToken, user}
else disabled or missing credentials
UR-->>C: Error OBJECT_NOT_FOUND
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)spec/ParseUser.spec.jsThanks 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 “foo@example.com” when the user typed “ foo@example.com ”.
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.
Moumouls
left a comment
Moumouls
left a comment
There was a problem hiding this comment.
issue: potential security breach, it's uncommon to signup a user by email without validating his email, this feature can expose a breach or will be useless in common production app.
the approach should be inverted and like "autoLoginOnSignup", i you provided the correct email/username and password to the login endpoint, you login the user, otherwise you try to signup and then return error code if email validation for example is needed.
Also i don't know how the server SDK, all libs will react by returning a email validation sent on the login endpoint
Moumouls
left a comment
Moumouls
left a comment
There was a problem hiding this comment.
issue: if this feature is merged like this, i strongly suggest to add tests to covering edges cases and email validation/user account creation are not bypassed
That should not be possible. If validation is needed, then the login endpoint should return the same response as the the sign-up endpoint would.
What's the difference? If we extend
Good point; it seems more tests need to be added before merging. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(4 hunks)types/Options/index.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- types/Options/index.d.ts
- src/Options/Definitions.js
- spec/ParseUser.spec.js
- src/Options/docs.js
- src/Config.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Applied to files:
src/Routers/UsersRouter.jssrc/Options/index.js
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
src/Options/index.js
📚 Learning: 2025-12-02T06:55:53.808Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T06:55:53.808Z
Learning: When reviewing Parse Server PRs that add or modify Parse Server options, always verify that changes are properly reflected in three files: src/Options/index.js (where changes originate), src/Options/Definitions.js, and src/Options/docs.js. The correct workflow is: make changes in index.js first, then run `npm run definitions` to automatically replicate the changes to Definitions.js and docs.js.
Applied to files:
src/Options/index.js
🪛 Biome (2.1.2)
src/Options/index.js
[error] 196-196: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Node 22
- GitHub Check: Check Types
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: Benchmarks
🔇 Additional comments (5)
src/Routers/UsersRouter.js (4)
205-247: Race condition and session cleanup properly handled.The implementation correctly addresses:
- Race conditions by catching duplicate errors (lines 223-226) and re-authenticating
- Temporary session cleanup (lines 231-241) with proper error handling
- Consistent error behavior by ensuring wrong passwords return
OBJECT_NOT_FOUNDHowever, note that the fundamental design concerns raised by the maintainer in the PR comments remain: whether auto-signup-on-login is the right approach vs. auto-login-on-signup, and whether email verification requirements are being properly enforced throughout this flow.
Based on PR comments from the maintainer.
367-380: LGTM - Good refactoring.This helper consolidates the credential extraction logic used in multiple places, reducing duplication and improving maintainability.
788-788: LGTM - Improved error logging.Using
JSON.stringify(e)ensures all error properties are captured in logs, improving observability for debugging authentication failures.
414-441: Verify thatrest.createenforces all signup policies includingpreventSignupWithUnverifiedEmailandverifyUserEmails.The implementation delegates to
rest.createfor user creation, which should apply existing validation logic. However, manual verification is needed to confirm that email verification policies are properly enforced whenrest.createis called in the auto-signup context, particularly to ensurepreventSignupWithUnverifiedEmailandverifyUserEmailssettings are respected.src/Options/index.js (1)
196-200: Verify email verification enforcement during auto-signup flow.Ensure that when
autoSignupOnLoginis enabled, all email verification requirements are applied—specifically thatpreventSignupWithUnverifiedEmailandverifyUserEmailssettings are enforced during the auto-signup process, just as they are during normal signup.Additionally, consider the maintainer's suggestion to evaluate an alternative approach: implementing "autoLoginOnSignup" where the signup endpoint handles both signup and login in a single request, rather than modifying the login endpoint to create users.
| return { | ||
| username: hasUsername ? rawUsername : rawEmail, | ||
| email: hasEmail ? rawEmail : undefined, | ||
| password, |
There was a problem hiding this comment.
Consider validation implications of username fallback to email.
When only an email is provided (line 408), the username is set to the email address. This could cause issues:
- Email format may not meet username validation requirements (e.g., special characters, length limits)
- Could conflict with existing usernames if someone's username happens to match another user's email
- May be unexpected behavior for users
Verify that this approach is validated correctly:
#!/bin/bash
# Check username validation rules that might conflict with email format
rg -n -C3 'username.*validation|validateUsername' --type=jsConsider requiring an explicit username for auto-signup, or document this behavior clearly in the option description.
🤖 Prompt for AI Agents
In src/Routers/UsersRouter.js around lines 407 to 410, the code falls back to
using the raw email as the username which can violate username validation rules
and create uniqueness/conflict issues; update the signup flow so that when
username is derived from email you (a) run the same username validation routine
(validateUsername/username validation rules) against the email-derived username
and reject the request with a clear error if it fails, or (b) instead require an
explicit username for auto-signup (document this option), or (c) if you prefer
auto-generation, create a sanitized/normalized username from the email (strip
invalid chars, enforce length) then check DB uniqueness and resolve collisions
(append numeric suffix) before saving; implement one of these fixes and ensure
the error path returns a descriptive message and the DB uniqueness check is
performed prior to persisting.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9873 +/- ##
==========================================
- Coverage 92.56% 92.50% -0.07%
==========================================
Files 191 191
Lines 15544 15592 +48
Branches 177 177
==========================================
+ Hits 14389 14424 +35
- Misses 1143 1156 +13
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Moumouls Just a ping so we can continue with this PR; there are some open questions. |
|
@swittk are you still willing to work on this PR? |
4 participants
Pull Request
Issue
Closes: #9560
Approach
autoSignupOnLoginboolean flag for Parse-server configurationTasks
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.