From 1dc86f07695fabab6f41f4162beab57e7392334c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 06 2017 06:02:55 +0000 Subject: [PATCH 1/3] Do not allow to delete a read-only project In the case where a project is deleted while gitolite is refreshing its Acls, gitolite will re-create the git repo, thus preventing the project to be re-created later (or refork in the cases of forks). Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 2f28a29..0eafd21 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -1100,19 +1100,28 @@ Delete Project
-
+ +   Delete the {{ repo.fullname }} project + + {% else %} + - -
+ + + + {% endif %}
diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 3108d23..caf46f6 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1425,6 +1425,15 @@ def delete_repo(repo, username=None, namespace=None): 403, 'You are not allowed to change the settings for this project') + if repo.read_only: + flask.flash( + 'The ACLs of this project are being refreshed in the backend ' + 'this prevents the project from being deleted. Please wait ' + 'for this task to finish before trying again. Thanks!') + return flask.redirect(flask.url_for( + 'view_settings', repo=repo.name, username=username, + namespace=namespace)) + try: SESSION.delete(repo) SESSION.commit() diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index d2f82a4..d8794e8 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -2625,6 +2625,12 @@ index 0000000..fb7093d output = self.app.post('/test/delete') self.assertEqual(output.status_code, 302) + # Ensure the project isn't read-only + repo = pagure.get_authorized_project(self.session, 'test') + repo.read_only = False + self.session.add(repo) + self.session.commit() + with tests.user_set(pagure.APP, user): # Only git repo item = pagure.lib.model.Project( @@ -2815,6 +2821,56 @@ index 0000000..fb7093d @patch('pagure.lib.notify.send_email') @patch('pagure.ui.repo.admin_session_timedout') + def test_delete_read_only_repo(self, ast, send_email): + """ Test the delete_repo endpoint when the repo is read_only """ + ast.return_value = False + send_email.return_value = True + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos')) + + # All repo there + item = pagure.lib.model.Project( + user_id=1, # pingou + name='test', + description='test project #1', + hook_token='aaabbbiii', + ) + self.session.add(item) + self.session.commit() + + # Create all the git repos + tests.create_projects_git(os.path.join(self.path, 'repos')) + tests.create_projects_git(os.path.join(self.path, 'docs')) + tests.create_projects_git( + os.path.join(self.path, 'tickets'), bare=True) + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + + user = tests.FakeUser(username='pingou') + with tests.user_set(pagure.APP, user): + + repo = pagure.get_authorized_project(self.session, 'test') + self.assertNotEqual(repo, None) + repo.read_only = True + self.session.add(repo) + self.session.commit() + + output = self.app.post('/test/delete', follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + u'Settings - test - Pagure', output.data) + self.assertIn( + u'The ACLs of this project are being refreshed in the ' + u'backend this prevents the project from being deleted. ' + u'Please wait for this task to finish before trying again. ' + u'Thanks!', output.data) + self.assertIn( + u'title="Action disabled while project\'s ACLs are being refreshed">', + output.data) + + @patch('pagure.lib.notify.send_email') + @patch('pagure.ui.repo.admin_session_timedout') def test_delete_repo(self, ast, send_email): """ Test the delete_repo endpoint. """ ast.return_value = False @@ -2841,6 +2897,12 @@ index 0000000..fb7093d output = self.app.post('/test/delete') self.assertEqual(output.status_code, 302) + # Ensure the project isn't read-only + repo = pagure.get_authorized_project(self.session, 'test') + repo.read_only = False + self.session.add(repo) + self.session.commit() + user = tests.FakeUser(username='pingou') with tests.user_set(pagure.APP, user): tests.create_projects_git(os.path.join(self.path, 'repos')) @@ -2865,6 +2927,7 @@ index 0000000..fb7093d name='test', description='test project #1', hook_token='aaabbbggg', + read_only=False, ) self.session.add(item) self.session.commit() @@ -2885,6 +2948,7 @@ index 0000000..fb7093d name='test', description='test project #1', hook_token='aaabbbhhh', + read_only=False, ) self.session.add(item) self.session.commit() @@ -2905,6 +2969,7 @@ index 0000000..fb7093d name='test', description='test project #1', hook_token='aaabbbiii', + read_only=False, ) self.session.add(item) self.session.commit() @@ -3041,6 +3106,7 @@ index 0000000..fb7093d is_fork=True, parent_id=2, hook_token='aaabbbjjj', + read_only=False, ) self.session.add(item) self.session.commit() @@ -3082,6 +3148,12 @@ index 0000000..fb7093d tests.create_projects(self.session) tests.create_projects_git(os.path.join(self.path, 'repos')) + # Ensure the project isn't read-only + repo = pagure.get_authorized_project(self.session, 'test') + repo.read_only = False + self.session.add(repo) + self.session.commit() + user = tests.FakeUser(username='pingou') with tests.user_set(pagure.APP, user): # Check before deleting the project @@ -3122,6 +3194,7 @@ index 0000000..fb7093d name='test', description='test project #1', hook_token='aaabbbiii', + read_only=False, ) self.session.add(item) self.session.commit() @@ -3156,6 +3229,13 @@ index 0000000..fb7093d self.session.commit() self.assertEqual(msg, 'User added') + # Ensure the project isn't read-only (because adding an user + # will trigger an ACL refresh, thus read-only) + repo = pagure.get_authorized_project(self.session, 'test') + repo.read_only = False + self.session.add(repo) + self.session.commit() + # Check before deleting the project output = self.app.get('/') self.assertEqual(output.status_code, 200) @@ -3202,6 +3282,7 @@ index 0000000..fb7093d name='test', description='test project #1', hook_token='aaabbbiii', + read_only=False, ) self.session.add(item) self.session.commit() @@ -3250,6 +3331,13 @@ index 0000000..fb7093d self.session.commit() self.assertEqual(msg, 'Group added') + # Ensure the project isn't read-only (because adding a group + # will trigger an ACL refresh, thus read-only) + repo = pagure.get_authorized_project(self.session, 'test') + repo.read_only = False + self.session.add(repo) + self.session.commit() + # check if group where we expect it repo = pagure.get_authorized_project(self.session, 'test') self.assertEqual(len(repo.projects_groups), 1) @@ -3296,6 +3384,7 @@ index 0000000..fb7093d name='test', description='test project #1', hook_token='aaabbbiii', + read_only=False, ) self.session.add(item) self.session.commit() diff --git a/tests/test_pagure_flask_ui_repo_delete_project.py b/tests/test_pagure_flask_ui_repo_delete_project.py index 902d1e5..95a61de 100644 --- a/tests/test_pagure_flask_ui_repo_delete_project.py +++ b/tests/test_pagure_flask_ui_repo_delete_project.py @@ -54,6 +54,10 @@ class PagureFlaskDeleteRepotests(tests.Modeltests): project = pagure.get_authorized_project( self.session, project_name='test') self.assertIsNotNone(project) + # Ensure the project isn't read-only + project.read_only = False + self.session.add(project) + self.session.commit() # Create a fork task_id = pagure.lib.fork_project( From 88830c46d6239d702396c7f218a4a57e13b0c830 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 06 2017 06:02:55 +0000 Subject: [PATCH 2/3] Log when deleting a git repo Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index caf46f6..77a3b0c 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1453,12 +1453,16 @@ def delete_repo(repo, username=None, namespace=None): try: for path in paths: + _log.info('Deleting: %s' % path) shutil.rmtree(path) except (OSError, IOError) as err: _log.exception(err) flask.flash( 'Could not delete all the repos from the system', 'error') + for path in paths: + _log.info('Path: %s - exists: %s' % (path, os.path.exists(path))) + return flask.redirect( flask.url_for('view_user', username=flask.g.fas_user.username)) From 5b06f78b74a616cc0e1fc2804092fb281076e9fd Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 06 2017 06:04:57 +0000 Subject: [PATCH 3/3] Move deleting a project into its own task This way we can more easily update the gitolite.conf file, remove the git repos from the disk and finally the project from the DB. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git_auth.py b/pagure/lib/git_auth.py index 14384f2..a5470fd 100644 --- a/pagure/lib/git_auth.py +++ b/pagure/lib/git_auth.py @@ -78,6 +78,22 @@ class GitAuthHelper(object): """ pass + @classmethod + @abc.abstractmethod + def remove_acls(self, session, project): + """ This is the method that is called by pagure to remove a project + from the configuration file. + + :arg cls: the current class + :type: GitAuthHelper + :arg session: the session with which to connect to the database + :arg project: the project to remove from the gitolite configuration + file. + :type project: pagure.lib.model.Project + + """ + pass + def _read_file(filename): """ Reads the specified file and return its content. @@ -420,6 +436,97 @@ class Gitolite2Auth(GitAuthHelper): if postconfig: stream.write(postconfig + '\n') + @classmethod + def remove_acls(cls, session, project): + """ Remove a project from the configuration file for gitolite. + + :arg cls: the current class + :type: Gitolite2Auth + :arg session: the session with which to connect to the database + :arg project: the project to remove from the gitolite configuration + file. + :type project: pagure.lib.model.Project + + """ + _log.info('Remove project from the gitolite configuration file') + + if not project: + raise RuntimeError('Project undefined') + + configfile = pagure.APP.config['GITOLITE_CONFIG'] + preconf = pagure.APP.config.get('GITOLITE_PRE_CONFIG') or None + postconf = pagure.APP.config.get('GITOLITE_POST_CONFIG') or None + + if not os.path.exists(configfile): + _log.info( + 'Not configuration file found at: %s... bailing' % configfile) + return + + 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) + + config = [] + groups = cls._generate_groups_config(session) + + _log.info('Removing the project from the configuration') + + current_config = cls._get_current_config( + configfile, preconfig, postconfig) + + current_config = cls._clean_current_config( + current_config, project) + + config = current_config + config + + if config: + _log.info('Cleaning the groups from the loaded config') + config = cls._clean_groups(config) + + else: + current_config = cls._get_current_config( + configfile, preconfig, postconfig) + + _log.info( + 'Cleaning the groups from the config on disk') + config = cls._clean_groups(config) + + if not config: + return + + _log.info('Writing the configuration to: %s', configfile) + with open(configfile, 'w') as stream: + if preconfig: + stream.write(preconfig + '\n') + stream.write('# end of header\n') + + if groups: + for key, users in groups.iteritems(): + stream.write('@%s = %s\n' % (key, ' '.join(users))) + stream.write('# end of groups\n\n') + + prev = None + for row in config: + if prev is None: + prev = row + if prev == row == '': + continue + stream.write(row + '\n') + prev = row + + stream.write('# end of body\n') + + if postconfig: + stream.write(postconfig + '\n') + @staticmethod def _get_gitolite_command(): """ Return the gitolite command to run based on the info in the @@ -517,3 +624,22 @@ class GitAuthTestHelper(GitAuthHelper): 'with args: project=%s, group=%s' % (project, group) print(out) return out + + @classmethod + def remove_acls(cls, session, project): + """ Print a statement about which a project would be removed from + the configuration file for gitolite. + + :arg cls: the current class + :type: GitAuthHelper + :arg session: the session with which to connect to the database + :arg project: the project to remove from the gitolite configuration + file. + :type project: pagure.lib.model.Project + + """ + + out = 'Called GitAuthTestHelper.remove_acls() ' \ + 'with args: project=%s' % (project.fullname) + print(out) + return out diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 7fadeca..d9bf248 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -105,19 +105,93 @@ def generate_gitolite_acls(namespace=None, name=None, user=None, group=None): 'Calling helper: %s with arg: project=%s, group=%s', helper, project, group_obj) helper.generate_acls(project=project, group=group_obj) - pagure.lib.update_read_only_mode( - session, project, read_only=False) + + pagure.lib.update_read_only_mode(session, project, read_only=False) try: session.commit() - _log.debug('Project %s is in Read Only Mode', project) + _log.debug('Project %s is no longer in Read Only Mode', project) except SQLAlchemyError: session.rollback() - _log.error( + _log.exception( 'Failed to unmark read_only for: %s project', project) session.remove() gc_clean() +@conn.task(queue=APP.config.get('GITOLITE_CELERY_QUEUE', None)) +def delete_project(namespace=None, name=None, user=None): + """ Delete a project in pagure. + + This is achieved in three steps: + - Remove the project from gitolite.conf + - Remove the git repositories on disk + - Remove the project from the DB + + :kwarg namespace: the namespace of the project + :type namespace: None or str + :kwarg name: the name of the project + :type name: None or str + :kwarg user: the user of the project, only set if the project is a fork + :type user: None or str + + """ + session = pagure.lib.create_session() + project = pagure.lib._get_project( + session, namespace=namespace, name=name, user=user, + case=APP.config.get('CASE_SENSITIVE', False)) + + if not project: + raise RuntimeError( + 'Project: %s/%s from user: %s not found in the DB' % ( + namespace, name, user)) + + # Remove the project from gitolite.conf + helper = pagure.lib.git_auth.get_git_auth_helper( + APP.config['GITOLITE_BACKEND']) + _log.debug('Got helper: %s', helper) + + _log.debug( + 'Calling helper: %s with arg: project=%s', helper, project.fullname) + helper.remove_acls(session=session, project=project) + + # Remove the git repositories on disk + paths = [] + for key in [ + 'GIT_FOLDER', 'DOCS_FOLDER', + 'TICKETS_FOLDER', 'REQUESTS_FOLDER']: + if APP.config[key]: + path = os.path.join(APP.config[key], project.path) + if os.path.exists(path): + paths.append(path) + + try: + for path in paths: + _log.info('Deleting: %s' % path) + shutil.rmtree(path) + except (OSError, IOError) as err: + _log.exception(err) + raise RuntimeError( + 'Could not delete all the repos from the system') + + for path in paths: + _log.info('Path: %s - exists: %s' % (path, os.path.exists(path))) + + # Remove the project from the DB + username = project.user.user + try: + session.delete(project) + session.commit() + except SQLAlchemyError: + session.rollback() + _log.exception( + 'Failed to delete project: %s from the DB', project.fullname) + session.remove() + + gc_clean() + + return ret('view_user', username=username) + + @conn.task def create_project(username, namespace, name, add_readme, ignore_existing_repo): diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 77a3b0c..55bb5ba 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -20,7 +20,6 @@ import datetime import json import logging -import shutil import os from cStringIO import StringIO from math import ceil @@ -47,6 +46,7 @@ import pagure.exceptions import pagure.lib import pagure.lib.git import pagure.lib.plugins +import pagure.lib.tasks import pagure.forms import pagure import pagure.ui.plugins @@ -1434,37 +1434,9 @@ def delete_repo(repo, username=None, namespace=None): 'view_settings', repo=repo.name, username=username, namespace=namespace)) - try: - SESSION.delete(repo) - SESSION.commit() - except SQLAlchemyError as err: # pragma: no cover - SESSION.rollback() - _log.exception(err) - flask.flash('Could not delete the project', 'error') - - paths = [] - for key in [ - 'GIT_FOLDER', 'DOCS_FOLDER', - 'TICKETS_FOLDER', 'REQUESTS_FOLDER']: - if APP.config[key]: - path = os.path.join(APP.config[key], repo.path) - if os.path.exists(path): - paths.append(path) - - try: - for path in paths: - _log.info('Deleting: %s' % path) - shutil.rmtree(path) - except (OSError, IOError) as err: - _log.exception(err) - flask.flash( - 'Could not delete all the repos from the system', 'error') - - for path in paths: - _log.info('Path: %s - exists: %s' % (path, os.path.exists(path))) - - return flask.redirect( - flask.url_for('view_user', username=flask.g.fas_user.username)) + task = pagure.lib.tasks.delete_project.delay( + repo.namespace, repo.name, repo.user.user if repo.is_fork else None) + return pagure.wait_for_task(task.id) @APP.route('//hook_token', methods=['POST']) diff --git a/tests/test_pagure_lib_gitolite_config.py b/tests/test_pagure_lib_gitolite_config.py index d74881f..f6421c6 100644 --- a/tests/test_pagure_lib_gitolite_config.py +++ b/tests/test_pagure_lib_gitolite_config.py @@ -807,6 +807,127 @@ repo requests/somenamespace/test3 #print data self.assertEqual(data, exp) + def test_remove_acls(self): + """ Test the remove_acls function of pagure.lib.git when deleting + a project """ + + with open(self.outputconf, 'w') as stream: + pass + + helper = pagure.lib.git_auth.get_git_auth_helper('gitolite3') + helper.write_gitolite_acls( + self.session, + self.outputconf, + project=-1, + ) + self.assertTrue(os.path.exists(self.outputconf)) + + with open(self.outputconf) as stream: + data = stream.read().decode('utf-8') + + exp = u"""@grp2 = foo +@grp = pingou +# end of groups + +%s + +# end of body +""" % CORE_CONFIG + + #print data + self.assertEqual(data, exp) + + # Test removing a project from the existing config + project = pagure.get_authorized_project( + self.session, project_name='test') + + helper.remove_acls(self.session, project=project) + + with open(self.outputconf) as stream: + data = stream.read().decode('utf-8') + + exp = u"""@grp2 = foo +@grp = pingou +# end of groups + +repo test2 + R = @all + RW+ = pingou + +repo docs/test2 + R = @all + RW+ = pingou + +repo tickets/test2 + RW+ = pingou + +repo requests/test2 + RW+ = pingou + +repo somenamespace/test3 + R = @all + RW+ = pingou + +repo docs/somenamespace/test3 + R = @all + RW+ = pingou + +repo tickets/somenamespace/test3 + RW+ = pingou + +repo requests/somenamespace/test3 + RW+ = pingou + +# end of body +""" + + #print data + self.assertEqual(data, exp) + + def test_remove_acls_no_project(self): + """ Test the remove_acls function of pagure.lib.git when no project + is specified """ + + with open(self.outputconf, 'w') as stream: + pass + + helper = pagure.lib.git_auth.get_git_auth_helper('gitolite3') + helper.write_gitolite_acls( + self.session, + self.outputconf, + project=-1, + ) + self.assertTrue(os.path.exists(self.outputconf)) + + with open(self.outputconf) as stream: + data = stream.read().decode('utf-8') + + exp = u"""@grp2 = foo +@grp = pingou +# end of groups + +%s + +# end of body +""" % CORE_CONFIG + + #print data + self.assertEqual(data, exp) + + # Test nothing changes if no project is specified + + self.assertRaises( + RuntimeError, + helper.remove_acls, + self.session, + project=None + ) + + with open(self.outputconf) as stream: + data = stream.read().decode('utf-8') + + self.assertEqual(data, exp) + if __name__ == '__main__': unittest.main(verbosity=2)