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 @@ -1100,6 +1100,7 @@ 'PhabricatorApplicationMetaMTA' => 'applications/metamta/application/PhabricatorApplicationMetaMTA.php', 'PhabricatorApplicationNotifications' => 'applications/notification/application/PhabricatorApplicationNotifications.php', 'PhabricatorApplicationNuance' => 'applications/nuance/application/PhabricatorApplicationNuance.php', + 'PhabricatorApplicationOAuthServer' => 'applications/oauthserver/application/PhabricatorApplicationOAuthServer.php', 'PhabricatorApplicationOwners' => 'applications/owners/application/PhabricatorApplicationOwners.php', 'PhabricatorApplicationPHIDTypeApplication' => 'applications/meta/phid/PhabricatorApplicationPHIDTypeApplication.php', 'PhabricatorApplicationPHPAST' => 'applications/phpast/application/PhabricatorApplicationPHPAST.php', @@ -1717,6 +1718,7 @@ 'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/PhabricatorOAuthServerAuthorizationCode.php', 'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/PhabricatorOAuthServerClient.php', 'PhabricatorOAuthServerClientQuery' => 'applications/oauthserver/query/PhabricatorOAuthServerClientQuery.php', + 'PhabricatorOAuthServerConsoleController' => 'applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php', 'PhabricatorOAuthServerController' => 'applications/oauthserver/controller/PhabricatorOAuthServerController.php', 'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/PhabricatorOAuthServerDAO.php', 'PhabricatorOAuthServerScope' => 'applications/oauthserver/PhabricatorOAuthServerScope.php', @@ -3753,6 +3755,7 @@ 'PhabricatorApplicationMetaMTA' => 'PhabricatorApplication', 'PhabricatorApplicationNotifications' => 'PhabricatorApplication', 'PhabricatorApplicationNuance' => 'PhabricatorApplication', + 'PhabricatorApplicationOAuthServer' => 'PhabricatorApplication', 'PhabricatorApplicationOwners' => 'PhabricatorApplication', 'PhabricatorApplicationPHIDTypeApplication' => 'PhabricatorPHIDType', 'PhabricatorApplicationPHPAST' => 'PhabricatorApplication', @@ -4454,6 +4457,7 @@ 'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerClient' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerClientQuery' => 'PhabricatorOffsetPagedQuery', + 'PhabricatorOAuthServerConsoleController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerController' => 'PhabricatorController', 'PhabricatorOAuthServerDAO' => 'PhabricatorLiskDAO', 'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase', diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -19,26 +19,6 @@ public function getURIMap() { return $this->getResourceURIMapRules() + array( - '/oauthserver/' => array( - 'auth/' => 'PhabricatorOAuthServerAuthController', - 'test/' => 'PhabricatorOAuthServerTestController', - 'token/' => 'PhabricatorOAuthServerTokenController', - 'clientauthorization/' => array( - '' => 'PhabricatorOAuthClientAuthorizationListController', - 'delete/(?P[^/]+)/' => - 'PhabricatorOAuthClientAuthorizationDeleteController', - 'edit/(?P[^/]+)/' => - 'PhabricatorOAuthClientAuthorizationEditController', - ), - 'client/' => array( - '' => 'PhabricatorOAuthClientListController', - 'create/' => 'PhabricatorOAuthClientEditController', - 'delete/(?P[^/]+)/' => 'PhabricatorOAuthClientDeleteController', - 'edit/(?P[^/]+)/' => 'PhabricatorOAuthClientEditController', - 'view/(?P[^/]+)/' => 'PhabricatorOAuthClientViewController', - ), - ), - '/~/' => array( '' => 'DarkConsoleController', 'data/(?P[^/]+)/' => 'DarkConsoleDataController', diff --git a/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php b/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/application/PhabricatorApplicationOAuthServer.php @@ -0,0 +1,62 @@ + array( + '' => 'PhabricatorOAuthServerConsoleController', + 'auth/' => 'PhabricatorOAuthServerAuthController', + 'test/' => 'PhabricatorOAuthServerTestController', + 'token/' => 'PhabricatorOAuthServerTokenController', + 'clientauthorization/' => array( + '' => 'PhabricatorOAuthClientAuthorizationListController', + 'delete/(?P[^/]+)/' => + 'PhabricatorOAuthClientAuthorizationDeleteController', + 'edit/(?P[^/]+)/' => + 'PhabricatorOAuthClientAuthorizationEditController', + ), + 'client/' => array( + '' => 'PhabricatorOAuthClientListController', + 'create/' => 'PhabricatorOAuthClientEditController', + 'delete/(?P[^/]+)/' => 'PhabricatorOAuthClientDeleteController', + 'edit/(?P[^/]+)/' => 'PhabricatorOAuthClientEditController', + 'view/(?P[^/]+)/' => 'PhabricatorOAuthClientViewController', + ), + ), + ); + } + +} 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 @@ -11,140 +11,177 @@ } public function processRequest() { - $request = $this->getRequest(); - $current_user = $request->getUser(); + $request = $this->getRequest(); + $viewer = $request->getUser(); + $server = new PhabricatorOAuthServer(); $client_phid = $request->getStr('client_id'); $scope = $request->getStr('scope', array()); $redirect_uri = $request->getStr('redirect_uri'); - $state = $request->getStr('state'); $response_type = $request->getStr('response_type'); - $response = new PhabricatorOAuthResponse(); // state is an opaque value the client sent us for their own purposes // we just need to send it right back to them in the response! - if ($state) { - $response->setState($state); - } + $state = $request->getStr('state'); + if (!$client_phid) { - $response->setError('invalid_request'); - $response->setErrorDescription( - 'Required parameter client_id not specified.'); - return $response; + return $this->buildErrorResponse( + 'invalid_request', + pht('Malformed Request'), + pht( + 'Required parameter %s was not present in the request.', + phutil_tag('strong', array(), 'client_id'))); } - $server->setUser($current_user); + + $server->setUser($viewer); + $is_authorized = false; + $authorization = null; + $uri = null; + $name = null; // one giant try / catch around all the exciting database stuff so we // can return a 'server_error' response if something goes wrong! try { $client = id(new PhabricatorOAuthServerClient()) ->loadOneWhere('phid = %s', $client_phid); + if (!$client) { - $response->setError('invalid_request'); - $response->setErrorDescription( - 'Client with id '.$client_phid.' not found.'); - return $response; + return $this->buildErrorResponse( + 'invalid_request', + pht('Invalid Client Application'), + pht( + 'Request parameter %s does not specify a valid client application.', + phutil_tag('strong', array(), 'client_id'))); } + + $name = $client->getName(); $server->setClient($client); if ($redirect_uri) { $client_uri = new PhutilURI($client->getRedirectURI()); $redirect_uri = new PhutilURI($redirect_uri); if (!($server->validateSecondaryRedirectURI($redirect_uri, $client_uri))) { - $response->setError('invalid_request'); - $response->setErrorDescription( - 'The specified redirect URI is invalid. The redirect URI '. - 'must be a fully-qualified domain with no fragments and '. - 'must have the same domain and at least the same query '. - 'parameters as the redirect URI the client registered.'); - return $response; + return $this->buildErrorResponse( + 'invalid_request', + pht('Invalid Redirect URI'), + pht( + 'Request parameter %s specifies an invalid redirect URI. '. + 'The redirect URI must be a fully-qualified domain with no '. + 'fragments, and must have the same domain and at least '. + 'the same query parameters as the redirect URI the client '. + 'registered.', + phutil_tag('strong', array(), 'redirect_uri'))); } - $uri = $redirect_uri; + $uri = $redirect_uri; } else { - $uri = new PhutilURI($client->getRedirectURI()); + $uri = new PhutilURI($client->getRedirectURI()); } - // we've now validated this request enough overall such that we - // can safely redirect to the client with the response - $response->setClientURI($uri); if (empty($response_type)) { - $response->setError('invalid_request'); - $response->setErrorDescription( - 'Required parameter response_type not specified.'); - return $response; + return $this->buildErrorResponse( + 'invalid_request', + pht('Invalid Response Type'), + pht( + 'Required request parameter %s is missing.', + phutil_tag('strong', array(), 'response_type'))); } + if ($response_type != 'code') { - $response->setError('unsupported_response_type'); - $response->setErrorDescription( - 'The authorization server does not support obtaining an '. - 'authorization code using the specified response_type. '. - 'You must specify the response_type as "code".'); - return $response; + return $this->buildErrorResponse( + 'unsupported_response_type', + pht('Unsupported Response Type'), + pht( + 'Request parameter %s specifies an unsupported response type. '. + 'Valid response types are: %s.', + phutil_tag('strong', array(), 'response_type'), + implode(', ', array('code')))); } + if ($scope) { if (!PhabricatorOAuthServerScope::validateScopesList($scope)) { - $response->setError('invalid_scope'); - $response->setErrorDescription( - 'The requested scope is invalid, unknown, or malformed.'); - return $response; + return $this->buildErrorResponse( + 'invalid_scope', + pht('Invalid Scope'), + pht( + 'Request parameter %s specifies an unsupported scope.', + phutil_tag('strong', array(), 'scope'))); } $scope = PhabricatorOAuthServerScope::scopesListToDict($scope); } - list($is_authorized, - $authorization) = $server->userHasAuthorizedClient($scope); - if ($is_authorized) { - $return_auth_code = true; - $unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites(); - } else if ($request->isFormPost()) { + // NOTE: We're always requiring a confirmation dialog to redirect. + // Partly this is a general defense against redirect attacks, and + // partly this shakes off anchors in the URI (which are not shaken + // by 302'ing). + + $auth_info = $server->userHasAuthorizedClient($scope); + list($is_authorized, $authorization) = $auth_info; + + if ($request->isFormPost()) { + // TODO: We should probably validate this more? It feels a little funky. $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request); + if ($authorization) { $authorization->setScope($scope)->save(); } else { - $authorization = $server->authorizeClient($scope); + $authorization = $server->authorizeClient($scope); } - $return_auth_code = true; - $unguarded_write = null; - } else { - $return_auth_code = false; - $unguarded_write = null; + + $is_authorized = true; } + } catch (Exception $e) { + return $this->buildErrorResponse( + 'server_error', + pht('Server Error'), + pht( + 'The authorization server encountered an unexpected condition '. + 'which prevented it from fulfilling the request.')); + } - if ($return_auth_code) { - // step 1 -- generate authorization code - $auth_code = - $server->generateAuthorizationCode($uri); + // When we reach this part of the controller, we can be in two states: + // + // 1. The user has not authorized the application yet. We want to + // give them an "Authorize this application?" dialog. + // 2. The user has authorized the application. We want to give them + // a "Confirm Login" dialog. - // step 2 return it - $content = array( + if ($is_authorized) { + + // The second case is simpler, so handle it first. The user either + // authorized the application previously, or has just authorized the + // application. Show them a confirm dialog with a normal link back to + // the application. This shakes anchors from the URI. + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $auth_code = $server->generateAuthorizationCode($uri); + unset($unguarded); + + $full_uri = $this->addQueryParams( + $uri, + array( 'code' => $auth_code->getCode(), 'scope' => $authorization->getScopeString(), - ); - $response->setContent($content); - return $response; - } - unset($unguarded_write); - } catch (Exception $e) { - // Note we could try harder to determine between a server_error - // vs temporarily_unavailable. Good enough though. - $response->setError('server_error'); - $response->setErrorDescription( - 'The authorization server encountered an unexpected condition '. - 'which prevented it from fulfilling the request. '); - return $response; - } + 'state' => $state, + )); + + // TODO: It would be nice to give the user more options here, like + // reviewing permissions, canceling the authorization, or aborting + // the workflow. - // display time -- make a nice form for the user to grant the client - // access to the granularity specified by $scope - $name = $client->getName(); - $title = pht('Authorize %s?', $name); - $panel = new AphrontPanelView(); - $panel->setWidth(AphrontPanelView::WIDTH_FORM); - $panel->setHeader($title); + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle(pht('Authenticate: %s', $name)) + ->appendParagraph( + pht( + 'This application ("%s") is authorized to use your Phabricator '. + 'credentials. Continue to complete the authentication workflow.', + phutil_tag('strong', array(), $name))) + ->addCancelButton((string)$full_uri, pht('Continue to Application')); - $description = - "Do want to authorize {$name} to access your ". - "Phabricator account data?"; + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + // Here, we're confirming authorization for the application. if ($scope) { if ($authorization) { @@ -154,10 +191,10 @@ $desired_scopes = $scope; } if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) { - $response->setError('invalid_scope'); - $response->setErrorDescription( - 'The requested scope is invalid, unknown, or malformed.'); - return $response; + return $this->buildErrorResponse( + 'invalid_scope', + pht('Invalid Scope'), + pht('The requested scope is invalid, unknown, or malformed.')); } } else { $desired_scopes = array( @@ -166,31 +203,67 @@ ); } - $cancel_uri = clone $uri; - $cancel_params = array( - 'error' => 'access_denied', - 'error_description' => - 'The resource owner (aka the user) denied the request.' - ); - $cancel_uri->setQueryParams($cancel_params); - $form = id(new AphrontFormView()) - ->setUser($current_user) + ->addHiddenInput('client_id', $client_phid) + ->addHiddenInput('redirect_uri', $redirect_uri) + ->addHiddenInput('response_type', $response_type) + ->addHiddenInput('state', $state) + ->addHiddenInput('scope', $request->getStr('scope')) + ->setUser($viewer) ->appendChild( - id(new AphrontFormStaticControl()) - ->setValue($description)) - ->appendChild( - PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes)) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue('Authorize') - ->addCancelButton($cancel_uri)); + PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes)); + + $cancel_msg = pht('The user declined to authorize this application.'); + $cancel_uri = $this->addQueryParams( + $uri, + array( + 'error' => 'access_denied', + 'error_description' => $cancel_msg, + )); + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle(pht('Authorize "%s"?', $name)) + ->setSubmitURI($request->getRequestURI()->getPath()) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->appendParagraph( + pht( + 'Do you want to authorize the external application "%s" to '. + 'access your Phabricator account data?', + phutil_tag('strong', array(), $name))) + ->appendChild($form->buildLayoutView()) + ->addSubmitButton(pht('Authorize Access')) + ->addCancelButton((string)$cancel_uri, pht('Do Not Authorize')); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + + private function buildErrorResponse($code, $title, $message) { + $viewer = $this->getRequest()->getUser(); + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle(pht('OAuth: %s', $title)) + ->appendParagraph($message) + ->appendParagraph( + pht('OAuth Error Code: %s', phutil_tag('tt', array(), $code))) + ->addCancelButton('/', pht('Alas!')); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + + private function addQueryParams(PhutilURI $uri, array $params) { + $full_uri = clone $uri; - $panel->appendChild($form); + foreach ($params as $key => $value) { + if (strlen($value)) { + $full_uri->setQueryParam($key, $value); + } + } - return $this->buildStandardPageResponse( - $panel, - array('title' => $title)); + return $full_uri; } } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php new file mode 100644 --- /dev/null +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerConsoleController.php @@ -0,0 +1,43 @@ +getRequest(); + $viewer = $request->getUser(); + + $menu = id(new PHUIObjectItemListView()) + ->setUser($viewer); + + $menu->addItem( + id(new PHUIObjectItemView()) + ->setHeader(pht('Authorizations')) + ->setHref($this->getApplicationURI('clientauthorization/')) + ->addAttribute( + pht( + 'Review your authorizations.'))); + + $menu->addItem( + id(new PHUIObjectItemView()) + ->setHeader(pht('Applications')) + ->setHref($this->getApplicationURI('client/')) + ->addAttribute( + pht( + 'Create a new OAuth application.'))); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb(pht('Console')); + + return $this->buildApplicationPage( + array( + $crumbs, + $menu, + ), + array( + 'title' => pht('OAuth Server Console'), + 'device' => true, + )); + } + +}