Skip to content

Commit

Permalink
feat: improve the permission strategy related to proof
Browse files Browse the repository at this point in the history
- allow anonymous proof upload (a new proof.anonymous field tracks
  which proof was uploaded by an anonymous user)
- allow to retrieve and list all proofs, irrespective of your
  authentication status
- allow users to add a price to a proof they don't own
- allow moderators to update/delete a proof

Tracking history for the proof table is meant to be made in a future PR
  • Loading branch information
raphael0202 committed Dec 5, 2024
1 parent 42f1c5e commit a4d50fc
Show file tree
Hide file tree
Showing 9 changed files with 108 additions and 55 deletions.
10 changes: 7 additions & 3 deletions open_prices/api/prices/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down
61 changes: 35 additions & 26 deletions open_prices/api/proofs/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)
Expand Down Expand Up @@ -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)


Expand All @@ -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,
Expand All @@ -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)


Expand All @@ -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}"}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
)
Expand All @@ -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):
Expand Down Expand Up @@ -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)
56 changes: 40 additions & 16 deletions open_prices/api/proofs/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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,
Expand All @@ -31,28 +47,31 @@ 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
ordering_fields = ["date", "price_count", "created"]
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):
Expand Down Expand Up @@ -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)

Expand Down
6 changes: 0 additions & 6 deletions open_prices/prices/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 2 additions & 4 deletions open_prices/prices/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions open_prices/proofs/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
17 changes: 17 additions & 0 deletions open_prices/proofs/migrations/0007_proof_anonymous.py
Original file line number Diff line number Diff line change
@@ -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),
),
]
1 change: 1 addition & 0 deletions open_prices/proofs/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions open_prices/users/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit a4d50fc

Please sign in to comment.