Login function with Python and SQLite

Question

I'm a new programmer who is currently working on a personal project. It is supposed to be a login program which stores user data in a SQLite database.

I was wondering if anyone could look at my code and provide any constructive criticism. Please contribute, regardless of how insignificant the flaw may seem. Without further ado, here is my code:

import sqlite3, time


def is_valid_username(username):

    find_user = ("SELECT * FROM user WHERE username = ?")
    cursor.execute(find_user,[(username)])
    results = cursor.fetchall()             
    return results

def is_valid_password(username, password):


    find_user = ("SELECT * FROM user WHERE username = ? AND password = ?")
    cursor.execute(find_user, [(username), (password)])
    result = cursor.fetchall()
    return result
  
def create_new_user():
    while True:
        username = input("Enter a new username: ")
        find_user = ("SELECT * FROM user WHERE username = ?")
        cursor.execute(find_user, [(username)])

        if (cursor.fetchall()):
            print("Username taken, please try again")
        else:
            break
    firstName = input("Enter your first name: ")
    surname = input("Enter your surname: ")
    password = input("Enter your password: ")
    repassword = input("Reenter your password: ")
    while password != repassword:
        print("Passwords do not match, try again.")
        password = input("Enter your password: ")
        repassword = input("Reenter your password: ")
    insert = "INSERT INTO user(username, firstname, surname, password) VALUES(?,?,?,?)"
    cursor.execute(insert, [(username), (firstName), (surname), (password)])
    db.commit()
    print("Account created")
    time.sleep(2)
    return

with sqlite3.connect("Users.db") as db:
    cursor = db.cursor()

    cursor.execute('''
    CREATE TABLE IF NOT EXISTS user(
    userID INTEGER PRIMARY KEY,
    username VARCHAR(20) NOT NULL,
    firstname VARCHAR(20) NOT NULL,
    surname VARCHAR(20) NOT NULL,
    password VARCHAR(20) NOT NULL);
    ''')
   
for i in range(3):
    username = input("Please input username: ")
    if (is_valid_username(username)):
        password = input("Please input password: ")
        if is_valid_password(username, password):
            print('Username and password accepted, welcome.')
            time.sleep(3)
            exit()
        else:
            print("Incorrect password, please try again?: ")
    else:
        Q1 = input("Not valid user. Would you like to create an account?: ")
        reply_yes = ('y', 'yes')
        if Q1.lower() in reply_yes:
            create_new_user()'

Answer 1

security

Storing cleartext user passwords is just a bad idea. Prefer to store argon2id hash of the password, and then compare against hash of user input. Sooner or later one of the servers running your proposed library code will be compromised, exposing passwords in the clear to a malicious adversary, who can use them to attack unrelated sites due to folks re-using credentials.

An implementation is available from pypi.

design of Public API

is_valid_username() and is_valid_password() are good identifiers. But the non-boolean return type is slightly surprising, and no """docstring""" explains the details.

Consider making a weaker promise, via return bool(results). That preserves flexibility to revise your Public API in future, as you can be confident that no caller relied on finding e.g. some surname data in the result.

avoid SELECT *

It's fine for interactive debugging, but don't say SELECT * in production code. The idea is that columns may be added, removed, or re-ordered, and code consuming the query results should not be affected by that. Here it doesn't make a difference to what the boolean result will be. But nonetheless consider using a simpler SELECT username ... query, or even SELECT COUNT(*) ....

For some queries (though not in the OP schema) selecting fewer columns lets the query complete faster. For example, a query filtering on WHERE surname LIKE 'Smith%' could locate a thousand matching rows via an index on that column, and count them up, without doing a thousand useless probes of the main table's rows to retrieve e.g. firstname which will just be immediately discarded. We call this taking advantage of "a covering index", one which covers all requested columns.

nit: Rather than defining a userID column, the conventional name for this PK would be simply id. As written the OP code invites confusion between an engineer specifying userID versus username.

nit: While "singular name" is usually a better fit for a table identifier than "plural name" would be, here you might consider calling it users. Some non-sqlite databases, including postgres, would interpret the singular user as a SQL reserved keyword instead of a table name.

indexing

We've got a relational database at our disposal, which can potentially offer high performance. Yet we're not taking advantage of it.

    find_user = ("SELECT * FROM user WHERE username = ? AND password = ?")

An EXPLAIN SELECT ... will reveal that the query plan is "tablescan". That is, no better than grep $user /etc/passwd. We don't even get to bail out early, since there could be multiple matching rows.

Prefer to CREATE INDEX on that username column.

relational integrity

        find_user = ("SELECT * FROM user WHERE username = ?")
        cursor.execute(find_user, [(username)])
        if (cursor.fetchall()):
            print("Username taken, please try again")

This is good, and correct. (Though oddly it doesn't take advantage of the is_valid_username() helper function that appears above it.)

However, imposing the uniqueness requirement via app logic is not a best practice. In this context it would be far more natural to create a UNIQUE index on the username column.

conventional naming

    firstName = input("Enter your first name: ")

PEP 8 asks that you please spell it first_name.

Answer 2

Imports

This line:

import sqlite3, time

is typically split into 2 lines:

import sqlite3
import time

Layout

Conventionally, simple if statements do not use parentheses:

if (cursor.fetchall()):

would be cleaner as:

if cursor.fetchall():

Similarly for the execute statements with arrays:

cursor.execute(find_user,[(username)])

would be cleaner as:

cursor.execute(find_user, [username])

You should use consistent vertical whitespace. For example, in this function:

def is_valid_password(username, password):


    find_user = ("SELECT * FROM user WHERE username = ? AND password = ?")

there should not be 2 blank lines. This is more typical:

def is_valid_password(username, password):
    find_user = ("SELECT * FROM user WHERE username = ? AND password = ?")

Simpler

These lines in the is_valid_username function:

results = cursor.fetchall()             
return results

can be combined into one line:

return cursor.fetchall()             

This eliminates the generically-named result variable.

The same goes for the is_valid_password function.

The following code:

Q1 = input("Not valid user. Would you like to create an account?: ")
reply_yes = ('y', 'yes')
if Q1.lower() in reply_yes:
    create_new_user()'

can be simplified as:

create_user = input("Not valid user. Would you like to create an account?: ").lower()
if create_user[0] == 'y':
    create_new_user()

It is common to user lower directly with input, and you can just test the first letter of the reply.

DRY

The code for entering the password repeated in a couple places. Consider using a function for that.

Documentation

The PEP 8 style guide recommends adding docstrings for functions. Also add a docstring at the top of the code to summarize its purpose:

"""
Login program which stores user data in a SQLite database.
"""

Answer 3

What about connecting to your database?....creating the cursor? I think those are necessary.

conn = sqlite3.connect("database.db")
c = conn.cursor()