From 16c3c211718f0ea1b7e17839585bde8fb1aafdcc Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 17 2019 13:46:50 +0000 Subject: [PATCH 1/13] Remove the unused allowed_clients role This is a leftover artifact from copying the code from ODCS. --- diff --git a/conf/config.py b/conf/config.py index 38cc71f..a7bf4e7 100644 --- a/conf/config.py +++ b/conf/config.py @@ -162,15 +162,6 @@ class BaseConfiguration(object): KRB_AUTH_CCACHE_FILE = tempfile.mkstemp( suffix=str(os.getpid()), prefix="freshmaker_cc_") - # Users are required to be in allowed_clients to generate composes, - # you can add group names or usernames (it can be normal user or host - # principal) into ALLOWED_CLIENTS. The group names are from ldap for - # kerberos users or FAS for openidc users. - ALLOWED_CLIENTS = { - 'groups': [], - 'users': [], - } - # Users in ADMINS are granted with admin permission. ADMINS = { 'groups': [], diff --git a/dev_scripts/config.py.template b/dev_scripts/config.py.template index c55900c..7436a95 100644 --- a/dev_scripts/config.py.template +++ b/dev_scripts/config.py.template @@ -165,15 +165,6 @@ class BaseConfiguration(object): KRB_AUTH_CCACHE_FILE = tempfile.mkstemp( suffix=str(os.getpid()), prefix="freshmaker_cc_") - # Users are required to be in allowed_clients to generate composes, - # you can add group names or usernames (it can be normal user or host - # principal) into ALLOWED_CLIENTS. The group names are from ldap for - # kerberos users or FAS for openidc users. - ALLOWED_CLIENTS = { - 'groups': [], - 'users': [], - } - # Users in ADMINS are granted with admin permission. ADMINS = { 'groups': [], diff --git a/freshmaker/auth.py b/freshmaker/auth.py index df018fb..eba43c2 100644 --- a/freshmaker/auth.py +++ b/freshmaker/auth.py @@ -246,9 +246,9 @@ def init_auth(login_manager, backend): def requires_role(role): """Check if user is in the configured role. - :param str role: role name, supported roles: 'allowed_clients', 'admins'. + :param str role: the role name """ - valid_roles = ['allowed_clients', 'admins'] + valid_roles = ['admins'] if role not in valid_roles: raise ValueError( "Unknown role <%s> specified, supported roles: %s." % ( diff --git a/tests/test_views.py b/tests/test_views.py index b47abf2..870c460 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -42,16 +42,10 @@ def user_loader(username): class ViewBaseTest(helpers.ModelsTestCase): def setUp(self): super(ViewBaseTest, self).setUp() - patched_allowed_clients = {'groups': ['freshmaker-clients'], - 'users': ['dev']} patched_admins = {'groups': ['admin'], 'users': ['root']} - self.patch_allowed_clients = patch.object(freshmaker.auth.conf, - 'allowed_clients', - new=patched_allowed_clients) self.patch_admins = patch.object(freshmaker.auth.conf, 'admins', new=patched_admins) - self.patch_allowed_clients.start() self.patch_admins.start() self.patch_oidc_base_namespace = patch.object( @@ -66,7 +60,6 @@ class ViewBaseTest(helpers.ModelsTestCase): def tearDown(self): super(ViewBaseTest, self).tearDown() - self.patch_allowed_clients.stop() self.patch_admins.stop() self.patch_oidc_base_namespace.stop() From d46df70078726657ff3f860aec7f5c4f4e2e80f2 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 18 2019 18:31:47 +0000 Subject: [PATCH 2/13] Stop supporting nosetest Some of the tests rely on pytest, so it makes sense to just use pytest instead. --- diff --git a/.copr/freshmaker.spec.in b/.copr/freshmaker.spec.in index d72c899..8a709dc 100644 --- a/.copr/freshmaker.spec.in +++ b/.copr/freshmaker.spec.in @@ -36,7 +36,6 @@ BuildRequires: python3-fedora BuildRequires: python3-flask BuildRequires: python3-flask-sqlalchemy BuildRequires: python3-mock -BuildRequires: python3-nose BuildRequires: python3-psutil BuildRequires: python3-pytest BuildRequires: python3-pyOpenSSL @@ -114,9 +113,6 @@ done install -d -m 0755 %{buildroot}%{_datadir}/freshmaker install -p -m 0644 contrib/freshmaker.wsgi %{buildroot}%{_datadir}/freshmaker -# %check -# nosetests-%{python3_version} -v - %files %doc README.md diff --git a/freshmaker/config.py b/freshmaker/config.py index 3de81a2..c3ff24c 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -78,7 +78,8 @@ def init_config(app): config_section = os.environ['FRESHMAKER_CONFIG_SECTION'] # TestConfiguration shall only be used for running tests, otherwise... - if any(['nosetests' in arg or 'noserunner.py' in arg or 'py.test' in arg or 'pytest.py' in arg for arg in sys.argv]): + test_executables = {'py.test', 'pytest', 'pytest.py'} + if os.path.basename(sys.argv[0]) in test_executables: config_section = 'TestConfiguration' from conf import config config_module = config From db9072ce65a0e61e95fcd0a3b64156d60b9e8034 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 18 2019 18:32:00 +0000 Subject: [PATCH 3/13] Don't recommend using sudo when installing Freshmaker in the virtualenv --- diff --git a/README.md b/README.md index 93b9bdf..28e00a3 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ Create and activate a [Python virtual environment](https://virtualenv.pypa.io/en Install the dependencies with: - sudo python3 setup.py install + python3 setup.py develop Install the requirements: From 72b12fc646bdef44ac0618ff41b6f44065b35002 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 18 2019 18:32:00 +0000 Subject: [PATCH 4/13] Simplify the recommended command to run the tests --- diff --git a/README.md b/README.md index 28e00a3..9ff8a15 100644 --- a/README.md +++ b/README.md @@ -95,4 +95,4 @@ Install the requirements useful to run the tests: Run the tests: - python3 -m pytest tests/ + pytest tests/ From 230880d9343002decf55491eb3364e8e555d4ed0 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 5/13] Migrate to using Rover groups for LDAP group membership verification This means that we query for the specific user and get the memberOf attribute instead of filtering by memberUid on the groups. This means that we can't safely use the common names of the group since it is not unique. This is because the groups are not filtered to a specific LDAP base DN since we are querying for the user. --- diff --git a/conf/config.py b/conf/config.py index a7bf4e7..bf643bb 100644 --- a/conf/config.py +++ b/conf/config.py @@ -179,9 +179,8 @@ class BaseConfiguration(object): # For example: ldap://ldap.example.com/ AUTH_LDAP_SERVER = '' - # Group base to query groups from LDAP server. - # Generally, it would be, for example, ou=groups,dc=example,dc=com - AUTH_LDAP_GROUP_BASE = '' + # The base to query for users in LDAP. For example, ou=users,dc=example,dc=com. + AUTH_LDAP_USER_BASE = '' AUTH_OPENIDC_USERINFO_URI = 'https://id.fedoraproject.org/openidc/UserInfo' @@ -275,7 +274,7 @@ class TestConfiguration(BaseConfiguration): AUTH_BACKEND = 'noauth' AUTH_LDAP_SERVER = 'ldap://ldap.example.com' - AUTH_LDAP_GROUP_BASE = 'ou=groups,dc=example,dc=com' + AUTH_LDAP_USER_BASE = 'ou=users,dc=example,dc=com' MAX_THREAD_WORKERS = 1 HANDLER_BUILD_WHITELIST = { diff --git a/conf/configrh.py b/conf/configrh.py index ae90077..9b8e0b4 100644 --- a/conf/configrh.py +++ b/conf/configrh.py @@ -62,7 +62,7 @@ class BaseConfiguration(config.BaseConfiguration): AUTH_BACKEND = 'kerberos' # Replace with real ldap server URL AUTH_LDAP_SERVER = '' - AUTH_LDAP_GROUP_BASE = 'ou=groups,dc=redhat,dc=com' + AUTH_LDAP_USER_BASE = 'ou=users,dc=redhat,dc=com' HANDLER_BUILD_WHITELIST = { 'BrewSignRPMHandler': { diff --git a/dev_scripts/config.py.template b/dev_scripts/config.py.template index 7436a95..76ce120 100644 --- a/dev_scripts/config.py.template +++ b/dev_scripts/config.py.template @@ -182,9 +182,8 @@ class BaseConfiguration(object): # For example: ldap://ldap.example.com/ AUTH_LDAP_SERVER = '' - # Group base to query groups from LDAP server. - # Generally, it would be, for example, ou=groups,dc=example,dc=com - AUTH_LDAP_GROUP_BASE = '' + # The base to query for users in LDAP. For example, ou=users,dc=example,dc=com. + AUTH_LDAP_USER_BASE = '' AUTH_OPENIDC_USERINFO_URI = 'https://id.fedoraproject.org/openidc/UserInfo' diff --git a/freshmaker/auth.py b/freshmaker/auth.py index eba43c2..73d954e 100644 --- a/freshmaker/auth.py +++ b/freshmaker/auth.py @@ -27,8 +27,6 @@ import requests import ldap import flask -from itertools import chain - from flask import g from flask_login import login_required as _login_required from werkzeug.exceptions import Unauthorized @@ -48,9 +46,9 @@ def _validate_kerberos_config(): errors.append("kerberos authentication enabled with no LDAP server " "configured, check AUTH_LDAP_SERVER in your config.") - if not conf.auth_ldap_group_base: - errors.append("kerberos authentication enabled with no LDAP group " - "base configured, check AUTH_LDAP_GROUP_BASE in your " + if not conf.auth_ldap_user_base: + errors.append("kerberos authentication enabled with no LDAP user " + "base configured, check AUTH_LDAP_USER_BASE in your " "config.") if errors: @@ -124,14 +122,33 @@ def load_krb_or_ssl_user_from_request(request): def query_ldap_groups(uid): - client = ldap.initialize(conf.auth_ldap_server) - groups = client.search_s(conf.auth_ldap_group_base, - ldap.SCOPE_ONELEVEL, - attrlist=['cn', 'gidNumber'], - filterstr='memberUid={0}'.format(uid)) + """ + Get the user's LDAP groups. - group_names = list(chain(*[info['cn'] for _, info in groups])) - return group_names + :param str uid: the user's uid LDAP attribute + :return: a set of distinguished names representing the user's group membership + :rtype: set + """ + client = ldap.initialize(conf.auth_ldap_server) + users = client.search_s( + conf.auth_ldap_user_base, + ldap.SCOPE_ONELEVEL, + attrlist=['memberOf'], + filterstr=f'(&(uid={uid})(objectClass=posixAccount))', + ) + + group_distinguished_names = set() + if users: + # users will only contain one entry if the user exists in the LDAP directory + # since the LDAP filter is limited to a single user. + _, user_attributes = users[0] + group_distinguished_names = { + # The value of group is the entire distinguished name of the group + group.decode('utf-8') + for group in user_attributes.get('memberOf', []) + } + + return group_distinguished_names @commit_on_success diff --git a/tests/test_auth.py b/tests/test_auth.py index 8a183e4..3cb2b1c 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -141,10 +141,14 @@ class TestLoadKrbOrSSLUserFromRequest(ModelsTestCase): class TestLoadKrbUserFromRequest(ModelsTestCase): + sample_groups = { + 'cn=admins,ou=groups,dc=example,dc=com', + 'cn=devel,ou=groups,dc=example,dc=com', + } @patch('freshmaker.auth.query_ldap_groups') def test_create_new_user(self, query_ldap_groups): - query_ldap_groups.return_value = ['devel', 'admins'] + query_ldap_groups.return_value = self.sample_groups environ_base = { 'REMOTE_USER': 'newuser@EXAMPLE.COM' @@ -161,11 +165,11 @@ class TestLoadKrbUserFromRequest(ModelsTestCase): # Ensure user's groups are created self.assertEqual(2, len(flask.g.groups)) - self.assertEqual(['admins', 'devel'], sorted(flask.g.groups)) + self.assertEqual(self.sample_groups, flask.g.groups) @patch('freshmaker.auth.query_ldap_groups') def test_return_existing_user(self, query_ldap_groups): - query_ldap_groups.return_value = ['devel', 'admins'] + query_ldap_groups.return_value = self.sample_groups original_users_count = db.session.query(User.id).count() environ_base = { @@ -179,7 +183,7 @@ class TestLoadKrbUserFromRequest(ModelsTestCase): db.session.query(User.id).count()) self.assertEqual(self.user.id, flask.g.user.id) self.assertEqual(self.user.username, flask.g.user.username) - self.assertEqual(['admins', 'devel'], sorted(flask.g.groups)) + self.assertEqual(self.sample_groups, flask.g.groups) def test_401_if_remote_user_not_present(self): with app.test_request_context(): @@ -298,17 +302,23 @@ class TestQueryLdapGroups(FreshmakerTestCase): @patch('freshmaker.auth.ldap.initialize') def test_get_groups(self, initialize): initialize.return_value.search_s.return_value = [ - ('cn=odcsdev,ou=Groups,dc=example,dc=com', - {'gidNumber': ['5523'], 'cn': ['odcsdev']}), - ('cn=freshmakerdev,ou=Groups,dc=example,dc=com', - {'gidNumber': ['17861'], 'cn': ['freshmakerdev']}), - ('cn=devel,ou=Groups,dc=example,dc=com', - {'gidNumber': ['5781'], 'cn': ['devel']}) + ( + 'uid=tom_hanks,ou=users,dc=example,dc=com', + { + 'memberOf': [ + b'cn=Toy Story,ou=groups,dc=example,dc=com', + b'cn=Forrest Gump,ou=groups,dc=example,dc=com', + ], + } + ) ] - groups = query_ldap_groups('me') - self.assertEqual(sorted(['odcsdev', 'freshmakerdev', 'devel']), - sorted(groups)) + groups = query_ldap_groups('tom_hanks') + expected = { + 'cn=Toy Story,ou=groups,dc=example,dc=com', + 'cn=Forrest Gump,ou=groups,dc=example,dc=com', + } + self.assertEqual(expected, groups) class TestInitAuth(FreshmakerTestCase): @@ -353,7 +363,7 @@ class TestInitAuth(FreshmakerTestCase): self.assertRaises(ValueError, init_auth, self.login_manager, 'kerberos') - def test_init_auths_no_ldap_group_base(self): - with patch.object(freshmaker.auth.conf, 'auth_ldap_group_base', ''): + def test_init_auths_no_ldap_user_base(self): + with patch.object(freshmaker.auth.conf, 'auth_ldap_user_base', ''): self.assertRaises(ValueError, init_auth, self.login_manager, 'kerberos') From e4b2ffc29740c58eeae4d69905e3724626539ace Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 6/13] Use a permissions dictionary where each key is a role instead of separate configurations per role The configured permissions dictionary is converted to a defaultdict, so any role not defined will default to returning {'groups': [], 'users': []}. --- diff --git a/conf/config.py b/conf/config.py index bf643bb..3146925 100644 --- a/conf/config.py +++ b/conf/config.py @@ -162,12 +162,6 @@ class BaseConfiguration(object): KRB_AUTH_CCACHE_FILE = tempfile.mkstemp( suffix=str(os.getpid()), prefix="freshmaker_cc_") - # Users in ADMINS are granted with admin permission. - ADMINS = { - 'groups': [], - 'users': [], - } - # Select which authentication backend to work with. There are 3 choices # noauth: no authentication is enabled. Useful for development particularly. # kerberos: Kerberos authentication is enabled. diff --git a/dev_scripts/config.py.template b/dev_scripts/config.py.template index 76ce120..e8634a8 100644 --- a/dev_scripts/config.py.template +++ b/dev_scripts/config.py.template @@ -165,12 +165,6 @@ class BaseConfiguration(object): KRB_AUTH_CCACHE_FILE = tempfile.mkstemp( suffix=str(os.getpid()), prefix="freshmaker_cc_") - # Users in ADMINS are granted with admin permission. - ADMINS = { - 'groups': [], - 'users': [], - } - # Select which authentication backend to work with. There are 3 choices # noauth: no authentication is enabled. Useful for development particularly. # kerberos: Kerberos authentication is enabled. diff --git a/freshmaker/auth.py b/freshmaker/auth.py index 73d954e..05897fb 100644 --- a/freshmaker/auth.py +++ b/freshmaker/auth.py @@ -265,20 +265,14 @@ def requires_role(role): :param str role: the role name """ - valid_roles = ['admins'] - if role not in valid_roles: - raise ValueError( - "Unknown role <%s> specified, supported roles: %s." % ( - role, str(valid_roles))) - def wrapper(f): @wraps(f) def wrapped(*args, **kwargs): if conf.auth_backend == 'noauth': return f(*args, **kwargs) - groups = getattr(conf, role).get('groups', []) - users = getattr(conf, role).get('users', []) + groups = conf.permissions[role]['groups'] + users = conf.permissions[role]['users'] in_groups = bool(set(flask.g.groups) & set(groups)) in_users = flask.g.user.username in users if in_groups or in_users: diff --git a/freshmaker/config.py b/freshmaker/config.py index c3ff24c..21e5574 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -24,6 +24,7 @@ # Filip Valder # Jan Kaluza +from collections import defaultdict import imp import os import threading @@ -351,6 +352,13 @@ class Config(object): 'type': int, 'default': 10, 'desc': 'Maximum number of thread workers used by Freshmaker.'}, + 'permissions': { + 'type': dict, + 'default': {}, + 'desc': 'The permissions with keys as role names and the values as dictionaries with ' + 'the keys "groups" and "users" which have values that are lists. Any roles not ' + 'provided as keys, will contain defaut empty values.' + }, } def __init__(self, conf_section_obj): @@ -448,6 +456,41 @@ class Config(object): raise ValueError("Unsupported messaging system.") self._messaging_sender = s + def _setifok_permissions(self, permissions): + invalid_value = ValueError( + 'The permissions configuration must be a dictionary with the keys as role names and ' + 'the values as dictionaries with the keys "users" and "groups", which must have values ' + 'that are lists. For example, {"admins": {"groups": [], "users": ["user"]}}.' + ) + if not isinstance(permissions, dict): + raise invalid_value + + for role, mapping in permissions.items(): + if not isinstance(mapping, dict): + raise invalid_value + + allowed_keys = {'users', 'groups'} + if mapping.keys() - allowed_keys: + raise invalid_value + + for key in allowed_keys: + if key not in mapping: + mapping[key] = [] + continue + + if not isinstance(mapping[key], list): + raise invalid_value + + for entry in mapping[key]: + if not isinstance(entry, str): + raise invalid_value + + # Use a default dict where any missing key will return {'groups': [], 'users': []}. This + # allows Freshmaker developers to add roles without needing to check if they key is set. + fixed_permissions = defaultdict(lambda: {'groups': [], 'users': []}) + fixed_permissions.update(permissions) + self._permissions = fixed_permissions + def _get_krb_auth_ccache_file(self): if not self._krb_auth_ccache_file: return self._krb_auth_ccache_file diff --git a/tests/test_config.py b/tests/test_config.py index c1dd92d..cf8f832 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -23,6 +23,8 @@ import os import threading +import pytest + from freshmaker import conf from tests import helpers @@ -34,3 +36,16 @@ class TestConfig(helpers.FreshmakerTestCase): conf.krb_auth_ccache_file, "freshmaker_cc_%s_%s" % (os.getpid(), threading.current_thread().ident)) + + +@pytest.mark.parametrize('value', ( + 'not a dict', + {'admins': 'not a dict'}, + {'admins': {'groups': 'not a list'}}, + {'admins': {'users': 'not a list'}}, + {'admins': {'invalid key': []}}, + {'admins': {'groups': [1]}}, +)) +def test_permissions(value): + with pytest.raises(ValueError, match='The permissions configuration must be a dictionary'): + conf.permissions = value diff --git a/tests/test_views.py b/tests/test_views.py index 870c460..5f06da1 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -19,6 +19,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. +from collections import defaultdict import unittest import json import datetime @@ -42,10 +43,10 @@ def user_loader(username): class ViewBaseTest(helpers.ModelsTestCase): def setUp(self): super(ViewBaseTest, self).setUp() - patched_admins = {'groups': ['admin'], 'users': ['root']} - self.patch_admins = patch.object(freshmaker.auth.conf, - 'admins', - new=patched_admins) + patched_admins = defaultdict(lambda: {'groups': [], 'users': []}) + patched_admins['admins'] = {'groups': ['admin'], 'users': ['root']} + self.patch_admins = patch.object( + freshmaker.auth.conf, 'permissions', new=patched_admins) self.patch_admins.start() self.patch_oidc_base_namespace = patch.object( From eff8f96fe15a86431305ba3867425aecf07d277a Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 7/13] Support multiple roles to access a particular API endpoint --- diff --git a/freshmaker/auth.py b/freshmaker/auth.py index 05897fb..e456330 100644 --- a/freshmaker/auth.py +++ b/freshmaker/auth.py @@ -260,25 +260,41 @@ def init_auth(login_manager, backend): raise ValueError('Unknown backend name {0}.'.format(backend)) -def requires_role(role): - """Check if user is in the configured role. +def user_has_role(role): + """ + Check if the current user has the role. + + :param str role: the role to check + :return: a boolean determining if the user has the role + :rtype: bool + """ + if conf.auth_backend == 'noauth': + return True + + groups = conf.permissions[role]['groups'] + users = conf.permissions[role]['users'] + in_groups = bool(set(flask.g.groups) & set(groups)) + in_users = flask.g.user.username in users + return in_groups or in_users - :param str role: the role name + +def requires_roles(roles): + """ + Assert the user has one of the required roles. + + :param list roles: the list of role names to verify + :raises freshmaker.errors.Forbidden: if the user is not in the role """ def wrapper(f): @wraps(f) def wrapped(*args, **kwargs): - if conf.auth_backend == 'noauth': + if any(user_has_role(role) for role in roles): return f(*args, **kwargs) - groups = conf.permissions[role]['groups'] - users = conf.permissions[role]['users'] - in_groups = bool(set(flask.g.groups) & set(groups)) - in_users = flask.g.user.username in users - if in_groups or in_users: - return f(*args, **kwargs) - raise Forbidden('User %s is not in role %s.' % ( - flask.g.user.username, role)) + raise Forbidden( + f'User {flask.g.user.username} does not have any of the following ' + f'roles: {", ".join(roles)}' + ) return wrapped return wrapper diff --git a/freshmaker/views.py b/freshmaker/views.py index b1389de..34a9010 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -38,7 +38,7 @@ from freshmaker.api_utils import filter_artifact_builds from freshmaker.api_utils import filter_events from freshmaker.api_utils import json_error from freshmaker.api_utils import pagination_metadata -from freshmaker.auth import login_required, requires_role, require_scopes +from freshmaker.auth import login_required, requires_roles, require_scopes from freshmaker.parsers.internal.manual_rebuild import FreshmakerManualRebuildParser from freshmaker.monitor import ( monitor_api, freshmaker_build_api_latency, freshmaker_event_api_latency) @@ -285,7 +285,7 @@ class EventAPI(MethodView): return json_error(404, "Not Found", "No such event found.") @login_required - @requires_role('admins') + @requires_roles(['admins']) def patch(self, id): """ Manage Freshmaker event defined by ID. The request must be @@ -368,7 +368,7 @@ class BuildAPI(MethodView): @login_required @require_scopes('submit-build') - @requires_role('admins') + @requires_roles(['admins']) def post(self): """ Trigger manual Freshmaker rebuild. The request must be From e4e21cb25d767d16a0d054959379fb588784e0e5 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 8/13] Use singular role names instead of plural This is because a user is an "admin", not an "admins". A role is different than a group, hence it shouldn't be plural. --- diff --git a/freshmaker/config.py b/freshmaker/config.py index 21e5574..6244384 100644 --- a/freshmaker/config.py +++ b/freshmaker/config.py @@ -460,7 +460,7 @@ class Config(object): invalid_value = ValueError( 'The permissions configuration must be a dictionary with the keys as role names and ' 'the values as dictionaries with the keys "users" and "groups", which must have values ' - 'that are lists. For example, {"admins": {"groups": [], "users": ["user"]}}.' + 'that are lists. For example, {"admin": {"groups": [], "users": ["user"]}}.' ) if not isinstance(permissions, dict): raise invalid_value diff --git a/freshmaker/views.py b/freshmaker/views.py index 34a9010..5f3f7a7 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -285,7 +285,7 @@ class EventAPI(MethodView): return json_error(404, "Not Found", "No such event found.") @login_required - @requires_roles(['admins']) + @requires_roles(['admin']) def patch(self, id): """ Manage Freshmaker event defined by ID. The request must be @@ -368,7 +368,7 @@ class BuildAPI(MethodView): @login_required @require_scopes('submit-build') - @requires_roles(['admins']) + @requires_roles(['admin']) def post(self): """ Trigger manual Freshmaker rebuild. The request must be diff --git a/tests/test_config.py b/tests/test_config.py index cf8f832..d7b07cd 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -40,11 +40,11 @@ class TestConfig(helpers.FreshmakerTestCase): @pytest.mark.parametrize('value', ( 'not a dict', - {'admins': 'not a dict'}, - {'admins': {'groups': 'not a list'}}, - {'admins': {'users': 'not a list'}}, - {'admins': {'invalid key': []}}, - {'admins': {'groups': [1]}}, + {'admin': 'not a dict'}, + {'admin': {'groups': 'not a list'}}, + {'admin': {'users': 'not a list'}}, + {'admin': {'invalid key': []}}, + {'admin': {'groups': [1]}}, )) def test_permissions(value): with pytest.raises(ValueError, match='The permissions configuration must be a dictionary'): diff --git a/tests/test_views.py b/tests/test_views.py index 5f06da1..ee04236 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -44,7 +44,7 @@ class ViewBaseTest(helpers.ModelsTestCase): def setUp(self): super(ViewBaseTest, self).setUp() patched_admins = defaultdict(lambda: {'groups': [], 'users': []}) - patched_admins['admins'] = {'groups': ['admin'], 'users': ['root']} + patched_admins['admin'] = {'groups': ['admin'], 'users': ['root']} self.patch_admins = patch.object( freshmaker.auth.conf, 'permissions', new=patched_admins) self.patch_admins.start() From 7d57e87bf036ae83cd7063661dca6cf45114ed04 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 9/13] Prevent tests that test authentication from affecting tests after it --- diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..e6127c6 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,36 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2019 Red Hat, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +import flask +import pytest + + +@pytest.fixture(autouse=True) +def clear_flask_g(): + """ + Clear the Flask global variables after each test. + + Many of the tests end up modifying flask.g such as for testing or mocking authentication. + If it isn't cleared, it would end up leaking into other tests which don't expect it. + """ + for attr in ('group', 'user'): + if hasattr(flask.g, attr): + delattr(flask.g, attr) diff --git a/tests/test_views.py b/tests/test_views.py index ee04236..88bc5a6 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -435,32 +435,6 @@ class TestViews(helpers.ModelsTestCase): self.assertEqual(data['error'], 'Bad Request') self.assertTrue(data['message'].startswith('Unsupported action requested.')) - def test_patch_event_cancel(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", - "RHSA-2018-103", events.TestingEvent) - models.ArtifactBuild.create(db.session, event, "mksh", "module", build_id=1237, - state=ArtifactBuildState.PLANNED.value) - models.ArtifactBuild.create(db.session, event, "bash", "module", build_id=1238, - state=ArtifactBuildState.PLANNED.value) - models.ArtifactBuild.create(db.session, event, "dash", "module", build_id=1239, - state=ArtifactBuildState.BUILD.value) - models.ArtifactBuild.create(db.session, event, "tcsh", "module", build_id=1240, - state=ArtifactBuildState.DONE.value) - db.session.commit() - - resp = self.client.patch( - '/api/1/events/{}'.format(event.id), - data=json.dumps({'action': 'cancel'})) - data = json.loads(resp.get_data(as_text=True)) - - self.assertEqual(data['id'], event.id) - self.assertEqual(len(data['builds']), 4) - self.assertEqual(data['state_name'], 'CANCELED') - self.assertTrue(data['state_reason'].startswith( - 'Event id {} requested for canceling by user '.format(event.id))) - self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'CANCELED']), 3) - self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'DONE']), 1) - def test_query_event_types(self): resp = self.client.get('/api/1/event-types/') event_types = json.loads(resp.get_data(as_text=True))['items'] @@ -605,7 +579,7 @@ class TestViewsMultipleFilterValues(helpers.ModelsTestCase): self.assertEqual(len(evs), 2) -class TestManualTriggerRebuild(helpers.ModelsTestCase): +class TestManualTriggerRebuild(ViewBaseTest): def setUp(self): super(TestManualTriggerRebuild, self).setUp() self.client = app.test_client() @@ -620,9 +594,13 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): 123, 'name', 'REL_PREP', ['rpm']) with patch('freshmaker.models.datetime') as datetime_patch: datetime_patch.utcnow.return_value = datetime.datetime(2017, 8, 21, 13, 42, 20) - resp = self.client.post('/api/1/builds/', - data=json.dumps({'errata_id': 1}), - content_type='application/json') + + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/builds/', + data=json.dumps({'errata_id': 1}), + content_type='application/json', + ) data = json.loads(resp.get_data(as_text=True)) # Other fields are predictible. @@ -641,7 +619,7 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): u'time_done': None, u'url': u'/api/1/events/1', u'dry_run': False, - u'requester': 'tester1', + u'requester': 'root', u'requested_rebuilds': [], u'requester_metadata': {}}) publish.assert_called_once_with( @@ -657,9 +635,9 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): from_advisory_id.return_value = ErrataAdvisory( 123, 'name', 'REL_PREP', ['rpm']) - resp = self.client.post('/api/1/builds/', - data=json.dumps({'errata_id': 1, 'dry_run': True}), - content_type='application/json') + payload = {'errata_id': 1, 'dry_run': True} + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') data = json.loads(resp.get_data(as_text=True)) # Other fields are predictible. @@ -677,10 +655,12 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): from_advisory_id.return_value = ErrataAdvisory( 123, 'name', 'REL_PREP', ['rpm']) - resp = self.client.post( - '/api/1/builds/', data=json.dumps({ - 'errata_id': 1, 'container_images': ["foo-1-1", "bar-1-1"]}), - content_type='application/json') + payload = { + 'errata_id': 1, + 'container_images': ['foo-1-1', 'bar-1-1'], + } + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') data = json.loads(resp.get_data(as_text=True)) # Other fields are predictible. @@ -699,10 +679,12 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): from_advisory_id.return_value = ErrataAdvisory( 123, 'name', 'REL_PREP', ['rpm']) - resp = self.client.post( - '/api/1/builds/', data=json.dumps({ - 'errata_id': 1, 'metadata': {"foo": ["bar"]}}), - content_type='application/json') + payload = { + 'errata_id': 1, + 'metadata': {'foo': ['bar']}, + } + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') data = json.loads(resp.get_data(as_text=True)) # Other fields are predictible. @@ -727,12 +709,15 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): from_advisory_id.return_value = ErrataAdvisory( 123, 'name', 'REL_PREP', ['rpm']) - resp = self.client.post( - '/api/1/builds/', data=json.dumps({ - 'errata_id': 1, 'container_images': ["foo-1-1"], - 'freshmaker_event_id': 1}), - content_type='application/json') + payload = { + 'errata_id': 1, + 'container_images': ['foo-1-1'], + 'freshmaker_event_id': 1, + } + with self.test_request_context(user='root'): + resp = self.client.post('/api/1/builds/', json=payload, content_type='application/json') data = json.loads(resp.get_data(as_text=True)) + # Other fields are predictible. self.assertEqual(data['requested_rebuilds'], ["foo-1-1"]) assert add_dependency.call_count == 1 @@ -743,6 +728,33 @@ class TestManualTriggerRebuild(helpers.ModelsTestCase): 'container_images': ["foo-1-1"], 'freshmaker_event_id': 1}) +class TestPatchAPI(ViewBaseTest): + def test_patch_event_cancel(self): + event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", + "RHSA-2018-103", events.TestingEvent) + models.ArtifactBuild.create(db.session, event, "mksh", "module", build_id=1237, + state=ArtifactBuildState.PLANNED.value) + models.ArtifactBuild.create(db.session, event, "bash", "module", build_id=1238, + state=ArtifactBuildState.PLANNED.value) + models.ArtifactBuild.create(db.session, event, "dash", "module", build_id=1239, + state=ArtifactBuildState.BUILD.value) + models.ArtifactBuild.create(db.session, event, "tcsh", "module", build_id=1240, + state=ArtifactBuildState.DONE.value) + db.session.commit() + + with self.test_request_context(user='root'): + resp = self.client.patch(f'/api/1/events/{event.id}', json={'action': 'cancel'}) + data = json.loads(resp.get_data(as_text=True)) + + self.assertEqual(data['id'], event.id) + self.assertEqual(len(data['builds']), 4) + self.assertEqual(data['state_name'], 'CANCELED') + self.assertTrue(data['state_reason'].startswith( + 'Event id {} requested for canceling by user '.format(event.id))) + self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'CANCELED']), 3) + self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'DONE']), 1) + + class TestOpenIDCLogin(ViewBaseTest): """Test that OpenIDC login""" From 80255588e8db2d52da36ad5f655bc0fc9ccbec9b Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 10/13] Add the "manual_rebuilder" role to allow non-admins to request manual rebuilds Users with the "manual_rebuilder" role can only cancel their own events. Users with the "admin" role can continue to cancel any event. --- diff --git a/freshmaker/views.py b/freshmaker/views.py index 5f3f7a7..56a8dde 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -38,7 +38,7 @@ from freshmaker.api_utils import filter_artifact_builds from freshmaker.api_utils import filter_events from freshmaker.api_utils import json_error from freshmaker.api_utils import pagination_metadata -from freshmaker.auth import login_required, requires_roles, require_scopes +from freshmaker.auth import login_required, requires_roles, require_scopes, user_has_role from freshmaker.parsers.internal.manual_rebuild import FreshmakerManualRebuildParser from freshmaker.monitor import ( monitor_api, freshmaker_build_api_latency, freshmaker_event_api_latency) @@ -285,7 +285,7 @@ class EventAPI(MethodView): return json_error(404, "Not Found", "No such event found.") @login_required - @requires_roles(['admin']) + @requires_roles(['admin', 'manual_rebuilder']) def patch(self, id): """ Manage Freshmaker event defined by ID. The request must be @@ -316,34 +316,36 @@ class EventAPI(MethodView): 400, "Bad Request", "Missing action in request." " Don't know what to do with the event.") - if data['action'] == 'cancel': - event = models.Event.query.filter_by(id=id).first() - if not event: - return json_error(400, "Not Found", "No such event found.") - - msg = "Event id %s requested for canceling by user %s" % \ - (event.id, g.user.username) - log.info(msg) - - event.transition(EventState.CANCELED, msg) - event.builds_transition( - ArtifactBuildState.CANCELED.value, - "Build canceled before running on external build system.", - filters={'state': ArtifactBuildState.PLANNED.value}) - builds_id = event.builds_transition( - ArtifactBuildState.CANCELED.value, None, - filters={'state': ArtifactBuildState.BUILD.value}) - db.session.commit() - - data["action"] = self._freshmaker_manage_prefix + data["action"] - data["event_id"] = event.id - data["builds_id"] = builds_id - messaging.publish("manage.eventcancel", data) - # Return back the JSON representation of Event to client. - return jsonify(event.json()), 200 - else: + if data["action"] != "cancel": + return json_error(400, "Bad Request", "Unsupported action requested.") + + event = models.Event.query.filter_by(id=id).first() + if not event: + return json_error(400, "Not Found", "No such event found.") + + if event.requester != g.user.username and not user_has_role("admin"): return json_error( - 400, "Bad Request", "Unsupported action requested.") + 403, "Forbidden", "You must be an admin to cancel someone else's event.") + + msg = "Event id %s requested for canceling by user %s" % (event.id, g.user.username) + log.info(msg) + + event.transition(EventState.CANCELED, msg) + event.builds_transition( + ArtifactBuildState.CANCELED.value, + "Build canceled before running on external build system.", + filters={'state': ArtifactBuildState.PLANNED.value}) + builds_id = event.builds_transition( + ArtifactBuildState.CANCELED.value, None, + filters={'state': ArtifactBuildState.BUILD.value}) + db.session.commit() + + data["action"] = self._freshmaker_manage_prefix + data["action"] + data["event_id"] = event.id + data["builds_id"] = builds_id + messaging.publish("manage.eventcancel", data) + # Return back the JSON representation of Event to client. + return jsonify(event.json()), 200 class BuildAPI(MethodView): @@ -368,7 +370,7 @@ class BuildAPI(MethodView): @login_required @require_scopes('submit-build') - @requires_roles(['admin']) + @requires_roles(['admin', 'manual_rebuilder']) def post(self): """ Trigger manual Freshmaker rebuild. The request must be diff --git a/tests/test_views.py b/tests/test_views.py index 88bc5a6..649715f 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -43,11 +43,12 @@ def user_loader(username): class ViewBaseTest(helpers.ModelsTestCase): def setUp(self): super(ViewBaseTest, self).setUp() - patched_admins = defaultdict(lambda: {'groups': [], 'users': []}) - patched_admins['admin'] = {'groups': ['admin'], 'users': ['root']} - self.patch_admins = patch.object( - freshmaker.auth.conf, 'permissions', new=patched_admins) - self.patch_admins.start() + patched_permissions = defaultdict(lambda: {'groups': [], 'users': []}) + patched_permissions['admin'] = {'groups': ['admin'], 'users': ['root']} + patched_permissions['manual_rebuilder'] = {'groups': [], 'users': ['tom_hanks']} + self.patched_permissions = patch.object( + freshmaker.auth.conf, 'permissions', new=patched_permissions) + self.patched_permissions.start() self.patch_oidc_base_namespace = patch.object( freshmaker.auth.conf, 'oidc_base_namespace', @@ -61,7 +62,7 @@ class ViewBaseTest(helpers.ModelsTestCase): def tearDown(self): super(ViewBaseTest, self).tearDown() - self.patch_admins.stop() + self.patched_permissions.stop() self.patch_oidc_base_namespace.stop() @contextlib.contextmanager @@ -730,8 +731,14 @@ class TestManualTriggerRebuild(ViewBaseTest): class TestPatchAPI(ViewBaseTest): def test_patch_event_cancel(self): - event = models.Event.create(db.session, "2017-00000000-0000-0000-0000-000000000003", - "RHSA-2018-103", events.TestingEvent) + event = models.Event.create( + db.session, + '2017-00000000-0000-0000-0000-000000000003', + 'RHSA-2018-103', + events.TestingEvent, + # Tests that admins can cancel any event, regardless of the requester + requester='tom_hanks', + ) models.ArtifactBuild.create(db.session, event, "mksh", "module", build_id=1237, state=ArtifactBuildState.PLANNED.value) models.ArtifactBuild.create(db.session, event, "bash", "module", build_id=1238, @@ -754,6 +761,43 @@ class TestPatchAPI(ViewBaseTest): self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'CANCELED']), 3) self.assertEqual(len([b for b in data['builds'] if b['state_name'] == 'DONE']), 1) + def test_patch_event_cancel_user(self): + event = models.Event.create( + db.session, + '2017-00000000-0000-0000-0000-000000000003', + 'RHSA-2018-103', + events.TestingEvent, + requester='tom_hanks', + ) + db.session.commit() + + with self.test_request_context(user='tom_hanks'): + resp = self.client.patch(f'/api/1/events/{event.id}', json={'action': 'cancel'}) + assert resp.status_code == 200 + + def test_patch_event_cancel_user_not_their_event(self): + event = models.Event.create( + db.session, + '2017-00000000-0000-0000-0000-000000000003', + 'RHSA-2018-103', + events.TestingEvent, + requester='han_solo', + ) + db.session.commit() + + with self.test_request_context(user='tom_hanks'): + resp = self.client.patch(f'/api/1/events/{event.id}', json={'action': 'cancel'}) + assert resp.status_code == 403 + assert resp.json['message'] == 'You must be an admin to cancel someone else\'s event.' + + def test_patch_event_not_allowed(self): + with self.test_request_context(user='john_smith'): + resp = self.client.patch(f'/api/1/events/1', json={'action': 'cancel'}) + assert resp.status_code == 403 + assert resp.json['message'] == ( + 'User john_smith does not have any of the following roles: admin, manual_rebuilder' + ) + class TestOpenIDCLogin(ViewBaseTest): """Test that OpenIDC login""" From 9fb6174365d49b798c8f363e4d5ede1fc79335f2 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 11/13] Use Python 3.6 to run the unit tests with tox This will allow us to support modern syntax such as f-strings. --- diff --git a/tox.ini b/tox.ini index 2c0952d..6e046de 100644 --- a/tox.ini +++ b/tox.ini @@ -4,7 +4,7 @@ # and then run "tox" from this directory. [tox] -envlist = py35, flake8, bandit +envlist = py36, flake8, bandit [testenv] skip_install = True From 2fa9bf3165f3d1d11195d0c3a0ea3a89680f02c6 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 12/13] Fix the Freshmaker description in the documentation --- diff --git a/docs/index.rst b/docs/index.rst index 4f2e7ed..087545d 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -7,8 +7,9 @@ Freshmaker ========== -The ODCS (On Demand Compose Service) is a service allowing to generate temporary -compose (mainly the RPM repository) with packages from Koji using the REST API. +Freshmaker is a service that automatically rebuilds content. It is currently +written to handle rebuilding container images with CVEs when new RPMs are +available that address those CVEs. .. toctree:: :maxdepth: 2 From 7942d69a5bb173f9043ca5335fbbad3840346dcc Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 21 2019 15:34:00 +0000 Subject: [PATCH 13/13] Document the permissions configuration --- diff --git a/docs/configuration.rst b/docs/configuration.rst new file mode 100644 index 0000000..52bcf26 --- /dev/null +++ b/docs/configuration.rst @@ -0,0 +1,33 @@ +============= +Configuration +============= + +Freshmaker uses the configuration file located at +``/etc/freshmaker/config.py``. This is based on ``conf/config.py``, and many +default values are inherited from ``freshmaker/config.py``. If the +configuration you are interested in is not documented here, check both of those +files. + + +Permissions +=========== + +Freshmaker permissions are defined by using a dictionary, where the keys +are role names, and the values are dictionaries that have the keys ``groups`` +and ``users``. If defined, these keys must have lists as values. If a role is +not defined, these values will default to empty lists. + +The following is an example of this: + +.. sourcecode:: python + + PERMISSIONS = { + 'admin': { + 'groups': ['fresmaker-admins'], + 'users': ['tom_hanks'], + }, + 'manual_rebuilder': { + 'groups': ['freshmaker-users'], + }, + } + diff --git a/docs/index.rst b/docs/index.rst index 087545d..17f9090 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -18,6 +18,7 @@ available that address those CVEs. about api_v1 api_v2 + configuration messaging_api dev_scripts