Add a skeleton for configurable MFA provider types
ClosedPublic
Actions

Authored by epriestley on Dec 29 2018, 1:54 AM.

Details

Reviewers

amckinley

Maniphest Tasks

T13222: 2018 Week 48-51 Bonus Content
T13231: Duo MFA Support

Commits

rPa62f334d9503: Add a skeleton for configurable MFA provider types

Summary

Ref T13222. Ref T13231. See PHI912. I'm planning to turn MFA providers into concrete objects, so you can disable and configure them.

Currently, we only support TOTP, which doesn't require any configuration, but other provider types (like Duo or Yubikey OTP) do require some configuration (server URIs, API keys, etc). TOTP could also have some configuration, like "bits of entropy" or "allowed window size" or whatever, if we want.

Add concrete objects for this and standard transaction / policy / query support. These objects don't do anything interesting yet and don't actually interact with MFA, this is just skeleton code for now.

Test Plan

Screen Shot 2018-12-28 at 5.53.25 PM.png (892×1 px, 155 KB)

Screen Shot 2018-12-28 at 5.53.27 PM.png (892×1 px, 205 KB)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Dec 29 2018, 1:54 AM

Owners added a subscriber: Restricted Owners Package.Dec 29 2018, 1:54 AM

Harbormaster completed remote builds in B21451: Diff 47597.Dec 29 2018, 1:56 AM

epriestley requested review of this revision.Dec 29 2018, 1:56 AM

epriestley added inline comments.Dec 29 2018, 1:57 AM

src/applications/auth/storage/PhabricatorAuthFactorProvider.php
18–20	My plan here is: Active: users can add new factors. Deprecated: users can continue using their existing factors, but can not add new ones (example: you want to move from TOTP to Duo, so you deprecate TOTP). Disabled: users can not add or use factors.

epriestley mentioned this in T13222: 2018 Week 48-51 Bonus Content.Jan 2 2019, 6:59 PM

amckinley accepted this revision.Jan 3 2019, 12:18 AM

amckinley added inline comments.

resources/sql/autopatches/20181228.auth.01.provider.sql
5	Maybe this should be `UNIQUE`?
resources/sql/autopatches/20181228.auth.03.name.sql
3	Or maybe this one should.
src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php
20	Shouldn't we have some kind of `getName` method on the provider objects?
src/applications/auth/xaction/PhabricatorAuthFactorProviderNameTransaction.php
40	You could also enforce name uniqueness here.

This revision is now accepted and ready to land.Jan 3 2019, 12:18 AM

epriestley added inline comments.Jan 3 2019, 12:36 AM

resources/sql/autopatches/20181228.auth.01.provider.sql
5	I think it's conceptually okay to have multiple factors for the same provider. For example, maybe two different subgroups at your org set up Duo independently, and then you adopt Phabricator, and then everyone realizes no one was talking to each other. So you configure both Duo accounts and call one "Ops Duo (Use This One)" and one "Eng Duo (Deprecated, Do Not Use)" and then move toward merging them, but everything works as-is for now and you don't need to transition half the org on day one. (Or you have a 128-bit TOTP source and a 256-bit TOTP source and move from one to the other, perhaps.) Most of the uses I can come up with are around gracefully transitioning between providers, but this kind of "one old version, one new version" smooth transition seems pretty reasonable.
resources/sql/autopatches/20181228.auth.03.name.sql
3	Yeah, you probably shouldn't have two providers with the same name. However, if we enforce `UNIQUE` on the column, you can't use the default/empty name for two providers at the same time. The field is optional and providers get sensible (and i18n-able) names if you leave it blank, and it seems reasonable to have a default-named SMS provider and a default-named TOTP provider at the same time. I guess we could put a unique key on both columns (`<providerFactorKey, name>`), but maybe having a default-named, disabled provider and a different, default-named, active provider of the same type is okay? This mostly just seems like it's probably not something we really need to enforce technically unless this UI proves way more confusing than I expect it to be, even though cases where two providers have the same name are probably not ideal.
src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php
20	Yeah, we ended up with a fair number of copies of this. I'll throw it in a method.