Create a new profile and sign in the user.

Create a new profile and sign in the user.

This CL changes the logic of the inline_login_handler to create a new
profile when the user selects this option on the sign-in email
confirmation dialog.

BUG=661307
Committed: https://crrev.com/c51fda8eb00536cc40b69efbdc38aadd60732fe8
Cr-Commit-Position: refs/heads/master@{#440612}

[msarda, 2016-12-20 14:26:10]

The CQ bit was checked by msarda@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 14:26:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarda, 2016-12-20 14:44:55]

Patchset #1 (id:1) has been deleted

[msarda, 2016-12-20 15:07:16]

msarda@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[msarda, 2016-12-20 15:07:16]

Please take a look.

[msarda, 2016-12-20 15:18:15]

Description was changed from

==========
Create a new profile and sign in the user

BUG=
==========

to

==========
Create a new profile and sign in the user.

This CL changes the logic of the inline_login_handler to create a new
profile when the user selects this option on the sign-in email
confirmation dialog.

BUG=661307
==========

[anthonyvd, 2016-12-21 19:09:28]

Hi Mihai,

This patch generally looks good. Can you add a few tests for the new codepaths?
This part of sign in code is already pretty intricate, so having more tests
would probably catch some future regressions.

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (left):

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:481: // TODO(zmin):
Remove this hack once the https://crbug.com/657924 fixed.
Are you sure you can now remove this?

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:45: // Attempts to signs
the user in |profile_|. Note that if the account to be
nit: sign with no 's'

[msarda, 2016-12-22 11:57:35]

Patchset #4 (id:80001) has been deleted

[msarda, 2016-12-22 11:58:06]

I'll add some unit tests for the new codepath (if this is possible).

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (left):

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:481: // TODO(zmin):
Remove this hack once the https://crbug.com/657924 fixed.
On 2016/12/21 19:09:28, anthonyvd wrote:
> Are you sure you can now remove this?

I have moved this change to its own CL
https://codereview.chromium.org/2600473002/

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.h (right):

https://codereview.chromium.org/2594703002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.h:45: // Attempts to signs
the user in |profile_|. Note that if the account to be
On 2016/12/21 19:09:28, anthonyvd wrote:
> nit: sign with no 's'

Done.

[msarda, 2016-12-22 16:12:49]

Concerning the tests:
1. The unit tests for one_click_signin_sync_starter_unittest.cc do not really
test the flow the the password separated flow.
2. I could not find any browser tests for the on one_click_signin_sync_starter
3. The browser tests in inline_login_ui_browsertest.cc mock the code in
one_click_signin_sync_starter, so adding tests in these will not actually test
this change.

I think we need to add unit tests for the one_click_signin_sync_starter, but I'd
like to do that in a separate CL.

[anthonyvd, 2016-12-22 18:02:57]

Makes sense, lgtm

[msarda, 2016-12-23 06:13:13]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2016-12-23 06:13:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 07:17:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 07:17:47]

Failed to apply patch for
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:145
  error: chrome/browser/ui/sync/one_click_signin_sync_starter.cc: patch does not
apply

Patch:       chrome/browser/ui/sync/one_click_signin_sync_starter.cc
Index: chrome/browser/ui/sync/one_click_signin_sync_starter.cc
diff --git a/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
b/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
index
a800de21ec191d204458ac65a8a84ff062fda50c..80a1aeebf208270de5e767c731152c566e8bf688
100644
--- a/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
+++ b/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
@@ -80,6 +80,7 @@ OneClickSigninSyncStarter::OneClickSigninSyncStarter(
     const std::string& email,
     const std::string& password,
     const std::string& refresh_token,
+    ProfileMode profile_mode,
     StartSyncMode start_mode,
     content::WebContents* web_contents,
     ConfirmationRequired confirmation_required,
@@ -100,13 +101,10 @@ OneClickSigninSyncStarter::OneClickSigninSyncStarter(
   BrowserList::AddObserver(this);
   Initialize(profile, browser);
 
-  // Policy is enabled, so pass in a callback to do extra policy-related UI
-  // before signin completes.
-  SigninManagerFactory::GetForProfile(profile_)->
-      StartSignInWithRefreshToken(
-          refresh_token, gaia_id, email, password,
-          base::Bind(&OneClickSigninSyncStarter::ConfirmSignin,
-                     weak_pointer_factory_.GetWeakPtr()));
+  SigninManagerFactory::GetForProfile(profile_)->StartSignInWithRefreshToken(
+      refresh_token, gaia_id, email, password,
+      base::Bind(&OneClickSigninSyncStarter::ConfirmSignin,
+                 weak_pointer_factory_.GetWeakPtr(), profile_mode));
 }
 
 void OneClickSigninSyncStarter::OnBrowserRemoved(Browser* browser) {
@@ -145,24 +143,39 @@ void OneClickSigninSyncStarter::Initialize(Profile*
profile, Browser* browser) {
   skip_sync_confirm_ = false;
 }
 
-void OneClickSigninSyncStarter::ConfirmSignin(const std::string& oauth_token) {
+void OneClickSigninSyncStarter::ConfirmSignin(ProfileMode profile_mode,
+                                              const std::string& oauth_token) {
   DCHECK(!oauth_token.empty());
   SigninManager* signin = SigninManagerFactory::GetForProfile(profile_);
-  // If this is a new signin (no account authenticated yet) try loading
-  // policy for this user now, before any signed in services are initialized.
-  if (!signin->IsAuthenticated()) {
-    policy::UserPolicySigninService* policy_service =
-        policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
-    policy_service->RegisterForPolicy(
-        signin->GetUsernameForAuthInProgress(),
-        oauth_token,
-        base::Bind(&OneClickSigninSyncStarter::OnRegisteredForPolicy,
-                   weak_pointer_factory_.GetWeakPtr()));
-    return;
-  } else {
+  if (signin->IsAuthenticated()) {
     // The user is already signed in - just tell SigninManager to continue
     // with its re-auth flow.
+    DCHECK_EQ(CURRENT_PROFILE, profile_mode);
     signin->CompletePendingSignin();
+    return;
+  }
+
+  switch (profile_mode) {
+    case CURRENT_PROFILE: {
+      // If this is a new signin (no account authenticated yet) try loading
+      // policy for this user now, before any signed in services are
+      // initialized.
+      policy::UserPolicySigninService* policy_service =
+          policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
+      policy_service->RegisterForPolicy(
+          signin->GetUsernameForAuthInProgress(), oauth_token,
+          base::Bind(&OneClickSigninSyncStarter::OnRegisteredForPolicy,
+                     weak_pointer_factory_.GetWeakPtr()));
+      break;
+    }
+    case NEW_PROFILE:
+      // If this is a new signin (no account authenticated yet) in a new
+      // profile, then just create the new signed-in profile and skip loading
+      // the policy as there is no need to ask the user again if they should be
+      // signed in to a new profile. Note that in this case the policy will be
+      // applied after the new profile is signed in.
+      CreateNewSignedInProfile();
+      break;
   }
 }
 
@@ -264,8 +277,7 @@ void OneClickSigninSyncStarter::OnPolicyFetchComplete(bool
success) {
 void OneClickSigninSyncStarter::CreateNewSignedInProfile() {
   SigninManager* signin = SigninManagerFactory::GetForProfile(profile_);
   DCHECK(!signin->GetUsernameForAuthInProgress().empty());
-  DCHECK(!dm_token_.empty());
-  DCHECK(!client_id_.empty());
+
   // Create a new profile and have it call back when done so we can inject our
   // signin credentials.
   size_t icon_index = g_browser_process->profile_manager()->
@@ -303,8 +315,6 @@ void OneClickSigninSyncStarter::CompleteInitForNewProfile(
       DCHECK(!old_signin_manager->GetUsernameForAuthInProgress().empty());
       DCHECK(!old_signin_manager->IsAuthenticated());
       DCHECK(!new_signin_manager->IsAuthenticated());
-      DCHECK(!dm_token_.empty());
-      DCHECK(!client_id_.empty());
 
       // Copy credentials from the old profile to the just-created profile,
       // and switch over to tracking that profile.
@@ -321,9 +331,15 @@ void OneClickSigninSyncStarter::CompleteInitForNewProfile(
       old_signin_manager->SignOut(signin_metrics::TRANSFER_CREDENTIALS,
                                  
signin_metrics::SignoutDelete::IGNORE_METRIC);
 
-      // Load policy for the just-created profile - once policy has finished
-      // loading the signin process will complete.
-      LoadPolicyWithCachedCredentials();
+      if (!dm_token_.empty()) {
+        // Load policy for the just-created profile - once policy has finished
+        // loading the signin process will complete.
+        DCHECK(!client_id_.empty());
+        LoadPolicyWithCachedCredentials();
+      } else {
+        // No policy to load - simply complete the signin process.
+        SigninManagerFactory::GetForProfile(profile_)->CompletePendingSignin();
+      }
 
       // Unlock the new profile.
       ProfileAttributesEntry* entry;

[msarda, 2016-12-23 09:08:06]

The CQ bit was checked by msarda@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 09:08:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 09:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 09:55:09]

Dry run: This issue passed the CQ dry run.

[msarda, 2016-12-23 10:13:08]

The CQ bit was checked by msarda@chromium.org

[msarda, 2016-12-23 10:13:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2594703002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-23 10:13:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 10:16:52]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482487988348650,
"parent_rev": "691100897e3f8f9812cea68aecd349e12eabebf3", "commit_rev":
"44ded333f71410a9ed772a2c34e254655b50489f"}

[commit-bot: I haz the power, 2016-12-23 10:17:20]

Description was changed from

==========
Create a new profile and sign in the user.

This CL changes the logic of the inline_login_handler to create a new
profile when the user selects this option on the sign-in email
confirmation dialog.

BUG=661307
==========

to

==========
Create a new profile and sign in the user.

This CL changes the logic of the inline_login_handler to create a new
profile when the user selects this option on the sign-in email
confirmation dialog.

BUG=661307

Review-Url: https://codereview.chromium.org/2594703002
==========

[commit-bot: I haz the power, 2016-12-23 10:17:21]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-12-23 10:19:14]

Description was changed from

==========
Create a new profile and sign in the user.

This CL changes the logic of the inline_login_handler to create a new
profile when the user selects this option on the sign-in email
confirmation dialog.

BUG=661307

Review-Url: https://codereview.chromium.org/2594703002
==========

to

==========
Create a new profile and sign in the user.

This CL changes the logic of the inline_login_handler to create a new
profile when the user selects this option on the sign-in email
confirmation dialog.

BUG=661307

Committed: https://crrev.com/c51fda8eb00536cc40b69efbdc38aadd60732fe8
Cr-Commit-Position: refs/heads/master@{#440612}
==========

[commit-bot: I haz the power, 2016-12-23 10:19:16]

Patchset 5 (id:??) landed as
https://crrev.com/c51fda8eb00536cc40b69efbdc38aadd60732fe8
Cr-Commit-Position: refs/heads/master@{#440612}