Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes related to Debian packaging work #86

Merged
merged 9 commits into from
Nov 1, 2018
15 changes: 15 additions & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
include requirements.txt
include README.md
include LICENSE
include setup.py
include Pipfile
include Pipfile.lock
include files/alembic.ini
include files/client.ini
include files/securedrop-client

recursive-include alembic *
recursive-include securedrop_client *

recursive-exclude alembic *.pyc
recursive-exclude securedrop_client *.pyc
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@ pyflakes:
find . \( -name _build -o -name var -o -path ./docs -o -path \) -type d -prune -o -name '*.py' -print0 | $(XARGS) pyflakes

pycodestyle:
find . \( -name _build -o -name var \) -type d -prune -o -name '*.py' -print0 | $(XARGS) -n 1 pycodestyle --repeat --exclude=build/*,docs/*,.vscode/* --ignore=E731,E402,W504
find . \( -name _build -o -name var \) -type d -prune -o -name '*.py' -print0 | $(XARGS) -n 1 pycodestyle --repeat --exclude=build/*,docs/*,.vscode/*,setup.py --ignore=E731,E402,W504

check: clean pycodestyle pyflakes test
14 changes: 7 additions & 7 deletions Pipfile.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions alembic/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
# add your model's MetaData object here
# for 'autogenerate' support
sys.path.insert(0, path.realpath(path.join(path.dirname(__file__), '..')))
# This path is purely for alembic to work on the packaged application
sys.path.insert(1, "/opt/venvs/securedrop-client/lib/python3.5/site-packages")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? Shouldn't we keep the alembic directory in a fix position relative to the app? The first path insertion should catch every case, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is how it can help in packaging. We need static config files to able to package them easily and without any dynamic magic at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand. What is the exact case that you're describing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first path is for the development environment, and the second path is for the case where we are running using Debian package. The alembic directory is installed in the standard /usr/share/securedrop-workstation/ location.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't the alembic directory placed somewhere relative to the source code? Wouldn't that be simpler?

from securedrop_client.models import Base # noqa
target_metadata = Base.metadata

Expand Down
5 changes: 5 additions & 0 deletions createdb.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/usr/bin/env python3
import sys

from securedrop_client.models import Base, make_engine
Base.metadata.create_all(make_engine(sys.argv[1]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment got lost, but I still think this is better as a here-doc since it's just for the dev env and it's simple enough to just shove into the bash script.

74 changes: 74 additions & 0 deletions files/alembic.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# A generic, single database configuration.

[alembic]
# path to migration scripts
script_location = /usr/share/securedrop-client/alembic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to be running alembic, we shouldn't be using a hard coded value you like this. We should use a template and a %%ALEMBIC_PATH%% or something similar to replace the values in a pre-alembic step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to help the Debian packaging. All packaged applications (as far as I know) in Fedora land also uses the same idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the alembic.ini is useless without this line

sqlalchemy.url = sqlite:////path/to/database.sqlite

The app at boot could pull an alembic.ini.tmpl from /var/lib/ or /usr/share and then inject that value before running alembic upgrade head.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


# template used to generate migration files
# file_template = %%(rev)s_%%(slug)s

# timezone to use when rendering the date
# within the migration file as well as the filename.
# string value is passed to dateutil.tz.gettz()
# leave blank for localtime
# timezone =

# max length of characters to apply to the
# "slug" field
#truncate_slug_length = 40

# set to 'true' to run the environment during
# the 'revision' command, regardless of autogenerate
# revision_environment = false

# set to 'true' to allow .pyc and .pyo files without
# a source .py file to be detected as revisions in the
# versions/ directory
# sourceless = false

# version location specification; this defaults
# to alembic/versions. When using multiple version
# directories, initial revisions must be specified with --version-path
# version_locations = %(here)s/bar %(here)s/bat alembic/versions

# the output encoding used when revision files
# are written from script.py.mako
# output_encoding = utf-8

sqlalchemy.url = sqlite:////home/user/.securedrop_client/svs.sqlite


# Logging configuration
[loggers]
keys = root,sqlalchemy,alembic

[handlers]
keys = console

[formatters]
keys = generic

[logger_root]
level = WARN
handlers = console
qualname =

[logger_sqlalchemy]
level = WARN
handlers =
qualname = sqlalchemy.engine

[logger_alembic]
level = INFO
handlers =
qualname = alembic

[handler_console]
class = StreamHandler
args = (sys.stderr,)
level = NOTSET
formatter = generic

[formatter_generic]
format = %(levelname)-5.5s [%(name)s] %(message)s
datefmt = %H:%M:%S
3 changes: 3 additions & 0 deletions files/client.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[client]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use .json as the config type because it allows for deeper nesting, lists, and also ops tools have nice functions for mangling/merging JSON?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we stick with ini for simplicity for now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

homedir=/home/user/.securedrop_client
use_securedrop_proxy=True
11 changes: 11 additions & 0 deletions files/securedrop-client
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/sh

mkdir -p ~/.securedrop_client
chmod 0700 ~/.securedrop_client
cd /opt/venvs/securedrop-client

# Now let us try to run alembic first
./bin/alembic -c /usr/share/securedrop-client/alembic.ini upgrade head

# Now execute the actual client
./bin/sd-client
18 changes: 9 additions & 9 deletions requirements-build.txt
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
requests==2.20.0 --hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c --hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279
pathlib2==2.3.2 --hash=sha256:8eb170f8d0d61825e09a95b38be068299ddeda82f35e96c3301a8a5e7604cb83 --hash=sha256:d1aa2a11ba7b8f7b21ab852b1fb5afb277e1bb99d5dfc663380b5015c0d80c5a
python-editor==1.0.3 --hash=sha256:a3c066acee22a1c94f63938341d4fb374e3fdd69366ed6603d7b24bed1efc565
certifi==2018.10.15 --hash=sha256:339dc09518b07e2fa7eda5450740925974815557727d6bd35d319c1524a04a4c --hash=sha256:6d58c986d22b038c8c0df30d639f23a3e6d172a05c3583e766f4c0b785c0986a
idna==2.7 --hash=sha256:156a6814fb5ac1fc6850fb002e0852d56c0c8d2531923a51032d1b70760e186e --hash=sha256:684a38a6f903c1d71d6d5fac066b58d7768af4de2b832e426ec79c30daa94a16
markupsafe==1.0 --hash=sha256:a6be69091dac236ea9c6bc7d012beab42010fa914c459791d627dad4910eb665
python-dateutil==2.7.3 --hash=sha256:1adb80e7a782c12e52ef9a8182bebeb73f1d7e24e374397af06fb4956c8dc5c0 --hash=sha256:e27001de32f627c22380a688bcc43ce83504a7bc5da472209b4c70f02829f0b8
pathlib2==2.3.2 --hash=sha256:8eb170f8d0d61825e09a95b38be068299ddeda82f35e96c3301a8a5e7604cb83 --hash=sha256:d1aa2a11ba7b8f7b21ab852b1fb5afb277e1bb99d5dfc663380b5015c0d80c5a
mako==1.0.7 --hash=sha256:4e02fde57bd4abb5ec400181e4c314f56ac3e49ba4fb8b0d50bba18cb27d25ae
securedrop-sdk==0.0.3 --hash=sha256:5e3ebfde6ef63fc9a614da5b3b9820a93b827f2f7ecb4a72178ebe6d8e2f6d2a
urllib3==1.24 --hash=sha256:41c3db2fc01e5b907288010dec72f9d0a74e37d6994e6eb56849f59fea2265ae --hash=sha256:8819bba37a02d143296a4d032373c4dd4aca11f6d4c9973335ca75f9c8475f59
requests==2.20.0 --hash=sha256:99dcfdaaeb17caf6e526f32b6a7b780461512ab3f1d992187801694cba42770c --hash=sha256:a84b8c9ab6239b578f22d1c21d51b696dcfe004032bb80ea832398d6909d7279
markupsafe==1.0 --hash=sha256:a6be69091dac236ea9c6bc7d012beab42010fa914c459791d627dad4910eb665
python-dateutil==2.7.5 --hash=sha256:063df5763652e21de43de7d9e00ccf239f953a832941e37be541614732cdfc93 --hash=sha256:88f9287c0174266bb0d8cedd395cfba9c58e87e5ad86b2ce58859bc11be3cf02 --hash=sha256:78f89228bd45978e424099c06c597055ee6651d900ecd2df8e4c1d69b03ca5b6
arrow==0.12.1 --hash=sha256:a558d3b7b6ce7ffc74206a86c147052de23d3d4ef0e17c210dd478c53575c4cd
six==1.11.0 --hash=sha256:70e8a77beed4562e7f14fe23a786b54f6296e34344c23bc42f07b15018ff98e9 --hash=sha256:832dc0e10feb1aa2c68dcc57dbb658f1c7e65b9b61af69048abc87a2db00a0eb
sqlalchemy==1.2.12 --hash=sha256:c5951d9ef1d5404ed04bae5a16b60a0779087378928f997a294d1229c6ca4d3e
python-editor==1.0.3 --hash=sha256:a3c066acee22a1c94f63938341d4fb374e3fdd69366ed6603d7b24bed1efc565
chardet==3.0.4 --hash=sha256:84ab92ed1c4d4f16916e05906b6b75a6c0fb5db821cc65e70cbd64a3e2a5eaae --hash=sha256:fc323ffcaeaed0e0a02bf4d117757b98aed530d9ed4531e3e15460124c106691
alembic==1.0.1 --hash=sha256:0fe570f23dc48fb1bbda6f6a396f1c0c28d7045c0ad14018c104a511e6c1fe8a
sqlalchemy==1.2.12 --hash=sha256:c5951d9ef1d5404ed04bae5a16b60a0779087378928f997a294d1229c6ca4d3e
securedrop-sdk==0.0.1 --hash=sha256:82373118c49a141881332575c9ac1618973390a7d5c32ed29b3d64cb1f0d91e8
certifi==2018.10.15 --hash=sha256:339dc09518b07e2fa7eda5450740925974815557727d6bd35d319c1524a04a4c --hash=sha256:6d58c986d22b038c8c0df30d639f23a3e6d172a05c3583e766f4c0b785c0986a
six==1.11.0 --hash=sha256:70e8a77beed4562e7f14fe23a786b54f6296e34344c23bc42f07b15018ff98e9 --hash=sha256:832dc0e10feb1aa2c68dcc57dbb658f1c7e65b9b61af69048abc87a2db00a0eb
12 changes: 6 additions & 6 deletions run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ done

SDC_HOME=${SDC_HOME:-$(mktemp -d)}

export SDC_HOME

chmod 0700 $SDC_HOME

echo "Running app with home directory: $SDC_HOME"

# create the database for local testing
./createdb.py $SDC_HOME

python - << EOF
from securedrop_client.models import Base, make_engine
Base.metadata.create_all(make_engine("$SDC_HOME"))
EOF

exec python -m securedrop_client --sdc-home "$SDC_HOME" $@
exec python -m securedrop_client --sdc-home "$SDC_HOME" --no-proxy $@
15 changes: 14 additions & 1 deletion securedrop_client/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import signal
import sys
import socket
import configparser
from argparse import ArgumentParser
from sqlalchemy.orm import sessionmaker
from PyQt5.QtWidgets import QApplication, QMessageBox
Expand Down Expand Up @@ -105,6 +106,9 @@ def arg_parser() -> ArgumentParser:
type=expand_to_absolute,
help=('SecureDrop Client home directory for storing files and state. '
'(Default {})'.format(DEFAULT_SDC_HOME)))
parser.add_argument(
'--no-proxy', action='store_true',
help='Use proxy AppVM name to connect to server.')
return parser


Expand Down Expand Up @@ -159,7 +163,8 @@ def start_app(args, qt_args) -> None:
Session = sessionmaker(bind=engine)
session = Session()

client = Client("http://localhost:8081/", gui, session, args.sdc_home)
client = Client("http://localhost:8081/", gui, session,
args.sdc_home, not args.no_proxy)
client.setup()

configure_signal_handlers(app)
Expand All @@ -171,7 +176,15 @@ def start_app(args, qt_args) -> None:


def run() -> None:
config_file = "/usr/share/securedrop-client/client.ini"
args, qt_args = arg_parser().parse_known_args()
if args.sdc_home == expand_to_absolute(DEFAULT_SDC_HOME) and \
os.path.exists(config_file): # pragma: no cover
config = configparser.ConfigParser()
config.read(config_file)
args.sdc_home = config["client"]["homedir"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you overriding the sdc_home after reading a config from sdc_home?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reading the config from /etc/securdrop-client/client.ini file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I still think this make no sense. The client is passed a sdc_home, then we read another value, then we sed that other value to sdc_home. I feel like this isn't a good pattern and is kind confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done to make sure that we use the whatever values are there in the config file. This part is to be used only after installing via debian package, and we can improve this in future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a hardcoded home directory in Qubes, so we're using that hard coded value to read a hardcoded config to then switch the home to something else hardcoded. This seems like a lot of indirection.

use_proxy = config["client"].getboolean("use_securedrop_proxy")
args.no_proxy = not use_proxy
# reinsert the program's name
qt_args.insert(0, 'securedrop-client')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, I think that the CLI parsing should all happen outside the app, and then we pass those as args to start_app (or run_app, whatever the name was). In side there, we handle the logic of how to include values from the configs. I think reusing the Namespace from argparse as the "master config" seems wrong. We might want to have a Config class that does all this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to leave this for a future PR.

start_app(args, qt_args)
9 changes: 5 additions & 4 deletions securedrop_client/logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ class Client(QObject):

timeout_api_call = pyqtSignal() # Indicates there was a timeout.

def __init__(self, hostname, gui, session, home: str) -> None:
def __init__(self, hostname, gui, session,
home: str, proxy: bool = True) -> None:
"""
The hostname, gui and session objects are used to coordinate with the
various other layers of the application: the location of the SecureDrop
Expand All @@ -106,6 +107,7 @@ def __init__(self, hostname, gui, session, home: str) -> None:
self.home = home # The "home" directory for client files.
self.data_dir = os.path.join(self.home, 'data') # File data.
self.timer = None # call timeout timer
self.proxy = proxy

def setup(self):
"""
Expand Down Expand Up @@ -187,9 +189,8 @@ def login(self, username, password, totp):
Given a username, password and time based one-time-passcode (TOTP),
create a new instance representing the SecureDrop api and authenticate.
"""

self.api = sdclientapi.API(self.hostname, username, password, totp)

self.api = sdclientapi.API(self.hostname, username,
password, totp, self.proxy)
self.call_api(self.api.authenticate, self.on_authenticate,
self.on_login_timeout)

Expand Down
44 changes: 44 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import os
import setuptools

with open("README.md", "r") as fh:
long_description = fh.read()

# The CSS file
package_resources = ["securedrop_client/resources/css/sdclient.css"]

# All other graphics used in the client
for name in os.listdir('./securedrop_client/resources/images/'):
package_resources.append(os.path.join(
"./securedrop_client/resources/images", name))

setuptools.setup(
name="securedrop-client",
version="0.0.1",
author="Freedom of the Press Foundation",
author_email="[email protected]",
description="SecureDrop Workstation client application",
long_description=long_description,
long_description_content_type="text/markdown",
license="GPLv3+",
install_requires=["SQLALchemy", "alembic", "securedrop-sdk",
"pathlib2", "arrow", "certifi"],
python_requires=">=3.5",
url="https://github.com/freedomofpress/securedrop-proxy",
packages=["securedrop_client", "securedrop_client.gui",
"securedrop_client.resources"],
include_package_data=True,
classifiers=(
"Development Status :: 3 - Alpha",
"Programming Language :: Python :: 3",
"Topic :: Software Development :: Libraries :: Python Modules",
"License :: OSI Approved :: GNU General Public License v3 or later (GPLv3+)",
"Intended Audience :: Developers",
"Operating System :: OS Independent",
),
entry_points={
'console_scripts': [
'sd-client = securedrop_client.app:run',
],
},
)
3 changes: 2 additions & 1 deletion tests/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def test_start_app(safe_tmpdir):
mock_qt_args = mock.MagicMock()
sdc_home = str(safe_tmpdir)
mock_args.sdc_home = sdc_home
mock_args.proxy = False

with mock.patch('securedrop_client.app.configure_logging') as conf_log, \
mock.patch('securedrop_client.app.QApplication') as mock_app, \
Expand All @@ -117,7 +118,7 @@ def test_start_app(safe_tmpdir):
mock_win.assert_called_once_with()
mock_client.assert_called_once_with('http://localhost:8081/',
mock_win(), mock_session_class(),
sdc_home)
sdc_home, False)


PERMISSIONS_CASES = [
Expand Down