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(); }