New issue
Advanced search Search tips

Issue 796544 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task


Sign in to add a comment

Port consumers of //components/signin classes to use Identity Service client library

Project Member Reported by blundell@chromium.org, Dec 20 2017

Issue description

The primary interface of the Identity Service client library, IdentityManager, now exists (//services/identity/public/cpp/identity_manager.h). In parallel with continuing to build out that interface, we can convert consumers of the signin core code to use IdentityManager instead. This signin core code will eventually be isolated in the Identity Service implementation.

Core signin classes most notably include:
- SigninManager(Base)
- ProfileOAuth2TokenService
- AccountTrackerService
 
Blockedon: 796545
Blockedon: 798408
Blockedon: 798411
Blockedon: 798412
Blockedon: 798413
Blockedon: 798699
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 11 2018

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

commit e4ed08b133ec1411d8b81e84c2eef51f683378d5
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jan 11 18:38:44 2018

Change IdentityManager DCHECK on ChromeOS to reflect reality of tests

IdentityManager currently has a DCHECK in its constructor that the
SigninManager is authenticated. This DCHECK should always hold in
production. However, it is very much not guaranteed in testing contexts:
all kinds of unittests and browsertests do not bother to set the
authenticated account info (and even when they do, it might be much
later than the construction of the IdentityManager).

To accommodate this reality while keeping the spirit of the DCHECK, this
CL moves the DCHECK to IdentityManager::GetPrimaryAccountInfo(),
checking that the IdentityManager's primary account info is the same as
that held by the SigninManager. This latter DCHECK will go off in tests
if (a) those tests set the authenticated account info in the
SigninManager and (b) those tests run code that gets the primary account
info from IdentityManager. At that point, the tests should be changed
to set the primary account info in IdentityManager via a testing API
at the same place where they set the authenticated account info in
SigninManager.

Note that this CL does not yet introduce the testing API mentioned
above as I want to hit a concrete case in order to design the exact
shape of the API.

Bug: 796544
Change-Id: I7a6513247cc718e06567cac17151970097da0077
Reviewed-on: https://chromium-review.googlesource.com/861465
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528679}
[modify] https://crrev.com/e4ed08b133ec1411d8b81e84c2eef51f683378d5/services/identity/public/cpp/identity_manager.cc

Blockedon: 801968
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 15 2018

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

commit aa31960e426f678621ed7d424b284d002718dfe1
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jan 15 14:52:30 2018

NTP Snippets: Remove dead includes of signin code

Helps to clarify the refactorings that need to be done to convert
NTP Snippets to use //services/identity/public/cpp.

Bug: 796544
Change-Id: Iec9e62845d66db26b8b3bb34ec95a4fd351ce90c
Reviewed-on: https://chromium-review.googlesource.com/861785
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529279}
[modify] https://crrev.com/aa31960e426f678621ed7d424b284d002718dfe1/components/ntp_snippets/remote/json_request.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 18 2018

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

commit 9ae080fcabc2a9a4c8172ed31a5375a8c4f1de3d
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jan 18 10:34:02 2018

Add API to query whether IdentityManager has a primary account

Simple convenience wrapper over calling
IdentityManager::GetPrimaryAccountInfo() and checking whether it is
empty.

Will be used by an upcoming CL.

Bug: 796544
Change-Id: I967abd045f9b555492c99ed3ee4ae972682e2988
Reviewed-on: https://chromium-review.googlesource.com/870117
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530111}
[modify] https://crrev.com/9ae080fcabc2a9a4c8172ed31a5375a8c4f1de3d/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/9ae080fcabc2a9a4c8172ed31a5375a8c4f1de3d/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/9ae080fcabc2a9a4c8172ed31a5375a8c4f1de3d/services/identity/public/cpp/identity_manager_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 30 2018

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

commit 4632ec083af62c80ee9cf2b384ccdff2080e6e2f
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jan 30 11:33:54 2018

Ensure that IdentityManager gets signin events before signin observers

IdentityManager currently is informed of signin and signout events by
observing SigninManager. This is problematic when incrementally
converting the codebase to use IdentityManager rather than directly
using SigninManager: other consumers also observe SigninManager, and
the order of receiving observer notifications is not defined between
these consumers and IdentityManager. This lack of ordering can result
in consumers receiving (e.g.) a notification from SigninManager that
signin has occurred while the IdentityManager still believes that the
user is signed out.

In the long term this won't be a problem, because there will be no
direct consumers of SigninManager. However, for the conversion period
we need to ensure that IdentityManager is notified of signin and signout
events before any SigninManager observers. This CL makes that change
by
(a) adding a SigninManager::DiagnosticsClient interface whose callbacks
fire just before the SigninManager::Observer callbacks
(b) changing IdentityManager to modify its internal view of the primary
account in response to the DiagnosticsClient callbacks rather than
SigninManager::Observer callbacks.

This CL also adds IdentityManager unittests of this property that fail
before the production change is made.

Bug: 796544
Change-Id: I089813d458e122e93b143099ba2a284f972e42fa
Reviewed-on: https://chromium-review.googlesource.com/883347
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532838}
[modify] https://crrev.com/4632ec083af62c80ee9cf2b384ccdff2080e6e2f/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/4632ec083af62c80ee9cf2b384ccdff2080e6e2f/components/signin/core/browser/signin_manager.h
[modify] https://crrev.com/4632ec083af62c80ee9cf2b384ccdff2080e6e2f/services/identity/DEPS
[modify] https://crrev.com/4632ec083af62c80ee9cf2b384ccdff2080e6e2f/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/4632ec083af62c80ee9cf2b384ccdff2080e6e2f/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/4632ec083af62c80ee9cf2b384ccdff2080e6e2f/services/identity/public/cpp/identity_manager_unittest.cc

Blockedon: 808978
Blockedon: 809027
Blockedon: 809030
Blockedon: 809033
Blockedon: 809539
Project Member

Comment 17 by bugdroid1@chromium.org, Feb 9 2018

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

commit 4cb750653065022638f6cfcc810f2da21554f43e
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Feb 09 15:40:25 2018

[iOS WebView] Add IdentityManagerFactory for iOS WebView

An upcoming refactoring will introduce usage of IdentityManager in
Autofill. In turn, Autofill is used on iOS WebView. Hence we need to
add a factory for producing IdentityManager in the context of iOS
WebView. This is unproblematic to do: the factory is copied from
//ios/chrome with only naming modifications (unfortunately, git cannot
see these as copies regardless of how I muck with settings for
similarity and finding copies).

Bug: 796544
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Idd4fe0eaf5d1e3b83ce80c15fb1052b8ee049846
Reviewed-on: https://chromium-review.googlesource.com/911468
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535719}
[modify] https://crrev.com/4cb750653065022638f6cfcc810f2da21554f43e/ios/web_view/BUILD.gn
[modify] https://crrev.com/4cb750653065022638f6cfcc810f2da21554f43e/ios/web_view/internal/DEPS
[add] https://crrev.com/4cb750653065022638f6cfcc810f2da21554f43e/ios/web_view/internal/signin/web_view_identity_manager_factory.h
[add] https://crrev.com/4cb750653065022638f6cfcc810f2da21554f43e/ios/web_view/internal/signin/web_view_identity_manager_factory.mm

Blockedon: 813560
Blockedon: 814787
Project Member

Comment 20 by bugdroid1@chromium.org, Feb 22 2018

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

commit b24251e600a2dea981b5dda6ed9517a329471109
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Feb 22 21:58:12 2018

Make sync integration tests IdentityManager-friendly

The codebase is in the process of being incrementally converted from
usage of //components/signin to usage of //services/identity/public/cpp.
In the process of this conversion, IdentityManager can end up being
instantiated in the context of sync_integration_tests. IdentityManager
has internal DCHECKs that verify that its view of the primary account
is consistent with SigninManager on signin/signout events. As
ProfileSyncServiceHarness triggers such events in an ad-hoc fashion,
these DCHECKs can fire. This CL modifies ProfileSyncServiceHarness so
that it goes through IdentityManager to set the primary account info.
Doing this sets the primary account information both in IdentityManager
and also in SigninManager and ProfileOAuth2TokenService.

Ideally this flow would go through identity_test_utils.h, but as
 https://crbug.com/814307  details, making that change is challenging.

This change is concretely needed for an upcoming conversion to avoid
tickling this issue :).

Bug: 796544
Change-Id: Id6eac4f32ff316b7400df2cbeb5484a98cd1b1c6
Reviewed-on: https://chromium-review.googlesource.com/913579
Reviewed-by: Nicolas Zea <zea@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538578}
[modify] https://crrev.com/b24251e600a2dea981b5dda6ed9517a329471109/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
[modify] https://crrev.com/b24251e600a2dea981b5dda6ed9517a329471109/services/identity/public/cpp/identity_manager.h

Project Member

Comment 21 by bugdroid1@chromium.org, Feb 27 2018

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

commit ee1bf455d5170c6dbcc377d6830878bcde7cbfb0
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Feb 27 18:15:37 2018

Make ChromeSessionManager's "signin flow" IdentityManager-friendly

The codebase is in the process of being incrementally converted from
usage of //components/signin to usage of //services/identity/public/cpp.
In the process of this conversion, IdentityManager can end up being
instantiated in the context of browser_tests. IdentityManager
has internal DCHECKs that verify that its view of the primary account
is consistent with SigninManagerBase. As ChromeSessionManager sets
signin information manually on SigninManager, these DCHECKs can fire.
This CL modifies ChromeSessionManager so that it goes through
IdentityManager to set the primary account info.  Doing this sets the
primary account information both in IdentityManager and also in
SigninManagerBase.

Note that one complexity is that this flow does not set the refresh
token. In the long term we would ideally set the refresh token as part
of this flow in order to unify this flow with other signin flows in the
codebase. However, that is a behavioral change that is orthogonal to the
change being made here. In this CL, we simply add a method to
IdentityManager that allows for setting only the GAIA ID/email address
but not the refresh token. https://crbug.com/814787 tracks the work
required to remove this method and port this flow to use more
mainstream Identity Service APIs.

This change is concretely needed for an upcoming conversion to avoid
tickling this issue :).

Bug: 796544
Change-Id: I405a8883bef03c2aaa94b01ed7bebf5e41a1164d
Reviewed-on: https://chromium-review.googlesource.com/931469
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539498}
[modify] https://crrev.com/ee1bf455d5170c6dbcc377d6830878bcde7cbfb0/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/ee1bf455d5170c6dbcc377d6830878bcde7cbfb0/chrome/browser/chromeos/login/session/chrome_session_manager.cc
[modify] https://crrev.com/ee1bf455d5170c6dbcc377d6830878bcde7cbfb0/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/ee1bf455d5170c6dbcc377d6830878bcde7cbfb0/services/identity/public/cpp/identity_manager.h

Project Member

Comment 22 by bugdroid1@chromium.org, Apr 23 2018

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

commit 523c662a78133425ffead1c689acfb3058762023
Author: Marc Treib <treib@chromium.org>
Date: Mon Apr 23 11:03:31 2018

Migrate chromeos::UserSessionManager to IdentityManager

identity::IdentityManager is the new API that replaces
SigninManager[Base].

Bug:  825190 , 796544, 814787
Change-Id: If6dff8e3d7e93c21bbc7392bf434dfe73da9f92d
Reviewed-on: https://chromium-review.googlesource.com/1013575
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552667}
[modify] https://crrev.com/523c662a78133425ffead1c689acfb3058762023/chrome/browser/chromeos/login/session/user_session_manager.cc
[modify] https://crrev.com/523c662a78133425ffead1c689acfb3058762023/services/identity/public/cpp/identity_manager.h

Blocking: 843510
Project Member

Comment 24 by bugdroid1@chromium.org, May 30 2018

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

commit 953eaeb57d42ad934fe3f7843371a995858810ce
Author: Colin Blundell <blundell@chromium.org>
Date: Wed May 30 10:52:32 2018

Remove PrimaryAccountAccessTokenFetcher observing OnRefreshTokensLoaded

PrimaryAccountAccessTokenFetcher currently has the following flow:
- wait for user to sign in
- wait for primary account refresh token to become available
- if during that waiting refresh tokens are loaded, fail (by making an
  access token request that will be guaranteed to fail)

The third step above leads to inconsistency:

Assume that a user is signed in but their token is not available on
disk (e.g., it's been corrupted). If a PrimaryAccountAccessTokenFetcher
is created *before* the tokens are loaded from disk, that fetcher will
return an error to its client once tokens are loaded. If a
PrimaryAccountAccessTokenFetcher is created after the tokens are loaded,
that fetcher will wait until the refresh token actually becomes
available (e.g., via a reauth). It will potentially wait forever.

There seems no reason to have the distinction of whether a
PrimaryAccountAccessTokenFetcher was created before or after tokens were
loaded from disk result in behavioral differences that are visible to
clients of PrimaryAccountAccessTokenFetcher.

Note that by definition this change can result in behavioral changes for
the cases where PrimaryAccountAccessTokenFetchers are created before
tokens are loaded from disk. However, as clients of
PrimaryAccountAccessTokenFetcher do not query whether tokens have been
loaded from disk or not, any such resulting behaviors are already
possible in the existing codebase (in the case where the fetchers
happen to be created after tokens were loaded from disk, i.e., any time
after startup).

From treib@chromium.org, the original author of this code:

"I've been trying to remember if I had any particular reason for adding
this logic, but I think I just copied it from whatever I used as a model
for the very first AccessTokenFetcher implementation.

I completely agree with your reasoning that the distinction (based on
whether tokens were already loaded from disk or not) doesn't make sense."

Bug: 796544
Change-Id: I1bf855663ddee961ca68886500b92943df274f06
Reviewed-on: https://chromium-review.googlesource.com/1076289
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562795}
[modify] https://crrev.com/953eaeb57d42ad934fe3f7843371a995858810ce/services/identity/public/cpp/primary_account_access_token_fetcher.cc
[modify] https://crrev.com/953eaeb57d42ad934fe3f7843371a995858810ce/services/identity/public/cpp/primary_account_access_token_fetcher.h
[modify] https://crrev.com/953eaeb57d42ad934fe3f7843371a995858810ce/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Blockedon: 848642
Blockedon: 849652
Project Member

Comment 27 by bugdroid1@chromium.org, Jun 6 2018

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

commit 64fdfcf72598778bc4a54815da43d2a3b1ae4c76
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jun 06 11:51:42 2018

Remove PSSStartupTest as friend of IdentityManager

APIs are now in place that allow ProfileSyncServiceStartupTest to use
identity_test_utils.h, which is preferable as that has a direct
conversion path to the long-term IdentityTestEnvironment test
infrastructure.

Bug: 796544
Change-Id: I2850f950353849a50638195bd2c4276089b011a3
Reviewed-on: https://chromium-review.googlesource.com/1085463
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564860}
[modify] https://crrev.com/64fdfcf72598778bc4a54815da43d2a3b1ae4c76/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/64fdfcf72598778bc4a54815da43d2a3b1ae4c76/services/identity/public/cpp/identity_manager.h

Blockedon: 849591
Project Member

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

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

commit dc841e345e485afc44999286850605eb2da76686
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 11 13:05:46 2018

PrimaryAccountAccessTokenFetcher: Remove listening for signin failed

PrimaryAccountAccessTokenFetcher currently listens for
SigninManagerBase::Observer::GoogleSigninFailed() and stops waiting
for the user to sign in, returning the error to its client. However,
there's no particular reason it *should* stop waiting for the user to
sign in in this case:
- It's not part of the documented contract of
  PrimaryAccountAccessTokenFetcher
- It's not conceptually different than other cases such as the user
  signing out while the fetcher is waiting for a refresh token to
  become available. However, in no other such case does the fetcher
  abort waiting for the primary account to become available.

Conceptually, the contract of PrimaryAccountAccessTokenFetcher's
WaitUntilAvailable mode is that the fetcher will wait until the
primary account becomes available. Hence, the client understands that
the fetcher might wait forever.

This CL simply rips out the listening to GoogleSigninFailed(). The
concrete motivation (in addition to the consistency reasons presented
above) is that we will shortly be porting this class to talk to
IdentityManager. IdentityManager doesn't currently expose this
observer callback and I want to wait to see if there's a strong
reason for providing it before doing so.

Bug: 796544
Change-Id: I45ff3eb0eeddf54a80fae58039e41c946097c9c2
Reviewed-on: https://chromium-review.googlesource.com/1092852
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565981}
[modify] https://crrev.com/dc841e345e485afc44999286850605eb2da76686/services/identity/public/cpp/primary_account_access_token_fetcher.cc
[modify] https://crrev.com/dc841e345e485afc44999286850605eb2da76686/services/identity/public/cpp/primary_account_access_token_fetcher.h

Project Member

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

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

commit 177b89d318c4edb765b9d7beec4012242da901b7
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 11 16:04:39 2018

PrimaryAccountAccessTokenFetcher: Simplify observing of signin classes

PrimaryAccountAccessTokenFetcher currently has a very targeted
observance of SigninManager and ProfileOAuth2TokenService:
- It observes SigninManager during the duration of time from which it
  determines it needs to see a signin to when that signin occurs.
- It observes PO2TS during the duration of time from which it
  determines that the user is signed in without a refresh token to when
  the refresh token becomes available.

This targeted observance allows for having nice DCHECKs around exactly
what state of waiting the token fetcher is in when given observer
callbacks occur. However, we will shortly be porting this class to
talk to IdentityManager. After the porting, it will no longer be
possible to maintain this targeted observance, as by design
IdentityManager::Observer folds the functionality of
SigninManagerBase::Observer and OAuth2TokenService::Observer together.
Thus, for example, if the token fetcher is obstensibly waiting for a
refresh token to become available, it might in fact see the user sign
in again (if they managed to sign out and sign back in in the interim,
for example).

This CL changes PrimaryAccountAccessTokenFetcher in anticipation of
that porting:
- It starts observing SigninManager and PO2TS immediately when
  launching in WaitUntilAvailable mode via ScopedObserver instances if
  credentials are not immediately available.
- When it starts an access token request, it removes these observers
  if they are present (in addition to not being present if launched
  in RequestImmediately mode, access token requests get retried once by
  the fetcher in WaitUntilAvailable mode; thus, it's not guaranteed that
  these observers will be present at the time of starting an access
  token request).
- Otherwise, the observers are removed implicitly on destruction.

A side effect of these changes is that the nice DCHECKs mentioned
above must necessarily be removed. In fact, there could be behavioral
changes; e.g., PrimaryAccountAccessTokenFetcher could observe a signin
event where it hadn't before in the case mentioned above. However,
these behavioral changes are harmless, as
PrimaryAccountAccessTokenFetcher's observer callback implementations
are essentially stateless: they simply wait for the moment when both
the primary account info and the refresh token for that account are
available, and then synchronously fire off an access token request and
stop listening for further signin events. Observing more signin-related
events will simply result in more checks for whether that moment has
arrived.

Bug: 796544
Change-Id: I58738bb697e38008012f7b8bc759ed8c8382c516
Reviewed-on: https://chromium-review.googlesource.com/1092575
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566023}
[modify] https://crrev.com/177b89d318c4edb765b9d7beec4012242da901b7/services/identity/public/cpp/primary_account_access_token_fetcher.cc
[modify] https://crrev.com/177b89d318c4edb765b9d7beec4012242da901b7/services/identity/public/cpp/primary_account_access_token_fetcher.h

Project Member

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

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

commit 1a291953692699605e4315bb21256caf608133e4
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 11 18:54:33 2018

PrimaryAccountAccessTokenFetcher: Remove unsuitable unittest

PrimaryAccountAccessTokenFetcher has a unittest that checks that the
fetcher takes no action on the firing of the event that tokens were
loaded by the token service. This test only exists as an artifact of
the time when PAATF *did* take action on this event (it would return
an error to its client if it received that notification while waiting
for a refresh token to become available for the primary account).

We are shortly going to port PAATF to talk to IdentityManager. Just as
IdentityManager does not even expose this event in its observer API,
likewise the test infrastructer built around IdentityManager does not
expose any way to trigger this event. Hence, this test will both
become irrelevant (as there is no way for the production code to take
action on a tokens loaded event) and impossible to implement (as there
is no way for the test to trigger a tokens loaded event). Test-be-gone.

Note that part of the test checks that setting a refresh token for an
account other than the primary account after a fetcher has been
created doesn't cause any action on the part of the fetcher. That part
has a TODO(blundell) to move into a separate test. On inspection,
it turns out that the ShouldIgnoreRefreshTokensForOtherAccounts test
already tests exactly that (when it adds a token for
|account_id + "3"|). Hence, it's not necessary to preserve that part
either.

Bug: 796544
Change-Id: Ic1f7ed455feaea45ea39b3013de3b3097c85c238
Reviewed-on: https://chromium-review.googlesource.com/1095280
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566081}
[modify] https://crrev.com/1a291953692699605e4315bb21256caf608133e4/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Jun 12 2018

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

commit a29afeacbd0d2f72af6e7bde75317d59f2d29af1
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jun 12 14:05:47 2018

Allow PrimaryAccountAccessTokenFetcher to be reset from client callback

A common use case (*the* common use case) for clients of
PrimaryAccountAccessTokenFetcher is for these clients to want to
destroy the fetcher when they receive the callback that the access
token request completed. However, PrimaryAccountAccessTokenFetcher
passes the arguments of that callback as const references, meaning
that it's not safe for clients to destroy the fetcher until the
callback completes. Clients currently go through a moderately
complicated dance to ensure that this flow occurs safely.

This CL changes that callback to take its parameters by value, making
it safe for clients to simply destroy the fetcher from within the
invocation of the callback. This CL also goes through the clients,
simplifying any that can now be simplified.

TBR=jochen@chromium.org

Bug: 796544
Change-Id: Iaaf151a5025be3e455a129c6334fb1ec6170901a
Reviewed-on: https://chromium-review.googlesource.com/1090710
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566408}
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/chrome/browser/feedback/feedback_uploader_chrome.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/chrome/browser/feedback/feedback_uploader_chrome.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/chromeos/services/device_sync/cryptauth_token_fetcher_impl.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/chromeos/services/device_sync/cryptauth_token_fetcher_impl.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/autofill/core/browser/payments/payments_client.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/autofill/core/browser/payments/payments_client.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/browser_sync/sync_auth_manager.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/browser_sync/sync_auth_manager.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/feed/core/feed_networking_host.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/history/core/browser/web_history_service.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/ntp_snippets/breaking_news/subscription_manager_impl.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/ntp_snippets/breaking_news/subscription_manager_impl.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/omnibox/browser/contextual_suggestions_service.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/omnibox/browser/contextual_suggestions_service.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/suggestions/suggestions_service_impl.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/components/suggestions/suggestions_service_impl.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/services/identity/public/cpp/identity_manager_unittest.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/services/identity/public/cpp/primary_account_access_token_fetcher.cc
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/services/identity/public/cpp/primary_account_access_token_fetcher.h
[modify] https://crrev.com/a29afeacbd0d2f72af6e7bde75317d59f2d29af1/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Project Member

Comment 33 by bugdroid1@chromium.org, Jun 13 2018

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

commit 14e334f137a2ed71af44602420c7e930caa4361a
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jun 13 10:30:24 2018

Handle incognito mode properly in iOS IdentityManager factories

https://chromium-review.googlesource.com/c/chromium/src/+/1090277
fixed //chrome's IdentityManagerFactory to properly handle incognito
Profiles. The factories in //ios need to be fixed as well.

After discussion with sdefresne@, I now realize that it's a safer
pattern to have the //chrome-level class tying IdentityManager to
KeyedService *wrap* IdentityManager rather than *hold* IdentityManager.
The concrete reason is that that way all of the KeyedServiceFactory
methods will Just Work as expected rather than needing custom code
in the factory subclass to deal with the holder (as I had previously
directed should be added to //chrome for dealing with incognito
Profiles).

This CL thus changes all the IdentityManager factories to have a
private *subclass* of IdentityManager rather than a private *holder* of
IdentityManager. By doing this, we also ensure that incognito mode will
now be handled properly in the iOS IdentityManager factories without
requiring any custom code (i.e., it will simply use the default
KeyedServiceFactory infrastructure, which returns nullptr from
GetServiceForBrowserState() when the browser state is incognito).

Bug: 796544
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I2a11fafd28f9c1003a40bcfd2cea50403276ea90
Reviewed-on: https://chromium-review.googlesource.com/1092497
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566788}
[modify] https://crrev.com/14e334f137a2ed71af44602420c7e930caa4361a/chrome/browser/signin/identity_manager_factory.cc
[modify] https://crrev.com/14e334f137a2ed71af44602420c7e930caa4361a/ios/chrome/browser/signin/identity_manager_factory.cc
[modify] https://crrev.com/14e334f137a2ed71af44602420c7e930caa4361a/ios/web_view/internal/signin/web_view_identity_manager_factory.mm

Project Member

Comment 34 by bugdroid1@chromium.org, Jun 22 2018

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

commit e9bf0d10c2d64bb15b6853e3cb469ba68cb61523
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jun 22 16:04:29 2018

Change IdentityManager API to take in account ID

We've made the decision that IdentityManager APIs that query account
information should take in account IDs, with those that *return*
information continuing to return full AccountInfo structs. This CL
changes the one remaining API that takes in an AccountInfo
(RemoveAccessTokenFromCache()) to take in the account ID instead.

Bug: 796544
Change-Id: I1d6894fa40045fa54956f777723e1e2d3383098d
Reviewed-on: https://chromium-review.googlesource.com/1110224
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569642}
[modify] https://crrev.com/e9bf0d10c2d64bb15b6853e3cb469ba68cb61523/components/autofill/core/browser/payments/payments_client.cc
[modify] https://crrev.com/e9bf0d10c2d64bb15b6853e3cb469ba68cb61523/components/browser_sync/sync_auth_manager.cc
[modify] https://crrev.com/e9bf0d10c2d64bb15b6853e3cb469ba68cb61523/components/history/core/browser/web_history_service.cc
[modify] https://crrev.com/e9bf0d10c2d64bb15b6853e3cb469ba68cb61523/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/e9bf0d10c2d64bb15b6853e3cb469ba68cb61523/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/e9bf0d10c2d64bb15b6853e3cb469ba68cb61523/services/identity/public/cpp/identity_manager_unittest.cc

Blockedon: 856538
Blockedon: 869418
Blockedon: 870669
Blockedon: 883318
Blockedon: -797949
Blockedon: 883330
Blockedon: -797946
Blockedon: -809440
Project Member

Comment 43 by bugdroid1@chromium.org, Oct 10

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

commit 0313825d88b92a6d978d77eb8b4ed284e5ecc749
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Oct 10 13:22:40 2018

IdentityManager: Link to Google doc giving conversion information

This documentation CL adds a link to the Google doc that we are using
for enabling developers to convert usage of legacy signin APIs to
usage of IdentityManager.

Bug: 796544
Change-Id: I6df002179d29b966e2ec822b29fcf2ebf08748d8
Reviewed-on: https://chromium-review.googlesource.com/c/1270895
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598301}
[modify] https://crrev.com/0313825d88b92a6d978d77eb8b4ed284e5ecc749/services/identity/public/cpp/README.md

Blockedon: 903718
Blockedon: 908840
Blockedon: -797899
Blockedon: -797927
Blockedon: -808989
Blockedon: -809539
Blockedon: 913850
Blockedon: -797931
Blockedon: 915149
Blockedon: 915150
Blockedon: 921598

Sign in to add a comment