Skip to content

Commit

Permalink
Use journalist designations, not UUIDs, in data directory
Browse files Browse the repository at this point in the history 
Base directory and file names in the data directory on the journalist
designation of their sources, plus file counters and extensions,
instead of UUIDs.

Remove handling of changes to source journalist designations on the
server; local files will no longer be renamed. The ability to change
journalist designations will soon be removed from SecureDrop core, so
this will not be needed.
  • Loading branch information
@rmol @sssoleileraaa
rmol authored and sssoleileraaa committed Feb 1, 2020
1 parent 82cb509 commit 078c3d0
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 203 deletions.
22 changes: 13 additions & 9 deletions securedrop_client/db.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ def collection(self) -> List:
datetime.datetime(datetime.MINYEAR, 1, 1))))
return collection

@property
def journalist_filename(self) -> str:
valid_chars = 'abcdefghijklmnopqrstuvwxyz1234567890-_'
return ''.join([c for c in self.journalist_designation.lower().replace(
' ', '_') if c in valid_chars])


class Message(Base):

Expand Down Expand Up @@ -130,9 +136,8 @@ def location(self, data_dir: str) -> str:
return os.path.abspath(
os.path.join(
data_dir,
self.__class__.__name__,
self.uuid,
self.filename,
self.source.journalist_filename,
os.path.splitext(self.filename)[0] + '.txt'
)
)

Expand Down Expand Up @@ -197,9 +202,9 @@ def location(self, data_dir: str) -> str:
return os.path.abspath(
os.path.join(
data_dir,
self.__class__.__name__,
self.uuid,
self.filename,
self.source.journalist_filename,
'{}-{}-doc'.format(self.file_counter, self.source.journalist_filename),
self.filename
)
)

Expand Down Expand Up @@ -270,9 +275,8 @@ def location(self, data_dir: str) -> str:
return os.path.abspath(
os.path.join(
data_dir,
self.__class__.__name__,
self.uuid,
self.filename,
self.source.journalist_filename,
os.path.splitext(self.filename)[0] + '.txt'
)
)

Expand Down
24 changes: 0 additions & 24 deletions securedrop_client/storage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,6 @@ def __update_submissions(model: Union[Type[File], Type[Message]],
local_submission = [s for s in local_submissions
if s.uuid == submission.uuid][0]

# Update files on disk to match new filename.
if (
(local_submission.is_downloaded and not local_submission.is_decrypted) and
(local_submission.filename != submission.filename)
):
rename_file(data_dir, local_submission, submission.filename)
local_submission.filename = submission.filename

local_submission.size = submission.size
local_submission.is_read = submission.is_read
local_submission.download_url = submission.download_url
Expand Down Expand Up @@ -258,10 +250,6 @@ def update_replies(remote_replies: List[SDKReply], local_replies: List[Reply],
for reply in remote_replies:
if reply.uuid in local_uuids:
local_reply = [r for r in local_replies if r.uuid == reply.uuid][0]
# Update files on disk to match new filename.
if (local_reply.filename != reply.filename):
rename_file(data_dir, local_reply, reply.filename)
local_reply.filename = reply.filename

user = find_or_create_user(reply.journalist_uuid, reply.journalist_username, session)
local_reply.journalist_id = user.id
Expand Down Expand Up @@ -521,18 +509,6 @@ def delete_single_submission_or_reply_on_disk(obj_db: Union[File, Message, Reply
logging.info('File %s already deleted, skipping', file_to_delete)


def rename_file(data_dir: str, file: Union[Message, File, Reply], new_filename: str) -> None:
try:
current_location = file.location(data_dir)
new_location = os.path.join(
os.path.dirname(current_location),
os.path.basename(new_filename)
)
os.rename(current_location, new_location)
except OSError as e:
logger.debug('File could not be renamed: {}'.format(e))


def source_exists(session: Session, source_uuid: str) -> bool:
try:
session.query(Source).filter_by(uuid=source_uuid).one()
Expand Down
183 changes: 13 additions & 170 deletions tests/test_storage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
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, \
delete_single_submission_or_reply_on_disk, 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, \
mark_all_pending_drafts_as_failed, delete_local_source_by_uuid
Expand Down Expand Up @@ -242,92 +242,6 @@ def test_update_sources(homedir, mocker):
assert mock_session.commit.call_count == 1


def test_update_files_renames_file_on_disk(homedir, mocker):
"""
Check that:
* Submissions are renamed on disk after sync.
"""
data_dir = os.path.join(homedir, 'data')
mock_session = mocker.MagicMock()

# Local submission that needs to be updated
local_submission = factory.File(
source=factory.Source(),
filename="1-unquestionable_complaint-msg.gpg",
is_downloaded=True,
is_decrypted=False,
)
local_submissions = [local_submission]

# Add submission file to test directory
add_test_file_to_temp_dir(data_dir, local_submission.location(data_dir))

mock_session.query().filter_by.return_value = [local_submission.source]

# Remote submission with new filename
remote_submission = mocker.MagicMock()
remote_submission.uuid = local_submission.uuid
remote_submission.filename = '1-spotted-potato-msg.gpg'
remote_submissions = [remote_submission]

update_files(remote_submissions, local_submissions, mock_session, data_dir)

assert local_submission.filename == remote_submission.filename
assert os.path.exists(local_submission.location(data_dir))


def test_update_replies_renames_file_on_disk(homedir, mocker):
"""
Check that:
* Replies are renamed on disk after sync.
"""
data_dir = os.path.join(homedir, 'data')
mock_session = mocker.MagicMock()
# Remote replies with new filename
server_filename = '1-spotted-potato-reply.gpg'
remote_reply = mocker.MagicMock()
remote_reply.uuid = 'test-uuid'
remote_reply.filename = server_filename
remote_replies = [remote_reply]

# Local reply that needs to be updated
local_filename = '1-pericardial-surfacing-reply.gpg'
local_reply = mocker.MagicMock()
local_reply.uuid = 'test-uuid'
local_reply.filename = local_filename
local_reply.location = lambda data_dir: os.path.join(
data_dir, "Reply", local_reply.uuid, local_reply.filename
)
local_replies = [local_reply]

# Add reply file to test directory
add_test_file_to_temp_dir(
data_dir,
os.path.join(
os.path.dirname(local_reply.location(data_dir)),
os.path.splitext(local_reply.filename)[0]
)
)

# There needs to be a corresponding local_source and local_user
local_source = mocker.MagicMock()
local_source.uuid = 'test-source-uuid'
local_source.id = 123

local_user = mocker.MagicMock()
local_user.username = 'journalist designation'
local_user.id = 42

mock_focu = mocker.MagicMock(return_value=local_user)
mocker.patch('securedrop_client.storage.find_or_create_user', mock_focu)

update_replies(remote_replies, local_replies, mock_session, data_dir)

assert local_reply.filename == remote_reply.filename


def add_test_file_to_temp_dir(home_dir, filename):
"""
Add test file with the given filename to data dir.
Expand Down Expand Up @@ -506,10 +420,6 @@ def test_update_files(homedir, mocker):
* New submissions have an entry in the local database.
* Local submission not returned by the remote server are deleted from the
local database.
* `rename_file` is called if local data files need to be updated with new
filenames to match remote. Note: The only reason this should happen is if
there is a new journalist_designation that causes remote filenames to be
updated.
"""
data_dir = os.path.join(homedir, 'data')
mock_session = mocker.MagicMock()
Expand All @@ -529,7 +439,8 @@ def test_update_files(homedir, mocker):
local_sub_update.uuid = remote_sub_update.uuid
local_sub_update.is_downloaded = True
local_sub_update.is_decrypted = False
local_sub_update.filename = "overwrite_this.filename"
local_filename = "originalsubmissionname.txt"
local_sub_update.filename = local_filename
local_sub_delete = mocker.MagicMock()
local_sub_delete.uuid = str(uuid.uuid4())
local_sub_delete.filename = "local_sub_delete.filename"
Expand All @@ -539,18 +450,15 @@ def test_update_files(homedir, mocker):
local_source.uuid = source.uuid
local_source.id = 666 # };-)
mock_session.query().filter_by.return_value = [local_source, ]
patch_rename_file = mocker.patch('securedrop_client.storage.rename_file')

update_files(remote_submissions, local_submissions, mock_session, data_dir)

# Check the expected local submission object has been updated with values
# from the API.
assert local_sub_update.filename == remote_sub_update.filename
assert local_sub_update.filename == local_filename
assert local_sub_update.size == remote_sub_update.size
assert local_sub_update.is_read == remote_sub_update.is_read
# Check that rename_file is called if the local storage filenames need to
# be updated
assert patch_rename_file.called

# Check the expected local source object has been created with values from
# the API.
assert mock_session.add.call_count == 1
Expand All @@ -573,10 +481,6 @@ def test_update_messages(homedir, mocker):
* New messages have an entry in the local database.
* Local messages not returned by the remote server are deleted from the
local database.
* `rename_file` is called if local data files need to be updated with new
filenames to match remote. Note: The only reason this should happen is if
there is a new journalist_designation that causes remote filenames to be
updated.
"""
data_dir = os.path.join(homedir, 'data')
mock_session = mocker.MagicMock()
Expand All @@ -594,7 +498,8 @@ def test_update_messages(homedir, mocker):
# be deleted from the local database).
local_message_update = mocker.MagicMock()
local_message_update.uuid = remote_message_update.uuid
local_message_update.filename = "overwrite_this.filename"
local_filename = "originalsubmissionname.txt"
local_message_update.filename = local_filename
local_message_update.is_downloaded = True
local_message_update.is_decrypted = False
local_message_delete = mocker.MagicMock()
Expand All @@ -612,17 +517,14 @@ def test_update_messages(homedir, mocker):
[local_user, ], ]
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')

update_messages(remote_messages, local_messages, mock_session, data_dir)

# Check the expected local message object has been updated with values
# from the API.
assert local_message_update.filename == remote_message_update.filename
assert local_message_update.filename == local_filename
assert local_message_update.size == remote_message_update.size
# Check that rename_file is called if the local storage filenames need to
# be updated
assert patch_rename_file.called

# Check the expected local source object has been created with values from
# the API.
assert mock_session.add.call_count == 1
Expand All @@ -647,10 +549,6 @@ def test_update_replies(homedir, mocker):
* Local replies not returned by the remote server are deleted from the
local database.
* References to journalist's usernames are correctly handled.
* `rename_file` is called if local data files need to be updated with new
filenames to match remote. Note: The only reason this should happen is if
there is a new journalist_designation that causes remote filenames to be
updated.
"""
data_dir = os.path.join(homedir, 'data')
mock_session = mocker.MagicMock()
Expand All @@ -668,7 +566,8 @@ def test_update_replies(homedir, mocker):
# be deleted from the local database).
local_reply_update = mocker.MagicMock()
local_reply_update.uuid = remote_reply_update.uuid
local_reply_update.filename = "overwrite_this.filename"
local_filename = "originalsubmissionname.txt"
local_reply_update.filename = local_filename
local_reply_update.journalist_uuid = str(uuid.uuid4())
local_reply_delete = mocker.MagicMock()
local_reply_delete.uuid = str(uuid.uuid4())
Expand All @@ -686,18 +585,15 @@ def test_update_replies(homedir, mocker):
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')

update_replies(remote_replies, local_replies, mock_session, data_dir)

# Check the expected local reply object has been updated with values
# from the API.
assert local_reply_update.journalist_id == local_user.id
assert local_reply_update.filename == remote_reply_update.filename
assert local_reply_update.filename == local_filename
assert local_reply_update.size == remote_reply_update.size
# Check that rename_file is called if the local storage filenames need to
# be updated
assert patch_rename_file.called

# Check the expected local source object has been created with values from
# the API.
assert mock_session.add.call_count == 1
Expand Down Expand Up @@ -1021,59 +917,6 @@ def test_delete_single_submission_or_reply_race_guard(homedir, mocker):
mock_remove.call_count == 1


def test_rename_file_does_not_throw(homedir):
"""
If file cannot be found then OSError is caught and logged.
"""
data_dir = os.path.join(homedir, 'data')

original_file = factory.File(
source=factory.Source(),
filename="1-unquestionable_complaint-msg.gpg",
is_downloaded=True,
is_decrypted=False,
)
new_filename = "1-demonstrable_reciprocity-msg.gpg"
rename_file(data_dir, original_file, new_filename)

assert not os.path.exists(original_file.location(data_dir))

original_file.filename = new_filename

assert not os.path.exists(original_file.location(data_dir))


def test_rename_file_success(homedir):
"""
If file was renamed successfully then we should be able to retrieve the
file's content using the new filename.
"""
data_dir = os.path.join(homedir, 'data')
original_file = factory.File(
source=factory.Source(),
filename="1-unquestionable_complaint-msg.gpg",
is_downloaded=True,
is_decrypted=False,
)
contents = 'bar'
os.makedirs(os.path.dirname(original_file.location(data_dir)))
with open(original_file.location(data_dir), 'w') as f:
f.write(contents)

new_filename = "1-demonstrable_reciprocity-msg.gpg"
rename_file(os.path.join(homedir, 'data'), original_file, new_filename)

assert not os.path.exists(original_file.location(data_dir))

original_file.filename = new_filename

assert os.path.exists(original_file.location(data_dir))

with open(original_file.location(data_dir)) as f:
out = f.read()
assert out == contents


def test_source_exists_true(homedir, mocker):
'''
Check that method returns True if a source is return from the query.
Expand Down

0 comments on commit 078c3d0

Please sign in to comment.