Implement Windows GPO support for "dlpref" in component updater.

chrome/installer/util/google_update_settings.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/installer/util/google_update_settings.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <limits>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "base/win/win_util.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/app_registration_data.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/channel_info.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/installation_state.h"
 #include "chrome/installer/util/product.h"
 
 using base::win::RegKey;
 using installer::InstallationState;
 
 const wchar_t GoogleUpdateSettings::kPoliciesKey[] =
     L"SOFTWARE\\Policies\\Google\\Update";
 const wchar_t GoogleUpdateSettings::kUpdatePolicyValue[] = L"UpdateDefault";
+const wchar_t GoogleUpdateSettings::kDownloadPreference[] =
+    L"DownloadPreference";
+const wchar_t GoogleUpdateSettings::kDownloadPreferenceCacheable[] =
+    L"cacheable";
 const wchar_t GoogleUpdateSettings::kUpdateOverrideValuePrefix[] = L"Update";
 const wchar_t GoogleUpdateSettings::kCheckPeriodOverrideMinutes[] =
     L"AutoUpdateCheckPeriodMinutes";
 
 // Don't allow update periods longer than six weeks.
 const int GoogleUpdateSettings::kCheckPeriodOverrideMinutesMax =
     60 * 24 * 7 * 6;
 
 const GoogleUpdateSettings::UpdatePolicy
 GoogleUpdateSettings::kDefaultUpdatePolicy =
@@ skipping 714 matching lines @@
     // permissions to make changes (the most likely reason is that there is no
     // policy set). Simply return whether or not we think updates are enabled.
     return AreAutoupdatesEnabled();
   }
 
 #endif
   // Non Google Chrome isn't going to autoupdate.
   return true;
 }
 
+// Reads and sanitizes the value of
+// "HKLM\SOFTWARE\Policies\Google\Update\DownloadPreference". A valid
+// group policy option must be a single alpha numeric word of up to 32
+// characters.
+base::string16 GoogleUpdateSettings::GetDownloadPreference() {
+  // Validates that the argument value matches `[a-zA-z]{0-32}`.
+  struct Validator {
+   public:
+    static bool Validate(const base::string16& value) {
+      const size_t kMaxValueLength = 32;
+      if (value.size() > kMaxValueLength)
+        return false;
+      for (auto ch : value) {
+        if (!base::IsAsciiAlpha(ch))
+          return false;
+      }
+      return true;
+    }
+  };
+
+  RegKey policy_key;
+  base::string16 value;
+  if (policy_key.Open(HKEY_LOCAL_MACHINE, kPoliciesKey, KEY_QUERY_VALUE) ==
+          ERROR_SUCCESS &&

[gab, 2016/01/21 21:08:49]

Is this git cl format's output? Would feel more natural to me to wrap the last
parameter of the Open() call and have this line be:

                 , KEY_QUERY_VALUE) == ERROR_SUCESS &&

If it's git cl format though then I guess that's okay..

[Sorin Jianu, 2016/01/25 20:04:58]

Correct.

+      policy_key.ReadValue(kDownloadPreference, &value) == ERROR_SUCCESS &&
+      Validator::Validate(value)) {

[gab, 2016/01/22 16:06:50]

Actually, why use a static method inside a local class? Can't the body of that
method just be in the if's body below?

[Sorin Jianu, 2016/01/25 20:04:58]

I tried to follow the existing structure of the code and encapsulate the
validation logic in a function.

[gab, 2016/01/25 20:33:50]

Hmmm I don't see code defining a local static method inside a struct in this
file?

Regardless though, you can move lines 787 to 794 in the body of this if and
forgo the helper method altogether.

[Sorin Jianu, 2016/01/25 21:27:56]

I meant I followed the structure of:
if (open(key) &&
    read(value) &&
    validate(value))....

What we are discussing here is whether to abstract out the validation step, as
above, or code up the validation inside the function itself. I prefer writing
functions.

To me this is a minor point, I think the code looks wors, and it is more
error-prone due to the multiple returns. I inlined the code anyway. 

[gab, 2016/01/26 03:44:55]

Thanks, I prefer the latest version inline. To me the advantage of having a
separate function is either code re-use or testability, never of which was being
used here; hence my preference.

[Sorin Jianu, 2016/01/26 18:09:48]

Acknowledged.

+    return value;
+  }
+
+  return base::string16();
+}
+
 void GoogleUpdateSettings::RecordChromeUpdatePolicyHistograms() {
   const bool is_multi_install = InstallUtil::IsMultiInstall(
       BrowserDistribution::GetDistribution(), IsSystemInstall());
   const base::string16 app_guid =
       BrowserDistribution::GetSpecificDistribution(
           is_multi_install ? BrowserDistribution::CHROME_BINARIES :
                              BrowserDistribution::CHROME_BROWSER)->GetAppGuid();
 
   bool is_overridden = false;
   const UpdatePolicy update_policy = GetAppUpdatePolicy(app_guid,
@@ skipping 188 matching lines @@
   }
 
   // If the key or value was not present, return the empty string.
   if (result == ERROR_FILE_NOT_FOUND || result == ERROR_PATH_NOT_FOUND) {
     experiment_labels->clear();
     return true;
   }
 
   return result == ERROR_SUCCESS;
 }