cros: Remove user directory when encryption migration fails.

cros: Remove user directory when encryption migration fails.

When the migration UI is notified by cryptohome that cryptohome gets unexpected 
errors during migration and it gives up migration, we should remove the
user's cryptohome to make sure that the user will be able to log in to
the Desktop next time.

After the user's cryptohome is removed, we get password verification errors
on the existing user pod three times.
It seems an independent issue, so I filed crbug.com/715474 and will look into separately.

BUG=713556
TEST=manually tested by forcing cryptohome fail during migration and confirming the cryptohome is gone.

Review-Url: https://codereview.chromium.org/2838303003
Cr-Commit-Position: refs/heads/master@{#467608}
Committed: https://chromium.googlesource.com/chromium/src/+/1339ec6addf0faa53da2c7418a0de5b6803d89cf

[fukino, 2017-04-26 09:56:01]

Description was changed from

==========
cros: Remove user directory when encryption migration fails.

BUG=713556
==========

to

==========
cros: Remove user directory when encryption migration fails.

When the migration UI is notified by cryptohome that cryptohome gets unexpected 
errors during migration and it gives up migration, we should remove the
user's cryptohome to make sure that the user will be able to log in to
the Desktop next time.

After the user's cryptohome is removed, we get password verification errors
on the existing user pod three times.
It seems an independent issue, so I filed crbug.com/715474 and will look into
separately.

BUG=713556
TEST=manually tested by forcing cryptohome fail during migration and confirming
the cryptohome is gone.
==========

[fukino, 2017-04-26 09:57:44]

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

[fukino, 2017-04-26 09:57:44]

Xiyuan, could you take a look?

[fukino, 2017-04-26 09:58:09]

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

[commit-bot: I haz the power, 2017-04-26 09:58:16]

Dry run: 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-26 10:25:16]

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

[commit-bot: I haz the power, 2017-04-26 10:25:17]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-04-26 11:01:43]

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:317:
cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
Instead of calling AsyncRemove directly, we should be able to call
UserManager::RemoveUser() to avoid http://crbug.com/715474, but I'm concerned
about that UserManager::RemoveUser() can't remove owner profile.

Maybe we should remove cryptohome by AsyncRemove call and call
UserManager::RemoveUserList() to remove user pod?

[xiyuan, 2017-04-26 17:48:24]

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:317:
cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
On 2017/04/26 11:01:43, fukino wrote:
> Instead of calling AsyncRemove directly, we should be able to call
> UserManager::RemoveUser() to avoid http://crbug.com/715474, but I'm concerned
> about that UserManager::RemoveUser() can't remove owner profile.
> 
> Maybe we should remove cryptohome by AsyncRemove call and call
> UserManager::RemoveUserList() to remove user pod?

Agree that we should not use RemoveUser().

We can use 
  UserManager::Get()->SaveUserOAuthStatus(
        account_id, user_manager::User::OAUTH2_TOKEN_STATUS_INVALID);

to force user to go through Gaia on the next sign-in. And Gaia flow would
restore the token status on success.

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:327:
LOG(ERROR) << "Removing cryptohome failed. return code: " << return_code;
nit: LOG_IF(ERROR, !success) << ....

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:380:
RemoveCryptohome();
I think we should only RemoveCryptohome() only when migration really fails. Do
this on failed MigrateToDircrypto call feels too aggressive.

Maybe only do
  UpdateUIState(UIState::MIGRATION_FAILED);

[fukino, 2017-04-27 01:36:00]

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

[commit-bot: I haz the power, 2017-04-27 01:36:35]

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-27 01:38:23]

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:317:
cryptohome::AsyncMethodCaller::GetInstance()->AsyncRemove(
On 2017/04/26 17:48:24, xiyuan wrote:
> On 2017/04/26 11:01:43, fukino wrote:
> > Instead of calling AsyncRemove directly, we should be able to call
> > UserManager::RemoveUser() to avoid http://crbug.com/715474, but I'm
concerned
> > about that UserManager::RemoveUser() can't remove owner profile.
> > 
> > Maybe we should remove cryptohome by AsyncRemove call and call
> > UserManager::RemoveUserList() to remove user pod?
> 
> Agree that we should not use RemoveUser().
> 
> We can use 
>   UserManager::Get()->SaveUserOAuthStatus(
>         account_id, user_manager::User::OAUTH2_TOKEN_STATUS_INVALID);
> 
> to force user to go through Gaia on the next sign-in. And Gaia flow would
> restore the token status on success.

Thank you for the suggestion!
Updated the CL to invalidate token status, and confirmed that the user with
missing cryptohome can log in in the first attempt after the call.
Done.

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:327:
LOG(ERROR) << "Removing cryptohome failed. return code: " << return_code;
On 2017/04/26 17:48:24, xiyuan wrote:
> nit: LOG_IF(ERROR, !success) << ....

Done.

https://codereview.chromium.org/2838303003/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:380:
RemoveCryptohome();
On 2017/04/26 17:48:24, xiyuan wrote:
> I think we should only RemoveCryptohome() only when migration really fails. Do
> this on failed MigrateToDircrypto call feels too aggressive.
> 
> Maybe only do
>   UpdateUIState(UIState::MIGRATION_FAILED);

Done.

[commit-bot: I haz the power, 2017-04-27 02:10:01]

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

[commit-bot: I haz the power, 2017-04-27 02:10:02]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-04-27 02:50:42]

lgtm

https://codereview.chromium.org/2838303003/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2838303003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:318:
user_manager::UserManager::Get()->SaveUserOAuthStatus(
nit: Add a comment to document why we do this. Something along the lines:

// Set invalid token status so that user is forced to go through Gaia
// on the next sign-in.

[fukino, 2017-04-27 06:13:59]

Thank you!

https://codereview.chromium.org/2838303003/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2838303003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:318:
user_manager::UserManager::Get()->SaveUserOAuthStatus(
On 2017/04/27 02:50:42, xiyuan wrote:
> nit: Add a comment to document why we do this. Something along the lines:
> 
> // Set invalid token status so that user is forced to go through Gaia
> // on the next sign-in.

Done.

[fukino, 2017-04-27 06:14:06]

The CQ bit was checked by fukino@chromium.org

[fukino, 2017-04-27 06:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2838303003/#ps40001
(title: "Add a comment about token invalidation.")

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

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-27 06:48:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493273646224210,
"parent_rev": "5cdc120ce13ae37a6019159936d54920556f9b54", "commit_rev":
"1339ec6addf0faa53da2c7418a0de5b6803d89cf"}

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

Description was changed from

==========
cros: Remove user directory when encryption migration fails.

When the migration UI is notified by cryptohome that cryptohome gets unexpected 
errors during migration and it gives up migration, we should remove the
user's cryptohome to make sure that the user will be able to log in to
the Desktop next time.

After the user's cryptohome is removed, we get password verification errors
on the existing user pod three times.
It seems an independent issue, so I filed crbug.com/715474 and will look into
separately.

BUG=713556
TEST=manually tested by forcing cryptohome fail during migration and confirming
the cryptohome is gone.
==========

to

==========
cros: Remove user directory when encryption migration fails.

When the migration UI is notified by cryptohome that cryptohome gets unexpected 
errors during migration and it gives up migration, we should remove the
user's cryptohome to make sure that the user will be able to log in to
the Desktop next time.

After the user's cryptohome is removed, we get password verification errors
on the existing user pod three times.
It seems an independent issue, so I filed crbug.com/715474 and will look into
separately.

BUG=713556
TEST=manually tested by forcing cryptohome fail during migration and confirming
the cryptohome is gone.

Review-Url: https://codereview.chromium.org/2838303003
Cr-Commit-Position: refs/heads/master@{#467608}
Committed:
https://chromium.googlesource.com/chromium/src/+/1339ec6addf0faa53da2c7418a0d...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1339ec6addf0faa53da2c7418a0d...