Opened 15 years ago
Closed 11 years ago
#12103 closed New feature (fixed)
Add AuthenticationForm.confirm_login_allowed to allow customizing the login policy
Reported by: | Ethan Jucovy | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | ethan.jucovy@…, bmheight@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I've built a site that uses django-registration to manage user account creation. I allow inactive users to log in, with minimal functionality: I use middleware to kick them to a page that reminds them to confirm their account.
To do this, I copied the whole AuthenticationForm.clean()
method from django.contrib.auth.forms
to my custom authentication_form, and removed two lines:
elif not self.user_cache.is_active: raise forms.ValidationError(_("This account is inactive."))
From my code's perspective, it would be nicer if the base AuthenticationForm
had a class-level setting to toggle whether this check happens. Then all I would have to do is subclass the form with an override, like
class MyAuthenticationForm(AuthenticationForm): allow_inactive_logins = True
Attachments (10)
Change History (25)
by , 15 years ago
Attachment: | 12103.diff added |
---|
comment:1 by , 15 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
by , 15 years ago
Attachment: | 12103_with_documentation.diff added |
---|
comment:2 by , 15 years ago
I added a documentation patch. It's more verbose than I'd like.
While doing this I noticed that the AuthenticationForm
's behavior re: inactive users is undocumented, and the documentation for User.is_active
is misleading. I filed #12113 about that.
comment:3 by , 15 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
I don't see any problem with the idea of configuring the ability to allow or reject logins on the basis of some check other than 'is_active', but it should be in the form of a method that can be overridden on a subclass.
by , 15 years ago
Attachment: | 12103_method_on_form.diff added |
---|
Control login policy with an overridable method on the form
comment:4 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | 12103_method_on_form.2.diff added |
---|
Minor update to proposed documentation (pass
is clearer than return None
)
follow-up: 6 comment:5 by , 14 years ago
Needs documentation: | unset |
---|
I don't see anything wrong with this implementation and would be +1 to implementing it.
Would allow me to get rid of my current kluge in making this functionality work and have a standard django supported interface.
This however still needs tests added to it. If ejucovy doesn't wish to do this part I can probably put something together in that realm.
follow-up: 7 comment:6 by , 14 years ago
Replying to lasko:
I don't see anything wrong with this implementation and would be +1 to implementing it.
Would allow me to get rid of my current kluge in making this functionality work and have a standard django supported interface.
This however still needs tests added to it. If ejucovy doesn't wish to do this part I can probably put something together in that realm.
Thanks, I'd appreciate that!
comment:7 by , 14 years ago
No problem ejucovy, I do however have one concern here with regards to your patch.
Line 83:
self.confirm_login_allowed(user_cache)
This is a pretty obvious NameError waiting to happen as user_cache is never defined and should be self.user_cache
Once this is fixed the patch looks decent. I'll start working on the tests
Replying to ejucovy:
Replying to lasko:
I don't see anything wrong with this implementation and would be +1 to implementing it.
Would allow me to get rid of my current kluge in making this functionality work and have a standard django supported interface.
This however still needs tests added to it. If ejucovy doesn't wish to do this part I can probably put something together in that realm.
Thanks, I'd appreciate that!
by , 14 years ago
Attachment: | 12103_tests.diff added |
---|
Tests associated to the patch. Unfortunately I did this with doctests. I intend to write this into a unittest but this should suffice for now.
comment:8 by , 14 years ago
Cc: | added |
---|---|
Needs tests: | unset |
by , 14 years ago
Attachment: | 12103_with_tests.diff added |
---|
regenerated patch against trunk@15402; including lasko's tests
comment:9 by , 14 years ago
Patch needs improvement: | unset |
---|
I've added a new version of the patch 12103_with_tests.diff -- which includes lasko's tests.
comment:10 by , 14 years ago
Patch needs improvement: | set |
---|
Did you actually run the tests? By the look of the patch they aren't even syntacticly correct.
by , 14 years ago
Attachment: | 12103_with_working_tests.diff added |
---|
comment:11 by , 14 years ago
Patch needs improvement: | unset |
---|---|
Severity: | → Normal |
Type: | → Uncategorized |
Oops. Let's try that again. The auth
tests pass for me with this patch: http://code.djangoproject.com/attachment/ticket/12103/12103_with_working_tests.diff
by , 14 years ago
Attachment: | 12103_with_better_working_tests.diff added |
---|
comment:12 by , 14 years ago
Easy pickings: | unset |
---|---|
Type: | Uncategorized → New feature |
Version: | 1.1 → SVN |
I realized the tests in my patch could be improved; here's a new version of the patch:
http://code.djangoproject.com/attachment/ticket/12103/12103_with_better_working_tests.diff
The tests now exercise/document custom ValidationError
logic in an AuthenticationForm
subclass, in addition to logic that disables the default user.is_inactive
check.
comment:13 by , 13 years ago
UI/UX: | unset |
---|
I updated the patch to make sure it applies cleanly (and tests pass) on the current trunk@HEAD (https://code.djangoproject.com/attachment/ticket/12103/12103_at_r17205.diff) -- any chance this can be merged in before the 1.4 release?
comment:14 by , 11 years ago
Summary: | Setting in contrib.auth AuthenticationForm to let inactive users log in → Add AuthenticationForm.confirm_login_allowed to allow customizing the login policy |
---|
This looks good to me. I've updated the patch and made some minor edits PR.
comment:15 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Here's a simple patch to show what I'm imagining.
I'll also try to figure out how/where best to document this.