From 80ddfece07a30b98e04cd4083896bb0ed1ad11e7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:34:20 +0000 Subject: [PATCH 1/8] Re-use the is_admin() function and fix it for the API This reduce some (partial) duplication of logic while keeping the API working Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 42a5012..078073b 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -298,10 +298,7 @@ def is_repo_admin(repo_obj): user = flask.g.fas_user.username - admin_users = APP.config.get('PAGURE_ADMIN_USERS', []) - if not isinstance(admin_users, list): - admin_users = [admin_users] - if user in admin_users: + if is_admin(): return True usergrps = [ @@ -321,10 +318,7 @@ def is_repo_committer(repo_obj): user = flask.g.fas_user.username - admin_users = APP.config.get('PAGURE_ADMIN_USERS', []) - if not isinstance(admin_users, list): - admin_users = [admin_users] - if user in admin_users: + if is_admin(): return True usergrps = [ @@ -344,10 +338,7 @@ def is_repo_user(repo_obj): user = flask.g.fas_user.username - admin_users = APP.config.get('PAGURE_ADMIN_USERS', []) - if not isinstance(admin_users, list): - admin_users = [admin_users] - if user in admin_users: + if is_admin(): return True usergrps = [ diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index c3c22aa..cd83a24 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -111,10 +111,16 @@ def check_api_acls(acls, optional=False): if acls and set(token.acls_list).intersection(set(acls)): token_auth = True flask.g.fas_user = token.user + # To get a token, in the `fas` auth user must have signed + # the CLA, so just set it to True + flask.g.fas_user.cla_done = True flask.g.token = token elif not acls and optional: token_auth = True flask.g.fas_user = token.user + # To get a token, in the `fas` auth user must have signed + # the CLA, so just set it to True + flask.g.fas_user.cla_done = True flask.g.token = token elif optional: return From 8ede9ec96f28d25d3a8b0010ff0c43d7851e6911 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:34:20 +0000 Subject: [PATCH 2/8] No need to check the number of groups for the admins and small style change Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 078073b..f1ee87d 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -273,7 +273,7 @@ def is_admin(): auth_method = APP.config.get('PAGURE_AUTH', None) if auth_method == 'fas': - if not user.cla_done or len(user.groups) < 1: + if not user.cla_done: return False admin_users = APP.config.get('PAGURE_ADMIN_USERS', []) @@ -283,7 +283,7 @@ def is_admin(): return True admins = APP.config['ADMIN_GROUP'] - if isinstance(admins, basestring): + if not isinstance(admins, list): admins = [admins] admins = set(admins or []) groups = set(flask.g.fas_user.groups) From fd7da80a9ff453b7ded17bc70e14ea8f497cd6b1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:34:20 +0000 Subject: [PATCH 3/8] Implement giving a project to someone This way the "owner" of the project can be changed. Fixes https://pagure.io/pagure/issue/1982 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/default_config.py b/pagure/default_config.py index b151e2b..b19c37c 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -54,6 +54,9 @@ ENABLE_NEW_PROJECTS = True # Enables / Disables deleting projects on this pagure instance ENABLE_DEL_PROJECTS = True +# Enables / Disables giving projects on this pagure instance +ENABLE_GIVE_PROJECTS = True + # Enables / Disables managing access to the repos ENABLE_USER_MNGT = True diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index ff26915..3476bb2 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -1059,6 +1059,36 @@ {% endif %} + {% if config.get('ENABLE_GIVE_PROJECTS', True) + and repo.user.user == g.fas_user.username + and not repo.is_fork %} +
+
+
+ Give Project +
+
+
+ {{ tag_form.csrf_token }} + + +
+
+
+
+ {% endif %} + {% if config.get('ENABLE_DEL_PROJECTS', True) %}
@@ -1216,5 +1246,28 @@ $('.extend-form').click(function(e) { $(tgt).append(form); }); +{% if config.get('ENABLE_GIVE_PROJECTS', True) + and repo.user.user == g.fas_user.username + and not repo.is_fork %} +$('#user').selectize({ + valueField: 'user', + labelField: 'user', + searchField: 'user', + maxItems: 1, + create: false, + load: function(query, callback) { + if (!query.length) return callback(); + $.getJSON( + "{{ url_for('api_ns.api_users') }}", { + pattern: query.term + }, + function( data ) { + callback( data.users.map(function(x) { return { user: x }; }) ); + } + ); + } +}); +{% endif %} + {% endblock %} diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index edd6237..b304a52 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -2560,3 +2560,60 @@ def delete_report(repo, username=None, namespace=None): return flask.redirect(flask.url_for( 'view_settings', username=username, repo=repo.name, namespace=namespace)) + + +@APP.route('//give', methods=['POST']) +@APP.route('///give', methods=['POST']) +@APP.route('/fork///give', methods=['POST']) +@APP.route( + '/fork////give', + methods=['POST']) +@login_required +def give_project(repo, username=None, namespace=None): + """ Give a project to someone else. + """ + if not APP.config.get('ENABLE_GIVE_PROJECTS', True): + flask.abort(404) + + if admin_session_timedout(): + flask.flash('Action canceled, try it again', 'error') + url = flask.url_for( + 'view_settings', username=username, repo=repo, + namespace=namespace) + return flask.redirect( + flask.url_for('auth_login', next=url)) + + repo = flask.g.repo + + if not flask.g.repo_admin: + flask.abort( + 403, + 'You are not allowed to change the settings for this project') + + if flask.g.fas_user.username != repo.user.user and not pagure.is_admin(): + flask.abort( + 403, + 'You are not allowed to give this project') + + form = pagure.forms.ConfirmationForm() + + if form.validate_on_submit(): + new_username = flask.request.form.get('user', '').strip() + new_owner = pagure.lib.search_user( + SESSION, username=new_username) + if not new_owner: + flask.abort( + 500, + 'No such user %s found' % new_username) + try: + repo.user = new_owner + SESSION.add(repo) + SESSION.commit() + flask.flash('Project updated') + except SQLAlchemyError as err: # pragma: no cover + SESSION.rollback() + flask.flash(str(err), 'error') + + return flask.redirect(flask.url_for( + 'view_repo', username=username, repo=repo.name, + namespace=namespace)) diff --git a/tests/test_pagure_flask_ui_app_give_project.py b/tests/test_pagure_flask_ui_app_give_project.py new file mode 100644 index 0000000..66399be --- /dev/null +++ b/tests/test_pagure_flask_ui_app_give_project.py @@ -0,0 +1,262 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2017 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import unittest +import shutil +import sys +import tempfile +import os + +from mock import patch, MagicMock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure +import pagure.lib +import tests + + +class PagureFlaskGiveRepotests(tests.Modeltests): + """ Tests for give a project on pagure """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskGiveRepotests, self).setUp() + + pagure.APP.config['TESTING'] = True + pagure.SESSION = self.session + pagure.ui.SESSION = self.session + pagure.ui.app.SESSION = self.session + pagure.ui.filters.SESSION = self.session + pagure.ui.repo.SESSION = self.session + + pagure.APP.config['VIRUS_SCAN_ATTACHMENTS'] = False + pagure.APP.config['GIT_FOLDER'] = self.path + pagure.APP.config['REQUESTS_FOLDER'] = os.path.join( + self.path, 'requests') + pagure.APP.config['TICKETS_FOLDER'] = os.path.join( + self.path, 'tickets') + pagure.APP.config['DOCS_FOLDER'] = os.path.join( + self.path, 'docs') + pagure.APP.config['UPLOAD_FOLDER_URL'] = '/releases/' + pagure.APP.config['UPLOAD_FOLDER_PATH'] = os.path.join( + self.path, 'releases') + self.app = pagure.APP.test_client() + + tests.create_projects(self.session) + tests.create_projects_git(self.path, bare=True) + + def _check_user(self, user='pingou'): + project = pagure.get_authorized_project( + self.session, project_name='test') + self.assertEqual(project.user.user, user) + + def test_give_project_no_project(self): + """ Test the give_project endpoint. """ + + # No such project + output = self.app.post('/test42/give') + self.assertEqual(output.status_code, 404) + + def test_give_project_no_csrf(self): + """ Test the give_project endpoint. """ + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + + self._check_user() + + # Missing CSRF + data = { + 'user': 'foo', + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Overview - test - Pagure', + output.data) + + self._check_user() + + def test_give_project_invalid_user(self): + """ Test the give_project endpoint. """ + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + self._check_user() + + # Invalid user + data = { + 'user': 'foobar', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 500) + self.assertIn( + '

No such user foobar found

', + output.data) + + self._check_user() + + def test_give_project_not_owner(self): + """ Test the give_project endpoint. """ + + user = tests.FakeUser() + user.username = 'foo' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + self._check_user() + + # User isn't the admin + data = { + 'user': 'foo', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 403) + self.assertIn( + '

You are not allowed to change the settings for this ' + 'project

', output.data) + + self._check_user() + + def test_give_project_not_admin(self): + """ Test the give_project endpoint. """ + + user = tests.FakeUser() + user.username = 'foo' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + self._check_user() + + # User isn't the admin + data = { + 'user': 'foo', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 403) + self.assertIn( + '

You are not allowed to change the settings for this ' + 'project

', output.data) + + self._check_user() + + def test_give_project_not_owner(self): + """ Test the give_project endpoint. """ + project = pagure.get_authorized_project( + self.session, project_name='test') + + msg = pagure.lib.add_user_to_project( + self.session, + project=project, + new_user='foo', + user='pingou', + access='admin') + self.session.commit() + self.assertEqual(msg, 'User added') + + user = tests.FakeUser() + user.username = 'foo' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + self._check_user() + + # User isn't the owner + data = { + 'user': 'foo', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 403) + self.assertIn( + '

You are not allowed to give this project

', + output.data) + + self._check_user() + + @patch.dict('pagure.APP.config', {'PAGURE_ADMIN_USERS': 'foo'}) + def test_give_project_not_owner_but_admin(self): + """ Test the give_project endpoint. """ + + user = tests.FakeUser() + user.username = 'foo' + user.cla_done = True + user.groups = ['foo'] + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + self._check_user() + + # User isn't the owner but is an instance admin + data = { + 'user': 'foo', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + '\n Project updated\n', + output.data) + + self._check_user('foo') + + @patch.dict('pagure.APP.config', {'PAGURE_ADMIN_USERS': 'foo'}) + def test_give_project(self): + """ Test the give_project endpoint. """ + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + self._check_user() + + # All good + data = { + 'user': 'foo', + 'csrf_token': csrf_token, + } + + output = self.app.post( + '/test/give', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + '\n Project updated\n', + output.data) + + self._check_user('foo') + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 87f4fc45c0b0ef3e3398918a49379a1dc2ea82ad Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:34:20 +0000 Subject: [PATCH 4/8] The "owner" of the project may no longer be its creator but its main admin Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/repo_info.html b/pagure/templates/repo_info.html index 26e44e2..36af040 100644 --- a/pagure/templates/repo_info.html +++ b/pagure/templates/repo_info.html @@ -62,7 +62,7 @@ git push -u origin master {{ repo.user.default_email | avatar(20) | safe }} {{ repo.user.fullname }} ({{ repo.user.user }}) - - creator + - main admin
{% for access in repo.contributors %} {% for user in repo.contributors[access] %} From ff8c8b61e0a157b0bf569bb19fd37b5bcedb6580 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 01 2017 14:49:02 +0000 Subject: [PATCH 5/8] Adjust information returned to the user, error code and messages Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index b304a52..11b70a0 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -2603,16 +2603,19 @@ def give_project(repo, username=None, namespace=None): SESSION, username=new_username) if not new_owner: flask.abort( - 500, + 404, 'No such user %s found' % new_username) try: repo.user = new_owner SESSION.add(repo) SESSION.commit() - flask.flash('Project updated') + flask.flash( + 'The project has been transferred to %s' % new_username) except SQLAlchemyError as err: # pragma: no cover SESSION.rollback() - flask.flash(str(err), 'error') + flask.flash( + 'Due to a database error, this project could not be ' + 'transferred.', 'error') return flask.redirect(flask.url_for( 'view_repo', username=username, repo=repo.name, From 1544e0a7d1103c6a875bbf6a97c2526e07d2c84f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 01 2017 14:49:17 +0000 Subject: [PATCH 6/8] Improve docstring in the tests to better explain what is tested Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_app_give_project.py b/tests/test_pagure_flask_ui_app_give_project.py index 66399be..5ae1439 100644 --- a/tests/test_pagure_flask_ui_app_give_project.py +++ b/tests/test_pagure_flask_ui_app_give_project.py @@ -205,7 +205,11 @@ class PagureFlaskGiveRepotests(tests.Modeltests): @patch.dict('pagure.APP.config', {'PAGURE_ADMIN_USERS': 'foo'}) def test_give_project_not_owner_but_admin(self): - """ Test the give_project endpoint. """ + """ Test the give_project endpoint. + + Test giving a project when the person giving the project is a pagure + admin (instance wide admin) but not a project admin. + """ user = tests.FakeUser() user.username = 'foo' From 0575a5995713aae155f558425c1293664c42ea2a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 02 2017 08:24:04 +0000 Subject: [PATCH 7/8] Fix the unit-tests Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_app_give_project.py b/tests/test_pagure_flask_ui_app_give_project.py index 5ae1439..b7554f3 100644 --- a/tests/test_pagure_flask_ui_app_give_project.py +++ b/tests/test_pagure_flask_ui_app_give_project.py @@ -110,7 +110,7 @@ class PagureFlaskGiveRepotests(tests.Modeltests): output = self.app.post( '/test/give', data=data, follow_redirects=True) - self.assertEqual(output.status_code, 500) + self.assertEqual(output.status_code, 404) self.assertIn( '

No such user foobar found

', output.data) @@ -230,7 +230,8 @@ class PagureFlaskGiveRepotests(tests.Modeltests): '/test/give', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( - '\n Project updated\n', + '\n The project has been ' + 'transferred to foo\n', output.data) self._check_user('foo') @@ -256,7 +257,8 @@ class PagureFlaskGiveRepotests(tests.Modeltests): '/test/give', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( - '\n Project updated\n', + '\n The project has been ' + 'transferred to foo\n', output.data) self._check_user('foo') From 4ec145f849174984c4c6a33d0edb829798a8419f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: May 02 2017 10:01:52 +0000 Subject: [PATCH 8/8] Drop un-used variable Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 11b70a0..422b4a8 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -2611,7 +2611,7 @@ def give_project(repo, username=None, namespace=None): SESSION.commit() flask.flash( 'The project has been transferred to %s' % new_username) - except SQLAlchemyError as err: # pragma: no cover + except SQLAlchemyError: # pragma: no cover SESSION.rollback() flask.flash( 'Due to a database error, this project could not be '