Opened 15 months ago
Closed 11 months ago
#35693 closed Cleanup/optimization (wontfix)
Password validators aren't callable
| Reported by: | iamkorniichuk | Owned by: | Antoliny | 
|---|---|---|---|
| Component: | contrib.auth | Version: | dev | 
| Severity: | Normal | Keywords: | validators password callable | 
| Cc: | iamkorniichuk, Ryan Hiebert | Triage Stage: | Accepted | 
| Has patch: | yes | Needs documentation: | yes | 
| Needs tests: | no | Patch needs improvement: | yes | 
| Easy pickings: | no | UI/UX: | no | 
Description
By default validating fields use this syntax validator(value) but password validators don't implement .__call__(). Therefore making them unusable outside of validate_password().
This is small change doesn't change any behavior but add support for intuitive approach to password validators.
Change History (10)
comment:1 by , 15 months ago
comment:2 by , 15 months ago
| Keywords: | validators added; validator validator removed | 
|---|---|
| Needs documentation: | set | 
| Needs tests: | set | 
| Owner: | set to | 
| Status: | new → assigned | 
| Triage Stage: | Unreviewed → Accepted | 
| Version: | 5.1 → dev | 
Thank you iamkorniichuk for creating this ticket.
As I mentioned in your PR, I agree that the inconsistency between the django.core.validators providing __call__ but password validators not providing it feels off. Accepting on that basis, also I couldn't find a previous conversation or request about this issue.
Please note that the PR definitely needs tests and docs updates, besides a small release note for 5.2.
Good luck!
comment:4 by , 11 months ago
| Owner: | changed from to | 
|---|
comment:5 by , 11 months ago
If we provide .__call__ magic method to the password validator class, it can be used like a regular validator class.
# password_validation.py
class CommonPasswordValidator:
    def __call__(self, *args, **kwargs):
        return self.validate(*args, **kwargs)
# views.py
from django import forms
from django.contrib.auth.password_validation import CommonPasswordValidator
valid_common_password = CommonPasswordValidator()
class TestForm(forms.Form):
  email = forms.EmailField()
  password = forms.CharField(validators=[valid_common_password])
>>> form = TestForm(data={"email":"antoliny0919@gmail.com", "password":"1234"})
>>> form.is_valid()
False
>>> form.errors
{'password': ['This password is too common.']}
However, unlike the general validation class, the password validation requires two arguments for verification. (password, user)
def validate_password(password, user=None, password_validators=None):
    ...
    for validator in password_validators:
        try:
            validator.validate(password, user)
Fortunately, the validation class validate method specifies the default value of the user argument to None, so there is no problem calling it, but I don't know if call is required for classes such as UserAttributeSimilarityValidator because the validate is terminated when the user argument does not exist.
I think it's a good way to add .__call__magic method to the password validation class, but there are questions as above. I'm sorry, but could you please review it again?
I'm wonder the opinions of fellow's :) (About the expected effect of adding the .__call__magic method).
comment:6 by , 11 months ago
| Needs documentation: | unset | 
|---|---|
| Needs tests: | unset | 
comment:7 by , 11 months ago
| Needs documentation: | set | 
|---|---|
| Patch needs improvement: | set | 
comment:8 by , 11 months ago
Also commented on the PR but I'm not 100% sure this is worth it just for the consistency, but if we were to do this:
- validators also have an __eq__method, if we are adding the call for consistency, I feel like we should add this for consistency. We should also consider whether to inherit fromBaseValidatorfor example.
- I would expect the docs in the Writing your own validator section to be updated.
- Any custom validator not provided by Django will not have this change
I think the next steps would be to raise this on forum and see if there is a consensus that this additional is valuable and wanted.
I would suggest instead that the Password validation docs make it clear that these validators are not field validators and perhaps say "password validator" instead of "validator" consistently
comment:10 by , 11 months ago
| Cc: | added | 
|---|---|
| Resolution: | → wontfix | 
| Status: | assigned → closed | 
By default validating fields use this syntax validator(value) but password validators don’t implement
.__call__(). Therefore making them unusable outside ofvalidate_password().
As Ryan pointed out in the forum discussion, these are not “unusable outside validate_password()” as they can be used with:
CharField(..., validators=[MinimumLengthValidator().validate]
Those that need a user will pass silently when a user is not passed (adding a .__call__() doesn’t change that).
So I think the problem this wanted to solve “make them usable outside validate_password()” is not really a problem and, as this was accepted to solve this, I will close this ticket as wontfix.
In the forum I think there were good ideas and thoughts around the direction of validators and password validators. I think these thoughts need more time to crystalize before we jump into making a change here. Once there is a solid proposal with support from the community, that should probably be a new ticket (as I imagine it might be more than just adding __call__).
I think it will be enough to add such a method to
django.contrib.auth.password_validationclasses:def __call__(self, *args, **kwargs): return self.validate(*args, **kwargs)