Policy: Ignore ForceSafeSearch if ForceGoogleSafeSearch or ForceYoutubeSafetyMode are enabled

chrome/browser/net/chrome_network_delegate.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/browser/net/chrome_network_delegate.h"
 
 #include <stdlib.h>
 
 #include <vector>
 
@@ skipping 379 matching lines @@
 #endif
 
   extensions_delegate_->ForwardStartRequestStatus(request);
 
   if (!enable_referrers_->GetValue())
     request->SetReferrer(std::string());
   if (enable_do_not_track_ && enable_do_not_track_->GetValue())
     request->SetExtraRequestHeaderByName(kDNTHeader, "1", true /* override */);
 
   bool force_safe_search =
-      (force_safe_search_ && force_safe_search_->GetValue()) ||
       (force_google_safe_search_ && force_google_safe_search_->GetValue());
+  if (!force_safe_search &&

[Thiemo Nagel, 2015/04/14 14:39:58]

This yields force_safe_search=true for FSS=1, FGSS=0 and FYTSM=undef, but the
correct value in that case would be force_safe_search=false.

[Marc Treib, 2015/04/14 14:47:38]

Note that any of these being null does *not* mean "the policy has no defined
value", it just means "this member hasn't been initialized yet".
(The three are also initialized together:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...)
At this point, we can't tell "policy is explicitly disabled" from "policy is not
set".

[Thiemo Nagel, 2015/04/14 14:55:37]

Can't we use PrefMember<>::IsManaged() for that purpose?

[Marc Treib, 2015/04/14 15:02:01]

Right, that should work! Might be a bit awkward to test though.
I'll update the CL tomorrow, have to run now...

[Andrew T Wilson (Slow), 2015/04/14 15:02:05]

I actually don't think we want force_safe_search to depend on youtube safety
mode, but I'll defer to Thiemo here. If we do, then we probably just want to
depend on the *presence* of it, not the actual value (so, even if they set
force_google_safe_search to false, it should override the force_safe_search
value.

[Thiemo Nagel, 2015/04/14 15:27:31]

If someone used FSS for the sake of YouTube Safety Mode and thus sets FSS=1,
FGSS=undef and FYTSM=1, we probably don't want to force Safe Search on them. 
[FSS=1, FGSS=undef, FYTSM=1] should be symmetric to [FSS=1, FGSS=1,
FYTSM=undef].
Absolutely.

[Mattias Nissler (ping if slow), 2015/04/15 07:48:31]

Stupid question: Why do we actually still have a prefs::kForceSafeSearch? From a
quick look at the code, it seems like we can just get rid of the legacy pref,
map the legacy policy to prefs in the correct way in the policy handler, and
have all consuming code just look at either prefs::kForceGoogleSafeSearch or
prefs::kForceYouTubeSafetyMode.

[Thiemo Nagel, 2015/04/15 08:03:56]

Sounds good!

[Marc Treib, 2015/04/15 12:12:12]

That would mean that we basically have to re-evaluate all three policies when
any of them change. (E.g. legacy SafeSearch is already set to 1. Now
GoogleSafeSearch is set; we'd have to remove the YouTube pref.)
Not sure if the policy system easily supports this - Thiemo?

[Mattias Nissler (ping if slow), 2015/04/15 12:14:13]

If I'm not mistaken, we do a full policy->pref mapping whenever policy changes,
so that shouldn't be a problem.

[Thiemo Nagel, 2015/04/15 12:32:22]

Mattias knows the policy stack much better than I do.  :)

+      !(force_youtube_safety_mode_ && force_youtube_safety_mode_->GetValue())) {
+    // In case neither ForceGoogleSafeSearch nor ForceYoutubeSafetyMode are
+    // enabled, fall back to the deprecated ForceSafeSearch. crbug.com/476908
+    force_safe_search = (force_safe_search_ && force_safe_search_->GetValue());
+  }
 
   net::CompletionCallback wrapped_callback = callback;
   if (force_safe_search) {
     wrapped_callback = base::Bind(&ForceGoogleSafeSearchCallbackWrapper,
                                   callback,
                                   base::Unretained(request),
                                   base::Unretained(new_url));
   }
 
   int rv = extensions_delegate_->OnBeforeURLRequest(
       request, wrapped_callback, new_url);
 
   if (force_safe_search && rv == net::OK && new_url->is_empty())
     safe_search_util::ForceGoogleSafeSearch(request, new_url);
 
   if (connect_interceptor_)
     connect_interceptor_->WitnessURLRequest(request);
 
   return rv;
 }
 
 int ChromeNetworkDelegate::OnBeforeSendHeaders(
     net::URLRequest* request,
     const net::CompletionCallback& callback,
     net::HttpRequestHeaders* headers) {
   bool force_safety_mode =
-      (force_safe_search_ && force_safe_search_->GetValue()) ||
       (force_youtube_safety_mode_ && force_youtube_safety_mode_->GetValue());
+  if (!force_safety_mode &&

[Thiemo Nagel, 2015/04/14 14:39:58]

See above.

+      !(force_google_safe_search_ && force_google_safe_search_->GetValue())) {
+    // In case neither ForceGoogleSafeSearch nor ForceYoutubeSafetyMode are
+    // enabled, fall back to the deprecated ForceSafeSearch. crbug.com/476908
+    force_safety_mode = (force_safe_search_ && force_safe_search_->GetValue());

[Andrew T Wilson (Slow), 2015/04/14 15:02:05]

Ditto here - we want people to be able to set force_youtube_safety_mode_ = false
and always have it take effect, even if forceSafeSearch = true.

+  }
   if (force_safety_mode)
     safe_search_util::ForceYouTubeSafetyMode(request, headers);
 
   return extensions_delegate_->OnBeforeSendHeaders(request, callback, headers);
 }
 
 void ChromeNetworkDelegate::OnSendHeaders(
     net::URLRequest* request,
     const net::HttpRequestHeaders& headers) {
   extensions_delegate_->OnSendHeaders(request, headers);
@@ skipping 247 matching lines @@
   return experimental_web_platform_features_enabled_;
 }
 
 bool ChromeNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
     const net::URLRequest& request,
     const GURL& target_url,
     const GURL& referrer_url) const {
   ReportInvalidReferrerSend(target_url, referrer_url);
   return true;
 }