Add Sync datatype for shared managed user settings.

Add Sync datatype for shared managed user settings.

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

[Bernhard Bauer, 2013-11-25 18:54:12]

Please review. Thanks!

[Denis Kuznetsov (DE-MUC), 2013-11-26 10:42:30]

https://codereview.chromium.org/76333002/diff/180001/sync/protocol/managed_us...
File sync/protocol/managed_user_shared_setting_specifics.proto (right):

https://codereview.chromium.org/76333002/diff/180001/sync/protocol/managed_us...
sync/protocol/managed_user_shared_setting_specifics.proto:19: optional string
mu_id = 1;
I believe that this protobuf should act more like Manager's one (have specific
fields, rather than key-value pairs): most of the values need to change at once,
transactionally, e.g. password and version of the password.
In addition, we need similar acknowledgment flag (or, better, version) to make
sure it is safe to apply password changes on device.

[Bernhard Bauer, 2013-11-26 11:05:46]

https://codereview.chromium.org/76333002/diff/180001/sync/protocol/managed_us...
File sync/protocol/managed_user_shared_setting_specifics.proto (right):

https://codereview.chromium.org/76333002/diff/180001/sync/protocol/managed_us...
sync/protocol/managed_user_shared_setting_specifics.proto:19: optional string
mu_id = 1;
On 2013/11/26 10:42:30, Denis Kuznetsov wrote:
> I believe that this protobuf should act more like Manager's one (have specific
> fields, rather than key-value pairs): most of the values need to change at
once,
> transactionally, e.g. password and version of the password.

Well, transaction semantics are exactly what you don't get with the optimistic
concurrency model of Sync. What if the user decides to change their avatar while
the manager changes the password for that user?

Also, this way we can add additional fields without having to change the Sync
protobuf.

> In addition, we need similar acknowledgment flag (or, better, version) to make
> sure it is safe to apply password changes on device.

I think we'll be able to retire the acknowledgement flag for the MANAGED_USER
datatype once we get synchronous out-of-band user creation. If you want to have
a password version, just make that an additional shared setting.

[Denis Kuznetsov (DE-MUC), 2013-11-27 16:11:17]

On 2013/11/26 11:05:46, Bernhard Bauer wrote:
>
https://codereview.chromium.org/76333002/diff/180001/sync/protocol/managed_us...
> File sync/protocol/managed_user_shared_setting_specifics.proto (right):
> 
>
https://codereview.chromium.org/76333002/diff/180001/sync/protocol/managed_us...
> sync/protocol/managed_user_shared_setting_specifics.proto:19: optional string
> mu_id = 1;
> On 2013/11/26 10:42:30, Denis Kuznetsov wrote:
> > I believe that this protobuf should act more like Manager's one (have
specific
> > fields, rather than key-value pairs): most of the values need to change at
> once,
> > transactionally, e.g. password and version of the password.
> 
> Well, transaction semantics are exactly what you don't get with the optimistic
> concurrency model of Sync. What if the user decides to change their avatar
while
> the manager changes the password for that user?
> 
> Also, this way we can add additional fields without having to change the Sync
> protobuf.
> 
> > In addition, we need similar acknowledgment flag (or, better, version) to
make
> > sure it is safe to apply password changes on device.
> 
> I think we'll be able to retire the acknowledgement flag for the MANAGED_USER
> datatype once we get synchronous out-of-band user creation. If you want to
have
> a password version, just make that an additional shared setting.

lgtm then.

[Bernhard Bauer, 2013-11-27 17:33:59]

Adding Tim, as Nicolas is OOO.

[Bernhard Bauer, 2013-12-03 17:38:15]

After discussion with Albert and Denis, added an acknowledgement flag.

[Denis Kuznetsov (DE-MUC), 2013-12-06 15:01:13]

On 2013/12/03 17:38:15, Bernhard Bauer wrote:
> After discussion with Albert and Denis, added an acknowledgement flag.

still lgtm

[Bernhard Bauer, 2013-12-16 22:58:09]

Ping? :)

[Nicolas Zea, 2013-12-17 19:33:16]

I don't think you need to the internal_api/ changes. If you're using the
SyncableService API, those methods aren't invoked.

https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
File sync/internal_api/public/base/model_type.h (left):

https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
sync/internal_api/public/base/model_type.h:115: 
prefer that you leave these newlines in, they separate the actual types from the
helper enums

https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
File sync/protocol/managed_user_shared_setting_specifics.proto (right):

https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
sync/protocol/managed_user_shared_setting_specifics.proto:28: optional bool
acknowledged = 4 [default = false];
Once set to true at the server, will this ever become false? When does it get
reset?

[Bernhard Bauer, 2013-12-17 19:48:10]

https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
File sync/internal_api/public/base/model_type.h (left):

https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
sync/internal_api/public/base/model_type.h:115: 
On 2013/12/17 19:33:17, Nicolas Zea wrote:
> prefer that you leave these newlines in, they separate the actual types from
the
> helper enums

Done. I think that was just a merge conflict resolved badly. Thanks for catching
it!

https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
File sync/protocol/managed_user_shared_setting_specifics.proto (right):

https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
sync/protocol/managed_user_shared_setting_specifics.proto:28: optional bool
acknowledged = 4 [default = false];
On 2013/12/17 19:33:17, Nicolas Zea wrote:
> Once set to true at the server, will this ever become false? When does it get
> reset?

This will be set to false again by the client if the value is changed and the
client wants to get an acknowledgement of the change.

[Nicolas Zea, 2013-12-17 19:55:08]

On 2013/12/17 19:48:10, Bernhard Bauer wrote:
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> File sync/internal_api/public/base/model_type.h (left):
> 
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> sync/internal_api/public/base/model_type.h:115: 
> On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > prefer that you leave these newlines in, they separate the actual types from
> the
> > helper enums
> 
> Done. I think that was just a merge conflict resolved badly. Thanks for
catching
> it!
> 
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> File sync/protocol/managed_user_shared_setting_specifics.proto (right):
> 
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> sync/protocol/managed_user_shared_setting_specifics.proto:28: optional bool
> acknowledged = 4 [default = false];
> On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > Once set to true at the server, will this ever become false? When does it
get
> > reset?
> 
> This will be set to false again by the client if the value is changed and the
> client wants to get an acknowledgement of the change.

So, on the second write, the client will set the value to false, and wait for it
to transition to true again by the server? In other words, writes coming from
the client will always have the value set to false?

Is this meant to protect against races/simultaneous writes in any way? Or just
for the datatype to have an idea of when the value is synced (even if it's not
the value set by this particular client)?

[Bernhard Bauer, 2013-12-17 20:17:34]

On 2013/12/17 19:55:08, Nicolas Zea wrote:
> On 2013/12/17 19:48:10, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> > File sync/internal_api/public/base/model_type.h (left):
> > 
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> > sync/internal_api/public/base/model_type.h:115: 
> > On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > > prefer that you leave these newlines in, they separate the actual types
from
> > the
> > > helper enums
> > 
> > Done. I think that was just a merge conflict resolved badly. Thanks for
> catching
> > it!
> > 
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> > File sync/protocol/managed_user_shared_setting_specifics.proto (right):
> > 
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> > sync/protocol/managed_user_shared_setting_specifics.proto:28: optional bool
> > acknowledged = 4 [default = false];
> > On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > > Once set to true at the server, will this ever become false? When does it
> get
> > > reset?
> > 
> > This will be set to false again by the client if the value is changed and
the
> > client wants to get an acknowledgement of the change.
> 
> So, on the second write, the client will set the value to false, and wait for
it
> to transition to true again by the server? In other words, writes coming from
> the client will always have the value set to false?
> 
> Is this meant to protect against races/simultaneous writes in any way? Or just
> for the datatype to have an idea of when the value is synced (even if it's not
> the value set by this particular client)?

Denis can elaborate on the details, but basically for Chrome OS we want to be
sure that a given change (for the password) has been seen by the server and will
eventually be synced to other clients. Otherwise we might run into a situation
where we update the password locally too early.

[Denis Kuznetsov (DE-MUC), 2013-12-18 15:25:46]

On 2013/12/17 20:17:34, Bernhard Bauer wrote:
> On 2013/12/17 19:55:08, Nicolas Zea wrote:
> > On 2013/12/17 19:48:10, Bernhard Bauer wrote:
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> > > File sync/internal_api/public/base/model_type.h (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> > > sync/internal_api/public/base/model_type.h:115: 
> > > On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > > > prefer that you leave these newlines in, they separate the actual types
> from
> > > the
> > > > helper enums
> > > 
> > > Done. I think that was just a merge conflict resolved badly. Thanks for
> > catching
> > > it!
> > > 
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> > > File sync/protocol/managed_user_shared_setting_specifics.proto (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> > > sync/protocol/managed_user_shared_setting_specifics.proto:28: optional
bool
> > > acknowledged = 4 [default = false];
> > > On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > > > Once set to true at the server, will this ever become false? When does
it
> > get
> > > > reset?
> > > 
> > > This will be set to false again by the client if the value is changed and
> the
> > > client wants to get an acknowledgement of the change.
> > 
> > So, on the second write, the client will set the value to false, and wait
for
> it
> > to transition to true again by the server? In other words, writes coming
from
> > the client will always have the value set to false?
> > 
> > Is this meant to protect against races/simultaneous writes in any way? Or
just
> > for the datatype to have an idea of when the value is synced (even if it's
not
> > the value set by this particular client)?
> 
> Denis can elaborate on the details, but basically for Chrome OS we want to be
> sure that a given change (for the password) has been seen by the server and
will
> eventually be synced to other clients. Otherwise we might run into a situation
> where we update the password locally too early.

When we change password, we need update both key in device TPM/cryptohome as
well as store it on a server.
If we store it on server first, then we can always recover passwod if something
goes wrong while we're setting password to TPM/cryptohome (Supervised user's
manager can always set  password from sync to TPM).
However, if we set password to TPM first, and then something is wrong with sync,
we end up in somewhat inconsistent state, like we have user with password, but
we can't later re-create user with same password on other device.

[Nicolas Zea, 2013-12-18 21:18:03]

On 2013/12/18 15:25:46, Denis Kuznetsov wrote:
> On 2013/12/17 20:17:34, Bernhard Bauer wrote:
> > On 2013/12/17 19:55:08, Nicolas Zea wrote:
> > > On 2013/12/17 19:48:10, Bernhard Bauer wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> > > > File sync/internal_api/public/base/model_type.h (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/internal_api/public...
> > > > sync/internal_api/public/base/model_type.h:115: 
> > > > On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > > > > prefer that you leave these newlines in, they separate the actual
types
> > from
> > > > the
> > > > > helper enums
> > > > 
> > > > Done. I think that was just a merge conflict resolved badly. Thanks for
> > > catching
> > > > it!
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> > > > File sync/protocol/managed_user_shared_setting_specifics.proto (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/76333002/diff/210001/sync/protocol/managed_us...
> > > > sync/protocol/managed_user_shared_setting_specifics.proto:28: optional
> bool
> > > > acknowledged = 4 [default = false];
> > > > On 2013/12/17 19:33:17, Nicolas Zea wrote:
> > > > > Once set to true at the server, will this ever become false? When does
> it
> > > get
> > > > > reset?
> > > > 
> > > > This will be set to false again by the client if the value is changed
and
> > the
> > > > client wants to get an acknowledgement of the change.
> > > 
> > > So, on the second write, the client will set the value to false, and wait
> for
> > it
> > > to transition to true again by the server? In other words, writes coming
> from
> > > the client will always have the value set to false?
> > > 
> > > Is this meant to protect against races/simultaneous writes in any way? Or
> just
> > > for the datatype to have an idea of when the value is synced (even if it's
> not
> > > the value set by this particular client)?
> > 
> > Denis can elaborate on the details, but basically for Chrome OS we want to
be
> > sure that a given change (for the password) has been seen by the server and
> will
> > eventually be synced to other clients. Otherwise we might run into a
situation
> > where we update the password locally too early.
> 
> When we change password, we need update both key in device TPM/cryptohome as
> well as store it on a server.
> If we store it on server first, then we can always recover passwod if
something
> goes wrong while we're setting password to TPM/cryptohome (Supervised user's
> manager can always set  password from sync to TPM).
> However, if we set password to TPM first, and then something is wrong with
sync,
> we end up in somewhat inconsistent state, like we have user with password, but
> we can't later re-create user with same password on other device.

Got it, thanks. And, presumably if the possibility of another device setting a
different password is a concern, you'll be able to verify from the sync data
whether the password is the same or not, and deal with that appropriately.

LGTM

[Bernhard Bauer, 2013-12-18 21:25:04]

On 2013/12/18 21:18:03, Nicolas Zea wrote:
> Got it, thanks. And, presumably if the possibility of another device setting a
> different password is a concern, you'll be able to verify from the sync data
> whether the password is the same or not, and deal with that appropriately.

Yeah, exactly.

[commit-bot: I haz the power, 2013-12-18 21:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/76333002/230001

[commit-bot: I haz the power, 2013-12-18 22:28:03]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-18 22:56:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/76333002/230001

[commit-bot: I haz the power, 2013-12-18 23:59:47]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-19 00:53:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/76333002/250001

[commit-bot: I haz the power, 2013-12-19 03:28:09]

Change committed as 241779