From 196c969d1694d858210810f05b90faae723a905f Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Thu, 17 Oct 2019 10:51:44 -0400 Subject: [PATCH 01/12] app: persisting replies in the database if pending, failed --- .../86b01b6290da_add_reply_send_status.py | 130 ++++++++++++++++++ create_dev_data.py | 8 +- securedrop_client/api_jobs/downloads.py | 8 +- securedrop_client/api_jobs/uploads.py | 50 ++++++- securedrop_client/db.py | 22 +++ securedrop_client/gui/widgets.py | 81 +++++++++-- securedrop_client/storage.py | 9 +- tests/conftest.py | 12 +- 8 files changed, 302 insertions(+), 18 deletions(-) create mode 100644 alembic/versions/86b01b6290da_add_reply_send_status.py diff --git a/alembic/versions/86b01b6290da_add_reply_send_status.py b/alembic/versions/86b01b6290da_add_reply_send_status.py new file mode 100644 index 000000000..175c9b76e --- /dev/null +++ b/alembic/versions/86b01b6290da_add_reply_send_status.py @@ -0,0 +1,130 @@ +"""add reply send status + +Revision ID: 86b01b6290da +Revises: 36a79ffcfbfb +Create Date: 2019-10-17 09:45:07.643076 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '86b01b6290da' +down_revision = '36a79ffcfbfb' +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table('replysendstatuses', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('name', sa.String(length=36), nullable=False), + sa.PrimaryKeyConstraint('id', name=op.f('pk_replysendstatuses')), + sa.UniqueConstraint('name', name=op.f('uq_replysendstatuses_name')) + ) + + # Set the initial statuses: PENDING, FAILED, SUCCEEDED + conn = op.get_bind() + conn.execute(''' + INSERT INTO replysendstatuses + ('name') + VALUES + ('PENDING'), + ('SUCCEEDED'), + ('FAILED'); + ''') + + op.rename_table('replies', 'replies_tmp') + op.add_column('replies_tmp', sa.Column('send_status_id', sa.Integer(), nullable=True)) + op.create_table( + 'replies', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('uuid', sa.String(length=36), nullable=False), + sa.Column('source_id', sa.Integer(), nullable=False), + sa.Column('filename', sa.String(length=255), nullable=False), + sa.Column('file_counter', sa.Integer(), nullable=False), + sa.Column('size', sa.Integer(), nullable=True), + sa.Column('content', sa.Text(), nullable=True), + sa.Column('is_decrypted', sa.Boolean(name='is_decrypted'), nullable=True), + sa.Column('is_downloaded', sa.Boolean(name='is_downloaded'), nullable=True), + sa.Column('journalist_id', sa.Integer(), nullable=True), + sa.Column('send_status_id', sa.Integer(), nullable=True), + sa.PrimaryKeyConstraint('id', name=op.f('pk_replies')), + sa.UniqueConstraint('source_id', 'file_counter', name='uq_messages_source_id_file_counter'), + sa.UniqueConstraint('uuid', name=op.f('uq_replies_uuid')), + sa.ForeignKeyConstraint( + ['source_id'], + ['sources.id'], + name=op.f('fk_replies_source_id_sources')), + sa.ForeignKeyConstraint( + ['journalist_id'], + ['users.id'], + name=op.f('fk_replies_journalist_id_users')), + sa.CheckConstraint( + 'CASE WHEN is_downloaded = 0 THEN content IS NULL ELSE 1 END', + name='replies_compare_download_vs_content'), + sa.CheckConstraint( + 'CASE WHEN is_downloaded = 0 THEN is_decrypted IS NULL ELSE 1 END', + name='replies_compare_is_downloaded_vs_is_decrypted'), + sa.ForeignKeyConstraint( + ['send_status_id'], + ['replysendstatuses.id'], + op.f('fk_replies_send_status_replysendstatuses'), + ) + ) + + conn.execute(''' + INSERT INTO replies + SELECT id, uuid, source_id, filename, file_counter, size, content, is_decrypted, + is_downloaded, journalist_id, send_status_id + FROM replies_tmp + ''') + op.drop_table('replies_tmp') + + +def downgrade(): + op.rename_table('replies', 'replies_tmp') + + op.create_table( + 'replies', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('uuid', sa.String(length=36), nullable=False), + sa.Column('source_id', sa.Integer(), nullable=False), + sa.Column('filename', sa.String(length=255), nullable=False), + sa.Column('file_counter', sa.Integer(), nullable=False), + sa.Column('size', sa.Integer(), nullable=True), + sa.Column('content', sa.Text(), nullable=True), + sa.Column('is_decrypted', sa.Boolean(name='is_decrypted'), nullable=True), + sa.Column('is_downloaded', sa.Boolean(name='is_downloaded'), nullable=True), + sa.Column('journalist_id', sa.Integer(), nullable=True), + sa.PrimaryKeyConstraint('id', name=op.f('pk_replies')), + sa.UniqueConstraint('source_id', 'file_counter', name='uq_messages_source_id_file_counter'), + sa.UniqueConstraint('uuid', name=op.f('uq_replies_uuid')), + sa.ForeignKeyConstraint( + ['source_id'], + ['sources.id'], + name=op.f('fk_replies_source_id_sources')), + sa.ForeignKeyConstraint( + ['journalist_id'], + ['users.id'], + name=op.f('fk_replies_journalist_id_users')), + sa.CheckConstraint( + 'CASE WHEN is_downloaded = 0 THEN content IS NULL ELSE 1 END', + name='replies_compare_download_vs_content'), + sa.CheckConstraint( + 'CASE WHEN is_downloaded = 0 THEN is_decrypted IS NULL ELSE 1 END', + name='replies_compare_is_downloaded_vs_is_decrypted'), + sa.CheckConstraint( + 'CASE WHEN is_decrypted = 0 THEN content IS NULL ELSE 1 END', + name='replies_compare_is_decrypted_vs_content')) + + conn = op.get_bind() + conn.execute(''' + INSERT INTO replies + SELECT id, uuid, source_id, filename, file_counter, size, content, is_decrypted, + is_downloaded, journalist_id + FROM replies_tmp + ''') + op.drop_table('replies_tmp') + op.drop_table('replysendstatuses') diff --git a/create_dev_data.py b/create_dev_data.py index 087196cfe..1498fb46c 100755 --- a/create_dev_data.py +++ b/create_dev_data.py @@ -3,7 +3,8 @@ import os import sys from securedrop_client.config import Config -from securedrop_client.db import Base, make_session_maker +from securedrop_client.db import Base, make_session_maker, ReplySendStatus +from securedrop_client.api_jobs.uploads import ReplySendStatusCodes sdc_home = sys.argv[1] session = make_session_maker(sdc_home)() @@ -13,3 +14,8 @@ f.write(json.dumps({ 'journalist_key_fingerprint': '65A1B5FF195B56353CC63DFFCC40EF1228271441', })) + +for reply_send_status in ReplySendStatusCodes: + reply_status = ReplySendStatus(reply_send_status.value) + session.add(reply_status) + session.commit() diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py index 0824d1548..2aec44e94 100644 --- a/securedrop_client/api_jobs/downloads.py +++ b/securedrop_client/api_jobs/downloads.py @@ -14,8 +14,9 @@ from sqlalchemy.orm.session import Session from securedrop_client.api_jobs.base import ApiJob +from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from securedrop_client.crypto import GpgHelper, CryptoError -from securedrop_client.db import File, Message, Reply +from securedrop_client.db import File, Message, Reply, ReplySendStatus from securedrop_client.storage import mark_as_decrypted, mark_as_downloaded, \ set_message_or_reply_content @@ -279,11 +280,14 @@ def call_decrypt(self, filepath: str, session: Session = None) -> str: ''' with NamedTemporaryFile('w+') as plaintext_file: self.gpg.decrypt_submission_or_reply(filepath, plaintext_file.name, is_doc=False) + success_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.SUCCEEDED.value).one() set_message_or_reply_content( model_type=Message, uuid=self.uuid, session=session, - content=plaintext_file.read()) + content=plaintext_file.read(), + send_status_id=success_status.id) return "" diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py index 2fd03b4ef..341af484a 100644 --- a/securedrop_client/api_jobs/uploads.py +++ b/securedrop_client/api_jobs/uploads.py @@ -1,3 +1,4 @@ +from enum import Enum import logging import sdclientapi @@ -6,11 +7,17 @@ from securedrop_client.api_jobs.base import ApiJob from securedrop_client.crypto import GpgHelper -from securedrop_client.db import Reply, Source +from securedrop_client.db import Reply, ReplySendStatus, Source logger = logging.getLogger(__name__) +class ReplySendStatusCodes(Enum): + PENDING = 'PENDING' + SUCCEEDED = 'SUCCEEDED' + FAILED = 'FAILED' + + class SendReplyJob(ApiJob): def __init__(self, source_uuid: str, reply_uuid: str, message: str, gpg: GpgHelper) -> None: super().__init__() @@ -29,27 +36,62 @@ def call_api(self, api_client: API, session: Session) -> str: ''' try: encrypted_reply = self.gpg.encrypt_to_source(self.source_uuid, self.message) - sdk_reply = self._make_call(encrypted_reply, api_client) + + # Before we send the reply, add to the database with a PENDING reply send status source = session.query(Source).filter_by(uuid=self.source_uuid).one() + reply_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.PENDING.value).one() + + # Filename will be (interaction_count + 1) || journalist_designation + filename = '{}-{}-reply.gpg'.format(source.interaction_count + 1, + source.journalist_designation) reply_db_object = Reply( uuid=self.reply_uuid, source_id=source.id, + filename=filename, journalist_id=api_client.token_journalist_uuid, - filename=sdk_reply.filename, content=self.message, is_downloaded=True, - is_decrypted=True + is_decrypted=True, + send_status_id=reply_status.id, ) session.add(reply_db_object) session.commit() + sdk_reply = self._make_call(encrypted_reply, api_client) + + # Update reply send status to SUCCEEDED + reply_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.SUCCEEDED.value).one() + reply_db_object.send_status_id = reply_status.id + # Update filename in case it changed on the server + reply_db_object.filename = sdk_reply.filename + session.add(reply_db_object) + session.commit() + return reply_db_object.uuid except RequestTimeoutError as e: message = "Failed to send reply for source {id} due to Exception: {error}".format( id=self.source_uuid, error=e) + + # Update reply send status to FAILED + reply_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.FAILED.value).one() + reply_db_object.send_status_id = reply_status.id + session.add(reply_db_object) + session.commit() + raise SendReplyJobTimeoutError(message, self.reply_uuid) except Exception as e: message = "Failed to send reply for source {id} due to Exception: {error}".format( id=self.source_uuid, error=e) + + # Update reply send status to FAILED + reply_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.FAILED.value).one() + reply_db_object.send_status_id = reply_status.id + session.add(reply_db_object) + session.commit() + raise SendReplyJobError(message, self.reply_uuid) def _make_call(self, encrypted_reply: str, api_client: API) -> sdclientapi.Reply: diff --git a/securedrop_client/db.py b/securedrop_client/db.py index 69e3401db..2205319cd 100644 --- a/securedrop_client/db.py +++ b/securedrop_client/db.py @@ -203,6 +203,13 @@ class Reply(Base): nullable=True, ) + # This tracks the sending status of the reply. + send_status_id = Column( + Integer, + ForeignKey('replysendstatuses.id') + ) + send_status = relationship("ReplySendStatus") + def __init__(self, **kwargs: Any) -> None: if 'file_counter' in kwargs: raise TypeError('Cannot manually set file_counter') @@ -214,6 +221,21 @@ def __repr__(self) -> str: return ''.format(self.filename) +class ReplySendStatus(Base): + + __tablename__ = 'replysendstatuses' + + id = Column(Integer, primary_key=True) + name = Column(String(36), unique=True, nullable=False) + + def __init__(self, name: str) -> None: + super().__init__() + self.name = name + + def __repr__(self) -> str: + return ''.format(self.name) + + class User(Base): __tablename__ = 'users' diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index 074d52cb5..ab81a8e5b 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -737,6 +737,7 @@ def on_source_changed(self): source = self.source_list.get_current_source() if source: + self.controller.session.refresh(source) # Try to get the SourceConversationWrapper from the persistent dict, # else we create it. try: @@ -1567,18 +1568,70 @@ class ReplyWidget(SpeechBubble): Represents a reply to a source. """ - CSS_COLOR_BAR_REPLY_FAIL = ''' - background-color: #ff3366; + CSS_REPLY_FAILED = ''' + #message { + min-width: 540px; + max-width: 540px; + font-family: 'Source Sans Pro'; + font-weight: 400; + font-size: 15px; + background-color: #fff; + color: #3b3b3b; + padding: 16px; + } + #color_bar { + min-height: 5px; + max-height: 5px; + background-color: #ff3366; + border: 0px; + } ''' - CSS_COLOR_BAR_REPLY = ''' - background-color: #0065db; + CSS_REPLY_SUCCEEDED = ''' + #message { + min-width: 540px; + max-width: 540px; + font-family: 'Source Sans Pro'; + font-weight: 400; + font-size: 15px; + background-color: #fff; + color: #3b3b3b; + padding: 16px; + } + #color_bar { + min-height: 5px; + max-height: 5px; + background-color: #0065db; + border: 0px; + } + ''' + + # Custom pending CSS styling simulates the effect of opacity which is only + # supported by tooltips for QSS. + CSS_REPLY_PENDING = ''' + #message { + min-width: 540px; + max-width: 540px; + font-family: 'Source Sans Pro'; + font-weight: 400; + font-size: 15px; + color: #A9AAAD; + background-color: #F7F8FC; + padding: 16px; + } + #color_bar { + min-height: 5px; + max-height: 5px; + background-color: #8715FF; + border: 0px; + } ''' def __init__( self, message_id: str, message: str, + reply_status: str, update_signal, message_succeeded_signal, message_failed_signal, @@ -1587,11 +1640,19 @@ def __init__( self.message_id = message_id # Set styles - self.color_bar.setStyleSheet(self.CSS_COLOR_BAR_REPLY) + self._set_reply_state(reply_status) message_succeeded_signal.connect(self._on_reply_success) message_failed_signal.connect(self._on_reply_failure) + def _set_reply_state(self, status: str) -> None: + if status == 'SUCCEEDED': + self.setStyleSheet(self.CSS_REPLY_SUCCEEDED) + elif status == 'FAILED': + self.setStyleSheet(self.CSS_REPLY_FAILED) + elif status == 'PENDING': + self.setStyleSheet(self.CSS_REPLY_PENDING) + @pyqtSlot(str) def _on_reply_success(self, message_id: str) -> None: """ @@ -1600,6 +1661,7 @@ def _on_reply_success(self, message_id: str) -> None: """ if message_id == self.message_id: logger.debug('Message {} succeeded'.format(message_id)) + self._set_reply_state('SUCCEEDED') @pyqtSlot(str) def _on_reply_failure(self, message_id: str) -> None: @@ -1609,7 +1671,7 @@ def _on_reply_failure(self, message_id: str) -> None: """ if message_id == self.message_id: logger.debug('Message {} failed'.format(message_id)) - self.color_bar.setStyleSheet(self.CSS_COLOR_BAR_REPLY_FAIL) + self._set_reply_state('FAILED') class FileWidget(QWidget): @@ -2172,9 +2234,11 @@ def add_reply(self, reply: Reply) -> None: else: content = '' + logger.debug('adding reply: with status {}'.format(reply.send_status.name)) conversation_item = ReplyWidget( reply.uuid, content, + reply.send_status.name, self.controller.reply_ready, self.controller.reply_succeeded, self.controller.reply_failed) @@ -2187,6 +2251,7 @@ def add_reply_from_reply_box(self, uuid: str, content: str) -> None: conversation_item = ReplyWidget( uuid, content, + 'PENDING', self.controller.reply_ready, self.controller.reply_succeeded, self.controller.reply_failed) @@ -2352,8 +2417,8 @@ def disable(self): def send_reply(self) -> None: """ - Send reply and emit a signal so that the gui can be updated immediately, even before the - the reply is saved locally. + Send reply and emit a signal so that the gui can be updated immediately indicating + that it is a pending reply. """ reply_text = self.text_edit.toPlainText().strip() if reply_text: diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 0c0dbb4ba..cc81c7d56 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -29,7 +29,8 @@ from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.orm.session import Session -from securedrop_client.db import Source, Message, File, Reply, User +from securedrop_client.api_jobs.uploads import ReplySendStatusCodes +from securedrop_client.db import Source, Message, File, Reply, User, ReplySendStatus from sdclientapi import API from sdclientapi import Source as SDKSource from sdclientapi import Submission as SDKSubmission @@ -269,11 +270,15 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], reply.journalist_uuid, reply.journalist_username, session) + # All replies fetched from the server have succeeded in being sent. + success_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.SUCCEEDED.value).one() nr = Reply(uuid=reply.uuid, journalist_id=user.id, source_id=source.id, filename=reply.filename, - size=reply.size) + size=reply.size, + send_status_id=success_status.id) session.add(nr) logger.debug('Added new reply {}'.format(reply.uuid)) diff --git a/tests/conftest.py b/tests/conftest.py index 9b397ede5..2b8604830 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,8 +6,9 @@ from configparser import ConfigParser from datetime import datetime from securedrop_client.config import Config +from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from securedrop_client.app import configure_locale_and_language -from securedrop_client.db import Base, make_session_maker, Source +from securedrop_client.db import Base, make_session_maker, Source, ReplySendStatus from uuid import uuid4 @@ -93,6 +94,15 @@ def session(session_maker): sess.close() +@pytest.fixture(scope='function') +def reply_status_codes(session) -> None: + for reply_send_status in ReplySendStatusCodes: + reply_status = ReplySendStatus(reply_send_status.value) + session.add(reply_status) + session.commit() + return + + @pytest.fixture(scope='function') def source(session) -> dict: args = { From c281524e57ac4d5a6f40db23cc1613260c0662eb Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Mon, 21 Oct 2019 12:27:20 -0400 Subject: [PATCH 02/12] app: add pending reply before adding to send queue --- securedrop_client/api_jobs/uploads.py | 24 ++--------------------- securedrop_client/logic.py | 28 +++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py index 341af484a..409dee5bc 100644 --- a/securedrop_client/api_jobs/uploads.py +++ b/securedrop_client/api_jobs/uploads.py @@ -7,7 +7,7 @@ from securedrop_client.api_jobs.base import ApiJob from securedrop_client.crypto import GpgHelper -from securedrop_client.db import Reply, ReplySendStatus, Source +from securedrop_client.db import Reply, ReplySendStatus logger = logging.getLogger(__name__) @@ -36,27 +36,7 @@ def call_api(self, api_client: API, session: Session) -> str: ''' try: encrypted_reply = self.gpg.encrypt_to_source(self.source_uuid, self.message) - - # Before we send the reply, add to the database with a PENDING reply send status - source = session.query(Source).filter_by(uuid=self.source_uuid).one() - reply_status = session.query(ReplySendStatus).filter_by( - name=ReplySendStatusCodes.PENDING.value).one() - - # Filename will be (interaction_count + 1) || journalist_designation - filename = '{}-{}-reply.gpg'.format(source.interaction_count + 1, - source.journalist_designation) - reply_db_object = Reply( - uuid=self.reply_uuid, - source_id=source.id, - filename=filename, - journalist_id=api_client.token_journalist_uuid, - content=self.message, - is_downloaded=True, - is_decrypted=True, - send_status_id=reply_status.id, - ) - session.add(reply_db_object) - session.commit() + reply_db_object = session.query(Reply).filter_by(uuid=self.reply_uuid).one() sdk_reply = self._make_call(encrypted_reply, api_client) # Update reply send status to SUCCEEDED diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index c1497f281..fc11f42dd 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -34,7 +34,7 @@ from securedrop_client.api_jobs.downloads import FileDownloadJob, MessageDownloadJob, \ ReplyDownloadJob, DownloadChecksumMismatchException from securedrop_client.api_jobs.uploads import SendReplyJob, SendReplyJobError, \ - SendReplyJobTimeoutError + SendReplyJobTimeoutError, ReplySendStatusCodes from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobException from securedrop_client.crypto import GpgHelper, CryptoError from securedrop_client.export import Export @@ -341,7 +341,8 @@ def on_get_current_user_success(self, result) -> None: result['first_name'], result['last_name'], self.session) - self.gui.show_main_window(user) + self.user = user + self.gui.show_main_window(self.user) def on_get_current_user_failure(self, result: Exception) -> None: self.api = None @@ -749,6 +750,29 @@ def send_reply(self, source_uuid: str, reply_uuid: str, message: str) -> None: """ Send a reply to a source. """ + # Before we send the reply, add to the database with a PENDING reply send status + source = self.session.query(db.Source).filter_by(uuid=source_uuid).one() + reply_status = self.session.query(db.ReplySendStatus).filter_by( + name=ReplySendStatusCodes.PENDING.value).one() + + # Filename will be (interaction_count + 1) || journalist_designation + source.interaction_count += 1 + filename = '{}-{}-reply.gpg'.format(source.interaction_count, + source.journalist_designation) + reply_db_object = db.Reply( + uuid=reply_uuid, + source_id=source.id, + filename=filename, + journalist_id=self.user.uuid, + content=message, + is_downloaded=True, + is_decrypted=True, + send_status_id=reply_status.id, + ) + self.session.add(reply_db_object) + self.session.add(source) + self.session.commit() + job = SendReplyJob( source_uuid, reply_uuid, From 99586978b3da2848cb73d4bb80350b4622fb0356 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Mon, 21 Oct 2019 12:32:45 -0400 Subject: [PATCH 03/12] app: failed/pending replies don't get deleted by the sync action --- securedrop_client/storage.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index cc81c7d56..b59ee78e7 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -62,9 +62,12 @@ def get_local_files(session: Session) -> List[File]: def get_local_replies(session: Session) -> List[Reply]: """ - Return all reply objects from the local database. + Return all reply objects from the local database that are successful + (i.e. those that are not failed or pending). """ - return session.query(Reply).all() + success_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.SUCCEEDED.value).one() + return session.query(Reply).filter_by(send_status_id=success_status.id).all() def get_remote_data(api: API) -> Tuple[List[SDKSource], List[SDKSubmission], List[SDKReply]]: @@ -242,7 +245,7 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], * Existing replies are updated in the local database. * New replies have an entry created in the local database. * Local replies not returned in the remote replies are deleted from the - local database. + local database unless they are pending or failed. If a reply references a new journalist username, add them to the database as a new user. @@ -284,7 +287,8 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], # The uuids remaining in local_uuids do not exist on the remote server, so # delete the related records. - for deleted_reply in [r for r in local_replies if r.uuid in local_uuids]: + replies_to_delete = [r for r in local_replies if r.uuid in local_uuids] + for deleted_reply in replies_to_delete: delete_single_submission_or_reply_on_disk(deleted_reply, data_dir) session.delete(deleted_reply) logger.debug('Deleted reply {}'.format(deleted_reply.uuid)) From 1325068df6271c06ef899fded53a2667c26905ba Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Thu, 24 Oct 2019 17:16:37 -0400 Subject: [PATCH 04/12] app: create separate object for draft/pending/failed replies during the sync, we don't attempt to delete draft replies in the source.collection they aren't stored on disk, but will get deleted by the cascade delete when the source is deleted. however we also ensure that duplicate drafts are cleaned up on sync to handle the scenario where a ReplySendJob "fails" but the reply _was_ actually saved properly on the server. --- .../versions/86b01b6290da_add_reply_draft.py | 67 +++++++++ .../86b01b6290da_add_reply_send_status.py | 130 ------------------ create_dev_data.py | 12 +- securedrop_client/api_jobs/downloads.py | 8 +- securedrop_client/api_jobs/uploads.py | 58 ++++++-- securedrop_client/db.py | 44 +++++- securedrop_client/gui/widgets.py | 19 ++- securedrop_client/logic.py | 19 +-- securedrop_client/storage.py | 29 ++-- 9 files changed, 195 insertions(+), 191 deletions(-) create mode 100644 alembic/versions/86b01b6290da_add_reply_draft.py delete mode 100644 alembic/versions/86b01b6290da_add_reply_send_status.py diff --git a/alembic/versions/86b01b6290da_add_reply_draft.py b/alembic/versions/86b01b6290da_add_reply_draft.py new file mode 100644 index 000000000..411ee82c6 --- /dev/null +++ b/alembic/versions/86b01b6290da_add_reply_draft.py @@ -0,0 +1,67 @@ +"""add reply draft + +Revision ID: 86b01b6290da +Revises: 36a79ffcfbfb +Create Date: 2019-10-17 09:45:07.643076 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '86b01b6290da' +down_revision = '36a79ffcfbfb' +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table('replysendstatuses', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('name', sa.String(length=36), nullable=False), + sa.PrimaryKeyConstraint('id', name=op.f('pk_replysendstatuses')), + sa.UniqueConstraint('name', name=op.f('uq_replysendstatuses_name')) + ) + + # Set the initial in-progress send statuses: PENDING, FAILED + conn = op.get_bind() + conn.execute(''' + INSERT INTO replysendstatuses + ('name') + VALUES + ('PENDING'), + ('FAILED'); + ''') + + op.create_table( + 'draftreplies', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('uuid', sa.String(length=36), nullable=False), + sa.Column('timestamp', sa.DateTime(), nullable=False), + sa.Column('source_id', sa.Integer(), nullable=False), + sa.Column('journalist_id', sa.Integer(), nullable=True), + sa.Column('file_counter', sa.Integer(), nullable=False), + sa.Column('content', sa.Text(), nullable=True), + sa.Column('send_status_id', sa.Integer(), nullable=True), + sa.PrimaryKeyConstraint('id', name=op.f('pk_draftreplies')), + sa.UniqueConstraint('uuid', name=op.f('uq_draftreplies_uuid')), + sa.ForeignKeyConstraint( + ['source_id'], + ['sources.id'], + name=op.f('fk_draftreplies_source_id_sources')), + sa.ForeignKeyConstraint( + ['journalist_id'], + ['users.id'], + name=op.f('fk_draftreplies_journalist_id_users')), + sa.ForeignKeyConstraint( + ['send_status_id'], + ['replysendstatuses.id'], + op.f('fk_draftreplies_send_status_id_replysendstatuses'), + ) + ) + + +def downgrade(): + op.drop_table('draftreplies') + op.drop_table('replysendstatuses') diff --git a/alembic/versions/86b01b6290da_add_reply_send_status.py b/alembic/versions/86b01b6290da_add_reply_send_status.py deleted file mode 100644 index 175c9b76e..000000000 --- a/alembic/versions/86b01b6290da_add_reply_send_status.py +++ /dev/null @@ -1,130 +0,0 @@ -"""add reply send status - -Revision ID: 86b01b6290da -Revises: 36a79ffcfbfb -Create Date: 2019-10-17 09:45:07.643076 - -""" -from alembic import op -import sqlalchemy as sa - - -# revision identifiers, used by Alembic. -revision = '86b01b6290da' -down_revision = '36a79ffcfbfb' -branch_labels = None -depends_on = None - - -def upgrade(): - op.create_table('replysendstatuses', - sa.Column('id', sa.Integer(), nullable=False), - sa.Column('name', sa.String(length=36), nullable=False), - sa.PrimaryKeyConstraint('id', name=op.f('pk_replysendstatuses')), - sa.UniqueConstraint('name', name=op.f('uq_replysendstatuses_name')) - ) - - # Set the initial statuses: PENDING, FAILED, SUCCEEDED - conn = op.get_bind() - conn.execute(''' - INSERT INTO replysendstatuses - ('name') - VALUES - ('PENDING'), - ('SUCCEEDED'), - ('FAILED'); - ''') - - op.rename_table('replies', 'replies_tmp') - op.add_column('replies_tmp', sa.Column('send_status_id', sa.Integer(), nullable=True)) - op.create_table( - 'replies', - sa.Column('id', sa.Integer(), nullable=False), - sa.Column('uuid', sa.String(length=36), nullable=False), - sa.Column('source_id', sa.Integer(), nullable=False), - sa.Column('filename', sa.String(length=255), nullable=False), - sa.Column('file_counter', sa.Integer(), nullable=False), - sa.Column('size', sa.Integer(), nullable=True), - sa.Column('content', sa.Text(), nullable=True), - sa.Column('is_decrypted', sa.Boolean(name='is_decrypted'), nullable=True), - sa.Column('is_downloaded', sa.Boolean(name='is_downloaded'), nullable=True), - sa.Column('journalist_id', sa.Integer(), nullable=True), - sa.Column('send_status_id', sa.Integer(), nullable=True), - sa.PrimaryKeyConstraint('id', name=op.f('pk_replies')), - sa.UniqueConstraint('source_id', 'file_counter', name='uq_messages_source_id_file_counter'), - sa.UniqueConstraint('uuid', name=op.f('uq_replies_uuid')), - sa.ForeignKeyConstraint( - ['source_id'], - ['sources.id'], - name=op.f('fk_replies_source_id_sources')), - sa.ForeignKeyConstraint( - ['journalist_id'], - ['users.id'], - name=op.f('fk_replies_journalist_id_users')), - sa.CheckConstraint( - 'CASE WHEN is_downloaded = 0 THEN content IS NULL ELSE 1 END', - name='replies_compare_download_vs_content'), - sa.CheckConstraint( - 'CASE WHEN is_downloaded = 0 THEN is_decrypted IS NULL ELSE 1 END', - name='replies_compare_is_downloaded_vs_is_decrypted'), - sa.ForeignKeyConstraint( - ['send_status_id'], - ['replysendstatuses.id'], - op.f('fk_replies_send_status_replysendstatuses'), - ) - ) - - conn.execute(''' - INSERT INTO replies - SELECT id, uuid, source_id, filename, file_counter, size, content, is_decrypted, - is_downloaded, journalist_id, send_status_id - FROM replies_tmp - ''') - op.drop_table('replies_tmp') - - -def downgrade(): - op.rename_table('replies', 'replies_tmp') - - op.create_table( - 'replies', - sa.Column('id', sa.Integer(), nullable=False), - sa.Column('uuid', sa.String(length=36), nullable=False), - sa.Column('source_id', sa.Integer(), nullable=False), - sa.Column('filename', sa.String(length=255), nullable=False), - sa.Column('file_counter', sa.Integer(), nullable=False), - sa.Column('size', sa.Integer(), nullable=True), - sa.Column('content', sa.Text(), nullable=True), - sa.Column('is_decrypted', sa.Boolean(name='is_decrypted'), nullable=True), - sa.Column('is_downloaded', sa.Boolean(name='is_downloaded'), nullable=True), - sa.Column('journalist_id', sa.Integer(), nullable=True), - sa.PrimaryKeyConstraint('id', name=op.f('pk_replies')), - sa.UniqueConstraint('source_id', 'file_counter', name='uq_messages_source_id_file_counter'), - sa.UniqueConstraint('uuid', name=op.f('uq_replies_uuid')), - sa.ForeignKeyConstraint( - ['source_id'], - ['sources.id'], - name=op.f('fk_replies_source_id_sources')), - sa.ForeignKeyConstraint( - ['journalist_id'], - ['users.id'], - name=op.f('fk_replies_journalist_id_users')), - sa.CheckConstraint( - 'CASE WHEN is_downloaded = 0 THEN content IS NULL ELSE 1 END', - name='replies_compare_download_vs_content'), - sa.CheckConstraint( - 'CASE WHEN is_downloaded = 0 THEN is_decrypted IS NULL ELSE 1 END', - name='replies_compare_is_downloaded_vs_is_decrypted'), - sa.CheckConstraint( - 'CASE WHEN is_decrypted = 0 THEN content IS NULL ELSE 1 END', - name='replies_compare_is_decrypted_vs_content')) - - conn = op.get_bind() - conn.execute(''' - INSERT INTO replies - SELECT id, uuid, source_id, filename, file_counter, size, content, is_decrypted, - is_downloaded, journalist_id - FROM replies_tmp - ''') - op.drop_table('replies_tmp') - op.drop_table('replysendstatuses') diff --git a/create_dev_data.py b/create_dev_data.py index 1498fb46c..4c55a87d1 100755 --- a/create_dev_data.py +++ b/create_dev_data.py @@ -1,7 +1,9 @@ #!/usr/bin/env python3 import json import os +from sqlalchemy.orm.exc import NoResultFound import sys + from securedrop_client.config import Config from securedrop_client.db import Base, make_session_maker, ReplySendStatus from securedrop_client.api_jobs.uploads import ReplySendStatusCodes @@ -16,6 +18,10 @@ })) for reply_send_status in ReplySendStatusCodes: - reply_status = ReplySendStatus(reply_send_status.value) - session.add(reply_status) - session.commit() + try: + reply_status = session.query(ReplySendStatus).filter_by( + name=reply_send_status.value).one() + except NoResultFound: + reply_status = ReplySendStatus(reply_send_status.value) + session.add(reply_status) + session.commit() diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py index 2aec44e94..0824d1548 100644 --- a/securedrop_client/api_jobs/downloads.py +++ b/securedrop_client/api_jobs/downloads.py @@ -14,9 +14,8 @@ from sqlalchemy.orm.session import Session from securedrop_client.api_jobs.base import ApiJob -from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from securedrop_client.crypto import GpgHelper, CryptoError -from securedrop_client.db import File, Message, Reply, ReplySendStatus +from securedrop_client.db import File, Message, Reply from securedrop_client.storage import mark_as_decrypted, mark_as_downloaded, \ set_message_or_reply_content @@ -280,14 +279,11 @@ def call_decrypt(self, filepath: str, session: Session = None) -> str: ''' with NamedTemporaryFile('w+') as plaintext_file: self.gpg.decrypt_submission_or_reply(filepath, plaintext_file.name, is_doc=False) - success_status = session.query(ReplySendStatus).filter_by( - name=ReplySendStatusCodes.SUCCEEDED.value).one() set_message_or_reply_content( model_type=Message, uuid=self.uuid, session=session, - content=plaintext_file.read(), - send_status_id=success_status.id) + content=plaintext_file.read()) return "" diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py index 409dee5bc..7516a952f 100644 --- a/securedrop_client/api_jobs/uploads.py +++ b/securedrop_client/api_jobs/uploads.py @@ -3,18 +3,19 @@ import sdclientapi from sdclientapi import API, RequestTimeoutError +from sqlalchemy import and_ from sqlalchemy.orm.session import Session from securedrop_client.api_jobs.base import ApiJob from securedrop_client.crypto import GpgHelper -from securedrop_client.db import Reply, ReplySendStatus +from securedrop_client.db import DraftReply, Reply, ReplySendStatus, Source logger = logging.getLogger(__name__) class ReplySendStatusCodes(Enum): + """In progress (sending) replies can currently have the following statuses""" PENDING = 'PENDING' - SUCCEEDED = 'SUCCEEDED' FAILED = 'FAILED' @@ -35,17 +36,46 @@ def call_api(self, api_client: API, session: Session) -> str: we can return the reply uuid. ''' try: + draft_reply_db_object = session.query(DraftReply).filter_by(uuid=self.reply_uuid).one() + source = session.query(Source).filter_by(uuid=self.source_uuid).one() + session.commit() + encrypted_reply = self.gpg.encrypt_to_source(self.source_uuid, self.message) - reply_db_object = session.query(Reply).filter_by(uuid=self.reply_uuid).one() + interaction_count = source.interaction_count + 1 + filename = '{}-{}-reply.gpg'.format(interaction_count, + source.journalist_designation) + reply_db_object = Reply( + uuid=self.reply_uuid, + source_id=source.id, + filename=filename, + journalist_id=api_client.token_journalist_uuid, + content=self.message, + is_downloaded=True, + is_decrypted=True, + ) sdk_reply = self._make_call(encrypted_reply, api_client) - # Update reply send status to SUCCEEDED - reply_status = session.query(ReplySendStatus).filter_by( - name=ReplySendStatusCodes.SUCCEEDED.value).one() - reply_db_object.send_status_id = reply_status.id - # Update filename in case it changed on the server + # Update filename and file_counter in case they changed on the server + new_file_counter = int(sdk_reply.filename.split('-')[0]) + reply_db_object.file_counter = new_file_counter reply_db_object.filename = sdk_reply.filename + + draft_file_counter = draft_reply_db_object.file_counter + draft_timestamp = draft_reply_db_object.timestamp + + # If there were replies also in draft state sent after this one, + # re-position them after this successfully sent reply. + for draft_reply in session.query(DraftReply) \ + .filter(and_(DraftReply.source_id == source.id, + DraftReply.timestamp > draft_timestamp, + DraftReply.file_counter == draft_file_counter)) \ + .all(): + draft_reply.file_counter = new_file_counter + session.add(draft_reply) + + # Delete draft, add reply to replies table. session.add(reply_db_object) + session.delete(draft_reply_db_object) session.commit() return reply_db_object.uuid @@ -53,11 +83,11 @@ def call_api(self, api_client: API, session: Session) -> str: message = "Failed to send reply for source {id} due to Exception: {error}".format( id=self.source_uuid, error=e) - # Update reply send status to FAILED + # Update draft reply send status to FAILED reply_status = session.query(ReplySendStatus).filter_by( name=ReplySendStatusCodes.FAILED.value).one() - reply_db_object.send_status_id = reply_status.id - session.add(reply_db_object) + draft_reply_db_object.send_status_id = reply_status.id + session.add(draft_reply_db_object) session.commit() raise SendReplyJobTimeoutError(message, self.reply_uuid) @@ -65,11 +95,11 @@ def call_api(self, api_client: API, session: Session) -> str: message = "Failed to send reply for source {id} due to Exception: {error}".format( id=self.source_uuid, error=e) - # Update reply send status to FAILED + # Update draft reply send status to FAILED reply_status = session.query(ReplySendStatus).filter_by( name=ReplySendStatusCodes.FAILED.value).one() - reply_db_object.send_status_id = reply_status.id - session.add(reply_db_object) + draft_reply_db_object.send_status_id = reply_status.id + session.add(draft_reply_db_object) session.commit() raise SendReplyJobError(message, self.reply_uuid) diff --git a/securedrop_client/db.py b/securedrop_client/db.py index 2205319cd..0c89fa835 100644 --- a/securedrop_client/db.py +++ b/securedrop_client/db.py @@ -1,3 +1,4 @@ +import datetime import os from typing import Any, List, Union # noqa: F401 @@ -54,7 +55,11 @@ def collection(self) -> List: collection.extend(self.messages) collection.extend(self.files) collection.extend(self.replies) - collection.sort(key=lambda x: x.file_counter) + collection.extend(self.draftreplies) + # Sort first by the file_counter, then by timestamp (used only for draft replies). + collection.sort(key=lambda x: (x.file_counter, + getattr(x, "timestamp", + datetime.datetime(datetime.MINYEAR, 1, 1)))) return collection @@ -203,6 +208,37 @@ class Reply(Base): nullable=True, ) + def __init__(self, **kwargs: Any) -> None: + if 'file_counter' in kwargs: + raise TypeError('Cannot manually set file_counter') + filename = kwargs['filename'] + kwargs['file_counter'] = int(filename.split('-')[0]) + super().__init__(**kwargs) + + def __repr__(self) -> str: + return ''.format(self.filename) + + +class DraftReply(Base): + + __tablename__ = 'draftreplies' + + id = Column(Integer, primary_key=True) + uuid = Column(String(36), unique=True, nullable=False) + timestamp = Column(DateTime, nullable=False) + source_id = Column(Integer, ForeignKey('sources.id'), nullable=False) + source = relationship("Source", + backref=backref("draftreplies", order_by=id, + cascade="delete")) + journalist_id = Column(Integer, ForeignKey('users.id')) + journalist = relationship( + "User", backref=backref('draftreplies', order_by=id)) + + # Tracks where in this conversation the draft reply was sent. + # This points to the file_counter of the previous conversation item. + file_counter = Column(Integer, nullable=False) + content = Column(Text) + # This tracks the sending status of the reply. send_status_id = Column( Integer, @@ -211,14 +247,10 @@ class Reply(Base): send_status = relationship("ReplySendStatus") def __init__(self, **kwargs: Any) -> None: - if 'file_counter' in kwargs: - raise TypeError('Cannot manually set file_counter') - filename = kwargs['filename'] - kwargs['file_counter'] = int(filename.split('-')[0]) super().__init__(**kwargs) def __repr__(self) -> str: - return ''.format(self.filename) + return ''.format(self.uuid) class ReplySendStatus(Base): diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index ab81a8e5b..f7c9bd7d3 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -22,7 +22,7 @@ import sys from gettext import gettext as _ -from typing import Dict, List # noqa: F401 +from typing import Dict, List, Union # noqa: F401 from uuid import uuid4 from PyQt5.QtCore import Qt, pyqtSlot, pyqtSignal, QEvent, QTimer, QSize, pyqtBoundSignal, QObject from PyQt5.QtGui import QIcon, QPalette, QBrush, QColor, QFont, QLinearGradient @@ -30,7 +30,7 @@ QPushButton, QVBoxLayout, QLineEdit, QScrollArea, QDialog, QAction, QMenu, QMessageBox, \ QToolButton, QSizePolicy, QPlainTextEdit, QStatusBar, QGraphicsDropShadowEffect, QApplication -from securedrop_client.db import Source, Message, File, Reply, User +from securedrop_client.db import DraftReply, Source, Message, File, Reply, User from securedrop_client.storage import source_exists from securedrop_client.export import ExportStatus from securedrop_client.gui import SecureQLabel, SvgLabel, SvgPushButton, SvgToggleButton @@ -2188,9 +2188,9 @@ def update_conversation(self, collection: list) -> None: self.controller.session.refresh(self.source) # add new items for conversation_item in collection: - if conversation_item.filename.endswith('msg.gpg'): + if isinstance(conversation_item, Message): self.add_message(conversation_item) - elif conversation_item.filename.endswith('reply.gpg'): + elif isinstance(conversation_item, (DraftReply, Reply)): self.add_reply(conversation_item) else: self.add_file(conversation_item) @@ -2225,7 +2225,7 @@ def add_message(self, message: Message) -> None: conversation_item = MessageWidget(message.uuid, content, self.controller.message_ready) self.conversation_layout.addWidget(conversation_item, alignment=Qt.AlignLeft) - def add_reply(self, reply: Reply) -> None: + def add_reply(self, reply: Union[DraftReply, Reply]) -> None: """ Add a reply from a journalist to the source. """ @@ -2234,11 +2234,16 @@ def add_reply(self, reply: Reply) -> None: else: content = '' - logger.debug('adding reply: with status {}'.format(reply.send_status.name)) + try: + send_status = reply.send_status.name + except AttributeError: + send_status = 'SUCCEEDED' + + logger.debug('adding reply: with status {}'.format(send_status)) conversation_item = ReplyWidget( reply.uuid, content, - reply.send_status.name, + send_status, self.controller.reply_ready, self.controller.reply_succeeded, self.controller.reply_failed) diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index fc11f42dd..f0fdc52fe 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -17,6 +17,7 @@ along with this program. If not, see . """ import arrow +from datetime import datetime import inspect import logging import os @@ -750,27 +751,21 @@ def send_reply(self, source_uuid: str, reply_uuid: str, message: str) -> None: """ Send a reply to a source. """ - # Before we send the reply, add to the database with a PENDING reply send status + # Before we send the reply, add the draft to the database with a PENDING + # reply send status. source = self.session.query(db.Source).filter_by(uuid=source_uuid).one() reply_status = self.session.query(db.ReplySendStatus).filter_by( name=ReplySendStatusCodes.PENDING.value).one() - - # Filename will be (interaction_count + 1) || journalist_designation - source.interaction_count += 1 - filename = '{}-{}-reply.gpg'.format(source.interaction_count, - source.journalist_designation) - reply_db_object = db.Reply( + draft_reply = db.DraftReply( uuid=reply_uuid, + timestamp=datetime.utcnow(), source_id=source.id, - filename=filename, journalist_id=self.user.uuid, + file_counter=source.interaction_count, content=message, - is_downloaded=True, - is_decrypted=True, send_status_id=reply_status.id, ) - self.session.add(reply_db_object) - self.session.add(source) + self.session.add(draft_reply) self.session.commit() job = SendReplyJob( diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index b59ee78e7..2bc919f07 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -29,8 +29,7 @@ from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.orm.session import Session -from securedrop_client.api_jobs.uploads import ReplySendStatusCodes -from securedrop_client.db import Source, Message, File, Reply, User, ReplySendStatus +from securedrop_client.db import DraftReply, Source, Message, File, Reply, User from sdclientapi import API from sdclientapi import Source as SDKSource from sdclientapi import Submission as SDKSubmission @@ -62,12 +61,9 @@ def get_local_files(session: Session) -> List[File]: def get_local_replies(session: Session) -> List[Reply]: """ - Return all reply objects from the local database that are successful - (i.e. those that are not failed or pending). + Return all reply objects from the local database that are successful. """ - success_status = session.query(ReplySendStatus).filter_by( - name=ReplySendStatusCodes.SUCCEEDED.value).one() - return session.query(Reply).filter_by(send_status_id=success_status.id).all() + return session.query(Reply).all() def get_remote_data(api: API) -> Tuple[List[SDKSource], List[SDKSubmission], List[SDKReply]]: @@ -167,7 +163,8 @@ def update_sources(remote_sources: List[SDKSource], # delete the related records. for deleted_source in [s for s in local_sources if s.uuid in local_uuids]: for document in deleted_source.collection: - delete_single_submission_or_reply_on_disk(document, data_dir) + if isinstance(document, (Message, File, Reply)): + delete_single_submission_or_reply_on_disk(document, data_dir) session.delete(deleted_source) logger.debug('Deleted source {}'.format(deleted_source.uuid)) @@ -273,15 +270,21 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], reply.journalist_uuid, reply.journalist_username, session) - # All replies fetched from the server have succeeded in being sent. - success_status = session.query(ReplySendStatus).filter_by( - name=ReplySendStatusCodes.SUCCEEDED.value).one() + + # All replies fetched from the server have succeeded in being sent, + # so we should delete the corresponding draft locally if it exists. + try: + draft_reply_db_object = session.query(DraftReply).filter_by( + uuid=reply.uuid).one() + session.delete(draft_reply_db_object) + except NoResultFound: + pass # No draft locally stored corresponding to this reply. + nr = Reply(uuid=reply.uuid, journalist_id=user.id, source_id=source.id, filename=reply.filename, - size=reply.size, - send_status_id=success_status.id) + size=reply.size) session.add(nr) logger.debug('Added new reply {}'.format(reply.uuid)) From 935de8a874bf708970ba5dd6063de6182460d949 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Thu, 24 Oct 2019 17:17:23 -0400 Subject: [PATCH 05/12] test: update tests for pending/failed/draft replies --- tests/api_jobs/test_uploads.py | 45 ++++++++++++++++++++++++++++------ tests/factory.py | 18 ++++++++++++++ tests/gui/test_widgets.py | 6 ++++- tests/test_logic.py | 6 +++-- tests/test_models.py | 45 +++++++++++++++++++++++++++++++++- tests/test_storage.py | 3 +-- 6 files changed, 110 insertions(+), 13 deletions(-) diff --git a/tests/api_jobs/test_uploads.py b/tests/api_jobs/test_uploads.py index 5f335c0c0..4641335d3 100644 --- a/tests/api_jobs/test_uploads.py +++ b/tests/api_jobs/test_uploads.py @@ -13,13 +13,17 @@ def test_SendReplyJobTimeoutError(): assert str(error) == 'mock_message' -def test_send_reply_success(homedir, mocker, session, session_maker): +def test_send_reply_success(homedir, mocker, session, session_maker, + reply_status_codes): ''' Check that the "happy path" of encrypting a message and sending it to the server behaves as expected. ''' source = factory.Source() session.add(source) + msg_uuid = 'xyz456' + draft_reply = factory.DraftReply(uuid=msg_uuid) + session.add(draft_reply) session.commit() gpg = GpgHelper(homedir, session_maker, is_qubes=False) @@ -29,7 +33,6 @@ def test_send_reply_success(homedir, mocker, session, session_maker): encrypted_reply = 's3kr1t m3ss1dg3' mock_encrypt = mocker.patch.object(gpg, 'encrypt_to_source', return_value=encrypted_reply) - msg_uuid = 'xyz456' msg = 'wat' mock_reply_response = sdclientapi.Reply(uuid=msg_uuid, filename='5-dummy-reply') @@ -58,7 +61,8 @@ def test_send_reply_success(homedir, mocker, session, session_maker): assert reply.journalist_id == api_client.token_journalist_uuid -def test_send_reply_failure_gpg_error(homedir, mocker, session, session_maker): +def test_send_reply_failure_gpg_error(homedir, mocker, session, session_maker, + reply_status_codes): ''' Check that if gpg fails when sending a message, we do not call the API, and ensure that SendReplyJobError is raised when there is a CryptoError so we can handle it in @@ -66,6 +70,9 @@ def test_send_reply_failure_gpg_error(homedir, mocker, session, session_maker): ''' source = factory.Source() session.add(source) + msg_uuid = 'xyz456' + draft_reply = factory.DraftReply(uuid=msg_uuid) + session.add(draft_reply) session.commit() gpg = GpgHelper(homedir, session_maker, is_qubes=False) @@ -74,7 +81,6 @@ def test_send_reply_failure_gpg_error(homedir, mocker, session, session_maker): api_client.token_journalist_uuid = 'journalist ID sending the reply' mock_encrypt = mocker.patch.object(gpg, 'encrypt_to_source', side_effect=CryptoError) - msg_uuid = 'xyz456' msg = 'wat' mock_reply_response = sdclientapi.Reply(uuid=msg_uuid, filename='5-dummy-reply') @@ -103,14 +109,21 @@ def test_send_reply_failure_gpg_error(homedir, mocker, session, session_maker): replies = session.query(db.Reply).filter_by(uuid=msg_uuid).all() assert len(replies) == 0 + # Ensure that the draft reply is still in the db + drafts = session.query(db.DraftReply).filter_by(uuid=msg_uuid).all() + assert len(drafts) == 1 -def test_send_reply_failure_unknown_error(homedir, mocker, session, session_maker): + +def test_send_reply_failure_unknown_error(homedir, mocker, session, session_maker, + reply_status_codes): ''' Check that if the SendReplyJob api call fails when sending a message that SendReplyJobError is raised and the reply is not added to the local database. ''' source = factory.Source() session.add(source) + draft_reply = factory.DraftReply(uuid='mock_reply_uuid') + session.add(draft_reply) session.commit() api_client = mocker.MagicMock() mocker.patch.object(api_client, 'reply_source', side_effect=Exception) @@ -125,14 +138,21 @@ def test_send_reply_failure_unknown_error(homedir, mocker, session, session_make replies = session.query(db.Reply).filter_by(uuid='mock_reply_uuid').all() assert len(replies) == 0 + # Ensure that the draft reply is still in the db + drafts = session.query(db.DraftReply).filter_by(uuid='mock_reply_uuid').all() + assert len(drafts) == 1 + -def test_send_reply_failure_timeout_error(homedir, mocker, session, session_maker): +def test_send_reply_failure_timeout_error(homedir, mocker, session, session_maker, + reply_status_codes): ''' Check that if the SendReplyJob api call fails because of a RequestTimeoutError that a SendReplyJobTimeoutError is raised. ''' source = factory.Source() session.add(source) + draft_reply = factory.DraftReply(uuid='mock_reply_uuid') + session.add(draft_reply) session.commit() api_client = mocker.MagicMock() mocker.patch.object(api_client, 'reply_source', side_effect=sdclientapi.RequestTimeoutError) @@ -147,8 +167,13 @@ def test_send_reply_failure_timeout_error(homedir, mocker, session, session_make replies = session.query(db.Reply).filter_by(uuid='mock_reply_uuid').all() assert len(replies) == 0 + # Ensure that the draft reply is still in the db + drafts = session.query(db.DraftReply).filter_by(uuid='mock_reply_uuid').all() + assert len(drafts) == 1 -def test_send_reply_failure_when_repr_is_none(homedir, mocker, session, session_maker): + +def test_send_reply_failure_when_repr_is_none(homedir, mocker, session, session_maker, + reply_status_codes): ''' Check that the SendReplyJob api call results in a SendReplyJobError and nothing else, e.g. no TypeError, when an api call results in an exception that returns None for __repr__ @@ -160,6 +185,8 @@ def __repr__(self): source = factory.Source(uuid='mock_reply_uuid') session.add(source) + draft_reply = factory.DraftReply(uuid='mock_reply_uuid') + session.add(draft_reply) session.commit() api_client = mocker.MagicMock() mocker.patch.object(api_client, 'reply_source', side_effect=MockException('mock')) @@ -175,3 +202,7 @@ def __repr__(self): encrypt_fn.assert_called_once_with(source.uuid, 'mock_message') replies = session.query(db.Reply).filter_by(uuid='mock_reply_uuid').all() assert len(replies) == 0 + + # Ensure that the draft reply is still in the db + drafts = session.query(db.DraftReply).filter_by(uuid='mock_reply_uuid').all() + assert len(drafts) == 1 diff --git a/tests/factory.py b/tests/factory.py index a96d835fb..9e4d2063a 100644 --- a/tests/factory.py +++ b/tests/factory.py @@ -12,6 +12,7 @@ MESSAGE_COUNT = 0 FILE_COUNT = 0 REPLY_COUNT = 0 +DRAFT_REPLY_COUNT = 0 USER_COUNT = 0 @@ -82,6 +83,23 @@ def Reply(**attrs): return db.Reply(**defaults) +def DraftReply(**attrs): + global DRAFT_REPLY_COUNT + DRAFT_REPLY_COUNT += 1 + defaults = dict( + timestamp=datetime.utcnow(), + source_id=1, + journalist_id=1, + file_counter=1, + uuid='draft-reply-uuid-{}'.format(REPLY_COUNT), + content='content', + ) + + defaults.update(attrs) + + return db.DraftReply(**defaults) + + def File(**attrs): global FILE_COUNT FILE_COUNT += 1 diff --git a/tests/gui/test_widgets.py b/tests/gui/test_widgets.py index 332369690..bf653ec9c 100644 --- a/tests/gui/test_widgets.py +++ b/tests/gui/test_widgets.py @@ -1236,6 +1236,7 @@ def test_ReplyWidget_init(mocker): ReplyWidget( 'mock id', 'hello', + 'dummy', mock_update_signal, mock_success_signal, mock_failure_signal, @@ -1852,7 +1853,7 @@ def test_ConversationView_add_reply_from_reply_box(mocker): cv.add_reply_from_reply_box('abc123', 'test message') reply_widget.assert_called_once_with( - 'abc123', 'test message', reply_ready, reply_succeeded, reply_failed) + 'abc123', 'test message', 'PENDING', reply_ready, reply_succeeded, reply_failed) cv.conversation_layout.addWidget.assert_called_once_with( reply_widget_res, alignment=Qt.AlignRight) @@ -1890,6 +1891,7 @@ def test_ConversationView_add_reply(mocker, session, source): mock_reply_widget.assert_called_once_with( reply.uuid, content, + 'SUCCEEDED', mock_reply_ready_signal, mock_reply_succeeded_signal, mock_reply_failed_signal) @@ -1933,6 +1935,7 @@ def test_ConversationView_add_reply_no_content(mocker, session, source): mock_reply_widget.assert_called_once_with( reply.uuid, '', + 'SUCCEEDED', mock_reply_ready_signal, mock_reply_succeeded_signal, mock_reply_failed_signal) @@ -2272,6 +2275,7 @@ def test_ReplyWidget_success_failure_slots(mocker): widget = ReplyWidget(msg_id, 'lol', + 'PENDING', mock_update_signal, mock_success_signal, mock_failure_signal) diff --git a/tests/test_logic.py b/tests/test_logic.py index 7092c7a4d..ef72deaf8 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -1345,12 +1345,14 @@ def test_Controller_delete_source(homedir, config, mocker, session_maker): ) -def test_Controller_send_reply_success(homedir, config, mocker, session_maker, session): +def test_Controller_send_reply_success(homedir, config, mocker, session_maker, session, + reply_status_codes): ''' Check that a SendReplyJob is submitted to the queue when send_reply is called. ''' mock_gui = mocker.MagicMock() co = Controller('http://localhost', mock_gui, session_maker, homedir) + co.user = factory.User() mock_success_signal = mocker.MagicMock() mock_failure_signal = mocker.MagicMock() @@ -1362,9 +1364,9 @@ def test_Controller_send_reply_success(homedir, config, mocker, session_maker, s source = factory.Source() session.add(source) + msg_uuid = 'xyz456' session.commit() - msg_uuid = 'xyz456' msg = 'wat' co.send_reply(source.uuid, msg_uuid, msg) diff --git a/tests/test_models.py b/tests/test_models.py index 2178d9431..e9285d7d9 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,7 +1,8 @@ +import datetime import pytest from tests import factory -from securedrop_client.db import Reply, File, Message, User +from securedrop_client.db import DraftReply, Reply, File, Message, ReplySendStatus, User def test_user_fullname(): @@ -72,6 +73,18 @@ def test_string_representation_of_reply(): reply.__repr__() +def test_string_representation_of_draft_reply(): + user = User(username='hehe') + source = factory.Source() + draft_reply = DraftReply(source=source, journalist=user, uuid='test') + draft_reply.__repr__() + + +def test_string_representation_of_send_reply_status(): + reply_status = ReplySendStatus(name='teehee') + reply_status.__repr__() + + def test_source_collection(): # Create some test submissions and replies source = factory.Source() @@ -92,6 +105,36 @@ def test_source_collection(): assert source.collection[2] == message +def test_source_collection_ordering_with_multiple_draft_replies(): + # Create some test submissions, replies, and draft replies. + source = factory.Source() + file_1 = File(source=source, uuid="test", size=123, filename="1-test.doc.gpg", + download_url='http://test/test') + message_2 = Message(source=source, uuid="test", size=123, filename="2-test.doc.gpg", + download_url='http://test/test') + user = User(username='hehe') + reply_3 = Reply(source=source, journalist=user, filename="3-reply.gpg", + size=1234, uuid='test') + draft_reply_4 = DraftReply(uuid='4', source=source, journalist=user, file_counter=3, + timestamp=datetime.datetime(2000, 6, 6, 6, 0)) + draft_reply_5 = DraftReply(uuid='5', source=source, journalist=user, file_counter=3, + timestamp=datetime.datetime(2001, 6, 6, 6, 0)) + reply_6 = Reply(source=source, journalist=user, filename="4-reply.gpg", + size=1234, uuid='test2') + source.files = [file_1] + source.messages = [message_2] + source.replies = [reply_3, reply_6] + source.draftreplies = [draft_reply_4, draft_reply_5] + + # Now these items should be in the source collection in the proper order + assert source.collection[0] == file_1 + assert source.collection[1] == message_2 + assert source.collection[2] == reply_3 + assert source.collection[3] == draft_reply_4 + assert source.collection[4] == draft_reply_5 + assert source.collection[5] == reply_6 + + def test_file_init(): ''' Check that: diff --git a/tests/test_storage.py b/tests/test_storage.py index 160e9096f..1e16bc005 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -646,8 +646,7 @@ def test_update_replies(homedir, mocker): local_user.username = remote_reply_create.journalist_username local_user.id = 42 mock_session.query().filter_by.side_effect = [[local_source, ], - [local_user, ], - [local_user, ], ] + NoResultFound()] mock_focu = mocker.MagicMock(return_value=local_user) mocker.patch('securedrop_client.storage.find_or_create_user', mock_focu) patch_rename_file = mocker.patch('securedrop_client.storage.rename_file') From 73e13b7b78a563666c3311512f7223ccb6716bd1 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Fri, 25 Oct 2019 17:09:48 -0400 Subject: [PATCH 06/12] test: ensure sync does not delete (non-existent) DraftReply files --- tests/test_storage.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_storage.py b/tests/test_storage.py index 1e16bc005..1fec34f5f 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -167,6 +167,8 @@ def test_update_sources(homedir, mocker): * New sources have an entry in the local database. * Local sources not returned by the remote server are deleted from the local database. + * We don't attempt to delete the (non-existent) files associated with + draft replies. """ mock_session = mocker.MagicMock() # Some source objects from the API, one of which will exist in the local @@ -182,8 +184,14 @@ def test_update_sources(homedir, mocker): local_source1.uuid = source_update.uuid local_source2 = mocker.MagicMock() local_source2.uuid = str(uuid.uuid4()) + draft_reply = factory.DraftReply(uuid='mock_reply_uuid') + local_source2.collection = [draft_reply] local_sources = [local_source1, local_source2] + file_delete_fcn = mocker.patch( + 'securedrop_client.storage.delete_single_submission_or_reply_on_disk') + update_sources(remote_sources, local_sources, mock_session, homedir) + # Check the expected local source object has been updated with values from # the API. assert local_source1.journalist_designation == \ @@ -208,6 +216,10 @@ def test_update_sources(homedir, mocker): # Ensure the record for the local source that is missing from the results # of the API is deleted. mock_session.delete.assert_called_once_with(local_source2) + # Ensure that we didn't attempt to delete files associated with draft replies, + # as they don't have files (content stored directly in the database). + assert file_delete_fcn.call_count == 0 + # Session is committed to database. assert mock_session.commit.call_count == 1 From dc878cdb65661d809dfe68e50d42a2fd0f0c32a7 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Thu, 31 Oct 2019 17:50:27 -0400 Subject: [PATCH 07/12] app: make pending color bar blue (addresses UX feedback) --- securedrop_client/gui/widgets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py index f7c9bd7d3..3cf1b06a6 100644 --- a/securedrop_client/gui/widgets.py +++ b/securedrop_client/gui/widgets.py @@ -1622,7 +1622,7 @@ class ReplyWidget(SpeechBubble): #color_bar { min-height: 5px; max-height: 5px; - background-color: #8715FF; + background-color: #0065db; border: 0px; } ''' From c3fb66694388d99c152b3df84d3d3c71806e5f80 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Thu, 31 Oct 2019 19:36:02 -0400 Subject: [PATCH 08/12] test: add coverage for draft cleanup during sync --- tests/test_storage.py | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/test_storage.py b/tests/test_storage.py index 1fec34f5f..a4fe7a347 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -1,6 +1,7 @@ """ Tests for storage sync logic. """ +import datetime import pytest import os import uuid @@ -689,6 +690,44 @@ def test_update_replies(homedir, mocker): assert mock_session.commit.call_count == 1 +def test_update_replies_cleanup_drafts(homedir, mocker, session): + """ + Check that draft replies are deleted if they correspond to a reply fetched from + the server. + """ + data_dir = os.path.join(homedir, 'data') + # Source object related to the submissions. + source = factory.Source() + user = factory.User() + session.add(source) + session.add(user) + session.commit() + + # One reply will exist on the server. + remote_reply_create = make_remote_reply(source.uuid, 'hehe') + remote_replies = [remote_reply_create] + + # One draft reply will exist in the local database corresponding to the server reply. + draft_reply = db.DraftReply(uuid=remote_reply_create.uuid, source=source, journalist=user, + file_counter=3, timestamp=datetime.datetime(2000, 6, 6, 6, 0)) + session.add(draft_reply) + session.commit() + + # We have no replies locally stored. + local_replies = [] + + update_replies(remote_replies, local_replies, session, data_dir) + + # Check the expected local source object has been created with values from + # the API. + new_local_replies = session.query(db.Reply).all() + assert len(new_local_replies) == 1 + + # Check that the draft is now gone. + new_draft_replies = session.query(db.DraftReply).all() + assert len(new_draft_replies) == 0 + + def test_find_or_create_user_existing_uuid(mocker): """ Return an existing user object with the referenced uuid. From 48807078e16c0d276cb1d8f62581e7747a8fe2af Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Mon, 4 Nov 2019 11:18:05 -0500 Subject: [PATCH 09/12] test: additional coverage for expected ordering of drafts/replies --- tests/api_jobs/test_uploads.py | 75 ++++++++++++++++++++++++++++++++++ tests/test_models.py | 5 ++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/tests/api_jobs/test_uploads.py b/tests/api_jobs/test_uploads.py index 4641335d3..4e2ba3c25 100644 --- a/tests/api_jobs/test_uploads.py +++ b/tests/api_jobs/test_uploads.py @@ -1,3 +1,4 @@ +import datetime import pytest import sdclientapi @@ -61,6 +62,80 @@ def test_send_reply_success(homedir, mocker, session, session_maker, assert reply.journalist_id == api_client.token_journalist_uuid +def test_drafts_ordering(homedir, mocker, session, session_maker, + reply_status_codes): + ''' + Check that if a reply is successful, drafts sent before and after + continue to appear in the same order. + ''' + source = factory.Source(interaction_count=1) + session.add(source) + msg_uuid = 'xyz456' + + draft_reply = factory.DraftReply(uuid=msg_uuid, file_counter=1) + session.add(draft_reply) + session.commit() + + # Draft reply from the previous queue job. + draft_reply_before = factory.DraftReply( + timestamp=draft_reply.timestamp - datetime.timedelta(minutes=1), + source_id=source.id, + file_counter=draft_reply.file_counter, + uuid='foo' + ) + session.add(draft_reply_before) + + # Draft reply that the queue will operate on next. + draft_reply_after = factory.DraftReply( + timestamp=draft_reply.timestamp + datetime.timedelta(minutes=1), + source_id=source.id, + file_counter=draft_reply.file_counter, + uuid='bar' + ) + + session.add(draft_reply_after) + session.commit() + + gpg = GpgHelper(homedir, session_maker, is_qubes=False) + + api_client = mocker.MagicMock() + api_client.token_journalist_uuid = 'journalist ID sending the reply' + + encrypted_reply = 's3kr1t m3ss1dg3' + mock_encrypt = mocker.patch.object(gpg, 'encrypt_to_source', return_value=encrypted_reply) + msg = 'wat' + + mock_reply_response = sdclientapi.Reply(uuid=msg_uuid, filename='2-dummy-reply') + api_client.reply_source = mocker.MagicMock() + api_client.reply_source.return_value = mock_reply_response + + mock_sdk_source = mocker.Mock() + mock_source_init = mocker.patch('securedrop_client.logic.sdclientapi.Source', + return_value=mock_sdk_source) + + job = SendReplyJob( + source.uuid, + msg_uuid, + msg, + gpg, + ) + + job.call_api(api_client, session) + + # ensure message gets encrypted + mock_encrypt.assert_called_once_with(source.uuid, msg) + mock_source_init.assert_called_once_with(uuid=source.uuid) + + # assert reply got added to db + reply = session.query(db.Reply).filter_by(uuid=msg_uuid).one() + assert reply.journalist_id == api_client.token_journalist_uuid + + # Check the ordering displayed to the user + assert source.collection[0] == draft_reply_before + assert source.collection[1] == reply + assert source.collection[2] == draft_reply_after + + def test_send_reply_failure_gpg_error(homedir, mocker, session, session_maker, reply_status_codes): ''' diff --git a/tests/test_models.py b/tests/test_models.py index e9285d7d9..ec27df805 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -121,10 +121,12 @@ def test_source_collection_ordering_with_multiple_draft_replies(): timestamp=datetime.datetime(2001, 6, 6, 6, 0)) reply_6 = Reply(source=source, journalist=user, filename="4-reply.gpg", size=1234, uuid='test2') + draft_reply_7 = DraftReply(uuid='6', source=source, journalist=user, file_counter=4, + timestamp=datetime.datetime(2002, 6, 6, 6, 0)) source.files = [file_1] source.messages = [message_2] source.replies = [reply_3, reply_6] - source.draftreplies = [draft_reply_4, draft_reply_5] + source.draftreplies = [draft_reply_4, draft_reply_5, draft_reply_7] # Now these items should be in the source collection in the proper order assert source.collection[0] == file_1 @@ -133,6 +135,7 @@ def test_source_collection_ordering_with_multiple_draft_replies(): assert source.collection[3] == draft_reply_4 assert source.collection[4] == draft_reply_5 assert source.collection[5] == reply_6 + assert source.collection[6] == draft_reply_7 def test_file_init(): From 03c116b7144fe35184b2bd09c30c2af5eb6c83c5 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Mon, 4 Nov 2019 12:30:20 -0500 Subject: [PATCH 10/12] login/out should fail any pending replies --- securedrop_client/logic.py | 3 +++ securedrop_client/storage.py | 22 +++++++++++++++++++++- tests/test_logic.py | 9 +++++++++ tests/test_storage.py | 22 +++++++++++++++++++++- 4 files changed, 54 insertions(+), 2 deletions(-) diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index f0fdc52fe..7f0350a0d 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -308,6 +308,7 @@ 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. """ + storage.mark_all_pending_drafts_as_failed(self.session) self.api = sdclientapi.API(self.hostname, username, password, totp, self.proxy) self.call_api(self.api.authenticate, self.on_authenticate_success, @@ -355,6 +356,7 @@ def login_offline_mode(self): """ self.gui.hide_login() self.gui.show_main_window() + storage.mark_all_pending_drafts_as_failed(self.session) self.is_authenticated = False self.update_sources() @@ -500,6 +502,7 @@ def logout(self): self.on_logout_failure) self.api = None self.api_job_queue.logout() + storage.mark_all_pending_drafts_as_failed(self.session) self.gui.logout() self.is_authenticated = False diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 2bc919f07..7a0c8a538 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -29,7 +29,8 @@ from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.orm.session import Session -from securedrop_client.db import DraftReply, Source, Message, File, Reply, User +from securedrop_client.db import DraftReply, Source, Message, File, Reply, ReplySendStatus, User +from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from sdclientapi import API from sdclientapi import Source as SDKSource from sdclientapi import Submission as SDKSubmission @@ -509,3 +510,22 @@ def get_message(session: Session, uuid: str) -> Message: def get_reply(session: Session, uuid: str) -> Reply: return session.query(Reply).filter_by(uuid=uuid).one() + + +def mark_all_pending_drafts_as_failed(session: Session) -> None: + """ + When we login (offline or online) or logout, we need to set all + the pending replies as failed. + """ + pending_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.PENDING.value).one() + failed_status = session.query(ReplySendStatus).filter_by( + name=ReplySendStatusCodes.FAILED.value).one() + + pending_drafts = session.query(DraftReply).filter_by( + send_status=pending_status + ).all() + for pending_draft in pending_drafts: + pending_draft.send_status = failed_status + + session.commit() diff --git a/tests/test_logic.py b/tests/test_logic.py index ef72deaf8..a50aeaa4c 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -132,6 +132,8 @@ def test_Controller_login(homedir, config, mocker, session_maker): """ mock_gui = mocker.MagicMock() mock_api = mocker.patch('securedrop_client.logic.sdclientapi.API') + fail_draft_replies = mocker.patch( + 'securedrop_client.storage.mark_all_pending_drafts_as_failed') co = Controller('http://localhost', mock_gui, session_maker, homedir) co.call_api = mocker.MagicMock() @@ -141,6 +143,7 @@ def test_Controller_login(homedir, config, mocker, session_maker): co.call_api.assert_called_once_with(mock_api().authenticate, co.on_authenticate_success, co.on_authenticate_failure) + fail_draft_replies.assert_called_once_with(co.session) def test_Controller_login_offline_mode(homedir, config, mocker): @@ -681,6 +684,8 @@ def test_Controller_logout_success(homedir, config, mocker, session_maker): co.api_job_queue.logout = mocker.MagicMock() co.call_api = mocker.MagicMock() info_logger = mocker.patch('securedrop_client.logic.logging.info') + fail_draft_replies = mocker.patch( + 'securedrop_client.storage.mark_all_pending_drafts_as_failed') logout_method = co.api.logout co.logout() co.call_api.assert_called_with( @@ -693,6 +698,7 @@ def test_Controller_logout_success(homedir, config, mocker, session_maker): co.gui.logout.assert_called_once_with() msg = 'Client logout successful' info_logger.assert_called_once_with(msg) + fail_draft_replies.called_once_with(co.session) def test_Controller_logout_failure(homedir, config, mocker, session_maker): @@ -709,6 +715,8 @@ def test_Controller_logout_failure(homedir, config, mocker, session_maker): co.api_job_queue.logout = mocker.MagicMock() co.call_api = mocker.MagicMock() info_logger = mocker.patch('securedrop_client.logic.logging.info') + fail_draft_replies = mocker.patch( + 'securedrop_client.storage.mark_all_pending_drafts_as_failed') logout_method = co.api.logout co.logout() @@ -723,6 +731,7 @@ def test_Controller_logout_failure(homedir, config, mocker, session_maker): co.gui.logout.assert_called_once_with() msg = 'Client logout failure' info_logger.assert_called_once_with(msg) + fail_draft_replies.called_once_with(co.session) def test_Controller_set_activity_status(homedir, config, mocker, session_maker): diff --git a/tests/test_storage.py b/tests/test_storage.py index a4fe7a347..b8ec1ccca 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -11,12 +11,14 @@ from sqlalchemy.orm.exc import NoResultFound import securedrop_client.db +from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from securedrop_client.storage import get_local_sources, get_local_messages, get_local_replies, \ get_remote_data, update_local_storage, update_sources, update_files, update_messages, \ update_replies, find_or_create_user, find_new_messages, find_new_replies, \ delete_single_submission_or_reply_on_disk, rename_file, get_local_files, find_new_files, \ source_exists, set_message_or_reply_content, mark_as_downloaded, mark_as_decrypted, get_file, \ - get_message, get_reply, update_and_get_user, update_missing_files, mark_as_not_downloaded + get_message, get_reply, update_and_get_user, update_missing_files, mark_as_not_downloaded, \ + mark_all_pending_drafts_as_failed from securedrop_client import db from tests import factory @@ -1077,3 +1079,21 @@ def test_get_reply(mocker, session): result = get_reply(session, reply.uuid) assert result == reply + + +def test_pending_replies_are_marked_as_failed_on_logout_login(mocker, session, + reply_status_codes): + source = factory.Source() + pending_status = session.query(db.ReplySendStatus).filter_by( + name=ReplySendStatusCodes.PENDING.value).one() + failed_status = session.query(db.ReplySendStatus).filter_by( + name=ReplySendStatusCodes.FAILED.value).one() + pending_draft_reply = factory.DraftReply(source=source, + send_status=pending_status) + session.add(source) + session.add(pending_draft_reply) + + mark_all_pending_drafts_as_failed(session) + + for draft in session.query(db.DraftReply).all(): + assert draft.send_status == failed_status From 95db9b4feffb70a936b4b9666ff0f12d6a13bbba Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Tue, 5 Nov 2019 18:08:57 -0500 Subject: [PATCH 11/12] app, test: run draft re-ordering after sync in addition to reply send if a user sends multiple replies A, B, C, the order should always be A, B, C, even if: * A fails * B sends successfully * C is pending we ensure that once a reply sends successfully - or we find that a reply _did_ send but we have it marked as failed as we never got the response (h/t @creviera for testing this case) - that we ensure that C appears after B. this is done by updating the file_counter. --- create_dev_data.py | 3 +- securedrop_client/api_jobs/uploads.py | 22 +++----------- securedrop_client/db.py | 7 +++++ securedrop_client/logic.py | 4 +-- securedrop_client/storage.py | 43 +++++++++++++++++++++------ tests/conftest.py | 4 +-- tests/test_storage.py | 23 +++++++++----- 7 files changed, 65 insertions(+), 41 deletions(-) diff --git a/create_dev_data.py b/create_dev_data.py index 4c55a87d1..808bb33a9 100755 --- a/create_dev_data.py +++ b/create_dev_data.py @@ -5,8 +5,7 @@ import sys from securedrop_client.config import Config -from securedrop_client.db import Base, make_session_maker, ReplySendStatus -from securedrop_client.api_jobs.uploads import ReplySendStatusCodes +from securedrop_client.db import Base, make_session_maker, ReplySendStatus, ReplySendStatusCodes sdc_home = sys.argv[1] session = make_session_maker(sdc_home)() diff --git a/securedrop_client/api_jobs/uploads.py b/securedrop_client/api_jobs/uploads.py index 7516a952f..72f38ff42 100644 --- a/securedrop_client/api_jobs/uploads.py +++ b/securedrop_client/api_jobs/uploads.py @@ -1,24 +1,17 @@ -from enum import Enum import logging import sdclientapi from sdclientapi import API, RequestTimeoutError -from sqlalchemy import and_ from sqlalchemy.orm.session import Session from securedrop_client.api_jobs.base import ApiJob from securedrop_client.crypto import GpgHelper -from securedrop_client.db import DraftReply, Reply, ReplySendStatus, Source +from securedrop_client.db import DraftReply, Reply, ReplySendStatus, ReplySendStatusCodes, Source +from securedrop_client.storage import update_draft_replies logger = logging.getLogger(__name__) -class ReplySendStatusCodes(Enum): - """In progress (sending) replies can currently have the following statuses""" - PENDING = 'PENDING' - FAILED = 'FAILED' - - class SendReplyJob(ApiJob): def __init__(self, source_uuid: str, reply_uuid: str, message: str, gpg: GpgHelper) -> None: super().__init__() @@ -63,15 +56,8 @@ def call_api(self, api_client: API, session: Session) -> str: draft_file_counter = draft_reply_db_object.file_counter draft_timestamp = draft_reply_db_object.timestamp - # If there were replies also in draft state sent after this one, - # re-position them after this successfully sent reply. - for draft_reply in session.query(DraftReply) \ - .filter(and_(DraftReply.source_id == source.id, - DraftReply.timestamp > draft_timestamp, - DraftReply.file_counter == draft_file_counter)) \ - .all(): - draft_reply.file_counter = new_file_counter - session.add(draft_reply) + update_draft_replies(session, source.id, draft_timestamp, + draft_file_counter, new_file_counter) # Delete draft, add reply to replies table. session.add(reply_db_object) diff --git a/securedrop_client/db.py b/securedrop_client/db.py index 0c89fa835..3a7ee6e1c 100644 --- a/securedrop_client/db.py +++ b/securedrop_client/db.py @@ -1,4 +1,5 @@ import datetime +from enum import Enum import os from typing import Any, List, Union # noqa: F401 @@ -268,6 +269,12 @@ def __repr__(self) -> str: return ''.format(self.name) +class ReplySendStatusCodes(Enum): + """In progress (sending) replies can currently have the following statuses""" + PENDING = 'PENDING' + FAILED = 'FAILED' + + class User(Base): __tablename__ = 'users' diff --git a/securedrop_client/logic.py b/securedrop_client/logic.py index 7f0350a0d..0a104923a 100644 --- a/securedrop_client/logic.py +++ b/securedrop_client/logic.py @@ -35,7 +35,7 @@ from securedrop_client.api_jobs.downloads import FileDownloadJob, MessageDownloadJob, \ ReplyDownloadJob, DownloadChecksumMismatchException from securedrop_client.api_jobs.uploads import SendReplyJob, SendReplyJobError, \ - SendReplyJobTimeoutError, ReplySendStatusCodes + SendReplyJobTimeoutError from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobException from securedrop_client.crypto import GpgHelper, CryptoError from securedrop_client.export import Export @@ -758,7 +758,7 @@ def send_reply(self, source_uuid: str, reply_uuid: str, message: str) -> None: # reply send status. source = self.session.query(db.Source).filter_by(uuid=source_uuid).one() reply_status = self.session.query(db.ReplySendStatus).filter_by( - name=ReplySendStatusCodes.PENDING.value).one() + name=db.ReplySendStatusCodes.PENDING.value).one() draft_reply = db.DraftReply( uuid=reply_uuid, timestamp=datetime.utcnow(), diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 7a0c8a538..23e11c6e1 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -19,18 +19,19 @@ You should have received a copy of the GNU Affero General Public License along with this program. If not, see . """ +from datetime import datetime import logging import glob import os from dateutil.parser import parse from typing import List, Tuple, Type, Union -from sqlalchemy import or_ +from sqlalchemy import and_, or_ from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.orm.session import Session -from securedrop_client.db import DraftReply, Source, Message, File, Reply, ReplySendStatus, User -from securedrop_client.api_jobs.uploads import ReplySendStatusCodes +from securedrop_client.db import (DraftReply, Source, Message, File, Reply, ReplySendStatus, + ReplySendStatusCodes, User) from sdclientapi import API from sdclientapi import Source as SDKSource from sdclientapi import Submission as SDKSubmission @@ -272,21 +273,28 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply], reply.journalist_username, session) + nr = Reply(uuid=reply.uuid, + journalist_id=user.id, + source_id=source.id, + filename=reply.filename, + size=reply.size) + session.add(nr) + # All replies fetched from the server have succeeded in being sent, # so we should delete the corresponding draft locally if it exists. try: draft_reply_db_object = session.query(DraftReply).filter_by( uuid=reply.uuid).one() + + update_draft_replies(session, draft_reply_db_object.source.id, + draft_reply_db_object.timestamp, + draft_reply_db_object.file_counter, + nr.file_counter) session.delete(draft_reply_db_object) + except NoResultFound: pass # No draft locally stored corresponding to this reply. - nr = Reply(uuid=reply.uuid, - journalist_id=user.id, - source_id=source.id, - filename=reply.filename, - size=reply.size) - session.add(nr) logger.debug('Added new reply {}'.format(reply.uuid)) # The uuids remaining in local_uuids do not exist on the remote server, so @@ -357,6 +365,23 @@ def update_missing_files(data_dir: str, session: Session) -> None: mark_as_not_downloaded(file.uuid, session) +def update_draft_replies(session: Session, source_id: int, timestamp: datetime, + old_file_counter: int, new_file_counter: int) -> None: + """ + When we confirm a sent reply, if there are drafts that were sent after it, + we need to reposition them to ensure that they appear _after_ the confirmed + replies. + """ + for draft_reply in session.query(DraftReply) \ + .filter(and_(DraftReply.source_id == source_id, + DraftReply.timestamp > timestamp, + DraftReply.file_counter == old_file_counter)) \ + .all(): + draft_reply.file_counter = new_file_counter + session.add(draft_reply) + session.commit() + + def find_new_files(session: Session) -> List[File]: return session.query(File).filter_by(is_downloaded=False).all() diff --git a/tests/conftest.py b/tests/conftest.py index 2b8604830..575abc285 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,9 +6,9 @@ from configparser import ConfigParser from datetime import datetime from securedrop_client.config import Config -from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from securedrop_client.app import configure_locale_and_language -from securedrop_client.db import Base, make_session_maker, Source, ReplySendStatus +from securedrop_client.db import (Base, make_session_maker, Source, ReplySendStatus, + ReplySendStatusCodes) from uuid import uuid4 diff --git a/tests/test_storage.py b/tests/test_storage.py index b8ec1ccca..729b4ad4d 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -11,7 +11,6 @@ from sqlalchemy.orm.exc import NoResultFound import securedrop_client.db -from securedrop_client.api_jobs.uploads import ReplySendStatusCodes from securedrop_client.storage import get_local_sources, get_local_messages, get_local_replies, \ get_remote_data, update_local_storage, update_sources, update_files, update_messages, \ update_replies, find_or_create_user, find_new_messages, find_new_replies, \ @@ -59,7 +58,7 @@ def make_remote_reply(source_uuid, journalist_uuid='testymctestface'): """ source_url = '/api/v1/sources/{}'.format(source_uuid) return Reply(filename='1-reply.filename', journalist_uuid=journalist_uuid, - journalist_username='test', + journalist_username='test', file_counter=1, is_deleted_by_source=False, reply_url='test', size=1234, source_url=source_url, uuid=str(uuid.uuid4())) @@ -713,6 +712,12 @@ def test_update_replies_cleanup_drafts(homedir, mocker, session): draft_reply = db.DraftReply(uuid=remote_reply_create.uuid, source=source, journalist=user, file_counter=3, timestamp=datetime.datetime(2000, 6, 6, 6, 0)) session.add(draft_reply) + # Another draft reply will exist that should be moved to _after_ the new reply + # once we confirm the previous reply. This ensures consistent ordering of interleaved + # drafts (pending and failed) with replies, messages, and files from the user's perspective. + draft_reply_new = db.DraftReply(uuid='foo', source=source, journalist=user, + file_counter=3, timestamp=datetime.datetime(2001, 6, 6, 6, 0)) + session.add(draft_reply_new) session.commit() # We have no replies locally stored. @@ -720,14 +725,16 @@ def test_update_replies_cleanup_drafts(homedir, mocker, session): update_replies(remote_replies, local_replies, session, data_dir) - # Check the expected local source object has been created with values from - # the API. + # Check the expected local source object has been created. new_local_replies = session.query(db.Reply).all() assert len(new_local_replies) == 1 - # Check that the draft is now gone. + # Check that the only draft is the one sent with uuid 'foo' and its file_counter now + # matches the file_counter of the updated reply. This ensures consistent ordering. new_draft_replies = session.query(db.DraftReply).all() - assert len(new_draft_replies) == 0 + assert len(new_draft_replies) == 1 + assert new_draft_replies[0].file_counter == new_local_replies[0].file_counter + assert new_draft_replies[0].uuid == draft_reply_new.uuid def test_find_or_create_user_existing_uuid(mocker): @@ -1085,9 +1092,9 @@ def test_pending_replies_are_marked_as_failed_on_logout_login(mocker, session, reply_status_codes): source = factory.Source() pending_status = session.query(db.ReplySendStatus).filter_by( - name=ReplySendStatusCodes.PENDING.value).one() + name=db.ReplySendStatusCodes.PENDING.value).one() failed_status = session.query(db.ReplySendStatus).filter_by( - name=ReplySendStatusCodes.FAILED.value).one() + name=db.ReplySendStatusCodes.FAILED.value).one() pending_draft_reply = factory.DraftReply(source=source, send_status=pending_status) session.add(source) From 44c439490770b350ba80144c34b7796ab1d12307 Mon Sep 17 00:00:00 2001 From: redshiftzero Date: Wed, 6 Nov 2019 10:41:00 -0500 Subject: [PATCH 12/12] app: explain the client-side re-ordering in a docstring --- securedrop_client/storage.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/securedrop_client/storage.py b/securedrop_client/storage.py index 23e11c6e1..494871c76 100644 --- a/securedrop_client/storage.py +++ b/securedrop_client/storage.py @@ -368,9 +368,30 @@ def update_missing_files(data_dir: str, session: Session) -> None: def update_draft_replies(session: Session, source_id: int, timestamp: datetime, old_file_counter: int, new_file_counter: int) -> None: """ - When we confirm a sent reply, if there are drafts that were sent after it, + When we confirm a sent reply R, if there are drafts that were sent after it, we need to reposition them to ensure that they appear _after_ the confirmed - replies. + replies. We do this by updating the file_counter stored on the drafts sent + after reply R. + + Example: + 1. Reply Q, has file_counter=2 + 2. User adds DraftReply R, it has file_counter=2 + 3. User adds DraftReply S, it has file_counter=2 and + timestamp(S) > timestamp(R). + 4. DraftReply R saved on server with file_counter=4 (this can happen as other + journalist can be sending replies), it is converted to Reply R locally. + 5. We must now update file_counter on DraftReply S such that it appears + after Reply R in the conversation view. + + Args: + session (Session): The SQLAlchemy session object to be used. + source_id (int): this is the ID of the source that the reply R corresponds to. + timestamp (datetime): this is the timestamp of the draft that corresponds to + reply R. + old_file_counter (int): this is the file_counter of the draft that + corresponds to reply R. + new_file_counter (int): this is the file_counter of the reply R confirmed + as successfully sent from the server. """ for draft_reply in session.query(DraftReply) \ .filter(and_(DraftReply.source_id == source_id,