7
\$\begingroup\$

This is my first project using Python. I made a simple password generator that checks user input. How can I improve it?

import random
def password_generator():
    password = []
    letters = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u",
               "v", "w", "x", "y", "z"]
    password_length = 0
    password_numbers = []
    password_letters = []
    # Input the length of the password
    while True:
        password_length_input = input("Choose the length of your password with numbers between 6 and 15:\n")
        if not password_length_input.isnumeric():
            print(f"{password_length_input} is not a number, try again:")
            continue
        else:
            password_length = int(password_length_input)
            print(f"Password length: {password_length}")
        if 6 <= password_length <= 15:
            break
        else:
            print("The password must be between 6 and 15 characters, try again:")
            continue
    # Input the amount of numbers in password
    while True:
        password_numbers_input = \
            input(f"Choose the amount of numbers you want in your password, max {password_length}\n")
        if not password_numbers_input.isnumeric():
            print(f"{password_numbers_input} is not a number try again")
            continue
        elif int(password_numbers_input) > password_length:
            password_numbers = 0
            print(f"The value is too high, choose maximum {password_length} numbers")
            continue
        else:
            password_numbers = int(password_numbers_input)
            print(f"Password numbers: {password_numbers}")
            for number in range(0,password_numbers):
                password.append(random.randrange(0,9))
            break
    # Check for numbers and letters in password
    while True:
        if password_numbers == password_length:
            print(f"The password will be only {password_numbers} numbers, no letters.")
            break
        else:
            password_letters = password_length - password_numbers
            print(f"""Your password will be {password_length} characters with {password_numbers} numbers and {password_letters} letters.""")
            for letter in range(0,password_letters):
                password.append(random.choice(letters))
            break
    random.shuffle(password)
    password_string = ''.join([str(item) for item in password])
    print(f"Your password is:\n{password_string}")

password_generator()

Usage example:

Choose the length of your password with numbers between 6 and 15:
Password length: 8
Choose the amount of numbers you want in your password, max 8
Password numbers: 2
Your password will be 8 characters with 2 numbers and 6 letters.
Your password is:
pzc11bmf
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Are you required to ask the user interactively for the 2 pieces of information? I ask because a more typical design for a password creation script would just get the required parameters directly on the command line and then just print the password, with no interactivity at all: python create-password 8 2 \$\endgroup\$
    – FMc
    Sep 11, 2020 at 8:02
  • 1
    \$\begingroup\$ Note: random should not be used for security purposes, but you can use secrets instead; you probably should replace random with secrets.SystemRandom() if generating real passwords or any other cryptographically secure value. \$\endgroup\$
    – 0x2b3bfa0
    Sep 11, 2020 at 12:27

5 Answers 5

10
\$\begingroup\$
letters = ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u",`
           "v", "w", "x", "y", "z"]

This method of writing the alphabet is very error prone. I would import string and use string.ascii_lowercase in place of letters. If you want to generate your own range of letters for whatever reason, I would write

letters = [chr(n) for n in range(ord('a'), ord('z') + 1)]

since there's then no danger of omitting or duplicating a letter.


password_length = 0
password_numbers = []
password_letters = []

These default values are never used. The defaults for password_numbers and password_letters don't make sense since those variables hold numbers. I would delete all three lines.


if not password_length_input.isnumeric():
    print(f"{password_length_input} is not a number, try again:")
    continue
else:
    password_length = int(password_length_input)
    print(f"Password length: {password_length}")

I would instead write

try:
    password_length = int(password_length_input)
except ValueError:
    print(f"{password_length_input} is not a number, try again:")
    continue
print(f"Password length: {password_length}")

while True:
    if password_numbers == password_length:
        ...
        break
    else:
        ...
        break

There is no sense in having a while loop here since you always break out of it on the first iteration.


range(0,password_numbers)

You can just write range(password_numbers).


password.append(random.randrange(0,9))

This will append a digit from 0 to 8 inclusive, never 9. If you want all ten digits you should write random.randrange(10). Or, perhaps better, use random.choice(string.digits).


password_string = ''.join([str(item) for item in password])

If you use string.digits then every element of password will be a character so you can simplify this to password_string = ''.join(password).

\$\endgroup\$
7
\$\begingroup\$

A more straightforward way of generating a random string:

import random
import string

    def get_random_string(length):
        letters = string.ascii_lowercase
        result_str = ''.join(random.choice(letters) for i in range(length))
        print("Random string of length", length, "is:", result_str)
    
    get_random_string(8)
    get_random_string(8)
    get_random_string(6)

borrowed from here, and there are more examples.

Now if you have specific requirements like a minimum number of digits, you can either tweak the formula, or generate two lists and merge them while shuffling the values.

There is an example in the link I quoted above: "Generate a random alphanumeric string with a fixed count of letters and digits" => merging two list comprehensions.

The way you are doing it is procedural but not Pythonic. It is kinda reinventing the wheel.

At the very least, your list of allowed characters should look like this:

letters = "abcdefghijklmnopqrstuvwxyz"

Then you pick out a random letter eg letters[3] will return 'd' since the list is 0-based and Python treats strings as sequences of characters. Using shuffle like you are already doing, you can write more concise code.

\$\endgroup\$
4
\$\begingroup\$

Your password generator is good to start with, here are a few ways you can improve.

  1. Use a more secure method of generating random numbers and letters. The random library that you are currently using is not suitable for generating cryptographic quality random numbers and characters. Instead, you should use the secrets module which is specifically designed for generating secure random numbers.
  2. You could also include special characters like '#', '@', '$' and so on. To include them, you can add them to your list of characters, along with upper-case letters as well, this way it would make the password even more secure.
  3. Another thing to consider is the minimum length of the password, as per security standard, it should be minimum of 8 characters. Also the number of digits, special characters and upper case letters required could also be made a requirement.
  4. Your current code only checks for numeric input for the password length and number of digits but no validation for input of the number of special characters and upper-case letters, this could be added as well.
  5. Finally, you could create a function that checks for the strength of the generated password, and prompt the user to generate a new password if the generated password doesn't meet certain strength criteria.

You can find more information on how to generate secure random numbers in the Python documentation: https://docs.python.org/3/library/secrets.html

Also, you may find it helpful to read the OWASP password storage verifier, it has a lot of information on how to create strong passwords and how to store them: https://owasp.org/www-project-cheat-sheets/cheatsheets/Password_Storage_Cheat_Sheet/

\$\endgroup\$
3
\$\begingroup\$

First

I suggest creating separate methods for each while loops.

Second

"while True" loop is not a good practice, I think. Instead of that, use condition.

Third

I suggest creating PasswordGenerator class which will contain your code. It will help you to expand your code in the future.

Base structure for your project

 class PasswordGenerator():
     
    check_declared_password_length():
        ...
        
    check_amount_of_password_numbers():
        ...
    *
    *
    *

For the end remember to create functions with one responsibility. After that, you can write unit tests for each of that and it will be easier.

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

I definitely don't recommend using random for generating secure passwords. The reason being random is predictable and anyone can guess the next set of passwords that it is going to generate. So it is better to replace random with secrets and use secrets.SystemRandom()

Also, note one more flaw as you are using random.choice() method it can repeat the characters while generating. So if you don’t want to repeat the characters and still want to use random, then use random.sample() method.

If you are looking for a secure and robust password, Python has a module called as secrets, and you could utilize this to generate a random secured password.

The algorithm used by secrets is less predictable when compared to the random string module generation.

import secrets
import string

def generateRandomCryptoPassword(length):
    # Generate random password using cryptographic way
    letters = string.ascii_lowercase + string.digits + string.punctuation
    result_str = ''.join(secrets.choice(letters) for i in range(length))
    print("Strong Password using Crpyto of length", length, "is:", result_str)


generateRandomCryptoPassword(12)

Source : Generate a random string of a given length in Python

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your assertion that all of random is predictable is incorrect. If you read secrets' source then you'll see secrets.SystemRandom is random.SystemRandom. \$\endgroup\$
    – Peilonrayz
    Oct 3, 2021 at 23:41
  • 1
    \$\begingroup\$ what you are saying is partially true, it internally uses random.SystemRandom. but t the same time if you look at the secrets.py they do retrieve a random strings in hex first and they convert into base64 which is more secure. (base64.urlsafe_b64encode(tok).rstrip(b'=').decode('ascii') \$\endgroup\$ Oct 3, 2021 at 23:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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