From 8f8e863613c0935a0272fdbe7825480f9a0781b0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Feb 2019 05:22:39 -0800 Subject: [PATCH] When users follow an email login link but an install does not use passwords, try to get them to link an account Summary: Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords. If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow. This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options". Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future: - When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro". - This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird. So: step forward, but more work to be done. Test Plan: {F6211115} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20170 --- resources/celerity/map.php | 6 +- src/__phutil_library_map__.php | 4 + .../PhabricatorAuthApplication.php | 2 + .../PhabricatorAuthOneTimeLoginController.php | 32 ++++- .../PhabricatorAuthSetExternalController.php | 110 ++++++++++++++++++ .../PhabricatorAuthLinkMessageType.php | 18 +++ .../auth/provider/PhabricatorAuthProvider.php | 2 +- .../PhabricatorPasswordAuthProvider.php | 3 +- webroot/rsrc/css/phui/phui-object-box.css | 5 + 9 files changed, 174 insertions(+), 8 deletions(-) create mode 100644 src/applications/auth/controller/PhabricatorAuthSetExternalController.php create mode 100644 src/applications/auth/message/PhabricatorAuthLinkMessageType.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2dd5cf0966..b1a97639c2 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => 'e0f5d66f', + 'core.pkg.css' => '4ed8ce1f', 'core.pkg.js' => '5c737607', 'differential.pkg.css' => 'b8df73d4', 'differential.pkg.js' => '67c9ea4c', @@ -164,7 +164,7 @@ return array( 'rsrc/css/phui/phui-left-right.css' => '68513c34', 'rsrc/css/phui/phui-lightbox.css' => '4ebf22da', 'rsrc/css/phui/phui-list.css' => '470b1adb', - 'rsrc/css/phui/phui-object-box.css' => '9b58483d', + 'rsrc/css/phui/phui-object-box.css' => 'f434b6be', 'rsrc/css/phui/phui-pager.css' => 'd022c7ad', 'rsrc/css/phui/phui-pinboard-view.css' => '1f08f5d8', 'rsrc/css/phui/phui-property-list-view.css' => 'cad62236', @@ -833,7 +833,7 @@ return array( 'phui-left-right-css' => '68513c34', 'phui-lightbox-css' => '4ebf22da', 'phui-list-view-css' => '470b1adb', - 'phui-object-box-css' => '9b58483d', + 'phui-object-box-css' => 'f434b6be', 'phui-oi-big-ui-css' => '9e037c7a', 'phui-oi-color-css' => 'b517bfa0', 'phui-oi-drag-ui-css' => 'da15d3dc', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e3287df74c..b10c0e109f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2272,6 +2272,7 @@ phutil_register_library_map(array( 'PhabricatorAuthInviteVerifyException' => 'applications/auth/exception/PhabricatorAuthInviteVerifyException.php', 'PhabricatorAuthInviteWorker' => 'applications/auth/worker/PhabricatorAuthInviteWorker.php', 'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php', + 'PhabricatorAuthLinkMessageType' => 'applications/auth/message/PhabricatorAuthLinkMessageType.php', 'PhabricatorAuthListController' => 'applications/auth/controller/config/PhabricatorAuthListController.php', 'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php', 'PhabricatorAuthLoginMessageType' => 'applications/auth/message/PhabricatorAuthLoginMessageType.php', @@ -2370,6 +2371,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSessionPHIDType' => 'applications/auth/phid/PhabricatorAuthSessionPHIDType.php', 'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php', 'PhabricatorAuthSessionRevoker' => 'applications/auth/revoker/PhabricatorAuthSessionRevoker.php', + 'PhabricatorAuthSetExternalController' => 'applications/auth/controller/PhabricatorAuthSetExternalController.php', 'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php', 'PhabricatorAuthSetupCheck' => 'applications/config/check/PhabricatorAuthSetupCheck.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', @@ -8023,6 +8025,7 @@ phutil_register_library_map(array( 'PhabricatorAuthInviteVerifyException' => 'PhabricatorAuthInviteDialogException', 'PhabricatorAuthInviteWorker' => 'PhabricatorWorker', 'PhabricatorAuthLinkController' => 'PhabricatorAuthController', + 'PhabricatorAuthLinkMessageType' => 'PhabricatorAuthMessageType', 'PhabricatorAuthListController' => 'PhabricatorAuthProviderConfigController', 'PhabricatorAuthLoginController' => 'PhabricatorAuthController', 'PhabricatorAuthLoginMessageType' => 'PhabricatorAuthMessageType', @@ -8142,6 +8145,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSessionPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthSessionRevoker' => 'PhabricatorAuthRevoker', + 'PhabricatorAuthSetExternalController' => 'PhabricatorAuthController', 'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController', 'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php index 4e0baff229..3446ad597d 100644 --- a/src/applications/auth/application/PhabricatorAuthApplication.php +++ b/src/applications/auth/application/PhabricatorAuthApplication.php @@ -86,7 +86,9 @@ final class PhabricatorAuthApplication extends PhabricatorApplication { => 'PhabricatorAuthSSHKeyRevokeController', 'view/(?P\d+)/' => 'PhabricatorAuthSSHKeyViewController', ), + 'password/' => 'PhabricatorAuthSetPasswordController', + 'external/' => 'PhabricatorAuthSetExternalController', 'mfa/' => array( $this->getQueryRoutePattern() => diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php index 353f31562c..d176a67119 100644 --- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php @@ -225,17 +225,45 @@ final class PhabricatorAuthOneTimeLoginController return (string)new PhutilURI($panel_uri, $params); } - $providers = id(new PhabricatorAuthProviderConfigQuery()) + // Check if the user already has external accounts linked. If they do, + // it's not obvious why they aren't using them to log in, but assume they + // know what they're doing. We won't send them to the link workflow. + $accounts = id(new PhabricatorExternalAccountQuery()) + ->setViewer($user) + ->withUserPHIDs(array($user->getPHID())) + ->execute(); + + $configs = id(new PhabricatorAuthProviderConfigQuery()) ->setViewer($user) ->withIsEnabled(true) ->execute(); + $linkable = array(); + foreach ($configs as $config) { + if (!$config->getShouldAllowLink()) { + continue; + } + + $provider = $config->getProvider(); + if (!$provider->isLoginFormAButton()) { + continue; + } + + $linkable[] = $provider; + } + + // If there's at least one linkable provider, and the user doesn't already + // have accounts, send the user to the link workflow. + if (!$accounts && $linkable) { + return '/auth/external/'; + } + // If there are no configured providers and the user is an administrator, // send them to Auth to configure a provider. This is probably where they // want to go. You can end up in this state by accidentally losing your // first session during initial setup, or after restoring exported data // from a hosted instance. - if (!$providers && $user->getIsAdmin()) { + if (!$configs && $user->getIsAdmin()) { return '/auth/'; } diff --git a/src/applications/auth/controller/PhabricatorAuthSetExternalController.php b/src/applications/auth/controller/PhabricatorAuthSetExternalController.php new file mode 100644 index 0000000000..51dfcab53f --- /dev/null +++ b/src/applications/auth/controller/PhabricatorAuthSetExternalController.php @@ -0,0 +1,110 @@ +getViewer(); + + $configs = id(new PhabricatorAuthProviderConfigQuery()) + ->setViewer($viewer) + ->withIsEnabled(true) + ->execute(); + + $linkable = array(); + foreach ($configs as $config) { + if (!$config->getShouldAllowLink()) { + continue; + } + + // For now, only buttons get to appear here: for example, we can't + // reasonably embed an entire LDAP form into this UI. + $provider = $config->getProvider(); + if (!$provider->isLoginFormAButton()) { + continue; + } + + $linkable[] = $config; + } + + if (!$linkable) { + return $this->newDialog() + ->setTitle(pht('No Linkable External Providers')) + ->appendParagraph( + pht( + 'Currently, there are no configured external auth providers '. + 'which you can link your account to.')) + ->addCancelButton('/'); + } + + $text = PhabricatorAuthMessage::loadMessageText( + $viewer, + PhabricatorAuthLinkMessageType::MESSAGEKEY); + if (!strlen($text)) { + $text = pht( + 'You can link your Phabricator account to an external account to '. + 'allow you to log in more easily in the future. To continue, choose '. + 'an account to link below. If you prefer not to link your account, '. + 'you can skip this step.'); + } + + $remarkup_view = new PHUIRemarkupView($viewer, $text); + $remarkup_view = phutil_tag( + 'div', + array( + 'class' => 'phui-object-box-instructions', + ), + $remarkup_view); + + PhabricatorCookies::setClientIDCookie($request); + + $view = array(); + foreach ($configs as $config) { + $provider = $config->getProvider(); + + $form = $provider->buildLinkForm($this); + + if ($provider->isLoginFormAButton()) { + require_celerity_resource('auth-css'); + $form = phutil_tag( + 'div', + array( + 'class' => 'phabricator-link-button pl', + ), + $form); + } + + $view[] = $form; + } + + $form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendControl( + id(new AphrontFormSubmitControl()) + ->addCancelButton('/', pht('Skip This Step'))); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Link External Account')); + + $box = id(new PHUIObjectBoxView()) + ->setViewer($viewer) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($remarkup_view) + ->appendChild($view) + ->appendChild($form); + + $main_view = id(new PHUITwoColumnView()) + ->setFooter($box); + + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb(pht('Link External Account')) + ->setBorder(true); + + return $this->newPage() + ->setTitle(pht('Link External Account')) + ->setCrumbs($crumbs) + ->appendChild($main_view); + } + +} diff --git a/src/applications/auth/message/PhabricatorAuthLinkMessageType.php b/src/applications/auth/message/PhabricatorAuthLinkMessageType.php new file mode 100644 index 0000000000..17991231c1 --- /dev/null +++ b/src/applications/auth/message/PhabricatorAuthLinkMessageType.php @@ -0,0 +1,18 @@ +renderLoginForm($controller->getRequest(), $mode = 'link'); } diff --git a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php index 146e41706a..d39e378798 100644 --- a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php @@ -159,8 +159,7 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider { return $dialog; } - public function buildLinkForm( - PhabricatorAuthLinkController $controller) { + public function buildLinkForm($controller) { throw new Exception(pht("Password providers can't be linked.")); } diff --git a/webroot/rsrc/css/phui/phui-object-box.css b/webroot/rsrc/css/phui/phui-object-box.css index 4999a4c2c2..f95e36eedc 100644 --- a/webroot/rsrc/css/phui/phui-object-box.css +++ b/webroot/rsrc/css/phui/phui-object-box.css @@ -158,3 +158,8 @@ div.phui-object-box.phui-object-box-flush { margin-top: 8px; margin-bottom: 8px; } + +.phui-object-box-instructions { + padding: 16px; + border-bottom: 1px solid {$thinblueborder}; +}