This seems like a nice feature for the security to protect accounts in ways related to the cyber. This feature is enabled by #2820919: Add device detection/ID provided by modules: fingerprintjs2.

There are potentially a lot of scenarios. I'm going to tackle just one of them for now.

Account is created for the user - don't send an email b/c the only information we have is about the admins account, not the user.

On a first login for account:
- old device id is blank (cookie is not set)
- new device id is detected/set, no prior data for this uid in login_history
-so- don't send an email

subsequent login on a device, with device id cookie, browser has been updated:
- old device id is set and authentic
- new device id does not match - send new device id
-so- don't send an email

subsequent login on a device, cookies were cleared:
- old device id is not set/valid
- new device id matches a prior login based on querying login_history for this device_id and uid
-so- don't send an email

subsequent login, cookies are cleared or invalid:
- old device id is not set/valid
- new device id is not found in login_history for this uid but there are prior logins for this uid
-so- do send an email!

The final scenario could also happen because the person is logging in from a legitimately new device OR because an attacker has stolen their credentials. it's particularly this last scenario that we want to protect against, but we want to do that while sending as few emails as possible.

CommentFileSizeAuthor
#9 2825057_send_email_for_new_logins-9.patch9.75 KBgreggles
#7 2825057_send_email_for_new_logins-7.patch9.69 KBgreggles
#6 2825057_send_email_for_new_logins-6.patch4.64 KBgreggles
#4 2825057_send_email_for_new_logins-4.patch9.76 KBgreggles
#3 2825057_send_email_for_new_logins.patch4.66 KBgreggles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles created an issue. See original summary.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Status: Active » Needs review

I'd like to add some tests, but here's a first cut.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
FileSize
2825057_send_email_for_new_logins.patch4.66 KB
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
FileSize
2825057_send_email_for_new_logins-4.patch9.76 KB

OK, now with tests for this feature, which also happens to test much of the detection / device id code.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane at CARD.com commented
Status: Needs review » Needs work

In login_history_send_mail_new_login_device(), the 

if (!empty($old_device_id)) {..}

could be moved to the variable conditional in login_history_user_login() since if a old device ID is not valid the whole send email on new login process isn't worth calling.

You're using token [current-date:html5_tools_iso8601] in login_history_mail() which is from html5_tools module and isn't a dependency of Login History.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Status: Needs work » Needs review
FileSize
2825057_send_email_for_new_logins-6.patch4.64 KB

could be moved to the variable conditional in login_history_user_login

I debated about that too and decided to keep all the logic about when to send a mail or not in one function. Maybe the variable check should actually be moved into the function too? Maybe the logic about when to send should be split from the actual sending?

Thanks for noticing that token! I changed it to be the site's "short" date. Seem good?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
FileSize
2825057_send_email_for_new_logins-7.patch9.69 KB

The tests got dropped between comment 4 and 6.

Status: Needs review » Needs work

The last submitted patch, 7: 2825057_send_email_for_new_logins-7.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Status: Needs work » Needs review
FileSize
2825057_send_email_for_new_logins-9.patch9.75 KB
coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane at CARD.com commented
Status: Needs review » Reviewed & tested by the community

Yeah no blocker on the conditions in the function. short date sounds good.

  • greggles committed fd60ab0 on 7.x-1.x 
    Issue #2825057 by greggles: Optionally email a user if they are logging...
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for the review, Ben.

Now pushed, so moving to 8.x to be ported.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Assigned: greggles » Unassigned

I'm not likely to work on this for 8.x, so unassigning it.

Prashant.c’s picture

Prashant.c
he/him
Primary language English
Location Dharamshala
CreditAttribution: Prashant.c as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented

Do you have any plans to port this to the latest version of the module?