Check password is not None in User.check_password

[berryp]

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.

[Gabriel Hurley]

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

[laurentluce]

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

[laurentluce]

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

[Russell Keith-Magee]

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.

[laurentluce]

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

[Russell Keith-Magee]

(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.

[Russell Keith-Magee]

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!

[Jacob]

Milestone 1.3 deleted