Improve access logic and URL structure #386
Conversation
Reviewer's Guide by SourceryThis PR enhances the event management system by implementing video access functionality. The changes introduce a new VideoAccessAuthenticator view for handling video access permissions and add a video access button to the event page. The implementation includes token generation for secure access and proper permission checks. Sequence diagram for video access authenticationsequenceDiagram
actor User
participant EventPage
participant VideoAccessAuthenticator
participant VideoSystem
User->>EventPage: Click "Enable video access"
EventPage->>VideoAccessAuthenticator: Request video access
VideoAccessAuthenticator->>VideoAccessAuthenticator: Check video configuration
alt Configuration incomplete or plugin disabled
VideoAccessAuthenticator-->>User: Permission Denied
else Configuration complete
VideoAccessAuthenticator->>VideoAccessAuthenticator: Check user permissions
alt User lacks permissions
VideoAccessAuthenticator-->>User: Permission Denied
else User has permissions
VideoAccessAuthenticator->>VideoAccessAuthenticator: Generate token
VideoAccessAuthenticator->>VideoSystem: Redirect with token
end
end
Class diagram for VideoAccessAuthenticatorclassDiagram
class VideoAccessAuthenticator {
+get(request, *args, **kwargs)
+generate_token_url(request)
}
VideoAccessAuthenticator --> APIView
class APIView {
<<abstract>>
}
File-Level Changes
Possibly linked issues
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.
src/pretix/eventyay_common/tasks.py
Outdated
| @@ -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)
src/pretix/eventyay_common/utils.py
Outdated
| 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')
src/pretix/eventyay_common/utils.py
Outdated
| 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.
src/pretix/eventyay_common/tasks.py
Outdated
| event_timezone = data.get("timezone") | ||
| locale = data.get("locale") | ||
| token = data.get("token") | ||
| has_permission = data.get("has_permission") |
There was a problem hiding this comment.
Please make token, has_permission explicit parameters of create_world function.
src/pretix/eventyay_common/tasks.py
Outdated
| headers=headers, | ||
| ) | ||
| except requests.exceptions.ConnectionError as e: | ||
| logger.error(f"Connection error: {e}") |
There was a problem hiding this comment.
Wrong use of logger. This has been remined many times.
src/pretix/eventyay_common/utils.py
Outdated
| @param request: user request | ||
| @return: jwt | ||
| """ | ||
| uid_token = encode_email(request.user.email) |
There was a problem hiding this comment.
I would like to create a unique identification for user authentication in the video system.
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The URL pattern
url('', event.VideoAccessAuthenticator.as_view())is too broad and may conflict with other routes. Consider using a more specific path likeurl('video-access/', ...) - The JWT token expiration of 30 days for admin access seems excessive. Consider reducing this to a shorter period (e.g., 24 hours) and implementing refresh tokens if longer access is needed
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 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.
| "uid": uid_token, | ||
| "traits": list( | ||
| { | ||
| "eventyay-video-event-{}-orgnanizer".format(request.event.slug), |
There was a problem hiding this comment.
issue (typo): Fix typo in trait name 'orgnanizer' -> 'organizer'
| """ | ||
| if plugin_list is None: | ||
| return [] | ||
| return plugin_list.split(",") |
There was a problem hiding this comment.
suggestion: Make plugin parsing more robust by handling empty strings
Consider handling empty strings and stripping whitespace: 'return [p.strip() for p in plugin_list.split(",") if p.strip()]'
| return plugin_list.split(",") | |
| return [p.strip() for p in plugin_list.split(",") if p.strip()] |
src/pretix/eventyay_common/urls.py
Outdated
| @@ -19,5 +19,6 @@ | |||
| url(r'^events/add$', event.EventCreateView.as_view(), name='events.add'), | |||
| url(r'^event/(?P<organizer>[^/]+)/(?P<event>[^/]+)/', include([ | |||
| url(r'^settings/$', event.EventUpdate.as_view(), name='event.update'), | |||
| url('', event.VideoAccessAuthenticator.as_view(), name='event.create_access_to_video'), | |||
There was a problem hiding this comment.
issue (bug_risk): Use a specific URL pattern for video access endpoint
Using an empty string as URL pattern will match all routes. Consider using a specific path like 'video-access/' to avoid routing conflicts.
lcduong
force-pushed
the
feature-240-v2
branch
from
b96879a
to
e38ce35
Compare
lcduong
force-pushed
the
feature-240-v2
branch
from
4397e88
to
e38ce35
Compare
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 page by adding a button for video system access with admin permissions and improve the access logic by implementing a VideoAccessAuthenticator class to manage permissions and token generation.
New Features:
Enhancements: