From c6b0376d610b7e2b8462ce5bba52415cdd1cd2e0 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 1 Feb 2017 14:32:57 -0500 Subject: [PATCH 1/9] Remove unnecessary email generation in OAuth login Handled by the `emaIl_required` flag already --- endpoints/oauthlogin.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index f1bffc064..538117f95 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -1,5 +1,4 @@ import logging -import uuid from flask import request, redirect, url_for, Blueprint from peewee import IntegrityError @@ -60,16 +59,12 @@ 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, metadata=metadata or {}, - prompts=prompts) + prompts=prompts, + email_required=features.MAILING) # Success, tell analytics analytics.track(to_login.username, 'register', {'service': service_name.lower()}) From 2c35383724f70b1661834d057597c5e4b2b10658 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 1 Feb 2017 15:26:21 -0500 Subject: [PATCH 2/9] Allow OAuth and OIDC login engines to bind to fields in internal auth This feature is subtle but very important: Currently, when a user logs in via an "external" auth system (such as Github), they are either logged into an existing bound account or a new account is created for them in the database. While this normally works jut fine, it hits a roadblock when the *internal* auth system configured is not the database, but instead something like LDAP. In that case, *most* Enterprise customers will prefer that logging in via external auth (like OIDC) will also *automatically* bind the newly created account to the backing *internal* auth account. For example, login via PingFederate OIDC (backed by LDAP) should also bind the new QE account to the associated LDAP account, via either username or email. This change allows for this binding field to be specified, and thereafter will perform the proper lookups and bindings. --- endpoints/api/test/__init__.py | 0 endpoints/api/test/conftest.py | 6 +- endpoints/oauthlogin.py | 180 ++++++++++++++++++++---------- endpoints/test/test_oauthlogin.py | 177 +++++++++++++++++++++++++++++ oauth/base.py | 11 ++ 5 files changed, 315 insertions(+), 59 deletions(-) create mode 100644 endpoints/api/test/__init__.py create mode 100644 endpoints/test/test_oauthlogin.py diff --git a/endpoints/api/test/__init__.py b/endpoints/api/test/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/endpoints/api/test/conftest.py b/endpoints/api/test/conftest.py index 707fc24ca..07128a0c7 100644 --- a/endpoints/api/test/conftest.py +++ b/endpoints/api/test/conftest.py @@ -11,8 +11,10 @@ from data import model from data.database import (close_db_filter, db) from data.model.user import LoginWrappedDBUser from endpoints.api import api_bp +from endpoints.web import web + from initdb import initialize_database, populate_database -from path_converters import APIRepositoryPathConverter, RegexConverter +from path_converters import APIRepositoryPathConverter, RegexConverter, RepositoryPathConverter @pytest.fixture() @@ -33,7 +35,9 @@ def app(appconfig): app.url_map.converters['regex'] = RegexConverter app.url_map.converters['apirepopath'] = APIRepositoryPathConverter + app.url_map.converters['repopath'] = RepositoryPathConverter app.register_blueprint(api_bp, url_prefix='/api') + app.register_blueprint(web, url_prefix='/') app.config.update(appconfig) return app diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index 538117f95..d5d4ac591 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -1,11 +1,12 @@ import logging +from collections import namedtuple from flask import request, redirect, url_for, Blueprint from peewee import IntegrityError import features -from app import app, analytics, get_app_url, oauth_login +from app import app, analytics, get_app_url, oauth_login, authentication from auth.auth_context import get_authenticated_user from auth.process import require_session_login from data import model @@ -21,6 +22,100 @@ oauthlogin = Blueprint('oauthlogin', __name__) oauthlogin_csrf_protect = csrf_protect(OAUTH_CSRF_TOKEN_NAME, 'state', all_methods=True) + +OAuthResult = namedtuple('oauthresult', ['to_login', 'service_name', 'error_message', + 'register_redirect']) + +def _oauthresult(to_login=None, service_name=None, error_message=None, register_redirect=False): + return OAuthResult(to_login, service_name, error_message, register_redirect) + +def _get_response(result): + if result.error_message is not None: + return _render_ologin_error(result.service_name, result.error_message, result.register_redirect) + + return _perform_login(result.to_login, result.service_name) + +def _conduct_oauth_login(auth_system, login_service, lid, lusername, lemail, metadata=None): + """ Conducts login from the result of an OAuth service's login flow and returns + the status of the login, as well as the followup step. """ + service_id = login_service.service_id() + service_name = login_service.service_name() + + # Check for an existing account *bound to this service*. If found, conduct login of that account + # and redirect. + to_login = model.user.verify_federated_login(service_id, lid) + if to_login is not None: + return _oauthresult(to_login=to_login, service_name=service_name) + + # If the login service has a bound field name, and we have a defined internal auth type that is + # not the database, then search for an existing account with that matching field. This allows + # users to setup SSO while also being backed by something like LDAP. + bound_field_name = login_service.login_binding_field() + if auth_system.federated_service is not None and bound_field_name is not None: + # Perform lookup. + logger.debug('Got oauth bind field name of "%s"', bound_field_name) + lookup_value = None + if bound_field_name == 'username': + lookup_value = lusername + elif bound_field_name == 'email': + lookup_value = lemail + + if lookup_value is None: + logger.error('Missing lookup value for OAuth login') + return _oauthresult(service_name=service_name, + error_message='Configuration error in this provider') + + (user_obj, err) = auth_system.link_user(lookup_value) + if err is not None: + logger.debug('%s %s not found: %s', bound_field_name, lookup_value, err) + msg = '%s %s not found in backing auth system' % (bound_field_name, lookup_value) + return _oauthresult(service_name=service_name, error_message=msg) + + # Found an existing user. Bind their internal auth account to this service as well. + result = _attach_service(login_service, user_obj, lid, lusername) + if result.error_message is not None: + return result + + return _oauthresult(to_login=user_obj, service_name=service_name) + + # Otherwise, we need to create a new user account. + if not features.USER_CREATION: + error_message = 'User creation is disabled. Please contact your administrator' + return _oauthresult(service_name=service_name, error_message=error_message) + + # Try to create the user + try: + # Generate a valid username. + new_username = None + for valid in generate_valid_usernames(lusername): + if model.user.get_user_or_org(valid): + continue + + new_username = valid + break + + prompts = model.user.get_default_user_prompts(features) + to_login = model.user.create_federated_user(new_username, lemail, service_id, lid, + set_password_notification=True, + metadata=metadata or {}, + prompts=prompts, + email_required=features.MAILING) + + # Success, tell analytics + analytics.track(to_login.username, 'register', {'service': service_name.lower()}) + return _oauthresult(to_login=to_login, service_name=service_name) + + except model.InvalidEmailAddressException: + message = "The e-mail address %s is already associated " % (lemail, ) + message = message + "with an existing %s account." % (app.config['REGISTRY_TITLE_SHORT'], ) + 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 _oauthresult(service_name=service_name, error_message=message, + register_redirect=True) + + except model.DataModelException as ex: + return _oauthresult(service_name=service_name, error_message=ex.message) + def _render_ologin_error(service_name, error_message=None, register_redirect=False): """ Returns a Flask response indicating an OAuth error. """ @@ -38,56 +133,33 @@ def _render_ologin_error(service_name, error_message=None, register_redirect=Fal 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. """ - - 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) - - # 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): - continue - - new_username = valid - break - - 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, - metadata=metadata or {}, - prompts=prompts, - email_required=features.MAILING) - - # Success, tell analytics - analytics.track(to_login.username, 'register', {'service': service_name.lower()}) - - except model.InvalidEmailAddressException: - message = "The e-mail address %s is already associated " % (email, ) - message = message + "with an existing %s account." % (app.config['REGISTRY_TITLE_SHORT'], ) - 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) - - except model.DataModelException as ex: - return _render_ologin_error(service_name, ex.message) - +def _perform_login(to_login, service_name): + """ Attempts to login the given user, returning the Flask result of whether the login succeeded. + """ if common_login(to_login): if model.user.has_user_prompts(to_login): return redirect(url_for('web.updateuser')) else: return redirect(url_for('web.index')) + else: + return _render_ologin_error(service_name, 'Could not login. Account may be disabled') - return _render_ologin_error(service_name) +def _attach_service(login_service, user_obj, lid, lusername): + """ Attaches the given user account to the given service, with the given service user ID and + service username. + """ + metadata = { + 'service_username': lusername + } + try: + model.user.attach_federated_login(user_obj, login_service.service_id(), lid, + metadata=metadata) + return _oauthresult(to_login=user_obj) + except IntegrityError: + err = '%s account %s is already attached to a %s account' % ( + login_service.service_name(), lusername, app.config['REGISTRY_TITLE_SHORT']) + return _oauthresult(service_name=login_service.service_name(), error_message=err) def _register_service(login_service): """ Registers the given login service, adding its callback and attach routes to the blueprint. """ @@ -112,8 +184,9 @@ def _register_service(login_service): 'service_username': lusername } - return _conduct_oauth_login(login_service.service_id(), login_service.service_name(), lid, - lusername, lemail, metadata=metadata) + result = _conduct_oauth_login(authentication, login_service, lid, lusername, lemail, + metadata=metadata) + return _get_response(result) @require_session_login @@ -132,19 +205,10 @@ def _register_service(login_service): return _render_ologin_error(login_service.service_name(), ole.message) # Conduct attach. - metadata = { - 'service_username': lusername - } - user_obj = get_authenticated_user() - - 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) + result = _attach_service(login_service, user_obj, lid, lusername) + if result.error_message is not None: + return _get_response(result) return redirect(url_for('web.user_view', path=user_obj.username, tab='external')) diff --git a/endpoints/test/test_oauthlogin.py b/endpoints/test/test_oauthlogin.py new file mode 100644 index 000000000..340e33c1a --- /dev/null +++ b/endpoints/test/test_oauthlogin.py @@ -0,0 +1,177 @@ +import pytest + +from endpoints.oauthlogin import _conduct_oauth_login +from endpoints.api.test.conftest import app, appconfig, database_uri, init_db_path, sqlitedb_file + +from oauth.services.github import GithubOAuthService + +from data import model, database +from data.users import get_users_handler, DatabaseUsers +from test.test_ldap import mock_ldap + + +@pytest.fixture(params=[None, 'username', 'email']) +def login_service(request, app): + config = {'GITHUB': {}} + if request is not None: + config['GITHUB']['LOGIN_BINDING_FIELD'] = request.param + + return GithubOAuthService(config, 'GITHUB') + + +@pytest.fixture(params=['Database', 'LDAP']) +def auth_system(request): + return _get_users_handler(request.param) + +def _get_users_handler(auth_type): + config = {} + config['AUTHENTICATION_TYPE'] = auth_type + config['LDAP_BASE_DN'] = ['dc=quay', 'dc=io'] + config['LDAP_ADMIN_DN'] = 'uid=testy,ou=employees,dc=quay,dc=io' + config['LDAP_ADMIN_PASSWD'] = 'password' + config['LDAP_USER_RDN'] = ['ou=employees'] + + return get_users_handler(config, None, None) + +def test_existing_account(auth_system, login_service): + login_service_lid = 'someexternaluser' + + # Create an existing bound federated user. + created_user = model.user.create_federated_user('someuser', 'example@example.com', + login_service.service_id(), + login_service_lid, False) + existing_user_count = database.User.select().count() + + with mock_ldap(): + result = _conduct_oauth_login(auth_system, login_service, + login_service_lid, login_service_lid, + 'example@example.com') + + assert result.to_login == created_user + + # Ensure that no addtional users were created. + current_user_count = database.User.select().count() + assert current_user_count == existing_user_count + + +def test_new_account_via_database(login_service): + existing_user_count = database.User.select().count() + login_service_lid = 'someexternaluser' + internal_auth = DatabaseUsers() + + # Conduct login. Since the external user doesn't (yet) bind to a user in the database, + # a new user should be created and bound to the external service. + result = _conduct_oauth_login(internal_auth, login_service, login_service_lid, login_service_lid, + 'example@example.com') + assert result.to_login is not None + + current_user_count = database.User.select().count() + assert current_user_count == existing_user_count + 1 + + # Find the user and ensure it is bound. + new_user = model.user.get_user(login_service_lid) + federated_login = model.user.lookup_federated_login(new_user, login_service.service_id()) + assert federated_login is not None + + +@pytest.mark.parametrize('binding_field, lusername, lemail, expected_error', [ + # No binding field + newly seen user -> New unlinked user + (None, 'someunknownuser', 'someemail@example.com', None), + + # username binding field + unknown username -> Error. + ('username', 'someunknownuser', 'foo@bar.com', + 'username someunknownuser not found in backing auth system'), + + # email binding field + unknown email address -> Error. + ('email', 'someuser', 'someemail@example.com', + 'email someemail@example.com not found in backing auth system'), + + # No binding field + newly seen user -> New unlinked user. + (None, 'someuser', 'foo@bar.com', None), + + # username binding field + valid username -> fully bound user. + ('username', 'someuser', 'foo@bar.com', None), + + # email binding field + valid email -> fully bound user. + ('email', 'someuser', 'foo@bar.com', None), + + # username binding field + valid username + invalid email -> fully bound user. + ('username', 'someuser', 'another@email.com', None), + + # email binding field + valid email + invalid username -> fully bound user. + ('email', 'someotherusername', 'foo@bar.com', None), +]) +def test_new_account_via_ldap(binding_field, lusername, lemail, expected_error, app): + existing_user_count = database.User.select().count() + + config = {'GITHUB': {}} + if binding_field is not None: + config['GITHUB']['LOGIN_BINDING_FIELD'] = binding_field + + external_auth = GithubOAuthService(config, 'GITHUB') + internal_auth = _get_users_handler('LDAP') + + with mock_ldap(): + # Conduct OAuth login. + result = _conduct_oauth_login(internal_auth, external_auth, 'someid', lusername, lemail) + assert result.error_message == expected_error + + current_user_count = database.User.select().count() + if expected_error is None: + # Ensure that the new user was created and that it is bound to both the + # external login service and to LDAP (if a binding_field was given). + assert current_user_count == existing_user_count + 1 + assert result.to_login is not None + + # Check the service bindings. + external_login = model.user.lookup_federated_login(result.to_login, + external_auth.service_id()) + assert external_login is not None + + internal_login = model.user.lookup_federated_login(result.to_login, + internal_auth.federated_service) + if binding_field is not None: + assert internal_login is not None + else: + assert internal_login is None + + else: + # Ensure that no addtional users were created. + assert current_user_count == existing_user_count + + +def test_existing_account_in_ldap(app): + config = {'GITHUB': {'LOGIN_BINDING_FIELD': 'username'}} + + external_auth = GithubOAuthService(config, 'GITHUB') + internal_auth = _get_users_handler('LDAP') + + # Add an existing federated user bound to the LDAP account associated with `someuser`. + bound_user = model.user.create_federated_user('someuser', 'foo@bar.com', + internal_auth.federated_service, 'someuser', False) + + existing_user_count = database.User.select().count() + + with mock_ldap(): + # Conduct OAuth login with the same lid and bound field. This should find the existing LDAP + # user (via the `username` binding), and then bind Github to it as well. + result = _conduct_oauth_login(internal_auth, external_auth, bound_user.username, + bound_user.username, bound_user.email) + assert result.error_message is None + + # Ensure that the same user was returned, and that it is now bound to the Github account + # as well. + assert result.to_login.id == bound_user.id + + # Ensure that no additional users were created. + current_user_count = database.User.select().count() + assert current_user_count == existing_user_count + + # Check the service bindings. + external_login = model.user.lookup_federated_login(result.to_login, + external_auth.service_id()) + assert external_login is not None + + internal_login = model.user.lookup_federated_login(result.to_login, + internal_auth.federated_service) + assert internal_login is not None diff --git a/oauth/base.py b/oauth/base.py index 0afa418c9..e7a6232c8 100644 --- a/oauth/base.py +++ b/oauth/base.py @@ -64,6 +64,17 @@ class OAuthService(object): def client_secret(self): return self.config.get('CLIENT_SECRET') + def login_binding_field(self): + """ Returns the name of the field (`username` or `email`) used for auto binding an external + login service account to an *internal* login service account. For example, if the external + login service is GitHub and the internal login service is LDAP, a value of `email` here + will cause login-with-Github to conduct a search (via email) in LDAP for a user, an auto + bind the external and internal users together. May return None, in which case no binding + is performing, and login with this external account will simply create a new account in the + database. + """ + return self.config.get('LOGIN_BINDING_FIELD', None) + 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(), From 7b386e9d633158267135b9f81affcc208e80e88f Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 9 Feb 2017 11:33:38 -0800 Subject: [PATCH 3/9] Move endpoint test fixtures to a non-conftest file --- endpoints/api/test/test_security.py | 2 +- endpoints/{api => }/test/__init__.py | 0 endpoints/{api/test/conftest.py => test/fixtures.py} | 0 endpoints/test/test_oauthlogin.py | 2 +- 4 files changed, 2 insertions(+), 2 deletions(-) rename endpoints/{api => }/test/__init__.py (100%) rename endpoints/{api/test/conftest.py => test/fixtures.py} (100%) diff --git a/endpoints/api/test/test_security.py b/endpoints/api/test/test_security.py index 28441d6bd..21475d529 100644 --- a/endpoints/api/test/test_security.py +++ b/endpoints/api/test/test_security.py @@ -1,11 +1,11 @@ import datetime - import pytest from data import model from endpoints.api import api from endpoints.api.superuser import SuperUserRepositoryBuildLogs, SuperUserRepositoryBuildResource from endpoints.api.superuser import SuperUserRepositoryBuildStatus +from endpoints.test.fixtures import app, appconfig, database_uri, init_db_path, sqlitedb_file def client_with_identity(auth_username, client): diff --git a/endpoints/api/test/__init__.py b/endpoints/test/__init__.py similarity index 100% rename from endpoints/api/test/__init__.py rename to endpoints/test/__init__.py diff --git a/endpoints/api/test/conftest.py b/endpoints/test/fixtures.py similarity index 100% rename from endpoints/api/test/conftest.py rename to endpoints/test/fixtures.py diff --git a/endpoints/test/test_oauthlogin.py b/endpoints/test/test_oauthlogin.py index 340e33c1a..9cb365f33 100644 --- a/endpoints/test/test_oauthlogin.py +++ b/endpoints/test/test_oauthlogin.py @@ -1,7 +1,7 @@ import pytest from endpoints.oauthlogin import _conduct_oauth_login -from endpoints.api.test.conftest import app, appconfig, database_uri, init_db_path, sqlitedb_file +from endpoints.test.fixtures import app, appconfig, database_uri, init_db_path, sqlitedb_file from oauth.services.github import GithubOAuthService From d47696b69c40d9a61f8ccee3ec0220e24fdc2ad1 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 9 Feb 2017 12:52:23 -0800 Subject: [PATCH 4/9] Add support for `sub` binding field --- endpoints/oauthlogin.py | 4 +++- endpoints/test/test_oauthlogin.py | 29 ++++++++++++++++++----------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index d5d4ac591..f887fe2ed 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -55,7 +55,9 @@ def _conduct_oauth_login(auth_system, login_service, lid, lusername, lemail, met # Perform lookup. logger.debug('Got oauth bind field name of "%s"', bound_field_name) lookup_value = None - if bound_field_name == 'username': + if bound_field_name == 'sub': + lookup_value = lid + elif bound_field_name == 'username': lookup_value = lusername elif bound_field_name == 'email': lookup_value = lemail diff --git a/endpoints/test/test_oauthlogin.py b/endpoints/test/test_oauthlogin.py index 9cb365f33..0a8ad3632 100644 --- a/endpoints/test/test_oauthlogin.py +++ b/endpoints/test/test_oauthlogin.py @@ -74,34 +74,41 @@ def test_new_account_via_database(login_service): assert federated_login is not None -@pytest.mark.parametrize('binding_field, lusername, lemail, expected_error', [ +@pytest.mark.parametrize('binding_field, lid, lusername, lemail, expected_error', [ # No binding field + newly seen user -> New unlinked user - (None, 'someunknownuser', 'someemail@example.com', None), + (None, 'someid', 'someunknownuser', 'someemail@example.com', None), + + # sub binding field + unknown sub -> Error. + ('sub', 'someid', 'someuser', 'foo@bar.com', + 'sub someid not found in backing auth system'), # username binding field + unknown username -> Error. - ('username', 'someunknownuser', 'foo@bar.com', + ('username', 'someid', 'someunknownuser', 'foo@bar.com', 'username someunknownuser not found in backing auth system'), # email binding field + unknown email address -> Error. - ('email', 'someuser', 'someemail@example.com', + ('email', 'someid', 'someuser', 'someemail@example.com', 'email someemail@example.com not found in backing auth system'), # No binding field + newly seen user -> New unlinked user. - (None, 'someuser', 'foo@bar.com', None), + (None, 'someid', 'someuser', 'foo@bar.com', None), # username binding field + valid username -> fully bound user. - ('username', 'someuser', 'foo@bar.com', None), + ('username', 'someid', 'someuser', 'foo@bar.com', None), + + # sub binding field + valid sub -> fully bound user. + ('sub', 'someuser', 'someusername', 'foo@bar.com', None), # email binding field + valid email -> fully bound user. - ('email', 'someuser', 'foo@bar.com', None), + ('email', 'someid', 'someuser', 'foo@bar.com', None), # username binding field + valid username + invalid email -> fully bound user. - ('username', 'someuser', 'another@email.com', None), + ('username', 'someid', 'someuser', 'another@email.com', None), # email binding field + valid email + invalid username -> fully bound user. - ('email', 'someotherusername', 'foo@bar.com', None), + ('email', 'someid', 'someotherusername', 'foo@bar.com', None), ]) -def test_new_account_via_ldap(binding_field, lusername, lemail, expected_error, app): +def test_new_account_via_ldap(binding_field, lid, lusername, lemail, expected_error, app): existing_user_count = database.User.select().count() config = {'GITHUB': {}} @@ -113,7 +120,7 @@ def test_new_account_via_ldap(binding_field, lusername, lemail, expected_error, with mock_ldap(): # Conduct OAuth login. - result = _conduct_oauth_login(internal_auth, external_auth, 'someid', lusername, lemail) + result = _conduct_oauth_login(internal_auth, external_auth, lid, lusername, lemail) assert result.error_message == expected_error current_user_count = database.User.select().count() From 6736e69ebd3922528e1250c88a79a6bbf6f0f930 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 9 Feb 2017 12:52:33 -0800 Subject: [PATCH 5/9] Add end-to-end OIDC binding test --- test/test_oauth_login.py | 47 ++++++++++++++++++++++++++++++++-------- test/testconfig.py | 1 + 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/test/test_oauth_login.py b/test/test_oauth_login.py index ce5dfcd5f..9d71a74e7 100644 --- a/test/test_oauth_login.py +++ b/test/test_oauth_login.py @@ -9,10 +9,23 @@ from Crypto.PublicKey import RSA from httmock import urlmatch, HTTMock from jwkest.jwk import RSAKey -from app import app +from app import app, authentication from data import model from endpoints.oauthlogin import oauthlogin as oauthlogin_bp from test.test_endpoints import EndpointTestCase +from test.test_ldap import mock_ldap + +class AuthForTesting(object): + def __init__(self, auth_engine): + self.auth_engine = auth_engine + self.existing_state = None + + def __enter__(self): + self.existing_state = authentication.state + authentication.state = self.auth_engine + + def __exit__(self, type, value, traceback): + authentication.state = self.existing_state try: app.register_blueprint(oauthlogin_bp, url_prefix='/oauth2') @@ -22,16 +35,18 @@ except ValueError: class OAuthLoginTestCase(EndpointTestCase): def invoke_oauth_tests(self, callback_endpoint, attach_endpoint, service_name, service_ident, - new_username): + new_username, test_attach=True): # Test callback. created = self.invoke_oauth_test(callback_endpoint, service_name, service_ident, new_username) # Delete the created user. + self.assertNotEquals(created.username, 'devtable') model.user.delete_user(created, []) # Test attach. - self.login('devtable', 'password') - self.invoke_oauth_test(attach_endpoint, service_name, service_ident, 'devtable') + if 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. @@ -111,7 +126,7 @@ class OAuthLoginTestCase(EndpointTestCase): self.invoke_oauth_tests('github_oauth_callback', 'github_oauth_attach', 'github', 'someid', 'someusername') - def test_oidc_auth(self): + def _get_oidc_mocks(self): private_key = RSA.generate(2048) generatedjwk = RSAKey(key=private_key.publickey()).serialize() kid = 'somekey' @@ -123,7 +138,7 @@ class OAuthLoginTestCase(EndpointTestCase): 'nbf': int(time.time()), 'iat': int(time.time()), 'exp': int(time.time() + 600), - 'sub': 'cooluser', + 'sub': 'cool.user', } token_headers = { @@ -143,7 +158,7 @@ class OAuthLoginTestCase(EndpointTestCase): @urlmatch(netloc=r'fakeoidc', path='/user') def user_handler(_, __): content = { - 'sub': 'cooluser', + 'sub': 'cool.user', 'preferred_username': 'someusername', 'email': 'someemail@example.com', 'email_verified': True, @@ -169,9 +184,23 @@ class OAuthLoginTestCase(EndpointTestCase): } return py_json.dumps(content) - with HTTMock(discovery_handler, jwks_handler, token_handler, user_handler): + return (discovery_handler, jwks_handler, token_handler, user_handler) + + def test_oidc_database_auth(self): + oidc_mocks = self._get_oidc_mocks() + with HTTMock(*oidc_mocks): self.invoke_oauth_tests('testoidc_oauth_callback', 'testoidc_oauth_attach', 'testoidc', - 'cooluser', 'someusername') + 'cool.user', 'someusername') + + def test_oidc_ldap_auth(self): + # Test with database auth. + oidc_mocks = self._get_oidc_mocks() + with mock_ldap() as ldap: + with AuthForTesting(ldap): + with HTTMock(*oidc_mocks): + self.invoke_oauth_tests('testoidc_oauth_callback', 'testoidc_oauth_attach', 'testoidc', + 'cool.user', 'cool_user', test_attach=False) + if __name__ == '__main__': unittest.main() diff --git a/test/testconfig.py b/test/testconfig.py index e9d6c03db..ae7af6f93 100644 --- a/test/testconfig.py +++ b/test/testconfig.py @@ -87,6 +87,7 @@ class TestConfig(DefaultConfig): 'CLIENT_SECRET': 'bar', 'OIDC_SERVER': 'http://fakeoidc', 'DEBUGGING': True, + 'LOGIN_BINDING_FIELD': 'sub', } RECAPTCHA_SITE_KEY = 'somekey' From 421c5d6012272aea57f13759ce478da4a5a05321 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 9 Feb 2017 12:52:44 -0800 Subject: [PATCH 6/9] Fix bug where the login service ID doesn't exist --- data/model/user.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/data/model/user.py b/data/model/user.py index 8c7d28ae9..28ad75174 100644 --- a/data/model/user.py +++ b/data/model/user.py @@ -370,6 +370,13 @@ def update_user_metadata(user, given_name=None, family_name=None, company=None): remove_user_prompt(user, UserPromptTypes.ENTER_COMPANY) +def _get_login_service(service_id): + try: + return LoginService.get(LoginService.name == service_id) + except LoginService.DoesNotExist: + return LoginService.create(name=service_id) + + def create_federated_user(username, email, service_id, service_ident, set_password_notification, metadata={}, email_required=True, prompts=tuple()): @@ -380,12 +387,7 @@ def create_federated_user(username, email, service_id, service_ident, new_user.verified = True new_user.save() - try: - service = LoginService.get(LoginService.name == service_id) - except LoginService.DoesNotExist: - service = LoginService.create(name=service_id) - - FederatedLogin.create(user=new_user, service=service, + FederatedLogin.create(user=new_user, service=_get_login_service(service_id), service_ident=service_ident, metadata_json=json.dumps(metadata)) @@ -395,10 +397,10 @@ def create_federated_user(username, email, service_id, service_ident, return new_user -def attach_federated_login(user, service_id, service_ident, metadata={}): - service = LoginService.get(LoginService.name == service_id) +def attach_federated_login(user, service_id, service_ident, metadata=None): + service = _get_login_service(service_id) FederatedLogin.create(user=user, service=service, service_ident=service_ident, - metadata_json=json.dumps(metadata)) + metadata_json=json.dumps(metadata or {})) return user From cc4258c01502f6f580965ceea8c51077792c083b Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 9 Feb 2017 12:57:09 -0800 Subject: [PATCH 7/9] Blacklist any OIDC service ids that may conflict with our own --- oauth/loginmanager.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/oauth/loginmanager.py b/oauth/loginmanager.py index e65e100eb..f1e78357e 100644 --- a/oauth/loginmanager.py +++ b/oauth/loginmanager.py @@ -1,12 +1,15 @@ from oauth.services.github import GithubOAuthService from oauth.services.google import GoogleOAuthService from oauth.oidc import OIDCLoginService +from data.users import UserAuthentication CUSTOM_LOGIN_SERVICES = { 'GITHUB_LOGIN_CONFIG': GithubOAuthService, 'GOOGLE_LOGIN_CONFIG': GoogleOAuthService, } +PREFIX_BLACKLIST = ['ldap', 'jwt', 'keystone'] + class OAuthLoginManager(object): """ Helper class which manages all registered OAuth login services. """ def __init__(self, config): @@ -21,6 +24,10 @@ class OAuthLoginManager(object): if custom_service.login_enabled(config): self.services.append(custom_service) else: + prefix = key[0:len(key) - len('_LOGIN_CONFIG')].lower() + if prefix in PREFIX_BLACKLIST: + raise Exception('Cannot use reserved config name %s' % key) + self.services.append(OIDCLoginService(config, key)) def get_service(self, service_id): From 0167e1e7bf69c568c447cd5341048b88a6517299 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 16 Feb 2017 14:50:44 -0500 Subject: [PATCH 8/9] Style fixes --- endpoints/api/test/test_security.py | 1 + endpoints/oauthlogin.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/endpoints/api/test/test_security.py b/endpoints/api/test/test_security.py index 21475d529..2d2babbe7 100644 --- a/endpoints/api/test/test_security.py +++ b/endpoints/api/test/test_security.py @@ -1,4 +1,5 @@ import datetime + import pytest from data import model diff --git a/endpoints/oauthlogin.py b/endpoints/oauthlogin.py index f887fe2ed..ddecdb73d 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauthlogin.py @@ -151,7 +151,7 @@ def _attach_service(login_service, user_obj, lid, lusername): service username. """ metadata = { - 'service_username': lusername + 'service_username': lusername, } try: @@ -183,7 +183,7 @@ def _register_service(login_service): # Conduct login. metadata = { - 'service_username': lusername + 'service_username': lusername, } result = _conduct_oauth_login(authentication, login_service, lid, lusername, lemail, From 198bdf88bc0cead90e390ee837103a5d6ec8002e Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 16 Feb 2017 15:56:16 -0500 Subject: [PATCH 9/9] Move OAuth login into its own endpoints module --- endpoints/oauth/__init__.py | 0 endpoints/{oauthlogin.py => oauth/login.py} | 39 ++++++++++--------- .../test/test_login.py} | 18 ++++----- oauth/loginmanager.py | 2 +- test/test_oauth_login.py | 2 +- web.py | 2 +- 6 files changed, 32 insertions(+), 31 deletions(-) create mode 100644 endpoints/oauth/__init__.py rename endpoints/{oauthlogin.py => oauth/login.py} (87%) rename endpoints/{test/test_oauthlogin.py => oauth/test/test_login.py} (94%) diff --git a/endpoints/oauth/__init__.py b/endpoints/oauth/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/endpoints/oauthlogin.py b/endpoints/oauth/login.py similarity index 87% rename from endpoints/oauthlogin.py rename to endpoints/oauth/login.py index ddecdb73d..f1c32917f 100644 --- a/endpoints/oauthlogin.py +++ b/endpoints/oauth/login.py @@ -23,17 +23,17 @@ oauthlogin = Blueprint('oauthlogin', __name__) oauthlogin_csrf_protect = csrf_protect(OAUTH_CSRF_TOKEN_NAME, 'state', all_methods=True) -OAuthResult = namedtuple('oauthresult', ['to_login', 'service_name', 'error_message', +OAuthResult = namedtuple('oauthresult', ['user_obj', 'service_name', 'error_message', 'register_redirect']) -def _oauthresult(to_login=None, service_name=None, error_message=None, register_redirect=False): - return OAuthResult(to_login, service_name, error_message, register_redirect) +def _oauthresult(user_obj=None, service_name=None, error_message=None, register_redirect=False): + return OAuthResult(user_obj, service_name, error_message, register_redirect) def _get_response(result): if result.error_message is not None: return _render_ologin_error(result.service_name, result.error_message, result.register_redirect) - return _perform_login(result.to_login, result.service_name) + return _perform_login(result.user_obj, result.service_name) def _conduct_oauth_login(auth_system, login_service, lid, lusername, lemail, metadata=None): """ Conducts login from the result of an OAuth service's login flow and returns @@ -43,9 +43,9 @@ def _conduct_oauth_login(auth_system, login_service, lid, lusername, lemail, met # Check for an existing account *bound to this service*. If found, conduct login of that account # and redirect. - to_login = model.user.verify_federated_login(service_id, lid) - if to_login is not None: - return _oauthresult(to_login=to_login, service_name=service_name) + user_obj = model.user.verify_federated_login(service_id, lid) + if user_obj is not None: + return _oauthresult(user_obj=user_obj, service_name=service_name) # If the login service has a bound field name, and we have a defined internal auth type that is # not the database, then search for an existing account with that matching field. This allows @@ -78,7 +78,7 @@ def _conduct_oauth_login(auth_system, login_service, lid, lusername, lemail, met if result.error_message is not None: return result - return _oauthresult(to_login=user_obj, service_name=service_name) + return _oauthresult(user_obj=user_obj, service_name=service_name) # Otherwise, we need to create a new user account. if not features.USER_CREATION: @@ -97,21 +97,22 @@ def _conduct_oauth_login(auth_system, login_service, lid, lusername, lemail, met break prompts = model.user.get_default_user_prompts(features) - to_login = model.user.create_federated_user(new_username, lemail, service_id, lid, + user_obj = model.user.create_federated_user(new_username, lemail, service_id, lid, set_password_notification=True, metadata=metadata or {}, prompts=prompts, email_required=features.MAILING) # Success, tell analytics - analytics.track(to_login.username, 'register', {'service': service_name.lower()}) - return _oauthresult(to_login=to_login, service_name=service_name) + analytics.track(user_obj.username, 'register', {'service': service_name.lower()}) + return _oauthresult(user_obj=user_obj, service_name=service_name) except model.InvalidEmailAddressException: - message = "The e-mail address %s is already associated " % (lemail, ) - message = message + "with an existing %s account." % (app.config['REGISTRY_TITLE_SHORT'], ) - 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, ) + message = ("The e-mail address {0} is already associated " + "with an existing {1} account. \n" + "Please log in with your username and password and " + "associate your {2} account to use it in the future.") + message = message.format(lemail, app.config['REGISTRY_TITLE_SHORT'], service_name) return _oauthresult(service_name=service_name, error_message=message, register_redirect=True) @@ -135,11 +136,11 @@ def _render_ologin_error(service_name, error_message=None, register_redirect=Fal resp.status_code = 400 return resp -def _perform_login(to_login, service_name): +def _perform_login(user_obj, service_name): """ Attempts to login the given user, returning the Flask result of whether the login succeeded. """ - if common_login(to_login): - if model.user.has_user_prompts(to_login): + if common_login(user_obj): + if model.user.has_user_prompts(user_obj): return redirect(url_for('web.updateuser')) else: return redirect(url_for('web.index')) @@ -157,7 +158,7 @@ def _attach_service(login_service, user_obj, lid, lusername): try: model.user.attach_federated_login(user_obj, login_service.service_id(), lid, metadata=metadata) - return _oauthresult(to_login=user_obj) + return _oauthresult(user_obj=user_obj) except IntegrityError: err = '%s account %s is already attached to a %s account' % ( login_service.service_name(), lusername, app.config['REGISTRY_TITLE_SHORT']) diff --git a/endpoints/test/test_oauthlogin.py b/endpoints/oauth/test/test_login.py similarity index 94% rename from endpoints/test/test_oauthlogin.py rename to endpoints/oauth/test/test_login.py index 0a8ad3632..5d97d5955 100644 --- a/endpoints/test/test_oauthlogin.py +++ b/endpoints/oauth/test/test_login.py @@ -1,6 +1,6 @@ import pytest -from endpoints.oauthlogin import _conduct_oauth_login +from endpoints.oauth.login import _conduct_oauth_login from endpoints.test.fixtures import app, appconfig, database_uri, init_db_path, sqlitedb_file from oauth.services.github import GithubOAuthService @@ -47,7 +47,7 @@ def test_existing_account(auth_system, login_service): login_service_lid, login_service_lid, 'example@example.com') - assert result.to_login == created_user + assert result.user_obj == created_user # Ensure that no addtional users were created. current_user_count = database.User.select().count() @@ -63,7 +63,7 @@ def test_new_account_via_database(login_service): # a new user should be created and bound to the external service. result = _conduct_oauth_login(internal_auth, login_service, login_service_lid, login_service_lid, 'example@example.com') - assert result.to_login is not None + assert result.user_obj is not None current_user_count = database.User.select().count() assert current_user_count == existing_user_count + 1 @@ -128,14 +128,14 @@ def test_new_account_via_ldap(binding_field, lid, lusername, lemail, expected_er # Ensure that the new user was created and that it is bound to both the # external login service and to LDAP (if a binding_field was given). assert current_user_count == existing_user_count + 1 - assert result.to_login is not None + assert result.user_obj is not None # Check the service bindings. - external_login = model.user.lookup_federated_login(result.to_login, + external_login = model.user.lookup_federated_login(result.user_obj, external_auth.service_id()) assert external_login is not None - internal_login = model.user.lookup_federated_login(result.to_login, + internal_login = model.user.lookup_federated_login(result.user_obj, internal_auth.federated_service) if binding_field is not None: assert internal_login is not None @@ -168,17 +168,17 @@ def test_existing_account_in_ldap(app): # Ensure that the same user was returned, and that it is now bound to the Github account # as well. - assert result.to_login.id == bound_user.id + assert result.user_obj.id == bound_user.id # Ensure that no additional users were created. current_user_count = database.User.select().count() assert current_user_count == existing_user_count # Check the service bindings. - external_login = model.user.lookup_federated_login(result.to_login, + external_login = model.user.lookup_federated_login(result.user_obj, external_auth.service_id()) assert external_login is not None - internal_login = model.user.lookup_federated_login(result.to_login, + internal_login = model.user.lookup_federated_login(result.user_obj, internal_auth.federated_service) assert internal_login is not None diff --git a/oauth/loginmanager.py b/oauth/loginmanager.py index f1e78357e..457fc2343 100644 --- a/oauth/loginmanager.py +++ b/oauth/loginmanager.py @@ -24,7 +24,7 @@ class OAuthLoginManager(object): if custom_service.login_enabled(config): self.services.append(custom_service) else: - prefix = key[0:len(key) - len('_LOGIN_CONFIG')].lower() + prefix = key.rstrip('_LOGIN_CONFIG').lower() if prefix in PREFIX_BLACKLIST: raise Exception('Cannot use reserved config name %s' % key) diff --git a/test/test_oauth_login.py b/test/test_oauth_login.py index 9d71a74e7..7496a059e 100644 --- a/test/test_oauth_login.py +++ b/test/test_oauth_login.py @@ -11,7 +11,7 @@ from jwkest.jwk import RSAKey from app import app, authentication from data import model -from endpoints.oauthlogin import oauthlogin as oauthlogin_bp +from endpoints.oauth.login import oauthlogin as oauthlogin_bp from test.test_endpoints import EndpointTestCase from test.test_ldap import mock_ldap diff --git a/web.py b/web.py index 237829c0f..c07d1eea2 100644 --- a/web.py +++ b/web.py @@ -8,7 +8,7 @@ from endpoints.bitbuckettrigger import bitbuckettrigger from endpoints.githubtrigger import githubtrigger from endpoints.gitlabtrigger import gitlabtrigger from endpoints.keyserver import key_server -from endpoints.oauthlogin import oauthlogin +from endpoints.oauth.login import oauthlogin from endpoints.realtime import realtime from endpoints.web import web from endpoints.webhooks import webhooks