Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796293003:
Network traffic annotation added to google_apis/gaia. (Closed)
|
Created:
3 years, 8 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 google_apis/gaia.
Network traffic annotation is added to network requests of
google_apis/gaia/gaia_oauth_client.cc
google_apis/gaia/oauth2_access_token_fetcher_impl.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2796293003
Cr-Commit-Position: refs/heads/master@{#476214}
Committed: https://chromium.googlesource.com/chromium/src/+/147280e2989d9a8f499e53135d405003488cd88c
Patch Set 1 #
Total comments: 40
Patch Set 2 : Annotations updated. #Patch Set 3 : gaia_auth_fetcher removed. #Patch Set 4 : Annotation moved to input argument. #
Total comments: 1
Patch Set 5 : Annotation moved to caller. #Patch Set 6 : Moved oauth2_api_call_flow.* to CL 2888053003 #
Total comments: 43
Patch Set 7 : Annotations updated. #
Total comments: 4
Patch Set 8 : Comments addressed. #Patch Set 9 : Leak reverted! #
Total comments: 9
Patch Set 10 : Comments addressed. #Patch Set 11 : Annotation updated. #Patch Set 12 : Annotation updated. #
Messages
Total messages: 44 (13 generated)
rhalavati@chromium.org changed reviewers: + msarda@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified 4 files related to gaia and added annotation templates to them. Please review them and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
On 2017/04/05 10:56:57, Ramin Halavati wrote: > We are annotating all network requests in Chromium with a new > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit > the requests and configure Chrome in a way that meets their security policies > and expectations. Furthermore, it allows us to generate better debugging data in > chrome://net-internals and measure bandwidth consumption on a per-request-type > basis. > > I've modified 4 files related to gaia and added annotation templates to them. > Please review them and suggest the required answers for the empty parts (marked > with '...'). Please note that the claims should be thorough and covering all > cases. In case you believe that annotation should be passed to the modified > function instead of originating from it, please tell me to change the CL > accordingly. > > Please take a look at the protobuf scheme in: > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > for the definition of the annotations. > > You can find a sample annotation in: > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > Entriprise policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > And more description on enterprise policy settings is here: > http://dev.chromium.org/administrators/policy-list-3 > > Please tell me if you need any clarifications from my side. If you want to learn > more, see: > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. Hi, A friendly reminder on this.
On 2017/04/11 06:44:20, Ramin Halavati wrote: > On 2017/04/05 10:56:57, Ramin Halavati wrote: > > We are annotating all network requests in Chromium with a new > > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to > audit > > the requests and configure Chrome in a way that meets their security policies > > and expectations. Furthermore, it allows us to generate better debugging data > in > > chrome://net-internals and measure bandwidth consumption on a per-request-type > > basis. > > > > I've modified 4 files related to gaia and added annotation templates to them. > > Please review them and suggest the required answers for the empty parts > (marked > > with '...'). Please note that the claims should be thorough and covering all > > cases. In case you believe that annotation should be passed to the modified > > function instead of originating from it, please tell me to change the CL > > accordingly. > > > > Please take a look at the protobuf scheme in: > > > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > > for the definition of the annotations. > > > > You can find a sample annotation in: > > > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > > > Entriprise policy options are here: > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > > > And more description on enterprise policy settings is here: > > http://dev.chromium.org/administrators/policy-list-3 > > > > Please tell me if you need any clarifications from my side. If you want to > learn > > more, see: > > > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. > > Hi, > > A friendly reminder on this. Hi Mihai, A gentle reminder on this.
On 2017/04/18 11:33:03, Ramin Halavati wrote: > On 2017/04/11 06:44:20, Ramin Halavati wrote: > > On 2017/04/05 10:56:57, Ramin Halavati wrote: > > > We are annotating all network requests in Chromium with a new > > > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to > > audit > > > the requests and configure Chrome in a way that meets their security > policies > > > and expectations. Furthermore, it allows us to generate better debugging > data > > in > > > chrome://net-internals and measure bandwidth consumption on a > per-request-type > > > basis. > > > > > > I've modified 4 files related to gaia and added annotation templates to > them. > > > Please review them and suggest the required answers for the empty parts > > (marked > > > with '...'). Please note that the claims should be thorough and covering all > > > cases. In case you believe that annotation should be passed to the modified > > > function instead of originating from it, please tell me to change the CL > > > accordingly. > > > > > > Please take a look at the protobuf scheme in: > > > > > > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > > > for the definition of the annotations. > > > > > > You can find a sample annotation in: > > > > > > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > > > > > Entriprise policy options are here: > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > > > > > And more description on enterprise policy settings is here: > > > http://dev.chromium.org/administrators/policy-list-3 > > > > > > Please tell me if you need any clarifications from my side. If you want to > > learn > > > more, see: > > > > > > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. > > > > Hi, > > > > A friendly reminder on this. > > Hi Mihai, > > A gentle reminder on this. I am OOO for a this whole period. Taking a look now.
msarda@chromium.org changed reviewers: + rogerta@chromium.org
+Roger for advice on one of the fetchers. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... google_apis/gaia/gaia_auth_fetcher.cc:228: net::NetworkTrafficAnnotationTag traffic_annotation = This is a helper method that creates a fetcher that interacts with GAIA (as you can see the body, the headers, the urls and the load flags are configurable). It is not clear to me if this needs to be annotated here with somethign generic or if it should be an argument passed to this function. As you can see here https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... ,this method is protected, and is called from 13 different places in this file. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:183: sender: "..." OAuth2 client. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:184: description: "..." This request is used to fetch user information. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:185: trigger: "..." The main trigger for this request in the AccountTrackerService that fetches the user info soon after the user signs in. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:186: data: "..." The request is authorized, so the header of this request contains the OAUth2 access token of the account. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:187: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:191: setting: "..." "This feature cannot be disabled in settings." https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:192: chrome_policy { Remove block https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:198: policy_exception_justification: "..." "Not implemented. Disabling this fetcher would break features" "that require user information about of the account that is signed in" "(e.g. the profile switcher UI, the settings UI etc)". https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:103: sender: "..." OAuth2 Access Token Fetcher https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:104: description: "..." This request is used by the Token Service to fetch an OAuth2 access token for a known Google account. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:105: trigger: "..." This request can be triggered at any moment when any service requests an OAuth2 access token from the Token Service. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:106: data: "..." The body of the request includes the following information: Chrome OAuth2 client id and secret, the set of OAuth2 scopes and the OAuth2 refresh token. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:107: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:111: setting: "..." This feature cannot be disabled in settings. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:112: chrome_policy { Remove block. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:118: policy_exception_justification: "..." "Not implemented. Disabling Oauth2 Access Token Fetcher would break " "features need access tokens (e.g. sync, chrome identity API etc)." https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_api... File google_apis/gaia/oauth2_api_call_flow.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_api... google_apis/gaia/oauth2_api_call_flow.cc:83: net::NetworkTrafficAnnotationTag traffic_annotation = I do not know what this fetcher is supposed to do. Roger: May I ask you to maybe provide a description for this?
Thank you Mihai, comments addressed, please review. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... google_apis/gaia/gaia_auth_fetcher.cc:228: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/03 09:03:53, msarda wrote: > This is a helper method that creates a fetcher that interacts with GAIA (as you > can see the body, the headers, the urls and the load flags are configurable). It > is not clear to me if this needs to be annotated here with somethign generic or > if it should be an argument passed to this function. > > As you can see here > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... > ,this method is protected, and is called from 13 different places in this file. OK, do you that suggest I add annotation as an input to CreateAndStartGaiaFetcher function, or constructor of GaiaAuthFetcher? https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:183: sender: "..." On 2017/05/03 09:03:53, msarda wrote: > OAuth2 client. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:184: description: "..." On 2017/05/03 09:03:53, msarda wrote: > This request is used to fetch user information. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:185: trigger: "..." On 2017/05/03 09:03:53, msarda wrote: > The main trigger for this request in the AccountTrackerService that fetches the > user info soon after the user signs in. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:186: data: "..." On 2017/05/03 09:03:53, msarda wrote: > The request is authorized, so the header of this request contains the OAUth2 > access token of the account. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:187: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/05/03 09:03:53, msarda wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:191: setting: "..." On 2017/05/03 09:03:53, msarda wrote: > "This feature cannot be disabled in settings." Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:192: chrome_policy { On 2017/05/03 09:03:53, msarda wrote: > Remove block Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:198: policy_exception_justification: "..." On 2017/05/03 09:03:53, msarda wrote: > "Not implemented. Disabling this fetcher would break features" > "that require user information about of the account that is signed in" > "(e.g. the profile switcher UI, the settings UI etc)". Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... google_apis/gaia/gaia_oauth_client.cc:242: net::DefineNetworkTrafficAnnotation("...", R"( Should I use the same annotation for this function or it is different? https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:103: sender: "..." On 2017/05/03 09:03:53, msarda wrote: > OAuth2 Access Token Fetcher Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:104: description: "..." On 2017/05/03 09:03:53, msarda wrote: > This request is used by the Token Service to fetch an OAuth2 access token for a > known Google account. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:105: trigger: "..." On 2017/05/03 09:03:53, msarda wrote: > This request can be triggered at any moment when any service requests an OAuth2 > access token from the Token Service. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:106: data: "..." On 2017/05/03 09:03:54, msarda wrote: > The body of the request includes the following information: Chrome OAuth2 client > id and secret, the set of OAuth2 scopes and the OAuth2 refresh token. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:107: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/05/03 09:03:53, msarda wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:111: setting: "..." On 2017/05/03 09:03:53, msarda wrote: > This feature cannot be disabled in settings. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:112: chrome_policy { On 2017/05/03 09:03:54, msarda wrote: > Remove block. Done. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:118: policy_exception_justification: "..." On 2017/05/03 09:03:54, msarda wrote: > "Not implemented. Disabling Oauth2 Access Token Fetcher would break " > "features need access tokens (e.g. sync, chrome identity API etc)." Done.
On 2017/05/03 09:45:07, Ramin Halavati wrote: > Thank you Mihai, comments addressed, please review. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... > File google_apis/gaia/gaia_auth_fetcher.cc (right): > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... > google_apis/gaia/gaia_auth_fetcher.cc:228: net::NetworkTrafficAnnotationTag > traffic_annotation = > On 2017/05/03 09:03:53, msarda wrote: > > This is a helper method that creates a fetcher that interacts with GAIA (as > you > > can see the body, the headers, the urls and the load flags are configurable). > It > > is not clear to me if this needs to be annotated here with somethign generic > or > > if it should be an argument passed to this function. > > > > As you can see here > > > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... > > ,this method is protected, and is called from 13 different places in this > file. > > OK, do you that suggest I add annotation as an input to > CreateAndStartGaiaFetcher function, or constructor of GaiaAuthFetcher? > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > File google_apis/gaia/gaia_oauth_client.cc (right): > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:183: sender: "..." > On 2017/05/03 09:03:53, msarda wrote: > > OAuth2 client. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:184: description: "..." > On 2017/05/03 09:03:53, msarda wrote: > > This request is used to fetch user information. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:185: trigger: "..." > On 2017/05/03 09:03:53, msarda wrote: > > The main trigger for this request in the AccountTrackerService that fetches > the > > user info soon after the user signs in. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:186: data: "..." > On 2017/05/03 09:03:53, msarda wrote: > > The request is authorized, so the header of this request contains the OAUth2 > > access token of the account. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:187: destination: > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > On 2017/05/03 09:03:53, msarda wrote: > > GOOGLE_OWNED_SERVICE > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:191: setting: "..." > On 2017/05/03 09:03:53, msarda wrote: > > "This feature cannot be disabled in settings." > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:192: chrome_policy { > On 2017/05/03 09:03:53, msarda wrote: > > Remove block > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:198: policy_exception_justification: "..." > On 2017/05/03 09:03:53, msarda wrote: > > "Not implemented. Disabling this fetcher would break features" > > "that require user information about of the account that is signed in" > > "(e.g. the profile switcher UI, the settings UI etc)". > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_oauth... > google_apis/gaia/gaia_oauth_client.cc:242: > net::DefineNetworkTrafficAnnotation("...", R"( > Should I use the same annotation for this function or it is different? > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right): > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:103: sender: "..." > On 2017/05/03 09:03:53, msarda wrote: > > OAuth2 Access Token Fetcher > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:104: description: "..." > On 2017/05/03 09:03:53, msarda wrote: > > This request is used by the Token Service to fetch an OAuth2 access token for > a > > known Google account. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:105: trigger: "..." > On 2017/05/03 09:03:53, msarda wrote: > > This request can be triggered at any moment when any service requests an > OAuth2 > > access token from the Token Service. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:106: data: "..." > On 2017/05/03 09:03:54, msarda wrote: > > The body of the request includes the following information: Chrome OAuth2 > client > > id and secret, the set of OAuth2 scopes and the OAuth2 refresh token. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:107: destination: > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > On 2017/05/03 09:03:53, msarda wrote: > > GOOGLE_OWNED_SERVICE > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:111: setting: "..." > On 2017/05/03 09:03:53, msarda wrote: > > This feature cannot be disabled in settings. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:112: chrome_policy { > On 2017/05/03 09:03:54, msarda wrote: > > Remove block. > > Done. > > https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_acc... > google_apis/gaia/oauth2_access_token_fetcher_impl.cc:118: > policy_exception_justification: "..." > On 2017/05/03 09:03:54, msarda wrote: > > "Not implemented. Disabling Oauth2 Access Token Fetcher would break " > > "features need access tokens (e.g. sync, chrome identity API etc)." > > Done. Hi Mihai, A gentle reminder on this (there is also a question on the other gaia CL: https://codereview.chromium.org/2797173003). :)
https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... google_apis/gaia/gaia_auth_fetcher.cc:228: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/03 09:45:06, Ramin Halavati wrote: > On 2017/05/03 09:03:53, msarda wrote: > > This is a helper method that creates a fetcher that interacts with GAIA (as > you > > can see the body, the headers, the urls and the load flags are configurable). > It > > is not clear to me if this needs to be annotated here with somethign generic > or > > if it should be an argument passed to this function. > > > > As you can see here > > > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... > > ,this method is protected, and is called from 13 different places in this > file. > > OK, do you that suggest I add annotation as an input to > CreateAndStartGaiaFetcher function, or constructor of GaiaAuthFetcher? If we need to annotate each call differently, then it must be passed in the method. However, it is not clear to me in what detail we need to go for each of these fetchers (can we aggregate the ones that are part of the same flow even though they fetch different things?).
https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/gaia_auth_... google_apis/gaia/gaia_auth_fetcher.cc:228: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/10 09:01:54, msarda wrote: > On 2017/05/03 09:45:06, Ramin Halavati wrote: > > On 2017/05/03 09:03:53, msarda wrote: > > > This is a helper method that creates a fetcher that interacts with GAIA (as > > you > > > can see the body, the headers, the urls and the load flags are > configurable). > > It > > > is not clear to me if this needs to be annotated here with somethign generic > > or > > > if it should be an argument passed to this function. > > > > > > As you can see here > > > > > > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.h?typ... > > > ,this method is protected, and is called from 13 different places in this > > file. > > > > OK, do you that suggest I add annotation as an input to > > CreateAndStartGaiaFetcher function, or constructor of GaiaAuthFetcher? > > If we need to annotate each call differently, then it must be passed in the > method. > > However, it is not clear to me in what detail we need to go for each of these > fetchers (can we aggregate the ones that are part of the same flow even though > they fetch different things?). Can we can have a generic one here? If we decide to separate them in future, we can use (the not landed yet) new annotation definition functions which let part of annotation be defined in another function and part of it here.
Thank you Mihai for clarifiacations. Moved google_apis/gaia/gaia_auth_fetcher.cc to another CL. Please review.
Description was changed from ========== Network traffic annotation added to google_apis/gaia. Network traffic annotation is added to network requests of google_apis/gaia/gaia_auth_fetcher.cc google_apis/gaia/gaia_oauth_client.cc google_apis/gaia/oauth2_access_token_fetcher_impl.cc google_apis/gaia/oauth2_api_call_flow.cc BUG=656607 ========== to ========== Network traffic annotation added to google_apis/gaia. Network traffic annotation is added to network requests of google_apis/gaia/gaia_oauth_client.cc google_apis/gaia/oauth2_access_token_fetcher_impl.cc google_apis/gaia/oauth2_api_call_flow.cc BUG=656607 ==========
Sorry for delay. See comment below. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_api... File google_apis/gaia/oauth2_api_call_flow.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_api... google_apis/gaia/oauth2_api_call_flow.cc:83: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/03 09:03:54, msarda wrote: > I do not know what this fetcher is supposed to do. > > Roger: May I ask you to maybe provide a description for this? OAuth2ApiCallFlow helps write code to call google apis. Users of this class derive from it, then provide implementations of the CreateApiCallXXX and GetRequestXXX methods for the particular google api that needs to be called. This method creates the URL fetcher based on the return value of these implementations. All this to say that I don't think this function knows the semantics of the call being made. It seems like OAuth2ApiCallFlow should have a virtual function like: virtual net::NetworkTrafficAnnotationTag GetNetworkAnnotation() = 0; that derived classes need to also implement. Does not seem like there are lots of derived classes: https://cs.chromium.org/search/?q=%22public+OAuth2ApiCallFlow%22&type=cs
Thank you Roger, please review. https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_api... File google_apis/gaia/oauth2_api_call_flow.cc (right): https://codereview.chromium.org/2796293003/diff/1/google_apis/gaia/oauth2_api... google_apis/gaia/oauth2_api_call_flow.cc:83: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/15 15:14:18, Roger Tawa wrote: > On 2017/05/03 09:03:54, msarda wrote: > > I do not know what this fetcher is supposed to do. > > > > Roger: May I ask you to maybe provide a description for this? > > OAuth2ApiCallFlow helps write code to call google apis. Users of this class > derive from it, then provide implementations of the CreateApiCallXXX and > GetRequestXXX methods for the particular google api that needs to be called. > This method creates the URL fetcher based on the return value of these > implementations. > > All this to say that I don't think this function knows the semantics of the call > being made. It seems like OAuth2ApiCallFlow should have a virtual function > like: > > virtual net::NetworkTrafficAnnotationTag GetNetworkAnnotation() = 0; > > that derived classes need to also implement. Does not seem like there are lots > of derived classes: > > https://cs.chromium.org/search/?q=%22public+OAuth2ApiCallFlow%22&type=cs What about adding NetworkTrafficAnnotation as an input argument to this function? If it seems good to you, I will move this class from this CL to a new one and add the 3 other classes that would be affected by this change to the new CL.
https://codereview.chromium.org/2796293003/diff/60001/google_apis/gaia/gaia_o... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/60001/google_apis/gaia/gaia_o... google_apis/gaia/gaia_oauth_client.cc:241: net::NetworkTrafficAnnotationTag traffic_annotation = This is again one of these cases when this is a generic method that creates a fetcher that does not have any idea of the semantics of the data that is contains. I think it would make sense to pass a net::NetworkTrafficAnnotationTag as an argument to this function.
Thank you Mihai, annotation moved to caller functions. Please review.
Description was changed from ========== Network traffic annotation added to google_apis/gaia. Network traffic annotation is added to network requests of google_apis/gaia/gaia_oauth_client.cc google_apis/gaia/oauth2_access_token_fetcher_impl.cc google_apis/gaia/oauth2_api_call_flow.cc BUG=656607 ========== to ========== Network traffic annotation added to google_apis/gaia. Network traffic annotation is added to network requests of google_apis/gaia/gaia_oauth_client.cc google_apis/gaia/oauth2_access_token_fetcher_impl.cc BUG=656607 ==========
Thank you for your patience on these reviews. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:128: description: "..." This request exchanges an authorization code for an OAuth 2.0 refresh token and an OAuth 2.0 access token. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:129: trigger: "..." This request is triggered at when another service of Chrome requires an access token and a refresh token (e.g. Cloud Print, Chrome Remote Desktop etc.) See https://developers.google.com/identity/protocols/OAuth2 for more information about the Google implementation of the OAuth 2.0 protocol. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:130: data: "..." The Google console client ID and client secret of the caller, the OAuth authorization code and the redirect URI. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:131: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:135: setting: "..." This feature cannot be disabled in settings. However I am not 100% certain about the statement: "but if user signs out of Chrome, this request would not be made." https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:136: chrome_policy { I have no idea if this is gated on any policy. It may be gated on SigninAllowed, but I think that is an artifact today (basically, you need to be signed in today as otherwise there is no account that you may use in Chrome). But I am not sure. Maybe rogerta@ knows. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:172: sender: "..." Same as above https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:173: description: "..." This request fetches a fresh access token that can be used to authenticate an API call to a Google web endpoint. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:174: trigger: "..." This is called whenever the caller needs a fresh OAuth 2.0 access token. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:175: data: "..." The OAuth 2.0 refresh token, the Google console client ID and client secret of the caller, and optionally the scopes of the API for which the access token should be authorized. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:176: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:180: setting: "..." Same as above. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:224: sender: "OAuth2 Client" I am a bit split about the sender. In the other changes in gaia_auth_fetcher, we used the sender "Chrome - Google authentication API". The files in here also make calls to GAIA, however they are more related to OAuth 2.0 calls. Should we use "Chrome - Google authentication API" or "OAuth 2.0 Client" in this file? https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:230: "The OAUth2 access token of the account." s/OAUth2/OAuth 2.0 https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:271: sender: "..." Same as above. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:272: description: "..." This request fetches information about an OAuth 2.0 access token - the response is a dictionary of response values. The provided access token may have any scope, and basic results will be returned: issued_to, audience, scope, expires_in, access_type. In addition, if the https://www.googleapis.com/auth/userinfo.email scope is present, the email and verified_email fields will be returned. If the https://www.googleapis.com/auth/userinfo.profile scope is present, the user_id field will be returned. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:273: trigger: "..." This is triggered after a Google account is added to the browser. It it also triggered after each successful fetch of an OAuth 2.0 access token. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:274: data: "..." The OAuth 2.0 access token. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:275: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:279: setting: "..." "This feature cannot be disabled in settings." https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right): https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:103: sender: "OAuth2 Access Token Fetcher" Here and below: s/OAuth2/Oauth 2.0 https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:119: "Not implemented. Disabling Oauth2 Access Token Fetcher would " Not sure about this policy statement. See comment in the other file. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:119: "Not implemented. Disabling Oauth2 Access Token Fetcher would " s/Oauth2/OAuth 2.0
Thank you Mihai, comments addressed, please review. Roger, Please verify settings and policy cases in google_apis/gaia/gaia_oauth_client.cc. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:128: description: "..." On 2017/05/22 11:49:35, msarda wrote: > This request exchanges an authorization code for an OAuth 2.0 refresh token and > an OAuth 2.0 access token. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:129: trigger: "..." On 2017/05/22 11:49:35, msarda wrote: > This request is triggered at when another service of Chrome requires an access > token and a refresh token (e.g. Cloud Print, Chrome Remote Desktop etc.) > See https://developers.google.com/identity/protocols/OAuth2 for more information > about the Google implementation of the OAuth 2.0 protocol. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:130: data: "..." On 2017/05/22 11:49:35, msarda wrote: > The Google console client ID and client secret of the caller, the OAuth > authorization code and the redirect URI. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:131: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/05/22 11:49:36, msarda wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:172: sender: "..." On 2017/05/22 11:49:35, msarda wrote: > Same as above Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:173: description: "..." On 2017/05/22 11:49:36, msarda wrote: > This request fetches a fresh access token that can be used to authenticate an > API call to a Google web endpoint. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:174: trigger: "..." On 2017/05/22 11:49:36, msarda wrote: > This is called whenever the caller needs a fresh OAuth 2.0 access token. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:175: data: "..." On 2017/05/22 11:49:36, msarda wrote: > The OAuth 2.0 refresh token, the Google console client ID and client secret of > the caller, and optionally the scopes of the API for which the access token > should be authorized. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:176: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/05/22 11:49:35, msarda wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:180: setting: "..." On 2017/05/22 11:49:36, msarda wrote: > Same as above. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:224: sender: "OAuth2 Client" On 2017/05/22 11:49:35, msarda wrote: > I am a bit split about the sender. In the other changes in gaia_auth_fetcher, we > used the sender "Chrome - Google authentication API". > > The files in here also make calls to GAIA, however they are more related to > OAuth 2.0 calls. Should we use "Chrome - Google authentication API" or "OAuth > 2.0 Client" in this file? I am not sure, I used the latter, but if you have more feelings about the former, I will change it. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:230: "The OAUth2 access token of the account." On 2017/05/22 11:49:35, msarda wrote: > s/OAUth2/OAuth 2.0 Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:271: sender: "..." On 2017/05/22 11:49:35, msarda wrote: > Same as above. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:272: description: "..." On 2017/05/22 11:49:36, msarda wrote: > This request fetches information about an OAuth 2.0 access token - the response > is a dictionary of response values. The provided access token may have any > scope, and basic results will be returned: issued_to, audience, scope, > expires_in, access_type. In addition, if the > https://www.googleapis.com/auth/userinfo.email scope is present, the email and > verified_email fields will be returned. If the > https://www.googleapis.com/auth/userinfo.profile scope is present, the > user_id field will be returned. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:273: trigger: "..." On 2017/05/22 11:49:36, msarda wrote: > This is triggered after a Google account is added to the browser. It it also > triggered after each successful fetch of an OAuth 2.0 access token. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:274: data: "..." On 2017/05/22 11:49:35, msarda wrote: > The OAuth 2.0 access token. Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:275: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/05/22 11:49:36, msarda wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:279: setting: "..." On 2017/05/22 11:49:35, msarda wrote: > "This feature cannot be disabled in settings." Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right): https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:103: sender: "OAuth2 Access Token Fetcher" On 2017/05/22 11:49:36, msarda wrote: > Here and below: > s/OAuth2/Oauth 2.0 Done. https://codereview.chromium.org/2796293003/diff/100001/google_apis/gaia/oauth... google_apis/gaia/oauth2_access_token_fetcher_impl.cc:119: "Not implemented. Disabling Oauth2 Access Token Fetcher would " On 2017/05/22 11:49:36, msarda wrote: > s/Oauth2/OAuth 2.0 Done.
lgtm https://codereview.chromium.org/2796293003/diff/120001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2796293003/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:768: "This request exchanges an Oauth2 access token for an uber-auth " s/Oauth2/Oauth 2.0 https://codereview.chromium.org/2796293003/diff/120001/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
Thank you Mihai, comments addressed. Roger, Any comments on settings and policy cases in google_apis/gaia/gaia_oauth_client.cc. https://codereview.chromium.org/2796293003/diff/120001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_auth_fetcher.cc (right): https://codereview.chromium.org/2796293003/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:768: "This request exchanges an Oauth2 access token for an uber-auth " On 2017/05/24 09:07:36, msarda wrote: > s/Oauth2/Oauth 2.0 Done. https://codereview.chromium.org/2796293003/diff/120001/google_apis/gaia/gaia_... google_apis/gaia/gaia_auth_fetcher.cc:807: "This request exchanges an OAuthLogin-scoped oauth2 access token " On 2017/05/24 09:07:36, msarda wrote: > s/oauth2/Oauth 2.0 Done.
All looks good, thanks Ramin. lgtm
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thanks Roger. Martin, Any comments?
What's the relationship between this CL and https://codereview.chromium.org/2872253002 ?
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...
On 2017/05/26 12:32:35, msramek wrote: > What's the relationship between this CL and > https://codereview.chromium.org/2872253002 ? Sorry, leaked CL, removed files from the other CL.
https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:146: "This feature cannot be disabled in settings, but if user signs " nit: the user (as in the other CL) https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:244: description: "This request is used to fetch user information." What kind of information? Some basic information from the user's Google Account? Or from the service to which the token is scoped to? Etc. I get that this is probably a generic function, but the current description is valid for pretty much any request :) https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:246: "The main trigger for this request in the AccountTrackerService " typo: is in
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you Martin, comments addressed. Mihai, There is one last question for you. https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:146: "This feature cannot be disabled in settings, but if user signs " On 2017/05/26 13:44:20, msramek wrote: > nit: the user (as in the other CL) Done. https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:246: "The main trigger for this request in the AccountTrackerService " On 2017/05/26 13:44:20, msramek wrote: > typo: is in Done.
LGTM. https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:244: description: "This request is used to fetch user information." On 2017/05/26 13:44:20, msramek wrote: > What kind of information? > > Some basic information from the user's Google Account? Or from the service to > which the token is scoped to? Etc. I get that this is probably a generic > function, but the current description is valid for pretty much any request :) Let's rephrase this to something like: "This request is used to fetch profile information about the user (e.g. the email, the ID of the account, the full name, the profile picture)."
Thank you Mihai. I added another inline question. https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:244: description: "This request is used to fetch user information." On 2017/05/31 07:44:25, msarda wrote: > On 2017/05/26 13:44:20, msramek wrote: > > What kind of information? > > > > Some basic information from the user's Google Account? Or from the service to > > which the token is scoped to? Etc. I get that this is probably a generic > > function, but the current description is valid for pretty much any request :) > > Let's rephrase this to something like: > "This request is used to fetch profile information about the user (e.g. the > email, the ID of the account, the full name, the profile picture)." By "e.g." you mean other items may also be fetched? If yes, can we name them as well?
https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:244: description: "This request is used to fetch user information." On 2017/05/31 07:50:53, Ramin Halavati wrote: > On 2017/05/31 07:44:25, msarda wrote: > > On 2017/05/26 13:44:20, msramek wrote: > > > What kind of information? > > > > > > Some basic information from the user's Google Account? Or from the service > to > > > which the token is scoped to? Etc. I get that this is probably a generic > > > function, but the current description is valid for pretty much any request > :) > > > > Let's rephrase this to something like: > > "This request is used to fetch profile information about the user (e.g. the > > email, the ID of the account, the full name, the profile picture)." > > By "e.g." you mean other items may also be fetched? If yes, can we name them as > well? The data we get is a dictionary. I do not know the entire fields on this dictionary (this is also something controller by the server, so new fields may be added in the future without Chrome actually processing them). Chrome processes them in various places - take a look at the classes that override the method OnGetUserInfoResponse: https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_oauth_client.h?rcl... The one that I think extracts most of this data is here: https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t...
Thank you Mihai. Martin? https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... File google_apis/gaia/gaia_oauth_client.cc (right): https://codereview.chromium.org/2796293003/diff/160001/google_apis/gaia/gaia_... google_apis/gaia/gaia_oauth_client.cc:244: description: "This request is used to fetch user information." On 2017/05/31 08:32:14, msarda wrote: > On 2017/05/31 07:50:53, Ramin Halavati wrote: > > On 2017/05/31 07:44:25, msarda wrote: > > > On 2017/05/26 13:44:20, msramek wrote: > > > > What kind of information? > > > > > > > > Some basic information from the user's Google Account? Or from the service > > to > > > > which the token is scoped to? Etc. I get that this is probably a generic > > > > function, but the current description is valid for pretty much any request > > :) > > > > > > Let's rephrase this to something like: > > > "This request is used to fetch profile information about the user (e.g. the > > > email, the ID of the account, the full name, the profile picture)." > > > > By "e.g." you mean other items may also be fetched? If yes, can we name them > as > > well? > > The data we get is a dictionary. I do not know the entire fields on this > dictionary (this is also something controller by the server, so new fields may > be added in the future without Chrome actually processing them). Chrome > processes them in various places - take a look at the classes that override the > method OnGetUserInfoResponse: > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_oauth_client.h?rcl... > > The one that I think extracts most of this data is here: > https://cs.chromium.org/chromium/src/components/signin/core/browser/account_t... Thank you. I think as here we mostly care about what is sent, we can keep it at this level.
LGTM, thanks for the updates!
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2796293003/#ps220001 (title: "Annotation updated.")
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": 220001, "attempt_start_ts": 1496293169596900,
"parent_rev": "8c2a1a922dba795bf450b28d8da670ba74715f36", "commit_rev":
"147280e2989d9a8f499e53135d405003488cd88c"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to google_apis/gaia. Network traffic annotation is added to network requests of google_apis/gaia/gaia_oauth_client.cc google_apis/gaia/oauth2_access_token_fetcher_impl.cc BUG=656607 ========== to ========== Network traffic annotation added to google_apis/gaia. Network traffic annotation is added to network requests of google_apis/gaia/gaia_oauth_client.cc google_apis/gaia/oauth2_access_token_fetcher_impl.cc BUG=656607 Review-Url: https://codereview.chromium.org/2796293003 Cr-Commit-Position: refs/heads/master@{#476214} Committed: https://chromium.googlesource.com/chromium/src/+/147280e2989d9a8f499e53135d40... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/147280e2989d9a8f499e53135d40... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
