Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

check if already unlocked and mounted #39

Merged
merged 1 commit into from
Dec 16, 2019
Merged

check if already unlocked and mounted #39

merged 1 commit into from
Dec 16, 2019

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Dec 12, 2019

Description

Fixes #12, going with option 2 in the expected results: #12 (comment)

We now check if the device is already unlocked and mounted. If the device is mounted, we don't attempt to mount again, instead we use the current mountpoint to copy files to.

We will need to create a followup issue with a new status code to inform the client that the device is already unlocked and no user passphrase is necessary.

**Important: ** If the device is unlocked by an external program we will still lock it after a file transfer. This is to err on the side of caution.

Test Plan

  1. checkout master branch and follow STR in cannot export file if drive is already unlocked #12
  2. checkout this pr branch and follow STR in cannot export file if drive is already unlocked #12 and see that the problem is fixed

@sssoleileraaa sssoleileraaa force-pushed the issue-12-fix branch 2 times, most recently from e54d7d6 to 10f3674 Compare December 13, 2019 07:17
@sssoleileraaa sssoleileraaa marked this pull request as ready for review December 13, 2019 07:17
@eloquence eloquence added this to Ready for Review in SecureDrop Team Board Dec 13, 2019
@sssoleileraaa sssoleileraaa force-pushed the issue-12-fix branch 2 times, most recently from 4d2d659 to 0302908 Compare December 13, 2019 21:30
securedrop_export/export.py Outdated Show resolved Hide resolved
securedrop_export/export.py Show resolved Hide resolved
securedrop_export/export.py Outdated Show resolved Hide resolved
@redshiftzero
Copy link
Contributor

in testing this I keep getting an ERROR_GENERIC due to umount /media/user/<drivename>: not mounted in the finally block of copy_submission, not sure why I'm getting in this state... 🤔 (this is in the case when I've unlocked the drive using Files)

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Dec 13, 2019

umount /media/user/: not mounted

could you copy your logs around the log line Unmounting drive from {}'.format(self.mountpoint))? it's strange because i added a line of code that checks if the drive is mounted before unmounting:

if os.path.exists(self.mountpoint):
     logger.info('Unmounting drive from {}'.format(self.mountpoint))
     subprocess.check_call(["sudo", "umount", self.mountpoint])

it's possible that if you restarted the export vm that you forgot to copy the export deb package with my changes.

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Dec 13, 2019

or actually looks like i accidentally used rstrip instead of strip here:

    def mount_volume(self):
        # If the drive is already mounted then we don't need to mount it again
        output = subprocess.check_output(
            ["lsblk", "-o", "MOUNTPOINT", "--noheadings", self.device])
        mountpoint = output.decode('utf-8').strip()
        if mountpoint:
            logger.debug('The device is already mounted')
            self.mountpoint = mountpoint
            return

fixed, ready for a retest

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

OK this looks good now.

For the record, in reproducing #12 I could reproduce that an ERROR_GENERIC occurs but that was after the files are successfully exported (and thus the success message popped up). Regardless, I can confirm on this diff that I no longer see this behavior for both 1. the drive is already mounted/unlocked and 2. the drive is not mounted/unlocked cases - the export is successful in both cases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

cannot export file if drive is already unlocked
2 participants