Opened 8 years ago

Closed 4 years ago

#27807 closed Bug (wontfix)

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: no
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 8 years ago.
Demonstration of possible Model.username_validator bug

Download all attachments as: .zip

Change History (30)

by johnrork, 8 years ago

Attachment: myproject.zip added

Demonstration of possible Model.username_validator bug

comment:1 by Tim Graham, 8 years ago

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 by johnrork, 8 years ago

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?

Version 0, edited 8 years ago by johnrork (next)

comment:3 by johnrork, 8 years ago

Here is an example a working AUTH_USERNAME_VALIDATORS setting:

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

comment:4 by Claude Paroz, 8 years ago

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 by Tim Graham, 8 years ago

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 by Claude Paroz, 8 years ago

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 by Tim Graham <timograham@…>, 8 years ago

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 by Tim Graham, 8 years ago

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 by Claude Paroz, 8 years ago

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 by Tobias Wiese, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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 by Jarek Glowacki, 5 years ago

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 by Shekhar Nath Gyanwali, 5 years ago

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 4 years ago by Shekhar Nath Gyanwali (previous) (diff)

comment:17 by Shekhar Nath Gyanwali, 4 years ago

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

comment:18 by Jacob Walls, 4 years ago

Patch needs improvement: set

comment:19 by Mariusz Felisiak, 4 years ago

Has patch: set

comment:20 by Shekhar Nath Gyanwali, 4 years ago

Submitted pull request with the changes as per suggestion.

comment:21 by Jacob Walls, 4 years ago

Patch needs improvement: unset

comment:22 by Jacob Walls, 4 years ago

Patch needs improvement: set

comment:23 by Shekhar Nath Gyanwali, 4 years ago

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 by Shekhar Nath Gyanwali, 4 years ago

Fixed typo, apologies completely missed that one.

comment:25 by Jacob Walls, 4 years ago

Patch needs improvement: set

comment:26 by Shekhar Nath Gyanwali, 4 years ago

Patch needs improvement: unset

comment:27 by Shekhar Nath Gyanwali, 4 years ago

Added more test and fixed typo/unwanted spaces etc.

comment:28 by Shekhar Nath Gyanwali, 4 years ago

Fixed documentation typo and removed username_validator variable.

comment:29 by Carlton Gibson, 4 years ago

Resolution: wontfix
Status: assignedclosed

I'm going to close this as wontfix. The issue doesn't justify new settings or signals or extensions to the validator API or and so on.

UnicodeUsernameValidator is a good default, and Use a custom user model is not unreasonable. People who can't do that can add validation at the form level, in clean(), or even monkey-patch the field if they absolutely must, but flexibility for it's own sake is not worth significant complication here.

Happy to look at a patch that is small and self-contained, but generally the whole issue seems moot.

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