4
\$\begingroup\$

I am going to make this code below into the second release of my library, but I want to make sure I haven't missed anything. If there are improvements that can be made, I'd like to implement them.

This code is also on GitHub.

import random
import secrets
import string
import urllib.request

class Simple():
    def __init__(self, length: int, characters = None):
        '''A simple password'''
        self.length = length
        self.characters = characters
        self.output = []

    def generate(self, num_of_passwords: int):
        '''
        Generates a password depending on the num_of_passwords
        and the arugments provided in the simple class
        '''
        characters = ''
        if self.characters is None:
            characters = string.ascii_letters + string.digits
        else:
            characters = self.characters

        for i in range(num_of_passwords):
            password = ''
            for c in range(self.length):
                password += secrets.choice(characters)
            self.output.append(password)
        return self.output

    def return_result(self, index: int):
        '''
        Returns the password which is at the specified index
        in the output list.
        '''
        try:
            return self.output[index]
        except IndexError:
            print(f'Incorrect index specified. Please provide an index relevant to the number of passwords generated')

    def clear_results(self):
        '''Clears the output list if you want to make way for new passwords'''
        self.output.clear()

class Complex(Simple):
    def __init__(self, length, string_method, numbers=True, special_chars=False):
        '''
        Creates a customisable password depending on length,
        string_method, numbers and special_chars
        '''
        characters = ''
        self.output = []

        methods: dict = {
            "upper": string.ascii_uppercase,
            "lower": string.ascii_lowercase,
            "both": string.ascii_letters,
        }

        characters += methods[string_method]

        if numbers:
            characters += string.digits
        if special_chars:
            characters += string.punctuation

        super().__init__(length=length, characters=characters)

class Memorable(Simple):
    def __init__(self, numbers=True):
        '''A memorable password'''
        self.numbers = numbers
        self.output = []

    def generate(self, num_of_passwords: int):
        '''Gets some random words'''
        word_url = "http://svnweb.freebsd.org/csrg/share/dict/words?view=co&content-type=text/plain"
        headers = {'User-Agent': 'Mozilla/5.0 (Windows NT 6.1; Win64; x64)'}
        req = urllib.request.Request(word_url, headers=headers)
        response = response = urllib.request.urlopen(req)
        long_txt = response.read().decode()
        words = long_txt.splitlines()

        '''
        Generates the password containing 2 words
        and numbers if self.numbers == True
        '''
        for i in range(num_of_passwords):
            password = ''
            two_words = ''
            for i in range(2):
                two_words += secrets.choice(words).title()
            password = two_words
            if self.numbers == True:
                for i in range(random.randint(3, 4)):
                    password += secrets.choice(string.digits)
            self.output.append(password)
        return self.output

# Test Scenarios

if __name__ == "__main__":
    var = Memorable()
    print(var.generate(3))

\$\endgroup\$
2
  • \$\begingroup\$ You're getting your dictionary from the web? That's one of the first places I would attack this, through DNS spoofing or flat-out interception. Don't you have a standard system wordlist (e.g. /usr/share/dict/words) that you could use instead? \$\endgroup\$ Commented Mar 15, 2021 at 17:01
  • \$\begingroup\$ well i wasnt sure if making a package allowed you to have files other than the ones required to make a library work. sorry :/ how can i implement a standard system wordlist? \$\endgroup\$ Commented Mar 15, 2021 at 17:16

1 Answer 1

3
\$\begingroup\$

1. Add even more typing

It's good that you use some basic type annotations but you should use them everywhere, not just on a few select parameters. Have a look at the typing module, especially if you want to support Python versions before 3.9. For example, your function signatures could be

def __init__(self, length: int, characters: Optional[str] = None):
def generate(self, num_of_passwords: int) -> None:
def __init__(self, length: int, string_method: Literal["upper", "lower", "both"], numbers: bool = True, special_chars: bool = False):

2. Keyword-only parameters

This is a personal suggestion and I am sure you will find people who disagree with me on it. In your current code, Complex(5, "lower", True, False) is a valid call; yet, it is completely meaningless to a reader. By making the last two parameters keyword-only, you force the user to be more expressive: Complex(5, "lower", numbers=True, special_chars=False).

On that note, I suggest to give the parameters more expressive names such as include_numbers and include_special_characters.

3. Add better docstrings

You should ask yourself: who do you write your code documentation for? Right now, comments like "A simple password" don't provide much helpful information to anyone except for maybe yourself. What makes a password "simple"? Does it only contain numbers and letters? Is it short? Is it easy to remember? The same holds true for "complex", "memorable" and other terms. I'll do a shameless plug of my own recent blog post on that topic for more information.

4. Security

In the context of "passwords", "security" should always be mentioned, even if you feel that for your particular purpose you don't actually need it. Toby Speight already commented that getting words from a dictionary in the web with a hardcoded URL gives a pretty good target for attackers. An easy and platform-independent workaround would be to include a complete word dictionary in your package itself so there is no need to get it via HTTP.

Another issue might come up due to the fact that random, which you use to build your passwords, is not cryptographically secure. The secrets module, which you also used, is already a better alternative.

5. Enums

This point, again, might be a bit controversial. Instead of using "method" strings, "upper", "lower", and "both", I'd suggest to use an enum class instead, which makes it easier for your IDE to help you and gives you errors more quickly should you try to use a value that does not exist.

class PasswordGenerationStrategy(enum.Enum):
  Upper = "upper"
  Lower = "lower"
  Both = "both"
\$\endgroup\$

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.