From bee2551dc239b05ea48a1c38dd1b451010882cf5 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 19 Jan 2017 14:51:12 -0500 Subject: [PATCH 01/13] Temporarily remove Dex login support This will be added back in later in this PR as part of proper generic OIDC support --- endpoints/oauthlogin.py | 77 ----------------------------------------- util/config/oauth.py | 19 ---------- 2 files changed, 96 deletions(-) diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index 17cb6da20..bbb5d86f5 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -292,80 +292,3 @@ def decode_user_jwt(token, oidc_provider): audience=oidc_provider.client_id(), issuer=oidc_provider.issuer) - -@oauthlogin.route('/dex/callback', methods=['GET', 'POST']) -@route_show_if(features.DEX_LOGIN) -@oauthlogin_csrf_protect -def dex_oauth_callback(): - error = request.values.get('error', None) - if error: - return render_ologin_error(dex_login.public_title, error) - - code = request.values.get('code') - if not code: - return render_ologin_error(dex_login.public_title, 'Missing OAuth code') - - token = dex_login.exchange_code_for_token(app.config, client, code, client_auth=True, - form_encode=True) - if token is None: - return render_ologin_error(dex_login.public_title) - - try: - payload = decode_user_jwt(token, dex_login) - except InvalidTokenError: - logger.exception('Exception when decoding returned JWT') - return render_ologin_error( - dex_login.public_title, - 'Could not decode response. Please contact your system administrator about this error.', - ) - - username = get_email_username(payload) - metadata = {} - - dex_id = payload['sub'] - email_address = payload['email'] - - if not payload.get('email_verified', False): - return render_ologin_error( - dex_login.public_title, - 'A verified e-mail address is required for login. Please verify your ' + - 'e-mail address in %s and try again.' % dex_login.public_title, - ) - - - return conduct_oauth_login(dex_login, dex_id, username, email_address, - metadata=metadata) - - -@oauthlogin.route('/dex/callback/attach', methods=['GET', 'POST']) -@route_show_if(features.DEX_LOGIN) -@require_session_login -@oauthlogin_csrf_protect -def dex_oauth_attach(): - code = request.args.get('code') - token = dex_login.exchange_code_for_token(app.config, client, code, redirect_suffix='/attach', - client_auth=True, form_encode=True) - if token is None: - return render_ologin_error(dex_login.public_title) - - try: - payload = decode_user_jwt(token, dex_login) - except InvalidTokenError: - logger.exception('Exception when decoding returned JWT') - return render_ologin_error( - dex_login.public_title, - 'Could not decode response. Please contact your system administrator about this error.', - ) - - user_obj = current_user.db_user() - dex_id = payload['sub'] - metadata = {} - - try: - model.user.attach_federated_login(user_obj, 'dex', dex_id, metadata=metadata) - except IntegrityError: - err = '%s account is already attached to a %s account' % (dex_login.public_title, - app.config['REGISTRY_TITLE_SHORT']) - return render_ologin_error(dex_login.public_title, err) - - return redirect(url_for('web.user_view', path=user_obj.username, tab='external')) diff --git a/util/config/oauth.py b/util/config/oauth.py index 44bf084f2..7676e7954 100644 --- a/util/config/oauth.py +++ b/util/config/oauth.py @@ -349,22 +349,3 @@ class OIDCConfig(OAuthConfig): # Reload the key so that we can give a key *instance* to PyJWT to work around its weird parsing # issues. return load_der_public_key(rsa_key.key.exportKey('DER'), backend=default_backend()) - - -class DexOAuthConfig(OIDCConfig): - def service_name(self): - return 'Dex' - - @property - def public_title(self): - return self.get_public_config()['OIDC_TITLE'] - - def get_public_config(self): - return { - 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint(), - - # TODO(jschorr): This should ideally come from the Dex side. - 'OIDC_TITLE': 'Dex', - 'OIDC_LOGO': 'https://tectonic.com/assets/ico/favicon-96x96.png' - } From 4755d08677c2b01c50b651ee95eed9495020dc48 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 19 Jan 2017 15:23:15 -0500 Subject: [PATCH 02/13] Refactor and rename the standard OAuth services --- app.py | 21 +-- endpoints/oauthlogin.py | 13 +- test/test_endpoints.py | 44 ----- util/oauth/__init__.py | 0 util/oauth/base.py | 63 +++++++ util/{config/oauth.py => oauth/services.py} | 183 ++------------------ 6 files changed, 82 insertions(+), 242 deletions(-) create mode 100644 util/oauth/__init__.py create mode 100644 util/oauth/base.py rename util/{config/oauth.py => oauth/services.py} (52%) diff --git a/app.py b/app.py index 953af51d8..e89328fbc 100644 --- a/app.py +++ b/app.py @@ -31,19 +31,15 @@ from util.saas.analytics import Analytics from util.saas.useranalytics import UserAnalytics from util.saas.exceptionlog import Sentry from util.names import urn_generator +from util.oauth.services import GoogleOAuthService, GithubOAuthService, GitLabOAuthService from util.config.configutil import generate_secret_key -from util.config.oauth import (GoogleOAuthConfig, GithubOAuthConfig, GitLabOAuthConfig, - DexOAuthConfig) from util.config.provider import get_config_provider from util.config.superusermanager import SuperUserManager from util.label_validator import LabelValidator -from util.license import LicenseValidator, LICENSE_FILENAME +from util.license import LicenseValidator from util.metrics.metricqueue import MetricQueue from util.metrics.prometheus import PrometheusPlugin -from util.names import urn_generator -from util.saas.analytics import Analytics from util.saas.cloudwatch import start_cloudwatch_sender -from util.saas.exceptionlog import Sentry from util.secscan.api import SecurityScannerAPI from util.security.instancekeys import InstanceKeys from util.security.signing import Signer @@ -204,13 +200,12 @@ license_validator.start() start_cloudwatch_sender(metric_queue, app) -github_login = GithubOAuthConfig(app.config, 'GITHUB_LOGIN_CONFIG') -github_trigger = GithubOAuthConfig(app.config, 'GITHUB_TRIGGER_CONFIG') -gitlab_trigger = GitLabOAuthConfig(app.config, 'GITLAB_TRIGGER_CONFIG') -google_login = GoogleOAuthConfig(app.config, 'GOOGLE_LOGIN_CONFIG') -dex_login = DexOAuthConfig(app.config, 'DEX_LOGIN_CONFIG') +github_login = GithubOAuthService(app.config, 'GITHUB_LOGIN_CONFIG') +github_trigger = GithubOAuthService(app.config, 'GITHUB_TRIGGER_CONFIG') +gitlab_trigger = GitLabOAuthService(app.config, 'GITLAB_TRIGGER_CONFIG') +google_login = GoogleOAuthService(app.config, 'GOOGLE_LOGIN_CONFIG') -oauth_apps = [github_login, github_trigger, gitlab_trigger, google_login, dex_login] +oauth_apps = [github_login, github_trigger, gitlab_trigger, google_login] image_replication_queue = WorkQueue(app.config['REPLICATION_QUEUE_NAME'], tf, has_namespace=False) dockerfile_build_queue = WorkQueue(app.config['DOCKERFILE_BUILD_QUEUE_NAME'], tf, @@ -240,7 +235,7 @@ model.config.register_image_cleanup_callback(secscan_api.cleanup_layers) @login_manager.user_loader def load_user(user_uuid): - logger.debug('User loader loading deferred user with uuid: %s' % user_uuid) + logger.debug('User loader loading deferred user with uuid: %s', user_uuid) return LoginWrappedDBUser(user_uuid) class LoginWrappedDBUser(UserMixin): diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index bbb5d86f5..98cca1839 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -7,7 +7,7 @@ from peewee import IntegrityError import features -from app import app, analytics, get_app_url, github_login, google_login, dex_login +from app import app, analytics, get_app_url, github_login, google_login from auth.process import require_session_login from data import model from endpoints.common import common_login, route_show_if @@ -281,14 +281,3 @@ def github_oauth_attach(): return redirect(url_for('web.user_view', path=user_obj.username, tab='external')) -def decode_user_jwt(token, oidc_provider): - try: - return decode(token, oidc_provider.get_public_key(), algorithms=['RS256'], - audience=oidc_provider.client_id(), - issuer=oidc_provider.issuer) - except InvalidTokenError: - # Public key may have expired. Try to retrieve an updated public key and use it to decode. - return decode(token, oidc_provider.get_public_key(force_refresh=True), algorithms=['RS256'], - audience=oidc_provider.client_id(), - issuer=oidc_provider.issuer) - diff --git a/test/test_endpoints.py b/test/test_endpoints.py index 685946ddf..bf12038d3 100644 --- a/test/test_endpoints.py +++ b/test/test_endpoints.py @@ -218,50 +218,6 @@ class OAuthLoginTestCase(EndpointTestCase): self.invoke_oauth_tests('github_oauth_callback', 'github_oauth_attach', 'github', 'someid', 'someusername') - def test_dex_oauth(self): - # TODO(jschorr): Add tests for invalid and expired keys. - - # Generate a public/private key pair for the OIDC transaction. - private_key = RSA.generate(2048) - jwk = RSAKey(key=private_key.publickey()).serialize() - token = jwt.encode({ - 'iss': 'https://oidcserver/', - 'aud': 'someclientid', - 'sub': 'someid', - 'exp': int(time.time()) + 60, - 'iat': int(time.time()), - 'nbf': int(time.time()), - 'email': 'someemail@example.com', - 'email_verified': True, - }, private_key.exportKey('PEM'), 'RS256') - - @urlmatch(netloc=r'oidcserver', path='/.well-known/openid-configuration') - def wellknown_handler(url, _): - return py_json.dumps({ - 'authorization_endpoint': 'http://oidcserver/auth', - 'token_endpoint': 'http://oidcserver/token', - 'jwks_uri': 'http://oidcserver/keys', - }) - - @urlmatch(netloc=r'oidcserver', path='/token') - def account_handler(url, request): - if request.body.find("code=somecode") > 0: - return py_json.dumps({ - 'access_token': token, - }) - else: - return {'status_code': 400, 'content': '{"message": "Invalid code"}'} - - @urlmatch(netloc=r'oidcserver', path='/keys') - def keys_handler(_, __): - return py_json.dumps({ - "keys": [jwk], - }) - - with HTTMock(wellknown_handler, account_handler, keys_handler): - self.invoke_oauth_tests('dex_oauth_callback', 'dex_oauth_attach', 'dex', - 'someid', 'someemail') - class WebEndpointTestCase(EndpointTestCase): def test_index(self): diff --git a/util/oauth/__init__.py b/util/oauth/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/util/oauth/base.py b/util/oauth/base.py new file mode 100644 index 000000000..9ede7cfdc --- /dev/null +++ b/util/oauth/base.py @@ -0,0 +1,63 @@ +class OAuthService(object): + """ A base class for defining an external service, exposed via OAuth. """ + def __init__(self, config, key_name): + self.key_name = key_name + self.config = config.get(key_name) or {} + + def service_name(self): + raise NotImplementedError + + def token_endpoint(self): + raise NotImplementedError + + def user_endpoint(self): + raise NotImplementedError + + def validate_client_id_and_secret(self, http_client, app_config): + raise NotImplementedError + + def client_id(self): + return self.config.get('CLIENT_ID') + + def client_secret(self): + return self.config.get('CLIENT_SECRET') + + 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'], + self.service_name().lower(), + redirect_suffix) + + def exchange_code_for_token(self, app_config, http_client, code, form_encode=False, + redirect_suffix='', client_auth=False): + payload = { + 'code': code, + 'grant_type': 'authorization_code', + 'redirect_uri': self.get_redirect_uri(app_config, redirect_suffix) + } + + headers = { + 'Accept': 'application/json' + } + + auth = None + if client_auth: + auth = (self.client_id(), self.client_secret()) + else: + payload['client_id'] = self.client_id() + payload['client_secret'] = self.client_secret() + + token_url = self.token_endpoint() + if form_encode: + get_access_token = http_client.post(token_url, data=payload, headers=headers, auth=auth) + else: + get_access_token = http_client.post(token_url, params=payload, headers=headers, auth=auth) + + if get_access_token.status_code / 100 != 2: + return None + + json_data = get_access_token.json() + if not json_data: + return None + + return json_data.get('access_token', None) diff --git a/util/config/oauth.py b/util/oauth/services.py similarity index 52% rename from util/config/oauth.py rename to util/oauth/services.py index 7676e7954..d5877fc14 100644 --- a/util/config/oauth.py +++ b/util/oauth/services.py @@ -1,87 +1,9 @@ -import urlparse -import json -import logging -import time - -from cachetools import TTLCache -from cachetools.func import lru_cache - -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives.serialization import load_der_public_key - -from jwkest.jwk import KEYS from util import slash_join +from util.oauth.base import OAuthService -logger = logging.getLogger(__name__) - -class OAuthConfig(object): +class GithubOAuthService(OAuthService): def __init__(self, config, key_name): - self.key_name = key_name - self.config = config.get(key_name) or {} - - def service_name(self): - raise NotImplementedError - - def token_endpoint(self): - raise NotImplementedError - - def user_endpoint(self): - raise NotImplementedError - - def validate_client_id_and_secret(self, http_client, app_config): - raise NotImplementedError - - def client_id(self): - return self.config.get('CLIENT_ID') - - def client_secret(self): - return self.config.get('CLIENT_SECRET') - - 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'], - self.service_name().lower(), - redirect_suffix) - - - def exchange_code_for_token(self, app_config, http_client, code, form_encode=False, - redirect_suffix='', client_auth=False): - payload = { - 'code': code, - 'grant_type': 'authorization_code', - 'redirect_uri': self.get_redirect_uri(app_config, redirect_suffix) - } - - headers = { - 'Accept': 'application/json' - } - - auth = None - if client_auth: - auth = (self.client_id(), self.client_secret()) - else: - payload['client_id'] = self.client_id() - payload['client_secret'] = self.client_secret() - - token_url = self.token_endpoint() - if form_encode: - get_access_token = http_client.post(token_url, data=payload, headers=headers, auth=auth) - else: - get_access_token = http_client.post(token_url, params=payload, headers=headers, auth=auth) - - if get_access_token.status_code / 100 != 2: - return None - - json_data = get_access_token.json() - if not json_data: - return None - - return json_data.get('access_token', None) - - -class GithubOAuthConfig(OAuthConfig): - def __init__(self, config, key_name): - super(GithubOAuthConfig, self).__init__(config, key_name) + super(GithubOAuthService, self).__init__(config, key_name) def service_name(self): return 'GitHub' @@ -174,10 +96,9 @@ class GithubOAuthConfig(OAuthConfig): } - -class GoogleOAuthConfig(OAuthConfig): +class GoogleOAuthService(OAuthService): def __init__(self, config, key_name): - super(GoogleOAuthConfig, self).__init__(config, key_name) + super(GoogleOAuthService, self).__init__(config, key_name) def service_name(self): return 'Google' @@ -215,13 +136,16 @@ class GoogleOAuthConfig(OAuthConfig): } -class GitLabOAuthConfig(OAuthConfig): +class GitLabOAuthService(OAuthService): def __init__(self, config, key_name): - super(GitLabOAuthConfig, self).__init__(config, key_name) + super(GitLabOAuthService, self).__init__(config, key_name) def _endpoint(self): return self.config.get('GITLAB_ENDPOINT', 'https://gitlab.com') + def user_endpoint(self): + raise NotImplementedError + def api_endpoint(self): return self._endpoint() @@ -262,90 +186,3 @@ class GitLabOAuthConfig(OAuthConfig): 'AUTHORIZE_ENDPOINT': self.authorize_endpoint(), 'GITLAB_ENDPOINT': self._endpoint(), } - - -OIDC_WELLKNOWN = ".well-known/openid-configuration" -PUBLIC_KEY_CACHE_TTL = 3600 # 1 hour - -class OIDCConfig(OAuthConfig): - def __init__(self, config, key_name): - super(OIDCConfig, self).__init__(config, key_name) - - self._public_key_cache = TTLCache(1, PUBLIC_KEY_CACHE_TTL, missing=self._get_public_key) - self._config = config - self._http_client = config['HTTPCLIENT'] - - @lru_cache(maxsize=1) - def _oidc_config(self): - if self.config.get('OIDC_SERVER'): - return self._load_via_discovery(self._config.get('DEBUGGING', False)) - else: - return {} - - def _load_via_discovery(self, is_debugging): - oidc_server = self.config['OIDC_SERVER'] - if not oidc_server.startswith('https://') and not is_debugging: - raise Exception('OIDC server must be accessed over SSL') - - discovery_url = urlparse.urljoin(oidc_server, OIDC_WELLKNOWN) - discovery = self._http_client.get(discovery_url, timeout=5) - - if discovery.status_code / 100 != 2: - raise Exception("Could not load OIDC discovery information") - - try: - return json.loads(discovery.text) - except ValueError: - logger.exception('Could not parse OIDC discovery for url: %s', discovery_url) - raise Exception("Could not parse OIDC discovery information") - - def authorize_endpoint(self): - return self._oidc_config().get('authorization_endpoint', '') + '?' - - def token_endpoint(self): - return self._oidc_config().get('token_endpoint') - - def user_endpoint(self): - return None - - def validate_client_id_and_secret(self, http_client, app_config): - pass - - def get_public_config(self): - return { - 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint() - } - - @property - def issuer(self): - return self.config.get('OIDC_ISSUER', self.config['OIDC_SERVER']) - - def get_public_key(self, force_refresh=False): - """ Retrieves the public key for this handler. """ - # If force_refresh is true, we expire all the items in the cache by setting the time to - # the current time + the expiration TTL. - if force_refresh: - self._public_key_cache.expire(time=time.time() + PUBLIC_KEY_CACHE_TTL) - - # Retrieve the public key from the cache. If the cache does not contain the public key, - # it will internally call _get_public_key to retrieve it and then save it. The None is - # a random key chose to be stored in the cache, and could be anything. - return self._public_key_cache[None] - - def _get_public_key(self, _): - """ Retrieves the public key for this handler. """ - keys_url = self._oidc_config()['jwks_uri'] - - keys = KEYS() - keys.load_from_url(keys_url) - - if not list(keys): - raise Exception('No keys provided by OIDC provider') - - rsa_key = list(keys)[0] - rsa_key.deserialize() - - # Reload the key so that we can give a key *instance* to PyJWT to work around its weird parsing - # issues. - return load_der_public_key(rsa_key.key.exportKey('DER'), backend=default_backend()) From 19f7acf57541643fef7a38e9ee51b250f1e332f7 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 20 Jan 2017 15:21:08 -0500 Subject: [PATCH 03/13] Lay foundation for truly dynamic external logins Moves all the external login services into a set of classes that share as much code as possible. These services are then registered on both the client and server, allowing us in the followup change to dynamically register new handlers --- app.py | 9 +- endpoints/api/user.py | 6 +- endpoints/common.py | 16 +- endpoints/oauthlogin.py | 272 +++++------------- {util/oauth => oauth}/__init__.py | 0 oauth/base.py | 124 ++++++++ oauth/login.py | 81 ++++++ oauth/loginmanager.py | 19 ++ oauth/oidc.py | 110 +++++++ oauth/services/__init__.py | 0 .../services.py => oauth/services/github.py | 150 +++++----- oauth/services/gitlab.py | 56 ++++ oauth/services/google.py | 76 +++++ static/directives/external-login-button.html | 9 +- .../directives/external-logins-manager.html | 17 +- static/directives/icon-image-view.html | 4 + static/directives/signin-form.html | 2 +- .../js/directives/ui/external-login-button.js | 3 +- .../directives/ui/external-logins-manager.js | 4 +- static/js/directives/ui/icon-image-view.js | 18 ++ static/js/services/external-login-service.js | 109 +------ templates/base.html | 1 + test/test_api_security.py | 2 +- test/test_endpoints.py | 4 +- test/testconfig.py | 3 + util/oauth/base.py | 63 ---- 26 files changed, 686 insertions(+), 472 deletions(-) rename {util/oauth => oauth}/__init__.py (100%) create mode 100644 oauth/base.py create mode 100644 oauth/login.py create mode 100644 oauth/loginmanager.py create mode 100644 oauth/oidc.py create mode 100644 oauth/services/__init__.py rename util/oauth/services.py => oauth/services/github.py (54%) create mode 100644 oauth/services/gitlab.py create mode 100644 oauth/services/google.py create mode 100644 static/directives/icon-image-view.html create mode 100644 static/js/directives/ui/icon-image-view.js delete mode 100644 util/oauth/base.py diff --git a/app.py b/app.py index e89328fbc..17baed75d 100644 --- a/app.py +++ b/app.py @@ -25,13 +25,15 @@ from data.queue import WorkQueue, BuildMetricQueueReporter from data.userevent import UserEventsBuilderModule from data.userfiles import Userfiles from data.users import UserAuthentication +from oauth.services.github import GithubOAuthService +from oauth.services.gitlab import GitLabOAuthService +from oauth.loginmanager import OAuthLoginManager from storage import Storage from util import get_app_url from util.saas.analytics import Analytics from util.saas.useranalytics import UserAnalytics from util.saas.exceptionlog import Sentry from util.names import urn_generator -from util.oauth.services import GoogleOAuthService, GithubOAuthService, GitLabOAuthService from util.config.configutil import generate_secret_key from util.config.provider import get_config_provider from util.config.superusermanager import SuperUserManager @@ -200,12 +202,11 @@ license_validator.start() start_cloudwatch_sender(metric_queue, app) -github_login = GithubOAuthService(app.config, 'GITHUB_LOGIN_CONFIG') github_trigger = GithubOAuthService(app.config, 'GITHUB_TRIGGER_CONFIG') gitlab_trigger = GitLabOAuthService(app.config, 'GITLAB_TRIGGER_CONFIG') -google_login = GoogleOAuthService(app.config, 'GOOGLE_LOGIN_CONFIG') -oauth_apps = [github_login, github_trigger, gitlab_trigger, google_login] +oauth_login = OAuthLoginManager(app.config) +oauth_apps = [github_trigger, gitlab_trigger] image_replication_queue = WorkQueue(app.config['REPLICATION_QUEUE_NAME'], tf, has_namespace=False) dockerfile_build_queue = WorkQueue(app.config['DOCKERFILE_BUILD_QUEUE_NAME'], tf, diff --git a/endpoints/api/user.py b/endpoints/api/user.py index c969926dd..1ba32a12f 100644 --- a/endpoints/api/user.py +++ b/endpoints/api/user.py @@ -702,16 +702,16 @@ class GenerateExternalToken(ApiResource): return {'token': generate_csrf_token(OAUTH_CSRF_TOKEN_NAME)} -@resource('/v1/detachexternal/') +@resource('/v1/detachexternal/') @show_if(features.DIRECT_LOGIN) @internal_only class DetachExternal(ApiResource): """ Resource for detaching an external login. """ @require_user_admin @nickname('detachExternalLogin') - def post(self, servicename): + def post(self, service_id): """ Request that the current user be detached from the external login service. """ - model.user.detach_external_login(get_authenticated_user(), servicename) + model.user.detach_external_login(get_authenticated_user(), service_id) return {'success': True} diff --git a/endpoints/common.py b/endpoints/common.py index 6fd7ff348..fd52ee868 100644 --- a/endpoints/common.py +++ b/endpoints/common.py @@ -16,7 +16,7 @@ from flask_principal import identity_changed import endpoints.decorated # Register the various exceptions via decorators. import features -from app import app, oauth_apps, LoginWrappedDBUser, user_analytics, license_validator +from app import app, oauth_apps, oauth_login, LoginWrappedDBUser, user_analytics, license_validator from auth import scopes from auth.permissions import QuayDeferredPermissionUser from config import frontend_visible_config @@ -189,6 +189,19 @@ def render_page_template(name, route_data=None, **kwargs): cache_buster = cachebusters.get(filename, random_string()) if not debugging else 'debugging' yield (filename, cache_buster) + def get_external_login_config(): + login_config = [] + for login_service in oauth_login.services: + login_config.append({ + 'id': login_service.service_id(), + 'title': login_service.service_name(), + 'config': login_service.get_public_config(), + 'icon': login_service.get_icon(), + 'scopes': login_service.get_login_scopes(), + }) + + return login_config + def get_oauth_config(): oauth_config = {} for oauth_app in oauth_apps: @@ -215,6 +228,7 @@ def render_page_template(name, route_data=None, **kwargs): feature_set=features.get_features(), config_set=frontend_visible_config(app.config), oauth_set=get_oauth_config(), + external_login_set=get_external_login_config(), scope_set=scopes.app_scopes(app.config), vuln_priority_set=PRIORITY_LEVELS, enterprise_logo=app.config.get('ENTERPRISE_LOGO_URL', ''), diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index 98cca1839..0c7865213 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -1,19 +1,18 @@ import logging -import requests from flask import request, redirect, url_for, Blueprint -from flask_login import current_user from peewee import IntegrityError import features -from app import app, analytics, get_app_url, github_login, google_login +from app import app, analytics, get_app_url, oauth_login +from auth.auth_context import get_authenticated_user from auth.process import require_session_login from data import model -from endpoints.common import common_login, route_show_if +from endpoints.common import common_login from endpoints.web import index from endpoints.csrf import csrf_protect, OAUTH_CSRF_TOKEN_NAME -from util.security.jwtutil import decode, InvalidTokenError +from oauth.login import OAuthLoginException from util.validation import generate_valid_usernames logger = logging.getLogger(__name__) @@ -22,7 +21,9 @@ oauthlogin = Blueprint('oauthlogin', __name__) oauthlogin_csrf_protect = csrf_protect(OAUTH_CSRF_TOKEN_NAME, 'state', all_methods=True) -def render_ologin_error(service_name, error_message=None, register_redirect=False): +def _render_ologin_error(service_name, error_message=None, register_redirect=False): + """ Returns a Flask response indicating an OAuth error. """ + user_creation = bool(features.USER_CREATION and features.DIRECT_LOGIN) error_info = { 'reason': 'ologinerror', @@ -37,27 +38,15 @@ def render_ologin_error(service_name, error_message=None, register_redirect=Fals resp.status_code = 400 return resp +def _conduct_oauth_login(service_id, service_name, user_id, username, email, metadata=None): + """ Conducts login from the result of an OAuth service's login flow. """ -def get_user(service, token): - token_param = { - 'access_token': token, - 'alt': 'json', - } - got_user = client.get(service.user_endpoint(), params=token_param) - if got_user.status_code != requests.codes.ok: - return {} - - return got_user.json() - - -def conduct_oauth_login(service, user_id, username, email, metadata=None): - service_name = service.service_name() - to_login = model.user.verify_federated_login(service_name.lower(), user_id) + to_login = model.user.verify_federated_login(service_id, user_id) if not to_login: # See if we can create a new user. if not features.USER_CREATION: error_message = 'User creation is disabled. Please contact your administrator' - return render_ologin_error(service_name, error_message) + return _render_ologin_error(service_name, error_message) # Try to create the user try: @@ -70,7 +59,7 @@ def conduct_oauth_login(service, user_id, username, email, metadata=None): break prompts = model.user.get_default_user_prompts(features) - to_login = model.user.create_federated_user(new_username, email, service_name.lower(), + to_login = model.user.create_federated_user(new_username, email, service_id, user_id, set_password_notification=True, metadata=metadata or {}, prompts=prompts) @@ -84,10 +73,10 @@ def conduct_oauth_login(service, user_id, username, email, metadata=None): message = message + "\nPlease log in with your username and password and " message = message + "associate your %s account to use it in the future." % (service_name, ) - return render_ologin_error(service_name, message, register_redirect=True) + return _render_ologin_error(service_name, message, register_redirect=True) except model.DataModelException as ex: - return render_ologin_error(service_name, ex.message) + return _render_ologin_error(service_name, ex.message) if common_login(to_login): if model.user.has_user_prompts(to_login): @@ -95,189 +84,78 @@ def conduct_oauth_login(service, user_id, username, email, metadata=None): else: return redirect(url_for('web.index')) - return render_ologin_error(service_name) + return _render_ologin_error(service_name) -def get_email_username(user_data): - username = user_data['email'] - at = username.find('@') - if at > 0: - username = username[0:at] +def _register_service(login_service): + """ Registers the given login service, adding its callback and attach routes to the blueprint. """ - return username + @oauthlogin_csrf_protect + def callback_func(): + # Check for a callback error. + error = request.args.get('error', None) + if error: + return _render_ologin_error(login_service.service_name(), error) + + # Exchange the OAuth code for login information. + code = request.args.get('code') + try: + lid, lusername, lemail = login_service.exchange_code_for_login(app.config, client, code, '') + except OAuthLoginException as ole: + return _render_ologin_error(login_service.service_name(), ole.message) + + # Conduct login. + metadata = { + 'service_username': lusername + } + + return _conduct_oauth_login(login_service.service_id(), login_service.service_name(), lid, + lusername, lemail, metadata=metadata) -@oauthlogin.route('/google/callback', methods=['GET']) -@route_show_if(features.GOOGLE_LOGIN) -@oauthlogin_csrf_protect -def google_oauth_callback(): - error = request.args.get('error', None) - if error: - return render_ologin_error('Google', error) + @require_session_login + @oauthlogin_csrf_protect + def attach_func(): + # Check for a callback error. + error = request.args.get('error', None) + if error: + return _render_ologin_error(login_service.service_name(), error) - code = request.args.get('code') - token = google_login.exchange_code_for_token(app.config, client, code, form_encode=True) - if token is None: - return render_ologin_error('Google') + # Exchange the OAuth code for login information. + code = request.args.get('code') + try: + lid, lusername, _ = login_service.exchange_code_for_login(app.config, client, code, '/attach') + except OAuthLoginException as ole: + return _render_ologin_error(login_service.service_name(), ole.message) - user_data = get_user(google_login, token) - if not user_data or not user_data.get('id', None) or not user_data.get('email', None): - return render_ologin_error('Google') + # Conduct attach. + metadata = { + 'service_username': lusername + } - if not user_data.get('verified_email', False): - return render_ologin_error( - 'Google', - 'A verified e-mail address is required for login. Please verify your ' + - 'e-mail address in Google and try again.', - ) + user_obj = get_authenticated_user() - username = get_email_username(user_data) - metadata = { - 'service_username': user_data['email'] - } + try: + model.user.attach_federated_login(user_obj, login_service.service_id(), lid, + metadata=metadata) + except IntegrityError: + err = '%s account %s is already attached to a %s account' % ( + login_service.service_name(), lusername, app.config['REGISTRY_TITLE_SHORT']) + return _render_ologin_error(login_service.service_name(), err) - return conduct_oauth_login(google_login, user_data['id'], username, user_data['email'], - metadata=metadata) + return redirect(url_for('web.user_view', path=user_obj.username, tab='external')) -@oauthlogin.route('/github/callback', methods=['GET']) -@route_show_if(features.GITHUB_LOGIN) -@oauthlogin_csrf_protect -def github_oauth_callback(): - error = request.args.get('error', None) - if error: - return render_ologin_error('GitHub', error) - - # Exchange the OAuth code. - code = request.args.get('code') - token = github_login.exchange_code_for_token(app.config, client, code) - if token is None: - return render_ologin_error('GitHub') - - # Retrieve the user's information. - user_data = get_user(github_login, token) - if not user_data or 'login' not in user_data: - return render_ologin_error('GitHub') - - username = user_data['login'] - github_id = user_data['id'] - - v3_media_type = { - 'Accept': 'application/vnd.github.v3' - } - - token_param = { - 'access_token': token, - } - - # Retrieve the user's orgnizations (if organization filtering is turned on) - if github_login.allowed_organizations() is not None: - get_orgs = client.get(github_login.orgs_endpoint(), params=token_param, - headers={'Accept': 'application/vnd.github.moondragon+json'}) - - organizations = set([org.get('login').lower() for org in get_orgs.json()]) - matching_organizations = organizations & set(github_login.allowed_organizations()) - if not matching_organizations: - err = """You are not a member of an allowed GitHub organization. - Please contact your system administrator if you believe this is in error.""" - return render_ologin_error('GitHub', err) - - # Find the e-mail address for the user: we will accept any email, but we prefer the primary - get_email = client.get(github_login.email_endpoint(), params=token_param, - headers=v3_media_type) - if get_email.status_code / 100 != 2: - return render_ologin_error('GitHub') - - found_email = None - for user_email in get_email.json(): - if not github_login.is_enterprise() and not user_email['verified']: - continue - - found_email = user_email['email'] - if user_email['primary']: - break - - if found_email is None: - err = 'There is no verified e-mail address attached to the GitHub account.' - return render_ologin_error('GitHub', err) - - metadata = { - 'service_username': username - } - - return conduct_oauth_login(github_login, github_id, username, found_email, metadata=metadata) - - -@oauthlogin.route('/google/callback/attach', methods=['GET']) -@route_show_if(features.GOOGLE_LOGIN) -@require_session_login -@oauthlogin_csrf_protect -def google_oauth_attach(): - code = request.args.get('code') - token = google_login.exchange_code_for_token(app.config, client, code, - redirect_suffix='/attach', form_encode=True) - if token is None: - return render_ologin_error('Google') - - user_data = get_user(google_login, token) - if not user_data or not user_data.get('id', None): - return render_ologin_error('Google') - - if not user_data.get('verified_email', False): - return render_ologin_error( - 'Google', - 'A verified e-mail address is required for login. Please verify your ' + - 'e-mail address in Google and try again.', - ) - - google_id = user_data['id'] - user_obj = current_user.db_user() - - username = get_email_username(user_data) - metadata = { - 'service_username': user_data['email'] - } - - try: - model.user.attach_federated_login(user_obj, 'google', google_id, metadata=metadata) - except IntegrityError: - err = 'Google account %s is already attached to a %s account' % ( - username, app.config['REGISTRY_TITLE_SHORT']) - return render_ologin_error('Google', err) - - return redirect(url_for('web.user_view', path=user_obj.username, tab='external')) - - -@oauthlogin.route('/github/callback/attach', methods=['GET']) -@route_show_if(features.GITHUB_LOGIN) -@require_session_login -@oauthlogin_csrf_protect -def github_oauth_attach(): - code = request.args.get('code') - token = github_login.exchange_code_for_token(app.config, client, code) - if token is None: - return render_ologin_error('GitHub') - - user_data = get_user(github_login, token) - if not user_data: - return render_ologin_error('GitHub') - - github_id = user_data['id'] - user_obj = current_user.db_user() - - username = user_data['login'] - metadata = { - 'service_username': username - } - - try: - model.user.attach_federated_login(user_obj, 'github', github_id, metadata=metadata) - except IntegrityError: - err = 'Github account %s is already attached to a %s account' % ( - username, app.config['REGISTRY_TITLE_SHORT']) - - return render_ologin_error('GitHub', err) - - return redirect(url_for('web.user_view', path=user_obj.username, tab='external')) + oauthlogin.add_url_rule('/%s/callback' % login_service.service_id(), + '%s_oauth_callback' % login_service.service_id(), + callback_func, + methods=['GET']) + oauthlogin.add_url_rule('/%s/callback/attach' % login_service.service_id(), + '%s_oauth_attach' % login_service.service_id(), + attach_func, + methods=['GET']) +# Register the routes for each of the login services. +for current_service in oauth_login.services: + _register_service(current_service) diff --git a/util/oauth/__init__.py b/oauth/__init__.py similarity index 100% rename from util/oauth/__init__.py rename to oauth/__init__.py diff --git a/oauth/base.py b/oauth/base.py new file mode 100644 index 000000000..f25bbc6ab --- /dev/null +++ b/oauth/base.py @@ -0,0 +1,124 @@ +import logging + +logger = logging.getLogger(__name__) + +class OAuthExchangeCodeException(Exception): + """ Exception raised if a code exchange fails. """ + pass + +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): + self.key_name = key_name + self.config = config.get(key_name) or {} + + def service_id(self): + """ The internal ID for this service. Must match the URL portion for the service, e.g. `github` + """ + raise NotImplementedError + + def service_name(self): + """ The user-readable name for the service, e.g. `GitHub`""" + raise NotImplementedError + + def token_endpoint(self): + """ The endpoint at which the OAuth code can be exchanged for a token. """ + raise NotImplementedError + + def user_endpoint(self): + """ The endpoint at which user information can be looked up. """ + raise NotImplementedError + + def validate_client_id_and_secret(self, http_client, app_config): + """ Performs validation of the client ID and secret, raising an exception on failure. """ + raise NotImplementedError + + def requires_form_encoding(self): + """ Returns True if form encoding is necessary for the exchange_code_for_token call. """ + return False + + def client_id(self): + return self.config.get('CLIENT_ID') + + def client_secret(self): + return self.config.get('CLIENT_SECRET') + + 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'], + self.service_name().lower(), + redirect_suffix) + + def get_user_info(self, http_client, token): + token_param = { + 'access_token': token, + 'alt': 'json', + } + + got_user = http_client.get(self.user_endpoint(), params=token_param) + if got_user.status_code // 100 != 2: + raise OAuthGetUserInfoException('Non-2XX response code for user_info call: %s' % + got_user.status_code) + + user_info = got_user.json() + if user_info is None: + raise OAuthGetUserInfoException() + + return user_info + + def exchange_code_for_token(self, app_config, http_client, code, form_encode=False, + redirect_suffix='', client_auth=False): + """ Exchanges an OAuth access code for the associated OAuth token. """ + json_data = self._exchange_code(app_config, http_client, code, form_encode, redirect_suffix, + client_auth) + + access_token = json_data.get('access_token', None) + if access_token is None: + logger.debug('Got successful get_access_token response %s', json_data) + raise OAuthExchangeCodeException('Missing `access_token` in OAuth response') + + return access_token + + def _exchange_code(self, app_config, http_client, code, form_encode=False, redirect_suffix='', + client_auth=False): + payload = { + 'code': code, + 'grant_type': 'authorization_code', + 'redirect_uri': self.get_redirect_uri(app_config, redirect_suffix) + } + + headers = { + 'Accept': 'application/json' + } + + auth = None + if client_auth: + auth = (self.client_id(), self.client_secret()) + else: + payload['client_id'] = self.client_id() + payload['client_secret'] = self.client_secret() + + token_url = self.token_endpoint() + if form_encode: + get_access_token = http_client.post(token_url, data=payload, headers=headers, auth=auth) + else: + get_access_token = http_client.post(token_url, params=payload, headers=headers, auth=auth) + + if get_access_token.status_code // 100 != 2: + logger.debug('Got get_access_token response %s', get_access_token.text) + raise OAuthExchangeCodeException('Got non-2XX response for code exchange: %s' % + get_access_token.status_code) + + json_data = get_access_token.json() + if not json_data: + raise OAuthExchangeCodeException('Got non-JSON response for code exchange') + + if 'error' in json_data: + raise OAuthExchangeCodeException(json_data.get('error_description', json_data['error'])) + + return json_data diff --git a/oauth/login.py b/oauth/login.py new file mode 100644 index 000000000..a4aa5524e --- /dev/null +++ b/oauth/login.py @@ -0,0 +1,81 @@ +import logging + +import features + +from oauth.base import OAuthService, OAuthExchangeCodeException, OAuthGetUserInfoException + +logger = logging.getLogger(__name__) + +class OAuthLoginException(Exception): + """ Exception raised if a login operation fails. """ + pass + +class OAuthLoginService(OAuthService): + """ A base class for defining an OAuth-compliant service that can be used for, amongst other + things, login and authentication. """ + + def get_login_service_id(self, user_info): + """ Returns the internal ID for the given user under this login service. """ + raise NotImplementedError + + def get_login_service_username(self, user_info): + """ Returns the username for the given user under this login service. """ + raise NotImplementedError + + def get_verified_user_email(self, app_config, http_client, token, user_info): + """ Returns the verified email address for the given user, if any or None if none. """ + raise NotImplementedError + + def get_icon(self): + """ Returns the icon to display for this login service. """ + raise NotImplementedError + + def get_login_scopes(self): + """ Returns the list of scopes for login for this service. """ + raise NotImplementedError + + def service_verify_user_info_for_login(self, app_config, http_client, token, user_info): + """ Performs service-specific verification of user information for login. On failure, a service + should raise a OAuthLoginService. + """ + # By default, does nothing. + pass + + def exchange_code_for_login(self, app_config, http_client, code, redirect_suffix): + """ Exchanges the given OAuth access code for user information on behalf of a user trying to + login or attach their account. Raises a OAuthLoginService exception on failure. Returns + a tuple consisting of (service_id, service_username, email) + """ + + # Retrieve the token for the OAuth code. + try: + token = self.exchange_code_for_token(app_config, http_client, code, + redirect_suffix=redirect_suffix, + form_encode=self.requires_form_encoding()) + except OAuthExchangeCodeException as oce: + raise OAuthLoginException(oce.message) + + # Retrieve the user's information with the token. + try: + user_info = self.get_user_info(http_client, token) + except OAuthGetUserInfoException as oge: + raise OAuthLoginException(oge.message) + + if user_info.get('id', None) is None: + logger.debug('Got user info response %s', user_info) + raise OAuthLoginException('Missing `id` column in returned user information') + + # Perform any custom verification for this login service. + self.service_verify_user_info_for_login(app_config, http_client, token, user_info) + + # Retrieve the user's email address (if necessary). + email_address = self.get_verified_user_email(app_config, http_client, token, user_info) + if features.MAILING and email_address is None: + raise OAuthLoginException('A verified email address is required to login with this service') + + service_user_id = self.get_login_service_id(user_info) + service_username = self.get_login_service_username(user_info) + + logger.debug('Completed successful exchange for service %s: %s, %s, %s', + self.service_id(), service_user_id, service_username, email_address) + return (service_user_id, service_username, email_address) diff --git a/oauth/loginmanager.py b/oauth/loginmanager.py new file mode 100644 index 000000000..6e15ba7d8 --- /dev/null +++ b/oauth/loginmanager.py @@ -0,0 +1,19 @@ +import features + +from oauth.services.github import GithubOAuthService +from oauth.services.google import GoogleOAuthService + +class OAuthLoginManager(object): + """ Helper class which manages all registered OAuth login services. """ + def __init__(self, config): + self.services = [] + + # Register the endpoints for each of the OAuth login services. + # TODO(jschorr): make this dynamic. + if config.get('GITHUB_LOGIN_CONFIG') is not None and features.GITHUB_LOGIN: + github_service = GithubOAuthService(config, 'GITHUB_LOGIN_CONFIG') + self.services.append(github_service) + + if config.get('GOOGLE_LOGIN_CONFIG') is not None and features.GOOGLE_LOGIN: + google_service = GoogleOAuthService(config, 'GOOGLE_LOGIN_CONFIG') + self.services.append(google_service) diff --git a/oauth/oidc.py b/oauth/oidc.py new file mode 100644 index 000000000..161be7418 --- /dev/null +++ b/oauth/oidc.py @@ -0,0 +1,110 @@ +import time +import json +import logging +import urlparse + +from cachetools import lru_cache +from cachetools.ttl import TTLCache + +from util.oauth.base import OAuthService + +logger = logging.getLogger(__name__) + + +def decode_user_jwt(token, oidc_provider): + try: + return decode(token, oidc_provider.get_public_key(), algorithms=['RS256'], + audience=oidc_provider.client_id(), + issuer=oidc_provider.issuer) + except InvalidTokenError: + # Public key may have expired. Try to retrieve an updated public key and use it to decode. + return decode(token, oidc_provider.get_public_key(force_refresh=True), algorithms=['RS256'], + audience=oidc_provider.client_id(), + issuer=oidc_provider.issuer) + +OIDC_WELLKNOWN = ".well-known/openid-configuration" +PUBLIC_KEY_CACHE_TTL = 3600 # 1 hour + +class OIDCConfig(OAuthService): + def __init__(self, config, key_name): + super(OIDCConfig, self).__init__(config, key_name) + + self._public_key_cache = TTLCache(1, PUBLIC_KEY_CACHE_TTL, missing=self._get_public_key) + self._config = config + self._http_client = config['HTTPCLIENT'] + + @lru_cache(maxsize=1) + def _oidc_config(self): + if self.config.get('OIDC_SERVER'): + return self._load_via_discovery(self._config.get('DEBUGGING', False)) + else: + return {} + + def _load_via_discovery(self, is_debugging): + oidc_server = self.config['OIDC_SERVER'] + if not oidc_server.startswith('https://') and not is_debugging: + raise Exception('OIDC server must be accessed over SSL') + + discovery_url = urlparse.urljoin(oidc_server, OIDC_WELLKNOWN) + discovery = self._http_client.get(discovery_url, timeout=5) + + if discovery.status_code / 100 != 2: + raise Exception("Could not load OIDC discovery information") + + try: + return json.loads(discovery.text) + except ValueError: + logger.exception('Could not parse OIDC discovery for url: %s', discovery_url) + raise Exception("Could not parse OIDC discovery information") + + def authorize_endpoint(self): + return self._oidc_config().get('authorization_endpoint', '') + '?' + + def token_endpoint(self): + return self._oidc_config().get('token_endpoint') + + def user_endpoint(self): + return None + + def validate_client_id_and_secret(self, http_client, app_config): + pass + + def get_public_config(self): + return { + 'CLIENT_ID': self.client_id(), + 'AUTHORIZE_ENDPOINT': self.authorize_endpoint(), + 'OIDC': True, + } + + @property + def issuer(self): + return self.config.get('OIDC_ISSUER', self.config['OIDC_SERVER']) + + def get_public_key(self, force_refresh=False): + """ Retrieves the public key for this handler. """ + # If force_refresh is true, we expire all the items in the cache by setting the time to + # the current time + the expiration TTL. + if force_refresh: + self._public_key_cache.expire(time=time.time() + PUBLIC_KEY_CACHE_TTL) + + # Retrieve the public key from the cache. If the cache does not contain the public key, + # it will internally call _get_public_key to retrieve it and then save it. The None is + # a random key chose to be stored in the cache, and could be anything. + return self._public_key_cache[None] + + def _get_public_key(self, _): + """ Retrieves the public key for this handler. """ + keys_url = self._oidc_config()['jwks_uri'] + + keys = KEYS() + keys.load_from_url(keys_url) + + if not list(keys): + raise Exception('No keys provided by OIDC provider') + + rsa_key = list(keys)[0] + rsa_key.deserialize() + + # Reload the key so that we can give a key *instance* to PyJWT to work around its weird parsing + # issues. + return load_der_public_key(rsa_key.key.exportKey('DER'), backend=default_backend()) diff --git a/oauth/services/__init__.py b/oauth/services/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/util/oauth/services.py b/oauth/services/github.py similarity index 54% rename from util/oauth/services.py rename to oauth/services/github.py index d5877fc14..cb4f3977d 100644 --- a/util/oauth/services.py +++ b/oauth/services/github.py @@ -1,13 +1,32 @@ -from util import slash_join -from util.oauth.base import OAuthService +import logging -class GithubOAuthService(OAuthService): +from oauth.login import OAuthLoginService, OAuthLoginException +from util import slash_join + +logger = logging.getLogger(__name__) + +class GithubOAuthService(OAuthLoginService): def __init__(self, config, key_name): super(GithubOAuthService, self).__init__(config, key_name) + def service_id(self): + return 'github' + def service_name(self): + if self.is_enterprise(): + return 'GitHub Enterprise' + return 'GitHub' + def get_icon(self): + return 'fa-github' + + def get_login_scopes(self): + if self.config.get('ORG_RESTRICT'): + return ['user:email', 'read:org'] + + return ['user:email'] + def allowed_organizations(self): if not self.config.get('ORG_RESTRICT', False): return None @@ -25,7 +44,7 @@ class GithubOAuthService(OAuthService): return self.config.get('GITHUB_ENDPOINT', 'https://github.com') def is_enterprise(self): - return self._endpoint().find('.github.com') < 0 + return self._api_endpoint().find('.github.com') < 0 def authorize_endpoint(self): return slash_join(self._endpoint(), '/login/oauth/authorize') + '?' @@ -95,94 +114,63 @@ class GithubOAuthService(OAuthService): 'ORG_RESTRICT': self.config.get('ORG_RESTRICT', False) } + def get_login_service_id(self, user_info): + return user_info['id'] -class GoogleOAuthService(OAuthService): - def __init__(self, config, key_name): - super(GoogleOAuthService, self).__init__(config, key_name) + def get_login_service_username(self, user_info): + return user_info['login'] - def service_name(self): - return 'Google' - - def authorize_endpoint(self): - return 'https://accounts.google.com/o/oauth2/auth?response_type=code&' - - def token_endpoint(self): - return 'https://accounts.google.com/o/oauth2/token' - - def user_endpoint(self): - return 'https://www.googleapis.com/oauth2/v1/userinfo' - - def validate_client_id_and_secret(self, http_client, app_config): - # To verify the Google client ID and secret, we hit the - # https://www.googleapis.com/oauth2/v3/token endpoint with an invalid request. If the client - # ID or secret are invalid, we get returned a 403 Unauthorized. Otherwise, we get returned - # another response code. - url = 'https://www.googleapis.com/oauth2/v3/token' - data = { - 'code': 'fakecode', - 'client_id': self.client_id(), - 'client_secret': self.client_secret(), - 'grant_type': 'authorization_code', - 'redirect_uri': 'http://example.com' + def get_verified_user_email(self, app_config, http_client, token, user_info): + v3_media_type = { + 'Accept': 'application/vnd.github.v3' } - result = http_client.post(url, data=data, timeout=5) - return result.status_code != 401 - - def get_public_config(self): - return { - 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint() + token_param = { + 'access_token': token, } + # Find the e-mail address for the user: we will accept any email, but we prefer the primary + get_email = http_client.get(self.email_endpoint(), params=token_param, headers=v3_media_type) + if get_email.status_code // 100 != 2: + raise OAuthLoginException('Got non-2XX status code for emails endpoint: %s' % + get_email.status_code) -class GitLabOAuthService(OAuthService): - def __init__(self, config, key_name): - super(GitLabOAuthService, self).__init__(config, key_name) + verified_emails = [email for email in get_email.json() if email['verified']] + primary_emails = [email for email in get_email.json() if email['primary']] - def _endpoint(self): - return self.config.get('GITLAB_ENDPOINT', 'https://gitlab.com') + # Special case: We don't care about whether an e-mail address is "verified" under GHE. + if self.is_enterprise() and not verified_emails: + verified_emails = primary_emails - def user_endpoint(self): - raise NotImplementedError + allowed_emails = (primary_emails or verified_emails or []) + return allowed_emails[0]['email'] if len(allowed_emails) > 0 else None - def api_endpoint(self): - return self._endpoint() + def service_verify_user_info_for_login(self, app_config, http_client, token, user_info): + # Retrieve the user's orgnizations (if organization filtering is turned on) + if self.allowed_organizations() is None: + return - def get_public_url(self, suffix): - return slash_join(self._endpoint(), suffix) - - def service_name(self): - return 'GitLab' - - def authorize_endpoint(self): - return slash_join(self._endpoint(), '/oauth/authorize') - - def token_endpoint(self): - return slash_join(self._endpoint(), '/oauth/token') - - def validate_client_id_and_secret(self, http_client, app_config): - url = self.token_endpoint() - redirect_uri = self.get_redirect_uri(app_config, redirect_suffix='trigger') - data = { - 'code': 'fakecode', - 'client_id': self.client_id(), - 'client_secret': self.client_secret(), - 'grant_type': 'authorization_code', - 'redirect_uri': redirect_uri + moondragon_media_type = { + 'Accept': 'application/vnd.github.moondragon+json' } - # We validate by checking the error code we receive from this call. - result = http_client.post(url, data=data, timeout=5) - value = result.json() - if not value: - return False - - return value.get('error', '') != 'invalid_client' - - def get_public_config(self): - return { - 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint(), - 'GITLAB_ENDPOINT': self._endpoint(), + token_param = { + 'access_token': token, } + + get_orgs = http_client.get(self.orgs_endpoint(), params=token_param, + headers=moondragon_media_type) + + if get_orgs.status_code // 100 != 2: + logger.debug('get_orgs response: %s', get_orgs.json()) + raise OAuthLoginException('Got non-2XX response for org lookup: %s' % + get_orgs.status_code) + + organizations = set([org.get('login').lower() for org in get_orgs.json()]) + matching_organizations = organizations & set(self.allowed_organizations()) + if not matching_organizations: + logger.debug('Found organizations %s, but expected one of %s', organizations, + self.allowed_organizations()) + err = """You are not a member of an allowed GitHub organization. + Please contact your system administrator if you believe this is in error.""" + raise OAuthLoginException(err) diff --git a/oauth/services/gitlab.py b/oauth/services/gitlab.py new file mode 100644 index 000000000..195ce177b --- /dev/null +++ b/oauth/services/gitlab.py @@ -0,0 +1,56 @@ +from oauth.base import OAuthService +from util import slash_join + +class GitLabOAuthService(OAuthService): + def __init__(self, config, key_name): + super(GitLabOAuthService, self).__init__(config, key_name) + + def service_id(self): + return 'gitlab' + + def service_name(self): + return 'GitLab' + + def _endpoint(self): + return self.config.get('GITLAB_ENDPOINT', 'https://gitlab.com') + + def user_endpoint(self): + raise NotImplementedError + + def api_endpoint(self): + return self._endpoint() + + def get_public_url(self, suffix): + return slash_join(self._endpoint(), suffix) + + def authorize_endpoint(self): + return slash_join(self._endpoint(), '/oauth/authorize') + + def token_endpoint(self): + return slash_join(self._endpoint(), '/oauth/token') + + def validate_client_id_and_secret(self, http_client, app_config): + url = self.token_endpoint() + redirect_uri = self.get_redirect_uri(app_config, redirect_suffix='trigger') + data = { + 'code': 'fakecode', + 'client_id': self.client_id(), + 'client_secret': self.client_secret(), + 'grant_type': 'authorization_code', + 'redirect_uri': redirect_uri + } + + # We validate by checking the error code we receive from this call. + result = http_client.post(url, data=data, timeout=5) + value = result.json() + if not value: + return False + + return value.get('error', '') != 'invalid_client' + + def get_public_config(self): + return { + 'CLIENT_ID': self.client_id(), + 'AUTHORIZE_ENDPOINT': self.authorize_endpoint(), + 'GITLAB_ENDPOINT': self._endpoint(), + } diff --git a/oauth/services/google.py b/oauth/services/google.py new file mode 100644 index 000000000..19c6d27de --- /dev/null +++ b/oauth/services/google.py @@ -0,0 +1,76 @@ +from oauth.login import OAuthLoginService + +def _get_email_username(email_address): + username = email_address + at = username.find('@') + if at > 0: + username = username[0:at] + + return username + +class GoogleOAuthService(OAuthLoginService): + def __init__(self, config, key_name): + super(GoogleOAuthService, self).__init__(config, key_name) + + def service_id(self): + return 'google' + + def service_name(self): + return 'Google' + + def get_icon(self): + return 'fa-google' + + def get_login_scopes(self): + return ['openid', 'email'] + + def authorize_endpoint(self): + return 'https://accounts.google.com/o/oauth2/auth?response_type=code&' + + def token_endpoint(self): + return 'https://accounts.google.com/o/oauth2/token' + + def user_endpoint(self): + return 'https://www.googleapis.com/oauth2/v1/userinfo' + + def requires_form_encoding(self): + return True + + def validate_client_id_and_secret(self, http_client, app_config): + # To verify the Google client ID and secret, we hit the + # https://www.googleapis.com/oauth2/v3/token endpoint with an invalid request. If the client + # ID or secret are invalid, we get returned a 403 Unauthorized. Otherwise, we get returned + # another response code. + url = 'https://www.googleapis.com/oauth2/v3/token' + data = { + 'code': 'fakecode', + 'client_id': self.client_id(), + 'client_secret': self.client_secret(), + 'grant_type': 'authorization_code', + 'redirect_uri': 'http://example.com' + } + + result = http_client.post(url, data=data, timeout=5) + return result.status_code != 401 + + def get_public_config(self): + return { + 'CLIENT_ID': self.client_id(), + 'AUTHORIZE_ENDPOINT': self.authorize_endpoint() + } + + def get_login_service_id(self, user_info): + return user_info['id'] + + def get_login_service_username(self, user_info): + return _get_email_username(user_info['email']) + + def get_verified_user_email(self, app_config, http_client, token, user_info): + if not user_info.get('verified_email', False): + return None + + return user_info['email'] + + def service_verify_user_info_for_login(self, app_config, http_client, token, user_info): + # Nothing to do. + pass diff --git a/static/directives/external-login-button.html b/static/directives/external-login-button.html index abea9dfda..19a1e1680 100644 --- a/static/directives/external-login-button.html +++ b/static/directives/external-login-button.html @@ -1,14 +1,13 @@ diff --git a/static/directives/external-logins-manager.html b/static/directives/external-logins-manager.html index 648a6c44f..f06c3ecf8 100644 --- a/static/directives/external-logins-manager.html +++ b/static/directives/external-logins-manager.html @@ -14,27 +14,24 @@ - - - {{ provider.title() }} + + {{ provider.title }} - Attached to {{ provider.title() }} account - - - {{ provider.getUserInfo(externalLoginInfo[provider.id]).username }} - + Attached to {{ provider.title }} account + + {{ externalLoginInfo[provider.id].metadata.service_username }} - Not attached to {{ provider.title() }} + Not attached to {{ provider.title }} - Detach Account diff --git a/static/directives/icon-image-view.html b/static/directives/icon-image-view.html new file mode 100644 index 000000000..c213782e4 --- /dev/null +++ b/static/directives/icon-image-view.html @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/static/directives/signin-form.html b/static/directives/signin-form.html index 9b2ac0ff8..455b9128f 100644 --- a/static/directives/signin-form.html +++ b/static/directives/signin-form.html @@ -6,7 +6,7 @@
-
diff --git a/static/js/directives/ui/external-login-button.js b/static/js/directives/ui/external-login-button.js index f8ec07e53..db79c2fee 100644 --- a/static/js/directives/ui/external-login-button.js +++ b/static/js/directives/ui/external-login-button.js @@ -12,12 +12,11 @@ angular.module('quay').directive('externalLoginButton', function () { 'signInStarted': '&signInStarted', 'redirectUrl': '=redirectUrl', 'isLink': '=isLink', - 'provider': '@provider', + 'provider': '=provider', 'action': '@action' }, controller: function($scope, $timeout, $interval, ApiService, KeyService, CookieService, ExternalLoginService) { $scope.signingIn = false; - $scope.providerInfo = ExternalLoginService.getProvider($scope.provider); $scope.startSignin = function() { $scope.signInStarted({'service': $scope.provider}); diff --git a/static/js/directives/ui/external-logins-manager.js b/static/js/directives/ui/external-logins-manager.js index 4706b4e88..480327686 100644 --- a/static/js/directives/ui/external-logins-manager.js +++ b/static/js/directives/ui/external-logins-manager.js @@ -34,11 +34,11 @@ angular.module('quay').directive('externalLoginsManager', function () { } }); - $scope.detachExternalLogin = function(kind) { + $scope.detachExternalLogin = function(service_id) { if (!Features.DIRECT_LOGIN) { return; } var params = { - 'servicename': kind + 'service_id': service_id }; ApiService.detachExternalLogin(null, params).then(function() { diff --git a/static/js/directives/ui/icon-image-view.js b/static/js/directives/ui/icon-image-view.js new file mode 100644 index 000000000..02fccd046 --- /dev/null +++ b/static/js/directives/ui/icon-image-view.js @@ -0,0 +1,18 @@ +/** + * An element which displays either an icon or an image, depending on the value. + */ +angular.module('quay').directive('iconImageView', function () { + var directiveDefinitionObject = { + priority: 0, + templateUrl: '/static/directives/icon-image-view.html', + replace: false, + transclude: true, + restrict: 'C', + scope: { + 'value': '@value' + }, + controller: function($scope, $element) { + } + }; + return directiveDefinitionObject; +}); \ No newline at end of file diff --git a/static/js/services/external-login-service.js b/static/js/services/external-login-service.js index 7d6061df6..1b4b4f24e 100644 --- a/static/js/services/external-login-service.js +++ b/static/js/services/external-login-service.js @@ -1,19 +1,18 @@ /** * Service which exposes the supported external logins. */ -angular.module('quay').factory('ExternalLoginService', ['KeyService', 'Features', 'Config', - function(KeyService, Features, Config) { +angular.module('quay').factory('ExternalLoginService', ['Features', 'Config', + function(Features, Config) { var externalLoginService = {}; - externalLoginService.getLoginUrl = function(service, action) { - var serviceInfo = externalLoginService.getProvider(service); - if (!serviceInfo) { return ''; } + externalLoginService.EXTERNAL_LOGINS = window.__external_login; - var loginUrl = KeyService.getConfiguration(serviceInfo.key, 'AUTHORIZE_ENDPOINT'); - var clientId = KeyService.getConfiguration(serviceInfo.key, 'CLIENT_ID'); + externalLoginService.getLoginUrl = function(loginService, action) { + var loginUrl = loginService['config']['AUTHORIZE_ENDPOINT']; + var clientId = loginService['config']['CLIENT_ID']; - var scope = serviceInfo.scopes(); - var redirectUri = Config.getUrl('/oauth2/' + service + '/callback'); + var scope = loginService.scopes.join(' '); + var redirectUri = Config.getUrl('/oauth2/' + loginService['id'] + '/callback'); if (action == 'attach') { redirectUri += '/attach'; @@ -24,96 +23,6 @@ angular.module('quay').factory('ExternalLoginService', ['KeyService', 'Features' return url; }; - var DEX = { - id: 'dex', - key: 'DEX_LOGIN_CONFIG', - - title: function() { - return KeyService.getConfiguration('DEX_LOGIN_CONFIG', 'OIDC_TITLE'); - }, - - icon: function() { - return {'url': KeyService.getConfiguration('DEX_LOGIN_CONFIG', 'OIDC_LOGO') }; - }, - - scopes: function() { - return 'openid email profile' - }, - - enabled: Features.DEX_LOGIN - }; - - var GITHUB = { - id: 'github', - key: 'GITHUB_LOGIN_CONFIG', - - title: function() { - return KeyService.isEnterprise('github') ? 'GitHub Enterprise' : 'GitHub'; - }, - - icon: function() { - return {'icon': 'fa-github'}; - }, - - hasUserInfo: true, - getUserInfo: function(service_info) { - username = service_info['metadata']['service_username']; - return { - 'username': username, - 'endpoint': KeyService['githubEndpoint'] + username - } - }, - - scopes: function() { - var scopes = 'user:email'; - if (KeyService.getConfiguration('GITHUB_LOGIN_CONFIG', 'ORG_RESTRICT')) { - scopes += ' read:org'; - } - - return scopes; - }, - - enabled: Features.GITHUB_LOGIN - }; - - var GOOGLE = { - id: 'google', - key: 'GOOGLE_LOGIN_CONFIG', - - title: function() { - return 'Google'; - }, - - icon: function() { - return {'icon': 'fa-google'}; - }, - - scopes: function() { - return 'openid email'; - }, - - enabled: Features.GOOGLE_LOGIN - }; - - externalLoginService.ALL_EXTERNAL_LOGINS = [ - DEX, GITHUB, GOOGLE - ]; - - externalLoginService.EXTERNAL_LOGINS = externalLoginService.ALL_EXTERNAL_LOGINS.filter(function(el) { - return el.enabled; - }); - - externalLoginService.getProvider = function(providerId) { - for (var i = 0; i < externalLoginService.EXTERNAL_LOGINS.length; ++i) { - var current = externalLoginService.EXTERNAL_LOGINS[i]; - if (current.id == providerId) { - return current; - } - } - - return null; - }; - externalLoginService.hasSingleSignin = function() { return externalLoginService.EXTERNAL_LOGINS.length == 1 && !Features.DIRECT_LOGIN; }; @@ -122,7 +31,7 @@ angular.module('quay').factory('ExternalLoginService', ['KeyService', 'Features' // 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].id); + return externalLoginService.getLoginUrl(externalLoginService.EXTERNAL_LOGINS[0]); } return null; diff --git a/templates/base.html b/templates/base.html index da1baddf3..f9d704af9 100644 --- a/templates/base.html +++ b/templates/base.html @@ -39,6 +39,7 @@ window.__features = {{ feature_set|tojson|safe }}; window.__config = {{ config_set|tojson|safe }}; window.__oauth = {{ oauth_set|tojson|safe }}; + window.__external_login = {{ external_login_set|tojson|safe }}; window.__auth_scopes = {{ scope_set|tojson|safe }}; window.__vuln_priority = {{ vuln_priority_set|tojson|safe }} window.__token = '{{ csrf_token() }}'; diff --git a/test/test_api_security.py b/test/test_api_security.py index a23ff75ed..7cb0d21b8 100644 --- a/test/test_api_security.py +++ b/test/test_api_security.py @@ -507,7 +507,7 @@ class TestSignin(ApiTestCase): class TestDetachExternal(ApiTestCase): def setUp(self): ApiTestCase.setUp(self) - self._set_url(DetachExternal, servicename='someservice') + self._set_url(DetachExternal, service_id='someservice') def test_post_anonymous(self): self._run_test('POST', 401, None, {}) diff --git a/test/test_endpoints.py b/test/test_endpoints.py index bf12038d3..216021245 100644 --- a/test/test_endpoints.py +++ b/test/test_endpoints.py @@ -16,7 +16,7 @@ from Crypto.PublicKey import RSA from flask import url_for from jwkest.jwk import RSAKey -from app import app +from app import app, oauth_login from data import model from data.database import ServiceKeyApprovalType from endpoints import keyserver @@ -30,7 +30,7 @@ from initdb import setup_database_for_testing, finished_database_for_testing from test.helpers import assert_action_logged try: - app.register_blueprint(oauthlogin_bp, url_prefix='/oauth') + app.register_blueprint(oauthlogin_bp, url_prefix='/oauth2') except ValueError: # This blueprint was already registered pass diff --git a/test/testconfig.py b/test/testconfig.py index ef395613f..eb976aa65 100644 --- a/test/testconfig.py +++ b/test/testconfig.py @@ -76,6 +76,9 @@ class TestConfig(DefaultConfig): PROMETHEUS_AGGREGATOR_URL = None + GITHUB_LOGIN_CONFIG = {} + GOOGLE_LOGIN_CONFIG = {} + FEATURE_GITHUB_LOGIN = True FEATURE_GOOGLE_LOGIN = True FEATURE_DEX_LOGIN = True diff --git a/util/oauth/base.py b/util/oauth/base.py deleted file mode 100644 index 9ede7cfdc..000000000 --- a/util/oauth/base.py +++ /dev/null @@ -1,63 +0,0 @@ -class OAuthService(object): - """ A base class for defining an external service, exposed via OAuth. """ - def __init__(self, config, key_name): - self.key_name = key_name - self.config = config.get(key_name) or {} - - def service_name(self): - raise NotImplementedError - - def token_endpoint(self): - raise NotImplementedError - - def user_endpoint(self): - raise NotImplementedError - - def validate_client_id_and_secret(self, http_client, app_config): - raise NotImplementedError - - def client_id(self): - return self.config.get('CLIENT_ID') - - def client_secret(self): - return self.config.get('CLIENT_SECRET') - - 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'], - self.service_name().lower(), - redirect_suffix) - - def exchange_code_for_token(self, app_config, http_client, code, form_encode=False, - redirect_suffix='', client_auth=False): - payload = { - 'code': code, - 'grant_type': 'authorization_code', - 'redirect_uri': self.get_redirect_uri(app_config, redirect_suffix) - } - - headers = { - 'Accept': 'application/json' - } - - auth = None - if client_auth: - auth = (self.client_id(), self.client_secret()) - else: - payload['client_id'] = self.client_id() - payload['client_secret'] = self.client_secret() - - token_url = self.token_endpoint() - if form_encode: - get_access_token = http_client.post(token_url, data=payload, headers=headers, auth=auth) - else: - get_access_token = http_client.post(token_url, params=payload, headers=headers, auth=auth) - - if get_access_token.status_code / 100 != 2: - return None - - json_data = get_access_token.json() - if not json_data: - return None - - return json_data.get('access_token', None) From fda203e4d75305b9ed356554bdf37a8ad1e244d8 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 23 Jan 2017 17:53:34 -0500 Subject: [PATCH 04/13] Add proper and tested OIDC support on the server Note that this will still not work on the client side; the followup CL for the client side is right after this one. --- data/model/user.py | 16 +- endpoints/common.py | 1 - endpoints/oauthlogin.py | 8 + external_libraries.py | 2 +- oauth/base.py | 11 +- oauth/login.py | 4 + oauth/loginmanager.py | 25 +-- oauth/oidc.py | 244 +++++++++++++++++++++------- oauth/services/github.py | 3 + oauth/services/google.py | 3 + oauth/test/test_loginmanager.py | 62 ++++++++ oauth/test/test_oidc.py | 273 ++++++++++++++++++++++++++++++++ test/test_endpoints.py | 100 +----------- test/test_oauth_login.py | 175 ++++++++++++++++++++ test/testconfig.py | 9 +- 15 files changed, 756 insertions(+), 180 deletions(-) create mode 100644 oauth/test/test_loginmanager.py create mode 100644 oauth/test/test_oidc.py create mode 100644 test/test_oauth_login.py diff --git a/data/model/user.py b/data/model/user.py index ddb27400d..87e239d73 100644 --- a/data/model/user.py +++ b/data/model/user.py @@ -368,7 +368,7 @@ def update_user_metadata(user, given_name=None, family_name=None, company=None): remove_user_prompt(user, UserPromptTypes.ENTER_COMPANY) -def create_federated_user(username, email, service_name, service_ident, +def create_federated_user(username, email, service_id, service_ident, set_password_notification, metadata={}, email_required=True, prompts=tuple()): prompts = set(prompts) @@ -378,7 +378,11 @@ def create_federated_user(username, email, service_name, service_ident, new_user.verified = True new_user.save() - service = LoginService.get(LoginService.name == service_name) + try: + service = LoginService.get(LoginService.name == service_id) + except LoginService.DoesNotExist: + service = LoginService.create(name=service_id) + FederatedLogin.create(user=new_user, service=service, service_ident=service_ident, metadata_json=json.dumps(metadata)) @@ -389,20 +393,20 @@ def create_federated_user(username, email, service_name, service_ident, return new_user -def attach_federated_login(user, service_name, service_ident, metadata={}): - service = LoginService.get(LoginService.name == service_name) +def attach_federated_login(user, service_id, service_ident, metadata={}): + service = LoginService.get(LoginService.name == service_id) FederatedLogin.create(user=user, service=service, service_ident=service_ident, metadata_json=json.dumps(metadata)) return user -def verify_federated_login(service_name, service_ident): +def verify_federated_login(service_id, service_ident): try: found = (FederatedLogin .select(FederatedLogin, User) .join(LoginService) .switch(FederatedLogin).join(User) - .where(FederatedLogin.service_ident == service_ident, LoginService.name == service_name) + .where(FederatedLogin.service_ident == service_ident, LoginService.name == service_id) .get()) return found.user except FederatedLogin.DoesNotExist: diff --git a/endpoints/common.py b/endpoints/common.py index fd52ee868..30fd11947 100644 --- a/endpoints/common.py +++ b/endpoints/common.py @@ -197,7 +197,6 @@ def render_page_template(name, route_data=None, **kwargs): 'title': login_service.service_name(), 'config': login_service.get_public_config(), 'icon': login_service.get_icon(), - 'scopes': login_service.get_login_scopes(), }) return login_config diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index 0c7865213..f1bffc064 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -1,4 +1,5 @@ import logging +import uuid from flask import request, redirect, url_for, Blueprint from peewee import IntegrityError @@ -50,6 +51,7 @@ def _conduct_oauth_login(service_id, service_name, user_id, username, email, met # Try to create the user try: + # Generate a valid username. new_username = None for valid in generate_valid_usernames(username): if model.user.get_user_or_org(valid): @@ -58,6 +60,11 @@ def _conduct_oauth_login(service_id, service_name, user_id, username, email, met new_username = valid break + # Generate a valid email. If the email is None and the MAILING feature is turned + # off, simply place in a fake email address. + if email is None and not features.MAILING: + email = '%s@fake.example.com' % (str(uuid.uuid4())) + prompts = model.user.get_default_user_prompts(features) to_login = model.user.create_federated_user(new_username, email, service_id, user_id, set_password_notification=True, @@ -102,6 +109,7 @@ def _register_service(login_service): try: lid, lusername, lemail = login_service.exchange_code_for_login(app.config, client, code, '') except OAuthLoginException as ole: + logger.exception('Got login exception') return _render_ologin_error(login_service.service_name(), ole.message) # Conduct login. diff --git a/external_libraries.py b/external_libraries.py index d99431283..f2f9c3832 100644 --- a/external_libraries.py +++ b/external_libraries.py @@ -22,7 +22,7 @@ EXTERNAL_JS = [ ] EXTERNAL_CSS = [ - 'netdna.bootstrapcdn.com/font-awesome/4.6.0/css/font-awesome.css', + 'netdna.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.css', 'netdna.bootstrapcdn.com/bootstrap/3.3.2/css/bootstrap.min.css', 'fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,700', 's3.amazonaws.com/cdn.core-os.net/icons/core-icons.css', diff --git a/oauth/base.py b/oauth/base.py index f25bbc6ab..93b1c8a3c 100644 --- a/oauth/base.py +++ b/oauth/base.py @@ -51,7 +51,7 @@ class OAuthService(object): 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'], - self.service_name().lower(), + self.service_id(), redirect_suffix) def get_user_info(self, http_client, token): @@ -74,8 +74,8 @@ class OAuthService(object): def exchange_code_for_token(self, app_config, http_client, code, form_encode=False, redirect_suffix='', client_auth=False): """ Exchanges an OAuth access code for the associated OAuth token. """ - json_data = self._exchange_code(app_config, http_client, code, form_encode, redirect_suffix, - client_auth) + json_data = self.exchange_code(app_config, http_client, code, form_encode, redirect_suffix, + client_auth) access_token = json_data.get('access_token', None) if access_token is None: @@ -84,8 +84,9 @@ class OAuthService(object): return access_token - def _exchange_code(self, app_config, http_client, code, form_encode=False, redirect_suffix='', - client_auth=False): + def exchange_code(self, app_config, http_client, code, form_encode=False, redirect_suffix='', + client_auth=False): + """ Exchanges an OAuth access code for associated OAuth token and other data. """ payload = { 'code': code, 'grant_type': 'authorization_code', diff --git a/oauth/login.py b/oauth/login.py index a4aa5524e..268d030f7 100644 --- a/oauth/login.py +++ b/oauth/login.py @@ -14,6 +14,10 @@ class OAuthLoginService(OAuthService): """ A base class for defining an OAuth-compliant service that can be used for, amongst other things, login and authentication. """ + def login_enabled(self): + """ Returns true if the login service is enabled. """ + raise NotImplementedError + def get_login_service_id(self, user_info): """ Returns the internal ID for the given user under this login service. """ raise NotImplementedError diff --git a/oauth/loginmanager.py b/oauth/loginmanager.py index 6e15ba7d8..2443d20dd 100644 --- a/oauth/loginmanager.py +++ b/oauth/loginmanager.py @@ -1,7 +1,11 @@ -import features - from oauth.services.github import GithubOAuthService from oauth.services.google import GoogleOAuthService +from oauth.oidc import OIDCLoginService + +CUSTOM_LOGIN_SERVICES = { + 'GITHUB_LOGIN_CONFIG': GithubOAuthService, + 'GOOGLE_LOGIN_CONFIG': GoogleOAuthService, +} class OAuthLoginManager(object): """ Helper class which manages all registered OAuth login services. """ @@ -9,11 +13,12 @@ class OAuthLoginManager(object): self.services = [] # Register the endpoints for each of the OAuth login services. - # TODO(jschorr): make this dynamic. - if config.get('GITHUB_LOGIN_CONFIG') is not None and features.GITHUB_LOGIN: - github_service = GithubOAuthService(config, 'GITHUB_LOGIN_CONFIG') - self.services.append(github_service) - - if config.get('GOOGLE_LOGIN_CONFIG') is not None and features.GOOGLE_LOGIN: - google_service = GoogleOAuthService(config, 'GOOGLE_LOGIN_CONFIG') - self.services.append(google_service) + for key in config.keys(): + # All keys which end in _LOGIN_CONFIG setup a login service. + if key.endswith('_LOGIN_CONFIG'): + if key in CUSTOM_LOGIN_SERVICES: + custom_service = CUSTOM_LOGIN_SERVICES[key](config, key) + if custom_service.login_enabled(config): + self.services.append(custom_service) + else: + self.services.append(OIDCLoginService(config, key)) diff --git a/oauth/oidc.py b/oauth/oidc.py index 161be7418..b51668a06 100644 --- a/oauth/oidc.py +++ b/oauth/oidc.py @@ -3,108 +3,244 @@ import json import logging import urlparse +import jwt + from cachetools import lru_cache from cachetools.ttl import TTLCache +from cryptography.hazmat.backends import default_backend +from cryptography.hazmat.primitives.serialization import load_der_public_key +from jwkest.jwk import KEYS -from util.oauth.base import OAuthService +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__) -def decode_user_jwt(token, oidc_provider): - try: - return decode(token, oidc_provider.get_public_key(), algorithms=['RS256'], - audience=oidc_provider.client_id(), - issuer=oidc_provider.issuer) - except InvalidTokenError: - # Public key may have expired. Try to retrieve an updated public key and use it to decode. - return decode(token, oidc_provider.get_public_key(force_refresh=True), algorithms=['RS256'], - audience=oidc_provider.client_id(), - issuer=oidc_provider.issuer) - OIDC_WELLKNOWN = ".well-known/openid-configuration" PUBLIC_KEY_CACHE_TTL = 3600 # 1 hour +ALLOWED_ALGORITHMS = ['RS256'] +JWT_CLOCK_SKEW_SECONDS = 30 -class OIDCConfig(OAuthService): +class DiscoveryFailureException(Exception): + """ Exception raised when OIDC discovery fails. """ + pass + + +class PublicKeyLoadException(Exception): + """ Exception raised if loading the OIDC public key fails. """ + pass + + +class OIDCLoginService(OAuthService): + """ Defines a generic service for all OpenID-connect compatible login services. """ def __init__(self, config, key_name): - super(OIDCConfig, self).__init__(config, key_name) + super(OIDCLoginService, self).__init__(config, key_name) - self._public_key_cache = TTLCache(1, PUBLIC_KEY_CACHE_TTL, missing=self._get_public_key) - self._config = config + self._public_key_cache = TTLCache(1, PUBLIC_KEY_CACHE_TTL, missing=self._load_public_key) + self._id = key_name[0:key_name.find('_')].lower() self._http_client = config['HTTPCLIENT'] + self._mailing = config.get('FEATURE_MAILING', False) - @lru_cache(maxsize=1) - def _oidc_config(self): - if self.config.get('OIDC_SERVER'): - return self._load_via_discovery(self._config.get('DEBUGGING', False)) - else: - return {} + def service_id(self): + return self._id - def _load_via_discovery(self, is_debugging): - oidc_server = self.config['OIDC_SERVER'] - if not oidc_server.startswith('https://') and not is_debugging: - raise Exception('OIDC server must be accessed over SSL') + def service_name(self): + return self.config.get('SERVICE_NAME', self.service_id()) - discovery_url = urlparse.urljoin(oidc_server, OIDC_WELLKNOWN) - discovery = self._http_client.get(discovery_url, timeout=5) + def get_icon(self): + return self.config.get('SERVICE_ICON', 'fa-user-circle') - if discovery.status_code / 100 != 2: - raise Exception("Could not load OIDC discovery information") + def get_login_scopes(self): + default_scopes = ['openid'] - try: - return json.loads(discovery.text) - except ValueError: - logger.exception('Could not parse OIDC discovery for url: %s', discovery_url) - raise Exception("Could not parse OIDC discovery information") + if self.user_endpoint() is not None: + default_scopes.append('profile') + + if self._mailing: + default_scopes.append('email') + + return self._oidc_config().get('scopes_supported', default_scopes) def authorize_endpoint(self): - return self._oidc_config().get('authorization_endpoint', '') + '?' + return self._oidc_config().get('authorization_endpoint', '') + '?response_type=code&' def token_endpoint(self): return self._oidc_config().get('token_endpoint') def user_endpoint(self): - return None + return self._oidc_config().get('userinfo_endpoint') def validate_client_id_and_secret(self, http_client, app_config): - pass + # 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) + if check_auth_url.status_code // 100 != 2: + raise Exception('Got non-200 status code for authorization endpoint') + + def requires_form_encoding(self): + return True def get_public_config(self): return { 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint(), 'OIDC': True, } + def exchange_code_for_login(self, app_config, http_client, code, redirect_suffix): + # Exchange the code for the access token and id_token + try: + json_data = self.exchange_code(app_config, http_client, code, + redirect_suffix=redirect_suffix, + form_encode=self.requires_form_encoding()) + except OAuthExchangeCodeException as oce: + raise OAuthLoginException(oce.message) + + # Make sure we received both. + access_token = json_data.get('access_token', None) + if access_token is None: + logger.debug('Missing access_token in response: %s', json_data) + raise OAuthLoginException('Missing `access_token` in OIDC response') + + id_token = json_data.get('id_token', None) + if id_token is None: + logger.debug('Missing id_token in response: %s', json_data) + raise OAuthLoginException('Missing `id_token` in OIDC response') + + # Decode the id_token. + try: + decoded_id_token = self._decode_user_jwt(id_token) + except InvalidTokenError as ite: + logger.exception('Got invalid token error on OIDC decode: %s', ite.message) + raise OAuthLoginException('Could not decode OIDC token') + except PublicKeyLoadException as pke: + logger.exception('Could not load public key during OIDC decode: %s', pke.message) + raise OAuthLoginException('Could find public OIDC key') + + # Retrieve the user information. + try: + user_info = self.get_user_info(http_client, access_token) + except OAuthGetUserInfoException as oge: + raise OAuthLoginException(oge.message) + + # Verify subs. + if user_info['sub'] != decoded_id_token['sub']: + raise OAuthLoginException('Mismatch in `sub` returned by OIDC user info endpoint') + + # Check if we have a verified email address. + email_address = user_info.get('email') if user_info.get('email_verified') else None + if self._mailing: + if email_address is None: + raise OAuthLoginException('A verified email address is required to login with this service') + + # Check for a preferred username. + lusername = user_info.get('preferred_username') or user_info.get('sub') + return decoded_id_token['sub'], lusername, email_address + @property - def issuer(self): + def _issuer(self): return self.config.get('OIDC_ISSUER', self.config['OIDC_SERVER']) - def get_public_key(self, force_refresh=False): - """ Retrieves the public key for this handler. """ + @lru_cache(maxsize=1) + def _oidc_config(self): + if self.config.get('OIDC_SERVER'): + return self._load_oidc_config_via_discovery(self.config.get('DEBUGGING', False)) + else: + return {} + + def _load_oidc_config_via_discovery(self, is_debugging): + """ Attempts to load the OIDC config via the OIDC discovery mechanism. If is_debugging is True, + non-secure connections are alllowed. Raises an DiscoveryFailureException on failure. + """ + oidc_server = self.config['OIDC_SERVER'] + if not oidc_server.startswith('https://') and not is_debugging: + raise DiscoveryFailureException('OIDC server must be accessed over SSL') + + discovery_url = urlparse.urljoin(oidc_server, OIDC_WELLKNOWN) + discovery = self._http_client.get(discovery_url, timeout=5, verify=not is_debugging) + if discovery.status_code // 100 != 2: + logger.debug('Got %s response for OIDC discovery: %s', discovery.status_code, discovery.text) + raise DiscoveryFailureException("Could not load OIDC discovery information") + + try: + return json.loads(discovery.text) + except ValueError: + logger.exception('Could not parse OIDC discovery for url: %s', discovery_url) + raise DiscoveryFailureException("Could not parse OIDC discovery information") + + def _decode_user_jwt(self, token): + """ Decodes the given JWT under the given provider and returns it. Raises an InvalidTokenError + exception on an invalid token or a PublicKeyLoadException if the public key could not be + loaded for decoding. + """ + # Find the key to use. + headers = jwt.get_unverified_header(token) + kid = headers.get('kid', None) + if kid is None: + raise InvalidTokenError('Missing `kid` header') + + try: + return decode(token, self._get_public_key(kid), algorithms=ALLOWED_ALGORITHMS, + audience=self.client_id(), + issuer=self._issuer, + leeway=JWT_CLOCK_SKEW_SECONDS, + options=dict(require_nbf=False)) + except InvalidTokenError: + # Public key may have expired. Try to retrieve an updated public key and use it to decode. + return decode(token, self._get_public_key(kid, force_refresh=True), + algorithms=ALLOWED_ALGORITHMS, + audience=self.client_id(), + issuer=self._issuer, + leeway=JWT_CLOCK_SKEW_SECONDS, + options=dict(require_nbf=False)) + + def _get_public_key(self, kid, force_refresh=False): + """ Retrieves the public key for this handler with the given kid. Raises a + PublicKeyLoadException on failure. """ + # If force_refresh is true, we expire all the items in the cache by setting the time to # the current time + the expiration TTL. if force_refresh: self._public_key_cache.expire(time=time.time() + PUBLIC_KEY_CACHE_TTL) # Retrieve the public key from the cache. If the cache does not contain the public key, - # it will internally call _get_public_key to retrieve it and then save it. The None is - # a random key chose to be stored in the cache, and could be anything. - return self._public_key_cache[None] + # it will internally call _load_public_key to retrieve it and then save it. + return self._public_key_cache[kid] - def _get_public_key(self, _): - """ Retrieves the public key for this handler. """ + def _load_public_key(self, kid): + """ Loads the public key for this handler from the OIDC service. Raises PublicKeyLoadException + on failure. + """ keys_url = self._oidc_config()['jwks_uri'] - keys = KEYS() - keys.load_from_url(keys_url) + # Load the keys. + try: + keys = KEYS() + keys.load_from_url(keys_url, verify=not self.config.get('DEBUGGING', False)) + except Exception as ex: + logger.exception('Exception loading public key') + raise PublicKeyLoadException(ex.message) - if not list(keys): - raise Exception('No keys provided by OIDC provider') + # Find the matching key. + keys_found = keys.by_kid(kid) + if len(keys_found) == 0: + raise PublicKeyLoadException('Public key %s not found' % kid) - rsa_key = list(keys)[0] - rsa_key.deserialize() + rsa_keys = [key for key in keys_found if key.kty == 'RSA'] + if len(rsa_keys) == 0: + raise PublicKeyLoadException('No RSA form of public key %s not found' % kid) + + matching_key = rsa_keys[0] + matching_key.deserialize() # Reload the key so that we can give a key *instance* to PyJWT to work around its weird parsing # issues. - return load_der_public_key(rsa_key.key.exportKey('DER'), backend=default_backend()) + return load_der_public_key(matching_key.key.exportKey('DER'), backend=default_backend()) diff --git a/oauth/services/github.py b/oauth/services/github.py index cb4f3977d..da25775f7 100644 --- a/oauth/services/github.py +++ b/oauth/services/github.py @@ -9,6 +9,9 @@ class GithubOAuthService(OAuthLoginService): def __init__(self, config, key_name): super(GithubOAuthService, self).__init__(config, key_name) + def login_enabled(self, config): + return config.get('FEATURE_GITHUB_LOGIN', False) + def service_id(self): return 'github' diff --git a/oauth/services/google.py b/oauth/services/google.py index 19c6d27de..aaedfbac0 100644 --- a/oauth/services/google.py +++ b/oauth/services/google.py @@ -12,6 +12,9 @@ class GoogleOAuthService(OAuthLoginService): def __init__(self, config, key_name): super(GoogleOAuthService, self).__init__(config, key_name) + def login_enabled(self, config): + return config.get('FEATURE_GOOGLE_LOGIN', False) + def service_id(self): return 'google' diff --git a/oauth/test/test_loginmanager.py b/oauth/test/test_loginmanager.py new file mode 100644 index 000000000..491216104 --- /dev/null +++ b/oauth/test/test_loginmanager.py @@ -0,0 +1,62 @@ +from oauth.loginmanager import OAuthLoginManager +from oauth.services.github import GithubOAuthService +from oauth.services.google import GoogleOAuthService +from oauth.oidc import OIDCLoginService + +def test_login_manager_github(): + config = { + 'FEATURE_GITHUB_LOGIN': True, + 'GITHUB_LOGIN_CONFIG': {}, + } + + loginmanager = OAuthLoginManager(config) + assert len(loginmanager.services) == 1 + assert isinstance(loginmanager.services[0], GithubOAuthService) + +def test_github_disabled(): + config = { + 'GITHUB_LOGIN_CONFIG': {}, + } + + loginmanager = OAuthLoginManager(config) + assert len(loginmanager.services) == 0 + +def test_login_manager_google(): + config = { + 'FEATURE_GOOGLE_LOGIN': True, + 'GOOGLE_LOGIN_CONFIG': {}, + } + + loginmanager = OAuthLoginManager(config) + assert len(loginmanager.services) == 1 + assert isinstance(loginmanager.services[0], GoogleOAuthService) + +def test_google_disabled(): + config = { + 'GOOGLE_LOGIN_CONFIG': {}, + } + + loginmanager = OAuthLoginManager(config) + assert len(loginmanager.services) == 0 + +def test_oidc(): + config = { + 'SOMECOOL_LOGIN_CONFIG': {}, + 'HTTPCLIENT': None, + } + + loginmanager = OAuthLoginManager(config) + assert len(loginmanager.services) == 1 + assert isinstance(loginmanager.services[0], OIDCLoginService) + +def test_multiple_oidc(): + config = { + 'SOMECOOL_LOGIN_CONFIG': {}, + 'ANOTHER_LOGIN_CONFIG': {}, + 'HTTPCLIENT': None, + } + + loginmanager = OAuthLoginManager(config) + assert len(loginmanager.services) == 2 + assert isinstance(loginmanager.services[0], OIDCLoginService) + assert isinstance(loginmanager.services[1], OIDCLoginService) diff --git a/oauth/test/test_oidc.py b/oauth/test/test_oidc.py new file mode 100644 index 000000000..cf9612136 --- /dev/null +++ b/oauth/test/test_oidc.py @@ -0,0 +1,273 @@ +# pylint: disable=redefined-outer-name, unused-argument, C0103, C0111, too-many-arguments + +import json +import time +import urlparse + +import jwt +import pytest +import requests + +from httmock import urlmatch, HTTMock +from Crypto.PublicKey import RSA +from jwkest.jwk import RSAKey + +from oauth.oidc import OIDCLoginService, OAuthLoginException + +@pytest.fixture() +def http_client(): + sess = requests.Session() + adapter = requests.adapters.HTTPAdapter(pool_connections=100, + pool_maxsize=100) + sess.mount('http://', adapter) + sess.mount('https://', adapter) + return sess + +@pytest.fixture(params=[True, False]) +def app_config(http_client, request): + return { + 'PREFERRED_URL_SCHEME': 'http', + 'SERVER_HOSTNAME': 'localhost', + 'FEATURE_MAILING': request.param, + + 'SOMEOIDC_TEST_SERVICE': { + 'CLIENT_ID': 'foo', + 'CLIENT_SECRET': 'bar', + 'SERVICE_NAME': 'Some Cool Service', + 'SERVICE_ICON': 'http://some/icon', + 'OIDC_SERVER': 'http://fakeoidc', + 'DEBUGGING': True, + }, + + 'HTTPCLIENT': http_client, + } + +@pytest.fixture() +def oidc_service(app_config): + return OIDCLoginService(app_config, 'SOMEOIDC_TEST_SERVICE') + +@pytest.fixture() +def discovery_content(): + return { + 'scopes_supported': ['profile'], + 'authorization_endpoint': 'http://fakeoidc/authorize', + 'token_endpoint': 'http://fakeoidc/token', + 'userinfo_endpoint': 'http://fakeoidc/userinfo', + 'jwks_uri': 'http://fakeoidc/jwks', + } + +@pytest.fixture() +def discovery_handler(discovery_content): + @urlmatch(netloc=r'fakeoidc', path=r'.+openid.+') + def handler(_, __): + return json.dumps(discovery_content) + + return handler + +@pytest.fixture(scope="module") # Slow to generate, only do it once. +def signing_key(): + private_key = RSA.generate(2048) + jwk = RSAKey(key=private_key.publickey()).serialize() + return { + 'id': 'somekey', + 'private_key': private_key.exportKey('PEM'), + 'jwk': jwk, + } + +@pytest.fixture() +def id_token(oidc_service, signing_key, app_config): + token_data = { + 'iss': oidc_service.config['OIDC_SERVER'], + 'aud': oidc_service.client_id(), + 'nbf': int(time.time()), + 'iat': int(time.time()), + 'exp': int(time.time() + 600), + 'sub': 'cooluser', + } + + token_headers = { + 'kid': signing_key['id'], + } + + return jwt.encode(token_data, signing_key['private_key'], 'RS256', headers=token_headers) + +@pytest.fixture() +def valid_code(): + return 'validcode' + +@pytest.fixture() +def token_handler(oidc_service, id_token, valid_code): + @urlmatch(netloc=r'fakeoidc', path=r'/token') + def handler(_, request): + params = urlparse.parse_qs(request.body) + if params.get('redirect_uri')[0] != 'http://localhost/oauth2/someoidc/callback': + return {'status_code': 400, 'content': 'Invalid redirect URI'} + + if params.get('client_id')[0] != oidc_service.client_id(): + return {'status_code': 401, 'content': 'Invalid client id'} + + if params.get('client_secret')[0] != oidc_service.client_secret(): + return {'status_code': 401, 'content': 'Invalid client secret'} + + if params.get('code')[0] != valid_code: + return {'status_code': 401, 'content': 'Invalid code'} + + if params.get('grant_type')[0] != 'authorization_code': + return {'status_code': 400, 'content': 'Invalid authorization type'} + + content = { + 'access_token': 'sometoken', + 'id_token': id_token, + } + return {'status_code': 200, 'content': json.dumps(content)} + + return handler + +@pytest.fixture() +def jwks_handler(signing_key): + def jwk_with_kid(kid, jwk): + jwk = jwk.copy() + jwk.update({'kid': kid}) + return jwk + + @urlmatch(netloc=r'fakeoidc', path=r'/jwks') + def handler(_, __): + content = {'keys': [jwk_with_kid(signing_key['id'], signing_key['jwk'])]} + return {'status_code': 200, 'content': json.dumps(content)} + + return handler + +@pytest.fixture() +def emptykeys_jwks_handler(): + @urlmatch(netloc=r'fakeoidc', path=r'/jwks') + def handler(_, __): + content = {'keys': []} + return {'status_code': 200, 'content': json.dumps(content)} + + return handler + +@pytest.fixture(params=["someusername", None]) +def preferred_username(request): + return request.param + +@pytest.fixture +def userinfo_handler(oidc_service, preferred_username): + @urlmatch(netloc=r'fakeoidc', path=r'/userinfo') + def handler(_, __): + content = { + 'sub': 'cooluser', + 'preferred_username':preferred_username, + 'email': 'foo@example.com', + 'email_verified': True, + } + + return {'status_code': 200, 'content': json.dumps(content)} + + return handler + +@pytest.fixture() +def invalidsub_userinfo_handler(oidc_service): + @urlmatch(netloc=r'fakeoidc', path=r'/userinfo') + def handler(_, __): + content = { + 'sub': 'invalidsub', + 'preferred_username': 'someusername', + 'email': 'foo@example.com', + 'email_verified': True, + } + + return {'status_code': 200, 'content': json.dumps(content)} + + return handler + +@pytest.fixture() +def missingemail_userinfo_handler(oidc_service, preferred_username): + @urlmatch(netloc=r'fakeoidc', path=r'/userinfo') + def handler(_, __): + content = { + 'sub': 'cooluser', + 'preferred_username': preferred_username, + } + + return {'status_code': 200, 'content': json.dumps(content)} + + return handler + +def test_basic_config(oidc_service): + assert oidc_service.service_id() == 'someoidc' + assert oidc_service.service_name() == 'Some Cool Service' + assert oidc_service.get_icon() == 'http://some/icon' + +def test_discovery(oidc_service, http_client, discovery_handler): + with HTTMock(discovery_handler): + assert oidc_service.authorize_endpoint() == 'http://fakeoidc/authorize?response_type=code&' + assert oidc_service.token_endpoint() == 'http://fakeoidc/token' + assert oidc_service.user_endpoint() == 'http://fakeoidc/userinfo' + assert oidc_service.get_login_scopes() == ['profile'] + +def test_public_config(oidc_service, discovery_handler): + with HTTMock(discovery_handler): + assert oidc_service.get_public_config()['OIDC'] + assert oidc_service.get_public_config()['CLIENT_ID'] == 'foo' + + assert 'CLIENT_SECRET' not in oidc_service.get_public_config() + assert 'bar' not in oidc_service.get_public_config().values() + +def test_exchange_code_invalidcode(oidc_service, discovery_handler, app_config, http_client, + token_handler): + with HTTMock(token_handler, discovery_handler): + with pytest.raises(OAuthLoginException): + oidc_service.exchange_code_for_login(app_config, http_client, 'testcode', '') + +def test_exchange_code_validcode(oidc_service, discovery_handler, app_config, http_client, + token_handler, userinfo_handler, jwks_handler, valid_code, + preferred_username): + with HTTMock(jwks_handler, token_handler, userinfo_handler, discovery_handler): + lid, lusername, lemail = oidc_service.exchange_code_for_login(app_config, http_client, + valid_code, '') + + assert lid == 'cooluser' + assert lemail == 'foo@example.com' + + if preferred_username is not None: + assert lusername == preferred_username + else: + assert lusername == lid + +def test_exchange_code_missingemail(oidc_service, discovery_handler, app_config, http_client, + token_handler, missingemail_userinfo_handler, jwks_handler, + valid_code, preferred_username): + with HTTMock(jwks_handler, token_handler, missingemail_userinfo_handler, discovery_handler): + if app_config['FEATURE_MAILING']: + # Should fail because there is no valid email address. + with pytest.raises(OAuthLoginException): + oidc_service.exchange_code_for_login(app_config, http_client, valid_code, '') + else: + # Should succeed because, while there is no valid email address, it isn't necessary with + # mailing disabled. + lid, lusername, lemail = oidc_service.exchange_code_for_login(app_config, http_client, + valid_code, '') + + assert lid == 'cooluser' + assert lemail is None + + if preferred_username is not None: + assert lusername == preferred_username + else: + assert lusername == lid + +def test_exchange_code_invalidsub(oidc_service, discovery_handler, app_config, http_client, + token_handler, invalidsub_userinfo_handler, jwks_handler, + valid_code): + with HTTMock(jwks_handler, token_handler, invalidsub_userinfo_handler, discovery_handler): + # Should fail because the sub of the user info doesn't match that returned by the id_token. + with pytest.raises(OAuthLoginException): + oidc_service.exchange_code_for_login(app_config, http_client, valid_code, '') + +def test_exchange_code_missingkey(oidc_service, discovery_handler, app_config, http_client, + token_handler, userinfo_handler, emptykeys_jwks_handler, + valid_code): + with HTTMock(emptykeys_jwks_handler, token_handler, userinfo_handler, discovery_handler): + # Should fail because the key is missing. + with pytest.raises(OAuthLoginException): + oidc_service.exchange_code_for_login(app_config, http_client, valid_code, '') diff --git a/test/test_endpoints.py b/test/test_endpoints.py index 216021245..256b54bd4 100644 --- a/test/test_endpoints.py +++ b/test/test_endpoints.py @@ -8,7 +8,6 @@ import base64 from urllib import urlencode from urlparse import urlparse, urlunparse, parse_qs from datetime import datetime, timedelta -from httmock import urlmatch, HTTMock import jwt @@ -16,7 +15,7 @@ from Crypto.PublicKey import RSA from flask import url_for from jwkest.jwk import RSAKey -from app import app, oauth_login +from app import app from data import model from data.database import ServiceKeyApprovalType from endpoints import keyserver @@ -25,16 +24,9 @@ from endpoints.api.user import Signin from endpoints.keyserver import jwk_with_kid from endpoints.csrf import OAUTH_CSRF_TOKEN_NAME from endpoints.web import web as web_bp -from endpoints.oauthlogin import oauthlogin as oauthlogin_bp from initdb import setup_database_for_testing, finished_database_for_testing from test.helpers import assert_action_logged -try: - app.register_blueprint(oauthlogin_bp, url_prefix='/oauth2') -except ValueError: - # This blueprint was already registered - pass - try: app.register_blueprint(web_bp, url_prefix='') except ValueError: @@ -129,96 +121,6 @@ class EndpointTestCase(unittest.TestCase): self.assertEquals(rv.status_code, 200) -class OAuthLoginTestCase(EndpointTestCase): - def invoke_oauth_tests(self, callback_endpoint, attach_endpoint, service_name, service_ident, - new_username): - # Test callback. - created = self.invoke_oauth_test(callback_endpoint, service_name, service_ident, new_username) - - # Delete the created user. - model.user.delete_user(created, []) - - # Test attach. - self.login('devtable', 'password') - self.invoke_oauth_test(attach_endpoint, service_name, service_ident, 'devtable') - - def invoke_oauth_test(self, endpoint_name, service_name, service_ident, username): - # No CSRF. - self.getResponse('oauthlogin.' + endpoint_name, expected_code=403) - - # Invalid CSRF. - self.getResponse('oauthlogin.' + endpoint_name, state='somestate', expected_code=403) - - # Valid CSRF, invalid code. - self.getResponse('oauthlogin.' + endpoint_name, state='someoauthtoken', - code='invalidcode', expected_code=400) - - # Valid CSRF, valid code. - self.getResponse('oauthlogin.' + endpoint_name, state='someoauthtoken', - code='somecode', expected_code=302) - - # Ensure the user was added/modified. - found_user = model.user.get_user(username) - self.assertIsNotNone(found_user) - - federated_login = model.user.lookup_federated_login(found_user, service_name) - self.assertIsNotNone(federated_login) - self.assertEquals(federated_login.service_ident, service_ident) - return found_user - - def test_google_oauth(self): - @urlmatch(netloc=r'accounts.google.com', path='/o/oauth2/token') - def account_handler(_, request): - if request.body.find("code=somecode") > 0: - content = {'access_token': 'someaccesstoken'} - return py_json.dumps(content) - else: - return {'status_code': 400, 'content': '{"message": "Invalid code"}'} - - @urlmatch(netloc=r'www.googleapis.com', path='/oauth2/v1/userinfo') - def user_handler(_, __): - content = { - 'id': 'someid', - 'email': 'someemail@example.com', - 'verified_email': True, - } - return py_json.dumps(content) - - with HTTMock(account_handler, user_handler): - self.invoke_oauth_tests('google_oauth_callback', 'google_oauth_attach', 'google', - 'someid', 'someemail') - - def test_github_oauth(self): - @urlmatch(netloc=r'github.com', path='/login/oauth/access_token') - def account_handler(url, _): - if url.query.find("code=somecode") > 0: - content = {'access_token': 'someaccesstoken'} - return py_json.dumps(content) - else: - return {'status_code': 400, 'content': '{"message": "Invalid code"}'} - - @urlmatch(netloc=r'github.com', path='/api/v3/user') - def user_handler(_, __): - content = { - 'id': 'someid', - 'login': 'someusername' - } - return py_json.dumps(content) - - @urlmatch(netloc=r'github.com', path='/api/v3/user/emails') - def email_handler(_, __): - content = [{ - 'email': 'someemail@example.com', - 'verified': True, - 'primary': True, - }] - return py_json.dumps(content) - - with HTTMock(account_handler, email_handler, user_handler): - self.invoke_oauth_tests('github_oauth_callback', 'github_oauth_attach', 'github', - 'someid', 'someusername') - - class WebEndpointTestCase(EndpointTestCase): def test_index(self): self.getResponse('web.index') diff --git a/test/test_oauth_login.py b/test/test_oauth_login.py new file mode 100644 index 000000000..9590a39aa --- /dev/null +++ b/test/test_oauth_login.py @@ -0,0 +1,175 @@ +import json as py_json +import time +import unittest + +import jwt + +from Crypto.PublicKey import RSA +from httmock import urlmatch, HTTMock +from jwkest.jwk import RSAKey + +from app import app +from data import model +from endpoints.oauthlogin import oauthlogin as oauthlogin_bp +from test.test_endpoints import EndpointTestCase + +try: + app.register_blueprint(oauthlogin_bp, url_prefix='/oauth2') +except ValueError: + # This blueprint was already registered + pass + +class OAuthLoginTestCase(EndpointTestCase): + def invoke_oauth_tests(self, callback_endpoint, attach_endpoint, service_name, service_ident, + new_username): + # Test callback. + created = self.invoke_oauth_test(callback_endpoint, service_name, service_ident, new_username) + + # Delete the created user. + model.user.delete_user(created, []) + + # Test attach. + self.login('devtable', 'password') + self.invoke_oauth_test(attach_endpoint, service_name, service_ident, 'devtable') + + def invoke_oauth_test(self, endpoint_name, service_name, service_ident, username): + # No CSRF. + self.getResponse('oauthlogin.' + endpoint_name, expected_code=403) + + # Invalid CSRF. + self.getResponse('oauthlogin.' + endpoint_name, state='somestate', expected_code=403) + + # Valid CSRF, invalid code. + self.getResponse('oauthlogin.' + endpoint_name, state='someoauthtoken', + code='invalidcode', expected_code=400) + + # Valid CSRF, valid code. + self.getResponse('oauthlogin.' + endpoint_name, state='someoauthtoken', + code='somecode', expected_code=302) + + # Ensure the user was added/modified. + found_user = model.user.get_user(username) + self.assertIsNotNone(found_user) + + federated_login = model.user.lookup_federated_login(found_user, service_name) + self.assertIsNotNone(federated_login) + self.assertEquals(federated_login.service_ident, service_ident) + return found_user + + def test_google_oauth(self): + @urlmatch(netloc=r'accounts.google.com', path='/o/oauth2/token') + def account_handler(_, request): + if request.body.find("code=somecode") > 0: + content = {'access_token': 'someaccesstoken'} + return py_json.dumps(content) + else: + return {'status_code': 400, 'content': '{"message": "Invalid code"}'} + + @urlmatch(netloc=r'www.googleapis.com', path='/oauth2/v1/userinfo') + def user_handler(_, __): + content = { + 'id': 'someid', + 'email': 'someemail@example.com', + 'verified_email': True, + } + return py_json.dumps(content) + + with HTTMock(account_handler, user_handler): + self.invoke_oauth_tests('google_oauth_callback', 'google_oauth_attach', 'google', + 'someid', 'someemail') + + def test_github_oauth(self): + @urlmatch(netloc=r'github.com', path='/login/oauth/access_token') + def account_handler(url, _): + if url.query.find("code=somecode") > 0: + content = {'access_token': 'someaccesstoken'} + return py_json.dumps(content) + else: + return {'status_code': 400, 'content': '{"message": "Invalid code"}'} + + @urlmatch(netloc=r'github.com', path='/api/v3/user') + def user_handler(_, __): + content = { + 'id': 'someid', + 'login': 'someusername' + } + return py_json.dumps(content) + + @urlmatch(netloc=r'github.com', path='/api/v3/user/emails') + def email_handler(_, __): + content = [{ + 'email': 'someemail@example.com', + 'verified': True, + 'primary': True, + }] + return py_json.dumps(content) + + with HTTMock(account_handler, email_handler, user_handler): + self.invoke_oauth_tests('github_oauth_callback', 'github_oauth_attach', 'github', + 'someid', 'someusername') + + def test_oidc_auth(self): + private_key = RSA.generate(2048) + generatedjwk = RSAKey(key=private_key.publickey()).serialize() + kid = 'somekey' + private_pem = private_key.exportKey('PEM') + + token_data = { + 'iss': app.config['TESTOIDC_LOGIN_CONFIG']['OIDC_SERVER'], + 'aud': app.config['TESTOIDC_LOGIN_CONFIG']['CLIENT_ID'], + 'nbf': int(time.time()), + 'iat': int(time.time()), + 'exp': int(time.time() + 600), + 'sub': 'cooluser', + } + + token_headers = { + 'kid': kid, + } + + id_token = jwt.encode(token_data, private_pem, 'RS256', headers=token_headers) + + @urlmatch(netloc=r'fakeoidc', path='/token') + def token_handler(_, request): + if request.body.find("code=somecode") >= 0: + content = {'access_token': 'someaccesstoken', 'id_token': id_token} + return py_json.dumps(content) + else: + return {'status_code': 400, 'content': '{"message": "Invalid code"}'} + + @urlmatch(netloc=r'fakeoidc', path='/user') + def user_handler(_, __): + content = { + 'sub': 'cooluser', + 'preferred_username': 'someusername', + 'email': 'someemail@example.com', + 'email_verified': True, + } + return py_json.dumps(content) + + @urlmatch(netloc=r'fakeoidc', path='/jwks') + def jwks_handler(_, __): + jwk = generatedjwk.copy() + jwk.update({'kid': kid}) + + content = {'keys': [jwk]} + return py_json.dumps(content) + + @urlmatch(netloc=r'fakeoidc', path='.+openid.+') + def discovery_handler(_, __): + content = { + 'scopes_supported': ['profile'], + 'authorization_endpoint': 'http://fakeoidc/authorize', + 'token_endpoint': 'http://fakeoidc/token', + 'userinfo_endpoint': 'http://fakeoidc/userinfo', + 'jwks_uri': 'http://fakeoidc/jwks', + } + return py_json.dumps(content) + + with HTTMock(discovery_handler, jwks_handler, token_handler, user_handler): + self.invoke_oauth_tests('testoidc_oauth_callback', 'testoidc_oauth_attach', 'testoidc', + 'cooluser', 'someusername') + +if __name__ == '__main__': + unittest.main() + diff --git a/test/testconfig.py b/test/testconfig.py index eb976aa65..e9d6c03db 100644 --- a/test/testconfig.py +++ b/test/testconfig.py @@ -81,11 +81,12 @@ class TestConfig(DefaultConfig): FEATURE_GITHUB_LOGIN = True FEATURE_GOOGLE_LOGIN = True - FEATURE_DEX_LOGIN = True - DEX_LOGIN_CONFIG = { - 'CLIENT_ID': 'someclientid', - 'OIDC_SERVER': 'https://oidcserver/', + TESTOIDC_LOGIN_CONFIG = { + 'CLIENT_ID': 'foo', + 'CLIENT_SECRET': 'bar', + 'OIDC_SERVER': 'http://fakeoidc', + 'DEBUGGING': True, } RECAPTCHA_SITE_KEY = 'somekey' From a9791ea419c2e8de7e20d2f3d743db2608ef2e1d Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 23 Jan 2017 19:06:19 -0500 Subject: [PATCH 05/13] 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) From adb2ff0b818ad949ecf8c1071520d8a0aeb6560e Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 24 Jan 2017 15:20:03 -0500 Subject: [PATCH 06/13] Switch base classes in OAuth to use ABC --- oauth/base.py | 22 ++++++++++++++++------ oauth/login.py | 23 +++++++++++++++++------ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/oauth/base.py b/oauth/base.py index 15a82c1c4..ee290bbf5 100644 --- a/oauth/base.py +++ b/oauth/base.py @@ -1,6 +1,9 @@ import logging import urllib +from abc import ABCMeta, abstractmethod +from six import add_metaclass + from util import get_app_url logger = logging.getLogger(__name__) @@ -13,36 +16,43 @@ class OAuthGetUserInfoException(Exception): """ Exception raised if a call to get user information fails. """ pass +@add_metaclass(ABCMeta) class OAuthService(object): """ A base class for defining an external service, exposed via OAuth. """ def __init__(self, config, key_name): self.key_name = key_name self.config = config.get(key_name) or {} + @abstractmethod def service_id(self): """ The internal ID for this service. Must match the URL portion for the service, e.g. `github` """ - raise NotImplementedError + pass + @abstractmethod def service_name(self): """ The user-readable name for the service, e.g. `GitHub`""" - raise NotImplementedError + pass + @abstractmethod def token_endpoint(self): """ The endpoint at which the OAuth code can be exchanged for a token. """ - raise NotImplementedError + pass + @abstractmethod def user_endpoint(self): """ The endpoint at which user information can be looked up. """ - raise NotImplementedError + pass + @abstractmethod def validate_client_id_and_secret(self, http_client, app_config): """ Performs validation of the client ID and secret, raising an exception on failure. """ - raise NotImplementedError + pass + @abstractmethod def authorize_endpoint(self): """ Endpoint for authorization. """ - raise NotImplementedError + pass def requires_form_encoding(self): """ Returns True if form encoding is necessary for the exchange_code_for_token call. """ diff --git a/oauth/login.py b/oauth/login.py index 268d030f7..55c94be69 100644 --- a/oauth/login.py +++ b/oauth/login.py @@ -1,5 +1,8 @@ import logging +from abc import ABCMeta, abstractmethod +from six import add_metaclass + import features from oauth.base import OAuthService, OAuthExchangeCodeException, OAuthGetUserInfoException @@ -10,33 +13,41 @@ class OAuthLoginException(Exception): """ Exception raised if a login operation fails. """ pass + +@add_metaclass(ABCMeta) class OAuthLoginService(OAuthService): """ A base class for defining an OAuth-compliant service that can be used for, amongst other things, login and authentication. """ + @abstractmethod def login_enabled(self): """ Returns true if the login service is enabled. """ - raise NotImplementedError + pass + @abstractmethod def get_login_service_id(self, user_info): """ Returns the internal ID for the given user under this login service. """ - raise NotImplementedError + pass + @abstractmethod def get_login_service_username(self, user_info): """ Returns the username for the given user under this login service. """ - raise NotImplementedError + pass + @abstractmethod def get_verified_user_email(self, app_config, http_client, token, user_info): """ Returns the verified email address for the given user, if any or None if none. """ - raise NotImplementedError + pass + @abstractmethod def get_icon(self): """ Returns the icon to display for this login service. """ - raise NotImplementedError + pass + @abstractmethod def get_login_scopes(self): """ Returns the list of scopes for login for this service. """ - raise NotImplementedError + pass def service_verify_user_info_for_login(self, app_config, http_client, token, user_info): """ Performs service-specific verification of user information for login. On failure, a service From 8573535b8c84c9352b9812767412e93e5cd233aa Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 24 Jan 2017 15:20:19 -0500 Subject: [PATCH 07/13] Add comment clarifying how we validate client {ID, secret} in Gitlab --- oauth/services/gitlab.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/oauth/services/gitlab.py b/oauth/services/gitlab.py index 195ce177b..f05d9c617 100644 --- a/oauth/services/gitlab.py +++ b/oauth/services/gitlab.py @@ -30,6 +30,10 @@ class GitLabOAuthService(OAuthService): return slash_join(self._endpoint(), '/oauth/token') def validate_client_id_and_secret(self, http_client, app_config): + # We validate the client ID and secret by hitting the OAuth token exchange endpoint with + # the real client ID and secret, but a fake auth code to exchange. Gitlab's implementation will + # return `invalid_client` as the `error` if the client ID or secret is invalid; otherwise, it + # will return another error. url = self.token_endpoint() redirect_uri = self.get_redirect_uri(app_config, redirect_suffix='trigger') data = { From 90b6a534c1ecf4ba529c3c728b38a7988d9a5547 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 26 Jan 2017 12:00:43 -0500 Subject: [PATCH 08/13] Change verify param in OIDC to read better --- oauth/oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth/oidc.py b/oauth/oidc.py index f5c274259..f858bb1b3 100644 --- a/oauth/oidc.py +++ b/oauth/oidc.py @@ -156,7 +156,7 @@ class OIDCLoginService(OAuthService): raise DiscoveryFailureException('OIDC server must be accessed over SSL') discovery_url = urlparse.urljoin(oidc_server, OIDC_WELLKNOWN) - discovery = self._http_client.get(discovery_url, timeout=5, verify=not is_debugging) + discovery = self._http_client.get(discovery_url, timeout=5, verify=is_debugging is False) if discovery.status_code // 100 != 2: logger.debug('Got %s response for OIDC discovery: %s', discovery.status_code, discovery.text) raise DiscoveryFailureException("Could not load OIDC discovery information") From ce5fafcbd8e6805f7ef77a0aea12916da678cc98 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 26 Jan 2017 12:00:54 -0500 Subject: [PATCH 09/13] Fix pylint ignores to use names --- oauth/test/test_oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth/test/test_oidc.py b/oauth/test/test_oidc.py index cf9612136..e8bdd0c0e 100644 --- a/oauth/test/test_oidc.py +++ b/oauth/test/test_oidc.py @@ -1,4 +1,4 @@ -# pylint: disable=redefined-outer-name, unused-argument, C0103, C0111, too-many-arguments +# pylint: disable=redefined-outer-name, unused-argument, invalid-name, missing-docstring, too-many-arguments import json import time From f8deb85751f35aee81b2d305dc5505ad5fd2aa6f Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 26 Jan 2017 12:01:55 -0500 Subject: [PATCH 10/13] Clarify OAuth logging message when missing access_token --- oauth/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth/base.py b/oauth/base.py index ee290bbf5..0afa418c9 100644 --- a/oauth/base.py +++ b/oauth/base.py @@ -109,7 +109,7 @@ class OAuthService(object): access_token = json_data.get('access_token', None) if access_token is None: - logger.debug('Got successful get_access_token response %s', json_data) + logger.debug('Got successful get_access_token response with missing token: %s', json_data) raise OAuthExchangeCodeException('Missing `access_token` in OAuth response') return access_token From c9812864be96d928ee602445835827ceb1ba4e12 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 26 Jan 2017 12:03:56 -0500 Subject: [PATCH 11/13] Small JS fixes for the PR --- static/directives/icon-image-view.html | 2 +- static/js/directives/ui/icon-image-view.js | 2 +- static/js/services/external-login-service.js | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/static/directives/icon-image-view.html b/static/directives/icon-image-view.html index c213782e4..13b6105da 100644 --- a/static/directives/icon-image-view.html +++ b/static/directives/icon-image-view.html @@ -1,4 +1,4 @@ - \ No newline at end of file + diff --git a/static/js/directives/ui/icon-image-view.js b/static/js/directives/ui/icon-image-view.js index 02fccd046..019f5ea38 100644 --- a/static/js/directives/ui/icon-image-view.js +++ b/static/js/directives/ui/icon-image-view.js @@ -15,4 +15,4 @@ angular.module('quay').directive('iconImageView', function () { } }; return directiveDefinitionObject; -}); \ No newline at end of file +}); diff --git a/static/js/services/external-login-service.js b/static/js/services/external-login-service.js index a911fd07a..4f83ea4a7 100644 --- a/static/js/services/external-login-service.js +++ b/static/js/services/external-login-service.js @@ -5,7 +5,7 @@ angular.module('quay').factory('ExternalLoginService', ['Features', 'Config', 'A function(Features, Config, ApiService) { var externalLoginService = {}; - externalLoginService.EXTERNAL_LOGINS = window.__external_login; + externalLoginService.EXTERNAL_LOGINS = window.__external_login || []; externalLoginService.getLoginUrl = function(loginService, action, callback) { var errorDisplay = ApiService.errorDisplay('Could not load external login service ' + @@ -31,8 +31,7 @@ angular.module('quay').factory('ExternalLoginService', ['Features', 'Config', 'A externalLoginService.getSingleSigninUrl = function(callback) { if (!externalLoginService.hasSingleSignin()) { - callback(null); - return; + return callback(null); } // If there is a single external login service and direct login is disabled, From cf6033b423284da479cf32dbf16878ef6b2ac6fe Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 30 Jan 2017 11:40:45 -0500 Subject: [PATCH 12/13] Move http_client fixture to root-level conftest --- conftest.py | 12 ++++++++++++ oauth/test/test_oidc.py | 10 ---------- 2 files changed, 12 insertions(+), 10 deletions(-) create mode 100644 conftest.py diff --git a/conftest.py b/conftest.py new file mode 100644 index 000000000..b2488d926 --- /dev/null +++ b/conftest.py @@ -0,0 +1,12 @@ +import pytest + +import requests + +@pytest.fixture() +def http_client(): + sess = requests.Session() + adapter = requests.adapters.HTTPAdapter(pool_connections=100, + pool_maxsize=100) + sess.mount('http://', adapter) + sess.mount('https://', adapter) + return sess diff --git a/oauth/test/test_oidc.py b/oauth/test/test_oidc.py index e8bdd0c0e..4f15bfe0d 100644 --- a/oauth/test/test_oidc.py +++ b/oauth/test/test_oidc.py @@ -6,7 +6,6 @@ import urlparse import jwt import pytest -import requests from httmock import urlmatch, HTTMock from Crypto.PublicKey import RSA @@ -14,15 +13,6 @@ from jwkest.jwk import RSAKey from oauth.oidc import OIDCLoginService, OAuthLoginException -@pytest.fixture() -def http_client(): - sess = requests.Session() - adapter = requests.adapters.HTTPAdapter(pool_connections=100, - pool_maxsize=100) - sess.mount('http://', adapter) - sess.mount('https://', adapter) - return sess - @pytest.fixture(params=[True, False]) def app_config(http_client, request): return { From f5dbc350f8ff135fb2aee0717481ca9a328c8133 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 30 Jan 2017 17:28:25 -0500 Subject: [PATCH 13/13] Fix missed tests and revert conftest change (breaks docker build) --- .dockerignore | 1 + conftest.py | 12 ------------ oauth/test/test_oidc.py | 10 ++++++++++ test/test_github.py | 6 +++--- util/config/validator.py | 10 ++++++---- 5 files changed, 20 insertions(+), 19 deletions(-) delete mode 100644 conftest.py diff --git a/.dockerignore b/.dockerignore index 0a5878cef..ce563e77b 100644 --- a/.dockerignore +++ b/.dockerignore @@ -20,3 +20,4 @@ coverage .cache .npm-debug.log test/__pycache__ +__pycache__ diff --git a/conftest.py b/conftest.py deleted file mode 100644 index b2488d926..000000000 --- a/conftest.py +++ /dev/null @@ -1,12 +0,0 @@ -import pytest - -import requests - -@pytest.fixture() -def http_client(): - sess = requests.Session() - adapter = requests.adapters.HTTPAdapter(pool_connections=100, - pool_maxsize=100) - sess.mount('http://', adapter) - sess.mount('https://', adapter) - return sess diff --git a/oauth/test/test_oidc.py b/oauth/test/test_oidc.py index 4f15bfe0d..e8bdd0c0e 100644 --- a/oauth/test/test_oidc.py +++ b/oauth/test/test_oidc.py @@ -6,6 +6,7 @@ import urlparse import jwt import pytest +import requests from httmock import urlmatch, HTTMock from Crypto.PublicKey import RSA @@ -13,6 +14,15 @@ from jwkest.jwk import RSAKey from oauth.oidc import OIDCLoginService, OAuthLoginException +@pytest.fixture() +def http_client(): + sess = requests.Session() + adapter = requests.adapters.HTTPAdapter(pool_connections=100, + pool_maxsize=100) + sess.mount('http://', adapter) + sess.mount('https://', adapter) + return sess + @pytest.fixture(params=[True, False]) def app_config(http_client, request): return { diff --git a/test/test_github.py b/test/test_github.py index 4b2fc5d2c..1296af155 100644 --- a/test/test_github.py +++ b/test/test_github.py @@ -1,6 +1,6 @@ import unittest -from util.config.oauth import GithubOAuthConfig +from oauth.services.github import GithubOAuthService class TestGithub(unittest.TestCase): def test_basic_enterprise_config(self): @@ -12,7 +12,7 @@ class TestGithub(unittest.TestCase): } } - github_trigger = GithubOAuthConfig(config, 'GITHUB_TRIGGER_CONFIG') + github_trigger = GithubOAuthService(config, 'GITHUB_TRIGGER_CONFIG') self.assertTrue(github_trigger.is_enterprise()) self.assertEquals('https://github.somedomain.com/login/oauth/authorize?', github_trigger.authorize_endpoint()) self.assertEquals('https://github.somedomain.com/login/oauth/access_token', github_trigger.token_endpoint()) @@ -33,7 +33,7 @@ class TestGithub(unittest.TestCase): } } - github_trigger = GithubOAuthConfig(config, 'GITHUB_TRIGGER_CONFIG') + github_trigger = GithubOAuthService(config, 'GITHUB_TRIGGER_CONFIG') self.assertTrue(github_trigger.is_enterprise()) self.assertEquals('https://github.somedomain.com/login/oauth/authorize?', github_trigger.authorize_endpoint()) self.assertEquals('https://github.somedomain.com/login/oauth/access_token', github_trigger.token_endpoint()) diff --git a/util/config/validator.py b/util/config/validator.py index 9ff7ea973..3c31ca056 100644 --- a/util/config/validator.py +++ b/util/config/validator.py @@ -22,7 +22,9 @@ from data.users.externaljwt import ExternalJWTAuthN from data.users.externalldap import LDAPConnection, LDAPUsers from data.users.keystone import get_keystone_users from storage import get_storage_driver -from util.config.oauth import GoogleOAuthConfig, GithubOAuthConfig, GitLabOAuthConfig +from oauth.services.github import GithubOAuthService +from oauth.services.google import GoogleOAuthService +from oauth.services.gitlab import GitLabOAuthService from util.secscan.api import SecurityScannerAPI from util.registry.torrent import torrent_jwt from util.security.signing import SIGNING_ENGINES @@ -159,7 +161,7 @@ def _validate_gitlab(config, user_obj, _): raise ConfigValidationException('Missing Client Secret') client = app.config['HTTPCLIENT'] - oauth = GitLabOAuthConfig(config, 'GITLAB_TRIGGER_CONFIG') + oauth = GitLabOAuthService(config, 'GITLAB_TRIGGER_CONFIG') result = oauth.validate_client_id_and_secret(client, app.config) if not result: raise ConfigValidationException('Invalid client id or client secret') @@ -193,7 +195,7 @@ def _validate_github_with_key(config_key, config): 'organization') client = app.config['HTTPCLIENT'] - oauth = GithubOAuthConfig(config, config_key) + oauth = GithubOAuthService(config, config_key) result = oauth.validate_client_id_and_secret(client, app.config) if not result: raise ConfigValidationException('Invalid client id or client secret') @@ -239,7 +241,7 @@ def _validate_google_login(config, user_obj, _): raise ConfigValidationException('Missing Client Secret') client = app.config['HTTPCLIENT'] - oauth = GoogleOAuthConfig(config, 'GOOGLE_LOGIN_CONFIG') + oauth = GoogleOAuthService(config, 'GOOGLE_LOGIN_CONFIG') result = oauth.validate_client_id_and_secret(client, app.config) if not result: raise ConfigValidationException('Invalid client id or client secret')