From a04944f9399994710eacbe65bf464021e4c27920 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 05 2018 14:42:50 +0000 Subject: Fix redirecting the user when they remove themselves from a project We used to redirect the user to the settings page but if the user remove themselves from the project, they can no longer access this page and they end up with a nice 403 page. With this commit, we check if they are removing themselves and if they do we simply redirect them to the front page of the project. Fixes https://pagure.io/pagure/issue/2937 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/flask_app.py b/pagure/flask_app.py index 1aa8a78..935f167 100644 --- a/pagure/flask_app.py +++ b/pagure/flask_app.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2014-2017 - Copyright Red Hat Inc + (c) 2014-2018 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -400,3 +400,12 @@ def end_request(response): gc.collect() return response + + +def _get_user(username): + """ Check if user exists or not + """ + try: + return pagure.lib.get_user(flask.g.session, username) + except pagure.exceptions.PagureException as e: + flask.abort(404, e.message) diff --git a/pagure/ui/app.py b/pagure/ui/app.py index 986260b..2b4a65c 100644 --- a/pagure/ui/app.py +++ b/pagure/ui/app.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2014-2017 - Copyright Red Hat Inc + (c) 2014-2018 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -22,7 +22,7 @@ import pagure.lib.git import pagure.forms import pagure.ui.filters from pagure.config import config as pagure_config -from pagure.flask_app import admin_session_timedout +from pagure.flask_app import _get_user, admin_session_timedout from pagure.ui import UI_NS from pagure.utils import ( authenticated, @@ -34,15 +34,6 @@ from pagure.utils import ( _log = logging.getLogger(__name__) -def _get_user(username): - """ Check if user exists or not - """ - try: - return pagure.lib.get_user(flask.g.session, username) - except pagure.exceptions.PagureException as e: - flask.abort(404, e.message) - - def _filter_acls(repos, acl, user): """ Filter the given list of repositories to return only the ones where the user has the specified acl. diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 45b6bda..8db2a44 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2014-2017 - Copyright Red Hat Inc + (c) 2014-2018 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -49,6 +49,7 @@ import pagure.lib.tasks import pagure.forms import pagure.ui.plugins from pagure.config import config as pagure_config +from pagure.flask_app import _get_user from pagure.lib import encoding_utils from pagure.ui import UI_NS from pagure.utils import ( @@ -1534,11 +1535,15 @@ def remove_user(repo, userid, username=None, namespace=None): if not pagure_config.get('ENABLE_USER_MNGT', True): flask.abort(404, 'User management not allowed in the pagure instance') + current_user = _get_user(username=flask.g.fas_user.username) + repo = flask.g.repo form = pagure.forms.ConfirmationForm() + delete_themselves = False if form.validate_on_submit(): userids = [str(user.id) for user in repo.users] + delete_themselves = str(current_user.id) in userids if str(userid) not in userids: flask.flash('User does not have any access on the repo', 'error') @@ -1573,9 +1578,11 @@ def remove_user(repo, userid, username=None, namespace=None): _log.exception(err) flask.flash('User could not be removed', 'error') + endpoint = 'ui_ns.view_settings' + if delete_themselves: + endpoint = 'ui_ns.view_repo' return flask.redirect(flask.url_for( - 'ui_ns.view_settings', repo=repo.name, username=username, - namespace=namespace)) + endpoint, repo=repo.name, username=username, namespace=namespace)) @UI_NS.route('//adddeploykey/', methods=('GET', 'POST')) diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index 922b6f2..33c9a9f 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -723,6 +723,51 @@ class PagureFlaskRepotests(tests.Modeltests): mock_log.assert_called_with(ANY, topic='project.user.removed', msg=ANY) @patch('pagure.decorators.admin_session_timedout') + @patch('pagure.lib.notify.log') + def test_remove_user_self(self, mock_log, ast): + """ Test the remove_user endpoint when removing themselves. """ + ast.return_value = False + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos')) + + # Add an user to a project + repo = pagure.lib.get_authorized_project(self.session, 'test') + self.assertEqual(len(repo.users), 0) + msg = pagure.lib.add_user_to_project( + session=self.session, + project=repo, + new_user='foo', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg, 'User added') + self.assertEqual(len(repo.users), 1) + + # Let user foo remove themselves + user = tests.FakeUser(username='foo') + with tests.user_set(self.app.application, user): + csrf_token = self.get_csrf() + data = {'csrf_token': csrf_token} + + output = self.app.post( + '/test/dropuser/2', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + u'Overview - test - Pagure', output.data) + self.assertIn( + u'

\n test', + output.data) + self.assertIn( + u'\n User removed', output.data) + + self.session = pagure.lib.create_session(self.dbpath) + repo = pagure.lib.get_authorized_project(self.session, 'test') + self.assertEqual(len(repo.users), 0) + + mock_log.assert_called_with(ANY, topic='project.user.removed', msg=ANY) + + @patch('pagure.decorators.admin_session_timedout') def test_remove_group_project_when_user_mngt_off(self, ast): """ Test the remove_group_project endpoint when user management is turned off in the pagure instance"""