From 34c890b7e1a831ab935033f7bbc9df46988efeb8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Mar 2014 13:30:48 -0700 Subject: [PATCH] Use modern UI and policies in OAuth client editing Summary: Updates this stuff a bit: - Add a global create permission for OAuth applications. The primary goal is to reduce attack surface area by making it more difficult for an adversary to do anything which requires that they create and configure an OAuth application/client. Normal users shouldn't generally need to create applications, OAuth is complex, and doing things with user accounts is inherently somewhat administrative. - Use normal policies to check create and edit permissions, now that we have infrastructure for it. - Use modern UI kit. Test Plan: - Created a client. - Edited a client. - Tried to create a client as a non-admin. - Tried to edit a client I don't own. {F131511} {F131512} {F131513} {F131514} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D8562 --- src/__phutil_library_map__.php | 2 + .../PhabricatorApplicationOAuthServer.php | 8 + ...atorOAuthServerCapabilityCreateClients.php | 20 ++ .../PhabricatorOAuthServerController.php | 2 +- .../PhabricatorOAuthClientBaseController.php | 7 +- .../PhabricatorOAuthClientEditController.php | 230 +++++++----------- .../storage/PhabricatorOAuthServerClient.php | 17 ++ 7 files changed, 138 insertions(+), 148 deletions(-) create mode 100644 src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6cf2c6cd80..3fd843f268 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1718,6 +1718,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerAuthController' => 'applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php', 'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/PhabricatorOAuthServerAuthorizationCode.php', 'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'applications/oauthserver/panel/PhabricatorOAuthServerAuthorizationsSettingsPanel.php', + 'PhabricatorOAuthServerCapabilityCreateClients' => 'applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php', 'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/PhabricatorOAuthServerClient.php', 'PhabricatorOAuthServerClientQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerClientQuery.php', 'PhabricatorOAuthServerConsoleController' => 'applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php', @@ -4471,6 +4472,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerAuthController' => 'PhabricatorAuthController', 'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'PhabricatorSettingsPanel', + 'PhabricatorOAuthServerCapabilityCreateClients' => 'PhabricatorPolicyCapability', 'PhabricatorOAuthServerClient' => array( 0 => 'PhabricatorOAuthServerDAO', diff --git a/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php b/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php index a33d41563c..0c30e4e90b 100644 --- a/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php +++ b/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php @@ -48,4 +48,12 @@ final class PhabricatorApplicationOAuthServer extends PhabricatorApplication { ); } + protected function getCustomCapabilities() { + return array( + PhabricatorOAuthServerCapabilityCreateClients::CAPABILITY => array( + 'default' => PhabricatorPolicies::POLICY_ADMIN, + ), + ); + } + } diff --git a/src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php b/src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php new file mode 100644 index 0000000000..8b8bc5176e --- /dev/null +++ b/src/applications/oauthserver/capability/PhabricatorOAuthServerCapabilityCreateClients.php @@ -0,0 +1,20 @@ +getRequest()->getUser(); diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php index 3bbb2f2fd2..677444ac96 100644 --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientBaseController.php @@ -1,15 +1,14 @@ clientPHID; } + private function setClientPHID($phid) { $this->clientPHID = $phid; return $this; diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php index a326bfe060..46e65071fb 100644 --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php @@ -1,173 +1,117 @@ isEdit; - } - private function setIsClientEdit($is_edit) { - $this->isEdit = (bool) $is_edit; - return $this; - } - - protected function getExtraClientFilters() { - if ($this->isClientEdit()) { - $filters = array( - array('url' => $this->getFilter(), - 'label' => 'Edit Client') - ); - } else { - $filters = array(); - } - return $filters; - } - - public function getFilter() { - if ($this->isClientEdit()) { - $filter = 'client/edit/'.$this->getClientPHID(); - } else { - $filter = 'client/create'; - } - return $filter; - } + extends PhabricatorOAuthClientBaseController { public function processRequest() { - $request = $this->getRequest(); - $current_user = $request->getUser(); - $error = null; - $bad_redirect = false; - $phid = $this->getClientPHID(); - // if we have a phid, then we're editing - $this->setIsClientEdit($phid); + $request = $this->getRequest(); + $viewer = $request->getUser(); - if ($this->isClientEdit()) { - $client = id(new PhabricatorOAuthServerClient()) - ->loadOneWhere('phid = %s', - $phid); - $title = 'Edit OAuth Client'; - // validate the client - if (empty($client)) { + $phid = $this->getClientPHID(); + if ($phid) { + $client = id(new PhabricatorOAuthServerClientQuery()) + ->setViewer($viewer) + ->withPHIDs(array($phid)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$client) { return new Aphront404Response(); } - if ($client->getCreatorPHID() != $current_user->getPHID()) { - $message = 'Access denied to edit client with id '.$phid.'. '. - 'Only the user who created the client has permission to '. - 'edit the client.'; - return id(new Aphront403Response()) - ->setForbiddenText($message); - } - $submit_button = 'Save OAuth Client'; - $secret = null; - // new client - much simpler + + $title = pht('Edit OAuth Application: %s', $client->getName()); + $submit_button = pht('Save Application'); + $crumb_text = pht('Edit'); + $cancel_uri = $client->getViewURI(); + $is_new = false; } else { - $client = new PhabricatorOAuthServerClient(); - $title = 'Create OAuth Client'; - $submit_button = 'Create OAuth Client'; - $secret = Filesystem::readRandomCharacters(32); + $this->requireApplicationCapability( + PhabricatorOAuthServerCapabilityCreateClients::CAPABILITY); + + $client = PhabricatorOAuthServerClient::initializeNewClient($viewer); + + $title = pht('Create OAuth Application'); + $submit_button = pht('Create Application'); + $crumb_text = pht('Create Application'); + $cancel_uri = $this->getApplicationURI(); + $is_new = true; } + $errors = array(); + $e_redirect = true; + $e_name = true; if ($request->isFormPost()) { $redirect_uri = $request->getStr('redirect_uri'); $client->setName($request->getStr('name')); $client->setRedirectURI($redirect_uri); - if ($secret) { - $client->setSecret($secret); - } - $client->setCreatorPHID($current_user->getPHID()); - $uri = new PhutilURI($redirect_uri); - $server = new PhabricatorOAuthServer(); - if (!$server->validateRedirectURI($uri)) { - $error = new AphrontErrorView(); - $error->setSeverity(AphrontErrorView::SEVERITY_ERROR); - $error->setTitle( - 'Redirect URI must be a fully qualified domain name '. - 'with no fragments. See '. - 'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 '. - 'for more information on the correct format.'); - $bad_redirect = true; - } else { - $client->save(); - // refresh the phid in case its a create - $phid = $client->getPHID(); - if ($this->isClientEdit()) { - return id(new AphrontRedirectResponse()) - ->setURI('/oauthserver/client/?edited='.$phid); - } else { - return id(new AphrontRedirectResponse()) - ->setURI('/oauthserver/client/?new='.$phid); - } - } - } - $panel = new AphrontPanelView(); - if ($this->isClientEdit()) { - $delete_button = phutil_tag( - 'a', - array( - 'href' => $client->getDeleteURI(), - 'class' => 'grey button', - ), - 'Delete OAuth Client'); - $panel->addButton($delete_button); + if (!strlen($client->getName())) { + $errors[] = pht('You must choose a name for this OAuth application.'); + $e_name = pht('Required'); + } + + $server = new PhabricatorOAuthServer(); + $uri = new PhutilURI($redirect_uri); + if (!$server->validateRedirectURI($uri)) { + $errors[] = pht( + 'Redirect URI must be a fully qualified domain name '. + 'with no fragments. See %s for more information on the correct '. + 'format.', + 'http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2'); + $e_redirect = pht('Invalid'); + } + + if (!$errors) { + $client->save(); + $view_uri = $client->getViewURI(); + return id(new AphrontRedirectResponse())->setURI($view_uri); + } } - $panel->setHeader($title); $form = id(new AphrontFormView()) - ->setUser($current_user) + ->setUser($viewer) ->appendChild( id(new AphrontFormTextControl()) - ->setLabel('Name') - ->setName('name') - ->setValue($client->getName())); - if ($this->isClientEdit()) { - $form - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel('ID') - ->setValue($phid)) - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel('Secret') - ->setValue($client->getSecret())); - } - $form + ->setLabel('Name') + ->setName('name') + ->setValue($client->getName()) + ->setError($e_name)) ->appendChild( id(new AphrontFormTextControl()) - ->setLabel('Redirect URI') - ->setName('redirect_uri') - ->setValue($client->getRedirectURI()) - ->setError($bad_redirect)); - if ($this->isClientEdit()) { - $created = phabricator_datetime($client->getDateCreated(), - $current_user); - $updated = phabricator_datetime($client->getDateModified(), - $current_user); - $form - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel('Created') - ->setValue($created)) - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel('Last Updated') - ->setValue($updated)); - } - $form + ->setLabel('Redirect URI') + ->setName('redirect_uri') + ->setValue($client->getRedirectURI()) + ->setError($e_redirect)) ->appendChild( id(new AphrontFormSubmitControl()) - ->setValue($submit_button)); + ->addCancelButton($cancel_uri) + ->setValue($submit_button)); - $panel->appendChild($form); - return $this->buildStandardPageResponse( - array($error, - $panel + $crumbs = $this->buildApplicationCrumbs(); + if (!$is_new) { + $crumbs->addTextCrumb( + $client->getName(), + $client->getViewURI()); + } + $crumbs->addTextCrumb($crumb_text); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->setFormErrors($errors) + ->setForm($form); + + return $this->buildApplicationPage( + array( + $crumbs, + $box, ), - array('title' => $title)); + array( + 'title' => $title, + 'device' => true, + )); } } diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php index e8f0541e75..04958c1600 100644 --- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php +++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php @@ -21,6 +21,12 @@ final class PhabricatorOAuthServerClient return '/oauthserver/client/delete/'.$this->getPHID().'/'; } + public static function initializeNewClient(PhabricatorUser $actor) { + return id(new PhabricatorOAuthServerClient()) + ->setCreatorPHID($actor->getPHID()) + ->setSecret(Filesystem::readRandomCharacters(32)); + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -39,6 +45,7 @@ final class PhabricatorOAuthServerClient public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } @@ -46,14 +53,24 @@ final class PhabricatorOAuthServerClient switch ($capability) { case PhabricatorPolicyCapability::CAN_VIEW: return PhabricatorPolicies::POLICY_USER; + case PhabricatorPolicyCapability::CAN_EDIT: + return PhabricatorPolicies::POLICY_NOONE; } } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + return ($viewer->getPHID() == $this->getCreatorPHID()); + } return false; } public function describeAutomaticCapability($capability) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + return pht("Only an application's creator can edit it."); + } return null; }