diff --git a/src/applications/conduit/call/ConduitCall.php b/src/applications/conduit/call/ConduitCall.php --- a/src/applications/conduit/call/ConduitCall.php +++ b/src/applications/conduit/call/ConduitCall.php @@ -65,10 +65,6 @@ return $this->handler->shouldAllowUnguardedWrites(); } - public function getRequiredScope() { - return $this->handler->getRequiredScope(); - } - public function getErrorDescription($code) { return $this->handler->getErrorDescription($code); } diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -45,7 +45,6 @@ $auth_error = null; $conduit_username = '-'; if ($call->shouldRequireAuthentication()) { - $metadata['scope'] = $call->getRequiredScope(); $auth_error = $this->authenticateUser($api_request, $metadata, $method); // If we've explicitly authenticated the user here and either done // CSRF validation or are using a non-web authentication mechanism. @@ -185,11 +184,6 @@ // First, verify the signature. try { $protocol_data = $metadata; - - // TODO: We should stop writing this into the protocol data when - // processing a request. - unset($protocol_data['scope']); - ConduitClient::verifySignature( $method, $api_request->getAllParameters(), @@ -362,11 +356,8 @@ $user); } - // handle oauth $access_token = idx($metadata, 'access_token'); - $method_scope = idx($metadata, 'scope'); - if ($access_token && - $method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) { + if ($access_token) { $token = id(new PhabricatorOAuthServerAccessToken()) ->loadOneWhere('token = %s', $access_token); if (!$token) { @@ -377,25 +368,33 @@ } $oauth_server = new PhabricatorOAuthServer(); - $valid = $oauth_server->validateAccessToken($token, - $method_scope); - if (!$valid) { + $authorization = $oauth_server->authorizeToken($token); + if (!$authorization) { return array( 'ERR-INVALID-AUTH', - pht('Access token is invalid.'), + pht('Access token is invalid or expired.'), ); } - // valid token, so let's log in the user! - $user_phid = $token->getUserPHID(); - $user = id(new PhabricatorUser()) - ->loadOneWhere('phid = %s', $user_phid); + $user = id(new PhabricatorPeopleQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($token->getUserPHID())) + ->executeOne(); if (!$user) { return array( 'ERR-INVALID-AUTH', pht('Access token is for invalid user.'), ); } + + $ok = $this->authorizeOAuthMethodAccess($authorization, $method); + if (!$ok) { + return array( + 'ERR-OAUTH-ACCESS', + pht('You do not have authorization to call this method.'), + ); + } + return $this->validateAuthenticatedUser( $api_request, $user); @@ -642,4 +641,30 @@ return array($metadata, $params); } + private function authorizeOAuthMethodAccess( + PhabricatorOAuthClientAuthorization $authorization, + $method_name) { + + $method = ConduitAPIMethod::getConduitMethod($method_name); + if (!$method) { + return false; + } + + $required_scope = $method->getRequiredScope(); + switch ($required_scope) { + case ConduitAPIMethod::SCOPE_ALWAYS: + return true; + case ConduitAPIMethod::SCOPE_NEVER: + return false; + } + + $authorization_scope = $authorization->getScope(); + if (!empty($authorization_scope[$required_scope])) { + return true; + } + + return false; + } + + } diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php --- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php +++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php @@ -142,6 +142,36 @@ pht('Errors'), $error_description); + + $scope = $method->getRequiredScope(); + switch ($scope) { + case ConduitAPIMethod::SCOPE_ALWAYS: + $oauth_icon = 'fa-globe green'; + $oauth_description = pht( + 'OAuth clients may always call this method.'); + break; + case ConduitAPIMethod::SCOPE_NEVER: + $oauth_icon = 'fa-ban red'; + $oauth_description = pht( + 'OAuth clients may never call this method.'); + break; + default: + $oauth_icon = 'fa-unlock-alt blue'; + $oauth_description = pht( + 'OAuth clients may call this method after requesting access to '. + 'the "%s" scope.', + $scope); + break; + } + + $view->addProperty( + pht('OAuth Scope'), + array( + id(new PHUIIconView())->setIcon($oauth_icon), + ' ', + $oauth_description, + )); + $view->addSectionHeader( pht('Description'), PHUIPropertyListView::ICON_SUMMARY); $view->addTextContent( diff --git a/src/applications/conduit/method/ConduitAPIMethod.php b/src/applications/conduit/method/ConduitAPIMethod.php --- a/src/applications/conduit/method/ConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitAPIMethod.php @@ -15,6 +15,8 @@ const METHOD_STATUS_UNSTABLE = 'unstable'; const METHOD_STATUS_DEPRECATED = 'deprecated'; + const SCOPE_NEVER = 'scope.never'; + const SCOPE_ALWAYS = 'scope.always'; /** * Get a short, human-readable text summary of the method. @@ -108,8 +110,7 @@ } public function getRequiredScope() { - // by default, conduit methods are not accessible via OAuth - return PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE; + return self::SCOPE_NEVER; } public function executeMethod(ConduitAPIRequest $request) { diff --git a/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php b/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php --- a/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitGetCapabilitiesConduitAPIMethod.php @@ -24,6 +24,10 @@ return 'dict'; } + public function getRequiredScope() { + return self::SCOPE_ALWAYS; + } + protected function execute(ConduitAPIRequest $request) { $authentication = array( 'token', diff --git a/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php b/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php --- a/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitQueryConduitAPIMethod.php @@ -18,6 +18,10 @@ return 'dict'; } + public function getRequiredScope() { + return self::SCOPE_ALWAYS; + } + protected function execute(ConduitAPIRequest $request) { $methods = id(new PhabricatorConduitMethodQuery()) ->setViewer($request->getUser()) 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 @@ -29,7 +29,6 @@ final class PhabricatorOAuthServer extends Phobject { const AUTHORIZATION_CODE_TIMEOUT = 300; - const ACCESS_TOKEN_TIMEOUT = 3600; private $user; private $client; @@ -158,39 +157,35 @@ /** * @task token */ - public function validateAccessToken( - PhabricatorOAuthServerAccessToken $token, - $required_scope) { - - $created_time = $token->getDateCreated(); - $must_be_used_by = $created_time + self::ACCESS_TOKEN_TIMEOUT; - $expired = time() > $must_be_used_by; - $authorization = id(new PhabricatorOAuthClientAuthorization()) - ->loadOneWhere( - 'userPHID = %s AND clientPHID = %s', - $token->getUserPHID(), - $token->getClientPHID()); + public function authorizeToken( + PhabricatorOAuthServerAccessToken $token) { + $user_phid = $token->getUserPHID(); + $client_phid = $token->getClientPHID(); + + $authorization = id(new PhabricatorOAuthClientAuthorizationQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withUserPHIDs(array($user_phid)) + ->withClientPHIDs(array($client_phid)) + ->executeOne(); if (!$authorization) { - return false; - } - $token_scope = $authorization->getScope(); - if (!isset($token_scope[$required_scope])) { - return false; + return null; } - $valid = true; - if ($expired) { - $valid = false; - // check if the scope includes "offline_access", which makes the - // token valid despite being expired - if (isset( - $token_scope[PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS])) { - $valid = true; + // 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. + + $is_expired = $token->isExpired(); + if ($is_expired) { + $offline_access = PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS; + $authorization_scope = $authorization->getScope(); + if (empty($authorization_scope[$offline_access])) { + return null; } } - return $valid; + return $authorization; } /** diff --git a/src/applications/oauthserver/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/PhabricatorOAuthServerScope.php --- a/src/applications/oauthserver/PhabricatorOAuthServerScope.php +++ b/src/applications/oauthserver/PhabricatorOAuthServerScope.php @@ -4,13 +4,7 @@ const SCOPE_OFFLINE_ACCESS = 'offline_access'; const SCOPE_WHOAMI = 'whoami'; - const SCOPE_NOT_ACCESSIBLE = 'not_accessible'; - /* - * Note this does not contain SCOPE_NOT_ACCESSIBLE which is magic - * used to simplify code for data that is not currently accessible - * via OAuth. - */ public static function getScopesDict() { return array( self::SCOPE_OFFLINE_ACCESS => 1, 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 @@ -144,7 +144,7 @@ $result = array( 'access_token' => $access_token->getToken(), 'token_type' => 'Bearer', - 'expires_in' => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT, + 'expires_in' => $access_token->getExpiresDuration(), ); return $response->setContent($result); } catch (Exception $e) { diff --git a/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php b/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php --- a/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php +++ b/src/applications/oauthserver/query/PhabricatorOAuthClientAuthorizationQuery.php @@ -7,7 +7,7 @@ private $userPHIDs; private $clientPHIDs; - public function witHPHIDs(array $phids) { + public function withPHIDs(array $phids) { $this->phids = $phids; return $this; } @@ -22,19 +22,12 @@ return $this; } + public function newResultObject() { + return new PhabricatorOAuthClientAuthorization(); + } + protected function loadPage() { - $table = new PhabricatorOAuthClientAuthorization(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T auth %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $authorizations) { @@ -49,43 +42,44 @@ foreach ($authorizations as $key => $authorization) { $client = idx($clients, $authorization->getClientPHID()); + if (!$client) { + $this->didRejectResult($authorization); unset($authorizations[$key]); continue; } + $authorization->attachClient($client); } return $authorizations; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->userPHIDs) { + if ($this->userPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'userPHID IN (%Ls)', $this->userPHIDs); } - if ($this->clientPHIDs) { + if ($this->clientPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'clientPHID IN (%Ls)', $this->clientPHIDs); } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php --- a/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php +++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerAccessToken.php @@ -22,4 +22,18 @@ ) + parent::getConfiguration(); } + public function isExpired() { + $now = PhabricatorTime::getNow(); + $expires_epoch = $this->getExpiresEpoch(); + return ($now > $expires_epoch); + } + + public function getExpiresEpoch() { + return $this->getDateCreated() + 3600; + } + + public function getExpiresDuration() { + return PhabricatorTime::getNow() - $this->getExpiresEpoch(); + } + }