Update API export to allow input session id #178
Conversation
Reviewer's Guide by SourceryThis pull request updates the API export functionality to allow input session IDs. The changes are implemented by modifying the schedule view to handle session IDs passed as query parameters. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider the security implications of allowing unauthenticated access to exports when the 'talks' parameter is present. Ensure this is intentional and properly secured.
- Add tests to cover the new functionality and changed behavior, especially around the handling of the 'talks' parameter and its impact on authentication.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if request.GET.get('talks'): | ||
| exporter.talk_ids = request.GET.get('talks').split(',') | ||
| else: | ||
| return HttpResponseRedirect(self.request.event.urls.login) |
There was a problem hiding this comment.
🚨 issue (security): Security and logic flow concerns in authentication bypass
The new condition allows unauthenticated access to functionality previously requiring login. This could expose sensitive information or operations. Consider maintaining authentication and implementing a secure method for sharing specific talks. Additionally, this change increases the complexity of the logic flow, potentially making the code harder to maintain.
| @@ -114,7 +114,10 @@ def get(self, request, *args, **kwargs): | |||
|
|
|||
| exporter.schedule = self.schedule | |||
| if "-my" in exporter.identifier and self.request.user.id is None: | |||
| return HttpResponseRedirect(self.request.event.urls.login) | |||
| if request.GET.get('talks'): | |||
There was a problem hiding this comment.
Here is the original code:
if "-my" in exporter.identifier and self.request.user.id is None:
return HttpResponseRedirect(self.request.event.urls.login)
favs_talks = SubmissionFavourite.objects.filter(user=self.request.user.id)The author wanted to prevent SubmissionFavourite.objects.filter() from receiving None, so there is the check self.request.user.id is None, to force user logging-in.
But you insert your code in this if flow control and nullify the guard. After your code, return HttpResponseRedirect doesn't run and None strips to SubmissionFavourite.objects.filter.
There was a problem hiding this comment.
Hi @hongquan, this view is using for both talk and video component:
- From talk front end API call, 'talks' won't be put as request param, so if User not login, they will be forced to login page.
- From video API call, 'talks' is input as request params and from video, we not expecting User will be redirect to Talk component to login.
Idea for this changes is reuse the current API for Video schedule export, not impact flow at talk component
There was a problem hiding this comment.
@lcduong Could you add your explanation as comment above that line?
This PR closes/references issue fossasia/eventyay-video#190)
How has this been tested?
Checklist
Summary by Sourcery
Enhance the API export functionality to accept session IDs through query parameters, improving flexibility in session data retrieval.
Enhancements: