PR#1445: Raise Unathorized exception instead of general one whenever OIDC auth fails - fm-orchestrator

fm-orchestrator

#1445 Raise Unathorized exception instead of general one whenever OIDC auth fails

Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.

vmaljulin/fm-orchestrator FACTORY-3904 into master

Raise Unathorized exception instead of general one whenever OIDC auth fails

Valerij Maljulin • 4 years ago

2c02919

module_build_service/auth.py

file modified

+7 -7

		`@@ -121,10 +121,10 @@`

		`try:`
		`extended_data = _get_user_info(token)`
		`- except Exception as e:`
		`- error = "Cannot verify determine user groups: %s" % str(e)`
		`+ except Exception:`
		`+ error = "OpenIDC auth error: Cannot determine the user's groups"`
		`log.exception(error)`
		`- raise Exception(error)`
		`+ raise Unauthorized(error)`

		`username = data["username"]`
		`# If the user is part of the whitelist, then the group membership check is skipped`
		`@@ -133,10 +133,10 @@`
		`else:`
		`try:`
		`groups = set(extended_data["groups"])`
		`- except Exception as e:`
		`- error = "Could not find groups in UserInfo from OIDC %s" % str(e)`
		`- log.exception(extended_data)`
		`- raise Exception(error)`
		`+ except Exception:`
		`+ error = "Could not find groups in UserInfo from OIDC"`
		`+ log.exception("%s (extended_data: %s)", error, extended_data)`
		`+ raise Unauthorized(error)`

		`return username, groups`

tests/test_auth.py

file modified

+34 -1

		`@@ -3,6 +3,7 @@`
		`from os import path, environ`

		`import pytest`
		`+ import requests`
		`import mock`
		`from mock import patch, PropertyMock, Mock`
		`import kerberos`
		`@@ -45,7 +46,7 @@`
		`mocked_get_token_info = {`
		`"active": False,`
		`"username": name,`
		`- "scope": ("openid https://id.fedoraproject.org/scope/groups mbs-scope"),`
		`+ "scope": "openid https://id.fedoraproject.org/scope/groups mbs-scope"`
		`}`
		`get_token_info.return_value = mocked_get_token_info`

		`@@ -63,6 +64,38 @@`
		`module_build_service.auth.get_user(request)`
		`assert str(cm.value) == "OIDC token invalid or expired."`

		`+ @patch("module_build_service.auth._get_token_info")`
		`+ @patch("module_build_service.auth._get_user_info")`
		`+ def test_get_user_not_in_groups(self, get_user_info, get_token_info):`
		`+ base_dir = path.abspath(path.dirname(__file__))`
		`+ client_secrets = path.join(base_dir, "client_secrets.json")`
		`+ with patch.dict(`
		`+ "module_build_service.app.config",`
		`+ {"OIDC_CLIENT_SECRETS": client_secrets, "OIDC_REQUIRED_SCOPE": "mbs-scope"},`
		`+ ):`
		`+ # https://www.youtube.com/watch?v=G-LtddOgUCE`
		`+ name = "Joey Jo Jo Junior Shabadoo"`
		`+ mocked_get_token_info = {`
		`+ "active": True,`
		`+ "username": name,`
		`+ "scope": "openid https://id.fedoraproject.org/scope/groups mbs-scope"`
		`+ }`
		`+ get_token_info.return_value = mocked_get_token_info`
		`+`
		`+ get_user_info.side_effect = requests.Timeout("It happens...")`
		`+`
		`+ headers = {"authorization": "Bearer foobar"}`
		`+ request = mock.MagicMock()`
		`+ request.headers.return_value = mock.MagicMock(spec_set=dict)`
		`+ request.headers.__getitem__.side_effect = headers.__getitem__`
		`+ request.headers.__setitem__.side_effect = headers.__setitem__`
		`+ request.headers.__contains__.side_effect = headers.__contains__`
		`+`
		`+ with pytest.raises(module_build_service.errors.Unauthorized) as cm:`
		`+ with app.app_context():`
		`+ module_build_service.auth.get_user(request)`
		`+ assert str(cm.value) == "OpenIDC auth error: Cannot determine the user's groups"`
		`+`
		`@pytest.mark.parametrize("allowed_users", (set(), {"Joey Jo Jo Junior Shabadoo"}))`
		`@patch.object(mbs_config.Config, "allowed_users", new_callable=PropertyMock)`
		`@patch("module_build_service.auth._get_token_info")`

vmaljulin commented 4 years ago

This fixes #1147

mprahl commented on line 9 of module_build_service/auth.py 4 years ago

Could you make this log.exception(error) instead? Using log.exception will automatically show the traceback and the original error message, so there's no need to duplicate it.