Switch CryptohomeAuthenticator from MountAsync() to MountEx()

Switch CryptohomeAuthenticator from MountAsync() to MountEx()

This CL switches CryptohomeAuthenticator's mount code from the old
MountAsync() method that takes a password to a MountEx() that takes a
Key for authentication.

BUG=367847
TEST=Updated unit tests

Committed: https://crrev.com/a0e5cd6e6e180eef15c6add9a89fc5f7b06a99ce
Cr-Commit-Position: refs/heads/master@{#292138}

[bartfab (slow), 2014-08-25 20:42:41]

Hi Denis,

Could you please review:

chromeos/login/auth/cryptohome_authenticator.cc 
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc

Hi Daniel,

Could you please review:

chromeos/cryptohome/cryptohome_parameters.*

[Daniel Erat, 2014-08-25 20:46:16]

I don't really know anything about cryptohome. Satoru, could you create a
chromeos/cryptohome/OWNERS file? Other chromeos/ subdirectories already have
this.

[satorux1, 2014-08-26 06:22:02]

Per git log, dkrahn seems to be most active in the directory. Let's make him the
owner. :)

[satorux1, 2014-08-26 06:22:33]

satorux@chromium.org changed reviewers:
+ dkrahn@google.com

[satorux1, 2014-08-26 06:22:33]

+dkrahn

[Denis Kuznetsov (DE-MUC), 2014-08-26 14:01:32]

lgtm with nit

https://chromiumcodereview.appspot.com/488633003/diff/20001/chromeos/login/au...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://chromiumcodereview.appspot.com/488633003/diff/20001/chromeos/login/au...
chromeos/login/auth/cryptohome_authenticator.cc:76: void
TriggerResolveWithHashWithLoginTimeMarker(
I think one With will be enough

[bartfab (slow), 2014-08-26 15:33:15]

https://codereview.chromium.org/488633003/diff/20001/chromeos/login/auth/cryp...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/488633003/diff/20001/chromeos/login/auth/cryp...
chromeos/login/auth/cryptohome_authenticator.cc:76: void
TriggerResolveWithHashWithLoginTimeMarker(
On 2014/08/26 14:01:32, Denis Kuznetsov wrote:
> I think one With will be enough

Done.

[Darren Krahn, 2014-08-26 17:59:47]

dkrahn@chromium.org changed reviewers:
+ dkrahn@chromium.org

[Darren Krahn, 2014-08-26 17:59:47]

lgtm

- Note: MountEx is subject to a 2 minute timeout, if you see failures (e.g. in
the test lab) that you didn't see before this might be the cause.

- I'm fine being the owner -- I think I'm already the owner of
chromeos/dbus/*cryptohome*

https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:565:
_));
optional: Multiple calls to MountEx should be avoided, is this something you
want to check with a Times(1)?

[bartfab (slow), 2014-08-26 18:08:47]

On 2014/08/26 17:59:47, Darren Krahn wrote:
> lgtm
> 
> - Note: MountEx is subject to a 2 minute timeout, if you see failures (e.g. in
> the test lab) that you didn't see before this might be the cause.

Is there a reason for this difference in behavior between AsyncMount() and
MountEx()? We need to make this switch to MountEx() to allow more complex
authentication methods. Should MountEx() not be a drop-in replacement for
AsyncMount(), deprecating the latter?

> 
> - I'm fine being the owner -- I think I'm already the owner of
> chromeos/dbus/*cryptohome*

I added an OWNER file to this CL and will ask for Daniel to stamp it.

> 
>
https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:565:
> _));
> optional: Multiple calls to MountEx should be avoided, is this something you
> want to check with a Times(1)?

[bartfab (slow), 2014-08-26 18:10:22]

Hi Daniel,

I am making Darren an owner of chromeos/cryptohome. Could you give your owner's
approval to this change?

https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:565:
_));
On 2014/08/26 17:59:47, Darren Krahn wrote:
> optional: Multiple calls to MountEx should be avoided, is this something you
> want to check with a Times(1)?

Done.

[Darren Krahn, 2014-08-26 18:47:05]

On 2014/08/26 18:08:47, bartfab wrote:
> On 2014/08/26 17:59:47, Darren Krahn wrote:
> > lgtm
> > 
> > - Note: MountEx is subject to a 2 minute timeout, if you see failures (e.g.
in
> > the test lab) that you didn't see before this might be the cause.
> 
> Is there a reason for this difference in behavior between AsyncMount() and
> MountEx()? We need to make this switch to MountEx() to allow more complex
> authentication methods. Should MountEx() not be a drop-in replacement for
> AsyncMount(), deprecating the latter?

The new method uses a 'normal' asynchronous dbus call. The old method used dbus
signals to achieve an asynchronous effect. It is one of our goals to deprecate
all methods that [ab]use signals this way, I just wanted you to be aware in case
it helps debug any wrinkles.

> 
> > 
> > - I'm fine being the owner -- I think I'm already the owner of
> > chromeos/dbus/*cryptohome*
> 
> I added an OWNER file to this CL and will ask for Daniel to stamp it.
> 
> > 
> >
>
https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
> > File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/488633003/diff/40001/chrome/browser/chromeos/...
> > chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:565:
> > _));
> > optional: Multiple calls to MountEx should be avoided, is this something you
> > want to check with a Times(1)?

[satorux1, 2014-08-27 00:51:34]

> The new method uses a 'normal' asynchronous dbus call. The old method used
dbus
> signals to achieve an asynchronous effect.

That's pretty cool! Glad to know that it's happening. LGTM based on dkrahn@

[bartfab (slow), 2014-08-27 10:04:10]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-27 10:05:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bartfab@chromium.org/488633003/60001

[commit-bot: I haz the power, 2014-08-27 10:17:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/46878)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/52235)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-27 10:19:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-27 10:19:06]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bartfab (slow), 2014-08-27 11:43:42]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-08-27 11:44:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bartfab@chromium.org/488633003/80001

[commit-bot: I haz the power, 2014-08-27 12:38:00]

Committed patchset #5 (80001) as 4c894f1a28c25a1d1b2a15d5106b5419674503ee

[commit-bot: I haz the power, 2014-09-10 02:51:14]

Patchset 5 (id:??) landed as
https://crrev.com/a0e5cd6e6e180eef15c6add9a89fc5f7b06a99ce
Cr-Commit-Position: refs/heads/master@{#292138}