ChromeOS cryptohome should be able to use gaia id as user identifier.

ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier.

BUG=462823
TEST=unit tests

Committed: https://crrev.com/627f65303a4a1a6b4d1beb62c9ea5b6ed5affda7
Cr-Commit-Position: refs/heads/master@{#379262}

[Alexander Alekseev, 2016-02-16 12:17:49]

alemate@chromium.org changed reviewers:
+ derat@chromium.org, dkrahn@chromium.org

[Alexander Alekseev, 2016-02-16 12:17:50]

Please review.

[Alexander Alekseev, 2016-02-16 12:30:54]

On 2016/02/16 12:17:50, Alexander Alekseev wrote:
> Please review.

Please review:

dkrahn@:
    c/b/c/attestation
    c/b/extensions/api/enterprise_platform_keys_private
    chromeos/attestation
    chromeos/cryptohome
    chromeos/dbus

derat@:
    c/b/c
    c/b/c/app_mode
    c/b/c/login
    c/b/c/login/auth
    c/b/c/login/easy_unlock
    c/b/c/login/session
    c/b/c/login/signin
    c/b/c/login/supervised
    c/b/c/login/users
    c/b/c/login/users/wallpaper
    c/b/c/policy
    chromeos/
    chromeos/dbus/session_manager*
    chromeos/login/auth
    chromeos/tpm

[Daniel Erat, 2016-02-16 18:27:07]

derat@chromium.org changed reviewers:
+ mnissler@chromium.org

[Daniel Erat, 2016-02-16 18:27:08]

Adding more people as a heads-up.

[Darren Krahn, 2016-02-17 20:48:04]

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

[Darren Krahn, 2016-02-17 20:48:05]

+xiyuan for login stuff

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:857:
cryptohome::Identification(user->GetAccountId()).id(),
Moving cryptohome::Identification to chromeos/dbus would allow this to work
without the .id(), correct?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:177:
cryptohome::Identification(EmptyAccountId()).id(),  // Not used.
Would a default constructor for cryptohome::Identification make sense?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:120:
const base::Callback<void(PrepareKeyResult)>& callback);
Just a heads up -- this will likely conflict with
https://chromiumcodereview.appspot.com/1685103003/.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/dbus/cryptohom...
File chromeos/dbus/cryptohome_client.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/dbus/cryptohom...
chromeos/dbus/cryptohome_client.h:252: // The |user_id| must be a valid
cryptohome user id.
Please clarify what is a 'valid cryptohome user id'. Maybe add 'as created by
cryptohome::Identification'? (more below)

[xiyuan, 2016-02-17 23:14:26]

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:70:
dict_update->SetStringWithoutPathExpansion(account_id.Serialize(), app_id);
The comment for Serialize() in account_id.h says:

  // These are (for now) unstable and cannot be used to store serialized data to
  // persistent storage. Only in-memory storage is safe.

Should the comments be updated? Or we should not use it as the serialized data
would be persisted here?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:641: const AccountId
account_id(AccountId::FromUserEmail(it->user_id));
Note it->user_id could be an arbitrary string set by the admin and not
necessarily a valid email. And there will never be a gaia id for it.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/app_launch_controller.cc:164: new
KioskProfileLoader(app.account_id.GetUserEmail(), false, this));
Update KioskProfileLoader to take AccountId as well?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.h:436: typedef
std::map<AccountId, std::string> PendingUserSessions;
nit: new preferred the style is 

using PendingUserSessions = std::map<AccountId, std::string>;

See https://chromium-cpp.appspot.com/

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:159: return false;  // Force
disable update.
debugging code?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:536:
account_id_.GetUserEmail(),
Should we use cryptohome::Identification(account_id_).id() instead since we were
using that for cryptohome_client_->BlockingGetSanitizedUsername(...) around line
250?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:128: const
AccountId& account_id_;
Is it intentional to store a ref here? Are we sure that the referenced AccountId
object would outlive this class?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:151:
signin_manager->GetAuthenticatedAccountInfo();
Not introduced by this CL but one edge case is that this would return an empty
AccountInfo when the profile does not have valid tokens (e.g. user password has
changed). Not sure what is the proper way to handle it though. Maybe return
EmptyAccountId() explicitly for such case? Or get the account id from user via
ProfileHelper::GetUserByProfile(GetProfile())?

https://codereview.chromium.org/1693383003/diff/20001/chromeos/attestation/mo...
File chromeos/attestation/mock_attestation_flow.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/attestation/mo...
chromeos/attestation/mock_attestation_flow.cc:8: #include
"components/signin/core/account_id/account_id.h"
not used?

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
File chromeos/cryptohome/cryptohome_parameters.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.cc:18: // Guest account has empty
GaiaId. Default to email.
Think this is true all non-Gaia accounts in addition to Guest, e.g. kiosk apps,
public session etc.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.h:45: };
DISALLOW_COPY_AND_ASSIGN() ?

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/hom...
File chromeos/cryptohome/homedir_methods_unittest.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/hom...
chromeos/cryptohome/homedir_methods_unittest.cc:125:
cryptohome::Identification(AccountId::FromUserEmail(kUserID)).id());
nit: remove "cryptohome::" since it is already in namespace cryptohome.

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

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/cry...
chromeos/login/auth/cryptohome_authenticator.cc:136: << ")";
What happens if we run into this? User sees a login error? Or we create a new
cryptohome and leave the old one around? None of these is ideal. I wonder
whether it makes sense to also have a error_callback to mount using the old id
so that we don't lock user out on the failure case.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/cry...
chromeos/login/auth/cryptohome_authenticator.cc:189:
attempt->user_context.GetAccountId().GetGaiaIdKey();
This looks like for the existing users. How can we be sure that we already have
gaiaId for those users?

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/cry...
chromeos/login/auth/cryptohome_authenticator.cc:533: :
AccountId::FromUserEmail(login::CanonicalizeUserID(app_user_id));
We should not call login::CanonicalizeUserID for kiosk app since |app_user_id|
is not necessarily an email. Since we already use the value as is, we should not
change it. Otherwise, existing kiosk app would be switched to new cryptohome and
lose all its local data.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
File chromeos/login/auth/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
chromeos/login/auth/utils.cc:7: #include "base/logging.h"
not used?

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
File chromeos/login/auth/utils.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
chromeos/login/auth/utils.h:13: // Lookup AccountId matching givem
Identification.
givem -> the given?

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
chromeos/login/auth/utils.h:14: class AccountId GetAccountId(const
Identification& user_id);
remove "class"?

https://codereview.chromium.org/1693383003/diff/20001/components/signin/core/...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/components/signin/core/...
components/signin/core/account_id/account_id.cc:53:
CHECK(!base::StartsWith(user_email, "g:", base::CompareCase::SENSITIVE))
nit: LOG_IF(FATAL, ...) or LOG_ASSERT(...)

Otherwise, the output log lines would not show up in chrome log.

[Alexander Alekseev, 2016-02-18 13:45:15]

Cryptohome and session_manager sources in chromeos/dbus/* are now using
cryptohome::Identification as argument. After this change I found a lot of other
tests I missed when creating the first version of this CL.

cryptohome::Identification has default constructor.

I rewrote cryptohome::Identification(AccountId) code to make sure only migrated
users will use GaiaId as cryptohome identifier.

PTAL.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:857:
cryptohome::Identification(user->GetAccountId()).id(),
On 2016/02/17 20:48:05, Darren Krahn wrote:
> Moving cryptohome::Identification to chromeos/dbus would allow this to work
> without the .id(), correct?

Yes.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:70:
dict_update->SetStringWithoutPathExpansion(account_id.Serialize(), app_id);
On 2016/02/17 23:14:25, xiyuan wrote:
> The comment for Serialize() in account_id.h says:
> 
>   // These are (for now) unstable and cannot be used to store serialized data
to
>   // persistent storage. Only in-memory storage is safe.
> 
> Should the comments be updated? Or we should not use it as the serialized data
> would be persisted here?

You're right. We should not use it here. I've replaced it with
cryptohome::Identification.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:641: const AccountId
account_id(AccountId::FromUserEmail(it->user_id));
On 2016/02/17 23:14:25, xiyuan wrote:
> Note it->user_id could be an arbitrary string set by the admin and not
> necessarily a valid email. And there will never be a gaia id for it.

It cannot be completely arbitrary, because we use it as cryptohome identifier.
At least session_manager has checks for this string to look like regular email.

As a part of migration to AccountId structure, we are replacing all strings that
are used "as user email" with AccountId::FromUserEmail(). So we have many
account ids with empty GaiaId. And this is why AccountId doesn't check this
string to be a valid e-mail.

The plan is to do further refactoring and introduce different account types (as
we have AccountId::GetAccountType) for regular/guest/kiosk/etc... But for now we
are moving away from std::string as user identifier everywhere.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:177:
cryptohome::Identification(EmptyAccountId()).id(),  // Not used.
On 2016/02/17 20:48:05, Darren Krahn wrote:
> Would a default constructor for cryptohome::Identification make sense?

Hmm. Actually only tests would benefit from this. Do you think we should add it
for tests?

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/app_launch_controller.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/app_launch_controller.cc:164: new
KioskProfileLoader(app.account_id.GetUserEmail(), false, this));
On 2016/02/17 23:14:25, xiyuan wrote:
> Update KioskProfileLoader to take AccountId as well?

Done.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.h:436: typedef
std::map<AccountId, std::string> PendingUserSessions;
On 2016/02/17 23:14:25, xiyuan wrote:
> nit: new preferred the style is 
> 
> using PendingUserSessions = std::map<AccountId, std::string>;
> 
> See https://chromium-cpp.appspot.com/

Done.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/wizard_controller.cc:159: return false;  // Force
disable update.
On 2016/02/17 23:14:25, xiyuan wrote:
> debugging code?

Yes. this was for debugging only. Removed.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:536:
account_id_.GetUserEmail(),
On 2016/02/17 23:14:25, xiyuan wrote:
> Should we use cryptohome::Identification(account_id_).id() instead since we
were
> using that for cryptohome_client_->BlockingGetSanitizedUsername(...) around
line
> 250?

you are right.
This is why I was talking about using cryptohome::Identification in
chromeos/dbus. I missed this string.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:128: const
AccountId& account_id_;
On 2016/02/17 23:14:25, xiyuan wrote:
> Is it intentional to store a ref here? Are we sure that the referenced
AccountId
> object would outlive this class?

This was "automatic substitution error". Fixed.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:151:
signin_manager->GetAuthenticatedAccountInfo();
On 2016/02/17 23:14:25, xiyuan wrote:
> Not introduced by this CL but one edge case is that this would return an empty
> AccountInfo when the profile does not have valid tokens (e.g. user password
has
> changed). Not sure what is the proper way to handle it though. Maybe return
> EmptyAccountId() explicitly for such case? Or get the account id from user via
> ProfileHelper::GetUserByProfile(GetProfile())?

Done.

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:120:
const base::Callback<void(PrepareKeyResult)>& callback);
On 2016/02/17 20:48:05, Darren Krahn wrote:
> Just a heads up -- this will likely conflict with
>
https://chromiumcodereview.appspot.com/1685103003/?_ga=1.68965585.1130779885.....

Thank you for pointing out. I'll look at it.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/attestation/mo...
File chromeos/attestation/mock_attestation_flow.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/attestation/mo...
chromeos/attestation/mock_attestation_flow.cc:8: #include
"components/signin/core/account_id/account_id.h"
On 2016/02/17 23:14:25, xiyuan wrote:
> not used?

Done.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
File chromeos/cryptohome/cryptohome_parameters.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.cc:18: // Guest account has empty
GaiaId. Default to email.
On 2016/02/17 23:14:25, xiyuan wrote:
> Think this is true all non-Gaia accounts in addition to Guest, e.g. kiosk
apps,
> public session etc.

Done.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.h:45: };
On 2016/02/17 23:14:26, xiyuan wrote:
> DISALLOW_COPY_AND_ASSIGN() ?

It is copied and used in PostTask in several places around the code.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/hom...
File chromeos/cryptohome/homedir_methods_unittest.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/cryptohome/hom...
chromeos/cryptohome/homedir_methods_unittest.cc:125:
cryptohome::Identification(AccountId::FromUserEmail(kUserID)).id());
On 2016/02/17 23:14:26, xiyuan wrote:
> nit: remove "cryptohome::" since it is already in namespace cryptohome.

Done.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/dbus/cryptohom...
File chromeos/dbus/cryptohome_client.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/dbus/cryptohom...
chromeos/dbus/cryptohome_client.h:252: // The |user_id| must be a valid
cryptohome user id.
On 2016/02/17 20:48:05, Darren Krahn wrote:
> Please clarify what is a 'valid cryptohome user id'. Maybe add 'as created by
> cryptohome::Identification'? (more below)

This is now replaced with cryptohome::Identification itself.

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

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/cry...
chromeos/login/auth/cryptohome_authenticator.cc:136: << ")";
On 2016/02/17 23:14:26, xiyuan wrote:
> What happens if we run into this? User sees a login error? Or we create a new
> cryptohome and leave the old one around? None of these is ideal. I wonder
> whether it makes sense to also have a error_callback to mount using the old id
> so that we don't lock user out on the failure case.

You're right. I've updated cryprohome::Identification.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/cry...
chromeos/login/auth/cryptohome_authenticator.cc:189:
attempt->user_context.GetAccountId().GetGaiaIdKey();
On 2016/02/17 23:14:26, xiyuan wrote:
> This looks like for the existing users. How can we be sure that we already
have
> gaiaId for those users?

You're right. I will also split migration from mount.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/cry...
chromeos/login/auth/cryptohome_authenticator.cc:533: :
AccountId::FromUserEmail(login::CanonicalizeUserID(app_user_id));
On 2016/02/17 23:14:26, xiyuan wrote:
> We should not call login::CanonicalizeUserID for kiosk app since |app_user_id|
> is not necessarily an email. Since we already use the value as is, we should
not
> change it. Otherwise, existing kiosk app would be switched to new cryptohome
and
> lose all its local data.

This has always happened in UserContext constructor (see
chromeos/login/auth/user_context.cc below). As now UserContext requires
AccountId, I moved this transformation here.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
File chromeos/login/auth/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
chromeos/login/auth/utils.cc:7: #include "base/logging.h"
On 2016/02/17 23:14:26, xiyuan wrote:
> not used?

Done.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
File chromeos/login/auth/utils.h (right):

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
chromeos/login/auth/utils.h:13: // Lookup AccountId matching givem
Identification.
On 2016/02/17 23:14:26, xiyuan wrote:
> givem -> the given?

Done.

https://codereview.chromium.org/1693383003/diff/20001/chromeos/login/auth/uti...
chromeos/login/auth/utils.h:14: class AccountId GetAccountId(const
Identification& user_id);
On 2016/02/17 23:14:26, xiyuan wrote:
> remove "class"?

Done.

https://codereview.chromium.org/1693383003/diff/20001/components/signin/core/...
File components/signin/core/account_id/account_id.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/components/signin/core/...
components/signin/core/account_id/account_id.cc:53:
CHECK(!base::StartsWith(user_email, "g:", base::CompareCase::SENSITIVE))
On 2016/02/17 23:14:26, xiyuan wrote:
> nit: LOG_IF(FATAL, ...) or LOG_ASSERT(...)
> 
> Otherwise, the output log lines would not show up in chrome log.

Done.

[xiyuan, 2016-02-18 17:22:33]

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:641: const AccountId
account_id(AccountId::FromUserEmail(it->user_id));
On 2016/02/18 13:45:14, Alexander Alekseev wrote:
> On 2016/02/17 23:14:25, xiyuan wrote:
> > Note it->user_id could be an arbitrary string set by the admin and not
> > necessarily a valid email. And there will never be a gaia id for it.
> 
> It cannot be completely arbitrary, because we use it as cryptohome identifier.
> At least session_manager has checks for this string to look like regular
email.
> 
> As a part of migration to AccountId structure, we are replacing all strings
that
> are used "as user email" with AccountId::FromUserEmail(). So we have many
> account ids with empty GaiaId. And this is why AccountId doesn't check this
> string to be a valid e-mail.
> 
> The plan is to do further refactoring and introduce different account types
(as
> we have AccountId::GetAccountType) for regular/guest/kiosk/etc... But for now
we
> are moving away from std::string as user identifier everywhere.

I am not objecting to the change. Just want to point out that the AccountId for
kiosk is a bit different.

https://codereview.chromium.org/1693383003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1693383003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:643:
AccountId::FromUserEmail(login::CanonicalizeUserID(it->user_id)));
We should use the |it->user_id| as is. There are existing kiosk apps there that
have local data. Change it would cause those apps to lose their local data and
would be costly and painful to recover. Because of this, the
login::CanonicalizeUserID() should not be used.

https://codereview.chromium.org/1693383003/diff/80001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/1693383003/diff/80001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:208: 'dbus/utils.h',
Where are those files? I only see login/auth/utils.*

[Alexander Alekseev, 2016-02-19 00:17:04]

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1693383003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:641: const AccountId
account_id(AccountId::FromUserEmail(it->user_id));
On 2016/02/18 17:22:33, xiyuan wrote:
> On 2016/02/18 13:45:14, Alexander Alekseev wrote:
> > On 2016/02/17 23:14:25, xiyuan wrote:
> > > Note it->user_id could be an arbitrary string set by the admin and not
> > > necessarily a valid email. And there will never be a gaia id for it.
> > 
> > It cannot be completely arbitrary, because we use it as cryptohome
identifier.
> > At least session_manager has checks for this string to look like regular
> email.
> > 
> > As a part of migration to AccountId structure, we are replacing all strings
> that
> > are used "as user email" with AccountId::FromUserEmail(). So we have many
> > account ids with empty GaiaId. And this is why AccountId doesn't check this
> > string to be a valid e-mail.
> > 
> > The plan is to do further refactoring and introduce different account types
> (as
> > we have AccountId::GetAccountType) for regular/guest/kiosk/etc... But for
now
> we
> > are moving away from std::string as user identifier everywhere.
> 
> I am not objecting to the change. Just want to point out that the AccountId
for
> kiosk is a bit different.

You're right. But it still looks like email. It cannot be canonicalized though.

https://codereview.chromium.org/1693383003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1693383003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:643:
AccountId::FromUserEmail(login::CanonicalizeUserID(it->user_id)));
On 2016/02/18 17:22:33, xiyuan wrote:
> We should use the |it->user_id| as is. There are existing kiosk apps there
that
> have local data. Change it would cause those apps to lose their local data and
> would be costly and painful to recover. Because of this, the
> login::CanonicalizeUserID() should not be used.

You are right. I missed this.

Fixed.

https://codereview.chromium.org/1693383003/diff/80001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/1693383003/diff/80001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:208: 'dbus/utils.h',
On 2016/02/18 17:22:33, xiyuan wrote:
> Where are those files? I only see login/auth/utils.*

I missed them. Now added.

[xiyuan, 2016-02-19 00:19:58]

LGTM but please wait for other reviewers as well.

[Darren Krahn, 2016-02-19 17:29:41]

On 2016/02/18 13:45:15, Alexander Alekseev wrote:
> On 2016/02/17 20:48:05, Darren Krahn wrote:
> > Would a default constructor for cryptohome::Identification make sense?
> 
> Hmm. Actually only tests would benefit from this. Do you think we should add
it
> for tests?

Either way is acceptable, it's up to you...

LGTM for chromeos/attestation, enterprise_platform_keys, and *cryptohome*

[Alexander Alekseev, 2016-02-20 05:22:52]

alemate@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Alexander Alekseev, 2016-02-20 05:22:54]

+dzhioev for:
   chromeos/login/
   components/user_manager/

[dzhioev (left Google), 2016-02-20 07:15:13]

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:186: const bool
is_gaiaid_migration_started = switches::IsGaiaIdMigrationStarted();
Let's put:

if (switches::IsGaiaIdMigrationStarted()) {
  DoMount(...);
  return;
}

at the beginning of the function and get rid of |is_gaiaid_migration_started|.

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:193: if
(is_gaiaid_migration_started && !create_if_nonexistent &&
Why do you check |create_if_nonexistent| here?

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:241:
MigrateCryptohomeToGaiaId(attempt, resolver, ephemeral,
MigrateCryptohomeToGaiaId name is confusing, because migration is not the
function main purpose. Maybe rename to MaybeMigrateToGaiaIdAndMount?

https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
File components/user_manager/known_user.cc (right):

https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
components/user_manager/known_user.cc:275: if (has_email || has_gaia_id) {
No need for {}

https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
components/user_manager/known_user.cc:275: if (has_email || has_gaia_id) {
when this condition is false?

[Alexander Alekseev, 2016-02-20 08:07:11]

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:186: const bool
is_gaiaid_migration_started = switches::IsGaiaIdMigrationStarted();
On 2016/02/20 07:15:13, dzhioev wrote:
> Let's put:
> 
> if (switches::IsGaiaIdMigrationStarted()) {
>   DoMount(...);
>   return;
> }
> 
> at the beginning of the function and get rid of |is_gaiaid_migration_started|.

Done.

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:193: if
(is_gaiaid_migration_started && !create_if_nonexistent &&
On 2016/02/20 07:15:13, dzhioev wrote:
> Why do you check |create_if_nonexistent| here?

create_if_nonexistent is set for GuestUser and new users only.
So this prevents attempts to rename non-existing cryptohomes.

https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:241:
MigrateCryptohomeToGaiaId(attempt, resolver, ephemeral,
On 2016/02/20 07:15:13, dzhioev wrote:
> MigrateCryptohomeToGaiaId name is confusing, because migration is not the
> function main purpose. Maybe rename to MaybeMigrateToGaiaIdAndMount?

Done.

https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
File components/user_manager/known_user.cc (right):

https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
components/user_manager/known_user.cc:275: if (has_email || has_gaia_id) {
On 2016/02/20 07:15:13, dzhioev wrote:
> when this condition is false? 

When data is corrupt.

https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
components/user_manager/known_user.cc:275: if (has_email || has_gaia_id) {
On 2016/02/20 07:15:13, dzhioev wrote:
> No need for {}

Done.

[dzhioev (left Google), 2016-02-23 00:36:41]

On 2016/02/20 08:07:11, Alexander Alekseev wrote:
>
https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
> File chromeos/login/auth/cryptohome_authenticator.cc (right):
> 
>
https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
> chromeos/login/auth/cryptohome_authenticator.cc:186: const bool
> is_gaiaid_migration_started = switches::IsGaiaIdMigrationStarted();
> On 2016/02/20 07:15:13, dzhioev wrote:
> > Let's put:
> > 
> > if (switches::IsGaiaIdMigrationStarted()) {
> >   DoMount(...);
> >   return;
> > }
> > 
> > at the beginning of the function and get rid of
|is_gaiaid_migration_started|.
> 
> Done.
> 
>
https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
> chromeos/login/auth/cryptohome_authenticator.cc:193: if
> (is_gaiaid_migration_started && !create_if_nonexistent &&
> On 2016/02/20 07:15:13, dzhioev wrote:
> > Why do you check |create_if_nonexistent| here?
> 
> create_if_nonexistent is set for GuestUser and new users only.
> So this prevents attempts to rename non-existing cryptohomes.
> 
>
https://codereview.chromium.org/1693383003/diff/100001/chromeos/login/auth/cr...
> chromeos/login/auth/cryptohome_authenticator.cc:241:
> MigrateCryptohomeToGaiaId(attempt, resolver, ephemeral,
> On 2016/02/20 07:15:13, dzhioev wrote:
> > MigrateCryptohomeToGaiaId name is confusing, because migration is not the
> > function main purpose. Maybe rename to MaybeMigrateToGaiaIdAndMount?
> 
> Done.
> 
>
https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
> File components/user_manager/known_user.cc (right):
> 
>
https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
> components/user_manager/known_user.cc:275: if (has_email || has_gaia_id) {
> On 2016/02/20 07:15:13, dzhioev wrote:
> > when this condition is false? 
> 
> When data is corrupt.
> 
>
https://codereview.chromium.org/1693383003/diff/100001/components/user_manage...
> components/user_manager/known_user.cc:275: if (has_email || has_gaia_id) {
> On 2016/02/20 07:15:13, dzhioev wrote:
> > No need for {}
> 
> Done.

LGTM

[Alexander Alekseev, 2016-02-23 07:18:23]

derat@ , what do you think about :
    chromeos/BUILD.gn
    chromeos/DEPS
    chromeos/chromeos.gyp
    chromeos/dbus/fake_session_manager_client.cc
    chromeos/dbus/fake_session_manager_client.h
    chromeos/dbus/mock_session_manager_client.h
    chromeos/dbus/session_manager_client.cc
    chromeos/dbus/session_manager_client.h
    chromeos/dbus/utils.cc
    chromeos/dbus/utils.h
    chromeos/tpm/tpm_token_info_getter.cc
    chromeos/tpm/tpm_token_info_getter.h
    chromeos/tpm/tpm_token_info_getter_unittest.cc

[Alexander Alekseev, 2016-02-23 09:06:59]

alemate@chromium.org changed reviewers:
+ rogerta@chromium.org

[Alexander Alekseev, 2016-02-23 09:07:00]

+rogerta@ : Please review:
  components/signin/core/account_id/account_id.cc
  chromeos/DEPS (as owner of components/signin/core/account_id/ )

[Roger Tawa OOO till Jul 10th, 2016-02-23 15:44:21]

lgtm account_id.cc and DEPS

[Daniel Erat, 2016-02-23 19:00:47]

derat@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Daniel Erat, 2016-02-23 19:00:49]

rubberstamp lgtm for ownerless chromeos/tpm/ directory.

i'm not an owner of chromeos/dbus/, though. adding stevenjb@ to review both
chromeos/dbus/ and the dependency changes in the toplevel chromeos/ files.

[stevenjb, 2016-02-23 19:27:34]

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
chromeos/dbus/cryptohome_client.cc:144: }
Are these changes codependent, or can we separate this addition from the
otherwise mechanical cryptohome_id change?

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.cc
File chromeos/dbus/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.cc...
chromeos/dbus/utils.cc:14: id_proto->set_account_id(id.id());
Is there a reason this can't be a member of cryptohome::AccountIdentifier?

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.h
File chromeos/dbus/utils.h (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.h#...
chromeos/dbus/utils.h:17: cryptohome::AccountIdentifier* id_proto);
Can we name this file dbus_utils.h and namespace this chromeos::dbus_utils?
Searching the code for just 'utils.h' is problematic and the chromeos::
namespace is huge.
(Or see other suggestion)

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:228: }
Can these be added separately?

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
File chromeos/login/auth/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
chromeos/login/auth/utils.cc:24: }
This is a strange place to put something in the cryptohome namespace. Also both
'utils.cc' and 'GetAccountId' are too generic.

It looks like this should really be:

user_manager::known_user::GetKnownIdFromUserId(user_id.id())

i.e. I don't see any reason why we should pass cryptohome::Identification to the
helper instead of cryptohome::Identification.id()

[Alexander Alekseev, 2016-02-24 01:52:57]

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
File chromeos/login/auth/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
chromeos/login/auth/utils.cc:24: }
On 2016/02/23 19:27:34, stevenjb wrote:
> This is a strange place to put something in the cryptohome namespace. Also
both
> 'utils.cc' and 'GetAccountId' are too generic.
> 
> It looks like this should really be:
> 
> user_manager::known_user::GetKnownIdFromUserId(user_id.id())
> 
> i.e. I don't see any reason why we should pass cryptohome::Identification to
the
> helper instead of cryptohome::Identification.id()

First of all, one of the important steps in this CL was moving away from
std::string when passing cryptohome id to chromeos/dbus/* . as you could see,
the first version of this CL didn't have this change. And I found a lot of code
I missed when incorrect ID stopped compiling. So this is done to prevent errors
in coding.

I needed some structure that could represent user ID in dbus/. It also should be
AccounId-constructible (which requires access to IsGaiaIdMigrationStarted , i.e.
chromeos internals ). So the cryptohome::Identifier suits here very well.

Reverse translation requires access to chromeos and user_manager. But
UserManager must not depend on ChromeOS, so I put the translation here.

Why is it strange to put this tranlation here as there is a lot of code in
chromeos/login/auth that belongs to cryptohome?

[Alexander Alekseev, 2016-02-24 09:04:31]

Description was changed from

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier,
including support for renaming existing cryptohomes to new id
(RenameCreptohome dbus call).

BUG=462823
TEST=unit tests
==========

to

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier,
including support for renaming existing cryptohomes to new id
(RenameCreptohome dbus call).

BUG=462823
TEST=unit tests
==========

[Alexander Alekseev, 2016-02-24 09:04:31]

alemate@chromium.org changed reviewers:
- mnissler@chromium.org

[Alexander Alekseev, 2016-02-24 09:27:42]

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
chromeos/dbus/cryptohome_client.cc:144: }
On 2016/02/23 19:27:34, stevenjb wrote:
> Are these changes codependent, or can we separate this addition from the
> otherwise mechanical cryptohome_id change?

This CL implements cryptohome migration, and I have verified on a physical
device that it works.

What changes do you mean?
Do you mean you would prefer splitting this CL? This is going to be tricky
because I have changed only 2 or 3 API. All the other changes depend on these
API.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.cc
File chromeos/dbus/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.cc...
chromeos/dbus/utils.cc:14: id_proto->set_account_id(id.id());
On 2016/02/23 19:27:34, stevenjb wrote:
> Is there a reason this can't be a member of cryptohome::AccountIdentifier?

What do you mean?
AccountIdentifier is autogenerated from protobuf.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.h
File chromeos/dbus/utils.h (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.h#...
chromeos/dbus/utils.h:17: cryptohome::AccountIdentifier* id_proto);
On 2016/02/23 19:27:34, stevenjb wrote:
> Can we name this file dbus_utils.h and namespace this chromeos::dbus_utils?
> Searching the code for just 'utils.h' is problematic and the chromeos::
> namespace is huge.
> (Or see other suggestion)

This function is now used in a single source file only, so I moved it there.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:228: }
On 2016/02/23 19:27:34, stevenjb wrote:
> Can these be added separately?

What do you mean?

[stevenjb, 2016-02-24 17:25:44]

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
chromeos/dbus/cryptohome_client.cc:144: }
On 2016/02/24 09:27:42, Alexander Alekseev wrote:
> On 2016/02/23 19:27:34, stevenjb wrote:
> > Are these changes codependent, or can we separate this addition from the
> > otherwise mechanical cryptohome_id change?
> 
> This CL implements cryptohome migration, and I have verified on a physical
> device that it works.
> 
> What changes do you mean?
> Do you mean you would prefer splitting this CL? This is going to be tricky
> because I have changed only 2 or 3 API. All the other changes depend on these
> API.

I mean specifically adding a new method to the API. Please add RenameCryptohome
in a separate CL.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.cc
File chromeos/dbus/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/utils.cc...
chromeos/dbus/utils.cc:14: id_proto->set_account_id(id.id());
On 2016/02/24 09:27:42, Alexander Alekseev wrote:
> On 2016/02/23 19:27:34, stevenjb wrote:
> > Is there a reason this can't be a member of cryptohome::AccountIdentifier?
> 
> What do you mean?
> AccountIdentifier is autogenerated from protobuf.

Ah, I see. It is too bad that isn't namespaced so that we can more easily see
that.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:228: }
On 2016/02/24 09:27:42, Alexander Alekseev wrote:
> On 2016/02/23 19:27:34, stevenjb wrote:
> > Can these be added separately?
> 
> What do you mean?

I was hoping it would be reasonable to add EnsureCryptohomeMigratedToGaiaId in a
separate CL (along with RenameCryptohome). With a CL this large if there are any
bugs in it (which may not show up until it gets to the PFQ), it will be very
difficult to revert it down the line. Breaking this up, even a little, would
help a lot.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
File chromeos/login/auth/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
chromeos/login/auth/utils.cc:24: }
On 2016/02/24 01:52:57, Alexander Alekseev wrote:
> On 2016/02/23 19:27:34, stevenjb wrote:
> > This is a strange place to put something in the cryptohome namespace. Also
> both
> > 'utils.cc' and 'GetAccountId' are too generic.
> > 
> > It looks like this should really be:
> > 
> > user_manager::known_user::GetKnownIdFromUserId(user_id.id())
> > 
> > i.e. I don't see any reason why we should pass cryptohome::Identification to
> the
> > helper instead of cryptohome::Identification.id()
> 
> First of all, one of the important steps in this CL was moving away from
> std::string when passing cryptohome id to chromeos/dbus/* . as you could see,
> the first version of this CL didn't have this change. And I found a lot of
code
> I missed when incorrect ID stopped compiling. So this is done to prevent
errors
> in coding.
> 
> I needed some structure that could represent user ID in dbus/. It also should
be
> AccounId-constructible (which requires access to IsGaiaIdMigrationStarted ,
i.e.
> chromeos internals ). So the cryptohome::Identifier suits here very well.
> 
> Reverse translation requires access to chromeos and user_manager. But
> UserManager must not depend on ChromeOS, so I put the translation here.
> 
> Why is it strange to put this tranlation here as there is a lot of code in
> chromeos/login/auth that belongs to cryptohome?

I do not see a single instance of "namespace cryptohome" in src/chromeos/login,
and I find its use here very confusing.

I understand your argument for passing in cryptohome::Identification, but this
does not seem like the best place for this glue code. 

Why not make this cryptohome::Identification::GetAccountId()? You are already
have all of the same dependencies there.

[Alexander Alekseev, 2016-02-25 07:40:46]

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/dbus/cryptoho...
chromeos/dbus/cryptohome_client.cc:144: }
On 2016/02/24 17:25:43, stevenjb wrote:
> On 2016/02/24 09:27:42, Alexander Alekseev wrote:
> > On 2016/02/23 19:27:34, stevenjb wrote:
> > > Are these changes codependent, or can we separate this addition from the
> > > otherwise mechanical cryptohome_id change?
> > 
> > This CL implements cryptohome migration, and I have verified on a physical
> > device that it works.
> > 
> > What changes do you mean?
> > Do you mean you would prefer splitting this CL? This is going to be tricky
> > because I have changed only 2 or 3 API. All the other changes depend on
these
> > API.
> 
> I mean specifically adding a new method to the API. Please add
RenameCryptohome
> in a separate CL.
> 

Done.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
File chromeos/login/auth/cryptohome_authenticator.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/cr...
chromeos/login/auth/cryptohome_authenticator.cc:228: }
On 2016/02/24 17:25:43, stevenjb wrote:
> On 2016/02/24 09:27:42, Alexander Alekseev wrote:
> > On 2016/02/23 19:27:34, stevenjb wrote:
> > > Can these be added separately?
> > 
> > What do you mean?
> 
> I was hoping it would be reasonable to add EnsureCryptohomeMigratedToGaiaId in
a
> separate CL (along with RenameCryptohome). With a CL this large if there are
any
> bugs in it (which may not show up until it gets to the PFQ), it will be very
> difficult to revert it down the line. Breaking this up, even a little, would
> help a lot.
> 

Done.

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
File chromeos/login/auth/utils.cc (right):

https://codereview.chromium.org/1693383003/diff/140001/chromeos/login/auth/ut...
chromeos/login/auth/utils.cc:24: }
On 2016/02/24 17:25:43, stevenjb wrote:
> On 2016/02/24 01:52:57, Alexander Alekseev wrote:
> > On 2016/02/23 19:27:34, stevenjb wrote:
> > > This is a strange place to put something in the cryptohome namespace. Also
> > both
> > > 'utils.cc' and 'GetAccountId' are too generic.
> > > 
> > > It looks like this should really be:
> > > 
> > > user_manager::known_user::GetKnownIdFromUserId(user_id.id())
> > > 
> > > i.e. I don't see any reason why we should pass cryptohome::Identification
to
> > the
> > > helper instead of cryptohome::Identification.id()
> > 
> > First of all, one of the important steps in this CL was moving away from
> > std::string when passing cryptohome id to chromeos/dbus/* . as you could
see,
> > the first version of this CL didn't have this change. And I found a lot of
> code
> > I missed when incorrect ID stopped compiling. So this is done to prevent
> errors
> > in coding.
> > 
> > I needed some structure that could represent user ID in dbus/. It also
should
> be
> > AccounId-constructible (which requires access to IsGaiaIdMigrationStarted ,
> i.e.
> > chromeos internals ). So the cryptohome::Identifier suits here very well.
> > 
> > Reverse translation requires access to chromeos and user_manager. But
> > UserManager must not depend on ChromeOS, so I put the translation here.
> > 
> > Why is it strange to put this tranlation here as there is a lot of code in
> > chromeos/login/auth that belongs to cryptohome?
> 
> I do not see a single instance of "namespace cryptohome" in
src/chromeos/login,
> and I find its use here very confusing.
> 
> I understand your argument for passing in cryptohome::Identification, but this
> does not seem like the best place for this glue code. 
> 
> Why not make this cryptohome::Identification::GetAccountId()? You are already
> have all of the same dependencies there.
> 

Done.

[Alexander Alekseev, 2016-02-25 07:42:50]

Description was changed from

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier,
including support for renaming existing cryptohomes to new id
(RenameCreptohome dbus call).

BUG=462823
TEST=unit tests
==========

to

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier.

BUG=462823
TEST=unit tests
==========

[stevenjb, 2016-02-25 17:16:40]

Thanks! LGTM

[Alexander Alekseev, 2016-02-26 00:50:13]

+sky@ for:
    chrome/browser/browsing_data/browsing_data_remover.cc
    chrome/browser/policy/cloud/cloud_policy_browsertest.cc
    chrome/browser/profiles/profile_io_data.cc
    chrome/browser/ui/startup/startup_browser_creator.cc


Please review.

[Alexander Alekseev, 2016-02-26 02:13:03]

alemate@chromium.org changed reviewers:
+ sky@chromium.org

[Alexander Alekseev, 2016-02-26 02:13:05]

+sky@ for:
    chrome/browser/browsing_data/browsing_data_remover.cc
    chrome/browser/policy/cloud/cloud_policy_browsertest.cc
    chrome/browser/profiles/profile_io_data.cc
    chrome/browser/ui/startup/startup_browser_creator.cc


Please review.

[Alexander Alekseev, 2016-02-26 03:20:17]

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

[commit-bot: I haz the power, 2016-02-26 03:21:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/200001

[commit-bot: I haz the power, 2016-02-26 04:23:55]

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

[commit-bot: I haz the power, 2016-02-26 04:23:57]

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

[Alexander Alekseev, 2016-02-26 05:56:19]

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

[commit-bot: I haz the power, 2016-02-26 05:56:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/220001

[commit-bot: I haz the power, 2016-02-26 06:49:23]

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

[commit-bot: I haz the power, 2016-02-26 06:49:25]

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

[sky, 2016-02-26 16:08:00]

LGTM

[Alexander Alekseev, 2016-03-02 15:13:03]

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

[commit-bot: I haz the power, 2016-03-02 15:13:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/240001

[commit-bot: I haz the power, 2016-03-02 15:16:49]

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

[commit-bot: I haz the power, 2016-03-02 15:16:51]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2016-03-02 21:31:58]

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

[commit-bot: I haz the power, 2016-03-02 21:32:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/260001

[commit-bot: I haz the power, 2016-03-02 21:51:22]

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

[commit-bot: I haz the power, 2016-03-02 21:51:25]

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

[Alexander Alekseev, 2016-03-02 23:18:28]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2016-03-02 23:18:30]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
dkrahn@chromium.org, dzhioev@chromium.org, rogerta@chromium.org,
derat@chromium.org, stevenjb@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1693383003/#ps280001
(title: "Fix rebase.")

[commit-bot: I haz the power, 2016-03-02 23:19:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/280001

[commit-bot: I haz the power, 2016-03-03 00:21:41]

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

[commit-bot: I haz the power, 2016-03-03 00:21:43]

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

[Alexander Alekseev, 2016-03-03 01:42:24]

alemate@chromium.org changed reviewers:
+ mnissler@chromium.org

[Alexander Alekseev, 2016-03-03 01:42:26]

+mnissler@

Mattias. Could you look at my comment?

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:174:
return GetAccountId().GetUserEmail();
mnissler@ : this change actually means that user cannot change his enterprise
status during active session.

Mattias, does this sounds reasonable? (I believe there are also other reasons
why it's not possible.)

[Alexander Alekseev, 2016-03-03 10:44:20]

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

[commit-bot: I haz the power, 2016-03-03 10:44:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/300001

[commit-bot: I haz the power, 2016-03-03 11:56:21]

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

[commit-bot: I haz the power, 2016-03-03 11:56:24]

Dry run: This issue passed the CQ dry run.

[Mattias Nissler (ping if slow), 2016-03-03 11:57:05]

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:174:
return GetAccountId().GetUserEmail();
On 2016/03/03 01:42:26, Alexander Alekseev wrote:
> mnissler@ : this change actually means that user cannot change his enterprise
> status during active session.

Can you elaborate? Looking at the code changes, I can't follow that conclusion.
Also, what exactly do you mean by "enterprise status"?

> 
> Mattias, does this sounds reasonable? (I believe there are also other reasons
> why it's not possible.)

I think we actually used to be able to correctly upgrade regular accounts to
enterprise accounts back in the days. We'd just re-check with the device
management server to see whether the server would be willing to supply policy
for the user. I can't tell from the top of my head whether this is still working
though.

[Alexander Alekseev, 2016-03-03 13:02:12]

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:174:
return GetAccountId().GetUserEmail();
On 2016/03/03 11:57:04, Mattias (ping if no reply) wrote:
> On 2016/03/03 01:42:26, Alexander Alekseev wrote:
> > mnissler@ : this change actually means that user cannot change his
enterprise
> > status during active session.
> 
> Can you elaborate? Looking at the code changes, I can't follow that
conclusion.
> Also, what exactly do you mean by "enterprise status"?
> 
> > 
> > Mattias, does this sounds reasonable? (I believe there are also other
reasons
> > why it's not possible.)
> 
> I think we actually used to be able to correctly upgrade regular accounts to
> enterprise accounts back in the days. We'd just re-check with the device
> management server to see whether the server would be willing to supply policy
> for the user. I can't tell from the top of my head whether this is still
working
> though.

The old code in EPKPChallengeKeyBase::IsUserManaged() decided if user is
enterprise based on account info data. The problem with account info is that it
is deleted when refresh token gets invalidated. We've recently fixed another
crash because of this.

But we have another flag in user_manager::User object (user->IsAffiliated() ),
which (at least in browser_tests I've been fixing) was mostly unsynced. More
over, in tests we were are usually checking account info data belonging to a
different user (against browser context). This change means that only
"user->IsAffiliated()" flag is used here.

If we are able to correctly upgrade accounts to enterprise state, this means
that these two flags are synchronized in production code. So my change is safe
here.

[Alexander Alekseev, 2016-03-04 10:15:13]

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/1693383003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:174:
return GetAccountId().GetUserEmail();
On 2016/03/03 13:02:12, Alexander Alekseev wrote:
> On 2016/03/03 11:57:04, Mattias (ping if no reply) wrote:
> > On 2016/03/03 01:42:26, Alexander Alekseev wrote:
> > > mnissler@ : this change actually means that user cannot change his
> enterprise
> > > status during active session.
> > 
> > Can you elaborate? Looking at the code changes, I can't follow that
> conclusion.
> > Also, what exactly do you mean by "enterprise status"?
> > 
> > > 
> > > Mattias, does this sounds reasonable? (I believe there are also other
> reasons
> > > why it's not possible.)
> > 
> > I think we actually used to be able to correctly upgrade regular accounts to
> > enterprise accounts back in the days. We'd just re-check with the device
> > management server to see whether the server would be willing to supply
policy
> > for the user. I can't tell from the top of my head whether this is still
> working
> > though.
> 
> The old code in EPKPChallengeKeyBase::IsUserManaged() decided if user is
> enterprise based on account info data. The problem with account info is that
it
> is deleted when refresh token gets invalidated. We've recently fixed another
> crash because of this.
> 
> But we have another flag in user_manager::User object (user->IsAffiliated() ),
> which (at least in browser_tests I've been fixing) was mostly unsynced. More
> over, in tests we were are usually checking account info data belonging to a
> different user (against browser context). This change means that only
> "user->IsAffiliated()" flag is used here.
> 
> If we are able to correctly upgrade accounts to enterprise state, this means
> that these two flags are synchronized in production code. So my change is safe
> here.

I've checked that IsUserAffiliated() is kept in sync via
UserCloudPolicyManagerChromeOS . So I'll land this CL.

[Alexander Alekseev, 2016-03-04 10:15:38]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2016-03-04 10:15:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rogerta@chromium.org, sky@chromium.org,
xiyuan@chromium.org, derat@chromium.org, dzhioev@chromium.org,
dkrahn@chromium.org
Link to the patchset: https://codereview.chromium.org/1693383003/#ps300001
(title: "Fixed unit tests.")

[commit-bot: I haz the power, 2016-03-04 10:15:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693383003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693383003/300001

[commit-bot: I haz the power, 2016-03-04 10:40:02]

Description was changed from

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier.

BUG=462823
TEST=unit tests
==========

to

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier.

BUG=462823
TEST=unit tests
==========

[commit-bot: I haz the power, 2016-03-04 10:40:05]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-03-04 10:41:24]

Description was changed from

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier.

BUG=462823
TEST=unit tests
==========

to

==========
ChromeOS cryptohome should be able to use gaia id as user identifier.

This CL adds support for using Gaia ID as cryptohome identifier.

BUG=462823
TEST=unit tests

Committed: https://crrev.com/627f65303a4a1a6b4d1beb62c9ea5b6ed5affda7
Cr-Commit-Position: refs/heads/master@{#379262}
==========

[commit-bot: I haz the power, 2016-03-04 10:41:27]

Patchset 16 (id:??) landed as
https://crrev.com/627f65303a4a1a6b4d1beb62c9ea5b6ed5affda7
Cr-Commit-Position: refs/heads/master@{#379262}