diff --git a/src/applications/oauthserver/PhabricatorOAuthServerScope.php b/src/applications/oauthserver/PhabricatorOAuthServerScope.php index be3b066a6e..38707fa3c3 100644 --- a/src/applications/oauthserver/PhabricatorOAuthServerScope.php +++ b/src/applications/oauthserver/PhabricatorOAuthServerScope.php @@ -18,22 +18,42 @@ final class PhabricatorOAuthServerScope { ); } - static public function getCheckboxControl($current_scopes) { + static public function getDefaultScope() { + return self::SCOPE_WHOAMI; + } + + static public 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; } - $checkboxes->setLabel('Scope'); - return $checkboxes; + if ($have_options) { + $checkboxes->setLabel(pht('Scope')); + return $checkboxes; + } + + return null; } static private function getCheckboxLabel($scope) { @@ -58,6 +78,7 @@ final class PhabricatorOAuthServerScope { $requested_scopes[$scope] = 1; } } + $requested_scopes[self::getDefaultScope()] = 1; return $requested_scopes; } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php index e38d276b93..c95aaaa823 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php @@ -13,7 +13,7 @@ final class PhabricatorOAuthServerAuthController $server = new PhabricatorOAuthServer(); $client_phid = $request->getStr('client_id'); - $scope = $request->getStr('scope', array()); + $scope = $request->getStr('scope'); $redirect_uri = $request->getStr('redirect_uri'); $response_type = $request->getStr('response_type'); @@ -119,6 +119,13 @@ final class PhabricatorOAuthServerAuthController 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'))); } // NOTE: We're always requiring a confirmation dialog to redirect. @@ -130,7 +137,6 @@ final class PhabricatorOAuthServerAuthController 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) { @@ -194,25 +200,16 @@ final class PhabricatorOAuthServerAuthController } // Here, we're confirming authorization for the application. - - if ($scope) { - if ($authorization) { - $desired_scopes = array_merge($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.')); - } + if ($authorization) { + $desired_scopes = array_merge($scope, $authorization->getScope()); } else { - $desired_scopes = array( - PhabricatorOAuthServerScope::SCOPE_WHOAMI => 1, - PhabricatorOAuthServerScope::SCOPE_OFFLINE_ACCESS => 1, - ); + $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.')); } $form = id(new AphrontFormView())