password is None in check_password

[Laurent Luce]

Hello,

Regarding the issue about password is None in check_password (http://
code.djangoproject.com/ticket/14354). I attached a patch with the
following changes:

- in set_password(), check for raw_password and if None or empty, call
set_unusable_password(), otherwise same as before

- in has_usable_password(), return True only if password is not None,
not empty, 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
inside this function anymore.

- basic.py tests are now unittests and not doctests

I assumed that has_usable_password() will be called directly by the
user code so I didn't modify check_password().

Please let me know if my changes match what is expected.

Laurent
http://www.laurentluce.com
http://www.gourmious.com

[Łukasz Rekucki]

Hi,

I started working on a somehow related ticket #14390. Adrian suggested
to create a utils module, so I wanted to put there all useful
password-related functions: check_password(), make_password(),
is_password_usable() and the UNUSABLE_PASSWORD constant. So I'm
interested in API that this functions (and thus User's methods) should
provide.

On 7 October 2010 23:13, Laurent Luce <lauren...@yahoo.com> wrote:
> Hello,
>
> Regarding the issue about password is None in check_password (http://
> code.djangoproject.com/ticket/14354). I attached a patch with the
> following changes:
>
> - in set_password(), check for raw_password and if None or empty, call
> set_unusable_password(), otherwise same as before

In theory, someone could allow empty passwords which will get hashed properly.

>
> - in has_usable_password(), return True only if password is not None,
> not empty, or '!'

There are actually lot of other unusable values - at the moment,
anything that isn't a MD5 hash or a legitimate password string in
algo$salt$hash format. Is it ok, to only special case this three
values ?

>
> - 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
> inside this function anymore.
>
> - basic.py tests are now unittests and not doctests

Good work :)

--
Łukasz Rekucki

[Laurent Luce]

I noticed that create_user() is currently setting password to unusable
if it is None or empty. However, set_password() is accepting an empty
password. I decided to follow the first rule in the patch I submitted
but I am kind of confused now. Can someone indicate what we should
accept as a password?

On Oct 7, 3:25 pm, Łukasz Rekucki <lreku...@gmail.com> wrote:
> Hi,
>
> I started working on a somehow related ticket #14390. Adrian suggested
> to create a utils module, so I wanted to put there all useful
> password-related functions: check_password(), make_password(),
> is_password_usable() and the UNUSABLE_PASSWORD constant. So I'm
> interested in API that this functions (and thus User's methods) should
> provide.
>

> On 7 October 2010 23:13, Laurent Luce <laurentluc...@yahoo.com> wrote:> Hello,

[Russell Keith-Magee]

On Fri, Oct 8, 2010 at 11:28 AM, Laurent Luce <lauren...@yahoo.com> wrote:
> I noticed that create_user() is currently setting password to unusable
> if it is None or empty. However, set_password() is accepting an empty
> password. I decided to follow the first rule in the patch I submitted
> but I am kind of confused now. Can someone indicate what we should
> accept as a password?

An empty string isn't a *good* password, but then neither is a single
character or a dictionary word, and we don't reject those. Policy
decisions like this aren't the domain of a web framework.

I'm sure there are also people using an empty password as the "I don't
actually want security" password. Plus, there's a history in the free
software community of using empty passwords as a protest [1] :-)

We can use None to mark an unusable password. Absent of a good
technical reason, I don't see why we should reject empty string.

As for the remainder of the patch on #14354 -- on the whole, it looks
good. I've put some review comments on the ticket.

[1] http://en.wikipedia.org/wiki/Richard_Stallman#Early_years

Yours,
Russ Magee %-)

[Laurent Luce]

Thanks for your feedback. I attached a new patch with the following
changes:

* 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

Let me know if the patch still needs improvement.

Laurent Luce

On Oct 7, 11:17 pm, Russell Keith-Magee <russ...@keith-magee.com>
wrote:

[Laurent Luce]

I just want to summarize the password handling changes before and
after this patch.

before:
- create_user() with empty password or None -> password set to '!'
unusable
- set_password() accepts everything
- has_usable_password() returning False for password equal to '!'

after:
- create_user() with password None -> password set to '!' unusable
- set_password() with None -> password set to '!' unusable
- has_usable_password() returning False for password equal to '!' or
None

Laurent