diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2234,6 +2234,8 @@ 'PhabricatorAuthFactorProviderListController' => 'applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php', 'PhabricatorAuthFactorProviderNameTransaction' => 'applications/auth/xaction/PhabricatorAuthFactorProviderNameTransaction.php', 'PhabricatorAuthFactorProviderQuery' => 'applications/auth/query/PhabricatorAuthFactorProviderQuery.php', + 'PhabricatorAuthFactorProviderStatus' => 'applications/auth/constants/PhabricatorAuthFactorProviderStatus.php', + 'PhabricatorAuthFactorProviderStatusTransaction' => 'applications/auth/xaction/PhabricatorAuthFactorProviderStatusTransaction.php', 'PhabricatorAuthFactorProviderTransaction' => 'applications/auth/storage/PhabricatorAuthFactorProviderTransaction.php', 'PhabricatorAuthFactorProviderTransactionQuery' => 'applications/auth/query/PhabricatorAuthFactorProviderTransactionQuery.php', 'PhabricatorAuthFactorProviderTransactionType' => 'applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php', @@ -7961,6 +7963,8 @@ 'PhabricatorAuthFactorProviderListController' => 'PhabricatorAuthProviderController', 'PhabricatorAuthFactorProviderNameTransaction' => 'PhabricatorAuthFactorProviderTransactionType', 'PhabricatorAuthFactorProviderQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorAuthFactorProviderStatus' => 'Phobject', + 'PhabricatorAuthFactorProviderStatusTransaction' => 'PhabricatorAuthFactorProviderTransactionType', 'PhabricatorAuthFactorProviderTransaction' => 'PhabricatorModularTransaction', 'PhabricatorAuthFactorProviderTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorAuthFactorProviderTransactionType' => 'PhabricatorModularTransactionType', diff --git a/src/applications/auth/constants/PhabricatorAuthFactorProviderStatus.php b/src/applications/auth/constants/PhabricatorAuthFactorProviderStatus.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/constants/PhabricatorAuthFactorProviderStatus.php @@ -0,0 +1,103 @@ +key = $status; + $result->spec = self::newSpecification($status); + + return $result; + } + + public function getName() { + return idx($this->spec, 'name', $this->key); + } + + public function getStatusHeaderIcon() { + return idx($this->spec, 'header.icon'); + } + + public function getStatusHeaderColor() { + return idx($this->spec, 'header.color'); + } + + public function isActive() { + return ($this->key === self::STATUS_ACTIVE); + } + + public function getListIcon() { + return idx($this->spec, 'list.icon'); + } + + public function getListColor() { + return idx($this->spec, 'list.color'); + } + + public function getFactorIcon() { + return idx($this->spec, 'factor.icon'); + } + + public function getFactorColor() { + return idx($this->spec, 'factor.color'); + } + + public function getOrder() { + return idx($this->spec, 'order', 0); + } + + public static function getMap() { + $specs = self::newSpecifications(); + return ipull($specs, 'name'); + } + + private static function newSpecification($key) { + $specs = self::newSpecifications(); + return idx($specs, $key, array()); + } + + private static function newSpecifications() { + return array( + self::STATUS_ACTIVE => array( + 'name' => pht('Active'), + 'header.icon' => 'fa-check', + 'header.color' => null, + 'list.icon' => null, + 'list.color' => null, + 'factor.icon' => 'fa-check', + 'factor.color' => 'green', + 'order' => 1, + ), + self::STATUS_DEPRECATED => array( + 'name' => pht('Deprecated'), + 'header.icon' => 'fa-ban', + 'header.color' => 'indigo', + 'list.icon' => 'fa-ban', + 'list.color' => 'indigo', + 'factor.icon' => 'fa-ban', + 'factor.color' => 'indigo', + 'order' => 2, + ), + self::STATUS_DISABLED => array( + 'name' => pht('Disabled'), + 'header.icon' => 'fa-times', + 'header.color' => 'red', + 'list.icon' => 'fa-times', + 'list.color' => 'red', + 'factor.icon' => 'fa-times', + 'factor.color' => 'grey', + 'order' => 3, + ), + ); + } + +} diff --git a/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php b/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php --- a/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php +++ b/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php @@ -197,6 +197,8 @@ ->addCancelButton('/', pht('Continue')); } + $views = array(); + $messages = array(); $messages[] = pht( @@ -210,7 +212,39 @@ ->setSeverity(PHUIInfoView::SEVERITY_WARNING) ->setErrors($messages); - return $view; + $views[] = $view; + + + $providers = id(new PhabricatorAuthFactorProviderQuery()) + ->setViewer($viewer) + ->withStatuses( + array( + PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE, + )) + ->execute(); + if (!$providers) { + $messages = array(); + + $required_key = 'security.require-multi-factor-auth'; + + $messages[] = pht( + 'This install has the configuration option "%s" enabled, but does '. + 'not have any active multifactor providers configured. This means '. + 'you are required to add MFA, but are also prevented from doing so. '. + 'An administrator must disable "%s" or enable an MFA provider to '. + 'allow you to continue.', + $required_key, + $required_key); + + $view = id(new PHUIInfoView()) + ->setTitle(pht('Multi-Factor Authentication is Misconfigured')) + ->setSeverity(PHUIInfoView::SEVERITY_ERROR) + ->setErrors($messages); + + $views[] = $view; + } + + return $views; } } diff --git a/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php b/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php --- a/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php +++ b/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderListController.php @@ -20,6 +20,16 @@ ->setHeader($provider->getDisplayName()) ->setHref($provider->getURI()); + $status = $provider->newStatus(); + + $icon = $status->getListIcon(); + $color = $status->getListColor(); + if ($icon !== null) { + $item->setStatusIcon("{$icon} {$color}", $status->getName()); + } + + $item->setDisabled(!$status->isActive()); + $list->addItem($item); } diff --git a/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php b/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php --- a/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php +++ b/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php @@ -58,6 +58,15 @@ ->setHeader($provider->getDisplayName()) ->setPolicyObject($provider); + $status = $provider->newStatus(); + + $header_icon = $status->getStatusHeaderIcon(); + $header_color = $status->getStatusHeaderColor(); + $header_name = $status->getName(); + if ($header_icon !== null) { + $view->setStatus($header_icon, $header_color, $header_name); + } + return $view; } diff --git a/src/applications/auth/editor/PhabricatorAuthFactorProviderEditEngine.php b/src/applications/auth/editor/PhabricatorAuthFactorProviderEditEngine.php --- a/src/applications/auth/editor/PhabricatorAuthFactorProviderEditEngine.php +++ b/src/applications/auth/editor/PhabricatorAuthFactorProviderEditEngine.php @@ -95,6 +95,8 @@ protected function buildCustomEditFields($object) { $factor_name = $object->getFactor()->getFactorName(); + $status_map = PhabricatorAuthFactorProviderStatus::getMap(); + return array( id(new PhabricatorStaticEditField()) ->setKey('displayType') @@ -109,6 +111,14 @@ ->setDescription(pht('Display name for the MFA provider.')) ->setValue($object->getName()) ->setPlaceholder($factor_name), + id(new PhabricatorSelectEditField()) + ->setKey('status') + ->setTransactionType( + PhabricatorAuthFactorProviderStatusTransaction::TRANSACTIONTYPE) + ->setLabel(pht('Status')) + ->setDescription(pht('Status of the MFA provider.')) + ->setValue($object->getStatus()) + ->setOptions($status_map), ); } diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -473,9 +473,20 @@ $factors = id(new PhabricatorAuthFactorConfigQuery()) ->setViewer($viewer) ->withUserPHIDs(array($viewer->getPHID())) - ->setOrderVector(array('-id')) + ->withFactorProviderStatuses( + array( + PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE, + PhabricatorAuthFactorProviderStatus::STATUS_DEPRECATED, + )) ->execute(); + // Sort factors in the same order that they appear in on the Settings + // panel. This means that administrators changing provider stautuses may + // change the order of prompts for users, but the alternative is that the + // Settings panel order disagrees with the prompt order, which seems more + // disruptive. + $factors = msort($factors, 'newSortVector'); + // If the account has no associated multi-factor auth, just issue a token // without putting the session into high security mode. This is generally // easier for users. A minor but desirable side effect is that when a user diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -54,11 +54,15 @@ return null; } - public function canCreateNewConfiguration(PhabricatorUser $user) { + public function canCreateNewConfiguration( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user) { return true; } - public function getConfigurationCreateDescription(PhabricatorUser $user) { + public function getConfigurationCreateDescription( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user) { return null; } diff --git a/src/applications/auth/factor/PhabricatorSMSAuthFactor.php b/src/applications/auth/factor/PhabricatorSMSAuthFactor.php --- a/src/applications/auth/factor/PhabricatorSMSAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorSMSAuthFactor.php @@ -67,7 +67,10 @@ return $messages; } - public function canCreateNewConfiguration(PhabricatorUser $user) { + public function canCreateNewConfiguration( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user) { + if (!$this->loadUserContactNumber($user)) { return false; } @@ -75,7 +78,9 @@ return true; } - public function getConfigurationCreateDescription(PhabricatorUser $user) { + public function getConfigurationCreateDescription( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user) { $messages = array(); diff --git a/src/applications/auth/query/PhabricatorAuthFactorConfigQuery.php b/src/applications/auth/query/PhabricatorAuthFactorConfigQuery.php --- a/src/applications/auth/query/PhabricatorAuthFactorConfigQuery.php +++ b/src/applications/auth/query/PhabricatorAuthFactorConfigQuery.php @@ -7,6 +7,7 @@ private $phids; private $userPHIDs; private $factorProviderPHIDs; + private $factorProviderStatuses; public function withIDs(array $ids) { $this->ids = $ids; @@ -28,6 +29,11 @@ return $this; } + public function withFactorProviderStatuses(array $statuses) { + $this->factorProviderStatuses = $statuses; + return $this; + } + public function newResultObject() { return new PhabricatorAuthFactorConfig(); } @@ -42,34 +48,54 @@ if ($this->ids !== null) { $where[] = qsprintf( $conn, - 'id IN (%Ld)', + 'config.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( $conn, - 'phid IN (%Ls)', + 'config.phid IN (%Ls)', $this->phids); } if ($this->userPHIDs !== null) { $where[] = qsprintf( $conn, - 'userPHID IN (%Ls)', + 'config.userPHID IN (%Ls)', $this->userPHIDs); } if ($this->factorProviderPHIDs !== null) { $where[] = qsprintf( $conn, - 'factorProviderPHID IN (%Ls)', + 'config.factorProviderPHID IN (%Ls)', $this->factorProviderPHIDs); } + if ($this->factorProviderStatuses !== null) { + $where[] = qsprintf( + $conn, + 'provider.status IN (%Ls)', + $this->factorProviderStatuses); + } + return $where; } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->factorProviderStatuses !== null) { + $joins[] = qsprintf( + $conn, + 'JOIN %R provider ON config.factorProviderPHID = provider.phid', + new PhabricatorAuthFactorProvider()); + } + + return $joins; + } + protected function willFilterPage(array $configs) { $provider_phids = mpull($configs, 'getFactorProviderPHID'); @@ -94,6 +120,10 @@ return $configs; } + protected function getPrimaryTableAlias() { + return 'config'; + } + public function getQueryApplicationClass() { return 'PhabricatorAuthApplication'; } diff --git a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php --- a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php +++ b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php @@ -80,6 +80,12 @@ return $this; } + public function newSortVector() { + return id(new PhutilSortVector()) + ->addInt($this->getFactorProvider()->newStatus()->getOrder()) + ->addInt($this->getID()); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/auth/storage/PhabricatorAuthFactorProvider.php b/src/applications/auth/storage/PhabricatorAuthFactorProvider.php --- a/src/applications/auth/storage/PhabricatorAuthFactorProvider.php +++ b/src/applications/auth/storage/PhabricatorAuthFactorProvider.php @@ -14,15 +14,11 @@ private $factor = self::ATTACHABLE; - const STATUS_ACTIVE = 'active'; - const STATUS_DEPRECATED = 'deprecated'; - const STATUS_DISABLED = 'disabled'; - public static function initializeNewProvider(PhabricatorAuthFactor $factor) { return id(new self()) ->setProviderFactorKey($factor->getFactorKey()) ->attachFactor($factor) - ->setStatus(self::STATUS_ACTIVE); + ->setStatus(PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE); } protected function getConfiguration() { @@ -117,6 +113,20 @@ return $this->getFactor()->getEnrollButtonText($this, $user); } + public function newStatus() { + $status_key = $this->getStatus(); + return PhabricatorAuthFactorProviderStatus::newForStatus($status_key); + } + + public function canCreateNewConfiguration(PhabricatorUser $user) { + return $this->getFactor()->canCreateNewConfiguration($this, $user); + } + + public function getConfigurationCreateDescription(PhabricatorUser $user) { + return $this->getFactor()->getConfigurationCreateDescription($this, $user); + } + + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/auth/xaction/PhabricatorAuthFactorProviderStatusTransaction.php b/src/applications/auth/xaction/PhabricatorAuthFactorProviderStatusTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/xaction/PhabricatorAuthFactorProviderStatusTransaction.php @@ -0,0 +1,103 @@ +getStatus(); + } + + public function applyInternalEffects($object, $value) { + $object->setStatus($value); + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $old_display = PhabricatorAuthFactorProviderStatus::newForStatus($old) + ->getName(); + $new_display = PhabricatorAuthFactorProviderStatus::newForStatus($new) + ->getName(); + + return pht( + '%s changed the status of this provider from %s to %s.', + $this->renderAuthor(), + $this->renderValue($old_display), + $this->renderValue($new_display)); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + $actor = $this->getActor(); + + $map = PhabricatorAuthFactorProviderStatus::getMap(); + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + + if (!isset($map[$new_value])) { + $errors[] = $this->newInvalidError( + pht( + 'Status "%s" is invalid. Valid statuses are: %s.', + $new_value, + implode(', ', array_keys($map))), + $xaction); + continue; + } + + $require_key = 'security.require-multi-factor-auth'; + $require_mfa = PhabricatorEnv::getEnvConfig($require_key); + + if ($require_mfa) { + $status_active = PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE; + if ($new_value !== $status_active) { + $active_providers = id(new PhabricatorAuthFactorProviderQuery()) + ->setViewer($actor) + ->withStatuses( + array( + $status_active, + )) + ->execute(); + $active_providers = mpull($active_providers, null, 'getID'); + unset($active_providers[$object->getID()]); + + if (!$active_providers) { + $errors[] = $this->newInvalidError( + pht( + 'You can not deprecate or disable the last active MFA '. + 'provider while "%s" is enabled, because new users would '. + 'be unable to enroll in MFA. Disable the MFA requirement '. + 'in Config, or create or enable another MFA provider first.', + $require_key)); + continue; + } + } + } + } + + return $errors; + } + + public function didCommitTransaction($object, $value) { + $status = PhabricatorAuthFactorProviderStatus::newForStatus($value); + + // If a provider has undergone a status change, reset the MFA enrollment + // cache for all users. This may immediately force a lot of users to redo + // MFA enrollment. + + // We could be more surgical about this: we only really need to affect + // users who had a factor under the provider, and only really need to + // do anything if a provider was disabled. This is just a little simpler. + + $table = new PhabricatorUser(); + $conn = $table->establishConnection('w'); + + queryfx( + $conn, + 'UPDATE %R SET isEnrolledInMultiFactor = 0', + $table); + } + +} diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -908,9 +908,15 @@ * @task factors */ public function updateMultiFactorEnrollment() { - $factors = id(new PhabricatorAuthFactorConfig())->loadAllWhere( - 'userPHID = %s', - $this->getPHID()); + $factors = id(new PhabricatorAuthFactorConfigQuery()) + ->setViewer($this) + ->withUserPHIDs(array($this->getPHID())) + ->withFactorProviderStatuses( + array( + PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE, + PhabricatorAuthFactorProviderStatus::STATUS_DEPRECATED, + )) + ->execute(); $enrolled = count($factors) ? 1 : 0; if ($enrolled !== $this->isEnrolledInMultiFactor) { diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php --- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php @@ -53,8 +53,8 @@ $factors = id(new PhabricatorAuthFactorConfigQuery()) ->setViewer($viewer) ->withUserPHIDs(array($user->getPHID())) - ->setOrderVector(array('-id')) ->execute(); + $factors = msort($factors, 'newSortVector'); $rows = array(); $rowc = array(); @@ -69,7 +69,16 @@ $rowc[] = null; } + $status = $provider->newStatus(); + $status_icon = $status->getFactorIcon(); + $status_color = $status->getFactorColor(); + + $icon = id(new PHUIIconView()) + ->setIcon("{$status_icon} {$status_color}") + ->setTooltip(pht('Provider: %s', $status->getName())); + $rows[] = array( + $icon, javelin_tag( 'a', array( @@ -95,21 +104,24 @@ pht("You haven't added any authentication factors to your account yet.")); $table->setHeaders( array( + null, pht('Name'), pht('Type'), pht('Created'), - '', + null, )); $table->setColumnClasses( array( + null, 'wide pri', - '', + null, 'right', 'action', )); $table->setRowClasses($rowc); $table->setDeviceVisibility( array( + true, true, false, false, @@ -129,12 +141,15 @@ $add_color = PHUIButtonView::GREY; } + $can_add = (bool)$this->loadActiveMFAProviders(); + $buttons[] = id(new PHUIButtonView()) ->setTag('a') ->setIcon('fa-plus') ->setText(pht('Add Auth Factor')) ->setHref($this->getPanelURI('?new=true')) ->setWorkflow(true) + ->setDisabled(!$can_add) ->setColor($add_color); $buttons[] = id(new PHUIButtonView()) @@ -155,21 +170,18 @@ // Check that we have providers before we send the user through the MFA // gate, so you don't authenticate and then immediately get roadblocked. - $providers = id(new PhabricatorAuthFactorProviderQuery()) - ->setViewer($viewer) - ->withStatuses(array(PhabricatorAuthFactorProvider::STATUS_ACTIVE)) - ->execute(); + $providers = $this->loadActiveMFAProviders(); + if (!$providers) { return $this->newDialog() ->setTitle(pht('No MFA Providers')) ->appendParagraph( pht( - 'There are no active MFA providers. At least one active provider '. - 'must be available to add new MFA factors.')) + 'This install does not have any configured, active MFA '. + 'providers. At least one provider must be configured and active '. + 'before you can add new MFA factors.')) ->addCancelButton($cancel_uri); } - $providers = mpull($providers, null, 'getPHID'); - $proivders = msortv($providers, 'newSortVector'); $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession( $viewer, @@ -184,8 +196,7 @@ // Only let the user continue creating a factor for a given provider if // they actually pass the provider's checks. - $selected_factor = $selected_provider->getFactor(); - if (!$selected_factor->canCreateNewConfiguration($viewer)) { + if (!$selected_provider->canCreateNewConfiguration($viewer)) { $selected_provider = null; } } @@ -200,8 +211,7 @@ $provider_uri = id(new PhutilURI($this->getPanelURI())) ->setQueryParam('providerPHID', $provider_phid); - $factor = $provider->getFactor(); - $is_enabled = $factor->canCreateNewConfiguration($viewer); + $is_enabled = $provider->canCreateNewConfiguration($viewer); $item = id(new PHUIObjectItemView()) ->setHeader($provider->getDisplayName()) @@ -216,7 +226,7 @@ $item->setDisabled(true); } - $create_description = $factor->getConfigurationCreateDescription( + $create_description = $provider->getConfigurationCreateDescription( $viewer); if ($create_description) { $item->appendChild($create_description); @@ -424,5 +434,22 @@ ->setDialog($dialog); } + private function loadActiveMFAProviders() { + $viewer = $this->getViewer(); + + $providers = id(new PhabricatorAuthFactorProviderQuery()) + ->setViewer($viewer) + ->withStatuses( + array( + PhabricatorAuthFactorProviderStatus::STATUS_ACTIVE, + )) + ->execute(); + + $providers = mpull($providers, null, 'getPHID'); + $providers = msortv($providers, 'newSortVector'); + + return $providers; + } + }