openvpn: Deny access to users not in group "vpn"

A new function called `user_group_view` has been added to
restrict access to only users in the groups "vpn" and "admin".

Some changes are made in AdminRequiredMiddleware to handle this.

Signed-off-by: Joseph Nuthalapati <njoseph@riseup.net>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

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."""

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'],

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

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)

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'),
 ]

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)

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

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)

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: