#20593 closed Bug (fixed)
Disagreement between User.set_password("") and User.check_password("")
Reported by: | Jonas Kölker | Owned by: | |
---|---|---|---|
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)
Change History (12)
comment:1 by , 11 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 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 , 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 , 11 years ago
Cc: | added |
---|---|
Needs documentation: | set |
Owner: | changed from | to
Status: | new → assigned |
comment:5 by , 11 years ago
Has patch: | set |
---|---|
Needs documentation: | unset |
Owner: | removed |
Status: | assigned → new |
Version: | 1.4 → 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 by , 11 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
comment:7 by , 11 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Status: | closed → 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 by , 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.