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 @@ -177,19 +177,6 @@ return null; } - // 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 $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 @@ -2,120 +2,20 @@ final class PhabricatorOAuthServerScope extends Phobject { - const SCOPE_OFFLINE_ACCESS = 'offline_access'; - const SCOPE_WHOAMI = 'whoami'; - - public static function getScopesDict() { - return array( - self::SCOPE_OFFLINE_ACCESS => 1, - self::SCOPE_WHOAMI => 1, - ); - } - - public static function getDefaultScope() { - return self::SCOPE_WHOAMI; - } - - public static function getCheckboxControl( - array $current_scopes) { - - $have_options = false; - $scopes = self::getScopesDict(); - $scope_keys = array_keys($scopes); - sort($scope_keys); - $default_scope = self::getDefaultScope(); - - $checkboxes = new AphrontFormCheckboxControl(); - foreach ($scope_keys as $scope) { - if ($scope == $default_scope) { - continue; - } - if (!isset($current_scopes[$scope])) { - continue; - } - - $checkboxes->addCheckbox( - $name = $scope, - $value = 1, - $label = self::getCheckboxLabel($scope), - $checked = isset($current_scopes[$scope])); - $have_options = true; - } - - if ($have_options) { - $checkboxes->setLabel(pht('Scope')); - return $checkboxes; - } - - return null; + public static function getScopeMap() { + return array(); } - private static function getCheckboxLabel($scope) { - $label = null; - switch ($scope) { - case self::SCOPE_OFFLINE_ACCESS: - $label = pht('Make access tokens granted to this client never expire.'); - break; - case self::SCOPE_WHOAMI: - $label = pht('Read access to Conduit method %s.', 'user.whoami'); - break; - } - - return $label; - } + public static function filterScope(array $scope) { + $valid_scopes = self::getScopeMap(); - public static function getScopesFromRequest(AphrontRequest $request) { - $scopes = self::getScopesDict(); - $requested_scopes = array(); - foreach ($scopes as $scope => $bit) { - if ($request->getBool($scope)) { - $requested_scopes[$scope] = 1; + foreach ($scope as $key => $scope_item) { + if (!isset($valid_scopes[$scope_item])) { + unset($scope[$key]); } } - $requested_scopes[self::getDefaultScope()] = 1; - return $requested_scopes; - } - - /** - * A scopes list is considered valid if each scope is a known scope - * and each scope is seen only once. Otherwise, the list is invalid. - */ - public static function validateScopesList($scope_list) { - $scopes = explode(' ', $scope_list); - $known_scopes = self::getScopesDict(); - $seen_scopes = array(); - foreach ($scopes as $scope) { - if (!isset($known_scopes[$scope])) { - return false; - } - if (isset($seen_scopes[$scope])) { - return false; - } - $seen_scopes[$scope] = 1; - } - - return true; - } - - /** - * A scopes dictionary is considered valid if each key is a known scope. - * Otherwise, the dictionary is invalid. - */ - public static function validateScopesDict($scope_dict) { - $known_scopes = self::getScopesDict(); - $unknown_scopes = array_diff_key($scope_dict, - $known_scopes); - return empty($unknown_scopes); - } - /** - * Transforms a space-delimited scopes list into a scopes dict. The list - * should be validated by @{method:validateScopesList} before - * transformation. - */ - public static function scopesListToDict($scope_list) { - $scopes = explode(' ', $scope_list); - return array_fill_keys($scopes, 1); + return $scope; } } 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 @@ -14,7 +14,6 @@ $server = new PhabricatorOAuthServer(); $client_phid = $request->getStr('client_id'); - $scope = $request->getStr('scope'); $redirect_uri = $request->getStr('redirect_uri'); $response_type = $request->getStr('response_type'); @@ -114,24 +113,11 @@ implode(', ', array('code')))); } - if ($scope) { - if (!PhabricatorOAuthServerScope::validateScopesList($scope)) { - 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); - } else { - return $this->buildErrorResponse( - 'invalid_request', - pht('Malformed Request'), - pht( - 'Required parameter %s was not present in the request.', - phutil_tag('strong', array(), 'scope'))); - } + + $requested_scope = $request->getStrList('scope'); + $requested_scope = array_fuse($requested_scope); + + $scope = PhabricatorOAuthServerScope::filterScope($requested_scope); // NOTE: We're always requiring a confirmation dialog to redirect. // Partly this is a general defense against redirect attacks, and @@ -142,8 +128,6 @@ list($is_authorized, $authorization) = $auth_info; if ($request->isFormPost()) { - $scope = PhabricatorOAuthServerScope::getScopesFromRequest($request); - if ($authorization) { $authorization->setScope($scope)->save(); } else { @@ -212,16 +196,9 @@ // Here, we're confirming authorization for the application. if ($authorization) { - $desired_scopes = array_merge($scope, $authorization->getScope()); + $missing_scope = array_diff_key($scope, $authorization->getScope()); } else { - $desired_scopes = $scope; - } - - if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) { - return $this->buildErrorResponse( - 'invalid_scope', - pht('Invalid Scope'), - pht('The requested scope is invalid, unknown, or malformed.')); + $missing_scope = $scope; } $form = id(new AphrontFormView()) @@ -230,9 +207,7 @@ ->addHiddenInput('response_type', $response_type) ->addHiddenInput('state', $state) ->addHiddenInput('scope', $request->getStr('scope')) - ->setUser($viewer) - ->appendChild( - PhabricatorOAuthServerScope::getCheckboxControl($desired_scopes)); + ->setUser($viewer); $cancel_msg = pht('The user declined to authorize this application.'); $cancel_uri = $this->addQueryParams( @@ -242,7 +217,7 @@ 'error_description' => $cancel_msg, )); - return $this->newDialog() + $dialog = $this->newDialog() ->setShortTitle(pht('Authorize Access')) ->setTitle(pht('Authorize "%s"?', $name)) ->setSubmitURI($request->getRequestURI()->getPath()) @@ -253,9 +228,41 @@ 'access your Phabricator account data, including your primary '. 'email address?', phutil_tag('strong', array(), $name))) - ->appendChild($form->buildLayoutView()) + ->appendForm($form) ->addSubmitButton(pht('Authorize Access')) ->addCancelButton((string)$cancel_uri, pht('Do Not Authorize')); + + if ($missing_scope) { + $dialog->appendParagraph( + pht( + 'This application has requested these additional permissions. '. + 'Authorizing it will grant it the permissions it requests:')); + foreach ($missing_scope as $scope_key => $ignored) { + // TODO: Once we introduce more scopes, explain them here. + } + } + + $unknown_scope = array_diff_key($requested_scope, $scope); + if ($unknown_scope) { + $dialog->appendParagraph( + pht( + 'This application also requested additional unrecognized '. + 'permissions. These permissions may have existed in an older '. + 'version of Phabricator, or may be from a future version of '. + 'Phabricator. They will not be granted.')); + + $unknown_form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel(pht('Unknown Scope')) + ->setValue(implode(', ', array_keys($unknown_scope))) + ->setDisabled(true)); + + $dialog->appendForm($unknown_form); + } + + return $dialog; } 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 @@ -154,8 +154,7 @@ unset($unguarded); $result = array( 'access_token' => $access_token->getToken(), - 'token_type' => 'Bearer', - 'expires_in' => $access_token->getExpiresDuration(), + 'token_type' => 'Bearer', ); return $response->setContent($result); } catch (Exception $e) { 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,18 +22,4 @@ ) + 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(); - } - } diff --git a/src/applications/people/conduit/UserWhoAmIConduitAPIMethod.php b/src/applications/people/conduit/UserWhoAmIConduitAPIMethod.php --- a/src/applications/people/conduit/UserWhoAmIConduitAPIMethod.php +++ b/src/applications/people/conduit/UserWhoAmIConduitAPIMethod.php @@ -19,7 +19,7 @@ } public function getRequiredScope() { - return PhabricatorOAuthServerScope::SCOPE_WHOAMI; + return self::SCOPE_ALWAYS; } protected function execute(ConduitAPIRequest $request) { diff --git a/src/docs/contributor/using_oauthserver.diviner b/src/docs/contributor/using_oauthserver.diviner --- a/src/docs/contributor/using_oauthserver.diviner +++ b/src/docs/contributor/using_oauthserver.diviner @@ -110,11 +110,7 @@ NOTE: See "Scopes" section below for more information on what data is currently exposed through the OAuth Server. -= Scopes = +Scopes +====== -There are only two scopes supported at this time. - -- **offline_access** - allows an access token to work indefinitely without - expiring. -- **whoami** - allows the client to access the results of Conduit.whoami on - behalf of the resource owner. +//This section has not been written yet.//