Rough intentions here:

Mail Engine: Move this mail to PhabricatorPeopleMailEngine, mostly for consistency. I don't think there's a huge structural advantage to doing this today, it's just a little cleaner.

One issue is that PeopleMailEngine takes setRecipient(PhabricatorUser $user), and we want to send this mail to the exact (verified) address the user specifies, even if it is not their primary address. One reason for this is consistency with user expectations (they typed x@y.com into the box, and aren't expecting mail at primary@company.com). Another reason is to support the use case where Google/Gmail are down and you're intentionally using this flow to bypass Google OAuth + Gmail primary email. We can likely add setRecipient(PhabricatorUser $user, PhutilEmailAddress $explicit_address = null) to force delivery to a particular address.

Mail Messaging: The mail itself is pretty bare-bones, and providing an explanation and remedy ("ignore this") seem reasonable. We can turn this into an auth message now.

Rate Limiting: This is an older endpoint with no SystemAction rate limiting. We can fairly easily add limits to:

• the rate at which a given remote address may request email logins; and
• the rate at which a given email address may have email logins generated.

These can both be set fairly low, e.g. a handful per hour.

Currently, most callers to SystemAction pass PHIDs, but SystemAction explicitly supports non-user/non-PHID actors, and the test in PhabricatorMailEmailEngine uses email addresses. Here, we'd use the user remote address for the first check, and the target email address for the second check. It may be useful to provide helper methods to support normalizing these actor types (e.g., email addresses should be case-insensitive).

We have two older calls to PhabricatorUserLog::loadRecentEventsFromThisIP(...), which is a legacy rate limiter which predates SystemAction. It would be nice to update at least the one in PhabricatorPasswordAuthProvider, partly for consistency and partly because it's not obvious that setting the user.logs GC to a very low setting like "1 second" means both "do not retain logs" and also means "effectively, do not rate limit password login attempts from the same remote address".

User Activity Logs: We should include these login link requests in the user activity logs.

I would generally like to move toward a world where the activity logs serve a security-audit type of role (e.g., login failures, password resets) and administrative actions (make administrator, create account, disable/enable user, etc) are in the timeline on the user profile instead. Particularly, these actions probably should vanish from the activity log eventually:

Create Account
Edit Account
Add/Remove Administrator
Add/Remove System Agent
Add/Remove Mailing List
Enable/Disable
Approve Registration
Delete User
Change Username

Other than "Create Account", these are all cases where an administrator is clicking a button on the user's profile (or some similar approximation). These events are more important to preserve and less likely to be relevant in a security/audit context than things like login failures.

Many of these are already on user profiles and maybe we can take some steps here toward cleaning this up alongside adding the new event.

Including Remote Address Information in Email: I'm somewhat on the fence about this. I'm not sure it's actually useful to users -- what are they realistically going to do with the information? Sometimes this kind of email will include the UA string too, but this is likewise probably not terribly useful, and I can just imagine the HackerOne report where an enterprising "researcher" has "discovered" that they can inject content into this email by crafting a clever UA string. For sophisticated users the remote address and UA string are probably not harmful but I'm not sure they're helpful, and for unsophisticated users they might be confusing.

Maybe a half-step toward this is to tie activity log events into email via PeopleMailEngine, so the mail can say some variation of "Internal Log Reference Number: 12345". Administrators can go look up that activity log entry by ID to pull the exact address (and UA or whatever else we store) information if a user reports the activity as suspicious. Installs which really want remote address information in the email would have a short path to a patch, or we could do this upstream in the future. This would also give us a path to putting a [ This is suspicious, report it! ] button in the actual email, which could take the user to /people/logs/1234/fn13lfn13l4/report-as-suspicious/ and let a user raise a particular event to administrators for review. This broadly feels like a more promising path forward than putting an IP in the email and wishing users the best, especially for less-technical users.

This could eventually open up a flow where the "report" UI lets you explain why the action is suspicious and choose a remedy (e.g., "Block all password resets for my account for 24/72/indefinite hours"), which also feels like a clear improvement if this is an ongoing issue.

To step back slightly, a possible question we might ask is "Is the activity log here for the long term?". It's kind of an older object with a bit of weirdness to it. If we start tying it into auth flows, the answer is probably an unambiguous "yes". I think that's fine and it's reasonable to imagine this as a long-term object, I'd just like to move some of the events out of it.