Disagreement between User.set_password("") and User.check_password("")

[Jonas Kölker]

At ​https://docs.djangoproject.com/en/1.4/topics/auth/, under set_unusable_password, it says "Marks the user as having no password set. This isn’t the same as having a blank string for a password", which suggests that the latter is possible. Also, set_password doesn't say anything about blank passwords not being allowed. Yet, when I run

    user.set_password(form.cleaned_data['new0'])
    assert user.check_password(form.cleaned_data['new1'])

with both new passwords being empty, I get an assertion failure. According to at least two core developers*, allowing a blank password is not a bug---which would indicate that set_password does the wrong thing.

(*)
one: https://code.djangoproject.com/ticket/4170
two: https://code.djangoproject.com/ticket/14354 plus ​https://groups.google.com/forum/#!msg/django-developers/NtpFFdYh-3g/erB1Qhd6m2UJ

Looking at hashers.py, I see

def make_password(password, salt=None, hasher='default'):
    """[... If password is None or blank then UNUSABLE_PASSWORD will be returned ...]"""
    if not password:
        return UNUSABLE_PASSWORD

That looks rather deliberate. If that's so I think the documentation should be updated, reflecting the fact that User.set_password("") does something rather anti-intuitive, and that there even more anti-intuitively exists a string s such that if you do set_password(s), then check_password(s) fails.

Also, I think silently forbidding blank passwords is really bad. If set_password were to throw a ValueError with an explanation of why you rather than I should decide whether to allow blank passwords, I would dislike it but at least its anti-intuitiveness would be explicit.

As things are at present, given the silent transition (or did I look in all the wrong places for the deprecation warnings?) users of Django applications could lock themselves out when they intend to have a blank password---the 180 degrees opposite effect of what they want.

---

I've set version=1.4 on this ticket, because that's the one I use in my production environment, i.e. the one where I have exhibited this behavior. Version 1.3 did something different, see ​https://github.com/django/django/blob/1.3/django/contrib/auth/models.py#L251

I've set easy=true, because I think the fix is a one-line patch and a four-line addition to/of a test case---which I'll be happy to write---or some documentation rewriting (which I'll leave to someone else). I think the best next step is for the group of core devs to agree on what the right thing for set_password("") to do is, then add that information to this ticket.

[Baptiste Mispelon]

Hi,

The backwards-incompatible change was made in commit 90e05aaeac612a4251640564aa65f103ac635e12 (which, as you noted, landed in 1.4).
There is no reference to a ticket nor is there a note in the release notes so I'm not sure the change was intentional.

I'm accepting this on the basis that this backwards-incompatible change should at least be documented, if not even reverted.

Thanks for the thorough report.

[wim@…]

In my opinion, set_password("") should raise an exception.

Rationale: passwords are used to identify users. Allowing them to have empty passwords is unsafe; and if you want people to be able to enter a website without having a password then use a different authentication backend which does not check for an empty password.

In practice, all django authentication forms require the password field to be filled, and people have to enter characters for the form to be valid, otherwise the form raises an error.

[Paul McMillan]

If a blank password was valid before, I agree with the reporter that it should still be valid, and I agree that set_password/check_password should be inverses. This function is not the place to enforce password conformance to any particular policy, and there are cases where an empty password is legitimately ok (e.g. when building a system authenticated by hardware tokens).

A correct patch will check for None rather than truthiness, and document the period during which an empty string was unintentionally disallowed. I would also like to see a unittest to make sure each of our included hashers correctly handles the empty string case.

[Sasha Romijn]

PaulM's suggestions make sense to me. PR: ​https://github.com/django/django/pull/1278

Also added a note in the release notes about this change. Tests successfully run.

[Tim Graham <timograham@…>]

In 2c4fe761a0e2b28e2c5c3b4bc506ee06824a443d:

Fixed #20593 -- Allow blank passwords in check_password() and set_password()

[Jonas Kölker]

Proper fix, plus test cases

[Jonas Kölker]

Assuming my understanding of what bmispelon and PaulM think should be done is correct, the patch at ​https://github.com/django/django/pull/1278 doesn't fix the problem.

I've attached my own fix. With my changes to the test suite and erikr's changes to hashers.py, the test cases fail---meaning that either his fix or my tests are invalid. If my tests are wrong, please explain why. Note: if after applying my patch you revert any of the hunks applied to hashers.py you will introduce a test case failure.

[Jonas Kölker]

Oh yay, I misread a summary of erikr's patch as if it were the whole patch. His patch is fine. Sorry for reopening. Nothing to see here, move along ;-)

[Simon Charette <charette.s@…>]

In 8759778185d0539bf9c11e3fda497a9486b9acab:

Fixed #20675 -- check_password should work when no password is specified.

The regression was introduced by 2c4fe761a. refs #20593.

[Simon Charette <charette.s@…>]

In 2de0d4c4523ca3d1d6744ba0f22b8ef33bedfa03:

[1.6.x] Fixed #20675 -- check_password should work when no password is specified.

The regression was introduced by 2c4fe761a. refs #20593.

Backport of 8759778185 from master.