feat: Add beforePasswordResetRequest hook
#9906
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! |
coratgerl
changed the title
beforePasswordResetRequest hook
✅ 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. |
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
beforePasswordResetRequest hookbeforePasswordResetRequest hook
📝 WalkthroughWalkthroughAdds a beforePasswordResetRequest Cloud trigger: registers the trigger type, exposes Parse.Cloud.beforePasswordResetRequest(...), invokes it during password reset requests in UsersRouter, and adds tests covering flows, validation, and request context. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant UsersRouter as UsersRouter\n(handleResetRequest)
participant Triggers as TriggerRegistry
participant Hook as beforePasswordResetRequest\nHandler
participant Email as EmailAdapter
Client->>UsersRouter: POST /requestPasswordReset (email or token)
UsersRouter->>UsersRouter: Lookup user (token or email/username)
UsersRouter->>UsersRouter: Sanitize auth data & expand attached files
UsersRouter->>Triggers: Invoke beforePasswordResetRequest(req with inflated user)
Triggers->>Hook: Call handler(req)
alt Handler allows
Hook-->>Triggers: Return success
Triggers-->>UsersRouter: Continue flow
UsersRouter->>Email: sendPasswordResetEmail(user)
Email-->>Client: 200 OK
else Handler throws
Hook-->>Triggers: Throw error
Triggers-->>UsersRouter: Propagate error
UsersRouter-->>Client: Error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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 |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9906 +/- ##
==========================================
- Coverage 93.06% 93.05% -0.01%
==========================================
Files 187 187
Lines 15187 15209 +22
Branches 177 177
==========================================
+ Hits 14134 14153 +19
- Misses 1041 1044 +3
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (5)
src/triggers.js (1)
62-66: Validation updated to restrict to _User — OK; consider throwing Error objects.Logic is correct; throwing strings matches existing pattern but is brittle. Consider new Parse.Error for future cleanup.
src/Routers/UsersRouter.js (1)
449-474: Minor: reduce variables and narrow scope.userResults is only used to grab the first element. Simplify to a single const userRecord to reduce cognitive load.
src/cloud-code/Parse.Cloud.js (1)
352-394: Add validator wiring and validate input (parity + future-proofing).Current implementation mirrors beforeLogin (rateLimit only). If you want validator features (skipWithMasterKey, requireMaster, etc.) or just to validate rateLimit shape, call validateValidator and pass validationHandler to addTrigger.
ParseCloud.beforePasswordResetRequest = function (handler, validationHandler) { let className = '_User'; if (typeof handler === 'string' || isParseObjectConstructor(handler)) { // validation will occur downstream, this is to maintain internal // code consistency with the other hook types. className = triggers.getClassName(handler); handler = arguments[1]; validationHandler = arguments.length >= 2 ? arguments[2] : null; } - triggers.addTrigger(triggers.Types.beforePasswordResetRequest, className, handler, Parse.applicationId); + validateValidator(validationHandler); + triggers.addTrigger( + triggers.Types.beforePasswordResetRequest, + className, + handler, + Parse.applicationId, + validationHandler + ); if (validationHandler && validationHandler.rateLimit) { addRateLimit( { requestPath: `/requestPasswordReset`, requestMethods: 'POST', ...validationHandler.rateLimit }, Parse.applicationId, true ); } };Also consider documenting the optional validationHandler param in the JSDoc.
spec/CloudCode.spec.js (2)
3782-3978: Prefer async/await without done in new tests to reduce flakiness.The new tests use
async done => { ...; done(); }. Dropdoneand rely on async function completion/throws instead. This is the repository’s preferred pattern. Based on learnings.
3800-3816: Add an assertion that password/authData are not exposed to Cloud Code.After the UsersRouter sanitization fix, add a quick check:
Parse.Cloud.beforePasswordResetRequest(req => { // existing expectations... + expect(req.object.get('password')).toBeUndefined(); + expect(req.object.get('authData')).toBeUndefined(); });Helps prevent regressions.
Also applies to: 3836-3859, 3878-3904, 3932-3962
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
spec/CloudCode.spec.js(2 hunks)src/Routers/UsersRouter.js(2 hunks)src/cloud-code/Parse.Cloud.js(1 hunks)src/triggers.js(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/CloudCode.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/CloudCode.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/CloudCode.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/CloudCode.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/CloudCode.spec.js
🧬 Code graph analysis (3)
src/Routers/UsersRouter.js (1)
src/triggers.js (2)
inflate(1002-1008)maybeRunTrigger(895-998)
src/cloud-code/Parse.Cloud.js (2)
src/Routers/CloudCodeRouter.js (1)
triggers(4-4)src/middlewares.js (2)
addRateLimit(520-623)addRateLimit(520-623)
spec/CloudCode.spec.js (2)
spec/helper.js (2)
Parse(4-4)reconfigureServer(180-214)src/cloud-code/Parse.Cloud.js (1)
emailAdapter(604-604)
⏰ 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). (14)
- GitHub Check: Docker Build
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Node 20
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Redis Cache
🔇 Additional comments (2)
src/triggers.js (2)
9-9: New trigger type added correctly.Entry in Types for beforePasswordResetRequest looks consistent and non-breaking.
291-296: Context exposure includes beforePasswordResetRequest — LGTM.Request.context now set for this trigger type; aligns with other auth-related hooks.
| userResults = await req.config.database.find('_User', { | ||
| _perishable_token: token, | ||
| _perishable_token_expires_at: { $lt: Parse._encode(new Date()) }, | ||
| }); | ||
| if (results && results[0] && results[0].email) { | ||
| email = results[0].email; | ||
| if (userResults && userResults.length > 0) { | ||
| userData = userResults[0]; | ||
| if (userData.email) { | ||
| email = userData.email; | ||
| } | ||
| } | ||
| // Or using email if no token provided | ||
| } else if (typeof email === 'string') { | ||
| userResults = await req.config.database.find( | ||
| '_User', | ||
| { $or: [{ email }, { username: email, email: { $exists: false } }] }, | ||
| { limit: 1 }, | ||
| Auth.maintenance(req.config) | ||
| ); |
There was a problem hiding this comment.
Token lookup uses inverted expiry comparator and lacks Auth.maintenance/limit.
- _perishable_token_expires_at should be greater than “now” for a valid token.
- Use Auth.maintenance like the email path for consistency.
- Limit results to 1.
Apply:
- userResults = await req.config.database.find('_User', {
- _perishable_token: token,
- _perishable_token_expires_at: { $lt: Parse._encode(new Date()) },
- });
+ userResults = await req.config.database.find(
+ '_User',
+ {
+ _perishable_token: token,
+ _perishable_token_expires_at: { $gt: Parse._encode(new Date()) },
+ },
+ { limit: 1 },
+ Auth.maintenance(req.config)
+ );🤖 Prompt for AI Agents
In src/Routers/UsersRouter.js around lines 454 to 471, the token lookup query
uses the wrong expiry comparator and is missing the same options as the email
path; change the _perishable_token_expires_at check to $gt Parse._encode(new
Date()) so only unexpired tokens match, and call req.config.database.find with
the same options as the email branch: pass { limit: 1 } and
Auth.maintenance(req.config) as the query options so the lookup is limited to
one result and runs under maintenance context.
There was a problem hiding this comment.
@mtrezza an opininion since this code already exists before my modifcations ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Note that I proposed the name
Maybe this would be more intuitive:
Then again, a password reset could be sent via other channels in the future, so maybe we stick with this:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
spec/CloudCode.spec.js (3)
3623-3675: Consider modernizing test patternThis test correctly validates that
afterFindhooks are not triggered during save operations. However, consider converting fromasync donecallback pattern to pureasync/await:Based on learnings
Apply this diff:
- it('afterFind should not be triggered when saving an object', async () => { + it('afterFind should not be triggered when saving an object', async () => { let beforeSaves = 0; Parse.Cloud.beforeSave('SavingTest', () => { beforeSaves++; }); // ... rest of test logic ... expect(beforeSaves).toEqual(3); expect(afterSaves).toEqual(3); expect(beforeFinds).toEqual(1); expect(afterFinds).toEqual(1); - done(); });
3836-3870: Test logic is correct; consider modernizing patternThe test properly validates that
beforePasswordResetRequestruns with a valid user and that the email is sent. However, similar to other tests in this suite, consider removing thedonecallback and using pure async/await pattern.Based on learnings
3959-3983: Improve error handling specificityThe try/catch block swallows all errors, which could mask unexpected failures. Consider testing both scenarios explicitly or at least validating the error type when one is thrown.
Apply this diff to make the test more explicit:
it('should not run beforePasswordResetRequest if email does not exist', async done => { let hit = 0; const emailAdapter = { sendVerificationEmail: () => Promise.resolve(), sendPasswordResetEmail: () => {}, sendMail: () => {}, }; await reconfigureServer({ emailAdapter: emailAdapter, publicServerURL: 'http://localhost:8378/1', + passwordPolicy: { + resetPasswordSuccessOnInvalidEmail: true + } }); Parse.Cloud.beforePasswordResetRequest(req => { hit++; }); - try { - await Parse.User.requestPasswordReset('nonexistent@parse.com'); - } catch (e) { - // May or may not throw depending on passwordPolicy.resetPasswordSuccessOnInvalidEmail - } + // Should not throw when resetPasswordSuccessOnInvalidEmail is true + await Parse.User.requestPasswordReset('nonexistent@parse.com'); expect(hit).toBe(0); done(); });Alternatively, add a second test for the case where
resetPasswordSuccessOnInvalidEmailis false to ensure comprehensive coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/CloudCode.spec.js(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/CloudCode.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/CloudCode.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/CloudCode.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/CloudCode.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/CloudCode.spec.js
🧬 Code graph analysis (1)
spec/CloudCode.spec.js (1)
spec/helper.js (2)
Parse(4-4)reconfigureServer(180-214)
🪛 GitHub Check: Lint
spec/CloudCode.spec.js
[warning] 4034-4034:
More than 2 blank lines 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). (5)
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (3)
spec/CloudCode.spec.js (3)
3310-3322: LGTM: Trigger validation messages updated correctlyThe error messages have been properly updated to include the new
beforePasswordResetRequesttrigger alongsidebeforeLoginandafterLogin. The changes are consistent across all validation checks.
3872-3957: LGTM: Blocking behavior well-testedThese tests properly validate that
beforePasswordResetRequestcan block password reset requests by throwing errors. The inclusion of a test with an attached file is important as it verifies that file expansion occurs before the hook invocation, preventing unnecessary token generation and email sending (as noted in the PR objectives).
3985-4030: LGTM: Request data and class validation testsThese tests properly validate that:
- The hook receives all expected request fields (object, headers, ip, installationId, context, config)
- Only the
_Userclass is allowed forbeforePasswordResetRequest, consistent with other user-authentication triggersThe test patterns match those used for
beforeLoginand other triggers, ensuring consistency across the test suite.
|
I think |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
spec/CloudCode.spec.js (1)
4659-4659: Remove extra blank lines.More than 2 blank lines are not allowed before the new
describeblock. Please remove the extra blank line to comply with the project's style guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/CloudCode.spec.js(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/CloudCode.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/CloudCode.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/CloudCode.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/CloudCode.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/CloudCode.spec.js
🧬 Code graph analysis (1)
spec/CloudCode.spec.js (2)
spec/helper.js (2)
Parse(4-4)reconfigureServer(180-214)src/cloud-code/Parse.Cloud.js (1)
emailAdapter(604-604)
🔇 Additional comments (1)
spec/CloudCode.spec.js (1)
3310-3322: LGTM!The trigger validation error messages have been correctly updated to include
beforePasswordResetRequestin the list of user-specific triggers. The changes are consistent across all test assertions.
| describe('beforePasswordResetRequest hook', () => { | ||
| it('should run beforePasswordResetRequest with valid user', async done => { | ||
| let hit = 0; | ||
| let sendPasswordResetEmailCalled = false; | ||
| const emailAdapter = { | ||
| sendVerificationEmail: () => Promise.resolve(), | ||
| sendPasswordResetEmail: () => { | ||
| sendPasswordResetEmailCalled = true; | ||
| }, | ||
| sendMail: () => {}, | ||
| }; | ||
|
|
||
| await reconfigureServer({ | ||
| appName: 'test', | ||
| emailAdapter: emailAdapter, | ||
| publicServerURL: 'http://localhost:8378/1', | ||
| }); | ||
|
|
||
| Parse.Cloud.beforePasswordResetRequest(req => { | ||
| hit++; | ||
| expect(req.object).toBeDefined(); | ||
| expect(req.object.get('email')).toEqual('test@parse.com'); | ||
| expect(req.object.get('username')).toEqual('testuser'); | ||
| }); | ||
|
|
||
| const user = new Parse.User(); | ||
| user.setUsername('testuser'); | ||
| user.setPassword('password'); | ||
| user.set('email', 'test@parse.com'); | ||
| await user.signUp(); | ||
|
|
||
| await Parse.User.requestPasswordReset('test@parse.com'); | ||
| expect(hit).toBe(1); | ||
| expect(sendPasswordResetEmailCalled).toBe(true); | ||
| done(); | ||
| }); | ||
|
|
||
| it('should be able to block password reset request if an error is thrown', async done => { | ||
| let hit = 0; | ||
| let sendPasswordResetEmailCalled = false; | ||
| const emailAdapter = { | ||
| sendVerificationEmail: () => Promise.resolve(), | ||
| sendPasswordResetEmail: () => { | ||
| sendPasswordResetEmailCalled = true; | ||
| }, | ||
| sendMail: () => {}, | ||
| }; | ||
|
|
||
| await reconfigureServer({ | ||
| appName: 'test', | ||
| emailAdapter: emailAdapter, | ||
| publicServerURL: 'http://localhost:8378/1', | ||
| }); | ||
|
|
||
| Parse.Cloud.beforePasswordResetRequest(req => { | ||
| hit++; | ||
| if (req.object.get('isBanned')) { | ||
| throw new Error('banned account'); | ||
| } | ||
| }); | ||
|
|
||
| const user = new Parse.User(); | ||
| user.setUsername('banneduser'); | ||
| user.setPassword('password'); | ||
| user.set('email', 'banned@parse.com'); | ||
| await user.signUp(); | ||
| await user.save({ isBanned: true }); | ||
|
|
||
| try { | ||
| await Parse.User.requestPasswordReset('banned@parse.com'); | ||
| throw new Error('should not have sent password reset email.'); | ||
| } catch (e) { | ||
| expect(e.message).toBe('banned account'); | ||
| } | ||
| expect(hit).toBe(1); | ||
| expect(sendPasswordResetEmailCalled).toBe(false); | ||
| done(); | ||
| }); | ||
|
|
||
| it('should be able to block password reset request if an error is thrown even if the user has an attached file', async done => { | ||
| let hit = 0; | ||
| let sendPasswordResetEmailCalled = false; | ||
| const emailAdapter = { | ||
| sendVerificationEmail: () => Promise.resolve(), | ||
| sendPasswordResetEmail: () => { | ||
| sendPasswordResetEmailCalled = true; | ||
| }, | ||
| sendMail: () => {}, | ||
| }; | ||
|
|
||
| await reconfigureServer({ | ||
| appName: 'test', | ||
| emailAdapter: emailAdapter, | ||
| publicServerURL: 'http://localhost:8378/1', | ||
| }); | ||
|
|
||
| Parse.Cloud.beforePasswordResetRequest(req => { | ||
| hit++; | ||
| if (req.object.get('isBanned')) { | ||
| throw new Error('banned account'); | ||
| } | ||
| }); | ||
|
|
||
| const user = new Parse.User(); | ||
| user.setUsername('banneduser2'); | ||
| user.setPassword('password'); | ||
| user.set('email', 'banned2@parse.com'); | ||
| await user.signUp(); | ||
| const base64 = 'V29ya2luZyBhdCBQYXJzZSBpcyBncmVhdCE='; | ||
| const file = new Parse.File('myfile.txt', { base64 }); | ||
| await file.save(); | ||
| await user.save({ isBanned: true, file }); | ||
|
|
||
| try { | ||
| await Parse.User.requestPasswordReset('banned2@parse.com'); | ||
| throw new Error('should not have sent password reset email.'); | ||
| } catch (e) { | ||
| expect(e.message).toBe('banned account'); | ||
| } | ||
| expect(hit).toBe(1); | ||
| expect(sendPasswordResetEmailCalled).toBe(false); | ||
| done(); | ||
| }); | ||
|
|
||
| it('should not run beforePasswordResetRequest if email does not exist', async done => { | ||
| let hit = 0; | ||
| const emailAdapter = { | ||
| sendVerificationEmail: () => Promise.resolve(), | ||
| sendPasswordResetEmail: () => {}, | ||
| sendMail: () => {}, | ||
| }; | ||
|
|
||
| await reconfigureServer({ | ||
| emailAdapter: emailAdapter, | ||
| publicServerURL: 'http://localhost:8378/1', | ||
| }); | ||
|
|
||
| Parse.Cloud.beforePasswordResetRequest(req => { | ||
| hit++; | ||
| }); | ||
|
|
||
| try { | ||
| await Parse.User.requestPasswordReset('nonexistent@parse.com'); | ||
| } catch (e) { | ||
| // May or may not throw depending on passwordPolicy.resetPasswordSuccessOnInvalidEmail | ||
| } | ||
| expect(hit).toBe(0); | ||
| done(); | ||
| }); | ||
|
|
||
| it('should have expected data in request in beforePasswordResetRequest', async done => { | ||
| const emailAdapter = { | ||
| sendVerificationEmail: () => Promise.resolve(), | ||
| sendPasswordResetEmail: () => {}, | ||
| sendMail: () => {}, | ||
| }; | ||
|
|
||
| await reconfigureServer({ | ||
| appName: 'test', | ||
| emailAdapter: emailAdapter, | ||
| publicServerURL: 'http://localhost:8378/1', | ||
| }); | ||
|
|
||
| Parse.Cloud.beforePasswordResetRequest(req => { | ||
| expect(req.object).toBeDefined(); | ||
| expect(req.object.get('email')).toBeDefined(); | ||
| expect(req.headers).toBeDefined(); | ||
| expect(req.ip).toBeDefined(); | ||
| expect(req.installationId).toBeDefined(); | ||
| expect(req.context).toBeDefined(); | ||
| expect(req.config).toBeDefined(); | ||
| }); | ||
|
|
||
| const user = new Parse.User(); | ||
| user.setUsername('testuser2'); | ||
| user.setPassword('password'); | ||
| user.set('email', 'test2@parse.com'); | ||
| await user.signUp(); | ||
| await Parse.User.requestPasswordReset('test2@parse.com'); | ||
| done(); | ||
| }); | ||
|
|
||
| it('should validate that only _User class is allowed for beforePasswordResetRequest', () => { | ||
| expect(() => { | ||
| Parse.Cloud.beforePasswordResetRequest('SomeClass', () => { }); | ||
| }).toThrow('Only the _User class is allowed for the beforeLogin, afterLogin, and beforePasswordResetRequest triggers'); | ||
| expect(() => { | ||
| Parse.Cloud.beforePasswordResetRequest(() => { }); | ||
| }).not.toThrow(); | ||
| expect(() => { | ||
| Parse.Cloud.beforePasswordResetRequest('_User', () => { }); | ||
| }).not.toThrow(); | ||
| expect(() => { | ||
| Parse.Cloud.beforePasswordResetRequest(Parse.User, () => { }); | ||
| }).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor tests to use async/await instead of callback pattern.
The test suite provides excellent coverage of the beforePasswordResetRequest hook functionality. However, tests should use async/await with promise-based patterns rather than callback patterns with done(), which is the preferred pattern for new tests in this repository.
Based on learnings.
Apply this refactor pattern to tests 1-5 (test 6 is already correct):
Example refactor for test 1:
- it('should run beforePasswordResetRequest with valid user', async done => {
+ it('should run beforePasswordResetRequest with valid user', async () => {
let hit = 0;
let sendPasswordResetEmailCalled = false;
const emailAdapter = {
sendVerificationEmail: () => Promise.resolve(),
sendPasswordResetEmail: () => {
sendPasswordResetEmailCalled = true;
},
sendMail: () => {},
};
await reconfigureServer({
appName: 'test',
emailAdapter: emailAdapter,
publicServerURL: 'http://localhost:8378/1',
});
Parse.Cloud.beforePasswordResetRequest(req => {
hit++;
expect(req.object).toBeDefined();
expect(req.object.get('email')).toEqual('test@parse.com');
expect(req.object.get('username')).toEqual('testuser');
});
const user = new Parse.User();
user.setUsername('testuser');
user.setPassword('password');
user.set('email', 'test@parse.com');
await user.signUp();
await Parse.User.requestPasswordReset('test@parse.com');
expect(hit).toBe(1);
expect(sendPasswordResetEmailCalled).toBe(true);
- done();
});Apply the same pattern to tests 2-5 by removing async done =>, removing the done() calls at the end, and ensuring all async operations use await.
🤖 Prompt for AI Agents
In spec/CloudCode.spec.js around lines 4660 to 4856, tests 1–5 for
beforePasswordResetRequest currently use the callback-style "async done =>" and
explicit done() calls; refactor each of those five tests to use promise-style
async/await by removing the done parameter and all done() invocations, ensure
every asynchronous call is awaited (e.g., reconfigureServer, user.signUp,
user.save, Parse.User.requestPasswordReset), and remove any try/catch that only
exists to call done(); keep existing assertions and control-flow (throwing on
unexpected success) but rely on async/await to surface failures.
|
@coratgerl Yes, I think if new channel are introduced, we don't want a separate trigger, but just a parameter that indicates the channel for example. So I'd stick with:
Which is what you currently have, so I think we're good, right? |
|
Yes seems logic for me |
3 participants
Issue
Fixes: #9510
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests