Details
- Reviewers
amckinley - Maniphest Tasks
- T13222: 2018 Week 48-51 Bonus Content
- Commits
- rP1c89b3175f1c: Improve UI messaging around "one-shot" vs "session upgrade" MFA
- Hit one-shot MFA on an "mfa" task in Maniphest.
- Hit session upgrade MFA in Settings > Multi-Factor.
Diff Detail
- Repository
- rP Phabricator
- Branch
- mfa18
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 21364 Build 29082: Run Core Tests Build 29081: arc lint + arc unit
Event Timeline
| 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. | |
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.