Change PolicyLoaderWin to load PReg files if possible.

Change PolicyLoaderWin to load PReg files if possible.

We now query whether GPO is present and read directly from the corresponding
PReg files. The registry is used only as a fallback source for policy values.
To accomplish this, PolicyLoaderWin now works in two steps: It first converts
the GPO input into base::Value representation and then maps that to proper
policy values. This also unifies the handling for Chrome policy and 3rd-party
policy.

BUG=
TEST=unit tests
TBR=grt@chromium.org, cpu@chromium.org, ben@chromium.org

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

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

[Mattias Nissler (ping if slow), 2013-04-04 20:58:04]

Hope you like it, it's shorter than before. The unit tests need some love, but
I'd like to give you the chance to take a look and tell me whether there are
fundamental problems with the new approach.

[Mattias Nissler (ping if slow), 2013-04-04 20:59:52]

Hm, it's actually not shorter? My diffstat must have fooled me ;)

[Mattias Nissler (ping if slow), 2013-04-04 21:00:24]

Oh, and the unit tests actually all pass, but the PReg reading is not covered.

[Joao da Silva, 2013-04-05 13:55:00]

Looks good. I'm not sure about how GPO does some things, questions inline.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:18: #pragma comment(lib,
"shlwapi.lib")
nit: order

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:90:
entry->GetAsDictionary(&result_dict);
This leaks |entry| if it's not a dictionary.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:178: if (key.ReadValue(it.Name(),
&wstring_value) == ERROR_SUCCESS) {
ReadValue() internally limits the value size to 1024 chars; the original
ReadRegistryString also handled non-NUL terminated strings.

I'd use ReadRegistryString() to read strings, am I missing something?

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:260: ConvertPolicyValue(**entry,
GetEntry(schema, schema::kItems)));
The GetEntry() call can be outside the loop

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:314: ConvertPolicyValue(*entry,
GetEntry(schema, schema::kItems)));
GetEntry() can be outside the loop

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:337: << " to " << result_type;
nit: indent

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:361: }
policy->LoadFrom(policy_dict, level, scope)

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:373: return false;
What happens in a machine without any policy? Does it fail here, or does it
succeed with an empty policy_object_list?

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:396: << "lpLink " <<
policy_object->lpLink << std::endl;
You've been having some fun eh :-)

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:399: if (policy_object->dwOptions &
GPO_FLAG_DISABLE)
It's not clear to me if this flag is set for boolean policies that are set to
"disabled" in the GPO editor, did you test?

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:402: if
(PathIsUNC(policy_object->lpFileSysPath)) {
Can policy_object->lpDSPath have more policy that isn't in lpFileSysPath? Maybe
this test should be

if (PathIsUNC(policy_object->lpFileSysPath) || !policy_object->lpDSPath.empty())

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:413: if
(!preg_parser::ReadFile(preg_file_path, &new_forced_policy))
This is going to read all the policies for all the managed keys; how about
extending ReadFile() to only load stuff within a path prefix?

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:428:
gpo_policy.MergeDictionary(&regular_policy);
Swap?

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:488:
json_schema_constants::kProperties, properties.release());
Using a DictionaryBuilder here may make this cleaner.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.h (right):

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.h:41: // Loads 3rd-party policy from
gpo_dict and merges it into |bundle|.
nit: |gpo_dict|

[Mattias Nissler (ping if slow), 2013-04-09 22:43:38]

This is tested and seems to be working fine now, ready for thorough review now.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:18: #pragma comment(lib,
"shlwapi.lib")
On 2013/04/05 13:55:01, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:90:
entry->GetAsDictionary(&result_dict);
On 2013/04/05 13:55:01, Joao da Silva wrote:
> This leaks |entry| if it's not a dictionary.

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:178: if (key.ReadValue(it.Name(),
&wstring_value) == ERROR_SUCCESS) {
On 2013/04/05 13:55:01, Joao da Silva wrote:
> ReadValue() internally limits the value size to 1024 chars; the original
> ReadRegistryString also handled non-NUL terminated strings.
> 
> I'd use ReadRegistryString() to read strings, am I missing something?

Good point. Instead of reviving ReadRegistryString, I saved a call to the
registry here given that we have a ReadValue call above already to find the
size.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:260: ConvertPolicyValue(**entry,
GetEntry(schema, schema::kItems)));
On 2013/04/05 13:55:01, Joao da Silva wrote:
> The GetEntry() call can be outside the loop

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:314: ConvertPolicyValue(*entry,
GetEntry(schema, schema::kItems)));
On 2013/04/05 13:55:01, Joao da Silva wrote:
> GetEntry() can be outside the loop

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:337: << " to " << result_type;
On 2013/04/05 13:55:01, Joao da Silva wrote:
> nit: indent

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:361: }
On 2013/04/05 13:55:01, Joao da Silva wrote:
> policy->LoadFrom(policy_dict, level, scope)

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:373: return false;
On 2013/04/05 13:55:01, Joao da Silva wrote:
> What happens in a machine without any policy? Does it fail here, or does it
> succeed with an empty policy_object_list?

Yes, I saw that happening in a pristine VM.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:396: << "lpLink " <<
policy_object->lpLink << std::endl;
On 2013/04/05 13:55:01, Joao da Silva wrote:
> You've been having some fun eh :-)

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:399: if (policy_object->dwOptions &
GPO_FLAG_DISABLE)
On 2013/04/05 13:55:01, Joao da Silva wrote:
> It's not clear to me if this flag is set for boolean policies that are set to
> "disabled" in the GPO editor, did you test?

This refers to the entire GroupPolicyObject, not individual policy settings.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:402: if
(PathIsUNC(policy_object->lpFileSysPath)) {
On 2013/04/05 13:55:01, Joao da Silva wrote:
> Can policy_object->lpDSPath have more policy that isn't in lpFileSysPath?
Maybe
> this test should be
> 
> if (PathIsUNC(policy_object->lpFileSysPath) ||
!policy_object->lpDSPath.empty())

I've actually looked at the DS portion, there's only meta data there.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:413: if
(!preg_parser::ReadFile(preg_file_path, &new_forced_policy))
On 2013/04/05 13:55:01, Joao da Silva wrote:
> This is going to read all the policies for all the managed keys; how about
> extending ReadFile() to only load stuff within a path prefix?

I had done that already, but not updated the uploaded CL yet :)

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:428:
gpo_policy.MergeDictionary(&regular_policy);
On 2013/04/05 13:55:01, Joao da Silva wrote:
> Swap?

Done.

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.cc:488:
json_schema_constants::kProperties, properties.release());
On 2013/04/05 13:55:01, Joao da Silva wrote:
> Using a DictionaryBuilder here may make this cleaner.

DictionaryBuilder is still in chrome/common/extensions, so I won't use it :)

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
File chrome/browser/policy/policy_loader_win.h (right):

https://codereview.chromium.org/13619014/diff/1/chrome/browser/policy/policy_...
chrome/browser/policy/policy_loader_win.h:41: // Loads 3rd-party policy from
gpo_dict and merges it into |bundle|.
On 2013/04/05 13:55:01, Joao da Silva wrote:
> nit: |gpo_dict|

Done.

[Joao da Silva, 2013-04-10 12:32:12]

lgtm

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:11: #include <shlwapi.h>  // For
PathIsUNC()
Suggestion: I find vertically aligned comments easier to parse visually in cases
like this, up to you

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:105: base::ScopedNativeLibrary
kernel32_lib_;
nit: fields after types

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:161: PGROUP_POLICY_OBJECT* gpo_list)
OVERRIDE{
nit: space between OVERRIDE and {

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:456: policy_list_(policy_list),
The order in the header is different.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:517: EnterCriticalPolicySection(scope
== POLICY_SCOPE_MACHINE);
Is this needed to read from the registry? If not then the lock/unlock can be
moved to ReadPolicyFromGPO.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:527: if
(!LeaveCriticalPolicySection(policy_lock))
Is this safe if Enter failed?

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win.h (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.h:79: bool ReadPolicyFromGPO(PolicyScope
scope, base::DictionaryValue* policy);
Seems a bit arbitrary that methods are used to read from GPO, and functions in
the anonymous namespace are used to read from the registry.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win_unittest.cc:463: }
nit: newline

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win_unittest.cc:810: 
Can you add a test where there is no GPO data, the registry is skipped and thus
there should be no policy set?

[Mattias Nissler (ping if slow), 2013-04-10 13:20:25]

Thanks for the review! Unless you have further comments on the test changes, I'm
going to break out the binary files, make sure try jobs succeed and then commit.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win.cc (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:11: #include <shlwapi.h>  // For
PathIsUNC()
On 2013/04/10 12:32:12, Joao da Silva wrote:
> Suggestion: I find vertically aligned comments easier to parse visually in
cases
> like this, up to you

With my nitpick hat on, I'd agree :) But being honest, I can read the
non-aligned style just as well.

I don't care really, so going with your preference.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:105: base::ScopedNativeLibrary
kernel32_lib_;
On 2013/04/10 12:32:12, Joao da Silva wrote:
> nit: fields after types

Done.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:161: PGROUP_POLICY_OBJECT* gpo_list)
OVERRIDE{
On 2013/04/10 12:32:12, Joao da Silva wrote:
> nit: space between OVERRIDE and {

Done.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:456: policy_list_(policy_list),
On 2013/04/10 12:32:12, Joao da Silva wrote:
> The order in the header is different.

Done.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:517: EnterCriticalPolicySection(scope
== POLICY_SCOPE_MACHINE);
On 2013/04/10 12:32:12, Joao da Silva wrote:
> Is this needed to read from the registry? If not then the lock/unlock can be
> moved to ReadPolicyFromGPO.

This makes sure GPO doesn't update behind our back while we're reading, so this
is the GPO way for ensuring you're getting a consistent snapshot.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.cc:527: if
(!LeaveCriticalPolicySection(policy_lock))
On 2013/04/10 12:32:12, Joao da Silva wrote:
> Is this safe if Enter failed?

Yes and no :) Yes, because there's no reason why Chrome or the Win API would
crash if the Enter call fails. No, because we might read inconsistent data (in
particular when reading from the registry), but I think doing a best-effort read
is still better than bailing out and running without policy.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win.h (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win.h:79: bool ReadPolicyFromGPO(PolicyScope
scope, base::DictionaryValue* policy);
On 2013/04/10 12:32:12, Joao da Silva wrote:
> Seems a bit arbitrary that methods are used to read from GPO, and functions in
> the anonymous namespace are used to read from the registry.

It is not entirely arbitrary, the GPO functions needs the registry key name (and
threading that through is tedious), while ReadRegistry() is truly generic.

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win_unittest.cc:463: }
On 2013/04/10 12:32:12, Joao da Silva wrote:
> nit: newline

We usually don't do that for OVERRIDE blocks belonging to the same base class?

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win_unittest.cc:810: 
On 2013/04/10 12:32:12, Joao da Silva wrote:
> Can you add a test where there is no GPO data, the registry is skipped and
thus
> there should be no policy set?

Done.

[Joao da Silva, 2013-04-10 13:32:17]

lgtm

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/13619014/diff/10001/chrome/browser/policy/pol...
chrome/browser/policy/policy_loader_win_unittest.cc:463: }
On 2013/04/10 13:20:25, Mattias Nissler wrote:
> On 2013/04/10 12:32:12, Joao da Silva wrote:
> > nit: newline
> 
> We usually don't do that for OVERRIDE blocks belonging to the same base class?

I haven't seen this before. The style guides don't mention anything about this
case either.

[Mattias Nissler (ping if slow), 2013-04-11 11:07:59]

TBR additions in preparation for committing.

kRegistryChromePolicyKey constant rename:
cpu for chrome/app/breakpad_win.cc
grt for chrome_frame/

ben for gyp changes.

[commit-bot: I haz the power, 2013-04-11 13:45:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/13619014/32001

[commit-bot: I haz the power, 2013-04-11 13:45:42]

Presubmit check for 13619014-32001 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/policy/browser_policy_connector.cc:230
    // call SetPolicyProviderForTesting() again. It is still owned by the test.

** Presubmit Warnings **
New code should not use wstrings.  If you are calling a cross-platform API that
accepts a wstring, fix the API.
    chrome_frame/policy_settings.cc:112

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/app/breakpad_win.cc
    chrome_frame/test/policy_settings_unittest.cc
    chrome_frame/policy_settings.cc
    chrome_frame/chrome_launcher.cc

Presubmit checks took 4.3s to calculate.

[commit-bot: I haz the power, 2013-04-11 13:49:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/13619014/32001

[commit-bot: I haz the power, 2013-04-11 13:49:25]

Presubmit check for 13619014-32001 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/policy/browser_policy_connector.cc:230
    // call SetPolicyProviderForTesting() again. It is still owned by the test.

** Presubmit Warnings **
New code should not use wstrings.  If you are calling a cross-platform API that
accepts a wstring, fix the API.
    chrome_frame/policy_settings.cc:112

[Mattias Nissler (ping if slow), 2013-04-11 15:23:32]

Committed patchset #5 manually as r193649 (presubmit successful).

[Mattias Nissler (ping if slow), 2013-04-12 10:11:42]

Julian, I'd like to reland this after fixing the unit tests, can you take a
look?

[pastarmovj, 2013-04-12 11:39:06]

lgtm

[commit-bot: I haz the power, 2013-04-12 12:19:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/13619014/11011

[commit-bot: I haz the power, 2013-04-12 12:20:00]

Failed to apply patch for chrome/browser/policy/policy_loader_win_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/policy/policy_loader_win_unittest.cc
  Hunk #12 succeeded at 413 with fuzz 1.
  Hunk #13 FAILED at 424.
  1 out of 20 hunks FAILED -- saving rejects to file
chrome/browser/policy/policy_loader_win_unittest.cc.rej

Patch:       chrome/browser/policy/policy_loader_win_unittest.cc
Index: chrome/browser/policy/policy_loader_win_unittest.cc
diff --git a/chrome/browser/policy/policy_loader_win_unittest.cc
b/chrome/browser/policy/policy_loader_win_unittest.cc
index
00b303b8d87eeb87aa42e18ce4c605102021acec..7fb482a46a7e6dc1a747113aa20605713b91df17
100644
--- a/chrome/browser/policy/policy_loader_win_unittest.cc
+++ b/chrome/browser/policy/policy_loader_win_unittest.cc
@@ -4,9 +4,15 @@
 
 #include "chrome/browser/policy/policy_loader_win.h"
 
+#include <userenv.h>
 #include <windows.h>
 
+#include <cstring>
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
 #include "base/json/json_writer.h"
+#include "base/path_service.h"
 #include "base/process.h"
 #include "base/string16.h"
 #include "base/string_util.h"
@@ -18,6 +24,7 @@
 #include "chrome/browser/policy/configuration_policy_provider_test.h"
 #include "chrome/browser/policy/policy_bundle.h"
 #include "chrome/browser/policy/policy_map.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/json_schema/json_schema_constants.h"
 #include "policy/policy_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -25,12 +32,18 @@
 namespace schema = json_schema_constants;
 
 using base::win::RegKey;
-using namespace policy::registry_constants;
 
 namespace policy {
 
 namespace {
 
+// Constants for registry key names.
+const wchar_t kPathSep[] = L"\\";
+const wchar_t kThirdParty[] = L"3rdparty";
+const wchar_t kMandatory[] = L"policy";
+const wchar_t kRecommended[] = L"recommended";
+const wchar_t kSchema[] = L"schema";
+
 // Installs |value| in the given registry |path| and |hive|, under the key
 // |name|. Returns false on errors.
 // Some of the possible Value types are stored after a conversion (e.g. doubles
@@ -219,13 +232,15 @@ class ScopedGroupPolicyRegistrySandbox {
   DISALLOW_COPY_AND_ASSIGN(ScopedGroupPolicyRegistrySandbox);
 };
 
-class TestHarness : public PolicyProviderTestHarness {
+class TestHarness : public PolicyProviderTestHarness,
+                    public AppliedGPOListProvider {
  public:
   explicit TestHarness(HKEY hive, PolicyScope scope);
   virtual ~TestHarness();
 
   virtual void SetUp() OVERRIDE;
 
+  // PolicyProviderTestHarness:
   virtual ConfigurationPolicyProvider* CreateProvider(
       const PolicyDefinitionList* policy_definition_list) OVERRIDE;
 
@@ -245,6 +260,14 @@ class TestHarness : public PolicyProviderTestHarness {
   virtual void Install3rdPartyPolicy(
       const base::DictionaryValue* policies) OVERRIDE;
 
+  // AppliedGPOListProvider:
+  virtual DWORD GetAppliedGPOList(DWORD flags,
+                                  LPCTSTR machine_name,
+                                  PSID sid_user,
+                                  GUID* extension_guid,
+                                  PGROUP_POLICY_OBJECT* gpo_list) OVERRIDE;
+  virtual BOOL FreeGPOList(PGROUP_POLICY_OBJECT gpo_list) OVERRIDE;
+
   // Creates a harness instance that will install policy in HKCU or HKLM,
   // respectively.
   static PolicyProviderTestHarness* CreateHKCU();
@@ -310,9 +333,23 @@ TestHarness::~TestHarness() {}
 
 void TestHarness::SetUp() {}
 
+DWORD TestHarness::GetAppliedGPOList(DWORD flags,
+                                     LPCTSTR machine_name,
+                                     PSID sid_user,
+                                     GUID* extension_guid,
+                                     PGROUP_POLICY_OBJECT* gpo_list) {
+  *gpo_list = NULL;
+  return ERROR_ACCESS_DENIED;
+}
+
+BOOL TestHarness::FreeGPOList(PGROUP_POLICY_OBJECT gpo_list) {
+  return TRUE;
+}
+
 ConfigurationPolicyProvider* TestHarness::CreateProvider(
     const PolicyDefinitionList* policy_list) {
-  scoped_ptr<AsyncPolicyLoader> loader(new PolicyLoaderWin(policy_list));
+  scoped_ptr<AsyncPolicyLoader> loader(
+      new PolicyLoaderWin(policy_list, kRegistryChromePolicyKey, this));
   return new AsyncPolicyProvider(loader.Pass());
 }
 
@@ -320,7 +357,7 @@ void TestHarness::InstallEmptyPolicy() {}
 
 void TestHarness::InstallStringPolicy(const std::string& policy_name,
                                       const std::string& policy_value) {
-  RegKey key(hive_, kRegistryMandatorySubKey, KEY_ALL_ACCESS);
+  RegKey key(hive_, kRegistryChromePolicyKey, KEY_ALL_ACCESS);
   ASSERT_TRUE(key.Valid());
   ASSERT_HRESULT_SUCCEEDED(key.WriteValue(UTF8ToUTF16(policy_name).c_str(),
                                           UTF8ToUTF16(policy_value).c_str()));
@@ -328,7 +365,7 @@ void TestHarness::InstallStringPolicy(const std::string&
policy_name,
 
 void TestHarness::InstallIntegerPolicy(const std::string& policy_name,
                                        int policy_value) {
-  RegKey key(hive_, kRegistryMandatorySubKey, KEY_ALL_ACCESS);
+  RegKey key(hive_, kRegistryChromePolicyKey, KEY_ALL_ACCESS);
   ASSERT_TRUE(key.Valid());
   key.WriteValue(UTF8ToUTF16(policy_name).c_str(),
                  static_cast<DWORD>(policy_value));
@@ -336,7 +373,7 @@ void TestHarness::InstallIntegerPolicy(const std::string&
policy_name,
 
 void TestHarness::InstallBooleanPolicy(const std::string& policy_name,
                                        bool policy_value) {
-  RegKey key(hive_, kRegistryMandatorySubKey, KEY_ALL_ACCESS);
+  RegKey key(hive_, kRegistryChromePolicyKey, KEY_ALL_ACCESS);
   ASSERT_TRUE(key.Valid());
   key.WriteValue(UTF8ToUTF16(policy_name).c_str(),
                  static_cast<DWORD>(policy_value));
@@ -345,7 +382,7 @@ void TestHarness::InstallBooleanPolicy(const std::string&
policy_name,
 void TestHarness::InstallStringListPolicy(const std::string& policy_name,
                                           const base::ListValue* policy_value)
{
   RegKey key(hive_,
-             (string16(kRegistryMandatorySubKey) + ASCIIToUTF16("\\") +
+             (string16(kRegistryChromePolicyKey) + ASCIIToUTF16("\\") +
               UTF8ToUTF16(policy_name)).c_str(),
              KEY_ALL_ACCESS);
   ASSERT_TRUE(key.Valid());
@@ -367,7 +404,7 @@ void TestHarness::InstallDictionaryPolicy(
     const base::DictionaryValue* policy_value) {
   std::string json;
   base::JSONWriter::Write(policy_value, &json);
-  RegKey key(hive_, kRegistryMandatorySubKey, KEY_ALL_ACCESS);
+  RegKey key(hive_, kRegistryChromePolicyKey, KEY_ALL_ACCESS);
   ASSERT_TRUE(key.Valid());
   key.WriteValue(UTF8ToUTF16(policy_name).c_str(),
                  UTF8ToUTF16(json).c_str());
@@ -376,7 +413,7 @@ void TestHarness::InstallDictionaryPolicy(
 void TestHarness::Install3rdPartyPolicy(const base::DictionaryValue* policies)
{
   // The first level entries are domains, and the second level entries map
   // components to their policy.
-  const string16 kPathPrefix = string16(kRegistryMandatorySubKey) + kPathSep +
+  const string16 kPathPrefix = string16(kRegistryChromePolicyKey) + kPathSep +
                                kThirdParty + kPathSep;
   for (base::DictionaryValue::Iterator domain(*policies);
        domain.HasNext(); domain.Advance()) {
@@ -387,7 +424,7 @@ void TestHarness::Install3rdPartyPolicy(const
base::DictionaryValue* policies) {
     }
     for (base::DictionaryValue::Iterator component(*components);
          component.HasNext(); component.Advance()) {
-      const string16 path = string16(kRegistryMandatorySubKey) + kPathSep +
+      const string16 path = string16(kRegistryChromePolicyKey) + kPathSep +
                             kThirdParty + kPathSep +
                             UTF8ToUTF16(domain.key()) + kPathSep +
                             UTF8ToUTF16(component.key());
@@ -422,26 +459,104 @@ INSTANTIATE_TEST_CASE_P(
     testing::Values(TestHarness::CreateHKCU, TestHarness::CreateHKLM));
 
 // Test cases for windows policy provider specific functionality.
-class PolicyLoaderWinTest : public PolicyTestBase {
+class PolicyLoaderWinTest : public PolicyTestBase,
+                            public AppliedGPOListProvider {
  protected:
-  PolicyLoaderWinTest() {}
+  // The policy key this tests places data under. This must match the data
+  // files in chrome/test/data/policy/gpo.
+  static const char16 kTestPolicyKey[];
+
+  PolicyLoaderWinTest()
+      : gpo_list_(NULL),
+        gpo_list_status_(ERROR_ACCESS_DENIED) {}
   virtual ~PolicyLoaderWinTest() {}
 
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_));
+    test_data_dir_ = test_data_dir_.AppendASCII("policy").AppendASCII("gpo");
+  }
+
+  // AppliedGPOListProvider:
+  virtual DWORD GetAppliedGPOList(DWORD flags,
+                                  LPCTSTR machine_name,
+                                  PSID sid_user,
+                                  GUID* extension_guid,
+                                  PGROUP_POLICY_OBJECT* gpo_list) OVERRIDE {
+    *gpo_list = gpo_list_;
+    return gpo_list_status_;
+  }
+  virtual BOOL FreeGPOList(PGROUP_POLICY_OBJECT gpo_list) OVERRIDE {
+    return TRUE;
+  }
+
+  void InitGPO(GROUP_POLICY_OBJECT* gpo,
+               DWORD options,
+               const base::FilePath& path,
+               GROUP_POLICY_OBJECT* next,
+               GROUP_POLICY_OBJECT* prev) {
+    memset(gpo, 0, sizeof(GROUP_POLICY_OBJECT));
+    gpo->dwOptions = options;
+    gpo->lpFileSysPath = const_cast<wchar_t*>(path.value().c_str());
+    gpo->pNext = next;
+    gpo->pPrev = prev;
+  }
+
   bool Matches(const PolicyBundle& expected) {
-    PolicyLoaderWin loader(&test_policy_definitions::kList);
+    PolicyLoaderWin loader(&test_policy_definitions::kList, kTestPolicyKey,
+                           this);
  …
(message too large)

[commit-bot: I haz the power, 2013-04-12 16:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/13619014/68001

[commit-bot: I haz the power, 2013-04-12 16:36:59]

Presubmit check for 13619014-68001 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/policy/browser_policy_connector.cc:230
    // call SetPolicyProviderForTesting() again. It is still owned by the test.

If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
New code should not use wstrings.  If you are calling a cross-platform API that
accepts a wstring, fix the API.
    chrome_frame/policy_settings.cc:112

Presubmit checks took 1.9s to calculate.

[Mattias Nissler (ping if slow), 2013-04-12 17:55:20]

Committed patchset #9 manually as r193961 (presubmit successful).