From 2a1d4db84d68fc5a1e47909258ea5c287f9e15d5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 10 2018 13:14:19 +0000 Subject: [PATCH 1/4] Fix detecting if the user is a committer via a group Fixes https://pagure.io/pagure/issue/3886 Fixes https://pagure.io/pagure/issue/3882 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/utils.py b/pagure/utils.py index 66e0aec..a833429 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -123,6 +123,7 @@ def is_repo_admin(repo_obj, username=None): def is_repo_committer(repo_obj, username=None, session=None): """ Return whether the user is a committer of the provided repo. """ + usergroups = set() if username is None: if not authenticated(): return False @@ -130,11 +131,14 @@ def is_repo_committer(repo_obj, username=None, session=None): return True username = flask.g.fas_user.username usergroups = set(flask.g.fas_user.groups) - else: - if not session: - session = flask.g.session + + if not session: + session = flask.g.session + try: user = pagure.lib.get_user(session, username) - usergroups = set(user.groups) + usergroups = usergroups.union(set(user.groups)) + except pagure.exceptions.PagureException: + return False # If the user is main admin -> yep if repo_obj.user.user == username: @@ -147,7 +151,7 @@ def is_repo_committer(repo_obj, username=None, session=None): # If they are in a group that has commit access -> yep for group in repo_obj.committer_groups: - if group in usergroups: + if group.group_name in usergroups: return True # If no direct committer, check EXTERNAL_COMMITTER info From 695f8cad97c700743e23a084d9954a24f4d1a6ad Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 10 2018 13:18:08 +0000 Subject: [PATCH 2/4] Ensure there is a session in flask.g and patch it correctly --- diff --git a/tests/test_pagure_flask.py b/tests/test_pagure_flask.py index 50de84f..46039d8 100644 --- a/tests/test_pagure_flask.py +++ b/tests/test_pagure_flask.py @@ -70,7 +70,8 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): g = munch.Munch() g.fas_user = tests.FakeUser(username='pingou') g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertTrue(output) @@ -82,7 +83,8 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): g = munch.Munch() g.fas_user = tests.FakeUser() g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertFalse(output) @@ -103,7 +105,8 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): g = munch.Munch() g.fas_user = user g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertFalse(output) @@ -116,10 +119,11 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): repo = pagure.lib._get_project(self.session, 'test') g = munch.Munch() - g.fas_user = tests.FakeUser() + g.fas_user = tests.FakeUser(username='foo') g.fas_user.groups.append('provenpackager') g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertTrue(output) @@ -141,7 +145,8 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): g.fas_user = tests.FakeUser() g.fas_user.groups.append('provenpackager') g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertFalse(output) @@ -157,7 +162,8 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): g.fas_user = tests.FakeUser(username='pingou') g.fas_user.groups.append('provenpackager') g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertTrue(output) @@ -177,7 +183,8 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): g = munch.Munch() g.fas_user = tests.FakeUser() g.authenticated = True - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertFalse(output) @@ -189,10 +196,11 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): repo = pagure.lib._get_project(self.session, 'test') g = munch.Munch() - g.fas_user = tests.FakeUser() + g.fas_user = tests.FakeUser(username='foo') g.authenticated = True g.fas_user.groups.append('provenpackager') - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertTrue(output) @@ -205,10 +213,11 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): repo = pagure.lib._get_project(self.session, 'test2') g = munch.Munch() - g.fas_user = tests.FakeUser() + g.fas_user = tests.FakeUser(username='foo') g.authenticated = True g.fas_user.groups.append('provenpackager') - with mock.patch('pagure.flask_app.flask.g', g): + g.session = self.session + with mock.patch('flask.g', g): output = pagure.utils.is_repo_committer(repo) self.assertFalse(output) From 8bba770472a364c6c53dbd3140312040d80e4fc0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 10 2018 13:18:36 +0000 Subject: [PATCH 3/4] Add test to ensure committers in a group with commit access are recognized --- diff --git a/tests/test_pagure_flask.py b/tests/test_pagure_flask.py index 46039d8..0f14e51 100644 --- a/tests/test_pagure_flask.py +++ b/tests/test_pagure_flask.py @@ -75,6 +75,54 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): output = pagure.utils.is_repo_committer(repo) self.assertTrue(output) + def test_is_repo_committer_logged_in_in_group(self): + """ Test is_repo_committer in pagure with the appropriate user logged + in. """ + # Create group + msg = pagure.lib.add_group( + self.session, + group_name='packager', + display_name='packager', + description='The Fedora packager groups', + group_type='user', + user='pingou', + is_admin=False, + blacklist=[]) + self.session.commit() + self.assertEqual(msg, 'User `pingou` added to the group `packager`.') + + # Add user to group + group = pagure.lib.search_groups(self.session, group_name='packager') + msg = pagure.lib.add_user_to_group( + self.session, + username='foo', + group=group, + user='pingou', + is_admin=True) + self.session.commit() + self.assertEqual(msg, 'User `foo` added to the group `packager`.') + + # Add group packager to project test + project = pagure.lib._get_project(self.session, 'test') + msg = pagure.lib.add_group_to_project( + self.session, + project=project, + new_group='packager', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'Group added') + + repo = pagure.lib._get_project(self.session, 'test') + + g = munch.Munch() + g.fas_user = tests.FakeUser(username='foo') + g.authenticated = True + g.session = self.session + with mock.patch('flask.g', g): + output = pagure.utils.is_repo_committer(repo) + self.assertTrue(output) + def test_is_repo_committer_logged_in_wrong_user(self): """ Test is_repo_committer in pagure with the wrong user logged in. """ @@ -222,6 +270,5 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): self.assertFalse(output) - if __name__ == '__main__': unittest.main(verbosity=2) From 7dbcb0e575f98ac4fd9f79969cf58c4a342907a8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 10 2018 13:21:30 +0000 Subject: [PATCH 4/4] Add test checking that group with ticket access aren't committer Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask.py b/tests/test_pagure_flask.py index 0f14e51..87ec36a 100644 --- a/tests/test_pagure_flask.py +++ b/tests/test_pagure_flask.py @@ -123,6 +123,55 @@ class PagureGetRemoteRepoPath(tests.SimplePagureTest): output = pagure.utils.is_repo_committer(repo) self.assertTrue(output) + def test_is_repo_committer_logged_in_in_ticket_group(self): + """ Test is_repo_committer in pagure with the appropriate user logged + in. """ + # Create group + msg = pagure.lib.add_group( + self.session, + group_name='packager', + display_name='packager', + description='The Fedora packager groups', + group_type='user', + user='pingou', + is_admin=False, + blacklist=[]) + self.session.commit() + self.assertEqual(msg, 'User `pingou` added to the group `packager`.') + + # Add user to group + group = pagure.lib.search_groups(self.session, group_name='packager') + msg = pagure.lib.add_user_to_group( + self.session, + username='foo', + group=group, + user='pingou', + is_admin=True) + self.session.commit() + self.assertEqual(msg, 'User `foo` added to the group `packager`.') + + # Add group packager to project test + project = pagure.lib._get_project(self.session, 'test') + msg = pagure.lib.add_group_to_project( + self.session, + project=project, + new_group='packager', + user='pingou', + access='ticket', + ) + self.session.commit() + self.assertEqual(msg, 'Group added') + + repo = pagure.lib._get_project(self.session, 'test') + + g = munch.Munch() + g.fas_user = tests.FakeUser(username='foo') + g.authenticated = True + g.session = self.session + with mock.patch('flask.g', g): + output = pagure.utils.is_repo_committer(repo) + self.assertFalse(output) + def test_is_repo_committer_logged_in_wrong_user(self): """ Test is_repo_committer in pagure with the wrong user logged in. """