Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872253002:
Network traffic annotation added to gaia_auth_fetcher. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 6 months ago CC:
chromium-reviews, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to gaia_auth_fetcher.
Network traffic annotation is added to network request of:
google_apis/gaia/gaia_auth_fetcher.h
google_apis/gaia/gaia_auth_fetcher.cc
google_apis/gaia/gaia_auth_fetcher_unittest.cc
ios/chrome/browser/signin/gaia_auth_fetcher_ios.h
ios/chrome/browser/signin/gaia_auth_fetcher_ios.mm
BUG=656607
Review-Url: https://codereview.chromium.org/2872253002
Cr-Commit-Position: refs/heads/master@{#475351}
Committed: https://chromium.googlesource.com/chromium/src/+/0d460449a6b80930bbddb2c54ca0340c8ea4bf7b
Patch Set 1 #Patch Set 2 : iOS files added. #
Total comments: 108
Patch Set 3 : All comments addressed. #
Total comments: 16
Patch Set 4 : Annotations updated. #
Total comments: 4
Patch Set 5 : Comments addressed. #
Total comments: 6
Patch Set 6 : Comment addressed. #
Total comments: 2
Patch Set 7 : Comments addressed. #
Messages
Total messages: 40 (26 generated)
rhalavati@chromium.org changed reviewers: + msarda@chromium.org
Mihai, Network traffic annotation added as input argument of GaiaAuthFetcher::CreateAndStartGaiaFetcher. Please review, and suggest annotation for use cases.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have done a pass. Note that I have a generic question on how we want to annotate the trigger for all these fetchers. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:516: void GaiaAuthFetcher::StartIssueAuthToken(const std::string& sid, I think this method is no longer used. I think it is fine for us to remove it instead of annotating it. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:527: sender: "Gaia Auth API" Here and everywhere else: Chrome - Google authentication API. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:560: description: "..." This request revokes an OAuth2 refresh token. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:561: trigger: "..." Here and everywhere else: Same as the comment below at StartGetCheckConnectionInfo https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:562: data: "..." The OAuth2 refresh token that should be revoked. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:566: cookies_allowed: false/true false https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:567: cookies_store: "..." Remove. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:626: net::DefineNetworkTrafficAnnotation("gaia_auth_", R"( What is "gaia_auth_" used for? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:629: description: "..." This request exchanges exchange the cookies of a Google signed-in user session for an OAuth2 refresh token. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:631: data: "..." This request includes the following data: the Google console client ID of the Chrome application, the ID of the device, the index of the session in the Google authentication cookies. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:635: cookies_allowed: false/true true https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:668: description: "..." This request exchanges an authorization code for an Oauth 2.0 refresh token. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:670: data: "..." This request includes the following data: * the Google console client ID and client secret of the Chrome application * the OAuth 2.0 authorization code * the ID of the device https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:674: cookies_allowed: false/true false https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:699: description: "..." This request fetches user information of a Google account. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:700: trigger: "..." This fetcher is only used after signing in with a child account. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:701: data: "..." The value of the Google authentication LSID cookie. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:705: cookies_allowed: false/true false https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:706: cookies_store: "..." ? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:742: description: "..." This request adds an account to the Google authentication cookies. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:744: data: "..." This request includes the following data: * the user-auth token * a string containing the result of connection checks for various Google web properties (optional) https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:748: cookies_allowed: false/true true https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:749: cookies_store: "..." ? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:779: sender: "Gaia Auth API" Here and everywhere else: Chrome - Google authentication API. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:780: description: "..." This request exchanges an Oauth2 access token for an uber-auth token that. This token may be used to add an account to the Google authentication cookies. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:781: trigger: "..." Same as the comment below at StartGetCheckConnectionInfo https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:782: data: "..." This request contains an OAuth2 access token. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:786: cookies_allowed: false/true true or false https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:787: cookies_store: "..." What should this be? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:813: sender: "Gaia Auth API" Chrome - Google authentication API. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:814: description: "..." This request exchanges an OAuthLogin-scoped oauth2 access token for a ClientLogin-style service tokens. The response to this request is the same as the response to a ClientLogin request, except that captcha challenges are never issued. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:815: trigger: "..." Same as the comment below at StartGetCheckConnectionInfo https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:816: data: "..." This request contains an OAuth2 access token and the service for which a ClientLogin-style should be delivered. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:821: cookies_store: "..." Here and everywhere else: What should this be? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:843: sender: "Gaia Auth API" Chrome - Google authentication API. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:844: description: "..." This request is used to list the accounts in the Google authentication cookies. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:845: trigger: "..." Same as the comment below at StartGetCheckConnectionInfo https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:846: data: "..." None. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:850: cookies_allowed: false/true true https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:851: cookies_store: "..." What should this be? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:874: sender: "Gaia Auth API" Chrome - Google authentication API. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:875: description: "..." This request is part of the Chrome - Google authentication API and allows its callers to sign out all Google accounts from the content area. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:876: trigger: "..." Same as the comment below at StartGetCheckConnectionInfo https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:877: data: "..." None. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:881: cookies_allowed: false/true true https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:882: cookies_store: "..." What should this be? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:903: sender: "Gaia Auth API" Chrome - Google authentication API. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:904: description: "..." This request is used to fetch from the Google authentication server the the list of URLs to check its connection info. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:905: trigger: "..." It is not clear to me what to say here: 1. This is part of an API, so it may be called by any user, so we may leave this as "Undefined - this is part of an API, so any caller may decide to call this method." 2. Today, this method is called from https://cs.chromium.org/chromium/src/components/signin/core/browser/gaia_cook... So it is not clear to me if we want this to be specific about this call. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:906: data: "..." None https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:910: cookies_allowed: false/true false https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:911: cookies_store: "..." Remove line https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:927: void GaiaAuthFetcher::StartListIDPSessions(const std::string& scopes, It looks like this method is never used - I would say we should probably remove it instead of annotating it: https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:959: void GaiaAuthFetcher::StartGetTokenResponse(const std::string& scopes, It looks like this method is never used - I would say we should probably remove it instead of annotating it: https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ...
Thank you very much Mihai, all comments addressed, [lease review. Generally, it's good if we elaborate more on descriptions for a non-technical reader, also please suggest completions for triggers that I could not completed. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:516: void GaiaAuthFetcher::StartIssueAuthToken(const std::string& sid, On 2017/05/15 12:23:49, msarda wrote: > I think this method is no longer used. I think it is fine for us to remove it > instead of annotating it. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:527: sender: "Gaia Auth API" On 2017/05/15 12:23:49, msarda wrote: > Here and everywhere else: Chrome - Google authentication API. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:560: description: "..." On 2017/05/15 12:23:50, msarda wrote: > This request revokes an OAuth2 refresh token. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:561: trigger: "..." On 2017/05/15 12:23:48, msarda wrote: > Here and everywhere else: Same as the comment below at > StartGetCheckConnectionInfo Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:562: data: "..." On 2017/05/15 12:23:48, msarda wrote: > The OAuth2 refresh token that should be revoked. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:566: cookies_allowed: false/true On 2017/05/15 12:23:50, msarda wrote: > false Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:567: cookies_store: "..." On 2017/05/15 12:23:48, msarda wrote: > Remove. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:626: net::DefineNetworkTrafficAnnotation("gaia_auth_", R"( On 2017/05/15 12:23:49, msarda wrote: > What is "gaia_auth_" used for? It's the unique_id of this request, it can be used to collect stats on different requests, debug network errors, etc. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:629: description: "..." On 2017/05/15 12:23:50, msarda wrote: > This request exchanges exchange the cookies of a Google signed-in user session > for an OAuth2 refresh token. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:631: data: "..." On 2017/05/15 12:23:50, msarda wrote: > This request includes the following data: the Google console client ID of the > Chrome application, the ID of the device, the index of the session in the Google > authentication cookies. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:635: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > true Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:668: description: "..." On 2017/05/15 12:23:49, msarda wrote: > This request exchanges an authorization code for an Oauth 2.0 refresh token. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:670: data: "..." On 2017/05/15 12:23:50, msarda wrote: > This request includes the following data: > * the Google console client ID and client secret of the Chrome application > * the OAuth 2.0 authorization code > * the ID of the device Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:674: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > false Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:699: description: "..." On 2017/05/15 12:23:50, msarda wrote: > This request fetches user information of a Google account. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:700: trigger: "..." On 2017/05/15 12:23:50, msarda wrote: > This fetcher is only used after signing in with a child account. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:701: data: "..." On 2017/05/15 12:23:48, msarda wrote: > The value of the Google authentication LSID cookie. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:705: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > false Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:706: cookies_store: "..." On 2017/05/15 12:23:50, msarda wrote: > ? Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:742: description: "..." On 2017/05/15 12:23:49, msarda wrote: > This request adds an account to the Google authentication cookies. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:744: data: "..." On 2017/05/15 12:23:50, msarda wrote: > This request includes the following data: > * the user-auth token > * a string containing the result of connection checks for various Google web > properties (optional) Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:748: cookies_allowed: false/true On 2017/05/15 12:23:48, msarda wrote: > true Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:749: cookies_store: "..." On 2017/05/15 12:23:48, msarda wrote: > ? Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:779: sender: "Gaia Auth API" On 2017/05/15 12:23:48, msarda wrote: > Here and everywhere else: Chrome - Google authentication API. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:780: description: "..." On 2017/05/15 12:23:49, msarda wrote: > This request exchanges an Oauth2 access token for an uber-auth token that. This > token may be used to add an account to the Google authentication cookies. Please elaborate more for a non-technical reader. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:781: trigger: "..." On 2017/05/15 12:23:49, msarda wrote: > Same as the comment below at StartGetCheckConnectionInfo Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:782: data: "..." On 2017/05/15 12:23:49, msarda wrote: > This request contains an OAuth2 access token. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:786: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > true or false Done, let's write 'True' is it's the privacy concerning case. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:787: cookies_store: "..." On 2017/05/15 12:23:49, msarda wrote: > What should this be? Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:813: sender: "Gaia Auth API" On 2017/05/15 12:23:48, msarda wrote: > Chrome - Google authentication API. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:814: description: "..." On 2017/05/15 12:23:50, msarda wrote: > This request exchanges an OAuthLogin-scoped oauth2 access token for a > ClientLogin-style service tokens. The response to this request is the same as > the response to a ClientLogin request, except that captcha challenges are never > issued. Please elaborate for a less technical reader. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:815: trigger: "..." On 2017/05/15 12:23:50, msarda wrote: > Same as the comment below at StartGetCheckConnectionInfo Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:816: data: "..." On 2017/05/15 12:23:48, msarda wrote: > This request contains an OAuth2 access token and the service for which a > ClientLogin-style should be delivered. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:821: cookies_store: "..." On 2017/05/15 12:23:50, msarda wrote: > Here and everywhere else: What should this be? Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:843: sender: "Gaia Auth API" On 2017/05/15 12:23:48, msarda wrote: > Chrome - Google authentication API. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:844: description: "..." On 2017/05/15 12:23:48, msarda wrote: > This request is used to list the accounts in the Google authentication cookies. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:845: trigger: "..." On 2017/05/15 12:23:49, msarda wrote: > Same as the comment below at StartGetCheckConnectionInfo Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:846: data: "..." On 2017/05/15 12:23:50, msarda wrote: > None. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:850: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > true Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:851: cookies_store: "..." On 2017/05/15 12:23:50, msarda wrote: > What should this be? Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:874: sender: "Gaia Auth API" On 2017/05/15 12:23:50, msarda wrote: > Chrome - Google authentication API. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:875: description: "..." On 2017/05/15 12:23:49, msarda wrote: > This request is part of the Chrome - Google authentication API and allows its > callers to sign out all Google accounts from the content area. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:876: trigger: "..." On 2017/05/15 12:23:48, msarda wrote: > Same as the comment below at StartGetCheckConnectionInfo Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:877: data: "..." On 2017/05/15 12:23:50, msarda wrote: > None. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:881: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > true Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:882: cookies_store: "..." On 2017/05/15 12:23:50, msarda wrote: > What should this be? If you don't modify it, the default is 'user'. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:903: sender: "Gaia Auth API" On 2017/05/15 12:23:48, msarda wrote: > Chrome - Google authentication API. Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:904: description: "..." On 2017/05/15 12:23:49, msarda wrote: > This request is used to fetch from the Google authentication server the the list > of URLs to check its connection info. Could you please elaborate on the description? https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:905: trigger: "..." On 2017/05/15 12:23:48, msarda wrote: > It is not clear to me what to say here: > 1. This is part of an API, so it may be called by any user, so we may leave this > as "Undefined - this is part of an API, so any caller may decide to call this > method." > 2. Today, this method is called from > https://cs.chromium.org/chromium/src/components/signin/core/browser/gaia_cook... > > So it is not clear to me if we want this to be specific about this call. I didn't get the description clearly, let's continue with trigger when that's done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:906: data: "..." On 2017/05/15 12:23:50, msarda wrote: > None Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:910: cookies_allowed: false/true On 2017/05/15 12:23:49, msarda wrote: > false Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:911: cookies_store: "..." On 2017/05/15 12:23:50, msarda wrote: > Remove line Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:927: void GaiaAuthFetcher::StartListIDPSessions(const std::string& scopes, On 2017/05/15 12:23:49, msarda wrote: > It looks like this method is never used - I would say we should probably remove > it instead of annotating it: > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... Done. https://codereview.chromium.org/2872253002/diff/60001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:959: void GaiaAuthFetcher::StartGetTokenResponse(const std::string& scopes, On 2017/05/15 12:23:50, msarda wrote: > It looks like this method is never used - I would say we should probably remove > it instead of annotating it: > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... Done.
LGTM with some suggestions. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:528: "This request is part of Gaia Auth API, and is triggered whenever " an OAuth 2.0 refresh token needs to be revoked. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:601: "..." and may be triggered at the end of the Chrome sign-in flow. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:646: "This request is part of Gaia Auth API, and is triggered whenever " and may be triggered at the end of the Chrome sign-in flow. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:773: "..." a new Google account is added to the browser. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:813: "..." after signing in with a child account. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:846: "authentication cookies.." Remove "." (no need for 2 ".." here). https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:849: "the list of all available Google accounts on the browser are " all available accounts in the Google authentication cookies are required. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:919: "..." Remove "whenever" and add "once after a Google account is added to the browser."
Patchset #4 (id:100001) has been deleted
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you very much Mihai. Martin, Any comments? https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:528: "This request is part of Gaia Auth API, and is triggered whenever " On 2017/05/22 09:43:07, msarda wrote: > an OAuth 2.0 refresh token needs to be revoked. Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:601: "..." On 2017/05/22 09:43:07, msarda wrote: > and may be triggered at the end of the Chrome sign-in flow. Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:646: "This request is part of Gaia Auth API, and is triggered whenever " On 2017/05/22 09:43:07, msarda wrote: > and may be triggered at the end of the Chrome sign-in flow. Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:773: "..." On 2017/05/22 09:43:07, msarda wrote: > a new Google account is added to the browser. Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:813: "..." On 2017/05/22 09:43:07, msarda wrote: > after signing in with a child account. Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:846: "authentication cookies.." On 2017/05/22 09:43:07, msarda wrote: > Remove "." (no need for 2 ".." here). Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:849: "the list of all available Google accounts on the browser are " On 2017/05/22 09:43:07, msarda wrote: > all available accounts in the Google authentication cookies are required. Done. https://codereview.chromium.org/2872253002/diff/80001/google_apis/gaia/gaia_a... google_apis/gaia/gaia_auth_fetcher.cc:919: "..." On 2017/05/22 09:43:07, msarda wrote: > Remove "whenever" and add "once after a Google account is added to the browser." Done.
https://codereview.chromium.org/2872253002/diff/120001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:529: "an OAuth 2.0 refresh token needs to be revoked." I just noticed this - we're using both "OAuth2" and "OAuth 2.0". I think we need to be consistent, so please change it to OAuth 2.0 everywhere in this file (this is the name on the OAuth site https://oauth.net/2/ ) https://codereview.chromium.org/2872253002/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:643: "This request exchanges an authorization code for an Oauth 2.0 " s/Oauth/OAuth
Thank you Mihai, comments addressed. https://codereview.chromium.org/2872253002/diff/120001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:529: "an OAuth 2.0 refresh token needs to be revoked." On 2017/05/22 11:20:02, msarda wrote: > I just noticed this - we're using both "OAuth2" and "OAuth 2.0". I think we need > to be consistent, so please change it to OAuth 2.0 everywhere in this file (this > is the name on the OAuth site https://oauth.net/2/ ) Done. https://codereview.chromium.org/2872253002/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:643: "This request exchanges an authorization code for an Oauth 2.0 " On 2017/05/22 11:20:02, msarda wrote: > s/Oauth/OAuth Done.
LGTM with one description improvement request. Thanks! https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:536: "This feature cannot be disabled in settings, but if user signs " nit: Not a native speaker, but should this be "the user"? (here and elsewhere) https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:808: "for a ClientLogin-style service tokens. The response to this " What is ClientLogin? Does this refer to the ClientLogin API? This page https://developers.google.com/identity/protocols/AuthForInstalledApps states that it's long deprecated, so I'm not sure if it makes sense to describe our OAuth2 login process in its terms.
Description was changed from ========== Network traffic annotation added to gaia_auth_fetcher. Network traffic annotation is added to network request of: google_apis/gaia/gaia_auth_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to gaia_auth_fetcher. Network traffic annotation is added to network request of: google_apis/gaia/gaia_auth_fetcher.h google_apis/gaia/gaia_auth_fetcher.cc google_apis/gaia/gaia_auth_fetcher_unittest.cc ios/chrome/browser/signin/gaia_auth_fetcher_ios.h ios/chrome/browser/signin/gaia_auth_fetcher_ios.mm BUG=656607 ==========
Thank you Martin, comment addressed. Mihai, There is one last question for you. https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:536: "This feature cannot be disabled in settings, but if user signs " On 2017/05/26 12:24:52, msramek wrote: > nit: Not a native speaker, but should this be "the user"? (here and elsewhere) Done.
https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:808: "for a ClientLogin-style service tokens. The response to this " On 2017/05/26 12:24:52, msramek wrote: > What is ClientLogin? Does this refer to the ClientLogin API? > > This page https://developers.google.com/identity/protocols/AuthForInstalledApps > states that it's long deprecated, so I'm not sure if it makes sense to describe > our OAuth2 login process in its terms. This is not part of an OAuth 2.0 process - it exchanges an OAuth 2.0 access token for a ClientLogin token. Apparently the Chrome child account code still uses ClientLogin here https://cs.chromium.org/chromium/src/components/signin/core/browser/child_acc... Martin: Do you have any specific suggestion on how to phrase this differently (I think the current description is accurate)? https://codereview.chromium.org/2872253002/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:807: "This request exchanges an OAuthLogin-scoped oauth2 access token " s/oauth2/Oauth 2.0
LGTM (but note that there is still one open comment from Mihai). https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:808: "for a ClientLogin-style service tokens. The response to this " On 2017/05/29 11:34:13, msarda wrote: > On 2017/05/26 12:24:52, msramek wrote: > > What is ClientLogin? Does this refer to the ClientLogin API? > > > > This page > https://developers.google.com/identity/protocols/AuthForInstalledApps > > states that it's long deprecated, so I'm not sure if it makes sense to > describe > > our OAuth2 login process in its terms. > > This is not part of an OAuth 2.0 process - it exchanges an OAuth 2.0 access > token for a ClientLogin token. Apparently the Chrome child account code still > uses ClientLogin here > https://cs.chromium.org/chromium/src/components/signin/core/browser/child_acc... > > Martin: Do you have any specific suggestion on how to phrase this differently (I > think the current description is accurate)? > > Ah, I guess I misunderstood originally. If we're still using ClientLogin, then I agree that no change is needed. Thanks!
Thank you, all comments addressed, landing. https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/140001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:808: "for a ClientLogin-style service tokens. The response to this " On 2017/05/29 13:39:31, msramek wrote: > On 2017/05/29 11:34:13, msarda wrote: > > On 2017/05/26 12:24:52, msramek wrote: > > > What is ClientLogin? Does this refer to the ClientLogin API? > > > > > > This page > > https://developers.google.com/identity/protocols/AuthForInstalledApps > > > states that it's long deprecated, so I'm not sure if it makes sense to > > describe > > > our OAuth2 login process in its terms. > > > > This is not part of an OAuth 2.0 process - it exchanges an OAuth 2.0 access > > token for a ClientLogin token. Apparently the Chrome child account code still > > uses ClientLogin here > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/child_acc... > > > > Martin: Do you have any specific suggestion on how to phrase this differently > (I > > think the current description is accurate)? > > > > > > Ah, I guess I misunderstood originally. If we're still using ClientLogin, then I > agree that no change is needed. Thanks! Acknowledged. https://codereview.chromium.org/2872253002/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2872253002/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:807: "This request exchanges an OAuthLogin-scoped oauth2 access token " On 2017/05/29 11:34:13, msarda wrote: > s/oauth2/Oauth 2.0 Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2872253002/#ps180001 (title: "Comments addressed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496065978113650,
"parent_rev": "88c8153ffb73869b07a17c3d804630e401c730fd", "commit_rev":
"0d460449a6b80930bbddb2c54ca0340c8ea4bf7b"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to gaia_auth_fetcher. Network traffic annotation is added to network request of: google_apis/gaia/gaia_auth_fetcher.h google_apis/gaia/gaia_auth_fetcher.cc google_apis/gaia/gaia_auth_fetcher_unittest.cc ios/chrome/browser/signin/gaia_auth_fetcher_ios.h ios/chrome/browser/signin/gaia_auth_fetcher_ios.mm BUG=656607 ========== to ========== Network traffic annotation added to gaia_auth_fetcher. Network traffic annotation is added to network request of: google_apis/gaia/gaia_auth_fetcher.h google_apis/gaia/gaia_auth_fetcher.cc google_apis/gaia/gaia_auth_fetcher_unittest.cc ios/chrome/browser/signin/gaia_auth_fetcher_ios.h ios/chrome/browser/signin/gaia_auth_fetcher_ios.mm BUG=656607 Review-Url: https://codereview.chromium.org/2872253002 Cr-Commit-Position: refs/heads/master@{#475351} Committed: https://chromium.googlesource.com/chromium/src/+/0d460449a6b80930bbddb2c54ca0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/0d460449a6b80930bbddb2c54ca0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
