Callback to function InitializeRegisteredDeviceWallpaper after cros settings can be trusted

Callback to function InitializeRegisteredDeviceWallpaper after cros settings can be trusted

InitializeWallpaper is being called in a very early stage(shell initialization). At such point, the cros settings may not be validated, so the value maybe wrong. Function InitializeWallpaper, however, depends on kAccountsPrefShowUserNamesOnSignIn to behavior correctly.

This CL made a callback to InitializeWallpaper if we detect that the settings is not yet validated.


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

[bshe, 2012-12-03 17:05:41]

Hi guys.

Could you please take a look at this CL? I have verified that the white
wallpaper for gaia login flow after enrolled is because we try to get the
kAccountsPrefShowUserNamesOnSignIn setting before the enterprise policy is
enforced. And this CL seems to fix the problem (verified on device) and seems
merge-able if necessary.

Thanks!

Biao

[bshe, 2012-12-03 17:07:08]

I will add test in a separate CL for the merit of merge.

On 2012/12/03 17:05:41, bshe wrote:
> Hi guys.
> 
> Could you please take a look at this CL? I have verified that the white
> wallpaper for gaia login flow after enrolled is because we try to get the
> kAccountsPrefShowUserNamesOnSignIn setting before the enterprise policy is
> enforced. And this CL seems to fix the problem (verified on device) and seems
> merge-able if necessary.
> 
> Thanks!
> 
> Biao

[Nikita (slow), 2012-12-04 08:25:01]

lgtm

[Mattias Nissler (ping if slow), 2012-12-04 09:31:18]

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:252: }
should this be moved to the top of the function? Otherwise you re-do everything
that happens between function entry and here (which may be OK).

Also, you might want to think about watching for changes to
kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of just
waiting until the policy is loaded from disk.

[bartfab (slow), 2012-12-04 09:59:39]

https://chromiumcodereview.appspot.com/11299304/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/11299304/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/login/wallpaper_manager.cc:252: }
On 2012/12/04 09:31:18, Mattias Nissler wrote:
> should this be moved to the top of the function? Otherwise you re-do
everything
> that happens between function entry and here (which may be OK).
> 
> Also, you might want to think about watching for changes to
> kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of just
> waiting until the policy is loaded from disk.

You will also run into trouble when settings are permanently untrusted, as
happens at certain points during enrollment. If the settings are permanently
untrusted, you get neither a CrosSettingsProvider::TRUSTED return value nor a
callback.

Mattias' suggestion should work around this. If you set up an observer for
kAccountPrefShowUserNamesOnSignIn, you will be informed when policy kicks in,
the setting gets updated and trustedness goes from PERMANENTLY_UNTRUSTED to
TRUSTED without notifying you.

[bshe, 2012-12-04 18:32:37]

Thanks for review!

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:252: }
I see. How often does the PERMANENTLY_UNTRUSTED situation happen? I saw the
RetrieveTrustedDevicePolicies uses the callback pattern as well. Sorry I may
understood it wrong, but won't it have the same issue for PERMANENTLY_UNTRUSTED
status?

On 2012/12/04 09:59:39, bartfab wrote:
> On 2012/12/04 09:31:18, Mattias Nissler wrote:
> > should this be moved to the top of the function? Otherwise you re-do
> everything
> > that happens between function entry and here (which may be OK).
> > 
> > Also, you might want to think about watching for changes to
> > kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of just
> > waiting until the policy is loaded from disk.
> 
> You will also run into trouble when settings are permanently untrusted, as
> happens at certain points during enrollment. If the settings are permanently
> untrusted, you get neither a CrosSettingsProvider::TRUSTED return value nor a
> callback.
> 
> Mattias' suggestion should work around this. If you set up an observer for
> kAccountPrefShowUserNamesOnSignIn, you will be informed when policy kicks in,
> the setting gets updated and trustedness goes from PERMANENTLY_UNTRUSTED to
> TRUSTED without notifying you.

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:252: }
Some of the code path is not depend on the setting. If I move this to the top of
the function, these code path will only be executed once the value is ready. I
was hoping to unblock these code paths so I add it here. I should probably wrap
everything in the else block after line 252 to a new function and callback the
new function so I can avoid redo everything above. But since this is targeting
as m23, and some of the code has modified since m23. I would really like to
avoid merge conflict if possible and increase the chance to be merged. And redo
everything above doesn't look like a big overhead either. So probably keep it
this way for now? Happy to do the change if you disagree. 

For add observers, it seems that we already observe it here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo...

So if the enterprise administrator flip the setting on the server, it should be
observed by that code and the "correct" wallpaper should be set by it as well. I
believe it is not necessary to add another observer for the setting again on
TOT?

There is one problem though. The code that guarantee wallpaper is set correctly
(https://chromiumcodereview.appspot.com/11190022/) is not in M23. So on m23, we
will have the correct wallpaper not being set issue. eg. when switch to GAIA
signin form, we stuck to previous wallpaper instead of default wallpaper. This
should be acceptable in M23 as far as I know.  

On 2012/12/04 09:31:18, Mattias Nissler wrote:
> should this be moved to the top of the function? Otherwise you re-do
everything
> that happens between function entry and here (which may be OK).
> 
> Also, you might want to think about watching for changes to
> kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of just
> waiting until the policy is loaded from disk.

[bshe, 2012-12-04 20:13:13]

Please see patchset 2 for proposed solution for permanently untrusted case.

It is based on two assumptions which I am not 100% sure it is right.

PTAL.

Thanks!

https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:247: if
(CrosSettingsProvider::TEMPORARILY_UNTRUSTED ==
girard@ and I came up with this to take care of the PERMANENTLY_UNTRUSTED
situation. If the setting is permanently untrusted, we should just proceed with
whatever value we get from settings or default to show user pod if we failed to
get value out from setting.

We assumed that it is OK to use the value in settings when status is
PERMANENTLY_UNTRUSTED and that value wont change to something else when we
initialize login webui (and try to get the same value in setttings again).

[bshe, 2012-12-05 14:58:35]

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/wallpaper_manager.cc:252: }
bartfab@ Sorry. Please ignore my comment about RetrieveTrustedDevicePolicies. It
looks like do nothing for PERMANENTLY_UNTRUSTED is intended for that function.

On 2012/12/04 18:32:37, bshe wrote:
> I see. How often does the PERMANENTLY_UNTRUSTED situation happen? I saw the
> RetrieveTrustedDevicePolicies uses the callback pattern as well. Sorry I may
> understood it wrong, but won't it have the same issue for
PERMANENTLY_UNTRUSTED
> status?
> 
> On 2012/12/04 09:59:39, bartfab wrote:
> > On 2012/12/04 09:31:18, Mattias Nissler wrote:
> > > should this be moved to the top of the function? Otherwise you re-do
> > everything
> > > that happens between function entry and here (which may be OK).
> > > 
> > > Also, you might want to think about watching for changes to
> > > kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of
just
> > > waiting until the policy is loaded from disk.
> > 
> > You will also run into trouble when settings are permanently untrusted, as
> > happens at certain points during enrollment. If the settings are permanently
> > untrusted, you get neither a CrosSettingsProvider::TRUSTED return value nor
a
> > callback.
> > 
> > Mattias' suggestion should work around this. If you set up an observer for
> > kAccountPrefShowUserNamesOnSignIn, you will be informed when policy kicks
in,
> > the setting gets updated and trustedness goes from PERMANENTLY_UNTRUSTED to
> > TRUSTED without notifying you.
>

[bshe, 2012-12-06 13:48:26]

On 2012/12/05 14:58:35, bshe wrote:
>
https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> 
>
https://codereview.chromium.org/11299304/diff/1/chrome/browser/chromeos/login...
> chrome/browser/chromeos/login/wallpaper_manager.cc:252: }
> bartfab@ Sorry. Please ignore my comment about RetrieveTrustedDevicePolicies.
It
> looks like do nothing for PERMANENTLY_UNTRUSTED is intended for that function.
> 
> On 2012/12/04 18:32:37, bshe wrote:
> > I see. How often does the PERMANENTLY_UNTRUSTED situation happen? I saw the
> > RetrieveTrustedDevicePolicies uses the callback pattern as well. Sorry I may
> > understood it wrong, but won't it have the same issue for
> PERMANENTLY_UNTRUSTED
> > status?
> > 
> > On 2012/12/04 09:59:39, bartfab wrote:
> > > On 2012/12/04 09:31:18, Mattias Nissler wrote:
> > > > should this be moved to the top of the function? Otherwise you re-do
> > > everything
> > > > that happens between function entry and here (which may be OK).
> > > > 
> > > > Also, you might want to think about watching for changes to
> > > > kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of
> just
> > > > waiting until the policy is loaded from disk.
> > > 
> > > You will also run into trouble when settings are permanently untrusted, as
> > > happens at certain points during enrollment. If the settings are
permanently
> > > untrusted, you get neither a CrosSettingsProvider::TRUSTED return value
nor
> a
> > > callback.
> > > 
> > > Mattias' suggestion should work around this. If you set up an observer for
> > > kAccountPrefShowUserNamesOnSignIn, you will be informed when policy kicks
> in,
> > > the setting gets updated and trustedness goes from PERMANENTLY_UNTRUSTED
to
> > > TRUSTED without notifying you.
> >

Friendly ping. Thanks.

[bartfab (slow), 2012-12-06 13:57:01]

Sorry about the delay and thanks for the ping.

https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:247: if
(CrosSettingsProvider::TEMPORARILY_UNTRUSTED ==
On 2012/12/04 20:13:14, bshe wrote:
> girard@ and I came up with this to take care of the PERMANENTLY_UNTRUSTED
> situation. If the setting is permanently untrusted, we should just proceed
with
> whatever value we get from settings or default to show user pod if we failed
to
> get value out from setting.
> 
> We assumed that it is OK to use the value in settings when status is
> PERMANENTLY_UNTRUSTED and that value wont change to something else when we
> initialize login webui (and try to get the same value in setttings again).

Normally, PERMANENTLY_UNTRUSTED means that we are missing some crucial bit of
the crypto chain (read: the owner key) and will never be able to verify settings
- hence, permanently untrusted.

However, during enrollment, the situation is different. When you first boot up a
brand new device, its settings are PERMANENTLY_UNTRUSTED because there is no
owner key and the policy system cannot verify any settings. Once you go through
enrollment though, an owner key suddenly appears and the status jumps from
PERMANENTLY_UNTRUSTED to TRUSTED.

You may choose to just not handle this. After a reboot, the device will behave
as expected, going through the TEMPORARILY_UNTRUSTED and TRUSTED states only. Or
you can handle it gracefully: Register an observer for the wallpaper pref. When
the value changes, you should fire off another trustedness check. If the device
has just gone through enrollment, you will now get trusted values, even if they
were PERMANENTLY_UNTRUSTED before.

My CL I pointed you at in our discussion does exactly this. Note though that
right now, the settings will still appear PERMANENTLY_UNTRUSTED when that
observer fires. This is a bug and I will be landing CL 11421099 RSN to fix it.

[bshe, 2012-12-06 15:31:22]

On 2012/12/06 13:57:01, bartfab wrote:
> Sorry about the delay and thanks for the ping.
> 
>
https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> 
>
https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
> chrome/browser/chromeos/login/wallpaper_manager.cc:247: if
> (CrosSettingsProvider::TEMPORARILY_UNTRUSTED ==
> On 2012/12/04 20:13:14, bshe wrote:
> > girard@ and I came up with this to take care of the PERMANENTLY_UNTRUSTED
> > situation. If the setting is permanently untrusted, we should just proceed
> with
> > whatever value we get from settings or default to show user pod if we failed
> to
> > get value out from setting.
> > 
> > We assumed that it is OK to use the value in settings when status is
> > PERMANENTLY_UNTRUSTED and that value wont change to something else when we
> > initialize login webui (and try to get the same value in setttings again).
> 
> Normally, PERMANENTLY_UNTRUSTED means that we are missing some crucial bit of
> the crypto chain (read: the owner key) and will never be able to verify
settings
> - hence, permanently untrusted.
> 
> However, during enrollment, the situation is different. When you first boot up
a
> brand new device, its settings are PERMANENTLY_UNTRUSTED because there is no
> owner key and the policy system cannot verify any settings. Once you go
through
> enrollment though, an owner key suddenly appears and the status jumps from
> PERMANENTLY_UNTRUSTED to TRUSTED.
> 
> You may choose to just not handle this. After a reboot, the device will behave
> as expected, going through the TEMPORARILY_UNTRUSTED and TRUSTED states only.
Or
> you can handle it gracefully: Register an observer for the wallpaper pref.
When
> the value changes, you should fire off another trustedness check. If the
device
> has just gone through enrollment, you will now get trusted values, even if
they
> were PERMANENTLY_UNTRUSTED before.
> 
> My CL I pointed you at in our discussion does exactly this. Note though that
> right now, the settings will still appear PERMANENTLY_UNTRUSTED when that
> observer fires. This is a bug and I will be landing CL 11421099 RSN to fix it.

Status may change from PERMANENTLY_UNTRUSTED (P_U) to TRUSTED (T) seems
contradict with comment for P_U here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/se...
you may probably want to modify the comment as well.

I think this patch is still fine if the only case a device changing from P_U to
T is during the enrollment process (There maybe other cases, please correct me
if I am wrong).
The bug I am trying to fix only happens after user "enrolled" and logged in. If
user have enrolled, we are going through the normal process (T_U to T). And this
CL seems fix the problem.
Before enrollment, we previously have no issue. So the original code path works
fine. My patch did not affect that case since the status returned is P_U before
enrollment. So it just skip
adding callback and proceed to the original code path.

I agree it is nicer to have an observer to observe the settings. But it looks
like we are adding some code complexity for a one time experience (enrollment)
which
previously had no wallpaper issue. (Again, if there are other cases that
settings may change, then my argument is not valid). Also after M23, we are no
longer rely on that setting to set wallpaper.
So it would be really nice to avoid using observer if possible.

[bartfab (slow), 2012-12-07 17:30:49]

On 2012/12/06 15:31:22, bshe wrote:
> On 2012/12/06 13:57:01, bartfab wrote:
> > Sorry about the delay and thanks for the ping.
> > 
> >
>
https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
> > File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/lo...
> > chrome/browser/chromeos/login/wallpaper_manager.cc:247: if
> > (CrosSettingsProvider::TEMPORARILY_UNTRUSTED ==
> > On 2012/12/04 20:13:14, bshe wrote:
> > > girard@ and I came up with this to take care of the PERMANENTLY_UNTRUSTED
> > > situation. If the setting is permanently untrusted, we should just proceed
> > with
> > > whatever value we get from settings or default to show user pod if we
failed
> > to
> > > get value out from setting.
> > > 
> > > We assumed that it is OK to use the value in settings when status is
> > > PERMANENTLY_UNTRUSTED and that value wont change to something else when we
> > > initialize login webui (and try to get the same value in setttings again).
> > 
> > Normally, PERMANENTLY_UNTRUSTED means that we are missing some crucial bit
of
> > the crypto chain (read: the owner key) and will never be able to verify
> settings
> > - hence, permanently untrusted.
> > 
> > However, during enrollment, the situation is different. When you first boot
up
> a
> > brand new device, its settings are PERMANENTLY_UNTRUSTED because there is no
> > owner key and the policy system cannot verify any settings. Once you go
> through
> > enrollment though, an owner key suddenly appears and the status jumps from
> > PERMANENTLY_UNTRUSTED to TRUSTED.
> > 
> > You may choose to just not handle this. After a reboot, the device will
behave
> > as expected, going through the TEMPORARILY_UNTRUSTED and TRUSTED states
only.
> Or
> > you can handle it gracefully: Register an observer for the wallpaper pref.
> When
> > the value changes, you should fire off another trustedness check. If the
> device
> > has just gone through enrollment, you will now get trusted values, even if
> they
> > were PERMANENTLY_UNTRUSTED before.
> > 
> > My CL I pointed you at in our discussion does exactly this. Note though that
> > right now, the settings will still appear PERMANENTLY_UNTRUSTED when that
> > observer fires. This is a bug and I will be landing CL 11421099 RSN to fix
it.
> 
> Status may change from PERMANENTLY_UNTRUSTED (P_U) to TRUSTED (T) seems
> contradict with comment for P_U here:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/se...
> you may probably want to modify the comment as well.

Yep. That comment is wrong. During regular operation, the transition PU -> T
will never happen. But during enrollment, it will.

> I think this patch is still fine if the only case a device changing from P_U
to
> T is during the enrollment process (There maybe other cases, please correct me
> if I am wrong).

None that I am aware of.

> The bug I am trying to fix only happens after user "enrolled" and logged in.
If
> user have enrolled, we are going through the normal process (T_U to T). And
this
> CL seems fix the problem.

If the device is already enrolled, you do not have to worry about the PU -> T
transition, correct.

> Before enrollment, we previously have no issue. So the original code path
works
> fine. My patch did not affect that case since the status returned is P_U
before
> enrollment. So it just skip
> adding callback and proceed to the original code path.

I see that your code does a return when the status is PU. Is that what you meant
to do? Should it not just proceed with reading the value and using it, even if
the value is untrusted?

> I agree it is nicer to have an observer to observe the settings. But it looks
> like we are adding some code complexity for a one time experience (enrollment)
> which
> previously had no wallpaper issue. (Again, if there are other cases that
> settings may change, then my argument is not valid). Also after M23, we are no
> longer rely on that setting to set wallpaper.

Ship it, I would say. It is reasonable to expect that users will restart the
device after enrollment to get the full experience.

> So it would be really nice to avoid using observer if possible.

Besides enrollment, the observer would also be useful/necessary if you expected
the value to change at runtime. I am not sure what value you are reading here
and whether it can change, so I cannot judge that.

[bshe1, 2012-12-10 15:34:11]

Thanks for comments.

On Fri, Dec 7, 2012 at 12:30 PM, <bartfab@chromium.org> wrote:

> On 2012/12/06 15:31:22, bshe wrote:
>
>> On 2012/12/06 13:57:01, bartfab wrote:
>> > Sorry about the delay and thanks for the ping.
>> >
>> >
>>
>
> https://codereview.chromium.**org/11299304/diff/4002/chrome/**
>
browser/chromeos/login/**wallpaper_manager.cc<https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/login/wallpaper_manager.cc>
>
>> > File chrome/browser/chromeos/login/**wallpaper_manager.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/11299304/diff/4002/chrome/**
>
browser/chromeos/login/**wallpaper_manager.cc#**newcode247<https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/login/wallpaper_manager.cc#newcode247>
>
>> > chrome/browser/chromeos/login/**wallpaper_manager.cc:247: if
>> > (CrosSettingsProvider::**TEMPORARILY_UNTRUSTED ==
>> > On 2012/12/04 20:13:14, bshe wrote:
>> > > girard@ and I came up with this to take care of the
>> PERMANENTLY_UNTRUSTED
>> > > situation. If the setting is permanently untrusted, we should just
>> proceed
>> > with
>> > > whatever value we get from settings or default to show user pod if we
>>
> failed
>
>> > to
>> > > get value out from setting.
>> > >
>> > > We assumed that it is OK to use the value in settings when status is
>> > > PERMANENTLY_UNTRUSTED and that value wont change to something else
>> when we
>> > > initialize login webui (and try to get the same value in setttings
>> again).
>> >
>> > Normally, PERMANENTLY_UNTRUSTED means that we are missing some crucial
>> bit
>>
> of
>
>> > the crypto chain (read: the owner key) and will never be able to verify
>> settings
>> > - hence, permanently untrusted.
>> >
>> > However, during enrollment, the situation is different. When you first
>> boot
>>
> up
>
>> a
>> > brand new device, its settings are PERMANENTLY_UNTRUSTED because there
>> is no
>> > owner key and the policy system cannot verify any settings. Once you go
>> through
>> > enrollment though, an owner key suddenly appears and the status jumps
>> from
>> > PERMANENTLY_UNTRUSTED to TRUSTED.
>> >
>> > You may choose to just not handle this. After a reboot, the device will
>>
> behave
>
>> > as expected, going through the TEMPORARILY_UNTRUSTED and TRUSTED states
>>
> only.
>
>> Or
>> > you can handle it gracefully: Register an observer for the wallpaper
>> pref.
>> When
>> > the value changes, you should fire off another trustedness check. If the
>> device
>> > has just gone through enrollment, you will now get trusted values, even
>> if
>> they
>> > were PERMANENTLY_UNTRUSTED before.
>> >
>> > My CL I pointed you at in our discussion does exactly this. Note though
>> that
>> > right now, the settings will still appear PERMANENTLY_UNTRUSTED when
>> that
>> > observer fires. This is a bug and I will be landing CL 11421099 RSN to
>> fix
>>
> it.
>
>  Status may change from PERMANENTLY_UNTRUSTED (P_U) to TRUSTED (T) seems
>> contradict with comment for P_U here:
>>
>
> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> chrome/browser/chromeos/**settings/cros_settings_**
>
provider.h&exact_package=**chromium&q=PERMANENTLY_**UNTRUSTED&type=cs&l=35<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/settings/cros_settings_provider.h&exact_package=chromium&q=PERMANENTLY_UNTRUSTED&type=cs&l=35>
>
>> you may probably want to modify the comment as well.
>>
>
> Yep. That comment is wrong. During regular operation, the transition PU ->
> T
> will never happen. But during enrollment, it will.
>
>
>  I think this patch is still fine if the only case a device changing from
>> P_U
>>
> to
>
>> T is during the enrollment process (There maybe other cases, please
>> correct me
>> if I am wrong).
>>
>
> None that I am aware of.
>
>
>  The bug I am trying to fix only happens after user "enrolled" and logged
>> in.
>>
> If
>
>> user have enrolled, we are going through the normal process (T_U to T).
>> And
>>
> this
>
>> CL seems fix the problem.
>>
>
> If the device is already enrolled, you do not have to worry about the PU
> -> T
> transition, correct.
>
>
>  Before enrollment, we previously have no issue. So the original code path
>>
> works
>
>> fine. My patch did not affect that case since the status returned is P_U
>>
> before
>
>> enrollment. So it just skip
>> adding callback and proceed to the original code path.
>>
>
> I see that your code does a return when the status is PU. Is that what you
> meant
> to do? Should it not just proceed with reading the value and using it,
> even if
> the value is untrusted?

That was my previous patch. Now the new patch only return if the status is
TU.
For PU, it will just proceed.


>
>
>  I agree it is nicer to have an observer to observe the settings. But it
>> looks
>> like we are adding some code complexity for a one time experience
>> (enrollment)
>> which
>> previously had no wallpaper issue. (Again, if there are other cases that
>> settings may change, then my argument is not valid). Also after M23, we
>> are no
>> longer rely on that setting to set wallpaper.
>>
>
> Ship it, I would say. It is reasonable to expect that users will restart
> the
> device after enrollment to get the full experience.
>
>
>  So it would be really nice to avoid using observer if possible.
>>
>
> Besides enrollment, the observer would also be useful/necessary if you
> expected
> the value to change at runtime. I am not sure what value you are reading
> here
> and whether it can change, so I cannot judge that.
>
So the value will only be used before showing login screen. It decides if
we should
show user pod or not at the login screen. If the device is not at login
screen when
the value change, we dont care. If the device is at login screen and the
value change,
existing_user_controller.cc already observer it here:
http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
chrome/browser/chromeos/login/**existing_user_controller.cc&**
exact_package=chromium&q=**ExistingUserCont&type=cs&l=170<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/login/existing_user_controller.cc&exact_package=chromium&q=ExistingUserCont&type=cs&l=170>
So I would say we probably dont need to observe it from here at least for


>
>
https://chromiumcodereview.**appspot.com/11299304/<https://chromiumcodereview...
>

[bartfab (slow), 2012-12-10 17:49:13]

On 12/10/2012 04:34 PM, Biao She wrote:
> Thanks for comments.
>
> On Fri, Dec 7, 2012 at 12:30 PM, <bartfab@chromium.org> wrote:
>
>> On 2012/12/06 15:31:22, bshe wrote:
>>
>>> On 2012/12/06 13:57:01, bartfab wrote:
>>>> Sorry about the delay and thanks for the ping.
>>>>
>>>>
>>>
>>
>> https://codereview.chromium.**org/11299304/diff/4002/chrome/**
>>
browser/chromeos/login/**wallpaper_manager.cc<https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/login/wallpaper_manager.cc>
>>
>>>> File chrome/browser/chromeos/login/**wallpaper_manager.cc (right):
>>>>
>>>>
>>>
>>
>> https://codereview.chromium.**org/11299304/diff/4002/chrome/**
>>
browser/chromeos/login/**wallpaper_manager.cc#**newcode247<https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/login/wallpaper_manager.cc#newcode247>
>>
>>>> chrome/browser/chromeos/login/**wallpaper_manager.cc:247: if
>>>> (CrosSettingsProvider::**TEMPORARILY_UNTRUSTED ==
>>>> On 2012/12/04 20:13:14, bshe wrote:
>>>>> girard@ and I came up with this to take care of the
>>> PERMANENTLY_UNTRUSTED
>>>>> situation. If the setting is permanently untrusted, we should just
>>> proceed
>>>> with
>>>>> whatever value we get from settings or default to show user pod if we
>>>
>> failed
>>
>>>> to
>>>>> get value out from setting.
>>>>>
>>>>> We assumed that it is OK to use the value in settings when status is
>>>>> PERMANENTLY_UNTRUSTED and that value wont change to something else
>>> when we
>>>>> initialize login webui (and try to get the same value in setttings
>>> again).
>>>>
>>>> Normally, PERMANENTLY_UNTRUSTED means that we are missing some crucial
>>> bit
>>>
>> of
>>
>>>> the crypto chain (read: the owner key) and will never be able to verify
>>> settings
>>>> - hence, permanently untrusted.
>>>>
>>>> However, during enrollment, the situation is different. When you first
>>> boot
>>>
>> up
>>
>>> a
>>>> brand new device, its settings are PERMANENTLY_UNTRUSTED because there
>>> is no
>>>> owner key and the policy system cannot verify any settings. Once you go
>>> through
>>>> enrollment though, an owner key suddenly appears and the status jumps
>>> from
>>>> PERMANENTLY_UNTRUSTED to TRUSTED.
>>>>
>>>> You may choose to just not handle this. After a reboot, the device will
>>>
>> behave
>>
>>>> as expected, going through the TEMPORARILY_UNTRUSTED and TRUSTED states
>>>
>> only.
>>
>>> Or
>>>> you can handle it gracefully: Register an observer for the wallpaper
>>> pref.
>>> When
>>>> the value changes, you should fire off another trustedness check. If the
>>> device
>>>> has just gone through enrollment, you will now get trusted values, even
>>> if
>>> they
>>>> were PERMANENTLY_UNTRUSTED before.
>>>>
>>>> My CL I pointed you at in our discussion does exactly this. Note though
>>> that
>>>> right now, the settings will still appear PERMANENTLY_UNTRUSTED when
>>> that
>>>> observer fires. This is a bug and I will be landing CL 11421099 RSN to
>>> fix
>>>
>> it.
>>
>>   Status may change from PERMANENTLY_UNTRUSTED (P_U) to TRUSTED (T) seems
>>> contradict with comment for P_U here:
>>>
>>
>> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
>> chrome/browser/chromeos/**settings/cros_settings_**
>>
provider.h&exact_package=**chromium&q=PERMANENTLY_**UNTRUSTED&type=cs&l=35<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/settings/cros_settings_provider.h&exact_package=chromium&q=PERMANENTLY_UNTRUSTED&type=cs&l=35>
>>
>>> you may probably want to modify the comment as well.
>>>
>>
>> Yep. That comment is wrong. During regular operation, the transition PU ->
>> T
>> will never happen. But during enrollment, it will.
>>
>>
>>   I think this patch is still fine if the only case a device changing from
>>> P_U
>>>
>> to
>>
>>> T is during the enrollment process (There maybe other cases, please
>>> correct me
>>> if I am wrong).
>>>
>>
>> None that I am aware of.
>>
>>
>>   The bug I am trying to fix only happens after user "enrolled" and logged
>>> in.
>>>
>> If
>>
>>> user have enrolled, we are going through the normal process (T_U to T).
>>> And
>>>
>> this
>>
>>> CL seems fix the problem.
>>>
>>
>> If the device is already enrolled, you do not have to worry about the PU
>> -> T
>> transition, correct.
>>
>>
>>   Before enrollment, we previously have no issue. So the original code path
>>>
>> works
>>
>>> fine. My patch did not affect that case since the status returned is P_U
>>>
>> before
>>
>>> enrollment. So it just skip
>>> adding callback and proceed to the original code path.
>>>
>>
>> I see that your code does a return when the status is PU. Is that what you
>> meant
>> to do? Should it not just proceed with reading the value and using it,
>> even if
>> the value is untrusted?
>
> That was my previous patch. Now the new patch only return if the status is
> TU.
> For PU, it will just proceed.

Sorry, I misread the code. Of course, you are right and the logic is 
correct.

>
>
>>
>>
>>   I agree it is nicer to have an observer to observe the settings. But it
>>> looks
>>> like we are adding some code complexity for a one time experience
>>> (enrollment)
>>> which
>>> previously had no wallpaper issue. (Again, if there are other cases that
>>> settings may change, then my argument is not valid). Also after M23, we
>>> are no
>>> longer rely on that setting to set wallpaper.
>>>
>>
>> Ship it, I would say. It is reasonable to expect that users will restart
>> the
>> device after enrollment to get the full experience.
>>
>>
>>   So it would be really nice to avoid using observer if possible.
>>>
>>
>> Besides enrollment, the observer would also be useful/necessary if you
>> expected
>> the value to change at runtime. I am not sure what value you are reading
>> here
>> and whether it can change, so I cannot judge that.
>>
> So the value will only be used before showing login screen. It decides if
> we should
> show user pod or not at the login screen. If the device is not at login
> screen when
> the value change, we dont care. If the device is at login screen and the
> value change,
> existing_user_controller.cc already observer it here:
> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> chrome/browser/chromeos/login/**existing_user_controller.cc&**
>
exact_package=chromium&q=**ExistingUserCont&type=cs&l=170<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/login/existing_user_controller.cc&exact_package=chromium&q=ExistingUserCont&type=cs&l=170>
> So I would say we probably dont need to observe it from here at least for

Yes, the login screen observes the value correctly. I have toggled pods 
on and off through policy many times and seen the login screen refresh 
properly.

>
>
>>
>>
https://chromiumcodereview.**appspot.com/11299304/<https://chromiumcodereview...
>>
>

[bshe, 2012-12-10 18:05:24]

Thanks for reply! Do you have any other comment or concern? If not, would you
mind to lgtm this CL so I know you are OK with it to upload to TOT?

Also, mnissler@, would you mind to take another look?

On 2012/12/10 17:49:13, bartfab wrote:
> On 12/10/2012 04:34 PM, Biao She wrote:
> > Thanks for comments.
> >
> > On Fri, Dec 7, 2012 at 12:30 PM, <mailto:bartfab@chromium.org> wrote:
> >
> >> On 2012/12/06 15:31:22, bshe wrote:
> >>
> >>> On 2012/12/06 13:57:01, bartfab wrote:
> >>>> Sorry about the delay and thanks for the ping.
> >>>>
> >>>>
> >>>
> >>
> >> https://codereview.chromium.**org/11299304/diff/4002/chrome/**
> >>
>
browser/chromeos/login/**wallpaper_manager.cc<https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/login/wallpaper_manager.cc>
> >>
> >>>> File chrome/browser/chromeos/login/**wallpaper_manager.cc (right):
> >>>>
> >>>>
> >>>
> >>
> >> https://codereview.chromium.**org/11299304/diff/4002/chrome/**
> >>
>
browser/chromeos/login/**wallpaper_manager.cc#**newcode247<https://codereview.chromium.org/11299304/diff/4002/chrome/browser/chromeos/login/wallpaper_manager.cc#newcode247>
> >>
> >>>> chrome/browser/chromeos/login/**wallpaper_manager.cc:247: if
> >>>> (CrosSettingsProvider::**TEMPORARILY_UNTRUSTED ==
> >>>> On 2012/12/04 20:13:14, bshe wrote:
> >>>>> girard@ and I came up with this to take care of the
> >>> PERMANENTLY_UNTRUSTED
> >>>>> situation. If the setting is permanently untrusted, we should just
> >>> proceed
> >>>> with
> >>>>> whatever value we get from settings or default to show user pod if we
> >>>
> >> failed
> >>
> >>>> to
> >>>>> get value out from setting.
> >>>>>
> >>>>> We assumed that it is OK to use the value in settings when status is
> >>>>> PERMANENTLY_UNTRUSTED and that value wont change to something else
> >>> when we
> >>>>> initialize login webui (and try to get the same value in setttings
> >>> again).
> >>>>
> >>>> Normally, PERMANENTLY_UNTRUSTED means that we are missing some crucial
> >>> bit
> >>>
> >> of
> >>
> >>>> the crypto chain (read: the owner key) and will never be able to verify
> >>> settings
> >>>> - hence, permanently untrusted.
> >>>>
> >>>> However, during enrollment, the situation is different. When you first
> >>> boot
> >>>
> >> up
> >>
> >>> a
> >>>> brand new device, its settings are PERMANENTLY_UNTRUSTED because there
> >>> is no
> >>>> owner key and the policy system cannot verify any settings. Once you go
> >>> through
> >>>> enrollment though, an owner key suddenly appears and the status jumps
> >>> from
> >>>> PERMANENTLY_UNTRUSTED to TRUSTED.
> >>>>
> >>>> You may choose to just not handle this. After a reboot, the device will
> >>>
> >> behave
> >>
> >>>> as expected, going through the TEMPORARILY_UNTRUSTED and TRUSTED states
> >>>
> >> only.
> >>
> >>> Or
> >>>> you can handle it gracefully: Register an observer for the wallpaper
> >>> pref.
> >>> When
> >>>> the value changes, you should fire off another trustedness check. If the
> >>> device
> >>>> has just gone through enrollment, you will now get trusted values, even
> >>> if
> >>> they
> >>>> were PERMANENTLY_UNTRUSTED before.
> >>>>
> >>>> My CL I pointed you at in our discussion does exactly this. Note though
> >>> that
> >>>> right now, the settings will still appear PERMANENTLY_UNTRUSTED when
> >>> that
> >>>> observer fires. This is a bug and I will be landing CL 11421099 RSN to
> >>> fix
> >>>
> >> it.
> >>
> >>   Status may change from PERMANENTLY_UNTRUSTED (P_U) to TRUSTED (T) seems
> >>> contradict with comment for P_U here:
> >>>
> >>
> >> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> >> chrome/browser/chromeos/**settings/cros_settings_**
> >>
>
provider.h&exact_package=**chromium&q=PERMANENTLY_**UNTRUSTED&type=cs&l=35<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/settings/cros_settings_provider.h&exact_package=chromium&q=PERMANENTLY_UNTRUSTED&type=cs&l=35>
> >>
> >>> you may probably want to modify the comment as well.
> >>>
> >>
> >> Yep. That comment is wrong. During regular operation, the transition PU ->
> >> T
> >> will never happen. But during enrollment, it will.
> >>
> >>
> >>   I think this patch is still fine if the only case a device changing from
> >>> P_U
> >>>
> >> to
> >>
> >>> T is during the enrollment process (There maybe other cases, please
> >>> correct me
> >>> if I am wrong).
> >>>
> >>
> >> None that I am aware of.
> >>
> >>
> >>   The bug I am trying to fix only happens after user "enrolled" and logged
> >>> in.
> >>>
> >> If
> >>
> >>> user have enrolled, we are going through the normal process (T_U to T).
> >>> And
> >>>
> >> this
> >>
> >>> CL seems fix the problem.
> >>>
> >>
> >> If the device is already enrolled, you do not have to worry about the PU
> >> -> T
> >> transition, correct.
> >>
> >>
> >>   Before enrollment, we previously have no issue. So the original code path
> >>>
> >> works
> >>
> >>> fine. My patch did not affect that case since the status returned is P_U
> >>>
> >> before
> >>
> >>> enrollment. So it just skip
> >>> adding callback and proceed to the original code path.
> >>>
> >>
> >> I see that your code does a return when the status is PU. Is that what you
> >> meant
> >> to do? Should it not just proceed with reading the value and using it,
> >> even if
> >> the value is untrusted?
> >
> > That was my previous patch. Now the new patch only return if the status is
> > TU.
> > For PU, it will just proceed.
> 
> Sorry, I misread the code. Of course, you are right and the logic is 
> correct.
> 
> >
> >
> >>
> >>
> >>   I agree it is nicer to have an observer to observe the settings. But it
> >>> looks
> >>> like we are adding some code complexity for a one time experience
> >>> (enrollment)
> >>> which
> >>> previously had no wallpaper issue. (Again, if there are other cases that
> >>> settings may change, then my argument is not valid). Also after M23, we
> >>> are no
> >>> longer rely on that setting to set wallpaper.
> >>>
> >>
> >> Ship it, I would say. It is reasonable to expect that users will restart
> >> the
> >> device after enrollment to get the full experience.
> >>
> >>
> >>   So it would be really nice to avoid using observer if possible.
> >>>
> >>
> >> Besides enrollment, the observer would also be useful/necessary if you
> >> expected
> >> the value to change at runtime. I am not sure what value you are reading
> >> here
> >> and whether it can change, so I cannot judge that.
> >>
> > So the value will only be used before showing login screen. It decides if
> > we should
> > show user pod or not at the login screen. If the device is not at login
> > screen when
> > the value change, we dont care. If the device is at login screen and the
> > value change,
> > existing_user_controller.cc already observer it here:
> > http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> > chrome/browser/chromeos/login/**existing_user_controller.cc&**
> >
>
exact_package=chromium&q=**ExistingUserCont&type=cs&l=170<http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/login/existing_user_controller.cc&exact_package=chromium&q=ExistingUserCont&type=cs&l=170>
> > So I would say we probably dont need to observe it from here at least for
> 
> Yes, the login screen observes the value correctly. I have toggled pods 
> on and off through policy many times and seen the login screen refresh 
> properly.
> 
> >
> >
> >>
> >>
>
https://chromiumcodereview.**appspot.com/11299304/%3Chttps://chromiumcoderevi...>
> >>
> >
>

[bartfab (slow), 2012-12-10 18:26:56]

LGTM. In case you missed it, Mattias had posted a comment that you may still
want to consider:

"should this be moved to the top of the function? Otherwise you re-do everything
that happens between function entry and here (which may be OK)."

[bshe, 2012-12-10 20:08:31]

Thanks for pointing it out.

I have add a new function to addree Mattias's comment. mnissler@ Could you
please take a look at patchset 3. Thanks!


On 2012/12/10 18:26:56, bartfab wrote:
> LGTM. In case you missed it, Mattias had posted a comment that you may still
> want to consider:
> 
> "should this be moved to the top of the function? Otherwise you re-do
everything
> that happens between function entry and here (which may be OK)."

[Mattias Nissler (ping if slow), 2012-12-11 00:19:40]

LGTM

[Nikita (slow), 2012-12-11 13:16:38]

lgtm

https://codereview.chromium.org/11299304/diff/16001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/16001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:646:
CrosSettings::Get()->PrepareTrustedValues(
nit: either move 2 spaces to the left or 2 spaces to the right

https://codereview.chromium.org/11299304/diff/16001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:649: return;
nit: 2 spaces indent here.

[bshe, 2012-12-11 14:36:17]

Done. Thanks!

https://codereview.chromium.org/11299304/diff/16001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/11299304/diff/16001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:646:
CrosSettings::Get()->PrepareTrustedValues(
On 2012/12/11 13:16:39, Nikita Kostylev wrote:
> nit: either move 2 spaces to the left or 2 spaces to the right

Done. Moved 2 spaces to the left.

https://codereview.chromium.org/11299304/diff/16001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:649: return;
On 2012/12/11 13:16:39, Nikita Kostylev wrote:
> nit: 2 spaces indent here.

Done.