Add AuthenticationForm.confirm_login_allowed to allow customizing the login policy

[Ethan Jucovy]

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

[Ethan Jucovy]

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

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

[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.

[Russell Keith-Magee]

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.

[Ethan Jucovy]

Control login policy with an overridable method on the form

[Ethan Jucovy]

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

[Brandon M Height]

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.

[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!

[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!

[Ethan Jucovy]

fix NameError

[Brandon M Height]

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.

[Ethan Jucovy]

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

[Ethan Jucovy]

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

[Łukasz Rekucki]

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

[Ethan Jucovy]

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

[Ethan Jucovy]

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.

[Ethan Jucovy]

updated patch for code-drift

[Ethan Jucovy]

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?

[Tim Graham]

This looks good to me. I've updated the patch and made some minor edits ​PR.

[Tim Graham <timograham@…>]

In a1889397a9f0e6a35189de455098b4c70923e561:

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

Thanks ejucovy and lasko for work on the patch.