New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 765525 link

Starred by 6 users

Issue metadata

Status: Started
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 795310



Sign in to add a comment

Unify cryptohome mount methods

Project Member Reported by hashimoto@chromium.org, Sep 15 2017

Issue description

Currently, cryptohome daemon provides 7 mount D-Bus methods: Mount, AsyncMount, MountEx, MountGuest, AsyncMountGuest, MountPublic, AsyncMountPublic.
https://chromium.git.corp.google.com/chromiumos/platform2/+/master/cryptohome/cryptohome.xml

Because of this, we have a lot of duplicated code in platform2/cryptohome/service.cc, and it's really difficult to understand the difference between these mount types.

We should rewrite user code of these methods to use MountEx, and eventually MountEx should be the only mount method provided by cryptohome daemon.
 

Comment 1 by kinaba@chromium.org, Sep 19 2017

Sounds nice!

At M61, I removed the Chrome-side usage of AsyncMountPublic. So it should be the first one that is possible to be cleaned up.
Cc: hidehiko@chromium.org
Owner: maajid@chromium.org
Status: Assigned (was: Available)

Comment 3 by maajid@chromium.org, Nov 14 2017

Status: Started (was: Assigned)
Cc: mnissler@chromium.org
Issue 795310 has been merged into this issue.
Blocking: 795310
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/5ab4ce7ea48ee38e08fee9113895278836e99aa6

commit 5ab4ce7ea48ee38e08fee9113895278836e99aa6
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Jan 31 01:47:17 2018

cryptohome: remove MountPublic and AsyncMountPublic

These methods have no users outside of cryptohome. CL:768329
removed AsyncMountPublic from chrome and CL:527754 switched the
MountPublic implementation to use MountEx. We can also get rid
of the command line action for these and just add a flag for
public mounts that will set things up for MountEx to do public
mounts.

BUG=chromium:765525
TEST=unit tests, command line utility

Change-Id: Id3c7cc7fce9e0125a090c8a36149ff5764a11a55
Reviewed-on: https://chromium-review.googlesource.com/887925
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/etc/Cryptohome.conf
[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/cryptohome.cc
[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/service.h
[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/interface.cc
[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/interface.h
[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/service.cc
[modify] https://crrev.com/5ab4ce7ea48ee38e08fee9113895278836e99aa6/cryptohome/dbus_bindings/org.chromium.CryptohomeInterface.xml

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/30f5a6605919844254e0d62e30a7fd6cff757d8b

commit 30f5a6605919844254e0d62e30a7fd6cff757d8b
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Jan 31 01:47:18 2018

system_api: remove MountPublic and AsyncMountPublic

The AsyncMountPublic method is completely dead, and users should
use MountEx instead of MountPublic anyway.  Remove the constants
so people can't call into them.

CQ-DEPEND=CL:887925
BUG=chromium:765525
TEST=emerge

Change-Id: Id80f68b4ecf9ca18b47173c8698fe99378fff1a0
Reviewed-on: https://chromium-review.googlesource.com/887951
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/30f5a6605919844254e0d62e30a7fd6cff757d8b/dbus/cryptohome/dbus-constants.h

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5b70e88bfe96c1a42d322dc7a7a2e2d166cb3bb

commit f5b70e88bfe96c1a42d322dc7a7a2e2d166cb3bb
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Mon Feb 05 18:34:56 2018

Remove SupervisedUserAuthenticator

This is dead code and we're using ExtendedAuthenticator to call
into cryptohome for supervised users. The only place this type
was actually used was for an enum which was actually the wrong
(but equivalent) enum, so change that to be the correct one.

BUG=chromium:765525

Change-Id: I108813ff8feaab283aaa85f61b32726304ebf431
Reviewed-on: https://chromium-review.googlesource.com/898252
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Eric Caruso <ejcaruso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534436}
[modify] https://crrev.com/f5b70e88bfe96c1a42d322dc7a7a2e2d166cb3bb/chrome/browser/chromeos/BUILD.gn
[delete] https://crrev.com/8c3cf479070c419cd0253846853801083418edea/chrome/browser/chromeos/login/supervised/supervised_user_authenticator.cc
[delete] https://crrev.com/8c3cf479070c419cd0253846853801083418edea/chrome/browser/chromeos/login/supervised/supervised_user_authenticator.h
[modify] https://crrev.com/f5b70e88bfe96c1a42d322dc7a7a2e2d166cb3bb/chrome/browser/chromeos/login/supervised/supervised_user_creation_controller.h
[modify] https://crrev.com/f5b70e88bfe96c1a42d322dc7a7a2e2d166cb3bb/chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.cc
[modify] https://crrev.com/f5b70e88bfe96c1a42d322dc7a7a2e2d166cb3bb/chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.h

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97344d1888a6b420420179db4a0127c2f315cb84

commit 97344d1888a6b420420179db4a0127c2f315cb84
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Mon Feb 05 19:22:26 2018

Remove AsyncMount from CryptohomeClient

This D-Bus method has no users and anyone who wants to mount
cryptohomes should be using MountEx. Delete the API to prevent
new users from coming up.

BUG=chromium:765525

Change-Id: I4baadcefbb137cdabc1a60536d5a8720c4b74f75
Reviewed-on: https://chromium-review.googlesource.com/898258
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Eric Caruso <ejcaruso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534453}
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/cryptohome/async_method_caller.cc
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/cryptohome/async_method_caller.h
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/cryptohome/mock_async_method_caller.cc
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/cryptohome/mock_async_method_caller.h
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/dbus/cryptohome_client.cc
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/dbus/cryptohome_client.h
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/dbus/fake_cryptohome_client.cc
[modify] https://crrev.com/97344d1888a6b420420179db4a0127c2f315cb84/chromeos/dbus/fake_cryptohome_client.h

Project Member

Comment 11 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/014d5ed239f0c1773f509497256c9226e86860d0

commit 014d5ed239f0c1773f509497256c9226e86860d0
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Tue Feb 06 03:08:23 2018

Use cryptohome libraries in platform_CryptohomeChangePassword

This test rolls its own cryptohome CLI calls for everything. Most
of these are strictly worse versions of the tools in the existing
cryptohome libraries but the call to MigrateKey isn't present
there. So, add that to the cryptohome libraries, and then we can
make this test a lot shorter.

This is the last remaining direct call to --action=mount in the
cryptohome CLI so we can now move the mount_vault function to use
--action=mount_ex and deprecate the legacy mount methods.

BUG=chromium:765525
TEST=run platform_CryptohomeChangePassword

Change-Id: Ie8010223eac4445698b7f4ea0209ce1affe16c8c
Reviewed-on: https://chromium-review.googlesource.com/898545
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/014d5ed239f0c1773f509497256c9226e86860d0/client/cros/cryptohome.py
[modify] https://crrev.com/014d5ed239f0c1773f509497256c9226e86860d0/client/site_tests/platform_CryptohomeChangePassword/platform_CryptohomeChangePassword.py

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/a6ceff7e2052583cf19479ca5ec6c5834d3079a3

commit a6ceff7e2052583cf19479ca5ec6c5834d3079a3
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Fri Feb 09 07:42:28 2018

Rewrite platform_CryptohomeTPMReOwn

This test is very old and does not make use of any of the
cryptohome-interfacing libraries, which would greatly simplify
the code. In addition a lot of code is duplicated between the
subtests. On top of that this is the last user of the non-async
mount on the CLI so moving to the cryptohome libraries allows us
to remove that method from cryptohome.

The test should also be a little faster since we only need to
reboot the DUT 3 times instead of 7.

BUG=chromium:765525
TEST=run platform_CryptohomeTPMReOwnServer

Change-Id: If472aeed2070bb22b4bcb759a2c3b5025ef637c8
Reviewed-on: https://chromium-review.googlesource.com/895143
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/a6ceff7e2052583cf19479ca5ec6c5834d3079a3/server/site_tests/platform_CryptohomeTPMReOwnServer/control
[modify] https://crrev.com/a6ceff7e2052583cf19479ca5ec6c5834d3079a3/server/site_tests/platform_CryptohomeTPMReOwnServer/platform_CryptohomeTPMReOwnServer.py
[modify] https://crrev.com/a6ceff7e2052583cf19479ca5ec6c5834d3079a3/client/site_tests/platform_CryptohomeTPMReOwn/platform_CryptohomeTPMReOwn.py

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/22acd6789dbee9061b1b8da5a4311254bdc4de98

commit 22acd6789dbee9061b1b8da5a4311254bdc4de98
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Fri Feb 09 07:41:36 2018

cryptohome: replace AsyncMount with MountEx

AsyncMount is a legacy mount endpoint and we want to replace
all users with MountEx where possible, so change this usage
to an equivalent MountEx call.

BUG=chromium:765525
TEST=platform_CryptohomeStress platform_CryptohomeTPMReOwn
  platform_CryptohomeChangePassword platform_CryptohomeMigrateKey

Change-Id: I0e604e99698acb1a43ca0ab7930b9b9f24fe8903
Reviewed-on: https://chromium-review.googlesource.com/898396
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/22acd6789dbee9061b1b8da5a4311254bdc4de98/client/cros/cryptohome.py

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/ed69e74ee8d6814c0102563d95b8db22af9b6089

commit ed69e74ee8d6814c0102563d95b8db22af9b6089
Author: Greg Kerr <kerrnel@chromium.org>
Date: Wed Feb 14 01:40:36 2018

cryptohome: Remove outdated TODO comment.

This comment is from 2010, and cryptohomed.cc is definitely not a
temporary replacement of shell scripts anymore, so this removes the
comment.

BUG=chromium:765525
TEST=build cryptohome

Change-Id: I445bdceba7babd0b5739f5a209b5f4c97c824712
Reviewed-on: https://chromium-review.googlesource.com/915029
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Will Drewry <wad@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/ed69e74ee8d6814c0102563d95b8db22af9b6089/cryptohome/cryptohomed.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/e2e8cb99fd76b065788e8ef6cb82ea54bfbdaf5d

commit e2e8cb99fd76b065788e8ef6cb82ea54bfbdaf5d
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Feb 14 05:17:22 2018

Tweak login_SameSessionTwice to avoid proxy

This moves another user off CryptohomeProxy so we can get rid of
it and remove AsyncMount.

BUG=chromium:765525
TEST=run test on cyan

Change-Id: I334e599a49edf02be2847422a2c598788d703e8b
Reviewed-on: https://chromium-review.googlesource.com/912099
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/e2e8cb99fd76b065788e8ef6cb82ea54bfbdaf5d/client/site_tests/login_SameSessionTwice/login_SameSessionTwice.py

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/a871064351f0a099320b07f751c3c69395971508

commit a871064351f0a099320b07f751c3c69395971508
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Feb 14 05:16:29 2018

Tweak platform_CryptohomeNonDirs to avoid proxy

This moves another user off CryptohomeProxy so we can get rid of
it and remove AsyncMount. In addition there is a failure message
that is the inverse of what the actual failure issue is, so this
was changed to avoid misleading people reading the test logs.

BUG=chromium:765525
TEST=run test on cyan

Change-Id: I36f4a4225b9c878337af327039ef0c4a57d60691
Reviewed-on: https://chromium-review.googlesource.com/912179
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/a871064351f0a099320b07f751c3c69395971508/client/site_tests/platform_CryptohomeNonDirs/platform_CryptohomeNonDirs.py

Project Member

Comment 17 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/3253d200c48c3460f1450b0a1c0b483087cadb17

commit 3253d200c48c3460f1450b0a1c0b483087cadb17
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Feb 14 05:17:04 2018

Tweak login_OwnershipApi to avoid cryptohome proxy

This moves another user off CryptohomeProxy so we can get rid of
it and remove AsyncMount.

BUG=chromium:765525
TEST=run test on cyan

Change-Id: Ibd7acd7eb0005eebfba7b9622bd4df1c85660d97
Reviewed-on: https://chromium-review.googlesource.com/912392
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/3253d200c48c3460f1450b0a1c0b483087cadb17/client/site_tests/login_OwnershipApi/login_OwnershipApi.py

Project Member

Comment 18 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/4f1c463ed59ae537c9cc7b664d00c82ea1b791d3

commit 4f1c463ed59ae537c9cc7b664d00c82ea1b791d3
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Feb 14 05:16:13 2018

Tweak login_CryptohomeOwnerQuery to avoid proxy

This moves another user off CryptohomeProxy so we can get rid of
it and remove AsyncMount.

BUG=chromium:765525
TEST=run test on cyan

Change-Id: I48c86b713d3c1ed9130d83af2b98abfa0986e011
Reviewed-on: https://chromium-review.googlesource.com/912393
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/4f1c463ed59ae537c9cc7b664d00c82ea1b791d3/client/site_tests/login_CryptohomeOwnerQuery/login_CryptohomeOwnerQuery.py

Project Member

Comment 19 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/a150dda0a6638125fedf71565bb3ece68bae2ec9

commit a150dda0a6638125fedf71565bb3ece68bae2ec9
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Feb 14 05:17:00 2018

Tweak platform_CryptohomeBadPerms to avoid proxy

This moves another user off CryptohomeProxy so we can get rid of
it and remove AsyncMount.

BUG=chromium:765525
TEST=run test on cyan

Change-Id: Iaa4d06397f10dba97c07b5be49e0d257a2073283
Reviewed-on: https://chromium-review.googlesource.com/912554
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/a150dda0a6638125fedf71565bb3ece68bae2ec9/client/site_tests/platform_CryptohomeBadPerms/platform_CryptohomeBadPerms.py

Project Member

Comment 20 by bugdroid1@chromium.org, Feb 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f5f9dabc32da362e8ce8cdb4c70ea3f073d4f426

commit f5f9dabc32da362e8ce8cdb4c70ea3f073d4f426
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Feb 28 20:23:20 2018

Tweak login_GuestAndActualSession to avoid proxy

This moves another user off CryptohomeProxy so we can get rid of
it and remove AsyncMount.

BUG=chromium:765525
TEST=run test on cyan

Change-Id: Ia30ace85baf80af5da014489e660919add459044
Reviewed-on: https://chromium-review.googlesource.com/933370
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/f5f9dabc32da362e8ce8cdb4c70ea3f073d4f426/client/site_tests/login_GuestAndActualSession/login_GuestAndActualSession.py

Project Member

Comment 21 by bugdroid1@chromium.org, Apr 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/289aee342dadf1f1282b03d7fb237c228eb1b6b1

commit 289aee342dadf1f1282b03d7fb237c228eb1b6b1
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Thu Apr 12 01:08:16 2018

cryptohome: remove Mount D-Bus call and AsyncMount method

AsyncMount users have all moved to MountEx. Service::Mount is
still around for the time being because it's used by stateful
recovery and Service::MountEx is tied to D-Bus bindings enough
that moving it over would be a much bigger deal than is in
scope for this patch.

However, since there are no remaining external users of Mount or
AsyncMount, we can get rid of the D-Bus endpoints and interface
functions, so it does still allow us to simplify somewhat.

CQ-DEPEND=CL:1000693
BUG=chromium:765525
TEST=unit tests

Change-Id: Ie38e01dbbcf2d908896a8f9911f2d949b3e531a5
Reviewed-on: https://chromium-review.googlesource.com/900283
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/etc/Cryptohome.conf
[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/cryptohome.cc
[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/service.h
[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/interface.cc
[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/interface.h
[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/service.cc
[modify] https://crrev.com/289aee342dadf1f1282b03d7fb237c228eb1b6b1/cryptohome/dbus_bindings/org.chromium.CryptohomeInterface.xml

Components: OS>Systems>Security
Cc: zuan@chromium.org
Not sure if this is still active, but John perhaps you can take a look as part of the refactoring?
Yes, it was mostly done: we have MountEx and MountGuestEx only now. The only remaining step is figuring out if it makes sense to merge the last two into one.

Sign in to add a comment