From 1cc81b1d0ae4b1a2377e877864f476aefb570720 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 14 Jan 2015 17:27:45 -0800 Subject: [PATCH] OAuthServer - hide client secret behind a "View Secret" action Summary: ...also adds policies on who can view and who can edit an action. Fixes T6949. Test Plan: viewed a secret through the new UI and it worked Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T6949 Differential Revision: https://secure.phabricator.com/D11401 --- .../20150114.oauthserver.client.policy.sql | 11 +++ src/__phutil_library_map__.php | 2 + .../PhabricatorOAuthServerApplication.php | 1 + .../PhabricatorOAuthClientEditController.php | 21 ++++++ ...PhabricatorOAuthClientSecretController.php | 70 +++++++++++++++++++ .../PhabricatorOAuthClientViewController.php | 16 +++-- .../storage/PhabricatorOAuthServerClient.php | 18 ++--- 7 files changed, 123 insertions(+), 16 deletions(-) create mode 100644 resources/sql/autopatches/20150114.oauthserver.client.policy.sql create mode 100644 src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php diff --git a/resources/sql/autopatches/20150114.oauthserver.client.policy.sql b/resources/sql/autopatches/20150114.oauthserver.client.policy.sql new file mode 100644 index 0000000000..8f46afc5c9 --- /dev/null +++ b/resources/sql/autopatches/20150114.oauthserver.client.policy.sql @@ -0,0 +1,11 @@ +ALTER TABLE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient + ADD viewPolicy VARBINARY(64) NOT NULL AFTER creatorPHID; + +UPDATE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient + SET viewPolicy = 'users' WHERE viewPolicy = ''; + +ALTER TABLE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient + ADD editPolicy VARBINARY(64) NOT NULL AFTER viewPolicy; + +UPDATE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient + SET editPolicy = creatorPHID WHERE viewPolicy = ''; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4b4da7f92e..2e10cb708d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2003,6 +2003,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthClientDeleteController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php', 'PhabricatorOAuthClientEditController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php', 'PhabricatorOAuthClientListController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php', + 'PhabricatorOAuthClientSecretController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php', 'PhabricatorOAuthClientViewController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php', 'PhabricatorOAuthResponse' => 'applications/oauthserver/PhabricatorOAuthResponse.php', 'PhabricatorOAuthServer' => 'applications/oauthserver/PhabricatorOAuthServer.php', @@ -5203,6 +5204,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthClientDeleteController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthClientEditController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthClientListController' => 'PhabricatorOAuthClientController', + 'PhabricatorOAuthClientSecretController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthClientViewController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthResponse' => 'AphrontResponse', 'PhabricatorOAuthServerAccessToken' => 'PhabricatorOAuthServerDAO', diff --git a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php index 0c9151b415..ef96668d4b 100644 --- a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php +++ b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php @@ -51,6 +51,7 @@ final class PhabricatorOAuthServerApplication extends PhabricatorApplication { 'delete/(?P[^/]+)/' => 'PhabricatorOAuthClientDeleteController', 'edit/(?P[^/]+)/' => 'PhabricatorOAuthClientEditController', 'view/(?P[^/]+)/' => 'PhabricatorOAuthClientViewController', + 'secret/(?P[^/]+)/' => 'PhabricatorOAuthClientSecretController', ), ), ); diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php index cf822ccd9a..1f1b0e387b 100644 --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php @@ -64,6 +64,8 @@ final class PhabricatorOAuthClientEditController $e_redirect = pht('Invalid'); } + $client->setViewPolicy($request->getStr('viewPolicy')); + $client->setEditPolicy($request->getStr('editPolicy')); if (!$errors) { $client->save(); $view_uri = $client->getViewURI(); @@ -71,6 +73,11 @@ final class PhabricatorOAuthClientEditController } } + $policies = id(new PhabricatorPolicyQuery()) + ->setViewer($viewer) + ->setObject($client) + ->execute(); + $form = id(new AphrontFormView()) ->setUser($viewer) ->appendChild( @@ -85,6 +92,20 @@ final class PhabricatorOAuthClientEditController ->setName('redirect_uri') ->setValue($client->getRedirectURI()) ->setError($e_redirect)) + ->appendChild( + id(new AphrontFormPolicyControl()) + ->setUser($viewer) + ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) + ->setPolicyObject($client) + ->setPolicies($policies) + ->setName('viewPolicy')) + ->appendChild( + id(new AphrontFormPolicyControl()) + ->setUser($viewer) + ->setCapability(PhabricatorPolicyCapability::CAN_EDIT) + ->setPolicyObject($client) + ->setPolicies($policies) + ->setName('editPolicy')) ->appendChild( id(new AphrontFormSubmitControl()) ->addCancelButton($cancel_uri) diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php new file mode 100644 index 0000000000..cdfc53e5b4 --- /dev/null +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php @@ -0,0 +1,70 @@ +getUser(); + + $client = id(new PhabricatorOAuthServerClientQuery()) + ->setViewer($viewer) + ->withPHIDs(array($this->getClientPHID())) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$client) { + return new Aphront404Response(); + } + + $view_uri = $client->getViewURI(); + $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession( + $viewer, + $request, + $view_uri); + + if ($request->isFormPost()) { + $secret = $client->getSecret(); + $body = id(new PHUIFormLayoutView()) + ->appendChild( + id(new AphrontFormTextAreaControl()) + ->setLabel(pht('Plaintext')) + ->setReadOnly(true) + ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT) + ->setValue($secret)); + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->setTitle(pht('Application Secret')) + ->appendChild($body) + ->addCancelButton($view_uri, pht('Done')); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + + $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); + + if ($is_serious) { + $body = pht( + 'The secret associated with this oauth application will be shown in '. + 'plain text on your screen.'); + } else { + $body = pht( + 'The secret associated with this oauth application will be shown in '. + 'plain text on your screen. Before continuing, wrap your arms around '. + 'your monitor to create a human shield, keeping it safe from prying '. + 'eyes. Protect company secrets!'); + } + return $this->newDialog() + ->setUser($viewer) + ->setTitle(pht('Really show application secret?')) + ->appendChild($body) + ->addSubmitButton(pht('Show Application Secret')) + ->addCancelButton($view_uri); + } + +} diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php index 8965c158b3..368de86f10 100644 --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php @@ -62,6 +62,8 @@ final class PhabricatorOAuthClientViewController ->withClientPHIDs(array($client->getPHID())) ->executeOne(); $is_authorized = (bool)$authorization; + $id = $client->getID(); + $phid = $client->getPHID(); $view = id(new PhabricatorActionListView()) ->setUser($viewer); @@ -74,6 +76,14 @@ final class PhabricatorOAuthClientViewController ->setDisabled(!$can_edit) ->setHref($client->getEditURI())); + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Show Application Secret')) + ->setIcon('fa-eye') + ->setHref($this->getApplicationURI("client/secret/{$phid}/")) + ->setDisabled(!$can_edit) + ->setWorkflow(true)); + $view->addAction( id(new PhabricatorActionView()) ->setName(pht('Delete Application')) @@ -88,7 +98,7 @@ final class PhabricatorOAuthClientViewController ->setIcon('fa-wrench') ->setWorkflow(true) ->setDisabled($is_authorized) - ->setHref($this->getApplicationURI('test/'.$client->getID().'/'))); + ->setHref($this->getApplicationURI('test/'.$id.'/'))); return $view; } @@ -103,10 +113,6 @@ final class PhabricatorOAuthClientViewController pht('Client ID'), $client->getPHID()); - $view->addProperty( - pht('Client Secret'), - $client->getSecret()); - $view->addProperty( pht('Redirect URI'), $client->getRedirectURI()); diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php index ea3637d788..bbc4264bb1 100644 --- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php +++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php @@ -10,6 +10,8 @@ final class PhabricatorOAuthServerClient protected $name; protected $redirectURI; protected $creatorPHID; + protected $viewPolicy; + protected $editPolicy; public function getEditURI() { return '/oauthserver/client/edit/'.$this->getPHID().'/'; @@ -26,7 +28,9 @@ final class PhabricatorOAuthServerClient public static function initializeNewClient(PhabricatorUser $actor) { return id(new PhabricatorOAuthServerClient()) ->setCreatorPHID($actor->getPHID()) - ->setSecret(Filesystem::readRandomCharacters(32)); + ->setSecret(Filesystem::readRandomCharacters(32)) + ->setViewPolicy(PhabricatorPolicies::POLICY_USER) + ->setEditPolicy($actor->getPHID()); } protected function getConfiguration() { @@ -69,25 +73,17 @@ final class PhabricatorOAuthServerClient public function getPolicy($capability) { switch ($capability) { case PhabricatorPolicyCapability::CAN_VIEW: - return PhabricatorPolicies::POLICY_USER; + return $this->getViewPolicy(); case PhabricatorPolicyCapability::CAN_EDIT: - return PhabricatorPolicies::POLICY_NOONE; + return $this->getEditPolicy(); } } 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; }