Enable login screen apps.

chrome/browser/extensions/chrome_process_manager_delegate.cc

 // Copyright 2014 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/extensions/chrome_process_manager_delegate.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/notification_service.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/browser/process_manager_factory.h"
 #include "extensions/common/one_shot_event.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#endif
+
 namespace extensions {
 
 ChromeProcessManagerDelegate::ChromeProcessManagerDelegate() {
   registrar_.Add(this,
                  chrome::NOTIFICATION_BROWSER_WINDOW_READY,
                  content::NotificationService::AllSources());
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_CREATED,
                  content::NotificationService::AllSources());
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::NotificationService::AllSources());
 }
 
 ChromeProcessManagerDelegate::~ChromeProcessManagerDelegate() {
 }
 
 bool ChromeProcessManagerDelegate::IsBackgroundPageAllowed(
-    content::BrowserContext* context) const {
+    content::BrowserContext* context,
+    const Extension* extension) const {
   Profile* profile = static_cast<Profile*>(context);
 
   bool is_normal_session = !profile->IsGuestSession() &&

[emaxx, 2017/03/06 17:04:03]

As per offline discussion and as per previous iterations, I'd personally find
the version with separate branches more understandable:
if (is_signin_profile) {
  ...
} else {
  ... // consider non-signin profile: calculate is_normal_profile
}

[achuithb, 2017/03/06 18:21:05]

I've re-added the else statment. My objection to this is that booleans and
if-else statements are easy to get wrong, if you forget to update a global
boolean in all paths, as we just did; and I didn't spot the error after looking
at this code dozens of times, and only saw it after printing out all the
booleans in this function, and observing that is_normal_session is true for the
signin screen.

The other benefit is that we preserve the old behavior. I'm interested in
limiting the changes we make to the logic in this delegate function.

[emaxx, 2017/03/07 01:36:42]

It's your call anyway, but in the end I think we both agree on the same goal:
make the signin profile case and the normal user case as independent as possible
(because the flags and conditions that make sense for one don't for the other).

The other option I may suggest is sacrificing the single returning point, and
doing the return statement from inside the "is_signin_profile" block directly.
Then we won't have to take care of is_normal_session inside this block, and
won't have to modify the code outside this block. WDYT?

[achuithb, 2017/03/07 14:17:39]

I like this idea. It isolates our changes in this function and makes it less
likely that this CL have some side effect. Done.

                            !profile->IsSystemProfile();
+  bool allow_for_login_profile = false;
+
 #if defined(OS_CHROMEOS)
-  is_normal_session = is_normal_session &&
-                      user_manager::UserManager::Get()->IsUserLoggedIn();
+  is_normal_session =
+      is_normal_session && user_manager::UserManager::Get()->IsUserLoggedIn();
+
+  const bool is_signin_profile =
+      chromeos::ProfileHelper::IsSigninProfile(profile) &&
+      !profile->IsOffTheRecord();
+
+  if (is_signin_profile) {
+    is_normal_session = false;
+
+    // Check for flag.
+    const bool login_screen_apps_enabled =

[emaxx, 2017/03/06 17:04:03]

I'm still feeling that this check here is excessive: we already have the check
against the enterprise policy (login_screen_apps_installed,
is_login_screen_app). As we may trust to the policy stack that the policy can't
be spoofed (its signature is validated), and as the policy won't be exposed to
anyone until we implement everything, I don't think there's much value in
keeping this condition here.

[achuithb, 2017/03/06 18:21:05]

I prefer to keep this for the following reasons:
1. It's a failsafe in case something else in our logic is wrong. If somehow a
login screen app gets side-installed, this will at least prevent it's background
page from launching. Our understanding of the various pathways in the extensions
code is still not complete, so I think being a bit more paranoid is fine. 
2. There's no downside to this check, and we can have faith that all the rest of
the logic after this can be easily turned off if something goes wrong.
3. This is a temporary flag, so this is just a short time nuisance at worst.

[emaxx, 2017/03/07 01:36:42]

Acknowledged.

+        base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kEnableLoginScreenApps);
+
+    // Check that login screen apps are installed by policy.
+    std::unique_ptr<base::DictionaryValue> login_screen_apps_list =
+        ExtensionManagementFactory::GetForBrowserContext(context)
+            ->GetForceInstallList();
+    const bool login_screen_apps_installed = !login_screen_apps_list->empty();
+
+    // Check match of extension id (if specified) with login screen app list.
+    bool is_login_screen_app = true;
+    if (login_screen_apps_installed && extension)
+      is_login_screen_app = login_screen_apps_list->HasKey(extension->id());

[emaxx, 2017/03/06 17:04:04]

nit: #include "extensions/common/extension.h"

[achuithb, 2017/03/06 18:21:05]

Done.

+
+    allow_for_login_profile = login_screen_apps_enabled &&
+                              login_screen_apps_installed &&
+                              is_login_screen_app;
+  }
 #endif
 
   // Disallow if the current session is a Guest mode session or login screen but
   // the current browser context is *not* off-the-record. Such context is
   // artificial and background page shouldn't be created in it.
   // http://crbug.com/329498
-  return is_normal_session || profile->IsOffTheRecord();
+  return is_normal_session || profile->IsOffTheRecord() ||
+         allow_for_login_profile;
 }
 
 bool ChromeProcessManagerDelegate::DeferCreatingStartupBackgroundHosts(
     content::BrowserContext* context) const {
   Profile* profile = static_cast<Profile*>(context);
 
   // The profile may not be valid yet if it is still being initialized.
   // In that case, defer loading, since it depends on an initialized profile.
   // Background hosts will be loaded later via NOTIFICATION_PROFILE_CREATED.
   // http://crbug.com/222473
@@ skipping 97 matching lines @@
     ProcessManager* incognito_manager =
         ProcessManagerFactory::GetForBrowserContextIfExists(
             profile->GetOffTheRecordProfile());
     if (incognito_manager) {
       incognito_manager->CloseBackgroundHosts();
     }
   }
 }
 
 }  // namespace extensions