From af295e0b26201e661d39674c7337976a8eaa1b09 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Tue, 21 Feb 2012 14:28:05 -0800 Subject: [PATCH] OAuth Server enhancements -- more complete access token response and groundwork for scope Summary: this patch makes the access token response "complete" relative to spec by returning when it expires AND that the token_type is in fact 'Bearer'. This patch also lays the groundwork for scope by fixing the underlying data model and adding the first scope checks for "offline_access" relative to expires and the "whoami" method. Further, conduit is augmented to open up individual methods for access via OAuth generally to enable "whoami" access. There's also a tidy little scope class to keep track of all the various scopes we plan to have as well as strings for display (T849 - work undone) Somewhat of a hack but Conduit methods by default have SCOPE_NOT_ACCESSIBLE. We then don't even bother with the OAuth stuff within conduit if we're not supposed to be accessing the method via Conduit. Felt relatively clean to me in terms of additional code complexity, etc. Next up ends up being T848 (scope in OAuth) and T849 (let user's authorize clients for specific scopes which kinds of needs T850). There's also a bunch of work that needs to be done to return the appropriate, well-formatted error codes. All in due time...! Test Plan: verified that an access_token with no scope doesn't let me see anything anymore. :( verified that access_tokens made awhile ago expire. :( Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley Maniphest Tasks: T888, T848 Differential Revision: https://secure.phabricator.com/D1657 --- resources/sql/patches/108.oauthscope.sql | 6 ++ src/__phutil_library_map__.php | 1 + .../api/PhabricatorConduitAPIController.php | 48 ++++++++--- .../conduit/controller/api/__init__.php | 2 + .../conduit/method/base/ConduitAPIMethod.php | 5 ++ .../conduit/method/base/__init__.php | 1 + .../whoami/ConduitAPI_user_whoami_Method.php | 6 +- .../conduit/method/user/whoami/__init__.php | 1 + .../PhabricatorOAuthServerAuthController.php | 10 ++- .../oauthserver/controller/auth/__init__.php | 1 + .../PhabricatorOAuthServerTokenController.php | 38 ++++++--- .../scope/PhabricatorOAuthServerScope.php | 37 ++++++++ .../oauthserver/scope/__init__.php | 10 +++ .../server/PhabricatorOAuthServer.php | 85 +++++++++++++++---- .../oauthserver/server/__init__.php | 1 + .../PhabricatorOAuthServerAccessToken.php | 1 - .../PhabricatorOAuthClientAuthorization.php | 5 +- 17 files changed, 212 insertions(+), 46 deletions(-) create mode 100644 resources/sql/patches/108.oauthscope.sql create mode 100644 src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php create mode 100644 src/applications/oauthserver/scope/__init__.php diff --git a/resources/sql/patches/108.oauthscope.sql b/resources/sql/patches/108.oauthscope.sql new file mode 100644 index 0000000000..874c80536e --- /dev/null +++ b/resources/sql/patches/108.oauthscope.sql @@ -0,0 +1,6 @@ +ALTER TABLE `phabricator_oauth_server`.`oauth_server_oauthclientauthorization` + ADD `scope` text NOT NULL; + +ALTER TABLE `phabricator_oauth_server`.`oauth_server_oauthserveraccesstoken` + DROP `dateExpires`; + diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d1123b3ead..427ba31cf8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -619,6 +619,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerAuthorizationCode' => 'applications/oauthserver/storage/authorizationcode', 'PhabricatorOAuthServerClient' => 'applications/oauthserver/storage/client', 'PhabricatorOAuthServerDAO' => 'applications/oauthserver/storage/base', + 'PhabricatorOAuthServerScope' => 'applications/oauthserver/scope', 'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/test', 'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/token', 'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink', diff --git a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php index 0a9bec59c0..f86895fea9 100644 --- a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php @@ -114,6 +114,7 @@ class PhabricatorConduitAPIController $allow_unguarded_writes = false; $auth_error = null; if ($method_handler->shouldRequireAuthentication()) { + $metadata['scope'] = $method_handler->getRequiredScope(); $auth_error = $this->authenticateUser($api_request, $metadata); // If we've explicitly authenticated the user here and either done // CSRF validation or are using a non-web authentication mechanism. @@ -248,24 +249,45 @@ class PhabricatorConduitAPIController } // handle oauth + // TODO - T897 (make error codes for OAuth more correct to spec) + // and T891 (strip shield from Conduit response) $access_token = $request->getStr('access_token'); - if ($access_token) { + $method_scope = $metadata['scope']; + if ($access_token && + $method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) { $token = id(new PhabricatorOAuthServerAccessToken()) ->loadOneWhere('token = %s', $access_token); - if ($token) { - // TODO - T888 -- add expiration date and refresh tokens to oauth - $user_phid = $token->getUserPHID(); - if ($user_phid) { - $user = id(new PhabricatorUser()) - ->loadOneWhere('phid = %s', - $user_phid); - if ($user) { - $api_request->setUser($user); - return null; - } - } + if (!$token) { + return array( + 'ERR-INVALID-AUTH', + 'Access token does not exist.', + ); } + + $oauth_server = new PhabricatorOAuthServer(); + $valid = $oauth_server->validateAccessToken($token, + $method_scope); + if (!$valid) { + return array( + 'ERR-INVALID-AUTH', + 'Access token is invalid.', + ); + } + + // valid token, so let's log in the user! + $user_phid = $token->getUserPHID(); + $user = id(new PhabricatorUser()) + ->loadOneWhere('phid = %s', + $user_phid); + if (!$user) { + return array( + 'ERR-INVALID-AUTH', + 'Access token is for invalid user.', + ); + } + $api_request->setUser($user); + return null; } // Handle sessionless auth. TOOD: This is super messy. diff --git a/src/applications/conduit/controller/api/__init__.php b/src/applications/conduit/controller/api/__init__.php index ccb5a219ec..cf4c7cab98 100644 --- a/src/applications/conduit/controller/api/__init__.php +++ b/src/applications/conduit/controller/api/__init__.php @@ -13,6 +13,8 @@ phutil_require_module('phabricator', 'applications/conduit/method/base'); phutil_require_module('phabricator', 'applications/conduit/protocol/request'); phutil_require_module('phabricator', 'applications/conduit/protocol/response'); phutil_require_module('phabricator', 'applications/conduit/storage/methodcalllog'); +phutil_require_module('phabricator', 'applications/oauthserver/scope'); +phutil_require_module('phabricator', 'applications/oauthserver/server'); phutil_require_module('phabricator', 'applications/oauthserver/storage/accesstoken'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/src/applications/conduit/method/base/ConduitAPIMethod.php b/src/applications/conduit/method/base/ConduitAPIMethod.php index 5fe2e93faa..a46c144d16 100644 --- a/src/applications/conduit/method/base/ConduitAPIMethod.php +++ b/src/applications/conduit/method/base/ConduitAPIMethod.php @@ -35,6 +35,11 @@ abstract class ConduitAPIMethod { return idx($this->defineErrorTypes(), $error_code, 'Unknown Error'); } + public function getRequiredScope() { + // by default, conduit methods are not accessible via OAuth + return PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE; + } + public function executeMethod(ConduitAPIRequest $request) { return $this->execute($request); } diff --git a/src/applications/conduit/method/base/__init__.php b/src/applications/conduit/method/base/__init__.php index 50c97a4d58..4b29e73d30 100644 --- a/src/applications/conduit/method/base/__init__.php +++ b/src/applications/conduit/method/base/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/oauthserver/scope'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'parser/uri'); diff --git a/src/applications/conduit/method/user/whoami/ConduitAPI_user_whoami_Method.php b/src/applications/conduit/method/user/whoami/ConduitAPI_user_whoami_Method.php index 0f080571e9..1be4109b05 100644 --- a/src/applications/conduit/method/user/whoami/ConduitAPI_user_whoami_Method.php +++ b/src/applications/conduit/method/user/whoami/ConduitAPI_user_whoami_Method.php @@ -1,7 +1,7 @@ buildUserInformationDictionary($request->getUser()); } diff --git a/src/applications/conduit/method/user/whoami/__init__.php b/src/applications/conduit/method/user/whoami/__init__.php index 7ee715a1bd..f0b6453676 100644 --- a/src/applications/conduit/method/user/whoami/__init__.php +++ b/src/applications/conduit/method/user/whoami/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'applications/conduit/method/user/base'); +phutil_require_module('phabricator', 'applications/oauthserver/scope'); phutil_require_source('ConduitAPI_user_whoami_Method.php'); diff --git a/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php index 0432cf05e7..177defb801 100644 --- a/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/auth/PhabricatorOAuthServerAuthController.php @@ -49,11 +49,15 @@ extends PhabricatorAuthController { ); } - if ($server->userHasAuthorizedClient($client)) { + $server->setClient($client); + if ($server->userHasAuthorizedClient()) { $return_auth_code = true; $unguarded_write = AphrontWriteGuard::beginScopedUnguardedWrites(); } else if ($request->isFormPost()) { - $server->authorizeClient($client); + // TODO -- T848 (add scope to Phabricator OAuth) + // should have some $scope based off of user submission here...! + $scope = array(PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1); + $server->authorizeClient($scope); $return_auth_code = true; $unguarded_write = null; } else { @@ -64,7 +68,7 @@ extends PhabricatorAuthController { if ($return_auth_code) { // step 1 -- generate authorization code $auth_code = - $server->generateAuthorizationCode($client); + $server->generateAuthorizationCode(); // step 2 -- error or return it if (!$auth_code) { diff --git a/src/applications/oauthserver/controller/auth/__init__.php b/src/applications/oauthserver/controller/auth/__init__.php index 7ce9dcf3e9..8a0cd0c8a6 100644 --- a/src/applications/oauthserver/controller/auth/__init__.php +++ b/src/applications/oauthserver/controller/auth/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/auth/controller/base'); phutil_require_module('phabricator', 'applications/oauthserver/response'); +phutil_require_module('phabricator', 'applications/oauthserver/scope'); phutil_require_module('phabricator', 'applications/oauthserver/server'); phutil_require_module('phabricator', 'applications/oauthserver/storage/client'); phutil_require_module('phabricator', 'view/form/base'); diff --git a/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php index 18179c597d..d61ba06f96 100644 --- a/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php +++ b/src/applications/oauthserver/controller/token/PhabricatorOAuthServerTokenController.php @@ -32,6 +32,7 @@ extends PhabricatorAuthController { $client_phid = $request->getStr('client_id'); $client_secret = $request->getStr('client_secret'); $response = new PhabricatorOAuthResponse(); + $server = new PhabricatorOAuthServer(); if (!$code) { return $response->setMalformed( 'Required parameter code missing.' @@ -48,6 +49,15 @@ extends PhabricatorAuthController { ); } + $client = id(new PhabricatorOAuthServerClient()) + ->loadOneWhere('phid = %s', $client_phid); + if (!$client) { + return $response->setNotFound( + 'Client with client_id '.$client_phid.' not found.' + ); + } + $server->setClient($client); + $auth_code = id(new PhabricatorOAuthServerAuthorizationCode()) ->loadOneWhere('code = %s', $code); if (!$auth_code) { @@ -55,9 +65,17 @@ extends PhabricatorAuthController { 'Authorization code '.$code.' not found.' ); } + + $user_phid = $auth_code->getUserPHID(); $user = id(new PhabricatorUser()) - ->loadOneWhere('phid = %s', $auth_code->getUserPHID()); - $server = new PhabricatorOAuthServer($user); + ->loadOneWhere('phid = %s', $user_phid); + if (!$user) { + return $response->setNotFound( + 'User with phid '.$user_phid.' not found.' + ); + } + $server->setUser($user); + $test_code = new PhabricatorOAuthServerAuthorizationCode(); $test_code->setClientSecret($client_secret); $test_code->setClientPHID($client_phid); @@ -69,19 +87,15 @@ extends PhabricatorAuthController { ); } - $client = id(new PhabricatorOAuthServerClient()) - ->loadOneWhere('phid = %s', $client_phid); - if (!$client) { - return $response->setNotFound( - 'Client with client_id '.$client_phid.' not found.' - ); - } - $scope = AphrontWriteGuard::beginScopedUnguardedWrites(); - $access_token = $server->generateAccessToken($client); + $access_token = $server->generateAccessToken(); if ($access_token) { $auth_code->delete(); - $result = array('access_token' => $access_token->getToken()); + $result = array( + 'access_token' => $access_token->getToken(), + 'token_type' => 'Bearer', + 'expires_in' => PhabricatorOAuthServer::ACCESS_TOKEN_TIMEOUT, + ); return $response->setContent($result); } diff --git a/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php new file mode 100644 index 0000000000..c2cee2bc6c --- /dev/null +++ b/src/applications/oauthserver/scope/PhabricatorOAuthServerScope.php @@ -0,0 +1,37 @@ + 1, + self::SCOPE_WHOAMI => 1, + ); + } + +} diff --git a/src/applications/oauthserver/scope/__init__.php b/src/applications/oauthserver/scope/__init__.php new file mode 100644 index 0000000000..cb94a7e09a --- /dev/null +++ b/src/applications/oauthserver/scope/__init__.php @@ -0,0 +1,10 @@ +user) { + throw new Exception('You must setUser before you can getUser!'); + } return $this->user; } - public function __construct(PhabricatorUser $user) { - if (!$user) { - throw new Exception('Must specify a Phabricator $user to constructor!'); - } + public function setUser(PhabricatorUser $user) { $this->user = $user; + return $this; + } + + /** + * @group internal + */ + private function getClient() { + if (!$this->client) { + throw new Exception('You must setClient before you can getClient!'); + } + return $this->client; + } + + public function setClient(PhabricatorOAuthServerClient $client) { + $this->client = $client; + return $this; } /** * @task auth */ - public function userHasAuthorizedClient( - PhabricatorOAuthServerClient $client) { + public function userHasAuthorizedClient() { $authorization = id(new PhabricatorOAuthClientAuthorization())-> loadOneWhere('userPHID = %s AND clientPHID = %s', $this->getUser()->getPHID(), - $client->getPHID()); + $this->getClient->getPHID()); if (empty($authorization)) { return false; @@ -85,20 +102,21 @@ final class PhabricatorOAuthServer { /** * @task auth */ - public function authorizeClient(PhabricatorOAuthServerClient $client) { + public function authorizeClient(array $scope) { $authorization = new PhabricatorOAuthClientAuthorization(); $authorization->setUserPHID($this->getUser()->getPHID()); - $authorization->setClientPHID($client->getPHID()); + $authorization->setClientPHID($this->getClient()->getPHID()); + $authorization->setScope($scope); $authorization->save(); } /** * @task auth */ - public function generateAuthorizationCode( - PhabricatorOAuthServerClient $client) { + public function generateAuthorizationCode() { - $code = Filesystem::readRandomCharacters(32); + $code = Filesystem::readRandomCharacters(32); + $client = $this->getClient(); $authorization_code = new PhabricatorOAuthServerAuthorizationCode(); $authorization_code->setCode($code); @@ -113,15 +131,14 @@ final class PhabricatorOAuthServer { /** * @task token */ - public function generateAccessToken(PhabricatorOAuthServerClient $client) { + public function generateAccessToken() { $token = Filesystem::readRandomCharacters(32); $access_token = new PhabricatorOAuthServerAccessToken(); $access_token->setToken($token); $access_token->setUserPHID($this->getUser()->getPHID()); - $access_token->setClientPHID($client->getPHID()); - $access_token->setDateExpires(0); + $access_token->setClientPHID($this->getClient()->getPHID()); $access_token->save(); return $access_token; @@ -148,4 +165,42 @@ final class PhabricatorOAuthServer { return (time() < $must_be_used_by); } + /** + * @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()); + + if (!$authorization) { + return false; + } + $token_scope = $authorization->getScope(); + if (!isset($token_scope[$required_scope])) { + return false; + } + + 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; + } + } + + return $valid; + } + } diff --git a/src/applications/oauthserver/server/__init__.php b/src/applications/oauthserver/server/__init__.php index 02ec597437..6fa671b012 100644 --- a/src/applications/oauthserver/server/__init__.php +++ b/src/applications/oauthserver/server/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/oauthserver/scope'); phutil_require_module('phabricator', 'applications/oauthserver/storage/accesstoken'); phutil_require_module('phabricator', 'applications/oauthserver/storage/authorizationcode'); phutil_require_module('phabricator', 'applications/oauthserver/storage/clientauthorization'); diff --git a/src/applications/oauthserver/storage/accesstoken/PhabricatorOAuthServerAccessToken.php b/src/applications/oauthserver/storage/accesstoken/PhabricatorOAuthServerAccessToken.php index 4d5eebedcd..6831ed40cd 100644 --- a/src/applications/oauthserver/storage/accesstoken/PhabricatorOAuthServerAccessToken.php +++ b/src/applications/oauthserver/storage/accesstoken/PhabricatorOAuthServerAccessToken.php @@ -25,5 +25,4 @@ extends PhabricatorOAuthServerDAO { protected $token; protected $userPHID; protected $clientPHID; - protected $dateExpires; } diff --git a/src/applications/oauthserver/storage/clientauthorization/PhabricatorOAuthClientAuthorization.php b/src/applications/oauthserver/storage/clientauthorization/PhabricatorOAuthClientAuthorization.php index 5f39e06540..f35c87278e 100644 --- a/src/applications/oauthserver/storage/clientauthorization/PhabricatorOAuthClientAuthorization.php +++ b/src/applications/oauthserver/storage/clientauthorization/PhabricatorOAuthClientAuthorization.php @@ -26,11 +26,14 @@ extends PhabricatorOAuthServerDAO { protected $phid; protected $userPHID; protected $clientPHID; - + protected $scope; public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, + self::CONFIG_SERIALIZATION => array( + 'scope' => self::SERIALIZATION_JSON, + ), ) + parent::getConfiguration(); }