New issue
Advanced search Search tips

Issue 729542 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 683124
issue 729543
issue 729589



Sign in to add a comment

Port identity extension API impl away from observing AccountTracker to observing PO2TS

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

Issue description

To implement the sending of the onSignInEventChanged() event, the identity extension API implementation currently observes AccountTracker. Per crbug.com/729540, the canonical observer interface that we will expose via the Identity Service is the OAuth2TokenService observer interface. As a preliminary to migrating this implementation to use the Identity Service, we need to migrate it from listening to AccountTracker to listening to ProfileOAuth2TokenService.

Note that accomplishing this is not entirely obvious: AccountTracker notifies its observers in response to O2TS::OnRefreshTokenAvailable()/O2TS::OnRefreshTokenRevoked(), but also in additional cases. Need to figure out how to map those additional cases to listening to O2TS.

 
Labels: IdentityService
Blocking: 683124
Blocking: 729543
Blocking: 729589
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 1 2017

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

commit a423b41ade15bc67e3ccc6aefe3947843f875707
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Aug 01 17:15:23 2017

Add browsertests of chrome.identity.OnSignInChanged event impl

Upcoming work is going to move the implementation of the
chrome.identity.OnSignInChanged extension API event to be a consumer
of the Identity Service. That implementation currently works by being
a gaia::AccountTracker::Observer. Unfortunately, the precise
semantics of both the identity extension API event and
gaia::AccountTracker::Observer are undocumented. This CL adds
browsertests of the implementation of chrome.identity.OnSignInChanged
that serve to effectively nail down its semantics to what
gaia::AccountTracker::Observer currently provides; that way future
changes will be assured of having no user-visible effect.

These semantics are as follows (determined by code inspection of
AccountTracker, and in particular by its behavior in its impl of
the underlying OAuth2TokenService::Observer and
IdentityProvider::Observer interfaces):

- An event fires when the primary account signs in, signs out, has
  a refresh token available, or has a refresh token revoked.
- An event fires for a secondary account when it has a refresh token
  available or revoked IF there is a primary account available.
- When the primary account signs out, an event fires not only for the
  primary account but also for all secondary accounts that currently
  have a refresh token available.

Bug:  729542 
Change-Id: If4145919254a9a924b06f38a632708780fe7d9dc
Reviewed-on: https://chromium-review.googlesource.com/593654
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491027}
[modify] https://crrev.com/a423b41ade15bc67e3ccc6aefe3947843f875707/chrome/browser/extensions/api/identity/identity_api.cc
[modify] https://crrev.com/a423b41ade15bc67e3ccc6aefe3947843f875707/chrome/browser/extensions/api/identity/identity_api.h
[modify] https://crrev.com/a423b41ade15bc67e3ccc6aefe3947843f875707/chrome/browser/extensions/api/identity/identity_apitest.cc

Owner: blundell@chromium.org
Status: Started (was: Available)
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 27 2017

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

commit e95466de34baf75eb4cbdd757b6570c28008b6f2
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Sep 27 17:07:20 2017

Add more browsertests of chrome.identity.onSignInChanged event impl

This CL follows up on https://chromium-review.googlesource.com/593654 by
adding another browsertest. This browsertest covers an aspect of the
semantics that I had not realized before:
- When the primary account signs in, a signin event fires not just for
  that account but also for all secondary accounts that have refresh
  tokens available.

As described fully in the prior CL, these semantics are defined by
examination of the AccountTracker implementation in preparation for
refactoring away from using AccountTracker.

This CL also updates the test with my new understanding that when a
single state change causes multiple events to fire, the order in which
these events fire is effectively undefined (to be precise, it's defined
by internal implementation details of AccountTracker and
ProfileOAuth2TokenService that are not part of those classes' public
contracts).

Bug:  729542 
Change-Id: If10d3c25f0a5be015678dabae882d2eafc6f1212
Reviewed-on: https://chromium-review.googlesource.com/684835
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504711}
[modify] https://crrev.com/e95466de34baf75eb4cbdd757b6570c28008b6f2/chrome/browser/extensions/api/identity/identity_apitest.cc

Components: Services>SignIn
Components: Internals>Services>Identity
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 21 2018

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

commit 1f7826eeb68096157f8067db9e8edf82a26278b0
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Mar 21 11:13:54 2018

Remove tests for undefined chrome.identity.onSigninChanged behavior

The chrome.identity.onSigninChanged extension API event is defined
as follows:

"Fired when signin state changes for an account on the user's profile."
(https://developer.chrome.com/apps/identity#event-onSignInChanged)

The current browser-side implementation uses gaia::AccountTracker,
which ignores events for secondary accounts if there is no primary
account (i.e., syncing account) present. Thus, this browser-side
implementation also fires events for secondary accounts only if there is
a primary account present.

However:

(1) This behavior is not defined in the documented semantics above
(2) In practice, this case has historically never been encountered by
end users, as it has been impossible in desktop Chrome to have a
secondary account without a primary account present.

In this CL, we are removing browsertests that enforce this undocumented
behavior (note that we ourselves added these browsertests merely to
document the existing implementation's behavior before refactoring it).
By removing the requirement that the implementation behave in a specific
way in this case, we will ease the upcoming refactoring significantly.

Note that post-project DICE, it will be possible for this case to be
encountered by end users. In that world, it will actually be more
sensible for events to fire for secondary accounts regardless of
whether or not the user has designated a syncing account. This behavior
is what will occur post the refactoring that will follow this CL.

Bug:  729542 
Change-Id: I02c1ffbcbfd732ba17056e939eb6f86ab3b26576
Reviewed-on: https://chromium-review.googlesource.com/966035
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544665}
[modify] https://crrev.com/1f7826eeb68096157f8067db9e8edf82a26278b0/chrome/browser/extensions/api/identity/identity_apitest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 22 2018

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

commit 798a107597af39aba4fd365d920d58015516c9b4
Author: Colin Blundell <blundell@chromium.org>
Date: Thu Mar 22 15:43:47 2018

Change undefined behavior of chrome.identity.onSigninChanged event

The chrome.identity.onSigninChanged extension API event is defined
as follows:

"Fired when signin state changes for an account on the user's profile."
(https://developer.chrome.com/apps/identity#event-onSignInChanged)

The current browser-side implementation uses gaia::AccountTracker,
which ignores events for secondary accounts if there is no primary
account (i.e., syncing account) present. Thus, this browser-side
implementation also fires events for secondary accounts only if there is
a primary account present.

However:

(1) This behavior is not defined in the documented semantics above
(2) In practice, this case has historically never been encountered by
end users, as it has been impossible in desktop Chrome to have a
secondary account without a primary account present.

This CL changes this undocumented behavior, so that events for
secondary events will fire regardless of whether a primary account is
present. Post-project DICE, it will be possible for this case to be
encountered by end users. In that world, it will actually be more
sensible for events to fire for secondary accounts regardless of
whether or not the user has designated a syncing account.

The CL implements this change by porting IdentityAPI away from using
gaia::AccountTracker to observing ProfileOAuth2TokenService and
AccountTrackerService directly. By doing so, IdentityAPI observes
(and fires events for) signin change events for secondary accounts
regardless of whether a primary account is present.

This change has a side benefit in removing usage of the AccountTracker
class, which is deprecated with an eye toward complete removal
(see details in crbug.com/729590). It will be followed up by a
conversion of this code to use the Identity Service client library.

To 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.onSignInChanged.addListener((account, signed_in) => {console.log(account.id + " " + signed_in);} )
Sign out of the browser. Verify that you receive a callback at the console
with a value of false.
Sign back in. Verify that you receive another callback at the console for
the same account ID with a value of true.

Bug:  729589 ,  729542 ,  769700 
Change-Id: I99dd48d34b380067ac34a2207effd9d3279191fd
Reviewed-on: https://chromium-review.googlesource.com/596368
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545081}
[modify] https://crrev.com/798a107597af39aba4fd365d920d58015516c9b4/chrome/browser/extensions/api/identity/identity_api.cc
[modify] https://crrev.com/798a107597af39aba4fd365d920d58015516c9b4/chrome/browser/extensions/api/identity/identity_api.h
[modify] https://crrev.com/798a107597af39aba4fd365d920d58015516c9b4/chrome/browser/extensions/api/identity/identity_apitest.cc

Status: Fixed (was: Started)

Sign in to add a comment