From a9791ea419c2e8de7e20d2f3d743db2608ef2e1d Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 23 Jan 2017 19:06:19 -0500 Subject: [PATCH] Have external login always make an API request to get the authorization URL This makes the OIDC lookup lazy, ensuring that the rest of the registry and app continues working even if one OIDC provider goes down. --- endpoints/api/user.py | 57 ++++++++++++++++--- oauth/base.py | 22 ++++++- oauth/loginmanager.py | 7 +++ oauth/oidc.py | 11 +--- .../js/directives/ui/external-login-button.js | 6 +- static/js/directives/ui/header-bar.js | 4 +- static/js/pages/signin.js | 9 +-- static/js/services/external-login-service.js | 41 ++++++------- test/test_api_security.py | 20 ++++++- 9 files changed, 128 insertions(+), 49 deletions(-) diff --git a/endpoints/api/user.py b/endpoints/api/user.py index 1ba32a12f..06e0250f7 100644 --- a/endpoints/api/user.py +++ b/endpoints/api/user.py @@ -11,7 +11,9 @@ from peewee import IntegrityError import features -from app import app, billing as stripe, authentication, avatar, user_analytics, all_queues +from app import (app, billing as stripe, authentication, avatar, user_analytics, all_queues, + oauth_login) + from auth import scopes from auth.auth_context import get_authenticated_user from auth.permissions import (AdministerOrganizationPermission, CreateRepositoryPermission, @@ -24,11 +26,12 @@ from endpoints.api import (ApiResource, nickname, resource, validate_json_reques query_param, require_scope, format_date, show_if, require_fresh_login, path_param, define_json_response, RepositoryParamResource, page_support) -from endpoints.exception import NotFound, InvalidToken +from endpoints.exception import NotFound, InvalidToken, InvalidRequest, DownstreamIssue from endpoints.api.subscribe import subscribe from endpoints.common import common_login from endpoints.csrf import generate_csrf_token, OAUTH_CSRF_TOKEN_NAME from endpoints.decorators import anon_allowed +from oauth.oidc import DiscoveryFailureException from util.useremails import (send_confirmation_email, send_recovery_email, send_change_email, send_password_changed, send_org_recovery_email) from util.names import parse_single_urn @@ -692,14 +695,50 @@ class Signout(ApiResource): return {'success': True} -@resource('/v1/externaltoken') +@resource('/v1/externallogin/') @internal_only -class GenerateExternalToken(ApiResource): - """ Resource for generating a token for external login. """ - @nickname('generateExternalLoginToken') - def post(self): - """ Generates a CSRF token explicitly for OIDC/OAuth-associated login. """ - return {'token': generate_csrf_token(OAUTH_CSRF_TOKEN_NAME)} +class ExternalLoginInformation(ApiResource): + """ Resource for both setting a token for external login and returning its authorization + url. + """ + schemas = { + 'GetLogin': { + 'type': 'object', + 'description': 'Information required to an retrieve external login URL.', + 'required': [ + 'kind', + ], + 'properties': { + 'kind': { + 'type': 'string', + 'description': 'The kind of URL', + 'enum': ['login', 'attach'], + }, + }, + }, + } + + + @nickname('retrieveExternalLoginAuthorizationUrl') + @anon_allowed + @validate_json_request('GetLogin') + def post(self, service_id): + """ Generates the auth URL and CSRF token explicitly for OIDC/OAuth-associated login. """ + login_service = oauth_login.get_service(service_id) + if login_service is None: + raise InvalidRequest() + + csrf_token = generate_csrf_token(OAUTH_CSRF_TOKEN_NAME) + kind = request.get_json()['kind'] + redirect_suffix = '/attach' if kind == 'attach' else '' + + try: + login_scopes = login_service.get_login_scopes() + auth_url = login_service.get_auth_url(app.config, redirect_suffix, csrf_token, login_scopes) + return {'auth_url': auth_url} + except DiscoveryFailureException as dfe: + logger.exception('Could not discovery OAuth endpoint information') + raise DownstreamIssue(dfe.message) @resource('/v1/detachexternal/') diff --git a/oauth/base.py b/oauth/base.py index 93b1c8a3c..15a82c1c4 100644 --- a/oauth/base.py +++ b/oauth/base.py @@ -1,4 +1,7 @@ import logging +import urllib + +from util import get_app_url logger = logging.getLogger(__name__) @@ -10,7 +13,6 @@ class OAuthGetUserInfoException(Exception): """ Exception raised if a call to get user information fails. """ pass - class OAuthService(object): """ A base class for defining an external service, exposed via OAuth. """ def __init__(self, config, key_name): @@ -38,6 +40,10 @@ class OAuthService(object): """ Performs validation of the client ID and secret, raising an exception on failure. """ raise NotImplementedError + def authorize_endpoint(self): + """ Endpoint for authorization. """ + raise NotImplementedError + def requires_form_encoding(self): """ Returns True if form encoding is necessary for the exchange_code_for_token call. """ return False @@ -48,6 +54,20 @@ class OAuthService(object): def client_secret(self): return self.config.get('CLIENT_SECRET') + def get_auth_url(self, app_config, redirect_suffix, csrf_token, scopes): + """ Retrieves the authorization URL for this login service. """ + redirect_uri = '%s/oauth2/%s/callback%s' % (get_app_url(app_config), self.service_id(), + redirect_suffix) + params = { + 'client_id': self.client_id(), + 'redirect_uri': redirect_uri, + 'scope': ' '.join(scopes), + 'state': csrf_token, + } + + authorize_url = '%s%s' % (self.authorize_endpoint(), urllib.urlencode(params)) + return authorize_url + def get_redirect_uri(self, app_config, redirect_suffix=''): return '%s://%s/oauth2/%s/callback%s' % (app_config['PREFERRED_URL_SCHEME'], app_config['SERVER_HOSTNAME'], diff --git a/oauth/loginmanager.py b/oauth/loginmanager.py index 2443d20dd..e65e100eb 100644 --- a/oauth/loginmanager.py +++ b/oauth/loginmanager.py @@ -22,3 +22,10 @@ class OAuthLoginManager(object): self.services.append(custom_service) else: self.services.append(OIDCLoginService(config, key)) + + def get_service(self, service_id): + for service in self.services: + if service.service_id() == service_id: + return service + + return None \ No newline at end of file diff --git a/oauth/oidc.py b/oauth/oidc.py index b51668a06..f5c274259 100644 --- a/oauth/oidc.py +++ b/oauth/oidc.py @@ -14,7 +14,6 @@ from jwkest.jwk import KEYS from oauth.base import OAuthService, OAuthExchangeCodeException, OAuthGetUserInfoException from oauth.login import OAuthLoginException from util.security.jwtutil import decode, InvalidTokenError -from util import get_app_url logger = logging.getLogger(__name__) @@ -28,7 +27,6 @@ class DiscoveryFailureException(Exception): """ Exception raised when OIDC discovery fails. """ pass - class PublicKeyLoadException(Exception): """ Exception raised if loading the OIDC public key fails. """ pass @@ -75,14 +73,7 @@ class OIDCLoginService(OAuthService): def validate_client_id_and_secret(self, http_client, app_config): # TODO: find a way to verify client secret too. - redirect_url = '%s/oauth2/%s/callback' % (get_app_url(app_config), self.service_id()) - scopes_string = ' '.join(self.get_login_scopes()) - authorize_url = '%sclient_id=%s&redirect_uri=%s&scope=%s' % (self.authorize_endpoint(), - self.client_id(), - redirect_url, - scopes_string) - - check_auth_url = http_client.get(authorize_url) + check_auth_url = http_client.get(self.get_auth_url()) if check_auth_url.status_code // 100 != 2: raise Exception('Got non-200 status code for authorization endpoint') diff --git a/static/js/directives/ui/external-login-button.js b/static/js/directives/ui/external-login-button.js index db79c2fee..f2abfe6b8 100644 --- a/static/js/directives/ui/external-login-button.js +++ b/static/js/directives/ui/external-login-button.js @@ -20,9 +20,7 @@ angular.module('quay').directive('externalLoginButton', function () { $scope.startSignin = function() { $scope.signInStarted({'service': $scope.provider}); - ApiService.generateExternalLoginToken().then(function(data) { - var url = ExternalLoginService.getLoginUrl($scope.provider, $scope.action || 'login'); - url = url + '&state=' + encodeURIComponent(data['token']); + ExternalLoginService.getLoginUrl($scope.provider, $scope.action || 'login', function(url) { // Save the redirect URL in a cookie so that we can redirect back after the service returns to us. var redirectURL = $scope.redirectUrl || window.location.toString(); @@ -34,7 +32,7 @@ angular.module('quay').directive('externalLoginButton', function () { $timeout(function() { document.location = url; }, 250); - }, ApiService.errorDisplay('Could not perform sign in')); + }); }; } }; diff --git a/static/js/directives/ui/header-bar.js b/static/js/directives/ui/header-bar.js index f41e6f313..8342df346 100644 --- a/static/js/directives/ui/header-bar.js +++ b/static/js/directives/ui/header-bar.js @@ -16,7 +16,9 @@ angular.module('quay').directive('headerBar', function () { PlanService, ApiService, NotificationService, Config, Features, DocumentationService, ExternalLoginService) { - $scope.externalSigninUrl = ExternalLoginService.getSingleSigninUrl(); + ExternalLoginService.getSingleSigninUrl(function(url) { + $scope.externalSigninUrl = url; + }); var hotkeysAdded = false; var userUpdated = function(cUser) { diff --git a/static/js/pages/signin.js b/static/js/pages/signin.js index f8bd1e92c..168e1eda8 100644 --- a/static/js/pages/signin.js +++ b/static/js/pages/signin.js @@ -11,9 +11,10 @@ function SignInCtrl($scope, $location, ExternalLoginService, Features) { $scope.redirectUrl = '/'; - var singleUrl = ExternalLoginService.getSingleSigninUrl(); - if (singleUrl) { - document.location = singleUrl; - } + ExternalLoginService.getSingleSigninUrl(function(singleUrl) { + if (singleUrl) { + document.location = singleUrl; + } + }); } })(); diff --git a/static/js/services/external-login-service.js b/static/js/services/external-login-service.js index 1b4b4f24e..a911fd07a 100644 --- a/static/js/services/external-login-service.js +++ b/static/js/services/external-login-service.js @@ -1,40 +1,43 @@ /** * Service which exposes the supported external logins. */ -angular.module('quay').factory('ExternalLoginService', ['Features', 'Config', - function(Features, Config) { +angular.module('quay').factory('ExternalLoginService', ['Features', 'Config', 'ApiService', + function(Features, Config, ApiService) { var externalLoginService = {}; externalLoginService.EXTERNAL_LOGINS = window.__external_login; - externalLoginService.getLoginUrl = function(loginService, action) { - var loginUrl = loginService['config']['AUTHORIZE_ENDPOINT']; - var clientId = loginService['config']['CLIENT_ID']; + externalLoginService.getLoginUrl = function(loginService, action, callback) { + var errorDisplay = ApiService.errorDisplay('Could not load external login service ' + + 'information. Please contact your service ' + + 'administrator.') - var scope = loginService.scopes.join(' '); - var redirectUri = Config.getUrl('/oauth2/' + loginService['id'] + '/callback'); + var params = { + 'service_id': loginService['id'] + }; - if (action == 'attach') { - redirectUri += '/attach'; - } + var data = { + 'kind': action + }; - var url = loginUrl + 'client_id=' + clientId + '&scope=' + scope + '&redirect_uri=' + - redirectUri; - return url; + ApiService.retrieveExternalLoginAuthorizationUrl(data, params).then(function(resp) { + callback(resp['auth_url']); + }, errorDisplay); }; externalLoginService.hasSingleSignin = function() { return externalLoginService.EXTERNAL_LOGINS.length == 1 && !Features.DIRECT_LOGIN; }; - externalLoginService.getSingleSigninUrl = function() { - // If there is a single external login service and direct login is disabled, - // then redirect to the external login directly. - if (externalLoginService.hasSingleSignin()) { - return externalLoginService.getLoginUrl(externalLoginService.EXTERNAL_LOGINS[0]); + externalLoginService.getSingleSigninUrl = function(callback) { + if (!externalLoginService.hasSingleSignin()) { + callback(null); + return; } - return null; + // If there is a single external login service and direct login is disabled, + // then redirect to the external login directly. + externalLoginService.getLoginUrl(externalLoginService.EXTERNAL_LOGINS[0], 'login', callback); }; return externalLoginService; diff --git a/test/test_api_security.py b/test/test_api_security.py index 7cb0d21b8..058c1c2de 100644 --- a/test/test_api_security.py +++ b/test/test_api_security.py @@ -28,7 +28,7 @@ from endpoints.api.repositorynotification import RepositoryNotification, Reposit from endpoints.api.user import (PrivateRepositories, ConvertToOrganization, Recovery, Signout, Signin, User, UserAuthorizationList, UserAuthorization, UserNotification, VerifyUser, DetachExternal, StarredRepositoryList, StarredRepository, - ClientKey) + ClientKey, ExternalLoginInformation) from endpoints.api.repotoken import RepositoryToken, RepositoryTokenList from endpoints.api.prototype import PermissionPrototype, PermissionPrototypeList from endpoints.api.logs import UserLogs, OrgLogs, RepositoryLogs @@ -504,6 +504,24 @@ class TestSignin(ApiTestCase): self._run_test('POST', 403, 'devtable', {u'username': 'E9RY', u'password': 'LQ0N'}) +class TestExternalLoginInformation(ApiTestCase): + def setUp(self): + ApiTestCase.setUp(self) + self._set_url(ExternalLoginInformation, service_id='someservice') + + def test_post_anonymous(self): + self._run_test('POST', 400, None, {}) + + def test_post_freshuser(self): + self._run_test('POST', 400, 'freshuser', {}) + + def test_post_reader(self): + self._run_test('POST', 400, 'reader', {}) + + def test_post_devtable(self): + self._run_test('POST', 400, 'devtable', {}) + + class TestDetachExternal(ApiTestCase): def setUp(self): ApiTestCase.setUp(self)