Merge pull request #1006 from ae-utbm/pedagogy-perms

Improve pedagogy permissions

club/views.py

@@ -256,7 +256,7 @@ class ClubMembersView(ClubTabsMixin, CanViewMixin, DetailFormView):
     def get_form_kwargs(self):
         kwargs = super().get_form_kwargs()
         kwargs["request_user"] = self.request.user
-        kwargs["club"] = self.get_object()
+        kwargs["club"] = self.object
         kwargs["club_members"] = self.members
         return kwargs
 
@@ -273,9 +273,9 @@ def form_valid(self, form):
         users = data.pop("users", [])
         users_old = data.pop("users_old", [])
         for user in users:
-            Membership(club=self.get_object(), user=user, **data).save()
+            Membership(club=self.object, user=user, **data).save()
         for user in users_old:
-            membership = self.get_object().get_membership_for(user)
+            membership = self.object.get_membership_for(user)
             membership.end_date = timezone.now()
             membership.save()
         return resp
@@ -285,9 +285,7 @@ def dispatch(self, request, *args, **kwargs):
         return super().dispatch(request, *args, **kwargs)
 
     def get_success_url(self, **kwargs):
-        return reverse_lazy(
-            "club:club_members", kwargs={"club_id": self.get_object().id}
-        )
+        return reverse_lazy("club:club_members", kwargs={"club_id": self.object.id})
 
 
 class ClubOldMembersView(ClubTabsMixin, CanViewMixin, DetailView):

core/auth/api_permissions.py

@@ -37,8 +37,11 @@ def bar_delete(self, bar_id: int):
     ```
 """
 
+import operator
+from functools import reduce
 from typing import Any
 
+from django.contrib.auth.models import Permission
 from django.http import HttpRequest
 from ninja_extra import ControllerBase
 from ninja_extra.permissions import BasePermission
@@ -56,6 +59,46 @@ def has_permission(self, request: HttpRequest, controller: ControllerBase) -> bo
         return request.user.is_in_group(pk=self._group_pk)
 
 
+class HasPerm(BasePermission):
+    """Check that the user has the required perm.
+
+    If multiple perms are given, a comparer function can also be passed,
+    in order to change the way perms are checked.
+
+    Example:
+        ```python
+        # this route will require both permissions
+        @route.put("/foo", permissions=[HasPerm(["foo.change_foo", "foo.add_foo"])]
+        def foo(self): ...
+
+        # This route will require at least one of the perm,
+        # but it's not mandatory to have all of them
+        @route.put(
+            "/bar",
+            permissions=[HasPerm(["foo.change_bar", "foo.add_bar"], op=operator.or_)],
+        )
+        def bar(self): ...
+    """
+
+    def __init__(
+        self, perms: str | Permission | list[str | Permission], op=operator.and_
+    ):
+        """
+        Args:
+            perms: a permission or a list of permissions the user must have
+            op: An operator to combine multiple permissions (in most cases,
+                it will be either `operator.and_` or `operator.or_`)
+        """
+        super().__init__()
+        if not isinstance(perms, (list, tuple, set)):
+            perms = [perms]
+        self._operator = op
+        self._perms = perms
+
+    def has_permission(self, request: HttpRequest, controller: ControllerBase) -> bool:
+        return reduce(self._operator, (request.user.has_perm(p) for p in self._perms))
+
+
 class IsRoot(BasePermission):
     """Check that the user is root."""
 

core/management/commands/populate.py

@@ -895,13 +895,16 @@ def _create_groups(self) -> PopulatedGroups:
 
         subscribers = Group.objects.create(name="Subscribers")
         subscribers.permissions.add(
-            *list(perms.filter(codename__in=["add_news", "add_uvcommentreport"]))
+            *list(perms.filter(codename__in=["add_news", "add_uvcomment"]))
         )
         old_subscribers = Group.objects.create(name="Old subscribers")
         old_subscribers.permissions.add(
             *list(
                 perms.filter(
                     codename__in=[
+                        "view_uv",
+                        "view_uvcomment",
+                        "add_uvcommentreport",
                         "view_user",
                         "view_picture",
                         "view_album",
@@ -973,9 +976,9 @@ def _create_groups(self) -> PopulatedGroups:
         )
         pedagogy_admin.permissions.add(
             *list(
-                perms.filter(content_type__app_label="pedagogy").values_list(
-                    "pk", flat=True
-                )
+                perms.filter(content_type__app_label="pedagogy")
+                .exclude(codename__in=["change_uvcomment"])
+                .values_list("pk", flat=True)
             )
         )
         self.reset_index("core", "auth")

core/views/__init__.py

@@ -28,8 +28,7 @@
     HttpResponseServerError,
 )
 from django.shortcuts import render
-from django.utils.functional import cached_property
-from django.views.generic.detail import SingleObjectMixin
+from django.views.generic.detail import BaseDetailView
 from django.views.generic.edit import FormView
 from sentry_sdk import last_event_id
 
@@ -54,17 +53,12 @@ def internal_servor_error(request):
     return HttpResponseServerError(render(request, "core/500.jinja"))
 
 
-class DetailFormView(SingleObjectMixin, FormView):
+class DetailFormView(FormView, BaseDetailView):
     """Class that allow both a detail view and a form view."""
 
-    def get_object(self):
-        """Get current group from id in url."""
-        return self.cached_object
-
-    @cached_property
-    def cached_object(self):
-        """Optimisation on group retrieval."""
-        return super().get_object()
+    def post(self, request, *args, **kwargs):
+        self.object = self.get_object()
+        return super().post(request, *args, **kwargs)
 
 
 # F403: those star-imports would be hellish to refactor

core/views/user.py

@@ -66,7 +66,6 @@
 )
 from core.views.mixins import QuickNotifMixin, TabedViewMixin
 from counter.models import Refilling, Selling
-from counter.views.student_card import StudentCardFormView
 from eboutic.models import Invoice
 from subscription.models import Subscription
 from trombi.views import UserTrombiForm
@@ -566,6 +565,8 @@ def get_context_data(self, **kwargs):
         if not hasattr(self.object, "trombi_user"):
             kwargs["trombi_form"] = UserTrombiForm()
         if hasattr(self.object, "customer"):
+            from counter.views.student_card import StudentCardFormView
+
             kwargs["student_card_fragment"] = StudentCardFormView.get_template_data(
                 self.object.customer
             ).render(self.request)

pedagogy/api.py

@@ -1,13 +1,13 @@
+import operator
 from typing import Annotated
 
 from annotated_types import Ge
-from django.conf import settings
 from ninja import Query
 from ninja_extra import ControllerBase, api_controller, paginate, route
 from ninja_extra.exceptions import NotFound
 from ninja_extra.pagination import PageNumberPaginationExtra, PaginatedResponseSchema
 
-from core.auth.api_permissions import IsInGroup, IsRoot, IsSubscriber
+from core.auth.api_permissions import HasPerm
 from pedagogy.models import UV
 from pedagogy.schemas import SimpleUvSchema, UvFilterSchema, UvSchema
 from pedagogy.utbm_api import find_uv
@@ -17,7 +17,11 @@
 class UvController(ControllerBase):
     @route.get(
         "/{year}/{code}",
-        permissions=[IsRoot | IsInGroup(settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)],
+        permissions=[
+            # this route will almost always be called in the context
+            # of a UV creation/edition
+            HasPerm(["pedagogy.add_uv", "pedagogy.change_uv"], op=operator.or_)
+        ],
         url_name="fetch_uv_from_utbm",
         response=UvSchema,
     )
@@ -34,8 +38,8 @@ def fetch_from_utbm_api(
         "",
         response=PaginatedResponseSchema[SimpleUvSchema],
         url_name="fetch_uvs",
-        permissions=[IsSubscriber | IsInGroup(settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)],
+        permissions=[HasPerm("pedagogy.view_uv")],
     )
     @paginate(PageNumberPaginationExtra, page_size=100)
     def fetch_uv_list(self, search: Query[UvFilterSchema]):
-        return search.filter(UV.objects.all())
+        return search.filter(UV.objects.values())

pedagogy/models.py

@@ -20,10 +20,12 @@
 # Place - Suite 330, Boston, MA 02111-1307, USA.
 #
 #
+from typing import Self
 
 from django.conf import settings
 from django.core import validators
 from django.db import models
+from django.db.models import Exists, OuterRef
 from django.urls import reverse
 from django.utils import timezone
 from django.utils.functional import cached_property
@@ -145,14 +147,6 @@ def __str__(self):
     def get_absolute_url(self):
         return reverse("pedagogy:uv_detail", kwargs={"uv_id": self.id})
 
-    def is_owned_by(self, user):
-        """Can be created by superuser, root or pedagogy admin user."""
-        return user.is_in_group(pk=settings.SITH_GROUP_PEDAGOGY_ADMIN_ID)
-
-    def can_be_viewed_by(self, user):
-        """Only visible by subscribers."""
-        return user.is_subscribed
-
     def __grade_average_generic(self, field):
         comments = self.comments.filter(**{field + "__gte": 0})
         if not comments.exists():
@@ -191,6 +185,22 @@ def grade_work_load_average(self):
         return self.__grade_average_generic("grade_work_load")
 
 
+class UVCommentQuerySet(models.QuerySet):
+    def viewable_by(self, user: User) -> Self:
+        if user.has_perms(["pedagogy.view_uvcomment", "pedagogy.view_uvcommentreport"]):
+            # the user can view uv comment reports,
+            # so he can view non-moderated comments
+            return self
+        if user.has_perm("pedagogy.view_uvcomment"):
+            return self.filter(reports=None)
+        return self.filter(author=user)
+
+    def annotate_is_reported(self) -> Self:
+        return self.annotate(
+            is_reported=Exists(UVCommentReport.objects.filter(comment=OuterRef("pk")))
+        )
+
+
 class UVComment(models.Model):
     """A comment about an UV."""
 
@@ -243,6 +253,8 @@ class UVComment(models.Model):
     )
     publish_date = models.DateTimeField(_("publish date"), blank=True)
 
+    objects = UVCommentQuerySet.as_manager()
+
     def __str__(self):
         return f"{self.uv} - {self.author}"
 
@@ -251,15 +263,6 @@ def save(self, *args, **kwargs):
             self.publish_date = timezone.now()
         super().save(*args, **kwargs)
 
-    def is_owned_by(self, user):
-        """Is owned by a pedagogy admin, a superuser or the author himself."""
-        return self.author == user or user.is_owner(self.uv)
-
-    @cached_property
-    def is_reported(self):
-        """Return True if someone reported this UV."""
-        return self.reports.exists()
-
 
 # TODO : it seems that some views were meant to be implemented
 #        to use this model.
@@ -323,7 +326,3 @@ def __str__(self):
     @cached_property
     def uv(self):
         return self.comment.uv
-
-    def is_owned_by(self, user):
-        """Can be created by a pedagogy admin, a superuser or a subscriber."""
-        return user.is_subscribed or user.is_owner(self.comment.uv)

pedagogy/templates/pedagogy/guide.jinja

@@ -19,7 +19,7 @@
 {% endblock head %}
 
 {% block content %}
-  {% if can_create_uv %}
+  {% if user.has_perm("pedagogy.add_uv") %}
     <div class="action-bar">
       <p>
         <a href="{{ url('pedagogy:uv_create') }}">{% trans %}Create UV{% endtrans %}</a>
@@ -94,8 +94,10 @@
           <td>{% trans %}Credit type{% endtrans %}</td>
           <td><i class="fa fa-leaf"></i></td>
           <td><i class="fa-regular fa-sun"></i></td>
-          {% if can_create_uv %}
+          {%- if user.has_perm("pedagogy.change_uv") -%}
             <td>{% trans %}Edit{% endtrans %}</td>
+          {%- endif -%}
+          {%- if user.has_perm("pedagogy.delete_uv") -%}
             <td>{% trans %}Delete{% endtrans %}</td>
           {% endif %}
         </tr>
@@ -109,8 +111,10 @@
             <td x-text="uv.credit_type"></td>
             <td><i :class="uv.semester.includes('AUTUMN') && 'fa fa-leaf'"></i></td>
             <td><i :class="uv.semester.includes('SPRING') && 'fa-regular fa-sun'"></i></td>
-            {% if can_create_uv -%}
+            {%- if user.has_perm("pedagogy.change_uv") -%}
               <td><a :href="`/pedagogy/uv/${uv.id}/edit`">{% trans %}Edit{% endtrans %}</a></td>
+            {%- endif -%}
+            {%- if user.has_perm("pedagogy.delete_uv") -%}
               <td><a :href="`/pedagogy/uv/${uv.id}/delete`">{% trans %}Delete{% endtrans %}</a></td>
             {%- endif -%}
           </tr>

pedagogy/templates/pedagogy/uv_detail.jinja

@@ -89,7 +89,7 @@
         <div id="leave_comment_not_allowed">
           <p>{% trans %}You already posted a comment on this UV. If you want to comment again, please modify or delete your previous comment.{% endtrans %}</p>
         </div>
-      {% else %}
+      {% elif user.has_perm("pedagogy.add_uvcomment") %}
         <div id="leave_comment">
           <h2>{% trans %}Leave comment{% endtrans %}</h2>
           <div>
@@ -146,9 +146,9 @@
       {% endif %}
       <br>
 
-      {% if object.comments.exists() %}
+      {% if comments %}
         <h2>{% trans %}Comments{% endtrans %}</h2>
-        {% for comment in object.comments.order_by("-publish_date").all() %}
+        {% for comment in comments %}
           <div id="{{ comment.id }}" class="comment-container">
 
             <div class="grade-block">
@@ -183,16 +183,28 @@
                 </p>
               {% endif %}
 
-              {% if user.is_owner(comment) %}
+              {% if comment.author_id == user.id or user.has_perm("pedagogy.change_comment") %}
                 <p class="actions">
-                  <a href="{{ url('pedagogy:comment_update', comment_id=comment.id) }}">{% trans %}Edit{% endtrans %}</a>
-                  <a href="{{ url('pedagogy:comment_delete', comment_id=comment.id) }}">{% trans %}Delete{% endtrans %}</a>
+                  <a href="{{ url('pedagogy:comment_update', comment_id=comment.id) }}">
+                    {% trans %}Edit{% endtrans %}
+                  </a>
+              {% endif %}
+              {% if comment.author_id == user.id or user.has_perm("pedagogy.delete_comment") %}
+                <a href="{{ url('pedagogy:comment_delete', comment_id=comment.id) }}">
+                  {% trans %}Delete{% endtrans %}
+                </a>
                 </p>
               {% endif %}
             </div>
 
             <div class="comment-end-bar">
-              <div class="report"><p><a href="{{ url('pedagogy:comment_report', comment_id=comment.id) }}">{% trans %}Report this comment{% endtrans %}</a></p></div>
+              <div class="report">
+                <p>
+                  <a href="{{ url('pedagogy:comment_report', comment_id=comment.id) }}">
+                    {% trans %}Report this comment{% endtrans %}
+                  </a>
+                </p>
+              </div>
 
               <div class="date"><p>{{ comment.publish_date.strftime('%d/%m/%Y') }}</p></div>
 
@@ -209,7 +221,7 @@
   <script type="text/javascript">
     $("#return_noscript").hide();
     $("#return_js").show();
-    var icons = {
+    const icons = {
       header: "fa fa-toggle-right",
       activeHeader: "fa fa-toggle-down"
     };