From 61ac283c383c844b47a6124498e6b18b516a28eb Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 31 2018 12:13:10 +0000 Subject: [PATCH 1/6] Fix the order of the before_request functions Local authentication needs to call _check_session_cookie before the request is ran but that call requires a session to connect to the database with and that is done in set_request so instead of ensuring _check_session_cookie is always run first, make it be after set_request. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/flask_app.py b/pagure/flask_app.py index 14e00a4..088bbfc 100644 --- a/pagure/flask_app.py +++ b/pagure/flask_app.py @@ -149,7 +149,7 @@ def create_app(config=None): # 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_funcs[None].insert(0, login._check_session_cookie) + app.before_request(login._check_session_cookie) app.after_request(login._send_session_cookie) if perfrepo: From 10406faf8063dfad7489214c61cfc0cbaad83f5e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 31 2018 12:13:10 +0000 Subject: [PATCH 2/6] Rename the session variable to visit_session This way it is less confusing with the DB session. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/login.py b/pagure/ui/login.py index 851d9a2..43c224c 100644 --- a/pagure/ui/login.py +++ b/pagure/ui/login.py @@ -414,23 +414,23 @@ def _check_session_cookie(): if cookie_name and cookie_name in flask.request.cookies: sessionid = flask.request.cookies.get(cookie_name) - session = pagure.lib.login.get_session_by_visitkey( + visit_session = pagure.lib.login.get_session_by_visitkey( flask.g.session, sessionid) - if session and session.user: + if visit_session and visit_session.user: now = datetime.datetime.now() - if now > session.expiry: + if now > visit_session.expiry: flask.flash('Session timed-out', 'error') elif pagure.config.config.get('CHECK_SESSION_IP', True) \ - and session.user_ip != flask.request.remote_addr: + and visit_session.user_ip != flask.request.remote_addr: flask.flash('Session expired', 'error') else: new_expiry = now + datetime.timedelta(days=30) - session_id = session.visit_key - user = session.user - login_time = session.created + session_id = visit_session.visit_key + user = visit_session.user + login_time = visit_session.created - session.expiry = new_expiry - flask.g.session.add(session) + visit_session.expiry = new_expiry + flask.g.session.add(visit_session) try: flask.g.session.commit() except SQLAlchemyError as err: # pragma: no cover From de8a0a0635b722ae1f824b66b5674e58935617c5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 31 2018 12:13:10 +0000 Subject: [PATCH 3/6] Reduce the number of calls to authenticated() in favor of flask.g This reduces a little bit the number of duplicated calls. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/flask_app.py b/pagure/flask_app.py index 088bbfc..2891c87 100644 --- a/pagure/flask_app.py +++ b/pagure/flask_app.py @@ -240,9 +240,9 @@ def set_request(): flask.g.new_user = True flask.session['_new_user'] = False + flask.g.authenticated = pagure.utils.authenticated() flask.g.admin = pagure.utils.is_admin() - flask.g.authenticated = pagure.utils.authenticated() # Retrieve the variables in the URL args = flask.request.view_args or {} # Check if there is a `repo` and an `username` @@ -255,7 +255,7 @@ def set_request(): if repo: flask.g.repo = pagure.lib.get_authorized_project( flask.g.session, repo, user=username, namespace=namespace) - if pagure.utils.authenticated(): + if flask.g.authenticated: flask.g.repo_forked = pagure.lib.get_authorized_project( flask.g.session, repo, user=flask.g.fas_user.username, namespace=namespace) diff --git a/pagure/utils.py b/pagure/utils.py index f58057a..1485452 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -49,7 +49,7 @@ def is_safe_url(target): # pragma: no cover def is_admin(): """ Return whether the user is admin for this application or not. """ - if not authenticated(): + if not flask.g.authenticated: return False user = flask.g.fas_user From b99552044bcf8a114999685d69e23f9b3383fe0c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 31 2018 12:13:10 +0000 Subject: [PATCH 4/6] Set the end_request be called at the end of the request earlier. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/flask_app.py b/pagure/flask_app.py index 2891c87..1aa8a78 100644 --- a/pagure/flask_app.py +++ b/pagure/flask_app.py @@ -145,6 +145,7 @@ def create_app(config=None): app.register_blueprint(PV) app.before_request(set_request) + app.after_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': @@ -155,7 +156,6 @@ def create_app(config=None): if perfrepo: # Do this at the very end, so that this after_request comes last. app.after_request(perfrepo.print_stats) - app.after_request(end_request) app.add_url_rule('/login/', view_func=auth_login, methods=['GET', 'POST']) app.add_url_rule('/logout/', view_func=auth_logout) From bcc20eb15d822c63b77dccb7b8544321186e470a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 31 2018 12:13:10 +0000 Subject: [PATCH 5/6] Fix the local login Ensure the local login flask controller is imported when it should be Fix running the tests for local login Signed-off-by: Pierre-Yves Chibon Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/__init__.py b/pagure/ui/__init__.py index 4080a88..b0a9047 100644 --- a/pagure/ui/__init__.py +++ b/pagure/ui/__init__.py @@ -21,6 +21,8 @@ if pagure.config.config.get('ENABLE_TICKETS', True): import pagure.ui.issues # noqa: E402 import pagure.ui.plugins # noqa: E402 import pagure.ui.repo # noqa: E402 +if pagure.config.config['PAGURE_AUTH'] == 'local': + import pagure.ui.login # noqa: E402 @UI_NS.errorhandler(404) diff --git a/pagure/ui/login.py b/pagure/ui/login.py index 43c224c..de3e5b9 100644 --- a/pagure/ui/login.py +++ b/pagure/ui/login.py @@ -440,8 +440,9 @@ def _check_session_cookie(): _log.exception(err) flask.g.fas_session_id = session_id - flask.g.fas_user = user if user: + flask.g.fas_user = user + flask.g.authenticated = pagure.utils.authenticated() flask.g.fas_user.login_time = login_time diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index 36fade0..e9abbfe 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -38,6 +38,27 @@ import pagure.ui.login class PagureFlaskLogintests(tests.SimplePagureTest): """ Tests for flask app controller of pagure """ + def setUp(self): + """ Create the application with PAGURE_AUTH being local. """ + super(PagureFlaskLogintests, self).setUp() + + app = pagure.flask_app.create_app({ + 'DB_URL': self.dbpath, + 'PAGURE_AUTH': 'local' + }) + # Remove the log handlers for the tests + app.logger.handlers = [] + + self.app = app.test_client() + + @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) + def test_front_page(self): + """ Test the front page. """ + # First access the front page + output = self.app.get('/') + self.assertEqual(output.status_code, 200) + self.assertIn('Home - Pagure', output.data) + @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) def test_new_user(self): @@ -440,6 +461,7 @@ class PagureFlaskLogintests(tests.SimplePagureTest): self.assertIn('Login - Pagure', output.data) self.assertIn('Password changed', output.data) + @patch('pagure.ui.login._check_session_cookie', MagicMock(return_value=True)) @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'}) def test_change_password(self): """ Test the change_password endpoint. """ From ecfd557459a75085e8dc51b4686e6aeabb9bc6c0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 31 2018 12:13:10 +0000 Subject: [PATCH 6/6] Fix checking if the user is authenticated Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/repo_master.html b/pagure/templates/repo_master.html index 76e7904..ad33a25 100644 --- a/pagure/templates/repo_master.html +++ b/pagure/templates/repo_master.html @@ -358,7 +358,7 @@ {% endif %} - {% if authenticated and repo.settings.get('pull_request_access_only') %} + {% if g.authenticated and repo.settings.get('pull_request_access_only') %}