17
\$\begingroup\$

I have been working on a small project intended for helping beginner Python programmers getting an idea of what a real project could be like (as opposed to programming exercises and such).

This application can create rudimentary user accounts and allow login with password verification. It doesn't do anything beyond that. The user data is stored in a JSON file.

I've written it to be as simple to understand as possible (without commenting each line), and have attempted to keep it to one instruction per line, whenever possible, so please keep that in mind when reviewing, that is done on purpose, I know the code could be made shorter by combining multiple instructions.

I'm interested in all improvements, but in particular, improvements to make the code easier to understand.


Saved user records look like this in JSON file:

[
  {
    "username": "test1",
    "password": "$2b$12$2ksuZeBJsvvEhoTWIdbSReGp753Tb1/cNvlVC2gmjOHRkDeoenl2K",
    "created": "2018-07-07 00:16:28.293901",
    "active": true
  },
  {
    "username": "test2",
    "password": "$2b$12$n80DfPOxo4gIu504o/Yn6.CZCd1aa6s7YDhaFVB7ZJFJpogqIjRc6",
    "created": "2018-07-07 00:28:07.315781",
    "active": true
  }
  ...
]

application.py

This is the launching point, nothing to it, but included for completeness.

from user_interface import *

if __name__ == '__main__':
    main_menu()

encryption.py

This is where I handle encrypting and checking passwords using bcrypt.

import bcrypt


def encrypt_password(password: str) -> str:
    """
    Encrypt a password with a randomly generated salt then a hash.
    Cost rounds added to slow down the process in case of rainbow table/brute force attack.
    :param password: The password to encrypt in clear text.
    :return: The encrypted password as a unicode string.
    """
    encoded_password = password.encode('utf8')
    cost_rounds = 12
    random_salt = bcrypt.gensalt(cost_rounds)
    hashed_password = bcrypt.hashpw(encoded_password, random_salt).decode('utf8', 'strict')
    return hashed_password


def check_password(password: str, password_hash: str) -> bool:
    """
    Check a password against its encrypted hash for a match.
    :param password: the password to check in clear text. (Unicode)
    :param password_hash: The encrypted hash to check against. (Unicode)
    :return: Whether the password and the hash match
    """
    encoded_password = password.encode('utf8')
    encoded_password_hash = password_hash.encode('utf8')
    password_matches = bcrypt.checkpw(encoded_password, encoded_password_hash)
    return password_matches


if __name__ == '__main__':
    test_password = 'Password1'
    hashed_test_password = encrypt_password(test_password)
    print(f'hashed_password: {hashed_test_password}')
    password_matches_hash = check_password(test_password, hashed_test_password)
    print(f'password matches hash? {password_matches_hash}')

json_handling.py

This module handles file operations, specifically on JSON files.

import json
from pathlib import Path
from typing import List, Union


def create_file_if_not_exists(file_path: str) -> None:
    """
    Checks if a file exists at the given path, and creates it if it doesn't.
    :param file_path: the path of the file to check/create, which can be relative or absolute.
    :return: None
    """
    Path(file_path).touch()


def get_json_file_contents(file_path: str) -> Union[List, None]:
    """
    Reads and return the contents of a JSON file.
    :param file_path: The path where the JSON file is located.
    :return: The contents of the file, or None if there if the file is empty or not found.
    """
    try:
        json_file = open(file_path)
    except IOError:
        return None
    try:
        file_contents = json.load(json_file)
    except ValueError:
        file_contents = None
    json_file.close()
    return file_contents

user_storage.py

This module is responsible for various tasks specifically related to handling creating and retrieving user data from the JSON file.

import datetime
from typing import Dict
from encryption import *
from json_handling import *


DEFAULT_FILE_PATH = 'data/users.json'


def prepare_new_user_data(username: str, password: str) -> Dict:
    """
    Return user data ready for storage.
    :param username: The username for this user.
    :param password: The password for this user, in clear text.
    :return: A Dict containing user data ready to store, including encrypted password.
    """
    new_user = {
        'username': username,
        'password': encrypt_password(password),
        'created': str(datetime.datetime.now()),
        'active': True
    }
    return new_user


def check_if_user_already_exists(username: str, json_file_path: str=DEFAULT_FILE_PATH) -> bool:
    """
    Queries a JSON file and returns whether it already exists.
    :param username: The username to check for duplication.
    :param json_file_path: The path where the JSON file is located.
    :return: Whether the username already exists.
    """
    all_users = get_json_file_contents(json_file_path)
    if not all_users:
        return False
    for user in all_users:
        if user['username'] == username:
            return True
    return False


def add_user(username: str, password: str, json_file_path: str=DEFAULT_FILE_PATH) -> None:
    """
    Adds a user to a JSON file, unless it is a duplicate, in which case it raises a ValueError.
    :param username: The username of the user to add.
    :param password: The password of the user to add, in clear text.
    :param json_file_path: The path where the JSON file to add the user to is located.
    :return: None
    """
    create_file_if_not_exists(json_file_path)
    is_duplicate_user = check_if_user_already_exists(username, json_file_path)
    if is_duplicate_user:
        raise ValueError(f'Username "{username}" already exists.')
    new_user = prepare_new_user_data(username, password)
    all_users = get_json_file_contents(json_file_path)
    if not all_users:
        all_users = []
    all_users.append(new_user)
    with open(json_file_path, 'w') as users_file:
        json.dump(all_users, users_file, indent=2)


def retrieve_user(username: str, json_filepath: str=DEFAULT_FILE_PATH) -> Union[Dict, None]:
    """
    Returns a single user record from the target JSON file.
    :param username: the username to search for.
    :param json_filepath: The path where the JSON file to retrieve the user from is located.
    :return: The user record as a Dict, or None if it is not found.
    """
    all_users = get_json_file_contents(json_filepath)
    for user in all_users:
        if user['username'] == username:
            return user
    return None


def authenticate_username_and_password(username: str, password: str) -> bool:
    """
    Verify that the provided username and password match what is stored in the user data,
    for authentication purposes.
    :param username: The user's username.
    :param password: The user's password, in clear text.
    :return: Whether the authentication was successful.
    """
    user = retrieve_user(username)
    password_hash = user['password']
    if not user:
        return False
    if not check_password(password, password_hash):
        return False
    return True


if __name__ == '__main__':
    test_username = 'test1'
    test_password = 'Password1'
    print(prepare_new_user_data(test_username, test_password))
    test_file_path = 'data/test_database.json'
    create_file_if_not_exists(test_file_path)
    add_user(test_username, test_password, test_file_path)
    print(get_json_file_contents(test_file_path))

user_interface.py

This module handles interaction between the application and the user, which is done right now through the console.

import getpass
import re
from user_storage import *


def main_menu() -> None:
    """
    Displays the main menu of the application.
    :return: None
    """
    menu = '\n'.join([
        'Select an option by entering its number and pressing Enter.',
        '1. Create a user account',
        '2. Log in to existing account',
        '---'
    ])
    print(menu)
    valid_selections = [1, 2]
    input_is_valid = False
    selection = None
    while not input_is_valid:
        try:
            selection = int(input('Selection: '))
            if selection in valid_selections:
                input_is_valid = True
            else:
                print('The number you entered is not a valid selection.')
        except ValueError:
            print('The value you entered is not a number.')
    handle_main_menu_selection(selection)


def handle_main_menu_selection(selection: int) -> None:
    """
    Calls the function related to the selection the user made.
    :param selection: The user's selection.
    :return: None
    """
    if selection == 1:
        create_new_user_menu()
    elif selection == 2:
        user_login_menu()
    else:
        raise ValueError(f'Selection {selection} is invalid.')


def create_new_user_menu() -> None:
    """
    Displays the account creation menu, including asking the user for username and password.
    :return: None
    """
    menu = '\n'.join([
        '---',
        'Account creation',
        'Username must...',
        '\t- be at least 3 characters long',
        '\t- contain only letters, numbers, and underscores',
        'Password must...',
        '\t- be at least 8 characters long',
        '---'
    ])
    print(menu)
    user_added_successfully = False
    username = ''
    while not user_added_successfully:
        try:
            username = get_username_input()
            password = get_password_input()
            user_added_successfully = try_adding_user(username, password)
            if not user_added_successfully:
                print(f'Username "{username}" already exists.')
        except ValueError as error:
            print(str(error))


def try_adding_user(username: str, password: str) -> bool:
    """
    Attempts to add a user to the user database file.
    :param username: The username provided by the user.
    :param password: The password provided to the user, in clear text.
    :return: Whether the user was added successfully.
    """
    try:
        add_user(username, password)
        return True
    except ValueError:
        return False


def user_login_menu() -> None:
    menu = '\n'.join([
        '---',
        'User login',
        '---'
    ])
    print(menu)
    login_successful = False
    while not login_successful:
        username = get_username_input()
        password = get_password_input()
        login_successful = authenticate_username_and_password(username, password)
        if not login_successful:
            print('Incorrect username or password.')
    print('Login successful.')


def get_username_input() -> str:
    """
    Request username input from the user.
    :return: The username entered by the user.
    """
    minimum_length = 3
    username = input('Enter username: ')
    if len(username) < minimum_length:
        raise ValueError('Username must be at least 3 characters.')
    # match upper & lower case letters, numbers, and underscores
    pattern = re.compile('^([a-zA-Z0-9_]+)$')
    if not pattern.match(username):
        raise ValueError('Username must consist of only letters, numbers, and underscores.')
    return username


def get_password_input() -> str:
    """
    Request password input from the user.
    :return: The password entered by the user.
    """
    minimum_length = 8
    password = getpass.getpass('Enter password: ')
    if len(password) < minimum_length:
        raise ValueError('Password must be at least 8 characters.')
    return password


if __name__ == '__main__':
    main_menu()
\$\endgroup\$
  • 7
    \$\begingroup\$ (Not enough for an answer) Encrypt != Hash. I think it's important for anyone working with passwords to understand that as early as possible in the process. \$\endgroup\$ – Gerrit0 Jul 8 '18 at 5:56
  • \$\begingroup\$ I'm listening, what am I missing? \$\endgroup\$ – Phrancis Jul 8 '18 at 6:39
  • \$\begingroup\$ Jörg's answer below covers this - though it's rather optimistic regarding "impossible" \$\endgroup\$ – Gerrit0 Jul 8 '18 at 14:17
  • \$\begingroup\$ @Phrancis an encryption can be decrypted if you know the method. A Hash value can only be checked upon but not de-hashed. Even if you know the hashing function, you still need to guess the password first and apply the hash function upon the password that you guess and then check if the hash values are the same. \$\endgroup\$ – user1841243 Jul 9 '18 at 14:31
  • \$\begingroup\$ I understand now, thank you. And the salt makes sure the hash is randomized so even if you had all the passwords, you couldn't look for duplicate hashes to guess they have the same password, for example. \$\endgroup\$ – Phrancis Jul 10 '18 at 2:52
16
\$\begingroup\$

Avoid wildcard imports

from ... import * is not recommended. Especially when there are imports from multiple packages, it makes it difficult to know where some symbol comes from. If you find it tedious to spell out the package names when using functions, consider importing with a shorter alias.

Always use context manager when opening files

You did use with open(...) in some places, but not in get_json_file_contents. Maybe I'm missing something, but I suggest to rewrite this with a context manager.

Return empty objects instead of None when possible

Callers of get_json_file_contents must check for None before looping over the returned value. If you don't mind changing get_json_file_contents to return an empty list, then callers can become simpler.

Note that retrieve_user doesn't check for None before using the returned value. That looks like a bug.

Logical error

There is a logical error here:

user = retrieve_user(username)
password_hash = user['password']
if not user:
    return False

The if statement should come before accessing user['password'], otherwise it doesn't make sense, and probably a bug.

Style

This is not Pythonic:

if not check_password(password, password_hash):
    return False
return True

It would be better to use the boolean expression directly:

return check_password(password, password_hash)

Readability

I think it would be better to add more blank lines to visually separate bigger logical steps. In particular, in add_user. Currently it looks like a long blob of code.

Some of the names are a bit too long for my taste, with redundant terms. For example in check_if_user_already_exists the term "already" is definitely redundant. You will probably not like it, but I would reduce this one to user_exists.

\$\endgroup\$
  • 1
    \$\begingroup\$ In fact, wouldn't "check_password" be better called something like "is_password_valid" of it is meant to be used in if checks \$\endgroup\$ – Gnudiff Jul 8 '18 at 15:43
12
\$\begingroup\$

Your code is confusing, since it refers to encryption in multiple places, e.g. the filename encryption.py, the function name encrypt_password, and multiple times in the docstrings for both encrypt_password and check_password, yet, the code has nothing to do with encryption.

In fact, storing passwords in encrypted form is one of the cardinal sins of information security, so if your code were doing what it claims to do, it would be fundamentally broken.

Thankfully, your code doesn't encrypt the passwords, it salts and hashes them … but then why doesn't it say so? The distinction between hashing (which is impossible to reverse) and encryption (whose sole purpose is that it can be reversed, otherwise it would be useless) is fundamentally important in Information Security, so confusing the reader of your code, especially if they are not experienced cryptologists, is not a good idea.

As a sidenote, bcrypt is no longer considered state-of-the-art in password-based key-derivation functions. It can too easily be sped up using GPGPUs, ASICs, and FPGAs. Consider using a more modern algorithm such as Argon2 (which seems to be the current consensus in the Information Security community, as far as I can tell).

\$\endgroup\$
  • \$\begingroup\$ I set the bcrypt cycles to 2^12 to slow it down, it takes roughly 0.5 second to hash a password, does that help mitigate the speeding up? \$\endgroup\$ – Phrancis Jul 8 '18 at 21:48
  • \$\begingroup\$ I've updated my code to use Argon2 now, thank you for the recommendation. I've also renamed things to hash instead of encrypt. \$\endgroup\$ – Phrancis Jul 9 '18 at 0:14
8
\$\begingroup\$

This is also not a very good design from the viewpoint of algorithmic complexity. Whenever you want to check if a user already exists, or to retrieve a user, you need to read the whole file content. Then you need to iterate over the list of users comparing the username until you do (or do not) find the user you are looking for. This just screams for a hashmap/dictionary, with the username being the key and the user the value, instead of a list. You instantly get \$\mathcal{O}(1)\$ behavior instead of \$\mathcal{O}(N)\$.

At least theoretically, practically having to read the whole file still gives you \$\mathcal{O}(N)\$ behavior. Therefore I would normally consider making a database class, which just happens to store its data in JSON, keeping the dictionary in memory and only reading it upon "connecting" to the DB, but writing whenever a user is added.

That might be too much for those new users, though, and a bundle of functions can also work.


If the user enters an invalid username or password in user_login_menu, the resulting ValueError is not caught. It should probably just return with an unsuccessful login in that case (and thereby not reveal anything about what requirements usernames and passwords need to fulfill).

Also

menu = '\n'.join([
    '---',
    'User login',
    '---'
])
print(menu)

is arguably less readable than just doing

print('---')
print('User login')
print('---')
\$\endgroup\$
  • 2
    \$\begingroup\$ About that print, triple quoted strings (and optionally textwrap.dedent) is even more readable. \$\endgroup\$ – 409_Conflict Jul 8 '18 at 17:19
  • \$\begingroup\$ Wow I wish I knew about textwrap.dedent long ago, that's so handy, thank you for suggesting it @MathiasEttinger! \$\endgroup\$ – Phrancis Jul 11 '18 at 4:01
7
\$\begingroup\$
  • Testing for not allUsers in check_if_user_already_exists is superficial. If so, the loop is effectively a no-op.

  • I recommend get_json_file_contents to return [] instead of None. This way you don't need to test for not allUsers, this time in addUser.

  • The validation of username length and composition certainly does not belong to UI. It is a policy, so consider moving it to add_user (or try_add_user). Besides, it does not look right to test it in the login phase: invalid username is guaranteed to not be found.

    Ditto for password validation.

  • I don't see the reason for try_adding_user to exist. It only intercepts ValueError raised by add_user, which in turn complicates the logic of create_new_user_menu.

  • handle_main_menu_selection right to exist is also questionable. At least it creates a multiple maintenance problem: to add another selection you need to edit code in two places. Consider an array of lambdas to be called directly by main_menu.

  • I don't see how handle_main_menu_selection may ever raise. The selection is guaranteed to be valid.

\$\endgroup\$

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.