From 215fdce4117f380e6927b0efa9d6fce21aec9696 Mon Sep 17 00:00:00 2001
From: thomas girod <thgirod@hotmail.com>
Date: Thu, 25 Jul 2024 17:27:23 +0200
Subject: [PATCH 1/3] Fix button to remove a user from picture

---
 core/static/sas/picture.scss          | 15 +++---
 core/static/user/user_godfathers.scss |  2 -
 sas/api.py                            | 33 ++++++++++--
 sas/schemas.py                        | 17 +++++--
 sas/templates/sas/picture.jinja       | 73 ++++++++++++++++++++++-----
 sas/views.py                          |  6 ---
 6 files changed, 107 insertions(+), 39 deletions(-)

diff --git a/core/static/sas/picture.scss b/core/static/sas/picture.scss
index d65bf826..a466d031 100644
--- a/core/static/sas/picture.scss
+++ b/core/static/sas/picture.scss
@@ -199,12 +199,6 @@
     > form {
       > p {
         box-sizing: border-box;
-
-        > input {
-          width: 100%;
-          max-width: 100%;
-          box-sizing: border-box;
-        }
       }
 
       > .results_on_deck > div {
@@ -219,12 +213,15 @@
           right: 0;
         }
       }
-
-      > input {
-        width: 100%;
+      input {
+        min-width: 100%;
         max-width: 100%;
         box-sizing: border-box;
       }
+
+      button {
+        font-weight: bold;
+      }
     }
   }
 }
diff --git a/core/static/user/user_godfathers.scss b/core/static/user/user_godfathers.scss
index 1075c8b8..9f922c07 100644
--- a/core/static/user/user_godfathers.scss
+++ b/core/static/user/user_godfathers.scss
@@ -32,7 +32,6 @@
     width: 100%;
   }
 
-  // Django moment
   > div.mini_profile_link {
     position: relative;
 
@@ -106,7 +105,6 @@
     }
   }
 
-  // Django moment
   > a.mini_profile_link {
     display: none;
   }
diff --git a/sas/api.py b/sas/api.py
index c1159df7..9e347edb 100644
--- a/sas/api.py
+++ b/sas/api.py
@@ -1,17 +1,23 @@
+from django.conf import settings
 from ninja import Query
 from ninja_extra import ControllerBase, api_controller, route
 from ninja_extra.exceptions import PermissionDenied
 from ninja_extra.permissions import IsAuthenticated
+from pydantic import NonNegativeInt
 
+from core.api_permissions import IsOldSubscriber
 from core.models import User
-from sas.models import Picture
-from sas.schemas import PictureFilterSchema, PictureSchema
+from sas.models import PeoplePictureRelation, Picture
+from sas.schemas import (
+    PictureFilterSchema,
+    PictureSchema,
+)
 
 
-@api_controller("/sas")
-class SasController(ControllerBase):
+@api_controller("/sas/pictures")
+class PicturesController(ControllerBase):
     @route.get(
-        "/picture",
+        "",
         response=list[PictureSchema],
         permissions=[IsAuthenticated],
         url_name="pictures",
@@ -45,3 +51,20 @@ class SasController(ControllerBase):
             picture.compressed_url = picture.get_download_compressed_url()
             picture.thumb_url = picture.get_download_thumb_url()
         return pictures
+
+
+@api_controller("/sas/relation")
+class UsersIdentifiedController(ControllerBase):
+    @route.delete("/{relation_id}", permissions=[IsOldSubscriber])
+    def delete_relation(self, relation_id: NonNegativeInt):
+        relation: PeoplePictureRelation = self.get_object_or_exception(
+            PeoplePictureRelation, pk=relation_id
+        )
+        user = self.context.request.user
+        if (
+            relation.user_id != user.id
+            and not user.is_root
+            and not user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
+        ):
+            raise PermissionDenied
+        relation.delete()
diff --git a/sas/schemas.py b/sas/schemas.py
index a8e74d20..14388a2d 100644
--- a/sas/schemas.py
+++ b/sas/schemas.py
@@ -1,10 +1,10 @@
 from datetime import datetime
 
-from ninja import FilterSchema, ModelSchema
-from pydantic import Field
+from ninja import FilterSchema, ModelSchema, Schema
+from pydantic import Field, NonNegativeInt
 
 from core.schemas import SimpleUserSchema
-from sas.models import Picture
+from sas.models import PeoplePictureRelation, Picture
 
 
 class PictureFilterSchema(FilterSchema):
@@ -23,3 +23,14 @@ class PictureSchema(ModelSchema):
     full_size_url: str
     compressed_url: str
     thumb_url: str
+
+
+class PictureCreateRelationSchema(Schema):
+    user_id: NonNegativeInt
+    picture_id: NonNegativeInt
+
+
+class CreatedPictureRelationSchema(ModelSchema):
+    class Meta:
+        model = PeoplePictureRelation
+        fields = ["id", "user", "picture"]
diff --git a/sas/templates/sas/picture.jinja b/sas/templates/sas/picture.jinja
index 191eac98..04222704 100644
--- a/sas/templates/sas/picture.jinja
+++ b/sas/templates/sas/picture.jinja
@@ -4,11 +4,19 @@
   <link rel="stylesheet" href="{{ scss('sas/picture.scss') }}">
 {%- endblock -%}
 
+{%- block additional_js -%}
+  <script src="{{ static('core/js/alpinejs.min.js') }}" defer></script>
+{%- endblock -%}
+
 {% block head %}
   {{ super() }}
 
   {% if picture.get_previous() %}
-    <link rel="preload" as="image" href="{{ url("sas:download_compressed", picture_id=picture.get_previous().id) }}">
+    <link
+      rel="preload"
+      as="image"
+      href="{{ url("sas:download_compressed", picture_id=picture.get_previous().id) }}"
+    >
   {% endif %}
   {% if picture.get_next() %}
     <link rel="preload" as="image" href="{{ url("sas:download_compressed", picture_id=picture.get_next().id) }}">
@@ -36,7 +44,8 @@
 
   <div class="title">
     <h3>{{ picture.get_display_name() }}</h3>
-    <h4>{{ picture.parent.children.filter(id__lte=picture.id).count() }} / {{ picture.parent.children.count() }}</h4>
+    <h4>{{ picture.parent.children.filter(id__lte=picture.id).count() }}
+      / {{ picture.parent.children.count() }}</h4>
   </div>
   <br>
 
@@ -100,7 +109,9 @@
           <h5>{% trans %}Tools{% endtrans %}</h5>
           <div>
             <div>
-              <a class="text" href="{{ picture.get_download_url() }}">{% trans %}HD version{% endtrans %}</a>
+              <a class="text" href="{{ picture.get_download_url() }}">
+                {% trans %}HD version{% endtrans %}
+              </a>
               <br>
               <a class="text danger" href="?ask_removal">{% trans %}Ask for removal{% endtrans %}</a>
             </div>
@@ -139,20 +150,18 @@
           {{ form.as_p() }}
           <input type="submit" value="{% trans %}Go{% endtrans %}" />
         </form>
-        <ul>
-          {% for r in picture.people.all() %}
+        <ul x-data="user_identification">
+          <template x-for="item in items" :key="item.id">
             <li>
-              <a class="user" href="{{ r.user.get_absolute_url() }}">
-                {% if r.user.profile_pict %}
-                  <div class="profile-pic" style="background-image: url('{{ r.user.profile_pict.get_download_url() }}');"></div>
-                {% endif %}
-                <span>{{ r.user.get_short_name() }}</span>
+              <a class="user" :href="item.user.url">
+                <img class="profile-pic" :src="item.user.picture" alt="image de profil"/>
+                <span x-text="item.user.name"></span>
               </a>
-              {% if user == r.user or user.can_edit(picture) %}
-                <a class="delete" href="?remove_user={{ r.user.id }}">❌</a>
-              {% endif %}
+              <template x-if="can_be_removed(item)">
+                <a class="delete clickable" @click="remove(item)">❌</a>
+              </template>
             </li>
-          {% endfor %}
+          </template>
         </ul>
       </div>
     </div>
@@ -162,6 +171,42 @@
 {% block script %}
   {{ super() }}
   <script>
+    document.addEventListener("alpine:init", () => {
+      Alpine.data("user_identification", () => ({
+        items: [
+          {%- for r in picture.people.select_related("user", "user__profile_pict") -%}
+            {
+              id: {{ r.id }},
+              user: {
+                id: {{ r.user.id }},
+                name: "{{ r.user.get_short_name()|safe }}",
+                url: "{{ r.user.get_absolute_url() }}",
+                {% if r.user.profile_pict %}
+                  picture: "{{ r.user.profile_pict.get_download_url() }}",
+                {% else %}
+                  picture: "{{ static('core/img/unknown.jpg') }}",
+                {% endif %}
+              },
+            },
+          {%- endfor -%}
+        ],
+        can_be_removed(item) {
+          {# If user is root or sas admin, he has the right, at "compile" time.
+             If not, he can delete only its own identification. #}
+          {% if user.is_root or user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID) %}
+            return true;
+          {% else %}
+            return item.user.id === {{ user.id }};
+          {% endif %}
+        },
+        async remove(item) {
+          const res = await fetch(`/api/sas/relation/${item.id}`, {method: "DELETE"});
+          if (res.ok) {
+            this.items = this.items.filter((i) => i.id !== item.id)
+          }
+        },
+      }))
+    });
     $(() => {
       $(document).keydown((e) => {
         switch (e.keyCode) {
diff --git a/sas/views.py b/sas/views.py
index 367771dc..9a885538 100644
--- a/sas/views.py
+++ b/sas/views.py
@@ -143,12 +143,6 @@ class PictureView(CanViewMixin, DetailView, FormMixin):
             self.object.rotate(270)
         if "rotate_left" in request.GET:
             self.object.rotate(90)
-        if "remove_user" in request.GET:
-            user = get_object_or_404(User, pk=int(request.GET["remove_user"]))
-            if user.id == request.user.id or request.user.is_in_group(
-                pk=settings.SITH_GROUP_SAS_ADMIN_ID
-            ):
-                user.picture.filter(picture=self.object).delete()
         if "ask_removal" in request.GET.keys():
             self.object.is_moderated = False
             self.object.asked_for_removal = True

From 191b05c305ca0a7db32431293ffea0043c7df680 Mon Sep 17 00:00:00 2001
From: thomas girod <thgirod@hotmail.com>
Date: Thu, 25 Jul 2024 23:31:54 +0200
Subject: [PATCH 2/3] Fix button to remove a user from picture

---
 sas/api.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sas/api.py b/sas/api.py
index 9e347edb..5d202942 100644
--- a/sas/api.py
+++ b/sas/api.py
@@ -14,7 +14,7 @@ from sas.schemas import (
 )
 
 
-@api_controller("/sas/pictures")
+@api_controller("/sas/picture")
 class PicturesController(ControllerBase):
     @route.get(
         "",

From 043dcfb28338d762beb88e0d0e9016ccecd7f61b Mon Sep 17 00:00:00 2001
From: thomas girod <thgirod@hotmail.com>
Date: Fri, 26 Jul 2024 14:25:26 +0200
Subject: [PATCH 3/3] add tests

---
 sas/api.py            | 29 ++++++++++++++----------
 sas/tests/test_api.py | 52 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/sas/api.py b/sas/api.py
index 5d202942..ea4c1c74 100644
--- a/sas/api.py
+++ b/sas/api.py
@@ -5,13 +5,9 @@ from ninja_extra.exceptions import PermissionDenied
 from ninja_extra.permissions import IsAuthenticated
 from pydantic import NonNegativeInt
 
-from core.api_permissions import IsOldSubscriber
 from core.models import User
 from sas.models import PeoplePictureRelation, Picture
-from sas.schemas import (
-    PictureFilterSchema,
-    PictureSchema,
-)
+from sas.schemas import PictureFilterSchema, PictureSchema
 
 
 @api_controller("/sas/picture")
@@ -28,11 +24,17 @@ class PicturesController(ControllerBase):
         A user with an active subscription can see any picture, as long
         as it has been moderated and not asked for removal.
         An unsubscribed user can see the pictures he has been identified on
-        (only the moderated ones, too)
+        (only the moderated ones, too).
 
         Notes:
             Trying to fetch the pictures of another user with this route
             while being unsubscribed will just result in an empty response.
+
+        Notes:
+            Unsubscribed users who are identified is not a rare case.
+            They can be UTT students, faluchards from other schools,
+            or even Richard Stallman (that ain't no joke,
+            cf. https://ae.utbm.fr/user/32663/pictures/)
         """
         user: User = self.context.request.user
         if not user.is_subscribed and filters.users_identified != {user.id}:
@@ -53,14 +55,17 @@ class PicturesController(ControllerBase):
         return pictures
 
 
-@api_controller("/sas/relation")
+@api_controller("/sas/relation", tags="User identification on SAS pictures")
 class UsersIdentifiedController(ControllerBase):
-    @route.delete("/{relation_id}", permissions=[IsOldSubscriber])
+    @route.delete("/{relation_id}", permissions=[IsAuthenticated])
     def delete_relation(self, relation_id: NonNegativeInt):
-        relation: PeoplePictureRelation = self.get_object_or_exception(
-            PeoplePictureRelation, pk=relation_id
-        )
-        user = self.context.request.user
+        """Untag a user from a SAS picture.
+
+        Root and SAS admins can delete any picture identification.
+        All other users can delete their own identification.
+        """
+        relation = self.get_object_or_exception(PeoplePictureRelation, pk=relation_id)
+        user: User = self.context.request.user
         if (
             relation.user_id != user.id
             and not user.is_root
diff --git a/sas/tests/test_api.py b/sas/tests/test_api.py
index 917f73d0..58079e70 100644
--- a/sas/tests/test_api.py
+++ b/sas/tests/test_api.py
@@ -1,10 +1,12 @@
+from django.conf import settings
+from django.db import transaction
 from django.test import TestCase
 from django.urls import reverse
 from model_bakery import baker
 from model_bakery.recipe import Recipe
 
 from core.baker_recipes import old_subscriber_user, subscriber_user
-from core.models import User
+from core.models import RealGroup, User
 from sas.models import Album, PeoplePictureRelation, Picture
 
 
@@ -32,6 +34,8 @@ class TestSas(TestCase):
             baker.make(PeoplePictureRelation, picture=pictures[4], user=cls.user_b)
             baker.make(PeoplePictureRelation, picture=pictures[4], user=cls.user_c)
 
+
+class TestPictureSearch(TestSas):
     def test_anonymous_user_forbidden(self):
         res = self.client.get(reverse("api:pictures"))
         assert res.status_code == 403
@@ -101,3 +105,49 @@ class TestSas(TestCase):
             + f"?users_identified={self.user_a.id}&users_identified={self.user_b.id}"
         )
         assert res.status_code == 403
+
+
+class TestPictureRelation(TestSas):
+    def test_delete_relation_route_forbidden(self):
+        """Test that unauthorized users are properly 403ed"""
+        # take a picture where user_a isn't identified
+        relation = PeoplePictureRelation.objects.exclude(user=self.user_a).first()
+
+        res = self.client.delete(f"/api/sas/relation/{relation.id}")
+        assert res.status_code == 403
+
+        for user in baker.make(User), self.user_a:
+            self.client.force_login(user)
+            res = self.client.delete(f"/api/sas/relation/{relation.id}")
+            assert res.status_code == 403
+
+    def test_delete_relation_with_authorized_users(self):
+        """Test that deletion works as intended when called by an authorized user."""
+        relation: PeoplePictureRelation = self.user_a.pictures.first()
+        sas_admin_group = RealGroup.objects.get(pk=settings.SITH_GROUP_SAS_ADMIN_ID)
+        sas_admin = baker.make(User, groups=[sas_admin_group])
+        root = baker.make(User, is_superuser=True)
+        for user in sas_admin, root, self.user_a:
+            with transaction.atomic():
+                self.client.force_login(user)
+                res = self.client.delete(f"/api/sas/relation/{relation.id}")
+                assert res.status_code == 200
+                assert not PeoplePictureRelation.objects.filter(pk=relation.id).exists()
+                transaction.set_rollback(True)
+        public = baker.make(User)
+        relation = public.pictures.create(picture=relation.picture)
+        self.client.force_login(public)
+        res = self.client.delete(f"/api/sas/relation/{relation.id}")
+        assert res.status_code == 200
+        assert not PeoplePictureRelation.objects.filter(pk=relation.id).exists()
+
+    def test_delete_twice(self):
+        """Test a duplicate call on the delete route."""
+        self.client.force_login(baker.make(User, is_superuser=True))
+        relation = PeoplePictureRelation.objects.first()
+        res = self.client.delete(f"/api/sas/relation/{relation.id}")
+        assert res.status_code == 200
+        relation_count = PeoplePictureRelation.objects.count()
+        res = self.client.delete(f"/api/sas/relation/{relation.id}")
+        assert res.status_code == 404
+        assert PeoplePictureRelation.objects.count() == relation_count