From 44158ded730957ea29280793c841b61ae67d1976 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 14 2018 08:09:20 +0000 Subject: [PATCH 1/2] Move earlier in the stack the methods used to authenticate with local accounts Without this change, too much of the requests had gone through before the methods checking if the user is authenticated were called and thus a number of actions requiring the user to be authenticated were not available. Fixes https://pagure.io/pagure/issue/3290 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/flask_app.py b/pagure/flask_app.py index 7d8f484..ef29267 100644 --- a/pagure/flask_app.py +++ b/pagure/flask_app.py @@ -121,6 +121,11 @@ def create_app(config=None): from pagure.ui.oidc_login import oidc, fas_user_from_oidc oidc.init_app(app) app.before_request(fas_user_from_oidc) + if auth == 'local': + # Only import the login controller if the app is set up for local login + import pagure.ui.login as login + app.before_request(login._check_session_cookie) + app.after_request(login._send_session_cookie) # Report error by email if not app.debug and not pagure_config.get('DEBUG', False): @@ -153,12 +158,6 @@ def create_app(config=None): app.before_request(set_request) app.teardown_request(end_request) - # Only import the login controller if the app is set up for local login - if pagure_config.get('PAGURE_AUTH', None) == 'local': - import pagure.ui.login as login - app.before_request(login._check_session_cookie) - app.after_request(login._send_session_cookie) - if perfrepo: # Do this at the very end, so that this after_request comes last. app.after_request(perfrepo.print_stats) @@ -223,8 +222,9 @@ def logout(): def set_request(): """ Prepare every request. """ flask.session.permanent = True - flask.g.session = pagure.lib.create_session( - flask.current_app.config['DB_URL']) + if not hasattr(flask.g, 'session') or not flask.g.session: + flask.g.session = pagure.lib.create_session( + flask.current_app.config['DB_URL']) flask.g.version = pagure.__version__ flask.g.confirmationform = pagure.forms.ConfirmationForm() diff --git a/pagure/ui/login.py b/pagure/ui/login.py index 89f6c64..591a658 100644 --- a/pagure/ui/login.py +++ b/pagure/ui/login.py @@ -413,6 +413,10 @@ def logout(): def _check_session_cookie(): """ Set the user into flask.g if the user is logged in. """ + if not hasattr(flask.g, 'session') or not flask.g.session: + flask.g.session = pagure.lib.create_session( + flask.current_app.config['DB_URL']) + cookie_name = pagure.config.config.get('SESSION_COOKIE_NAME', 'pagure') cookie_name = '%s_local_cookie' % cookie_name session_id = None From 1baa7f8479a459fbd116c4872eb0771cf594ca79 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 14 2018 08:09:20 +0000 Subject: [PATCH 2/2] Add some more tests for local auth Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index d371d11..046308c 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -316,6 +316,101 @@ class PagureFlaskLogintests(tests.SimplePagureTest): 'href="/logout/?next=http://localhost/">', output_text) @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) + @patch.dict('pagure.config.config', {'CHECK_SESSION_IP': False}) + def test_do_login_and_redirect(self): + """ Test the do_login endpoint with a non-default redirect. """ + # This has all the data needed + data = { + 'username': 'foouser', + 'password': 'barpass', + 'csrf_token': self.get_csrf(url='/login/'), + 'next_url': 'http://localhost/test/', + } + + # Create a local user + self.test_new_user() + self.session.commit() + + # Confirm the user so that we can log in + item = pagure.lib.search_user(self.session, username='foouser') + self.assertEqual(item.user, 'foouser') + self.assertNotEqual(item.token, None) + + # Remove the token + item.token = None + self.session.add(item) + self.session.commit() + + # Check the user + item = pagure.lib.search_user(self.session, username='foouser') + self.assertEqual(item.user, 'foouser') + self.assertEqual(item.token, None) + + # Add a test project to the user + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos')) + output = self.app.get('/test') + output_text = output.get_data(as_text=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Overview - test - Pagure', output_text) + + # Login and redirect to the test project + output = self.app.post( + '/dologin', data=data, follow_redirects=True, + environ_base={'REMOTE_ADDR': '127.0.0.1'}) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Overview - test - Pagure', output_text) + self.assertIn( + '', output_text) + self.assertIn( + 'Settings', output_text) + + @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) + @patch.dict('pagure.config.config', {'CHECK_SESSION_IP': False}) + def test_has_settings(self): + """ Test that user can see the Settings button when they are logged + in. """ + # Create a local user + self.test_new_user() + self.session.commit() + + # Remove the token + item = pagure.lib.search_user(self.session, username='foouser') + item.token = None + self.session.add(item) + self.session.commit() + + # Check the user + item = pagure.lib.search_user(self.session, username='foouser') + self.assertEqual(item.user, 'foouser') + self.assertEqual(item.token, None) + + # Add a test project to the user + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos')) + output = self.app.get('/test') + output_text = output.get_data(as_text=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Overview - test - Pagure', output_text) + + # Login and redirect to the test project + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + output = self.app.get('/test') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Overview - test - Pagure', output_text) + self.assertIn( + 'Settings', + output_text) + + @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) def test_non_ascii_password(self): """ Test login and create user functionality when the password is @@ -803,7 +898,6 @@ class PagureFlaskLogintests(tests.SimplePagureTest): g.fas_user = user self.assertFalse(pagure.flask_app.admin_session_timedout()) - @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) def test_force_logout(self): """ Test forcing logout. """