Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15639 closed (needsinfo)

make_random_password in django.contrib.auth.models UserManager(models.Manager):

Reported by: anonymous Owned by: nobody
Component: Uncategorized Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

make_random_password in django.contrib.auth.models UserManager(models.Manager):

appears to use 'choice' from the python random module.

    def make_random_password(self, length=10, allowed_chars='abcdefghjkmnpqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789'):
        "Generates a random password with the given length and given allowed_chars"
        # Note that default value of allowed_chars does not have "I" or letters
        # that look like it -- just to avoid confusion.
        from random import choice
        return ''.join([choice(allowed_chars) for i in range(length)])

I propose the following instead:

    def make_random_password(self, length=10, allowed_chars='abcdefghjkmnpqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789'):
        "Generates a random password with the given length and given allowed_chars"
        # Note that default value of allowed_chars does not have "I" or letters
        # that look like it -- just to avoid confusion.
        from random import SystemRandom as random
        return ''.join([random().choice(allowed_chars) for i in range(length)])

Attachments (0)

Change History (4)

comment:1 Changed 3 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

It's hard to consider such a proposal without a rationale. Can you explain why the proposed change would be a fix or enhancement over the current code?

comment:2 Changed 3 years ago by russellm

The only reason I can think of to generate a random password would be to send it in cleartext. To which, the answer is a definitive No. Not ever.

comment:3 Changed 3 years ago by DrMeers

I imagine the reporter's rationale was that random.SystemRandom might provide better randomisation the standard implementation. No idea whether that is the case. Russ, the method already exists in Django apparently?

comment:4 Changed 3 years ago by anonymous

@DrMeets - that is the case.

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.