New issue
Advanced search Search tips

Issue 866518 link

Starred by 2 users
Status: Started
Owner:
msarda@chromium.org
Cc:
blundell@chromium.org
msarda@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

SigninClient::PreSignOut() is called even when SigninManager::SignOut() is prohibitted

Project Member Reported by chcunningham@chromium.org, Jul 23 
This issue tracks cleanup of some counter intuitive code. 

SigninManager::SignOut() may be prohibited (e.g. cloud managed accounts). 
This is exposed via SigninMnager::IsSignoutProhibted().

SigninManager::SignOut() abandons signout when IsSignoutProhibited() returns true [0]. But this abandonment occurs (by design) *after* we notify the SigninClient::PreSignOut() [1].

At present, one of the uses cases of later-abandoned PreSignOut() notification is to completely remove the profile for enterprise accounts [2] (which are the same accounts where IsSignoutProhibited() returns true). Sync data for managed accounts belongs to the accounts' organization, so removing the profile makes sense. But this should probably be done at a higher level and we should simply avoid making a call to SignOut() when its not logically what we want.

[0] https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_manager.cc?rcl=12d366cdb7c89178d310fd49db319247e7d21fa6&l=217
[1] https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_manager.cc?rcl=12d366cdb7c89178d310fd49db319247e7d21fa6&l=188
[2] https://www.google.com/url?q=https://cs.chromium.org/chromium/src/chrome/browser/signin/chrome_signin_client.cc?rcl%3D3f0eda935338567fd3c770fd24b44a3af46a4b3b%26l%3D287&sa=D&ust=1532361605747000&usg=AFQjCNE-KrX47oiFNSGtfwu8QMJXN4iifw

Comment 2 by chcunningham@chromium.org, Aug 13

Blockedon: 873116
Owner: chcunningham@chromium.org
Adding blocking bug to properly plumb reasons for android. 

Android makes one call to ProhibitSignOut(false) in SigninHelper.java. This call can be removed because the signout is not a user-triggered signout (its done in response to internal account rename), so we don't want this signout to be prohibited. 

In the future, android will give a reason indicating this signout is for internal reasons and we can remove the ProhibitSignout(false) call above.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45fefd334fc65149b83f211f22973bb47568e67a

commit 45fefd334fc65149b83f211f22973bb47568e67a
Author: chcunningham <chcunningham@chromium.org>
Date: Mon Sep 17 19:02:03 2018

[Android] SingninHelper: give correct reason for rename SignOut()

This SignOut() call is not triggered by a user clicking signout
settings. This CL passes a reason to indicate that SignOut was not user
driven.

Bug: 873116, 866518
Change-Id: I2990769d11ec5d02add1b37dabf54926b47e9d32
Reviewed-on: https://chromium-review.googlesource.com/1170775
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591760}
[modify] https://crrev.com/45fefd334fc65149b83f211f22973bb47568e67a/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java
[modify] https://crrev.com/45fefd334fc65149b83f211f22973bb47568e67a/components/signin/core/browser/signin_metrics.h
[modify] https://crrev.com/45fefd334fc65149b83f211f22973bb47568e67a/tools/metrics/histograms/enums.xml
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c8848adcd82add45a4fd5356dab53e5d442bb882

commit c8848adcd82add45a4fd5356dab53e5d442bb882
Author: chcunningham <chcunningham@chromium.org>
Date: Fri Sep 28 21:07:56 2018

Move SigninManager::IsSignoutProhibited to SigninClient

The old SigninManager::IsSignoutProhibited() had a few smells
1. callers of SM::SignOut() were expected to always call SignOut()
   even when prohibited. This allowed the *chrome* client to do special
   cleanup.
2. ProhibitSignout() was really meant to narrowly prohibit user-triggered
   sign-out for scenarios like cloud-managed accounts. But the prohibition
   was applied broadly for all SignOut() calls, leading to weird states
   when sign-out was triggered for internal reasons (e.g. cloud policy
   changes).

These issues came up in planning an analogous API for the new
IdentityManager. This CL implements the proposed fixes
https://docs.google.com/document/d/14esaufZT0fr258zL5I69y7YBBc24XsxtDU0B7wpWxBo/edit#bookmark=id.t69fnqpy377k

Namely:
- Prohibiting sign-out is now up to the SigninClient. SignOut() signals
  the source of the sign-out (user-driven vs internal reasons) and the client
  runs a callback indicating whether the sign-out may continue.
- signin_util::SetUserSignoutAllowedForProfile() chrome/ code
  allow/block user-driven sign-outs.
- Other implementers of SigninClient never block sign-out and are unchanged.

NOTE: For now, signin_util::SetUserSignoutAllowedForProfile() actually
prohibits most non-user-driven signouts to avoid changing sign-out behavior in this
large plumbing CL. This will change in a follow up CL where non-user-driven
sign-outs are always permitted.
https://chromium-review.googlesource.com/c/chromium/src/+/1175797/


Bug: 866518
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I4758706d796f953cbb68ff1b4a0c8d2985d12fb3
Reviewed-on: https://chromium-review.googlesource.com/1162173
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595214}
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/android/signin/signin_manager_android.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/android/signin/signin_manager_android.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/policy/cloud/user_policy_signin_service.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/signin/chrome_signin_client.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/signin/chrome_signin_client_unittest.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/signin/signin_util.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/signin/signin_util.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/supervised_user/child_accounts/child_account_service.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/sync/sync_ui_util.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/chrome/browser/ui/webui/settings/people_handler.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/fake_signin_manager.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/signin_client.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/signin_client.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/signin_manager.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/signin_manager_unittest.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/signin_metrics.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/test_signin_client.cc
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/components/signin/core/browser/test_signin_client.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/ios/web_view/internal/signin/ios_web_view_signin_client.h
[modify] https://crrev.com/c8848adcd82add45a4fd5356dab53e5d442bb882/ios/web_view/internal/signin/ios_web_view_signin_client.mm

Comment 6 by chcunningham@chromium.org, Oct 1

Blockedon: -873116
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/caa4b2b3ef948bff8bb4427868264c6ea493a36c

commit caa4b2b3ef948bff8bb4427868264c6ea493a36c
Author: chcunningham <chcunningham@chromium.org>
Date: Sat Oct 27 01:59:52 2018

Update TODO in ChromeSigninClient

msarda@ is graciously taking the remaining work.

Bug: 866518
Change-Id: Ia1720219386ee51955e74e461e2a4cdf17134bc2
Reviewed-on: https://chromium-review.googlesource.com/c/1175797
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603291}
[modify] https://crrev.com/caa4b2b3ef948bff8bb4427868264c6ea493a36c/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/caa4b2b3ef948bff8bb4427868264c6ea493a36c/chrome/browser/signin/chrome_signin_client_unittest.cc

Comment 8 by chcunningham@chromium.org, Oct 29

Owner: msarda@chromium.org
At this time the refactoring of IsSignoutProhibited into SigninClient is largely finished. msarda@ will take on the lingering TODO in comment 7.
►

Sign in to add a comment