1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 05:50:55 +01:00

Improve handling of "Deny" responses from Duo

Summary:
Ref T13231. See <https://discourse.phabricator-community.org/t/duo-integration-crashes-if-user-is-not-enrolled-and-enrollment-is-disabled/2340/5>

(There's an actual bug here, although I'm not sure exactly what's going on on the Duo side in the report.)

Test Plan:
To reproduce this, I was only able to actually "Deny" my account explicitly in Duo.

  - With "Deny", tried to add a factor. Got a nice helpful error message.
  - Undenied, added a factor, re-denied, tried to pass an MFA gate. Got another nice helpful error message.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231

Differential Revision: https://secure.phabricator.com/D20065
This commit is contained in:
epriestley 2019-01-29 20:45:18 -08:00
parent 93b512b63c
commit e9b2d667ee
2 changed files with 27 additions and 5 deletions

View file

@ -147,7 +147,7 @@ abstract class PhabricatorAuthFactor extends Phobject {
$viewer,
$challenges);
if ($new_challenges instanceof PhabricatorAuthFactorResult) {
if ($this->isAuthResult($new_challenges)) {
unset($unguarded);
return $new_challenges;
}
@ -200,7 +200,7 @@ abstract class PhabricatorAuthFactor extends Phobject {
return $result;
}
if (!($result instanceof PhabricatorAuthFactorResult)) {
if (!$this->isAuthResult($result)) {
throw new Exception(
pht(
'Expected "newResultFromIssuedChallenges()" to return null or '.
@ -232,7 +232,7 @@ abstract class PhabricatorAuthFactor extends Phobject {
$request,
$challenges);
if (!($result instanceof PhabricatorAuthFactorResult)) {
if (!$this->isAuthResult($result)) {
throw new Exception(
pht(
'Expected "newResultFromChallengeResponse()" to return an object '.
@ -408,6 +408,10 @@ abstract class PhabricatorAuthFactor extends Phobject {
$provider,
$user);
if ($this->isAuthResult($properties)) {
return $properties;
}
foreach ($properties as $key => $value) {
$sync_token->setTemporaryTokenProperty($key, $value);
}
@ -555,4 +559,8 @@ abstract class PhabricatorAuthFactor extends Phobject {
->execute();
}
final protected function isAuthResult($object) {
return ($object instanceof PhabricatorAuthFactorResult);
}
}

View file

@ -157,6 +157,10 @@ final class PhabricatorDuoAuthFactor
PhabricatorUser $user) {
$token = $this->loadMFASyncToken($provider, $request, $form, $user);
if ($this->isAuthResult($token)) {
$form->appendChild($this->newAutomaticControl($token));
return;
}
$enroll = $token->getTemporaryTokenProperty('duo.enroll');
$duo_id = $token->getTemporaryTokenProperty('duo.user-id');
@ -350,6 +354,7 @@ final class PhabricatorDuoAuthFactor
$external_uri = null;
$result_code = $result['response']['result'];
$status_message = $result['response']['status_msg'];
switch ($result_code) {
case 'auth':
case 'allow':
@ -376,7 +381,13 @@ final class PhabricatorDuoAuthFactor
return $this->newResult()
->setIsError(true)
->setErrorMessage(
pht('Your account is not permitted to access this system.'));
pht(
'Your Duo account ("%s") is not permitted to access this '.
'system. Contact your Duo administrator for help. '.
'The Duo preauth API responded with status message ("%s"): %s',
$duo_user,
$result_code,
$status_message));
}
// Duo's "/enroll" API isn't repeatable for the same username. If we're
@ -476,7 +487,10 @@ final class PhabricatorDuoAuthFactor
->setIsError(true)
->setErrorMessage(
pht(
'Duo has denied you access. Duo status message ("%s"): %s',
'Your Duo account ("%s") is not permitted to access this '.
'system. Contact your Duo administrator for help. The Duo '.
'preauth API responded with status message ("%s"): %s',
$duo_user,
$next_step,
$status_message));
}