Handle incognito mode properly in iOS IdentityManager factories

https://chromium-review.googlesource.com/c/chromium/src/+/1090277
fixed //chrome's IdentityManagerFactory to properly handle incognito
Profiles. The factories in //ios need to be fixed as well.

After discussion with sdefresne@, I now realize that it's a safer
pattern to have the //chrome-level class tying IdentityManager to
KeyedService *wrap* IdentityManager rather than *hold* IdentityManager.
The concrete reason is that that way all of the KeyedServiceFactory
methods will Just Work as expected rather than needing custom code
in the factory subclass to deal with the holder (as I had previously
directed should be added to //chrome for dealing with incognito
Profiles).

This CL thus changes all the IdentityManager factories to have a
private *subclass* of IdentityManager rather than a private *holder* of
IdentityManager. By doing this, we also ensure that incognito mode will
now be handled properly in the iOS IdentityManager factories without
requiring any custom code (i.e., it will simply use the default
KeyedServiceFactory infrastructure, which returns nullptr from
GetServiceForBrowserState() when the browser state is incognito).

Bug: 796544
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I2a11fafd28f9c1003a40bcfd2cea50403276ea90
Reviewed-on: https://chromium-review.googlesource.com/1092497Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566788}

chrome/browser/signin/identity_manager_factory.cc

@@ -14,22 +14,20 @@
 #include "components/signin/core/browser/signin_manager.h"
 #include "services/identity/public/cpp/identity_manager.h"
 
-// Class that wraps IdentityManager in a KeyedService (as IdentityManager is a
-// client-side library intended for use by any process, it would be a layering
+// Subclass that wraps IdentityManager in a KeyedService (as IdentityManager is
+// a client-side library intended for use by any process, it would be a layering
 // violation for IdentityManager itself to have direct knowledge of
 // KeyedService).
-class IdentityManagerHolder : public KeyedService {
+// NOTE: Do not add any code here that further ties IdentityManager to Profile
+// without communicating with {blundell, sdefresne}@chromium.org.
+class IdentityManagerWrapper : public KeyedService,
+                               public identity::IdentityManager {
  public:
-  explicit IdentityManagerHolder(Profile* profile)
-      : identity_manager_(
+  explicit IdentityManagerWrapper(Profile* profile)
+      : identity::IdentityManager(
             SigninManagerFactory::GetForProfile(profile),
             ProfileOAuth2TokenServiceFactory::GetForProfile(profile),
             AccountTrackerServiceFactory::GetForProfile(profile)) {}
-
-  identity::IdentityManager* identity_manager() { return &identity_manager_; }
-
- private:
-  identity::IdentityManager identity_manager_;
 };
 
 IdentityManagerFactory::IdentityManagerFactory()
@@ -46,18 +44,8 @@ IdentityManagerFactory::~IdentityManagerFactory() {}
 // static
 identity::IdentityManager* IdentityManagerFactory::GetForProfile(
     Profile* profile) {
-  // IdentityManager is nullptr in incognito mode by design, which will
-  // concretely manifest in |holder| being nullptr below. Handle that case by
-  // short-circuiting out early. This is the only case in which |holder| is
-  // expected to be nullptr.
-  if (profile->IsOffTheRecord())
-    return nullptr;
-
-  IdentityManagerHolder* holder = static_cast<IdentityManagerHolder*>(
+  return static_cast<IdentityManagerWrapper*>(
       GetInstance()->GetServiceForBrowserContext(profile, true));
-  DCHECK(holder);
-
-  return holder->identity_manager();
 }
 
 // static
@@ -67,5 +55,5 @@ IdentityManagerFactory* IdentityManagerFactory::GetInstance() {
 
 KeyedService* IdentityManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new IdentityManagerHolder(Profile::FromBrowserContext(context));
+  return new IdentityManagerWrapper(Profile::FromBrowserContext(context));
 }

ios/chrome/browser/signin/identity_manager_factory.cc

@@ -15,23 +15,22 @@
 #include "ios/chrome/browser/signin/signin_manager_factory.h"
 #include "services/identity/public/cpp/identity_manager.h"
 
-// Class that wraps IdentityManager in a KeyedService (as IdentityManager is a
-// client-side library intended for use by any process, it would be a layering
+// Subclass that wraps IdentityManager in a KeyedService (as IdentityManager is
+// a client-side library intended for use by any process, it would be a layering
 // violation for IdentityManager itself to have direct knowledge of
 // KeyedService).
-class IdentityManagerHolder : public KeyedService {
+// NOTE: Do not add any code here that further ties IdentityManager to
+// ChromeBrowserState without communicating with
+// {blundell, sdefresne}@chromium.org.
+class IdentityManagerWrapper : public KeyedService,
+                               public identity::IdentityManager {
  public:
-  explicit IdentityManagerHolder(ios::ChromeBrowserState* browser_state)
-      : identity_manager_(
+  explicit IdentityManagerWrapper(ios::ChromeBrowserState* browser_state)
+      : identity::IdentityManager(
             ios::SigninManagerFactory::GetForBrowserState(browser_state),
             ProfileOAuth2TokenServiceFactory::GetForBrowserState(browser_state),
             ios::AccountTrackerServiceFactory::GetForBrowserState(
                 browser_state)) {}
-
-  identity::IdentityManager* identity_manager() { return &identity_manager_; }
-
- private:
-  identity::IdentityManager identity_manager_;
 };
 
 IdentityManagerFactory::IdentityManagerFactory()
@@ -48,10 +47,8 @@ IdentityManagerFactory::~IdentityManagerFactory() {}
 // static
 identity::IdentityManager* IdentityManagerFactory::GetForBrowserState(
     ios::ChromeBrowserState* browser_state) {
-  IdentityManagerHolder* holder = static_cast<IdentityManagerHolder*>(
+  return static_cast<IdentityManagerWrapper*>(
       GetInstance()->GetServiceForBrowserState(browser_state, true));
-
-  return holder->identity_manager();
 }
 
 // static
@@ -61,6 +58,6 @@ IdentityManagerFactory* IdentityManagerFactory::GetInstance() {
 
 std::unique_ptr<KeyedService> IdentityManagerFactory::BuildServiceInstanceFor(
     web::BrowserState* browser_state) const {
-  return std::make_unique<IdentityManagerHolder>(
+  return std::make_unique<IdentityManagerWrapper>(
       ios::ChromeBrowserState::FromBrowserState(browser_state));
 }

ios/web_view/internal/signin/web_view_identity_manager_factory.mm

@@ -21,23 +21,22 @@
 
 namespace ios_web_view {
 
-// Class that wraps IdentityManager in a KeyedService (as IdentityManager is a
-// client-side library intended for use by any process, it would be a layering
+// Subclass that wraps IdentityManager in a KeyedService (as IdentityManager is
+// a client-side library intended for use by any process, it would be a layering
 // violation for IdentityManager itself to have direct knowledge of
 // KeyedService).
-class IdentityManagerHolder : public KeyedService {
+// NOTE: Do not add any code here that further ties IdentityManager to
+// WebViewBrowserState without communicating with
+// {blundell, sdefresne}@chromium.org.
+class IdentityManagerWrapper : public KeyedService,
+                               public identity::IdentityManager {
  public:
-  explicit IdentityManagerHolder(WebViewBrowserState* browser_state)
-      : identity_manager_(
+  explicit IdentityManagerWrapper(WebViewBrowserState* browser_state)
+      : identity::IdentityManager(
             WebViewSigninManagerFactory::GetForBrowserState(browser_state),
             WebViewOAuth2TokenServiceFactory::GetForBrowserState(browser_state),
             WebViewAccountTrackerServiceFactory::GetForBrowserState(
                 browser_state)) {}
-
-  identity::IdentityManager* identity_manager() { return &identity_manager_; }
-
- private:
-  identity::IdentityManager identity_manager_;
 };
 
 WebViewIdentityManagerFactory::WebViewIdentityManagerFactory()
@@ -54,10 +53,8 @@ WebViewIdentityManagerFactory::~WebViewIdentityManagerFactory() {}
 // static
 identity::IdentityManager* WebViewIdentityManagerFactory::GetForBrowserState(
     WebViewBrowserState* browser_state) {
-  IdentityManagerHolder* holder = static_cast<IdentityManagerHolder*>(
+  return static_cast<IdentityManagerWrapper*>(
       GetInstance()->GetServiceForBrowserState(browser_state, true));
-
-  return holder->identity_manager();
 }
 
 // static
@@ -68,7 +65,7 @@ WebViewIdentityManagerFactory* WebViewIdentityManagerFactory::GetInstance() {
 std::unique_ptr<KeyedService>
 WebViewIdentityManagerFactory::BuildServiceInstanceFor(
     web::BrowserState* browser_state) const {
-  return std::make_unique<IdentityManagerHolder>(
+  return std::make_unique<IdentityManagerWrapper>(
       WebViewBrowserState::FromBrowserState(browser_state));
 }