Page MenuHomePhabricator
Differential D19885

Require MFA implementations to return a formal result object when validating factors
ClosedPublic
Actions

Authored by epriestley on Dec 13 2018, 8:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 4, 6:59 PM
Unknown Object (File)
Thu, May 2, 3:46 AM
Unknown Object (File)
Wed, May 1, 12:28 AM
Unknown Object (File)
Mon, Apr 29, 6:22 PM
Unknown Object (File)
Tue, Apr 23, 6:47 PM
Unknown Object (File)
Tue, Apr 23, 6:47 PM
Unknown Object (File)
Fri, Apr 19, 8:45 AM
Unknown Object (File)
Thu, Apr 11, 9:53 AM
View All Files
Subscribers
None

Details

Reviewers
amckinley
Maniphest Tasks
T13222: 2018 Week 48-51 Bonus Content
Commits
rPc731508d748a: Require MFA implementations to return a formal result object when validating…
Summary

Ref T13222. See PHI873. Currently, MFA implementations return this weird sort of ad-hoc dictionary from validation, which is later used to render form/control stuff.

I want to make this more formal to handle token reuse / session binding cases, and let MFA factors share more code around challenges. Formalize this into a proper object instead of an ad-hoc bundle of properties.

Test Plan
  • Answered a TOTP MFA prompt wrong (nothing, bad value).
  • Answered a TOTP MFA prompt properly.
  • Added new TOTP MFA, survived enrollment.

Diff Detail

Repository
rP Phabricator
Branch
mfa6
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21333
Build 29031: Run Core Tests
Build 29030: arc lint + arc unit

Event Timeline

epriestley created this revision.Dec 13 2018, 8:41 PM
Harbormaster completed remote builds in B21333: Diff 47482.Dec 13 2018, 8:42 PM
epriestley requested review of this revision.Dec 13 2018, 8:42 PM
amckinley accepted this revision.Dec 13 2018, 9:17 PM
amckinley added inline comments.
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
504–511

Does this type checking live here because someone might have added their own implementation of PhabricatorAuthFactor?

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
174

I'm assuming this was intentional, and that PhabricatorAuthFactorResult.hint is really more of an error-hint.

This revision is now accepted and ready to land.Dec 13 2018, 9:17 PM
epriestley added inline comments.Dec 13 2018, 9:22 PM
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
504–511

Yeah, this is just a guard rail in case someone has another MFA implementation somewhere that's expecting it can return a string or a picture of a dog or something.

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
174

"Hint" may not be the best name, but I'm trying to distinguish between "hint next to the input about what's wrong with the thing you input" and "big error message in a banner". Not 100% sure where things are headed yet so this might change. If I don't need both, I'll likely change this to setError(...).

epriestley added a comment.Dec 14 2018, 12:17 AM

D19886 ended up renaming "Hint" to "Error Message" and pushing the instanceof X logic into the abstract base AuthFactor class, using this sorta thing:

final public function getX() {
  $x = $this->newX();
  if ($x is bad) { cry };
  return $x;
}

abstract protected function newX();

...since that seemed cleaner by the time I got things working.

Closed by commit rPc731508d748a: Require MFA implementations to return a formal result object when validating… (authored by epriestley). · Explain WhyDec 17 2018, 2:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 47482

View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

Loading...
View Options

src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php

Loading...
View Options

src/applications/auth/factor/PhabricatorAuthFactor.php

Loading...
View Options

src/applications/auth/factor/PhabricatorAuthFactorResult.php

Loading...
View Options

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

Loading...
Phabricator · Built by Phacility