PR#3383: Ensure we check the required group membership when giving a project away - pagure

pagure

#3383 Ensure we check the required group membership when giving a project away

Merged 6 years ago by pingou. Opened 6 years ago by pingou.

give_requires_group into master

Be consistent about logging error and flashing them to the user

Pierre-Yves Chibon • 6 years ago

d9dd521

Ensure we check the required group membership when giving a project away

Pierre-Yves Chibon • 6 years ago

411690f

pagure/lib/__init__.py

file modified

+17 -1

		`@@ -4880,14 +4880,30 @@`
		`return query.all()`


		`- def set_project_owner(session, project, user):`
		`+ def set_project_owner(session, project, user, required_groups=None):`
		`''' Set the ownership of a project`
		`:arg session: the session to use to connect to the database.`
		`:arg project: a Project object representing the project's ownership to`
		`change.`
		`:arg user: a User object representing the new owner of the project.`
		`+ :arg required_groups: a dict of {pattern: [list of groups]} the new user`
		`+ should be in to become owner if one of the pattern matches the`
		`+ project fullname.`
		`:return: None`
		`'''`
		`+`
		`+ if required_groups:`
		`+ for key in required_groups:`
		`+ if fnmatch.fnmatch(project.fullname, key):`
		`+ user_grps = set(user.groups)`
		`+ req_grps = set(required_groups[key])`
		`+ if not user_grps.intersection(req_grps):`
		`+ raise pagure.exceptions.PagureException(`
		`+ 'This user must be in one of the following groups '`
		`+ 'to be allowed to be added to this project: %s' %`
		`+ ', '.join(req_grps)`
		`+ )`
		`+`
		`for contributor in project.users:`
		`if user.id == contributor.id:`
		`project.users.remove(contributor)`

pagure/ui/repo.py

file modified

+19 -6

		`@@ -1056,6 +1056,7 @@`
		`'Updating checksums for %s of project %s in task: %s' % (`
		`filenames, repo.fullname, task.id))`
		`except pagure.exceptions.PagureException as err:`
		`+ _log.debug(err)`
		`flask.flash(str(err), 'error')`
		`except Exception as err: # pragma: no cover`
		`_log.exception(err)`
		`@@ -1127,7 +1128,8 @@`
		`namespace=repo.namespace))`
		`except pagure.exceptions.PagureException as msg:`
		`flask.g.session.rollback()`
		`- flask.flash(msg, 'error')`
		`+ _log.debug(msg)`
		`+ flask.flash(str(msg), 'error')`
		`except SQLAlchemyError as err: # pragma: no cover`
		`flask.g.session.rollback()`
		`flask.flash(str(err), 'error')`
		`@@ -1713,7 +1715,8 @@`
		`namespace=namespace) + '#deploykey-tab')`
		`except pagure.exceptions.PagureException as msg:`
		`flask.g.session.rollback()`
		`- flask.flash(msg, 'error')`
		`+ _log.debug(msg)`
		`+ flask.flash(str(msg), 'error')`
		`except SQLAlchemyError as err: # pragma: no cover`
		`flask.g.session.rollback()`
		`_log.exception(err)`
		`@@ -1783,7 +1786,8 @@`
		`namespace=namespace) + '#usersgroups-tab')`
		`except pagure.exceptions.PagureException as msg:`
		`flask.g.session.rollback()`
		`- flask.flash(msg, 'error')`
		`+ _log.debug(msg)`
		`+ flask.flash(str(msg), 'error')`
		`except SQLAlchemyError as err: # pragma: no cover`
		`flask.g.session.rollback()`
		`_log.exception(err)`
		`@@ -1910,7 +1914,8 @@`
		`namespace=namespace) + '#usersgroups-tab')`
		`except pagure.exceptions.PagureException as msg:`
		`flask.g.session.rollback()`
		`- flask.flash(msg, 'error')`
		`+ _log.debug(msg)`
		`+ flask.flash(str(msg), 'error')`
		`except SQLAlchemyError as err: # pragma: no cover`
		`flask.g.session.rollback()`
		`_log.exception(err)`
		`@@ -2405,7 +2410,8 @@`
		`flask.g.session.commit()`
		`flask.flash(msg)`
		`except pagure.exceptions.PagureException as msg:`
		`- flask.flash(msg, 'error')`
		`+ _log.debug(msg)`
		`+ flask.flash(str(msg), 'error')`

		`return flask.redirect(return_point)`

		`@@ -2663,7 +2669,10 @@`
		`'No such user %s found' % new_username)`
		`try:`
		`old_main_admin = repo.user.user`
		`- pagure.lib.set_project_owner(flask.g.session, repo, new_owner)`
		`+ pagure.lib.set_project_owner(`
		`+ flask.g.session, repo, new_owner,`
		`+ required_groups=pagure_config.get('REQUIRED_GROUPS')`
		`+ )`
		`# If the person doing the action is the former main admin, keep`
		`# them as admins`
		`if flask.g.fas_user.username == old_main_admin:`
		`@@ -2674,6 +2683,10 @@`
		`pagure.lib.git.generate_gitolite_acls(project=repo)`
		`flask.flash(`
		`'The project has been transferred to %s' % new_username)`
		`+ except pagure.exceptions.PagureException as msg:`
		`+ flask.g.session.rollback()`
		`+ _log.debug(msg)`
		`+ flask.flash(str(msg), 'error')`
		`except SQLAlchemyError: # pragma: no cover`
		`flask.g.session.rollback()`
		`flask.flash(`

tests/test_pagure_flask_ui_app_give_project.py

file modified

+90

		`@@ -43,6 +43,7 @@`

		`tests.create_projects(self.session)`
		`tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)`
		`+ self._check_user(user='pingou')`

		`def _check_user(self, user='pingou'):`
		`self.session.commit()`
		`@@ -322,6 +323,95 @@`
		`self.assertEqual(len(project.users), 1)`
		`self.assertEqual(project.users[0].user, 'pingou')`

		`+ @patch.dict('pagure.config.config', {'REQUIRED_GROUPS': {'*': ['packager']}})`
		`+ @patch.dict('pagure.config.config', {'PAGURE_ADMIN_USERS': 'foo'})`
		`+ @patch('pagure.lib.git.generate_gitolite_acls', MagicMock())`
		`+ def test_give_project_not_in_required_group(self):`
		`+ """ Test the give_project endpoint. """`
		`+`
		`+ user = tests.FakeUser()`
		`+ user.username = 'pingou'`
		`+ with tests.user_set(self.app.application, user):`
		`+ csrf_token = self.get_csrf()`
		`+`
		`+ self._check_user()`
		`+`
		`+ # User not a packager`
		`+ 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(`
		`+ '</i> This user must be in one of the following groups to '`
		`+ 'be allowed to be added to this project: packager</div>',`
		`+ output.get_data(as_text=True))`
		`+`
		`+ self._check_user(user='pingou')`
		`+`
		`+ @patch.dict('pagure.config.config', {'REQUIRED_GROUPS': {'*': ['packager']}})`
		`+ @patch.dict('pagure.config.config', {'PAGURE_ADMIN_USERS': 'foo'})`
		`+ @patch('pagure.lib.git.generate_gitolite_acls', MagicMock())`
		`+ def test_give_project_in_required_group(self):`
		`+ """ Test the give_project endpoint. """`
		`+`
		`+ # Create the packager group`
		`+ msg = pagure.lib.add_group(`
		`+ self.session,`
		`+ group_name='packager',`
		`+ display_name='packager group',`
		`+ description=None,`
		`+ group_type='user',`
		`+ user='pingou',`
		`+ is_admin=False,`
		`+ blacklist=[],`
		`+ )`
		`+ self.session.commit()`
		+ self.assertEqual(msg, 'User `pingou` added to the group `packager`.')
		`+`
		`+ # Add foo to the packager 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=False,`
		`+ )`
		`+ self.session.commit()`
		+ self.assertEqual(msg, 'User `foo` added to the group `packager`.')
		`+`
		`+ # pingou transferts test to foo`
		`+ user = tests.FakeUser()`
		`+ user.username = 'pingou'`
		`+ with tests.user_set(self.app.application, user):`
		`+ csrf_token = self.get_csrf()`
		`+`
		`+ self._check_user()`
		`+`
		`+ # User not a packager`
		`+ 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(`
		`+ '</i> The project has been transferred to foo</div>',`
		`+ output.get_data(as_text=True))`
		`+`
		`+ self._check_user('foo')`
		`+ # Make sure that the user giving the project is still an admin`
		`+ project = pagure.lib.get_authorized_project(`
		`+ self.session, project_name='test')`
		`+ self.assertEqual(len(project.users), 1)`
		`+ self.assertEqual(project.users[0].user, 'pingou')`
		`+`

		`if __name__ == '__main__':`
		`unittest.main(verbosity=2)`