From 6f3bd13cf5da0f62b9b7df8304105c82bb9bd2ee Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Feb 2019 05:22:39 -0800 Subject: [PATCH] Begin adding more guidance to the "One-Time Login" flow Summary: Ref T13244. See PHI774. If an install does not use password auth, the "one-time login" flow (via "Welcome" email or "bin/auth recover") is pretty rough. Current behavior: - If an install uses passwords, the user is prompted to set a password. - If an install does not use passwords, you're dumped to `/settings/external/` to link an external account. This is pretty sketchy and this UI does not make it clear what users are expected to do (link an account) or why (so they can log in). Instead, improve this flow: - Password reset flow is fine. - (Future Change) If there are external linkable accounts (like Google) and the user doesn't have any linked, I want to give users a flow like a password reset flow that says "link to an external account". - (This Change) If you're an administrator and there are no providers at all, go to "/auth/" so you can set something up. - (This Change) If we don't hit on any other rules, just go home? This may be tweaked a bit as we go, but basically I want to refine the "/settings/external/" case into a more useful flow which gives users more of a chance of surviving it. Test Plan: Logged in with passwords enabled (got password reset), with nothing enabled as an admin (got sent to Auth), and with something other than passwords enabled (got sent home). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13244 Differential Revision: https://secure.phabricator.com/D20094 --- .../PhabricatorAuthOneTimeLoginController.php | 86 ++++++++++++------- .../PhabricatorAuthProviderConfigQuery.php | 60 ++++--------- 2 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php index 0cac95f53d..f51f379d2b 100644 --- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php @@ -119,38 +119,9 @@ final class PhabricatorAuthOneTimeLoginController } unset($unguarded); - $next = '/'; - if (!PhabricatorPasswordAuthProvider::getPasswordProvider()) { - $next = '/settings/panel/external/'; - } else { + $next_uri = $this->getNextStepURI($target_user); - // We're going to let the user reset their password without knowing - // the old one. Generate a one-time token for that. - $key = Filesystem::readRandomCharacters(16); - $password_type = - PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE; - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - id(new PhabricatorAuthTemporaryToken()) - ->setTokenResource($target_user->getPHID()) - ->setTokenType($password_type) - ->setTokenExpires(time() + phutil_units('1 hour in seconds')) - ->setTokenCode(PhabricatorHash::weakDigest($key)) - ->save(); - unset($unguarded); - - $panel_uri = '/auth/password/'; - - $next = (string)id(new PhutilURI($panel_uri)) - ->setQueryParams( - array( - 'key' => $key, - )); - - $request->setTemporaryCookie(PhabricatorCookies::COOKIE_HISEC, 'yes'); - } - - PhabricatorCookies::setNextURICookie($request, $next, $force = true); + PhabricatorCookies::setNextURICookie($request, $next_uri, $force = true); $force_full_session = false; if ($link_type === PhabricatorAuthSessionEngine::ONETIME_RECOVER) { @@ -206,4 +177,57 @@ final class PhabricatorAuthOneTimeLoginController return id(new AphrontDialogResponse())->setDialog($dialog); } + + private function getNextStepURI(PhabricatorUser $user) { + $request = $this->getRequest(); + + // If we have password auth, let the user set or reset their password after + // login. + $have_passwords = PhabricatorPasswordAuthProvider::getPasswordProvider(); + if ($have_passwords) { + // We're going to let the user reset their password without knowing + // the old one. Generate a one-time token for that. + $key = Filesystem::readRandomCharacters(16); + $password_type = + PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE; + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + id(new PhabricatorAuthTemporaryToken()) + ->setTokenResource($user->getPHID()) + ->setTokenType($password_type) + ->setTokenExpires(time() + phutil_units('1 hour in seconds')) + ->setTokenCode(PhabricatorHash::weakDigest($key)) + ->save(); + unset($unguarded); + + $panel_uri = '/auth/password/'; + + $request->setTemporaryCookie(PhabricatorCookies::COOKIE_HISEC, 'yes'); + + return (string)id(new PhutilURI($panel_uri)) + ->setQueryParams( + array( + 'key' => $key, + )); + } + + $providers = id(new PhabricatorAuthProviderConfigQuery()) + ->setViewer($user) + ->withIsEnabled(true) + ->execute(); + + // 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()) { + return '/auth/'; + } + + // If we didn't find anywhere better to send them, give up and just send + // them to the home page. + return '/'; + } + } diff --git a/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php b/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php index 626c80348f..1d21342e4e 100644 --- a/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php +++ b/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php @@ -6,11 +6,7 @@ final class PhabricatorAuthProviderConfigQuery private $ids; private $phids; private $providerClasses; - - const STATUS_ALL = 'status:all'; - const STATUS_ENABLED = 'status:enabled'; - - private $status = self::STATUS_ALL; + private $isEnabled; public function withPHIDs(array $phids) { $this->phids = $phids; @@ -22,40 +18,26 @@ final class PhabricatorAuthProviderConfigQuery return $this; } - public function withStatus($status) { - $this->status = $status; - return $this; - } - public function withProviderClasses(array $classes) { $this->providerClasses = $classes; return $this; } - public static function getStatusOptions() { - return array( - self::STATUS_ALL => pht('All Providers'), - self::STATUS_ENABLED => pht('Enabled Providers'), - ); + public function withIsEnabled($is_enabled) { + $this->isEnabled = $is_enabled; + return $this; + } + + public function newResultObject() { + return new PhabricatorAuthProviderConfig(); } protected function loadPage() { - $table = new PhabricatorAuthProviderConfig(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } - protected function buildWhereClause(AphrontDatabaseConnection $conn) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); if ($this->ids !== null) { $where[] = qsprintf( @@ -78,22 +60,14 @@ final class PhabricatorAuthProviderConfigQuery $this->providerClasses); } - $status = $this->status; - switch ($status) { - case self::STATUS_ALL: - break; - case self::STATUS_ENABLED: - $where[] = qsprintf( - $conn, - 'isEnabled = 1'); - break; - default: - throw new Exception(pht("Unknown status '%s'!", $status)); + if ($this->isEnabled !== null) { + $where[] = qsprintf( + $conn, + 'isEnabled = %d', + (int)$this->isEnabled); } - $where[] = $this->buildPagingClause($conn); - - return $this->formatWhereClause($conn, $where); + return $where; } public function getQueryApplicationClass() {