Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#14354 closed (fixed)

Check password is not None in User.check_password

Reported by: berryp Owned by: laurentluce
Component: contrib.auth Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I recently had an unexpected situation where users with no passwords would receive an error when trying to login. This is due to the fact that the User.check_password method does not check for missing passwords before calling get_hexdigest.

It could be argued that all users should either have a password or an unusable password "!". However, as I am authenticating against a database that belongs to another system it is not an option to go and change all empty passwords to unusable ones. I would not expect authentication to raise an exception in this occasion.

To get around this problem I simply inserted the following two lines at the top of the check_password function:

if self.password is None:
    return False

Additionally, would it not be a good idea to check that the password is not UNUSABLE_PASSWORD before trying to execute the code that checks the password? This would be a lot more elegant than executing code that is ultimately going to fail.

Attachments (3)

auth.diff (6.9 KB ) - added by laurentluce 13 years ago.
auth.2.diff (8.4 KB ) - added by laurentluce 13 years ago.
auth.3.diff (8.5 KB ) - added by laurentluce 13 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 by Gabriel Hurley, 13 years ago

Triage Stage: UnreviewedAccepted

Seems like a valid point to me. I think it might be a little cleaner to alter has_usable_password to check for whatever cases are invalid (UNUSABLE_PASSWORD, None, ), and then call has_usable_password internally.

Also, this should definitely have a test case. For bonus points, since Django is slowly trying to eliminate doctests, you could convert the contrib.auth.tests.basic test case into a unit test (it's not that many lines).

comment:2 by laurentluce, 13 years ago

milestone: 1.3
Owner: changed from nobody to laurentluce

comment:3 by laurentluce, 13 years ago

Status: newassigned

by laurentluce, 13 years ago

Attachment: auth.diff added

comment:4 by laurentluce, 13 years ago

Has patch: set

I attached a patch with the following changes:

  • in set_password(), check for raw_password and if None or , call set_unusable_password(), otherwise same as before
  • in has_usable_password(), return True only if password is not None, or '!'
  • because of the 2 changes above, we can simplify a bit create_user() by just calling set_password() for all cases. No need to test password and branch out.
  • basic.py tests are now unittests and not doctests

comment:5 by laurentluce, 13 years ago

Reformat the description.

I attached a patch with the following changes:

  • in set_password(), check for raw_password and if None or , call set_unusable_password(), otherwise same as before
  • in has_usable_password(), return True only if password is not None, or '!'
  • because of the 2 changes above, we can simplify a bit create_user() by just calling set_password() for all cases. No need to test password and branch out.
  • basic.py tests are now unittests and not doctests

comment:6 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set

Looks good, Laurent. Some minor review comments:

  • As discussed on the list, the check for invalidity should allow empty string, and use None to mark an unusable password.
  • Empty setup/teardown methods aren't needed. There's no need to formally declare a no-op.
  • The tests for the createsuperuser management commands output to stdout - tests shouldn't have any visible manifestation. As part of the migration to use unittests, we're modifying the admin commands to take stdout/stderr arguments, and writing output to those streams. See loaddata/dumpdata for examples. This allows the test commands (or any other programmatic usage) to provide a stream that will be used for output (which provides a second way to fix this test problem -- invoke the test with verbosity=0).
  • While you're in that areas, the "superuser created" text should also be covered by a verbosity check; verbosity=0 should output nothing.

by laurentluce, 13 years ago

Attachment: auth.2.diff added

by laurentluce, 13 years ago

Attachment: auth.3.diff added

comment:7 by laurentluce, 13 years ago

Patch needs improvement: unset

New patch attached:

  • allow empty string in set_password()
  • has_usable_password() returns false if password is '!' or None
  • add unit test set_password(None)
  • add verbosity option to createsuperuser command + unit test
  • output msg to stdout in createsuperuser command + update unit tests

comment:8 by Russell Keith-Magee, 13 years ago

Resolution: fixed
Status: assignedclosed

(In [14054]) [1.2.X] Fixed #14354 -- Normalized the handling of empty/null passwords in contrib.auth. This also updates the createsuperuser command to be more testable, and migrates some auth doctests. Thanks to berryp for the report, and Laurent Luce for the patch.

Backport of r14053 from trunk.

comment:9 by Russell Keith-Magee, 13 years ago

Laurent - thanks for the revised patch. For your benefit next time, I made a couple of minor tweaks before pushing to trunk:

  • Line length in the unit tests. Where practical and legible, keep lines under 80 chars
  • A trailing newline on the 'superuser created' message
  • A couple of explanatory comments and newline breaks in the tests so it's easy to follow what's going on.

Other than that, thanks for the contribution!

comment:10 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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