Merge pull request #1305 from ae-utbm/user-all-groups

User all groups

core/auth/mixins.py

@@ -307,6 +307,7 @@ class PermissionOrClubBoardRequiredMixin(PermissionRequiredMixin):
             return False
         if super().has_permission():
             return True
-        return self.club is not None and any(
+        return (
-            g.id == self.club.board_group_id for g in self.request.user.cached_groups
+            self.club is not None
+            and self.club.board_group_id in self.request.user.all_groups
         )

core/models.py

@@ -356,23 +356,27 @@ class User(AbstractUser):
         )
         if group_id is None:
             return False
-        if group_id == settings.SITH_GROUP_SUBSCRIBERS_ID:
+        return group_id in self.all_groups
-            return self.is_subscribed
-        if group_id == settings.SITH_GROUP_ROOT_ID:
-            return self.is_root
-        return any(g.id == group_id for g in self.cached_groups)
 
     @cached_property
-    def cached_groups(self) -> list[Group]:
+    def all_groups(self) -> dict[int, Group]:
         """Get the list of groups this user is in."""
-        return list(self.groups.all())
+        additional_groups = []
+        if self.is_subscribed:
+            additional_groups.append(settings.SITH_GROUP_SUBSCRIBERS_ID)
+        if self.is_superuser:
+            additional_groups.append(settings.SITH_GROUP_ROOT_ID)
+        qs = self.groups.all()
+        if additional_groups:
+            # This is somewhat counter-intuitive, but this query runs way faster with
+            # a UNION rather than a OR (in average, 0.25ms vs 14ms).
+            # For the why, cf. https://dba.stackexchange.com/questions/293836/why-is-an-or-statement-slower-than-union
+            qs = qs.union(Group.objects.filter(id__in=additional_groups))
+        return {g.id: g for g in qs}
 
     @cached_property
     def is_root(self) -> bool:
-        if self.is_superuser:
+        return self.is_superuser or settings.SITH_GROUP_ROOT_ID in self.all_groups
-            return True
-        root_id = settings.SITH_GROUP_ROOT_ID
-        return any(g.id == root_id for g in self.cached_groups)
 
     @cached_property
     def is_board_member(self) -> bool:
@@ -1099,10 +1103,7 @@ class PageQuerySet(models.QuerySet):
             return self.filter(view_groups=settings.SITH_GROUP_PUBLIC_ID)
         if user.has_perm("core.view_page"):
             return self.all()
-        groups_ids = [g.id for g in user.cached_groups]
+        return self.filter(view_groups__in=user.all_groups)
-        if user.is_subscribed:
-            groups_ids.append(settings.SITH_GROUP_SUBSCRIBERS_ID)
-        return self.filter(view_groups__in=groups_ids)
 
 
 # This function prevents generating migration upon settings change
@@ -1376,7 +1377,7 @@ class PageRev(models.Model):
         return self.page.can_be_edited_by(user)
 
     def is_owned_by(self, user: User) -> bool:
-        return any(g.id == self.page.owner_group_id for g in user.cached_groups)
+        return self.page.owner_group_id in user.all_groups
 
     def similarity_ratio(self, text: str) -> float:
         """Similarity ratio between this revision's content and the given text.

core/tests/test_core.py

@@ -418,16 +418,16 @@ class TestUserIsInGroup(TestCase):
         group_in = baker.make(Group)
         self.public_user.groups.add(group_in)
 
-        # clear the cached property `User.cached_groups`
+        # clear the cached property `User.all_groups`
-        self.public_user.__dict__.pop("cached_groups", None)
+        self.public_user.__dict__.pop("all_groups", None)
         # Test when the user is in the group
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(2):
             self.public_user.is_in_group(pk=group_in.id)
         with self.assertNumQueries(0):
             self.public_user.is_in_group(pk=group_in.id)
 
         group_not_in = baker.make(Group)
-        self.public_user.__dict__.pop("cached_groups", None)
+        self.public_user.__dict__.pop("all_groups", None)
         # Test when the user is not in the group
         with self.assertNumQueries(1):
             self.public_user.is_in_group(pk=group_not_in.id)

election/views.py

@@ -1,7 +1,6 @@
 from typing import TYPE_CHECKING
 
 from cryptography.utils import cached_property
-from django.conf import settings
 from django.contrib import messages
 from django.contrib.auth.mixins import (
     LoginRequiredMixin,
@@ -115,16 +114,9 @@ class VoteFormView(LoginRequiredMixin, UserPassesTestMixin, FormView):
     def test_func(self):
         if not self.election.can_vote(self.request.user):
             return False
-
+        return self.election.vote_groups.filter(
-        groups = set(self.election.vote_groups.values_list("id", flat=True))
+            id__in=self.request.user.all_groups
-        if (
+        ).exists()
-            settings.SITH_GROUP_SUBSCRIBERS_ID in groups
-            and self.request.user.is_subscribed
-        ):
-            # the subscriber group isn't truly attached to users,
-            # so it must be dealt with separately
-            return True
-        return self.request.user.groups.filter(id__in=groups).exists()
 
     def vote(self, election_data):
         with transaction.atomic():
@@ -238,15 +230,9 @@ class RoleCreateView(LoginRequiredMixin, UserPassesTestMixin, CreateView):
             return False
         if self.request.user.has_perm("election.add_role"):
             return True
-        groups = set(self.election.edit_groups.values_list("id", flat=True))
+        return self.election.edit_groups.filter(
-        if (
+            id__in=self.request.user.all_groups
-            settings.SITH_GROUP_SUBSCRIBERS_ID in groups
+        ).exists()
-            and self.request.user.is_subscribed
-        ):
-            # the subscriber group isn't truly attached to users,
-            # so it must be dealt with separately
-            return True
-        return self.request.user.groups.filter(id__in=groups).exists()
 
     def get_initial(self):
         return {"election": self.election}
@@ -279,14 +265,7 @@ class ElectionListCreateView(LoginRequiredMixin, UserPassesTestMixin, CreateView
             .union(self.election.edit_groups.values("id"))
             .values_list("id", flat=True)
         )
-        if (
+        return not groups.isdisjoint(self.request.user.all_groups.keys())
-            settings.SITH_GROUP_SUBSCRIBERS_ID in groups
-            and self.request.user.is_subscribed
-        ):
-            # the subscriber group isn't truly attached to users,
-            # so it must be dealt with separately
-            return True
-        return self.request.user.groups.filter(id__in=groups).exists()
 
     def get_initial(self):
         return {"election": self.election}

sas/static/bundled/sas/viewer-index.ts

@@ -109,15 +109,14 @@ interface ViewerConfig {
   /** id of the first picture to load on the page */
   firstPictureId: number;
   /** if the user is sas admin */
-  userIsSasAdmin: boolean;
+  userCanModerate: boolean;
 }
 
 /**
  * Load user picture page with a nice download bar
  **/
-exportToHtml("loadViewer", (config: ViewerConfig) => {
 document.addEventListener("alpine:init", () => {
-    Alpine.data("picture_viewer", () => ({
+  Alpine.data("picture_viewer", (config: ViewerConfig) => ({
     /**
      * All the pictures that can be displayed on this picture viewer
      **/
@@ -208,8 +207,7 @@ exportToHtml("loadViewer", (config: ViewerConfig) => {
         }
         this.pushstate = History.Replace;
         this.currentPicture = this.pictures.find(
-            (i: PictureSchema) =>
+          (i: PictureSchema) => i.id === Number.parseInt(event.state.sasPictureId, 10),
-              i.id === Number.parseInt(event.state.sasPictureId, 10),
         );
       });
       this.pushstate = History.Replace; /* Avoid first url push */
@@ -231,11 +229,7 @@ exportToHtml("loadViewer", (config: ViewerConfig) => {
         url: this.currentPicture.sas_url,
       };
       if (this.pushstate === History.Replace) {
-          window.history.replaceState(
+        window.history.replaceState(updateArgs.data, updateArgs.unused, updateArgs.url);
-            updateArgs.data,
-            updateArgs.unused,
-            updateArgs.url,
-          );
         this.pushstate = History.Push;
       } else {
         window.history.pushState(updateArgs.data, updateArgs.unused, updateArgs.url);
@@ -251,7 +245,7 @@ exportToHtml("loadViewer", (config: ViewerConfig) => {
         this.nextPicture?.preload();
         this.previousPicture?.preload();
       });
-        if (this.currentPicture.asked_for_removal && config.userIsSasAdmin) {
+      if (this.currentPicture.asked_for_removal && config.userCanModerate) {
         await Promise.all([
           this.currentPicture.loadIdentifications(),
           this.currentPicture.loadModeration(),
@@ -317,7 +311,7 @@ exportToHtml("loadViewer", (config: ViewerConfig) => {
      * Check if an identification can be removed by the currently logged user
      */
     canBeRemoved(identification: IdentifiedUserSchema): boolean {
-        return config.userIsSasAdmin || identification.user.id === config.userId;
+      return config.userCanModerate || identification.user.id === config.userId;
     },
 
     /**
@@ -337,4 +331,3 @@ exportToHtml("loadViewer", (config: ViewerConfig) => {
     },
   }));
 });
-});

sas/templates/sas/picture.jinja

@@ -17,10 +17,8 @@
 
 {% from "sas/macros.jinja" import print_path %}
 
-{% set user_is_sas_admin = user.is_root or user.is_in_group(pk = settings.SITH_GROUP_SAS_ADMIN_ID) %}
-
 {% block content %}
-  <main x-data="picture_viewer">
+  <main x-data="picture_viewer(config)">
     <code>
       <a href="{{ url('sas:main') }}">SAS</a> / {{ print_path(album) }} <span x-text="currentPicture.name"></span>
     </code>
@@ -50,15 +48,13 @@
               It will be hidden to other users until it has been moderated.
             {% endtrans %}
           </p>
-          {% if user_is_sas_admin %}
+          {% if user.has_perm("sas.moderate_sasfile") %}
             <template x-if="currentPicture.asked_for_removal">
               <div>
                 <h5>{% trans %}The following issues have been raised:{% endtrans %}</h5>
                 <template x-for="req in (currentPicture.moderationRequests ?? [])" :key="req.id">
                   <div>
-                    <h6
+                    <h6 x-text="`${req.author.first_name} ${req.author.last_name}`"></h6>
-                      x-text="`${req.author.first_name} ${req.author.last_name}`"
-                    ></h6>
                     <i x-text="Intl.DateTimeFormat(
                                '{{ LANGUAGE_CODE }}',
                                {dateStyle: 'long', timeStyle: 'short'}
@@ -70,7 +66,7 @@
             </template>
           {% endif %}
         </div>
-        {% if user_is_sas_admin %}
+        {% if user.has_perm("sas.moderate_sasfile") %}
           <div class="alert-aside">
             <button class="btn btn-blue" @click="moderatePicture()">
               {% trans %}Moderate{% endtrans %}
@@ -204,16 +200,13 @@
 {% endblock %}
 
 {% block script %}
-  {{ super() }}
   <script>
-    window.addEventListener("DOMContentLoaded", () => {
+    const config = {
-      loadViewer({
       albumId: {{ album.id }},
       albumUrl: "{{ album.get_absolute_url() }}",
       firstPictureId: {{ picture.id }},  {# id of the first picture to show after page load #}
       userId: {{ user.id }},
-        userIsSasAdmin: {{ user_is_sas_admin|tojson }}
+      userCanModerate: {{ user.has_perm("sas.moderate_sasfile")|tojson }}
-      });
+    }
-    })
   </script>
 {% endblock %}

sas/tests/test_views.py

@@ -161,16 +161,22 @@ class TestSasModeration(TestCase):
         assert len(res.context_data["pictures"]) == 1
         assert res.context_data["pictures"][0] == self.to_moderate
 
-        res = self.client.post(
-            reverse("sas:moderation"),
-            data={"album_id": self.to_moderate.id, "picture_id": self.to_moderate.id},
-        )
-
     def test_moderation_page_forbidden(self):
         self.client.force_login(self.simple_user)
         res = self.client.get(reverse("sas:moderation"))
         assert res.status_code == 403
 
+    def test_moderate_album(self):
+        self.client.force_login(self.moderator)
+        url = reverse("sas:moderation")
+        album = baker.make(
+            Album, is_moderated=False, parent_id=settings.SITH_SAS_ROOT_DIR_ID
+        )
+        res = self.client.post(url, data={"album_id": album.id, "moderate": ""})
+        assertRedirects(res, url)
+        album.refresh_from_db()
+        assert album.is_moderated
+
     def test_moderate_picture(self):
         self.client.force_login(self.moderator)
         res = self.client.get(

sas/views.py

@@ -15,10 +15,10 @@
 from typing import Any
 
 from django.conf import settings
-from django.core.exceptions import PermissionDenied
+from django.contrib.auth.mixins import PermissionRequiredMixin
 from django.db.models import Count, OuterRef, Subquery
 from django.http import Http404, HttpResponseRedirect
-from django.shortcuts import get_object_or_404
+from django.shortcuts import get_object_or_404, redirect
 from django.urls import reverse
 from django.utils.safestring import SafeString
 from django.views.generic import CreateView, DetailView, TemplateView
@@ -191,18 +191,13 @@ class UserPicturesView(UserTabsMixin, CanViewMixin, DetailView):
 # Admin views
 
 
-class ModerationView(TemplateView):
+class ModerationView(PermissionRequiredMixin, TemplateView):
     template_name = "sas/moderation.jinja"
-
+    permission_required = "sas.moderate_sasfile"
-    def get(self, request, *args, **kwargs):
-        if request.user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
-            return super().get(request, *args, **kwargs)
-        raise PermissionDenied
 
     def post(self, request, *args, **kwargs):
         if "album_id" not in request.POST:
             raise Http404
-        if request.user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
         album = get_object_or_404(Album, pk=request.POST["album_id"])
         if "moderate" in request.POST:
             album.moderator = request.user
@@ -210,7 +205,7 @@ class ModerationView(TemplateView):
             album.save()
         elif "delete" in request.POST:
             album.delete()
-        return super().get(request, *args, **kwargs)
+        return redirect(self.request.path)
 
     def get_context_data(self, **kwargs):
         kwargs = super().get_context_data(**kwargs)