This is my code to generate and store password localy hashed in txt file. I would like to kindly ask you whether you could review my code. Btw it is coded as console app. I am currently working on making GUI. Thanks in advance.

import random
import hashlib

class randomPass():
    action = ''
    def __init__(self):
        # SETUP
        self.passdic = {}

        self.num_lines = sum(1 for line in open('passwords.txt'))
        with open('passwords.txt', 'r') as rf:
            for i in range(self.num_lines):
                self.content = rf.readline()
                self.content = list(self.content.split())
                self.passdic[self.content[0]] = self.content[1]

        self.leng = 14
        self.action = ''
        self.gPass = ''
        # work as i want to
        self.myPass = ''

        print('perform one of the actions')
        while self.action != 'break':
            print('new action please')
            self.action = input()
            if self.action == 'generate':
                print('determine your generated password length, We recommend above 14 characters')
                self.leng = int(input())
                self.gPass = self.generateRandomPassWord(self.leng)
            elif self.action == 'read':
            elif self.action == 'save':
                print(self.action + 'is not a valid action')

    def generateRandomPassWord(self, lengthOfPassword=14):
        znaky = ['"', "'", '!', '#', '$', '%', '&', '(', ')', '*', '+', ',', '-', '.', '/', '0', '1', '2', '3',
                 '4', '5', '6', '7', '8', '9', ':', ';', '<', '=', '>', '?', '@', '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', '[',
                 ']', '^', '_', '`', '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', '{', '|', '}', '~']

        pword = ''.join([znaky[random.randint(0, len(znaky) - 1)] for i in range(lengthOfPassword)])
        return pword

    def savePass(self, generatedPassword):
        print('to save the generated pass word type your key password')
        self.myPass = hashlib.md5((input().encode('utf-8'))).hexdigest()
        if self.myPass in self.passdic:
            print('your key password already exists!')
            with open('passwords.txt', 'a') as af:

                af.write(self.myPass + ' ' + generatedPassword + '\n')

            self.passdic[self.myPass] = generatedPassword
            print('password stored')

    def passRead(self):
        print('to read the generated pass word type your key password')
        self.myPass = hashlib.md5((input().encode('utf-8'))).hexdigest()
        if self.myPass in self.passdic:
            print('we are sorry your password is non existant')

a = randomPass()

There is no particular need for this to be a class. While the reading of the file and the writing of the file are coupled, IMO this is not yet enough to make it into a class.

What is especially confusing is that you use members even for local variables which are not needed in the other methods.

Now, first, your reading of the file can be greatly simplified:

def read_password_file(file_name="passwords.txt"):
    with open(file_name) as f:
        return {key: password for key, password in map(str.split, f)}

This also allows using a different file than the default. It uses a dictionary comprehension and the fact that files are iterable.

The generation of the random passwords should be separate from the CLI interface (in case you want to change one without the other). The generation itself can also be simplified by using the string module:

import random
from string import ascii_lowercase, ascii_uppercase, digits, punctuation

ALLOWED_CHARS = ascii_lowercase + ascii_uppercase + digits + punctuation

def generate_random_pass_word(length=14, allowed_chars=ALLOWED_CHARS):
    return "".join(random.choices(allowed_chars, k=length))

This uses the random.choices function introduced in Python 3.6.

Your saving and retrieving from the passwords dictionary can become this:

import hashlib

def get_key(name):
    return hashlib.md5(name.encode('utf-8')).hexdigest()

def save_password(passwords, password, file_name="passwords.txt"):
    if password is None:
        print('First generate a password')
    print('to save the generated pass word give it a name')
    name = get_key(input())
    if name in passwords:
        print('your key password already exists!')
        with open(file_name, 'a') as f:
            f.write(f"{name} {password}\n")
        passwords[name] = password
        print('password stored')

def read_password(passwords):
    print('to read the generated pass word type its name')
    name = get_key(input())
    if names in passwords:
        print('we are sorry your password is non existent')

Note that I call your self.myPass what it is, a simple name. It is not really a password (you could look it up in the file and use a rainbow table to get the cleartext) and even the context of having to think of a good password to store your good password seems ridiculous (do you use this password generation function to generate this password? What password do you use to store that password?).

I also used the new f-string (Python 3.6+) to simplify the writing to the file.

And finally, your main function, which has the user interface:

def main():
    pass, action = None, None
    passwords = read_password_file()

    print('perform one of the actions')
    while action != 'break':
        action = input('new action please\n')
        if action == 'generate':
            print('determine your generated password length, We recommend above 14 characters')
            length = int(input())
            pass = generate_random_pass_word(length)
            print('generated:', pass)
        elif action == 'read':
        elif action == 'save':
            save_password(passwords, pass)
            print(action + 'is not a valid action')

if __name__ == "__main__":

Note that I put the calling of main under a if __name__ == "__main__": guard to allow importing from this script from another script.

  • \$\begingroup\$ Thank you! I am planning to rewrite this to a simple GUI with tkinter module. Do you recommen me to put GUI and password generation in separate class? Btw the point behind this is only having to remember your simple password such as your name and you can use it to protect even the most private things. \$\endgroup\$ Nov 28 '18 at 17:41
  • \$\begingroup\$ @MatejNovosad Yes, I would put the GUI, the CLI and the password generation in different functions/classes. \$\endgroup\$
    – Graipher
    Nov 28 '18 at 18:17
  • \$\begingroup\$ @MatejNovosad Well, if your name is your password, then now you have a weak password protecting your strong password, so now the only difference from having a weak password in the first place is some security by obscurity and the weak password being under your control (instead of on someone else's server). You are probably better of in having something like a password vault, which is protected by a single strong password, instead of each password being protected by its own potentially weak password. \$\endgroup\$
    – Graipher
    Nov 28 '18 at 18:20
  • \$\begingroup\$ @MatejNovosad In addition, your logic is slightly wrong in another place. If I manage to read that file, I know all your passwords. They are saved in cleartext. I don't know for which site, but I know that they are passwords you use somewhere. You usually want to encrypt that part, not the identifier. \$\endgroup\$
    – Graipher
    Nov 28 '18 at 18:27
  • \$\begingroup\$ truth is when you manage to get to that file you can use generated passwords you have found in dictionary attack. \$\endgroup\$ Nov 28 '18 at 18:41

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.