Translate device-local account IDs to user IDs

Translate device-local account IDs to user IDs

This CL decouples the namespaces used for device-local accounts in policy
and user management. In addition to the account ID assigned by the server,
each device-local account is given a user ID that always follows a
canonical format and allows the account type to be identified just by
looking at the user ID, regardless of the format chosen for the account ID
by the server.

BUG=240276
TEST=Manual in VM

TBR=jhawkins (chrome/browser/ui/webui/policy_ui.cc)

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

[bartfab (slow), 2013-05-13 16:14:53]

Hi Mattias,

Can you please review the policy/device settings side of this CL?

Hi Nikita,

Can you please review chrome/browser/chromeos/*

Hi James,

Can you please do an owner's review of chrome/browser/ui/webui/policy_ui.cc

[Mattias Nissler (ping if slow), 2013-05-15 08:46:40]

FWIW, here's a batch of comments from an earlier review pass I didn't finish.
Starting a new pass now.

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account.cc:19: static const char
kPublicAccountDomainPrefix[] = "public-accounts";
no need for static

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account.cc:53:
crypto::SHA256HashString(account_id, &hash, sizeof(hash));
Is this fast enough?

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account.cc:54: return
base::HexEncode(&hash, sizeof(hash)) + "@" +
I think Xiyuan mentioned something about requiring the app id for Kiosk Apps in
the user id. That would mean we need to generate different IDs for
TYPE_KIOSK_APP.

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account.cc:80: &kiosk_app_id))) {
This conditional is too long. Please break it up - maybe decode the
type-specific parts below?

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account.cc:91: if
(!account_ids.insert(account_id).second) {
account_ids is a set - how can this access second?

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account.h:44:
std::vector<DeviceLocalAccount> DecodeDeviceLocalAccountsList(
Could use a comment mentioning what format 
|list| is supposed to be in.

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/14927015/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:74: :
broker(NULL) {
nit: this file doesn't put newlines for empty function bodies elsewhere

[Mattias Nissler (ping if slow), 2013-05-15 09:38:47]

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/existing_user_controller.cc:902: void
ExistingUserController::ConfigurePublicSessionAutoLogin() {
Hm, maybe a better approach for this would be to put the auto-login flag into
the User object?

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1227: continue;
Can you retain the comment from line 1490 here?

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:119:
DeviceLocalAccountTest() : user_id_1_(GenerateDeviceLocalAccountUserId(
Is this clang-format at work? Common Chromium style would have put a line break
before the colon I guess...

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:105: if
(!it->second.broker)
I think this is inconsistent with UpdateAccountList now, which does create the
broker if the device_management_service_ is available. The latter may have
become just available, so we should probably re-check whether we want to create
the broker here.

Now that you have the additional wrapper layer, you might want to make a
ConnectIfApplicable() call on the wrapper?

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:121:
it->second.broker->Disconnect();
Maybe just make BrokerWrapper::Disconnect() function?

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:132: if
(!entry->second.broker) {
Could just move this to a BrokerWrapper::GetBroker() function, which you can
also use elsewhere.

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:122:
chromeos::ScopedTestCrosSettings test_cros_settings_;
Wait, doesn't this conflict with DeviceSettingsTestBase? You probably have two
DeviceSettingsService instances now.

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (left):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_test_helper.h:175:
DeviceSettingsService device_settings_service_;
And everything moves in the direction of global singletons again. I had a
conversation with stevenjb@ recently about this - it's always easy to make steps
towards using global singletons for everything, whereas introducing proper
ownership graphs is hard and undermined if we move code towards globals. What's
your thoughts on this? If I'm the only one fighting this fight, I'm willing to
just give up.

[Nikita (slow), 2013-05-15 09:40:30]

chromeos/browser/login lgtm

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1189: bool
UserManagerImpl::UpdateAndCleanUpPublicAccounts(
I think that this method better be splitted into a smaller methods - currently
it is 100 lines.

How about extracting few helper functions to anonymous namespace?

[bartfab (slow), 2013-05-17 11:14:27]

Hi everyone, could you take another look?

Hi Xiyuan, could you take a look at the kiosk app changes?

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.cc:19: static const char
kPublicAccountDomainPrefix[] = "public-accounts";
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> no need for static

Done.

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.cc:53:
crypto::SHA256HashString(account_id, &hash, sizeof(hash));
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> Is this fast enough?

Good point. Actually, hashing is completely unnecessary here. I just HexEncode()
the account_id now.

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.cc:54: return
base::HexEncode(&hash, sizeof(hash)) + "@" +
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> I think Xiyuan mentioned something about requiring the app id for Kiosk Apps
in
> the user id. That would mean we need to generate different IDs for
> TYPE_KIOSK_APP.

Thanks for reminding me of that. I talked to Xiyuan and updated the kiosk app
code accordingly.

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.cc:80: &kiosk_app_id))) {
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> This conditional is too long. Please break it up - maybe decode the
> type-specific parts below?

Done.

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.cc:91: if
(!account_ids.insert(account_id).second) {
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> account_ids is a set - how can this access second?

std::set::insert() returns an std::pair. The second element of this pair is a
boolean that tells you whether the insertion actually happened.

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account.h:44:
std::vector<DeviceLocalAccount> DecodeDeviceLocalAccountsList(
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> Could use a comment mentioning what format 
> |list| is supposed to be in.

Done.

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://chromiumcodereview.appspot.com/14927015/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:74: :
broker(NULL) {
On 2013/05/15 08:46:40, Mattias Nissler wrote:
> nit: this file doesn't put newlines for empty function bodies elsewhere

Done.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/login/existing_user_controller.cc:902: void
ExistingUserController::ConfigurePublicSessionAutoLogin() {
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> Hm, maybe a better approach for this would be to put the auto-login flag into
> the User object?

I don't think that would be right. Auto-login is not a property of an individual
device-local account but a single, global setting. If you do not want
ExistingUserController to be managing the auto-login settings separately, the
approach I would take is to create a DeviceLocalAccounts class that contains a
the account list (an std::vector) and the auto-login settings (int + std::string
and/or a reference into the account list).

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1189: bool
UserManagerImpl::UpdateAndCleanUpPublicAccounts(
On 2013/05/15 09:40:30, Nikita Kostylev wrote:
> I think that this method better be splitted into a smaller methods - currently
> it is 100 lines.
> 
> How about extracting few helper functions to anonymous namespace?

Done. Also, this CL is now built on top of
https://codereview.chromium.org/15198005/

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1227: continue;
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> Can you retain the comment from line 1490 here?

Done.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:119:
DeviceLocalAccountTest() : user_id_1_(GenerateDeviceLocalAccountUserId(
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> Is this clang-format at work? Common Chromium style would have put a line
break
> before the colon I guess...

No, this was my own silly formatting. Done.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:105: if
(!it->second.broker)
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> I think this is inconsistent with UpdateAccountList now, which does create the
> broker if the device_management_service_ is available. The latter may have
> become just available, so we should probably re-check whether we want to
create
> the broker here.
> 
> Now that you have the additional wrapper layer, you might want to make a
> ConnectIfApplicable() call on the wrapper?

Done, although I am not sure adding the helper methods makes things better as we
have to pass a lot of state around now.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:121:
it->second.broker->Disconnect();
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> Maybe just make BrokerWrapper::Disconnect() function?

Done.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:132: if
(!entry->second.broker) {
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> Could just move this to a BrokerWrapper::GetBroker() function, which you can
> also use elsewhere.

Done.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:122:
chromeos::ScopedTestCrosSettings test_cros_settings_;
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> Wait, doesn't this conflict with DeviceSettingsTestBase? You probably have two
> DeviceSettingsService instances now.

Done.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (left):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/settings/device_settings_test_helper.h:175:
DeviceSettingsService device_settings_service_;
On 2013/05/15 09:38:47, Mattias Nissler wrote:
> And everything moves in the direction of global singletons again. I had a
> conversation with stevenjb@ recently about this - it's always easy to make
steps
> towards using global singletons for everything, whereas introducing proper
> ownership graphs is hard and undermined if we move code towards globals.
What's
> your thoughts on this? If I'm the only one fighting this fight, I'm willing to
> just give up.

I agree. For some reason, I thought it would be much harder to mock out things
without global singletons in this case. It was actually quite simple in the end.
Done.

[Mattias Nissler (ping if slow), 2013-05-17 14:29:44]

This will likely be my last review feedback for a week. I'm cool with this in
general, so feel free to get an LGTM from another reviewer and commit once all
remaining questions are sorted.

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/24001/chrome/browser/chr...
chrome/browser/chromeos/login/existing_user_controller.cc:902: void
ExistingUserController::ConfigurePublicSessionAutoLogin() {
On 2013/05/17 11:14:28, bartfab wrote:
> On 2013/05/15 09:38:47, Mattias Nissler wrote:
> > Hm, maybe a better approach for this would be to put the auto-login flag
into
> > the User object?
> 
> I don't think that would be right. Auto-login is not a property of an
individual
> device-local account but a single, global setting. If you do not want
> ExistingUserController to be managing the auto-login settings separately, the
> approach I would take is to create a DeviceLocalAccounts class that contains a
> the account list (an std::vector) and the auto-login settings (int +
std::string
> and/or a reference into the account list).

What I'm saying is that ExistingUserController should probably not read the auto
login property directly from the persistence layer and instead UserManager
should be in charge of telling the login UI what the auto-login account should
be.

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:117: const
std::vector<policy::DeviceLocalAccount> device_local_accounts =
We might better go through KioskAppManager to find the username here?

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1085: // TODO(bartfab): Add
KioskAppUsers to the users_ list and keep metadata like
not sure whether we want to do this half-way. Maybe it's simpler to keep the
<app_id>@kiosk-apps.device-local-accounts.localhost for kiosk apps for now.

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account.cc:22: 
nit: you can nuke the newlines between the constants.

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:138: if
(!client)
Hm, could as well do if(client) connect

[xiyuan, 2013-05-17 15:46:52]

kiosk app part LGTM

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:117: const
std::vector<policy::DeviceLocalAccount> device_local_accounts =
On 2013/05/17 14:29:44, Mattias Nissler wrote:
> We might better go through KioskAppManager to find the username here?

Let's add a new method to KioskAppManager and move the logic there.

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:140: << "'.";
nit: think "'" is more correct since we have an opening "'" before |user_id_|.

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://chromiumcodereview.appspot.com/14927015/diff/46001/chrome/browser/chr...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:126: it =
device_local_accounts.erase(it);
nit: we could break out here since kiosk app id should be unique in the list.

[bartfab (slow), 2013-05-17 16:08:47]

Comments addressed.

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/14927015/diff/24001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/existing_user_controller.cc:902: void
ExistingUserController::ConfigurePublicSessionAutoLogin() {
On 2013/05/17 14:29:44, Mattias Nissler wrote:
> On 2013/05/17 11:14:28, bartfab wrote:
> > On 2013/05/15 09:38:47, Mattias Nissler wrote:
> > > Hm, maybe a better approach for this would be to put the auto-login flag
> into
> > > the User object?
> > 
> > I don't think that would be right. Auto-login is not a property of an
> individual
> > device-local account but a single, global setting. If you do not want
> > ExistingUserController to be managing the auto-login settings separately,
the
> > approach I would take is to create a DeviceLocalAccounts class that contains
a
> > the account list (an std::vector) and the auto-login settings (int +
> std::string
> > and/or a reference into the account list).
> 
> What I'm saying is that ExistingUserController should probably not read the
auto
> login property directly from the persistence layer and instead UserManager
> should be in charge of telling the login UI what the auto-login account should
> be.

A near-term goal is to create User objects for all device-local accounts and
keep them in UserManagerImpl::users_. At that point, I agree,
ExistingUserController should just ask the UserManager for the right User.

Right now, the only complete list of device-local accounts exists in
CrosSettings. It is unavoidable that the list be read from CrosSettings and then
searched for the right entry. I can move the chunk of code that does that to
UserManager but this will not really buy us anything: Every LOC I remove here
will have to be added to UserManagerImpl. Plus, I will have to inflate the
UserManager interface by adding yet another method.

The careful reader will notice that UserManagerImpl already does read and parse
the list of device-local accounts. It might appear that UserManagerImpl can just
extract and store the auto-login user ID at that point. Unfortunately, that will
not work. The reading and parsing happens only after the device settings have
become trusted while this method here may run earlier.

Since this method already accesses the CrosSettings backend, I think it is most
appropriate to leave the reading and parsing of the device-local account list
here. Once device-local accounts appear in UserManagerImpl::users_, all of the
code can go away.

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc (right):

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:117: const
std::vector<policy::DeviceLocalAccount> device_local_accounts =
On 2013/05/17 15:46:52, xiyuan wrote:
> On 2013/05/17 14:29:44, Mattias Nissler wrote:
> > We might better go through KioskAppManager to find the username here?
> 
> Let's add a new method to KioskAppManager and move the logic there.

Done.

I reused the existing KioskAppManager::GetApp(). I think this is even neater
than adding a new special-purpose method.

While here, I also made some of the dependencies more explicit - part of the
neverending crusade against global singletons :).

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:140: << "'.";
On 2013/05/17 15:46:52, xiyuan wrote:
> nit: think "'" is more correct since we have an opening "'" before |user_id_|.

It is a single quote already - this is probably hard to see in an editor as
there are so many quotes in a row :).

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:126: it =
device_local_accounts.erase(it);
On 2013/05/17 15:46:52, xiyuan wrote:
> nit: we could break out here since kiosk app id should be unique in the list.

Done.

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1085: // TODO(bartfab): Add
KioskAppUsers to the users_ list and keep metadata like
On 2013/05/17 14:29:44, Mattias Nissler wrote:
> not sure whether we want to do this half-way. Maybe it's simpler to keep the
> <mailto:app_id>@kiosk-apps.device-local-accounts.localhost for kiosk apps for
now.

I would prefer to go for
${opaque_identifier}@kiosk-apps.device-local-accounts.localhost straight away.
That way, this CL implements a well-defined step that moves all device-local
accounts to a new naming scheme with opaque identifiers. It is not a half-way
step at all.

Adding device-local accounts to the users_ list is a separate improvement that
can then be done in a second step.

Any move that changes user IDs is a risky undertaking. Stuff may break. I would
rather do it once, right now, than having to do it twice:

Now:
${app_id}@kiosk-apps.localhost
=>
${app_id}@kiosk-apps.device-local-accounts.localhost

Later:
${app_id}@kiosk-apps.device-local-accounts.localhost
=>
${opaque_identifier}@kiosk-apps.device-local-accounts.localhost

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account.cc:22: 
On 2013/05/17 14:29:44, Mattias Nissler wrote:
> nit: you can nuke the newlines between the constants.

Done.

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/14927015/diff/46001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:138: if
(!client)
On 2013/05/17 14:29:44, Mattias Nissler wrote:
> Hm, could as well do if(client) connect

Done.

[bartfab (slow), 2013-05-17 16:09:53]

Hi Joao,

With Mattias out for a week, could you take a look at the policy side of things?
As you can see, Mattias had posted a few more comments and questions that should
all be addressed now.

[xiyuan, 2013-05-17 16:16:13]

Cool.

Kiosk app part LGTM++

[Joao da Silva, 2013-05-17 18:10:54]

lgtm with nits. Please see the comment about the break in a for loop, something
may be wrong there.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:31: }  // namespace
nit: The empty namespace { } can be removed

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.h (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.h:30: explicit
KioskAppLauncher(KioskAppManager* kiosk_app_manager,
nit: no need to be explicit

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:97: // Don't insert if the
app if it's already in the list.
nit: please fix the comment :-)

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:128: break;
I'm a bit confused here; why is |it| updated if the iteration is stopped? The
comment says that this removes the "entries", so should this break be removed?

To make this equivalent to the previous code I think the break should be
removed; but if not, then |it| can be updated in the for() statement.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account.cc:65:
scoped_ptr<base::ListValue> list(new base::ListValue);
Suggestion: this memory is never transferred, so why not a plain
base::ListValue?

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account.h:38: std::string
kiosk_app_update_url;
Can these fields be const?

[xiyuan, 2013-05-17 18:20:10]

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:128: break;
On 2013/05/17 18:10:55, Joao da Silva wrote:
> I'm a bit confused here; why is |it| updated if the iteration is stopped? The
> comment says that this removes the "entries", so should this break be removed?
> 
> To make this equivalent to the previous code I think the break should be
> removed; but if not, then |it| can be updated in the for() statement.

There should only be one entry per app id. It is an error if we have more than
one. Agree that we could just erase and break.

[bartfab (slow), 2013-05-21 13:27:07]

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.cc:31: }  // namespace
On 2013/05/17 18:10:55, Joao da Silva wrote:
> nit: The empty namespace { } can be removed

Done.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_launcher.h (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_launcher.h:30: explicit
KioskAppLauncher(KioskAppManager* kiosk_app_manager,
On 2013/05/17 18:10:55, Joao da Silva wrote:
> nit: no need to be explicit

Done.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:97: // Don't insert if the
app if it's already in the list.
On 2013/05/17 18:10:55, Joao da Silva wrote:
> nit: please fix the comment :-)

Done.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:128: break;
On 2013/05/17 18:20:10, xiyuan wrote:
> On 2013/05/17 18:10:55, Joao da Silva wrote:
> > I'm a bit confused here; why is |it| updated if the iteration is stopped?
The
> > comment says that this removes the "entries", so should this break be
removed?
> > 
> > To make this equivalent to the previous code I think the break should be
> > removed; but if not, then |it| can be updated in the for() statement.
> 
> There should only be one entry per app id. It is an error if we have more than
> one. Agree that we could just erase and break.

Done.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account.cc (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account.cc:65:
scoped_ptr<base::ListValue> list(new base::ListValue);
On 2013/05/17 18:10:55, Joao da Silva wrote:
> Suggestion: this memory is never transferred, so why not a plain
> base::ListValue?

Done.

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/14927015/diff/54004/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_local_account.h:38: std::string
kiosk_app_update_url;
On 2013/05/17 18:10:55, Joao da Silva wrote:
> Can these fields be const?

Unfortunately, no, because DeviceLocalAccount needs to support the assignment
operator so that an std::vector<DeviceLocalAccount> can be created.

[bartfab (slow), 2013-05-21 13:30:28]

Hi Julian, with Mattias out for the week, could you take a look at the
following:

chrome/browser/chromeos/settings/*

[bartfab (slow), 2013-05-21 13:30:48]

Hi Julian, with Mattias out for the week, could you take a look at the
following:

chrome/browser/chromeos/settings/*

[Joao da Silva, 2013-05-21 13:32:30]

lgtm

https://codereview.chromium.org/14927015/diff/61001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14927015/diff/61001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:97: // Don't insert if the
app if it's already in the list.
nit: the comment hasn't been fixed :-/ There are two "ifs", it's not clear what
is meant.

[bartfab (slow), 2013-05-21 13:37:42]

https://codereview.chromium.org/14927015/diff/61001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/14927015/diff/61001/chrome/browser/chromeos/a...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:97: // Don't insert if the
app if it's already in the list.
On 2013/05/21 13:32:30, Joao da Silva wrote:
> nit: the comment hasn't been fixed :-/ There are two "ifs", it's not clear
what
> is meant.

Sorry, I misunderstood your nit. I thought you wanted me to restore the comment
"Add the new account" a few lines further down :). Done now.

[pastarmovj, 2013-05-21 13:48:39]

Owner blessing for the chromeos/settings. lgtm

[James Hawkins, 2013-05-21 20:56:18]

lgtm

[commit-bot: I haz the power, 2013-05-22 08:39:45]

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

[commit-bot: I haz the power, 2013-05-22 09:01:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-22 09:09:53]

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

[commit-bot: I haz the power, 2013-05-22 09:31:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-22 09:44:56]

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

[commit-bot: I haz the power, 2013-05-22 15:37:30]

Change committed as 201532