Child users: Persist parent approvals for extensions across sessions

If a supervised user tries to install an extension, the parent
approval dialog appears requiring the parent's password. Currently, if
the supervised user signs out and signs back in, the parent approval
is not remembered. On the chrome://extensions page, all installed
extensions go back to being force-disabled. If the supervised user
clicks on the enable toggle, the parent approval dialog appears again
asking for the parent's password. This behavior is undesirable.

This CL fixes this bug by using a Chrome pref instead of
SupervisedUserSettingsService to sync and persist the map of approved
extensions across sessions.

Bug: 1065765
Change-Id: Ia92ec980b6eccf9381e3645ddf55a5cc7be09ceb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2127986
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756147}

chrome/browser/extensions/api/management/chrome_management_api_delegate.cc

@@ -592,7 +592,7 @@ void ChromeManagementAPIDelegate::EnableExtension(
   // for, and received parent permission to install the extension.
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForBrowserContext(context);
-  supervised_user_service->AddExtensionApproval(*extension);
+  supervised_user_service->AddOrUpdateExtensionApproval(*extension);
 #endif
 
   // If the extension was disabled for a permissions increase, the Management

chrome/browser/extensions/api/webstore_private/webstore_private_api.cc

@@ -509,7 +509,7 @@ void WebstorePrivateBeginInstallWithManifest3Function::
     OnParentPermissionReceived() {
   SupervisedUserService* service =
       SupervisedUserServiceFactory::GetForProfile(chrome_details_.GetProfile());
-  service->AddExtensionApproval(*dummy_extension_);
+  service->AddOrUpdateExtensionApproval(*dummy_extension_);
 
   HandleInstallProceed();
   Release();  // Matches the AddRef in Run().

chrome/browser/supervised_user/supervised_user_constants.cc

@@ -10,9 +10,6 @@ namespace supervised_users {
 const char kAccountConsistencyMirrorRequired[] =
     "AccountConsistencyMirrorRequired";
 #endif
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-const char kApprovedExtensions[] = "ApprovedExtensions";
-#endif
 const char kAuthorizationHeaderFormat[] = "Bearer %s";
 const char kCameraMicDisabled[] = "CameraMicDisabled";
 const char kContentPackDefaultFilteringBehavior[] =

chrome/browser/supervised_user/supervised_user_constants.h

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_SUPERVISED_USER_SUPERVISED_USER_CONSTANTS_H_
 #define CHROME_BROWSER_SUPERVISED_USER_SUPERVISED_USER_CONSTANTS_H_
 
-#include "extensions/buildflags/buildflags.h"
-
 namespace supervised_users {
 
 // Keys for supervised user settings. These are configured remotely and mapped
@@ -14,9 +12,6 @@ namespace supervised_users {
 #if defined(OS_CHROMEOS)
 extern const char kAccountConsistencyMirrorRequired[];
 #endif
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-extern const char kApprovedExtensions[];
-#endif
 extern const char kAuthorizationHeaderFormat[];
 extern const char kCameraMicDisabled[];
 extern const char kContentPackDefaultFilteringBehavior[];

chrome/browser/supervised_user/supervised_user_extensions_metrics_recorder.cc

@@ -19,16 +19,16 @@ constexpr char kRemovedActionName[] = "SupervisedUsers_Extensions_Removed";
 
 // static
 void SupervisedUserExtensionsMetricsRecorder::RecordExtensionsUmaMetrics(
-    syncer::SyncChange::SyncChangeType type) {
+    SupervisedUserService::ApprovedExtensionChange type) {
   switch (type) {
-    case syncer::SyncChange::ACTION_ADD:
+    case SupervisedUserService::ApprovedExtensionChange::kNew:
       // Record UMA metrics for custodian approval for a new extension.
       base::RecordAction(
           base::UserMetricsAction(kNewExtensionApprovalGrantedActionName));
       base::UmaHistogramEnumeration(
           kHistogramName, UmaExtensionState::kNewExtensionApprovalGranted);
       break;
-    case syncer::SyncChange::ACTION_UPDATE:
+    case SupervisedUserService::ApprovedExtensionChange::kUpdate:
       // Record UMA metrics for child approval for a newer version of an
       // existing extension.
       base::RecordAction(
@@ -36,14 +36,11 @@ void SupervisedUserExtensionsMetricsRecorder::RecordExtensionsUmaMetrics(
       base::UmaHistogramEnumeration(
           kHistogramName, UmaExtensionState::kNewVersionApprovalGranted);
       break;
-    case syncer::SyncChange::ACTION_DELETE:
+    case SupervisedUserService::ApprovedExtensionChange::kRemove:
       // Record UMA metrics for removing an extension.
       base::RecordAction(base::UserMetricsAction(kRemovedActionName));
       base::UmaHistogramEnumeration(kHistogramName,
                                     UmaExtensionState::kRemoved);
       break;
-    case syncer::SyncChange::ACTION_INVALID:
-      NOTREACHED();
-      break;
   }
 }

chrome/browser/supervised_user/supervised_user_extensions_metrics_recorder.h

@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_SUPERVISED_USER_SUPERVISED_USER_EXTENSIONS_METRICS_RECORDER_H_
 #define CHROME_BROWSER_SUPERVISED_USER_SUPERVISED_USER_EXTENSIONS_METRICS_RECORDER_H_
 
-#include "components/sync/model/sync_change.h"
+#include "chrome/browser/supervised_user/supervised_user_service.h"
 
 // Records UMA metrics for child users using extensions.
 // TODO(tobyhuang): Reevaluate if this class should be converted to a namespace
@@ -38,7 +38,7 @@ class SupervisedUserExtensionsMetricsRecorder {
       const SupervisedUserExtensionsMetricsRecorder&) = delete;
 
   static void RecordExtensionsUmaMetrics(
-      syncer::SyncChange::SyncChangeType type);
+      SupervisedUserService::ApprovedExtensionChange type);
 };
 
 #endif  // CHROME_BROWSER_SUPERVISED_USER_SUPERVISED_USER_EXTENSIONS_METRICS_RECORDER_H_

chrome/browser/supervised_user/supervised_user_pref_store.cc

@@ -52,16 +52,20 @@ SupervisedUserSettingsPrefMappingEntry kSupervisedUserSettingsPrefMapping[] = {
         prefs::kSupervisedUserManualURLs,
     },
     {
-        supervised_users::kForceSafeSearch, prefs::kForceGoogleSafeSearch,
+        supervised_users::kForceSafeSearch,
+        prefs::kForceGoogleSafeSearch,
     },
     {
-        supervised_users::kSafeSitesEnabled, prefs::kSupervisedUserSafeSites,
+        supervised_users::kSafeSitesEnabled,
+        prefs::kSupervisedUserSafeSites,
     },
     {
-        supervised_users::kSigninAllowed, prefs::kSigninAllowed,
+        supervised_users::kSigninAllowed,
+        prefs::kSigninAllowed,
     },
     {
-        supervised_users::kUserName, prefs::kProfileName,
+        supervised_users::kUserName,
+        prefs::kProfileName,
     },
 };
 

chrome/browser/supervised_user/supervised_user_service.h

@@ -32,7 +32,6 @@
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "base/strings/string16.h"
 #include "chrome/browser/ui/supervised_user/parent_permission_dialog.h"
-#include "components/sync/model/sync_change.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/management_policy.h"
@@ -40,6 +39,7 @@
 
 class Browser;
 class PermissionRequestCreator;
+class PrefService;
 class Profile;
 class SupervisedUserServiceObserver;
 class SupervisedUserSettingsService;
@@ -86,6 +86,20 @@ class SupervisedUserService : public KeyedService,
     virtual bool SetActive(bool active) = 0;
   };
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  // These enum values represent operations to manage the
+  // kSupervisedUserApprovedExtensions user pref, which maps extension ids to
+  // approved versions.
+  enum class ApprovedExtensionChange {
+    // Adds a new approved extension to the pref.
+    kNew,
+    // Updates the version of an already-approved extension.
+    kUpdate,
+    // Removes extension approval.
+    kRemove
+  };
+#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
+
   ~SupervisedUserService() override;
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
@@ -191,20 +205,19 @@ class SupervisedUserService : public KeyedService,
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   // Updates the map of approved extensions to add approval for |extension|.
-  void AddExtensionApproval(const extensions::Extension& extension);
+  void AddOrUpdateExtensionApproval(const extensions::Extension& extension);
 
   // Updates the map of approved extensions to remove approval for |extension|.
   void RemoveExtensionApproval(const extensions::Extension& extension);
 
-  // Updates the map of approved extensions.
-  // If possible, use the simpler methods declared above.
-  // If |type| is SyncChangeType::ADD, then add custodian approval for enabling
-  // the extension by adding the approved version to the map of approved
-  // extensions. If |type| is SyncChangeType::DELETE, then remove the extension
-  // from the map of approved extensions.
-  void UpdateApprovedExtensions(const std::string& extension_id,
-                                const std::string& version,
-                                syncer::SyncChange::SyncChangeType type);
+  // Simulates a custodian or child approval for enabling the extension coming
+  // in through Sync by adding the approved version to the map of approved
+  // extensions. Removes approval by passing in
+  // ApprovedExtensionChange::kRemove. It doesn't simulate a change in the
+  // disable reasons.
+  void UpdateApprovedExtensionForTesting(const std::string& extension_id,
+                                         const std::string& version,
+                                         ApprovedExtensionChange type);
 
   bool GetSupervisedUserExtensionsMayRequestPermissionsPref() const;
 
@@ -276,16 +289,37 @@ class SupervisedUserService : public KeyedService,
   // "Permissions for sites, apps and extensions" toggle.
   bool ShouldBlockExtension(const std::string& extension_id) const;
 
-  // Extensions helper to SetActive().
-  void SetExtensionsActive();
-
   // Enables/Disables extensions upon change in approved version of the
   // extension_id. This function is idempotent.
   void ChangeExtensionStateIfNecessary(const std::string& extension_id);
+
+  // Updates the map of approved extensions.
+  // Use AddOrUpdateExtensionApproval() or RemoveExtensionApproval() for public
+  // access.
+  // If |type| is kNew, then adds custodian approval for enabling the extension
+  // by adding the approved version to the map of approved extensions.
+  // If |type| is kUpdate, then updates the approved version for the extension
+  // in the map.
+  // If |type| is kRemove, then removes the extension from the map of approved
+  // extensions.
+  void UpdateApprovedExtension(const std::string& extension_id,
+                               const std::string& version,
+                               ApprovedExtensionChange type);
+
+  // Updates the map of approved extensions when the corresponding preference is
+  // changed.
+  void UpdateApprovedExtensions();
+
+  // Extensions helper to SetActive().
+  void SetExtensionsActive();
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
+  // Returns the SupervisedUserSettingsService associated with |profile_|.
   SupervisedUserSettingsService* GetSettingsService();
 
+  // Returns the PrefService associated with |profile_|.
+  PrefService* GetPrefService();
+
   size_t FindEnabledPermissionRequestCreator(size_t start);
   void AddPermissionRequestInternal(
       const CreatePermissionRequestCallback& create_request,

chrome/common/pref_names.cc

@@ -128,6 +128,13 @@ const char kURLsToRestoreOnStartup[] = "session.startup_urls";
 // Boolean that is true when user feedback to Google is allowed.
 const char kUserFeedbackAllowed[] = "feedback_allowed";
 
+#if BUILDFLAG(ENABLE_SUPERVISED_USERS) && BUILDFLAG(ENABLE_EXTENSIONS)
+// DictionaryValue that maps extension ids to the approved version of this
+// extension for a supervised user. Missing extensions are not approved.
+const char kSupervisedUserApprovedExtensions[] =
+    "profile.managed.approved_extensions";
+#endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS) && BUILDFLAG(ENABLE_EXTENSIONS)
+
 // Stores the email address associated with the google account of the custodian
 // of the supervised user, set when the supervised user is created.
 const char kSupervisedUserCustodianEmail[] = "profile.managed.custodian_email";

chrome/common/pref_names.h

@@ -43,6 +43,9 @@ extern const char kSessionExitedCleanly[];
 extern const char kSessionExitType[];
 extern const char kObservedSessionTime[];
 extern const char kSiteEngagementLastUpdateTime[];
+#if BUILDFLAG(ENABLE_SUPERVISED_USERS) && BUILDFLAG(ENABLE_EXTENSIONS)
+extern const char kSupervisedUserApprovedExtensions[];
+#endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS) && BUILDFLAG(ENABLE_EXTENSIONS)
 extern const char kSupervisedUserCustodianEmail[];
 extern const char kSupervisedUserCustodianName[];
 extern const char kSupervisedUserCustodianObfuscatedGaiaId[];