Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block user accounts if an incorrect password was entered 5 times #527

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

YunhwanJeong
Copy link

@YunhwanJeong YunhwanJeong commented Sep 10, 2024

Feature Overview:

  • This PR is an implementation of Block user accounts if an incorrect password was entered 5 times #519.
  • This PR introduces a login activity tracking system that monitors incorrect password attempts per user. If the number of failed attempts exceeds a predefined limit, the user’s account is locked, preventing further login attempts. A successful login resets the failed attempts counter, enhancing account security and preventing unauthorized access.
Screenshot 2024-09-09 at 11 26 04 PM

Key Changes:

  1. Login Activity Tracking:
    • Introduced a new user_login_activity table to store login-related activities, including failed attempts and account lock status.
    • Added a new service (src/login-activity/service.ts) to handle the logic related to tracking failed login attempts and locking accounts.
  2. Account Locking Logic:
    • Implemented a maximum failed attempts limit. If the login attempt is made when the account is locked, increment the failed_login_attempts and create log entry with the loginFailedAccountLocked event.
    • If the limit is reached, the account is immediately locked, and a log entry is created with the accountLocked event.
    • Users with locked accounts are presented with an appropriate error message, instructing them to contact the administrator to unlock their account.
  3. Resetting Failed Attempts:
    • After a successful login, the failed attempts counter is reset to ensure that future incorrect password attempts are tracked accurately.
  4. Controller Enhancements:
    • Updated the post method in the login controller to incorporate the new login activity tracking and account locking logic.
    • Ensured that the resetFailedLoginAttempts function is executed after a successful login to maintain data integrity.
  5. Logging:
    • Enhanced logging to include new events such as loginFailedAccountLocked and accountLocked, providing better visibility into account security incidents.

Security Improvements:

This feature improves the security of user accounts by preventing brute-force attacks and unauthorized access through repeated incorrect password attempts.

Testing & Validation:

  • Manual testing was conducted to validate the correct behavior under various scenarios, including successful logins, incorrect password attempts, and account lockouts.

Next Steps:

  • Should monitor the feature in production to ensure it behaves as expected and doesn’t introduce any performance overhead.
  • Consider implementing an admin feature like resetting the failed_login_attempts and account_locked easily.
  • Consider implementing additional security features such as IP-based throttling and suspicious login detection in future iterations.

Checklist

  • Performed a self-review.
  • Tested manually.
  • Added tests.

- accountLocked: log when the account is locked.
- loginFailedAccountLocked: log if the login fails while the account is locked.
- Considered race condition in the incrementFailedLoginAttempts and the ensureUserLoginActivityRecord functions.
Copy link
Member

@evert evert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is a great contribution!

Happy to chat about my feedback. They're loosely held opionions

Comment on lines +16 to +17
accountLocked = 13,
loginFailedAccountLocked = 14,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth adding some comments to these 2 lines so it's a bit clearer what the difference is.

await services.loginActivity.incrementFailedLoginAttempts(user);
log(EventType.loginFailedAccountLocked, ctx.ip(), user.id, ctx.request.headers.get('User-Agent'));
return this.redirectToLogin(ctx, '', `Too many failed login attempts, please contact ${requireSetting('smtp.emailFrom')} to unlock your account.`);
}

if (!await services.user.validatePassword(user, ctx.request.body.password)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a larger request, but I wonder if most of this logic should go into the service. The reason is that this is not the only place where we check a username and password, there are two more places:

  • OAuth2 has a password flow
  • I recently added a 'authorization_challenge` OAuth2 flow.

These 3 features all do a username/password check so it kinda makes sense that they all increment the 'failed login' counter if a password fails.

I'm not sure if this should just be part of 'validatePassword' or a new function.

const MAX_FAILED_ATTEMPTS = 5;

export function reachedMaxAttempts(attempts: number) {
return attempts === MAX_FAILED_ATTEMPTS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be >= instead of ===

@@ -0,0 +1,65 @@
import { UserLoginActivityRecord } from 'knex/types/tables.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is login-attempts a better name for this?

return attempts === MAX_FAILED_ATTEMPTS;
}

async function getLoginActivity(user: User): Promise<UserLoginActivityRecord | undefined> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning undefined, how about always returning a record? Because maybe if there's no database record yet, you can guess what the record should be for the initial state

export async function incrementFailedLoginAttempts(user: User): Promise<number> {
await ensureUserLoginActivityRecord(user);

return await db.transaction(async (trx) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants