Opened 7 years ago

Closed 3 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: master
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)

12103.diff (1.1 KB) - added by Ethan Jucovy 7 years ago.
12103_with_documentation.diff (1.8 KB) - added by Ethan Jucovy 7 years ago.
12103_method_on_form.diff (3.0 KB) - added by Ethan Jucovy 7 years ago.
Control login policy with an overridable method on the form
12103_method_on_form.2.diff (3.0 KB) - added by Ethan Jucovy 7 years ago.
Minor update to proposed documentation (pass is clearer than return None)
12103_method_on_form_corrected.diff (3.0 KB) - added by Ethan Jucovy 7 years ago.
fix NameError
12103_tests.diff (779 bytes) - added by Brandon M Height 7 years ago.
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.
12103_with_tests.diff (3.8 KB) - added by Ethan Jucovy 6 years ago.
regenerated patch against trunk@15402; including lasko's tests
12103_with_working_tests.diff (3.8 KB) - added by Ethan Jucovy 6 years ago.
12103_with_better_working_tests.diff (4.9 KB) - added by Ethan Jucovy 6 years ago.
12103_at_r17205.diff (5.0 KB) - added by Ethan Jucovy 5 years ago.
updated patch for code-drift

Download all attachments as: .zip

Change History (25)

Changed 7 years ago by Ethan Jucovy

Attachment: 12103.diff added

comment:1 Changed 7 years ago by Ethan Jucovy

Has patch: set
Needs documentation: set

Here's a simple patch to show what I'm imagining.

I'll also try to figure out how/where best to document this.

Changed 7 years ago by Ethan Jucovy

comment:2 Changed 7 years ago by Ethan Jucovy

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 Changed 7 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

Changed 7 years ago by Ethan Jucovy

Attachment: 12103_method_on_form.diff added

Control login policy with an overridable method on the form

comment:4 Changed 7 years ago by Ethan Jucovy

Cc: ethan.jucovy@… added

Changed 7 years ago by Ethan Jucovy

Attachment: 12103_method_on_form.2.diff added

Minor update to proposed documentation (pass is clearer than return None)

comment:5 Changed 7 years ago by Brandon M Height

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.

comment:6 in reply to:  5 ; Changed 7 years ago by Ethan Jucovy

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 in reply to:  6 Changed 7 years ago by Brandon M Height

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!

Changed 7 years ago by Ethan Jucovy

fix NameError

Changed 7 years ago by Brandon M Height

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 Changed 7 years ago by Brandon M Height

Cc: bmheight@… added
Needs tests: unset

Changed 6 years ago by Ethan Jucovy

Attachment: 12103_with_tests.diff added

regenerated patch against trunk@15402; including lasko's tests

comment:9 Changed 6 years ago by Ethan Jucovy

Patch needs improvement: unset

I've added a new version of the patch 12103_with_tests.diff -- which includes lasko's tests.

comment:10 Changed 6 years ago by Łukasz Rekucki

Patch needs improvement: set

Did you actually run the tests? By the look of the patch they aren't even syntacticly correct.

Changed 6 years ago by Ethan Jucovy

comment:11 Changed 6 years ago by Ethan Jucovy

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

Changed 6 years ago by Ethan Jucovy

comment:12 Changed 6 years ago by Ethan Jucovy

Easy pickings: unset
Type: UncategorizedNew feature
Version: 1.1SVN

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.

Changed 5 years ago by Ethan Jucovy

Attachment: 12103_at_r17205.diff added

updated patch for code-drift

comment:13 Changed 5 years ago by Ethan Jucovy

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 Changed 3 years ago by Tim Graham

Summary: Setting in contrib.auth AuthenticationForm to let inactive users log inAdd 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 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In a1889397a9f0e6a35189de455098b4c70923e561:

Fixed #12103 -- Added AuthenticationForm.confirm_login_allowed to allow customizing the logic policy.

Thanks ejucovy and lasko for work on the patch.

Note: See TracTickets for help on using tickets.
Back to Top