[Win] Implement core mechanism to honor Windows Group Policy

[Win] Implement core mechanism to honor Windows Group Policy

BUG=42640
TEST=--gtest_filter=WinConfigurationPolicyProviderTest*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47910

[danno, 2010-05-19 12:14:52]

Hi Marc-Antoine,

This CL is ready for review. It's almost exclusively Windows-specific code for
supporting Windows Group Policy. Pam suggested that you would be a good starting
reviewer, if you think somebody else should take a look, please forward.

Danno

[M-A Ruel, 2010-05-19 12:35:05]

http://codereview.chromium.org/2119005/diff/20001/19006
File chrome/browser/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/2119005/diff/20001/19006#newcode15
chrome/browser/configuration_policy_provider_win.cc:15: const TCHAR
WinConfigurationPolicyProvider::kHomepageRegistryValueName[] =
s/TCHAR/wchar_t/ everywhere.

http://codereview.chromium.org/2119005/diff/20001/19006#newcode37
chrome/browser/configuration_policy_provider_win.cc:37: LONG long_result =
RegGetValue(hive_key, kPolicyRegistrySubKey,
Please use base/registry.h, e.g. rewrite all the following code.

http://codereview.chromium.org/2119005/diff/20001/19007
File chrome/browser/configuration_policy_provider_win.h (right):

http://codereview.chromium.org/2119005/diff/20001/19007#newcode8
chrome/browser/configuration_policy_provider_win.h:8: #include <windows.h>
This is not needed, after fix the other changes below.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode14
chrome/browser/configuration_policy_provider_win.h:14: // A class that
implements a |ConfigurationPolicyProvider| via the
Implements |ConfigurationPolicyProvider| via ...

I know that it's a class. :) In general, I prefer the shortest but still correct
description. But I'm a style freak.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode31
chrome/browser/configuration_policy_provider_win.h:31: friend class
WinConfigurationPolicyProviderTest;
Since you really have a lot of friends, it's maybe worth to put the members you
want to test as protected and create a class create a class in
configuration_policy_provider_win_unittest.cc that would inherit from
WinConfigurationPolicyProvider and would grant access to these members in the
unit test.

I'm saying that because FRIEND_TEST is painful when we need to mark as test as
DISABLED_ or FLAKY_.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode70
chrome/browser/configuration_policy_provider_win.h:70: // A utility routine that
returns a buffer filled with the registry
Returns a buffer ...

http://codereview.chromium.org/2119005/diff/20001/19007#newcode75
chrome/browser/configuration_policy_provider_win.h:75: const TCHAR* value_name,
DWORD* type,
s/TCHAR/wchar_t/
s/DWORD/uint32/

http://codereview.chromium.org/2119005/diff/20001/19007#newcode78
chrome/browser/configuration_policy_provider_win.h:78: // A utility routine that
returns a buffer filled with the registry
Returns a buffer ...

I guess you get the idea.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode80
chrome/browser/configuration_policy_provider_win.h:80: // HKCU is checked first,
if the registry value is found there, a buffer
Is it kind of weird that you look for HKCU and then HKLM? I mean, then the user
can hack this with a simple .reg file.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode86
chrome/browser/configuration_policy_provider_win.h:86: uint8*
GetRegistryPolicyData(LPCTSTR value_name,
s/LPCTSTR/const wchar_t*/
s/LPDWORD/uint32*/

and the rest too

[danno, 2010-05-20 12:38:04]

Here's a re-written version using base/registry.h and addressing the other
feedback.

Note that there are still a few places where DWORD is used. This is because the
interface DWORD is used explicitly in the RegKey interface, and as it is
typedefed to unsigned long, it is not interchangeable with uint32, which is
unsigned int.

http://codereview.chromium.org/2119005/diff/20001/19006
File chrome/browser/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/2119005/diff/20001/19006#newcode15
chrome/browser/configuration_policy_provider_win.cc:15: const TCHAR
WinConfigurationPolicyProvider::kHomepageRegistryValueName[] =
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> s/TCHAR/wchar_t/ everywhere.

Done.

http://codereview.chromium.org/2119005/diff/20001/19006#newcode37
chrome/browser/configuration_policy_provider_win.cc:37: LONG long_result =
RegGetValue(hive_key, kPolicyRegistrySubKey,
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> Please use base/registry.h, e.g. rewrite all the following code.

Done.

http://codereview.chromium.org/2119005/diff/20001/19007
File chrome/browser/configuration_policy_provider_win.h (right):

http://codereview.chromium.org/2119005/diff/20001/19007#newcode8
chrome/browser/configuration_policy_provider_win.h:8: #include <windows.h>
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> This is not needed, after fix the other changes below.

Done.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode14
chrome/browser/configuration_policy_provider_win.h:14: // A class that
implements a |ConfigurationPolicyProvider| via the
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> Implements |ConfigurationPolicyProvider| via ...
> 
> I know that it's a class. :) In general, I prefer the shortest but still
correct
> description. But I'm a style freak.

Done.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode31
chrome/browser/configuration_policy_provider_win.h:31: friend class
WinConfigurationPolicyProviderTest;
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> Since you really have a lot of friends, it's maybe worth to put the members
you
> want to test as protected and create a class create a class in
> configuration_policy_provider_win_unittest.cc that would inherit from
> WinConfigurationPolicyProvider and would grant access to these members in the
> unit test.
> 
> I'm saying that because FRIEND_TEST is painful when we need to mark as test as
> DISABLED_ or FLAKY_.

Done.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode70
chrome/browser/configuration_policy_provider_win.h:70: // A utility routine that
returns a buffer filled with the registry
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> Returns a buffer ...

Removed in base/registry.h port.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode75
chrome/browser/configuration_policy_provider_win.h:75: const TCHAR* value_name,
DWORD* type,
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> s/TCHAR/wchar_t/
> s/DWORD/uint32/

Done.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode78
chrome/browser/configuration_policy_provider_win.h:78: // A utility routine that
returns a buffer filled with the registry
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> Returns a buffer ...
> 
> I guess you get the idea.

Removed in base/registry.h port.

http://codereview.chromium.org/2119005/diff/20001/19007#newcode80
chrome/browser/configuration_policy_provider_win.h:80: // HKCU is checked first,
if the registry value is found there, a buffer
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> Is it kind of weird that you look for HKCU and then HKLM? I mean, then the
user
> can hack this with a simple .reg file.

You're right, HKLM takes precedence over HKCU. However, in a managed system,
it's my understanding you still can't arbitrarily change HKCU/Software/Policies:
http://technet.microsoft.com/en-us/library/cc962657.aspx

http://codereview.chromium.org/2119005/diff/20001/19007#newcode86
chrome/browser/configuration_policy_provider_win.h:86: uint8*
GetRegistryPolicyData(LPCTSTR value_name,
On 2010/05/19 12:35:06, Marc-Antoine Ruel wrote:
> s/LPCTSTR/const wchar_t*/
> s/LPDWORD/uint32*/
> 
> and the rest too

Done.

[M-A Ruel, 2010-05-20 13:45:53]

http://codereview.chromium.org/2119005/diff/33001/33006
File chrome/browser/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/2119005/diff/33001/33006#newcode42
chrome/browser/configuration_policy_provider_win.cc:42: buffer.reset(new
uint8[value_size]);
use value_size + 2 and memset it to 0. This way you won't have to do the
std::find stuff below.

http://codereview.chromium.org/2119005/diff/33001/33006#newcode151
chrome/browser/configuration_policy_provider_win.cc:151: break;
not necessary

http://codereview.chromium.org/2119005/diff/33001/33007
File chrome/browser/configuration_policy_provider_win.h (right):

http://codereview.chromium.org/2119005/diff/33001/33007#newcode10
chrome/browser/configuration_policy_provider_win.h:10: #include
"testing/gtest/include/gtest/gtest_prod.h"
not needed anymore

http://codereview.chromium.org/2119005/diff/33001/33008
File chrome/browser/configuration_policy_provider_win_unittest.cc (right):

http://codereview.chromium.org/2119005/diff/33001/33008#newcode298
chrome/browser/configuration_policy_provider_win_unittest.cc:298: 
remove extra lines

http://codereview.chromium.org/2119005/diff/33001/33009
File chrome/browser/configuration_policy_store.h (right):

http://codereview.chromium.org/2119005/diff/33001/33009#newcode20
chrome/browser/configuration_policy_store.h:20: kPolicyCookiesMode
I'm worried. People will lose their current cookie settings?

[danno, 2010-05-20 15:26:54]

I have addressed all of the feedback with changes in the patch set or my own
comments. Since I'm in Munich and will be leaving the office soon, if there are
only nits left, then please "LGTM with nit fixes:" so I can commit in the
morning.

Thanks,
Danno

http://codereview.chromium.org/2119005/diff/33001/33006
File chrome/browser/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/2119005/diff/33001/33006#newcode42
chrome/browser/configuration_policy_provider_win.cc:42: buffer.reset(new
uint8[value_size]);
On 2010/05/20 13:45:53, Marc-Antoine Ruel wrote:
> use value_size + 2 and memset it to 0. This way you won't have to do the
> std::find stuff below.

Done.

http://codereview.chromium.org/2119005/diff/33001/33006#newcode151
chrome/browser/configuration_policy_provider_win.cc:151: break;
On 2010/05/20 13:45:53, Marc-Antoine Ruel wrote:
> not necessary

Done.

http://codereview.chromium.org/2119005/diff/33001/33007
File chrome/browser/configuration_policy_provider_win.h (right):

http://codereview.chromium.org/2119005/diff/33001/33007#newcode10
chrome/browser/configuration_policy_provider_win.h:10: #include
"testing/gtest/include/gtest/gtest_prod.h"
On 2010/05/20 13:45:53, Marc-Antoine Ruel wrote:
> not needed anymore

Done.

http://codereview.chromium.org/2119005/diff/33001/33008
File chrome/browser/configuration_policy_provider_win_unittest.cc (right):

http://codereview.chromium.org/2119005/diff/33001/33008#newcode298
chrome/browser/configuration_policy_provider_win_unittest.cc:298: 
On 2010/05/20 13:45:53, Marc-Antoine Ruel wrote:
> remove extra lines

Done.

http://codereview.chromium.org/2119005/diff/33001/33009
File chrome/browser/configuration_policy_store.h (right):

http://codereview.chromium.org/2119005/diff/33001/33009#newcode20
chrome/browser/configuration_policy_store.h:20: kPolicyCookiesMode
On 2010/05/20 13:45:53, Marc-Antoine Ruel wrote:
> I'm worried. People will lose their current cookie settings?
If a policy is set by an administrator, it simply overrides preferences set by
the user. In this case, this policy controls "how cookies and other site data
are saved on your computer", which is an option that's already available in the
settings dialog. Changing this setting only alters how the browser reacts to
future requests to store cookies and leave existing cookies untouched.

[M-A Ruel, 2010-05-20 15:36:38]

lgtm with nits.

http://codereview.chromium.org/2119005/diff/41001/22007
File chrome/browser/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/2119005/diff/41001/22007#newcode66
chrome/browser/configuration_policy_provider_win.cc:66: *result =
string16(reinterpret_cast<const wchar_t*>(buffer.get()));
Please use
result->assign(reinterpret_cast<const wchar_t*>(buffer.get()));
to not create an unneeded temporary object.

http://codereview.chromium.org/2119005/diff/41001/22009
File chrome/browser/configuration_policy_provider_win_unittest.cc (right):

http://codereview.chromium.org/2119005/diff/41001/22009#newcode133
chrome/browser/configuration_policy_provider_win_unittest.cc:133: DCHECK(result
== ERROR_SUCCESS);
Never use DCHECK in test code, it won't be executed in release...

Please use the _EQ form:
EXPECT_EQ(ERROR_SUCCESS, result);
so the value is output in the log.

http://codereview.chromium.org/2119005/diff/41001/22009#newcode134
chrome/browser/configuration_policy_provider_win_unittest.cc:134: 
empty line not needed

http://codereview.chromium.org/2119005/diff/41001/22009#newcode142
chrome/browser/configuration_policy_provider_win_unittest.cc:142: 0);
no need to use 2 lines for this function call.