From 60bbca218524fc1e06e15d643a5c813cfb1c4daf Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 23 May 2016 15:08:51 -0400 Subject: [PATCH] Fix setup tool when binding to external auth We now query the external auth provider for the external service's identifier before adding the linking row into the database. This fixes the case where the external service resolves a different identifier for the same username. Fixes #1477 --- data/model/user.py | 21 ++--- data/users/__init__.py | 81 ++++++++++--------- endpoints/api/suconfig.py | 26 ++++-- .../directives/config/config-setup-tool.html | 7 +- static/js/core-config-setup.js | 32 +++++++- test/test_api_usage.py | 46 +++++++++++ 6 files changed, 151 insertions(+), 62 deletions(-) diff --git a/data/model/user.py b/data/model/user.py index 29eb51f85..28620b5a1 100644 --- a/data/model/user.py +++ b/data/model/user.py @@ -305,16 +305,7 @@ def list_entity_robot_permission_teams(entity_name, include_permissions=False): return TupleSelector(query, fields) -def confirm_attached_federated_login(user, service_name): - """ Verifies that the given user has a federated service identity for the specified service. - If none found, a row is added for that service and user. - """ - with db_transaction(): - if not lookup_federated_login(user, service_name): - attach_federated_login(user, service_name, user.username) - - -def create_federated_user(username, email, service_name, service_id, +def create_federated_user(username, email, service_name, service_ident, set_password_notification, metadata={}): if not is_create_user_allowed(): raise TooManyUsersException() @@ -325,7 +316,7 @@ def create_federated_user(username, email, service_name, service_id, service = LoginService.get(LoginService.name == service_name) FederatedLogin.create(user=new_user, service=service, - service_ident=service_id, + service_ident=service_ident, metadata_json=json.dumps(metadata)) if set_password_notification: @@ -334,20 +325,20 @@ def create_federated_user(username, email, service_name, service_id, return new_user -def attach_federated_login(user, service_name, service_id, metadata={}): +def attach_federated_login(user, service_name, service_ident, metadata={}): service = LoginService.get(LoginService.name == service_name) - FederatedLogin.create(user=user, service=service, service_ident=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_id): +def verify_federated_login(service_name, service_ident): try: found = (FederatedLogin .select(FederatedLogin, User) .join(LoginService) .switch(FederatedLogin).join(User) - .where(FederatedLogin.service_ident == service_id, LoginService.name == service_name) + .where(FederatedLogin.service_ident == service_ident, LoginService.name == service_name) .get()) return found.user except FederatedLogin.DoesNotExist: diff --git a/data/users/__init__.py b/data/users/__init__.py index bcdd81912..4c1f2ba4c 100644 --- a/data/users/__init__.py +++ b/data/users/__init__.py @@ -29,6 +29,48 @@ def get_federated_service_name(authentication_type): LDAP_CERT_FILENAME = 'ldap.crt' +def get_users_handler(config, config_provider, override_config_dir): + """ Returns a users handler for the authentication configured in the given config object. """ + authentication_type = config.get('AUTHENTICATION_TYPE', 'Database') + + if authentication_type == 'Database': + return DatabaseUsers() + + if authentication_type == 'LDAP': + ldap_uri = config.get('LDAP_URI', 'ldap://localhost') + base_dn = config.get('LDAP_BASE_DN') + admin_dn = config.get('LDAP_ADMIN_DN') + admin_passwd = config.get('LDAP_ADMIN_PASSWD') + user_rdn = config.get('LDAP_USER_RDN', []) + uid_attr = config.get('LDAP_UID_ATTR', 'uid') + email_attr = config.get('LDAP_EMAIL_ATTR', 'mail') + + allow_tls_fallback = config.get('LDAP_ALLOW_INSECURE_FALLBACK', False) + tls_cert_path = None + if config_provider.volume_file_exists(LDAP_CERT_FILENAME): + with config_provider.get_volume_file(LDAP_CERT_FILENAME) as f: + tls_cert_path = f.name + + return LDAPUsers(ldap_uri, base_dn, admin_dn, admin_passwd, user_rdn, uid_attr, email_attr, + tls_cert_path, allow_tls_fallback) + + if authentication_type == 'JWT': + verify_url = config.get('JWT_VERIFY_ENDPOINT') + issuer = config.get('JWT_AUTH_ISSUER') + max_fresh_s = config.get('JWT_AUTH_MAX_FRESH_S', 300) + return ExternalJWTAuthN(verify_url, issuer, override_config_dir, config['HTTPCLIENT'], + max_fresh_s) + + if authentication_type == 'Keystone': + auth_url = config.get('KEYSTONE_AUTH_URL') + keystone_admin_username = config.get('KEYSTONE_ADMIN_USERNAME') + keystone_admin_password = config.get('KEYSTONE_ADMIN_PASSWORD') + keystone_admin_tenant = config.get('KEYSTONE_ADMIN_TENANT') + return KeystoneUsers(auth_url, keystone_admin_username, keystone_admin_password, + keystone_admin_tenant) + + raise RuntimeError('Unknown authentication type: %s' % authentication_type) + class UserAuthentication(object): def __init__(self, app=None, config_provider=None, override_config_dir=None): self.app_secret_key = None @@ -40,44 +82,7 @@ class UserAuthentication(object): def init_app(self, app, config_provider, override_config_dir): self.app_secret_key = app.config['SECRET_KEY'] - - authentication_type = app.config.get('AUTHENTICATION_TYPE', 'Database') - - if authentication_type == 'Database': - users = DatabaseUsers() - elif authentication_type == 'LDAP': - ldap_uri = app.config.get('LDAP_URI', 'ldap://localhost') - base_dn = app.config.get('LDAP_BASE_DN') - admin_dn = app.config.get('LDAP_ADMIN_DN') - admin_passwd = app.config.get('LDAP_ADMIN_PASSWD') - user_rdn = app.config.get('LDAP_USER_RDN', []) - uid_attr = app.config.get('LDAP_UID_ATTR', 'uid') - email_attr = app.config.get('LDAP_EMAIL_ATTR', 'mail') - - allow_tls_fallback = app.config.get('LDAP_ALLOW_INSECURE_FALLBACK', False) - tls_cert_path = None - if config_provider.volume_file_exists(LDAP_CERT_FILENAME): - with config_provider.get_volume_file(LDAP_CERT_FILENAME) as f: - tls_cert_path = f.name - - users = LDAPUsers(ldap_uri, base_dn, admin_dn, admin_passwd, user_rdn, uid_attr, email_attr, - tls_cert_path, allow_tls_fallback) - - elif authentication_type == 'JWT': - verify_url = app.config.get('JWT_VERIFY_ENDPOINT') - issuer = app.config.get('JWT_AUTH_ISSUER') - max_fresh_s = app.config.get('JWT_AUTH_MAX_FRESH_S', 300) - users = ExternalJWTAuthN(verify_url, issuer, override_config_dir, - app.config['HTTPCLIENT'], max_fresh_s) - elif authentication_type == 'Keystone': - auth_url = app.config.get('KEYSTONE_AUTH_URL') - keystone_admin_username = app.config.get('KEYSTONE_ADMIN_USERNAME') - keystone_admin_password = app.config.get('KEYSTONE_ADMIN_PASSWORD') - keystone_admin_tenant = app.config.get('KEYSTONE_ADMIN_TENANT') - users = KeystoneUsers(auth_url, keystone_admin_username, keystone_admin_password, - keystone_admin_tenant) - else: - raise RuntimeError('Unknown authentication type: %s' % authentication_type) + users = get_users_handler(app.config, config_provider, override_config_dir) # register extension with app app.extensions = getattr(app, 'extensions', {}) diff --git a/endpoints/api/suconfig.py b/endpoints/api/suconfig.py index 5a5cd9346..502c048d4 100644 --- a/endpoints/api/suconfig.py +++ b/endpoints/api/suconfig.py @@ -9,7 +9,7 @@ from endpoints.api import (ApiResource, nickname, resource, internal_only, show_ require_fresh_login, request, validate_json_request, verify_not_prod) from endpoints.common import common_login -from app import app, config_provider, superusers +from app import app, config_provider, superusers, OVERRIDE_CONFIG_DIRECTORY from data import model from data.database import configure from auth.permissions import SuperUserPermission @@ -19,7 +19,7 @@ from util.config.configutil import add_enterprise_config_defaults from util.config.database import sync_database_with_config from util.config.validator import validate_service_for_config, CONFIG_FILENAMES from data.runmigration import run_alembic_migration -from data.users import get_federated_service_name +from data.users import get_federated_service_name, get_users_handler import features @@ -175,7 +175,10 @@ class SuperUserConfig(ApiResource): }, 'hostname': { 'type': 'string' - } + }, + 'password': { + 'type': 'string' + }, }, }, } @@ -213,9 +216,22 @@ class SuperUserConfig(ApiResource): # If the authentication system is not the database, link the superuser account to the # the authentication system chosen. if config_object.get('AUTHENTICATION_TYPE', 'Database') != 'Database': - service_name = get_federated_service_name(config_object['AUTHENTICATION_TYPE']) current_user = get_authenticated_user() - model.user.confirm_attached_federated_login(current_user, service_name) + if current_user is None: + abort(401) + + service_name = get_federated_service_name(config_object['AUTHENTICATION_TYPE']) + if not model.user.lookup_federated_login(current_user, service_name): + # Verify the user's credentials and retrieve the user's external username+email. + handler = get_users_handler(config_object, config_provider, OVERRIDE_CONFIG_DIRECTORY) + (result, err_msg) = handler.verify_credentials(current_user.username, + request.get_json().get('password', '')) + if not result: + logger.error('Could not save configuration due to external auth failure: %s', err_msg) + abort(400) + + # Link the existing user to the external user. + model.user.attach_federated_login(current_user, service_name, result.username) # Ensure database is up-to-date with config sync_database_with_config(config_object) diff --git a/static/directives/config/config-setup-tool.html b/static/directives/config/config-setup-tool.html index 0f46a77eb..8e07f1521 100644 --- a/static/directives/config/config-setup-tool.html +++ b/static/directives/config/config-setup-tool.html @@ -1022,7 +1022,12 @@ diff --git a/static/js/core-config-setup.js b/static/js/core-config-setup.js index 384e8c878..51ba81a48 100644 --- a/static/js/core-config-setup.js +++ b/static/js/core-config-setup.js @@ -11,6 +11,8 @@ angular.module("core-config-setup", ['angularFileUpload']) 'configurationSaved': '&configurationSaved' }, controller: function($rootScope, $scope, $element, $timeout, ApiService) { + var authPassword = null; + $scope.HOSTNAME_REGEX = '^[a-zA-Z-0-9\.]+(:[0-9]+)?$'; $scope.GITHOST_REGEX = '^https?://([a-zA-Z0-9]+\.?\/?)+$'; @@ -188,10 +190,21 @@ angular.module("core-config-setup", ['angularFileUpload']) 'password': opt_password || '' }; + var errorDisplay = ApiService.errorDisplay( + 'Could not validate configuration. Please report this error.', + function() { + authPassword = null; + }); + ApiService.scValidateConfig(data, params).then(function(resp) { serviceInfo.status = resp.status ? 'success' : 'error'; serviceInfo.errorMessage = $.trim(resp.reason || ''); - }, ApiService.errorDisplay('Could not validate configuration. Please report this error.')); + + if (!resp.status) { + authPassword = null; + } + + }, errorDisplay); }; $scope.checkValidateAndSave = function() { @@ -262,6 +275,8 @@ angular.module("core-config-setup", ['angularFileUpload']) $scope.savingConfiguration = false; $scope.validating = $scope.getServices($scope.config); + authPassword = opt_password; + $('#validateAndSaveModal').modal({ keyboard: false, backdrop: 'static' @@ -282,15 +297,26 @@ angular.module("core-config-setup", ['angularFileUpload']) var data = { 'config': $scope.config, - 'hostname': window.location.host + 'hostname': window.location.host, + 'password': authPassword || '' }; + var errorDisplay = ApiService.errorDisplay( + 'Could not save configuration. Please report this error.', + function() { + authPassword = null; + }); + ApiService.scUpdateConfig(data).then(function(resp) { + authPassword = null; + $scope.savingConfiguration = false; $scope.mapped.$hasChanges = false; + $('#validateAndSaveModal').modal('hide'); + $scope.configurationSaved({'config': $scope.config}); - }, ApiService.errorDisplay('Could not save configuration. Please report this error.')); + }, errorDisplay); }; // Convert storage config to an array diff --git a/test/test_api_usage.py b/test/test_api_usage.py index 2aefabd02..b3c5f3a87 100644 --- a/test/test_api_usage.py +++ b/test/test_api_usage.py @@ -15,6 +15,7 @@ from playhouse.test_utils import assert_query_count, _QueryLogHandler from httmock import urlmatch, HTTMock from cryptography.hazmat.primitives import serialization from cryptography.hazmat.backends import default_backend +from mockldap import MockLdap from endpoints.api import api_bp, api from endpoints.building import PreparedBuild @@ -3523,6 +3524,51 @@ class TestSuperUserConfig(ApiTestCase): json = self.getJsonResponse(SuperUserConfigFile, params=dict(filename='ssl.cert')) self.assertTrue(json['exists']) + def test_update_with_external_auth(self): + self.login(ADMIN_ACCESS_USER) + + # Run a mock LDAP. + mockldap = MockLdap({ + 'dc=quay,dc=io': {'dc': ['quay', 'io']}, + 'ou=employees,dc=quay,dc=io': { + 'dc': ['quay', 'io'], + 'ou': 'employees' + }, + 'uid=' + ADMIN_ACCESS_USER + ',ou=employees,dc=quay,dc=io': { + 'dc': ['quay', 'io'], + 'ou': 'employees', + 'uid': [ADMIN_ACCESS_USER], + 'userPassword': ['password'], + 'mail': [ADMIN_ACCESS_EMAIL], + }, + }) + + config = { + 'AUTHENTICATION_TYPE': 'LDAP', + 'LDAP_BASE_DN': ['dc=quay', 'dc=io'], + 'LDAP_ADMIN_DN': 'uid=devtable,ou=employees,dc=quay,dc=io', + 'LDAP_ADMIN_PASSWD': 'password', + 'LDAP_USER_RDN': ['ou=employees'], + 'LDAP_UID_ATTR': 'uid', + 'LDAP_EMAIL_ATTR': 'mail', + } + + mockldap.start() + try: + # Try writing some config with an invalid password. + self.putResponse(SuperUserConfig, data={'config': config, 'hostname': 'foo'}, expected_code=400) + self.putResponse(SuperUserConfig, + data={'config': config, 'password': 'invalid', 'hostname': 'foo'}, expected_code=400) + + # Write the config with the valid password. + self.putResponse(SuperUserConfig, + data={'config': config, 'password': 'password', 'hostname': 'foo'}, expected_code=200) + + # Ensure that the user row has been linked. + self.assertEquals(ADMIN_ACCESS_USER, model.user.verify_federated_login('ldap', ADMIN_ACCESS_USER).username) + finally: + mockldap.stop() + @urlmatch(netloc=r'(.*\.)?mockclairservice', path=r'/v1/layers/(.+)')