diff --git a/plinth/middleware.py b/plinth/middleware.py index 77e11d64e..498e9b709 100644 --- a/plinth/middleware.py +++ b/plinth/middleware.py @@ -3,6 +3,8 @@ Common Django middleware. """ +import logging + from django import urls from django.conf import settings from django.contrib import messages @@ -11,14 +13,13 @@ from django.core.exceptions import PermissionDenied from django.shortcuts import render from django.utils.deprecation import MiddlewareMixin from django.utils.translation import ugettext_lazy as _ -import logging - from stronghold.utils import is_view_func_public import plinth from plinth import setup from plinth.package import PackageException from plinth.utils import is_user_admin + from . import views logger = logging.getLogger(__name__) @@ -26,6 +27,7 @@ logger = logging.getLogger(__name__) class SetupMiddleware(MiddlewareMixin): """Django middleware to show pre-setup message and setup progress.""" + @staticmethod def process_view(request, view_func, view_args, view_kwargs): """Handle a request as Django middleware request handler.""" @@ -80,6 +82,13 @@ class SetupMiddleware(MiddlewareMixin): class AdminRequiredMiddleware(MiddlewareMixin): """Django middleware for authenticating requests for admin areas.""" + + @staticmethod + def check_user_group(view_func, request): + if hasattr(view_func, 'GROUP_NAME'): + return request.user.groups.filter( + name=getattr(view_func, 'GROUP_NAME')).exists() + @staticmethod def process_view(request, view_func, view_args, view_kwargs): """Reject non-admin access to views that are private and not marked.""" @@ -88,11 +97,14 @@ class AdminRequiredMiddleware(MiddlewareMixin): return if not is_user_admin(request): - raise PermissionDenied + if not AdminRequiredMiddleware.check_user_group( + view_func, request): + raise PermissionDenied class FirstSetupMiddleware(MiddlewareMixin): """Django middleware to block all interactions before first setup.""" + @staticmethod def process_view(request, view_func, view_args, view_kwargs): """Block all user interactions when first setup is pending.""" diff --git a/plinth/modules/openvpn/__init__.py b/plinth/modules/openvpn/__init__.py index 237fec2ce..a2d6c7be9 100644 --- a/plinth/modules/openvpn/__init__.py +++ b/plinth/modules/openvpn/__init__.py @@ -76,7 +76,8 @@ class OpenVPNApp(app_module.App): 'shortcut-openvpn', info.name, short_description=info.short_description, icon=info.icon_filename, description=info.description + [download_profile], - configure_url=reverse_lazy('openvpn:index'), login_required=True) + configure_url=reverse_lazy('openvpn:index'), login_required=True, + allowed_groups=['vpn']) self.add(shortcut) firewall = Firewall('firewall-openvpn', info.name, ports=['openvpn'], diff --git a/plinth/modules/openvpn/tests/openvpn.feature b/plinth/modules/openvpn/tests/openvpn.feature index 180ecd695..70f7e8429 100644 --- a/plinth/modules/openvpn/tests/openvpn.feature +++ b/plinth/modules/openvpn/tests/openvpn.feature @@ -17,12 +17,18 @@ Scenario: Download openvpn profile Given the openvpn application is enabled Then the openvpn profile should be downloadable -Scenario: OpenVPN user group +Scenario: User of 'vpn' group Given the openvpn application is enabled When I create a user named vpnuser with password openvpnrock$0 in group vpn And I'm logged in as the user vpnuser with password openvpnrock$0 Then the openvpn profile should be downloadable +Scenario: User not of 'vpn' group + Given the openvpn application is enabled + When I create a user named nonvpnuser with password whatever@123 + And I'm logged in as the user nonvpnuser with password whatever@123 + Then openvpn app should not be visible on the front page + @backups Scenario: Backup and restore openvpn Given the openvpn application is enabled diff --git a/plinth/modules/openvpn/tests/test_functional.py b/plinth/modules/openvpn/tests/test_functional.py index 07d05b87a..31fb2c782 100644 --- a/plinth/modules/openvpn/tests/test_functional.py +++ b/plinth/modules/openvpn/tests/test_functional.py @@ -9,6 +9,9 @@ from plinth.tests import functional scenarios('openvpn.feature') +base_url = functional.config['DEFAULT']['URL'] +shortcut_href = '?selected=shortcut-openvpn' + @given('I download openvpn profile') def openvpn_download_profile(session_browser): @@ -20,6 +23,13 @@ def openvpn_profile_downloadable(session_browser): _download_profile(session_browser) +@then('openvpn app should not be visible on the front page') +def openvpn_app_not_on_front_page(session_browser): + session_browser.visit(base_url) + links = session_browser.find_link_by_href(shortcut_href) + assert len(links) == 0 + + @then('the openvpn profile downloaded should be same as before') def openvpn_profile_download_compare(session_browser, openvpn_download_profile): @@ -29,8 +39,7 @@ def openvpn_profile_download_compare(session_browser, def _download_profile(browser): """Return the content of the current user's OpenVPN profile.""" - default_url = functional.config['DEFAULT']['URL'] - browser.visit(default_url) - browser.click_link_by_href('?selected=shortcut-openvpn') - return functional.download_file( - browser, f'{default_url}/plinth/apps/openvpn/profile/') + browser.visit(base_url) + browser.click_link_by_href(shortcut_href) + profile_url = f'{base_url}/plinth/apps/openvpn/profile/' + return functional.download_file(browser, profile_url) diff --git a/plinth/modules/openvpn/urls.py b/plinth/modules/openvpn/urls.py index 6045e4607..4cdc881d1 100644 --- a/plinth/modules/openvpn/urls.py +++ b/plinth/modules/openvpn/urls.py @@ -5,7 +5,7 @@ URLs for the OpenVPN module. from django.conf.urls import url -from plinth.utils import non_admin_view +from plinth.utils import user_group_view from . import views @@ -13,6 +13,6 @@ urlpatterns = [ url(r'^apps/openvpn/$', views.OpenVPNAppView.as_view(), name='index'), url(r'^apps/openvpn/setup/$', views.setup, name='setup'), url(r'^apps/openvpn/ecc/$', views.ecc, name='ecc'), - url(r'^apps/openvpn/profile/$', non_admin_view(views.profile), + url(r'^apps/openvpn/profile/$', user_group_view(views.profile, 'vpn'), name='profile'), ] diff --git a/plinth/modules/users/tests/test_functional.py b/plinth/modules/users/tests/test_functional.py index a54ae822b..e48bbe8ab 100644 --- a/plinth/modules/users/tests/test_functional.py +++ b/plinth/modules/users/tests/test_functional.py @@ -118,12 +118,6 @@ def generate_ssh_keys(session_browser, tmp_path_factory): str(key_file)]) -@when( - parsers.parse('I create a user named {name:w} with password {password:w}')) -def create_user(session_browser, name, password): - functional.create_user(session_browser, name, password) - - @when(parsers.parse('I rename the user {old_name:w} to {new_name:w}')) def rename_user(session_browser, old_name, new_name): _rename_user(session_browser, old_name, new_name) diff --git a/plinth/tests/functional/step_definitions.py b/plinth/tests/functional/step_definitions.py index bc8740c65..abc5d883f 100644 --- a/plinth/tests/functional/step_definitions.py +++ b/plinth/tests/functional/step_definitions.py @@ -161,10 +161,17 @@ def bind_assert_forwarders(session_browser, forwarders): assert functional.get_forwarders(session_browser) == forwarders +@when( + parsers.parse('I create a user named {name:w} with password {password:S}')) +def create_user(session_browser, name, password): + if not functional.user_exists(session_browser, name): + functional.create_user(session_browser, name, password) + + @when( parsers.parse('I create a user named {name:w} with password {password:S} ' 'in group {group:w}')) -def create_user(session_browser, name, password, group): +def create_user_in_group(session_browser, name, password, group): if not functional.user_exists(session_browser, name): functional.create_user(session_browser, name, password, groups=[group]) diff --git a/plinth/tests/test_middleware.py b/plinth/tests/test_middleware.py index 8567c70d1..7a5e8b855 100644 --- a/plinth/tests/test_middleware.py +++ b/plinth/tests/test_middleware.py @@ -27,6 +27,7 @@ def fixture_kwargs(): class TestSetupMiddleware: """Test cases for setup middleware.""" + @staticmethod @pytest.fixture(name='middleware') def fixture_middleware(load_cfg): @@ -119,6 +120,7 @@ class TestSetupMiddleware: class TestAdminMiddleware: """Test cases for admin middleware.""" + @staticmethod @pytest.fixture(name='middleware') def fixture_middleware(load_cfg): @@ -142,6 +144,30 @@ class TestAdminMiddleware: with pytest.raises(PermissionDenied): middleware.process_view(web_request, **kwargs) + @staticmethod + def test_group_view_is_denied_for_non_group_user(web_request, middleware, + kwargs): + """Test that group view is allowed for an admin user.""" + web_request.user.groups.filter().exists = Mock(return_value=False) + web_request.session = MagicMock() + with patch( + 'plinth.middleware.AdminRequiredMiddleware.check_user_group', + lambda x, y: False): + with pytest.raises(PermissionDenied): + middleware.process_view(web_request, **kwargs) + + @staticmethod + def test_group_view_is_allowed_for_group_user(web_request, middleware, + kwargs): + """Test that group view is allowed for an admin user.""" + web_request.user.groups.filter().exists = Mock(return_value=False) + web_request.session = MagicMock() + with patch( + 'plinth.middleware.AdminRequiredMiddleware.check_user_group', + lambda x, y: True): + response = middleware.process_view(web_request, **kwargs) + assert response is None + @staticmethod def test_that_admin_view_is_allowed_for_admin_user(web_request, middleware, kwargs): @@ -152,8 +178,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) diff --git a/plinth/utils.py b/plinth/utils.py index 2d01a5777..9ee082fc9 100644 --- a/plinth/utils.py +++ b/plinth/utils.py @@ -52,6 +52,12 @@ def non_admin_view(func): return func +def user_group_view(func, group_name): + """Decorator to mark a view as accessible by admin or group users.""" + setattr(func, 'GROUP_NAME', group_name) + return func + + def is_user_admin(request, cached=False): """Return whether user is an administrator.""" if not request.user.is_authenticated: