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 4 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
2 changes: 1 addition & 1 deletion server/venueless/api/urls.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from django.urls import include, path, re_path
from rest_framework import routers

from . import views

world_router = routers.DefaultRouter()
Expand All @@ -16,5 +15,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()),
]
84 changes: 76 additions & 8 deletions server/venueless/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from rest_framework.decorators import api_view, permission_classes
from rest_framework.response import Response
from rest_framework.views import APIView
from django.forms.models import model_to_dict

from venueless.api.auth import (
ApiAccessRequiredPermission,
Expand All @@ -27,6 +28,9 @@
from venueless.core.services.world import notify_schedule_change, notify_world_change

from ..core.models import Room, World
import jwt
from django.conf import settings
from django.utils.crypto import get_random_string

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -115,6 +119,70 @@ 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,
}
]
}

# if world already exists, update it, otherwise create a new world
try:
if World.objects.filter(id=request.data.get('id')).exists():
world = World.objects.get(id=request.data.get('id'))
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): Use .get() with default value or check key existence

Using .get() without a default value on request.data can raise KeyError if the key doesn't exist. Consider using .get('id', None) or check if 'id' in request.data before accessing it.

world_id = request.data.get('id')
if world_id is not None:
    world = World.objects.get(id=world_id)

world.title = request.data.get('title')[request.data.get('locale')] or request.data.get('title')[
'en']
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'],
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(f"An error occurred while creating a world: %s" % 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 Expand Up @@ -183,19 +251,19 @@ def get(request, *args, **kwargs):
talk_config = world.config.get("pretalx")
user = User.objects.filter(token_id=request.user)
talk_base_url = (
talk_config.get("domain")
+ "/"
+ talk_config.get("event")
+ "/schedule/export/"
talk_config.get("domain")
+ "/"
+ talk_config.get("event")
+ "/schedule/export/"
)
export_endpoint = "schedule." + export_type
talk_url = talk_base_url + export_endpoint
if "my" in export_type and user:
user_state = user.first().client_state
if (
user_state
and user_state.get("schedule")
and user_state.get("schedule").get("favs")
user_state
and user_state.get("schedule")
and user_state.get("schedule").get("favs")
):
talk_list = user_state.get("schedule").get("favs")
talk_list_str = ",".join(talk_list)
Expand Down Expand Up @@ -231,7 +299,7 @@ def schedule_update(request, **kwargs):

pretalx_config = request.world.config.get("pretalx", {})
if domain != get_domain(
pretalx_config.get("domain")
pretalx_config.get("domain")
) or event != pretalx_config.get("event"):
return Response("Incorrect domain or event data", status=401)

Expand Down
1 change: 1 addition & 0 deletions server/venueless/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"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")
Copy link

Choose a reason for hiding this comment

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

🚨 suggestion (security): Consider using environment variables for configuration

Using environment variables for configuration settings like DOMAIN_PATH can improve security and flexibility, especially when deploying in different environments.

Suggested change
DOMAIN_PATH = config.get("venueless", "domain_path", fallback="https://app.eventyay.com")
DOMAIN_PATH = os.environ.get('VENUELESS_DOMAIN_PATH', '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