Reviewed & tested by the community
Project:
Login History
Version:
2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
16 Sep 2021 at 10:13 UTC
Updated:
29 Jan 2025 at 19:39 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
manuel.adanMain changes:
Test coverage for this new fature to be added.
Comment #3
jcnventuraAs a new feature, this is now for the 2.x branch.
Comment #4
prashant.cPatch 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.
Comment #5
ricardopeters commentedFrom 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.Comment #6
ricardopeters commentedRerolled patch vs 2.x fixed removed controller, and fixed additions from cleanup calls.
Comment #7
ricardopeters commentedI messed up the patch of #6, sorry bout that, #7 should be fine
Comment #8
prashant.cComment #9
wjackson commentedRerolled patch #7 to resolve failures against the recent commits to the login_history.module file.
Comment #11
wjackson commentedThe 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.
Comment #12
manthan.chauhan commentedI have rerolled the patch to resolve a conflict in the update hook number.
Kindly review the updated patch.
Comment #13
prashant.cComment #14
earthday47Another 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.
Comment #15
rajdip_755Comment #16
rajdip_755Hi 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.ymlfile, 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.
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.
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 !
Comment #17
arunsahijpal commentedOkay @rajdip_755,
Looking into this.
Comment #19
arunsahijpal commentedHi @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.
Comment #20
jcnventuraPlease base it on the current state of 2.x
Comment #21
arunsahijpal commentedHi @jcnventura,
I've rebased the branch, Please have a look.
Comment #22
anish.ir commentedHii @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.
Please have a look.
Comment #23
arunsahijpal commentedThanks @anish.ir for reporting this error.
I've removed the unnecessary part of code, please check.
Comment #24
anish.ir commentedThank 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 :
I have done proper testing of the changes and everything works fine.
Comment #25
jcnventuraMR13 has a couple of .orig files that need to be removed
Comment #28
arunsahijpal commented@jcnventura,
Deleted .orig files, pls check.
Comment #29
jcnventuraThanks @arunsahijpal
I'll want to test this myself before merging, but it looks very good.
Comment #30
nathanraystone commentedAnother patch update to combine #12 and #14 so they apply cleanly.