14
\$\begingroup\$

I wrote this login system in the evening and i would like to know what should I fix / change, if should I use classes and, about readability, if this code is OK. I'm a begginer in coding so made this small project only for exercising, but good to know wether if i'm things doing right or wrong.

import os.path

credentials = []
database_path = "C:\\Users\\filip\\PycharmProjects\\database"
database = os.listdir(database_path)


def initialize():
    choose_option = int(input('0. Exit\n1. Login\n2. Sign up\n\nOption: '))
    if choose_option == 1:
        login_func()
    elif choose_option == 2:
        get_username(), get_password(), add_credentials_to_database()
    elif choose_option == 0:
        exit('Exiting...')
    else:
        print("You should enter '0', '1', or '2'.")
        initialize()
        

def get_username():
    username = input('Enter username: ')
    if f'{username}.txt' not in database:
        if len(username) >= 8:
            credentials.append(username)
        else:
            print('Username too short (min 8 characters)\n\n')
            get_username()
    else:
        print('Username already exists')
        get_username()


def get_password():
    password = input('Enter password: ')
    if len(password) >= 8:
        confirm_password = input('Confirm password: ')
        if password == confirm_password:
            credentials.append(password)
        else:
            print('Password does not match with confirmation\n\n')
            get_password()
    else:
        print('Password too short (min 8 characters)\n\n')
        get_password()
    

def add_credentials_to_database():
    credentials_file = os.path.join(database_path, f'{credentials[0]}.txt')
    file = open(credentials_file, 'w')
    file.write(credentials[1])
    file.close()
    print('Account created successfully\n\n')
    initialize()

def login_func():
    database = os.listdir(database_path) #Refreshes the database, so if you create an account you can login without having to run the program again
    enter_login = input('Enter username: ')
    if f'{enter_login}.txt' in database:
        for file in database:
            if f'{enter_login}.txt' == file:
                enter_password = input('Enter password: ')
                file = open(f"C:\\Users\\filip\\PycharmProjects\\database\\{file}", 'r')
                if enter_password == file.read():
                    print('WELCOME!')
                else:
                    print('Invalid password\n\n')
                    login_func()
    else:
        print('Invalid username\n\n')
        login_func()


if __name__ == '__main__':
    initialize()

Also, if you remember, please tell me in the comments simple projects that you did when you were learning, so I can do it too. ps: I know this login system is not secure at all but maybe later I could do some encryptation on those credencials...

\$\endgroup\$
7
  • 3
    \$\begingroup\$ I made a program that could score a Scrabble game, very fun indeed. \$\endgroup\$ – unkn0wn.dev Aug 11 '20 at 0:13
  • \$\begingroup\$ did it have an real interface? \$\endgroup\$ – filip augusto Aug 11 '20 at 0:20
  • 2
    \$\begingroup\$ Did you test this? Does it work? \$\endgroup\$ – Mast Aug 11 '20 at 6:52
  • 4
    \$\begingroup\$ Minesweeper is a great starter \$\endgroup\$ – konijn Aug 11 '20 at 8:40
  • 1
    \$\begingroup\$ @filip augusto yes it was a CLI interface \$\endgroup\$ – unkn0wn.dev Aug 12 '20 at 7:44
17
\$\begingroup\$

When entering passwords, you may want to opt for using the getpass library. It allows you (if the console supports it) to hide passwords as they're being typed. You'd use getpass.getpass just as you are using input to ask for the password. The only difference is, the password won't show in the console as it's typed.


You're using recursion in get_password in order to ask the user again. This isn't a good idea. If the user fails too many times, your program can crash. Just use a loop:

def get_password():
    while True:
        password = input('Enter password: ')
        if len(password) >= 8:
            confirm_password = input('Confirm password: ')
            if password == confirm_password:
                credentials.append(password)
                return
            else:
                print('Password does not match with confirmation\n\n')
        else:
            print('Password too short (min 8 characters)\n\n')

I'm looping until correct input is entered, then I'm just returning from within the infinite loop.

And the same goes for the rest of your functions. Recursion is great when used properly, but it carries more problems than most other looping methods. Use it in cases where it's appropriate (like when the problem itself is recursive), not just for general looping.


add_credentials would be safer if it used a with block to close the file for you:

def add_credentials_to_database():
    credentials_file = os.path.join(database_path, f'{credentials[0]}.txt')
    with open(credentials_file, 'w') as file:
        file.write(credentials[1])
    print('Account created successfully\n\n')
    initialize()

Now, even if an error were to happen half-way through the function, there's a greater chance that the file would be closed. Making a practice of using with also prevents you from forgetting to add a call to file.close().


At the top you have:

credentials = []
database_path = "C:\\Users\\filip\\PycharmProjects\\database"
database = os.listdir(database_path)

A few things to note:

  • credentials shouldn't be a global here. Global mutable states (which credentials is) tend to complicate code and make it harder to understand. In this set up, I would probably make credentials a local variable of initialize, have get_password and the like to return the data instead of adding it to the list directly, and manually pass that data to add_credentials_to_database.

  • database_path is a module-level constant, and as such should be in upper-case like DATABASE_PATH according to PEP8.

  • Arguably database is also (effectively) constant, so it should also be in upper-case.


In initialize, I would get rid of the int conversion of choose_option. All the call to int is doing is allowing for a ValueError to be raised if the user enters in a dumb menu option like "a". Just use the raw string value and compare against it:

choose_option = input('0. Exit\n1. Login\n2. Sign up\n\nOption: ')
if choose_option == "1":
    . . .

If you actually needed it as an integer to do math on it or something, it would be different. It's simply an "indicator value" though, so the type of choose_option doesn't really matter, and keeping it as a string has less problems.



And my favorite projects that I still do whenever I learn a new language are:

  • A naïve, bruteforce prime number finder (and then maybe a sieve). Fairly basic.
  • Conway's Game of Life. Harder, and has the potential to turn into a large project.
  • A Mandelbrot Set image generator. Even harder, and usually becomes a larger project.
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thank you so much this was very usefull, tomorrow i'll applicate the changes you suggested!! Then I will post it \$\endgroup\$ – filip augusto Aug 11 '20 at 2:04
  • 4
    \$\begingroup\$ @filipaugusto No problem. And just note, code in questions can't be altered once posted. If you'd like a review of the new code, it needs to be as a separate question. \$\endgroup\$ – Carcigenicate Aug 11 '20 at 2:20
  • \$\begingroup\$ Ok, but could you do a really quick look at it? would be nice \$\endgroup\$ – filip augusto Aug 11 '20 at 10:14
  • \$\begingroup\$ @filipaugusto If you post it as a separate question, I should have time to review it later. \$\endgroup\$ – Carcigenicate Aug 11 '20 at 13:39
11
\$\begingroup\$

As you mentioned, as-is the system is not at all secure. However, it is easy to hash passwords properly, and this is a great opportunity to learn the concept.

The idea of password hashing is to add a random salt, which makes it harder to use precomputed tables for breaking the password, and a one-way function which can only be "encrypted", but not "decrypted". The output of the one-way function is stored. When the user inputs a new password, the one-way function is applied again and the output is compared to the stored value. There is no way to decrypt the password without first knowing it.

One of the best current password hashing schemes is bcrypt. For Python, this is provided by the bcrypt module, which has to be installed. On Linux distributions, it is usually available in package python3-bcrypt. On Windows and Mac, you can install it with pip install bcrypt.

There is a good tutorial on bcrypt usage here. When applied to your code, you'd generate the salt and compute hash when user first provides the password:

            credentials.append(password)

becomes:

            password_utf8 = password.encode('utf-8') # Convert to UTF-8 for hashing
            salt = bcrypt.gensalt() # Generate random salt
            hash = bcrypt.hashpw(password_utf8, salt) # Apply one-way function
            credentials.append(hash) # Store the hash

And when verifying, use bcrypt.checkpw():

if enter_password == file.read():

becomes:

if bcrypt.checkpw(enter_password, file.read()):

If you then look into the file, you'll see a string like $2b$12$viuQE9cX.ZCoqZewzRqJz.Ae76yYRH.4fkOKe9CJPl3ssm6n50AiC, which is the hashed password.

\$\endgroup\$
3
  • \$\begingroup\$ That just added a whole new level of security, didn't know that was that easy. Also, about storing credentials, do you know a more efficient and safe way of doing it? Because seems that creating a .txt file than writing the password on it is not good. When I was first writing the project, I just improvised a way to store, and that was what came to my mind. \$\endgroup\$ – filip augusto Aug 13 '20 at 10:51
  • \$\begingroup\$ @filipaugusto For scalability of storing more information (multiple users, usernames, access rights etc.), one can consider a range of formats from JSON to a sqlite database. But the security aspect will remain the same - whoever can read the file, can read the stored data, and there is no simple way to avoid that. In real systems the solution is to restrict access to the file with file permissions and system-level user accounts. \$\endgroup\$ – jpa Aug 13 '20 at 12:37
  • \$\begingroup\$ Ok I got it, so i'll keep it the same \$\endgroup\$ – filip augusto Aug 14 '20 at 10:37
3
\$\begingroup\$

You loop over the database when it isn't necessary:

    if f'{enter_login}.txt' in database:
        for file in database:
            if f'{enter_login}.txt' == file:
                enter_password = input('Enter password: ')
                file = open(f"C:\\Users\\filip\\PycharmProjects\\database\\{file}", 'r')
                if enter_password == file.read():
                    print('WELCOME!')
                else:
                    print('Invalid password\n\n')
                    login_func()
    else:
        print('Invalid username\n\n')
        login_func()

can become:

    if f'{enter_login}.txt' in database:
        enter_password = input('Enter password: ')
        file = open(f"C:\\Users\\filip\\PycharmProjects\\database\\{enter_login}.txt", 'r')
        if enter_password == file.read():
            print('WELCOME!')
        else:
            print('Invalid password\n\n')
            login_func()
    else:
        print('Invalid username\n\n')
        login_func()

The way you had it would probably only get slow if you have hundreds or thousands of users, but it's good to be aware of when you're doing unnecessary work.

I also recommend using pathlib instead of manually manipulating strings that represent file paths.

Here's what that would look like: (incorporating Carcigenicate's answer)

from pathlib import Path
from getpass import getpass 

credentials = []
DATABASE_PATH = Path("C:\\Users\\filip\\PycharmProjects\\database")

def get_user_file(username):
    return DATABASE_PATH / f'{username}.txt'


def initialize():
    while True:
        choose_option = input('0. Exit\n1. Login\n2. Sign up\n\nOption: ')
        if choose_option == '1':
            login_func()
            return
        elif choose_option == '2':
            username = get_username()
            password = get_password()

            add_credentials_to_database(username, password)
        elif choose_option == '0':
            exit('Exiting...')
            return
        else:
            print("You should enter '0', '1', or '2'.")
        

def get_username():
    while True:
        username = input('Enter username: ')
        if get_user_file(username).exists():
            print('Username already exists')
        elif len(username) >= 8:
            return username
        else:
            print('Username too short (min 8 characters)\n\n')


def get_password():
    while True:
        password = getpass('Enter password: ')
        if len(password) >= 8:
            confirm_password = getpass('Confirm password: ')
            if password == confirm_password:
                return password
            else:
                print('Password does not match with confirmation\n\n')
        else:
            print('Password too short (min 8 characters)\n\n')
    

def add_credentials_to_database(username, password):
    get_user_file(username).write_text(password)
    print('Account created successfully\n\n')


def login_func():
    while True:
        login = get_user_file(input('Enter username: '))
        if login.exists():
            enter_password = getpass('Enter password: ')
            if enter_password == login.read_text():
                print('WELCOME!')
                return
            else:
                print('Invalid password\n\n')
        else:
            print('Invalid username\n\n')


if __name__ == '__main__':
    initialize()
\$\endgroup\$
3
  • \$\begingroup\$ That was a cleaver answer, thank you Jasmijn, made it much shorter! \$\endgroup\$ – filip augusto Aug 13 '20 at 10:51
  • \$\begingroup\$ As you mentioned path, if I were to make this .py into a .exe, how would I store their credentials if I don't know their path, like, In the code above the program goes through \Users\filip, what if I don't know their user? \$\endgroup\$ – filip augusto Aug 13 '20 at 10:57
  • 1
    \$\begingroup\$ I'd use the appdirs library for that (pypi.org/project/appdirs) \$\endgroup\$ – Jasmijn Aug 13 '20 at 12:11

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.