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

michield commented 4 months ago

Description

This does two things.

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

Related Issue

Screenshots (if appropriate):

michield commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

I think this is ready to merge

utagawal commented 2 months ago

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 commented 2 months ago

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 https://github.com/phpList/phplist3/issues/1038#issuecomment-2094089405

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

phpList / phplist3