PR#3318: Move earlier in the stack the methods used to authenticate with local accounts - pagure

pagure

#3318 Move earlier in the stack the methods used to authenticate with local accounts

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

fix_auth_check into master

Add some more tests for local auth

Pierre-Yves Chibon • 6 years ago

1baa7f8

Move earlier in the stack the methods used to authenticate with local accounts

Pierre-Yves Chibon • 6 years ago

44158de

pagure/flask_app.py

file modified

+8 -8

		`@@ -121,6 +121,11 @@`
		`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 @@`
		`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 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()`

pagure/ui/login.py

file modified

		`@@ -413,6 +413,10 @@`
		`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`

tests/test_pagure_flask_ui_login.py

file modified

+95 -1

		`@@ -316,6 +316,101 @@`
		`'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(`
		`+ '<title>Overview - test - Pagure</title>', 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(`
		`+ '<title>Overview - test - Pagure</title>', output_text)`
		`+ self.assertIn(`
		`+ '<a class="dropdown-item" '`
		`+ 'href="/logout/?next=http://localhost/test/">', output_text)`
		`+ self.assertIn(`
		`+ '<span class="d-none d-md-inline">Settings</span>', 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(`
		`+ '<title>Overview - test - Pagure</title>', 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(`
		`+ '<title>Overview - test - Pagure</title>', output_text)`
		`+ self.assertIn(`
		`+ '<span class="d-none d-md-inline">Settings</span>',`
		`+ 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 @@`
		`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. """`

pingou commented 6 years ago

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 pingou@pingoured.fr

1 new commit added