fix: Account lockout does not reset failed login count after duration expires#10538
fix: Account lockout does not reset failed login count after duration expires#10538dblythy wants to merge 1 commit into
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughAdds a helper method to reset the failed login count and lockout expiration when a previous lockout has expired, rewrites ChangesAccount Lockout Expiry Reset
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountLockout
participant UserRecord
Client->>AccountLockout: handleLoginAttempt(loginSuccessful=false)
AccountLockout->>UserRecord: _notLocked()
UserRecord-->>AccountLockout: lockout has expired
AccountLockout->>AccountLockout: _resetFailedLoginCountIfExpired()
AccountLockout->>UserRecord: reset failed_login_count, delete lockout_expires_at
AccountLockout->>AccountLockout: _handleFailedLoginAttempt()
AccountLockout-->>Client: fresh attempt window
Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10538 +/- ##
==========================================
- Coverage 92.66% 92.66% -0.01%
==========================================
Files 193 193
Lines 16981 16988 +7
Branches 248 248
==========================================
+ Hits 15736 15742 +6
- Misses 1224 1225 +1
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/AccountLockout.js (1)
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate OBJECT_NOT_FOUND swallow-catch.
The catch block here is identical to the one in
_setLockoutExpiration(Lines 59-71). Consider extracting a small shared helper to avoid drift between the two copies.♻️ Suggested helper extraction
+ _ignoreObjectNotFound(err) { + if ( + err && + err.code && + err.message && + err.code === Parse.Error.OBJECT_NOT_FOUND && + err.message === 'Object not found.' + ) { + return; + } + throw err; + } + _setLockoutExpiration() { ... - return this._config.database.update('_User', query, updateFields).catch(err => { - if ( - err && - err.code && - err.message && - err.code === Parse.Error.OBJECT_NOT_FOUND && - err.message === 'Object not found.' - ) { - return; - } else { - throw err; - } - }); + return this._config.database + .update('_User', query, updateFields) + .catch(this._ignoreObjectNotFound); }Also applies to: 138-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AccountLockout.js` around lines 59 - 71, The OBJECT_NOT_FOUND swallow-catch logic is duplicated in AccountLockout, so extract the shared handling into a small helper and reuse it from both the update path and _setLockoutExpiration. Move the identical err.code/err.message check into a uniquely named helper near the existing methods, then have both catch blocks delegate to it and return/throw based on the helper’s result to keep the behavior in sync and avoid drift.spec/AccountLockoutPolicy.spec.js (1)
345-368: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTight timing margin risks flakiness.
The lockout expires after 3000ms, but the test only waits 3100ms — a 100ms buffer that also has to absorb the latency of the signup + two failed-login DB round trips performed beforehand. Under CI load this margin could be consumed, causing an intermittent false failure.
Consider widening the buffer (e.g., wait 4000-5000ms, or increase
durationslightly) for more headroom.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/AccountLockoutPolicy.spec.js` around lines 345 - 368, The lockout reset test in AccountLockoutPolicy.spec.js is timing-sensitive and can flake because the wait after triggering lockout is too close to the configured duration. Update the test named “allows a fresh set of attempts after the lockout duration expires” to give more headroom by either increasing the `setTimeout` wait or slightly raising the `accountLockout.duration` value, keeping the existing flow through `reconfigureServer`, `loginWithWrongCredentialsShouldFail`, and `Parse.User.logIn` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/AccountLockoutPolicy.spec.js`:
- Around line 345-368: The lockout reset test in AccountLockoutPolicy.spec.js is
timing-sensitive and can flake because the wait after triggering lockout is too
close to the configured duration. Update the test named “allows a fresh set of
attempts after the lockout duration expires” to give more headroom by either
increasing the `setTimeout` wait or slightly raising the
`accountLockout.duration` value, keeping the existing flow through
`reconfigureServer`, `loginWithWrongCredentialsShouldFail`, and
`Parse.User.logIn` unchanged.
In `@src/AccountLockout.js`:
- Around line 59-71: The OBJECT_NOT_FOUND swallow-catch logic is duplicated in
AccountLockout, so extract the shared handling into a small helper and reuse it
from both the update path and _setLockoutExpiration. Move the identical
err.code/err.message check into a uniquely named helper near the existing
methods, then have both catch blocks delegate to it and return/throw based on
the helper’s result to keep the behavior in sync and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f91ce09-fb66-4585-9665-4df77aab2097
📒 Files selected for processing (2)
spec/AccountLockoutPolicy.spec.jssrc/AccountLockout.js
1 participant
Closes #9386
Summary by CodeRabbit
Bug Fixes
Tests