New issue
Advanced search Search tips

Issue 769700 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Change undocumented semantics of chrome.identity.onSignInChangedEvent()

Project Member Reported by blundell@chromium.org, Sep 28 2017

Issue description

At this time, the semantics of chrome.identity.onSignInChangedEvent() wrt secondary accounts are the following:

- When a secondary account's refresh token is available, the event fires only if the primary account is present.
- When a secondary account's refresh token is revoked, the event fires only if the primary account is present.
- When the primary account signs in, events are fired for all secondary accounts with refresh tokens.
- When the primary account signs out, events are fired for all secondary accounts with refresh tokens.

In a post-AccountConsistency world, it's not clear whether this coupling between the primary and secondary accounts still makes sense, or whether we should change the semantics so that events firing for secondary accounts is decoupled from the state of the primary account.
 
Owner: blundell@chromium.org
Status: Started (was: Available)
Summary: Change undocumented semantics of chrome.identity.onSignInChangedEvent() (was: Re-evaluate semantics of chrome.identity.onSignInChangedEvent() in post-AccountConsistency world)
Update:
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)

As noted above, 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.

Hence, we are going to change this undocumented/undefined behavior to fire events for all secondary accounts regardless of whether a primary account is present. This change:

(1) makes sense conceptually post project DICE
(2) eases the task of converting this use case to the Identity Service
Project Member

Comment 2 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