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.

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

RATELIMIT_VIEW Not Functioning as Expected with Class-Based Views #315

Open
Aryancsz opened this issue Mar 12, 2024 · 15 comments
Open

RATELIMIT_VIEW Not Functioning as Expected with Class-Based Views #315

Aryancsz opened this issue Mar 12, 2024 · 15 comments

Comments

@Aryancsz
Copy link

Aryancsz commented Mar 12, 2024

Issue Description

When applying the @method_decorator(ratelimit(key="user", rate="2/h", block=True), name="change_password") decorator to a method within a class-based view, the custom error message defined in RATELIMIT_VIEW is not being thrown. However, when the same decorator is applied to a function-based view, it works as expected.

Steps to Reproduce

  1. Define a class-based view with a method decorated with @method_decorator(ratelimit(key="user", rate="2/h", block=True), name="change_password").
  2. Ensure RATELIMIT_VIEW is properly defined with a custom error message.
  3. Attempt to trigger the rate limit by invoking the method.

Expected Behavior

The custom error message defined in RATELIMIT_VIEW should be thrown when the rate limit is exceeded, regardless of whether the view is class-based or function-based.

Actual Behavior

The custom error message is not thrown ( does throw permission denied ) when the rate limit is exceeded in a class-based view, while it works correctly in a function-based view.

Additional Information

  • Package version: 4.1.0

Workarounds Tried

  • Applying the decorator directly to URLs or the entire class view and it worked but resulted in loss of flexibility for different methods.

Note: This issue affects the flexibility and consistency of rate limiting in class-based views compared to function-based views.

@benjaoming
Copy link
Contributor

Having several @method_decorator(ratelimit(...)) on the dispatch() method of CBVs and not having issues. I have written test cases to provoke the rate limiting that look like this:

def assert_fail_on_repeat(client, url, no_times=20):
    for __ in range(no_times):
        response = client.post(url)
        assert response.status_code == 200
    response = client.post(url)
    assert response.status_code == 429

@Aryancsz
Copy link
Author

@benjaoming Yeah, but couldn't resolve the issue. As I mentioned rate-limiting indeed triggers but the custom error message doesn't.

@benjaoming
Copy link
Contributor

benjaoming commented Mar 13, 2024

@Aryancsz I think that's because the error is handled by middleware, which is bypassed while using Django's RequestFactory: https://github.com/jsocol/django-ratelimit/blob/main/django_ratelimit/middleware.py

So you would have to make additional calls to process the request+response and trigger the error page through the middleware.

@Aryancsz
Copy link
Author

@benjaoming is there any doc link or steps to make this work ?

@benjaoming
Copy link
Contributor

benjaoming commented Mar 13, 2024

@Aryancsz I couldn't find something quick on Google. Here's a rough+untested draft for inspiration. Can you tell me if it works for you?

from django_ratelimit.middleware import RateLimitMiddleware


# Using pytest - client=Django test client, rf=django's RequestFactory
def test_fail_on_repeat(client, rf):
    url = "/path/with/ratelimit"
    no_times = 20
    get_response = lambda request: client.post(url)
    middleware_get_response = RateLimitMiddleware(get_response)

    # Saturate the limit, make sure we can call the allowed number of times
    for __ in range(no_times):
        # Not sure that this Request object is used much, you might be able to find
        # a path to trim it out
        request = rf.post(url)

        # Call the instantiated middleware
        response = middleware_get_response(request)

        assert response.status_code == 200

    # Now call it again and check for an error
    response = middleware_get_response(request)
    assert response.status_code == 429

@benjaoming
Copy link
Contributor

@Aryancsz did you get it to work?

@Aryancsz
Copy link
Author

@Aryancsz did you get it to work?

Nope, not yet. Occupied in other works. Can you check for possible fixes ?

@benjaoming
Copy link
Contributor

@Aryancsz I posted an explanation and a recipe that I was wondering if you would try out?

I don't think there's anything wrong in django-ratelimit, I think that it's because Django middleware is disabled in the test client.

@Aryancsz
Copy link
Author

@benjaoming I have used "django_ratelimit.middleware.RatelimitMiddleware" middleware. and followed instructions on documentation for CBVs Here Is there anything else I am missing here ?

@benjaoming
Copy link
Contributor

@Aryancsz I'm suggesting if you can develop a test case that shows the error. In order to do that, you need to call the middleware manually because middleware isn't triggered in the test client.

You should also supply a code example of the view class that isn't working, so it's possible to verify how you've applied the decorator.

@Aryancsz
Copy link
Author

Aryancsz commented Mar 26, 2024

@benjaoming Here is an example:
settings.py

MIDDLEWARE = [
     ....
    "django_ratelimit.middleware.RatelimitMiddleware",
     ...
]

views.py

class ChangeUsernameView(APIView):
    http_method_names = ["post"]
    permission_classes = [IsAuthenticated]
    
    @method_decorator(ratelimit(key="user", rate="2/h", block=True), name="change_username")
    def post(self, request):
        user = request.user
        new_username = request.data.get("new_username")
        ....
        return Response({"status": "success"}, status=status.HTTP_200_OK)

Error : The user did not have permission to do that

@benjaoming
Copy link
Contributor

benjaoming commented Mar 26, 2024

@Aryancsz

It looks good to me, I think we need a test case to verify what's wrong.

What happens if you move it to dispatch ?

class ChangeUsernameView(APIView):
    http_method_names = ["post"]
    permission_classes = [IsAuthenticated]
    
    @method_decorator(ratelimit(key="user", method="POST", rate="2/h", block=True), name="change_username")
    def dispatch(self, *args, **kwargs):
        return super().dispatch(*args, **kwargs)


    def post(self, request):
        user = request.user
        new_username = request.data.get("new_username")
        ....
        return Response({"status": "success"}, status=status.HTTP_200_OK)


@taylorsloan
Copy link

taylorsloan commented May 1, 2024

@Aryancsz

It looks good to me, I think we need a test case to verify what's wrong.

What happens if you move it to dispatch ?

class ChangeUsernameView(APIView):
    http_method_names = ["post"]
    permission_classes = [IsAuthenticated]
    
    @method_decorator(ratelimit(key="user", method="POST", rate="2/h", block=True), name="change_username")
    def dispatch(self, *args, **kwargs):
        super().dispatch(*args, **kwargs)


    def post(self, request):
        user = request.user
        new_username = request.data.get("new_username")
        ....
        return Response({"status": "success"}, status=status.HTTP_200_OK)

I just tried this and it worked for me. Thank you! I just had to add a return statement to super().dispatch(*args, **kwargs)

@benjaoming
Copy link
Contributor

Ah yes, thanks @taylorsloan 🙏 I updated the example

@antipr000
Copy link

Thanks @benjaoming, I have been trying this for 4-5 hours and it was not working, used custom middlewares and what not! Finally this worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants