Sign out profile when local_state file has been changed.

1) Sign out profile when local_state auth info is overridden by Preferences 
2) Create force-sign-in policy cache
3) Trigger ProfileIsSigninRequriedChanged() when profile is locked/unlocked due to force sign in policy.

BUG=642059
Review-Url: https://codereview.chromium.org/2712883005
Cr-Commit-Position: refs/heads/master@{#457452}
Committed: https://chromium.googlesource.com/chromium/src/+/d36229e8166288e84af7485819ceeb1a347465a9

[zmin, 2017-02-23 22:20:05]

Description was changed from

==========
1) Sign out profile when local_state file has been changed.
2) Create force-sign-in policy cache
3) Trigger ProfileIsSigninRequriedChanged() when profile is locked/unlocked due
to force sign in policy.

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

to

==========
1) Sign out profile when local_state auth info is overridden by Preferences 
2) Create force-sign-in policy cache
3) Trigger ProfileIsSigninRequriedChanged() when profile is locked/unlocked due
to force sign in policy.

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

[zmin, 2017-02-23 22:36:30]

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

[zmin, 2017-02-23 22:36:31]

Sorry for the big CL. I tried to break it down a little with some descriptions,
hope it help.

rogerta@chromium.org: Please review changes in 
Sign out profile when local_state auth info is overridden by Preferences 
chrome/browser/profiles/profile_manager.cc
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc

Cache force-sign-in policy value:
chrome/browser/signin/signin_util.*

Adding a new metric and new parameter of SigninClient->PreSignOut() which will
be used by force window closing later.
chrome/browser/signin/chrome_signin_client.*
components/signin/core/browser/*

Format.
chrome/browser/signin/token_revoker_test_utils.cc

anthonyvd@chromium.org: Please review changes in 
Sign out profile when local_state auth info is overridden by Preferences 
chrome/browser/profiles/profile_manager.cc
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc

Trigger ProfileIsSigninRequriedChanged() when profile is locked/unlocked due
to force sign in policy.
chrome/browser/profiles/*

[zmin, 2017-02-23 23:02:46]

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

[commit-bot: I haz the power, 2017-02-23 23:03:41]

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, 2017-02-23 23:27:11]

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

[commit-bot: I haz the power, 2017-02-23 23:27:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zmin, 2017-02-23 23:54:01]

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

[commit-bot: I haz the power, 2017-02-23 23:55:09]

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, 2017-02-24 00:13:39]

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

[commit-bot: I haz the power, 2017-02-24 00:13:40]

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

[zmin, 2017-02-24 20:06:57]

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

[commit-bot: I haz the power, 2017-02-24 20:07:48]

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, 2017-02-24 20:28:21]

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

[commit-bot: I haz the power, 2017-02-24 20:28:22]

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

[zmin, 2017-02-24 20:56:12]

Patchset #3 (id:40001) has been deleted

[zmin, 2017-02-24 20:58:14]

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

[commit-bot: I haz the power, 2017-02-24 20:58:52]

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, 2017-02-24 22:22:46]

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

[commit-bot: I haz the power, 2017-02-24 22:22:47]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-02-27 22:08:26]

Hi Owen, a few nits but mostly a question about the cache.

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

https://codereview.chromium.org/2712883005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1559: bool
previous_authenticated_status = entry->IsAuthenticated();
nit: rename |previous_authenticated_status| to |was_authenticated|?

https://codereview.chromium.org/2712883005/diff/60001/chrome/browser/signin/s...
File chrome/browser/signin/signin_util.cc (right):

https://codereview.chromium.org/2712883005/diff/60001/chrome/browser/signin/s...
chrome/browser/signin/signin_util.cc:13: bool* g_is_force_signin_enabled_cache =
nullptr;
I don't understand why this is a pointer and why it needs to be dynamically
allocated.

Instead of making this a global, why not cache it as a member of
ChromeSigninClient?

https://codereview.chromium.org/2712883005/diff/60001/components/signin/core/...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/2712883005/diff/60001/components/signin/core/...
components/signin/core/browser/signin_metrics.h:50: AUTHENTICATION_FAILED,
Could we add "force signin" to this constants?  Maybe:

   AUTHENTICATION_FAILED_WITH_FORCED_SIGNIN

https://codereview.chromium.org/2712883005/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2712883005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:107497: +    policy is enabled.
Nit: Maybe something like:

Signed out because credentials are invalid and force-sign-in is enabled.

[zmin, 2017-03-07 20:04:11]

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

[commit-bot: I haz the power, 2017-03-07 20:05:06]

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

[zmin, 2017-03-07 20:06:42]

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

https://codereview.chromium.org/2712883005/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1559: bool
previous_authenticated_status = entry->IsAuthenticated();
On 2017/02/27 22:08:26, Roger Tawa wrote:
> nit: rename |previous_authenticated_status| to |was_authenticated|?

Done.

https://codereview.chromium.org/2712883005/diff/60001/chrome/browser/signin/s...
File chrome/browser/signin/signin_util.cc (right):

https://codereview.chromium.org/2712883005/diff/60001/chrome/browser/signin/s...
chrome/browser/signin/signin_util.cc:13: bool* g_is_force_signin_enabled_cache =
nullptr;
On 2017/02/27 22:08:26, Roger Tawa wrote:
> I don't understand why this is a pointer and why it needs to be dynamically
> allocated.
Using enum instead of pointer here.
 
> Instead of making this a global, why not cache it as a member of
> ChromeSigninClient?

Every profile has its own ChromeSigninClient while the policy value here is
global.

https://codereview.chromium.org/2712883005/diff/60001/components/signin/core/...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/2712883005/diff/60001/components/signin/core/...
components/signin/core/browser/signin_metrics.h:50: AUTHENTICATION_FAILED,
On 2017/02/27 22:08:26, Roger Tawa wrote:
> Could we add "force signin" to this constants?  Maybe:
> 
>    AUTHENTICATION_FAILED_WITH_FORCED_SIGNIN

Done.

https://codereview.chromium.org/2712883005/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2712883005/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:107497: +    policy is enabled.
On 2017/02/27 22:08:26, Roger Tawa wrote:
> Nit: Maybe something like:
> 
> Signed out because credentials are invalid and force-sign-in is enabled.

Done.

[commit-bot: I haz the power, 2017-03-07 20:11:35]

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

[commit-bot: I haz the power, 2017-03-07 20:11:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zmin, 2017-03-07 20:24:44]

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

[commit-bot: I haz the power, 2017-03-07 20:25:27]

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, 2017-03-07 20:53:11]

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

[commit-bot: I haz the power, 2017-03-07 20:53:12]

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

[zmin, 2017-03-07 20:57:08]

Patchset #5 (id:100001) has been deleted

[Roger Tawa OOO till Jul 10th, 2017-03-10 16:50:21]

lgtm

Thanks Owen.

[anthonyvd, 2017-03-13 18:47:30]

c/b/profiles lgtm with a small nit.

https://codereview.chromium.org/2712883005/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2712883005/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1577: bool was_authenticated_status =
entry->IsAuthenticated();
Can you move this 4 lines down to only have a single #if block?

[zmin, 2017-03-13 18:54:56]

https://codereview.chromium.org/2712883005/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2712883005/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1577: bool was_authenticated_status =
entry->IsAuthenticated();
On 2017/03/13 18:47:30, anthonyvd wrote:
> Can you move this 4 lines down to only have a single #if block?

Sign out only happened iff the authenticated state is True on local_state and
False on profile. So it have to be done before SetAuthInfo. 

Adding the #if here to avoid "never used variable" Compiler warning msg.

[anthonyvd, 2017-03-13 18:57:07]

https://codereview.chromium.org/2712883005/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2712883005/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1577: bool was_authenticated_status =
entry->IsAuthenticated();
On 2017/03/13 at 18:54:55, zmin wrote:
> On 2017/03/13 18:47:30, anthonyvd wrote:
> > Can you move this 4 lines down to only have a single #if block?
> 
> Sign out only happened iff the authenticated state is True on local_state and
False on profile. So it have to be done before SetAuthInfo. 
> 
> Adding the #if here to avoid "never used variable" Compiler warning msg.

Ah ok I see, SetAuthInfo() might change what IsAuthenticated() returns. Gotcha!

[zmin, 2017-03-13 19:46:58]

The CQ bit was checked by zmin@chromium.org

[commit-bot: I haz the power, 2017-03-13 19:47:30]

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, 2017-03-13 19:57:32]

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

[commit-bot: I haz the power, 2017-03-13 19:57:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zmin, 2017-03-13 19:59:33]

zmin@chromium.org changed reviewers:
+ jwd@chromium.org

[zmin, 2017-03-13 19:59:59]

+jwd for histograms.xml

Thanks,
Owen

[jwd, 2017-03-15 14:50:40]

lgtm

[zmin, 2017-03-15 15:44:20]

The CQ bit was checked by zmin@chromium.org

[commit-bot: I haz the power, 2017-03-15 15:44:36]

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, 2017-03-15 16:42:25]

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

[commit-bot: I haz the power, 2017-03-15 16:42:30]

Failed to apply patch for chrome/test/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: chrome/test/BUILD.gn:4510
  error: chrome/test/BUILD.gn: patch does not apply

Patch:       chrome/test/BUILD.gn
Index: chrome/test/BUILD.gn
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index
0ebccc1e3d7f0d35ad4bdce91ef53b3b70be6725..97196e58cad52f1567c11914e32a61f8caa16b23
100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -4510,9 +4510,8 @@ test("unit_tests") {
       "//third_party/ocmock",
     ]
 
-    sources += [
-      "../browser/ui/cocoa/applescript/apple_event_util_unittest.mm",
-    ]
+    sources +=
+        [ "../browser/ui/cocoa/applescript/apple_event_util_unittest.mm" ]
 
     if (mac_views_browser) {
       # TODO(tapted): Add chrome_unit_tests_views_non_mac_sources.
@@ -4783,6 +4782,7 @@ test("unit_tests") {
       "../browser/media/webrtc/native_desktop_media_list_unittest.cc",
      
"../browser/metrics/desktop_session_duration/desktop_session_duration_tracker_unittest.cc",
       "../browser/signin/signin_global_error_unittest.cc",
+      "../browser/signin/signin_util_unittest.cc",
       "../browser/ui/webui/signin/signin_create_profile_handler_unittest.cc",
       "../browser/ui/webui/signin/sync_confirmation_handler_unittest.cc",
       "../browser/upgrade_detector_impl_unittest.cc",

[zmin, 2017-03-15 21:22:53]

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

[commit-bot: I haz the power, 2017-03-15 21:23:50]

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, 2017-03-15 22:56:50]

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

[commit-bot: I haz the power, 2017-03-15 22:56:51]

Dry run: This issue passed the CQ dry run.

[zmin, 2017-03-16 16:08:30]

The CQ bit was checked by zmin@chromium.org

[zmin, 2017-03-16 16:08:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, anthonyvd@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2712883005/#ps140001
(title: "Merge from master")

[commit-bot: I haz the power, 2017-03-16 16:09:00]

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, 2017-03-16 16:14:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489680510690570,
"parent_rev": "a50545b9d3795b3902d9e0b12f0a30b94078a3b9", "commit_rev":
"d36229e8166288e84af7485819ceeb1a347465a9"}

[commit-bot: I haz the power, 2017-03-16 16:15:46]

Description was changed from

==========
1) Sign out profile when local_state auth info is overridden by Preferences 
2) Create force-sign-in policy cache
3) Trigger ProfileIsSigninRequriedChanged() when profile is locked/unlocked due
to force sign in policy.

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

to

==========
1) Sign out profile when local_state auth info is overridden by Preferences 
2) Create force-sign-in policy cache
3) Trigger ProfileIsSigninRequriedChanged() when profile is locked/unlocked due
to force sign in policy.

BUG=642059

Review-Url: https://codereview.chromium.org/2712883005
Cr-Commit-Position: refs/heads/master@{#457452}
Committed:
https://chromium.googlesource.com/chromium/src/+/d36229e8166288e84af7485819ce...
==========

[commit-bot: I haz the power, 2017-03-16 16:15:48]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d36229e8166288e84af7485819ce...