Fix permission issues

home/services/README.md

@@ -0,0 +1,91 @@
+# Permission Management
+
+This folder implements a permission management system that provides role-based access control (RBAC) for organisations and projects. The system implements a flexible permissions model with support for admin and project manager roles, along with granular view/edit permissions at both organisation and project levels.
+
+## Implementation
+
+The current implementation provides:
+
+- Abstract base class `BasePermissionService` with core permission methods
+- Permission decorators `requires_permission` for method-level access control for services
+
+Service classes can inherit from `BasePermissionService` to implement custom permission checks. The `requires_permission` decorator can be used to enforce access control at the method level.
+
+Why don't we use Django's built-in permissions system? The Django permissions system is designed for managing (global) access to models and views within a Django application. It is not designed to handle complex permission requirements such as role-based access control (RBAC) across multiple resources. The custom permission management system provides more flexibility and control over access control requirements.
+
+## Permission Model
+
+### Roles
+
+For simplicity, the system supports two roles:
+
+- Admin: Full access to organisation and its projects
+- Project Manager: Limited access based on granted permissions. A PM can be granted view and edit permissions for multiple projects within an organisation.
+
+### Permission Levels
+
+View: Read-only access
+Edit: Ability to modify resources
+Delete: Ability to remove resources
+Create: Ability to create new resources
+
+
+## Usage
+
+```python
+@requires_permission("view", obj_param="organisation")
+def get_organisation(self, user: User, organisation: Organisation) -> Organisation:
+    return organisation
+```
+
+The `requires_permission` decorator can be used to enforce access control at the method level. The decorator takes the permission level and the object parameter name as arguments. The permission level is used to check if the user has the required permission to access the object. The `obj_param` argument is used to specify the name of the object parameter in the method signature. If `organisation: Organisation` were renamed to `org: Organisation`, the decorator would be `@requires_permission("view", obj_param="org")`.
+
+
+## Future Improvements
+
+The current permission system utilises a decorator-based approach with service-level checks, centred around `@requires_permission` and role verification methods. Its strength lies in simplicity and maintainability - the decorator pattern makes permissions explicit, while centralised service logic ensures consistent enforcement across the application. This design fits well with the service-oriented architecture and makes permission checks reusable across different views.
+
+However, the system has notable limitations. Performance can be a concern due to multiple database queries per check with no built-in caching. The hardcoded roles (_Admin_ and _Project Manager_) make it inflexible for custom permission schemes. Some code duplication exists across services, and testing requires some mocking setups.
+
+The current approach works well for basic needs, its simplicity comes at the cost of flexibility and scalability. Future improvements could focus on implementing a policy-based system with better performance characteristics while maintaining the current system's clarity and ease of use, if required.
+
+For example:
+
+```python
+class Permission(Enum):
+    VIEW = "view"
+    EDIT = "edit"
+    DELETE = "delete"
+    CREATE = "create"
+
+@dataclass
+class OrganisationPolicy:
+    user: User
+    organisation: Organisation
+
+    def get_role(self) -> Optional[str]:
+        membership = self.organisation.organisationmembership_set.filter(
+            user=self.user
+        ).first()
+        return membership.role if membership else None
+
+    def can(self, permission: Permission) -> bool:
+        role = self.get_role()
+        if not role:
+            return False
+
+        permission_matrix = {
+            Permission.VIEW: [ROLE_ADMIN, ROLE_PROJECT_MANAGER],
+            Permission.EDIT: [ROLE_ADMIN],
+            Permission.DELETE: [ROLE_ADMIN],
+            Permission.CREATE: [ROLE_ADMIN],
+        }
+        return role in permission_matrix[permission]
+
+class OrganisationService:
+    def get_policy(self, user: User, org: Organisation) -> OrganisationPolicy:
+        return OrganisationPolicy(user, org)
+
+    def can_view(self, user: User, org: Organisation) -> bool:
+        return self.get_policy(user, org).can(Permission.VIEW)
+```

home/services/base.py

@@ -9,19 +9,30 @@
 T = TypeVar("T")
 
 
-def requires_permission(permission_type: str, obj_param: str = "instance"):
+def requires_permission(permission_type: str, obj_param: str) -> Callable:
     """
     Permission decorator for service methods.
 
     Args:
-        permission_type: Type of permission to check (view/edit/delete)
-        obj_param: Name of the parameter that contains the object to check permissions against
+        permission_type: Type of permission to check (view/create/edit/delete)
+        obj_param: Name of the parameter that contains the object to check permissions against.
+        Example: `requires_permission("view", "proj")` will check if the user has view permission for the `Project` object
+        passed as the "proj" parameter:
+
+        ```python
+        @requires_permission("view", "proj")
+        def get_project(self, user: User, proj: Project) -> Project:
+            return project
+        ```
     """
 
     def decorator(func: Callable) -> Callable:
         @wraps(func)
         def wrapper(service: Any, user: Any, *args, **kwargs) -> Any:
             # Get the object to check permissions against
+            if not obj_param:
+                raise ValueError("Object parameter name is required")
+
             obj = kwargs.get(obj_param) or (args[0] if args else None)
             if not obj:
                 raise ValueError(f"Could not find object parameter: {obj_param}")

home/services/organisation.py

@@ -3,9 +3,8 @@
 """
 
 from typing import Optional, Dict, List, Set, Literal
-from django.db import transaction
-from django.db.models.query import QuerySet
-from django.db.models import Count
+from django.db.models.query import QuerySet, Prefetch
+from django.db.models import Count, Exists, OuterRef
 from django.core.exceptions import PermissionDenied
 from .base import BasePermissionService, requires_permission
 from ..models import (
@@ -45,13 +44,16 @@ def can_manage_members(self, user: User, organisation: Organisation) -> bool:
         role = self.get_user_role(user, organisation)
         return role == ROLE_ADMIN
 
-    @requires_permission("view")
+    @requires_permission("view", obj_param="organisation")
     def get_organisation(self, user: User, organisation: Organisation) -> Organisation:
         """Get organisation if user has permission"""
         return organisation
 
     def get_user_organisation(self, user: User) -> Optional[Organisation]:
         """Get user's primary organisation"""
+        if not user or not user.is_authenticated:
+            return None
+
         return user.organisation_set.first()
 
     def get_user_organisation_ids(self, user: User) -> Set[int]:
@@ -67,18 +69,19 @@ def get_user_accessible_organisations(
     ) -> Dict[int, List[Organisation]]:
         """Get organisations user has access to for each project"""
         user_org_ids = self.get_user_organisation_ids(user)
+
         return {
             project.id: [
                 org for org in project.organisations.all() if org.id in user_org_ids
             ]
             for project in projects
         }
 
-    @requires_permission("edit")
+    @requires_permission("edit", obj_param="organisation")
     def update_organisation(
         self, user: User, organisation: Organisation, data: Dict
     ) -> Organisation:
-        """Update organization with provided data"""
+        """Update organisation with provided data"""
         for key, value in data.items():
             setattr(organisation, key, value)
         organisation.save()
@@ -87,7 +90,7 @@ def update_organisation(
     def create_organisation(
         self, user: User, name: str, description: str = ""
     ) -> Organisation:
-        """Create a new organization"""
+        """Create a new organisation"""
         if not self.can_create(user):
             raise PermissionDenied("User cannot create organisations")
 
@@ -115,7 +118,7 @@ def add_user_to_organisation(
             user=user, organisation=organisation, role=role, added_by=added_by
         )
 
-    @requires_permission("edit")
+    @requires_permission("edit", obj_param="organisation")
     def remove_user_from_organisation(
         self, user: User, organisation: Organisation, removed_user: User
     ) -> None:
@@ -131,23 +134,66 @@ def remove_user_from_organisation(
         ).delete()
 
     def get_organisation_projects(
-        self, organisation: Organisation, with_metrics: bool = True
+        self, organisation: Organisation, user: User = None, with_metrics: bool = True
     ) -> QuerySet[Project]:
         """Get projects for an organisation with optional metrics"""
 
-        projects = Project.objects.filter(
+        base_query = Project.objects.filter(
             projectorganisation__organisation=organisation
         )
 
+        if user:
+            if user.is_superuser: # Superusers can view all projects
+                projects = base_query
+            else:
+                # Get user role
+                membership = OrganisationMembership.objects.filter(
+                    user=user,
+                    organisation=organisation
+                ).values('role').first()
+
+                user_role = membership['role'] if membership else None
+
+                if user_role == ROLE_ADMIN:
+                    projects = base_query
+                elif user_role == ROLE_PROJECT_MANAGER:
+                    # Use EXISTS subquery instead of separate query and filter
+                    projects = base_query.filter(
+                        Exists(
+                            ProjectManagerPermission.objects.filter(
+                                user=user,
+                                project=OuterRef('pk')
+                            )
+                        )
+                    )
+                else: # User has no role
+                    return Project.objects.none()
+        else:
+            projects = base_query
+
+        # Add metrics
         if with_metrics:
             projects = projects.annotate(
-                survey_count=Count("survey__id", distinct=True),
-                manager_count=Count("projectmanagerpermission", distinct=True),
+                survey_count=Count(
+                    'survey__id',
+                    distinct=True
+                ),
+                manager_count=Count(
+                    'projectmanagerpermission',
+                    distinct=True
+                )
+            ).select_related(
+                'created_by'  # Add any other needed related fields
+            ).prefetch_related(
+                Prefetch(
+                    'projectmanagerpermission_set',
+                    queryset=ProjectManagerPermission.objects.select_related('user')
+                )
             )
-
-        return projects
 
-    @requires_permission("view")
+        return projects.order_by('-created_on')
+
+    @requires_permission("view", obj_param="organisation")
     def get_organisation_members(
         self, user: User, organisation: Organisation
     ) -> QuerySet[OrganisationMembership]:

home/services/project.py

@@ -20,7 +20,10 @@ class ProjectService(BasePermissionService):
     """Service for managing projects with integrated permissions"""
 
     def get_user_role(self, user: User, project: Project) -> Optional[str]:
-        """Get user's highest role across project's organisations"""
+        """
+        Get user's highest role across project's organisations
+        TODO: this assuming a project can be linked to multiple organisations, check if still the case
+        """
         project_orgs = project.organisations.all()
 
         # Check for admin role first
@@ -53,7 +56,7 @@ def can_view(self, user: User, project: Project) -> bool:
         if role == ROLE_ADMIN:
             return True
         elif role == ROLE_PROJECT_MANAGER:
-            return self.get_user_permission(project, user) is not None
+            return self.get_user_permission(user, project) is not None
 
         return False
 
@@ -63,7 +66,7 @@ def can_edit(self, user: User, project: Project) -> bool:
         if role == ROLE_ADMIN:
             return True
         elif role == ROLE_PROJECT_MANAGER:
-            permission = self.get_user_permission(project, user)
+            permission = self.get_user_permission(user, project)
             return permission and permission.permission == "EDIT"
 
         return False
@@ -81,22 +84,25 @@ def update_project(self, user: User, project: Project, data: Dict) -> Project:
         """Update project with provided data"""
         for key, value in data.items():
             setattr(project, key, value)
-            
+
         project.save()
         return project
 
-    @requires_permission("view")
+    @requires_permission("view", obj_param="project")
     def get_project(self, user: User, project: Project) -> Project:
         """Get project if user has permission"""
         return project
 
     def create_project(
-        self, user: User, name: str, organisation: Organisation, description: str="") -> Project:
+        self, user: User, name: str, organisation: Organisation, description: str = ""
+    ) -> Project:
         """Create a new project"""
         if not self.can_create(user):
             raise PermissionDenied("User cannot create projects")
 
-        project = Project.objects.create(name=name, description=description, created_by=user)
+        project = Project.objects.create(
+            name=name, description=description, created_by=user
+        )
         self.link_project_to_organisation(
             user=user, project=project, organisation=organisation, permission="EDIT"
         )
@@ -110,8 +116,7 @@ def delete_project(self, user: User, project: Project):
         project.delete()
         return parent_org
 
-
-    @requires_permission("edit", "project")
+    @requires_permission("edit", obj_param="project")
     def grant_permission(
         self,
         user: User,
@@ -136,7 +141,7 @@ def grant_permission(
             defaults={"granted_by": user, "permission": permission},
         )
 
-    @requires_permission("edit", "project")
+    @requires_permission("edit", obj_param="project")
     def revoke_permission(
         self, user: User, project: Project, project_manager: User
     ) -> None:

home/views.py

@@ -111,10 +111,16 @@ class MyOrganisationView(LoginRequiredMixin, OrganisationRequiredMixin, ListView
     def setup(self, request, *args, **kwargs):
         super().setup(request, *args, **kwargs)
         self.organisation = organisation_service.get_user_organisation(request.user)
+
+        if not self.organisation:
+            messages.error(request, "You are not a member of any organisation.")
+            return redirect("organisation_create")
+
 
     def get_queryset(self):
         queryset = organisation_service.get_organisation_projects(
-            self.organisation
+            organisation=self.organisation,
+            user=self.request.user,
         )
 
         search_query = self.request.GET.get('q')

survey/services/survey.py

@@ -9,7 +9,6 @@
 
 from home.services import BasePermissionService
 from home.models import User, Project
-from home.services.base import requires_permission
 from survey.models import Invitation, Survey, SurveyResponse
 
 import logging