Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer UUID validation inside serializer #3068

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Transfer UUID validation inside serializer #3068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60a810e
Transfer UUID validation inside serializer
dhruvkb Sep 27, 2023
9c58879
Add test case
dhruvkb Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion api/api/serializers/image_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from typing import Literal
from uuid import UUID

from rest_framework import serializers

Expand Down Expand Up @@ -124,7 +125,15 @@ class OembedRequestSerializer(serializers.Serializer):

@staticmethod
def validate_url(value):
return add_protocol(value)
url = add_protocol(value)
if url.endswith("/"):
url = url[:-1]
identifier = url.rsplit("/", 1)[1]
try:
uuid = UUID(identifier)
Comment on lines 127 to +133
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it odd that the URL validator is returning the image identifier.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to just be the way this parameter is set up - ultimately we want an identifier out of it, even if the parameter itself is the url field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's confusing since it doesn't even check if value is a valid URL or not. I'd like us to go with this alternative solution.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is for sure confusing that it doesn't check if value is a valid URL, but http://localhost:50280/v1/images/oembed/?url=2982c0bf-9c26-4543-a712-4d50a117ded8 for example works on main and on this branch but not on the proposed alternative. Given this is critical, do we know we want to make that change in behavior as part of the fix? FWIW I agree it makes more sense in general, but worth asking.

except ValueError:
raise serializers.ValidationError("Could not parse identifier from URL.")
return uuid


class OembedSerializer(BaseModelSerializer):
Expand Down
5 changes: 1 addition & 4 deletions api/api/views/image_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,7 @@ def oembed(self, request, *_, **__):

context = self.get_serializer_context()

url = params.validated_data["url"]
if url.endswith("/"):
url = url[:-1]
identifier = url.rsplit("/", 1)[1]
identifier = params.validated_data["url"]
image = get_object_or_404(Image, identifier=identifier)
if not (image.height and image.width):
image_file = requests.get(image.url, headers=self.OEMBED_HEADERS)
Expand Down
10 changes: 10 additions & 0 deletions api/test/test_image_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ def test_oembed_endpoint_with_non_existent_image():
assert response.status_code == 404


def test_oembed_endpoint_with_bad_identifier():
params = {
"url": "https://any.domain/any/path/not-a-valid-uuid",
}
response = requests.get(
f"{API_URL}/v1/images/oembed?{urlencode(params)}", verify=False
)
assert response.status_code == 400


@pytest.mark.parametrize(
"url",
[
Expand Down