Port consumers of //components/signin classes to use Identity Service client library |
||||||||||||||||||||||||||||||||||||||||
Issue descriptionThe 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 ⛆ |
|
|
,
Jan 2 2018
,
Jan 2 2018
,
Jan 2 2018
,
Jan 2 2018
,
Jan 3 2018
,
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
,
Jan 15 2018
,
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
,
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
,
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
,
Feb 5 2018
,
Feb 5 2018
,
Feb 5 2018
,
Feb 5 2018
,
Feb 6 2018
,
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
,
Feb 19 2018
,
Feb 22 2018
,
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
,
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
,
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
,
May 16 2018
,
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
,
Jun 1 2018
,
Jun 5 2018
,
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
,
Jun 6 2018
,
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
,
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
,
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
,
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
,
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
,
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
,
Jun 26 2018
,
Aug 2
,
Aug 3
,
Sep 12
,
Sep 12
,
Sep 12
,
Sep 12
,
Sep 12
,
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
,
Nov 9
,
Nov 27
,
Dec 5
,
Dec 5
,
Dec 10
,
Dec 10
,
Dec 11
,
Dec 11
,
Dec 14
,
Dec 14
,
Jan 14
|
|||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||||||||||||||||
Comment 1 by blundell@chromium.org
, Dec 20 2017