diff --git a/open_prices/api/prices/tests.py b/open_prices/api/prices/tests.py index af7cd2b5..963a3a36 100644 --- a/open_prices/api/prices/tests.py +++ b/open_prices/api/prices/tests.py @@ -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 91256b43..29c87467 100644 --- a/open_prices/api/proofs/tests.py +++ b/open_prices/api/proofs/tests.py @@ -68,14 +68,10 @@ def setUpTestData(cls): ) def test_proof_list(self): - # anonymous + # all proofs are read-accessible to anyone response = self.client.get(self.url) - self.assertEqual(response.status_code, 403) - # wrong token - response = self.client.get( - self.url, headers={"Authorization": f"Bearer {self.user_session.token}X"} - ) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["total"], 3) # All proofs are returned # authenticated # thanks to select_related and prefetch_related, we only have 6 # queries: @@ -91,8 +87,8 @@ def test_proof_list(self): ) self.assertEqual(response.status_code, 200) data = response.data - self.assertEqual(data["total"], 2) # only user's proofs - self.assertEqual(len(data["items"]), 2) + self.assertEqual(data["total"], 3) # only user's proofs + self.assertEqual(len(data["items"]), 3) item = data["items"][0] self.assertEqual(item["id"], self.proof.id) # default order self.assertIn("predictions", item) @@ -125,7 +121,7 @@ def test_proof_list_order_by(self): response = self.client.get( url, headers={"Authorization": f"Bearer {self.user_session.token}"} ) - self.assertEqual(response.data["total"], 2) + self.assertEqual(response.data["total"], 3) self.assertEqual(response.data["items"][0]["price_count"], 50) @@ -134,10 +130,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(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, @@ -149,7 +147,7 @@ def test_proof_list_filter_by_type(self): response = self.client.get( url, headers={"Authorization": f"Bearer {self.user_session.token}"} ) - self.assertEqual(response.data["total"], 1) + self.assertEqual(response.data["total"], 2) self.assertEqual(response.data["items"][0]["price_count"], 15) @@ -173,12 +171,8 @@ def test_proof_detail(self): self.assertEqual(response.data["detail"], "No Proof matches the given query.") # anonymous response = self.client.get(self.url) - self.assertEqual(response.status_code, 403) - # wrong token - response = self.client.get( - self.url, headers={"Authorization": f"Bearer {self.user_session_1.token}X"} - ) - self.assertEqual(response.status_code, 403) + 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}"} @@ -207,14 +201,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, @@ -346,6 +337,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 ) @@ -358,6 +351,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): @@ -389,3 +387,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 35eeff34..f8c6933b 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 IsAuthenticated +from rest_framework.permissions import BasePermission from rest_framework.request import Request from rest_framework.response import Response @@ -23,6 +23,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, @@ -31,9 +47,9 @@ class ProofViewSet( viewsets.GenericViewSet, ): authentication_classes = [CustomAuthentication] - permission_classes = [IsAuthenticated] + permission_classes = [ProofPermission] http_method_names = ["get", "post", "patch", "delete"] # disable "put" - queryset = Proof.objects.none() + queryset = Proof.objects.all() serializer_class = ProofFullSerializer filter_backends = [DjangoFilterBackend, filters.OrderingFilter] filterset_class = ProofFilter @@ -41,18 +57,21 @@ class ProofViewSet( ordering = ["created"] def get_queryset(self): - # only return proofs owned by the current user - if self.request.user.is_authenticated: - queryset = Proof.objects.filter(owner=self.request.user.user_id) - if self.request.method in ["GET"]: - # 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 queryset.select_related("location").prefetch_related( - "predictions" - ) - return queryset + if self.request.method in ["GET"]: + # 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"]: + 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): @@ -99,8 +118,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..b3e51a62 100644 --- a/open_prices/prices/models.py +++ b/open_prices/prices/models.py @@ -463,12 +463,6 @@ def clean(self, *args, **kwargs): ) if proof: - if proof.owner != self.owner: - validation_errors = utils.add_validation_error( - validation_errors, - "proof", - "Proof does not belong to the current user", - ) if not self.id: # skip these checks on update if proof.type in proof_constants.TYPE_SINGLE_SHOP_LIST: for PROOF_FIELD in Price.DUPLICATE_PROOF_FIELDS: diff --git a/open_prices/prices/tests.py b/open_prices/prices/tests.py index 6f01b774..643a1de1 100644 --- a/open_prices/prices/tests.py +++ b/open_prices/prices/tests.py @@ -374,10 +374,8 @@ def test_price_proof_validation(self): currency=user_proof_receipt.currency, owner=user_proof_receipt.owner, ) - # different price & proof owner - self.assertRaises( - ValidationError, - PriceFactory, + # different price & proof owner should not raise a ValidationError + PriceFactory( proof_id=proof_2.id, # different location_osm_id=user_proof_receipt.location_osm_id, location_osm_type=user_proof_receipt.location_osm_type, 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/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