From 37636f98f3c680756b657cb1459a91e04b691d86 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jul 03 2017 15:02:56 +0000 Subject: [PATCH 1/2] Do not reconfigure the logger when one is already configured Otherwise it breaks the entire logging Signed-off-by: Pierre-Yves Chibon --- diff --git a/dist_git_auth.py b/dist_git_auth.py index 2dd310b..55419bc 100644 --- a/dist_git_auth.py +++ b/dist_git_auth.py @@ -26,7 +26,7 @@ from pagure import APP # noqa: E402 from pagure.lib import model # noqa: E402 from pagure.lib.git_auth import Gitolite3Auth, _read_file # noqa: E402 -logging.config.dictConfig(APP.config.get('LOGGING') or {'version': 1}) + _log = logging.getLogger(__name__) cache = dogpile.cache.make_region().configure( From 3d57e18bbdbfa905760e68908c1a3c8c3f3763b8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jul 04 2017 14:27:54 +0000 Subject: [PATCH 2/2] Rework the plugin to reduce the diff it produces This is based on https://pagure.io/pagure/pull-request/2395 which rework how gitolite's configuration file can be updated. Signed-off-by: Pierre-Yves Chibon --- diff --git a/dist_git_auth.py b/dist_git_auth.py index 55419bc..1fe27fa 100644 --- a/dist_git_auth.py +++ b/dist_git_auth.py @@ -47,6 +47,7 @@ namespace2pdctype = { 'container': 'container', } + @cache.cache_on_arguments() def get_supported_branches(namespace, package): default_url = 'https://pdc.fedoraproject.org/rest_api/v1/' @@ -65,111 +66,94 @@ class DistGitoliteAuth(Gitolite3Auth): """ A dist-git's gitolite authentication module. """ @classmethod - def write_gitolite_acls( - cls, session, configfile, preconf=None, postconf=None): - ''' Generate the configuration file for gitolite for all projects - on the forge. - ''' - _log.info('Write down the dist-git\'s gitolite configuration file') - - preconfig = None - if preconf: - _log.info( - 'Loading the file to include at the top of the generated one') - preconfig = _read_file(preconf) - - postconfig = None - if postconf: - _log.info( - 'Loading the file to include at the end of the generated one') - postconfig = _read_file(postconf) - - global_pr_only = pagure.APP.config.get('PR_ONLY', False) - config = [] - groups = {} - query = session.query(model.Project).order_by(model.Project.id) - for project in query.all(): - _log.debug(' Processing project: %s', project.fullname) - for group in project.committer_groups: - if group.group_name not in groups: - groups[group.group_name] = [ - user.username for user in group.users] - - # Check if the project or the pagure instance enforce the PR - # only development model. - pr_only = project.settings.get('pull_request_access_only', False) - - for repos in ['repos', 'requests/']: - if repos == 'repos': - # Do not grant access to project enforcing the PR model - if pr_only or (global_pr_only and not project.is_fork): - continue - repos = '' - - config.append('repo %s%s' % (repos, project.fullname)) - if repos not in ['tickets/', 'requests/']: - config.append(' R = @all') - - access = 'RWC' - if project.is_fork: - access = 'RW+' - - if repos == '': - # First, whitelist the supported branches from PDC - for branch in get_supported_branches(project.namespace, project.name): - config.append(' %s %s = %s' % (access, branch, project.user.user)) - for user in project.committers: - if user != project.user: - config.append(' %s = %s' % (access, branch, user.user)) - - # Then, blacklist a pattern over that (after). - config.append(_blacklist) - - if project.committer_groups: - config.append(' %s+ = @%s' % (access, ' @'.join( - [ - group.group_name - for group in project.committer_groups - ] - ))) - - config.append(' %s = %s' % (access, project.user.user)) - for user in project.committers: - if user != project.user: - config.append(' %s = %s' % (access, user.user)) - - for deploykey in project.deploykeys: - access = 'R' - if deploykey.pushaccess: - access = 'RW' - if project.is_fork: - access = 'RW+' - # Note: the replace of / with _ is because gitolite - # users can't contain a /. At first, this might look - # like deploy keys in a project called - # $namespace_$project would give access to the repos of - # a project $namespace/$project or vica versa, however - # this is NOT the case because we add the deploykey.id - # to the end of the deploykey name, which means it is - # unique. The project name is solely there to make it - # easier to determine what project created the deploykey - # for admins. - config.append(' %s = deploykey_%s_%s' % - (access, - werkzeug.secure_filename(project.fullname), - deploykey.id)) - config.append('') - - with open(configfile, 'w') as stream: - if preconfig: - stream.write(preconfig + '\n') - - for key, users in groups.iteritems(): - stream.write('@%s = %s\n' % (key, ' '.join(users))) - stream.write('\n') - - for row in config: - stream.write(row + '\n') - - if postconfig: - stream.write(postconfig + '\n') + def _process_project(cls, project, config, groups, global_pr_only): + """ Generate the gitolite configuration for the specified project. + + :arg project: the project to generate the configuration for + :type project: pagure.lib.model.Project + :arg config: a list containing the different lines of the + configuration file + :type config: list + :arg groups: a dictionary containing the group name as key and the + users member of the group as values + :type groups: dict(str: list) + :arg global_pr_only: boolean on whether the pagure instance enforces + the PR workflow only or not + :type global_pr_only: bool + :return: a tuple containing the updated config and groups variables + :return type: tuple(list, dict(str: list)) + + """ + + _log.debug(' Processing project: %s', project.fullname) + for group in project.committer_groups: + if group.group_name not in groups: + groups[group.group_name] = [ + user.username for user in group.users] + + # Check if the project or the pagure instance enforce the PR + # only development model. + pr_only = project.settings.get('pull_request_access_only', False) + + for repos in ['repos', 'requests/']: + if repos == 'repos': + # Do not grant access to project enforcing the PR model + if pr_only or (global_pr_only and not project.is_fork): + continue + repos = '' + + config.append('repo %s%s' % (repos, project.fullname)) + if repos not in ['tickets/', 'requests/']: + config.append(' R = @all') + + access = 'RWC' + if project.is_fork: + access = 'RW+' + + if repos == '': + # First, whitelist the supported branches from PDC + for branch in get_supported_branches(project.namespace, project.name): + config.append(' %s %s = %s' % (access, branch, project.user.user)) + for user in project.committers: + if user != project.user: + config.append(' %s %s = %s' % (access, branch, user.user)) + + # Then, blacklist a pattern over that (after). + config.append(_blacklist) + + if project.committer_groups: + config.append(' %s+ = @%s' % (access, ' @'.join( + [ + group.group_name + for group in project.committer_groups + ] + ))) + + config.append(' %s = %s' % (access, project.user.user)) + for user in project.committers: + if user != project.user: + config.append(' %s = %s' % (access, user.user)) + + for deploykey in project.deploykeys: + access = 'R' + if deploykey.pushaccess: + access = 'RW' + if project.is_fork: + access = 'RW+' + # Note: the replace of / with _ is because gitolite + # users can't contain a /. At first, this might look + # like deploy keys in a project called + # $namespace_$project would give access to the repos of + # a project $namespace/$project or vica versa, however + # this is NOT the case because we add the deploykey.id + # to the end of the deploykey name, which means it is + # unique. The project name is solely there to make it + # easier to determine what project created the deploykey + # for admins. + config.append(' %s = deploykey_%s_%s' % + (access, + werkzeug.secure_filename(project.fullname), + deploykey.id)) + config.append('') + + return (groups, config) diff --git a/dist_git_auth_tests.py b/dist_git_auth_tests.py index fbf026f..25e9dfe 100644 --- a/dist_git_auth_tests.py +++ b/dist_git_auth_tests.py @@ -7,10 +7,12 @@ import mock # These are the tests from the pagure/ git repo. # Run with PYTHONPATH=.:/path/to/pagure/checkout nosetests dist_git_auth_tests.py +import pagure import tests import dist_git_auth + expected = """ repo test R = @all @@ -57,14 +59,17 @@ repo requests/somenamespace/test3 class DistGitoliteAuthTestCase(tests.Modeltests): + """ Test generating the gitolite configuration file for dist-git. """ maxDiff = None def setUp(self): + """ Set up the environment in which to run the tests. """ super(DistGitoliteAuthTestCase, self).setUp() self.configfile = tempfile.mkstemp()[1] def tearDown(self): + """ Tear down the environment in which the tests ran. """ try: os.remove(self.configfile) except: @@ -74,18 +79,134 @@ class DistGitoliteAuthTestCase(tests.Modeltests): @mock.patch('dist_git_auth.get_supported_branches') def test_write_gitolite_acls(self, get_supported_branches): + """ Test generating the entire gitolite configuration file + (project == -1). + + """ get_supported_branches.return_value = ['master', 'f9000'] print("Initializing DB.") tests.create_projects(self.session) + print("Generating %r" % self.configfile) dist_git_auth.DistGitoliteAuth.write_gitolite_acls( - self.session, self.configfile) + self.session, + configfile=self.configfile, + project=-1) + print("Checking the contents of %r" % self.configfile) with open(self.configfile, 'r') as f: contents = f.read() self.assertMultiLineEqual(contents.strip(), expected.strip()) + @mock.patch('dist_git_auth.get_supported_branches') + def test_write_gitolite_acls_none_project(self, get_supported_branches): + """ Test not touching the gitolite configuration file + (project == None). + + """ + get_supported_branches.return_value = ['master', 'f9000'] + print("Initializing DB.") + tests.create_projects(self.session) + + print("Generating %r" % self.configfile) + dist_git_auth.DistGitoliteAuth.write_gitolite_acls( + self.session, + configfile=self.configfile, + project=None) + + print("Checking the contents of %r" % self.configfile) + with open(self.configfile, 'r') as f: + contents = f.read() + self.assertMultiLineEqual(contents.strip(), '') + + @mock.patch('dist_git_auth.get_supported_branches') + def test_write_gitolite_acls_test_project(self, get_supported_branches): + """ Test updating the gitolite configuration file for just one + project (project == a pagure.lib.model.Project). + + """ + + get_supported_branches.return_value = ['master', 'f9000'] + self.test_write_gitolite_acls() + + print("Modifying the test project so the output differs.") + project = pagure.lib._get_project(self.session, 'test') + project.user_id = 2 + self.session.add(project) + self.session.commit() + + project = pagure.lib._get_project(self.session, 'test') + msg = pagure.lib.add_user_to_project( + self.session, + project=project, + new_user='pingou', + user='foo', + access='commit' + ) + self.assertEqual(msg, 'User added') + self.session.commit() + + print("Rewriting %r" % self.configfile) + project = pagure.lib._get_project(self.session, 'test') + dist_git_auth.DistGitoliteAuth.write_gitolite_acls( + self.session, + configfile=self.configfile, + project=project + ) + + print("Checking the contents of %r" % self.configfile) + with open(self.configfile, 'r') as f: + contents = f.read() + + expected = '''repo test2 + R = @all + RWC master = pingou + RWC f9000 = pingou + - f[0-9][0-9] = @all + - epel[0-9] = @all + - epel[0-9][0-9] = @all + - el[0-9] = @all + - olpc[0-9] = @all + RWC = pingou + +repo requests/test2 + RWC = pingou + +repo somenamespace/test3 + R = @all + RWC master = pingou + RWC f9000 = pingou + - f[0-9][0-9] = @all + - epel[0-9] = @all + - epel[0-9][0-9] = @all + - el[0-9] = @all + - olpc[0-9] = @all + RWC = pingou + +repo requests/somenamespace/test3 + RWC = pingou + +repo test + R = @all + RWC master = foo + RWC master = pingou + RWC f9000 = foo + RWC f9000 = pingou + - f[0-9][0-9] = @all + - epel[0-9] = @all + - epel[0-9][0-9] = @all + - el[0-9] = @all + - olpc[0-9] = @all + RWC = foo + RWC = pingou + +repo requests/test + RWC = foo + RWC = pingou''' + self.assertMultiLineEqual(contents.strip(), expected) + def test_get_supported_branches(self): + """ Test for real what is returned by PDC. """ expected = ['master', 'f26', 'f25', 'f24', 'el6'] actual = dist_git_auth.get_supported_branches('rpms', 'nethack') self.assertEquals(set(actual), set(expected))