Differential D20035

Get rid of "throwResult()" for control flow in MFA factors
ClosedPublic
Actions

Authored by epriestley on Fri, Jan 25, 6:07 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13222: 2018 Week 48-51 Bonus Content

Commits

rP29b4fad94173: Get rid of "throwResult()" for control flow in MFA factors

Summary

Depends on D20034. Ref T13222. This is just cleanup -- I thought we'd have like two of these, but we ended up having a whole lot in Duo and a decent number in SMS. Just let factors return a result explicitly if they can make a decision early. I think using instanceof for control flow is a lesser evil than using catch, on the balance.

Test Plan

grep, went through enroll/gate flows on SMS and Duo.

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.Fri, Jan 25, 6:07 PM

Harbormaster completed remote builds in B21698: Diff 47844.Fri, Jan 25, 6:08 PM

epriestley requested review of this revision.Fri, Jan 25, 6:08 PM

epriestley added a child revision: D20036: Convert "Rename User" from session MFA to one-shot MFA.Fri, Jan 25, 6:19 PM

amckinley accepted this revision.Sat, Jan 26, 7:18 PM

This revision is now accepted and ready to land.Sat, Jan 26, 7:18 PM

Closed by commit rP29b4fad94173: Get rid of "throwResult()" for control flow in MFA factors (authored by epriestley). · Explain WhyMon, Jan 28, 5:40 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		src/__phutil_library_map__.php (2 lines)
M		src/applications/auth/engine/PhabricatorAuthSessionEngine.php (24 lines)
D	M	src/applications/auth/exception/PhabricatorAuthFactorResultException.php (17 lines)
M		src/applications/auth/factor/PhabricatorAuthFactor.php (9 lines)
M		src/applications/auth/factor/PhabricatorSMSAuthFactor.php (12 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	47844	e017e5f		Fri, Jan 25, 6:07 PM	★	★
Diff 2	47864	bce4438	rP29b4fad94173ffc9fc4c94f7e3660d56b641d463	Mon, Jan 28, 5:40 PM	★	★

Status	Author	Revision
Closed	epriestley	D20043 Allow MFA enrollment guidance to be customized
Closed	epriestley	D20039 Bring Duo MFA upstream
Closed	epriestley	D20038 Require MFA to edit MFA providers
Closed	epriestley	D20037 Replace "Show Secret" in Passphrase with one-shot MFA
Closed	epriestley	D20036 Convert "Rename User" from session MFA to one-shot MFA
Closed	epriestley	D20035 Get rid of "throwResult()" for control flow in MFA factors
Closed	epriestley	D20034 Add more factor details to the Settings factor list
Closed	epriestley	D20033 Update documentation for MFA, including administrator guidance
Closed	epriestley	D20032 Expand outbound mailer documentation to mention SMS and include Twilio
Closed	epriestley	D20031 Allow MFA providers to be deprecated or disabled