From 3880be9a200d8e5c85ce1bacd1244f9a971a65ab Mon Sep 17 00:00:00 2001
From: Julian Rother <julian@cccv.de>
Date: Mon, 28 Feb 2022 01:12:50 +0100
Subject: [PATCH] Fix regression in service overview access behavior
When the service overview was introduced, it was meant to be optional. Thus
if the SERVICES config option was empty (the default), uffd returned 404.
Commit fa67bde (Migrate OAuth2 and API clients to database) introduced the
regression that accessing the service overview page when no services are
visible based on the permissions of the current user (or guest if not logged
in), 404 is returned.
This change fixes the regression and further changes the behavior to improve
consistency. Since fa67bde, the page is relevant to admin users regardless of
the SERVICES config option. Therefore uffd asks for login or reports missing
permissions in all cases it originally returned 404.
---
tests/test_services.py | 55 +++++++++++++++++++-
tests/utils.py | 3 ++
uffd/service/templates/service/overview.html | 2 +-
uffd/service/views.py | 52 ++++++++++--------
4 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/tests/test_services.py b/tests/test_services.py
index 269d1455..efd71898 100644
--- a/tests/test_services.py
+++ b/tests/test_services.py
@@ -42,12 +42,63 @@ class TestServices(UffdTestCase):
def test_overview(self):
r = self.client.get(path=url_for('service.overview'))
- dump('service_overview_public', r)
+ dump('service_overview_guest', r)
self.assertEqual(r.status_code, 200)
self.assertNotIn(b'https://example.com/', r.data)
self.login_as('user')
r = self.client.get(path=url_for('service.overview'))
- dump('service_overview', r)
+ dump('service_overview_user', r)
self.assertEqual(r.status_code, 200)
self.assertIn(b'https://example.com/', r.data)
+ def test_overview_disabled(self):
+ self.app.config['SERVICES'] = []
+ # Should return login page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_disabled_guest', r)
+ self.assertEqual(r.status_code, 200)
+ self.assertIn(b'name="password"', r.data)
+ self.login_as('user')
+ # Should return access denied page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_disabled_user', r)
+ self.assertEqual(r.status_code, 403)
+ self.login_as('admin')
+ # Should return (empty) overview page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_disabled_admin', r)
+ self.assertEqual(r.status_code, 200)
+
+ def test_overview_nonpublic(self):
+ self.app.config['SERVICES_PUBLIC'] = False
+ # Should return login page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_nonpublic_guest', r)
+ self.assertEqual(r.status_code, 200)
+ self.assertIn(b'name="password"', r.data)
+ self.login_as('user')
+ # Should return overview page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_nonpublic_user', r)
+ self.assertEqual(r.status_code, 200)
+ self.login_as('admin')
+ # Should return overview page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_nonpublic_admin', r)
+ self.assertEqual(r.status_code, 200)
+
+ def test_overview_public(self):
+ # Should return overview page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_public_guest', r)
+ self.assertEqual(r.status_code, 200)
+ self.login_as('user')
+ # Should return overview page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_public_user', r)
+ self.assertEqual(r.status_code, 200)
+ self.login_as('admin')
+ # Should return overview page
+ r = self.client.get(path=url_for('service.overview'), follow_redirects=True)
+ dump('service_overview_public_admin', r)
+ self.assertEqual(r.status_code, 200)
diff --git a/tests/utils.py b/tests/utils.py
index b043f3af..c9ec3e92 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -44,6 +44,9 @@ class UffdTestCase(unittest.TestCase):
return Mail.query.filter_by(uid='test').one_or_none()
def login_as(self, user, ref=None):
+ # It is currently not possible to login while already logged in as another
+ # user, so make sure that we are not logged in first
+ self.client.get(path=url_for('session.logout'), follow_redirects=True)
loginname = None
password = None
if user == 'user':
diff --git a/uffd/service/templates/service/overview.html b/uffd/service/templates/service/overview.html
index 67795006..5ed19e38 100644
--- a/uffd/service/templates/service/overview.html
+++ b/uffd/service/templates/service/overview.html
@@ -4,7 +4,7 @@
{% set iconstyle = 'style="width: 1.8em;"'|safe %}
-{% if not user %}
+{% if not request.user %}
<div class="alert alert-warning" role="alert">{{_("Some services may not be publicly listed! Log in to see all services you have access to.")}}</div>
{% endif %}
diff --git a/uffd/service/views.py b/uffd/service/views.py
index 1f5623ca..cd015833 100644
--- a/uffd/service/views.py
+++ b/uffd/service/views.py
@@ -1,3 +1,5 @@
+import functools
+
from flask import Blueprint, render_template, request, url_for, redirect, current_app, abort
from flask_babel import lazy_gettext
@@ -12,32 +14,40 @@ from uffd.database import db
bp = Blueprint('service', __name__, template_folder='templates')
-def service_admin_acl():
+def admin_acl():
return request.user and request.user.is_in_group(current_app.config['ACL_ADMIN_GROUP'])
-def service_overview_acl():
- return len(get_services(request.user)) > 0 or service_admin_acl()
+def overview_login_maybe_required(func):
+ @functools.wraps(func)
+ def decorator(*args, **kwargs):
+ if not current_app.config['SERVICES']:
+ return login_required(admin_acl)(func)(*args, **kwargs)
+ if not current_app.config['SERVICES_PUBLIC']:
+ return login_required()(func)(*args, **kwargs)
+ return func(*args, **kwargs)
+ return decorator
+
+def overview_navbar_visible():
+ return get_services(request.user) != [] or admin_acl()
@bp.route('/services/')
-@register_navbar(lazy_gettext('Services'), icon='sitemap', blueprint=bp, visible=service_overview_acl)
+@register_navbar(lazy_gettext('Services'), icon='sitemap', blueprint=bp, visible=overview_navbar_visible)
+@overview_login_maybe_required
def overview():
services = get_services(request.user)
- if not service_overview_acl():
- abort(404)
- banner = current_app.config.get('SERVICES_BANNER')
- # Set the banner to None if it is not public and no user is logged in
- if not (current_app.config['SERVICES_BANNER_PUBLIC'] or request.user):
- banner = None
- return render_template('service/overview.html', user=request.user, services=services, banner=banner)
+ banner = ''
+ if request.user or current_app.config['SERVICES_BANNER_PUBLIC']:
+ banner = current_app.config['SERVICES_BANNER']
+ return render_template('service/overview.html', services=services, banner=banner)
@bp.route('/service/admin')
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def index():
return render_template('service/index.html', services=Service.query.all())
@bp.route('/service/new')
@bp.route('/service/<int:id>')
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def show(id=None):
service = Service() if id is None else Service.query.get_or_404(id)
all_groups = Group.query.all()
@@ -46,7 +56,7 @@ def show(id=None):
@bp.route('/service/new', methods=['POST'])
@bp.route('/service/<int:id>', methods=['POST'])
@csrf_protect(blueprint=bp)
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def edit_submit(id=None):
if id is None:
service = Service()
@@ -68,7 +78,7 @@ def edit_submit(id=None):
@bp.route('/service/<int:id>/delete')
@csrf_protect(blueprint=bp)
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def delete(id):
service = Service.query.get_or_404(id)
db.session.delete(service)
@@ -77,7 +87,7 @@ def delete(id):
@bp.route('/service/<int:service_id>/oauth2/new')
@bp.route('/service/<int:service_id>/oauth2/<int:db_id>')
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def oauth2_show(service_id, db_id=None):
service = Service.query.get_or_404(service_id)
client = OAuth2Client() if db_id is None else OAuth2Client.query.filter_by(service_id=service_id, db_id=db_id).first_or_404()
@@ -86,7 +96,7 @@ def oauth2_show(service_id, db_id=None):
@bp.route('/service/<int:service_id>/oauth2/new', methods=['POST'])
@bp.route('/service/<int:service_id>/oauth2/<int:db_id>', methods=['POST'])
@csrf_protect(blueprint=bp)
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def oauth2_submit(service_id, db_id=None):
service = Service.query.get_or_404(service_id)
if db_id is None:
@@ -112,7 +122,7 @@ def oauth2_submit(service_id, db_id=None):
@bp.route('/service/<int:service_id>/oauth2/<int:db_id>/delete')
@csrf_protect(blueprint=bp)
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def oauth2_delete(service_id, db_id=None):
service = Service.query.get_or_404(service_id)
client = OAuth2Client.query.filter_by(service_id=service_id, db_id=db_id).first_or_404()
@@ -122,7 +132,7 @@ def oauth2_delete(service_id, db_id=None):
@bp.route('/service/<int:service_id>/api/new')
@bp.route('/service/<int:service_id>/api/<int:id>')
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def api_show(service_id, id=None):
service = Service.query.get_or_404(service_id)
client = APIClient() if id is None else APIClient.query.filter_by(service_id=service_id, id=id).first_or_404()
@@ -131,7 +141,7 @@ def api_show(service_id, id=None):
@bp.route('/service/<int:service_id>/api/new', methods=['POST'])
@bp.route('/service/<int:service_id>/api/<int:id>', methods=['POST'])
@csrf_protect(blueprint=bp)
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def api_submit(service_id, id=None):
service = Service.query.get_or_404(service_id)
if id is None:
@@ -152,7 +162,7 @@ def api_submit(service_id, id=None):
@bp.route('/service/<int:service_id>/api/<int:id>/delete')
@csrf_protect(blueprint=bp)
-@login_required(service_admin_acl)
+@login_required(admin_acl)
def api_delete(service_id, id=None):
service = Service.query.get_or_404(service_id)
client = APIClient.query.filter_by(service_id=service_id, id=id).first_or_404()
--
GitLab