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

fix: Only update user.last_login on successful authentication #1775

Merged
merged 4 commits into from
Jan 13, 2022

Conversation

blag
Copy link
Contributor

@blag blag commented Jan 7, 2022

Description

This PR moves one line around so the user.last_login field is only updated when the user has successfully authenticated.

Without this PR the user.last_login field is not very useful, as an attacker trying to brute force a login would continuously update the last_login field to the datetime of the latest unsuccessful authentication attempt, instead of the datetime of the last successful authentication attempt. I believe that "login" usually refers to a successful authentication attempt, so this keeps the behavior of the code consistent with the semantics of the field name.

I don't have a thorough understanding of what all user.last_login could be/is used for, but I can imagine that, when it records the last datetime of successful authentication attempt, it can be combined with user.fail_login_count to generate an average "brute force rate" for each user between successful logins. Without this PR, it is impossible to collect that data because the user.last_login will almost always be set to the datetime of the latest brute force attempt.

I have expanded the docstring to have a more thorough description of the method, and I have added tests for each line and branch of the BaseSecurityManager.update_user_auth_stat method so it is now completely covered by tests.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

@dpgaspar
Copy link
Owner

dpgaspar commented Jan 7, 2022

Agree, thank you @blag

@kaxil
Copy link

kaxil commented Jan 12, 2022

Can we merge this one @dpgaspar :)

@dpgaspar dpgaspar merged commit e2b744c into dpgaspar:master Jan 13, 2022
@blag blag deleted the fix-last-login branch April 27, 2022 22:33
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.

3 participants