New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 829304 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 824756



Sign in to add a comment

Sync: Remove Supervised User support

Project Member Reported by treib@chromium.org, Apr 5 2018

Issue description

Supervised Users are deprecated, and their special support in Sync (via SigninManagerWrapper) is blocking necessary refactorings ( bug 825190 ).
Since it's not possible anymore for custodians to add or update any restrictions, there little reason to keep Sync working for SUs. In fact, I'm not sure if it even does still work, since some of the server-side support is already gone.

While removing support, we need to make sure not to completely break existing SUs. It'd also be nice for existing restrictions to stay in place (which I *think* should happen automatically, since SUSettingsService persists them).
 

Comment 1 by treib@chromium.org, Apr 5 2018

Status: Started (was: Assigned)

Comment 2 by treib@chromium.org, Apr 17 2018

Labels: M-68
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 18 2018

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

commit 8455d78181378b9ffa537175a9c5460109883b02
Author: Marc Treib <treib@chromium.org>
Date: Wed Apr 18 09:32:13 2018

Sync: Remove SupervisedUserSigninManagerWrapper

This will effectively turn off Sync for Supervised Users.
Supervised Users have been deprecated, and it's not possible anymore for
custodians to add or update any restrictions, which was to main reason
for requiring Sync.

Bug:  829304 ,  809030 
Change-Id: I510cc9da1aa26147e890b94e17dcb147d5ca9034
Reviewed-on: https://chromium-review.googlesource.com/997654
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551619}
[modify] https://crrev.com/8455d78181378b9ffa537175a9c5460109883b02/chrome/browser/BUILD.gn
[modify] https://crrev.com/8455d78181378b9ffa537175a9c5460109883b02/chrome/browser/sync/profile_sync_service_factory.cc
[delete] https://crrev.com/f4b46947717b0127d9082fc3295aeca47ba714dc/chrome/browser/sync/supervised_user_signin_manager_wrapper.cc
[delete] https://crrev.com/f4b46947717b0127d9082fc3295aeca47ba714dc/chrome/browser/sync/supervised_user_signin_manager_wrapper.h

Comment 4 by treib@chromium.org, Apr 18 2018

With this, Sync support for SUs is gone. Assuming nothing explodes and this sticks, there are more things that can be cleaned up:
- SupervisedUserSyncService (for the SUPERVISED_USERS model type)
- SupervisedUserSharedSettingsService (for the SUPERVISED_USER_SHARED_SETTINGS model type)
- Related factories, observers etc
- PermissionRequestCreatorSync
- SupervisedUserRefreshTokenFetcher

However, some SU-Sync-related things are still needed, e.g. SupervisedUserSyncDataTypeController and SupervisedUserSettingsService.
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 18 2018

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

commit f9331623ff350729975925ca34dd70987b422d9e
Author: Marc Treib <treib@chromium.org>
Date: Wed Apr 18 12:17:25 2018

Sync cleanup: Remove SigninManagerWrapper::GetSyncScopeToUse

As of https://crrev.com/c/997654, Sync support for Supervised Users is
gone, so GetSyncScopeToUse isn't required anymore.

Bug:  829304 
Change-Id: Ie912abfa75eb15b05b7f7b23067ea83550bfdecf
Reviewed-on: https://chromium-review.googlesource.com/1016285
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551639}
[modify] https://crrev.com/f9331623ff350729975925ca34dd70987b422d9e/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/f9331623ff350729975925ca34dd70987b422d9e/components/sync/driver/signin_manager_wrapper.cc
[modify] https://crrev.com/f9331623ff350729975925ca34dd70987b422d9e/components/sync/driver/signin_manager_wrapper.h

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 18 2018

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

commit 43cfb0c69fb23109a3ca3c5945e2470e098f1808
Author: Marc Treib <treib@chromium.org>
Date: Wed Apr 18 13:11:28 2018

Cleanup: Delete unused Supervised User Sync code

This deletes PermissionRequestCreatorSync and
SupervisedUserRefreshTokenFetcher, both of which were unused.

Bug:  829304 
Change-Id: I55b0de3e74c933eede36d0af5dda6dcd5b65bac9
Reviewed-on: https://chromium-review.googlesource.com/1015571
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551649}
[modify] https://crrev.com/43cfb0c69fb23109a3ca3c5945e2470e098f1808/chrome/browser/BUILD.gn
[delete] https://crrev.com/9e916b23b4a98c510780c8c435fb1c21eeca7788/chrome/browser/supervised_user/legacy/permission_request_creator_sync.cc
[delete] https://crrev.com/9e916b23b4a98c510780c8c435fb1c21eeca7788/chrome/browser/supervised_user/legacy/permission_request_creator_sync.h
[delete] https://crrev.com/9e916b23b4a98c510780c8c435fb1c21eeca7788/chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
[delete] https://crrev.com/9e916b23b4a98c510780c8c435fb1c21eeca7788/chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.h
[delete] https://crrev.com/9e916b23b4a98c510780c8c435fb1c21eeca7788/chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher_unittest.cc
[modify] https://crrev.com/43cfb0c69fb23109a3ca3c5945e2470e098f1808/chrome/test/BUILD.gn
[modify] https://crrev.com/43cfb0c69fb23109a3ca3c5945e2470e098f1808/tools/traffic_annotation/summary/annotations.xml

Comment 7 by treib@chromium.org, Apr 18 2018

Blockedon: 824756

Comment 8 by treib@chromium.org, Apr 23 2018

Labels: -Pri-2 -M-68 Pri-3
The remaining cleanup work shouldn't block anything, so reducing prio.

Comment 9 by treib@chromium.org, Apr 23 2018

Blocking: -825190

Comment 10 by treib@chromium.org, Apr 25 2018

Blocking: 825190
Status: Fixed (was: Started)
Actually, let's just call this done. The Sync-relevant parts of SUs are gone, and there are other bugs for the non-Sync cleanup.
Status: Started (was: Fixed)
Reopening: SupervisedUserService still contains a bunch of Sync setup code that pokes around in ProfileSyncService and ProfileOAuth2TokenService. We should get rid of that too.
Blocking: -825190
Project Member

Comment 13 by bugdroid1@chromium.org, May 8 2018

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

commit 044a1435292dd1a81eb9e43f10c631e30e4c81fb
Author: Marc Treib <treib@chromium.org>
Date: Tue May 08 16:49:16 2018

SupervisedUserService: Remove Sync setup code

Supervised users are deprecated, and Sync hasn't been working for them
for a while.

Bug:  829304 
Change-Id: I30db644e5fecadeefe7b268fafd3fd0a67f68a0b
Reviewed-on: https://chromium-review.googlesource.com/1049650
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556835}
[modify] https://crrev.com/044a1435292dd1a81eb9e43f10c631e30e4c81fb/chrome/browser/supervised_user/supervised_user_service.cc
[modify] https://crrev.com/044a1435292dd1a81eb9e43f10c631e30e4c81fb/chrome/browser/supervised_user/supervised_user_service.h
[modify] https://crrev.com/044a1435292dd1a81eb9e43f10c631e30e4c81fb/chrome/common/chrome_switches.cc
[modify] https://crrev.com/044a1435292dd1a81eb9e43f10c631e30e4c81fb/chrome/common/chrome_switches.h

Comment 14 by treib@chromium.org, May 14 2018

Status: Fixed (was: Started)

Sign in to add a comment