@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
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.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20035
+24
-40
-2
src/__phutil_library_map__.php
-2
src/__phutil_library_map__.php
···
2241
2241
'PhabricatorAuthFactorProviderTransactionType' => 'applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php',
2242
2242
'PhabricatorAuthFactorProviderViewController' => 'applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php',
2243
2243
'PhabricatorAuthFactorResult' => 'applications/auth/factor/PhabricatorAuthFactorResult.php',
2244
-
'PhabricatorAuthFactorResultException' => 'applications/auth/exception/PhabricatorAuthFactorResultException.php',
2245
2244
'PhabricatorAuthFactorTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTestCase.php',
2246
2245
'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php',
2247
2246
'PhabricatorAuthHMACKey' => 'applications/auth/storage/PhabricatorAuthHMACKey.php',
···
7970
7969
'PhabricatorAuthFactorProviderTransactionType' => 'PhabricatorModularTransactionType',
7971
7970
'PhabricatorAuthFactorProviderViewController' => 'PhabricatorAuthFactorProviderController',
7972
7971
'PhabricatorAuthFactorResult' => 'Phobject',
7973
-
'PhabricatorAuthFactorResultException' => 'Exception',
7974
7972
'PhabricatorAuthFactorTestCase' => 'PhabricatorTestCase',
7975
7973
'PhabricatorAuthFinishController' => 'PhabricatorAuthController',
7976
7974
'PhabricatorAuthHMACKey' => 'PhabricatorAuthDAO',
+16
-8
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+16
-8
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
···
540
540
$provider = $factor->getFactorProvider();
541
541
$impl = $provider->getFactor();
542
542
543
-
try {
544
-
$new_challenges = $impl->getNewIssuedChallenges(
545
-
$factor,
546
-
$viewer,
547
-
$issued_challenges);
548
-
} catch (PhabricatorAuthFactorResultException $ex) {
549
-
$ok = false;
550
-
$validation_results[$factor_phid] = $ex->getResult();
543
+
$new_challenges = $impl->getNewIssuedChallenges(
544
+
$factor,
545
+
$viewer,
546
+
$issued_challenges);
547
+
548
+
// NOTE: We may get a list of challenges back, or may just get an early
549
+
// result. For example, this can happen on an SMS factor if all SMS
550
+
// mailers have been disabled.
551
+
if ($new_challenges instanceof PhabricatorAuthFactorResult) {
552
+
$result = $new_challenges;
553
+
554
+
if (!$result->getIsValid()) {
555
+
$ok = false;
556
+
}
557
+
558
+
$validation_results[$factor_phid] = $result;
551
559
$challenge_map[$factor_phid] = $issued_challenges;
552
560
continue;
553
561
}
-17
src/applications/auth/exception/PhabricatorAuthFactorResultException.php
-17
src/applications/auth/exception/PhabricatorAuthFactorResultException.php
···
1
-
<?php
2
-
3
-
final class PhabricatorAuthFactorResultException
4
-
extends Exception {
5
-
6
-
private $result;
7
-
8
-
public function __construct(PhabricatorAuthFactorResult $result) {
9
-
$this->result = $result;
10
-
parent::__construct();
11
-
}
12
-
13
-
public function getResult() {
14
-
return $this->result;
15
-
}
16
-
17
-
}
+5
-4
src/applications/auth/factor/PhabricatorAuthFactor.php
+5
-4
src/applications/auth/factor/PhabricatorAuthFactor.php
···
141
141
$viewer,
142
142
$challenges);
143
143
144
+
if ($new_challenges instanceof PhabricatorAuthFactorResult) {
145
+
unset($unguarded);
146
+
return $new_challenges;
147
+
}
148
+
144
149
assert_instances_of($new_challenges, 'PhabricatorAuthChallenge');
145
150
146
151
foreach ($new_challenges as $new_challenge) {
···
491
496
'style' => 'margin: 24px auto;',
492
497
),
493
498
$rows);
494
-
}
495
-
496
-
final protected function throwResult(PhabricatorAuthFactorResult $result) {
497
-
throw new PhabricatorAuthFactorResultException($result);
498
499
}
499
500
500
501
final protected function getInstallDisplayName() {
+3
-9
src/applications/auth/factor/PhabricatorSMSAuthFactor.php
+3
-9
src/applications/auth/factor/PhabricatorSMSAuthFactor.php
···
195
195
}
196
196
197
197
if (!$this->loadUserContactNumber($viewer)) {
198
-
$result = $this->newResult()
198
+
return $this->newResult()
199
199
->setIsError(true)
200
200
->setErrorMessage(
201
201
pht(
202
202
'Your account has no primary contact number.'));
203
-
204
-
$this->throwResult($result);
205
203
}
206
204
207
205
if (!$this->isSMSMailerConfigured()) {
208
-
$result = $this->newResult()
206
+
return $this->newResult()
209
207
->setIsError(true)
210
208
->setErrorMessage(
211
209
pht(
212
210
'No outbound mailer which can deliver SMS messages is '.
213
211
'configured.'));
214
-
215
-
$this->throwResult($result);
216
212
}
217
213
218
214
if (!$this->hasCSRF($config)) {
219
-
$result = $this->newResult()
215
+
return $this->newResult()
220
216
->setIsContinue(true)
221
217
->setErrorMessage(
222
218
pht(
223
219
'A text message with an authorization code will be sent to your '.
224
220
'primary contact number.'));
225
-
226
-
$this->throwResult($result);
227
221
}
228
222
229
223
// Otherwise, issue a new challenge.