From 04b9f946020de32c513b69954933630641e8ff8f Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Apr 2014 12:06:05 -0700 Subject: [PATCH] Give administrators selective access to System Agent settings panels Summary: Ref T4065. Give administrators an "Edit Settings" link from profiles, which allows selective edit of settings panels. Enable Conduit, SSH Keys, and VCS Password. Test Plan: - Used these panels for a bot. - Used these panels on my own account. - Tried to use these panels for a non-bot account, was denied. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4065 Differential Revision: https://secure.phabricator.com/D8668 --- .../panel/DiffusionSetPasswordPanel.php | 18 +++-- .../PhabricatorPeopleEditController.php | 45 ------------ .../PhabricatorPeopleProfileController.php | 8 +++ .../PhabricatorApplicationSettings.php | 3 +- .../PhabricatorSettingsMainController.php | 71 ++++++++++++++++++- .../panel/PhabricatorSettingsPanel.php | 43 ++++++++++- .../panel/PhabricatorSettingsPanelConduit.php | 15 ++-- .../panel/PhabricatorSettingsPanelSSHKeys.php | 66 +++++++++++------ 8 files changed, 190 insertions(+), 79 deletions(-) diff --git a/src/applications/diffusion/panel/DiffusionSetPasswordPanel.php b/src/applications/diffusion/panel/DiffusionSetPasswordPanel.php index e896a03184..b9d90c8306 100644 --- a/src/applications/diffusion/panel/DiffusionSetPasswordPanel.php +++ b/src/applications/diffusion/panel/DiffusionSetPasswordPanel.php @@ -2,6 +2,10 @@ final class DiffusionSetPasswordPanel extends PhabricatorSettingsPanel { + public function isEditableByAdministrators() { + return true; + } + public function getPanelKey() { return 'vcspassword'; } @@ -19,7 +23,8 @@ final class DiffusionSetPasswordPanel extends PhabricatorSettingsPanel { } public function processRequest(AphrontRequest $request) { - $user = $request->getUser(); + $viewer = $request->getUser(); + $user = $this->getUser(); $vcspassword = id(new PhabricatorRepositoryVCSPassword()) ->loadOneWhere( @@ -68,7 +73,12 @@ final class DiffusionSetPasswordPanel extends PhabricatorSettingsPanel { $e_password = pht('Does Not Match'); $e_confirm = pht('Does Not Match'); $errors[] = pht('Password and confirmation do not match.'); - } else if ($user->comparePassword($envelope)) { + } else if ($viewer->comparePassword($envelope)) { + // NOTE: The above test is against $viewer (not $user), so that the + // error message below makes sense in the case that the two are + // different, and because an admin reusing their own password is bad, + // while system agents generally do not have passwords anyway. + $e_password = pht('Not Unique'); $e_confirm = pht('Not Unique'); $errors[] = pht( @@ -97,7 +107,7 @@ final class DiffusionSetPasswordPanel extends PhabricatorSettingsPanel { $title = pht('Set VCS Password'); $form = id(new AphrontFormView()) - ->setUser($user) + ->setUser($viewer) ->appendRemarkupInstructions( pht( 'To access repositories hosted by Phabricator over HTTP, you must '. @@ -193,7 +203,7 @@ final class DiffusionSetPasswordPanel extends PhabricatorSettingsPanel { ->setFormErrors($errors); $remove_form = id(new AphrontFormView()) - ->setUser($user); + ->setUser($viewer); if ($vcspassword->getID()) { $remove_form diff --git a/src/applications/people/controller/PhabricatorPeopleEditController.php b/src/applications/people/controller/PhabricatorPeopleEditController.php index 1f0d2fe909..f447323a22 100644 --- a/src/applications/people/controller/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleEditController.php @@ -35,7 +35,6 @@ final class PhabricatorPeopleEditController $nav->setBaseURI(new PhutilURI($base_uri)); $nav->addLabel(pht('User Information')); $nav->addFilter('basic', pht('Basic Information')); - $nav->addFilter('cert', pht('Conduit Certificate')); $nav->addFilter('profile', pht('View Profile'), '/p/'.$user->getUsername().'/'); @@ -60,9 +59,6 @@ final class PhabricatorPeopleEditController case 'basic': $response = $this->processBasicRequest($user); break; - case 'cert': - $response = $this->processCertificateRequest($user); - break; default: return new Aphront404Response(); } @@ -327,47 +323,6 @@ final class PhabricatorPeopleEditController return array($form_box); } - private function processCertificateRequest($user) { - $request = $this->getRequest(); - $admin = $request->getUser(); - - $inst = pht('You can use this certificate '. - 'to write scripts or bots which interface with Phabricator over '. - 'Conduit.'); - $form = new AphrontFormView(); - $form - ->setUser($admin) - ->setAction($request->getRequestURI()) - ->appendChild( - phutil_tag('p', array('class' => 'aphront-form-instructions'), $inst)); - - if ($user->getIsSystemAgent()) { - $form - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Username')) - ->setValue($user->getUsername())) - ->appendChild( - id(new AphrontFormTextAreaControl()) - ->setLabel(pht('Certificate')) - ->setValue($user->getConduitCertificate())); - } else { - $form->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('Certificate')) - ->setValue( - pht('You may only view the certificates of System Agents.'))); - } - - $title = pht('Conduit Certificate'); - - $form_box = id(new PHUIObjectBoxView()) - ->setHeaderText($title) - ->setForm($form); - - return array($form_box); - } - private function getRoleInstructions() { $roles_link = phutil_tag( 'a', diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php index 18d6390da7..c6091fb187 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php @@ -64,6 +64,14 @@ final class PhabricatorPeopleProfileController ->setWorkflow(!$can_edit)); if ($viewer->getIsAdmin()) { + $actions->addAction( + id(new PhabricatorActionView()) + ->setIcon('wrench') + ->setName(pht('Edit Settings')) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit) + ->setHref('/settings/'.$user->getID().'/')); + if ($user->getIsAdmin()) { $empower_icon = 'lower-priority'; $empower_name = pht('Remove Administrator'); diff --git a/src/applications/settings/application/PhabricatorApplicationSettings.php b/src/applications/settings/application/PhabricatorApplicationSettings.php index 40950dedbf..8e4aa91e89 100644 --- a/src/applications/settings/application/PhabricatorApplicationSettings.php +++ b/src/applications/settings/application/PhabricatorApplicationSettings.php @@ -21,7 +21,8 @@ final class PhabricatorApplicationSettings extends PhabricatorApplication { public function getRoutes() { return array( '/settings/' => array( - '(?:panel/(?P[^/]+)/)?' => 'PhabricatorSettingsMainController', + '(?:(?P\d+)/)?(?:panel/(?P[^/]+)/)?' + => 'PhabricatorSettingsMainController', 'adjust/' => 'PhabricatorSettingsAdjustController', ), ); diff --git a/src/applications/settings/controller/PhabricatorSettingsMainController.php b/src/applications/settings/controller/PhabricatorSettingsMainController.php index 2b20285b73..572b087ff2 100644 --- a/src/applications/settings/controller/PhabricatorSettingsMainController.php +++ b/src/applications/settings/controller/PhabricatorSettingsMainController.php @@ -3,14 +3,48 @@ final class PhabricatorSettingsMainController extends PhabricatorController { + private $id; private $key; + private $user; + + private function getUser() { + return $this->user; + } + + private function isSelf() { + $viewer_phid = $this->getRequest()->getUser()->getPHID(); + $user_phid = $this->getUser()->getPHID(); + return ($viewer_phid == $user_phid); + } public function willProcessRequest(array $data) { + $this->id = idx($data, 'id'); $this->key = idx($data, 'key'); } public function processRequest() { $request = $this->getRequest(); + $viewer = $request->getUser(); + + if ($this->id) { + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withIDs(array($this->id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + + if (!$user) { + return new Aphront404Response(); + } + + $this->user = $user; + } else { + $this->user = $viewer; + } $panels = $this->buildPanels(); $nav = $this->renderSideNav($panels); @@ -19,13 +53,27 @@ final class PhabricatorSettingsMainController $panel = $panels[$key]; + $panel->setUser($this->getUser()); + $panel->setViewer($viewer); $response = $panel->processRequest($request); if ($response instanceof AphrontResponse) { return $response; } - $nav->appendChild($response); + $crumbs = $this->buildApplicationCrumbs(); + if (!$this->isSelf()) { + $crumbs->addTextCrumb( + $this->getUser()->getUsername(), + '/p/'.$this->getUser()->getUsername().'/'); + } + $crumbs->addTextCrumb($panel->getPanelName()); + $nav->appendChild( + array( + $crumbs, + $response, + )); + return $this->buildApplicationPage( $nav, array( @@ -54,6 +102,13 @@ final class PhabricatorSettingsMainController if (!$panel->isEnabled()) { continue; } + + if (!$this->isSelf()) { + if (!$panel->isEditableByAdministrators()) { + continue; + } + } + if (!empty($result[$key])) { throw new Exception(pht( "Two settings panels share the same panel key ('%s'): %s, %s.", @@ -61,17 +116,29 @@ final class PhabricatorSettingsMainController get_class($panel), get_class($result[$key]))); } + $result[$key] = $panel; } $result = msort($result, 'getPanelSortKey'); + if (!$result) { + throw new Exception(pht('No settings panels are available.')); + } + return $result; } private function renderSideNav(array $panels) { $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI($this->getApplicationURI('/panel/'))); + + if ($this->isSelf()) { + $base_uri = 'panel/'; + } else { + $base_uri = $this->getUser()->getID().'/panel/'; + } + + $nav->setBaseURI(new PhutilURI($this->getApplicationURI($base_uri))); $group = null; foreach ($panels as $panel) { diff --git a/src/applications/settings/panel/PhabricatorSettingsPanel.php b/src/applications/settings/panel/PhabricatorSettingsPanel.php index 630f80892b..daacf2140c 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanel.php @@ -17,6 +17,27 @@ */ abstract class PhabricatorSettingsPanel { + private $user; + private $viewer; + + public function setUser(PhabricatorUser $user) { + $this->user = $user; + return $this; + } + + public function getUser() { + return $this->user; + } + + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + /* -( Panel Configuration )------------------------------------------------ */ @@ -86,6 +107,18 @@ abstract class PhabricatorSettingsPanel { } + /** + * Return true if this panel is available to administrators while editing + * system agent accounts. + * + * @return bool True to enable edit by administrators. + * @task config + */ + public function isEditableByAdministrators() { + return false; + } + + /* -( Panel Implementation )----------------------------------------------- */ @@ -117,7 +150,15 @@ abstract class PhabricatorSettingsPanel { final public function getPanelURI($path = '') { $key = $this->getPanelKey(); $key = phutil_escape_uri($key); - return '/settings/panel/'.$key.'/'.ltrim($path, '/'); + + $path = ltrim($path, '/'); + + if ($this->getUser()->getPHID() != $this->getViewer()->getPHID()) { + $user_id = $this->getUser()->getID(); + return "/settings/{$user_id}/panel/{$key}/{$path}"; + } else { + return "/settings/panel/{$key}/{$path}"; + } } diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelConduit.php b/src/applications/settings/panel/PhabricatorSettingsPanelConduit.php index 4645c04d56..df050005c1 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelConduit.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanelConduit.php @@ -3,12 +3,16 @@ final class PhabricatorSettingsPanelConduit extends PhabricatorSettingsPanel { + public function isEditableByAdministrators() { + return true; + } + public function getPanelKey() { return 'conduit'; } public function getPanelName() { - return pht('Conduit'); + return pht('Conduit Certificate'); } public function getPanelGroup() { @@ -16,12 +20,13 @@ final class PhabricatorSettingsPanelConduit } public function processRequest(AphrontRequest $request) { - $user = $request->getUser(); + $user = $this->getUser(); + $viewer = $request->getUser(); if ($request->isFormPost()) { if (!$request->isDialogFormPost()) { $dialog = new AphrontDialogView(); - $dialog->setUser($user); + $dialog->setUser($viewer); $dialog->setTitle(pht('Really regenerate session?')); $dialog->setSubmitURI($this->getPanelURI()); $dialog->addSubmitButton(pht('Regenerate')); @@ -69,7 +74,7 @@ final class PhabricatorSettingsPanelConduit $cert_form = new AphrontFormView(); $cert_form - ->setUser($user) + ->setUser($viewer) ->appendChild(phutil_tag( 'p', array('class' => 'aphront-form-instructions'), @@ -93,7 +98,7 @@ final class PhabricatorSettingsPanelConduit $regen_form = new AphrontFormView(); $regen_form - ->setUser($user) + ->setUser($viewer) ->setAction($this->getPanelURI()) ->setWorkflow(true) ->appendChild(phutil_tag( diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php b/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php index 2ee5d21a76..0faf34863c 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php @@ -3,6 +3,10 @@ final class PhabricatorSettingsPanelSSHKeys extends PhabricatorSettingsPanel { + public function isEditableByAdministrators() { + return true; + } + public function getPanelKey() { return 'ssh'; } @@ -20,8 +24,8 @@ final class PhabricatorSettingsPanelSSHKeys } public function processRequest(AphrontRequest $request) { - - $user = $request->getUser(); + $viewer = $request->getUser(); + $user = $this->getUser(); $generate = $request->getStr('generate'); if ($generate) { @@ -37,7 +41,7 @@ final class PhabricatorSettingsPanelSSHKeys $id = nonempty($edit, $delete); if ($id && is_numeric($id)) { - // NOTE: Prevent editing/deleting of keys you don't own. + // NOTE: This prevents editing/deleting of keys not owned by the user. $key = id(new PhabricatorUserSSHKey())->loadOneWhere( 'userPHID = %s AND id = %d', $user->getPHID(), @@ -142,7 +146,7 @@ final class PhabricatorSettingsPanelSSHKeys } $form = id(new AphrontFormView()) - ->setUser($user) + ->setUser($viewer) ->addHiddenInput('edit', $is_new ? 'true' : $key->getID()) ->appendChild( id(new AphrontFormTextControl()) @@ -170,8 +174,8 @@ final class PhabricatorSettingsPanelSSHKeys } private function renderKeyListView(AphrontRequest $request) { - - $user = $request->getUser(); + $user = $this->getUser(); + $viewer = $request->getUser(); $keys = id(new PhabricatorUserSSHKey())->loadAllWhere( 'userPHID = %s', @@ -188,8 +192,8 @@ final class PhabricatorSettingsPanelSSHKeys $key->getName()), $key->getKeyComment(), $key->getKeyType(), - phabricator_date($key->getDateCreated(), $user), - phabricator_time($key->getDateCreated(), $user), + phabricator_date($key->getDateCreated(), $viewer), + phabricator_time($key->getDateCreated(), $viewer), javelin_tag( 'a', array( @@ -266,7 +270,8 @@ final class PhabricatorSettingsPanelSSHKeys AphrontRequest $request, PhabricatorUserSSHKey $key) { - $user = $request->getUser(); + $viewer = $request->getUser(); + $user = $this->getUser(); $name = phutil_tag('strong', array(), $key->getName()); @@ -277,7 +282,7 @@ final class PhabricatorSettingsPanelSSHKeys } $dialog = id(new AphrontDialogView()) - ->setUser($user) + ->setUser($viewer) ->addHiddenInput('delete', $key->getID()) ->setTitle(pht('Really delete SSH Public Key?')) ->appendChild(phutil_tag('p', array(), pht( @@ -291,10 +296,12 @@ final class PhabricatorSettingsPanelSSHKeys ->setDialog($dialog); } - private function processGenerate( - AphrontRequest $request) { + private function processGenerate(AphrontRequest $request) { + $user = $this->getUser(); $viewer = $request->getUser(); + $is_self = ($user->getPHID() == $viewer->getPHID()); + if ($request->isFormPost()) { $keys = PhabricatorSSHKeyGenerator::generateKeypair(); list($public_key, $private_key) = $keys; @@ -308,7 +315,7 @@ final class PhabricatorSettingsPanelSSHKeys )); $key = id(new PhabricatorUserSSHKey()) - ->setUserPHID($viewer->getPHID()) + ->setUserPHID($user->getPHID()) ->setName('id_rsa_phabricator') ->setKeyType('rsa') ->setKeyBody($public_key) @@ -320,6 +327,17 @@ final class PhabricatorSettingsPanelSSHKeys // disabling workflow on cancel so the page reloads, showing the new // key. + if ($is_self) { + $what_happened = pht( + 'The public key has been associated with your Phabricator '. + 'account. Use the button below to download the private key.'); + } else { + $what_happened = pht( + 'The public key has been associated with the %s account. '. + 'Use the button below to download the private key.', + phutil_tag('strong', array(), $user->getUsername())); + } + $dialog = id(new AphrontDialogView()) ->setTitle(pht('Download Private Key')) ->setUser($viewer) @@ -329,10 +347,7 @@ final class PhabricatorSettingsPanelSSHKeys ->appendParagraph( pht( 'Successfully generated a new keypair.')) - ->appendParagraph( - pht( - 'The public key has been associated with your Phabricator '. - 'account. Use the button below to download the private key.')) + ->appendParagraph($what_happened) ->appendParagraph( pht( 'After you download the private key, it will be destroyed. '. @@ -350,13 +365,22 @@ final class PhabricatorSettingsPanelSSHKeys try { PhabricatorSSHKeyGenerator::assertCanGenerateKeypair(); + + if ($is_self) { + $explain = pht( + 'This will generate an SSH keypair, associate the public key '. + 'with your account, and let you download the private key.'); + } else { + $explain = pht( + 'This will generate an SSH keypair, associate the public key with '. + 'the %s account, and let you download the private key.', + phutil_tag('strong', array(), $user->getUsername())); + } + $dialog ->addHiddenInput('generate', true) ->setTitle(pht('Generate New Keypair')) - ->appendParagraph( - pht( - "This will generate an SSH keypair, associate the public key ". - "with your account, and let you download the private key.")) + ->appendParagraph($explain) ->appendParagraph( pht( "Phabricator will not retain a copy of the private key."))