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.

Sign up for GitHub

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

Jump to bottom

Added api_view and throttle_class decorators to all views to throttle… #1012

Merged
merged 12 commits into from
Sep 3, 2023
Merged

Added api_view and throttle_class decorators to all views to throttle… #1012

merged 12 commits into from
Sep 3, 2023

Conversation

kennzi
Copy link
Collaborator

@kennzi kennzi commented Aug 2, 2023

… back-end api calls

@kennzi
Copy link
Collaborator Author

kennzi commented Aug 2, 2023

Closes #1009

@marlonkeating marlonkeating linked an issue Aug 2, 2023 that may be closed by this pull request
Throttle API calls to mitigate bot attacks #1009
Closed
@marlonkeating marlonkeating temporarily deployed to democracy-lab-staging August 2, 2023 23:35 Inactive
marlonkeating
marlonkeating requested changes Aug 3, 2023
View reviewed changes
civictechprojects/views.py Outdated
@@ -79,6 +79,8 @@ def group_tags_counts(request):

# TODO: Pass csrf token in ajax call so we can check for it
@csrf_exempt
@api_view()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the @api_view() lines? If so, then POST methods like group_create need to be called with 'POST', because otherwise it defaults to just allowing GET.

Suggested change
@api_view()
@api_view(['POST'])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I could find online, the @throttle_classes() decorator is meant to be used in conjunction with the @api_view() decorator and may not function correctly without it. I reviewed each of the views and updated the methods in the api_view() decorators, hopefully appropriately!

@marlonkeating marlonkeating temporarily deployed to democracy-lab-prod-mirror August 10, 2023 01:43 Inactive
marlonkeating
marlonkeating requested changes Aug 10, 2023
View reviewed changes
civictechprojects/views.py Outdated
@@ -139,6 +147,8 @@ def get_group(request, group_id):
return HttpResponse(status=404)


@api_view('GET', 'POST')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@api_view('GET', 'POST')
@api_view(['GET', 'POST'])

Page is failing to load with error message:

File "/app/civictechprojects/views.py", line 150, in <module>
@api_view('GET', 'POST')
TypeError: api_view() takes from 0 to 1 positional arguments but 2 were given

@marlonkeating marlonkeating temporarily deployed to democracy-lab-prod-mirror August 13, 2023 17:01 Inactive
marlonkeating
marlonkeating requested changes Aug 13, 2023
View reviewed changes
civictechprojects/views.py Outdated
@@ -110,6 +114,8 @@ def group_edit(request, group_id):

# TODO: Pass csrf token in ajax call so we can check for it
@csrf_exempt
@api_view(['GET', 'DELETE'])
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually use POST here, when we should have used DELETE, same with the other deletion operations.

Suggested change
@api_view(['GET', 'DELETE'])
@api_view(['GET', 'POST'])

@marlonkeating marlonkeating temporarily deployed to democracy-lab-prod-mirror August 16, 2023 23:51 Inactive
@marlonkeating marlonkeating temporarily deployed to democracy-lab-prod-mirror August 17, 2023 03:27 Inactive
marlonkeating
marlonkeating requested changes Aug 18, 2023
View reviewed changes
civictechprojects/views.py Outdated
@@ -644,6 +704,8 @@ def presign_project_thumbnail_upload(request):
raw_key=s3_key, file_name=file_name, file_type=file_type, acl="public-read")

# TODO: Replace with is_co_owner_or_owner
@api_view()
@throttle_classes([AnonRateThrottle, UserRateThrottle])
def volunteer_operation_is_authorized(request, volunteer_relation):
Copy link
Contributor

Choose a reason for hiding this comment

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

Throttling not needed here, this is a helper method (that should live in a file with other helper methods).

civictechprojects/views.py Outdated
Comment on lines 117 to 119
@api_view(['GET', 'POST'])
@throttle_classes([AnonRateThrottle, UserRateThrottle])
def group_delete(request, group_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

This operation is failing, and I'm not sure why. It's definitely being sent with POST.

civictechprojects/views.py Outdated
Comment on lines 403 to 405
@api_view(['GET', 'POST'])
@throttle_classes([AnonRateThrottle, UserRateThrottle])
def project_delete(request, project_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

This operation also failing for unknown reasons.

@0xE111
Copy link
Collaborator

0xE111 commented Aug 23, 2023

@kennzi I think adding @api_view and setting DEFAULT_THROTTLE_CLASSES is a great solution. One thing to notice is that you don't need @throttle_classes([AnonRateThrottle, UserRateThrottle]) because those throttle classes will be applied to any ApiView anyway, so I removed the decorator.

I added tests to some django views to ensure that throttling is working as expected.

However, now that every view becomes ApiView (after adding an @api_view decorator), internal CSRF verification is done by SessionAuthentication, and @csrf_exempt does not work anymore. We could fix this by either patching SessionAuthentication or by fixing csrf. Since disabling csrf is not good for security anyway, I decided to patch js code to send csrf token. I ended up updating apiHelper and ProjectAPIUtils to send X-CSRFToken on POST requests, and it seems to work.

Also important - def qiqo_webhook(request): seems to be a webhook endpoint which 1) has no reason to be throttled, and 2) must use @csrf_exempt, otherwise POST request from 3rd-party service will be rejected. So I removed @api_view from it.

One thing left is modifying another js function to send CSRF token in headers - export function deleteFromS3(s3Key). It uses raw xhr and tbh I don't know javascript at all to make it read the cookie value and send it as request header. Needs to be fixed by someone :(

@marlonkeating marlonkeating temporarily deployed to democracy-lab-prod-mirror September 3, 2023 04:35 Inactive
marlonkeating
marlonkeating approved these changes Sep 3, 2023
View reviewed changes
Copy link
Contributor

@marlonkeating marlonkeating left a comment

Choose a reason for hiding this comment

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

Looks great! Props on fixing our long-standing CSRF issues 🥇

@marlonkeating marlonkeating merged commit a61094a into master Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marlonkeating marlonkeating marlonkeating approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Throttle API calls to mitigate bot attacks
3 participants
@kennzi @0xE111 @marlonkeating