diff --git a/.gitignore b/.gitignore index 524ca86a..bcb2c491 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ test_module_build_service.db .mbs_local_build.db *.src.rpm .test_module_build_service.db +.eggs diff --git a/conf/client_secrets.json b/conf/client_secrets.json index 7fd5069d..8647ed86 100644 --- a/conf/client_secrets.json +++ b/conf/client_secrets.json @@ -7,6 +7,7 @@ "http://localhost:13747/" ], "token_uri": "https://id.stg.fedoraproject.org/openidc/Token", - "token_introspection_uri": "https://id.stg.fedoraproject.org/openidc/TokenInfo" + "token_introspection_uri": "https://id.stg.fedoraproject.org/openidc/TokenInfo", + "userinfo_uri": "https://id.stg.fedoraproject.org/openidc/UserInfo" } } diff --git a/conf/config.py b/conf/config.py index 97eed6e0..7521b4f2 100644 --- a/conf/config.py +++ b/conf/config.py @@ -57,8 +57,10 @@ class BaseConfiguration(object): PKGDB_API_URL = 'https://admin.stg.fedoraproject.org/pkgdb/api' - FAS_URL = 'https://admin.stg.fedoraproject.org/accounts' - REQUIRE_PACKAGER = True + ALLOWED_GROUPS = set([ + 'packager', + #'modularity-wg', + ]) # Available backends are: console, file, journal. LOG_BACKEND = 'journal' @@ -109,14 +111,6 @@ class DevConfiguration(BaseConfiguration): # user. See: https://infrastructure.fedoraproject.org/cgit/ansible.git/commit/?id=a28a93dad75248c30c1792ec35f588c8e317c067 KOJI_PROXYUSER = False - REQUIRE_PACKAGER = False - # You only need these FAS options if you turn on authorization - # with REQUIRE_PACKAGER=True - # FAS_USERNAME = 'put your fas username here' - # FAS_PASSWORD = 'put your fas password here....' - # FAS_PASSWORD = os.environ('FAS_PASSWORD') # you could store it here - # FAS_PASSWORD = commands.getoutput('pass your_fas_password').strip() - KOJI_CONFIG = path.join(confdir, 'koji.conf') KOJI_PROFILE = 'staging' KOJI_ARCHES = ['x86_64'] @@ -148,5 +142,4 @@ class TestConfiguration(BaseConfiguration): class ProdConfiguration(BaseConfiguration): - FAS_USERNAME = 'TODO' - # FAS_PASSWORD = 'another password' + pass diff --git a/contrib/submit_build.py b/contrib/submit_build.py index de4a38f0..ae6e967e 100644 --- a/contrib/submit_build.py +++ b/contrib/submit_build.py @@ -3,6 +3,11 @@ import socket import os import sys +try: + from urllib.parse import urlencode # py3 +except ImportError: + from urllib import urlencode # py2 + def listen_for_token(): """ Listens on port 13747 on localhost for a redirect request by OIDC @@ -65,7 +70,21 @@ print "Usage: submit_build.py [mbs_host] [oidc_token]" print "" if not token: print "Provide token as command line argument or visit following URL to obtain the token:" - print "https://id.stg.fedoraproject.org/openidc/Authorization?response_type=token&response_mode=form_post&nonce=1234&scope=openid%20profile%20email&client_id=mbs-authorizer&state=af0ifjsldkj&redirect_uri=http://localhost:13747/" + + query = urlencode({ + 'response_type': 'token', + 'response_mode': 'form_post', + 'nonce': '1234', + 'scope': ' '.join([ + 'openid', + 'profile', + 'email', + 'https://id.fedoraproject.org/scope/groups', + ]), + 'client_id': 'mbs-authorizer', + 'state': 'blahblahblah', + }) + "&redirect_uri=http://localhost:13747/" + print "https://id.stg.fedoraproject.org/openidc/Authorization?" + query print "We are waiting for you to finish the token generation..." if not token: diff --git a/module_build_service/auth.py b/module_build_service/auth.py index c5f047d9..dda2002b 100644 --- a/module_build_service/auth.py +++ b/module_build_service/auth.py @@ -23,16 +23,14 @@ """Auth system based on the client certificate and FAS account""" -from werkzeug.serving import WSGIRequestHandler - from module_build_service.errors import Unauthorized from module_build_service import app, log -import fedora.client import httplib2 import json from six.moves.urllib.parse import urlencode + def _json_loads(content): if not isinstance(content, str): content = content.decode('utf-8') @@ -72,9 +70,26 @@ def get_token_info(token): return _json_loads(content) -def get_username(request): + +def get_user_info(token): """ - Returns the client's username based on the OIDC token provided. + Asks the userinfo_uri for more information on a user. + """ + if not client_secrets: + return None + + headers = {'authorization': 'Bearer ' + token} + + resp, content = httplib2.Http().request( + client_secrets['userinfo_uri'], 'GET', + headers=headers) + + return _json_loads(content) + + +def get_user(request): + """ + Returns the client's username and groups based on the OIDC token provided. """ _load_secrets() @@ -87,33 +102,19 @@ def get_username(request): try: data = get_token_info(token) except Exception as e: - raise Unauthorized("Cannot verify OIDC token: %s" % str(e)) + error = "Cannot verify OIDC token: %s" % str(e) + log.exception(error) + raise Unauthorized(error) if not "active" in data or not data["active"]: raise Unauthorized("OIDC token invalid or expired.") - #TODO: Once we will get our own scope registered in Fedora infra, - # we can start checking it here. - return data["username"] + try: + extended_data = get_user_info(token) + except Exception as e: + error = "Cannot verify determine user groups: %s" % str(e) + log.exception(error) + raise Unauthorized(error) -def assert_is_packager(username, fas_kwargs): - """ Assert that a user is a packager by consulting FAS. - - When user is not a packager (is not in the packager FAS group), an - exception is raised. - - Note that `fas_kwargs` must contain values for `base_url`, `username`, and - `password`. These are required arguments for authenticating with FAS. - (Rida needs its own service account/password to talk to FAS). - """ - - FAS = fedora.client.AccountSystem(**fas_kwargs) - person = FAS.person_by_username(username) - - # Check that they have even applied in the first place... - if not 'packager' in person['group_roles']: - raise Unauthorized("%s is not in the packager group" % username) - - # Check more closely to make sure they're approved. - if person['group_roles']['packager']['role_status'] != 'approved': - raise Unauthorized("%s is not approved in the packager group" % username) + groups = set(extended_data['groups']) + return data["username"], groups diff --git a/module_build_service/config.py b/module_build_service/config.py index 4cb79e2a..3b906618 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -196,22 +196,10 @@ class Config(object): 'type': str, 'default': '', 'desc': ''}, - 'fas_url': { - 'type': str, - 'default': '', - 'desc': 'FAS URL'}, - 'fas_username': { - 'type': str, - 'default': '', - 'desc': 'FAS username'}, - 'fas_password': { - 'type': str, - 'default': '', - 'desc': 'FAS password'}, - 'require_packager': { - 'type': bool, - 'default': True, - 'desc': 'Turn on authorization against FAS'}, + 'allowed_groups': { + 'type': set, + 'default': set(['packager']), + 'desc': 'The set of groups allowed to submit builds.'}, 'log_backend': { 'type': str, 'default': None, @@ -327,7 +315,7 @@ class Config(object): if key in self._defaults: # type conversion for configuration item convert = self._defaults[key]['type'] - if convert in [bool, int, list, str]: + if convert in [bool, int, list, str, set]: try: setattr(self, key, convert(value)) except: diff --git a/module_build_service/views.py b/module_build_service/views.py index 837e7739..24ccba4b 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -91,13 +91,11 @@ class ModuleBuildAPI(MethodView): raise NotFound('No such module found.') def post(self): - username = module_build_service.auth.get_username(request) + username, groups = module_build_service.auth.get_user(request) - if conf.require_packager: - module_build_service.auth.assert_is_packager(username, fas_kwargs=dict( - base_url=conf.fas_url, - username=conf.fas_username, - password=conf.fas_password)) + if conf.allowed_groups and not (conf.allowed_groups & groups): + raise Unauthorized("%s is not in any of %r, only %r" % ( + username, conf.allowed_groups, groups)) try: r = json.loads(request.get_data().decode("utf-8")) @@ -126,17 +124,11 @@ class ModuleBuildAPI(MethodView): return jsonify(module.json()), 201 def patch(self, id): - username = module_build_service.auth.get_username(request) + username, groups = module_build_service.auth.get_user(request) - if conf.require_packager: - module_build_service.auth.assert_is_packager( - username, - fas_kwargs=dict( - base_url=conf.fas_url, - username=conf.fas_username, - password=conf.fas_password - ) - ) + if conf.allowed_groups and not (conf.allowed_groups & groups): + raise Unauthorized("%s is not in any of %r, only %r" % ( + username, conf.allowed_groups, groups)) module = models.ModuleBuild.query.filter_by(id=id).first() if not module: diff --git a/tests/test_auth.py b/tests/test_auth.py index ed97b177..6cce61fe 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -33,25 +33,25 @@ import module_build_service.errors class TestAuthModule(unittest.TestCase): @raises(module_build_service.errors.Unauthorized) - def test_get_username_no_token(self): + def test_get_user_no_token(self): request = mock.MagicMock() request.cookies.return_value = {} - module_build_service.auth.get_username(request) + module_build_service.auth.get_user(request) @raises(module_build_service.errors.Unauthorized) @patch('module_build_service.auth.get_token_info') - def test_get_username_failure(self, get_token_info): + def test_get_user_failure(self, get_token_info): def mocked_get_token_info(token): return {"active": False} get_token_info.return_value = mocked_get_token_info request = mock.MagicMock() request.cookies.return_value = {"oidc_token", "1234"} - module_build_service.auth.get_username(request) + module_build_service.auth.get_user(request) @raises(module_build_service.errors.Unauthorized) @patch('module_build_service.auth.get_token_info') - def test_get_username_good(self, get_token_info): + def test_get_user_good(self, get_token_info): # https://www.youtube.com/watch?v=G-LtddOgUCE name = "Joey Jo Jo Junior Shabadoo" def mocked_get_token_info(token): @@ -60,34 +60,5 @@ class TestAuthModule(unittest.TestCase): request = mock.MagicMock() request.cookies.return_value = {"oidc_token", "1234"} - result = module_build_service.auth.get_username(request) + result = module_build_service.auth.get_user(request) eq_(result, name) - - @mock.patch('fedora.client.AccountSystem') - def test_assert_is_packager(self, AccountSystem): - FAS = mock.MagicMock() - FAS.person_by_username.return_value = { - 'group_roles': { - 'packager': { - 'role_status': 'approved', - }, - }, - } - AccountSystem.return_value = FAS - # This should not raise an exception - module_build_service.auth.assert_is_packager('ralph', dict()) - - @raises(module_build_service.errors.Unauthorized) - @mock.patch('fedora.client.AccountSystem') - def test_assert_is_packager_failure(self, AccountSystem): - FAS = mock.MagicMock() - FAS.person_by_username.return_value = { - 'group_roles': { - 'packager': { - 'role_status': 'FAILLLL', - }, - }, - } - AccountSystem.return_value = FAS - # This should not raise an exception - module_build_service.auth.assert_is_packager('ralph', dict()) diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 837bc543..d7484c3f 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -32,7 +32,6 @@ from nose.tools import timed import module_build_service.messaging import module_build_service.scheduler.handlers.repos from module_build_service import db, models, conf -from module_build_service.messaging import RidaModule from mock import patch @@ -45,6 +44,8 @@ import module_build_service.scheduler.consumer base_dir = dirname(dirname(__file__)) cassette_dir = base_dir + '/vcr-request-data/' +user = ('Homer J. Simpson', set(['packager'])) + class MockedSCM(object): def __init__(self, mocked_scm, name, mmd_filename): self.mocked_scm = mocked_scm @@ -225,11 +226,9 @@ class TestBuild(unittest.TestCase): self.vcr.__exit__() @timed(30) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build(self, mocked_scm, mocked_assert_is_packager, - mocked_get_username): + def test_submit_build(self, mocked_scm, mocked_get_user): """ Tests the build of testmodule.yaml using TestModuleBuilder which succeeds everytime. @@ -281,11 +280,9 @@ class TestBuild(unittest.TestCase): self.assertEqual(buildroot_groups, []) @timed(30) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build_cancel(self, mocked_scm, mocked_assert_is_packager, - mocked_get_username): + def test_submit_build_cancel(self, mocked_scm, mocked_get_user): """ Submit all builds for a module and cancel the module build later. """ @@ -334,11 +331,9 @@ class TestBuild(unittest.TestCase): self.assertTrue(build.task_id in cancelled_tasks) @timed(30) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build_instant_complete(self, mocked_scm, mocked_assert_is_packager, - mocked_get_username): + def test_submit_build_instant_complete(self, mocked_scm, mocked_get_user): """ Tests the build of testmodule.yaml using TestModuleBuilder which succeeds everytime. @@ -366,11 +361,9 @@ class TestBuild(unittest.TestCase): self.assertTrue(build.module_build.state in [models.BUILD_STATES["done"], models.BUILD_STATES["ready"]] ) @timed(30) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build_concurrent_threshold( - self, mocked_scm, mocked_assert_is_packager, mocked_get_username): + def test_submit_build_concurrent_threshold(self, mocked_scm, mocked_get_user): """ Tests the build of testmodule.yaml using TestModuleBuilder with num_consecutive_builds set to 1. diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index 7eea81f0..7c8ddbbe 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -33,6 +33,11 @@ from tests import app, init_data from module_build_service.models import ComponentBuild import module_build_service.scm + +user = ('Homer J. Simpson', set(['packager'])) +other_user = ('some_other_user', set(['packager'])) + + class MockedSCM(object): def __init__(self, mocked_scm, name, mmd_filenames): """ @@ -204,11 +209,9 @@ class TestViews(unittest.TestCase): ' was provided for the \"modified_after\" parameter') self.assertEquals(data['status'], 400) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build(self, mocked_scm, mocked_assert_is_packager, - mocked_get_username): + def test_submit_build(self, mocked_scm, mocked_get_user): mocked_scm_obj = MockedSCM(mocked_scm, "fakemodule", "fakemodule.yaml") rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps( @@ -234,11 +237,9 @@ class TestViews(unittest.TestCase): mmd = _modulemd.ModuleMetadata() mmd.loads(data["modulemd"]) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_componentless_build(self, mocked_scm, mocked_assert_is_packager, - mocked_get_username): + def test_submit_componentless_build(self, mocked_scm, mocked_get_user): mocked_scm_obj = MockedSCM(mocked_scm, "fakemodule2", "fakemodule2.yaml") rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps( @@ -273,10 +274,8 @@ class TestViews(unittest.TestCase): self.assertEquals(data['status'], 401) self.assertEquals(data['error'], 'Unauthorized') - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') - def test_submit_build_scm_url_error(self, mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=user) + def test_submit_build_scm_url_error(self, mocked_get_user): rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps( {'scmurl': 'git://badurl.com'})) data = json.loads(rv.data) @@ -285,11 +284,8 @@ class TestViews(unittest.TestCase): self.assertEquals(data['status'], 401) self.assertEquals(data['error'], 'Unauthorized') - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') - def test_submit_build_scm_url_without_hash(self, - mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=user) + def test_submit_build_scm_url_without_hash(self, mocked_get_user): rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps( {'scmurl': 'git://pkgs.stg.fedoraproject.org/modules/' 'testmodule.git'})) @@ -300,12 +296,9 @@ class TestViews(unittest.TestCase): self.assertEquals(data['status'], 401) self.assertEquals(data['error'], 'Unauthorized') - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build_bad_modulemd(self, mocked_scm, - mocked_assert_is_packager, - mocked_get_username): + def test_submit_build_bad_modulemd(self, mocked_scm, mocked_get_user): mocked_scm_obj = MockedSCM(mocked_scm, "bad", "bad.yaml") rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps( @@ -316,11 +309,10 @@ class TestViews(unittest.TestCase): self.assertEquals(data['status'], 422) self.assertEquals(data['error'], 'Unprocessable Entity') - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') def test_submit_build_scm_parallalization(self, mocked_scm, - mocked_assert_is_packager, mocked_get_username): + mocked_get_user): def mocked_scm_get_latest(branch = "master"): time.sleep(1) return branch @@ -352,11 +344,10 @@ class TestViews(unittest.TestCase): # max to complete. self.assertTrue(time.time() - start < 3) - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') def test_submit_build_scm_non_available(self, mocked_scm, - mocked_assert_is_packager, mocked_get_username): + mocked_get_user): def mocked_scm_get_latest(): raise RuntimeError("Failed in mocked_scm_get_latest") @@ -372,11 +363,10 @@ class TestViews(unittest.TestCase): self.assertEquals(data['message'][:31], "Failed to get the latest commit") self.assertEquals(data['error'], "Unprocessable Entity") - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') + @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') - def test_submit_build_includedmodule(self, mocked_scm, mocked_assert_is_packager, - mocked_get_username): + def test_submit_build_includedmodule(self, mocked_scm, + mocked_get_user): mocked_scm_obj = MockedSCM(mocked_scm, "includedmodules", ["includedmodules.yaml", "fakemodule.yaml"]) @@ -408,10 +398,8 @@ class TestViews(unittest.TestCase): self.assertEquals(batches["bash"], 2) self.assertEquals(batches["file"], 3) - @patch('module_build_service.auth.get_username', return_value='some_other_user') - @patch('module_build_service.auth.assert_is_packager') - def test_cancel_build(self, mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=other_user) + def test_cancel_build(self, mocked_get_user): rv = self.client.patch('/module-build-service/1/module-builds/30', data=json.dumps({'state': 'failed'})) data = json.loads(rv.data) @@ -420,10 +408,8 @@ class TestViews(unittest.TestCase): self.assertEquals(data['state_reason'], 'Canceled by some_other_user.') - @patch('module_build_service.auth.get_username', return_value='Someone else') - @patch('module_build_service.auth.assert_is_packager') - def test_cancel_build_unauthorized(self, mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=('sammy', set())) + def test_cancel_build_unauthorized(self, mocked_get_user): rv = self.client.patch('/module-build-service/1/module-builds/30', data=json.dumps({'state': 'failed'})) data = json.loads(rv.data) @@ -431,10 +417,8 @@ class TestViews(unittest.TestCase): self.assertEquals(data['status'], 401) self.assertEquals(data['error'], 'Unauthorized') - @patch('module_build_service.auth.get_username', return_value='some_other_user') - @patch('module_build_service.auth.assert_is_packager') - def test_cancel_build_wrong_param(self, mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=other_user) + def test_cancel_build_wrong_param(self, mocked_get_user): rv = self.client.patch('/module-build-service/1/module-builds/30', data=json.dumps({'some_param': 'value'})) data = json.loads(rv.data) @@ -444,10 +428,8 @@ class TestViews(unittest.TestCase): self.assertEquals( data['message'], 'Invalid JSON submitted') - @patch('module_build_service.auth.get_username', return_value='some_other_user') - @patch('module_build_service.auth.assert_is_packager') - def test_cancel_build_wrong_state(self, mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=other_user) + def test_cancel_build_wrong_state(self, mocked_get_user): rv = self.client.patch('/module-build-service/1/module-builds/30', data=json.dumps({'state': 'some_state'})) data = json.loads(rv.data) @@ -457,10 +439,8 @@ class TestViews(unittest.TestCase): self.assertEquals( data['message'], 'The provided state change is not supported') - @patch('module_build_service.auth.get_username', return_value='Homer J. Simpson') - @patch('module_build_service.auth.assert_is_packager') - def test_submit_build_unsupported_scm_scheme(self, mocked_assert_is_packager, - mocked_get_username): + @patch('module_build_service.auth.get_user', return_value=user) + def test_submit_build_unsupported_scm_scheme(self, mocked_get_user): scmurl = 'unsupported://example.com/modules/' 'testmodule.git?#0000000000000000000000000000000000000000' rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(