Page MenuHomePhabricator

Start fleshing out PhabricatorAuthProviderViewController
ClosedPublic

Authored by amckinley on Jul 10 2019, 4:28 PM.

Details

Summary

Ref D20645. Start making this view a little more useful:

Screen Shot 2019-07-10 at 9.27.35 AM.png (496×586 px, 45 KB)

Test Plan

Mk. 1 eyeball

Diff Detail

Repository
rP Phabricator
Branch
auth-view
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 23121
Build 31746: Run Core Tests
Build 31745: arc lint + arc unit

Event Timeline

We currently have a problem where some of these settings don't make sense or aren't actually used but can be viewed/edited, and this makes it slightly worse.

For example, in Auth you can edit a "Username/Password" provider and toggle the "Allow Link" / "Allow Unlink", but these settings don't actually have any effect because the Provider has a hard-coded override which ignores the saved settings:

public function shouldAllowAccountLink() {
  return false;
}

public function shouldAllowAccountUnlink() {
  return false;
}

Mostly, this code is just old and there's no way to specify "linking doesn't make sense for this provider, so link-related options don't make sense either and shouldn't be configurable".

However, the newer options do actually have "does this option make sense?" methods, notably:

  • supportsAutoLogin()
  • shouldAllowEmailTrustConfiguration()

So I think the "don't make anything worse than it already is" change is to gate the "Auto Login" and "Trust Email" items with those calls, and leave the other items for later:

if ($config->supportsAutoLogin()) {
  $view->addItem(...);
}

The "make everything great" change is to, like, modularize all this stuff and make it consistent (e.g. supportsLinking() or somesuch), but that's a much more far-reaching change that probably breaks at least some third-party providers and runs into modular transactions and who knows what else.

This revision is now accepted and ready to land.Jul 10 2019, 5:08 PM

Gate auto-login and trust emails.