1
\$\begingroup\$

I completed my Notes Saver project in python tkinter and now I want to get it reviewed.

I want to how I could -

  • Improve my hashing system.
  • Improve my notes encryption system.
  • Improve the UX.
  • Improve code readability
  • Improve performance.

Working Directory

Notes Saver -
             |- database_funcs.py
             |- encrypt_hash.py
             |- verification.py
             |- gui.py
             |- main.py
             |- database.sql
             |- secret.key
             |- variables.env

A important note: Please dont create the secret.key file beforehand. It will cause error in loading the key. It will be created by the program.

Also update the email credentials in variables.env

The code is kinda long. Here is the github link. I am not experienced in github.

database_funcs.py

'''Database related functions'''

import sqlite3
import bcrypt
from tkinter.messagebox import showerror
from encrypt_hash import encrypt_json

import os
from dotenv import load_dotenv

load_dotenv('variables.env')

APP_NAME = os.getenv('APP_NAME')
DATABASE = os.getenv('DATABASE')

conn = sqlite3.connect(DATABASE)
cursor = conn.cursor()

def execute_query(query: str, args: tuple=()):
    '''Executes a query, commits it and returns the result of the query.'''

    result = cursor.execute(query, args)
    conn.commit()
    return result

execute_query('CREATE TABLE IF NOT EXISTS ACCOUNTS(EMAIL TEXT, PASSWORD TEXT, NOTES TEXT)')
'''
EMAIL             PASSWORD          NOTES
John@gmail.com    John's pw(hash)   '["Tab1", "Tab2"]'(encrypted)
'''

def get_all_accounts() -> dict:
    '''Returns a dict of all accounts with their passwords.
    For example: {'John@gmail.com': 'John's password'(hash), ...}'''

    accounts = execute_query('SELECT * FROM ACCOUNTS').fetchall()
    return {email: key for email, key, _ in accounts}

def get_account(email: str) -> tuple:
    '''Retuns account details of the email.
    For example: ('John@gmail.com': 'John's password'(hash), '["Tab1", "Tab2"]'(encrypted)'''

    return execute_query('SELECT * FROM ACCOUNTS WHERE EMAIL = ?', (email,)).fetchone()

def verify_login(email: str, key: str) -> bool:
    '''Checks if the email exist and key matches.'''

    accounts = get_all_accounts()
    if email in accounts.keys() and bcrypt.checkpw(key.encode(), accounts[email]):
        return True
    else:
        showerror(APP_NAME, 'Wrong credentials')
        return False

def verify_register(email: str, key: str) -> bool:
    '''Verifies if you can register using a email. Also checks the password requirement'''

    accounts = get_all_accounts()
    if email in accounts.keys():
        showerror(APP_NAME, 'The provided email has already been registered.')
    elif len(key) < 8:
        showerror(APP_NAME, 'Password should be atleast 8 charactars.')
    elif not key.isalnum():
        showerror(APP_NAME, 'Password should not contain non-alpahumeric charactars.')
    else:
        return True
    return False

def create_account(email: str, key: str) -> None:
    '''Creates a new account with the email and key and saves it in the database.'''

    hashed_key = bcrypt.hashpw(key.encode(), bcrypt.gensalt())
    encrypted_notes = encrypt_json([''])
    execute_query('INSERT INTO ACCOUNTS VALUES(?, ?, ?)', (email, hashed_key, encrypted_notes))

encrypt_hash.py

'''Encryption and hashing for Notes Saver'''

from cryptography.fernet import Fernet
import json

def load_key() -> str:
    '''Loads encryption key for fernet. If not available, creates one'''
    try:
        with open('secret.key', 'rb') as file:
            return file.read()
    except FileNotFoundError:
        key = Fernet.generate_key()
        with open('secret.key', 'wb') as file:
            file.write(key)
        return key

fernet = Fernet(load_key())

def encrypt_json(obj: list) -> str:
    '''Dumps the obj and then encrypts it.'''
    return fernet.encrypt(json.dumps(obj).encode())

def decrypt_json(obj: str) -> list:
    '''Decrypts the obj and then loads it.'''
    return json.loads(fernet.decrypt(obj).decode())

verification.py

'''Email Verification for Notes Saver'''

from smtplib import SMTP_SSL
from random import randint
from tkinter.messagebox import showerror, showinfo
from tkinter.simpledialog import askinteger

import os
from dotenv import load_dotenv

load_dotenv('variables.env')

EMAIL = os.getenv('EMAIL')
PASSWORD = os.getenv('PASSWORD')
APP_NAME = os.getenv('APP_NAME')

def send_verification_code(recipent: str) -> bool:
    '''Sends a verification code to recipent. Returns if verified'''

    code = randint(1000, 10000)
    server = SMTP_SSL('smtp.gmail.com', 465)
    server.login(EMAIL, PASSWORD)
    try:
        server.sendmail(
        EMAIL, recipent,
        f'Subject: Python\nYour verification code is {code}\nEnter this code to register your account at Notes Saver, If you did not register then ignore this email''')
    except:
        showerror('Invalid email')
    finally:
        server.quit()

    user_code = askinteger(APP_NAME, f'''Verification code sent at {recipent}, 
        Enter the verification code to register''')
    if user_code == code:
        showinfo(APP_NAME, 'Successfully registered')
        return True
    else:
        showerror(APP_NAME, 'Incorrect code, Verification Failed')
        return False

gui.py

'''GUI for Notes Saver'''

import tkinter as tk
from tkinter.ttk import Notebook, Style
from tkinter.messagebox import askyesno
from database_funcs import *
from encrypt_hash import *
from verification import *

import os
from dotenv import load_dotenv

load_dotenv('variables.env')

EMAIL = os.getenv('EMAIL')
PASSWORD = os.getenv('PASSWORD')
APP_NAME = os.getenv('APP_NAME')
DATABASE = os.getenv('DATABASE')
SECRET_FILE = os.getenv('SECRET_FILE')

class LoginWindow:
    '''Login Window class'''

    def __init__(self, win: tk.Tk) -> None:
        '''Initializing...'''

        self.win = win
        self.font = 'Arial 12'
        self.email_var = tk.StringVar(win)
        self.key_var = tk.StringVar(win)

    def run(self) -> None:
        '''Sets up the window'''

        for widget in self.win.winfo_children():
            widget.destroy()
        self.win.title(APP_NAME)
        self.win.geometry('250x175')
        self.win.resizable(False, False)
        self.build_layout()

    def get_details(self) -> None:
        '''Returns the email and key entered.'''

        return self.email_var.get().strip(), self.key_var.get().strip()

    def register(self) -> None:
        '''Registers the user'''

        email, key = self.get_details()
        if verify_register(email, key) and send_verification_code(email):
            create_account(email, key)
            NoteSaverWindow(self.win, get_account(email)).run()

    def login(self) -> None:
        '''Logs the user in'''

        email, key = self.get_details()
        if verify_login(email, key):
            NoteSaverWindow(self.win, get_account(email)).run()

    def build_layout(self) -> None:
        '''Builds the window layout'''

        for num in range(5):
            self.win.grid_rowconfigure(num, weight= 1)
        for num in range(2):
            self.win.grid_columnconfigure(num, weight= 1)

        tk.Label(self.win, text= 'Email Id:', font= self.font).grid(
            columnspan= 2, sticky= 'nwes', padx= 5, pady= 5)
        tk.Entry(self.win, textvariable= self.email_var, font= self.font).grid(
            columnspan= 2, sticky= 'nwes', padx= 5, pady= 5)
        tk.Label(self.win, text= 'Password:', font= self.font).grid(
            columnspan= 2, sticky= 'nwes', padx= 5, pady= 5)
        tk.Entry(self.win, textvariable= self.key_var, font= self.font).grid(
            columnspan= 2, sticky= 'nwes', padx= 5, pady= 5)
        tk.Button(self.win, text= 'Login', command= self.login, 
            font= self.font).grid(sticky= 'nwes', padx= 5, pady= 5)
        tk.Button(self.win, text= 'Register', command= self.register, 
            font= self.font).grid(row= 4, column= 1, sticky= 'nwes', 
                padx= 5, pady= 5)

class NoteSaverWindow:
    '''Note Saver Window class'''

    def __init__(self, win: tk.Tk, details: tuple) -> None:
        '''Intializing...'''

        self.email, self.key, self.notes = details
        self.notes = decrypt_json(self.notes)
        self.win = win
        self.tab_count = 1
        self.texts = []

    def run(self) -> None:
        '''Sets up the window'''

        for widget in self.win.winfo_children():
            widget.destroy()
        self.win.title(f"{self.email}'s Notes Saver")
        self.win.geometry('500x500')
        self.win.resizable(True, True)
        self.build_layout()

    def logout(self, event=None) -> None:
        '''Logs the user out'''

        LoginWindow(self.win).run()

    def add_tab(self, text_content='') -> None:
        '''Adds a new tab in the notebook'''

        text = self.build_text(self.notebook)    
        text.insert('1.0', text_content)  
        self.notebook.add(text, text=f'Tab{self.tab_count}')
        self.texts.append(text)
        self.tab_count += 1

    def delete_tab(self) -> None:
        '''Asks the user and deletes the tab'''
        for widget in self.notebook.winfo_children():
            current_tab = self.notebook.select()
            tab_name = self.notebook.tab(current_tab, 'text')
            if str(widget) == current_tab:
                if askyesno(APP_NAME, f'Do you want to delete {tab_name}?'):
                    widget.destroy()
                    self.tab_count -= 1
                break

    def build_text(self, parent) -> None:
        '''Builds a new text widget'''

        text = tk.Text(parent)
        text.pack(expand=True, fill='both')
        scroll_bar = tk.Scrollbar(text)
        scroll_bar.pack(side='right', fill='y')
        scroll_bar.config(command=text.yview) 
        text.config(yscrollcommand=scroll_bar.set)
        return text

    def save_notes(self, event=None):
        '''Saves all the notes'''

        self.notes.clear()
        for text in self.texts:
            try:
                self.notes.append(text.get(1.0, 'end'))
            except:
                pass

        encrypted_notes = encrypt_json(self.notes)
        execute_query('UPDATE ACCOUNTS SET NOTES = ? WHERE EMAIL = ?', (encrypted_notes, self.email))

    def build_layout(self):
        for num in range(5):
            self.win.grid_rowconfigure(num, weight=0)
        for num in range(2):
            self.win.grid_columnconfigure(num, weight=0)

        main_menu = tk.Menu(self.win)
        self.win['menu'] = main_menu

        file_menu = tk.Menu(main_menu, tearoff=0) 
        main_menu.add_cascade(label='File', menu=file_menu)
        file_menu.add_command(label='Save', command=self.save_notes)
        file_menu.add_separator()
        file_menu.add_command(label='Logout', command=lambda: LoginWindow(self.win).run())
        file_menu.add_command(label='Exit', command=quit)

        edit_menu = tk.Menu(main_menu, tearoff= 0)
        main_menu.add_cascade(label='Edit', menu=edit_menu) 
        edit_menu.add_command(label='Add New Tab', command=self.add_tab)
        edit_menu.add_command(label='Delete Tab', command=self.delete_tab)

        self.notebook = Notebook(self.win)
        self.notebook.pack(expand=True, fill='both')
        for note in self.notes:
            self.add_tab(note)

main.py

'''Notes Saver implemented with tkinter in python'''

__author__ = 'Random Coder 59'
__version__ = '1.0.1'
__email__ = 'randomcoder59@gmail.com'

from gui import *

if __name__ == '__main__':
    win = tk.Tk()
    style = Style(win)
    style.theme_use('clam')
    LoginWindow(win).run()
    win.mainloop()

database.sql

secret.key

Dont create it beforehand

variables.env

EMAIL=xxxxxxx@gmail.com
PASSWORD=xxxxxxx
APP_NAME=Notes Saver
DATABASE=database.sql
SECRET_FILE=secret.key

Update the email credentials.

Thanks!

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

This seems like it took some effort to get to the current state. Good work!

The code looks reasonably clean. If the functions do what their name says they do, I'm happy that they all do one thing and will be easy to change if needed.

I think it would be worth looking at two big picture ideas, coupling and usability.


Coupling

"How easy is it to cut out a piece of code?" If the answer is "very difficult" then our system is likely tightly coupled. Ideally, it is easy to remove code (say we find it doesn't work as intended and need to replace it) and it is easy to run that cut out code by itself (say we want to test it).

Most of the functions look good. There are some that could be improved.

def verify_register(email: str, key: str) -> bool:
    '''Verifies if you can register using a email. Also checks the password requirement'''

    accounts = get_all_accounts()
    if email in accounts.keys():
        showerror(APP_NAME, 'The provided email has already been registered.')
    elif len(key) < 8:
        showerror(APP_NAME, 'Password should be atleast 8 charactars.')
    elif not key.isalnum():
        showerror(APP_NAME, 'Password should not contain non-alpahumeric charactars.')
    else:
        return True
    return False

Suppose we want to test parts of the system. How easy is it to test this code by itself? I could imagine a first draft piece of code to test this might look something like:

# test_verification.py
from database_funcs import verify_register


def test_verify_register_on_simple_email_and_bad_password():
    email = "a@a.com"
    key = "AA"
    assert(not verify_register(email, key))

At best we'll have a GUI pop up with an error message. This isn't ideal if we want to run a dozen more tests and need to dismiss every error message by hand.

We want to separate the verification from the GUI.

def verify_register(email: str, key: str) -> bool:
    '''Verifies if you can register using a email. Also checks the password requirement'''

    accounts = get_all_accounts()
    if email in accounts.keys():
        return False
    elif len(key) < 8:
        return False
    elif not key.isalnum():
        return False
    else:
        return True

# gui.py
def register(self) -> None:
    '''Registers the user'''

    email, key = self.get_details()
    if verify_register(email, key) and send_verification_code(email):
        create_account(email, key)
        NoteSaverWindow(self.win, get_account(email)).run()
    else:
        showerror(APP_NAME, "Can not register using that username and password.")

Unfortunately, we have lost some of the code. Namely, the reason for the checks. Let's add them back in via a function with a descriptive name and docstring.

def valid_key(key: str) -> bool:
    """Verify a key meets password requirements.

    A key must be long enough and only contain alphanumeric characters."""
    return len(key) >= 8 and key.isalnum()

def verify_register(email: str, key: str) -> bool:
    '''Verifies if you can register using a email. Also checks the password requirement'''
    if not valid_key(key):
        return False

    accounts = get_all_accounts()
    if email in accounts.keys():
        # Email is in use.
        return False

    return True

Usability

A important note: Please dont create the secret.key file beforehand. It will cause error in loading the key. It will be created by the program.

This seems like important information. Could it be included in the code? You could try using the key from the file, and then tell the user what went wrong if it doesn't work.

What happens if a user accidentally deletes the secret.key file? Is the data lost forever or can it be retrieved by knowing their password?

Even worse, what happens if two people want to use the app? When the second person tries to log in, there will already be a secret.key file and the app will error. If they read this important note and see the file already exsists, they might delete the file. Then the first person can no longer access their notes.

A slightly more common scenario might be if a user wants to re-install the app. Can they do that without losing their data? What files would they need to keep intact? Is there instructions anywhere on how to re-install? (For the extra mile, can it be be done automatically?)

A securtiy concern is that the key to decrypt all of a user's notes is store in plaintext in a well-known location.

I think most of these problems disappear if we don't keep a secret.key file. I don't have the security expertise to recommend a solution. I would guess you want a long-term key (as opposed to a session key) to allow decrypting again in the future. From my limited experience asymmetric keys seem to be more common for that purpose.

\$\endgroup\$
1
  • \$\begingroup\$ I read about asymmetric keys... Even that would raise the same issue of deletion if any of the public or private keys are lost. \$\endgroup\$ Sep 21, 2021 at 14:30

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.