feat: Add autoSignupLogin to signup when user doesn't already exist
#9961
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds a new Parse Server option Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant LoginEndpoint as Login Endpoint
participant Auth as _authenticateUserFromRequest
participant AutoSignup as _autoSignupOnLogin
participant RestWrite as RestWrite (Create _User)
participant DB as User Database
Note over AutoSignup,Auth: New/changed interactions for auto-signup flow
Client->>LoginEndpoint: POST /login (username/email + password)
LoginEndpoint->>LoginEndpoint: _getLoginPayload(req)
LoginEndpoint->>Auth: _authenticateUserFromRequest(req)
Auth->>DB: Query user by username/email
alt User Found
DB-->>Auth: User record
Auth-->>LoginEndpoint: Auth success
LoginEndpoint-->>Client: 200 OK (user + session)
else User Not Found (OBJECT_NOT_FOUND)
DB-->>Auth: No user
Auth-->>LoginEndpoint: Error: OBJECT_NOT_FOUND
alt autoSignupOnLogin = true
LoginEndpoint->>AutoSignup: _autoSignupOnLogin(req)
AutoSignup->>AutoSignup: Build new _User payload (username/email, password)
AutoSignup->>RestWrite: Create _User
RestWrite->>DB: Insert user
DB-->>RestWrite: Insert result
AutoSignup->>DB: Fetch created user
DB-->>AutoSignup: User record
AutoSignup->>AutoSignup: Enforce email verification flags
AutoSignup-->>LoginEndpoint: { user, sessionToken?, authDataResponse }
LoginEndpoint-->>Client: 200 OK (new user + session)
else autoSignupOnLogin = false
LoginEndpoint-->>Client: 400 / OBJECT_NOT_FOUND
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-05-04T20:41:05.147ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-08-27T09:08:34.252ZApplied to files:
📚 Learning: 2025-11-08T13:46:04.940ZApplied to files:
📚 Learning: 2025-11-08T13:46:04.940ZApplied to files:
📚 Learning: 2025-10-16T19:27:05.311ZApplied to files:
📚 Learning: 2025-12-02T06:55:53.808ZApplied to files:
⏰ 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). (10)
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.3)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 |
✅ 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. |
![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 (3)
spec/ParseUser.spec.js (2)
225-235: Verify single session creation during auto-signup.The test should verify that only one session is created during auto-signup to ensure no session leakage. Consider adding an assertion similar to the test at lines 4148-4186 that checks session count.
Apply this diff to add session verification:
it('creates user on login when enabled (username + password)', async () => { await reconfigureServer({ autoSignupOnLogin: true }); const user = await Parse.User.logIn('auto-login-user', 'pass1234'); expect(user.id).toBeDefined(); expect(user.getSessionToken()).toBeDefined(); const stored = await new Parse.Query(Parse.User) .equalTo('username', 'auto-login-user') .first({ useMasterKey: true }); expect(stored).toBeTruthy(); expect(stored.id).toBe(user.id); + const sessionCount = await new Parse.Query('_Session') + .equalTo('user', user.toPointer()) + .count({ useMasterKey: true }); + expect(sessionCount).toBe(1); });
277-313: Document the email verification behavior for auto-signup.This test demonstrates that when
autoSignupOnLoginis enabled with email verification requirements, the user is created but login fails. This means a user record with an initial session token exists but cannot be used until verified. Consider documenting this behavior in code comments or updating the test description to make this edge case explicit, as it could lead to accumulation of unverified user accounts.src/Routers/UsersRouter.js (1)
188-193: Document username inference behavior.When auto-signup is triggered with email-only credentials, the username is set to the email value (line 189). This is tested and expected behavior (see test line 252-253 in spec/ParseUser.spec.js), but consider adding a comment here to clarify this design decision for future maintainers.
async _autoSignupOnLogin(req) { const { username, email, password } = this._getLoginPayload(req); + // When logging in with email only, use email as username const inferredUsername = username || email;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/ParseUser.spec.js(1 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)
🧰 Additional context used
🧠 Learnings (8)
📓 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-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.
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.
📚 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/docs.jssrc/Options/index.jssrc/Options/Definitions.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: 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/Options/docs.jssrc/Options/index.jssrc/Options/Definitions.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/docs.jssrc/Options/index.jssrc/Options/Definitions.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseUser.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/ParseUser.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/ParseUser.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/ParseUser.spec.js
🧬 Code graph analysis (2)
src/Routers/UsersRouter.js (2)
src/Auth.js (4)
req(538-538)req(551-551)Parse(1-1)user(522-522)src/RestWrite.js (11)
data(1730-1736)RestWrite(990-997)RestWrite(1155-1161)Auth(8-8)Auth(574-577)user(330-330)user(903-903)user(1517-1517)user(1798-1798)validatedAuthData(560-563)authData(453-453)
spec/ParseUser.spec.js (1)
src/TestUtils.js (1)
res(31-31)
🪛 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). (15)
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (7)
src/Options/Definitions.js (1)
116-122: LGTM! Option definition follows established patterns.The new
autoSignupOnLoginoption is correctly defined with all required fields (environment variable, parser, default value, and help text). The structure and formatting are consistent with other Parse Server options in this generated file.Based on learnings, the proper workflow has been followed: changes were made in
src/Options/index.jsandnpm run definitionswas executed to generate this file.src/Options/docs.js (1)
25-25: LGTM! Documentation is clear and complete.The JSDoc documentation for the new option accurately describes its behavior and default value, following the same format as other Parse Server options in this interface.
src/Options/index.js (2)
196-198: LGTM! Option definition is correct.The new
autoSignupOnLoginoption is properly defined with:
- Clear description of behavior
- Appropriate Flow type annotation (
?boolean)- Conservative default value (
false- opt-in feature)- Correct placement near related authentication options
The static analysis error reported by Biome is a false positive. The Flow syntax
?booleanfor nullable boolean types is valid and matches the pattern used throughout this file for other optional boolean properties.Based on learnings, this is the correct source file for Parse Server options, and the
npm run definitionscommand was executed to propagate changes toDefinitions.jsanddocs.js.
196-198: Note: Documentation checklist itemThe PR description shows "Add changes to documentation (guides, repository pages, code comments)" as unchecked. Based on learnings, README.md documentation is optional (though beneficial) for new Parse Server options. The option is properly documented in the code via JSDoc and inline comments. Consider whether additional user-facing documentation in README.md or guides would be helpful for this feature, but this is not a blocking issue.
src/Routers/UsersRouter.js (3)
64-95: LGTM! Clean credential extraction and validation.The method consolidates credential extraction logic with proper validation and consistent error codes.
102-104: LGTM! Clean refactor using centralized payload extraction.Using
_getLoginPayload(req)eliminates duplicate validation logic and improves maintainability.
273-273: Verify the newautoSignupOnLoginoption is properly documented in option files.When adding new Parse Server options, ensure:
- The option is defined in
src/Options/index.jsnpm run definitionshas been executed to sync changes tosrc/Options/Definitions.jsandsrc/Options/docs.js- README.md documentation (optional)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9961 +/- ##
==========================================
- Coverage 92.53% 92.52% -0.01%
==========================================
Files 190 190
Lines 15470 15510 +40
Branches 176 176
==========================================
+ Hits 14315 14351 +36
- Misses 1143 1147 +4
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Routers/UsersRouter.js (1)
182-186: JSDoc return type is incomplete.The documented return type
{{ user: Object, authDataResponse: any }}doesn't includesessionTokenandskipSessionCreationwhich are also returned./** * Auto sign-up when login misses existing user and option is enabled * @param {Object} req The request - * @returns {{ user: Object, authDataResponse: any }} + * @returns {{ user: Object, authDataResponse: any, sessionToken: string, skipSessionCreation: boolean }} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Routers/UsersRouter.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (1)
src/Routers/UsersRouter.js (2)
src/Auth.js (3)
req(538-538)req(551-551)user(522-522)src/RestWrite.js (11)
data(1730-1736)RestWrite(990-997)RestWrite(1155-1161)Auth(8-8)Auth(574-577)user(330-330)user(903-903)user(1517-1517)user(1798-1798)authData(453-453)sessionData(1012-1021)
⏰ 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). (15)
- GitHub Check: Node 22
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (4)
src/Routers/UsersRouter.js (4)
64-95: LGTM! Clean extraction and validation of login payload.Good refactor consolidating payload extraction logic. Using
OBJECT_NOT_FOUNDfor type validation errors is appropriate to avoid leaking information about which specific validation failed.
284-301: Auto-signup integration looks correct.The try/catch properly detects
OBJECT_NOT_FOUNDfrom authentication and delegates to_autoSignupOnLoginwhenautoSignupOnLoginis enabled. The session token from auto-signup is correctly assigned to the user object.
388-401: Session creation logic correctly avoids duplicates.The conditional check at line 389 ensures a new session is only created when auto-signup hasn't already created one. This addresses the duplicate session concern from past reviews.
102-104: LGTM! Clean refactor to use shared payload extraction.Using
_getLoginPayloadremoves duplication and ensures consistent validation between login and auto-signup paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Routers/UsersRouter.js (1)
319-323: Consider updating the PR title to follow Angular commit convention.Based on learnings, Parse Server PRs should use Angular commit convention for changelog generation. The current title has a typo and could be clearer:
Current:
feat: Add autoSignupLogin to signup when user doesn't already exist
Issues:
- Says "autoSignupLogin" but the option is named
autoSignupOnLogin- Says "to signup" which is confusing since this modifies the login flow
- Missing scope identifier
Suggested:
feat(auth): add autoSignupOnLogin option to auto-create user on login missThis format clearly conveys:
- Type: feat (new feature)
- Scope: auth (authentication subsystem)
- Description: what developers get (auto-user-creation when login finds no match)
Based on learnings, PR titles should make meaningful changelog entries for developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseUser.spec.js(1 hunks)src/Routers/UsersRouter.js(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/ParseUser.spec.js
🧰 Additional context used
🧠 Learnings (5)
📓 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.
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.
📚 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/Routers/UsersRouter.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Routers/UsersRouter.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
src/Routers/UsersRouter.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/Routers/UsersRouter.js
🧬 Code graph analysis (1)
src/Routers/UsersRouter.js (2)
src/Auth.js (2)
req(538-538)req(551-551)src/RestWrite.js (10)
request(823-829)request(958-964)verifyUserEmails(968-968)preventLoginWithUnverifiedEmail(969-969)RestWrite(990-997)RestWrite(1155-1161)Auth(8-8)Auth(574-577)validatedAuthData(560-563)authData(453-453)
⏰ 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). (16)
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 20
- GitHub Check: Node 18
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Node 22
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Benchmarks
🔇 Additional comments (5)
src/Routers/UsersRouter.js (5)
64-96: LGTM! Good abstraction for email verification flags.This method correctly handles both boolean and async function config options, and centralizes the resolution logic. This addresses the previous concern about inconsistent handling of
verifyUserEmailsandpreventLoginWithUnverifiedEmail.
98-129: LGTM! Clean payload extraction and validation.The method properly validates required fields and types, providing clear error messages. Good refactoring to centralize login payload handling.
136-205: LGTM! Clean refactoring with improved maintainability.The refactored method correctly delegates to
_getLoginPayloadand_resolveEmailVerificationFlags, eliminating code duplication while preserving the existing authentication logic.
309-450: LGTM! Clean integration of auto-signup flow.The
handleLogInmethod correctly integrates the auto-signup path:
- Attempts authentication first, falls back to auto-signup only on OBJECT_NOT_FOUND when configured
- Properly reuses the session token from auto-signup to avoid duplicate session creation (lines 326-328, 420-432)
- Maintains backward compatibility for normal login flow
This addresses the previous concern about duplicate session creation.
319-319: EnsureautoSignupOnLoginoption is documented in all three required files.Per Parse Server conventions, new options must be documented in
src/Options/index.js,src/Options/Definitions.js, andsrc/Options/docs.js. After making changes toindex.js, runnpm run definitionsto automatically replicate the changes to the other two files. Verify this workflow was followed for theautoSignupOnLoginoption.
|
@coratgerl mind that there is an existing PR for this feature with a discussion about the approach. I'm also unsure about the state of the other PR, it may even be finished. |
Hum, i checking the other PR and seems inactive since 2 months so I propose an approach here with edges cases tests like asked by @Moumouls. Like tests based on |
|
The other PR author has asked if any more changes were needed, but there wasn't any specific change request. It would be good to ask the other PR author if they're interested in continuing the PR, provided that we give a concrete feedback on what needs to change. |
Ok ! We can wait any response of other contributor and otherwise we can move forward here 🙂 |
|
Yes, but we'll have the same open questions. The reason the other PR got stale was because the questions were not resolved. Let's continue the discussion there and if the PR author abandoned the PR let's continue here. |
|
I think we can continue here no @mtrezza ? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/ParseUser.spec.js (1)
266-371: Good coverage for autoSignupOnLogin; consider adding a couple more edge‑case tests.The new suite nicely exercises disabled/enabled behavior, REST login, existing‑user reuse, and interaction with
preventLoginWithUnverifiedEmail. To lock in the intended semantics and catch regressions from the router logic, I recommend adding:
A test where
autoSignupOnLoginis true, a user already exists, and the password is wrong, asserting that:
- no new
_Useris created, and- the error remains
OBJECT_NOT_FOUND(no “leak” of username/email existence).A test covering
verifyUserEmails: true+preventSignupWithUnverifiedEmail: truewithautoSignupOnLoginenabled, asserting that no user or session is persisted when login attempts with a new email.These would align the tests closely with the option’s “only when no existing user is found” contract and the email‑verification semantics.
src/Routers/UsersRouter.js (1)
243-273: Email‑verification cleanup for auto‑signup is consistent; consider verifying behavior with preventSignupWithUnverifiedEmail only.The
_autoSignupOnLogincleanup path:
- Deletes both the
_Session(viaresponse.sessionToken) and the_Userrow when either:
verifyUserEmails && preventLoginWithUnverifiedEmail, orverifyUserEmails && preventSignupWithUnverifiedEmail,- then throws
EMAIL_NOT_FOUND.This is good: no orphaned “dead” accounts or sessions when email rules reject the auto‑signup, and your new test asserts the first branch (with
preventLoginWithUnverifiedEmail: true).Two follow‑ups worth checking:
preventSignupWithUnverifiedEmail‑only scenario
ForverifyUserEmails: true,preventSignupWithUnverifiedEmail: true,preventLoginWithUnverifiedEmail: false, it would be good to confirm (with a test) that auto‑signup:
- does not persist the new user or session, and
- still returns the intended
EMAIL_NOT_FOUNDerror.Session creation semantics when
verifyUserEmails: trueand both prevent‑options arefalse
In that configuration, regular signup can return an unauthenticated user without a session token. Because_autoSignupOnLogintrustsresponse.sessionTokenand setsskipSessionCreation: true, a login‑driven auto‑signup may also return a user withoutsessionToken. If that’s intentional, documenting it in the option docs would help; if not, you may want to special‑case “no sessionToken from signup” by allowinghandleLogInto create a login session instead.Neither of these is a blocker, but they’re worth verifying to ensure the new option’s behavior matches your expectations across all verification configurations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/ParseUser.spec.jssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/Routers/UsersRouter.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Options/docs.js
- src/Options/Definitions.js
🧰 Additional context used
🧠 Learnings (9)
📓 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-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.
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.
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Routers/UsersRouter.jsspec/ParseUser.spec.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/Routers/UsersRouter.jssrc/Options/index.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
src/Routers/UsersRouter.jsspec/ParseUser.spec.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/Routers/UsersRouter.jsspec/ParseUser.spec.jssrc/Options/index.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/Routers/UsersRouter.jsspec/ParseUser.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/ParseUser.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseUser.spec.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: 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/Options/index.js
🧬 Code graph analysis (2)
src/Routers/UsersRouter.js (1)
src/RestWrite.js (9)
request(823-829)request(958-964)user(330-330)user(903-903)user(1517-1517)user(1798-1798)RestWrite(990-997)RestWrite(1155-1161)sessionData(1012-1021)
spec/ParseUser.spec.js (2)
spec/helper.js (2)
reconfigureServer(191-225)Parse(4-4)spec/EmailVerificationToken.spec.js (1)
request(5-5)
🪛 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). (10)
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 22
- GitHub Check: Node 20
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (3)
src/Options/index.js (1)
196-198: autoSignupOnLogin option definition looks correct; ensure definitions/docs are regenerated.The Flow-typed field and
:DEFAULTdoc match the intended behavior and are consistent with neighboring options. Please just verify thatsrc/Options/Definitions.jsandsrc/Options/docs.jshave been updated vianpm run definitionsso this option is fully wired into configuration and docs. (The Biome parse error about?booleancan be ignored here as this file already uses Flow types.)src/Routers/UsersRouter.js (2)
64-96: Good centralization of email‑verification option handling.Extracting
_resolveEmailVerificationFlagsand reusing it in both_authenticateUserFromRequestand_autoSignupOnLoginmakes the behavior ofverifyUserEmails,preventLoginWithUnverifiedEmail, andpreventSignupWithUnverifiedEmailconsistent, including the function‑valued variants. Therequestobject passed to these callbacks mirrors existing patterns (master, ip, installationId,Parse.User.fromJSON), so this refactor should be backward compatible while reducing duplication.Also applies to: 186-193
98-129: Login payload extraction/validation helper looks correct and matches existing semantics.
_getLoginPayloadcorrectly:
- prefers
req.bodybut falls back toreq.queryif username/email are only present there,- enforces “username/email is required” and “password is required” with the existing Parse error codes, and
- rejects non‑string username/email/password with the same
OBJECT_NOT_FOUND / "Invalid username/password."behavior used elsewhere (covering the$regexinjection tests).This should keep all existing login/verifyPassword tests green while giving you a single point to adjust payload rules going forward.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseUser.spec.jssrc/Routers/UsersRouter.js
🧰 Additional context used
🧠 Learnings (8)
📓 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-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.
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.
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/ParseUser.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseUser.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/ParseUser.spec.jssrc/Routers/UsersRouter.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:
spec/ParseUser.spec.jssrc/Routers/UsersRouter.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/ParseUser.spec.jssrc/Routers/UsersRouter.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/ParseUser.spec.jssrc/Routers/UsersRouter.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/Routers/UsersRouter.js
🧬 Code graph analysis (2)
spec/ParseUser.spec.js (1)
src/TestUtils.js (1)
res(31-31)
src/Routers/UsersRouter.js (1)
src/RestWrite.js (6)
request(823-829)request(958-964)verifyUserEmails(968-968)preventLoginWithUnverifiedEmail(969-969)RestWrite(990-997)RestWrite(1155-1161)
🪛 GitHub Check: Lint
spec/ParseUser.spec.js
[failure] 393-393:
Trailing spaces not allowed
[failure] 387-387:
Trailing spaces not allowed
[failure] 380-380:
Trailing spaces not allowed
[failure] 374-374:
Trailing spaces not allowed
⏰ 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). (10)
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (9)
spec/ParseUser.spec.js (1)
266-399: Good test coverage for the newautoSignupOnLoginfeature.The test suite comprehensively covers:
- Disabled state behavior
- Username/password auto-signup
- Email/password auto-signup (with email as inferred username)
- Existing user login (no duplicate creation)
- Email verification constraints (
preventLoginWithUnverifiedEmail)- Wrong password for existing user (critical edge case that prevents account enumeration)
The tests properly validate database state after operations and use async/await patterns as recommended.
src/Routers/UsersRouter.js (8)
64-96: Good refactoring to centralize email verification flag resolution.The
_resolveEmailVerificationFlagsmethod correctly:
- Supports both boolean and async function config options (consistent with RestWrite.js patterns)
- Creates a proper request object for config callbacks
- Returns all three verification-related flags for reuse
This eliminates duplication and ensures consistent behavior across login flows.
98-129: Well-structured payload extraction and validation.The
_getLoginPayloadmethod properly:
- Falls back to query parameters when body fields are missing
- Validates required fields with appropriate error codes
- Performs type checking to prevent injection attacks (e.g., regex objects in credentials)
153-156: Good fix:userNotFoundflag addresses account enumeration concern.Adding
error.userNotFound = truewhen no user is found (vs. wrong password) allowshandleLogInto correctly distinguish:
- "User doesn't exist" → eligible for auto-signup
- "User exists but wrong password" → should NOT trigger auto-signup
This prevents the account enumeration vulnerability noted in past reviews.
266-275: Cleanup logic correctly removes both user and session on verification failure.The verification checks at lines 266-275 properly call
cleanupAutoSignup()before throwing errors, which removes both the session token AND the user record. This addresses the orphaned records concern from past reviews.Both checks intentionally throw the same
EMAIL_NOT_FOUNDerror to prevent information leakage about which specific verification constraint failed.
325-343: Correct implementation of auto-signup flow inhandleLogIn.The error handling correctly:
- Checks
error.userNotFound === truein addition toOBJECT_NOT_FOUNDcode (line 332) - prevents auto-signup for wrong password scenarios- Only triggers auto-signup when
autoSignupOnLoginconfig is enabled (line 329)- Properly propagates the session token from auto-signup result (lines 337-339)
- Re-throws non-eligible errors to maintain existing behavior (line 341)
430-443: Session creation correctly respects auto-signup flow.The conditional at line 431 ensures:
- A new session is created for normal login flows
- Session creation is skipped when auto-signup already created one (via
skipSessionCreation: true)This prevents duplicate sessions and maintains consistent behavior.
328-334: Option is properly documented.The
autoSignupOnLoginoption is correctly defined insrc/Options/index.js(line 198), with corresponding entries in the auto-generatedsrc/Options/Definitions.js(line 116) andsrc/Options/docs.js(line 25). Thenpm run definitionscommand was executed as expected.
245-258: User cleanup is correctly configured with appropriate permissions.The
cleanupAutoSignupfunction properly removes both session and user records. The use of{ acl: undefined }in the destroy calls grants master permissions as intended and is the standard Parse Server pattern for administrative deletions. No changes needed.Likely an incorrect or invalid review comment.
3 participants
Pull Request
Issue
Fixes: #9560
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.