After this initial implementation I didn’t work on it further because I feel it’s a bit of an annoying solution as you need to add to INSTALLED_APPS (to register a signal receiver), MIDDLEWARE, and AUTHENTICATION_BACKENDS, which I find a bit unwieldy. I could add a system check to make sure if it’s in one, it’s in all three. But I feel it complicates the design somewhat.
Another potential issue is that this adds two database models, one to track failed logins and another for locked out users. I’m not sure how people would feel about that. I do think this is better than using the cache as admins can go and reset this for users who lock themselves out (which happens frequently at work).
I would also need to add something to the password reset to make it more resistant to DoS against a single user account. Doing this in my repo seems… tricky as I don’t think there are any signals for password reset. It would also be better done in the confirm view rather than the complete view, as you don’t need to change your password to unlock an account for your device, only verify you can access the email address.
Then I looked at Claude’s PR, and I kind of like the approach there of doing everything in AuthenticationForm. Everything is in one place, and it has the advantage of being able to use the form errors (until then I was considering redirecting the user to a new “locked out” page). Unfortunately, this doesn’t work very well for this method as I need access to the response to set cookies. I suppose I could also override the default LoginView. I also think it would make the code a bit more annoying if we wanted people to opt into this functionality as we’d need a bunch of conditionals, and if you use the view with a different form or the form with a different view things could break. And indeed if they chose not to use the password reset views, you could still end up in a situation where a DoS attack is possible against single accounts.
So my own summary: I do think it’s better to skip the third party package here and go straight to core, but wondering what people think about:
Overriding AuthenticationForm, LoginView, and PasswordResetConfirmView
Adding two new models
If someone also has another idea how to implement this, I’m all ears. I did think about having an optional contrib.auth.ratelimit sub-app or something that would turn everything on, but not sure how everything would work together.
I don’t think having to change three settings fields is too much. It’s typical to have to add to INSTALLED_APPS and MIDDLEWARES. The extra auth backend change is where things differ. But you’re changing auth, so it makes sense.
I personally wouldn’t mind the extra models, but I’d like to vet other ideas first.
I’m not sold that the reseting of a locked account process needs to be part of core. Making that the third party library opens the door to allowing the rare limiting to be done via the cache, avoiding the models and addition to INSTALLED_APPS. The third party app can manage supporting resetting a locked user account. Most cache providers support wildcard searches or they’ll need the reporter to share their username/email. To find the third party app, maybe we link to the grid view for auth ratelimiting in djangopackages in the docs?
Hey @tom — erm… first, thanks for working on this.
Then, I think it’s going to be needed to draw in the @steering_council here. We tried and tried and tried, over multiple years, to get a consensus on what would be an acceptable solution for core here, in order to proceed with a GSoC proposal. Each year we failed to come up with anything that was acceptable. (In the end I despaired, and adjusted the ticket to be a docs fix.)
Claude’s work that you mention was already one of the rejected proposals, so I don’t see what’s changed. (Admittedly I’d need to go back over the history to remember why each suggestion wasn’t OK.)
Without a decision that yes, we’ll do rate-limiting, and that we’ll use a particular approach, I think we’re stuck. (Personally, I’d like SOME solution here, even if not perfect.)
Documenting the ecosystem options seems all that’s left failing that.
I’m not sold that the reseting of a locked account process needs to be part of core.
If we have the models, resetting the attempts is just a matter of deleting some rows from these models in the admin. So you get both. But I do agree in general. If we didn’t use the models, but the cache, I think as long as the password reset flow resets the attempts, we don’t need to do anything but document this (and maybe make it clear in the flow)
I haven’t really looked into using the cache rather than the database. It would I think require storing things a bit differently since we can’t filter on dates and so. I have some vague ideas but would need to put pen to paper, and not too keen on doing that until there is some kind of consensus.
(apparently I didn’t reply “properly” the first time)
Hi Carlton,
Did you look through the OWASP link? The solution here is distinct from Claude’s PR - I mentioned it only about where the code would live, not the actual design.
Without a decision that yes, we’ll do rate-limiting, and that we’ll use a particular approach
That’s why I started this thread, to propose this new (to us) solution. I’m aware my original post was quite lengthy and probably brought up way too many disparate points so that might not have been clear
Given, though, how many times various solutions have been rejected, we should at least do due diligence on the various proposals, and show how this new proposal addresses the concerns.
Frankly I’d be amazed if a totally new solution nothing like any of the past proposals had come up, and so we still need the @steering_council to give the approval for whatever is settled on.
Personally I’d much rather see efforts being invested towards a core generic throttling solution with pluggable backends (à la DRF) where the contrib.auth login use case is implemented to validate the design than a login tailored rate limiting solution.
If core shipped with a django.middleware.throttling.ThrottlingViewMiddleware and configuration view decorators like what we do with the CSRF tooling solving this problem for the login case, with or without the usage of device cookies, would likely be way less invasive to implement as a third party application.
