# User login database intended for beginners

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:

[
{
"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__':


### encryption.py

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

import bcrypt

"""
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.
:return: The encrypted password as a unicode string.
"""
cost_rounds = 12
random_salt = bcrypt.gensalt(cost_rounds)

"""
Check a password against its encrypted hash for a match.
:param password_hash: The encrypted hash to check against. (Unicode)
:return: Whether the password and the hash match
"""

if __name__ == '__main__':


### 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:
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'

"""
Return user data ready for storage.
:return: A Dict containing user data ready to store, including encrypted password.
"""
new_user = {
'created': str(datetime.datetime.now()),
'active': True
}
return new_user

"""
Queries a JSON file and returns whether it already exists.
:param json_file_path: The path where the JSON file is located.
"""
all_users = get_json_file_contents(json_file_path)
if not all_users:
return False
for user in all_users:
return True
return False

"""
Adds a user to a JSON file, unless it is a duplicate, in which case it raises a ValueError.
: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)
if is_duplicate_user:
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 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:
return user
return None

"""
Verify that the provided username and password match what is stored in the user data,
for authentication purposes.
:return: Whether the authentication was successful.
"""
if not user:
return False
return False
return True

if __name__ == '__main__':
test_file_path = 'data/test_database.json'
create_file_if_not_exists(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 *

"""
Displays the main menu of the application.
:return: None
"""
'Select an option by entering its number and pressing Enter.',
'1. Create a user account',
'---'
])
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.')

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

"""
:return: None
"""
'---',
'Account creation',
'\t- be at least 3 characters long',
'\t- contain only letters, numbers, and underscores',
'\t- be at least 8 characters long',
'---'
])
try:
except ValueError as error:
print(str(error))

"""
Attempts to add a user to the user database file.
:return: Whether the user was added successfully.
"""
try:
return True
except ValueError:
return False

'---',
'---'
])

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

"""
Request password input from the user.
:return: The password entered by the user.
"""
minimum_length = 8
raise ValueError('Password must be at least 8 characters.')

if __name__ == '__main__':

• (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. – Gerrit0 Jul 8 '18 at 5:56
• I'm listening, what am I missing? – Phrancis Jul 8 '18 at 6:39
• Jörg's answer below covers this - though it's rather optimistic regarding "impossible" – Gerrit0 Jul 8 '18 at 14:17
• @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. – user1841243 Jul 9 '18 at 14:31
• 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. – Phrancis Jul 10 '18 at 2:52

### 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)
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)


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.

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

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).

• 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? – Phrancis Jul 8 '18 at 21:48
• I've updated my code to use Argon2 now, thank you for the recommendation. I've also renamed things to hash instead of encrypt. – Phrancis Jul 9 '18 at 0:14

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([
'---',
'---'
])


is arguably less readable than just doing

print('---')
print('---')

• About that print, triple quoted strings (and optionally textwrap.dedent) is even more readable. – 409_Conflict Jul 8 '18 at 17:19
• Wow I wish I knew about textwrap.dedent long ago, that's so handy, thank you for suggesting it @MathiasEttinger! – Phrancis Jul 11 '18 at 4:01
• 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.

• 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.