Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953253002:
[DICE] Enable sync for an account that is already present in the token service. (Closed)
Description[DICE] Enable sync for an account that is already present in the token service.
This CL enables sync for the first account in the token service. It reuses
OneClickSigninSyncStarter to control the way sync is enabled.
It also needs to adapt the sync service and the policy service to cater for
the fact that the token service already has an refresh token for account
that is used to turn on sync.
This CL also removes dead code from the policy service.
BUG=733226
Review-Url: https://codereview.chromium.org/2953253002
Cr-Commit-Position: refs/heads/master@{#484496}
Committed: https://chromium.googlesource.com/chromium/src/+/94fc2359a4878bc58dd324865dfba382c85da052
Patch Set 1 : Prepare for review #
Total comments: 6
Patch Set 2 : Remove useless code from cloud policy #Patch Set 3 : Protect sync for DICE #
Total comments: 6
Patch Set 4 : Address code review #Messages
Total messages: 69 (51 generated)
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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_...)
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable sync for DICE format Enable sync [signin] Generate OAuth token on Dice Signin responses Introduces the DiceResponseHandler class to handle the Dice responses, which is a new profile keyed service. In this CL, only the SIGNIN response is implemented, and triggers a OAuth2 token request. BUG=730589 patch from issue 2942193002 at patchset 140001 (http://crrev.com/2942193002#ps140001) ========== to ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. BUG=730589 ==========
Description was changed from ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. BUG=730589 ========== to ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. BUG=733226 ==========
msarda@chromium.org changed reviewers: + rogerta@chromium.org
Roger: Please take a look. I did not add any unit test, but I think these changes yet, but I'd like your feedback on the current approach as I think I am touching code that is at the core of the sign-in process. I will try to add a browser test for this change as I think this is an interaction problem, but I think that will not be trivial.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msarda@chromium.org changed reviewers: + droger@chromium.org - rogerta@chromium.org
David: It looks like Roger is OOO until July 10th. Please take a look.
lgtm https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc (right): https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:72: #if !defined(OS_ANDROID) Be consistent with header (remove the ifdef, or delete the code). https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.h (left): https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:56: #if !defined(OS_ANDROID) Should we delete this block instead? What platform is using this? https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.h (right): https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:30: // A specialization of the UserPolicySigninServiceBase for the Android. remove "the"
msarda@chromium.org changed reviewers: + pastarmovj@chromium.org
pastarmovj@chromium.org: Please review changes related to the cloud policy. https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc (right): https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:72: #if !defined(OS_ANDROID) On 2017/06/26 09:17:50, droger wrote: > Be consistent with header (remove the ifdef, or delete the code). Done. https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.h (left): https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:56: #if !defined(OS_ANDROID) On 2017/06/26 09:17:50, droger wrote: > Should we delete this block instead? What platform is using this? Done. https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.h (right): https://codereview.chromium.org/2953253002/diff/100001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:30: // A specialization of the UserPolicySigninServiceBase for the Android. On 2017/06/26 09:17:50, droger wrote: > remove "the" > Done.
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. BUG=733226 ========== to ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. This CL also removes dead code from the policy service. BUG=733226 ==========
msarda@chromium.org changed reviewers: + zea@chromium.org
zea@: Please review changes in profile_sync_service.cc
msarda@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in chrome/browser/android/signin/signin_manager_android.cc
Android LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2953253002/diff/160001/components/browser_syn... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2953253002/diff/160001/components/browser_syn... components/browser_sync/profile_sync_service.cc:1971: OnRefreshTokenAvailable(account_id); Why isn't OnRefresTokenAvailable automatically being invoked by O2TS at the time the user signs in? It seems odd to have to trigger this here.
msarda@google.com changed reviewers: + msarda@google.com
https://codereview.chromium.org/2953253002/diff/160001/components/browser_syn... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2953253002/diff/160001/components/browser_syn... components/browser_sync/profile_sync_service.cc:1971: OnRefreshTokenAvailable(account_id); On 2017/06/28 19:43:24, Nicolas Zea wrote: > Why isn't OnRefresTokenAvailable automatically being invoked by O2TS at the time > the user signs in? It seems odd to have to trigger this here. The problem is the order here: Take the following example with DICE enabled: * On Monday, users starts Chrome and signs in to gmail. AT that moment, Chrome will get a refresh token and will fire a OnRefreshTokenAvailable. * Next Friday, the user decides to enable sync. At this point, SigninManager fires GoogleSigninSucceeded. There is no OnRefreshTokenAvailable notification being fired here as the token has not changed and has been around for 5 days already. The clients will need to be able to handle these events without making any assumption of the order in which they arrive. This is why for sync, I need to call OnRefreshTokenAvailable in this case. I will update the design document with this explanation and with what we expect clients to do to handle this correctly.
Why are we triggering GoogleSigninSucceeded in the second case? If the user's already signed in as of Monday, it seems strange that enabling Sync would change that?
With DICE, we plan to send OnRefreshTokenAvailable when a user signs to a Google web property and Chrome gets a refresh token for that account. We'll send GoogleSigninSucceeded only when the user enables sync (when SigninManager::CompletePendingSignin or SigninManager::OnExternalSignin method are called). Note that this matches what is done for mobile where there may be multiple accounts, but only one "authenticated account". See the design document for more information.
msarda@chromium.org changed reviewers: + pavely@chromium.org - msarda@google.com
Adding pavely@ as reviewer as Nicolas seems to have a very busy schedule this week (OOO followed by travel). Pavel: May I ask you to take a look at the changes in components/browser_sync/profile_sync_service.cc. The reasons for this change are explained in https://docs.google.com/document/d/1gx7_pjcWkCj52WABcAF_E1dsYnBJtwkxgsLvBoonc... (sections Sync Scenarios and Changes to the Chrome Sign-in services). Thank you.
atwilson@chromium.org changed reviewers: + atwilson@chromium.org
drive-by: policy pieces lgtm
LGTM with a few nits. https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service.cc (right): https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service.cc:123: registration_helper_.reset(new CloudPolicyClientRegistrationHelper( nit: Modernize this and the function above to base::MakeUnique call if possible. https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc (right): https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc:54: new OneClickSigninSyncStarter( nit: Please add a comment above that this class is of the suicidal type :) It is quite scary seeing this new without anything before it.
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service.cc (right): https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service.cc:123: registration_helper_.reset(new CloudPolicyClientRegistrationHelper( On 2017/07/05 13:56:21, pastarmovj wrote: > nit: Modernize this and the function above to base::MakeUnique call if possible. Done. https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc (right): https://codereview.chromium.org/2953253002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/signin_dice_internals_handler.cc:54: new OneClickSigninSyncStarter( On 2017/07/05 13:56:21, pastarmovj wrote: > nit: Please add a comment above that this class is of the suicidal type :) It > is quite scary seeing this new without anything before it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sync lgtm
The CQ bit was checked by msarda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, bauerb@chromium.org, atwilson@chromium.org, pastarmovj@chromium.org Link to the patchset: https://codereview.chromium.org/2953253002/#ps180001 (title: "Address code review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499329495701620,
"parent_rev": "611b35e0bbf518c76733783f0d21c48c3b298f51", "commit_rev":
"94fc2359a4878bc58dd324865dfba382c85da052"}
Message was sent while issue was closed.
Description was changed from ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. This CL also removes dead code from the policy service. BUG=733226 ========== to ========== [DICE] Enable sync for an account that is already present in the token service. This CL enables sync for the first account in the token service. It reuses OneClickSigninSyncStarter to control the way sync is enabled. It also needs to adapt the sync service and the policy service to cater for the fact that the token service already has an refresh token for account that is used to turn on sync. This CL also removes dead code from the policy service. BUG=733226 Review-Url: https://codereview.chromium.org/2953253002 Cr-Commit-Position: refs/heads/master@{#484496} Committed: https://chromium.googlesource.com/chromium/src/+/94fc2359a4878bc58dd324865dfb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/chromium/src/+/94fc2359a4878bc58dd324865dfb... |
