Lock profile before sign in when force sign in is enabled.

Lock profile before sign in when force sign in is enabled.
1) New profile attributes of profile locking
2) Block cross account sign in and bypass confirm dialog.
3) Postpone browser window creation after sign in
4) Show error message within the sign in dialog.

BUG=642059
Committed: https://crrev.com/5db87096a32b73bd2aaca04710152de495ba50ea
Cr-Commit-Position: refs/heads/master@{#430699}

[zmin, 2016-11-04 17:50:31]

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

[commit-bot: I haz the power, 2016-11-04 17:51:14]

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-11-04 18:17:43]

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

[commit-bot: I haz the power, 2016-11-04 18:17:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-11-04 18:26:46]

Patchset #2 (id:20001) has been deleted

[zmin, 2016-11-04 19:26:57]

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

[commit-bot: I haz the power, 2016-11-04 19:27:59]

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-11-04 20:06:20]

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

[commit-bot: I haz the power, 2016-11-04 20:06:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2016-11-04 20:30:43]

Patchset #2 (id:40001) has been deleted

[zmin, 2016-11-04 20:32:05]

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

[commit-bot: I haz the power, 2016-11-04 20:32:50]

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

[zmin, 2016-11-04 20:39:57]

zmin@chromium.org changed reviewers:
+ anthonyvd@chromium.org, rogerta@chromium.org

[zmin, 2016-11-04 20:39:58]

Hi Anthony, Roger, 

Can you review this CL please?

Thanks,
Owen

[zmin, 2016-11-04 20:42:54]

cc chrome-enterprise-changes

[anthonyvd, 2016-11-04 21:31:03]

Hi! Few comments/questions.

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.cc (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:111: return (
nit: The parens aren't needed.

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.h (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.h:147: bool
is_force_signin_enabled_;
Why store this instead of just checking the pref? It seems like this would be a
failure point if the pref changes at runtime, no?

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:457:
entry->LockForceSigninProfile(true);
Why isn't this still just SetIsSigninRequired considering that function calls
LockForceSigninProfile?  Doesn't this break non-force-signin lock?

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:237: void
HideUserManager(const base::FilePath profile_path,
This function does a lot more than just closing the User Manager, can it be
renamed to something more descriptive?

[zmin, 2016-11-04 22:11:53]

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.cc (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:111: return (
On 2016/11/04 21:31:02, anthonyvd wrote:
> nit: The parens aren't needed.

Done.

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.h (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.h:147: bool
is_force_signin_enabled_;
On 2016/11/04 21:31:02, anthonyvd wrote:
> Why store this instead of just checking the pref? It seems like this would be
a
> failure point if the pref changes at runtime, no?

The value of force sign in policy won't be changed during runtime
('dynamic_refresh': False,):
So I think it should be safe to cache it.

https://cs.chromium.org/chromium/src/components/policy/resources/policy_templ...

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:457:
entry->LockForceSigninProfile(true);
On 2016/11/04 21:31:02, anthonyvd wrote:
> Why isn't this still just SetIsSigninRequired considering that function calls
> LockForceSigninProfile?  Doesn't this break non-force-signin lock?

LockProfile() is a function I added recently and will only be called for
force-sign-in sign out.

Yes, I agree that the function name confuses people after this change. So I'll
change that.

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/2478173003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:237: void
HideUserManager(const base::FilePath profile_path,
On 2016/11/04 21:31:02, anthonyvd wrote:
> This function does a lot more than just closing the User Manager, can it be
> renamed to something more descriptive?

How about UnlockProfileAndHideLoginUI?

I also removed the last two args as I found that they are useless right bow.

[zmin, 2016-11-04 22:12:02]

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

[commit-bot: I haz the power, 2016-11-04 22:12:56]

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-11-04 23:14:17]

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

[commit-bot: I haz the power, 2016-11-04 23:14:18]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2016-11-08 15:16:11]

lgtm with a few question below.

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.cc (right):

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:29: is_force_signin_enabled_
= IsForceSigninEnabled();
Do you really need to save IsForceSigninEnabled() in a member?  Seems like you
could just call IsForceSigninEnabled() each time you needed it.  Is
LockForceSigninProfile() a function that is called often and is performance
critical?

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:243: if
(is_force_signin_enabled_)
Should this be a DCHECK instead of if statement?

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.cc:94: }
Shouldn't there be an else block here for when browser is null?

[zmin, 2016-11-08 18:39:08]

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_attributes_entry.cc (right):

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:29: is_force_signin_enabled_
= IsForceSigninEnabled();
On 2016/11/08 15:16:11, Roger Tawa wrote:
> Do you really need to save IsForceSigninEnabled() in a member?  Seems like you
> could just call IsForceSigninEnabled() each time you needed it.  Is
> LockForceSigninProfile() a function that is called often and is performance
> critical?

force_signin flag will be checked every time the SetIsSigninRequired get called.

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_attributes_entry.cc:243: if
(is_force_signin_enabled_)
On 2016/11/08 15:16:11, Roger Tawa wrote:
> Should this be a DCHECK instead of if statement?

SetIsSigninRequired() will call LockForceSigninProfile().

I'll move the if statement to SetIsSigninRequired() and add the DCHECK here. I
think it makes more sense.

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/login_ui_service.cc (right):

https://codereview.chromium.org/2478173003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/login_ui_service.cc:94: }
On 2016/11/08 15:16:11, Roger Tawa wrote:
> Shouldn't there be an else block here for when browser is null?

Without force-sign-in,  the DisplayLoginResult will be called only if browser is
not null.

With force-sign-in, the error message will be displayed in UserManager without
browser(so browser could be null). However, UserManager error message only
supports the new MaterialDesign. The old UI simply ignores the error message if
there is no browser.

[zmin, 2016-11-08 19:06:03]

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

[commit-bot: I haz the power, 2016-11-08 19:06:38]

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-11-08 19:58:04]

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

[commit-bot: I haz the power, 2016-11-08 19:58:05]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2016-11-08 20:08:32]

lgtm, thanks :)

[zmin, 2016-11-08 20:15:39]

The CQ bit was checked by zmin@chromium.org

[zmin, 2016-11-08 20:15:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/2478173003/#ps100001
(title: "rogerta's comments")

[commit-bot: I haz the power, 2016-11-08 20:16: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-11-08 20:22:03]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-11-08 20:41:34]

Description was changed from

==========
Lock profile before sign in when force sign in is enabled.
1) New profile attributes of profile locking
2) Block cross account sign in and bypass confirm dialog.
3) Postpone browser window creation after sign in
4) Show error message within the sign in dialog.

BUG=642059
==========

to

==========
Lock profile before sign in when force sign in is enabled.
1) New profile attributes of profile locking
2) Block cross account sign in and bypass confirm dialog.
3) Postpone browser window creation after sign in
4) Show error message within the sign in dialog.

BUG=642059

Committed: https://crrev.com/5db87096a32b73bd2aaca04710152de495ba50ea
Cr-Commit-Position: refs/heads/master@{#430699}
==========

[commit-bot: I haz the power, 2016-11-08 20:41:35]

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