-
Improvement
-
Resolution: Unresolved
-
Minor
-
None
-
Future Dev
Followup from MDL-66776: First I think it's cool that this feature landed and it's been on our radar for a while. The implementation has a few weaknesses which I think can all be mitigated and polished:
1) It cannot detect multiple ip's for 2+ sessions, ie a work computer and home computer so will generate regular annoying emails when it toggles back and forth. Rather than checking the lastip it should store an allowed list of ips and each ip should have its own expiry of say a month or some other configurable timeout. I don't buy the argument about fingerprinting as we are already doing this for 1 session, we just aren't doing it correctly for 2+.
2) The detection during web login is trivially circumvented by setting your user agent. The UA should never be used to say if it is sent or not, only used as information. I think the fix is a minor tweak so that web logins always queues up a notification regardless of the UA, but then if the device has logged in then it is ignored. fred raised this but it got missed.
https://github.com/moodle/moodle/blob/master/lib/moodlelib.php#L4599
3) There is a second weakness in that if you have a set of credentials and login then you have 2 minutes to go to the users notifications and turn off the notification. This notification should force sent as a raw email and not as a moodle notification. OR if it can always be sent as an email and then also sent as a notification in a way which does not re-send a second email then even better.
Alternatively if the notification is setup by default to be locked that might be ok too, but I think that this email should be treated in a similar way to an email confirm or password reset email so they have somewhat more guaranteed delivery.
4) Related to above, I think there should be a master enable disable setting for the sites which don't want it (eg school intranet), especially if it is converted to a raw email but even if it isn't.
5) Lang tweak - if your auth type doesn't allow password changes then the language text is misleading. I think there should be a second string for this, probably saying contact the site support.
https://github.com/moodle/moodle/blob/master/lib/classes/task/send_login_notifications.php#L59-L62
6) The language string doesn't show what the sites url is. I think it should show both the site url and not just the name in the subject. The first time I saw this email I thought it was spam as it wasn't obvious where it was from.
7) The user agent is raw and not human readable. I had to copy it into an online parser to turn mine into something a casual user could understand. I think it should display both the raw UA as well as the 'common name' such as 'Chrome 103 on Linux'
8) As well as showing the ip address is should also use the iplookup api in core to give a estimated location eg 'Sydney, Australia'. (Bonus points it should also link to the lookup page eg http://moodle.com/iplookup/?ip=xxx but that needs love which is already logged elsewhere)