Page MenuHomePhabricator

Allow installs to customize mail body guidance in the "Email Login" and "Set Password" emails
ClosedPublic

Authored by epriestley on Jul 19 2019, 2:41 PM.

Details

Summary

Depends on D20662. Ref T13343. Installs may reasonably want to change the guidance users receive in "Email Login"/"Forgot Password" email.

(In an upcoming change I plan to supply a piece of default guidance, but Auth Messages need a few tweaks for this.)

There's probably little reason to provide guidance on the "Set Password" flow, but any guidance one might issue on the "Email Login" flow probably doesn't make sense on the "Set Password" flow, so I've included it mostly to make it clear that this is a different flow from a user perspective.

Test Plan
  • Set custom "Email Login" and "Set Password" messages.
  • Generated "Email Login" mail by using the "Login via email" link on the login screen.
  • Generated "Set Password" email by trying to set a password on an account with no password yet.
  • Saw my custom messages in the resulting mail bodies.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jul 19 2019, 2:41 PM
epriestley requested review of this revision.Jul 19 2019, 2:42 PM

Are we worried about attackers changing the guidance to something like "To prove that your Phabricator account is in use, please email the following link to bob.hackerman@gmail.com and don't read the rest of this email"?

Yeah -- I lean toward thinking that we probably should make bin/auth lock also lock the guidance messages too. This class of attack feels like a bit of a stretch since no one reads instructions anyway, but letting an attacker replace the login screen with This page has moved temporarily, click [[ here ]] to go to the new login page. and then 9,000 newlines to push all the actual login controls off the page is at least sort of plausible-attack-flavored.

amckinley accepted this revision.Jul 19 2019, 8:46 PM
This revision is now accepted and ready to land.Jul 19 2019, 8:46 PM

Yeah -- I lean toward thinking that we probably should make bin/auth lock also lock the guidance messages too.

I can do that since I was just fiddling around in there. On a related note, I was thinking bin/auth should in general also be logging things to the User Activity logs; thoughts?

I can do that since I was just fiddling around in there.

Sounds good.

On a related note, I was thinking bin/auth should in general also be logging things to the User Activity logs; thoughts?

Which operations in particular? Nothing jumps out to me from a quick bin/auth help -- I think most things either don't have an acting user, don't apply to a user, already have some kind of logging, and/or don't affect users, I think?

In general, I don't think bin/auth needs to log for security/audit purposes, since an attacker with access to bin/auth can just connect to MySQL and do whatever they want without interacting with any logging.

If there are cases where bin/auth doing more logging could improve clarity I think that's reasonable, but slightly ahead of this in D20670 I'm splitting some messages out of this log and trying to focus the different logs more clearly around these questions:

  • "Is a remote attacker trying to gain unauthorized access to the system?" -> User activity log.
  • "Which administrative changes have been made to account @alice?" -> User profile timeline.
  • "What actions has @alice taken recently?" -> Transaction logs in Feed.

I think the user activity log is currently doing more than it probably should, and the only class of questions it's really good for answering are about unauthorized remote attackers attempting to gain access.

The only question I really ever use it to answer is "Does this guy emailing me for help logging in have a bunch of recent suspicious activity on his account?", and "help" is mostly MFA stuff, which we'd ideally get out of the human support loop eventually.

T13343 implies the question "Someone is clicking login buttons, which IPs should we ban?".