Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 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: UI/UX:

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 4 years ago.
auth.2.diff (8.4 KB) - added by laurentluce 4 years ago.
auth.3.diff (8.5 KB) - added by laurentluce 4 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by gabrielhurley

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 4 years ago by laurentluce

  • milestone set to 1.3
  • Owner changed from nobody to laurentluce

comment:3 Changed 4 years ago by laurentluce

  • Status changed from new to assigned

Changed 4 years ago by laurentluce

comment:4 Changed 4 years ago by laurentluce

  • 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 Changed 4 years ago by 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

comment:6 Changed 4 years ago by russellm

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

Changed 4 years ago by laurentluce

Changed 4 years ago by laurentluce

comment:7 Changed 4 years ago by laurentluce

  • 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 Changed 4 years ago by russellm

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

(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 Changed 4 years ago by russellm

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 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.