New issue
Advanced search Search tips

Issue 729540 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Sign in to add a comment

Identity Service: Design observer API

Project Member Reported by blundell@chromium.org, Jun 5 2017

Issue description

This interface should be backed by the Identity Service impl being an observer of PO2TS, per discussion with msarda@.

Note that there are similar-but-not-identical observer interfaces in the current codebase, e.g. being an observer of the AccountTracker. Client code will need to be ported to observing PO2TS before being ported to the Identity Service in order to be clear that all the migrations are correct. We will call out this fact in documentation when adding this observer interface to the Identity Service. 
 
Labels: IdentityService
Blocking: 683120
Blocking: 729543
Blocking: 729966
Note that some customers need the information of whether the account for which the refresh token was just made available/revoked is the authenticated account; see crbug.com/729966 for an example.
Blocking: 731065
Blocking: 731084
Owner: blundell@chromium.org
Status: Started (was: Available)
Owner: ----
Status: Available (was: Started)
Summary: Identity Service: Design observer API (was: Identity Service: Add ability to listen for refresh token being available/revoked)
After struggling with the design of this API just for the purposes of the chrome.identity.onSignInEventChanged() event implementation, I realized that is not a straightforward design problem. Conversations with bsazanov@ have also brought light the knowledge that we have to consider that the Android system API provides only coarse-grained updates in designing this API.

I think that it likely makes sense for us to find a use case or two for the observer API on mobile if possible, design the API for those use cases + Chrome identity API, and then implement that API for all of those use cases.

Components: Services>SignIn
Components: Internals>Services>Identity
Owner: blundell@chromium.org
Status: Started (was: Available)
Starting this now as it's needed for ongoing conversions (sync, GCM, PrimaryAccountAccessTokenFetcher). Going to start off with a very simple design and then we can easily iterate as needed.
Cc: treib@chromium.org
Project Member

Comment 15 by bugdroid1@chromium.org, May 31 2018

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

commit d7df96d788fd687a7c2ab2af0fb57d5d2206cc71
Author: Colin Blundell <blundell@chromium.org>
Date: Thu May 31 14:16:54 2018

Add DiagnosticsClient to ProfileOAuth2TokenService

Similar to what https://chromium-review.googlesource.com/883347 did for
SigninManager, this CL adds a
ProfileOAuth2TokenService::DiagnosticsClient interface. This interface
will be used to ensure that IdentityManager gets information on
token-related events before observers of PO2TS while the codebase is
being converted to use IdentityManager directly. Concretely, this
client interface allows IdentityManager to be informed of refresh token
availability/revocation before ProfileOAuth2TokenService clients, thus
ensuring that IdentityManager has a consistent view of token state
when any PO2TS observers receive these events.

One subtlety here is exactly where to place the calls informing the
diagnostics client that these events will be fired.
ProfileOAuth2TokenService actually itself observes its base class
OAuth2TokenService, and it is a necessary constraint that PO2TS
receive this event *before* any other O2TS observer (including
IdentityManager). Hence, we place these diagnostic calls within
ProfileOAuth2TokenService's implementations of the observer callbacks.

An upcoming CL will use this newly-introduced interface to add an
IdentityManager API wherein its clients can be notified of changes in
token state.

Bug: 729540
Change-Id: Ic9be84a31faa259d0e9b971a16311a9492ca7b97
Reviewed-on: https://chromium-review.googlesource.com/1075336
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563218}
[modify] https://crrev.com/d7df96d788fd687a7c2ab2af0fb57d5d2206cc71/components/signin/core/browser/profile_oauth2_token_service.cc
[modify] https://crrev.com/d7df96d788fd687a7c2ab2af0fb57d5d2206cc71/components/signin/core/browser/profile_oauth2_token_service.h

Project Member

Comment 16 by bugdroid1@chromium.org, May 31 2018

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

commit f11dfdeba0d29aa1b85256a8e545101dd2adbd2a
Author: Colin Blundell <blundell@chromium.org>
Date: Thu May 31 17:05:12 2018

Plumb AccountTrackerService into IdentityManager

IdentityManager will need AccountTrackerService in an upcoming  CL in
order to map account IDs received from ProfileOAuth2TokenService into
their corresponding AccountInfo objects. ProfileOAuth2TokenService does
not itself know about AccountTrackerService; while it would be possible
to have PO2TS take in AccountTrackerService and send out events with
AccountInfo objects, (1) it would be a bigger bear of a change to make,
and (2) IdentityManager will almost certainly have to observe
AccountTrackerService eventually.

TBR=jzw@chromium.org

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

Project Member

Comment 17 by bugdroid1@chromium.org, Jun 1 2018

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

commit c5ba909cf7b18fff0bf2ff26ae35d63544fde3f0
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jun 01 07:29:42 2018

IdentityManager: Add API for observing refresh token updates/removal

This CL adds an API to IdentityManager::Observer that allows observers
to be notified on refresh token updates and removals. This API follows
that of OAuth2TokenService::Observer and is intended for porting of
clients of the latter API. One extension in the new API is that on
token updates, we include the information of whether the updated token
is valid or invalid. Including this information means that clients
don't need to explicitly query the last authentication error associated
with the account, as they do when receiving OnRefreshTokenAvailable()
notifications from ProfileOAuth2TokenService. In particular, this
functionality will be required immediately to port sync.

Similarly to IdentityManager's interaction with SigninManager for other
parts of the IdentityManager::Observer API implementation,
IdentityManager implements ProfileOAuth2TokenService::DiagnosticsClient
rather than OAuth2TokenService::Observer to implement the new APIs. The
reason is to maintain consistency while the codebase is incrementally
converted to IdentityManager. If IdentityManager updates its state in
response to observing SigninManager/PO2TS, then some other observer of
those classes could get called back first. That other observer could
kick off a flow that ends up in querying IdentityManager (again, due to
incremental conversion). If IdentityManager's view does not match that of
PO2TS, the above-described flow will likely not behave as expected.
(This isn't theoretical:
https://bugs.chromium.org/p/chromium/issues/detail?id=804410).

The production changes are relatively minimal and straightforward. The
bulk of the change is in unit tests of the newly-added API.

Bug: 729540
Change-Id: I67b9c0842074148845d1313a0409f055d1f81e08
Reviewed-on: https://chromium-review.googlesource.com/1076047
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563559}
[modify] https://crrev.com/c5ba909cf7b18fff0bf2ff26ae35d63544fde3f0/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/c5ba909cf7b18fff0bf2ff26ae35d63544fde3f0/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/c5ba909cf7b18fff0bf2ff26ae35d63544fde3f0/services/identity/public/cpp/identity_manager_unittest.cc

Sign in to add a comment