New issue
Advanced search Search tips

Issue 806774 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 796544
issue 808984
issue 809539
issue 840703
issue 856538



Sign in to add a comment

Provide equivalent IdentityManager API to OAuth2TokenService::GetAccounts()

Project Member Reported by blundell@chromium.org, Jan 29 2018

Issue description

This should be implemented by having IdentityManager listen for refresh tokens being available and revoked, and then having IdentityManager cache the information of the available accounts locally. This implementation will map easily to eventual interaction of IdentityManager with the IdentityService.

Note that unlike the primary account, these accounts are not made available synchronously on startup. Clients that want to know when all the tokens are loaded must listen for the O2TS::Observer callback that all tokens were loaded. Enabling that usage model to transition to the Identity Service is crbug.com/740117.
 
Blocking: 796544
Components: Internals>Services>Identity
Status: Available (was: Untriaged)
Blocking: 808984
Blocking: 809539

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

Cc: treib@chromium.org

Comment 5 by treib@chromium.org, May 8 2018

Blocking: 840703
Cc: sdefresne@chromium.org
Owner: blundell@chromium.org
Status: Started (was: Available)

Comment 8 by treib@chromium.org, Jun 14 2018

Yay for "Started"!

One additional requirement that we previously talked about, but that isn't mentioned here yet: We'd like the accounts to be sorted in content area/cookie order, i.e. the same order in which they'll show up in the OneGoogleBar account switcher. More precisely: We care which account is the default (first) content area account, i.e. authuser=0.
I'm looking at that as well. At this point, I'm thinking about adding that as a separate API. I'm going to start writing up a short design doc for it.
Blocking: 856538
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 29 2018

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

commit cb05af1b8e952afa7bc2729ed16f838b278dbcd4
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jun 29 13:13:53 2018

IdentityManager: Add APIs for querying state of refresh tokens

This CL adds APIs to IdentityManager for obtaining the set of all
accounts with refresh tokens and querying whether the primary account
is available with a refresh token.

The design follows that of IdentityManager's caching of the primary
account information:
- IdentityManager initializes its state with the current state of
  ProfileOAuth2TokenService.
- IdentityManager updates this state in response to notifications from
  PO2TS that an account's refresh token was updated/removed.

TBR=bsazonov@chromium.org

Bug:  806774 
Change-Id: Idc4a37a15ced2e7d32f012a50e4e0d1fdb1aecdf
Reviewed-on: https://chromium-review.googlesource.com/1098668
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571451}
[modify] https://crrev.com/cb05af1b8e952afa7bc2729ed16f838b278dbcd4/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java
[modify] https://crrev.com/cb05af1b8e952afa7bc2729ed16f838b278dbcd4/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/cb05af1b8e952afa7bc2729ed16f838b278dbcd4/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/cb05af1b8e952afa7bc2729ed16f838b278dbcd4/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 2

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

commit f62594528efd0a3e606a7c9da9ebc4efbbe2452c
Author: Robert Liao <robliao@chromium.org>
Date: Mon Jul 02 23:50:45 2018

Revert "IdentityManager: Add APIs for querying state of refresh tokens"

This reverts commit cb05af1b8e952afa7bc2729ed16f838b278dbcd4.

Reason for revert: Causes a DCHECK followed by a crash.  http://crbug.com/859697 

Original change's description:
> IdentityManager: Add APIs for querying state of refresh tokens
> 
> This CL adds APIs to IdentityManager for obtaining the set of all
> accounts with refresh tokens and querying whether the primary account
> is available with a refresh token.
> 
> The design follows that of IdentityManager's caching of the primary
> account information:
> - IdentityManager initializes its state with the current state of
>   ProfileOAuth2TokenService.
> - IdentityManager updates this state in response to notifications from
>   PO2TS that an account's refresh token was updated/removed.
> 
> TBR=bsazonov@chromium.org
> 
> Bug:  806774 
> Change-Id: Idc4a37a15ced2e7d32f012a50e4e0d1fdb1aecdf
> Reviewed-on: https://chromium-review.googlesource.com/1098668
> Commit-Queue: Colin Blundell <blundell@chromium.org>
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#571451}

TBR=blundell@chromium.org,msarda@chromium.org,sdefresne@chromium.org,treib@chromium.org,bsazonov@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  806774 ,  859697 
Change-Id: I636d458101a5a579202dc4683faddfc0643cb19c
Reviewed-on: https://chromium-review.googlesource.com/1123119
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572048}
[modify] https://crrev.com/f62594528efd0a3e606a7c9da9ebc4efbbe2452c/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java
[modify] https://crrev.com/f62594528efd0a3e606a7c9da9ebc4efbbe2452c/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/f62594528efd0a3e606a7c9da9ebc4efbbe2452c/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/f62594528efd0a3e606a7c9da9ebc4efbbe2452c/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 3

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

commit 225bf60cc1383b273a55eb6a414caa358eecad06
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jul 03 13:12:42 2018

IdentityManager: Add APIs for querying state of refresh tokens

This CL adds APIs to IdentityManager for obtaining the set of all
accounts with refresh tokens and querying whether the primary account
is available with a refresh token.

The design follows that of IdentityManager's caching of the primary
account information:
- IdentityManager initializes its state with the current state of
  ProfileOAuth2TokenService.
- IdentityManager updates this state in response to notifications from
  PO2TS that an account's refresh token was updated/removed.

This CL is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1098668 with
the following addition:

In https://chromium-review.googlesource.com/c/chromium/src/+/1098668,
we made the assumption that when IdentityManager receives a
notification from ProfileOAuth2TokenService that a token will be
revoked, it had previously known about that account (either because
the account was available at IdentityManager startup or because
IdentityManager previously received a notification from PO2TS that a
refresh token had been made available for the account). However, as
demonstrated in the linked bug, it turns out that this assumption is
not valid in one case: while loading tokens during startup, PO2TS
sometimes revokes tokens without having previously made them available.
This violation of IdentityManager's assumption causes a crash.

It is not feasible at the current time to change this behavior on the
part of PO2TS, as AccountTrackerService should be made aware of these
events so that it removes the accounts. However, this situation poses
a challenge for IdentityManager: Should it forward on the callback to
its own observers or not? In this CL, we fix the crash and additionally
nail down the semantics that IdentityManager sends token removed
callbacks only for accounts of which it previously knew the existence.
The reasons for making this choice on the semantics are twofold:

(1) Sending an observer callback that a token was removed for an account
that was not previously present in
IdentityManager::GetAccountsWithRefreshTokens() seems like a violation of
the principle of least surprise.
(2) It's not clear that AccountTrackerService will always know about the
account in this case, in which case we wouldn't have the full information
to forward on in the observer callback.

This does mean that
IdentityManager::Observer::OnRefreshTokenRemovedForAccount() is not
strictly identical semantically to
ProfileOAuth2TokenService::Observer::OnRefreshTokenRevoked(). However, we
strongly believe that the only consumer that needs to know about
revocations in this corner case is AccountTrackerService, which will not
be converted to depend on IdentityManager as it lives below
IdentityManager.

TBR=bsazonov@chromium.org

Bug:  806774 
Change-Id: I9f77525e26825ab418b15c7d1c2a8e4f8e3b4910
Reviewed-on: https://chromium-review.googlesource.com/1124320
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572185}
[modify] https://crrev.com/225bf60cc1383b273a55eb6a414caa358eecad06/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java
[modify] https://crrev.com/225bf60cc1383b273a55eb6a414caa358eecad06/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/225bf60cc1383b273a55eb6a414caa358eecad06/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/225bf60cc1383b273a55eb6a414caa358eecad06/services/identity/public/cpp/identity_manager_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment