New issue
Advanced search Search tips

Issue 848642 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 796544



Sign in to add a comment

Port PrimaryAccountAccessTokenFetcher to be based on IdentityManager

Project Member Reported by blundell@chromium.org, Jun 1 2018

Issue description

PrimaryAccountAccessTokenFetcher currently talks directly to SigninManager/ProfileOAuth2TokenService. We need to instead have it talk to IdentityManager so that it is not coupled to these signin-internal classes.
 
Project Member

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

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

commit b4293cd68b2e169fbbf2459e22481a3cf1506d32
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 04 09:01:35 2018

PrimaryAccountAccessTokenFetcher: Remove unsuitable unittests

I am shortly going to port PrimaryAccountAccessTokenFetcher to be
based on IdentityManager rather than
SigninManager/ProfileOAuth2TokenService. In preparation, this CL
eliminates two tests that are not relevant in an IdentityManager-based
world. These tests tell SigninManager that authentication is in progress
and then test that PrimaryAccountAccessTokenFetcher doesn't return
until signin is complete. However, we are not currently planning on
exposing the idea of signin being in progress in IdentityManager.
Moreover, PrimaryAccountAccessTokenFetcher's production code does not
at any point query of SigninManager whether authentication is in
progress (and once we port it to IdentityManager, it will not be *able*
to, since we don't expose the state).

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

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 4 2018

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

commit abfc646f04fe2bff1be432de354a92bbf29c7622
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 04 09:49:19 2018

Make accountID explicit in PrimaryAccountAccessTokenFetcher unittest

The PrimaryAccountAccessTokenFetcher currently passes in a string
to SigninManager(Base)'s signin APIs and makes the assumption that this
string will end up being the account ID on all platforms. It validates
this assumption by passing in the string as the email address when
signing in on ChromeOS and as the Gaia ID when signing in on other
platforms (since in fact the account ID is defined as the email address
on ChromeOS and the Gaia ID on all other platforms).

However, this flow won't work when porting this code to use
IdentityManager. The corresponding IdentityTestEnvironment API takes
in only the email address (by design), thus disallowing one string from
playing two different roles depending on platform.

To facilitate this porting, this CL changes the
PrimaryAccountAccessTokenFetcher unittest to explicitly work with the
account ID that SigninManager(Base) has for the primary account after
the signin process has completed.

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

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 4 2018

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

commit 0605d7411404879ba447ba889f31201d51d23579
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jun 04 11:13:17 2018

IdentityManager: Add test APIs for setting primary account/refresh token

To date, IdentityManager's test APIs
(IdentityTestEnvironment/identity_test_utils.h) have intentionally only
supported making the primary account available, i.e., signed in with a
refresh token. However, PrimaryAccountAccessTokenFetcher unittests have
a legitimate use case for wanting these steps separate: there are tests
that test PrimaryAccountAccessTokenFetcher's correct operation in the
case where a fetcher is started with the primary account set but with
no token available for the primary account (with the token potentially
being made available later).

To prepare for porting PrimaryAccountAccessTokenFetcher, this CL adds
IdentityManager test APIs for setting just the primary account and
adding a refresh token for a given account. This CL also takes the
opportunity to make the test APIs that set the primary account return
the account ID, as this is useful to consumers that want to sign in
the primary account and then do followup operations on that account;
these consumers will generally need to know the primary account ID.

Bug:  848642 , 798699
Change-Id: I249e6851779545346b5f521afe18150bec20e69f
Reviewed-on: https://chromium-review.googlesource.com/1082197
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564055}
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_utils.cc
[modify] https://crrev.com/0605d7411404879ba447ba889f31201d51d23579/services/identity/public/cpp/identity_test_utils.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 16

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

commit 463a901e90aeeab8d8b4ea019abb778248acf63f
Author: Colin Blundell <blundell@chromium.org>
Date: Mon Jul 16 08:35:50 2018

Port PrimaryAccountAccessTokenFetcher to talk to IdentityManager

PAATF is currently built directly on top of SigninManager and
ProfileOAuth2TokenService. It instead should be layered on top of
IdentityManager, so that when the latter is converted to be backed by
the Identity Service PAATF automatically also becomes a true part of the
Identity Service client library.

This CL makes that change, after a rather large amount of prepatory work
giving IdentityManager all the necessary API surfaces. The actual change
is straightforward after that prepatory work. The only wrinkle is the
following:
- In the codebase currently clients create PAATF instances via an
  IdentityManager API, necessitating that identity_manager.h include
  primary_account_access_token_fetcher.h for some type definitions in
  that API.
- However, to port PAATF it is necessary for
  primary_account_access_token_fetcher.h to include identity_manager.h
  to be an IdentityManager::Observer.
- Thus, it is necessary *in this CL* to change all clients to construct
  PAATF instances directly rather than via IdentityManager. This is a
  completely straightforward change, but results in this CL touching a
  bunch more files than it otherwise would. The substantive changes in
  this CL are the ones in //services/identity.

TBR=jochen@chromium.org

Bug:  848642 
Change-Id: Ic8ecb00864b1c44f848f9daae03e01dd4f06dd05
Reviewed-on: https://chromium-review.googlesource.com/1095098
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575204}
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/chrome/browser/feedback/feedback_uploader_chrome.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/chrome/browser/printing/cloud_print/gcd_api_flow_impl.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/chrome/browser/search/background/ntp_background_service.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/chrome/browser/ui/desktop_ios_promotion/sms_service.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/autofill/core/browser/payments/payments_client.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/browser_sync/sync_auth_manager.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/cryptauth/cryptauth_client_impl.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/feed/core/feed_networking_host.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/history/core/browser/web_history_service.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/ntp_snippets/breaking_news/subscription_manager_impl.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/omnibox/browser/contextual_suggestions_service.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/components/suggestions/suggestions_service_impl.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/services/identity/public/cpp/identity_manager_unittest.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/services/identity/public/cpp/primary_account_access_token_fetcher.cc
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/services/identity/public/cpp/primary_account_access_token_fetcher.h
[modify] https://crrev.com/463a901e90aeeab8d8b4ea019abb778248acf63f/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment