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.

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

Implement API create World #248

Merged
merged 9 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions server/venueless/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@
"worlds/<str:world_id>/favourite-talk/",
views.UserFavouriteView.as_view(),
),
path("create-world/", views.CreateWorldView.as_view()),
path("worlds/<str:world_id>/export-talk", views.ExportView.as_view()),
]
88 changes: 88 additions & 0 deletions server/venueless/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@
from contextlib import suppress
from urllib.parse import urlparse

import jwt
import requests
from asgiref.sync import async_to_sync
from django.conf import settings
from django.core import exceptions
from django.db import transaction
from django.forms.models import model_to_dict
from django.http import JsonResponse
from django.shortcuts import get_object_or_404
from django.utils.crypto import get_random_string
from django.utils.timezone import now
from rest_framework import viewsets
from rest_framework.authentication import get_authorization_header
Expand Down Expand Up @@ -115,6 +119,90 @@ def get(self, request, **kwargs):
)


class CreateWorldView(APIView):
Copy link

Choose a reason for hiding this comment

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

🚨 suggestion (security): Implement proper authentication and authorization for CreateWorldView

The current implementation with empty authentication_classes and permission_classes could allow unauthorized access to world creation. Consider implementing appropriate authentication and permission checks.

class CreateWorldView(APIView):
    authentication_classes = [SessionAuthentication, BasicAuthentication]
    permission_classes = [IsAdminUser]

authentication_classes = [] # disables authentication
permission_classes = []

@staticmethod
def post(request, *args, **kwargs) -> JsonResponse:
payload = CreateWorldView.get_payload_from_token(request)

# check if user has permission to create world
if payload.get("has_permission"):
secret = get_random_string(length=64)
config = {
"JWT_secrets": [
{
"issuer": "any",
"audience": "venueless",
"secret": secret,
}
]
}

title_default = {
Copy link
Member

Choose a reason for hiding this comment

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

This impementation leave to many holes:

  • data.get() can return None and None.items() will throw exception.
  • set.pop() will throw exception if set is empty.

Also, if you don't use key, retrieving values from dict instead.

value for key, value in request.data.get("title").items() if value
}.pop()

# if world already exists, update it, otherwise create a new world
try:
if World.objects.filter(id=request.data.get("id")).exists():
Copy link
Member

Choose a reason for hiding this comment

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

Data received from outside work (form, HTTP request) have to be validated.

You are calling

World.objects.get(id=request.data.get("id")

but what if some attackers pass "id=abc"? This code will throw exception.

world = World.objects.get(id=request.data.get("id"))
world.title = (
request.data.get("title")[request.data.get("locale")]
or request.data.get("title")["en"]
or title_default
)
world.domain = "{}{}/{}".format(
settings.DOMAIN_PATH, settings.BASE_PATH, request.data.get("id")
)
world.locale = request.data.get("locale")
world.timezone = request.data.get("timezone")
world.save()
else:
world = World.objects.create(
id=request.data.get("id"),
title=request.data.get("title")[request.data.get("locale")]
or request.data.get("title")["en"]
Copy link
Member

Choose a reason for hiding this comment

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

What if data(get('title')) returns None or something not a dict. Validation needs to be applied to make sure the incoming data has the expected data type and shape.

or title_default,
domain="{}{}/{}".format(
settings.DOMAIN_PATH,
settings.BASE_PATH,
request.data.get("id"),
),
locale=request.data.get("locale"),
timezone=request.data.get("timezone"),
config=config,
)
except Exception as e:
Copy link

Choose a reason for hiding this comment

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

suggestion (bug_risk): Implement more granular exception handling

Catching a broad Exception can mask specific errors. Consider catching and handling specific exceptions (e.g., IntegrityError, ValidationError) to provide more informative error messages and easier debugging.

            except (IntegrityError, ValidationError) as e:
                logger.error(f"Error creating world: {e}")
                return JsonResponse({'error': str(e)}, status=400)
            except Exception as e:
                logger.error(f"Unexpected error creating world: {e}")
                return JsonResponse({'error': 'An unexpected error occurred'}, status=500)

Copy link
Member

Choose a reason for hiding this comment

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

This is a bad practice to catch a very general exception class. This has been reminded before.

Copy link

Choose a reason for hiding this comment

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

Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:

- Avoid suggesting changes that are already well-understood best practices unless there is a specific context or evidence that the current implementation is causing issues.
- Ensure that suggestions for code improvements are backed by specific examples or evidence of potential problems in the current code.
- Provide actionable and context-specific advice rather than general coding best practices.

logger.error("An error occurred while creating a world: {}".format(e))
return JsonResponse(
{"error": "Unable to create or update world"}, status=400
)

return JsonResponse(model_to_dict(world, exclude=["roles"]), status=201)
else:
return JsonResponse(
{"error": "World cannot be created due to missing permission"},
status=403,
)

@staticmethod
def get_payload_from_token(request):
auth_header = get_authorization_header(request).split()
if auth_header and auth_header[0].lower() == b"bearer":
if len(auth_header) == 1:
raise exceptions.AuthenticationFailed(
"Invalid token header. No credentials provided."
)
elif len(auth_header) > 2:
raise exceptions.AuthenticationFailed(
"Invalid token header. Token string should not contain spaces."
)
payload = jwt.decode(auth_header[1], settings.SECRET_KEY, algorithms=["HS256"])
Copy link

Choose a reason for hiding this comment

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

suggestion: Add exception handling for JWT decoding

JWT decoding can raise exceptions if the token is invalid or expired. Implement try-except blocks to handle potential jwt.DecodeError and jwt.ExpiredSignatureError exceptions.

try:
    payload = jwt.decode(auth_header[1], settings.SECRET_KEY, algorithms=["HS256"])
except jwt.ExpiredSignatureError:
    raise exceptions.AuthenticationFailed("Token has expired")
except jwt.DecodeError:
    raise exceptions.AuthenticationFailed("Invalid token")
return payload

Copy link
Member

Choose a reason for hiding this comment

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

Please follow sourcery-api.

return payload


class UserFavouriteView(APIView):
permission_classes = []

Expand Down
3 changes: 3 additions & 0 deletions server/venueless/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
"VENUELESS_DJANGO_SECRET", config.get("django", "secret", fallback="")
)
BASE_PATH = config.get("venueless", "base_path", fallback="")
DOMAIN_PATH = config.get(
"venueless", "domain_path", fallback="https://app.eventyay.com"
)
if not SECRET_KEY:
SECRET_FILE = os.path.join(DATA_DIR, ".secret")
if os.path.exists(SECRET_FILE):
Expand Down
Loading