From b0e3aaa35631c6425d43e08e1d271b5d5336e804 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Tue, 12 Oct 2021 20:19:57 -0700 Subject: [PATCH] middleware: Don't show setup view to non-admin users This is an improvement over !2069, which solved #2094. Tests: - Install an app. Success result is shown. - Install an app and kill the apt-get process in the middle. Error result is shown. - Click install on an app (email_server). Close the window without seeing the result. Access the app page (like email_server/my_aliases) as a non-admin user. No success/file message is shown. The page is shown properly. Access the page as admin, success message is shown. - Access an uninstalled/installed app page as anonymous user. User is redirected to login page. - Access an uninstalled/installed app page as non-admin user. Forbidden page is shown. - Access an uninstalled app page meant for non-admin users (such as email_server/my_aliases) as non-admin user. Forbidden page is shown. - Access an installed app page as admin. Success. - Access an installed app page meant for non-admin users (such as email_server/my_aliases) as admin. Success. Signed-off-by: Sunil Mohan Adapa Reviewed-by: Fioddor Superconcentrado --- plinth/middleware.py | 49 ++++++++++++++++----------- plinth/tests/test_middleware.py | 60 +++++++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 26 deletions(-) diff --git a/plinth/middleware.py b/plinth/middleware.py index 208dd66a2..963990cc2 100644 --- a/plinth/middleware.py +++ b/plinth/middleware.py @@ -25,6 +25,29 @@ from . import views logger = logging.getLogger(__name__) +def _collect_setup_result(request, module): + """Show success/fail message from previous install operation.""" + if not module.setup_helper.is_finished: + return + + exception = module.setup_helper.collect_result() + if not exception: + if not setup._is_module_essential(module): + messages.success(request, _('Application installed.')) + else: + if isinstance(exception, PackageException): + error_string = getattr(exception, 'error_string', str(exception)) + error_details = getattr(exception, 'error_details', '') + message = _('Error installing application: {string} ' + '{details}').format(string=error_string, + details=error_details) + else: + message = _('Error installing application: {error}') \ + .format(error=exception) + + messages.error(request, message) + + class SetupMiddleware(MiddlewareMixin): """Django middleware to show pre-setup message and setup progress.""" @@ -51,30 +74,18 @@ class SetupMiddleware(MiddlewareMixin): module_name = resolver_match.namespaces[0] module = plinth.module_loader.loaded_modules[module_name] - # Collect errors from any previous operations and show them - if module.setup_helper.is_finished: - exception = module.setup_helper.collect_result() - if not exception: - if not setup._is_module_essential(module): - messages.success(request, _('Application installed.')) - else: - if isinstance(exception, PackageException): - error_string = getattr(exception, 'error_string', - str(exception)) - error_details = getattr(exception, 'error_details', '') - message = _('Error installing application: {string} ' - '{details}').format(string=error_string, - details=error_details) - else: - message = _('Error installing application: {error}') \ - .format(error=exception) - - messages.error(request, message) + is_admin = is_user_admin(request) + # Collect and show setup operation result to admins + if is_admin: + _collect_setup_result(request, module) # Check if application is up-to-date if module.setup_helper.get_state() == 'up-to-date': return + if not is_admin: + raise PermissionDenied + # Only allow logged-in users to access any setup page view = login_required(views.SetupView.as_view()) return view(request, setup_helper=module.setup_helper) diff --git a/plinth/tests/test_middleware.py b/plinth/tests/test_middleware.py index 7a5e8b855..f3dc3157f 100644 --- a/plinth/tests/test_middleware.py +++ b/plinth/tests/test_middleware.py @@ -6,7 +6,7 @@ Test module for custom middleware. from unittest.mock import MagicMock, Mock, patch import pytest -from django.contrib.auth.models import AnonymousUser, User +from django.contrib.auth.models import AnonymousUser, Group, User from django.core.exceptions import PermissionDenied from django.http import HttpResponse from django.test.client import RequestFactory @@ -64,6 +64,7 @@ class TestSetupMiddleware: loaded_modules.__getitem__.return_value = module request = RequestFactory().get('/plinth/mockapp') + request.user = AnonymousUser() response = middleware.process_view(request, **kwargs) assert response is None @@ -72,6 +73,7 @@ class TestSetupMiddleware: @patch('plinth.module_loader.loaded_modules') @patch('django.urls.resolve') @patch('django.urls.reverse', return_value='users:login') + @pytest.mark.django_db def test_module_view(reverse, resolve, loaded_modules, setup_view, middleware, kwargs): """Test that only registered users can access the setup view.""" @@ -82,16 +84,36 @@ class TestSetupMiddleware: view = Mock() setup_view.as_view.return_value = view request = RequestFactory().get('/plinth/mockapp') + request.session = MagicMock() # Verify that anonymous users cannot access the setup page request.user = AnonymousUser() - middleware.process_view(request, **kwargs) - setup_view.as_view.assert_called_once_with() + with pytest.raises(PermissionDenied): + middleware.process_view(request, **kwargs) + + setup_view.as_view.assert_not_called() view.assert_not_called() - # Verify that logged-in users can access the setup page - request.user = User(username='johndoe') + # Verify that non-admin logged-in users can't access the setup page + user = User(username='johndoe') + user.save() + request.user = user + with pytest.raises(PermissionDenied): + middleware.process_view(request, **kwargs) + + setup_view.as_view.assert_not_called() + view.assert_not_called() + + # Verify that admin logged-in users can access the setup page + user = User(username='adminuser') + user.save() + group = Group(name='admin') + group.save() + user.groups.add(group) + user.save() + request.user = user middleware.process_view(request, **kwargs) + setup_view.as_view.assert_called_once_with() view.assert_called_once_with(request, setup_helper=module.setup_helper) @staticmethod @@ -99,6 +121,7 @@ class TestSetupMiddleware: @patch('plinth.module_loader.loaded_modules') @patch('django.urls.resolve') @patch('django.urls.reverse', return_value='users:login') + @pytest.mark.django_db def test_install_result_collection(reverse, resolve, loaded_modules, messages_success, middleware, kwargs): """Test that module installation result is collected properly.""" @@ -110,13 +133,36 @@ class TestSetupMiddleware: module.setup_helper.get_state.return_value = 'up-to-date' loaded_modules.__getitem__.return_value = module + # Admin user can't collect result request = RequestFactory().get('/plinth/mockapp') + user = User(username='adminuser') + user.save() + group = Group(name='admin') + group.save() + user.groups.add(group) + user.save() + request.user = user + request.session = MagicMock() response = middleware.process_view(request, **kwargs) assert response is None assert messages_success.called module.setup_helper.collect_result.assert_called_once_with() + # Non-admin user can't collect result + messages_success.reset_mock() + module.setup_helper.collect_result.reset_mock() + request = RequestFactory().get('/plinth/mockapp') + user = User(username='johndoe') + user.save() + request.user = user + request.session = MagicMock() + response = middleware.process_view(request, **kwargs) + + assert response is None + assert not messages_success.called + module.setup_helper.collect_result.assert_not_called() + class TestAdminMiddleware: """Test cases for admin middleware.""" @@ -178,8 +224,8 @@ class TestAdminMiddleware: assert response is None @staticmethod - def test_that_public_view_is_allowed_for_normal_user(web_request, - middleware, kwargs): + def test_that_public_view_is_allowed_for_normal_user( + web_request, middleware, kwargs): """Test that normal user is allowed for an public view""" kwargs = dict(kwargs) kwargs['view_func'] = public(HttpResponse)