add notification by email when an admin logs in from a new IP address.

[michield]

Description

This does two things.

1. ensure an admin is only logged in once
2. send a notification when a login is from an IP address that hasn't been seen before.

Related Issue

Screenshots (if appropriate):

[bramley]

A few comments after a quick skim-through the changed files

• Should all fields on the new admin_login table be "not null"?

• Should the detection of a new IP address be on a per-admin basis, i.e. a new IP address for that particular admin?

• The sending of a notification of login from a new IP address seems confusing. The function sendAdminCopy() doesn't return success/failure but even if it did, it tries to send to the getConfig('admin_address') address, so there is no point in trying to send a further email to the same address if that failed.

• I'm not sure that using sendAdminCopy() is appropriate though. It is described as "Send notifications about subscribe, update and unsubscribe". Maybe just send to the affected admin and also the system owner.

• Using empty() is often unnecessary, e.g. this can be simply if (!$enabled)

    $enabled = getConfig('notify_admin_login');
    if (empty($enabled)) {
      return;
    }

• There might be a need to view the contents of the admin_login table, otherwise would need to use a tool like phpmyadmin.
• The admin_login table won't exist until an upgrade is performed, so all the uses of the table are going to fail.

[michield]

A few comments after a quick skim-through the changed files

* Should all fields on the new `admin_login` table be "not null"?

Yes, good point, have added that.

* Should the detection of a new IP address be on a per-admin basis, i.e. a new IP address for that particular admin?

Yes, absolutely, that was an oversight.

* The sending of a notification of login from a new IP address seems confusing. The function sendAdminCopy() doesn't return success/failure but even if it did, it tries to send to the `getConfig('admin_address') `address, so there is no point in trying to send a further email to the same address if that failed.

* I'm not sure that using` sendAdminCopy()` is appropriate though. It is described as  "Send notifications about subscribe, update and unsubscribe". Maybe just send to the affected admin and also the system owner.

Yes, I agree, I'll change that.

* Using empty() is often unnecessary, e.g. this can be simply `if (!$enabled)`

    $enabled = getConfig('notify_admin_login');
    if (empty($enabled)) {
      return;
    }

Well, that's kind of cosmetic.

* There might be a need to view the contents of the admin_login table, otherwise would need to use a tool like phpmyadmin.

Yes, one to add at some point. There are more things we don't show in the UI, like the blacklist table.

* The admin_login table won't exist until an upgrade is performed, so all the uses of the table are going to fail.

Yes, I'll check that it won't block stuff like login, that would be bad.

[michield]

OK, I've made sure it doesn't break on a non-upgraded system. I tried it and in fact, it allows you to login, but then kicks you our immediately.

I've also updated the sending of the alert. I'm not actually sure if "sendEmail" returns success or failure, but if it doesn't that would be good to change. So, the second bit may never kick in, but we can review that later.

[bramley]

it allows you to login, but then kicks you our immediately.

It didn't happen for me, and you don't want that to happen otherwise the database cannot be upgraded.

One further small comment, the layout of the email is a bit untidy possibly due to line-wrapping

[michield]

Yes, the email content layout is really strange. The first one is slightly wonky, but any subsequent one is even worse

I'll try to figure out why, but sometimes life's too short ....

[michield]

Ah, the culprit is https://github.com/phpList/phplist3/blob/main/public_html/lists/admin/languages.php#L385

Not sure why we're stripping newlines out there. I wonder what effect it has to remove that.

[michield]

Ok, I've taken out the stripping of newlines. That should not matter for any other texts, as HTML ignores them anyway, so I think there's no issue with that. Also, reformatted the message a little to avoid line wrapping

[michield]

I think this is ready to merge

[utagawal]

Hello,

Upgraded to version 3.6.15 and it seems this feature is creating side effect on API connection with a superuser.
Have you tested this ?
We cannot connect anylonger to the phpList rest API from our phpBB extension. The log entry is :

Identification invalide de [IP v6 addresss] sur [ID name] (erreur OK)

[bramley]

Upgraded to version 3.6.15 and it seems this feature is creating side effect on API connection with a superuser. Have you tested this ? We cannot connect anylonger to the phpList rest API from our phpBB extension. The log entry is :

Identification invalide de [IP v6 addresss] sur [ID name] (erreur OK)

You might have the same problem as described here #1038 (comment)

A work-around is to rename or delete the admin_login table.