New issue
Advanced search Search tips

Issue 889764 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 880848
issue 887264



Sign in to add a comment

Add |id_token| field to AccessTokenInfo and populate it from AccessTokenFetcher/PrimaryAccountAccessTokenFetcher

Project Member Reported by blundell@chromium.org, Sep 27

Issue description

This information is supplied by OAuth2TokenService in a success response to a request for an access token but is not passed along by AccessTokenFetcher. At least one consumer needs it (see blocked bug).
 
Cc: -ma...@igalia.com blundell@chromium.org
Owner: ma...@igalia.com
Mario has already started this work :).

Mario, if possible it would be great to test that id_token information that comes from the token service is passed along as well. I think that we can do that fairly easily by adding overloads to the relevant FakeProfileOAuth2TokenService methods that taken in an id_token as well as the other fields. Are you up for investigating that as part of this bug? If not, I could look at it as a followup. Thanks!
Thanks Colin. Just to clarify, by "adding overloads to the relevant FakeProfileOAuth2TokenService methods", do you mean those like the following one?

  // Helper routines to issue tokens for pending requests.
  void IssueAllTokensForAccount(const std::string& account_id,
                                const std::string& access_token,
                                const base::Time& expiration);


..which would get an overload like this:

  // Helper routines to issue tokens for pending requests.
  void IssueAllTokensForAccount(const std::string& account_id,
                                const std::string& access_token,
                                const base::Time& expiration,
                                const std::string& id_token);

And then I suppose I'd need to adapt some unittest as well, access_token_fetcher_unittest.cc maybe? Anything else?

That's what I was thinking, yup! Feel free to tackle it in two stages though: a CL adding the field and passing an empty string in the tests with a TODO to pass valid data through and check that it's preserved, and then a CL fixing that TODO via the additions of the overloads.
Thanks for clarifying Colin, much appreciated. I also think it makes sense to split into 2 CLs, here comes the first one (no tests):

https://chromium-review.googlesource.com/c/chromium/src/+/1248625
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 27

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

commit 8f0249ae91750468210e7047a54e6cb76ed6d737
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Thu Sep 27 14:27:03 2018

Add |id_token| field to AccessTokenInfo

This hasn't yet been migrated from OAuth2AccessTokenConsumer::TokenResponse
and it will be useful at least to determine whether we are under advanced
protection from AdvancedProtectionStatusManager once a token is fetched.

Additional test coverage will be added in a follow-up patch.

TBR=thestig@chromium.org

Bug:  889764 
Change-Id: Id9a8bcc7b2e3a5238b7a1c98f7787a8e905b6add
Reviewed-on: https://chromium-review.googlesource.com/1248625
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594713}
[modify] https://crrev.com/8f0249ae91750468210e7047a54e6cb76ed6d737/chrome/browser/printing/cloud_print/gcd_api_flow_unittest.cc
[modify] https://crrev.com/8f0249ae91750468210e7047a54e6cb76ed6d737/services/identity/public/cpp/access_token_fetcher.cc
[modify] https://crrev.com/8f0249ae91750468210e7047a54e6cb76ed6d737/services/identity/public/cpp/access_token_fetcher_unittest.cc
[modify] https://crrev.com/8f0249ae91750468210e7047a54e6cb76ed6d737/services/identity/public/cpp/access_token_info.cc
[modify] https://crrev.com/8f0249ae91750468210e7047a54e6cb76ed6d737/services/identity/public/cpp/access_token_info.h
[modify] https://crrev.com/8f0249ae91750468210e7047a54e6cb76ed6d737/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 4

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

commit 844ab293fe660ce60031970a4bd83eab8e6650f5
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Thu Oct 04 13:46:32 2018

Test id_token with AccessTokenFetcher and PrimaryAccountAccessTokenFetcher

Add new overloads to FakeProfileOAuth2TokenService and IdentityTestEnvironment
methods so that we can specify the |id_token| that should be returned along
with a token when fetched via AccessTokenFetcher, and use that to check that
such information is indeed passed along from the relevant unit tests.

Bug:  889764 

Change-Id: I593dca07362ce7392d2cd2439369f1b440deb1b3
Reviewed-on: https://chromium-review.googlesource.com/c/1249086
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#596649}
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/components/signin/core/browser/fake_profile_oauth2_token_service.cc
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/components/signin/core/browser/fake_profile_oauth2_token_service.h
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/services/identity/public/cpp/DEPS
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/services/identity/public/cpp/access_token_fetcher_unittest.cc
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/services/identity/public/cpp/identity_test_environment.cc
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/services/identity/public/cpp/identity_test_environment.h
[modify] https://crrev.com/844ab293fe660ce60031970a4bd83eab8e6650f5/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Status: Fixed (was: Started)
Resolving now

Sign in to add a comment