From c5a9a1b8af93cad57e34026b643a65a4a884c252 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Mar 07 2019 08:37:14 +0000 Subject: Allow `auth.get_user()` method to be called multiple times. The `ImportModuleAPI` calls the `auth.get_user()` which auths the user using Kerberos. the `ImportModuleAPI` later calls `SCMHandler` which in its `__init__` method calls the `auth.get_user()` again. This leads to traceback in GSSAPI, because the user is already authed. This commit fixes this by caching the auth results in `flask.g`, which is reset after each request based on the Note in http://flask.pocoo.org/docs/1.0/appcontext/#storing-data. This commit also marks mutual auth as OPTIONAL in `mbs-cli`, because MBS server currently does not do mutual auth. --- diff --git a/client/mbs-cli b/client/mbs-cli index 02ded1c..63e0eb4 100755 --- a/client/mbs-cli +++ b/client/mbs-cli @@ -34,7 +34,7 @@ import sys import openidc_client import requests.exceptions from six.moves import urllib_parse -from requests_kerberos import HTTPKerberosAuth +import requests_kerberos env_config = { @@ -160,7 +160,9 @@ class MBSCli(object): if self._auth_mech == AuthMech.OpenIDC: headers['Authorization'] = 'Bearer {0}'.format(self._openidc_token) elif self._auth_mech == AuthMech.Kerberos: - request_data['auth'] = HTTPKerberosAuth() + # MBS server does not support mutual auth, so make it optional. + request_data['auth'] = requests_kerberos.HTTPKerberosAuth( + mutual_authentication=requests_kerberos.OPTIONAL) if headers: request_data['headers'] = headers diff --git a/module_build_service/auth.py b/module_build_service/auth.py index 94c7046..6c6b038 100644 --- a/module_build_service/auth.py +++ b/module_build_service/auth.py @@ -30,7 +30,7 @@ import ssl import requests import kerberos -from flask import Response +from flask import Response, g # Starting with Flask 0.9, the _app_ctx_stack is the correct one, # before that we need to use the _request_ctx_stack. try: @@ -325,8 +325,10 @@ def get_user(request): log.debug('Authorization is disabled.') return 'anonymous', {'packager'} - get_user_func_name = 'get_user_{0}'.format(conf.auth_method) - get_user_func = globals().get(get_user_func_name) - if not get_user_func: - raise RuntimeError('The function "{0}" is not implemented'.format(get_user_func_name)) - return get_user_func(request) + if "user" not in g and "groups" not in g: + get_user_func_name = 'get_user_{0}'.format(conf.auth_method) + get_user_func = globals().get(get_user_func_name) + if not get_user_func: + raise RuntimeError('The function "{0}" is not implemented'.format(get_user_func_name)) + g.user, g.groups = get_user_func(request) + return g.user, g.groups diff --git a/tests/test_auth.py b/tests/test_auth.py index c1493b7..c2270c8 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -32,6 +32,7 @@ from werkzeug.exceptions import Unauthorized as FlaskUnauthorized import module_build_service.auth import module_build_service.errors import module_build_service.config as mbs_config +from module_build_service import app class TestAuthModule: @@ -44,7 +45,8 @@ class TestAuthModule: request.cookies.return_value = {} with pytest.raises(module_build_service.errors.Unauthorized) as cm: - module_build_service.auth.get_user(request) + with app.app_context(): + module_build_service.auth.get_user(request) assert str(cm.value) == "No 'authorization' header found." @patch('module_build_service.auth._get_token_info') @@ -71,7 +73,8 @@ class TestAuthModule: request.headers.__contains__.side_effect = headers.__contains__ with pytest.raises(module_build_service.errors.Unauthorized) as cm: - module_build_service.auth.get_user(request) + with app.app_context(): + module_build_service.auth.get_user(request) assert str(cm.value) == "OIDC token invalid or expired." @pytest.mark.parametrize('allowed_users', (set(), set(['Joey Jo Jo Junior Shabadoo']))) @@ -100,13 +103,21 @@ class TestAuthModule: request.headers.__setitem__.side_effect = headers.__setitem__ request.headers.__contains__.side_effect = headers.__contains__ - username, groups = module_build_service.auth.get_user(request) + with app.app_context(): + username, groups = module_build_service.auth.get_user(request) + username_second_call, groups_second_call = module_build_service.auth.get_user( + request) assert username == name if allowed_users: assert groups == set() else: assert groups == set(get_user_info.return_value["groups"]) + # Test the real auth method has been called just once. + get_user_info.assert_called_once() + assert username_second_call == username + assert groups_second_call == groups + @patch.object(mbs_config.Config, 'no_auth', new_callable=PropertyMock, return_value=True) def test_disable_authentication(self, conf_no_auth): request = mock.MagicMock() @@ -118,7 +129,8 @@ class TestAuthModule: def test_misconfiguring_oidc_client_secrets_should_be_failed(self): request = mock.MagicMock() with pytest.raises(module_build_service.errors.Forbidden) as cm: - module_build_service.auth.get_user(request) + with app.app_context(): + module_build_service.auth.get_user(request) assert str(cm.value) == "OIDC_CLIENT_SECRETS must be set in server config." @patch('module_build_service.auth._get_token_info') @@ -145,7 +157,8 @@ class TestAuthModule: request.headers.__contains__.side_effect = headers.__contains__ with pytest.raises(module_build_service.errors.Unauthorized) as cm: - module_build_service.auth.get_user(request) + with app.app_context(): + module_build_service.auth.get_user(request) assert str(cm.value) == ("Required OIDC scope 'mbs-scope' not present: " "['openid', 'https://id.fedoraproject.org/scope/groups']") @@ -172,7 +185,8 @@ class TestAuthModule: request.headers.__contains__.side_effect = headers.__contains__ with pytest.raises(module_build_service.errors.Forbidden) as cm: - module_build_service.auth.get_user(request) + with app.app_context(): + module_build_service.auth.get_user(request) assert str(cm.value) == "OIDC_REQUIRED_SCOPE must be set in server config."