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
callproc
execute the functions which will escape any input passed into arguments of the function. Unfortunately, that requires more DB work on your side, separate from the Python. I can confirm this from my own tests of the same functions you've used (or similar) and then the string replacement and execute inserts the way you're doing them didn't work right. \$\endgroup\$