Improve access logic and URL structure #386
Conversation
Reviewer's Guide by SourceryThis pull request improves access logic and URL structure for the video system integration in the eventyay platform. It adds functionality to create and manage video platforms for events, enhances the event creation and update forms, and implements new API endpoints for video access authentication. Sequence diagram for video access authenticationsequenceDiagram
actor User
participant EventPage
participant VideoAccessAuthenticator
participant VideoSystem
User->>EventPage: Click 'Enable video access'
EventPage->>VideoAccessAuthenticator: GET /event/{organizer}/{event}/
VideoAccessAuthenticator->>VideoAccessAuthenticator: Check video configuration
alt Configuration not fulfilled
VideoAccessAuthenticator-->>User: PermissionDenied
else Configuration fulfilled
VideoAccessAuthenticator->>VideoAccessAuthenticator: Check user permissions
alt User has permission
VideoAccessAuthenticator->>VideoSystem: Redirect with token
VideoSystem-->>User: Video access granted
else User lacks permission
VideoAccessAuthenticator-->>User: PermissionDenied
end
end
Architecture diagram for video system integrationgraph TD;
subgraph EventyayPlatform
EventService
VideoAccessAuthenticator
CreateWorldTask
end
subgraph VideoSystem
VideoServer
end
EventService --> VideoAccessAuthenticator
VideoAccessAuthenticator -->|Token| VideoServer
CreateWorldTask -->|API Call| VideoServer
Updated class diagram for Event modelclassDiagram
class Event {
+Boolean is_video_create
+BooleanField is_video_create
}
class EventWizardFoundationForm {
+BooleanField is_video_create
}
class EventUpdateForm {
+BooleanField is_video_create
}
EventWizardFoundationForm --> Event
EventUpdateForm --> Event
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider making the
create_worldtask more resilient to failures of the external video service, possibly by implementing a retry mechanism or making it fully asynchronous. - The
encode_emailfunction inutils.pyuses a combination of hashing and random characters. While this provides some obfuscation, it may not be necessary if the email is not sensitive information. Consider simplifying this if possible.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 3 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return self.form_invalid(form) | ||
|
|
||
| @staticmethod | ||
| def reset_timezone(tz, dt): | ||
| return tz.localize(dt.replace(tzinfo=None)) if dt is not None else None | ||
|
|
||
|
|
||
| class VideoAccessAuthenticator(views.APIView): |
There was a problem hiding this comment.
🚨 suggestion (security): Improve JWT implementation for video access authentication
Consider using a shorter expiration time for the JWT (e.g., 1 hour instead of 30 days) and use a dedicated secret key for signing instead of the event settings. This will enhance security by reducing the window of opportunity for token misuse and separating concerns.
| @@ -103,6 +103,48 @@ def send_event_webhook(self, user_id, event, action): | |||
| except self.MaxRetriesExceededError: | |||
| logger.error("Max retries exceeded for sending organizer webhook.") | |||
|
|
|||
| @shared_task(bind=True, max_retries=5, default_retry_delay=60) # Retries up to 5 times with a 60-second delay | |||
| def create_world(self, is_video_create, data): | |||
There was a problem hiding this comment.
suggestion (bug_risk): Enhance error handling and consider rate limiting for external API calls
Implement more robust error handling for the HTTP request to the external service. Consider adding rate limiting to prevent potential abuse of the video creation functionality.
def create_world(self, is_video_create, data):
try:
with RateLimiter(max_calls=5, period=60): # Rate limit: 5 calls per minute
response = self.make_api_call(is_video_create, data)
response.raise_for_status()
except requests.exceptions.RequestException as e:
logger.error(f"Error creating video system: {str(e)}")
raise self.retry(exc=e, countdown=60)
| return final_result.upper() | ||
|
|
||
|
|
||
| def check_create_permission(request): |
There was a problem hiding this comment.
🚨 suggestion (security): Refine permission check for video creation
Consider implementing a more specific permission check for video creation, rather than relying solely on the 'can_create_events' permission. This will provide finer-grained control over who can create video platforms.
def check_create_video_permission(request):
"""
Check if the user has permission to create videos.
"""
return request.user.has_perm('eventyay_common.create_video')
| return token | ||
|
|
||
|
|
||
| def encode_email(email): |
There was a problem hiding this comment.
🚨 question (security): Clarify purpose and security implications of email encoding
The purpose and security benefits of the email encoding function are not clear. If this is intended for privacy or security purposes, consider using a more robust method or explain the rationale behind this implementation.
This PR related to issue fossasia/eventyay-video#240 Improve access logic and URL structure
It implement point 2 + 3 of the issue (point 1 is implemented by update nginx)
New button in event page of common, will redirect to video system with admin permission
Summary by Sourcery
Enhance the event management system by adding support for creating video platforms for events. Introduce a new button on the event page to enable video access, and implement a VideoAccessAuthenticator to manage video access permissions and redirection. Refactor event creation and update workflows to integrate video platform creation, and improve URL structure and access logic with new utility functions.
New Features:
Enhancements:
Build: