Force encryption migration if the device supports ARC.

Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
  from ExistingUserController to CryptohomeAuthenticator.
  The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is available, ExistingUserController
  flips the flag to ensure that the cryptohome is encrypted in 
  dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
  the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
  let the user sign in by using previously-used UserContext and LoginPerformer,
  and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2798023005
Cr-Commit-Position: refs/heads/master@{#462773}
Committed: https://chromium.googlesource.com/chromium/src/+/1b5ebe9539b3bd7e074075a2de5a355944ba07e8

[fukino, 2017-04-05 15:42:55]

Description was changed from

==========
Force encryption migration if the device supports ARC.

BUG=706017
==========

to

==========
Force encryption migration if the device supports ARC.

BUG=706017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-04-05 15:53:40]

Description was changed from

==========
Force encryption migration if the device supports ARC.

BUG=706017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
from ExistingUserController to CryptohomeAuthenticator.
The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is enabled, ExistingUserController
flips the flag to ensure that the cryptohome is encrypted in dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
let the user sign in by using previously-used UserContext and LoginPerformer,
and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-04-05 15:54:17]

Description was changed from

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
from ExistingUserController to CryptohomeAuthenticator.
The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is enabled, ExistingUserController
flips the flag to ensure that the cryptohome is encrypted in dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
let the user sign in by using previously-used UserContext and LoginPerformer,
and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
  from ExistingUserController to CryptohomeAuthenticator.
  The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is enabled, ExistingUserController
  flips the flag to ensure that the cryptohome is encrypted in 
  dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
  the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
  let the user sign in by using previously-used UserContext and LoginPerformer,
  and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-04-05 15:55:03]

Description was changed from

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
  from ExistingUserController to CryptohomeAuthenticator.
  The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is enabled, ExistingUserController
  flips the flag to ensure that the cryptohome is encrypted in 
  dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
  the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
  let the user sign in by using previously-used UserContext and LoginPerformer,
  and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
  from ExistingUserController to CryptohomeAuthenticator.
  The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is available, ExistingUserController
  flips the flag to ensure that the cryptohome is encrypted in 
  dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
  the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
  let the user sign in by using previously-used UserContext and LoginPerformer,
  and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-04-05 15:59:56]

fukino@chromium.org changed reviewers:
+ xiyuan@chromium.org

[fukino, 2017-04-05 15:59:57]

Hi Xiyuan, could you take a look?

https://codereview.chromium.org/2798023005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:622:
migration_screen->SetLoginPerformer(login_performer_.get());
I'm not sure if we can keep adding state to the migration screen...

If there is a better way for the migration screen to let the user perform login,
could you guide me?

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
chromeos/login/auth/cryptohome_authenticator.cc:607:
user_context.IsForcingDircrypto());
If the cryptohome is ephemeral, IsForcingDircrypto does not make sense (it will
be just ignored).
I'm passing user_context.IsForcingDircrypto() here just for consistency.

[xiyuan, 2017-04-05 21:15:11]

https://codereview.chromium.org/2798023005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:622:
migration_screen->SetLoginPerformer(login_performer_.get());
On 2017/04/05 15:59:56, fukino wrote:
> I'm not sure if we can keep adding state to the migration screen...
> 
> If there is a better way for the migration screen to let the user perform
login,
> could you guide me?

I am fine with adding new state. But let's not passing |login_performer_| out
directly. Instead, wrap PerformLogin into a callback and pass that callback.

It might be better to add this to ExistingUserController so that LoginPerformer
does not need to worry about that. 

e.g.

void ExistingUserController::ContinuePerformLogin(const UserContext&
user_context, LoginPerformer::AuthorizationMode auth_mode) {
  login_performer_->PerformLogin(user_context, auth_mode);
}

and here we can do

  migration_screen->SetContinueLoginCallback(
    base::BindOnce(&ExistingUserController::ContinuePerformLogin,
                   weak_factory_.GetWeakPtr()));

And run this callback when we need to continue login.

https://codereview.chromium.org/2798023005/diff/1/chromeos/cryptohome/cryptoh...
File chromeos/cryptohome/cryptohome_parameters.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chromeos/cryptohome/cryptoh...
chromeos/cryptohome/cryptohome_parameters.cc:234: : ephemeral(ephemeral),
force_dircrypto_if_available(false) {}
nit: put initialization in header

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
chromeos/login/auth/cryptohome_authenticator.cc:153: bool
force_dircrypto_if_available) {
I don't think we need this extra arg. |attempt| holds a copy of UserContext and
we could get this from there.

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
chromeos/login/auth/cryptohome_authenticator.cc:607:
user_context.IsForcingDircrypto());
On 2017/04/05 15:59:56, fukino wrote:
> If the cryptohome is ephemeral, IsForcingDircrypto does not make sense (it
will
> be just ignored).
> I'm passing user_context.IsForcingDircrypto() here just for consistency.

IMHO, not making much sense to force dircrypto for ephemeral mount.

Anyway, my other comment is to remove this arg and use the UserContext in
AuthAttemptState.

[fukino, 2017-04-06 13:22:25]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 13:22:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fukino, 2017-04-06 14:22:17]

Thank you for the review and suggestions!
Could you take another look?

https://codereview.chromium.org/2798023005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:622:
migration_screen->SetLoginPerformer(login_performer_.get());
On 2017/04/05 21:15:11, xiyuan wrote:
> On 2017/04/05 15:59:56, fukino wrote:
> > I'm not sure if we can keep adding state to the migration screen...
> > 
> > If there is a better way for the migration screen to let the user perform
> login,
> > could you guide me?
> 
> I am fine with adding new state. But let's not passing |login_performer_| out
> directly. Instead, wrap PerformLogin into a callback and pass that callback.
> 
> It might be better to add this to ExistingUserController so that
LoginPerformer
> does not need to worry about that. 
> 
> e.g.
> 
> void ExistingUserController::ContinuePerformLogin(const UserContext&
> user_context, LoginPerformer::AuthorizationMode auth_mode) {
>   login_performer_->PerformLogin(user_context, auth_mode);
> }
> 
> and here we can do
> 
>   migration_screen->SetContinueLoginCallback(
>     base::BindOnce(&ExistingUserController::ContinuePerformLogin,
>                    weak_factory_.GetWeakPtr()));
> 
> And run this callback when we need to continue login.

Done.
I updated it to pass the callback to screen handler instead of LoginPerformer.

https://codereview.chromium.org/2798023005/diff/1/chromeos/cryptohome/cryptoh...
File chromeos/cryptohome/cryptohome_parameters.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chromeos/cryptohome/cryptoh...
chromeos/cryptohome/cryptohome_parameters.cc:234: : ephemeral(ephemeral),
force_dircrypto_if_available(false) {}
On 2017/04/05 21:15:11, xiyuan wrote:
> nit: put initialization in header

Done.

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
chromeos/login/auth/cryptohome_authenticator.cc:153: bool
force_dircrypto_if_available) {
On 2017/04/05 21:15:11, xiyuan wrote:
> I don't think we need this extra arg. |attempt| holds a copy of UserContext
and
> we could get this from there.

Done.
I didn't noticed that we can use |attempt|. The patch gets much simpler. Thank
you!

https://codereview.chromium.org/2798023005/diff/1/chromeos/login/auth/cryptoh...
chromeos/login/auth/cryptohome_authenticator.cc:607:
user_context.IsForcingDircrypto());
On 2017/04/05 21:15:11, xiyuan wrote:
> On 2017/04/05 15:59:56, fukino wrote:
> > If the cryptohome is ephemeral, IsForcingDircrypto does not make sense (it
> will
> > be just ignored).
> > I'm passing user_context.IsForcingDircrypto() here just for consistency.
> 
> IMHO, not making much sense to force dircrypto for ephemeral mount.
> 
> Anyway, my other comment is to remove this arg and use the UserContext in
> AuthAttemptState.

I updated the code to use the UserContext in AuthAttemptState and not to add the
arguments.

[commit-bot: I haz the power, 2017-04-06 14:27:05]

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

[commit-bot: I haz the power, 2017-04-06 14:27:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fukino, 2017-04-06 14:30:01]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 14:30:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fukino, 2017-04-06 14:31:18]

fukino@chromium.org changed reviewers:
+ isherman@chromium.org

[fukino, 2017-04-06 14:31:19]

isherman@: Could you take a look at histograms.xml?
Thanks!

[commit-bot: I haz the power, 2017-04-06 15:33:33]

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

[commit-bot: I haz the power, 2017-04-06 15:33:34]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-06 16:08:59]

lgtm

https://codereview.chromium.org/2798023005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/2798023005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.h:225:
LoginPerformer::AuthorizationMode auth_mode);
nit: maybe we should swap the order of args so that we can bind |auth_mode| on
creating the callback and avoid having to include login_performer.h.

i.e.

void ContinuePerformLogin(LoginPerformer::AuthorizationMode auth_mode,
                          const UserContext& user_context);

And bind the auth mode when creating the callback

base::BindOnce(&ExistingUserController::ContinuePerformLogin,
               weak_factory_.GetWeakPtr(),
               login_performer_->auth_mode());

And callback only needs a UserContext to be invoked.

[Ilya Sherman, 2017-04-06 22:06:09]

histograms.xml lgtm

[fukino, 2017-04-07 02:05:49]

Thank you for the suggestion!

https://codereview.chromium.org/2798023005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/2798023005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.h:225:
LoginPerformer::AuthorizationMode auth_mode);
On 2017/04/06 16:08:59, xiyuan wrote:
> nit: maybe we should swap the order of args so that we can bind |auth_mode| on
> creating the callback and avoid having to include login_performer.h.
> 
> i.e.
> 
> void ContinuePerformLogin(LoginPerformer::AuthorizationMode auth_mode,
>                           const UserContext& user_context);
> 
> And bind the auth mode when creating the callback
> 
> base::BindOnce(&ExistingUserController::ContinuePerformLogin,
>                weak_factory_.GetWeakPtr(),
>                login_performer_->auth_mode());
> 
> And callback only needs a UserContext to be invoked.

Done.

[fukino, 2017-04-07 02:05:54]

The CQ bit was checked by fukino@chromium.org

[fukino, 2017-04-07 02:05:55]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2798023005/#ps60001
(title: "Bind |auth_mode|")

[commit-bot: I haz the power, 2017-04-07 02:06:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 05:35:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491530754409390,
"parent_rev": "2baaba9891fafd293c619bdbcb70d2e083a76d86", "commit_rev":
"1b5ebe9539b3bd7e074075a2de5a355944ba07e8"}

[commit-bot: I haz the power, 2017-04-07 05:36:31]

Description was changed from

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
  from ExistingUserController to CryptohomeAuthenticator.
  The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is available, ExistingUserController
  flips the flag to ensure that the cryptohome is encrypted in 
  dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
  the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
  let the user sign in by using previously-used UserContext and LoginPerformer,
  and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Force encryption migration if the device supports ARC.

This CL includes following changes:

* Add |is_forcing_dircrypto| to UserContext to pass the configuration
  from ExistingUserController to CryptohomeAuthenticator.
  The flag affects MountParameter for MountEx API.
* If a regular user signs in and ARC is available, ExistingUserController
  flips the flag to ensure that the cryptohome is encrypted in 
  dircrypto. 
* Add a user-facing flag to enable the migration, since the backend of
  the migration procedure is not completely ready yet.
* If a user choose "SKIP" in the migration screen, the screen handler
  let the user sign in by using previously-used UserContext and LoginPerformer,
  and dripping the |is_forcing_dircrypto| flag.

BUG=706017
TEST=manually tested following cases on samus with ARC support.
1) Enable the flag in owner profile, create a cryptohome in ecryptfs, and sign
in to the new profile. It showed migration screen.
2) Clicking "SKIP" on the screen shows the user's desktop.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2798023005
Cr-Commit-Position: refs/heads/master@{#462773}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b5ebe9539b3bd7e074075a2de5a...
==========

[commit-bot: I haz the power, 2017-04-07 05:36:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1b5ebe9539b3bd7e074075a2de5a...