[login_manager] Code to add the owner to the whitelist in a device policy

[login_manager] Code to add the owner to the whitelist in a device policy

Today, every time the owner logs in, we ensure he is on the whitelist and
that we have him recorded in the signed settings store as the owner.

Add code that can do the same thing for a device policy.

BUG=13842
TEST=unit tests.

Change-Id: I0073c7b0216879fb021f2319906ffd51bd54233f

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ac98340

[Chris Masone, 2011-04-08 00:05:53]

this CL isn't as big as it looks.

[gauravsh, 2011-04-08 04:58:49]

http://codereview.chromium.org/6815021/diff/1/Makefile
File Makefile (right):

http://codereview.chromium.org/6815021/diff/1/Makefile#newcode26
Makefile:26: $(PROTO_PATH)/chrome_device_policy.proto
nit: put these in alpha order? same for the additions below.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc
File device_policy.cc (right):

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode15
device_policy.cc:15: #include "login_manager/bindings/chrome_device_policy.pb.h"
nit: alpha order of #includes.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode17
device_policy.cc:17: #include "login_manager/owner_key.h"
nit: alpha order of includes.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode92
device_policy.cc:92: // Updtae the UserWhitelistProto inside the
ChromeDeviceSettingsProto we made.
Update

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode99
device_policy.cc:99: on_whitelist = (current_user == *it);
you should break here if on_whitelist gets set to true. Otherwise os_whitelist
is always set to the last comparison.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode105
device_policy.cc:105: if (current_user_is_owner && on_whitelist)
you set current_user_is_owner in the line above? is that temporary? If so, add a
todo. Otherwise, explain yourself.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode106
device_policy.cc:106: return TRUE;  // No changes are needed.
holy capslock batman! true?

(After going through the other files in the CL, you seem to use TRUE/FALSE
usually. However, in this file, there are some places you use true/false, and
TRUE/FALSE elsewhere. Change to be consistent.)

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode108
device_policy.cc:108: // |polval| now has what we want in it.  We need to put it
into
Confusing comment. |polval| was set much further above on line 87. This comment
implies that it happened just now. Maybe get rid of 'now'?

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode114
device_policy.cc:114: if (!key->Sign(data, new_data.length(), &sig)) {
should you NULL-check key?

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode119
device_policy.cc:119: utils.SetGError(error,
CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, err_msg);
so a NULL |error| will be handled correctly by SetGError?

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode120
device_policy.cc:120: return FALSE;
false?

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode129
device_policy.cc:129: return TRUE;
true?

http://codereview.chromium.org/6815021/diff/1/device_policy.h
File device_policy.h (right):

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode35
device_policy.h:35: virtual const enterprise_management::PolicyFetchResponse&
Get() const;
What does this do?

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode41
device_policy.h:41: virtual bool SerializeToString(std::string* output) const;
what does this do?

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode57
device_policy.h:57: static const char kDevicePolicyType[];
Maybe this is documented elsewhere but what is the format of kDevicePolicyType?

http://codereview.chromium.org/6815021/diff/1/device_policy_unittest.cc
File device_policy_unittest.cc (right):

http://codereview.chromium.org/6815021/diff/1/device_policy_unittest.cc#newco...
device_policy_unittest.cc:14: #include
"login_manager/bindings/chrome_device_policy.pb.h"
nit: alpha order.

http://codereview.chromium.org/6815021/diff/1/device_policy_unittest.cc#newco...
device_policy_unittest.cc:214: ASSERT_EQ(CountOwnerInWhitelist(pol,
current_user), 1);
are there cases where CountOwnerInWhitelist would return >=1 and you expect the
test to still pass? If not, why not just make CountOwnerInwhiteList return a
bool which says whether the owner only appeared once?

http://codereview.chromium.org/6815021/diff/1/session_manager_unittest.cc
File session_manager_unittest.cc (right):

http://codereview.chromium.org/6815021/diff/1/session_manager_unittest.cc#new...
session_manager_unittest.cc:238: // First, mimic attempt to whitelist the owner
and set a the
nit: s/a the/the/

[Chris Masone, 2011-04-08 05:57:41]

http://codereview.chromium.org/6815021/diff/1/Makefile
File Makefile (right):

http://codereview.chromium.org/6815021/diff/1/Makefile#newcode26
Makefile:26: $(PROTO_PATH)/chrome_device_policy.proto
On 2011/04/08 04:58:49, gauravsh wrote:
> nit: put these in alpha order? same for the additions below.

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc
File device_policy.cc (right):

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode15
device_policy.cc:15: #include "login_manager/bindings/chrome_device_policy.pb.h"
On 2011/04/08 04:58:49, gauravsh wrote:
> nit: alpha order of #includes.

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode17
device_policy.cc:17: #include "login_manager/owner_key.h"
On 2011/04/08 04:58:49, gauravsh wrote:
> nit: alpha order of includes.

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode92
device_policy.cc:92: // Updtae the UserWhitelistProto inside the
ChromeDeviceSettingsProto we made.
On 2011/04/08 04:58:49, gauravsh wrote:
> Update

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode99
device_policy.cc:99: on_whitelist = (current_user == *it);
On 2011/04/08 04:58:49, gauravsh wrote:
> you should break here if on_whitelist gets set to true. Otherwise os_whitelist
> is always set to the last comparison.

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode105
device_policy.cc:105: if (current_user_is_owner && on_whitelist)
On 2011/04/08 04:58:49, gauravsh wrote:
> you set current_user_is_owner in the line above? is that temporary? If so, add
a
> todo. Otherwise, explain yourself.
TODO added

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode106
device_policy.cc:106: return TRUE;  // No changes are needed.
On 2011/04/08 04:58:49, gauravsh wrote:
> holy capslock batman! true?
> 
> (After going through the other files in the CL, you seem to use TRUE/FALSE
> usually. However, in this file, there are some places you use true/false, and
> TRUE/FALSE elsewhere. Change to be consistent.)

TRUE/FALSE are the gboolean values.  I usually use true/false where I need a
bool, but this originally returned a gboolean.  I've changed to use true/false
now, to match the function prototype.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode108
device_policy.cc:108: // |polval| now has what we want in it.  We need to put it
into
On 2011/04/08 04:58:49, gauravsh wrote:
> Confusing comment. |polval| was set much further above on line 87. This
comment
> implies that it happened just now. Maybe get rid of 'now'?

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode114
device_policy.cc:114: if (!key->Sign(data, new_data.length(), &sig)) {
On 2011/04/08 04:58:49, gauravsh wrote:
> should you NULL-check key?

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode119
device_policy.cc:119: utils.SetGError(error,
CHROMEOS_LOGIN_ERROR_ILLEGAL_PUBKEY, err_msg);
On 2011/04/08 04:58:49, gauravsh wrote:
> so a NULL |error| will be handled correctly by SetGError?

by g_set_error inside SetGError, actually.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode120
device_policy.cc:120: return FALSE;
On 2011/04/08 04:58:49, gauravsh wrote:
> false?

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.cc#newcode129
device_policy.cc:129: return TRUE;
On 2011/04/08 04:58:49, gauravsh wrote:
> true?

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy.h
File device_policy.h (right):

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode35
device_policy.h:35: virtual const enterprise_management::PolicyFetchResponse&
Get() const;
On 2011/04/08 04:58:49, gauravsh wrote:
> What does this do?

It gets a const reference to the PolicyFetchResponse that lives in this
object...I felt like commenting this would be a "x = x + 1;  // Adds 1 to x."
situation.

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode41
device_policy.h:41: virtual bool SerializeToString(std::string* output) const;
On 2011/04/08 04:58:49, gauravsh wrote:
> what does this do?

Serializes the policy into the passed-in string object.  Kinda felt the same as
above.

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode57
device_policy.h:57: static const char kDevicePolicyType[];
On 2011/04/08 04:58:49, gauravsh wrote:
> Maybe this is documented elsewhere but what is the format of
kDevicePolicyType?

it is...I'll put a pointer to it here.

http://codereview.chromium.org/6815021/diff/1/device_policy_unittest.cc
File device_policy_unittest.cc (right):

http://codereview.chromium.org/6815021/diff/1/device_policy_unittest.cc#newco...
device_policy_unittest.cc:14: #include
"login_manager/bindings/chrome_device_policy.pb.h"
On 2011/04/08 04:58:49, gauravsh wrote:
> nit: alpha order.

Done.

http://codereview.chromium.org/6815021/diff/1/device_policy_unittest.cc#newco...
device_policy_unittest.cc:214: ASSERT_EQ(CountOwnerInWhitelist(pol,
current_user), 1);
On 2011/04/08 04:58:49, gauravsh wrote:
> are there cases where CountOwnerInWhitelist would return >=1 and you expect
the
> test to still pass? If not, why not just make CountOwnerInwhiteList return a
> bool which says whether the owner only appeared once?

This way I get to see what the count is right in the test failure message.  Is
it 0?  2?

http://codereview.chromium.org/6815021/diff/1/session_manager_unittest.cc
File session_manager_unittest.cc (right):

http://codereview.chromium.org/6815021/diff/1/session_manager_unittest.cc#new...
session_manager_unittest.cc:238: // First, mimic attempt to whitelist the owner
and set a the
On 2011/04/08 04:58:49, gauravsh wrote:
> nit: s/a the/the/

Done.

[gauravsh, 2011-04-08 18:07:47]

LGTM

http://codereview.chromium.org/6815021/diff/1/device_policy.h
File device_policy.h (right):

http://codereview.chromium.org/6815021/diff/1/device_policy.h#newcode41
device_policy.h:41: virtual bool SerializeToString(std::string* output) const;
On 2011/04/08 05:57:41, Chris Masone wrote:
> On 2011/04/08 04:58:49, gauravsh wrote:
> > what does this do?
> 
> Serializes the policy into the passed-in string object.  Kinda felt the same
as
> above.

Yeah, reading the full class definition provides the proper context.