Add first implemenation for SU password sync

Add first implemenation for SU password sync

TBR=wad@chromium.org
BUG=244472
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242148

[Denis Kuznetsov (DE-MUC), 2013-12-06 17:57:53]

Will, please look on security-related stuff (algorithms, key length, etc)
Nikita, please review rest of the stuff.

[Nikita (slow), 2013-12-09 16:42:15]

mostly nits

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

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.cc:224: user_context.username,
nit: It seems that this parameter will fit on the previous line and 2 others can
be aligned with it. I think this may be a bit more readable.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_performer.cc:229: UserContext
copy(user_context.username,
nit: Rename to user_context_copy?

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc
(right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:156:
creation_context_->local_user_id,
nit: Please move parameter to prev line and align 2 others with it.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:164:
authenticator_->AuthenticateToCreate(
nit: Can you please change alignment in the same way here if possible.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:209:
authenticator_->AddMasterKey(
nit: Can you please change alignment in the same way here if possible.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:223:
// TODO(antrim) : add password data to sync once API is ready.
nit: Bug # is missing. Just to make sure that we're tracking this on crbug.com.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:20:
const unsigned kNumIterations = 1234;
nit: Please add short comment about these constants.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.h
(right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:16: const
int kPlainPasswordSchema = 1;
Do you need this in header? I see that these are only used in this class.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:21: //
UserFlow implementation for signing in locally managed user.
SupervisedUserAuthentication class comment should be expanded.
It should contain some very basic information (one can always check the code for
more details) on what it does and some short explanation what is schema version
(which version there're now), password revision etc. Also mention when migration
happens. Keep it simple, so that at least some of this knowledge is documented
in the comments.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:32: //
target schema.
nit: Can you change "target schema" to something more specific?

"if user password schema is different from the latest schema"?

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:52:
std::string BuildPasswordForSchemaV2(const std::string& salt,
nit: Please add comment. Move to private API or to unnamed namespace?

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:58: bool
should_migrate_;
nit: migration_enabled_

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:61: int
target_version_;
nit: current_version_ or latest_version_

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc:18: #include
"chrome/browser/chromeos/login/supervised_user_manager.h"
nit: not used.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/supervised_user_manager.h (right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised_user_manager.h:21: extern const char
kPasswordVersion[];
nit: Rename to revision?

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised_user_manager.h:89: virtual
SupervisedUserAuthentication* GetSupervisedUserAuthentication() = 0;
nit: I think this may be renamed to GetAuthentication() since in this context it
is quite clear:

SupervisedUserAuthentication* authentication = UserManager::Get()->
    GetSupervisedUserManager()->GetAuthentication();

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

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:57: // A map from
user id to password schema version,
nit: revision, dot at the end.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:73: const char
kSchemaVersion[] = "SchemaVersion";
nit: One small comment about these would be helpful.

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/supervised_user_manager_impl.h (right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised_user_manager_impl.h:74: bool
GetUserValue(const std::string& user_id,
nit: Please add comments for these new functions.

[Nikita (slow), 2013-12-09 17:51:07]

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit: 2013

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:54: if
(kPasswordEncryptedWithSaltSchema == user_schema_version) {
nit: How about wrapping this into else if

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:62:
NOTREACHED();
nit: Put this block into else?

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:71: if
(schema == kPlainPasswordSchema)
nit: Please keep consistency with previous method: either first put variable or
constant.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:73: if
(schema == kPasswordEncryptedWithSaltSchema) {
nit:
if (v1) {
  ..
} else if (v2) {
  ..
} else {
  NOTREACHED();
}

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:75:
kSchemaVersion, base::IntToString(schema));
nit: Will it fit? First parameter on previous line, another one aligned.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:78:
kSalt, salt);
nit: Fits on the previous line.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:80:
kPasswordVersion, std::string("1"));
nit: first parameter on the previous line, another one aligned.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:80:
kPasswordVersion, std::string("1"));
nit: std::string("1") - magic constant?

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.h
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:52:
std::string BuildPasswordForSchemaV2(const std::string& salt,
nit: You name schema here as V2 while constant is called just 
kPasswordEncryptedWithSaltSchema.

Can it be replaced with smth like? 
kPlainPasswordSchemaV1
kPasswordEncryptedWithSaltSchemaV2

[Bernhard Bauer, 2013-12-11 14:46:43]

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:74:
password_data->SetStringWithoutPathExpansion(
You don't need to store everything as a string. You could just use
SetInteger...().

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.h
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:17: const
int kPasswordEncryptedWithSaltSchema = 2;
Could you use an enum?

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/supervised_user_manager.h (right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager.h:95: // Stores public
password-specific data from |password_info| for |user_id| in
Nit: newline before this line.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:211:
base::DictionaryValue* result) {
You don't really need to pass a dictionary here that can hold arbitrary
key-value pairs. You could just pass three out params, or create a struct for
them.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:249:
update->SetWithoutPathExpansion(user_id, new base::StringValue(value));
SetStringWithoutPathExpansion()

[Denis Kuznetsov (DE-MUC), 2013-12-12 19:45:24]

Fixed all first nits.
In upcoming patchset will fix things in tests and rework how data is stored in
LocalState (SupervisedUsers->userId->Keys->Values insead of
SupervisedUserKey->userId->Values)

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/login_performer.cc:224: user_context.username,
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: It seems that this parameter will fit on the previous line and 2 others
can
> be aligned with it. I think this may be a bit more readable.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/login_performer.cc:229: UserContext
copy(user_context.username,
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Rename to user_context_copy?

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:156:
creation_context_->local_user_id,
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Please move parameter to prev line and align 2 others with it.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:164:
authenticator_->AuthenticateToCreate(
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Can you please change alignment in the same way here if possible.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:209:
authenticator_->AddMasterKey(
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Can you please change alignment in the same way here if possible.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:223:
// TODO(antrim) : add password data to sync once API is ready.
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Bug # is missing. Just to make sure that we're tracking this on
http://crbug.com.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: 2013

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:20:
const unsigned kNumIterations = 1234;
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Please add short comment about these constants.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:54: if
(kPasswordEncryptedWithSaltSchema == user_schema_version) {
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: How about wrapping this into else if

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:62:
NOTREACHED();
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: Put this block into else?

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:71: if
(schema == kPlainPasswordSchema)
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: Please keep consistency with previous method: either first put variable
or
> constant.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:73: if
(schema == kPasswordEncryptedWithSaltSchema) {
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit:
> if (v1) {
>   ..
> } else if (v2) {
>   ..
> } else {
>   NOTREACHED();
> }

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:74:
password_data->SetStringWithoutPathExpansion(
On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> You don't need to store everything as a string. You could just use
> SetInteger...().

Yes, but (as I understand) if I at some point set it as integer, and in other
try to get it as string (or vice versa) it will fail.
And as they are written to disk / (will be) retrieved from sync as strings, it's
easier to pass them around as strings.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:75:
kSchemaVersion, base::IntToString(schema));
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: Will it fit? First parameter on previous line, another one aligned.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:78:
kSalt, salt);
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: Fits on the previous line.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:80:
kPasswordVersion, std::string("1"));
On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> nit: std::string("1") - magic constant?

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.h
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:16: const
int kPlainPasswordSchema = 1;
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> Do you need this in header? I see that these are only used in this class.

Moved it to enum inside the class.
For now it's inside public: section, but I will consider if it should be moved
to private: later.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:17: const
int kPasswordEncryptedWithSaltSchema = 2;
On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> Could you use an enum?

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:21: //
UserFlow implementation for signing in locally managed user.
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> SupervisedUserAuthentication class comment should be expanded.
> It should contain some very basic information (one can always check the code
for
> more details) on what it does and some short explanation what is schema
version
> (which version there're now), password revision etc. Also mention when
migration
> happens. Keep it simple, so that at least some of this knowledge is documented
> in the comments.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:32: //
target schema.
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Can you change "target schema" to something more specific?
> 
> "if user password schema is different from the latest schema"?

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:58: bool
should_migrate_;
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: migration_enabled_

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.h:61: int
target_version_;
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: current_version_ or latest_version_

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc:18: #include
"chrome/browser/chromeos/login/supervised_user_manager.h"
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: not used.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/supervised_user_manager.h (right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager.h:21: extern const char
kPasswordVersion[];
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: Rename to revision?

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager.h:89: virtual
SupervisedUserAuthentication* GetSupervisedUserAuthentication() = 0;
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: I think this may be renamed to GetAuthentication() since in this context
it
> is quite clear:
> 
> SupervisedUserAuthentication* authentication = UserManager::Get()->
>     GetSupervisedUserManager()->GetAuthentication();

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager.h:95: // Stores public
password-specific data from |password_info| for |user_id| in
On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> Nit: newline before this line.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:57: // A map from
user id to password schema version,
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: revision, dot at the end.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:73: const char
kSchemaVersion[] = "SchemaVersion";
On 2013/12/09 16:42:16, Nikita Kostylev wrote:
> nit: One small comment about these would be helpful.

Done.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:211:
base::DictionaryValue* result) {
On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> You don't really need to pass a dictionary here that can hold arbitrary
> key-value pairs. You could just pass three out params, or create a struct for
> them.
I want to keep things simple - same dictionary will be used to store data in
sync (json blob), and I want intermediate layers to be independent of actual
values needed for schema.
I like static typing, but in this case I believe that using dictionary will
serve better.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:249:
update->SetWithoutPathExpansion(user_id, new base::StringValue(value));
On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> SetStringWithoutPathExpansion()

Done.

[Bernhard Bauer, 2013-12-13 00:30:50]

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:74:
password_data->SetStringWithoutPathExpansion(
On 2013/12/12 19:45:24, Denis Kuznetsov wrote:
> On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> > You don't need to store everything as a string. You could just use
> > SetInteger...().
> 
> Yes, but (as I understand) if I at some point set it as integer, and in other
> try to get it as string (or vice versa) it will fail.
> And as they are written to disk / (will be) retrieved from sync as strings,
it's
> easier to pass them around as strings.

Sync doesn't know about the preferences as they are stored in Local State. The
API for you to use will take a string key and a base::Value setting, which could
be an integer. It will internally serialize the value to JSON (i.e. a string),
but that's an implementation detail.

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:211:
base::DictionaryValue* result) {
On 2013/12/12 19:45:24, Denis Kuznetsov wrote:
> On 2013/12/11 14:46:43, Bernhard Bauer wrote:
> > You don't really need to pass a dictionary here that can hold arbitrary
> > key-value pairs. You could just pass three out params, or create a struct
for
> > them.
> I want to keep things simple - same dictionary will be used to store data in
> sync (json blob), 

Like I explained above, it won't be the exact same dictionary.

> and I want intermediate layers to be independent of actual
> values needed for schema.

Well, the only thing that's independent of the actual schema is the interface,
and both client and implementor of the interface will need to know which keys
are in the dictionary. I don't think that will keep things easier, but if you
want to, I'm not going to stop you.

> I like static typing, but in this case I believe that using dictionary will
> serve better.

[Bernhard Bauer, 2013-12-14 03:02:30]

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:73: if
(schema == kPasswordEncryptedWithSaltSchema) {
On 2013/12/12 19:45:24, Denis Kuznetsov wrote:
> On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> > nit:
> > if (v1) {
> >   ..
> > } else if (v2) {
> >   ..
> > } else {
> >   NOTREACHED();
> > }
> 
> Done.

Um... sorry, but http://dev.chromium.org/developers/coding-style#Code_Formatting
says "Don't use else after return", and all the branches here end with a return
statement.

https://chromiumcodereview.appspot.com/101283003/diff/140001/chrome/browser/c...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/140001/chrome/browser/c...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:130:
SupervisedUserAuthentication::GetPasswordSchema(
Nit: No indentation here.

[Nikita (slow), 2013-12-15 06:55:12]

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:73: if
(schema == kPasswordEncryptedWithSaltSchema) {
On 2013/12/14 03:02:30, Bernhard Bauer wrote:
> On 2013/12/12 19:45:24, Denis Kuznetsov wrote:
> > On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> > > nit:
> > > if (v1) {
> > >   ..
> > > } else if (v2) {
> > >   ..
> > > } else {
> > >   NOTREACHED();
> > > }
> > 
> > Done.
> 
> Um... sorry, but
http://dev.chromium.org/developers/coding-style#Code_Formatting
> says "Don't use else after return", and all the branches here end with a
return
> statement.

So this should be like
bool result = false;

if (...) {
  ...
  result = true;
} else {
  ...
  result = false;
}

return result;

right?

[Bernhard Bauer, 2013-12-15 15:39:56]

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:73: if
(schema == kPasswordEncryptedWithSaltSchema) {
On 2013/12/15 06:55:12, Nikita Kostylev wrote:
> On 2013/12/14 03:02:30, Bernhard Bauer wrote:
> > On 2013/12/12 19:45:24, Denis Kuznetsov wrote:
> > > On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> > > > nit:
> > > > if (v1) {
> > > >   ..
> > > > } else if (v2) {
> > > >   ..
> > > > } else {
> > > >   NOTREACHED();
> > > > }
> > > 
> > > Done.
> > 
> > Um... sorry, but
> http://dev.chromium.org/developers/coding-style#Code_Formatting
> > says "Don't use else after return", and all the branches here end with a
> return
> > statement.
> 
> So this should be like
> bool result = false;
> 
> if (...) {
>   ...
>   result = true;
> } else {
>   ...
>   result = false;
> }
> 
> return result;
> 
> right?

No, the else should go, not the return :)

So it would be like

  if (...)
    return false;

  if (...)
    return true;

  NOTREACHED();
  return false;

[Nikita (slow), 2013-12-19 14:47:01]

As discussed with Denis offline there's no need to hold off this CL till
LocalState user storage is changed + migration implemented. That change deserves
a separate CL.

Let's finish with this CL ASAP to have something behind the flag hopefully
before holidays.

I know that design doc is still under discussion, Will please make that a top
priority.

[Nikita (slow), 2013-12-19 14:53:02]

lgtm on my comments

I can do another pass of there're significant changes.

https://codereview.chromium.org/101283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://codereview.chromium.org/101283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:74: //
nit: Comment is missing?

[Denis Kuznetsov (DE-MUC), 2013-12-19 16:22:16]

Marking Will as TBR

https://codereview.chromium.org/101283003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://codereview.chromium.org/101283003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:74: //
On 2013/12/19 14:53:03, Nikita Kostylev wrote:
> nit: Comment is missing?

Done.

[Nikita (slow), 2013-12-19 16:28:51]

On 2013/12/15 15:39:56, Bernhard Bauer wrote:
>
https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
> (right):
> 
>
https://codereview.chromium.org/101283003/diff/20001/chrome/browser/chromeos/...
> chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:73: if
> (schema == kPasswordEncryptedWithSaltSchema) {
> On 2013/12/15 06:55:12, Nikita Kostylev wrote:
> > On 2013/12/14 03:02:30, Bernhard Bauer wrote:
> > > On 2013/12/12 19:45:24, Denis Kuznetsov wrote:
> > > > On 2013/12/09 17:51:08, Nikita Kostylev wrote:
> > > > > nit:
> > > > > if (v1) {
> > > > >   ..
> > > > > } else if (v2) {
> > > > >   ..
> > > > > } else {
> > > > >   NOTREACHED();
> > > > > }
> > > > 
> > > > Done.
> > > 
> > > Um... sorry, but
> > http://dev.chromium.org/developers/coding-style#Code_Formatting
> > > says "Don't use else after return", and all the branches here end with a
> > return
> > > statement.
> > 
> > So this should be like
> > bool result = false;
> > 
> > if (...) {
> >   ...
> >   result = true;
> > } else {
> >   ...
> >   result = false;
> > }
> > 
> > return result;
> > 
> > right?
> 
> No, the else should go, not the return :)

lgtm but please fix all comments from Bernhard.

[Bernhard Bauer, 2013-12-19 18:09:57]

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
File
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/managed/locally_managed_user_creation_controller.cc:219:
// TODO(antrim) : add password data to sync once API is ready (crbug/316168).
Nit: If you write it as crbug.com/316168, it's easier to
autolink/copy-and-paste. Optionally, prepend http:// so it will definitely be
recognized as a link.

(Edit: Case in point: Rietveld :) )

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:50: 
Nit: remove two empty lines?

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:51: } //
namespace
Nit: two spaces before //

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:71: if
(SCHEMA_PLAIN == user_schema)
Nit: Could you swap these? It reads much better that way.

https://chromiumcodereview.appspot.com/101283003/diff/180001/chrome/browser/c...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:93: }
else if (SCHEMA_SALT_HASHED == schema) {
Nit: No else if you return in the previous branch.

[Bernhard Bauer, 2013-12-20 14:47:45]

LGTM with one nit:

https://codereview.chromium.org/101283003/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://codereview.chromium.org/101283003/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:49: }//
namespace
Nit: *Two* spaces please.

[commit-bot: I haz the power, 2013-12-20 17:27:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/101283003/310001

[commit-bot: I haz the power, 2013-12-20 19:48:42]

Change committed as 242148