New issue
Advanced search Search tips

Issue 887756 link

Starred by 1 user
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

SigninManager::DoSignOut() clears some state even when signout prohibited

Project Member Reported by chcunningham@chromium.org, Sep 20 
Sign-out is sometimes prohibitted (e.g. by cloud policy). But even when prohibited SigninManager::DoSignOut() still clears some signin state. 

It may be that this is intentional - some of the state is for authentication-in-progress, which may make sense to always clear. If so we can just add some comments to the code.

Comment 1 by blundell@chromium.org, Sep 21 

The relevant code is here: https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_manager.cc?q=signin_manager.cc&sq=package:chromium&g=0&l=216 (in particular, the code above that line in the method).

Comment 2 by msarda@chromium.org, Sep 21

Owner: msarda@chromium.org
Status: Assigned (was: Untriaged)

Comment 4 by msarda@chromium.org, Oct 9

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 27 

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

commit e98161eb3fcb6d04c6fabe32e4048a188dd70f19
Author: Mihai Sardarescu <msarda@chromium.org>
Date: Tue Nov 27 16:28:31 2018

Clear managed profile on start-up if having a primary account is not allowed.

This CL clears the profile when it is loaded is the primary account is no longer
allowed (e.g. profile was marked as a managed profile or the profile).

As this is a very destructive action (the profile directory is removed from disk),
the user is presented with a permanent browser modal dialog.

Screenshots for each platform:
Linux: https://drive.google.com/open?id=1HO693VkDnC0qjPxHJdFZr3CPqOs2Hidf
Windows: https://drive.google.com/open?id=1gvmvt2wUsA42nmcVWTZCB1F5encHJRRH

Design doc [Google internal only]:
https://docs.google.com/document/d/1QqWoV1I7WgrzeHJwm9agG56ZqXD_Q2L3LB3K7Ck89GI/edit?usp=sharing

Bug:  907474 , 887756

Change-Id: Ib1187d79a45e0829b57768582c5d11126b0a5c46
Reviewed-on: https://chromium-review.googlesource.com/c/1288829
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611135}
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/app/generated_resources.grd
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/BUILD.gn
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/policy/cloud/user_policy_signin_service.cc
[add] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/policy/cloud/user_policy_signin_service_internal.h
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/profiles/profile_manager.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/profiles/profile_metrics.h
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/signin/chrome_signin_client_unittest.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/signin/dice_browsertest.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/signin/signin_util.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/signin/signin_util.h
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/ui/android/simple_message_box_android.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/ui/simple_message_box.h
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/ui/views/simple_message_box_views.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/browser/ui/views/simple_message_box_views.h
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/components/signin/core/browser/signin_metrics.h
[modify] https://crrev.com/e98161eb3fcb6d04c6fabe32e4048a188dd70f19/tools/metrics/histograms/enums.xml
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 28 

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

commit b04e513f825d6ce22e147601cbfc75ea23eb9073
Author: Ken Rockot <rockot@google.com>
Date: Tue Nov 27 23:59:32 2018

Revert "Clear managed profile on start-up if having a primary account is not allowed."

This reverts commit e98161eb3fcb6d04c6fabe32e4048a188dd70f19.

Reason for revert: Sheriff suspecting this change is responsible for test failures  https://crbug.com/909034  -- if not, please reland apologies for the trouble!

Original change's description:
> Clear managed profile on start-up if having a primary account is not allowed.
> 
> This CL clears the profile when it is loaded is the primary account is no longer
> allowed (e.g. profile was marked as a managed profile or the profile).
> 
> As this is a very destructive action (the profile directory is removed from disk),
> the user is presented with a permanent browser modal dialog.
> 
> Screenshots for each platform:
> Linux: https://drive.google.com/open?id=1HO693VkDnC0qjPxHJdFZr3CPqOs2Hidf
> Windows: https://drive.google.com/open?id=1gvmvt2wUsA42nmcVWTZCB1F5encHJRRH
> 
> Design doc [Google internal only]:
> https://docs.google.com/document/d/1QqWoV1I7WgrzeHJwm9agG56ZqXD_Q2L3LB3K7Ck89GI/edit?usp=sharing
> 
> Bug:  907474 , 887756
> 
> Change-Id: Ib1187d79a45e0829b57768582c5d11126b0a5c46
> Reviewed-on: https://chromium-review.googlesource.com/c/1288829
> Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Reviewed-by: David Roger <droger@chromium.org>
> Reviewed-by: Owen Min <zmin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#611135}

TBR=ellyjones@chromium.org,pastarmovj@chromium.org,droger@chromium.org,msarda@chromium.org,zmin@chromium.org

Change-Id: I0c1468ce16d832ec57de9c456e1642e054fe9cfa
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  907474 , 887756
Reviewed-on: https://chromium-review.googlesource.com/c/1352646
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#611432}
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/app/generated_resources.grd
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/BUILD.gn
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/policy/cloud/user_policy_signin_service.cc
[delete] https://crrev.com/7357da2a70de65bf267e8da97dc31e5e96baee20/chrome/browser/policy/cloud/user_policy_signin_service_internal.h
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/profiles/profile_manager.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/profiles/profile_metrics.h
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/signin/chrome_signin_client_unittest.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/signin/dice_browsertest.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/signin/signin_util.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/signin/signin_util.h
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/ui/android/simple_message_box_android.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/ui/simple_message_box.h
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/ui/views/simple_message_box_views.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/browser/ui/views/simple_message_box_views.h
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/components/signin/core/browser/signin_metrics.h
[modify] https://crrev.com/b04e513f825d6ce22e147601cbfc75ea23eb9073/tools/metrics/histograms/enums.xml
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 28 

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

commit ca00b242db441c3d5dd9b3165738f426aa2fd6dc
Author: Mihai Sardarescu <msarda@chromium.org>
Date: Wed Nov 28 10:54:22 2018

Reland "Clear managed profile on start-up if having a primary account is not allowed."

This reverts commit b04e513f825d6ce22e147601cbfc75ea23eb9073.

Reason for revert:
This CL contains changes only for Windows, Linux and macOS (if you look
at the main changes in signin_util.{h|cc} file, you'll see they are ifdefed out on ChromeOS.

Browser test from the same test suite (WebviewClientCertsLoginTest) failed already at the
previous build on the same target:
* Build that fails a tests from WebviewClientCertsLoginTest and that does not include my CL:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20ChromiumOS%20MSan%20Tests/9853

* Build that fails a test from WebviewClientCertsLoginTest and that includes my CL:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20ChromiumOS%20MSan%20Tests/9854

Original change's description:
> Revert "Clear managed profile on start-up if having a primary account is not allowed."
>
> This reverts commit e98161eb3fcb6d04c6fabe32e4048a188dd70f19.
>
> Reason for revert: Sheriff suspecting this change is responsible for test failures  https://crbug.com/909034  -- if not, please reland apologies for the trouble!
>
> Original change's description:
> > Clear managed profile on start-up if having a primary account is not allowed.
> >
> > This CL clears the profile when it is loaded is the primary account is no longer
> > allowed (e.g. profile was marked as a managed profile or the profile).
> >
> > As this is a very destructive action (the profile directory is removed from disk),
> > the user is presented with a permanent browser modal dialog.
> >
> > Screenshots for each platform:
> > Linux: https://drive.google.com/open?id=1HO693VkDnC0qjPxHJdFZr3CPqOs2Hidf
> > Windows: https://drive.google.com/open?id=1gvmvt2wUsA42nmcVWTZCB1F5encHJRRH
> >
> > Design doc [Google internal only]:
> > https://docs.google.com/document/d/1QqWoV1I7WgrzeHJwm9agG56ZqXD_Q2L3LB3K7Ck89GI/edit?usp=sharing
> >
> > Bug:  907474 , 887756
> >
> > Change-Id: Ib1187d79a45e0829b57768582c5d11126b0a5c46
> > Reviewed-on: https://chromium-review.googlesource.com/c/1288829
> > Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
> > Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> > Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> > Reviewed-by: David Roger <droger@chromium.org>
> > Reviewed-by: Owen Min <zmin@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#611135}
>
> TBR=ellyjones@chromium.org,pastarmovj@chromium.org,droger@chromium.org,msarda@chromium.org,zmin@chromium.org
>
> Change-Id: I0c1468ce16d832ec57de9c456e1642e054fe9cfa
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  907474 , 887756
> Reviewed-on: https://chromium-review.googlesource.com/c/1352646
> Reviewed-by: Ken Rockot <rockot@google.com>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/master@{#611432}

TBR=ellyjones@chromium.org,pastarmovj@chromium.org,droger@chromium.org,rockot@google.com,msarda@chromium.org,zmin@chromium.org

Change-Id: I6da34b30e8c740b48da32cba4be3fcac5e5e6ea0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  907474 , 887756
Reviewed-on: https://chromium-review.googlesource.com/c/1352360
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611642}
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/app/generated_resources.grd
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/BUILD.gn
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/policy/cloud/user_policy_signin_service.cc
[add] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/policy/cloud/user_policy_signin_service_internal.h
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/profiles/profile_manager.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/profiles/profile_metrics.h
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/signin/chrome_signin_client_unittest.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/signin/dice_browsertest.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/signin/signin_util.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/signin/signin_util.h
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/ui/android/simple_message_box_android.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/ui/simple_message_box.h
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/ui/views/simple_message_box_views.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/browser/ui/views/simple_message_box_views.h
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/components/signin/core/browser/fake_signin_manager.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/components/signin/core/browser/signin_metrics.h
[modify] https://crrev.com/ca00b242db441c3d5dd9b3165738f426aa2fd6dc/tools/metrics/histograms/enums.xml
►

Sign in to add a comment