Skip to content

Conversation

@suhailnadaf509
Copy link
Contributor

Summary

This PR adds a new Speaker API and improves authentication, CORS configuration, and URL validation across the codebase.

Changes

  • New Speaker API

    • Added speaker.py with serializer and viewset for speaker data (code, name, bio, submissions).
    • Supports listing/retrieving speakers, pagination, and lookup by code (case-insensitive).
    • Public access enabled with AllowAny permission.
    • Registered endpoint in urls.py as /api/events/<event_id>/speakers/.
  • Auth Improvements

    • Updated api_auth.py to handle unauthenticated requests using event_id or slug.
    • Improved error handling and event resolution logic.
  • CORS Configuration

    • Updated settings.py to include 127.0.0.1 in the dev whitelist.
    • Enabled credentials and expanded allowed headers.
  • WebSocket Fix

    • Added initialization safety in consumers.py to prevent crashes when components aren’t ready.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Sorry @suhailnadaf509, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new Speaker API endpoint and enhances authentication handling, CORS configuration, and URL validation. The main focus is enabling public access to speaker data while improving the framework for handling authenticated and unauthenticated API requests.

Key Changes:

  • Added public Speaker API endpoint with serializers and viewsets for listing/retrieving speaker profiles
  • Enhanced authentication middleware to support unauthenticated requests by resolving events from URL parameters
  • Expanded URL validation to support local IP addresses and updated CORS settings for development

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
app/eventyay/api/views/speaker.py New viewset for speaker API with public access permissions and pagination
app/eventyay/api/serializers/speaker.py Serializer for speaker profiles including submissions data
app/eventyay/api/urls.py Registered speaker endpoint and adjusted router trailing slash configuration
app/eventyay/api/auth/api_auth.py Enhanced authentication to resolve events for unauthenticated requests
app/eventyay/config/settings.py Added 127.0.0.1 to CORS whitelist and configured additional CORS headers
app/eventyay/config/urls.py Enabled API URL pattern
app/eventyay/features/live/consumers.py Added safety check for component initialization
app/eventyay/webapp/src/lib/validators.js Expanded URL validator to support local IP addresses

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

const localurl = helpers.regex(/^https?:\/\/(localhost|127\.0\.0\.1|0\.0\.0\.0)(:[0-9]+)?(\/.*)?$/)
export function url(message) {
return helpers.withMessage(message, (value) => (!helpers.req(value) || _url(value) || relative(value) || (ENV_DEVELOPMENT && devurl(value))))
return helpers.withMessage(message, (value) => (!helpers.req(value) || _url(value) || relative(value) || localurl(value)))
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The localurl validation now applies in production environments (removed ENV_DEVELOPMENT check). This allows users to enter local URLs like http://localhost or http://127.0.0.1 in production, which is a security concern. Restore the environment check: (ENV_DEVELOPMENT && localurl(value))

Suggested change
return helpers.withMessage(message, (value) => (!helpers.req(value) || _url(value) || relative(value) || localurl(value)))
return helpers.withMessage(message, (value) => (!helpers.req(value) || _url(value) || relative(value) || (ENV_DEVELOPMENT && localurl(value))))

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is has been mentioned in #1122 and will be resolved soon with the addition of HRM functionality in the video webapp

Comment on lines 26 to +28
path("events/<str:event_id>/", include(event_router.urls)),
path("events/<str:event_id>", EventView.as_view(), name="root"),
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

This path pattern conflicts with line 27's include(event_router.urls) which already handles paths starting with events/<str:event_id>/. The pattern without a trailing slash will never match because Django's URL resolver will match the more specific pattern first. Either remove this duplicate or ensure proper ordering and trailing slash handling.

Suggested change
path("events/<str:event_id>/", include(event_router.urls)),
path("events/<str:event_id>", EventView.as_view(), name="root"),
path("events/<str:event_id>", EventView.as_view(), name="root"),
path("events/<str:event_id>/", include(event_router.urls)),

Copilot uses AI. Check for mistakes.
Comment on lines +25 to +28
try:
request.event = Event.objects.get(id=int(event_id))
except (ValueError, TypeError, Event.DoesNotExist):
try:
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

This implements a try-except pattern that always attempts integer conversion first, causing an exception on every slug-based lookup. Consider checking if event_id is numeric before attempting integer conversion: if event_id.isdigit(): request.event = Event.objects.get(id=int(event_id)) followed by the slug lookup.

Suggested change
try:
request.event = Event.objects.get(id=int(event_id))
except (ValueError, TypeError, Event.DoesNotExist):
try:
if event_id.isdigit():
try:
request.event = Event.objects.get(id=int(event_id))
except Event.DoesNotExist:
request.event = None
else:
try:

Copilot uses AI. Check for mistakes.
"""Pagination class that supports a 'limit' query parameter."""
page_size = 50
page_size_query_param = 'limit'
max_page_size = 10000
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The maximum page size of 10000 is extremely high and could cause performance issues or memory problems when retrieving large datasets. Consider reducing this to a more reasonable limit (e.g., 100-500) to prevent potential DoS through excessive pagination requests.

Suggested change
max_page_size = 10000
max_page_size = 500

Copilot uses AI. Check for mistakes.
"""
serializer_class = SpeakerSerializer
queryset = SpeakerProfile.objects.none()
lookup_field = "user__code__iexact"
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The lookup_field uses __iexact lookup which is not a valid field name but a query expression. This will cause Django to look for a field literally named 'user__code__iexact' rather than performing case-insensitive matching. Override get_object() method instead to implement case-insensitive lookup: def get_object(self): return self.get_queryset().get(user__code__iexact=self.kwargs[self.lookup_url_kwarg])

Copilot uses AI. Check for mistakes.
Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Please address AI comments or mark them as not relevant.

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts.

@suhailnadaf509 suhailnadaf509 marked this pull request as draft November 11, 2025 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants