From 20006af94b1f2315ab7f0531466b981cebca81c9 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 14:32:02 +0200 Subject: [PATCH 01/13] increase verbosity of test output --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index af218afb2..546f4fcb3 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ clean: find . | grep -E "(__pycache__)" | xargs rm -rf test: clean - xvfb-run python -m pytest + xvfb-run python -m pytest -v pyflakes: find . \( -name _build -o -name var -o -path ./docs -o -path \) -type d -prune -o -name '*.py' -print0 | $(XARGS) pyflakes @@ -33,6 +33,6 @@ 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 coverage: clean - xvfb-run python -m pytest --cov-config .coveragerc --cov-report term-missing --cov=securedrop_client tests/ + xvfb-run python -m pytest -v --cov-config .coveragerc --cov-report term-missing --cov=securedrop_client tests/ check: clean pycodestyle pyflakes coverage From f79664a99f984714e99ea30992b1bdf465d205a8 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 14:33:25 +0200 Subject: [PATCH 02/13] cli arg parsing --- run.py | 6 ------ run.sh | 3 +++ securedrop_client/__main__.py | 3 +++ securedrop_client/app.py | 22 ++++++++++++++++++++-- 4 files changed, 26 insertions(+), 8 deletions(-) delete mode 100755 run.py create mode 100755 run.sh create mode 100644 securedrop_client/__main__.py diff --git a/run.py b/run.py deleted file mode 100755 index ea3c6332a..000000000 --- a/run.py +++ /dev/null @@ -1,6 +0,0 @@ -#!/usr/bin/env python3 -from securedrop_client.app import run - - -if __name__ == '__main__': - run() diff --git a/run.sh b/run.sh new file mode 100755 index 000000000..f3eec1ec3 --- /dev/null +++ b/run.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash + +python -m securedrop_client diff --git a/securedrop_client/__main__.py b/securedrop_client/__main__.py new file mode 100644 index 000000000..fa396ea72 --- /dev/null +++ b/securedrop_client/__main__.py @@ -0,0 +1,3 @@ +from .app import run + +run() diff --git a/securedrop_client/app.py b/securedrop_client/app.py index 36cb3400c..d7de4f45b 100644 --- a/securedrop_client/app.py +++ b/securedrop_client/app.py @@ -21,6 +21,7 @@ import os import signal import sys +from argparse import ArgumentParser from sqlalchemy.orm import sessionmaker from PyQt5.QtWidgets import QApplication from PyQt5.QtCore import Qt, QTimer @@ -32,6 +33,7 @@ from securedrop_client.models import engine +DEFAULT_HOME = os.path.expanduser('~/.securedrop_client') LOG_DIR = os.path.join(str(pathlib.Path.home()), '.securedrop_client') LOG_FILE = os.path.join(LOG_DIR, 'securedrop_client.log') ENCODING = 'utf-8' @@ -71,7 +73,16 @@ def configure_logging(): sys.excepthook = excepthook -def run(): +def arg_parser() -> ArgumentParser: + parser = ArgumentParser('securedrop-client', + description='SecureDrop Journalist GUI') + parser.add_argument( + '-H', '--home', default=DEFAULT_HOME, + help='Home directory for storing files and state') + return parser + + +def start_app(args, qt_args) -> None: """ Create all the top-level assets for the application, set things up and run the application. Specific tasks include: @@ -87,7 +98,7 @@ def run(): configure_logging() logging.info('Starting SecureDrop Client {}'.format(__version__)) - app = QApplication(sys.argv) + app = QApplication(qt_args) app.setApplicationName('SecureDrop Client') app.setDesktopFileName('org.freedomofthepress.securedrop.client') app.setApplicationVersion(__version__) @@ -113,3 +124,10 @@ def signal_handler(*nargs) -> None: timer.timeout.connect(lambda: None) sys.exit(app.exec_()) + + +def run() -> None: + args, qt_args = arg_parser().parse_known_args() + # reinsert the program's name + qt_args.insert(0, 'securedrop-client') + start_app(args, qt_args) From cfea71e53628f160c4c4c0732d23dd22e1a108db Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 14:33:35 +0200 Subject: [PATCH 03/13] update tests to accomodate cli arg parsing --- tests/test_app.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_app.py b/tests/test_app.py index b00f1e2ee..e9e24a261 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -5,7 +5,7 @@ from PyQt5.QtWidgets import QApplication from unittest import mock from securedrop_client.app import (LOG_DIR, LOG_FILE, ENCODING, excepthook, - configure_logging, run) + configure_logging, start_app) app = QApplication([]) @@ -51,6 +51,8 @@ def test_run(): Ensure the expected things are configured and the application is started. """ mock_session_class = mock.MagicMock() + mock_args = mock.MagicMock() + mock_qt_args = mock.MagicMock() with mock.patch('securedrop_client.app.configure_logging') as conf_log, \ mock.patch('securedrop_client.app.QApplication') as mock_app, \ mock.patch('securedrop_client.app.Window') as mock_win, \ @@ -58,9 +60,9 @@ def test_run(): mock.patch('securedrop_client.app.sys') as mock_sys, \ mock.patch('securedrop_client.app.sessionmaker', return_value=mock_session_class): - run() + start_app(mock_args, mock_qt_args) conf_log.assert_called_once_with() - mock_app.assert_called_once_with(mock_sys.argv) + mock_app.assert_called_once_with(mock_qt_args) mock_win.assert_called_once_with() mock_client.assert_called_once_with('http://localhost:8081/', mock_win(), mock_session_class()) From 4a5d29e206cc1dceda0f04237a6ed5c629bcb5e7 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 14:57:57 +0200 Subject: [PATCH 04/13] remove hard coded home dir --- securedrop_client/app.py | 17 ++++++++++------- tests/test_app.py | 21 ++++++++++----------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/securedrop_client/app.py b/securedrop_client/app.py index d7de4f45b..b01a161a7 100644 --- a/securedrop_client/app.py +++ b/securedrop_client/app.py @@ -34,11 +34,13 @@ DEFAULT_HOME = os.path.expanduser('~/.securedrop_client') -LOG_DIR = os.path.join(str(pathlib.Path.home()), '.securedrop_client') -LOG_FILE = os.path.join(LOG_DIR, 'securedrop_client.log') ENCODING = 'utf-8' +def init(home: str) -> None: + os.makedirs(home, exist_ok=True) + + def excepthook(*exc_args): """ This function is called in the event of a catastrophic failure. @@ -49,18 +51,18 @@ def excepthook(*exc_args): sys.exit(1) -def configure_logging(): +def configure_logging(home: str) -> None: """ All logging related settings are set up by this function. """ - if not os.path.exists(LOG_DIR): - os.makedirs(LOG_DIR) + log_file = os.path.join(home, 'client.log') + # set logging format log_fmt = ('%(asctime)s - %(name)s:%(lineno)d(%(funcName)s) ' '%(levelname)s: %(message)s') formatter = logging.Formatter(log_fmt) # define log handlers such as for rotating log files - handler = TimedRotatingFileHandler(LOG_FILE, when='midnight', + handler = TimedRotatingFileHandler(log_file, when='midnight', backupCount=5, delay=0, encoding=ENCODING) handler.setFormatter(formatter) @@ -95,7 +97,8 @@ def start_app(args, qt_args) -> None: - configure the client (logic) object. - ensure the application is setup in the default safe starting state. """ - configure_logging() + init(args.home) + configure_logging(args.home) logging.info('Starting SecureDrop Client {}'.format(__version__)) app = QApplication(qt_args) diff --git a/tests/test_app.py b/tests/test_app.py index e9e24a261..edcc56a78 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -4,8 +4,8 @@ import sys from PyQt5.QtWidgets import QApplication from unittest import mock -from securedrop_client.app import (LOG_DIR, LOG_FILE, ENCODING, excepthook, - configure_logging, start_app) +from securedrop_client.app import ENCODING, excepthook, configure_logging, \ + start_app app = QApplication([]) @@ -25,7 +25,7 @@ def test_excpethook(): exit.assert_called_once_with(1) -def test_configure_logging(): +def test_configure_logging(tmpdir): """ Ensure logging directory is created and logging is configured in the expected (rotating logs) manner. @@ -33,13 +33,12 @@ def test_configure_logging(): with mock.patch('securedrop_client.app.TimedRotatingFileHandler') as \ log_conf, \ mock.patch('securedrop_client.app.os.path.exists', - return_value=False),\ - mock.patch('securedrop_client.app.logging') as logging, \ - mock.patch('securedrop_client.app.os.makedirs', - return_value=None) as mkdir: - configure_logging() - mkdir.assert_called_once_with(LOG_DIR) - log_conf.assert_called_once_with(LOG_FILE, when='midnight', + return_value=False), \ + mock.patch('securedrop_client.app.logging') as logging: + log_dir = str(tmpdir) + log_file = tmpdir.join('client.log') + configure_logging(log_dir) + log_conf.assert_called_once_with(log_file, when='midnight', backupCount=5, delay=0, encoding=ENCODING) logging.getLogger.assert_called_once_with() @@ -58,10 +57,10 @@ def test_run(): mock.patch('securedrop_client.app.Window') as mock_win, \ mock.patch('securedrop_client.app.Client') as mock_client, \ mock.patch('securedrop_client.app.sys') as mock_sys, \ + mock.patch('securedrop_client.app.init') as mock_init, \ mock.patch('securedrop_client.app.sessionmaker', return_value=mock_session_class): start_app(mock_args, mock_qt_args) - conf_log.assert_called_once_with() mock_app.assert_called_once_with(mock_qt_args) mock_win.assert_called_once_with() mock_client.assert_called_once_with('http://localhost:8081/', From 3fe048f78dcfc825447ffac1aa212612d684aaf4 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 16:11:09 +0200 Subject: [PATCH 05/13] inject homedir into client --- securedrop_client/app.py | 47 ++++++++++++++++++++++++++------------ securedrop_client/logic.py | 8 +++++-- securedrop_client/utils.py | 47 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 17 deletions(-) create mode 100644 securedrop_client/utils.py diff --git a/securedrop_client/app.py b/securedrop_client/app.py index b01a161a7..9cc56bf71 100644 --- a/securedrop_client/app.py +++ b/securedrop_client/app.py @@ -31,14 +31,15 @@ from securedrop_client.gui.main import Window from securedrop_client.resources import load_icon, load_css from securedrop_client.models import engine +from securedrop_client.utils import safe_mkdir -DEFAULT_HOME = os.path.expanduser('~/.securedrop_client') +DEFAULT_SDC_HOME = '~/.securedrop_client' ENCODING = 'utf-8' -def init(home: str) -> None: - os.makedirs(home, exist_ok=True) +def init(sdc_home: str) -> None: + safe_mkdir(sdc_home) def excepthook(*exc_args): @@ -51,11 +52,12 @@ def excepthook(*exc_args): sys.exit(1) -def configure_logging(home: str) -> None: +def configure_logging(sdc_home: str) -> None: """ All logging related settings are set up by this function. """ - log_file = os.path.join(home, 'client.log') + safe_mkdir(sdc_home, 'logs') + log_file = os.path.join(sdc_home, 'logs', 'client.log') # set logging format log_fmt = ('%(asctime)s - %(name)s:%(lineno)d(%(funcName)s) ' @@ -75,12 +77,31 @@ def configure_logging(home: str) -> None: sys.excepthook = excepthook +def configure_signal_handlers(app) -> None: + def signal_handler(*nargs) -> None: + app.quit() + + for sig in [signal.SIGINT, signal.SIGTERM]: + signal.signal(sig, signal_handler) + + +def expand_to_absolute(value: str) -> str: + ''' + Helper that expands a path to the absolute path so users can provide + arguments in the form ``~/my/dir/``. + ''' + return os.path.abspath(os.path.expanduser(value)) + + def arg_parser() -> ArgumentParser: parser = ArgumentParser('securedrop-client', description='SecureDrop Journalist GUI') parser.add_argument( - '-H', '--home', default=DEFAULT_HOME, - help='Home directory for storing files and state') + '-H', '--sdc-home', + default=DEFAULT_SDC_HOME, + type=expand_to_absolute, + help=('SecureDrop Client home directory for storing files and state. ' + '(Default {})'.format(DEFAULT_SDC_HOME))) return parser @@ -97,8 +118,8 @@ def start_app(args, qt_args) -> None: - configure the client (logic) object. - ensure the application is setup in the default safe starting state. """ - init(args.home) - configure_logging(args.home) + init(args.sdc_home) + configure_logging(args.sdc_home) logging.info('Starting SecureDrop Client {}'.format(__version__)) app = QApplication(qt_args) @@ -114,14 +135,10 @@ def start_app(args, qt_args) -> None: Session = sessionmaker(bind=engine) session = Session() - client = Client("http://localhost:8081/", gui, session) + client = Client("http://localhost:8081/", gui, session, args.sdc_home) client.setup() - def signal_handler(*nargs) -> None: - app.quit() - - for sig in [signal.SIGINT, signal.SIGTERM]: - signal.signal(sig, signal_handler) + configure_signal_handlers(app) timer = QTimer() timer.start(500) timer.timeout.connect(lambda: None) diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 6fd6ecbe2..5525264e8 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -21,6 +21,7 @@ import sdclientapi import arrow from securedrop_client import storage +from securedrop_client.utils import check_dir_permissions from PyQt5.QtCore import QObject, QThread, pyqtSignal, QTimer @@ -83,19 +84,22 @@ class Client(QObject): finish_api_call = pyqtSignal() # Acknowledges reciept of an API call. - def __init__(self, hostname, gui, session): + def __init__(self, hostname, gui, session, home: str) -> None: """ The hostname, gui and session objects are used to coordinate with the various other layers of the application: the location of the SecureDrop proxy, the user interface and SqlAlchemy local storage respectively. """ + + check_dir_permissions(home) + super().__init__() self.hostname = hostname # Location of the SecureDrop server. self.gui = gui # Reference to the UI window. self.api = None # Reference to the API for secure drop proxy. self.session = session # Reference to the SqlAlchemy session. self.api_thread = None # Currently active API call thread. - self.sync_flag = os.path.join(os.path.expanduser('~'), '.sdsync') + self.sync_flag = os.path.join(home, 'sync_flag') def setup(self): """ diff --git a/securedrop_client/utils.py b/securedrop_client/utils.py new file mode 100644 index 000000000..f946c814d --- /dev/null +++ b/securedrop_client/utils.py @@ -0,0 +1,47 @@ +import os + + +def safe_mkdir(sdc_home: str, relative_path: str=None) -> None: + ''' + Safely create directories while checking permissions along the way. + ''' + check_dir_permissions(sdc_home) + + if not relative_path: + return + + full_path = os.path.join(sdc_home, relative_path) + if not full_path == os.path.abspath(full_path): + raise ValueError('Path is not absolute: {}'.format(full_path)) + + path_components = split_path(relative_path) + + path_so_far = sdc_home + for component in path_components: + path_so_far = os.path.join(path_so_far, component) + check_dir_permissions(path_so_far) + os.makedirs(path_so_far, 0o0700, exist_ok=True) + + +def check_dir_permissions(dir_path: str) -> None: + ''' + Check that a directory has ``700`` as the final 3 bytes. Raises a + ``RuntimeError`` otherwise. + ''' + if os.path.exists(dir_path): + stat_res = os.stat(dir_path).st_mode + masked = stat_res & 0o777 + if masked & 0o077: + raise RuntimeError('Unsafe permissions ({}) on {}' + .format(oct(stat_res), dir_path)) + + +def split_path(path: str) -> list: + out = [] + + while path: + path, tail = os.path.split(path) + out.append(tail) + + out.reverse() + return out From 07e611e86e59a28a32ecd397d2f896d6caa8f13c Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 16:11:36 +0200 Subject: [PATCH 06/13] tests for homedir injection --- tests/conftest.py | 8 +++++ tests/test_app.py | 16 +++++---- tests/test_logic.py | 88 ++++++++++++++++++++++----------------------- 3 files changed, 61 insertions(+), 51 deletions(-) create mode 100644 tests/conftest.py diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 000000000..f3f04ffc8 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,8 @@ +import os +import pytest + + +@pytest.fixture(scope='function') +def safe_tmpdir(tmpdir): + os.chmod(str(tmpdir), 0o0700) + return tmpdir diff --git a/tests/test_app.py b/tests/test_app.py index edcc56a78..7527cd3e2 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -25,7 +25,7 @@ def test_excpethook(): exit.assert_called_once_with(1) -def test_configure_logging(tmpdir): +def test_configure_logging(safe_tmpdir): """ Ensure logging directory is created and logging is configured in the expected (rotating logs) manner. @@ -35,9 +35,8 @@ def test_configure_logging(tmpdir): mock.patch('securedrop_client.app.os.path.exists', return_value=False), \ mock.patch('securedrop_client.app.logging') as logging: - log_dir = str(tmpdir) - log_file = tmpdir.join('client.log') - configure_logging(log_dir) + log_file = safe_tmpdir.mkdir('logs').join('client.log') + configure_logging(str(safe_tmpdir)) log_conf.assert_called_once_with(log_file, when='midnight', backupCount=5, delay=0, encoding=ENCODING) @@ -45,23 +44,26 @@ def test_configure_logging(tmpdir): assert sys.excepthook == excepthook -def test_run(): +def test_run(safe_tmpdir): """ Ensure the expected things are configured and the application is started. """ mock_session_class = mock.MagicMock() mock_args = mock.MagicMock() mock_qt_args = mock.MagicMock() + sdc_home = str(safe_tmpdir) + mock_args.sdc_home = sdc_home + with mock.patch('securedrop_client.app.configure_logging') as conf_log, \ mock.patch('securedrop_client.app.QApplication') as mock_app, \ mock.patch('securedrop_client.app.Window') as mock_win, \ mock.patch('securedrop_client.app.Client') as mock_client, \ mock.patch('securedrop_client.app.sys') as mock_sys, \ - mock.patch('securedrop_client.app.init') as mock_init, \ mock.patch('securedrop_client.app.sessionmaker', return_value=mock_session_class): start_app(mock_args, mock_qt_args) mock_app.assert_called_once_with(mock_qt_args) mock_win.assert_called_once_with() mock_client.assert_called_once_with('http://localhost:8081/', - mock_win(), mock_session_class()) + mock_win(), mock_session_class(), + sdc_home) diff --git a/tests/test_logic.py b/tests/test_logic.py index 7d9ed2c78..91578ccb7 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -60,27 +60,27 @@ def test_APICallRunner_on_cancel_timeout(): cr.timer.stop.assert_called_once_with() -def test_Client_init(): +def test_Client_init(safe_tmpdir): """ The passed in gui, app and session instances are correctly referenced and, where appropriate, have a reference back to the client. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost/', mock_gui, mock_session) + cl = Client('http://localhost/', mock_gui, mock_session, str(safe_tmpdir)) assert cl.hostname == 'http://localhost/' assert cl.gui == mock_gui assert cl.session == mock_session assert cl.api_thread is None -def test_Client_setup(): +def test_Client_setup(safe_tmpdir): """ Ensure the application is set up with the following default state: """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.update_sources = mock.MagicMock() cl.setup() cl.gui.setup.assert_called_once_with(cl) @@ -88,26 +88,26 @@ def test_Client_setup(): cl.gui.show_login.assert_called_once_with() -def test_Client_call_api_existing_thread(): +def test_Client_call_api_existing_thread(safe_tmpdir): """ The client will ignore attempt to call API if an existing request is in progress. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.api_thread = True cl.call_api(mock.MagicMock(), mock.MagicMock(), mock.MagicMock()) assert cl.api_thread is True -def test_Client_call_api(): +def test_Client_call_api(safe_tmpdir): """ A new thread and APICallRunner is created / setup. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.finish_api_call = mock.MagicMock() with mock.patch('securedrop_client.logic.QThread') as mock_qthread, \ mock.patch('securedrop_client.logic.APICallRunner') as mock_runner: @@ -128,28 +128,28 @@ def test_Client_call_api(): cl.finish_api_call.connect(cl.api_runner.on_cancel_timeout) -def test_Client_call_reset_no_thread(): +def test_Client_call_reset_no_thread(safe_tmpdir): """ The client will ignore an attempt to reset an API call is there's no such call "in flight". """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.finish_api_call = mock.MagicMock() cl.api_thread = None cl.call_reset() assert cl.finish_api_call.emit.call_count == 0 -def test_Client_call_reset(): +def test_Client_call_reset(safe_tmpdir): """ Call reset emits the expected signal and resets the state of client attributes. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.finish_api_call = mock.MagicMock() cl.api_thread = True cl.call_reset() @@ -158,14 +158,14 @@ def test_Client_call_reset(): assert cl.api_thread is None -def test_Client_login(): +def test_Client_login(safe_tmpdir): """ Ensures the API is called in the expected manner for logging in the user given the username, password and 2fa token. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.call_api = mock.MagicMock() with mock.patch('securedrop_client.logic.sdclientapi.API') as mock_api: cl.login('username', 'password', '123456') @@ -174,27 +174,27 @@ def test_Client_login(): cl.on_login_timeout) -def test_Client_on_authenticate_failed(): +def test_Client_on_authenticate_failed(safe_tmpdir): """ If the server responds with a negative to the request to authenticate, make sure the user knows. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.on_authenticate(False) mock_gui.show_login_error.\ assert_called_once_with(error='There was a problem logging in. Please ' 'try again.') -def test_Client_on_authenticate_ok(): +def test_Client_on_authenticate_ok(safe_tmpdir): """ Ensure the client syncs when the user successfully logs in. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.sync_api = mock.MagicMock() cl.api = mock.MagicMock() cl.api.username = 'test' @@ -203,13 +203,13 @@ def test_Client_on_authenticate_ok(): cl.gui.set_logged_in_as.assert_called_once_with('test') -def test_Client_on_login_timeout(): +def test_Client_on_login_timeout(safe_tmpdir): """ Reset the form if the API call times out. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.call_reset = mock.MagicMock() cl.on_login_timeout() cl.call_reset.assert_called_once_with() @@ -218,61 +218,61 @@ def test_Client_on_login_timeout(): 'out. Please try again.') -def test_Client_authenticated_yes(): +def test_Client_authenticated_yes(safe_tmpdir): """ If the API is authenticated return True. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.api = mock.MagicMock() cl.api.token = {'token': 'foo'} assert cl.authenticated() is True -def test_Client_authenticated_no(): +def test_Client_authenticated_no(safe_tmpdir): """ If the API is authenticated return True. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.api = mock.MagicMock() cl.api.token = {'token': ''} assert cl.authenticated() is False -def test_Client_authenticated_no_api(): +def test_Client_authenticated_no_api(safe_tmpdir): """ If the API is authenticated return True. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.api = None assert cl.authenticated() is False -def test_Client_sync_api_not_authenticated(): +def test_Client_sync_api_not_authenticated(safe_tmpdir): """ If the API isn't authenticated, don't sync. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.authenticated = mock.MagicMock(return_value=False) cl.call_api = mock.MagicMock() cl.sync_api() assert cl.call_api.call_count == 0 -def test_Client_sync_api(): +def test_Client_sync_api(safe_tmpdir): """ Sync the API is authenticated. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.authenticated = mock.MagicMock(return_value=True) cl.call_api = mock.MagicMock() cl.sync_api() @@ -280,7 +280,7 @@ def test_Client_sync_api(): cl.on_login_timeout, cl.api) -def test_Client_last_sync_with_file(): +def test_Client_last_sync_with_file(safe_tmpdir): """ The flag indicating the time of the last sync with the API is stored in a dotfile in the user's home directory. If such a file exists, ensure an @@ -288,7 +288,7 @@ def test_Client_last_sync_with_file(): """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) timestamp = '2018-10-10 18:17:13+01:00' with mock.patch("builtins.open", mock.mock_open(read_data=timestamp)): result = cl.last_sync() @@ -296,25 +296,25 @@ def test_Client_last_sync_with_file(): assert result.format() == timestamp -def test_Client_last_sync_no_file(): +def test_Client_last_sync_no_file(safe_tmpdir): """ If there's no sync file, then just return None. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) with mock.patch("builtins.open", mock.MagicMock(side_effect=Exception())): assert cl.last_sync() is None -def test_Client_on_synced_no_result(): +def test_Client_on_synced_no_result(safe_tmpdir): """ If there's no result to syncing, then don't attempt to update local storage and perhaps implement some as-yet-undefined UI update. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.update_sources = mock.MagicMock() with mock.patch('securedrop_client.logic.storage') as mock_storage: cl.on_synced(False) @@ -322,13 +322,13 @@ def test_Client_on_synced_no_result(): cl.update_sources.assert_called_once_with() -def test_Client_on_synced_with_result(): +def test_Client_on_synced_with_result(safe_tmpdir): """ If there's a result to syncing, then update local storage. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.update_sources = mock.MagicMock() cl.api_runner = mock.MagicMock() cl.api_runner.result = (1, 2, 3, ) @@ -341,27 +341,27 @@ def test_Client_on_synced_with_result(): cl.update_sources.assert_called_once_with() -def test_Client_update_sync(): +def test_Client_update_sync(safe_tmpdir): """ Cause the UI to update with the result of self.last_sync(). """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.last_sync = mock.MagicMock() cl.update_sync() assert cl.last_sync.call_count == 1 cl.gui.show_sync.assert_called_once_with(cl.last_sync()) -def test_Client_update_sources(): +def test_Client_update_sources(safe_tmpdir): """ Ensure the UI displays a list of the available sources from local data store. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) with mock.patch('securedrop_client.logic.storage') as mock_storage: mock_storage.get_local_sources.return_value = (1, 2, 3) cl.update_sources() @@ -369,13 +369,13 @@ def test_Client_update_sources(): mock_gui.show_sources.assert_called_once_with([1, 2, 3]) -def test_Client_logout(): +def test_Client_logout(safe_tmpdir): """ The API is reset to None and the UI is set to logged out state. """ mock_gui = mock.MagicMock() mock_session = mock.MagicMock() - cl = Client('http://localhost', mock_gui, mock_session) + cl = Client('http://localhost', mock_gui, mock_session, str(safe_tmpdir)) cl.api = mock.MagicMock() cl.logout() assert cl.api is None From 8d34cce98a509fee01e1a295f90bc293e5bb97de Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 16:16:42 +0200 Subject: [PATCH 07/13] more helpful run script --- run.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/run.sh b/run.sh index f3eec1ec3..986b2fc62 100755 --- a/run.sh +++ b/run.sh @@ -1,3 +1,8 @@ #!/usr/bin/env bash +set -e -python -m securedrop_client +HOME=$(mktemp -d) + +echo "Running app with home directory: $HOME" + +exec python -m securedrop_client --home "$HOME" From 0acb19822658bc3f2c9c345165d15a0365791533 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Wed, 17 Oct 2018 16:47:52 +0200 Subject: [PATCH 08/13] factor out hardcoded DB_PATH --- run.sh | 7 +++++++ securedrop_client/app.py | 3 ++- securedrop_client/models.py | 12 ++++-------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/run.sh b/run.sh index 986b2fc62..e17882e16 100755 --- a/run.sh +++ b/run.sh @@ -5,4 +5,11 @@ HOME=$(mktemp -d) echo "Running app with home directory: $HOME" +# create the database for local testing + +python - << EOF +from securedrop_client.models import Base, make_engine +Base.metadata.create_all(make_engine("$HOME")) +EOF + exec python -m securedrop_client --home "$HOME" diff --git a/securedrop_client/app.py b/securedrop_client/app.py index 9cc56bf71..b82fbfd84 100644 --- a/securedrop_client/app.py +++ b/securedrop_client/app.py @@ -30,7 +30,7 @@ from securedrop_client.logic import Client from securedrop_client.gui.main import Window from securedrop_client.resources import load_icon, load_css -from securedrop_client.models import engine +from securedrop_client.models import make_engine from securedrop_client.utils import safe_mkdir @@ -132,6 +132,7 @@ def start_app(args, qt_args) -> None: app.setWindowIcon(load_icon(gui.icon)) app.setStyleSheet(load_css('sdclient.css')) + engine = make_engine(args.sdc_home) Session = sessionmaker(bind=engine) session = Session() diff --git a/securedrop_client/models.py b/securedrop_client/models.py index bc25f4408..695d4be91 100644 --- a/securedrop_client/models.py +++ b/securedrop_client/models.py @@ -5,12 +5,12 @@ from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import relationship, backref +Base = declarative_base() -# TODO: Store this in config file, see issue #2 -DB_PATH = os.path.abspath('svs.sqlite') -engine = create_engine('sqlite:///{}'.format(DB_PATH)) -Base = declarative_base() +def make_engine(home: str): + db_path = os.path.join(home, 'svs.sqlite') + return create_engine('sqlite:///{}'.format(db_path)) class Source(Base): @@ -104,7 +104,3 @@ def __init__(self, username): def __repr__(self): return "".format(self.username) - - -# Populate the database. -Base.metadata.create_all(engine) From 3f9ce66f596d16a07d6203802f82682c34961090 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Fri, 19 Oct 2018 16:51:34 +0200 Subject: [PATCH 09/13] tests for directory permissions --- tests/test_app.py | 72 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_logic.py | 47 +++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/tests/test_app.py b/tests/test_app.py index 7527cd3e2..2315c3891 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -1,6 +1,8 @@ """ Tests for the app module, which sets things up and runs the application. """ +import os +import pytest import sys from PyQt5.QtWidgets import QApplication from unittest import mock @@ -67,3 +69,73 @@ def test_run(safe_tmpdir): mock_client.assert_called_once_with('http://localhost:8081/', mock_win(), mock_session_class(), sdc_home) + + +PERMISSIONS_CASES = [ + { + 'should_pass': True, + 'home_perms': None, + 'sub_dirs': [], + }, + { + 'should_pass': True, + 'home_perms': 0o0700, + 'sub_dirs': [], + }, + { + 'should_pass': False, + 'home_perms': 0o0740, + 'sub_dirs': [], + }, + { + 'should_pass': False, + 'home_perms': 0o0704, + 'sub_dirs': [], + }, + { + 'should_pass': True, + 'home_perms': 0o0700, + 'sub_dirs': [('logs', 0o0700)], + }, + { + 'should_pass': False, + 'home_perms': 0o0700, + 'sub_dirs': [('logs', 0o0740)], + }, +] + + +def test_create_app_dir_permissions(tmpdir): + mock_session_class = mock.MagicMock() + mock_args = mock.MagicMock() + mock_qt_args = mock.MagicMock() + + for idx, case in enumerate(PERMISSIONS_CASES): + sdc_home = os.path.join(str(tmpdir), 'case-{}'.format(idx)) + + # optionally create the dir + if case['home_perms'] is not None: + os.mkdir(sdc_home, case['home_perms']) + + mock_args.sdc_home = sdc_home + + for subdir, perms in case['sub_dirs']: + full_path = os.path.join(sdc_home, subdir) + os.makedirs(full_path, perms) + + with mock.patch('logging.getLogger'), \ + mock.patch('securedrop_client.app.QApplication') as mock_app, \ + mock.patch('securedrop_client.app.Window') as mock_win, \ + mock.patch('securedrop_client.app.Client') as mock_client, \ + mock.patch('securedrop_client.app.sys') as mock_sys, \ + mock.patch('securedrop_client.app.sessionmaker', + return_value=mock_session_class): + + def func(): + start_app(mock_args, mock_qt_args) + + if case['should_pass']: + func() + else: + with pytest.raises(RuntimeError): + func() diff --git a/tests/test_logic.py b/tests/test_logic.py index 91578ccb7..021a2e2ca 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -3,6 +3,8 @@ expected. """ import arrow +import os +import pytest from securedrop_client import storage from securedrop_client.logic import APICallRunner, Client from unittest import mock @@ -380,3 +382,48 @@ def test_Client_logout(safe_tmpdir): cl.logout() assert cl.api is None cl.gui.logout.assert_called_once_with() + + +PERMISSIONS_CASES = [ + { + 'should_pass': True, + 'home_perms': None, + }, + { + 'should_pass': True, + 'home_perms': 0o0700, + }, + { + 'should_pass': False, + 'home_perms': 0o0740, + }, + { + 'should_pass': False, + 'home_perms': 0o0704, + }, +] + + +def test_create_client_dir_permissions(tmpdir): + ''' + Check that creating an app behaves appropriately with different + permissions on the various directories needed for it to function. + ''' + mock_gui = mock.MagicMock() + mock_session = mock.MagicMock() + + for idx, case in enumerate(PERMISSIONS_CASES): + sdc_home = os.path.join(str(tmpdir), 'case-{}'.format(idx)) + + # optionally create the dir + if case['home_perms'] is not None: + os.mkdir(sdc_home, case['home_perms']) + + def func() -> None: + Client('http://localhost', mock_gui, mock_session, sdc_home) + + if case['should_pass']: + func() + else: + with pytest.raises(RuntimeError): + func() From 4edbe3324265526fb300f692f530bf6cc8c5479c Mon Sep 17 00:00:00 2001 From: heartsucker Date: Sat, 20 Oct 2018 10:14:30 +0200 Subject: [PATCH 10/13] allow passing homedir to run.sh --- run.sh | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/run.sh b/run.sh index e17882e16..1113c6995 100755 --- a/run.sh +++ b/run.sh @@ -1,15 +1,29 @@ #!/usr/bin/env bash set -e -HOME=$(mktemp -d) +while [ -n "$1" ]; do + param="$1" + value="$2" + case $param in + --sdc-home) + SDC_HOME="$value" + shift + ;; + *) + break + esac + shift +done -echo "Running app with home directory: $HOME" +SDC_HOME=${SDC_HOME:-$(mktemp -d)} + +echo "Running app with home directory: $SDC_HOME" # create the database for local testing python - << EOF from securedrop_client.models import Base, make_engine -Base.metadata.create_all(make_engine("$HOME")) +Base.metadata.create_all(make_engine("$SDC_HOME")) EOF -exec python -m securedrop_client --home "$HOME" +exec python -m securedrop_client --sdc-home "$SDC_HOME" $@ From bcc141bc7bff0160c89535c7b13c2252d2b151d9 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Sat, 20 Oct 2018 10:00:22 +0200 Subject: [PATCH 11/13] updated readme --- README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/README.md b/README.md index 78b627d82..c10f0f18b 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,20 @@ pipenv install --dev pipenv shell ``` +## Run the client + +You can run the client with an ephemeral data directory: + +``` +./run.sh +``` + +If you want to persist data across restarts, you will need to run the client with: + +``` +./run.sh --sdc-home /path/to/my/dir/ +``` + ## Run tests ``` From 6b096b13736eb9c3b107728bc7a12a754fdcc755 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Mon, 22 Oct 2018 19:59:50 +0200 Subject: [PATCH 12/13] update to test logic --- Makefile | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 546f4fcb3..0c36d1783 100644 --- a/Makefile +++ b/Makefile @@ -23,8 +23,10 @@ clean: find . \( -name '*.tgz' -o -name dropin.cache \) -delete find . | grep -E "(__pycache__)" | xargs rm -rf +TESTS ?= tests +TESTOPTS ?= -v test: clean - xvfb-run python -m pytest -v + xvfb-run python -m pytest -v --cov-config .coveragerc --cov-report html --cov-report term-missing --cov=securedrop_client --cov-fail-under 100 $(TESTOPTS) $(TESTS) pyflakes: find . \( -name _build -o -name var -o -path ./docs -o -path \) -type d -prune -o -name '*.py' -print0 | $(XARGS) pyflakes @@ -32,7 +34,4 @@ 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 -coverage: clean - xvfb-run python -m pytest -v --cov-config .coveragerc --cov-report term-missing --cov=securedrop_client tests/ - -check: clean pycodestyle pyflakes coverage +check: clean pycodestyle pyflakes test From a08d7bf185ba59c0f6a7c6da34044328ba2c90a2 Mon Sep 17 00:00:00 2001 From: heartsucker Date: Mon, 22 Oct 2018 23:07:55 +0200 Subject: [PATCH 13/13] 100% test coverage --- tests/test_app.py | 66 +++++++++++++++++++++++++++++++++++++++++++-- tests/test_utils.py | 11 ++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 tests/test_utils.py diff --git a/tests/test_app.py b/tests/test_app.py index 2315c3891..28bfb950f 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -7,7 +7,7 @@ from PyQt5.QtWidgets import QApplication from unittest import mock from securedrop_client.app import ENCODING, excepthook, configure_logging, \ - start_app + start_app, arg_parser, DEFAULT_SDC_HOME, run, configure_signal_handlers app = QApplication([]) @@ -46,7 +46,7 @@ def test_configure_logging(safe_tmpdir): assert sys.excepthook == excepthook -def test_run(safe_tmpdir): +def test_start_app(safe_tmpdir): """ Ensure the expected things are configured and the application is started. """ @@ -139,3 +139,65 @@ def func(): else: with pytest.raises(RuntimeError): func() + + +def test_argparse(): + parser = arg_parser() + + return_value = '/some/path' + with mock.patch('os.path.expanduser', return_value=return_value) \ + as mock_expand: + args = parser.parse_args([]) + + # check that the default home is used when no args args supplied + mock_expand.assert_called_once_with(DEFAULT_SDC_HOME) + # check that sdc_home is set after parsing args + assert args.sdc_home == return_value + + +def test_main(): + with mock.patch('securedrop_client.app.run') as mock_run: + import securedrop_client.__main__ # noqa + + assert mock_run.called + + +def test_run(): + mock_args = mock.MagicMock() + mock_qt_args = [] + + def fake_known_args(): + return (mock_args, mock_qt_args) + + with mock.patch('securedrop_client.app.start_app') as mock_start_app, \ + mock.patch('argparse.ArgumentParser.parse_known_args', + side_effect=fake_known_args) as wat: + run() + + mock_start_app.assert_called_once_with(mock_args, mock_qt_args) + + +def test_signal_interception(): + # check that initializing an app calls configure_signal_handlers + with mock.patch('securedrop_client.app.QApplication'), \ + mock.patch('sys.exit'), \ + mock.patch('securedrop_client.models.make_engine'), \ + mock.patch('securedrop_client.app.init'), \ + mock.patch('securedrop_client.logic.Client.setup'), \ + mock.patch('securedrop_client.app.configure_logging'), \ + mock.patch('securedrop_client.app.configure_signal_handlers') \ + as mock_signal_handlers: + start_app(mock.MagicMock(), []) + assert mock_signal_handlers.called + + # check that a signal interception calls quit on the app + mock_app = mock.MagicMock() + with mock.patch.object(mock_app, 'quit') as mock_quit, \ + mock.patch('signal.signal') as mock_signal: + configure_signal_handlers(mock_app) + assert mock_signal.called + + assert not mock_quit.called + signal_handler = mock_signal.call_args_list[0][0][1] + signal_handler() + assert mock_quit.called diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 000000000..2737cf9a9 --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,11 @@ +import pytest +from securedrop_client.utils import safe_mkdir + + +def test_safe_makedirs_non_absolute(safe_tmpdir): + home_dir = str(safe_tmpdir) + + with pytest.raises(ValueError) as e_info: + safe_mkdir(home_dir, '..') + + assert 'not absolute' in str(e_info.value)