2
\$\begingroup\$

I created a simple login program that works how I wanted them to work. I separated the GUI and database codes into two different python files. I named the files as login.py and dbaseManip.py. The login.py consist of Tkinter syntax and all stuff related to GUI. On the other hand, dbaseManip.py holds the database related syntax such as the connection and the queries.

login.py

import tkinter as tk
from tkinter import messagebox
from tkinter import ttk

import dbaseManip as db

class loginForm(ttk.Frame):
    def __init__(self,parent,*arags,**kwargs):
        tk.Frame.__init__(self,parent)
        self.parent = parent

        self.parent.protocol("WM_DELETE_WINDOW",self.closing_window)

        window_width = 350
        window_height = 100
        screen_width = self.parent.winfo_screenwidth()
        screen_height = self.parent.winfo_screenheight()

        x_position = int((screen_width/2) - (window_width/2))
        y_position = int((screen_height/2) - (window_height/2))

        self.parent.geometry("{}x{}+{}+{}".format(window_width,window_height,x_position,y_position))

        self.widgeter()

    def widgeter(self):
        self.ent_username = ttk.Entry(self.parent)
        self.ent_username.insert(0,"username")
        self.ent_username.bind("<FocusIn>",self.foc_in_username)
        self.ent_username.bind("<FocusOut>",self.foc_out_username)


        self.ent_password = ttk.Entry(self.parent)
        self.ent_password.insert(0,"password")
        self.ent_password.bind("<FocusIn>",self.foc_in_password)
        self.ent_password.bind("<FocusOut>",self.foc_out_password)

        btn_login = ttk.Button(self.parent,text="Login",command=self.submit)
        btn_exit = ttk.Button(self.parent,text="Exit",command=self.closing_window)

        self.ent_username.grid(row=0,column=0,columnspan=3,sticky="nsew")
        self.ent_password.grid(row=1,column=0,columnspan=3,sticky="nsew")

        btn_login.grid(row=2,column=0,sticky="nsw")
        btn_exit.grid(row=2,column=1,sticky="nse")


    def foc_in_username(self, *args):
        if self.ent_username.get() == "username":
            self.ent_username.delete(0, tk.END)

    def foc_out_username(self,*args):
        if not self.ent_username.get():
            self.ent_username.insert(0,"username")

    def foc_in_password(self,*args):
        if self.ent_password.get() == "password":
            self.ent_password.delete(0,tk.END)
            self.ent_password.configure(show="*")

    def foc_out_password(self,*args):
        if not self.ent_password.get():
            self.ent_password.insert(0,"password")
            self.ent_password.configure(show="")

    def closing_window(self):
        answer = messagebox.askyesno("Exit","You want to exit the program?")
        if answer == True:
            self.parent.destroy()

    def submit(self):
        __verify = db.DatabaseManip(self.ent_username.get(),self.ent_password.get())
        __str_verify = str(__verify)
        if __str_verify == "correct":
            self.initialize_mainApplication()
        elif __str_verify is "incorrect":
            messagebox.showerror("Incorrect Password","You have entered incorrect password")
            # add deleter
        elif __str_verify == "notexist":
            messagebox.showerror("Username does not exist","The username you entered does not exist")


    def initialize_mainApplication(self):
        self.parent.destroy()
        self.parent = tk.Tk()
        self.mainApp = mainApplicaiton(self.parent)
        self.parent.mainloop()

class mainApplicaiton(tk.Frame):
    def __init__(self,parent):
        self.parent = parent

if __name__ == "__main__":
    root = tk.Tk()
    loginForm(root)
    root.mainloop()

dbaseManip.py

import mysql.connector

class DatabaseManip():
    def __init__(self,username,password):
        self.username = username
        self.password = password
        #
        # print(self.username)
        # print(self.password)

        self.dbaseInitializer()

    def dbaseInitializer(self):
        mydb = mysql.connector.connect(
            host="127.0.0.1",
            user="root",
            password="admin123",
            database="testdb"
        )

        self.myCursor = mydb.cursor()
        query_user_exists = "SELECT EXISTS(SELECT * FROM users WHERE username = %s)"
        self.myCursor.execute(query_user_exists,(self.username,))
        tuple_user_exists = self.myCursor.fetchone()
        total_user_exists = int(tuple_user_exists[0])

        if total_user_exists != 0:
            query_pass_validation = "SELECT password FROM users WHERE username = %s"
            self.myCursor.execute(query_pass_validation,(self.username,))
            __tuple_valid_pass = self.myCursor.fetchone()
            __password_validation = __tuple_valid_pass[0]

            if __password_validation == self.password:

                self.verdict = "correct"
            else:
                self.verdict = "incorrect"

        else:
            self.verdict = "notexist"

    def __str__(self):
        return self.verdict


    @property
    def username(self):
        return self.__username
    @username.setter
    def username(self,username):
        self.__username = username


    @property
    def password(self):
        return self.__password
    @password.setter
    def password(self,password):
        self.__password = password

This is how the system works:

  1. The login form will pop up
  2. After the user submitting their username and password, it will call the class DatabaseManip() that exists in dbaseManip.py.
  3. The DatabaseManip Class will validate the user's account.
  4. HERE COMES MY PROBLEM If the username does not exist in the database, it will assign a string "notexist" to a class variable called verdict. If the password is wrong, it will assign "incorrect". And lastly, if the validation is correct; it will assign "correct".
  5. The verdict result (which is either "correct","incorrect" or "notexist") will be returned to the GUI class in login.py using the __str__ method.
  6. In the loginForm class, a conditional statement will determine if the verdict from the databaseManip class is either "correct","incorrect" or "notexist". If it is "correct", a new window will pop-up. On the other hand, if it is either "incorrect" or "notexist", it will show an error message.

I think my way of coding this is extremely bad in terms of security and integrity. I'm not confident in the "correct", "incorrect", and "notexit" part. I would like to ask for advice to make this program more secure and not as wacky as the current state.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Firstly I'd suggest following PEP8 and/or using an auto-formatter like black, but if it's just you working on it that is a bit less of an issue than if other people were involved. Suffice to say, most code follows that and it usually gets flagged first thing (because it's so noticeable).

Reading the code, is that all there is, you are able to log in via a GUI? Then I'd say first thing would probably be not to hardcode the credentials to the database. So what/how's this securing anything? If the program has the credentials ... and the program is being run by the user who's authenticating ... what stops me from connecting directly to the database?


Apart from that fundamental issue looks mostly okay, no SQL injection since the execute will handle that.

__ as prefix for the variable names is unnecessary and confusing. I can't see why it's there at all.

Actually, SELECT password ... ... the password's stored in plaintext? Big no-no in anything used by other people. Do not store passwords in plaintext.

The properties at the end of DatabaseManip seem not very useful, in Python there's no need for them unless you want to control what/how values are stored in the object / how they're computed if they're not stored at all.

The __str__ method is there for what reason? That's very much confusing the responsibility of the class. It would somewhat make sense if it's being used interactively in a REPL, otherwise I can't see the reason for having it. Oh. Oh now I see it. Yeah, don't do that, that's bad style. How I'd expect things to go is something like this:

    def submit(self):
        result = db.DatabaseManip().check_credentials(self.ent_username,self.ent_password)
        if result == "correct":
            self.initialize_mainApplication()
        elif result == "incorrect":
            messagebox.showerror("Incorrect Password","You have entered incorrect password")
            # add deleter
        elif result == "notexist":
            messagebox.showerror("Username does not exist","The username you entered does not exist")
        else:
            messagebox.showerror("Unexpected validation result","Validation of credentials failed unexpectedly with result".format(result))

So here: Validating is an action, a method. The parameters of which aren't part of the helper object, but method parameters. The result is still a string (try making this an enumeration, that's more "correct" in a way as you really want to have a fixed set of possible results, not a string that can contain basically anything. Finally, the programmer error case is handled too (the last else).

Also note that ... is "foo" is likely incorrect, c.f. "foo" is "foobar"[0:3], is does a check for object identity, not equality! Not even numbers are same via is if they're not interned at the same time:

1000000 == 1000000 # _always_ True

x = 1000000
y = 1000000
x is y # _probably_ False

Also if x == True can obviously be simplified to if x.


So it's not all bad, simply try and stay away from "stringly"-typed things, use the language features as they're intended and have a goal for what your security requirements are. I've not gone into more detail on the last bit simply because I still don't understand the intent here. Suffice to say like this it would only work if the user can't exit the UI and open a shell / other means of executing code on the machine (like in a kiosk configuration with limited access to peripherals and the machine itself). Or if the actual database access was moved to a separate external service that (again) can't be circumvented by the local user.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.