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 fa67bde0 (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 fa67bde0, 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

@@ -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)

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':

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 %}
 

uffd/service/views.py

+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()