Opened 4 years ago

Last modified 4 days ago

#27807 assigned Bug

Overriding username validators doesn't work as documented

Reported by: johnrork Owned by: Shekhar Nath Gyanwali
Component: contrib.auth Version: 1.10
Severity: Normal Keywords: validation auth login forms username
Cc: Tobias Wiese Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The docs say you can change the default username validator by overriding the new username_validator attribute on a model that subclasses auth.User.

https://docs.djangoproject.com/en/1.10/ref/contrib/auth/#django.contrib.auth.models.User.username_validator

In practice, it seems that this value is always overwritten by the defaults, causing difficult-to-debug form validation errors.

class CustomUser(User):
    username_validator = validate_username
    
    class Meta:
        proxy = True 

>>> from myapp.models import CustomUser
>>> CustomUser.username_validator
<function validate_username at 0x105a5eea0>
>>> CustomUser._meta.get_field("username").validators
[<django.contrib.auth.validators.UnicodeUsernameValidator object at 0x105a5df28>, <django
.core.validators.MaxLengthValidator object at 0x105a622b0>]

I've attached a sample project in which you can replicate the issue:

  1. Log in at http://localhost:8000/admin with username admin and password password123
  2. Attempt to create a new Custom User with an apostrophe (single quote) in the username
  3. Get validation error despite both the form validator and CustomUser.username_validator allowing apostrophes
  4. Optionally uncomment the CustomUser.__init__ method to see expected behavior, even without a custom admin form

Attachments (1)

myproject.zip (14.7 KB) - added by johnrork 4 years ago.
Demonstration of possible Model.username_validator bug

Download all attachments as: .zip

Change History (26)

Changed 4 years ago by johnrork

Attachment: myproject.zip added

Demonstration of possible Model.username_validator bug

comment:1 Changed 4 years ago by Tim Graham

Severity: NormalRelease blocker
Summary: Overriding username validators does not seem to workOverriding username validators doesn't work as documented
Triage Stage: UnreviewedAccepted

In retrospect, it's a bit obvious that overriding won't work since the username field refers to username_validator directly. If it used AbstractUser.username_validator would that fix the issue?

class AbstractUser(model.Model):
    username_validator = UnicodeUsernameValidator() if six.PY3 else ASCIIUsernameValidator()

    username = models.CharField(...validators=[username_validator])

Marking as release blocker since it's a new feature in 1.10 (526575c64150e10dd8666d1ed3f86eedd00df2ed). Depending on how invasive a fix is required, it could be acceptable to remove the inaccurate documentation and revisit the ability to customize the validator in a sublcass in some future release.

comment:2 Changed 4 years ago by johnrork

Depending on how invasive a fix is required, it could be acceptable to remove the inaccurate documentation and revisit the ability to customize the validator in a sublcass in some future release.

The docs seem to indicate that subclassing is the only way to customize the username validator.

It would be nice if there was an easier way that worked better with existing codebases.

In my case, it would be enough if the existing validators accepted customizable arguments.

UnicodeUsernameValidator already accepts a regex, are settings.USERNAME_PATTERN and settings.USERNAME_LENGTH unreasonable pony requests?

An AUTH_USERNAME_VALIDATORS dict (à la AUTH_PASSWORD_VALIDATORS) would also work.

Last edited 4 years ago by johnrork (previous) (diff)

comment:3 Changed 4 years ago by johnrork

Here is an example a working AUTH_USERNAME_VALIDATORS setting:

https://github.com/johnrork/django/commit/8afbb76f3aca306ec7b678cb394fcedc8b2edf12

comment:4 Changed 4 years ago by Claude Paroz

I'm very sorry for this mess, as I'm the culprit here.

I tried to play with metaclasses and obtained that patch. However, other auth tests are horribly failing. I don't know if I'm simply doing something absolutely hacky and forbidden, or if it's just some isolation issue in tests.

comment:5 Changed 4 years ago by Tim Graham

Claude, after some brief debugging, the issue with your patch might be limited to the test suite where multiple user models are present. It looks like CustomValidatorUser initialization modifies the validators for the auth.User.username field and this change persists regardless of the user model in use. I'm not sure about the best way to proceed, offhand.

comment:6 Changed 4 years ago by Claude Paroz

Clearly I'm not confident to introduce such metaclass hacking in stable. So for 1.10 at least, I think the docs should be updated. For 1.11, I'm still undecided. Maybe the technical team might bring its expertise here.

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

In 714fdba:

[1.10.x] Refs #27807 -- Removed docs for User.username_validator.

The new override functionality claimed in refs #21379 doesn't work.

comment:8 Changed 4 years ago by Tim Graham

Severity: Release blockerNormal

I'm not against solving this, but considering it's recommended to start with a custom user model (#24370), I wouldn't invest a lot of effort into this functionality myself. We can forwardport that documentation change as necessary (at least the 1.10 releases notes portion must be ported).

comment:9 Changed 4 years ago by Claude Paroz

Yes, recommending a custom user model is a good thing. However, for those using the default contrib.auth with existing projects, and considering the migration to a custom user model is far from straightforward, it is still a bit annoying...

comment:10 Changed 16 months ago by Tobias Wiese

Cc: Tobias Wiese added

The Documentation since 1.11 until now (here and also referenced here) say that this feature exists since 1.10.

As it feature still doesn't work I just guess the removal form the docs somehow did not make it into master.

comment:11 Changed 16 months ago by Tim Graham <timograham@…>

In c84b91b7:

Refs #27807 -- Removed docs for User.username_validator.

The new override functionality claimed in refs #21379 doesn't work.
Forwardport of 714fdbaa7048c2321f6238d9421137c33d9af7cc from stable/1.10.x.

comment:12 Changed 16 months ago by Tim Graham <timograham@…>

In 53c83387:

[2.2.x] Refs #27807 -- Removed docs for User.username_validator.

The new override functionality claimed in refs #21379 doesn't work.
Forwardport of 714fdbaa7048c2321f6238d9421137c33d9af7cc from stable/1.10.x.

comment:13 Changed 16 months ago by Tim Graham <timograham@…>

In fb2b425:

[2.1.x] Refs #27807 -- Removed docs for User.username_validator.

The new override functionality claimed in refs #21379 doesn't work.
Forwardport of 714fdbaa7048c2321f6238d9421137c33d9af7cc from stable/1.10.x.

comment:14 Changed 16 months ago by Tim Graham <timograham@…>

In 331d7652:

[1.11.x] Refs #27807 -- Removed docs for User.username_validator.

The new override functionality claimed in refs #21379 doesn't work.
Forwardport of 714fdbaa7048c2321f6238d9421137c33d9af7cc from stable/1.10.x.

comment:15 Changed 6 months ago by Jarek Glowacki

Just dropping a diff here for failing tests if someone picks this up in future.
Though I'd also support just closing this as a wontfix.

diff --git a/tests/validation/models.py b/tests/validation/models.py
index e8e18cfbce..1069f5719f 100644
--- a/tests/validation/models.py
+++ b/tests/validation/models.py
@@ -1,5 +1,6 @@
 from datetime import datetime

+from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
 from django.db import models

@@ -134,3 +135,7 @@ assert str(assertion_error) == (
     "Model validation.MultipleAutoFields can't have more than one "
     "auto-generated field."
 )
+
+
+class ShadowUser(User):
+    username_validator = validate_answer_to_universe
diff --git a/tests/validation/test_override.py b/tests/validation/test_override.py
new file mode 100644
index 0000000000..2c0da854bd
--- /dev/null
+++ b/tests/validation/test_override.py
@@ -0,0 +1,13 @@
+from django.test import SimpleTestCase
+
+from .models import ShadowUser
+
+
+class TestUsernameValidatorOverride(SimpleTestCase):
+    def test_username_validator_override(self):
+        self.assertEqual(ShadowUser.username_validator.__name__, 'validate_answer_to_universe')
+
+        self.assertCountEqual(
+            ['validate_answer_to_universe'],
+            [getattr(v, '__name__', v.__class__.__name__) for v in ShadowUser._meta.get_field("username").validators]
+        )

comment:16 Changed 3 months ago by Shekhar Nath Gyanwali

Owner: changed from nobody to Shekhar Nath Gyanwali
Status: newassigned

Hi, I am working on this and it is my first django ticket.

Last edited 2 months ago by Shekhar Nath Gyanwali (previous) (diff)

comment:17 Changed 7 weeks ago by Shekhar Nath Gyanwali

Submitted a pull request. Please see pull request https://github.com/django/django/pull/13114 for more detail.

comment:18 Changed 7 weeks ago by Jacob Walls

Patch needs improvement: set

comment:19 Changed 3 weeks ago by felixxm

Has patch: set

comment:20 Changed 12 days ago by Shekhar Nath Gyanwali

Submitted pull request with the changes as per suggestion.

comment:21 Changed 10 days ago by Jacob Walls

Patch needs improvement: unset

comment:22 Changed 10 days ago by Jacob Walls

Patch needs improvement: set

comment:23 Changed 8 days ago by Shekhar Nath Gyanwali

Patch needs improvement: unset

Made all the changes based on feedback on PR.

I am not sure if release note is 100%, might take few iteration(my apologies).

comment:24 Changed 8 days ago by Shekhar Nath Gyanwali

Fixed typo, apologies completely missed that one.

comment:25 Changed 4 days ago by Jacob Walls

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top