Merge pull request #9523 from rtibbles/permissions_fixes

API and CSV fixes

kolibri/core/api.py

@@ -6,7 +6,6 @@
 from rest_framework import viewsets
 from rest_framework.decorators import action
 from rest_framework.filters import OrderingFilter
-from rest_framework.generics import get_object_or_404
 from rest_framework.mixins import CreateModelMixin as BaseCreateModelMixin
 from rest_framework.mixins import DestroyModelMixin
 from rest_framework.mixins import UpdateModelMixin as BaseUpdateModelMixin
@@ -214,27 +213,6 @@ def _get_lookup_filter(self):
 
         return {self.lookup_field: self.kwargs[lookup_url_kwarg]}
 
-    def _get_object_from_queryset(self, queryset):
-        """
-        Returns the object the view is displaying.
-        We override this to remove the DRF default behaviour
-        of filtering the queryset.
-        (rtibbles) There doesn't seem to be a use case for
-        querying a detail endpoint and also filtering by query
-        parameters that might result in a 404.
-        """
-        # Perform the lookup filtering.
-        filter_kwargs = self._get_lookup_filter()
-        obj = get_object_or_404(queryset, **filter_kwargs)
-
-        # May raise a permission denied
-        self.check_object_permissions(self.request, obj)
-
-        return obj
-
-    def get_object(self):
-        return self._get_object_from_queryset(self.get_queryset())
-
     def annotate_queryset(self, queryset):
         return queryset
 
@@ -259,10 +237,10 @@ def serialize(self, queryset):
         )
 
     def serialize_object(self, **filter_kwargs):
-        queryset = self.get_queryset()
         try:
             filter_kwargs = filter_kwargs or self._get_lookup_filter()
-            return self.serialize(queryset.filter(**filter_kwargs))[0]
+            queryset = self.get_queryset().filter(**filter_kwargs)
+            return self.serialize(self.filter_queryset(queryset))[0]
         except IndexError:
             raise Http404(
                 "No %s matches the given query." % queryset.model._meta.object_name

kolibri/core/auth/api.py

@@ -99,10 +99,8 @@ class KolibriAuthPermissionsFilter(filters.BaseFilterBackend):
     """
 
     def filter_queryset(self, request, queryset, view):
-        if request.method == "GET" and request.resolver_match.url_name.endswith(
-            "-list"
-        ):
-            # only filter down the queryset in the case of the list view being requested
+        if request.method == "GET":
+            # If a 'GET' method only return readable items to filter down the queryset.
             return request.user.filter_readable(queryset)
         # otherwise, return the full queryset, as permission checks will happen object-by-object
         # (and filtering here then leads to 404's instead of the more correct 403's)

kolibri/core/auth/csv_utils.py

@@ -17,6 +17,7 @@
 from kolibri.core.auth.models import FacilityUser
 from kolibri.core.query import SQCount
 from kolibri.core.utils.csv import open_csv_for_writing
+from kolibri.core.utils.csv import output_mapper
 
 
 logger = logging.getLogger(__name__)
@@ -96,14 +97,7 @@ def replace_multiple_classrooms(field, obj):
 )
 
 
-def map_output(obj):
-    mapped_obj = {}
-    for header, label in labels.items():
-        if header in output_mappings and header in obj:
-            mapped_obj[label] = output_mappings[header](obj)
-        elif header in obj:
-            mapped_obj[label] = obj[header]
-    return mapped_obj
+map_output = partial(output_mapper, labels=labels, output_mappings=output_mappings)
 
 
 input_fields = (

kolibri/core/auth/management/commands/bulkexportusers.py

@@ -27,6 +27,7 @@
 from kolibri.core.tasks.management.commands.base import AsyncCommand
 from kolibri.core.tasks.utils import get_current_job
 from kolibri.core.utils.csv import open_csv_for_writing
+from kolibri.core.utils.csv import output_mapper
 from kolibri.utils import conf
 
 try:
@@ -107,14 +108,7 @@ def kind_of_roles(field, obj):
 }
 
 
-def map_output(obj):
-    mapped_obj = {}
-    for header, label in labels.items():
-        if header in output_mappings and header in obj:
-            mapped_obj[label] = output_mappings[header](obj)
-        elif header in obj:
-            mapped_obj[label] = obj[header]
-    return mapped_obj
+map_output = partial(output_mapper, labels=labels, output_mappings=output_mappings)
 
 
 def translate_labels():

kolibri/core/auth/models.py

@@ -55,16 +55,14 @@
 from .errors import UserIsNotFacilityUser
 from .errors import UserIsNotMemberError
 from .permissions.auth import AllCanReadFacilityDataset
-from .permissions.auth import AnonUserCanReadFacilities
+from .permissions.auth import AnyUserCanReadFacilities
 from .permissions.auth import CoachesCanManageGroupsForTheirClasses
 from .permissions.auth import CoachesCanManageMembershipsForTheirGroups
 from .permissions.auth import CollectionSpecificRoleBasedPermissions
 from .permissions.auth import FacilityAdminCanEditForOwnFacilityDataset
-from .permissions.auth import MembersCanReadMembershipsOfTheirCollections
 from .permissions.base import BasePermissions
 from .permissions.base import RoleBasedPermissions
 from .permissions.general import IsAdminForOwnFacility
-from .permissions.general import IsFromSameFacility
 from .permissions.general import IsOwn
 from .permissions.general import IsSelf
 from kolibri.core.auth.constants.demographics import choices as GENDER_CHOICES
@@ -932,9 +930,8 @@ class Collection(AbstractFacilityDataModel):
     # Furthermore, no FacilityUser can create or delete a Facility. Permission to create a collection is governed
     # by roles in relation to the new collection's parent collection (see CollectionSpecificRoleBasedPermissions).
     permissions = (
-        IsFromSameFacility(read_only=True)
-        | CollectionSpecificRoleBasedPermissions()
-        | AnonUserCanReadFacilities()
+        CollectionSpecificRoleBasedPermissions()
+        | AnyUserCanReadFacilities()
         | CoachesCanManageGroupsForTheirClasses()
     )
 
@@ -1130,9 +1127,7 @@ class Membership(AbstractFacilityDataModel):
     )
     # Membership can be written by coaches under the coaches' group
     membership = CoachesCanManageMembershipsForTheirGroups()
-    # Members can read memberships of collections they are members of
-    own_collections = MembersCanReadMembershipsOfTheirCollections()
-    permissions = own | role | membership | own_collections
+    permissions = own | role | membership
 
     user = models.ForeignKey(
         "FacilityUser", related_name="memberships", blank=False, null=False

kolibri/core/auth/permissions/auth.py

@@ -1,7 +1,6 @@
 """
 The permissions classes in this module define the specific permissions that govern access to the models in the auth app.
 """
-from django.contrib.auth.models import AnonymousUser
 from django.db.models import Q
 
 from ..constants.collection_kinds import ADHOCLEARNERSGROUP
@@ -28,6 +27,7 @@ def __init__(self):
             can_be_read_by=(ADMIN, COACH),
             can_be_updated_by=(ADMIN,),
             can_be_deleted_by=None,
+            collection_field=".",
         )
 
     def user_can_create_object(self, user, obj):
@@ -55,28 +55,19 @@ def user_can_delete_object(self, user, obj):
             CollectionSpecificRoleBasedPermissions, self
         ).user_can_update_object(user, obj.parent)
 
-    def readable_by_user_filter(self, user):
-        if user.is_anonymous():
-            return q_none
-
-        # By default can read all collections in facility
-        return Q(dataset_id=user.dataset_id)
-
 
-class AnonUserCanReadFacilities(DenyAll):
+class AnyUserCanReadFacilities(DenyAll):
     """
-    Permissions class that allows reading the object if user is anonymous.
+    Permissions class that allows reading the object for any user.
     """
 
     def user_can_read_object(self, user, obj):
         if obj.kind == FACILITY:
-            return isinstance(user, AnonymousUser)
+            return True
         return False
 
     def readable_by_user_filter(self, user):
-        if isinstance(user, AnonymousUser):
-            return Q(kind=FACILITY)
-        return q_none
+        return Q(kind=FACILITY)
 
 
 class FacilityAdminCanEditForOwnFacilityDataset(BasePermissions):
@@ -196,24 +187,3 @@ def user_can_delete_object(self, user, obj):
 
     def readable_by_user_filter(self, user):
         return q_none
-
-
-class MembersCanReadMembershipsOfTheirCollections(BasePermissions):
-    def user_can_create_object(self, user, obj):
-        return False
-
-    def user_can_read_object(self, user, obj):
-        return user.is_member_of(obj.collection)
-
-    def user_can_update_object(self, user, obj):
-        return False
-
-    def user_can_delete_object(self, user, obj):
-        return False
-
-    def readable_by_user_filter(self, user):
-        if user.is_anonymous():
-            return q_none
-        # Add a special case where users with memberships in the same collection
-        # can also read memberships for other members
-        return Q(collection_id__in=user.memberships.all().values("collection_id"))

kolibri/core/auth/permissions/base.py

@@ -100,6 +100,7 @@ def __init__(
         :param tuple can_be_updated_by: a tuple of role kinds that should give a user permission to update the object
         :param tuple can_be_deleted_by: a tuple of role kinds that should give a user permission to delete the object
         :param str collection_field: the name of the field through which collections can be identified for the object.
+        (or "." if the object itself is the collection)
         :param is_syncable: Boolean indicating whether the model is a syncable model, if it is, we can use dataset_id
         to do quick filtering in some cases, if not, then we need to use the target_field as the source of the dataset_id.
         """
@@ -108,7 +109,17 @@ def __init__(
         self.can_be_updated_by = can_be_updated_by
         self.can_be_deleted_by = can_be_deleted_by
         self.target_field = target_field
-        self.collection_field = collection_field
+        if collection_field == ".":
+            # Process special keyword '.' to allow this class to be used with Collection objects, particularly the
+            # readably by user filter, where the collection is the object itself, so we point at the id,
+            # and for parent collections to its parent.
+            self.collection_field = "id"
+            self.parent_collection_field = "parent"
+        else:
+            # Otherwise, we just set the collection field to the value passed in (which defaults to "collection")
+            # and set the parent field as "<collection_field>__parent" to point to the parent of the FKed collection.
+            self.collection_field = collection_field
+            self.parent_collection_field = "{}__parent".format(self.collection_field)
         self.is_syncable = is_syncable
 
     def _get_target_object(self, obj):
@@ -207,7 +218,7 @@ def readable_by_user_filter(self, user):
             # which collection an object is associated with.
             q_filter = Q(
                 Q(**{"{}__in".format(self.collection_field): collection_ids})
-                | Q(**{"{}__parent__in".format(self.collection_field): collection_ids})
+                | Q(**{"{}__in".format(self.parent_collection_field): collection_ids})
             )
             # Also filter by the parents of collections, so that objects associated with LearnerGroup
             # or AdHocGroups will also be readable by those with coach permissions on the parent Classroom

kolibri/core/auth/permissions/general.py

@@ -114,35 +114,6 @@ def readable_by_user_filter(self, user):
         return Q(**{self.field_name: user.id})
 
 
-class IsFromSameFacility(BasePermissions):
-    """
-    Permissions class that only allows access to object if user is associated with the same facility as the object.
-    """
-
-    def __init__(self, field_name=".", read_only=False):
-        self.read_only = read_only
-
-    def _facility_dataset_is_same(self, user, obj):
-        return hasattr(user, "dataset") and user.dataset == obj.dataset
-
-    def user_can_create_object(self, user, obj):
-        return (not self.read_only) and self._facility_dataset_is_same(user, obj)
-
-    def user_can_read_object(self, user, obj):
-        return self._facility_dataset_is_same(user, obj)
-
-    def user_can_update_object(self, user, obj):
-        return (not self.read_only) and self._facility_dataset_is_same(user, obj)
-
-    def user_can_delete_object(self, user, obj):
-        return (not self.read_only) and self._facility_dataset_is_same(user, obj)
-
-    def readable_by_user_filter(self, user):
-        if hasattr(user, "dataset"):
-            return Q(dataset=user.dataset)
-        return q_none
-
-
 def _user_is_admin_for_own_facility(user, obj=None):
 
     # import here to avoid circular imports