diff --git a/resources/sql/autopatches/20160405.oauth.2.disable.sql b/resources/sql/autopatches/20160405.oauth.2.disable.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160405.oauth.2.disable.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_oauth_server.oauth_server_oauthserverclient + ADD isDisabled 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 @@ -2702,7 +2702,7 @@ 'PhabricatorOAuthClientAuthorization' => 'applications/oauthserver/storage/PhabricatorOAuthClientAuthorization.php', 'PhabricatorOAuthClientAuthorizationQuery' => 'applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php', 'PhabricatorOAuthClientController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientController.php', - 'PhabricatorOAuthClientDeleteController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php', + 'PhabricatorOAuthClientDisableController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientDisableController.php', 'PhabricatorOAuthClientEditController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php', 'PhabricatorOAuthClientListController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php', 'PhabricatorOAuthClientSecretController' => 'applications/oauthserver/controller/client/PhabricatorOAuthClientSecretController.php', @@ -7194,7 +7194,7 @@ ), 'PhabricatorOAuthClientAuthorizationQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorOAuthClientController' => 'PhabricatorOAuthServerController', - 'PhabricatorOAuthClientDeleteController' => 'PhabricatorOAuthClientController', + 'PhabricatorOAuthClientDisableController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthClientEditController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthClientListController' => 'PhabricatorOAuthClientController', 'PhabricatorOAuthClientSecretController' => 'PhabricatorOAuthClientController', diff --git a/src/applications/oauthserver/PhabricatorOAuthServer.php b/src/applications/oauthserver/PhabricatorOAuthServer.php --- a/src/applications/oauthserver/PhabricatorOAuthServer.php +++ b/src/applications/oauthserver/PhabricatorOAuthServer.php @@ -172,6 +172,11 @@ return null; } + $application = $authorization->getClient(); + if ($application->getIsDisabled()) { + return null; + } + // TODO: This should probably be reworked; expiration should be an // exclusive property of the token. For now, this logic reads: tokens for // authorizations with "offline_access" never expire. diff --git a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php --- a/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php +++ b/src/applications/oauthserver/application/PhabricatorOAuthServerApplication.php @@ -53,8 +53,8 @@ 'token/' => 'PhabricatorOAuthServerTokenController', $this->getEditRoutePattern('edit/') => 'PhabricatorOAuthClientEditController', - 'client/' => array( - 'delete/(?P\d+)/' => 'PhabricatorOAuthClientDeleteController', + 'client/' => array( + 'disable/(?P\d+)/' => 'PhabricatorOAuthClientDisableController', 'view/(?P\d+)/' => 'PhabricatorOAuthClientViewController', 'secret/(?P\d+)/' => 'PhabricatorOAuthClientSecretController', 'test/(?P\d+)/' => 'PhabricatorOAuthClientTestController', diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php @@ -62,6 +62,15 @@ phutil_tag('strong', array(), 'client_id'))); } + if ($client->getIsDisabled()) { + return $this->buildErrorResponse( + 'invalid_request', + pht('Application Disabled'), + pht( + 'The %s OAuth application has been disabled.', + phutil_tag('strong', array(), 'client_id'))); + } + $name = $client->getName(); $server->setClient($client); if ($redirect_uri) { diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php @@ -67,7 +67,7 @@ $response->setError('invalid_grant'); $response->setErrorDescription( pht( - 'Authorization code %d not found.', + 'Authorization code %s not found.', $code)); return $response; } @@ -102,11 +102,22 @@ $response->setError('invalid_client'); $response->setErrorDescription( pht( - 'Client with %s %d not found.', + 'Client with %s %s not found.', 'client_id', $client_phid)); return $response; } + + if ($client->getIsDisabled()) { + $response->setError('invalid_client'); + $response->setErrorDescription( + pht( + 'OAuth application "%s" has been disabled.', + $client->getName())); + + return $response; + } + $server->setClient($client); $user_phid = $auth_code->getUserPHID(); @@ -116,7 +127,7 @@ $response->setError('invalid_grant'); $response->setErrorDescription( pht( - 'User with PHID %d not found.', + 'User with PHID %s not found.', $user_phid)); return $response; } @@ -132,7 +143,7 @@ $response->setError('invalid_grant'); $response->setErrorDescription( pht( - 'Invalid authorization code %d.', + 'Invalid authorization code %s.', $code)); return $response; } diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php deleted file mode 100644 --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDeleteController.php +++ /dev/null @@ -1,40 +0,0 @@ -getViewer(); - - $client = id(new PhabricatorOAuthServerClientQuery()) - ->setViewer($viewer) - ->withIDs(array($request->getURIData('id'))) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - if (!$client) { - return new Aphront404Response(); - } - - // TODO: This should be "disable", not "delete"! - - if ($request->isFormPost()) { - $client->delete(); - $app_uri = $this->getApplicationURI(); - return id(new AphrontRedirectResponse())->setURI($app_uri); - } - - return $this->newDialog() - ->setTitle(pht('Delete OAuth Application?')) - ->appendParagraph( - pht( - 'Really delete the OAuth application %s?', - phutil_tag('strong', array(), $client->getName()))) - ->addCancelButton($client->getViewURI()) - ->addSubmitButton(pht('Delete Application')); - } - -} diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDisableController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDisableController.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientDisableController.php @@ -0,0 +1,67 @@ +getViewer(); + + $client = id(new PhabricatorOAuthServerClientQuery()) + ->setViewer($viewer) + ->withIDs(array($request->getURIData('id'))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$client) { + return new Aphront404Response(); + } + + $done_uri = $client->getViewURI(); + $is_disable = !$client->getIsDisabled(); + + if ($request->isFormPost()) { + $xactions = array(); + + $xactions[] = id(new PhabricatorOAuthServerTransaction()) + ->setTransactionType(PhabricatorOAuthServerTransaction::TYPE_DISABLED) + ->setNewValue((int)$is_disable); + + $editor = id(new PhabricatorOAuthServerEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($client, $xactions); + + return id(new AphrontRedirectResponse())->setURI($done_uri); + } + + if ($is_disable) { + $title = pht('Disable OAuth Application'); + $body = pht( + 'Really disable the %s OAuth application? Users will no longer be '. + 'able to authenticate against it, nor access Phabricator using '. + 'tokens generated by this application.', + phutil_tag('strong', array(), $client->getName())); + $button = pht('Disable Application'); + } else { + $title = pht('Enable OAuth Application'); + $body = pht( + 'Really enable the %s OAuth application? Users will be able to '. + 'authenticate against it, and existing tokens will become usable '. + 'again.', + phutil_tag('strong', array(), $client->getName())); + $button = pht('Enable Application'); + } + + return $this->newDialog() + ->setTitle($title) + ->appendParagraph($body) + ->addCancelButton($done_uri) + ->addSubmitButton($button); + } + +} diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientTestController.php @@ -15,36 +15,41 @@ return new Aphront404Response(); } - $view_uri = $client->getViewURI(); - - // Look for an existing authorization. - $authorization = id(new PhabricatorOAuthClientAuthorizationQuery()) - ->setViewer($viewer) - ->withUserPHIDs(array($viewer->getPHID())) - ->withClientPHIDs(array($client->getPHID())) - ->executeOne(); - if ($authorization) { - return $this->newDialog() - ->setTitle(pht('Already Authorized')) - ->appendParagraph( - pht( - 'You have already authorized this application to access your '. - 'account.')) - ->addCancelButton($view_uri, pht('Close')); - } + $done_uri = $client->getViewURI(); if ($request->isFormPost()) { $server = id(new PhabricatorOAuthServer()) ->setUser($viewer) ->setClient($client); - $scope = array(); - $authorization = $server->authorizeClient($scope); + // Create an authorization if we don't already have one. + $authorization = id(new PhabricatorOAuthClientAuthorizationQuery()) + ->setViewer($viewer) + ->withUserPHIDs(array($viewer->getPHID())) + ->withClientPHIDs(array($client->getPHID())) + ->executeOne(); + if (!$authorization) { + $scope = array(); + $authorization = $server->authorizeClient($scope); + } + + $access_token = $server->generateAccessToken(); - $id = $authorization->getID(); - $panel_uri = '/settings/panel/oauthorizations/?id='.$id; + $form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendInstructions( + pht( + 'Keep this token private, it allows any bearer to access '. + 'your account on behalf of this application.')) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel(pht('Token')) + ->setValue($access_token->getToken())); - return id(new AphrontRedirectResponse())->setURI($panel_uri); + return $this->newDialog() + ->setTitle(pht('OAuth Access Token')) + ->appendForm($form) + ->addCancelButton($done_uri, pht('Close')); } // TODO: It would be nice to put scope options in this dialog, maybe? @@ -53,10 +58,10 @@ ->setTitle(pht('Authorize Application?')) ->appendParagraph( pht( - 'This will create an authorization, permitting %s to access '. - 'your account.', + 'This will create an authorization and OAuth token, permitting %s '. + 'to access your account.', phutil_tag('strong', array(), $client->getName()))) - ->addCancelButton($view_uri) + ->addCancelButton($done_uri) ->addSubmitButton(pht('Authorize Application')); } } diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientViewController.php @@ -51,6 +51,12 @@ ->setHeader(pht('OAuth Application: %s', $client->getName())) ->setPolicyObject($client); + if ($client->getIsDisabled()) { + $header->setStatus('fa-ban', 'indigo', pht('Disabled')); + } else { + $header->setStatus('fa-check', 'green', pht('Enabled')); + } + return $header; } @@ -62,12 +68,6 @@ $client, PhabricatorPolicyCapability::CAN_EDIT); - $authorization = id(new PhabricatorOAuthClientAuthorizationQuery()) - ->setViewer($viewer) - ->withUserPHIDs(array($viewer->getPHID())) - ->withClientPHIDs(array($client->getPHID())) - ->executeOne(); - $is_authorized = (bool)$authorization; $id = $client->getID(); $view = id(new PhabricatorActionListView()) @@ -89,20 +89,30 @@ ->setDisabled(!$can_edit) ->setWorkflow(true)); + $is_disabled = $client->getIsDisabled(); + if ($is_disabled) { + $disable_text = pht('Enable Application'); + $disable_icon = 'fa-check'; + } else { + $disable_text = pht('Disable Application'); + $disable_icon = 'fa-ban'; + } + + $disable_uri = $this->getApplicationURI("client/disable/{$id}/"); + $view->addAction( id(new PhabricatorActionView()) - ->setName(pht('Delete Application')) - ->setIcon('fa-times') + ->setName($disable_text) + ->setIcon($disable_icon) ->setWorkflow(true) ->setDisabled(!$can_edit) - ->setHref($client->getDeleteURI())); + ->setHref($disable_uri)); $view->addAction( id(new PhabricatorActionView()) - ->setName(pht('Create Test Authorization')) - ->setIcon('fa-wrench') + ->setName(pht('Generate Test Token')) + ->setIcon('fa-plus') ->setWorkflow(true) - ->setDisabled($is_authorized) ->setHref($this->getApplicationURI("client/test/{$id}/"))); return $view; diff --git a/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php --- a/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php +++ b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php @@ -16,6 +16,7 @@ $types[] = PhabricatorOAuthServerTransaction::TYPE_NAME; $types[] = PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI; + $types[] = PhabricatorOAuthServerTransaction::TYPE_DISABLED; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; @@ -32,6 +33,8 @@ return $object->getName(); case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: return $object->getRedirectURI(); + case PhabricatorOAuthServerTransaction::TYPE_DISABLED: + return $object->getIsDisabled(); } } @@ -43,6 +46,8 @@ case PhabricatorOAuthServerTransaction::TYPE_NAME: case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: return $xaction->getNewValue(); + case PhabricatorOAuthServerTransaction::TYPE_DISABLED: + return (int)$xaction->getNewValue(); } } @@ -57,6 +62,9 @@ case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: $object->setRedirectURI($xaction->getNewValue()); return; + case PhabricatorOAuthServerTransaction::TYPE_DISABLED: + $object->setIsDisabled($xaction->getNewValue()); + return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -69,6 +77,7 @@ switch ($xaction->getTransactionType()) { case PhabricatorOAuthServerTransaction::TYPE_NAME: case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: + case PhabricatorOAuthServerTransaction::TYPE_DISABLED: return; } diff --git a/src/applications/oauthserver/query/PhabricatorOAuthServerClientSearchEngine.php b/src/applications/oauthserver/query/PhabricatorOAuthServerClientSearchEngine.php --- a/src/applications/oauthserver/query/PhabricatorOAuthServerClientSearchEngine.php +++ b/src/applications/oauthserver/query/PhabricatorOAuthServerClientSearchEngine.php @@ -73,7 +73,7 @@ array $clients, PhabricatorSavedQuery $query, array $handles) { - assert_instances_of($clients, 'PhabricatorOauthServerClient'); + assert_instances_of($clients, 'PhabricatorOAuthServerClient'); $viewer = $this->requireViewer(); @@ -86,6 +86,10 @@ ->setHref($client->getViewURI()) ->setObject($client); + if ($client->getIsDisabled()) { + $item->setDisabled(true); + } + $list->addItem($item); } diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php --- a/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php +++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerClient.php @@ -11,9 +11,10 @@ protected $name; protected $redirectURI; protected $creatorPHID; - protected $isTrusted = 0; + protected $isTrusted; protected $viewPolicy; protected $editPolicy; + protected $isDisabled; public function getEditURI() { $id = $this->getID(); @@ -25,17 +26,14 @@ return "/oauthserver/client/view/{$id}/"; } - public function getDeleteURI() { - $id = $this->getID(); - return "/oauthserver/client/delete/{$id}/"; - } - public static function initializeNewClient(PhabricatorUser $actor) { return id(new PhabricatorOAuthServerClient()) ->setCreatorPHID($actor->getPHID()) ->setSecret(Filesystem::readRandomCharacters(32)) ->setViewPolicy(PhabricatorPolicies::POLICY_USER) - ->setEditPolicy($actor->getPHID()); + ->setEditPolicy($actor->getPHID()) + ->setIsDisabled(0) + ->setIsTrusted(0); } protected function getConfiguration() { @@ -46,13 +44,9 @@ 'secret' => 'text32', 'redirectURI' => 'text255', 'isTrusted' => 'bool', + 'isDisabled' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( - 'key_phid' => null, - 'phid' => array( - 'columns' => array('phid'), - 'unique' => true, - ), 'creatorPHID' => array( 'columns' => array('creatorPHID'), ), diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php --- a/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php +++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php @@ -5,6 +5,7 @@ const TYPE_NAME = 'oauthserver.name'; const TYPE_REDIRECT_URI = 'oauthserver.redirect-uri'; + const TYPE_DISABLED = 'oauthserver.disabled'; public function getApplicationName() { return 'oauth_server'; @@ -44,6 +45,16 @@ $this->renderHandleLink($author_phid), $old, $new); + case self::TYPE_DISABLED: + if ($new) { + return pht( + '%s disabled this application.', + $this->renderHandleLink($author_phid)); + } else { + return pht( + '%s enabled this application.', + $this->renderHandleLink($author_phid)); + } } return parent::getTitle();