8
\$\begingroup\$

After almost completely redoing the first version I think I finally have it. I still have a few things to add/change, but I think it's pretty good and safe. I've improved security features as well as overall functionality.

I haven't run this program through pycharm yet, so there might be a few PEP 8 errors. It took me a few weeks just trying to learn about cryptography (not an easy subject). For @GarethRees post, I really didn't even start on version 2 until I implemented some sort of encryption that worked perfectly on Version 1. I tried using gpg as he recommended, but could never figure it out. I even tried it again after figuring out how to use pycrypto.

If there are any bugs just point them out you don't have to write a full review just whatever you think should be changed or added post it. I really appreciate feedback and will upvote all. If you want a .exe version just ask and I'll post a link. Hope everyone likes it.

#Programmer: DeliriousSyntax
#Date: 12-9-15
#File: AccountKeeperV2.py

#This program lets you store and create passwords

import CustomFunctions as CF
import pyperclip
import random
import shelve
import EcstaticCryption
import string

#Encrypting and Decrypting is done with pycrypto

"""
Task List:
Hide key after hitting enter
Create a settings GUI
Let user continuously generate a password untill satisfied
When changing password let user pick a new random password
Use string.punctuation in character list without messing up printing
Add Exclude similar characters option
Let user save settings without opening script
"""


class main:
    LOWER = True
    UPPER = True
    NUMBERS = True
    SYMBOLS = True
    COPY = True

    File = "Keeper.dat"

    template = ('\n-  Account:  {}  '
                '- Username:  {}  '
                '-  Password:  {} '
                ' -')

    key = input('Key: ')
    EC = EcstaticCryption.AESCipher(key)

    @property
    def CHARACTERS(self):
        CHAR = []
        if self.LOWER:CHAR.append(string.ascii_lowercase)
        if self.UPPER:CHAR.append(string.ascii_uppercase)
        if self.SYMBOLS:CHAR.append('!#$%&()*+,-.:;<=>?@[]^_`{|}~')
        if self.NUMBERS:CHAR.append(string.digits)
        return CHAR

    def all_accounts(self):
        with shelve.open(self.File) as f:
            print('\n')
            for account in f:
                print(account, end='  ')
            print('\n')

    def all_users(self, account):
        with shelve.open(self.File) as f:
            print('\n')
            for user in f[account]:
                print(user, end='  ')
            print('\n')

    def check_account(self, account):
        list_of_accounts = []
        with shelve.open(self.File) as f:
            if account in f:
                return True
            else:
                return False

    def check_username(self, account, username):
        list_of_users = []
        with shelve.open(self.File) as f:
            try:
                if username in f[account]:
                    return True
                else:
                    pass
            except KeyError:
                pass
            return False

    def generate_password(self, digits_in_pass):
        holder = []
        for _ in range(digits_in_pass):
            temp = random.SystemRandom().choice(self.CHARACTERS)
            holder.append(random.SystemRandom().choice(temp))
        return ''.join(holder)

    def save_account(self, entry):
        with shelve.open(self.File) as f:
            try:
                holder_account = f[entry[0]]
            except KeyError:
                holder_account = {}
            holder_account[entry[1]] = entry
            f[entry[0]] = holder_account

    def new_account(self, log=True):
        """Creates a new random password"""
        print('\n')
        account = input("Account: ")
        with shelve.open(self.File) as f:
            while True:
                username = input("Username: ")
                existing_username = self.check_username(account, username)
                if not existing_username:
                    break
                print("This account already exists!")
        password = input("Password (Type \"random\" for random password): ")
        if password.lower() == 'random':
            digits_in_pass = CF.valid_int("Length of password: ")
            password = self.generate_password(digits_in_pass)
        encrypted_password = self.EC.encrypt(password)
        entry = [account, username, encrypted_password]
        self.save_account(entry)
        if self.COPY:
            pyperclip.copy(password)
        if log:
            print(self.template.format(entry[0], entry[1], password))

    def print_account(self, account, username):
        with shelve.open(self.File) as f:
            entry = f[account][username]
            password = self.EC.decrypt(entry[2])
            print(self.template.format(entry[0], entry[1], password))
            if self.COPY:
                pyperclip.copy(password)

    def change_username(self, account, username, log=True):
        new_username = input('\nEnter new username: \n   ->')
        with shelve.open(self.File) as f:
            account_holder = f[account]
            account_holder[new_username] = account_holder.pop(username)
            account_holder[new_username][1] = new_username
            f[account] = account_holder
            if log:
                entry = f[account][new_username]
                password = self.EC.decrypt(entry[2])
                print(self.template.format(entry[0], entry[1], password))

    def change_password(self, account, username, log=True):
        new_password = input('Enter new password: \n   ->')
        with shelve.open(self.File) as f:
            f[account][username][2] = self.EC.encrypt(new_password)
            if log:
                entry = f[account][username]
                print(self.template.format(entry[0], entry[1], new_password))

    def delete_account(self, account, username):
        confirmation = input("\nType 'DELETE' to confirm deletion of this account...\n   ->")
        if confirmation.lower() == 'delete':
            with shelve.open(self.File) as f:
                account_holder = f[account]
                try:
                    del account_holder[username]
                    f[account] = account_holder
                    print('\nAccount deleted...')
                except KeyError:
                    print('Error deleting account!')

    def account_menu(self, account, username):
        print("\nAccount Found! What's next?\n"
        " 1) Print account\n"
        " 2) Change username\n"
        " 3) Change password\n"
        " 4) Delete account\n"
        " 5) Cancel")
        account_choice = input("   ->")
        if account_choice == '1':
            self.print_account(account, username)
        elif account_choice == '2':
            self.change_username(account, username)
        elif account_choice == '3':
            self.change_password(account, username)
        elif account_choice == '4':
            self.delete_account(account, username)
        else:
            pass

    def find_account(self):
        account = input('\n\nAccount: ')
        while account == 'all accounts':
            self.all_accounts()
            account = input('Account: ')
        existing_account = self.check_account(account)
        if existing_account:
            username = input('Username: ')
            while username == 'all users':
                self.all_users(account)
                username = input('Username: ')
            existing_user = self.check_username(account, username)
            if existing_user:
                self.account_menu(account, username)
            else:
                print('\nWe could not find your account.')
        else:
            print('\nCould not find any {} accounts.'.format(account))

    def program_start(self):
        """MAIN"""
        choice = None
        while True:
            print("\n\n\nMenu:\n"
                  " 1) Add an account\n"
                  " 2) Search for an existing account\n"
                  " 3) Exit")
            choice = input("   ->")
            if choice == "1":
                self.new_account()
            elif choice == "2":
                self.find_account()
            else:
                break

if __name__ == '__main__':
    print("Welcome to Account Keeper V2")
    m = main()
    m.program_start()
\$\endgroup\$
2
  • \$\begingroup\$ Don't forget to accept one of the answers, or comment on why you don't find any of the current answers good enough for acception. \$\endgroup\$ Commented Feb 28, 2016 at 18:43
  • \$\begingroup\$ @holroy Oh yeah thanks, just been busy and haven't check in on code review in a while. Between school, work, gym, and trying to create a big project to get noticed by employers it's hard to find time. \$\endgroup\$ Commented Feb 28, 2016 at 18:50

2 Answers 2

Reset to default
2
\$\begingroup\$

I've seen your question and would like to give you a full review, but haven't had the time yet. So now I decided to give you some of the recommendations, and then we'll see if I get the time to complete it or add to it later on.

  • Move shelve handling to class level – Instead of opening/closing the shelve all the time, I would suggest to let it follow the lifetime of the class, and add a sync method to the class. This would reduce the file operations, and you can keep the shelve open within the class.

    This would allow for all of the read access to be almost instant as the file is already read into memory (and the encrypted password should still be encrypted). And when doing changing password methods, you could trigger a sync, writing the shelve back to the file.

    Two links which might be helpful in doing this:

  • A better class name – main is a terrible name, you should choose a better name like PasswordHandler or AccountHandler. As already suggested I would move the shelve handling to the outer level, and possibly add a few class methods. I.e. a static class method to verify if a user has entered the correct password would be nice to have.

  • Allow methods not to use input – To improve the interface, I would change the methods allowing input to accept that input to be predefined as a parameter. I'll show some code for one of the functions which could accomplish this:

    def change_password(self, account, username, new_password=None, log=True):
        if not new_password:
            new_password = input('Enter new password: \n   ->')

        with shelve.open(self.File) as f:
            f[account][username][2] = self.EC.encrypt(new_password)
            if log:
                entry = f[account][username]
                print(self.template.format(entry[0], entry[1], new_password))

    This still uses the old code for the shelve handling, but you'll get the gist of the idea.

  • Let methods return list, not do the actual output – Instead of letting your methods, like all_users, do the print, in general it's better to let the method returning the list of users for extra handling, i.e. printing or sorting before printing or whatever...

  • Move class level code into __init__ – Doing stuff like key = input(...) is very strange, and should be moved into a method of it's own, namely the __init__, which is automatically called whenever you create an instance of your class.

There are some other minor aspects still not covered (neither in mine, nor the other answer which just came in), but these matters are some of the main issues I see in your code.

\$\endgroup\$
2
  • \$\begingroup\$ I thought __init__ was for items that would change and not constants.? \$\endgroup\$ Commented Dec 27, 2015 at 21:15
  • \$\begingroup\$ @DeliriousSyntax, __init__ is for class instanstation cases, yes. Having rogue code executed at import time is not good coding strategy. If you want them as class constants, then you should consider using static methods or similar. But having a input() at class level, which could be triggered wherevere, is a real code smell \$\endgroup\$ Commented Dec 27, 2015 at 21:18
2
\$\begingroup\$

This looks pretty good - I'd say the biggest opportunities for improvement are in style, ease of i18n, and documentation (I have no idea if any of these have been mentioned in a previous post, as I haven't read it).

I'm going to start with documentation - generally, most of your methods and parameter names are fairly straightforward, however if you write useful docstrings (I appreciate the numpydoc standard) you can use a tool like sphinx to create documentation (if you think anyone else might ever want to use your tool as a developer). Even if you don't think anyone else will ever program with this, writing documentation will help you understand it later.

On that note, your module as a whole should probably have a brief docstring describing its overall purpose.

I don't know that I like how hardcoded some things are (for example LOWER, UPPER, File, etc). It would be nice if these could be set somewhere, or passed as CLA, or in a config file, etc.

In check_account you don't ever use list_of_accounts, and you could just return account in f instead of having the if statements. Likewise, in check_username you don't use list_of_users, and you could probably simplify the conditions. I'm not familiar with shelve, but assuming it quacks like a dict you could just do return username in f.get(account, {}) and then avoid the try-except, the if-else, etc. If you don't want to do that, you don't need the else:pass at all.

You could probably turn generate_password into a list comprehension.

def generate_password(self, digits_in_pass):
    func = random.SystemRandom().choice
    return ''.join(func(func(self.CHARACTERS)) for _ in digits_in_pass)

This should be equivalent.

In save_account you could (presumably) use f.get() instead of the try-except.

In all of the places you use input I'd rather see you take that input as a parameter - this separates concerns in case you want to stop asking on the command line, and lets you build out whatever front-end you want.

In new_account you can use single quotes instead of escaping the double quotes, which is cleaner IMHO. I'd also strip the string in case they have whitespace on the ends, and if they have something close to random (use difflib) maybe emit a warning in case they misspelled it.

You have lots of magic numbers - maybe instead of indexing at 0, 1, and 2, use ACCOUNT_INDEX = 0 (or something). Then if you change the order, or use a new scheme, its easier to change it.

program_start shouldn't be a part of the class - it should be another function that makes use of the class.

Internationalization (i18n) is important - what if someone wanted to use a different language? I'd say that you should create a class (or something) that has all of the string literals you use as properties, and then returns them based on some locale or language that has been set. Then you can add translations easily, if you wanted to expand your userbase.

In terms of style - I don't really like the onelineing in CHARACTERS - you aren't losing points for adding some newlines, or at least spaces.

Your naming could use a little work - a class named main is not even a little bit helpful. I'd recommend something like AccountKeeper or PasswordManager or something like that. CHARACTERS is also not as helpful - maybe VALID_CHARACTERS would be better? Also, File isn't as helpful - either make it a constant as FILE or a variable as file.

all_accounts implies that you should get a list or something - calling it display_all_accounts is more honest - same for all_users.

Constants should be named in UPPER_SNAKE_CASE, classes in PascalCase, method/function/variable names in lower_snake_case. You can read more about naming and style conventions in PEP8

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.