Port identity extension API impl away from observing AccountTracker to observing PO2TS |
||||||||
Issue descriptionTo 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.
,
Jun 5 2017
,
Jun 5 2017
,
Jun 5 2017
,
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
,
Sep 27 2017
,
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
,
Nov 1 2017
,
Nov 7 2017
,
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
,
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
,
Mar 23 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by blundell@chromium.org
, Jun 5 2017