Opened 4 months ago

Closed 11 days 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, 4 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, 4 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, 2 months ago

Can I work on this ticket?

comment:4 by Antoliny, 4 weeks ago

Owner: changed from iamkorniichuk to Antoliny

comment:5 by Antoliny, 4 weeks 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).

Last edited 4 weeks ago by Antoliny (previous) (diff)

comment:6 by Antoliny, 3 weeks ago

Needs documentation: unset
Needs tests: unset

comment:7 by Sarah Boyce, 2 weeks ago

Needs documentation: set
Patch needs improvement: set

comment:8 by Sarah Boyce, 2 weeks 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, 11 days 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