Problem/Motivation

Failed login attempts are also relevant in security monitoring. It would be approppriate to add such support to the Login History module.

Steps to reproduce

N/A

Proposed resolution

Add support to log login failed attempts.

Remaining tasks

Review.

User interface changes

Just the related to the new event type.

API changes

To be extended for new event type, maintaining backward compatibility.

Data model changes

Extend existing one-time field to register the new type of event.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

manuel.adan created an issue. See original summary.

manuel.adan’s picture

Assigned: manuel.adan » Unassigned
Status: Active » Needs review
StatusFileSize
new11.28 KB

Main changes:

  • existing "one-time" field is renamed as "event type", adding a new value for the failed login state
  • hook_update changes DB schema without existing data losing and also updates existing views using the one-time field

Test coverage for this new fature to be added.

jcnventura’s picture

Version: 8.x-1.x-dev » 2.x-dev

As a new feature, this is now for the 2.x branch.

prashant.c’s picture

Version: 2.x-dev » 2.0.0-alpha1
StatusFileSize
new11.12 KB

Patch not applying on the latest tag/branch. In the Default branch (2.x) "Controller" does not exist. Cloned the repo and worked on tag 2.0.0-alpha1 to make the changes. Submitting the re-rolled patch requires reviews.

ricardopeters’s picture

From a security perspective, wouldn't it be interesting to see if people are trying with credentials that maybe don't have existing accounts, for instance former editors or admin/root attacks?

Seeing this comment:
// Only register failed login attempts for existing accounts.

ricardopeters’s picture

Version: 2.0.0-alpha1 » 2.x-dev
StatusFileSize
new26.54 KB

Rerolled patch vs 2.x fixed removed controller, and fixed additions from cleanup calls.

ricardopeters’s picture

StatusFileSize
new9.49 KB

I messed up the patch of #6, sorry bout that, #7 should be fine

prashant.c’s picture

wjackson’s picture

Rerolled patch #7 to resolve failures against the recent commits to the login_history.module file.

wjackson’s picture

The previous patch created an issue where when a failed login attempt was recorded, it used the time of the last successful log rather than the time of the failed login attempt. Additionally, the source branch was a bit out of date.

This branch should include the most recent changes from the 2.x version of the login_history module, the changes from the patch RicardoPeters previously provided in #7/#9, and the change to record the time of the failed login.

The attached patch was created from the merge request referenced in #10.

manthan.chauhan’s picture

I have rerolled the patch to resolve a conflict in the update hook number.
Kindly review the updated patch.

prashant.c’s picture

earthday47’s picture

Another tweak to the patch - if a previous version of the patch had been run already, it will throw an error. Wrapping the schema update in a fieldExists() to prevent error.

rajdip_755’s picture

Assigned: Unassigned » rajdip_755
rajdip_755’s picture

Assigned: rajdip_755 » Unassigned
Status: Needs review » Needs work
StatusFileSize
new97.27 KB

Hi all, I've reviewed the patch attached in #14 and #13.

After applying the patch #14 the login history view is not working due to the missing/broken handler error as there is no changes in the views.view.login_history.yml file, so we can conclude that this patch is incompatible to provide the solutions for this feature request.

For the patch #13, it working fine for me, and the failed login are listed successfully for me. But there are some pointers I want to mention for this patch.

  • I think it will be better if we can change the label of Event Type to "Login Type".
  • If the users are trying to login with the correct email ID and failed to login due to the wrong password, then it's should be marked and
    listed as failed login but currently it's not happening as the logic is implemented just for the user name in case of failed login listing ( implemented in the function _login_history_form_user_login_validate() ).

Attached here the screenshot of the login history view page below.

Login-history-page-after-patch-13

I'm moving this issue to Needs Work state. Please share your thoughts as well on the pointers I mentioned for the patch #13.

Thanks !

arunsahijpal’s picture

Assigned: Unassigned » arunsahijpal

Okay @rajdip_755,
Looking into this.

arunsahijpal’s picture

Assigned: arunsahijpal » Unassigned
Status: Needs work » Needs review

Hi @rajdip_755 and @manuel.adan
I've done the changes,
Now it is showing Failed attempt even if we enter email and incorrect password. Please have a look.

jcnventura’s picture

Status: Needs review » Needs work

Please base it on the current state of 2.x

arunsahijpal’s picture

Status: Needs work » Needs review

Hi @jcnventura,
I've rebased the branch, Please have a look.

anish.ir’s picture

Status: Needs review » Needs work

Hii @arunsahijpal,

Everything works fine as mentioned. But the function "login_history_update_8004" in login_history.install file is declared twice.

Here is the error for reference.

PHP Fatal error:  Cannot redeclare login_history_update_8004() (previously declared in /app/web/modules/custom/login_history-3233477/login_history.install:111) in /app/web/modules/custom/login_history-3233477/login_history.install on line 154

Fatal error: Cannot redeclare login_history_update_8004() (previously declared in /app/web/modules/custom/login_history-3233477/login_history.install:111) in /app/web/modules/custom/login_history-3233477/login_history.install on line 154

Please have a look.

arunsahijpal’s picture

Status: Needs work » Needs review

Thanks @anish.ir for reporting this error.
I've removed the unnecessary part of code, please check.

anish.ir’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @arunsahijpal for making the changes.

Everything looks fine as per the requirements.
The above MR consists the patch changes too mentioned in #14, which has added the below functionalities :

  • The failed login attempts are now being added to the database schema, in "login_history" table, where the validation is being done by the username and password. Also the entries are being properly managed (deleting user deletes the related entries).
  • The label of Event Type is changed to "Login Type".
  • The validation for unsuccessful email login attempt has also been added to the module, and not only for username.

I have done proper testing of the changes and everything works fine.

jcnventura’s picture

Status: Reviewed & tested by the community » Needs work

MR13 has a couple of .orig files that need to be removed

jcnventura changed the visibility of the branch 3233477-register-failed-login to hidden.

arunsahijpal’s picture

Status: Needs work » Needs review

@jcnventura,
Deleted .orig files, pls check.

jcnventura’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @arunsahijpal

I'll want to test this myself before merging, but it looks very good.

nathanraystone’s picture

Another patch update to combine #12 and #14 so they apply cleanly.