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
.
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:
- Log in at http://localhost:8000/admin with username
admin
and passwordpassword123
- Attempt to create a new Custom User with an apostrophe (single quote) in the username
- Get validation error despite both the form validator and
CustomUser.username_validator
allowing apostrophes - Optionally uncomment the
CustomUser.__init__
method to see expected behavior, even without a custom admin form
Attachments (1)
Change History (30)
by , 8 years ago
Attachment: | myproject.zip added |
---|
comment:1 by , 8 years ago
Severity: | Normal → Release blocker |
---|---|
Summary: | Overriding username validators does not seem to work → Overriding username validators doesn't work as documented |
Triage Stage: | Unreviewed → Accepted |
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 , 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?
An AUTH_USERNAME_VALIDATORS
dict (à la AUTH_PASSWORD_VALIDATORS
) would also work.
comment:3 by , 8 years ago
Here is an example a working AUTH_USERNAME_VALIDATORS
setting:
https://github.com/johnrork/django/commit/8afbb76f3aca306ec7b678cb394fcedc8b2edf12
comment:4 by , 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 , 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 , 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:8 by , 8 years ago
Severity: | Release blocker → Normal |
---|
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 , 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 , 6 years ago
Cc: | added |
---|
comment:15 by , 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 , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Hi, I am working on it. This is my first django ticket.
comment:17 by , 4 years ago
Submitted a pull request. Please see pull request https://github.com/django/django/pull/13114 for more detail.
comment:18 by , 4 years ago
Patch needs improvement: | set |
---|
comment:19 by , 4 years ago
Has patch: | set |
---|
comment:21 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:22 by , 4 years ago
Patch needs improvement: | set |
---|
comment:23 by , 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:25 by , 4 years ago
Patch needs improvement: | set |
---|
comment:26 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:29 by , 4 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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.
Demonstration of possible Model.username_validator bug