Login system using bcrypt and MySQL, to be used for any future projects

Question

Main function will ask what the user wants to do. LoginSystem class will handle the sign up and login process as well with the checking if password is right. passwordHashing will use bcrpyt to salt and hash the password.

import bcrypt
from loginDbMySQL import insert, logging_in


def main():
    
    login_instance = LoginSystem()
    check = True

    while check == True:
        value = input("1-Login, 2-Sign-up, 3-Exit\n")
        if value == "1":
            login_instance.login()
            check = True
        elif value == "2":
            login_instance.sign_up()
            check = True
        elif value == "3":
            check = False

class LoginSystem():

    def sign_up(self):
        username = input("Enter a username: ")
        password = input("Enter a password: ")
        
        password_hash = self.passwordHashing(password)
        insert(username, password_hash)

    def login(self):
        username = input("Enter a username: ")
        password = input("Enter a password: ")

        password_hash = logging_in(username)
        self.comparePassword(password, password_hash)

    def passwordHashing(self, password):

        password_bytes = password.encode('utf8')

        salt = bcrypt.gensalt(14)
    
        password_hash_byte = bcrypt.hashpw(password_bytes, salt)

        password_hash_str = password_hash_byte.decode()

        return password_hash_str

    def comparePassword(self,password,password_hash):

        if bcrypt.checkpw(password.encode('utf8'), password_hash.encode('utf8')):
            print("Welcome")
        else:
            print("Incorrect Information")


if __name__ == "__main__":
    main()

loginDbMySQL handles the database insertion, checking, and query's

import mysql.connector


mydb = mysql.connector.connect(
    host="localhost",
    user="root",
    password="*******",
    database="*******"
)

mycursor = mydb.cursor()

def insert(username,password_hash):

    mycursor = mydb.cursor(buffered=True)
    select_query = "SELECT usernames FROM login_table WHERE usernames= %s"
    mycursor.execute(select_query, (username,))
    check = mycursor.fetchone()

    if check == None:
        mycursor.execute("INSERT INTO login_table (usernames, password_hashs) VALUES (%s,%s)", (username, password_hash))
        mydb.commit()
        print("Account created!")

    else:
        print("Account already exist.")

def logging_in(username):

    mycursor = mydb.cursor(buffered=True)
    select_query = "SELECT * FROM login_table WHERE usernames= %s"
    mycursor.execute(select_query, (username,))
    check = mycursor.fetchone()
    
    if check == None:
        print("Incorrect information.")
    else:
        return check[1]

Answer 1

Let's start with the SQL and your loginDbMySQL stuff first.

You should probably reconsider the naming of your SQL table columns.

Bare minimum reproducible example with a table that has automatic incrementation, a UID as the primary key, and the username field 'username' and 'passwd_hash'. You should name columns to be descriptive of the value per record, not as plurals unless you are storing multiple items for each field in the record.

So, let's make the table accordingly, and also have a unique constraint on usernames - you don't want duplicate usernames. The username also becomes the primary key since we want unique usernames:

create or replace table login_tests.login_table
(
    uname varchar(50) not null,
    passwd_hash varchar(1000) not null
);

alter table login_tests.login_table
    add primary key (uname);

Note we're using uname as the primary key, so you can only have unique usernames. This is entirely permitted in SQL.

SQL query mechanisms being used are NOT SQL-Injection-Safe!

Next, let's look at one of the glaring problems in your code: your queries and routines are not injection-safe! I can easily pass a value in that can escape the 'argument' being called and simply cause chaos with SQL injections. The way around this is to build predetermined procedures in the DB and call that, and then pass the arguments in with callproc calls.

To start with, you have three separate queries - one to determine if a user exists, one to insert a user, and then a similar select query for logging in. But, we can use the same SELECT for the same task! Saves us code!

Also, you aren't really using SELECT functions for anything other than checking if a username exists on insert (fixed later), and getting hashed pw data for a given username.

Firstly, on the MySQL side we need to create the functions.

CREATE PROCEDURE GetPWHash(username varchar(50))
BEGIN
    SELECT passwd_hash FROM login_table WHERE uname = username;
END;

CREATE PROCEDURE GetUserData(username varchar(50))
BEGIN
    SELECT * FROM login_table WHERE uname = username:
END;

CREATE PROCEDURE AddUser(username varchar(50), pass_hash varchar(1000))
BEGIN
   INSERT INTO login_table(uname, passwd_hash) VALUES (username, pass_hash);
   COMMIT;
END;

We'll then call the relevant procedures with a cursor.callproc(ProcedureName, [argument]) call instead. It's not considered smart to build your queries in strings because it is typically extremely unlikely you will be able to properly escape the queries. The callproc function will automatically escape them before passing them to the function.

You'll see me replace your functions accordingly as we go and edit each function and call. So let's begin with those.

INSERT function doesn't need a select to check if the user exists - DB insert will simply fail if a user exists because of primary key restrictions.

You're doing testing here with an extra DB call which requires more disk I/O for reading.

So let's simply rewrite your insert function entirely. It's not really pythonic in its current state, and you make an unnecessary extra DB call. Further, you should consider a DB cursor for each one, without global declarations. So I'll provide you some revisions.

Let's start with your insert function:

def insert(username, password_hash):
    cursor = mydb.cursor(buffered=True)
    cursor.callproc('AddUser', [username, password_hash])
    cursor.close()

We should not access the 'username' ahead of time, we can simply catch the error (the callproc function throws a mysql.connector.errors.IntegrityError class error when we violate a constraint). No need to check if the user exists already, and as I included earlier I wrote a COMMIT into the AddUser function. No need to make the db commit as a result, it commits after it adds! (Actually confirmed this checking the DB after the callproc!)

Logging In Function: Use callproc!

Again, we should be using callproc and such so you eliminate an SQL Injection risk. Further, your 'logging in' function in loginDbMySQL is... not really 'logging in'. It's more fetching user data, so let's name it accordingly.

def logging_in(username):

    mycursor = mydb.cursor(buffered=True)
    mycursor.callproc('GetUserData', [username])
    select_query = "SELECT * FROM login_table WHERE usernames= %s"
    data = mycursor.fetchone()
    # INCOMPLETE FUNCTION

But because this is an incomplete function that does nothing, I essentially suggest removing it.

And in its place put a useful function - getUserPWHash - which will call the SQL and only return the PW hash for the specified user (if it exists).

def getUserPWHash(username):
    mycursor = mydb.cursor(buffered=True)
    mycursor.callproc('GetPWHash', [username])
    data = mycursor.fetchone()
    return data

This will be useful later, as you will see when we dig into your other code.

---

So, taking into account the SQL bits, we have a bunch of code tweaks on the flip side in your login.py script.

Let's start with your LogInSystem class.

Code Duplication - can be reduced by adding a static method for prompting for credentials

You have two cases where you would benefit from a separate function from which you can request credentials. Registration, and logging in. Let's start by adding a new method to your class for this - called promptForCreds. This is going to have a special decorator - @staticmethod - this tells the system that this function is able to stand on its own and does NOT require a dependency on the object itself (self) in the declaration of the method:

@staticmethod
def promptForCreds():
    username = input("Enter a username: ")
    password = input("Enter a password: ")

    return username, password

This will eventually be used here in your sign up and login functions. We'll apply this in a later revision suggestion.

Reduce the need of multiple variables - like in the SQL sections.

So you end up having a bunch of one-time-use variables in passwordHashing, like you did in the SQL bits.

We can simply replace the use of variables to hold values by directly putting the variables in line. If we name the items accordingly, we don't need additional variables to hold them, since it's clear what the objects are (they're already arguments passed into functions!)

Oh, and the passwordHashing method can also be a @staticmethod object too - it doesn't depend on any other components within your class specifically.

@staticmethod
def passwordHashing(password):
    return bcrypt.hashpw(password.encode('utf-8'), bcrypt.gensalt(14)).decode('utf-8')

This looks long but it can be broken across multiple lines if you keep the parentheses around the bcrypt.hashpw call. But, this reduces those one-use variables AND puts everything into a simple return call - and only one of them. No extra variables!

Another static method - comparePassword

This one's a minor change and pretty simple - just add a decorator ahead of comparePassword.

@staticmethod
def comparePassword(password, password_hash):
    if bcrypt.checkpw(password.encode('utf8'), password_hash.encode('utf8')):
        print("Welcome")
    else:
        print("Incorrect Information")

Remove code duplication in sign_up and login.

And now here's why I created that new method - we get to take four lines of code and condense it into two. This code is duplicated, yes, but we don't have to run the prompt twice individually.

def sign_up(self):
    username, password = self.promptForCreds()
    insert(username, self.passwordHashing(password))

def login(self):
    username, password = self.promptForCreds()
    password_hash = getUserPWHash(username)
    self.comparePassword(password, password_hash)

See what I did here? Username and password are obtained by the other function and then returned. Saves us from having to create the prompt twice! See what niceness I did there?

So your LoginSystem class becomes this:

class LoginSystem:
    @staticmethod
    def promptForCreds():
        username = input("Enter a username: ")
        password = input("Enter a password: ")
        return username, password

    @staticmethod
    def passwordHashing(password):
        return bcrypt.hashpw(password.encode('utf-8'), bcrypt.gensalt(14)).decode('utf-8')

    @staticmethod
    def comparePassword(password, password_hash):
        if bcrypt.checkpw(password.encode('utf8'), password_hash.encode('utf8')):
            print("Welcome")
        else:
            print("Incorrect Information")

    def sign_up(self):
        username, password = self.promptForCreds
        insert(username, self.passwordHashing(password))

    def login(self):
        username, password = self.promptForCreds
        password_hash = getUserPWHash(username)
        self.comparePassword(password, password_hash)

---

Now, let's look at your main() function.

Non-pythonic way of iterating over the checks!

You're doing something I wouldn't do here, which is using while var == True. Pythonically, you can reduce this to this: just do while var: for iterating while a variable is True. However, I suggest you do something else, which (once again) reduces the memory usage by not needing to define a variable.

def main():
    login_instance = LoginSystem()

    while True:
        value = input("1-Login, 2-Sign-up, 3-Exit\n")
        if value == "1":
            login_instance.login()
        elif value == "2":
            login_instance.sign_up()
        elif value == "3":
            pass  # Do nothing, we'll just exit as normal.
        else:
            continue  # Continue prompting until we get a valid value.

        break

This reduces the complexity and need to assign True or False. Just iterate infinitely and break on any value that is valid (1, 2, or 3), or keep going and prompting more until we get only one valid value.

You really should be triggering errors or returning Bools for your login functionality though.

The only problem with your approach is you have no way to catch whether a login was successful or not (or the same case with a registration).

You can approach this in one of two ways - either return errors on failures, or make your functions return a boolean instead of just printing statements.

I suggest you rename your comparePassword function to validateCredentials and then adjust it to return a True or False - True for valid credentials, False for invalid. Or, just return the result of bcrypt.checkpw:

@staticmethod
def validateCredentials(password, password_hash):
    return bcrypt.checkpw(password.encode('utf8'), password_hash.encode('utf8'))

Now, we'll use this for login functionality:

def login(self):
    username, password = self.promptForCreds
    password_hash = getUserPWHash(username)
    if self.validateCredentials(password, password_hash):
        print("Welcome!")
    else:
        print("Invalid Login")

Note that simply printing output is simple - you really should consider making this return a bool as well. This way you can utilize the results.

So that would look like this:

def login(self):
    username, password = self.promptForCreds
    password_hash = getUserPWHash(username)
    return self.validateCredentials(password, password_hash)

This would then mean we have to revise later for when value is 1:

# ...
        if value == "1":
            if login_instance.login():
                print("Welcome!")
            else:
                print("Access denied")

Now, we have a similar case with insert but we're going to use error codes instead.

Back in loginDbMySQL way earlier in this review, I basically removed print statements and error checking from your insert function. This was intentional, because we'll handle insert via error code data/returns.

So, in the conditional for when value is 2:

        elif value == "2":
            try:
                login_instance.sign_up()
                print("Sign up complete.")
            except mysql.connector.errors.IntegrityError:
                print("Unable to register account.")

This ultimately makes it a more usable login system function. It requires a single import added though - mysql.connector.errors. This is so we can import the error class for the exception handler.

---

---

The code with all my revisions:

login.py:

import bcrypt
import mysql.connector.errors
from loginDbMySQL import insert, getUserPWHash


class LoginSystem:
    @staticmethod
    def promptForCreds():
        username = input("Enter a username: ")
        password = input("Enter a password: ")
        return username, password

    @staticmethod
    def passwordHashing(password):
        return bcrypt.hashpw(password.encode('utf-8'), bcrypt.gensalt(14)).decode('utf-8')

    @staticmethod
    def validateCredentials(password, password_hash):
        return bcrypt.checkpw(password.encode('utf8'), password_hash.encode('utf8'))

    def sign_up(self):
        username, password = self.promptForCreds
        insert(username, self.passwordHashing(password))

    def login(self):
        username, password = self.promptForCreds
        password_hash = getUserPWHash(username)
        return self.validateCredentials(password, password_hash)


def main():
    login_instance = LoginSystem()

    while True:
        value = input("1-Login, 2-Sign-up, 3-Exit\n")
        if value == "1":
            if login_instance.login():
                print("Welcome!")
            else:
                print("Access denied")
        elif value == "2":
            try:
                login_instance.sign_up()
                print("Sign up complete.")
            except mysql.connector.errors.IntegrityError:
                print("Unable to register - username already taken.")
        elif value == "3":
            pass  # exit
        else:
            continue

        break


if __name__ == "__main__":
    main()

loginDbMySQL.py:

import mysql.connector


mydb = mysql.connector.connect(
    host="localhost",
    user="root",
    password="*******",
    database="*******"
)


def insert(username, password_hash):
    mycursor = mydb.cursor(buffered=True)
    mycursor.execute("AddUser", [username, password_hash])
    mycursor.close()
    print("Account created!")


def getUserPWHash(username):
    mycursor = mydb.cursor(buffered=True)
    mycursor.callproc('GetPWHash', [username])
    data = mycursor.fetchone()
    return data