Opened 21 months ago

Closed 21 months ago

Last modified 20 months ago

#20593 closed Bug (fixed)

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

Reported by: jonaskoelker Owned by: Tim Graham <timograham@…>
Component: contrib.auth Version: master
Severity: Normal Keywords: blank password
Cc: bmispelon@…, eromijn@…, jonaskoelker 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 jonaskoelker 21 months ago.
Proper fix, plus test cases

Download all attachments as: .zip

Change History (12)

comment:1 Changed 21 months ago by bmispelon

  • Cc bmispelon@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 21 months ago by 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.

comment:3 Changed 21 months ago by PaulM

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 Changed 21 months ago by erikr

  • Cc eromijn@… added
  • Needs documentation set
  • Owner changed from nobody to erikr
  • Status changed from new to assigned

comment:5 Changed 21 months ago by erikr

  • Has patch set
  • Needs documentation unset
  • Owner erikr deleted
  • Status changed from assigned to new
  • Version changed from 1.4 to master

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 Changed 21 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 2c4fe761a0e2b28e2c5c3b4bc506ee06824a443d:

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

Changed 21 months ago by jonaskoelker

Proper fix, plus test cases

comment:7 Changed 21 months ago by jonaskoelker

  • Cc jonaskoelker added
  • Resolution fixed deleted
  • Status changed from closed to new

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 Changed 21 months ago by jonaskoelker

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 Changed 21 months ago by bmispelon

  • Resolution set to fixed
  • Status changed from new to closed

comment:10 Changed 20 months ago by 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.

comment:11 Changed 20 months ago by 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.

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