cryptohome: Move stateless wrapper functions out of CryptohomeLibrary

cryptohome: Move stateless wrapper functions out of CryptohomeLibrary

These functions don't have to be in CryptohomeLibrary. If you need to
avoid real D-Bus method calls (ex. in tests), you can use
FakeCryptohomeClient.

BUG=141016
TEST=none
R=hashimoto@chromium.org, pneubeck@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226373

[satorux1, 2013-09-27 08:37:19]

hashimoto: everything
pneubeck: policy stuff

There may still be some tests failing, but the patch should be meaty enough to
start looking. thanks!

[pneubeck (no reviews), 2013-09-27 08:59:25]

Please consider the following highlevel comments:

1)
It can make sense to have clear layers, for example where code of upper layers
_only_ access CryptohomeLibrary and not CryptohomeClient. In that case, this
removal would be wrong.

However, it seems that you go into the opposite direction.
Maybe you even want to get rid of CryptohomeLibrary. In which case this change
is mostly ok.

2)
Why not just remove all wrappers except InstallAttributesGet and
InstallAttributesSet ? This are trivial wrappers which are easy to remove. Then
it would also be clearer what these calls depend on (namely DBusThreadManager
and CryptohomeClient).

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:77:
// first with methods in CryptohomeLibrary.
stale comment?

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enterprise_install_attributes.h:21: class
CryptohomeLibrary;
unused

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:71: #include
"chromeos/cryptohome/cryptohome_library.h"
unused after the removal below

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:135: if
(chromeos::CryptohomeLibrary::IsInitialized() &&
this shouldn't be required anymore (update the comment too please)

[satorux1, 2013-09-27 10:12:27]

On 2013/09/27 08:59:25, pneubeck wrote:
> Please consider the following highlevel comments:
> 
> 1)
> It can make sense to have clear layers, for example where code of upper layers
> _only_ access CryptohomeLibrary and not CryptohomeClient. In that case, this
> removal would be wrong.
> 
> However, it seems that you go into the opposite direction.
> Maybe you even want to get rid of CryptohomeLibrary. In which case this change
> is mostly ok.

Thank you for the high-level comment. I think we should remove
CryptohomeLibrary:

1) CryptohomeLibrary provides a layer for switching production and fake, but
CryptohomeClient provides the same. Dont' need two layers.
2) In many places, CryptohomeClient is directly used. Having CryptohomeClient,
which only provides a small subset of functions, makes the code inconsistent.

FWIW, there used to be a number of FooLibrary classes, which have been mostly
removed now. These were needed as a prod/fake switching layer when we were
issuing D-Bus method calls via a dlopen'ed library called libcros, which was
removed by hashimoto and me (crbug.com/127051)



> 2)
> Why not just remove all wrappers except InstallAttributesGet and
> InstallAttributesSet ? This are trivial wrappers which are easy to remove.
Then
> it would also be clearer what these calls depend on (namely DBusThreadManager
> and CryptohomeClient).

I kept these wrappers as removing them would make the client code verbose. For
instance:

  if (cryptohome_->TpmIsEnabled() &&
      !cryptohome_->TpmIsBeingOwned() &&
      cryptohome_->TpmIsOwned()) {

I think we can remove wrappers for TpmIs* once we have TpmGetState()


I'll address the inline comments next week.

>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> File
>
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
>
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:77:
> // first with methods in CryptohomeLibrary.
> stale comment?
> 
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right):
> 
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/policy/enterprise_install_attributes.h:21: class
> CryptohomeLibrary;
> unused
> 
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
> File chrome/browser/policy/browser_policy_connector.cc (right):
> 
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
> chrome/browser/policy/browser_policy_connector.cc:71: #include
> "chromeos/cryptohome/cryptohome_library.h"
> unused after the removal below
> 
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
> chrome/browser/policy/browser_policy_connector.cc:135: if
> (chromeos::CryptohomeLibrary::IsInitialized() &&
> this shouldn't be required anymore (update the comment too please)

[pneubeck (no reviews), 2013-09-27 11:55:16]

On 2013/09/27 10:12:27, satorux1 wrote:
> On 2013/09/27 08:59:25, pneubeck wrote:
> > Please consider the following highlevel comments:
> > 
> > 1)
> > It can make sense to have clear layers, for example where code of upper
layers
> > _only_ access CryptohomeLibrary and not CryptohomeClient. In that case, this
> > removal would be wrong.
> > 
> > However, it seems that you go into the opposite direction.
> > Maybe you even want to get rid of CryptohomeLibrary. In which case this
change
> > is mostly ok.
> 
> Thank you for the high-level comment. I think we should remove
> CryptohomeLibrary:
> 
> 1) CryptohomeLibrary provides a layer for switching production and fake, but
> CryptohomeClient provides the same. Dont' need two layers.
> 2) In many places, CryptohomeClient is directly used. Having CryptohomeClient,
> which only provides a small subset of functions, makes the code inconsistent.
> 
> FWIW, there used to be a number of FooLibrary classes, which have been mostly
> removed now. These were needed as a prod/fake switching layer when we were
> issuing D-Bus method calls via a dlopen'ed library called libcros, which was
> removed by hashimoto and me (crbug.com/127051)
> 
> 
> 
> > 2)
> > Why not just remove all wrappers except InstallAttributesGet and
> > InstallAttributesSet ? This are trivial wrappers which are easy to remove.
> Then
> > it would also be clearer what these calls depend on (namely
DBusThreadManager
> > and CryptohomeClient).
> 
> I kept these wrappers as removing them would make the client code verbose. For
> instance:
> 
>   if (cryptohome_->TpmIsEnabled() &&
>       !cryptohome_->TpmIsBeingOwned() &&
>       cryptohome_->TpmIsOwned()) {
> 
> I think we can remove wrappers for TpmIs* once we have TpmGetState()

That will not work anymore in any case, once the calls are asynchronous :-)

> 
> 
> I'll address the inline comments next week.
> 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> > File
> >
>
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> >
>
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:77:
> > // first with methods in CryptohomeLibrary.
> > stale comment?
> > 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> > File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right):
> > 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
> > chrome/browser/chromeos/policy/enterprise_install_attributes.h:21: class
> > CryptohomeLibrary;
> > unused
> > 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
> > File chrome/browser/policy/browser_policy_connector.cc (right):
> > 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
> > chrome/browser/policy/browser_policy_connector.cc:71: #include
> > "chromeos/cryptohome/cryptohome_library.h"
> > unused after the removal below
> > 
> >
>
https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
> > chrome/browser/policy/browser_policy_connector.cc:135: if
> > (chromeos::CryptohomeLibrary::IsInitialized() &&
> > this shouldn't be required anymore (update the comment too please)

[pneubeck (no reviews), 2013-09-27 11:55:35]

lgtm modulo the previous inline comments.

[hashimoto, 2013-09-30 07:53:46]

lgtm

[satorux1, 2013-10-01 05:24:03]

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(left):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:87:
chromeos::CryptohomeLibrary::SetForTest(cryptohome_library_.get());
This turned out to be necessary, as CryptohomeLibrary is used in
chromeos::DeviceOAuth2TokenService::SetAndSaveRefreshToken(). For now, added a
TODO comment.

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:77:
// first with methods in CryptohomeLibrary.
On 2013/09/27 08:59:25, pneubeck wrote:
> stale comment?

Turned out the comment was correct. Encryption was done using CrypthomoeLibrary
in DeviceOAuth2TokenService::SetAndSaveRefreshToken().

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/enterprise_install_attributes.h (right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/enterprise_install_attributes.h:21: class
CryptohomeLibrary;
On 2013/09/27 08:59:25, pneubeck wrote:
> unused

Good catch! Done.

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:135: if
(chromeos::CryptohomeLibrary::IsInitialized() &&
On 2013/09/27 08:59:25, pneubeck wrote:
> this shouldn't be required anymore (update the comment too please)

This turned out to be necessary, as removing it will cause tests to fail (the
code inside the if-block will be run if CryptohomeLibrary::IsInitialized() is
removed). For now, let me add a TODO here.

[pneubeck (no reviews), 2013-10-01 09:05:07]

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/24869003/diff/55001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:135: if
(chromeos::CryptohomeLibrary::IsInitialized() &&
On 2013/10/01 05:24:03, satorux1 wrote:
> On 2013/09/27 08:59:25, pneubeck wrote:
> > this shouldn't be required anymore (update the comment too please)
> 
> This turned out to be necessary, as removing it will cause tests to fail (the
> code inside the if-block will be run if CryptohomeLibrary::IsInitialized() is
> removed). For now, let me add a TODO here.

...how we love implicit dependencies...

[satorux1, 2013-10-02 01:04:28]

Committed patchset #4 manually as r226373 (presubmit successful).