Used ExtensionsPermissionsTracker to decide the login screen warning

1. Added Host permissions to trigger the full warning
2. Switched the login screen warning of the managed-guest session to be decided using ExtensionsPermissionsTracker
3. Modified 3 browser tests related to the old logic

Bug: 1015378
Change-Id: Icd8cf9a72a6a2cbed58e4d72260e2f62bc79c30e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090418
Commit-Queue: Aya Elsayed <ayaelattar@google.com>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751314}

chrome/browser/chromeos/extensions/extensions_permissions_tracker.cc

@@ -72,20 +72,25 @@ void ExtensionsPermissionsTracker::OnForcedExtensionsPrefChanged() {
 
 bool ExtensionsPermissionsTracker::IsSafePerms(
     const PermissionsData* perms_data) const {
-  APIPermissionSet api_permissions =
-      perms_data->active_permissions().apis().Clone();
+  const PermissionSet& active_permissions = perms_data->active_permissions();
+  const APIPermissionSet& api_permissions = active_permissions.apis();
   for (auto* permission : api_permissions) {
     if (!permission->info()->requires_managed_session_full_login_warning()) {
       return false;
     }
   }
-  ManifestPermissionSet manifest_permissions =
-      perms_data->active_permissions().manifest_permissions().Clone();
+  const ManifestPermissionSet& manifest_permissions =
+      active_permissions.manifest_permissions();
   for (const auto* permission : manifest_permissions) {
     if (!permission->RequiresManagedSessionFullLoginWarning()) {
       return false;
     }
   }
+  if (active_permissions.ShouldWarnAllHosts() ||
+      !active_permissions.effective_hosts().is_empty()) {
+    return false;
+  }
+
   return true;
 }
 

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -215,16 +215,6 @@ bool IsManagedSessionEnabled(policy::DeviceLocalAccountPolicyBroker* broker) {
   return entry->value && entry->value->GetBool();
 }
 
-base::span<const base::Value> GetListPolicyValue(
-    const policy::PolicyMap& policy_map,
-    const char* policy_key) {
-  const policy::PolicyMap::Entry* entry = policy_map.Get(policy_key);
-  if (!entry || !entry->value || !entry->value->is_list())
-    return {};
-
-  return entry->value->GetList();
-}
-
 bool AreRiskyPoliciesUsed(policy::DeviceLocalAccountPolicyBroker* broker) {
   const policy::PolicyMap& policy_map = broker->core()->store()->policy_map();
   for (const auto& it : policy_map) {
@@ -254,39 +244,6 @@ bool IsProxyUsed(const PrefService* local_state_prefs) {
   return mode != ProxyPrefs::MODE_DIRECT;
 }
 
-bool AreRiskyExtensionsForceInstalled(
-    policy::DeviceLocalAccountPolicyBroker* broker) {
-  const policy::PolicyMap& policy_map = broker->core()->store()->policy_map();
-
-  auto forcelist =
-      GetListPolicyValue(policy_map, policy::key::kExtensionInstallForcelist);
-
-  // Extension is risky if it's present in force-installed extensions and is not
-  // whitelisted for public sessions.
-
-  if (forcelist.empty())
-    return false;
-
-  for (const base::Value& extension : forcelist) {
-    if (!extension.is_string())
-      continue;
-
-    // Each extension entry contains an extension id and optional update URL
-    // separated by ';'.
-    std::vector<std::string> extension_items =
-        base::SplitString(extension.GetString(), ";", base::TRIM_WHITESPACE,
-                          base::SPLIT_WANT_NONEMPTY);
-    if (extension_items.empty())
-      continue;
-
-    // If current force-installed extension is not whitelisted for public
-    // sessions, consider the extension risky.
-    if (!extensions::IsWhitelistedForPublicSession(extension_items[0]))
-      return true;
-  }
-  return false;
-}
-
 bool PolicyHasWebTrustedAuthorityCertificate(
     policy::DeviceLocalAccountPolicyBroker* broker) {
   return policy::UserNetworkConfigurationUpdater::
@@ -1468,7 +1425,8 @@ bool ChromeUserManagerImpl::IsFullManagementDisclosureNeeded(
     policy::DeviceLocalAccountPolicyBroker* broker) const {
   return IsManagedSessionEnabled(broker) &&
          (AreRiskyPoliciesUsed(broker) ||
-          AreRiskyExtensionsForceInstalled(broker) ||
+          g_browser_process->local_state()->GetBoolean(
+              prefs::kManagedSessionUseFullLoginWarning) ||
           PolicyHasWebTrustedAuthorityCertificate(broker) ||
           IsProxyUsed(GetLocalState()));
 }

chrome/browser/chromeos/policy/device_local_account_browsertest.cc

@@ -151,6 +151,7 @@
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
 #include "net/base/url_util.h"
 #include "net/http/http_status_code.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -209,9 +210,6 @@ const char kHostedAppVersion[] = "1.0.0.0";
 const char kGoodExtensionID[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
 const char kGoodExtensionCRXPath[] = "extensions/good.crx";
 const char kGoodExtensionVersion[] = "1.0";
-// Chrome RDP extension
-const char kPublicSessionWhitelistedExtensionID[] =
-    "cbkkbcmdlboombapidmoeolnmdacpkch";
 const char kPackagedAppCRXPath[] = "extensions/platform_apps/app_window_2.crx";
 const char kShowManagedStorageID[] = "ongnjlefhnoajpbodoldndkbkdgfomlp";
 const char kShowManagedStorageCRXPath[] = "extensions/show_managed_storage.crx";
@@ -2508,6 +2506,10 @@ class ManagedSessionsTest : public DeviceLocalAccountTest {
         kGoodExtensionID, kGoodExtensionVersion,
         embedded_test_server()->GetURL(std::string("/") +
                                        kGoodExtensionCRXPath));
+    testing_update_manifest_provider->AddUpdate(
+        kShowManagedStorageID, kShowManagedStorageVersion,
+        embedded_test_server()->GetURL(std::string("/") +
+                                       kShowManagedStorageCRXPath));
     embedded_test_server()->RegisterRequestHandler(
         base::BindRepeating(&TestingUpdateManifestProvider::HandleRequest,
                             testing_update_manifest_provider));
@@ -2534,7 +2536,7 @@ class ManagedSessionsTest : public DeviceLocalAccountTest {
   void AddForceInstalledExtension() { AddExtension(kGoodExtensionID); }
 
   void AddForceInstalledWhitelistedExtension() {
-    AddExtension(kPublicSessionWhitelistedExtensionID);
+    AddExtension(kShowManagedStorageID);
   }
 
   void WaitForCertificateUpdate() {
@@ -2605,6 +2607,16 @@ IN_PROC_BROWSER_TEST_F(ManagedSessionsTest, ManagedSessionsEnabledNonRisky) {
       chromeos::ChromeUserManager::Get()->IsManagedSessionEnabledForUser(
           *user));
 
+  // Management disclosure warning is shown in the beginning, because
+  // kManagedSessionUseFullLoginWarning pref is set to true in the beginning.
+  ASSERT_TRUE(
+      chromeos::ChromeUserManager::Get()->IsFullManagementDisclosureNeeded(
+          broker));
+
+  ASSERT_NO_FATAL_FAILURE(StartLogin(std::string(), std::string()));
+  WaitForSessionStart();
+
+  // After the login, kManagedSessionUseFullLoginWarning pref is updated.
   // Check that management disclosure warning is not shown when managed sessions
   // are enabled, but policy settings are not risky.
   ASSERT_FALSE(
@@ -2635,6 +2647,20 @@ IN_PROC_BROWSER_TEST_F(ManagedSessionsTest, ForceInstalledExtension) {
       chromeos::ChromeUserManager::Get()->IsManagedSessionEnabledForUser(
           *user));
 
+  // Management disclosure warning is shown in the beginning, because
+  // kManagedSessionUseFullLoginWarning pref is set to true in the beginning.
+  ASSERT_TRUE(
+      chromeos::ChromeUserManager::Get()->IsFullManagementDisclosureNeeded(
+          broker));
+
+  ExtensionInstallObserver install_observer(kGoodExtensionID);
+
+  ASSERT_NO_FATAL_FAILURE(StartLogin(std::string(), std::string()));
+  WaitForSessionStart();
+
+  install_observer.Wait();
+
+  // After the login, kManagedSessionUseFullLoginWarning pref is updated.
   // Check that force-installed extension activates managed session mode for
   // device-local users.
   EXPECT_TRUE(
@@ -2665,6 +2691,20 @@ IN_PROC_BROWSER_TEST_F(ManagedSessionsTest, WhitelistedExtension) {
       chromeos::ChromeUserManager::Get()->IsManagedSessionEnabledForUser(
           *user));
 
+  // Management disclosure warning is shown in the beginning, because
+  // kManagedSessionUseFullLoginWarning pref is set to true in the beginning.
+  ASSERT_TRUE(
+      chromeos::ChromeUserManager::Get()->IsFullManagementDisclosureNeeded(
+          broker));
+
+  ExtensionInstallObserver install_observer(kShowManagedStorageID);
+
+  ASSERT_NO_FATAL_FAILURE(StartLogin(std::string(), std::string()));
+  WaitForSessionStart();
+
+  install_observer.Wait();
+
+  // After the login, kManagedSessionUseFullLoginWarning pref is updated.
   // Check that white-listed extension is not considered risky and doesn't
   // activate managed session mode.
   EXPECT_FALSE(