Page MenuHomePhabricator

Improve UI messaging around "one-shot" vs "session upgrade" MFA
ClosedPublic

Authored by epriestley on Dec 18 2018, 4:04 PM.

Details

Summary

Depends on D19899. Ref T13222. When we prompt you for one-shot MFA, we currently give you a lot of misleading text about your session staying in "high security mode".

Differentiate between one-shot and session upgrade MFA, and give the user appropriate cues and explanatory text.

Test Plan
  • Hit one-shot MFA on an "mfa" task in Maniphest.
  • Hit session upgrade MFA in Settings > Multi-Factor.

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.Dec 18 2018, 4:04 PM
epriestley requested review of this revision.Dec 18 2018, 4:06 PM
amckinley accepted this revision.Dec 18 2018, 10:32 PM
amckinley added inline comments.
src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php
102

This is minor, but the code/dialogues/comments seem to mix and match "MFA", "high security mode", "two-factor authentication", "2FA", etc pretty much at random. Just commenting now because I think this is the first usage of "multi-factor credentials" so far.

https://xkcd.com/927/

This revision is now accepted and ready to land.Dec 18 2018, 10:32 PM

I don't think we use "2FA" or "two-factor authentication" anywhere. I've tried to always use "MFA" or "multi-factor" because we've always supported more than two factors. Those terms may appear in tasks/discussion, but I think I'm probably not the author when they do. A quick grep doesn't turn anything up.

The use of "High Security" vs "MFA" is currently a bit inconsistent. I'm trying to move it toward this use:

  • "High Security" refers to the sticky "sudo" mode, and is intended as a more friendly term for "in sudo mode" that doesn't require you to know what sudo is.
  • Other flavors of "MFA" are used in the general case.

Here, since this is a user-facing prompt that less-technical users may run into, I spelled out "multi-factor credentials" in the explanatory text as a reminder about what the acronym is talking about, although it's probably reasonably obvious in context. I'm using the word "credentials" instead of "authentication" to hint that they (usually) need to respond to a challenge by presenting a credential, which I think is more clear than "provide authentication". Although "provide authentication" makes some amount of sense if you know what MFA is, I think it's not exactly grammatical and is a less helpful hint that "provide credentials" if you aren't sure.

Our existing use of "High Security" isn't entirely consistent with this, mostly because there was no one-shot before so there are some methods like isHighSecurity() which should probably be isMFA(). A specific example of this is AphrontRequest->isFormOrHisecPost(), where "Hisec" no longer refers to just the "sudo" mode (but, when it was written, it did). In a perfect world, this method would probably be isFormOrMFAPost(), but it's also possible that it simply shouldn't exist, so I haven't jumped on renaming it.

In the new code in D19897, I'm trying to be more consistent about this -- the new methods are ...MFA(), not ...Hisec().

Upshot: I think we're on a course toward consistency, and the inconsistency today is mostly not user-facing, I think, unless I missed some stuff.

This revision was automatically updated to reflect the committed changes.