New issue
Advanced search Search tips

Issue 729556 link

Starred by 2 users

Issue metadata

Status: WontFix
Owner:
Closed: Nov 29
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Sign in to add a comment

Migrate GetAuthTokenFunction away from calling synchronous methods for getting signin info

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

Issue description

As part of porting GetAuthTokenFunction to depending on signin code via the Identity Service, we need to determine how to port it away from using the synchronous methods:

- SigninManagerBase::GetAuthenticatedAccountId()
- ProfileOAuth2TokenService::RefreshTokenIsAvailable()

Note that analyzing this case will serve as an initial use case for developing our solution to  crbug.com/729554 .
 
Labels: IdentityService
Blocking: 729554
Blocking: 683124
Another synchronous function that GetAuthTokenFunction calls is the following:

- AccountTracker::FindAccountIdsByGaiaId() (via IdentityAPI).

Solving the usage of this method goes together with solving the usage of the above methods; it's called in the same flow as SigninManagerBase::GetAuthenticatedAccountId() for computing the account ID that GetAuthTokenFunction will use.
Blocking: 729582
Blocking: 729973
Owner: blundell@chromium.org
Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 23 2017

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

commit 2f987fdf1b82995030f3709a8dae6320687352ee
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Jun 23 11:15:53 2017

Have Identity extension API impl get account ID via Identity Service

As part of porting the Identity extension API impl to use the Identity
Service to fulfill all its needed signin functionality, this CL ports
IdentityGetAuthTokenFunction to obtain the account ID that it should use
from the Identity Service rather than directly from SigninManager and
AccountTrackerService. The only subtlety is in handling the case where
the extension is using a GAIA ID that does not correspond to that of
the primary (authenticated) account. To handle that case, we introduce
IdentityManager::GetAccountInfoFromGaiaId().

Bug:  729556 
Test: Install a Chrome extension with the identity permissions in its manifest.
Go to chrome://extensions, enable developer mode, and inspect the background
page of the above app. At the JS console that that brings up, execute:
chrome.identity.getAuthToken((token) => {console.log(token);} )
Verify that a long alphanumeric string appears rather than an error being
reported.

Change-Id: I9247c3a73d9049994eb9d41c9b440830c4e9fc92
Reviewed-on: https://chromium-review.googlesource.com/536754
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481847}
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/chrome/browser/extensions/api/identity/identity_get_auth_token_function.h
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/chrome/browser/profiles/profile_impl.cc
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/services/identity/identity_manager.cc
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/services/identity/identity_manager.h
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/services/identity/identity_manager_unittest.cc
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/services/identity/identity_service.cc
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/services/identity/identity_service.h
[modify] https://crrev.com/2f987fdf1b82995030f3709a8dae6320687352ee/services/identity/public/interfaces/identity_manager.mojom

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 27 2017

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

commit 57fb65023bddfdfcaf42478b35e9ec17e6f9c53d
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jun 27 13:39:25 2017

Add concept of "state of account" to Identity Service and example usage

Clients of signin code have a common use case of getting an AccountInfo
(e.g., for the primary account) followed by getting information about
the state of that account (e.g., whether it has a refresh token
available). This CL facilitates porting this use case to the Identity
Service by:

- Adding an AccountState Mojo interface. AccountState represents the
  current state of the account (as opposed to AccountInfo, which
  represents long-lived information about the account such as the
  Account ID).
- Having the methods that return an AccountInfo return an AccountState
  as well.

Note that AccountState currently only carries the information of
whether the account has a refresh token available, but we anticipate
adding other information over time (e.g., the last auth error that
was encountered for this account).

This CL also ports the implementation of the
chrome.identity.getAuthToken() function to use the new functionality
instead of directly calling
OAuth2TokenService::RefreshTokenIsAvailable() after receiving the
account ID.

Bug:  729554 ,  729556 
Change-Id: I9a1c3ecb6d9be491850e7ab99388515a34705eef
Reviewed-on: https://chromium-review.googlesource.com/538672
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482613}
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/chrome/browser/extensions/api/identity/identity_get_auth_token_function.h
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/chrome/browser/extensions/api/identity/identity_get_profile_user_info_function.cc
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/chrome/browser/extensions/api/identity/identity_get_profile_user_info_function.h
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/identity_manager.cc
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/identity_manager.h
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/identity_manager_unittest.cc
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/BUILD.gn
[add] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/account_state.cc
[add] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/account_state.h
[add] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/account_state.typemap
[add] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/account_state_struct_traits.cc
[add] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/account_state_struct_traits.h
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/cpp/typemaps.gni
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/interfaces/BUILD.gn
[add] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/interfaces/account_state.mojom
[modify] https://crrev.com/57fb65023bddfdfcaf42478b35e9ec17e6f9c53d/services/identity/public/interfaces/identity_manager.mojom

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 6 2017

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

commit 9e6f39a8535a8c3e9764f2fc0958236fffa96224
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Jul 06 11:56:27 2017

Eliminate GetAuthTokenFunction's knowledge of SigninManager

As part of converting the identity extension API impl to use the
Identity Service, this CL converts the last remaining use of
SigninManager in GetAuthTokenFunction to use the Identity Service
instead. The conversion is trivial, as the information was already
available via the Identity Service.

This CL also takes the opportunity to fold IdentitySigninFlow into
IdentityGetAuthTokenFunction, as the alternative was to extend the
delegate protocol between those two classes.

This CL also excludes browsertests of the interactive login flow
from being run on ChromeOS. The interactive login flow is always
short-circuited on ChromeOS, so these tests are not relevant on
that platform. Previously they were passing because the short-circuit
was in a function that was completely mocked out by the test. Now the
flow is changed such that the short-circuit runs before the function
that gets mocked (ShowLoginPopup()) is called.

Finally, this CL completely removes one browsertest that on inspection
is redundant with the browsertests just above it.

Bug:  729556 
Test: Install a Chrome extension with the identity permissions in its manifest.
Sign out of the browser.
Go to chrome://extensions, enable developer mode, and inspect the background
page of the above app. At the JS console that that brings up, execute:
chrome.identity.getAuthToken({interactive: true}, (token) => {console.log(token);} )
This should bring up a signin page in the browser. Verify that a long
alphanumeric string appears at the JS console once you have signed in to the
browser.

Change-Id: I6aa058e475c6809c947d1104859ee002f7ed9113
Reviewed-on: https://chromium-review.googlesource.com/558096
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484546}
[modify] https://crrev.com/9e6f39a8535a8c3e9764f2fc0958236fffa96224/chrome/browser/extensions/BUILD.gn
[modify] https://crrev.com/9e6f39a8535a8c3e9764f2fc0958236fffa96224/chrome/browser/extensions/api/identity/identity_api.h
[modify] https://crrev.com/9e6f39a8535a8c3e9764f2fc0958236fffa96224/chrome/browser/extensions/api/identity/identity_apitest.cc
[modify] https://crrev.com/9e6f39a8535a8c3e9764f2fc0958236fffa96224/chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc
[modify] https://crrev.com/9e6f39a8535a8c3e9764f2fc0958236fffa96224/chrome/browser/extensions/api/identity/identity_get_auth_token_function.h
[delete] https://crrev.com/41f2338e41b11fb6cf1ec6574856fa9a864196d7/chrome/browser/extensions/api/identity/identity_signin_flow.cc
[delete] https://crrev.com/41f2338e41b11fb6cf1ec6574856fa9a864196d7/chrome/browser/extensions/api/identity/identity_signin_flow.h

Components: Services>SignIn
Components: Internals>Services>Identity

Comment 13 by treib@chromium.org, Apr 18 2018

Blocking: 825190

Comment 14 by treib@chromium.org, Apr 25 2018

Blocking: -825190
Status: WontFix (was: Started)
Obsolete now that we're making a client library for preserving synchronous access via caching.

Sign in to add a comment