Code

Opened 5 years ago

Closed 12 months ago

#12103 closed New feature (fixed)

Add AuthenticationForm.confirm_login_allowed to allow customizing the login policy

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

Download all attachments as: .zip

Change History (25)

Changed 5 years ago by ejucovy

comment:1 Changed 5 years ago by ejucovy

  • Has patch set
  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement unset

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 5 years ago by ejucovy

comment:2 Changed 5 years ago by ejucovy

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 4 years ago by russellm

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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.

Changed 4 years ago by ejucovy

Control login policy with an overridable method on the form

comment:4 Changed 4 years ago by ejucovy

  • Cc ethan.jucovy@… added

Changed 4 years ago by ejucovy

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

comment:5 follow-up: Changed 4 years ago by lasko

  • 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 ; follow-up: Changed 4 years ago by 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!

comment:7 in reply to: ↑ 6 Changed 4 years ago by lasko

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 4 years ago by ejucovy

fix NameError

Changed 4 years ago by lasko

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 4 years ago by lasko

  • Cc bmheight@… added
  • Needs tests unset

Changed 3 years ago by ejucovy

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

comment:9 Changed 3 years ago by ejucovy

  • 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 3 years ago by lrekucki

  • Patch needs improvement set

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

Changed 3 years ago by ejucovy

comment:11 Changed 3 years ago by ejucovy

  • Patch needs improvement unset
  • Severity set to Normal
  • Type set to 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 3 years ago by ejucovy

comment:12 Changed 3 years ago by ejucovy

  • Easy pickings unset
  • Type changed from Uncategorized to New feature
  • Version changed from 1.1 to 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.

Changed 3 years ago by ejucovy

updated patch for code-drift

comment:13 Changed 3 years ago by ejucovy

  • 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 12 months ago by timo

  • Summary changed from Setting in contrib.auth AuthenticationForm to let inactive users log in to 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 Changed 12 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In a1889397a9f0e6a35189de455098b4c70923e561:

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

Thanks ejucovy and lasko for work on the patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.