New issue
Advanced search Search tips

Issue 859882 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Sign in to add a comment

Port consumers of GaiaCookieManagerService to IdentityManager

Project Member Reported by blundell@chromium.org, Jul 3

Issue description

Sync has a need to get the accounts stored in the user's Gaia cookie. It is not clear at this time whether this functionality should be implemented inside the Identity Service or not. It is currently provided by GaiaCookieManagerService, which intertwines this functionality with other functionality for *mutating* the state of the user's Gaia cookie based on browser policies like Mirror and DICE. The design doc here describes the approach that we are going to take in the near term to satisfy Sync's use case with a clean API while leaving the issue of whether this API will ultimately be implemented inside or outside of the Identity Service for conclusion in the future:

https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit#heading=h.y7yywe14x23d
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 4

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

commit 8700bb994f8ece64be09272e8ebcec7de05a1398
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jul 04 15:42:09 2018

Make SigninUiUtil depend explicitly on ListAccounts() behavior

We are shortly going to change the semantics of
GaiaCookieManagerService::ListAccounts() to populate its out-params even
in the case where it returns false to indicate that the accounts are
stale. A search through the codebase shows a minimal number of
production callsites that call ListAccounts() outside of an if
condition:

blundell:src(change_signin_ui_util) $  git grep "\<ListAccounts(" | grep -v "if (" | grep -v unittest
(standard input):1:(standard input):3:chrome/browser/signin/signin_ui_util.cc:    bool cookie_accounts_valid = cookie_manager_service->ListAccounts(
(standard input):2:(standard input):6:components/browser_sync/profile_sync_service.cc:      gaia_cookie_manager_service_->ListAccounts(
(standard input):7:(standard input):13:components/signin/core/browser/gaia_cookie_manager_service.cc:bool GaiaCookieManagerService::ListAccounts(
(standard input):8:(standard input):14:components/signin/core/browser/gaia_cookie_manager_service.h:    // to ListAccounts(). The GCMS will delay calling ListAccounts if other
(standard input):9:(standard input):15:components/signin/core/browser/gaia_cookie_manager_service.h:  bool ListAccounts(std::vector<gaia::ListedAccount>* accounts,

An examination of the two production callsites listed above shows that
ProfileSyncService actually does guard its usage in an if-condition, just
spread over multiple lines. SigninUiUtil, however, does not. This CL
changes SigninUiUtil to explicitly depend on the current behavior of
GaiaCookieManagerService::ListAccounts() so that its behavior won't
change when we change the semantics of
GaiaCookieManagerService::ListAccounts().

Bug: 859882
Change-Id: I8ee63b980edc9f6a2980d3a46702a72f93307ee4
Reviewed-on: https://chromium-review.googlesource.com/1126257
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572586}
[modify] https://crrev.com/8700bb994f8ece64be09272e8ebcec7de05a1398/chrome/browser/signin/signin_ui_util.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 5

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

commit edab9397d2d0a9e99a2536cb76889e05e03d52a1
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jul 05 15:20:02 2018

Extend GaiaCookieManagerService unittest of ListAccounts()


GaiaCookieManagerService::ListAccounts() takes in out-params to
populate with the current cached state of the accounts and returns a
bool indicating whether the cached state is valid. Its contract
specifies that if the returned bool is false, the out-params should be
ignored.

We are shortly going to be changing the semantics of
GaiaCookieManagerService::ListAccounts() to always populate its
out-parameters, even if its cached state is stale. This CL prepares
for that change by extending the unittests of GaiaCookieManagerService
to explicitly test the current behavior (i.e., that the out-params are
*not* populated with stale cached data).

Bug: 859882
Change-Id: Icf28ba3be7fabbbf70b7b51acc68f0f60bed5993
Reviewed-on: https://chromium-review.googlesource.com/1126103
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572799}
[modify] https://crrev.com/edab9397d2d0a9e99a2536cb76889e05e03d52a1/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 9

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

commit fb671babdc29f773731e4bc92439b14da172b228
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jul 09 14:57:28 2018

GaiaCookieManagerService: Change semantics of ListAccounts()

GaiaCookieManagerService::ListAccounts() takes in out-params to
populate with the current cached state of the accounts and returns a
bool indicating whether the cached state is valid. Its contract
specifies that if the returned bool is false, the out-params should be
ignored.

This CL changes the semantics of GaiaCookieManagerService::ListAccounts()
to always populate its out-parameters, even if its cached state is
stale. We verified manually that all current clients of this method
guard the usage of the out-params via a check that the returned bool is
true, so this semantic change will not affect any existing clients.

The reason for making this semantic change is that there is an
upcoming consumer that wants to have synchronous access to the
latest cached state of the accounts, regardless of whether that state
is stale.

Bug: 859882
Change-Id: I47fc55bb2fad6caf9a2fc5911c8f60f97e4bc5a6
Reviewed-on: https://chromium-review.googlesource.com/1128892
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573307}
[modify] https://crrev.com/fb671babdc29f773731e4bc92439b14da172b228/components/signin/core/browser/gaia_cookie_manager_service.cc
[modify] https://crrev.com/fb671babdc29f773731e4bc92439b14da172b228/components/signin/core/browser/gaia_cookie_manager_service.h
[modify] https://crrev.com/fb671babdc29f773731e4bc92439b14da172b228/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc

Update: After reflection, I am going to add these APIs on IdentityManager itself. That will be most straightforward and graceful for satisfying Sync's use case, and since Sync is such a primary use case for Signin (and the Identity Service), serving Sync's needs gracefully is a high design priority for this project. More details here:

https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit#

As noted in the doc, this decision can easily be changed (and the APIs split out to be layered on top on the client side) if we find compelling reasons to do so in the future.
I am going to bring these APIs up via 3 CLs:
- CL just threading in GaiaCookieManagerService
- CL implementing observer APIs for cookie jar changes
- CL for getting cached state of accounts in cookie jar
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 18

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

commit e3f61fa173c7d4cbef792a05b31489b1c094ad24
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jul 18 12:16:26 2018

[IdentityManager] Thread in GaiaCookieManagerService

Upcoming work will add APIs to IdentityManager to interact with the
user's Gaia accounts in the cookie jar. To implement these APIS,
IdentityManager will need to interact with GaiaCookieManagerService. In
preparation for that work, this CL threads GaiaCookieManagerService in
to IdentityManager.

Design doc:

https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit?pli=1#heading=h.y7yywe14x23d

TBR=rohitrao@chromium.org

Bug: 859882
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I40f1abd6c8beca89476567bbb94979ad42ff7df1
Reviewed-on: https://chromium-review.googlesource.com/1140156
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576011}
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/chrome/browser/signin/identity_manager_factory.cc
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/components/browser_sync/profile_sync_test_util.h
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/components/gcm_driver/gcm_account_tracker_unittest.cc
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/ios/chrome/browser/signin/identity_manager_factory.cc
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/ios/web_view/internal/signin/web_view_identity_manager_factory.mm
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/DEPS
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_manager_unittest.cc
[modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_test_environment.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 20

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

commit d8a60e779be20114b5de192bdf9685f5db2e5d9d
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 20 13:45:55 2018

[IdentityManager] API for observing updates to accounts in Gaia cookie

This CL adds an API to IdentityManager::Observer via which clients can
be notified of updates to the list of Gaia accounts in the cookie jar.
This API is intended to enable porting consumers of
GaiaCookieManagerService::Observer::OnGaiaAccountsInCookieUpdated().
Notably, whereas the former API deals in gaia::ListedAccount structs,
the IdentityManager API talks in terms of the more-mainstream
AccountInfo structs (parallel to other IdentityManager::Observer APIs).

The new API passes only the list of signed-in accounts, whereas the
previous API has more parameters. We will add more parameters if and
as we see that they are needed by consumers.

Bug: 859882
Change-Id: I4ff2ece0836e73fc616462c84ba88c600a026071
Reviewed-on: https://chromium-review.googlesource.com/1140325
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576846}
[modify] https://crrev.com/d8a60e779be20114b5de192bdf9685f5db2e5d9d/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/d8a60e779be20114b5de192bdf9685f5db2e5d9d/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/d8a60e779be20114b5de192bdf9685f5db2e5d9d/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 20

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

commit 8b34f56ae64d3083023ba3672604276fca318b64
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jul 20 15:38:26 2018

IdentityManager: Add ability to get accounts in the cookie jar

This CL adds an API to IdentityManager via which clients can obtain
the latest cached state of the accounts in the Gaia cookie in the cookie
jar, ordered by their order in the cookie jar. It serves as an initial
step to porting consumers of GaiaCookieManagerService::ListAccounts(),
although it intentionally provides only a subset of the functionality
(in particular, it does not indicate whether the cached state is known
to be stale).

https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit?pli=1#heading=h.w97eil1cygs2
sketches out the approach for extending this API to give IdentityManager
the full functionality required to port all consumers of
GaiaCookieManagerService::ListAccounts(). We are holding off on adding
more functionality until we encounter use cases that require it.

Bug: 859882
Change-Id: I062a9ce036aa886f1c105913eb1a774471bb63f8
Reviewed-on: https://chromium-review.googlesource.com/1141951
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576874}
[modify] https://crrev.com/8b34f56ae64d3083023ba3672604276fca318b64/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/8b34f56ae64d3083023ba3672604276fca318b64/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/8b34f56ae64d3083023ba3672604276fca318b64/services/identity/public/cpp/identity_manager_unittest.cc

Blocking: 880841
Owner: ----
Status: Available (was: Started)
The remaining work is to:
- Determine what other API surfaces of GaiaCookieManagerService{::Observer} will be needed by public clients
- Add the buildout of those APIs as blocking bugs here
- Burn them down
Labels: Proj-Servicification
Owner: lowell@chromium.org
Status: Assigned (was: Available)
Blockedon: 907782
Summary: Port consumers of GaiaCookieManagerService to IdentityManager (was: Provide next-generation API to access accounts in user's Gaia cookie)
Blockedon: 913481
Blockedon: 903718
Blockedon: 913872
Blockedon: 797931
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 7

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

commit b92aad536edcac81e0daa90deb484e668995d157
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Mon Jan 07 17:19:12 2019

Remove GaiaCookieManagerService dependency from AccountsInvestigator

This a 2nd out 3 CLs that will make AccountInvestigator and its unittests
not depend/use GaiaCookieManagerService anymore.

Here AccountInvestigator replaces inheritance of
GaiaCookieManagerService::Observer by IdentityManager::Observer.

Note that because of the existing AccountInfo <-> ListedAccount
conversions, a helper function was introduced with a TODO to convert
the whole AccountInvestigator and its unit tests to use the former.

BUG=859882, 909715 

Change-Id: Iaf23e19622258ce63957c6254df3aac652558a08
Reviewed-on: https://chromium-review.googlesource.com/c/1396240
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620363}
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/chrome/browser/signin/account_investigator_factory.cc
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator.cc
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator.h
[modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator_unittest.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 9

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

commit 7128827cfca113f04143286836cf17a7215bc91e
Author: Lowell Manners <lowell@chromium.org>
Date: Wed Jan 09 17:38:29 2019

Remove some unneeded references to GaiaCookieManagerService::Observer.

This CL deletes some empty GaiaCookieManagerService::Observer overrides
which created noise in grep results when searching for usages of
GaiaCookieManagerService callbacks.

Bug: 859882
Change-Id: Id8119e9faec009056938c7b7b1977388307914cb
Reviewed-on: https://chromium-review.googlesource.com/c/1400700
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621204}
[modify] https://crrev.com/7128827cfca113f04143286836cf17a7215bc91e/components/autofill/core/browser/personal_data_manager.h

Blockedon: 921002
Labels: -Pri-2 Pri-1
Labels: -Pri-1 Pri-2

Sign in to add a comment