Simple password generator with a GUI

Question

I made a simple password-generating app with a GUI written in Python and the tkinter library.

Here's how the GUI looks like:

The passwords are strings composed of random characters. The user can choose the length of the password, and the characters that are allowed to be in it.

Here's the source code:

import string
import tkinter as tk
from tkinter import ttk
import random
from tkinter.constants import DISABLED, E, END, NORMAL, NW, VERTICAL


class GUI(tk.Frame):

    def __init__(self, master):
        super().__init__(master)
        self.pack()
        self.widget_vars()
        self.create_widgets()
        self.style()

    def generate_password(self):
        passw = Password(self.length.get(), self.lower.get(), self.upper.get(),
                         self.digits.get(), self.punct.get())
        # You can only insert to Text if the state is NORMAL
        self.password_text.config(state=NORMAL)
        self.password_text.delete("1.0", END)   # Clears out password_text
        self.password_text.insert(END, passw.password)
        self.password_text.config(state=DISABLED)

    def widget_vars(self):
        self.length = tk.IntVar(self, value=16)
        self.lower = tk.BooleanVar(self, value=True)
        self.upper = tk.BooleanVar(self, value=True)
        self.digits = tk.BooleanVar(self, value=True)
        self.punct = tk.BooleanVar(self, value=True)

    def create_widgets(self):
        # Define widgets
        self.lower_checkbtn = ttk.Checkbutton(self, variable=self.lower)
        self.lower_label = ttk.Label(self, text="string.ascii_lower")
        self.upper_checkbtn = ttk.Checkbutton(self, variable=self.upper)
        self.upper_label = ttk.Label(self, text="string.ascii_upper")
        self.digits_checkbtn = ttk.Checkbutton(self, variable=self.digits)
        self.digits_label = ttk.Label(self, text="string.digits")
        self.punct_checkbtn = ttk.Checkbutton(self, variable=self.punct)
        self.punct_label = ttk.Label(self, text="string.punctuation")
        self.length_spinbox = ttk.Spinbox(self, from_=1, to=128, width=3,
                                          textvariable=self.length)
        self.length_label = ttk.Label(self, text="Password length")
        self.separator = ttk.Separator(self, orient=VERTICAL)
        self.generate_btn = ttk.Button(self, text="Generate password",
                                       command=self.generate_password)
        self.password_text = tk.Text(self, height=4, width=32, state=DISABLED)

        # Place widgets on the screen
        self.length_label.grid(column=0, row=0, rowspan=4, sticky=E)
        self.length_spinbox.grid(column=1, row=0, rowspan=4, padx=4, pady=2)
        self.lower_label.grid(column=3, row=0, sticky=E, padx=4)
        self.lower_checkbtn.grid(column=4, row=0, padx=4, pady=2)
        self.upper_label.grid(column=3, row=1, sticky=E, padx=4)
        self.upper_checkbtn.grid(column=4, row=1, padx=4, pady=2)
        self.digits_label.grid(column=3, row=2, sticky=E, padx=4)
        self.digits_checkbtn.grid(column=4, row=2, padx=4, pady=2)
        self.punct_label.grid(column=3, row=3, sticky=E, padx=4)
        self.punct_checkbtn.grid(column=4, row=3, padx=4, pady=2)
        self.separator.grid(column=2, row=0, rowspan=4, ipady=45)
        self.generate_btn.grid(columnspan=5, row=4, padx=4, pady=2)
        self.password_text.grid(columnspan=5, row=6, padx=4, pady=2)

        self.grid(padx=10, pady=10)

    def style(self):
        self.style = ttk.Style(self)
        self.style.theme_use("clam")


class Password:

    def __init__(self, length: int,
                 allow_lowercase: bool,
                 allow_uppercase: bool,
                 allow_digits: bool,
                 allow_punctuation: bool) -> None:
        self.length = length
        self.allow_lowercase = allow_lowercase
        self.allow_uppercase = allow_uppercase
        self.allow_digits = allow_digits
        self.allow_punctuation = allow_punctuation
        self.allowed_chars = self.gen_allowed_chars()
        self.password = self.gen_password()

    def gen_allowed_chars(self) -> str:
        # I use a string, because random.choice doesn't work with sets:
        chars = ''
        if self.allow_lowercase:
            chars += string.ascii_lowercase
        if self.allow_uppercase:
            chars += string.ascii_uppercase
        if self.allow_digits:
            chars += string.digits
        if self.allow_punctuation:
            chars += string.punctuation
        return chars

    def gen_password(self) -> str:
        password = ''
        for _ in range(self.length):
            password += random.choice(self.allowed_chars)
        return password


if __name__ == '__main__':
    root = tk.Tk()
    root.title("Password Generator")
    app = GUI(root)
    app.mainloop()

Can this code be improved in any way? Does this code follow common best practices? I'd appreciate some advice, especially on the GUI class (I'm new to tkinter).

Thank You in advance.

Answer 1

• Don't use random for passwords, use secrets
• "has-a-root" is a cleaner pattern here, I think, than "is-a-root"; in other words, don't inherit - instantiate
• Cut down the repetition in your options by generalizing to a collection of strings, each expected to be an attribute name on the string module. Represent this name consistently between the UI and the module lookup logic.
• Type-hint your method signatures.
• Prefer ''.join() over successive concatenation
• Try to avoid assigning new class members outside of __init__.
• Where possible, reduce the number of references you keep on your GUI class. Almost none of your controls actually need to have references kept.
• Do not call mainloop on your frame; call it on your root
• Name your variables
• Sort your grid declarations according to column and row
• Your Password is not a very useful representation of a class. Whether or not it is kept as-is, it should be made immutable. Also, distinguish between a password and a password generator. A password generator knowing all of its generator parameters but having no actual password state would be more useful. After such a representation is implemented, you could change your TK logic to trace on all of your options variables, and only upon such a change trace, re-initialize your generator. Repeat clicks on 'Generate' will reuse the same generator instance.
• Don't call things master. In this case "parent" is more appropriate.

Suggested

import secrets
import string
import tkinter as tk
from dataclasses import dataclass, field
from tkinter import ttk
from tkinter.constants import DISABLED, E, END, NORMAL, VERTICAL
from typing import Iterable, Collection, Protocol, Literal


TraceMode = Literal[
    'r',  # read
    'w',  # write
    'u',  # undefine
    'a',  # array
]


class TkTrace(Protocol):
    def __call__(self, name: str, index: str, mode: TraceMode): ...


class OptControl:
    NAMES = ('ascii_lowercase', 'ascii_uppercase', 'digits', 'punctuation')

    def __init__(self, parent: tk.Widget, name: str, trace: TkTrace) -> None:
        self.name = name
        self.var = tk.BooleanVar(parent, name=name, value=True)
        self.var.trace(mode='w', callback=trace)
        self.label = ttk.Label(parent, text=name)
        self.check = ttk.Checkbutton(parent, variable=self.var)

    @classmethod
    def make_all(cls, parent: tk.Widget, trace: TkTrace) -> Iterable['OptControl']:
        for name in cls.NAMES:
            yield cls(parent, name, trace)


class GUI:
    def __init__(self, parent: tk.Tk):
        self.root = tk.Frame(parent)
        self.root.pack()
        self.length = tk.IntVar(self.root, value=16)
        self.length.trace('w', self.opt_changed)
        self.opts = tuple(OptControl.make_all(self.root, self.opt_changed))
        self.password_text = self.create_widgets()
        self.style()
        self.opt_changed()

    @property
    def selected_opts(self) -> Iterable[str]:
        for opt in self.opts:
            if opt.var.get():
                yield opt.name

    def generate_password(self) -> None:
        # You can only insert to Text if the state is NORMAL
        self.password_text.config(state=NORMAL)
        self.password_text.delete('1.0', END)   # Clears out password_text
        self.password_text.insert(END, self.generator.gen_password())
        self.password_text.config(state=DISABLED)

    def opt_changed(self, *args) -> None:
        self.generator = PasswordGenerator(
            length=self.length.get(),
            opts=tuple(self.selected_opts),
        )

    def create_widgets(self) -> tk.Text:
        length_label = ttk.Label(self.root, text='Password length')
        length_label.grid(column=0, row=0, rowspan=4, sticky=E)

        generate_btn = ttk.Button(
            self.root, text='Generate password', command=self.generate_password)
        generate_btn.grid(column=0, row=4, columnspan=5, padx=4, pady=2)

        password_text = tk.Text(self.root, height=4, width=32, state=DISABLED)
        password_text.grid(column=0, row=6, columnspan=5, padx=4, pady=2)

        length_spinbox = ttk.Spinbox(
            self.root, from_=1, to=128, width=3, textvariable=self.length)
        length_spinbox.grid(column=1, row=0, rowspan=4, padx=4, pady=2)

        separator = ttk.Separator(self.root, orient=VERTICAL)
        separator.grid(column=2, row=0, rowspan=4, ipady=45)

        for row, opt in enumerate(self.opts):
            opt.label.grid(column=3, row=row, sticky=E, padx=4)
            opt.check.grid(column=4, row=row, padx=4, pady=2)

        self.root.grid(padx=10, pady=10)

        return password_text

    def style(self) -> None:
        style = ttk.Style(self.root)
        style.theme_use('clam')


@dataclass(frozen=True)
class PasswordGenerator:
    length: int
    opts: Collection[str]
    allowed_chars: str = field(init=False)

    def __post_init__(self):
        super().__setattr__('allowed_chars', ''.join(self._gen_allowed_chars()))

    def gen_password(self) -> str:
        return ''.join(self._gen_password_chars())

    def _gen_allowed_chars(self) -> Iterable[str]:
        for opt in self.opts:
            yield getattr(string, opt)

    def _gen_password_chars(self) -> Iterable[str]:
        for _ in range(self.length):
            yield secrets.choice(self.allowed_chars)


if __name__ == '__main__':
    root = tk.Tk()
    root.title('Password Generator')
    GUI(root)
    root.mainloop()

Answer 2

I don't think Password needs to be a class. __init__ is calling gen_password, so this really could be boiled down to a single function call:

def gen_password(length: int, lowercase: bool, uppercase: bool, digits: bool, punctuation: bool) -> str:
    char_groups = {
        'lowercase': string.ascii_lowercase,
        'uppercase': string.ascii_uppercase,
        'digits': string.digits,
        'punctuation': string.punctuation
    }

    # gets the corresponding keys for any True arguments in the
    # function call
    groups = (grp for grp, arg in zip(
        ('lowercase', 'uppercase', 'digits', 'punctuation'), 
        (lowercase, uppercase, digits, punctuation)
    ) if arg)

    # your set of allowed chars gets generated here
    allowed_chars = ''.join((char_groups[group] for group in groups))

    # instead of the for loop, just sample the chars
    return ''.join(random.sample(allowed_chars, k=length))

Which changes your generate_password function to look like:

    def generate_password(self):
        # I'd use keywords here for clarity
        passw = gen_password(
            length=self.length.get(), 
            lowercase=self.lower.get(), 
            uppercase=self.upper.get(),
            digits=self.digits.get(), 
            punctuation=self.punct.get()
        )
        # You can only insert to Text if the state is NORMAL
        self.password_text.config(state=NORMAL)
        self.password_text.delete("1.0", END)   # Clears out password_text
        self.password_text.insert(END, passw)
        self.password_text.config(state=DISABLED)