Page MenuHomePhabricator

Add a skeleton for configurable MFA provider types
ClosedPublic

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

Details

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

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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
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.
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.

epriestley updated this revision to Diff 47650.Jan 7 2019, 5:24 PM
  • Reduce code duplication for MFA factor provider object names ("MFA Provider 123").
This revision was automatically updated to reflect the committed changes.