Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#20593 closed Bug (fixed)

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

Reported by: Jonas Kölker Owned by: Tim Graham <timograham@…>
Component: contrib.auth Version: dev
Severity: Normal Keywords: blank password
Cc: bmispelon@…, eromijn@…, Jonas Kölker Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Attachments (1)

empty-password-fix.patch (6.4 KB ) - added by Jonas Kölker 11 years ago.
Proper fix, plus test cases

Download all attachments as: .zip

Change History (12)

comment:1 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added
Triage Stage: UnreviewedAccepted

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.

comment:2 by wim@…, 11 years ago

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.

comment:3 by Paul McMillan, 11 years ago

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.

comment:4 by Sasha Romijn, 11 years ago

Cc: eromijn@… added
Needs documentation: set
Owner: changed from nobody to Sasha Romijn
Status: newassigned

comment:5 by Sasha Romijn, 11 years ago

Has patch: set
Needs documentation: unset
Owner: Sasha Romijn removed
Status: assignednew
Version: 1.4master

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.

comment:6 by Tim Graham <timograham@…>, 11 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 2c4fe761a0e2b28e2c5c3b4bc506ee06824a443d:

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

by Jonas Kölker, 11 years ago

Attachment: empty-password-fix.patch added

Proper fix, plus test cases

comment:7 by Jonas Kölker, 11 years ago

Cc: Jonas Kölker added
Resolution: fixed
Status: closednew

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.

comment:8 by Jonas Kölker, 11 years ago

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 ;-)

comment:9 by Baptiste Mispelon, 11 years ago

Resolution: fixed
Status: newclosed

comment:10 by Simon Charette <charette.s@…>, 11 years ago

In 8759778185d0539bf9c11e3fda497a9486b9acab:

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

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

comment:11 by Simon Charette <charette.s@…>, 11 years ago

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.

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