Opened 13 months ago

Closed 9 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 iamkorniichuk, 13 months ago

I think it will be enough to add such a method to django.contrib.auth.password_validation classes:

def __call__(self, *args, **kwargs):
    return self.validate(*args, **kwargs)

comment:2 by Natalia Bidart, 13 months ago

Keywords: validators added; validator validator removed
Needs documentation: set
Needs tests: set
Owner: set to iamkorniichuk
Status: newassigned
Triage Stage: UnreviewedAccepted
Version: 5.1dev

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:3 by Antoliny, 11 months ago

Can I work on this ticket?

comment:4 by Antoliny, 10 months ago

Owner: changed from iamkorniichuk to Antoliny

comment:5 by Antoliny, 10 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?

Version 0, edited 10 months ago by Antoliny (next)

comment:6 by Antoliny, 9 months ago

Needs documentation: unset
Needs tests: unset

comment:7 by Sarah Boyce, 9 months ago

Needs documentation: set
Patch needs improvement: set

comment:8 by Sarah Boyce, 9 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 from BaseValidator for 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 Sarah Boyce, 9 months ago

Cc: Ryan Hiebert added
Resolution: wontfix
Status: assignedclosed

By default validating fields use this syntax validator(value) but password validators don’t implement .__call__(). Therefore making them unusable outside of validate_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__).

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