Duo integration crashes if user is not enrolled and enrollment is disabled


#1

Observed Behavior:
I just upgraded to 2019 Week 4 and set up Duo and attempted to add it to my personal account (alongside my existing TOTP config) and got the following traceback:

[2019-01-29 18:48:39] EXCEPTION: (DomainException) Attempting to iterate an object (of class PhabricatorAuthFactorResult) which is not iterable. at [<phutil>/src/object/Phobject.php:57]
 arcanist(), phabricator(custom=5), phutil()
   #0 <#2> Phobject::throwOnAttemptedIteration() called at [<phutil>/src/object/Phobject.php:49]
   #1 <#2> Phobject::rewind() called at [<phabricator>/src/applications/auth/factor/PhabricatorAuthFactor.php:411]
   #2 <#2> PhabricatorAuthFactor::loadMFASyncToken(PhabricatorAuthFactorProvider, AphrontRequest, AphrontFormView, PhabricatorUser) called at [<phabricator>/src/applications/auth/factor/PhabricatorDuoAuthFactor.php:159]
   #3 <#2> PhabricatorDuoAuthFactor::processAddFactorForm(PhabricatorAuthFactorProvider, AphrontFormView, AphrontRequest, PhabricatorUser) called at [<phabricator>/src/applications/auth/storage/PhabricatorAuthFactorProvider.php:93]
   #4 <#2> PhabricatorAuthFactorProvider::processAddFactorForm(AphrontFormView, AphrontRequest, PhabricatorUser) called at [<phabricator>/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php:294]
   #5 <#2> PhabricatorMultiFactorSettingsPanel::processNew(AphrontRequest) called at [<phabricator>/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php:39]
   #6 <#2> PhabricatorMultiFactorSettingsPanel::processRequest(AphrontRequest) called at [<phabricator>/src/applications/settings/controller/PhabricatorSettingsMainController.php:107]
   #7 <#2> PhabricatorSettingsMainController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:258]
   #8 phlog(DomainException) called at [<phabricator>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
   #9 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, DomainException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:670]
   #10 AphrontApplicationConfiguration::handleThrowable(DomainException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:263]
   #11 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:181]
   #12 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

It works fine after I enroll my user in Duo.

Expected Behavior:
Not crashing

Phabricator Version:
Phabricator version: internal fork most recently merged with 6bca2e077330af12a5551d4eda63a6e239d89e9f
libphutil version: 9144fc5a68966087477874babce11d3bf2cf8f0b

Reproduction Steps:

  1. Set up Duo integration with enrollment disabled from the Phabricator side
  2. Attempt to add it to a user who does not have a Duo account
  3. Get the above crash

#2

I think this happens if Duo sends back a response other than “auth”, “allow”, or “enroll” (assuming the method works as documented, this response should always be “deny”). There’s a bug in how we handle “deny” responses.

I’m not immediately sure how to make Duo return “deny” responses when accounts are not enrolled – locally, I always get “enroll” back. Do you have any idea what switches you might have flipped on the Duo side to make it give you “deny”?

I can probably enroll and then go into Duo and explicitly add the user to a deny list to hit this case (maybe?) but that doesn’t sound like what you did.


#3

So, I think the thing here is that duo said the user needs to be enrolled but I turned off the checkbox to allow enrollment on the Phabricator side in the MFA settings page.


#4

If I do that, I get this state instead:

44_PM

I can reproduce a state similar to the one you describe by explicitly setting the user to “Deny” on the Duo side, but I don’t know how to get to that state without actually denying them.


#5

In any case, https://secure.phabricator.com/D20065 almost certainly fixes the bug (and may report a more useful error), although I’m not entirely sure I’ve reproduced exactly what you hit or exercised every pathway here.


#6

Thanks for looking into it so quickly. I’ll test that once it hits stable.