diff --git a/open_prices/api/prices/tests.py b/open_prices/api/prices/tests.py index af7cd2b5..f0e17feb 100644 --- a/open_prices/api/prices/tests.py +++ b/open_prices/api/prices/tests.py @@ -330,7 +330,7 @@ def setUpTestData(cls): cls.user_proof = ProofFactory( type=proof_constants.TYPE_RECEIPT, owner=cls.user_session.user.user_id ) - cls.proof_2 = ProofFactory() + cls.proof_2 = ProofFactory(type=proof_constants.TYPE_PRICE_TAG) cls.data = { **PRICE_8001505005707, "location_osm_id": 652825274, @@ -338,6 +338,9 @@ def setUpTestData(cls): "proof_id": cls.user_proof.id, "source": "test", } + cls.data_2 = cls.data.copy() + cls.data_2["product_code"] = "1402506209800" + cls.data_2["location_osm_id"] = 169424088 def test_price_create_without_proof(self): data = self.data.copy() @@ -383,14 +386,15 @@ def test_price_create_with_proof(self): content_type="application/json", ) self.assertEqual(response.status_code, 400) - # not proof owner + # Users are allowed to add price on proofs they don't own response = self.client.post( self.url, - {**self.data, "proof_id": self.proof_2.id}, + {**self.data_2, "proof_id": self.proof_2.id}, headers={"Authorization": f"Bearer {self.user_session.token}"}, content_type="application/json", ) - self.assertEqual(response.status_code, 400) + self.assertEqual(response.status_code, 201) + self.assertEqual(response.data["product_code"], "1402506209800") # authenticated response = self.client.post( self.url, diff --git a/open_prices/api/proofs/tests.py b/open_prices/api/proofs/tests.py index 2c1f777b..7ba81e73 100644 --- a/open_prices/api/proofs/tests.py +++ b/open_prices/api/proofs/tests.py @@ -72,7 +72,8 @@ def test_proof_list(self): # thanks to select_related, we only have 2 queries: # - 1 to count the number of proofs of the user # - 1 to get the proofs and their associated locations (select_related) - with self.assertNumQueries(2): + # - 1 to get the proof predictions (prefetch_related) + with self.assertNumQueries(3): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) data = response.data @@ -110,10 +111,12 @@ class ProofListFilterApiTest(TestCase): def setUpTestData(cls): cls.url = reverse("api:proofs-list") cls.user_session = SessionFactory() + # type RECEIPT cls.proof = ProofFactory( **PROOF, price_count=15, owner=cls.user_session.user.user_id ) - ProofFactory(type=proof_constants.TYPE_PRICE_TAG, price_count=0) + # type RECEIPT, but not owned by the user + ProofFactory(type=proof_constants.TYPE_RECEIPT, price_count=15) ProofFactory( type=proof_constants.TYPE_PRICE_TAG, price_count=50, @@ -122,8 +125,10 @@ def setUpTestData(cls): def test_proof_list_filter_by_type(self): url = self.url + "?type=RECEIPT" - response = self.client.get(url) - self.assertEqual(response.data["total"], 1) + response = self.client.get( + url, headers={"Authorization": f"Bearer {self.user_session.token}"} + ) + self.assertEqual(response.data["total"], 2) self.assertEqual(response.data["items"][0]["price_count"], 15) def test_proof_list_filter_by_owner(self): @@ -156,6 +161,12 @@ def test_proof_detail(self): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["id"], self.proof.id) + # authenticated + response = self.client.get( + self.url, headers={"Authorization": f"Bearer {self.user_session_1.token}"} + ) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["id"], self.proof.id) self.assertIn("predictions", response.data) # returned in "detail" self.assertEqual(len(response.data["predictions"]), 1) prediction = response.data["predictions"][0] @@ -186,14 +197,11 @@ def tearDown(self): def test_proof_create(self): # anonymous response = self.client.post(self.url, self.data) - self.assertEqual(response.status_code, 403) - # wrong token - response = self.client.post( - self.url, - self.data, - headers={"Authorization": f"Bearer {self.user_session.token}X"}, - ) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 201) + self.assertTrue(response.data["file_path"] is not None) + self.assertEqual(response.data["owner"], None) + self.assertEqual(response.data["anonymous"], True) + # authenticated response = self.client.post( self.url, @@ -325,6 +333,8 @@ class ProofDeleteApiTest(TestCase): def setUpTestData(cls): cls.user_session_1 = SessionFactory() cls.user_session_2 = SessionFactory() + cls.user_session_3 = SessionFactory() + cls.moderator = SessionFactory(user__is_moderator=True) cls.proof = ProofFactory( **PROOF, price_count=15, owner=cls.user_session_1.user.user_id ) @@ -337,6 +347,11 @@ def setUpTestData(cls): date=cls.proof.date, owner=cls.proof.owner, ) + proof_2_data = PROOF.copy() + proof_2_data["date"] = "2024-06-06" + cls.proof_2 = ProofFactory( + **proof_2_data, price_count=0, owner=cls.user_session_3.user.user_id + ) cls.url = reverse("api:proofs-detail", args=[cls.proof.id]) def test_proof_delete(self): @@ -368,3 +383,14 @@ def test_proof_delete(self): self.assertEqual( Proof.objects.filter(owner=self.user_session_1.user.user_id).count(), 0 ) + + # Delete request from a moderator, should work + # Proof 1 was deleted, let's delete proof 2 + url_proof_2 = reverse("api:proofs-detail", args=[self.proof_2.id]) + response = self.client.delete( + url_proof_2, + headers={"Authorization": f"Bearer {self.moderator.token}"}, + ) + self.assertEqual(response.status_code, 204) + self.assertEqual(response.data, None) + self.assertEqual(Proof.objects.filter(id=self.proof_2.id).count(), 0) diff --git a/open_prices/api/proofs/views.py b/open_prices/api/proofs/views.py index 9352f35b..e81fcb48 100644 --- a/open_prices/api/proofs/views.py +++ b/open_prices/api/proofs/views.py @@ -4,7 +4,7 @@ from rest_framework import filters, mixins, status, viewsets from rest_framework.decorators import action from rest_framework.parsers import MultiPartParser -from rest_framework.permissions import IsAuthenticatedOrReadOnly +from rest_framework.permissions import BasePermission from rest_framework.request import Request from rest_framework.response import Response @@ -24,6 +24,22 @@ from open_prices.proofs.utils import store_file +class ProofPermission(BasePermission): + def has_permission(self, request, view): + if view.action in ("retrieve", "list"): + # Allow any user (even anonymous) to view any proof + return True + elif request.method == "POST" and request.path.startswith( + "/api/v1/proofs/upload" + ): + # Allow anonymous users to upload proofs + return True + + # Require authenticated user for the rest ("destroy", "update", + # Gemini proof processing) + return request.user and request.user.is_authenticated + + class ProofViewSet( mixins.ListModelMixin, mixins.RetrieveModelMixin, @@ -32,7 +48,7 @@ class ProofViewSet( viewsets.GenericViewSet, ): authentication_classes = [CustomAuthentication] - permission_classes = [IsAuthenticatedOrReadOnly] + permission_classes = [ProofPermission] http_method_names = ["get", "post", "patch", "delete"] # disable "put" queryset = Proof.objects.all() serializer_class = ProofFullSerializer @@ -42,16 +58,22 @@ class ProofViewSet( ordering = ["created"] def get_queryset(self): - queryset = self.queryset if self.request.method in ["GET"]: - queryset = queryset.select_related("location") - if self.action == "retrieve": - queryset = queryset.prefetch_related("predictions") + # Select all proofs along with their locations using a select + # related query (1 single query) + # Then prefetch all the predictions related to the proof using + # a prefetch related query (only 1 query for all proofs) + return self.queryset.select_related("location").prefetch_related( + "predictions" + ) elif self.request.method in ["PATCH", "DELETE"]: - # only return proofs owned by the current user - if self.request.user.is_authenticated: - queryset = queryset.filter(owner=self.request.user.user_id) - return queryset + if self.request.user.is_moderator: + return self.queryset + # for patch and delete actions, only return proofs owned by the + # current user if not a moderator + return self.queryset.filter(owner=self.request.user.user_id) + + return self.queryset def get_serializer_class(self): if self.request.method == "PATCH": @@ -99,8 +121,13 @@ def upload(self, request: Request) -> Response: serializer.is_valid(raise_exception=True) # get source source = get_source_from_request(self.request) + # Here, user is either a `open_prices.users.models.User` (if + # authenticated) or an `django.contrib.auth.models.AnonymousUser` + # (if not authenticated) + user = self.request.user + owner = None if user.is_anonymous else user.user_id # save - proof = serializer.save(owner=self.request.user.user_id, source=source) + proof = serializer.save(owner=owner, source=source, anonymous=user.is_anonymous) # return full proof return Response(ProofFullSerializer(proof).data, status=status.HTTP_201_CREATED) diff --git a/open_prices/prices/models.py b/open_prices/prices/models.py index da152611..07eb18a8 100644 --- a/open_prices/prices/models.py +++ b/open_prices/prices/models.py @@ -463,11 +463,16 @@ def clean(self, *args, **kwargs): ) if proof: - if proof.owner != self.owner: + if ( + proof.owner != self.owner + and proof.type != proof_constants.TYPE_PRICE_TAG + ): validation_errors = utils.add_validation_error( validation_errors, "proof", - "Proof does not belong to the current user", + "Proof does not belong to the current user. " + "Adding price to proof a user does not own is " + "only allowed for PRICE_TAG proofs", ) if not self.id: # skip these checks on update if proof.type in proof_constants.TYPE_SINGLE_SHOP_LIST: diff --git a/open_prices/prices/tests.py b/open_prices/prices/tests.py index 6f01b774..1b09ea4b 100644 --- a/open_prices/prices/tests.py +++ b/open_prices/prices/tests.py @@ -358,7 +358,14 @@ def test_price_proof_validation(self): currency="EUR", owner=user_session.user.user_id, ) - proof_2 = ProofFactory() + proof_gdpr = ProofFactory( + type=proof_constants.TYPE_GDPR_REQUEST, + location_osm_id=169450062, + location_osm_type=location_constants.OSM_TYPE_NODE, + date="2024-10-01", + currency="EUR", + ) + proof_price_tag = ProofFactory(type=proof_constants.TYPE_PRICE_TAG) # proof not set PriceFactory(proof_id=None, owner=user_proof_receipt.owner) # proof unknown @@ -374,16 +381,27 @@ def test_price_proof_validation(self): currency=user_proof_receipt.currency, owner=user_proof_receipt.owner, ) - # different price & proof owner + # difference price and proof owner should raise a ValidationError + # if the proof type is different than PRICE_TAG self.assertRaises( ValidationError, PriceFactory, - proof_id=proof_2.id, # different + proof_id=proof_gdpr.id, # gdpr proof + location_osm_id=proof_gdpr.location_osm_id, + location_osm_type=proof_gdpr.location_osm_type, + date=proof_gdpr.date, + currency=proof_gdpr.currency, + owner=user_proof_receipt.owner, # different owner + ) + # different price & proof owner should not raise a ValidationError + # for PRICE_TAG proofs + PriceFactory( + proof_id=proof_price_tag.id, location_osm_id=user_proof_receipt.location_osm_id, location_osm_type=user_proof_receipt.location_osm_type, date=user_proof_receipt.date, currency=user_proof_receipt.currency, - owner=user_proof_receipt.owner, + owner=user_proof_receipt.owner, # different owner ) # proof location_osm_id & location_osm_type self.assertRaises( diff --git a/open_prices/proofs/factories.py b/open_prices/proofs/factories.py index 4cac16b4..4a9c2884 100644 --- a/open_prices/proofs/factories.py +++ b/open_prices/proofs/factories.py @@ -12,6 +12,7 @@ class ProofFactory(DjangoModelFactory): class Meta: model = Proof + owner = factory.Faker("user_name") file_path = factory.Faker("file_path") mimetype = "image/jpeg" type = factory.fuzzy.FuzzyChoice(proof_constants.TYPE_LIST) diff --git a/open_prices/proofs/migrations/0007_proof_anonymous.py b/open_prices/proofs/migrations/0007_proof_anonymous.py new file mode 100644 index 00000000..6b26b07b --- /dev/null +++ b/open_prices/proofs/migrations/0007_proof_anonymous.py @@ -0,0 +1,17 @@ +# Generated by Django 5.1 on 2024-12-05 14:21 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("proofs", "0006_add_proof_prediction_table"), + ] + + operations = [ + migrations.AddField( + model_name="proof", + name="anonymous", + field=models.BooleanField(default=False), + ), + ] diff --git a/open_prices/proofs/models.py b/open_prices/proofs/models.py index ef6a9fab..6370b270 100644 --- a/open_prices/proofs/models.py +++ b/open_prices/proofs/models.py @@ -99,6 +99,7 @@ class Proof(models.Model): price_count = models.PositiveIntegerField(default=0, blank=True, null=True) owner = models.CharField(blank=True, null=True) + anonymous = models.BooleanField(default=False) source = models.CharField(blank=True, null=True) created = models.DateTimeField(default=timezone.now) diff --git a/open_prices/proofs/tests.py b/open_prices/proofs/tests.py index 8eb1c413..50792894 100644 --- a/open_prices/proofs/tests.py +++ b/open_prices/proofs/tests.py @@ -155,7 +155,11 @@ class ProofQuerySetTest(TestCase): def setUpTestData(cls): cls.proof_without_price = ProofFactory(type=proof_constants.TYPE_PRICE_TAG) cls.proof_with_price = ProofFactory(type=proof_constants.TYPE_GDPR_REQUEST) - PriceFactory(proof_id=cls.proof_with_price.id, price=1.0) + PriceFactory( + proof_id=cls.proof_with_price.id, + price=1.0, + owner=cls.proof_with_price.owner, + ) def test_has_type_single_shop(self): self.assertEqual(Proof.objects.count(), 2) @@ -204,6 +208,7 @@ def setUpTestData(cls): price=2.0, currency="EUR", date="2024-06-30", + owner=cls.proof_receipt.owner, ) def test_is_type_single_shop(self): diff --git a/open_prices/users/models.py b/open_prices/users/models.py index 77684319..aa128ddd 100644 --- a/open_prices/users/models.py +++ b/open_prices/users/models.py @@ -33,9 +33,14 @@ class Meta: verbose_name = "User" verbose_name_plural = "Users" + @property def is_authenticated(self): return True + @property + def is_anonymous(self): + return False + def update_price_count(self): from open_prices.prices.models import Price