From afa354f0141d236539712dcb0f7cfe7de72c9ae5 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Mar 19 2021 14:42:38 +0000 Subject: Use simpler permission configuration JIRA: RHELWF-386 --- diff --git a/docs/admin-guide.rst b/docs/admin-guide.rst index 47574ff..2426001 100644 --- a/docs/admin-guide.rst +++ b/docs/admin-guide.rst @@ -21,24 +21,35 @@ Option ``AUTH_METHOD`` is name of authentication method. This can be "OIDC", Waive Permission ================ -If ``PERMISSION_MAPPING`` option is unset, anyone is able to waive any test -result. +If ``PERMISSIONS`` option (and ``PERMISSION_MAPPING`` deprecated option) is +unset, anyone is able to waive any test result. If the option is set, it describes which users and groups can waive which test -cases. It is a mapping from test case name pattern to dict with user and group -lists. +cases. Field ``testcases`` contains a glob expression to match test case names +and map them to ``groups`` and/or ``users``. + +It is helpful to include metadata about permissions: ``name``, +``maintainer`` and ``description``. LDAP needs to be properly configured (i.e. options ``LDAP_HOST`` and ``LDAP_BASE``). .. code-block:: python - PERMISSION_MAPPING = { - "^kernel-qe\.": { - "groups": ["devel", "qa"], - "users": [] - }, - "": {"groups": ["waiverdb-admins"], "users": []}, + PERMISSIONS = { + { + "name": "kernel-qe", + "maintainers": ["alice@example.com"], + "testcases": ["kernel-qe.*"], + "groups": ["devel", "qa"], + "users": ["alice@example.com"] + }, + { + "name": "Admins", + "maintainers": ["bob@example.com"], + "testcases": ["*"], + "groups": ["waiverdb-admins"] + } } LDAP_HOST = 'ldap://ldap.example.com' LDAP_BASE = 'ou=Groups,dc=example,dc=com' diff --git a/tests/conftest.py b/tests/conftest.py index f12c7cd..1e43442 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -73,16 +73,6 @@ def enable_ssl(app, monkeypatch): @pytest.fixture() -def enable_permission_mapping(app, monkeypatch): - monkeypatch.setitem(app.config, 'PERMISSION_MAPPING', - { - "^testcase1.*": {"groups": ["factory-2-0"], "users": []}, # noqa - "^testcase2.*": {"groups": [], "users": ["foo"]}, # noqa - "^testcase4.*": {"groups": [], "users": []} # noqa - }) - - -@pytest.fixture() def enable_ldap_host(app, monkeypatch): monkeypatch.setitem(app.config, 'LDAP_HOST', 'ldap://ldap.something.com') diff --git a/tests/test_access_control.py b/tests/test_access_control.py index 7db11a0..a13c3fb 100644 --- a/tests/test_access_control.py +++ b/tests/test_access_control.py @@ -12,7 +12,29 @@ from werkzeug.exceptions import Unauthorized from waiverdb.cli import cli as waiverdb_cli -@pytest.mark.usefixtures('enable_permission_mapping') +@pytest.fixture() +def enable_permissions(app, monkeypatch): + permissions = [ + { + "testcases": ["testcase1.*"], + "groups": ["factory-2-0"], + "users": [] + }, + { + "testcases": ["testcase2.*"], + "groups": [], + "users": ["foo"] + }, + { + "testcases": ["testcase3"], + "groups": [], + "users": [] + }, + ] + monkeypatch.setitem(app.config, 'PERMISSIONS', permissions) + + +@pytest.mark.usefixtures('enable_permissions') @pytest.mark.usefixtures('enable_kerberos') @mock.patch.multiple("gssapi.SecurityContext", complete=True, __init__=mock.Mock(return_value=None), @@ -26,7 +48,7 @@ class TestAccessControl(object): data = { 'subject_type': 'koji_build', 'subject_identifier': 'glibc-2.26-27.fc27', - 'testcase': 'testcase1', + 'testcase': 'testcase1.functional', 'product_version': 'fool-1', 'waived': True, 'comment': 'it broke', @@ -40,7 +62,7 @@ class TestAccessControl(object): res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 500 assert res_data['message'] == ('LDAP_HOST and LDAP_SEARCHES also need to be defined ' - 'if PERMISSION_MAPPING is defined.') + 'if PERMISSIONS is defined.') @pytest.mark.usefixtures('enable_ldap_host') def test_ldap_host_defined_base_not(self, client, session): @@ -85,7 +107,7 @@ class TestAccessControl(object): assert res_data['subject'] == {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'} assert res_data['subject_type'] == 'koji_build' assert res_data['subject_identifier'] == 'glibc-2.26-27.fc27' - assert res_data['testcase'] == 'testcase1' + assert res_data['testcase'] == 'testcase1.functional' assert res_data['product_version'] == 'fool-1' assert res_data['waived'] is True assert res_data['comment'] == 'it broke' @@ -94,7 +116,7 @@ class TestAccessControl(object): @pytest.mark.usefixtures('enable_ldap_base') def test_user_has_permission(self, client, session, monkeypatch): monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab') - self.data['testcase'] = 'testcase2' + self.data['testcase'] = 'testcase2.integration' r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data), content_type='application/json', headers=self.headers) res_data = json.loads(r.get_data(as_text=True)) @@ -103,7 +125,7 @@ class TestAccessControl(object): assert res_data['subject'] == {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'} assert res_data['subject_type'] == 'koji_build' assert res_data['subject_identifier'] == 'glibc-2.26-27.fc27' - assert res_data['testcase'] == 'testcase2' + assert res_data['testcase'] == 'testcase2.integration' assert res_data['product_version'] == 'fool-1' assert res_data['waived'] is True assert res_data['comment'] == 'it broke' @@ -143,7 +165,7 @@ class TestAccessControl(object): @mock.patch('waiverdb.authorization.get_group_membership', return_value=(['factory-2-0', 'something-else'])) def test_proxied_by_has_permission(self, mocked_conn, mock_get_user, client, session): - self.data['testcase'] = 'testcase2' + self.data['testcase'] = 'testcase2.integration' self.data['username'] = 'foo' r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data), content_type='application/json', headers=self.headers) @@ -153,7 +175,7 @@ class TestAccessControl(object): assert res_data['subject'] == {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'} assert res_data['subject_type'] == 'koji_build' assert res_data['subject_identifier'] == 'glibc-2.26-27.fc27' - assert res_data['testcase'] == 'testcase2' + assert res_data['testcase'] == 'testcase2.integration' assert res_data['product_version'] == 'fool-1' assert res_data['waived'] is True assert res_data['comment'] == 'it broke' @@ -193,7 +215,7 @@ class TestAccessControl(object): "data": {"item": ["glibc-2.26-27.fc27"], "type": ["koji_build"]}, "id": 15, "product_version": "fool-1", - "testcase": "testcase2", + "testcase": "testcase2.integration", "timestamp": "2017-010-16T17:42:04.209638", "username": "foo", "proxied_by": "bodhi", diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index 775139e..23c3766 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -730,6 +730,44 @@ def test_config_endpoint_permissions_map(client): assert r.json['permission_mapping'] == config['PERMISSION_MAPPING'] +def test_permissions_endpoint(client): + config = { + 'PERMISSIONS': [ + { + "name": "^kernel-qe", + "maintainers": ["alice@example.com"], + "_testcase_regex_pattern": "^kernel-qe", + "groups": ["devel", "qa"], + "users": ["alice@example.com"], + }, + { + "name": "Greenwave Tests", + "maintainers": ["greenwave-dev@example.com"], + "testcases": ["greenwave-tests.*"], + "groups": [], + "users": ["HTTP/greenwave-dev.tests.example.com"] + } + ] + } + + with patch.dict(client.application.config, config): + r = client.get('/api/v1.0/permissions') + assert r.status_code == 200 + assert r.json == config['PERMISSIONS'] + + r = client.get('/api/v1.0/permissions?testcase=xxx') + assert r.status_code == 200 + assert r.json == [] + + r = client.get('/api/v1.0/permissions?testcase=greenwave-tests.test1') + assert r.status_code == 200 + assert r.json == config['PERMISSIONS'][1:] + + r = client.get('/api/v1.0/permissions?testcase=kernel-qe.test1') + assert r.status_code == 200 + assert r.json == config['PERMISSIONS'][0:1] + + def test_config_endpoint_superusers(client): config = { 'SUPERUSERS': ['alice', 'bob'] diff --git a/tests/test_authorization.py b/tests/test_authorization.py new file mode 100644 index 0000000..24c68ec --- /dev/null +++ b/tests/test_authorization.py @@ -0,0 +1,69 @@ +from waiverdb.api_v1 import permissions +from waiverdb.authorization import match_testcase_permissions + + +def test_permissions_mapping_compat(app, monkeypatch): + """ + Verify backwards compatibility with deprecated PERMISSION_MAPPING option. + """ + monkeypatch.setitem(app.config, 'PERMISSIONS', []) + monkeypatch.setitem(app.config, 'PERMISSION_MAPPING', {}) + assert permissions() == [] + + permission_mapping = { + "^kernel-qe": { + "groups": ["devel", "qa"], + "users": ["alice@example.com"], + "maintainer": "alice@example.com", + }, + } + monkeypatch.setitem(app.config, 'PERMISSION_MAPPING', permission_mapping) + assert permissions() == [ + { + "name": "^kernel-qe", + "maintainers": ["alice@example.com"], + "_testcase_regex_pattern": "^kernel-qe", + "groups": ["devel", "qa"], + "users": ["alice@example.com"], + } + ] + + permissions_override = [ + { + "name": "Greenwave Tests", + "maintainers": ["greenwave-dev@example.com"], + "testcases": ["greenwave-tests.*"], + "groups": [], + "users": ["HTTP/greenwave-dev.tests.example.com"] + } + ] + monkeypatch.setitem(app.config, 'PERMISSIONS', permissions_override) + assert permissions() == permissions_override + + +def test_match_testcase_permissions(): + """ + Verify that correct permissions are retrieved for given test case name. + """ + assert list(match_testcase_permissions("kernel-qe.test1", [])) == [] + + permissions = [ + { + "name": "^kernel-qe", + "maintainers": ["alice@example.com"], + "_testcase_regex_pattern": "^kernel-qe", + "groups": ["devel", "qa"], + "users": ["alice@example.com"], + }, + { + "name": "Greenwave Tests", + "maintainers": ["greenwave-dev@example.com"], + "testcases": ["greenwave-tests.*"], + "groups": [], + "users": ["HTTP/greenwave-dev.tests.example.com"] + } + ] + assert list(match_testcase_permissions("greenwave-tests.test1", permissions)) \ + == [permissions[1]] + assert list(match_testcase_permissions("kernel-qe.test1", permissions)) \ + == [permissions[0]] diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index 3031d05..e48e32a 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -14,7 +14,7 @@ from werkzeug.exceptions import ( from sqlalchemy.sql.expression import func, and_, or_ from waiverdb import __version__ -from waiverdb.authorization import verify_authorization +from waiverdb.authorization import match_testcase_permissions, verify_authorization from waiverdb.models import db from waiverdb.models.waivers import Waiver, subject_dict_to_type_identifier from waiverdb.utils import json_collection, jsonp @@ -90,6 +90,31 @@ def reqparse_since(since): return start, end +def permissions(): + """ + Return PERMISSIONS configuration. + PERMISSION_MAPPING converted to the new format. + """ + permissions_config = current_app.config.get('PERMISSIONS') + if permissions_config: + return permissions_config + + permission_mapping = current_app.config.get('PERMISSION_MAPPING') + if permission_mapping: + return [ + { + "name": testcase_pattern, + "maintainers": [props["maintainer"]] if "maintainer" in props else [], + "_testcase_regex_pattern": testcase_pattern, + "groups": props.get("groups", []), + "users": props.get("users", []), + } + for testcase_pattern, props in permission_mapping.items() + ] + + return [] + + def _filter_out_obsolete_waivers(query): """ Filters out obsolete waivers. @@ -138,6 +163,9 @@ RP['get_waivers'].add_argument('page', default=1, type=int, location='args') RP['get_waivers'].add_argument('limit', default=10, type=int, location='args') RP['get_waivers'].add_argument('proxied_by', location='args') +RP['get_permissions'] = reqparse.RequestParser() +RP['get_permissions'].add_argument('testcase', location='args') + RP['filter_waivers'] = reqparse.RequestParser() RP['filter_waivers'].add_argument('filters', type=valid_filter_list, required=True, location='json') RP['filter_waivers'].add_argument('include_obsolete', type=bool, default=False, location='json') @@ -336,8 +364,7 @@ class WaiversResource(Resource): return result, 201, headers def _verify_authorization(self, user, testcase): - permission_mapping = current_app.config.get('PERMISSION_MAPPING') - if not permission_mapping: + if not permissions(): return True ldap_host = current_app.config.get('LDAP_HOST') @@ -349,7 +376,7 @@ class WaiversResource(Resource): 'LDAP_SEARCH_STRING', '(memberUid={user})' ) ldap_searches = [{'BASE': ldap_base, 'SEARCH_STRING': ldap_search_string}] - return verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_searches) + return verify_authorization(user, testcase, permissions(), ldap_host, ldap_searches) def _create_waiver(self, args, user): proxied_by = None @@ -656,6 +683,9 @@ class ConfigResource(Resource): """ Returns the current configuration (PERMISSION_MAPPING and SUPERUSERS). + **Note:** PERMISSION_MAPPING is **deprecated**, + use :http:get:`/api/v1.0/permissions` instead. + **Sample response**: .. sourcecode:: none @@ -684,6 +714,50 @@ class ConfigResource(Resource): } +class PermissionsResource(Resource): + @jsonp + def get(self): + """ + Returns the waiver permissions. + + **Sample response**: + + .. sourcecode:: none + + HTTP/1.0 200 OK + Content-Length: 999 + Content-Type: application/json + Server: gunicorn/20.0.4 + Date: Wed, 10 Mar 2021 08:00:00 GMT + + [ + { + "name": "kernel-qe", + "maintainers": ["alice@example.com"], + "testcases": ["kernel-qe.*"], + "groups": ["devel", "qa"], + "users": ["alice@example.com"] + }, + { + "name": "Greenwave Tests", + "maintainers": ["greenwave-dev@example.com"], + "testcases": ["greenwave-tests.*"], + "groups": [], + "users": ["HTTP/greenwave-dev.tests.example.com"] + } + ] + + :json string testcase: If specified, only permissions for given test case is returned. + :statuscode 200: Permissions are returned. + """ + args = RP['get_permissions'].parse_args() + testcase = args['testcase'] + if testcase: + return list(match_testcase_permissions(testcase, permissions())) + + return permissions() + + class MonitorResource(Resource): def get(self): from waiverdb.monitor import MonitorAPI @@ -697,4 +771,5 @@ api.add_resource(FilteredWaiversResource, '/waivers/+filtered') api.add_resource(GetWaiversBySubjectsAndTestcases, '/waivers/+by-subjects-and-testcases') api.add_resource(AboutResource, '/about', strict_slashes=False) api.add_resource(ConfigResource, '/config', strict_slashes=False) +api.add_resource(PermissionsResource, '/permissions', strict_slashes=False) api.add_resource(MonitorResource, '/metrics') diff --git a/waiverdb/authorization.py b/waiverdb/authorization.py index 70bbb7e..19e4130 100644 --- a/waiverdb/authorization.py +++ b/waiverdb/authorization.py @@ -2,6 +2,7 @@ import logging import re +from fnmatch import fnmatch from werkzeug.exceptions import ( BadGateway, @@ -30,24 +31,38 @@ def get_group_membership(ldap, user, con, ldap_search): raise Unauthorized('Some error occurred initializing the LDAP connection.') -def verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_searches): +def match_testcase_permissions(testcase, permissions): + for permission in permissions: + if "testcases" in permission: + testcase_match = any( + fnmatch(testcase, testcase_pattern) + for testcase_pattern in permission["testcases"] + ) + elif "_testcase_regex_pattern" in permission: + testcase_match = re.search( + permission["_testcase_regex_pattern"], testcase) + else: + continue + + if testcase_match: + yield permission + + +def verify_authorization(user, testcase, permissions, ldap_host, ldap_searches): if not (ldap_host and ldap_searches): raise InternalServerError(('LDAP_HOST and LDAP_SEARCHES also need to be defined ' - 'if PERMISSION_MAPPING is defined.')) + 'if PERMISSIONS is defined.')) allowed_groups = [] - for testcase_pattern, permission in permission_mapping.items(): - testcase_match = re.search(testcase_pattern, testcase) - if testcase_match: - # checking if the user is allowed - if user in permission['users']: - return True - allowed_groups += permission['groups'] + for permission in match_testcase_permissions(testcase, permissions): + if user in permission.get('users', []): + return True + allowed_groups += permission.get('groups', []) try: import ldap except ImportError: - raise InternalServerError(('If PERMISSION_MAPPING is defined, ' + raise InternalServerError(('If PERMISSIONS is defined, ' 'python-ldap needs to be installed.')) try: diff --git a/waiverdb/config.py b/waiverdb/config.py index b4c4348..35a87f0 100644 --- a/waiverdb/config.py +++ b/waiverdb/config.py @@ -29,6 +29,8 @@ class Config(object): SQLALCHEMY_TRACK_MODIFICATIONS = True # A list of users are allowed to create waivers on behalf of other users. SUPERUSERS = [] + PERMISSIONS = [] + # Deprecated permission mapping PERMISSION_MAPPING = {}