Add logic to enterprise policy to allow ambient authentication

for profile type.

Ambient authentication in private modes can currently be toggled with
feature flag. The default value of the flag is to allow for all profile
types. This default value would be set to disallowed for OTR profile
types in the future. A policy is introduced for enterprise admins
who still want the ambient authentication for the OTR profiles.

This CL adds the logic to allow ambient authentication via policy
and the existing feature flag. As a consequence for now, if either
the policy or feature flag allows ambient authentication then the
resulting behanviour is that we allow ambient authentication.

This CL also adds browser tests.

Bug: 458508
Change-Id: I488cf40ec4c178739d441fe76fb15a3e3450cb56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1944493
Commit-Queue: Rohit Agarwal <roagarwal@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721494}

chrome/browser/net/profile_network_context_service.cc

@@ -174,12 +174,41 @@ void InitializeCorsExtraSafelistedRequestHeaderNamesForProfile(
   }
 }
 
-enum class AmbientAuthAllowedProfileTypes {
-  REGULAR_ONLY = 0,
-  INCOGNITO_AND_REGULAR = 1,
-  GUEST_AND_REGULAR = 2,
-  ALL = 3,
-};
+// Tests allowing ambient authentication with default credentials based on the
+// profile type.
+// TODO(https://crbug.com/458508): Currently, this is determined by OR of the
+// feature flag and policy. Next steps would be changing the feature value to
+// false after enough heads up and then removing the feature.
+bool IsAmbientAuthAllowedForProfile(const Profile* profile) {
+  if (profile->IsRegularProfile())
+    return true;
+
+  PrefService* local_state = g_browser_process->local_state();
+  DCHECK(local_state);
+  DCHECK(local_state->FindPreference(
+      prefs::kAmbientAuthenticationInPrivateModesEnabled));
+
+  net::AmbientAuthAllowedProfileTypes type =
+      static_cast<net::AmbientAuthAllowedProfileTypes>(local_state->GetInteger(
+          prefs::kAmbientAuthenticationInPrivateModesEnabled));
+
+  if (profile->IsGuestSession()) {
+    return base::FeatureList::IsEnabled(
+               features::kEnableAmbientAuthenticationInGuestSession) ||
+           type == net::AmbientAuthAllowedProfileTypes::GUEST_AND_REGULAR ||
+           type == net::AmbientAuthAllowedProfileTypes::ALL;
+  } else if (profile->IsIncognitoProfile()) {
+    return base::FeatureList::IsEnabled(
+               features::kEnableAmbientAuthenticationInIncognito) ||
+           type == net::AmbientAuthAllowedProfileTypes::INCOGNITO_AND_REGULAR ||
+           type == net::AmbientAuthAllowedProfileTypes::ALL;
+  }
+
+  // Profile type not yet supported.
+  NOTREACHED();
+
+  return false;
+}
 
 }  // namespace
 
@@ -314,7 +343,7 @@ void ProfileNetworkContextService::RegisterLocalStatePrefs(
   registry->RegisterListPref(prefs::kHSTSPolicyBypassList);
   registry->RegisterIntegerPref(
       prefs::kAmbientAuthenticationInPrivateModesEnabled,
-      static_cast<int>(AmbientAuthAllowedProfileTypes::REGULAR_ONLY));
+      static_cast<int>(net::AmbientAuthAllowedProfileTypes::REGULAR_ONLY));
 }
 
 void ProfileNetworkContextService::DisableQuicIfNotAllowed() {
@@ -600,28 +629,14 @@ ProfileNetworkContextService::CreateNetworkContextParams(
   network_context_params->http_auth_static_network_context_params =
       network::mojom::HttpAuthStaticNetworkContextParams::New();
 
-  // Allow/disallow ambient authentication with default credentials based on the
-  // profile type.
-  // TODO(https://crbug.com/458508): Allow this behavior to be controllable by
-  // policy.
-  if (profile_->IsGuestSession()) {
-    network_context_params->http_auth_static_network_context_params
-        ->allow_default_credentials =
-        base::FeatureList::IsEnabled(
-            features::kEnableAmbientAuthenticationInGuestSession)
-            ? net::HttpAuthPreferences::ALLOW_DEFAULT_CREDENTIALS
-            : net::HttpAuthPreferences::DISALLOW_DEFAULT_CREDENTIALS;
-  } else if (profile_->IsIncognitoProfile()) {
+  if (IsAmbientAuthAllowedForProfile(profile_)) {
     network_context_params->http_auth_static_network_context_params
         ->allow_default_credentials =
-        base::FeatureList::IsEnabled(
-            features::kEnableAmbientAuthenticationInIncognito)
-            ? net::HttpAuthPreferences::ALLOW_DEFAULT_CREDENTIALS
-            : net::HttpAuthPreferences::DISALLOW_DEFAULT_CREDENTIALS;
+        net::HttpAuthPreferences::ALLOW_DEFAULT_CREDENTIALS;
   } else {
     network_context_params->http_auth_static_network_context_params
         ->allow_default_credentials =
-        net::HttpAuthPreferences::ALLOW_DEFAULT_CREDENTIALS;
+        net::HttpAuthPreferences::DISALLOW_DEFAULT_CREDENTIALS;
   }
 
   network_context_params->cookie_manager_params =

chrome/browser/net/profile_network_context_service.h

@@ -33,7 +33,17 @@ class TrialComparisonCertVerifierController;
 
 namespace net {
 class ClientCertStore;
-}
+
+// Enum that specifies which profiles are allowed to do
+// ambient authentication.
+enum class AmbientAuthAllowedProfileTypes {
+  REGULAR_ONLY = 0,
+  INCOGNITO_AND_REGULAR = 1,
+  GUEST_AND_REGULAR = 2,
+  ALL = 3,
+};
+
+}  // namespace net
 
 namespace user_prefs {
 class PrefRegistrySyncable;
@@ -94,7 +104,7 @@ class ProfileNetworkContextService
       ProfileNetworkContextServiceCertVerifierBuiltinFeaturePolicyTest,
       Test);
 
-  friend class AmbientAuthenticationTest;
+  friend class AmbientAuthenticationTestWithPolicy;
 
   // Checks |quic_allowed_|, and disables QUIC if needed.
   void DisableQuicIfNotAllowed();

chrome/browser/net/profile_network_context_service_browsertest.cc

@@ -23,16 +23,22 @@
 #include "chrome/browser/net/profile_network_context_service_factory.h"
 #include "chrome/browser/policy/policy_test_utils.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_window.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/search_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_service.h"
+#include "components/search_engines/template_url_service.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/content_features.h"
 #include "content/public/test/simple_url_loader_test_helper.h"
@@ -154,25 +160,104 @@ IN_PROC_BROWSER_TEST_F(ProfileNetworkContextServiceBrowsertest, BrotliEnabled) {
   EXPECT_TRUE(base::Contains(encodings, "br"));
 }
 
-enum class AmbientAuthenticationStateIncognito { ENABLED, DISABLED };
+enum class AmbientAuthProfileBit {
+  INCOGNITO = 1,
+  GUEST = 2,
+};
+
+// Indicates the state of the feature flags
+// |kEnableAmbientAuthenticationInIncognito| and
+// |kEnableAmbientAuthenticationInGuestSession|
+enum class AmbientAuthenticationFeatureState {
+  GUEST_OFF_INCOGNITO_OFF = 0,  // 00
+  GUEST_OFF_INCOGNITO_ON = 1,   // 01
+  GUEST_ON_INCOGNITO_OFF = 2,   // 10
+  GUEST_ON_INCOGNITO_ON = 3,    // 11
+};
 
-class AmbientAuthenticationTest : public InProcessBrowserTest {
+class AmbientAuthenticationTestWithPolicy
+    : public policy::PolicyTest,
+      public testing::WithParamInterface<AmbientAuthenticationFeatureState> {
  public:
-  explicit AmbientAuthenticationTest(
-      AmbientAuthenticationStateIncognito state =
-          AmbientAuthenticationStateIncognito::ENABLED) {
-    scoped_feature_list_.InitWithFeatureState(
-        features::kEnableAmbientAuthenticationInIncognito,
-        state == AmbientAuthenticationStateIncognito::ENABLED);
+  AmbientAuthenticationTestWithPolicy() {
+    feature_state_ = GetParam();
+    CookTheFeatureList();
+    policy::PolicyTest::SetUpInProcessBrowserTestFixture();
+  }
+
+  void IsAmbientAuthAllowedForProfilesTest() {
+    PrefService* service = g_browser_process->local_state();
+    int policy_value =
+        service->GetInteger(prefs::kAmbientAuthenticationInPrivateModesEnabled);
+
+    EXPECT_TRUE(IsAmbientAuthAllowedForProfile(GetRegularProfile()));
+    EXPECT_EQ(IsAmbientAuthAllowedForProfile(GetIncognitoProfile()),
+              IsIncognitoAllowedInFeature() ||
+                  IsIncognitoAllowedInPolicy(policy_value));
+// TODO(crbug.com/1030624): Adapt this test for chromeos guest sessions too once
+// we have device level policy for guests
+#if !defined(OS_CHROMEOS)
+    EXPECT_EQ(
+        IsAmbientAuthAllowedForProfile(GetGuestProfile()),
+        IsGuestAllowedInFeature() || IsGuestAllowedInPolicy(policy_value));
+#endif
+  }
+
+ protected:
+  inline bool IsIncognitoAllowedInFeature() const {
+    return static_cast<int>(feature_state_) &
+           static_cast<int>(AmbientAuthProfileBit::INCOGNITO);
+  }
+
+  inline bool IsGuestAllowedInFeature() const {
+    return static_cast<int>(feature_state_) &
+           static_cast<int>(AmbientAuthProfileBit::GUEST);
+  }
+
+  inline bool IsIncognitoAllowedInPolicy(int policy_value) const {
+    return policy_value & static_cast<int>(AmbientAuthProfileBit::INCOGNITO);
   }
 
-  Profile* GetOffTheRecordProfile() {
+  inline bool IsGuestAllowedInPolicy(int policy_value) const {
+    return policy_value & static_cast<int>(AmbientAuthProfileBit::GUEST);
+  }
+
+  void CookTheFeatureList() {
+    std::vector<base::Feature> enabled_feature_list;
+    std::vector<base::Feature> disabled_feature_list;
+
+    if (IsIncognitoAllowedInFeature())
+      enabled_feature_list.push_back(
+          features::kEnableAmbientAuthenticationInIncognito);
+    else
+      disabled_feature_list.push_back(
+          features::kEnableAmbientAuthenticationInIncognito);
+
+    if (IsGuestAllowedInFeature())
+      enabled_feature_list.push_back(
+          features::kEnableAmbientAuthenticationInGuestSession);
+    else
+      disabled_feature_list.push_back(
+          features::kEnableAmbientAuthenticationInGuestSession);
+
+    scoped_feature_list_.InitWithFeatures(enabled_feature_list,
+                                          disabled_feature_list);
+  }
+
+  Profile* GetIncognitoProfile() {
     Profile* regular_profile = browser()->profile();
     Profile* off_the_record_profile = regular_profile->GetOffTheRecordProfile();
     EXPECT_TRUE(regular_profile->HasOffTheRecordProfile());
     return off_the_record_profile;
   }
 
+  Profile* GetGuestProfile() {
+    Profile* guest_profile = OpenGuestBrowser()->profile();
+    return guest_profile;
+  }
+
+  Profile* GetRegularProfile() { return browser()->profile(); }
+
   bool IsAmbientAuthAllowedForProfile(Profile* profile) {
     ProfileNetworkContextService* profile_network_context_service =
         ProfileNetworkContextServiceFactory::GetForContext(profile);
@@ -184,32 +269,77 @@ class AmbientAuthenticationTest : public InProcessBrowserTest {
                ->allow_default_credentials ==
            net::HttpAuthPreferences::ALLOW_DEFAULT_CREDENTIALS;
   }
+  // OpenGuestBrowser method code borrowed from
+  // chrome/browser/profiles/profile_window_browsertest.cc
+  Browser* OpenGuestBrowser() {
+    size_t num_browsers = BrowserList::GetInstance()->size();
+
+    // Create a guest browser nicely. Using CreateProfile() and CreateBrowser()
+    // does incomplete initialization that would lead to
+    // SystemUrlRequestContextGetter being leaked.
+    profiles::SwitchToGuestProfile(ProfileManager::CreateCallback());
+    ui_test_utils::WaitForBrowserToOpen();
+
+    DCHECK_NE(static_cast<Profile*>(nullptr),
+              g_browser_process->profile_manager()->GetProfileByPath(
+                  ProfileManager::GetGuestProfilePath()));
+    EXPECT_EQ(num_browsers + 1, BrowserList::GetInstance()->size());
+
+    Profile* guest = g_browser_process->profile_manager()->GetProfileByPath(
+        ProfileManager::GetGuestProfilePath());
+    Browser* browser = chrome::FindAnyBrowser(guest, true);
+    EXPECT_TRUE(browser);
+
+    // When |browser| closes a BrowsingDataRemover will be created and executed.
+    // It needs a loaded TemplateUrlService or else it hangs on to a
+    // CallbackList::Subscription forever.
+    search_test_utils::WaitForTemplateURLServiceToLoad(
+        TemplateURLServiceFactory::GetForProfile(guest));
+
+    return browser;
+  }
 
- protected:
+  void EnablePolicyWithValue(net::AmbientAuthAllowedProfileTypes value) {
+    SetPolicy(&policies_,
+              policy::key::kAmbientAuthenticationInPrivateModesEnabled,
+              std::make_unique<base::Value>(static_cast<int>(value)));
+    UpdateProviderPolicy(policies_);
+  }
+
+ private:
   base::test::ScopedFeatureList scoped_feature_list_;
+  AmbientAuthenticationFeatureState feature_state_;
+  policy::PolicyMap policies_;
 };
 
-class AmbientAuthenticationTestDisabled : public AmbientAuthenticationTest {
- public:
-  explicit AmbientAuthenticationTestDisabled()
-      : AmbientAuthenticationTest(
-            AmbientAuthenticationStateIncognito::DISABLED) {}
-};
+INSTANTIATE_TEST_CASE_P(
+    ,
+    AmbientAuthenticationTestWithPolicy,
+    testing::Values(AmbientAuthenticationFeatureState::GUEST_OFF_INCOGNITO_OFF,
+                    AmbientAuthenticationFeatureState::GUEST_OFF_INCOGNITO_ON,
+                    AmbientAuthenticationFeatureState::GUEST_ON_INCOGNITO_OFF,
+                    AmbientAuthenticationFeatureState::GUEST_ON_INCOGNITO_ON));
+
+IN_PROC_BROWSER_TEST_P(AmbientAuthenticationTestWithPolicy, RegularOnly) {
+  EnablePolicyWithValue(net::AmbientAuthAllowedProfileTypes::REGULAR_ONLY);
+  IsAmbientAuthAllowedForProfilesTest();
+}
 
-IN_PROC_BROWSER_TEST_F(AmbientAuthenticationTest,
-                       AllowDefaultCredentialsRegular) {
-  EXPECT_TRUE(IsAmbientAuthAllowedForProfile(browser()->profile()));
+IN_PROC_BROWSER_TEST_P(AmbientAuthenticationTestWithPolicy,
+                       IncognitoAndRegular) {
+  EnablePolicyWithValue(
+      net::AmbientAuthAllowedProfileTypes::INCOGNITO_AND_REGULAR);
+  IsAmbientAuthAllowedForProfilesTest();
 }
 
-IN_PROC_BROWSER_TEST_F(AmbientAuthenticationTest,
-                       AllowDefaultCredentialsIncognito) {
-  EXPECT_TRUE(IsAmbientAuthAllowedForProfile(GetOffTheRecordProfile()));
+IN_PROC_BROWSER_TEST_P(AmbientAuthenticationTestWithPolicy, GuestAndRegular) {
+  EnablePolicyWithValue(net::AmbientAuthAllowedProfileTypes::GUEST_AND_REGULAR);
+  IsAmbientAuthAllowedForProfilesTest();
 }
 
-IN_PROC_BROWSER_TEST_F(AmbientAuthenticationTestDisabled,
-                       DisallowDefaultCredentialsIncognito) {
-  EXPECT_TRUE(IsAmbientAuthAllowedForProfile(browser()->profile()));
-  EXPECT_FALSE(IsAmbientAuthAllowedForProfile(GetOffTheRecordProfile()));
+IN_PROC_BROWSER_TEST_P(AmbientAuthenticationTestWithPolicy, All) {
+  EnablePolicyWithValue(net::AmbientAuthAllowedProfileTypes::ALL);
+  IsAmbientAuthAllowedForProfilesTest();
 }
 
 // Test subclass that adds switches::kDiskCacheDir and switches::kDiskCacheSize