Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651343002:
CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. (Closed)
DescriptionCrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend.
The previous implementation assumed that the mount source for the home
directory is /home/.shadow/something. It holds for ecryptfs-based
cryptohome, but not for ext4-encryption backend, where the home directory
becomes merely a bind mount to the encrypted directory. Guest fs is not
affected.
BUG=chromium:684914
TEST=./telemetry/bin/run_tests CrOSInterfaceTest --browser=cros-chrome
TEST=./telemetry/bin/run_tests CrOSCryptohomeTest --browser=cros-chrome
Review-Url: https://codereview.chromium.org/2651343002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a448b4b8357a55bf68bce864c8b350bacf0c1081
Patch Set 1 #Patch Set 2 : Add test. #
Total comments: 8
Patch Set 3 : Made the test more descriptive #
Messages
Total messages: 28 (14 generated)
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kinaba@chromium.org changed reviewers: + achuith@chromium.org
PTAL: achuith. FYI: hashimoto.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you add a unit test for this in cros_interface_unittest.py?
done
friendly ping.
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/cros_interface_unittest.py (right): https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:220: def testIsCryptohomeMounted(self, mock_run_cmd): Sorry I don't understand this test at all - could you please explain?
https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/cros_interface_unittest.py (right): https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:220: def testIsCryptohomeMounted(self, mock_run_cmd): On 2017/02/01 03:05:04, achuithb wrote: > Sorry I don't understand this test at all - could you please explain? It is replacing RunCmdOnDevice by a mock method that emulates `cryptohome-path` and `/bin/df` command (on umounted, mounted, and guest-mounted home directory, distinguished by the account name.) Lines 236 - 244 are then enumerating the expected results. Let me know if you know better way. Another option might be to giving up testing here and instead adding some more in internal/backends/chrome/cros_unittest.py#CrOSCryptohomeTest, where we can exercise real login/logout.
https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/cros_interface_unittest.py (right): https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:220: def testIsCryptohomeMounted(self, mock_run_cmd): On 2017/02/01 03:32:52, kinaba wrote: > On 2017/02/01 03:05:04, achuithb wrote: > > Sorry I don't understand this test at all - could you please explain? > > It is replacing RunCmdOnDevice by a mock method that emulates > `cryptohome-path` and `/bin/df` command (on umounted, mounted, > and guest-mounted home directory, distinguished by the account name.) > > Lines 236 - 244 are then enumerating the expected results. > > > Let me know if you know better way. > Another option might be to giving up testing here and > instead adding some more in > internal/backends/chrome/cros_unittest.py#CrOSCryptohomeTest, > where we can exercise real login/logout. > I understood the part where you replace RunCmdOnDevice with a mock. What is unmount@gmail.com, guest@gmail.com, mount@gmail.com? df gets the mount info - what is args[2]? Sorry, but I'm having a really hard time understanding what's going on here. https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:238: self.assertFalse(cri.IsCryptohomeMounted('unmount@gmail.com', True)) Why is is_guest True here?
> > Let me know if you know better way. > Another option might be to giving up testing here and > instead adding some more in > internal/backends/chrome/cros_unittest.py#CrOSCryptohomeTest, > where we can exercise real login/logout. I don't know what such a test would look like, but I'd be open to it.
https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/cros_interface_unittest.py (right): https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:220: def testIsCryptohomeMounted(self, mock_run_cmd): The mock is emulating the device state that * There is a mock user unmount@gmail.com who is not logged in. * There is a mock user mount@gmail.com who is logged in. * There is a guest account logged in. (under a dummy user name guest@gmail.com This should have been '$guest' to fully match with the real dummy account name, though.) The testee method IsCryptohomeMounted queries the mount info by the following command: df --option `cryptohome-path user $USERNAME` The mocked cryptohome-path returns /home/user/arg[2] (or equally /home/user/$USERNAME), and the mocked df check arg[2] (or equally /home/user/$USERNAME), and depending of the user name, returns the mock mount info. That is, * For /home/user/unmount@gmail.com, an unmounted state: a disk device '/dev/sda1' mounted at '/home'. * For /home/user/mount@gmail.com, a mounted state: a disk device '/dev/sda1' mounted at '/home/user/mount@gmail.com'. * For /home/user/guest@gmail.com, a guest mount state: 'guestfs' mounted at '/home/user/guest@gmail.com'. https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:238: self.assertFalse(cri.IsCryptohomeMounted('unmount@gmail.com', True)) On 2017/02/02 00:21:46, achuithb wrote: > Why is is_guest True here? These series of 6 assertions just testing all the combination of arguments and ensuring the return value is as expected. This line describes the expectation that cri.IsCryptohomeMounted('unmount@gmail.com', True) needs to return False. That is, if a caller asks whether an not-logged-in user's cryptohome is mounted as a guest, the answer has to be False.
https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/cros_interface_unittest.py (right): https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:220: def testIsCryptohomeMounted(self, mock_run_cmd): Let's at least change guest@gmail.com to $guest for consistency.
Description was changed from ========== CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. The previous implementation assumed that the mount source for the home directory is /home/.shadow/something. It holds for ecryptfs-based cryptohome, but not for ext4-encryption backend, where the home directory becomes merely a bind mount to the encrypted directory. Guest fs is not affected. BUG=chromium:684914 ========== to ========== CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. The previous implementation assumed that the mount source for the home directory is /home/.shadow/something. It holds for ecryptfs-based cryptohome, but not for ext4-encryption backend, where the home directory becomes merely a bind mount to the encrypted directory. Guest fs is not affected. BUG=chromium:684914 TEST=./telemetry/bin/run_tests CrOSInterfaceTest --browser=cros-chrome ==========
Description was changed from ========== CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. The previous implementation assumed that the mount source for the home directory is /home/.shadow/something. It holds for ecryptfs-based cryptohome, but not for ext4-encryption backend, where the home directory becomes merely a bind mount to the encrypted directory. Guest fs is not affected. BUG=chromium:684914 TEST=./telemetry/bin/run_tests CrOSInterfaceTest --browser=cros-chrome ========== to ========== CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. The previous implementation assumed that the mount source for the home directory is /home/.shadow/something. It holds for ecryptfs-based cryptohome, but not for ext4-encryption backend, where the home directory becomes merely a bind mount to the encrypted directory. Guest fs is not affected. BUG=chromium:684914 TEST=./telemetry/bin/run_tests CrOSInterfaceTest --browser=cros-chrome TEST=./telemetry/bin/run_tests CrOSCryptohomeTest --browser=cros-chrome ==========
Added comments to the test, and also added a check for this fundtion or an existing real-divice test. https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/cros_interface_unittest.py (right): https://codereview.chromium.org/2651343002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/cros_interface_unittest.py:220: def testIsCryptohomeMounted(self, mock_run_cmd): On 2017/02/03 00:59:46, achuithb wrote: > Let's at least change mailto:guest@gmail.com to $guest for consistency. Done.
lgtm
The CQ bit was checked by kinaba@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486422801625520,
"parent_rev": "34a09f606d37769f6f8f499a4ae8cc57f9cbd6e5", "commit_rev":
"a448b4b8357a55bf68bce864c8b350bacf0c1081"}
Message was sent while issue was closed.
Description was changed from ========== CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. The previous implementation assumed that the mount source for the home directory is /home/.shadow/something. It holds for ecryptfs-based cryptohome, but not for ext4-encryption backend, where the home directory becomes merely a bind mount to the encrypted directory. Guest fs is not affected. BUG=chromium:684914 TEST=./telemetry/bin/run_tests CrOSInterfaceTest --browser=cros-chrome TEST=./telemetry/bin/run_tests CrOSCryptohomeTest --browser=cros-chrome ========== to ========== CrOS: Make IsCryptohomeMounted compatible with ext4-encryption backend. The previous implementation assumed that the mount source for the home directory is /home/.shadow/something. It holds for ecryptfs-based cryptohome, but not for ext4-encryption backend, where the home directory becomes merely a bind mount to the encrypted directory. Guest fs is not affected. BUG=chromium:684914 TEST=./telemetry/bin/run_tests CrOSInterfaceTest --browser=cros-chrome TEST=./telemetry/bin/run_tests CrOSCryptohomeTest --browser=cros-chrome Review-Url: https://codereview.chromium.org/2651343002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
