diff --git a/resources/sql/autopatches/20181222.banish.01.user.sql b/resources/sql/autopatches/20181222.banish.01.user.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20181222.banish.01.user.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_user.user + ADD isBanished BOOL NOT NULL; 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 @@ -4608,6 +4608,7 @@ 'PhabricatorUser' => 'applications/people/storage/PhabricatorUser.php', 'PhabricatorUserApproveTransaction' => 'applications/people/xaction/PhabricatorUserApproveTransaction.php', 'PhabricatorUserBadgesCacheType' => 'applications/people/cache/PhabricatorUserBadgesCacheType.php', + 'PhabricatorUserBanishTransaction' => 'applications/people/xaction/PhabricatorUserBanishTransaction.php', 'PhabricatorUserBlurbField' => 'applications/people/customfield/PhabricatorUserBlurbField.php', 'PhabricatorUserCache' => 'applications/people/storage/PhabricatorUserCache.php', 'PhabricatorUserCachePurger' => 'applications/cache/purger/PhabricatorUserCachePurger.php', @@ -10673,6 +10674,7 @@ ), 'PhabricatorUserApproveTransaction' => 'PhabricatorUserTransactionType', 'PhabricatorUserBadgesCacheType' => 'PhabricatorUserCacheType', + 'PhabricatorUserBanishTransaction' => 'PhabricatorUserTransactionType', 'PhabricatorUserBlurbField' => 'PhabricatorUserCustomField', 'PhabricatorUserCache' => 'PhabricatorUserDAO', 'PhabricatorUserCachePurger' => 'PhabricatorCachePurger', diff --git a/src/applications/auth/controller/PhabricatorDisabledUserController.php b/src/applications/auth/controller/PhabricatorDisabledUserController.php --- a/src/applications/auth/controller/PhabricatorDisabledUserController.php +++ b/src/applications/auth/controller/PhabricatorDisabledUserController.php @@ -15,10 +15,18 @@ return new Aphront404Response(); } + if ($viewer->getIsBanished()) { + $title = pht('Account Banished'); + $body = pht('Your account has been banished.'); + } else { + $title = pht('Account Disabled'); + $body = pht('Your account has been disabled.'); + } + return $this->newDialog() - ->setTitle(pht('Account Disabled')) - ->addCancelButton('/logout/', pht('Okay')) - ->appendParagraph(pht('Your account has been disabled.')); + ->setTitle($title) + ->appendParagraph($body) + ->addCancelButton('/logout/', pht('Okay')); } } diff --git a/src/applications/people/application/PhabricatorPeopleApplication.php b/src/applications/people/application/PhabricatorPeopleApplication.php --- a/src/applications/people/application/PhabricatorPeopleApplication.php +++ b/src/applications/people/application/PhabricatorPeopleApplication.php @@ -52,9 +52,7 @@ => 'PhabricatorPeopleInviteSendController', ), 'approve/(?P[1-9]\d*)/' => 'PhabricatorPeopleApproveController', - '(?Pdisapprove)/(?P[1-9]\d*)/' - => 'PhabricatorPeopleDisableController', - '(?Pdisable)/(?P[1-9]\d*)/' + '(?Pdisapprove|disable|banish)/(?P[1-9]\d*)/' => 'PhabricatorPeopleDisableController', 'empower/(?P[1-9]\d*)/' => 'PhabricatorPeopleEmpowerController', 'delete/(?P[1-9]\d*)/' => 'PhabricatorPeopleDeleteController', diff --git a/src/applications/people/controller/PhabricatorPeopleDisableController.php b/src/applications/people/controller/PhabricatorPeopleDisableController.php --- a/src/applications/people/controller/PhabricatorPeopleDisableController.php +++ b/src/applications/people/controller/PhabricatorPeopleDisableController.php @@ -29,6 +29,8 @@ // Disabling via "Disable" requires the permission only. $is_disapprove = ($via == 'disapprove'); + $is_banish = ($via == 'banish'); + if ($is_disapprove) { $done_uri = $this->getApplicationURI('query/approval/'); @@ -63,15 +65,28 @@ ->setTitle(pht('Something Stays Your Hand')) ->appendParagraph( pht( - 'Try as you might, you find you can not disable your own account.')) + 'Try as you might, you find you can not disable or banish '. + 'your own account.')) ->addCancelButton($done_uri, pht('Curses!')); } + // Make the "undisable/unbanish" action always apply the appropriate + // type of change, regardless of how the controller was invoked. + if (!$should_disable) { + $is_banish = $user->getIsBanished(); + } + if ($request->isFormPost()) { $xactions = array(); + if ($is_banish) { + $xaction_type = PhabricatorUserBanishTransaction::TRANSACTIONTYPE; + } else { + $xaction_type = PhabricatorUserDisableTransaction::TRANSACTIONTYPE; + } + $xactions[] = id(new PhabricatorUserTransaction()) - ->setTransactionType(PhabricatorUserDisableTransaction::TRANSACTIONTYPE) + ->setTransactionType($xaction_type) ->setNewValue($should_disable); id(new PhabricatorUserTransactionEditor()) @@ -85,26 +100,52 @@ return id(new AphrontRedirectResponse())->setURI($done_uri); } - if ($should_disable) { - $title = pht('Disable User?'); - $short_title = pht('Disable User'); - - $body = pht( - 'Disable %s? They will no longer be able to access Phabricator or '. - 'receive email.', - phutil_tag('strong', array(), $user->getUsername())); + $display_username = phutil_tag('strong', array(), $user->getUsername()); - $submit = pht('Disable User'); + if ($should_disable) { + if ($is_banish) { + $title = pht('Banish User?'); + $short_title = pht('Banish User'); + + $body = pht( + 'Banish %s? They will no longer be able to access Phabricator or '. + 'receive email, and their profile and comments will be hidden.', + $display_username); + + $submit = pht('Banish User'); + } else { + $title = pht('Disable User?'); + $short_title = pht('Disable User'); + + $body = pht( + 'Disable %s? They will no longer be able to access Phabricator or '. + 'receive email.', + $display_username); + + $submit = pht('Disable User'); + } } else { - $title = pht('Enable User?'); - $short_title = pht('Enable User'); - - $body = pht( - 'Enable %s? They will be able to access Phabricator and receive '. - 'email again.', - phutil_tag('strong', array(), $user->getUsername())); - - $submit = pht('Enable User'); + if ($is_banish) { + $title = pht('Unbanish User?'); + $short_title = pht('Unbanish User'); + + $body = pht( + 'Unbanish %s? They will be able to access Phabricator and receive '. + 'email again, and their profile and comments will become visible.', + $display_username); + + $submit = pht('Unbanish User'); + } else { + $title = pht('Enable User?'); + $short_title = pht('Enable User'); + + $body = pht( + 'Enable %s? They will be able to access Phabricator and receive '. + 'email again.', + $display_username); + + $submit = pht('Enable User'); + } } return $this->newDialog() diff --git a/src/applications/people/controller/PhabricatorPeopleProfileBadgesController.php b/src/applications/people/controller/PhabricatorPeopleProfileBadgesController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileBadgesController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileBadgesController.php @@ -16,6 +16,11 @@ return new Aphront404Response(); } + $response = $this->newProfileAccessResponse($user); + if ($response) { + return $response; + } + $class = 'PhabricatorBadgesApplication'; if (!PhabricatorApplication::isClassInstalledForViewer($class, $viewer)) { return new Aphront404Response(); diff --git a/src/applications/people/controller/PhabricatorPeopleProfileCommitsController.php b/src/applications/people/controller/PhabricatorPeopleProfileCommitsController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileCommitsController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileCommitsController.php @@ -18,6 +18,11 @@ return new Aphront404Response(); } + $response = $this->newProfileAccessResponse($user); + if ($response) { + return $response; + } + $class = 'PhabricatorDiffusionApplication'; if (!PhabricatorApplication::isClassInstalledForViewer($class, $viewer)) { return new Aphront404Response(); diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php @@ -74,6 +74,9 @@ if ($user->getIsAdmin()) { $roles[] = pht('Administrator'); } + if ($user->getIsBanished()) { + $roles[] = pht('Banished'); + } if ($user->getIsDisabled()) { $roles[] = pht('Disabled'); } @@ -106,7 +109,9 @@ require_celerity_resource('project-view-css'); - if ($user->getIsDisabled()) { + if ($user->getIsBanished()) { + $header->setStatus('fa-gavel', 'red', pht('Banished')); + } else if ($user->getIsDisabled()) { $header->setStatus('fa-ban', 'red', pht('Disabled')); } else { $header->setStatus($profile_icon, 'bluegrey', $profile_title); @@ -125,4 +130,23 @@ return $header; } + protected function newProfileAccessResponse(PhabricatorUser $user) { + if ($user->getIsBanished()) { + $has_disable = $this->hasApplicationCapability( + PeopleDisableUsersCapability::CAPABILITY); + + if (!$has_disable) { + return $this->newDialog() + ->setTitle(pht('User Banished')) + ->appendParagraph( + pht( + 'This user has been banished. You must have the '. + '"Can Disable Users" permission to view their profile.')) + ->addCancelButton('/', pht('Done')); + } + } + + return null; + } + } diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php @@ -22,6 +22,11 @@ return new Aphront404Response(); } + $response = $this->newProfileAccessResponse($user); + if ($response) { + return $response; + } + $this->setUser($user); $header = $this->buildProfileHeader(); @@ -144,12 +149,29 @@ ->setWorkflow(true) ->setHref($this->getApplicationURI('rename/'.$user->getID().'/'))); - if ($user->getIsDisabled()) { + $curtain->addAction( + id(new PhabricatorActionView()) + ->setIcon('fa-envelope') + ->setName(pht('Send Welcome Email')) + ->setWorkflow(true) + ->setDisabled(!$can_welcome) + ->setHref($this->getApplicationURI('welcome/'.$user->getID().'/'))); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setType(PhabricatorActionView::TYPE_DIVIDER)); + + $show_banish = false; + if ($user->getIsBanished()) { + $disable_icon = 'fa-check-circle-o'; + $disable_name = pht('Unbanish User'); + } else if ($user->getIsDisabled()) { $disable_icon = 'fa-check-circle-o'; $disable_name = pht('Enable User'); } else { $disable_icon = 'fa-ban'; $disable_name = pht('Disable User'); + $show_banish = true; } $curtain->addAction( @@ -160,6 +182,16 @@ ->setWorkflow(true) ->setHref($this->getApplicationURI('disable/'.$user->getID().'/'))); + if ($show_banish) { + $curtain->addAction( + id(new PhabricatorActionView()) + ->setIcon('fa-gavel') + ->setName(pht('Banish User')) + ->setDisabled(!$can_disable) + ->setWorkflow(true) + ->setHref($this->getApplicationURI('banish/'.$user->getID().'/'))); + } + $curtain->addAction( id(new PhabricatorActionView()) ->setIcon('fa-times') @@ -170,11 +202,7 @@ $curtain->addAction( id(new PhabricatorActionView()) - ->setIcon('fa-envelope') - ->setName(pht('Send Welcome Email')) - ->setWorkflow(true) - ->setDisabled(!$can_welcome) - ->setHref($this->getApplicationURI('welcome/'.$user->getID().'/'))); + ->setType(PhabricatorActionView::TYPE_DIVIDER)); return $curtain; } diff --git a/src/applications/people/controller/PhabricatorPeopleProfileRevisionsController.php b/src/applications/people/controller/PhabricatorPeopleProfileRevisionsController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileRevisionsController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileRevisionsController.php @@ -18,6 +18,11 @@ return new Aphront404Response(); } + $response = $this->newProfileAccessResponse($user); + if ($response) { + return $response; + } + $class = 'PhabricatorDifferentialApplication'; if (!PhabricatorApplication::isClassInstalledForViewer($class, $viewer)) { return new Aphront404Response(); diff --git a/src/applications/people/controller/PhabricatorPeopleProfileTasksController.php b/src/applications/people/controller/PhabricatorPeopleProfileTasksController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileTasksController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileTasksController.php @@ -18,6 +18,11 @@ return new Aphront404Response(); } + $response = $this->newProfileAccessResponse($user); + if ($response) { + return $response; + } + $class = 'PhabricatorManiphestApplication'; if (!PhabricatorApplication::isClassInstalledForViewer($class, $viewer)) { return new Aphront404Response(); diff --git a/src/applications/people/controller/PhabricatorPeopleProfileViewController.php b/src/applications/people/controller/PhabricatorPeopleProfileViewController.php --- a/src/applications/people/controller/PhabricatorPeopleProfileViewController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileViewController.php @@ -21,6 +21,11 @@ return new Aphront404Response(); } + $response = $this->newProfileAccessResponse($user); + if ($response) { + return $response; + } + $this->setUser($user); $header = $this->buildProfileHeader(); diff --git a/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php b/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php --- a/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php +++ b/src/applications/people/phid/PhabricatorPeopleUserPHIDType.php @@ -38,14 +38,21 @@ foreach ($handles as $phid => $handle) { $user = $objects[$phid]; - $realname = $user->getRealName(); $username = $user->getUsername(); + if ($user->getIsBanished()) { + $profile_image = celerity_get_resource_uri('/rsrc/image/avatar.png'); + $full_name = $username; + } else { + $profile_image = $user->getProfileImageURI(); + $full_name = $user->getFullName(); + } + $handle ->setName($username) ->setURI('/p/'.$username.'/') - ->setFullName($user->getFullName()) - ->setImageURI($user->getProfileImageURI()) + ->setFullName($full_name) + ->setImageURI($profile_image) ->setMailStampName('@'.$username); if ($user->getIsMailingList()) { diff --git a/src/applications/people/query/PhabricatorPeopleQuery.php b/src/applications/people/query/PhabricatorPeopleQuery.php --- a/src/applications/people/query/PhabricatorPeopleQuery.php +++ b/src/applications/people/query/PhabricatorPeopleQuery.php @@ -19,6 +19,7 @@ private $nameTokens; private $namePrefixes; private $isEnrolledInMultiFactor; + private $isBanished; private $needPrimaryEmail; private $needProfile; @@ -87,6 +88,11 @@ return $this; } + public function withIsBanished($banished) { + $this->isBanished = $banished; + return $this; + } + public function withNameLike($like) { $this->nameLike = $like; return $this; @@ -356,6 +362,13 @@ (int)$this->isEnrolledInMultiFactor); } + if ($this->isBanished !== null) { + $where[] = qsprintf( + $conn, + 'user.isBanished = %d', + (int)$this->isBanished); + } + return $where; } diff --git a/src/applications/people/query/PhabricatorPeopleSearchEngine.php b/src/applications/people/query/PhabricatorPeopleSearchEngine.php --- a/src/applications/people/query/PhabricatorPeopleSearchEngine.php +++ b/src/applications/people/query/PhabricatorPeopleSearchEngine.php @@ -84,6 +84,17 @@ pht( 'Pass true to find only users awaiting administrative approval, '. 'or false to omit these users.')), + id(new PhabricatorSearchThreeStateField()) + ->setLabel(pht('Banished')) + ->setKey('banished') + ->setOptions( + pht('(Show All)'), + pht('Show Only Banished Users'), + pht('Hide Banished Users')) + ->setDescription( + pht( + 'Pass true to find only users who have been banished, '. + 'or false to omit these users.')), ); $viewer = $this->requireViewer(); @@ -169,6 +180,10 @@ $query->withIsApproved(!$map['needsApproval']); } + if ($map['banished'] !== null) { + $query->withIsBanished($map['banished']); + } + if (idx($map, 'mfa') !== null) { $viewer = $this->requireViewer(); if (!$viewer->getIsAdmin()) { @@ -263,7 +278,10 @@ $item->addAttribute($primary_email->getAddress()); } - if ($user->getIsDisabled()) { + if ($user->getIsBanished()) { + $item->addIcon('fa-gavel', pht('Banished')); + $item->setDisabled(true); + } else if ($user->getIsDisabled()) { $item->addIcon('fa-ban', pht('Disabled')); $item->setDisabled(true); } @@ -336,9 +354,15 @@ $export = array(); foreach ($users as $user) { + if ($user->getIsBanished()) { + $real_name = null; + } else { + $real_name = $user->getRealName(); + } + $export[] = array( 'username' => $user->getUsername(), - 'realName' => $user->getRealName(), + 'realName' => $real_name, ); } 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 @@ -44,6 +44,7 @@ protected $isEmailVerified = 0; protected $isApproved = 0; protected $isEnrolledInMultiFactor = 0; + protected $isBanished = 0; protected $accountSecret; @@ -113,6 +114,10 @@ return false; } + if ($this->getIsBanished()) { + return false; + } + if (PhabricatorUserEmail::isEmailVerificationRequired()) { if (!$this->getIsEmailVerified()) { return false; @@ -229,6 +234,7 @@ 'availabilityCacheTTL' => 'uint32?', 'defaultProfileImagePHID' => 'phid?', 'defaultProfileImageVersion' => 'text64?', + 'isBanished' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, @@ -246,6 +252,9 @@ 'key_approved' => array( 'columns' => array('isApproved'), ), + 'key_banished' => array( + 'columns' => array('isBanished'), + ), ), self::CONFIG_NO_MUTATE => array( 'availabilityCache' => true, @@ -1443,9 +1452,19 @@ $roles[] = 'activated'; } + if ($this->getIsBanished()) { + $roles[] = 'banished'; + } + + if ($this->getIsBanished()) { + $real_name = null; + } else { + $real_name = $this->getRealName(); + } + return array( 'username' => $this->getUsername(), - 'realName' => $this->getRealName(), + 'realName' => $real_name, 'roles' => $roles, ); } diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php --- a/src/applications/people/storage/PhabricatorUserLog.php +++ b/src/applications/people/storage/PhabricatorUserLog.php @@ -18,6 +18,7 @@ const ACTION_SYSTEM_AGENT = 'system-agent'; const ACTION_MAILING_LIST = 'mailing-list'; const ACTION_DISABLE = 'disable'; + const ACTION_BANISH = 'banish'; const ACTION_APPROVE = 'approve'; const ACTION_DELETE = 'delete'; @@ -83,6 +84,7 @@ self::ACTION_FAIL_HISEC => pht('Hisec: Failed Attempt'), self::ACTION_MULTI_ADD => pht('Multi-Factor: Add Factor'), self::ACTION_MULTI_REMOVE => pht('Multi-Factor: Remove Factor'), + self::ACTION_BANISH => pht('Banish'), ); } diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserBanishTransaction.php copy from src/applications/people/xaction/PhabricatorUserDisableTransaction.php copy to src/applications/people/xaction/PhabricatorUserBanishTransaction.php --- a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php +++ b/src/applications/people/xaction/PhabricatorUserBanishTransaction.php @@ -1,12 +1,12 @@ getIsDisabled(); + return (bool)$object->getIsBanished(); } public function generateNewValue($object, $value) { @@ -14,12 +14,14 @@ } public function applyInternalEffects($object, $value) { - $object->setIsDisabled((int)$value); + $object + ->setIsDisabled((int)$value) + ->setIsBanished((int)$value); } public function applyExternalEffects($object, $value) { $this->newUserLog(PhabricatorUserLog::ACTION_DISABLE) - ->setOldValue((bool)$object->getIsDisabled()) + ->setOldValue((bool)$object->getIsBanished()) ->setNewValue((bool)$value) ->save(); } @@ -28,18 +30,16 @@ $new = $this->getNewValue(); if ($new) { return pht( - '%s disabled this user.', + '%s banished this user to the shadow realm.', $this->renderAuthor()); } else { return pht( - '%s enabled this user.', + '%s unbanished this user.', $this->renderAuthor()); } } public function shouldHideForFeed() { - // Don't publish feed stories about disabling users, since this can be - // a sensitive action. return true; } @@ -47,19 +47,19 @@ $errors = array(); foreach ($xactions as $xaction) { - $is_disabled = (bool)$object->getIsDisabled(); + $is_banished = (bool)$object->getIsBanished(); - if ((bool)$xaction->getNewValue() === $is_disabled) { + if ((bool)$xaction->getNewValue() === $is_banished) { continue; } - // You must have the "Can Disable Users" permission to disable a user. + // You must have the "Can Disable Users" permission to banish a user. $this->requireApplicationCapability( PeopleDisableUsersCapability::CAPABILITY); if ($this->getActingAsPHID() === $object->getPHID()) { $errors[] = $this->newInvalidError( - pht('You can not enable or disable your own account.')); + pht('You can not banish or unbanish your own account.')); } } @@ -69,11 +69,6 @@ public function getRequiredCapabilities( $object, PhabricatorApplicationTransaction $xaction) { - - // You do not need to be able to edit users to disable them. Instead, this - // requirement is replaced with a requirement that you have the "Can - // Disable Users" permission. - return null; } } diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php --- a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php +++ b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php @@ -53,13 +53,23 @@ continue; } + if ($object->getIsBanished()) { + $errors[] = $this->newInvalidError( + pht( + 'This user has been banished. To restore their account, you '. + 'must unbanish them.'), + $xaction); + continue; + } + // You must have the "Can Disable Users" permission to disable a user. $this->requireApplicationCapability( PeopleDisableUsersCapability::CAPABILITY); if ($this->getActingAsPHID() === $object->getPHID()) { $errors[] = $this->newInvalidError( - pht('You can not enable or disable your own account.')); + pht('You can not enable or disable your own account.'), + $xaction); } } diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -104,6 +104,7 @@ ->setViewer($viewer) ->setTemplate($template) ->withTransactionPHIDs(mpull($xactions, 'getPHID')) + ->needBanishments(true) ->execute(); $comment_map = msort($comment_map, 'getCommentVersion'); @@ -160,9 +161,16 @@ $comment_data = array(); if ($comments) { $removed = head($comments)->getIsDeleted(); + $banishment = head($comments)->getBanishment(); foreach ($comments as $comment) { - if ($removed) { + if ($banishment) { + // For consistency with the web UI, don't show any of the text if + // the author has been banished. + $content = array( + 'raw' => '', + ); + } else if ($removed) { // If the most recent version of the comment has been removed, // don't show the history. This is for consistency with the web // UI, which also prevents users from retrieving the content of diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php @@ -9,6 +9,11 @@ $xaction = id(new PhabricatorObjectQuery()) ->setViewer($viewer) ->withPHIDs(array($request->getURIData('phid'))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); if (!$xaction) { return new Aphront404Response(); diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php @@ -15,25 +15,20 @@ ->withPHIDs(array($phid)) ->setViewer($viewer) ->executeOne(); - if (!$xaction) { return new Aphront404Response(); } - if (!$xaction->getComment()) { - // You can't view history of a transaction with no comments. - return new Aphront404Response(); - } - - if ($xaction->getComment()->getIsRemoved()) { - // You can't view history of a transaction with a removed comment. - return new Aphront400Response(); + $response = $this->newAccessResponse($xaction); + if ($response) { + return $response; } $comments = id(new PhabricatorApplicationTransactionTemplatedCommentQuery()) ->setViewer($viewer) ->setTemplate($xaction->getApplicationTransactionCommentObject()) ->withTransactionPHIDs(array($xaction->getPHID())) + ->needBanishments(true) ->execute(); if (!$comments) { diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentQuoteController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentQuoteController.php --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentQuoteController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentQuoteController.php @@ -15,16 +15,9 @@ return new Aphront404Response(); } - if (!$xaction->getComment()) { - return new Aphront404Response(); - } - - if ($xaction->getComment()->getIsRemoved()) { - return new Aphront400Response(); - } - - if (!$xaction->hasComment()) { - return new Aphront404Response(); + $response = $this->newAccessResponse($xaction); + if ($response) { + return $response; } $content = $xaction->getComment()->getContent(); diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRawController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRawController.php --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRawController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentRawController.php @@ -3,6 +3,10 @@ final class PhabricatorApplicationTransactionCommentRawController extends PhabricatorApplicationTransactionController { + public function shouldAllowPublic() { + return true; + } + public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); $phid = $request->getURIData('phid'); @@ -11,19 +15,13 @@ ->withPHIDs(array($phid)) ->setViewer($viewer) ->executeOne(); - if (!$xaction) { return new Aphront404Response(); } - if (!$xaction->getComment()) { - // You can't view a raw comment if there is no comment. - return new Aphront404Response(); - } - - if ($xaction->getComment()->getIsRemoved()) { - // You can't view a raw comment if the comment is deleted. - return new Aphront400Response(); + $response = $this->newAccessResponse($xaction); + if ($response) { + return $response; } $obj_phid = $xaction->getObjectPHID(); @@ -32,6 +30,8 @@ ->withPHIDs(array($obj_phid)) ->executeOne(); + $done_uri = $obj_handle->getURI(); + $title = pht('Raw Comment'); $body = $xaction->getComment()->getContent(); $addendum = null; @@ -55,9 +55,8 @@ } } } - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) - ->addCancelButton($obj_handle->getURI()) + $dialog = $this->newDialog() + ->addCancelButton($done_uri) ->setTitle($title); $dialog @@ -73,11 +72,7 @@ $dialog->appendParagraph($addendum); } - return id(new AphrontDialogResponse())->setDialog($dialog); - } - - public function shouldAllowPublic() { - return true; + return $dialog; } } diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionController.php --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionController.php @@ -24,4 +24,49 @@ return $cancel_uri; } + protected function newAccessResponse( + PhabricatorApplicationTransaction $xaction) { + $viewer = $this->getViewer(); + + $comment = $xaction->getComment(); + if (!$comment) { + return new Aphront404Response(); + } + + $obj_phid = $xaction->getObjectPHID(); + $obj_handle = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($obj_phid)) + ->executeOne(); + + $done_uri = $obj_handle->getURI(); + + if ($comment->getIsRemoved()) { + return $this->newDialog() + ->setTitle(pht('Comment Removed')) + ->appendParagraph( + pht('This comment has been removed.')) + ->addCancelButton($done_uri); + } + + if ($comment->getIsDeleted()) { + return $this->newDialog() + ->setTitle(pht('Comment Deleted')) + ->appendParagraph( + pht('This comment has been deleted.')) + ->addCancelButton($done_uri); + } + + if ($comment->getBanishment()) { + return $this->newDialog() + ->setTitle(pht('Author Banished')) + ->appendParagraph( + pht('The user who made this comment has been banished.')) + ->addCancelButton($done_uri); + } + + return null; + } + + } diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php --- a/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php @@ -9,6 +9,7 @@ private $transactionPHIDs; private $isDeleted; private $hasTransaction; + private $needBanishments; abstract protected function getTemplate(); @@ -42,6 +43,11 @@ return $this; } + public function needBanishments($banishments) { + $this->needBanishments = $banishments; + return $this; + } + protected function loadPage() { $table = $this->getTemplate(); $conn_r = $table->establishConnection('r'); @@ -118,6 +124,44 @@ return $where; } + protected function didFilterPage(array $comments) { + if ($this->needBanishments) { + $type_user = PhabricatorPeopleUserPHIDType::TYPECONST; + + $author_phids = array(); + foreach ($comments as $comment) { + $author_phid = $comment->getAuthorPHID(); + if (!$author_phid) { + continue; + } + + if (phid_get_type($author_phid) !== $type_user) { + continue; + } + + $author_phids[$author_phid] = $author_phid; + } + + if ($author_phids) { + $authors = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($author_phids) + ->withIsBanished(true) + ->execute(); + $authors = mpull($authors, null, 'getPHID'); + } else { + $authors = array(); + } + + foreach ($comments as $comment) { + $author_phid = $comment->getAuthorPHID(); + $comment->attachBanishment((bool)idx($authors, $author_phid)); + } + } + + return $comments; + } + public function getQueryApplicationClass() { // TODO: Figure out the app via the template? return null; diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php --- a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php @@ -89,6 +89,7 @@ ->setTemplate($table->getApplicationTransactionCommentObject()) ->setViewer($this->getViewer()) ->withPHIDs($comment_phids) + ->needBanishments(true) ->execute(); $comments = mpull($comments, null, 'getPHID'); } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php @@ -19,6 +19,7 @@ protected $isDeleted = 0; private $oldComment = self::ATTACHABLE; + private $banishment = self::ATTACHABLE; abstract public function getApplicationTransactionObject(); @@ -101,6 +102,15 @@ return ($this->oldComment !== self::ATTACHABLE); } + public function attachBanishment($banishment) { + $this->banishment = $banishment; + return $this; + } + + public function getBanishment() { + return $this->assertAttached($this->banishment); + } + /* -( PhabricatorMarkupInterface )----------------------------------------- */ diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -294,7 +294,10 @@ $comment = $xaction->getComment(); if ($comment) { - if ($comment->getIsRemoved()) { + if ($comment->getBanishment()) { + // If the comment author has been banished, don't render anything. + return null; + } else if ($comment->getIsRemoved()) { return javelin_tag( 'span', array(