4
\$\begingroup\$

Out of boredom, I decided to start my personal project and I've chosen a simple Text Password Manager.

Note: For anyone out there, I strongly recommend you NOT to use this for any sensitive storage purposes as it DOESN'T provide encryption yet!. That'll probably come in a later release.


About Safer

My project is going to be called Safer and these are the tools I've used so far:

  • Python 3.8
  • SQLAlchemy
  • SQLite3

Current features:

  • Retrieve all saved passwords.
  • Create a new password.
  • Retrieve a single password (by its name).
  • Update a single password (by its name).
  • Delete a single password (by its name).

Upcoming features (out of this review's purpose but it gives the reviewer some context):

  • Do all of the above only if a master password is provided (and it also matches the one from the DB).
  • Create a master password if it doesn't exist.
  • Encrypt all the passwords.

What I'd like to get out of this review:

  • Is there a better way to restructure this project?
  • Are the project files named correctly?
  • Is my code modular enough?
  • What about the logic? Would you use other approach over another when it comes any of the functionality in my code?
  • Did I stick to the DRY principle enough? If not, what can I improve?
  • Have I used SqlAlchemy as I should've?
  • UX - User experience
  • Wherever is room from improvement, please do tell ^_^

Right now, my project looks like this:

├── README.md
├── backend
│   ├── __init__.py  // nothing here
│   ├── main.py      // run program from here (will probably be moved to root dir in the future)
│   ├── models.py    // all the models used by SQLAlchemy
│   └── views.py     // not really views, actions for my models.
├── config.py        // store all the needed configs here
├── requirements.txt // self-explanatory
├── safer.db         // sqlite db file
└── setup.cfg        // various pep8, style, type-annotations config

The code:

main.py

"""Main entry to our app.

Contains all the needed calls.
"""

from typing import Optional, Iterable
import sys
from getpass import getpass

from views import (
    create_master_password,
    create_password,
    delete_password,
    get_password_by_name,
    is_master_password_valid,
    list_all_passwords,
    update_password,
)

VALID_MASTER_PASS_ANSWERS = (
    "Y",
    "y",
    "Yes",
    "yes",
    "N",
    "n",
    "No",
    "no",
)

VALID_ACTIONS = (
    "1",
    "2",
    "3",
    "4",
    "5",
    "9",
)


def get_name(prompt: str) -> str:
    """Keep asking for a valid name until one is given.

    Arguments:
        prompt (str): Prompt message.

    Returns:
        string - name of the password
    """
    while True:
        name = input(prompt)
        if not name:
            print(
                "Name cannot be empty. We suggest you insert a "
                "descriptive name for your password."
            )
            continue

        return name


def get_password(prompt: str) -> str:
    """Keep asking for a valid password until one is given.

    Arguments:
        prompt (str): Prompt message.

    Returns:
        string - password
    """
    while True:
        password = getpass(prompt)
        if not password:
            print("Password cannot be empty.")
            continue

        if len(password) < 8:
            print("WARNING! This is a weak password.")

        return password


def get_option(prompt: str, options: Optional[Iterable[str]] = None) -> str:
    """Keep asking for a valid option until one is given.

    Arguments:
        prompt (str): Prompt message.
        options (tuple): Options to choose from

    Returns:
        string - valid option
    """
    while True:
        option = input(prompt)
        if not option:
            print("Please enter an option.")
            continue

        if option not in options:
            valid_options = ", ".join(options)
            print(f"Invalid option. Valid options: {valid_options}")
            continue

        return option


def main() -> None:
    """Main entry to our program."""

    has_master_password = get_option(
        "Do you have a master password? [Y/n]: ",
        options=VALID_MASTER_PASS_ANSWERS,
    )

    if has_master_password in ("Y", "y", "Yes", "yes"):
        master_password = getpass("Insert your master password: ")

        if not is_master_password_valid(master_password):
            raise ValueError("Please insert a valid master key.")

        what_next = get_option(
            """Choose your next action:

        1. View all passwords.
        2. Create new password.
        3. Show password by name.
        4. Update password by name.
        5. Delete password by name.

        9. Quit

        > """,
            options=VALID_ACTIONS,
        )

        if what_next == "1":
            list_all_passwords()

        if what_next == "2":
            name = get_name("New password name (unique!): ")
            value = get_password("New password: ")

            create_password(name, value)

        if what_next == "3":
            name = get_name("Password name: ")

            get_password_by_name(name)

        if what_next == "4":
            name = get_name("Password name: ")
            value = get_password("New password: ")

            update_password(name, value)

        if what_next == "5":
            name = get_name("Password name: ")

            delete_password(name)

        if what_next == "9":
            sys.exit()

    else:
        master_password = getpass("Insert your new master password: ")

        create_master_password(master_password)


if __name__ == "__main__":
    main()

views.py

"""Views module.

Contains basic actions that can be done against
MasterPassword and Password models.
"""

from typing import Any, Optional, Tuple, Union

from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker
from tabulate import tabulate

from config import SQLITE_FILEPATH
from models import Base, MasterPassword, Password

ENGINE = create_engine(SQLITE_FILEPATH)
Base.metadata.create_all(ENGINE)

Session = sessionmaker(bind=ENGINE)


class SaferSession:
    """Context manager for ease of session management."""

    def __init__(
        self, record: Optional[Union[MasterPassword, Password]] = None
    ) -> None:
        """Simple constructor.

        Arguments:
            record (tuple): Optional argument used if provided.

        Returns:
            None
        """
        self.record = record

    def __enter__(self) -> sessionmaker():
        """Create a session object and return it.

        Returns:
            session object
        """
        self.session = Session()
        return self.session

    def __exit__(self, *args: Tuple[None]) -> None:
        """Make sure the session object gets closed properly.

        Arguments:
            args (tuple): Not really used. Can be None as well.

        Returns:
            None
        """
        if self.record:
            self.session.add(self.record)

        self.session.commit()
        self.session.close()


def create_master_password(master_password: str) -> None:
    """Create a master password.

    Arguments:
        master_password (str): Desired master password

    Returns:
        None
    """
    with SaferSession(record=MasterPassword(value=master_password)):
        print("Master password has been created!")


def create_password(name: str, value: str) -> None:
    """Create a password and a name for it.

    Arguments:
        name (str): Name of the password.
        value (str): The password.

    Returns:
        None
    """
    with SaferSession(record=Password(name, value)):
        print(f"Successfully added {name} record.")


def is_master_password_valid(master_password: str) -> Optional[bool]:
    """Check if provided master password is valid or not.

    Arguments:
        master_password (str): The master password.

    Returns:
        True if the password matches or None otherwise
    """
    with SaferSession() as session:
        password_obj = session.query(MasterPassword).one_or_none()
        return password_obj.value == master_password if password_obj else None


def get_password_by_name(name: str) -> Any:
    """Get a password by its name.

    Arguments:
        name (str): Name of the password.

    Returns:
        password or None
    """
    with SaferSession() as session:
        try:
            password = session.query(Password)
            password = password.filter_by(name=name).first().value
        except AttributeError:
            password = None
            print(f"{name} could not be found!")
        return password


def update_password(name: str, new_value: str) -> None:
    """Update a specific password.

    Arguments:
        name (str): Name of the password that needs updating.
        new_value (str): New password.

    Returns:
        None
    """
    with SaferSession() as session:
        try:
            password = session.query(Password).filter_by(name=name).first()
            password.value = new_value
            print(f"Successfully updated {name} record.")
        except AttributeError:
            print(f"{name} could not be found!")
            return


def delete_password(name: str) -> None:
    """Delete a specific password.

    Arguments:
        name (str): NAme of the password that needs to be deleted.

    Returns:
        None
    """
    with SaferSession() as session:
        try:
            session.query(Password).filter(Password.name == name).delete()
            print(f"Successfully deleted {name} record.")
        except AttributeError:
            print(f"{name} could not be found!")
            return


def list_all_passwords() -> None:
    """List all passwords.

    Returns:
        None
    """
    with SaferSession() as session:
        passwords = session.query(Password).all()

        if not passwords:
            print("No passwords stored yet!")
            return

        table = [
            [password_obj.name, password_obj.value]
            for password_obj in passwords
        ]

        print(tabulate(table, ["Password Name", "Password"], tablefmt="grid"))

models.py

"""Models module.

Contains all the needed models.
"""

from sqlalchemy import Column, DateTime, Integer, String, func
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class Password(Base):
    """Password model."""

    __tablename__ = "passwords"

    id = Column(Integer, primary_key=True)
    name = Column(String(128), nullable=False, unique=True)
    value = Column(String, nullable=False)
    updated = Column(DateTime, default=func.now())

    def __init__(self, name: str, value: str) -> None:
        """Simple constructor

        Arguments:
            name (str): Name of the password.
            value (str): Password.

        Returns:
            None
        """
        self.name = name
        self.value = value

    def __repr__(self) -> str:
        """Representation of the Password object.

        Returns:
            Representation of the Password object as str
        """
        return f"<Password(name='{self.name}', value='{self.value}')>"


class MasterPassword(Base):
    """Master Password model."""

    __tablename__ = "master_password"

    id = Column(Integer, primary_key=True)
    value = Column(String, nullable=False)
    updated_at = Column(DateTime, default=func.now())

    def __init__(self, value: str) -> None:
        """Simple constructor.

        Arguments:
            value (str): Master password.

        Returns:
            None
        """
        self.value = value

    def __repr__(self) -> str:
        """Representation of the Master Password object.

        Returns:
            Representation of the Master Password object as str
        """
        return f"<MasterPassword(value='{self.value}')>"

config.py

SQLITE_FILEPATH = 'sqlite:////path_to_project_root/safer.db'

setup.cfg

[pylama]
linters = mccabe,pep8,pycodestyle,pyflakes,mypy,isort
ignore=W293

[pylama:*/__init__.py]
ignore=W0611

[pylama:pydocstyle]
convention = google

[pylama:mccabe]
max-complexity = 2

[pydocstyle]
convention = google

[isort]
multi_line_output=3
include_trailing_comma=True
force_grid_wrap=0
use_parentheses=True
line_length=79

[mypy]
check_untyped_defs = true
disallow_any_generics = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_redundant_casts = true
warn_return_any = true
warn_unused_ignores = true

You can also clone the project from here. Don't forget to change the path in the config.py!

\$\endgroup\$
1
+50
\$\begingroup\$

BUG

Your code doesn't execute right now, I'm guessing because you recently moved source code: ModuleNotFoundError: No module named 'config'. (Works again if you move config.py to backend/.)


Your questions

Is there a better way to restructure this project? Are the project files named correctly?

I would move: the entry file (main.py; which you either could call that or rename to something like safer.py) out of your source directory (to the root dir), the database (safer.db) as well as the config file (config.py) out of the root dir. The config file may currently only have a single entry, but I would expect it to grow with the project. You can additionally use configparser for the config, and logging instead of some of your info and debug messages.

I also think that views.py is poorly named, given that you yourself write "not really views, actions for my models" about it.

Since some of your functions are "private", you could consider naming them with a leading underscore to signal this.

Is my code modular enough?

You should replace the URI in config.py to a relative path if the database comes with the project. Look at pathlib and be careful about different operating systems.

What about the logic? Would you use other approach over another when it comes any of the functionality in my code?

I would prefer to have the "front-end" more object-oriented (especially since you already use OOP), and I would separate the "back-end" from the inputs and outputs. It would make it easier if the project grows (say you wanted to add a GUI later), but also for troubleshooting and testing. I would expect a method for getting a list of all passwords instead of having a function that simply prints to stdout the list of all passwords (list_all_passwords()). I would also look at regular expressions for validation of inputs.

I think you missed an opportunity to use dundermethods for accessing members (your methods get_password_by_name, delete_password, etc.).

I also find it a little strange that you draw your menu and have your switch cases in one place, but you have a separate function for taking the input (get_option()).

Did I stick to the DRY principle enough? If not, what can I improve?

Your global variables (VALID_...) and their usage is a bit repeated.

UX - User experience

It's a bit annoying that it prints out the menu again after each invalid choice, so that the screen eventually stacks up with duplicates. It's also a bit surprising that the app exits after choosing an option in the menu (at least with choice #1).


Other things

Readability, standard practises

Code looks pythonic and nice in models.py and view.py, slightly less good in the "front-end" (entry) file. I would also have liked to see some tests.

I think you over-document a little, a good example being:

    def __repr__(self) -> str:
        """Representation of the Password object.
        Returns:
            Representation of the Password object as str
        """
        return f"<Password(name='{self.name}', value='{self.value}')>"

I think you can assume that most readers will know what repr is and does.

I also saw that you only have three commits on your repo. You may want to work on your version control workflow.

Security

I don't think you should allow any type of password, and I think you should more than just notify the user that they've selected an insecure password. If you don't want to force strict passwords, you can just ask them to enter an insecure one again to confirm.

Context manager

I like the idea of a context manager your sessions, but be careful to handle potential errors in your __exit__ function.

Surprising behaviour/prompt

In the same vein, raise errors in your back-end but deal with them yourself in the front-end; don't do this:

    if not is_master_password_valid(master_password):
        raise ValueError("Please insert a valid master key.")

Refactoring

Some of your if-clauses should be elif (or you could refactor to dicts), and I would prefer to see your loops reworked.


PS.

  • Since you use typing anyway, you can use typing.NoReturn for your side-effect-only type-hints.

  • You don't need the __init__.py since Python3.3.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Encryption is not enough

In addition to your eventual encryption, you need to take measures to protect your data at the operating system level. At the least, make sure that the permissions are restrictive - this is possible on Windows, MacOS and Linux using various methods.

Sets

VALID_MASTER_PASS_ANSWERS and VALID_ACTIONS should be sets. Also, just store the lower-case versions of your answers, and convert input to lower-case for the purposes of case-insensitive comparison. As for valid actions, they're all integers - so store them as integers, and convert your input to an integer.

The case and set suggestions also apply to

if has_master_password in ("Y", "y", "Yes", "yes"):

Password strength

Length is not enough. Do a basic English word pass at the least. Since this is specifically a password management program you might want to do something more thorough like entropy measurement - there are libraries for this.

Redundant return

Drop the return from this:

    except AttributeError:
        print(f"{name} could not be found!")
        return
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

In get_option, you have a while loop:

while True:
    option = input(prompt)
    if not option:
        print("Please enter an option.")
        continue

    if option not in options:
        valid_options = ", ".join(options)
        print(f"Invalid option. Valid options: {valid_options}")
        continue

    return option

I think this would make more sense by making use of elif and else and dropping the continues:

while True:
    option = input(prompt)
    if not option:
        print("Please enter an option.")

    elif option not in options:
        valid_options = ", ".join(options)
        print(f"Invalid option. Valid options: {valid_options}")

    else:
        return option

And then similarly in get_password.


And then another similar case in get_name:

while True:
    name = input(prompt)
    if not name:
        print(
            "Name cannot be empty. We suggest you insert a "
            "descriptive name for your password."
        )
        continue

    return name

I think it would be much simpler to return at the top, instead of returning at the bottom and trying to divert execution away from the return using continue:

while True:
    name = input(prompt)
    if name:
        return name

    else:
        print("Name cannot be empty. We suggest you insert a "
              "descriptive name for your password.")

I also recommend tightening up the print as I have there. There's a point where spreading things out and making your function longer begins to hurt readability.

A fun party-trick suggestion though: that can actually be made even more succinct if you're using Python3.8+:

while True:
    if name := input(prompt):
        return name
    . . .

:= is an assignment expression.


Your menu dispatch in main should be using elifs:

if what_next == "1":
        list_all_passwords()

elif what_next == "2":
    name = get_name("New password name (unique!): ")
    value = get_password("New password: ")

. . .

You know that those checks will always be exclusive of each other (only one can ever be true). If "what_next == "1" is true, you're still doing all the rest of the checks whenlist_all_passwords returns, which is wasteful. It'll make a negligible here, but avoiding unnecessary overhead is a good habit to get into.


There's no need to include -> None. When type hinting __init__, since it must return None.


get_password_by_name could be cleaned up a bit too. You have:

with SaferSession() as session:
    try:
        password = session.query(Password)
        password = password.filter_by(name=name).first().value
    except AttributeError:
        password = None
        print(f"{name} could not be found!")
    return password

I'm not a fan of reassigning variable in most cases. If you want to debug and see intermittent results, you need to catch it before the second reassignment happens. I don't know what session.query(Password) returns, but is it itself a password? I think I'd give it a different name. This can be simplified though to remove that need:

with SaferSession() as session:
    try:
        result = session.query(Password)
        return result.filter_by(name=name).first().value

    except AttributeError:
        print(f"{name} could not be found!")
        return None
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ print("WARNING! This is a weak password.") wasn't really a bug because I didn't want to impose to the user to enter a stronger password but just to warn him and still let him do the action. Anyway, I'm still trying to decide if I want to impose the security to the users or just use w/e they'd like. Any opinion on this? \$\endgroup\$ – Grajdeanu Alex. Apr 6 at 14:13
  • \$\begingroup\$ @GrajdeanuAlex.You could allow them to initially pass in some kind of "policy" object that indicates a strictness level. It could just be an Enum with different levels like STRICT, LENIANT, NO_RESTRICTIONS. Then check for that internally and change your tests accordingly. \$\endgroup\$ – Carcigenicate Apr 6 at 14:15

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.