diff --git a/resources/sql/autopatches/20160404.oauth.1.xaction.sql b/resources/sql/autopatches/20160404.oauth.1.xaction.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160404.oauth.1.xaction.sql @@ -0,0 +1,19 @@ +CREATE TABLE {$NAMESPACE}_oauth_server.oauth_server_transaction ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + authorPHID VARBINARY(64) NOT NULL, + objectPHID VARBINARY(64) NOT NULL, + viewPolicy VARBINARY(64) NOT NULL, + editPolicy VARBINARY(64) NOT NULL, + commentPHID VARBINARY(64) DEFAULT NULL, + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + oldValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + newValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + contentSource LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + metadata LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_phid` (`phid`), + KEY `key_object` (`objectPHID`) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; 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 @@ -2723,9 +2723,13 @@ 'PhabricatorOAuthServerController' => 'applications/oauthserver/controller/PhabricatorOAuthServerController.php', 'PhabricatorOAuthServerCreateClientsCapability' => 'applications/oauthserver/capability/PhabricatorOAuthServerCreateClientsCapability.php', 'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/PhabricatorOAuthServerDAO.php', + 'PhabricatorOAuthServerEditEngine' => 'applications/oauthserver/editor/PhabricatorOAuthServerEditEngine.php', + 'PhabricatorOAuthServerEditor' => 'applications/oauthserver/editor/PhabricatorOAuthServerEditor.php', 'PhabricatorOAuthServerScope' => 'applications/oauthserver/PhabricatorOAuthServerScope.php', 'PhabricatorOAuthServerTestCase' => 'applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php', 'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php', + 'PhabricatorOAuthServerTransaction' => 'applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php', + 'PhabricatorOAuthServerTransactionQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php', 'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php', 'PhabricatorObjectHasAsanaSubtaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaSubtaskEdgeType.php', 'PhabricatorObjectHasAsanaTaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaTaskEdgeType.php', @@ -7206,6 +7210,7 @@ 'PhabricatorOAuthServerClient' => array( 'PhabricatorOAuthServerDAO', 'PhabricatorPolicyInterface', + 'PhabricatorApplicationTransactionInterface', 'PhabricatorDestructibleInterface', ), 'PhabricatorOAuthServerClientAuthorizationPHIDType' => 'PhabricatorPHIDType', @@ -7215,9 +7220,13 @@ 'PhabricatorOAuthServerController' => 'PhabricatorController', 'PhabricatorOAuthServerCreateClientsCapability' => 'PhabricatorPolicyCapability', 'PhabricatorOAuthServerDAO' => 'PhabricatorLiskDAO', + 'PhabricatorOAuthServerEditEngine' => 'PhabricatorEditEngine', + 'PhabricatorOAuthServerEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorOAuthServerScope' => 'Phobject', 'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase', 'PhabricatorOAuthServerTokenController' => 'PhabricatorOAuthServerController', + 'PhabricatorOAuthServerTransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorOAuthServerTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorObjectHandle' => array( 'Phobject', 'PhabricatorPolicyInterface', 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 @@ -188,24 +188,49 @@ return $authorization; } + public function validateRedirectURI($uri) { + try { + $this->assertValidRedirectURI($uri); + return true; + } catch (Exception $ex) { + return false; + } + } + /** * See http://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-3.1.2 * for details on what makes a given redirect URI "valid". */ - public function validateRedirectURI(PhutilURI $uri) { - if (!PhabricatorEnv::isValidRemoteURIForLink($uri)) { - return false; + public function assertValidRedirectURI($raw_uri) { + // This covers basics like reasonable formatting and the existence of a + // protocol. + PhabricatorEnv::requireValidRemoteURIForLink($raw_uri); + + $uri = new PhutilURI($raw_uri); + + $fragment = $uri->getFragment(); + if (strlen($fragment)) { + throw new Exception( + pht( + 'OAuth application redirect URIs must not contain URI '. + 'fragments, but the URI "%s" has a fragment ("%s").', + $raw_uri, + $fragment)); } - if ($uri->getFragment()) { - return false; + $protocol = $uri->getProtocol(); + switch ($protocol) { + case 'http': + case 'https': + break; + default: + throw new Exception( + pht( + 'OAuth application redirect URIs must only use the "http" or '. + '"https" protocols, but the URI "%s" uses the "%s" protocol.', + $raw_uri, + $protocol)); } - - if (!$uri->getDomain()) { - return false; - } - - return true; } /** 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 @@ -51,10 +51,10 @@ => 'PhabricatorOAuthClientListController', 'auth/' => 'PhabricatorOAuthServerAuthController', 'token/' => 'PhabricatorOAuthServerTokenController', + $this->getEditRoutePattern('edit/') => + 'PhabricatorOAuthClientEditController', 'client/' => array( - 'create/' => 'PhabricatorOAuthClientEditController', 'delete/(?P\d+)/' => 'PhabricatorOAuthClientDeleteController', - 'edit/(?P\d+)/' => 'PhabricatorOAuthClientEditController', 'view/(?P\d+)/' => 'PhabricatorOAuthClientViewController', 'secret/(?P\d+)/' => 'PhabricatorOAuthClientSecretController', 'test/(?P\d+)/' => 'PhabricatorOAuthClientTestController', diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientEditController.php @@ -4,129 +4,9 @@ extends PhabricatorOAuthClientController { public function handleRequest(AphrontRequest $request) { - $viewer = $this->getViewer(); - $id = $request->getURIData('id'); - - if ($id) { - $client = id(new PhabricatorOAuthServerClientQuery()) - ->setViewer($viewer) - ->withIDs(array($id)) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - if (!$client) { - return new Aphront404Response(); - } - - $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 { - $this->requireApplicationCapability( - PhabricatorOAuthServerCreateClientsCapability::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 (!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'); - } - - $client->setViewPolicy($request->getStr('viewPolicy')); - $client->setEditPolicy($request->getStr('editPolicy')); - if (!$errors) { - $client->save(); - $view_uri = $client->getViewURI(); - return id(new AphrontRedirectResponse())->setURI($view_uri); - } - } - - $policies = id(new PhabricatorPolicyQuery()) - ->setViewer($viewer) - ->setObject($client) - ->execute(); - - $form = id(new AphrontFormView()) - ->setUser($viewer) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Name')) - ->setName('name') - ->setValue($client->getName()) - ->setError($e_name)) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Redirect URI')) - ->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) - ->setValue($submit_button)); - - $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->newPage() - ->setCrumbs($crumbs) - ->setTitle($title) - ->appendChild($box); + return id(new PhabricatorOAuthServerEditEngine()) + ->setController($this) + ->buildResponse(); } } diff --git a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php --- a/src/applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php +++ b/src/applications/oauthserver/controller/client/PhabricatorOAuthClientListController.php @@ -3,38 +3,22 @@ final class PhabricatorOAuthClientListController extends PhabricatorOAuthClientController { - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { - $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) - ->setSearchEngine(new PhabricatorOAuthServerClientSearchEngine()) - ->setNavigation($this->buildSideNavView()); - - return $this->delegateToController($controller); + public function handleRequest(AphrontRequest $request) { + return id(new PhabricatorOAuthServerClientSearchEngine()) + ->setController($this) + ->buildResponse(); } protected function buildApplicationCrumbs() { $crumbs = parent::buildApplicationCrumbs(); - $can_create = $this->hasApplicationCapability( - PhabricatorOAuthServerCreateClientsCapability::CAPABILITY); - - $crumbs->addAction( - id(new PHUIListItemView()) - ->setHref($this->getApplicationURI('client/create/')) - ->setName(pht('Create Application')) - ->setDisabled(!$can_create) - ->setWorkflow(!$can_create) - ->setIcon('fa-plus-square')); + id(new PhabricatorOAuthServerEditEngine()) + ->setViewer($this->getViewer()) + ->addActionToCrumbs($crumbs); return $crumbs; } 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 @@ -22,6 +22,11 @@ $crumbs = $this->buildApplicationCrumbs(); $crumbs->addTextCrumb($client->getName()); + $timeline = $this->buildTransactionTimeline( + $client, + new PhabricatorOAuthServerTransactionQuery()); + $timeline->setShouldTerminate(true); + $box = id(new PHUIObjectBoxView()) ->setHeader($header) ->addPropertyList($properties); @@ -31,7 +36,11 @@ return $this->newPage() ->setCrumbs($crumbs) ->setTitle($title) - ->appendChild($box); + ->appendChild( + array( + $box, + $timeline, + )); } private function buildHeaderView(PhabricatorOAuthServerClient $client) { diff --git a/src/applications/oauthserver/editor/PhabricatorOAuthServerEditEngine.php b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditEngine.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditEngine.php @@ -0,0 +1,100 @@ +getViewer()); + } + + protected function newObjectQuery() { + return id(new PhabricatorOAuthServerClientQuery()); + } + + protected function getObjectCreateTitleText($object) { + return pht('Create OAuth Server'); + } + + protected function getObjectCreateButtonText($object) { + return pht('Create OAuth Server'); + } + + protected function getObjectEditTitleText($object) { + return pht('Edit OAuth Server: %s', $object->getName()); + } + + protected function getObjectEditShortText($object) { + return pht('Edit OAuth Server'); + } + + protected function getObjectCreateShortText() { + return pht('Create OAuth Server'); + } + + protected function getObjectName() { + return pht('OAuth Server'); + } + + protected function getObjectViewURI($object) { + return $object->getViewURI(); + } + + protected function getCreateNewObjectPolicy() { + return $this->getApplication()->getPolicy( + PhabricatorOAuthServerCreateClientsCapability::CAPABILITY); + } + + protected function buildCustomEditFields($object) { + return array( + id(new PhabricatorTextEditField()) + ->setKey('name') + ->setLabel(pht('Name')) + ->setIsRequired(true) + ->setTransactionType(PhabricatorOAuthServerTransaction::TYPE_NAME) + ->setDescription(pht('The name of the OAuth application.')) + ->setConduitDescription(pht('Rename the application.')) + ->setConduitTypeDescription(pht('New application name.')) + ->setValue($object->getName()), + id(new PhabricatorTextEditField()) + ->setKey('redirectURI') + ->setLabel(pht('Redirect URI')) + ->setIsRequired(true) + ->setTransactionType( + PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI) + ->setDescription( + pht('The redirect URI for OAuth handshakes.')) + ->setConduitDescription( + pht( + 'Change where this application redirects users to during OAuth '. + 'handshakes.')) + ->setConduitTypeDescription( + pht( + 'New OAuth application redirect URI.')) + ->setValue($object->getRedirectURI()), + ); + } + +} diff --git a/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/editor/PhabricatorOAuthServerEditor.php @@ -0,0 +1,137 @@ +getTransactionType()) { + case PhabricatorOAuthServerTransaction::TYPE_NAME: + return $object->getName(); + case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: + return $object->getRedirectURI(); + } + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorOAuthServerTransaction::TYPE_NAME: + case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: + return $xaction->getNewValue(); + } + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorOAuthServerTransaction::TYPE_NAME: + $object->setName($xaction->getNewValue()); + return; + case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: + $object->setRedirectURI($xaction->getNewValue()); + return; + } + + return parent::applyCustomInternalTransaction($object, $xaction); + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorOAuthServerTransaction::TYPE_NAME: + case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: + return; + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + switch ($type) { + case PhabricatorOAuthServerTransaction::TYPE_NAME: + $missing = $this->validateIsEmptyTextField( + $object->getName(), + $xactions); + + if ($missing) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('OAuth applications must have a name.'), + nonempty(last($xactions), null)); + + $error->setIsMissingFieldError(true); + $errors[] = $error; + } + break; + case PhabricatorOAuthServerTransaction::TYPE_REDIRECT_URI: + $missing = $this->validateIsEmptyTextField( + $object->getRedirectURI(), + $xactions); + if ($missing) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('OAuth applications must have a valid redirect URI.'), + nonempty(last($xactions), null)); + + $error->setIsMissingFieldError(true); + $errors[] = $error; + } else { + foreach ($xactions as $xaction) { + $redirect_uri = $xaction->getNewValue(); + + try { + $server = new PhabricatorOAuthServer(); + $server->assertValidRedirectURI($redirect_uri); + } catch (Exception $ex) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + $ex->getMessage(), + $xaction); + } + } + } + break; + } + + return $errors; + } + +} diff --git a/src/applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php b/src/applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/query/PhabricatorOAuthServerTransactionQuery.php @@ -0,0 +1,10 @@ +getID(); - return "/oauthserver/client/edit/{$id}/"; + return "/oauthserver/edit/{$id}/"; } public function getViewURI() { @@ -92,8 +93,32 @@ return null; } + +/* -( PhabricatorApplicationTransactionInterface )------------------------- */ + + + public function getApplicationTransactionEditor() { + return new PhabricatorOAuthServerEditor(); + } + + public function getApplicationTransactionObject() { + return $this; + } + + public function getApplicationTransactionTemplate() { + return new PhabricatorOAuthServerTransaction(); + } + + public function willRenderTimeline( + PhabricatorApplicationTransactionView $timeline, + AphrontRequest $request) { + return $timeline; + } + + /* -( PhabricatorDestructibleInterface )----------------------------------- */ + public function destroyObjectPermanently( PhabricatorDestructionEngine $engine) { diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php @@ -0,0 +1,52 @@ +getAuthorPHID(); + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_CREATE: + return pht( + '%s created this OAuth application.', + $this->renderHandleLink($author_phid)); + case self::TYPE_NAME: + return pht( + '%s renamed this application from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); + case self::TYPE_REDIRECT_URI: + return pht( + '%s changed the application redirect URI from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); + } + + return parent::getTitle(); + } + +} diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -575,8 +575,8 @@ * @return void * @task uri */ - public static function requireValidRemoteURIForLink($uri) { - $uri = new PhutilURI($uri); + public static function requireValidRemoteURIForLink($raw_uri) { + $uri = new PhutilURI($raw_uri); $proto = $uri->getProtocol(); if (!strlen($proto)) { @@ -584,7 +584,7 @@ pht( 'URI "%s" is not a valid linkable resource. A valid linkable '. 'resource URI must specify a protocol.', - $uri)); + $raw_uri)); } $protocols = self::getEnvConfig('uri.allowed-protocols'); @@ -593,7 +593,7 @@ pht( 'URI "%s" is not a valid linkable resource. A valid linkable '. 'resource URI must use one of these protocols: %s.', - $uri, + $raw_uri, implode(', ', array_keys($protocols)))); } @@ -603,7 +603,7 @@ pht( 'URI "%s" is not a valid linkable resource. A valid linkable '. 'resource URI must specify a domain.', - $uri)); + $raw_uri)); } }