From f9192d07f2c798b5f4257c7cdf6cfb566fe6e3cc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 10:43:40 -0700 Subject: [PATCH] Align web UI "Disable" and "Approve/Disapprove" flows with new "Can Disable Users" permission Summary: Depends on D19606. Ref T13189. See PHI642. - Disabling/enabling users no longer requires admin. Now, you just need "Can Disable Users". - Update the UI to appropriately show the action in black or grey depending on what clicking the button will do. - For "Approve/Disapprove", fix a couple bugs, then let them go through without respect for "Can Disable Users". This is conceptually a different action, even though it ultimately sets the "Disabled" flag. Test Plan: - Disabled/enabled users from the web UI as various users, including a non-administrator with "Can Disable Users". - Hit permissions errors from the web UI as various users, including an administrator without "Can Disable Users". - Saw the "Disable/Enable" action activate properly based on whether clicking the button would actually work. - Disapproved a user without "Can Disable Users" permission, tried to re-disapprove a user. - Approved a user, tried to reapprove a user. Reviewers: amckinley Maniphest Tasks: T13189 Differential Revision: https://secure.phabricator.com/D19607 --- .../PhabricatorPeopleApproveController.php | 7 ++++ .../PhabricatorPeopleDisableController.php | 34 +++++++++++++++++-- ...abricatorPeopleProfileManageController.php | 19 +++++++---- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/applications/people/controller/PhabricatorPeopleApproveController.php b/src/applications/people/controller/PhabricatorPeopleApproveController.php index 58cd2e2119..0e97ad6ee6 100644 --- a/src/applications/people/controller/PhabricatorPeopleApproveController.php +++ b/src/applications/people/controller/PhabricatorPeopleApproveController.php @@ -16,6 +16,13 @@ final class PhabricatorPeopleApproveController $done_uri = $this->getApplicationURI('query/approval/'); + if ($user->getIsApproved()) { + return $this->newDialog() + ->setTitle(pht('Already Approved')) + ->appendChild(pht('This user has already been approved.')) + ->addCancelButton($done_uri); + } + if ($request->isFormPost()) { id(new PhabricatorUserEditor()) ->setActor($viewer) diff --git a/src/applications/people/controller/PhabricatorPeopleDisableController.php b/src/applications/people/controller/PhabricatorPeopleDisableController.php index f51f42047b..9f2718086b 100644 --- a/src/applications/people/controller/PhabricatorPeopleDisableController.php +++ b/src/applications/people/controller/PhabricatorPeopleDisableController.php @@ -3,10 +3,14 @@ final class PhabricatorPeopleDisableController extends PhabricatorPeopleController { + public function shouldRequireAdmin() { + return false; + } + public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); $id = $request->getURIData('id'); - $via = $request->getURIData('id'); + $via = $request->getURIData('via'); $user = id(new PhabricatorPeopleQuery()) ->setViewer($viewer) @@ -20,11 +24,36 @@ final class PhabricatorPeopleDisableController // on profiles and also via the "X" action on the approval queue. We do // things slightly differently depending on the context the actor is in. + // In particular, disabling via "Disapprove" requires you be an + // administrator (and bypasses the "Can Disable Users" permission). + // Disabling via "Disable" requires the permission only. + $is_disapprove = ($via == 'disapprove'); if ($is_disapprove) { $done_uri = $this->getApplicationURI('query/approval/'); + + if (!$viewer->getIsAdmin()) { + return $this->newDialog() + ->setTitle(pht('No Permission')) + ->appendParagraph(pht('Only administrators can disapprove users.')) + ->addCancelButton($done_uri); + } + + if ($user->getIsApproved()) { + return $this->newDialog() + ->setTitle(pht('Already Approved')) + ->appendParagraph(pht('This user has already been approved.')) + ->addCancelButton($done_uri); + } + + // On the "Disapprove" flow, bypass the "Can Disable Users" permission. + $actor = PhabricatorUser::getOmnipotentUser(); $should_disable = true; } else { + $this->requireApplicationCapability( + PeopleDisableUsersCapability::CAPABILITY); + + $actor = $viewer; $done_uri = $this->getApplicationURI("manage/{$id}/"); $should_disable = !$user->getIsDisabled(); } @@ -46,7 +75,8 @@ final class PhabricatorPeopleDisableController ->setNewValue($should_disable); id(new PhabricatorUserTransactionEditor()) - ->setActor($viewer) + ->setActor($actor) + ->setActingAsPHID($viewer->getPHID()) ->setContentSourceFromRequest($request) ->setContinueOnMissingFields(true) ->setContinueOnNoEffect(true) diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php index 2ac3e6de89..9759a375c7 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php @@ -75,11 +75,22 @@ final class PhabricatorPeopleProfileManageController private function buildCurtain(PhabricatorUser $user) { $viewer = $this->getViewer(); + $is_self = ($user->getPHID() === $viewer->getPHID()); + $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $user, PhabricatorPolicyCapability::CAN_EDIT); + $is_admin = $viewer->getIsAdmin(); + $can_admin = ($is_admin && !$is_self); + + $has_disable = $this->hasApplicationCapability( + PeopleDisableUsersCapability::CAPABILITY); + $can_disable = ($has_disable && !$is_self); + + $can_welcome = ($is_admin && $user->canEstablishWebSessions()); + $curtain = $this->newCurtainView($user); $curtain->addAction( @@ -114,10 +125,6 @@ final class PhabricatorPeopleProfileManageController $empower_name = pht('Make Administrator'); } - $is_admin = $viewer->getIsAdmin(); - $is_self = ($user->getPHID() === $viewer->getPHID()); - $can_admin = ($is_admin && !$is_self); - $curtain->addAction( id(new PhabricatorActionView()) ->setIcon($empower_icon) @@ -146,7 +153,7 @@ final class PhabricatorPeopleProfileManageController id(new PhabricatorActionView()) ->setIcon($disable_icon) ->setName($disable_name) - ->setDisabled(!$can_admin) + ->setDisabled(!$can_disable) ->setWorkflow(true) ->setHref($this->getApplicationURI('disable/'.$user->getID().'/'))); @@ -158,8 +165,6 @@ final class PhabricatorPeopleProfileManageController ->setWorkflow(true) ->setHref($this->getApplicationURI('delete/'.$user->getID().'/'))); - $can_welcome = ($is_admin && $user->canEstablishWebSessions()); - $curtain->addAction( id(new PhabricatorActionView()) ->setIcon('fa-envelope')