6
\$\begingroup\$

This is a follow-up (somewhat) to this post. I said somewhat because I've kinda changed most of the logic.

Changes:

  • I've changed the project name
  • I've changed the CLI UX
  • I've added encryption / decryption
  • I've added a setup.py for easier installation.
  • I've added click
  • Temporarily removed type annotations & docstrings.

Review:

  • I'm not quite confident about my SQLAlchemy models (models.py) and how I used the PasswordMixin (if it's even worth adding it for only two models). Any advice on this?

  • The same as above goes for the PasswordViewMixin (views.py). Also, I don't like it how I kinda duplicated the logic of the methods in a model and it's specific view class. Any way of avoiding that?

  • Any OOP paradigms that I might have misused / not used etc.

  • I'd also like an overall review on the project as a whole even on small things like: project structure, naming best practices (e.g: I don't know if the views.py file should be called like that but it seemed right at that moment of writing.), the content of README.md, the setup.py file and so on.

  • Improvements regarding encryption/decryption workflow

  • Improvement regarding usage of the click library

Code

For those of you who want to run this locally, here is the github repository.

models.py

import os
import sys

from psycopg2 import OperationalError
from sqlalchemy import Column, Integer, String, create_engine
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import sessionmaker


ENGINE = create_engine(f'sqlite:///{os.path.dirname(os.path.dirname(__file__))}/dinopass.db')
SESSION = sessionmaker(bind=ENGINE)
Base = declarative_base()


class PasswordMixin:
    id = Column(Integer, primary_key=True)

    @classmethod
    def create(cls, **kwargs):
        return cls(**kwargs)

    @classmethod
    def get(cls, session):
        return session.query(cls).first()

    @classmethod
    def has_records(cls, session):
        return cls.get(session)

    @classmethod
    def purge(cls, session):
        return session.query(cls).delete()


class MasterPassword(Base, PasswordMixin):
    __tablename__ = 'master_password'

    salt = Column(String, nullable=False)
    hash_key = Column(String, nullable=False)

    def __init__(self, salt, hash_key):
        self.salt = salt
        self.hash_key = hash_key


class Password(Base, PasswordMixin):
    __tablename__ = 'passwords'

    name = Column(String, nullable=False, unique=True)
    value = Column(String, nullable=False)

    def __repr__(self):
        return f"<Password(name='{self.name}')>"

    def __str__(self):
        return f"<Password(name='{self.name}', value='***')>"

    def __init__(self, name, value):
        self.name = name
        self.value = value

    @classmethod
    def get_all(cls, session):
        return session.query(cls).all()

    @classmethod
    def get_by_name(cls, name, session):
        return session.query(cls).filter_by(name=name).first()

    @classmethod
    def update_by_field(cls, field, value, field_to_update, new_value, session):
        if not getattr(cls, field) and not isinstance(field, str):
            raise AttributeError(f'Invalid attribute name: {field}')

        if not getattr(cls, field_to_update) and not isinstance(field_to_update, str):
            raise AttributeError(f'Invalid field_to_update name: {field_to_update}')

        return session.query(cls).filter_by(**{field: value}).update({field_to_update: new_value})

    @classmethod
    def delete_by_name(cls, name, session):
        return session.query(cls).filter_by(name=name).delete()

    def to_dict(self):
        record = vars(self)
        record.pop('_sa_instance_state')
        record.pop('id')
        return record


try:
    Base.metadata.create_all(ENGINE)
except OperationalError as operational_error:
    sys.exit(f'Error when connecting to DB: {operational_error}. '
             f'Please make sure you have correctly set up your DB!')

views.py

from dinopass.encryption import encrypt, decrypt
from dinopass.models import MasterPassword, Password

from sqlalchemy.exc import IntegrityError


class PasswordViewMixin:
    model = None

    def __init__(self, db_session):
        if not self.model:
            raise NotImplementedError('Please specify a model!')

        self._db_session = db_session

    def get(self):
        return self.model.get(self._db_session)

    def purge(self):
        self.model.purge(self._db_session)
        self._db_session.commit()

    def has_records(self):
        return self.model.has_records(self._db_session)


class MasterPasswordView(PasswordViewMixin):
    model = MasterPassword

    @property
    def salt(self):
        return self.model.get(self._db_session).salt

    @property
    def hash_key(self):
        return self.model.get(self._db_session).hash_key

    def create(self, **kwargs):
        try:
            record = self.model.create(**kwargs)

            self._db_session.add(record)
            self._db_session.commit()

            return record
        except IntegrityError as integrity_error:
            self._db_session.rollback()
            return {'error': f'{str(integrity_error)}'}

    def is_valid(self, hash_key):
        return hash_key == self.hash_key


class PasswordView(PasswordViewMixin):
    model = Password

    @property
    def name(self):
        return self.model.get(self._db_session).name

    @property
    def value(self):
        return self.model.get(self._db_session).value

    def create(self, key, name, value):
        encrypted_value = encrypt(key, value)

        try:
            record = self.model.create(name=name, value=encrypted_value)

            self._db_session.add(record)
            self._db_session.commit()

            return record
        except IntegrityError as integrity_error:
            self._db_session.rollback()
            return {'error': f'{str(integrity_error)}'}

    def get_all(self, key):
        records = []
        for record in self.model.get_all(self._db_session):
            record.value = decrypt(key, record.value)
            records.append(record.to_dict())
        return records

    def get_by_name(self, key, name):
        record = self.model.get_by_name(name, self._db_session)
        if record:
            record.value = decrypt(key, record.value)
            return [record.to_dict()]
        return []

    def update(self, key, field, value, field_to_update, new_value):
        if field_to_update == 'value':
            new_value = encrypt(key, new_value)

        try:
            self.model.update_by_field(
                field=field,
                value=value,
                field_to_update=field_to_update,
                new_value=new_value,
                session=self._db_session
            )
            self._db_session.commit()
            return f'Successfully updated record matching {field}={value} ' \
                   f'with {field_to_update}={new_value}.'
        except IntegrityError as integrity_error:
            self._db_session.rollback()
            return f'{str(integrity_error)}'

    def delete(self, name):
        try:
            self.model.delete_by_name(name=name, session=self._db_session)
            self._db_session.commit()
            return f'Successfully deleted record with name={name}.'
        except IntegrityError as integrity_error:
            self._db_session.rollback()
            return f'{str(integrity_error)}'

encryption.py

import base64
import hashlib

from cryptography.fernet import Fernet, InvalidToken
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC


def generate_hash_key(master_password):
    return hashlib.sha512(master_password.encode()).hexdigest()


def generate_key_derivation(salt, master_password):
    """Generate Fernet Key:
    salt: os.urandom(16)
    password: bytes
    """

    kdf = PBKDF2HMAC(
        algorithm=hashes.SHA256(),
        length=32,
        salt=salt,
        iterations=100000,
        backend=default_backend()
    )
    key = base64.urlsafe_b64encode(kdf.derive(master_password.encode()))
    return key


def encrypt(key, value_to_encrypt):
    f = Fernet(key)
    encrypted_value = f.encrypt(value_to_encrypt.encode())
    return encrypted_value


def decrypt(key, encrypted_value):
    f = Fernet(key)
    try:
        return f.decrypt(encrypted_value).decode()
    except InvalidToken:
        return b''

helpers.py

from rich.console import Console
from rich.table import Table


def pp(title, data):
    title = f'[bold red][u]{title}[/u][/bold red]'
    table = Table(title=title, show_lines=True)

    console = Console()

    table.add_column("NAME", justify="center", style="magenta", no_wrap=True)
    table.add_column("PASSWORD", justify="center", style="bold green", no_wrap=True)

    for item in data:
        table.add_row(item['name'], item['value'])

    console.print(table)

cli.py

import os
import sys

from dinopass.encryption import generate_hash_key, generate_key_derivation
from dinopass.helpers import pp
from dinopass.models import SESSION
from dinopass.views import MasterPasswordView, PasswordView

import click


SALT_LENGTH = 16


@click.group(help="Simple CLI Password Manager for personal use")
@click.pass_context
def main(ctx):
    session = SESSION()

    password_view = PasswordView(session)
    master_password_view = MasterPasswordView(session)

    if master_password_view.has_records():
        master_password = click.prompt('Please enter your master password: ', hide_input=True)

        hash_key = generate_hash_key(master_password)
        key_derivation = generate_key_derivation(
            master_password_view.salt,
            master_password
        )

        if master_password_view.is_valid(hash_key):
            ctx.obj['key_derivation'] = key_derivation
            ctx.obj['password_view'] = password_view
        else:
            sys.exit('Invalid master password')
    else:
        if click.confirm(f'It looks like you do not have a master password yet. '
                         f'Would you like to create one now?', abort=True):

            master_password = click.prompt('Please enter your master password: ', hide_input=True)

            salt = os.urandom(SALT_LENGTH)
            hash_key = generate_hash_key(master_password)
            key_derivation = generate_key_derivation(salt, master_password)

            master_password_view.create(salt=salt, hash_key=hash_key)

            ctx.obj['key_derivation'] = key_derivation
            ctx.obj['password_view'] = password_view


@main.command(help='List all credentials.')
@click.pass_context
def all(ctx):
    password_view = ctx.obj['password_view']
    key_derivation = ctx.obj['key_derivation']

    data = password_view.get_all(key_derivation)
    if not data:
        click.echo('\n\nThere are no credentials stored yet\n\n')

    pp(title='ALL CREDENTIALS', data=data)


@main.command(help='Purge all credentials.')
@click.pass_context
def purge(ctx):
    if click.confirm(f'Are you sure you want to purge ALL the records?', abort=True):
        password_view = ctx.obj['password_view']
        password_view.purge()
        click.echo('\n\nALL the records have been deleted!\n\n')


@main.command(help='Create a new password with a specific name.')
@click.option('--name', prompt=True, help='Name of the password.')
@click.option('--password', prompt=True, hide_input=True, help='Your new password.')
@click.pass_context
def create(ctx, name: str, password: str):
    password_view = ctx.obj['password_view']
    key_derivation = ctx.obj['key_derivation']

    record = password_view.create(key_derivation, name, password)

    if hasattr(record, 'name'):
        click.echo(f'\n\nSuccessfully created record with name={name}\n\n')
    else:
        click.echo(f'\n\n{record["error"]}\n\n')


@main.command(help='Get a specific credential by name.')
@click.option('--name', prompt=True, help='Name of the password.')
@click.pass_context
def get(ctx, name: str):
    password_view = ctx.obj['password_view']
    key_derivation = ctx.obj['key_derivation']

    data = password_view.get_by_name(key_derivation, name)
    if not data:
        click.echo(f'\n\nThere is no record with name={name}\n\n')
        return
    pp(title=f'CREDENTIAL for {name}', data=data)


@main.command(help='Update a credential field matching a specific condition with a new value.')
@click.option('--field', prompt=True, help='Name of the field.')
@click.option('--value', prompt=True, help='Value of the field.')
@click.option('--field_to_update', prompt=True, help='Name of the field to update.')
@click.option('--new_value', prompt=True, help='New value')
@click.pass_context
def update(ctx, field: str, value: str, field_to_update: str, new_value: str):
    password_view = ctx.obj['password_view']
    key_derivation = ctx.obj['key_derivation']

    password_view.update(key_derivation, field, value, field_to_update, new_value)


@main.command(help='Delete a specific credential by name.')
@click.option('--name', prompt=True, help='Name of the password.')
@click.pass_context
def delete(ctx, name: str):
    if click.confirm(f'Are you sure you want to delete {name} record?', abort=True):
        password_view = ctx.obj['password_view']
        password_view.delete(name)
        click.echo(f'The record with name={name} has been deleted!')


def start():
    main(obj={})


if __name__ == '__main__':
    start()

What the code does

This is basically a simple CLI Password manager which, via CLI, should let you manage your passwords. For this, the application needs a master password and ask for one each time you're doing an action (At first run you'll be asked to create one which is going to be saved in MasterPassword model. All the other credentials are going to be saved to the Password model.

The following actions can be done:

  • List all your passwords (WARNING: It's going to be in clear text!)
  • Purge all your passwords (WARNING: This is permanent so do it at your own risk!)
  • Create a new password
  • Update an existing password
  • Retrieve an existing password (by name)
  • Delete an existing password

Running a command is as simple as:

python3 cli.py <command>

Or, if you've installed the app via setup.py:

dinopass <command>
\$\endgroup\$
0
5
+200
\$\begingroup\$

Looks like you got a good start, but there's plenty left to improve. Considering you're doing this as a one-man project, I imagine there will always be minor issues.

First of all, initial set-up. On a fresh, barebones Python installation your program will miss a lot of dependencies. It looks like some of those will be hauled in during the installation, but not completely:

cryptography has a tricky installation, possibly due to requiring Microsoft Visual C++ as an external dependency itself.

sqlalchemy typing-extensions pygments colorama commonmark pprintpp psycopg2 were still missing after the set-up as well.

There's inconsistent use of interpunction in your usermessages and I haven't found a method of destroying the master record completely (purge removes everything but the master password). When trying to pass arguments to the commands like the usage example, it ignores the arguments, asks for the parameters anyway like when not passing getting passed any arguments and then fails for no reason but having unexpected arguments.

>dinopass create NewUser Passw3
Please enter your master password: :
Name:
Name:
Name: NewUser
Password:
Usage: dinopass create [OPTIONS]
Try 'dinopass create --help' for help.

Error: Got unexpected extra arguments (NewUser Passw3)

Note that the usage guide states:

Usage: dinopass [OPTIONS] COMMAND [ARGS]...

Turns out the arguments are named.

>dinopass create --help
Please enter your master password: :
Usage: dinopass create [OPTIONS]

  Create a new password with a specific name.

Options:
  --name TEXT      Name of the password.
  --password TEXT  Your new password.
  --help           Show this message and exit.

That could've been more explicit, I guess. Do note that it requires a master password just to get to the --help of a command. You know, the password that can't be purged. So the UX could use a bit of work.

Now, the code.

You have the PasswordMixin, Password and MasterPassword in the same file. That's good. They're all very much tied together. I'm not sure models.py is the best name for it, but it will definitely suffice. Good use of decorators too. Is it worth having PasswordMixin just for the two other classes? I think so. Classes usually grow in size faster than they shrink again, so the value may become even greater in time. It's a great way of keeping things simpler and not repeating yourself.

The next file is called helpers.py. With a function called pp. pp is a terrible name. What are you doing here, redefining prettyprint? It handles the printing of the data table, but you can't tell by the name of the function.

def pp(title, data):
    title = f'[bold red][u]{title}[/u][/bold red]'
    table = Table(title=title, show_lines=True)

That's 5 title in 3 lines of code and it's actually 2 variations. You're redefining title here. Perhaps one of them could be named better to differentiate between them.

You got a decent separation of concerns going on between your files. However, I do think checking for the IntegrityError should be part of the MasterPassword class itself and not of the MasterPasswordViewer. The viewer shouldn't be concerned with something relatively low-level like that. But moving it is going to be non-trivial. Almost like your viewer is doing too much already.

The encryption definitely isn't the worst I've seen with hobby projects so I wouldn't worry too much about that at the moment. The usage of click is sensible too. It saves you a lot of boilerplate, that's usually a good thing with projects like this.

\$\endgroup\$
2
  • \$\begingroup\$ To expand on "were still missing after the set-up as well." It is possible to test this by simply using Tox/Nox as they setup a new virtual environment as part of the testing procedure. \$\endgroup\$ – Peilonrayz Jun 14 '20 at 17:51
  • 1
    \$\begingroup\$ @Peilonrayz Feel free to expand on that in an answer. I've lined it up, you bring it home. \$\endgroup\$ – Mast Jun 14 '20 at 18:19
3
\$\begingroup\$

This is an expansion of @Mast's great answer.

sqlalchemy typing-extensions pygments colorama commonmark pprintpp psycopg2 were still missing after the set-up as well.

Whilst when I installed it just now, I got most of these packages I didn't have psycop2. This is coming from an improperly configured setuptools package. We can see neither setup.py or requirements.txt have all of these packages listed.

You can test for this by using Tox or Nox. This is because both build a virtualenv for each test environment. Whilst this is primarily to be able to test one project over multiple Python versions, it has the benefit of being able to test your package before deployment. If you use a src layout then you can only import your code from the installed package, rather than from the current working directory. Meaning you can test if the built package works and contains all the information you need. This is useful if you're deploying assets with your Python package and need to test that they are built and deployed correctly.

As a contributor to Nox I'm more familiar with it then Tox and so I'll be focusing on that. But they both work in a similar way, it just comes down to which configuration file you want to use Python or an INI.

  1. We need to have a unit test. This can simply just be an assert True.

  2. We have to import your package and hope it imports all the needed imports. As your tests grow to cover all of your files (not lines of code) then all imports should be hit and this will be properly tested.

  3. We get pytest or unittest to run stand alone.

    $ pytest
    
  4. We build the Tox/Nox file running the single command from ¶3

tests/test_dinopass.py

import dinopass


def test_dinopass():
    assert True

noxfile.py

import nox


@nox.session()
def test(session):
    session.install("-e", ".")
    session.install("pytest")
    session.run("pytest")

Now you can just test your packages are installed correctly by using nox. Later you can add more tests and also run these just from one nox call. Personally I use nox to run all tests, coverage, linters, hinters and documentation. Integration with CI tools is then super simple. For example my .travis.yml just builds the test matrix and simply calls nox.

\$\endgroup\$

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.