14
\$\begingroup\$

Here's a password generator I created.

### THIS PROGRAM WAS CREATED FOR LEARNING PURPOSES  ###
###   AND SHOULD NOT BE USED TO CREATE PASSWORDS!   ###

import string
from enum import Enum
from random import randint, shuffle, choice

class PasswordError(Exception):
    __module__ = Exception.__module__

# Types of characters possible
class Types(Enum):
    CAP = 'CAP' # Capital
    SMA = 'SMA' # Small
    DIG = 'DIG' # Digits
    SPE = 'SPE' # Special

# Characters for each type of possible characters
type_chars = {
    Types.CAP: string.ascii_uppercase,
    Types.SMA: string.ascii_lowercase,
    Types.DIG: string.digits,
    Types.SPE: '!()-.?[]_`~;:@#$%^&*='
}

def password_generator(min_length=6, max_length=20, caps=1, small=1, digits=1, special=1):
    types = {
        Types.CAP: caps,
        Types.SMA: small,
        Types.DIG: digits,
        Types.SPE: special,
    }

    num_chars = sum(list(types.values())) # Number of mandatory characters
    min_length = max(num_chars, min_length) # In case 'num_chars' is greater

    # Number of characters required for each possible type of character
    # Is greater than maximum possible length
    if min_length > max_length:
        raise PasswordError(f'No password with the given criteria')

    length = randint(min_length, max_length)

    # List that stores the "types" of possible character
    char_list = []

    # Mandatory requirements
    for typ, val in zip(types.keys(), types.values()):
        char_list.extend([typ] * val)

    # The remaining values to fill
    for rem in range(length - num_chars):
        char_list.append(choice(list(types.keys())))

    shuffle(char_list)

    password = ''

    for typ in char_list:
        password += choice(type_chars[typ])

    return password

if __name__ == '__main__':
    min_length = int(input('Minimum number of characters required: '))
    max_length = int(input('Maximum number of characters possible: '))

    print()

    caps = int(input('Number of capital letters required: '))
    small = int(input('Number of small letters required: '))
    digits = int(input('Number of digits required: '))
    special = int(input('Number of special characters required: '))

    print()

    number = int(input('Number of passwords required: '))

    print('\n' + '-' * 50 + '\n')
    print('   Here are your passwords: \n')

    for i in range(number):
        print(password_generator(min_length, max_length, caps, small, digits, special))

You can also modify the mandatory types of characters (Like 3 caps, 2 digits, etc). Also, minimum length and maximum length can be specified.

I think this code looks a bit ugly. How do I improve this?

\$\endgroup\$
  • 5
    \$\begingroup\$ Please note that Official Guidelines state that requiring special character classes results in bad passwords. The only difference it makes it that it turns all password into Password123! instead, which add exactly zero security. \$\endgroup\$ – Gloweye Nov 25 '19 at 15:32
  • \$\begingroup\$ @Gloweye I didn't quite know that! I just based of my program from my experience with websites that said 'weak' for password and 'medium' for Password123! and 'strong' for P@ssword$123! \$\endgroup\$ – Srivaths Nov 25 '19 at 16:31
  • 3
    \$\begingroup\$ @Gloweye regardless, many sites still require special characters. This tool generates passwords. OP is not asking advice for what rules to use for a site they control. \$\endgroup\$ – Captain Man Nov 25 '19 at 18:51
  • 1
    \$\begingroup\$ I'd suggest including a warning in the output (or at least in the source as a comment) that this is (probably) not safe for real use; you're not a crypto expert. Neither am I, but I know enough to be cautious about where my random numbers come from (does Python make sure to seed its RNG from /dev/random or similar? Maybe but IDK), and that there are some agreed-upon standards for generating passwords for humans. \$\endgroup\$ – Peter Cordes Nov 26 '19 at 3:48
  • 2
    \$\begingroup\$ @PeterCordes You should read AlexV's answer below. Python has the secrets module for cryptographic-quality random numbers. \$\endgroup\$ – Gloweye Nov 26 '19 at 6:52
13
\$\begingroup\$

Refactoring in steps

Enumeration class

Types is too generic name for the enum representing char types. Renamed to CharTypes.
Instead CAP, SMA, DIG, SPE as enumeration members are better replaced with a more common/familiar and comprehensive abbreviations/associations:
UPPER, LOWER, DIGIT and SPECIAL.
Since string.ascii_uppercase and other string.* are essentially just string constants - they can be easily set as enumeration values:

class CharTypes(Enum):
    UPPER = string.ascii_uppercase     # Capital
    LOWER = string.ascii_lowercase     # Small
    DIGIT = string.digits              # Digits
    SPECIAL = '!()-.?[]_`~;:@#$%^&*='  # Special

thus, making all intermediate re-mappings like type_chars and types (in password_generator function) redundant and unnecessary.


password_generator function

The function signature is slightly changed in arguments names to conform with CharTypes members:

def password_generator(min_length=6, max_length=20, upper=1, lower=1, digits=1, special=1)

types mapping is eliminated as redundant.

char counts passed as arguments are gathered and summed at once:

char_counts = (upper, lower, digits, special)
num_chars = sum(char_counts)

Avoid overwriting/assigning to function argument like min_length = max(num_chars, min_length) as min_length might be potentially referenced as "original" argument value (and relied on) in other places in the function's body.
A safer way is assigning it to a separate variable:

min_len = max(num_chars, min_length)

length variable is renamed to target_length (to emphasize the final size).

char_list is renamed to char_types as it's aimed to accumulate CharTypes enum members

Two for loops which performed char_list.extend and char_list.append are efficiently replaced with 2 generators which further joined/merged by itertools.chain function:

char_types = list(chain(*([c_type] * num for c_type, num in zip(CharTypes, char_counts)),
                        (choice(char_types_enums) for _ in range(target_length - num_chars))))

Furthermore, itertools.chain is smart enough to skip empty generators (if let's say there's no remaining values to fill).

The last for loop (accumulating password from random chars) is simply replaced with str.join call on generator expression:

password = ''.join(choice(char_type.value) for char_type in char_types)

The whole crucial functionality is now shortened to the following:

import string
from enum import Enum
from random import randint, shuffle, choice
from itertools import chain


class PasswordError(Exception):
    pass


class CharTypes(Enum):
    UPPER = string.ascii_uppercase     # Capital
    LOWER = string.ascii_lowercase     # Small
    DIGIT = string.digits              # Digits
    SPECIAL = '!()-.?[]_`~;:@#$%^&*='  # Special


def password_generator(min_length=6, max_length=20, upper=1, lower=1, digits=1, special=1):
    char_counts = (upper, lower, digits, special)
    num_chars = sum(char_counts)   # Number of mandatory characters
    min_len = max(num_chars, min_length)   # In case 'num_chars' is greater

    # If number of characters required for each possible char type
    # is greater than maximum possible length
    if min_len > max_length:
        raise PasswordError(f'No password with the given criteria')

    target_length = randint(min_len, max_length)
    char_types_enums = list(CharTypes)  # get list of enums to pass `random.choice` call

    # List of char "types" comprised of: mandatory requirements + remaining values to fill
    char_types = list(chain(*([c_type] * num for c_type, num in zip(CharTypes, char_counts)),
                            (choice(char_types_enums) for _ in range(target_length - num_chars))))
    shuffle(char_types)

    password = ''.join(choice(char_type.value) for char_type in char_types)
    return password


if __name__ == '__main__':
    ....

Sample usage:

Minimum number of characters required: >? 10
Maximum number of characters possible: >? 30
Number of capital letters required: >? 5
Number of small letters required: >? 4
Number of digits required: >? 6
Number of special characters required: >? 5
Number of passwords required: >? 4
--------------------------------------------------

   Here are your passwords: 

32S%km3A^v04h9pwR-T7O;=0O
mh8a:38Q-pGS3PtGs)e0P1g)$(#0U1
z@a0r;b7v.~K!8S@R343J7L
Mie:8Ec0C=3Cz93HPHDFm_84#;6@
\$\endgroup\$
  • \$\begingroup\$ Yes, treating incoming args as const is often considered good style especially in a function of non-trivial size (and if nothing else makes debugging easier). But the choice of names is problematic here: min_len vs. min_length are visually very similar. A reader looking at some part of the function could easily miss that it was reading a different var name, not the arg. Naming is hard, especially when you need multiple variables with similar semantic meaning/purpose but holding different values. \$\endgroup\$ – Peter Cordes Nov 26 '19 at 7:19
  • \$\begingroup\$ -1 for not using secrets \$\endgroup\$ – Greg Schmit Nov 26 '19 at 16:12
  • \$\begingroup\$ @GregSchmit-ReinstateMonica, You would need to figure out one simple thing - I didn't review a cryptography but the codebase. But at least you've downvoted openly, although not very convincingly, in my opinion \$\endgroup\$ – RomanPerekhrest Nov 26 '19 at 16:26
  • 1
    \$\begingroup\$ AlexV already wrote an answer using secrets (why it has more upvotes), but yours is the accepted answer so it would be good to use secrets on yours as well. I'm sorry you're offended that you didn't know to use that module when generating secrets. \$\endgroup\$ – Greg Schmit Nov 26 '19 at 17:07
  • 2
    \$\begingroup\$ @GregSchmit-ReinstateMonica while recommending secrets would improve the quality, I don't see in this answer where random is specifically recommended. Answers need not cover every issue in every line of the code, and if an answer skips over certain components, I don't think that invalidates the rest of the answer. Is secrets the way to go for the cryptography? Absolutely. Is that relevant for this specific answer? I don't think so, especially since AlexV covered that bit perfectly in their answer \$\endgroup\$ – C.Nivs Nov 26 '19 at 19:51
23
\$\begingroup\$

The other answers have great hints that you should definitely follow.

However, since we are talking about passwords here, a little bit of extra security might not hurt. Especially if it's readily available in the secrets module in Python 3. secrets uses a cryptograhically strong random number generator (in contrast to the pseudo random number generator in the normal random module). There is a special warning in that regard in the docs of random. If you want to stick to random, at least use an instance of random.SystemRandom(), which is basically what secrets does. An example:

RNG = random.SystemRandom()

def password_generator(...):
    ...
    # The remaining values to fill
    type_ = list(types.keys())
    for rem in range(length - num_chars):
        char_list.append(RNG.choice(type_))
        # alternatively
        char_list.append(secrets.choice(type_))
    ...

random.choice uses what is called a pseudo-random number generator, i.e. an algorithm that generates a deterministic "randomly looking" sequence of bytes starting from a given seed. secrets.choice uses a randomness source implemented in the OS, which likely takes electrical noise and other things into consideration to generate non-deterministic random data. random.org has a comprehensive article on the differences at https://www.random.org/randomness/. And of course, there is also the obligatory Wikipedia page about Randomness.

\$\endgroup\$
  • \$\begingroup\$ Great answer! But can I ask why secrets.choice is cryptographically stronger than random.choice? The random library page doesn't quite explain it. \$\endgroup\$ – Srivaths Nov 25 '19 at 9:16
  • 2
    \$\begingroup\$ +1 for secrets. This is important information. Since other answer lack it, this is the best answer. \$\endgroup\$ – Gloweye Nov 25 '19 at 15:29
  • 2
    \$\begingroup\$ Sorry this wasn't the accepted answer. This really was a great answer and a great point, but it mostly improves the output cryptographically, but does not improve the code as a whole which @RomanPerekhrest's answer does. \$\endgroup\$ – Srivaths Nov 25 '19 at 16:39
4
\$\begingroup\$

There are a few things I'd suggest to clean up the code.

First, you should be able to write the following

for typ, val in zip(types.keys(), types.values()):
    char_list.extend([typ] * val)

without using zip by doing as follows

for typ, val in types.items():
    char_list.extend([typ] * val)

Comprehensions

Comprehensions are a great way to clean up.

The first case would be

for rem in range(length - num_chars):
    char_list.append(choice(list(types.keys())))

as

char_list.extend([choice(list(types.keys())) for _ in range(length - num_chars)])

And a second time

password = ''

for typ in char_list:
    password += choice(type_chars[typ])

return password

as

return "".join([choice(type_chars[typ]) for typ in char_list])

Functions

I'd probably put the following piece of code as a separate function to make it more modular and manageable

# List that stores the "types" of possible character
char_list = []

# Mandatory requirements
for typ, val in zip(types.keys(), types.values()):
    char_list.extend([typ] * val)

# The remaining values to fill
for rem in range(length - num_chars):
    char_list.append(choice(list(types.keys())))

shuffle(char_list)

Likewise, with the suggested list comprehension that makes the password.

def make_password(char_list)
    return "".join([choice(type_chars[typ]) for typ in char_list])

Optionals (fancy)

If you want to be very fancy, you can use dataclasses or attrs to package the options to the main function. This would allow you to make some validation of the input, namely, that everything you get are numbers (particularly ints or a string that can be parsed as such). Such a dataclass can be thought of as the communication layer between the front end (whatever is in the __main__ part of the program) and the backend part in generate_password.

I say this because your program will fail if you don't give numbers. For example in the first logic line num_chars = sum(list(types.values())).

\$\endgroup\$
2
\$\begingroup\$

You can generate a password simply with the random module.

import string
import random

ls = string.ascii_letters + string.digits + string.punctuation
length = random.randrange(6, 20) 
pwd = random.choices(ls, k=length)
print('new password: ', ''.join(pwd))

Output:

new password: xsC+.vrsZ<$\
\$\endgroup\$
  • 1
    \$\begingroup\$ Great idea! I should have thought of that! In the case of really needing to use the generator, I'll use this method. But right now, I'm trying to improve my programming skills! :-D \$\endgroup\$ – Srivaths Nov 25 '19 at 13:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.