Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797173003:
Network traffic annotation added to gaia_cookie_manager_service. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to gaia_cookie_manager_service.
Network traffic annotation is added to network request of .
components/signin/core/browser/gaia_cookie_manager_service.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2797173003
Cr-Commit-Position: refs/heads/master@{#471591}
Committed: https://chromium.googlesource.com/chromium/src/+/66ec91a89ac74f8c45561b225b372ffd3e997359
Patch Set 1 #
Total comments: 24
Patch Set 2 : Annotation updated. #
Total comments: 2
Patch Set 3 : Comment addressed. #
Total comments: 8
Patch Set 4 : Annotation updated. #
Total comments: 3
Patch Set 5 : Annotation updated. #
Total comments: 2
Patch Set 6 : nits #Messages
Total messages: 30 (9 generated)
rhalavati@chromium.org changed reviewers: + rogerta@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 gaia_cookie_manager_service and added annotation template to it. Please review it 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.
Description was changed from ========== Network traffic annotation added to gaia_cookie_manager_service. Network traffic annotation is added to network request of . components/signin/core/browser/gaia_cookie_manager_service.cc BUG=656607 ========== to ========== Network traffic annotation added to gaia_cookie_manager_service. Network traffic annotation is added to network request of . components/signin/core/browser/gaia_cookie_manager_service.cc BUG=656607 ==========
rogerta@chromium.org changed reviewers: + msarda@chromium.org - rogerta@chromium.org
rogerta@chromium.org changed reviewers: + rogerta@chromium.org
Changing reviewer to Mihai.
On 2017/04/10 15:16:49, Roger Tawa wrote: > Changing reviewer to Mihai. Hi Mihai, A gentle reminder on this.
https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:218: net::DefineNetworkTrafficAnnotation("...", R"( gaia_cookie_manager::external_cc_result https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:220: sender: "..." GaiaCookieManagerService::ExternalCcResultFetcher https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:221: description: "..." GaiaCookieManager::ExternalCcResultFetcher is used by the GaiaCookieManager when adding an account to the Gaia cookie. This fetcher is used to check the server connection state. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:222: trigger: "..." GaiaCookieManager::ExternalCcResultFetcher is used only once in the first merge session flow by the GaiaCookieManager. The value of the first fetch is stored for future uses. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:223: data: "..." No user-specific data (see https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?rc...) https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:224: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:227: cookies_allowed: false/true false. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:229: setting: "..." This feature cannot be disabled. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:231: [POLICY_NAME] { This feature cannot be enabled or disabled by policy. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:236: policy_exception_justification: "..." Not implemented. Disabling GaiaCookieManager would break features that depend on it (like account consistency and support for child accounts). It makes sense to control top level features that use the GaiaCookieManager.
Thank you very much. Annotation updated, please review. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:218: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/04/24 12:01:52, msarda wrote: > gaia_cookie_manager::external_cc_result Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:220: sender: "..." On 2017/04/24 12:01:52, msarda wrote: > GaiaCookieManagerService::ExternalCcResultFetcher We usually use shorter names, representing the component/module. |Sender| does not need to be unique. What about "Gaia Cookie Manager"? https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:221: description: "..." On 2017/04/24 12:01:52, msarda wrote: > GaiaCookieManager::ExternalCcResultFetcher is used by the GaiaCookieManager when > adding an account to the Gaia cookie. This fetcher is used to check the server > connection state. Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:222: trigger: "..." On 2017/04/24 12:01:52, msarda wrote: > GaiaCookieManager::ExternalCcResultFetcher is used only once in the first merge > session flow by the GaiaCookieManager. The value of the first fetch is stored > for future uses. Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:223: data: "..." On 2017/04/24 12:01:52, msarda wrote: > No user-specific data (see > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?rc...) Isn't there anything else sent? If yes, please briefly name them. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:224: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/24 12:01:52, msarda wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:227: cookies_allowed: false/true On 2017/04/24 12:01:52, msarda wrote: > false. Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:229: setting: "..." On 2017/04/24 12:01:52, msarda wrote: > This feature cannot be disabled. Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:231: [POLICY_NAME] { On 2017/04/24 12:01:52, msarda wrote: > This feature cannot be enabled or disabled by policy. Done. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:236: policy_exception_justification: "..." On 2017/04/24 12:01:52, msarda wrote: > Not implemented. Disabling GaiaCookieManager would break features that depend on > it (like account consistency and support for child accounts). It makes sense to > control top level features that use the GaiaCookieManager. Done.
LGTM. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:220: sender: "..." On 2017/04/25 05:22:28, Ramin Halavati wrote: > On 2017/04/24 12:01:52, msarda wrote: > > GaiaCookieManagerService::ExternalCcResultFetcher > > We usually use shorter names, representing the component/module. |Sender| does > not need to be unique. > > What about "Gaia Cookie Manager"? SGTM. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:223: data: "..." On 2017/04/25 05:22:28, Ramin Halavati wrote: > On 2017/04/24 12:01:52, msarda wrote: > > No user-specific data (see > > > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?rc...) > > Isn't there anything else sent? If yes, please briefly name them. I read again the code in https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?l=222 and I do not think we send any data in this request (we just load the given URL). https://codereview.chromium.org/2797173003/diff/20001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/20001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookie. It checks the server connection " s/cookie/cookies
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you very much, comment addressed. Martin, Any comments? https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:220: sender: "..." On 2017/04/25 07:50:49, msarda wrote: > On 2017/04/25 05:22:28, Ramin Halavati wrote: > > On 2017/04/24 12:01:52, msarda wrote: > > > GaiaCookieManagerService::ExternalCcResultFetcher > > > > We usually use shorter names, representing the component/module. |Sender| does > > not need to be unique. > > > > What about "Gaia Cookie Manager"? > > SGTM. Acknowledged. https://codereview.chromium.org/2797173003/diff/1/components/signin/core/brow... components/signin/core/browser/gaia_cookie_manager_service.cc:223: data: "..." On 2017/04/25 07:50:49, msarda wrote: > On 2017/04/25 05:22:28, Ramin Halavati wrote: > > On 2017/04/24 12:01:52, msarda wrote: > > > No user-specific data (see > > > > > > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?rc...) > > > > Isn't there anything else sent? If yes, please briefly name them. > > I read again the code in > https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_auth_fetcher.cc?l=222 > and I do not think we send any data in this request (we just load the given > URL). Thank you for confirmation. https://codereview.chromium.org/2797173003/diff/20001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/20001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookie. It checks the server connection " On 2017/04/25 07:50:49, msarda wrote: > s/cookie/cookies Done.
LGTM
https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookies to check the server connection " I'm not sure "Gaia" is understandable to an external reader. Could we just say Chrome/Google sign-in cookies or something such? https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:225: "state." Optional: Why do we need to know the server connection state?
Still LGTM. https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookies to check the server connection " On 2017/05/03 16:22:34, msramek (recovering) wrote: > I'm not sure "Gaia" is understandable to an external reader. Could we just say > Chrome/Google sign-in cookies or something such? We already use GAIA/Gaia in a lot of places in Chrome: https://cs.chromium.org/search/?q=Gaia+package:%5Echromium$&type=cs https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:225: "state." On 2017/05/03 16:22:34, msramek (recovering) wrote: > Optional: Why do we need to know the server connection state? It looks like this is an optional request that allows Gaia to avoid doing some server-side checks (see comment above on function |OnGetCheckConnectionInfoError|). That is all I know unfortunately.
https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookies to check the server connection " On 2017/05/09 08:35:42, msarda wrote: > On 2017/05/03 16:22:34, msramek (recovering) wrote: > > I'm not sure "Gaia" is understandable to an external reader. Could we just say > > Chrome/Google sign-in cookies or something such? > > We already use GAIA/Gaia in a lot of places in Chrome: > https://cs.chromium.org/search/?q=Gaia+package:%5Echromium$&type=cs Right, but we don't use it in HC articles or the privacy whitepaper. This text is to be read by enterprise admins configuring Chrome. I think it's fine to say "used by GaiaCookieManager" because, well, that's just how the class is called. But when we're explaining what it does, we should use terms that the reader will know.
https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookies to check the server connection " On 2017/05/09 09:09:45, msramek (recovering) wrote: > On 2017/05/09 08:35:42, msarda wrote: > > On 2017/05/03 16:22:34, msramek (recovering) wrote: > > > I'm not sure "Gaia" is understandable to an external reader. Could we just > say > > > Chrome/Google sign-in cookies or something such? > > > > We already use GAIA/Gaia in a lot of places in Chrome: > > https://cs.chromium.org/search/?q=Gaia+package:%5Echromium$&type=cs > > Right, but we don't use it in HC articles or the privacy whitepaper. This text > is to be read by enterprise admins configuring Chrome. > > I think it's fine to say "used by GaiaCookieManager" because, well, that's just > how the class is called. But when we're explaining what it does, we should use > terms that the reader will know. I did not know this will be used in "HC" articles or privacy whitepaper. In that case I agree we would need to be more careful about this text. Ramin: Is that so? Where will this text be used? Gaia is the Google authentication server. So I would change it to something like "Google authentication cookies".
Annotation updated, please review. https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:224: "account to the Gaia cookies to check the server connection " On 2017/05/09 09:17:31, msarda wrote: > On 2017/05/09 09:09:45, msramek (recovering) wrote: > > On 2017/05/09 08:35:42, msarda wrote: > > > On 2017/05/03 16:22:34, msramek (recovering) wrote: > > > > I'm not sure "Gaia" is understandable to an external reader. Could we just > > say > > > > Chrome/Google sign-in cookies or something such? > > > > > > We already use GAIA/Gaia in a lot of places in Chrome: > > > https://cs.chromium.org/search/?q=Gaia+package:%5Echromium$&type=cs > > > > Right, but we don't use it in HC articles or the privacy whitepaper. This text > > is to be read by enterprise admins configuring Chrome. > > > > I think it's fine to say "used by GaiaCookieManager" because, well, that's > just > > how the class is called. But when we're explaining what it does, we should use > > terms that the reader will know. > > I did not know this will be used in "HC" articles or privacy whitepaper. In that > case I agree we would need to be more careful about this text. > Ramin: Is that so? Where will this text be used? > > Gaia is the Google authentication server. So I would change it to something like > "Google authentication cookies". > Yes Mihai, as Martin mentioned, this data is also used for enterprise policy assertions and help on how to set required limitations. https://codereview.chromium.org/2797173003/diff/40001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:225: "state." On 2017/05/09 08:35:42, msarda wrote: > On 2017/05/03 16:22:34, msramek (recovering) wrote: > > Optional: Why do we need to know the server connection state? > > It looks like this is an optional request that allows Gaia to avoid doing some > server-side checks (see comment above on function > |OnGetCheckConnectionInfoError|). That is all I know unfortunately. Acknowledged.
lgtm
Thanks! I have one more improvement request :) https://codereview.chromium.org/2797173003/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:227: "This is used only once in the first merge session flow by the " Can we also update this for the external readers? When does the merge session flow happen from the user's perspective? Is it something like on Chrome startup, when an account is added, etc.?
https://codereview.chromium.org/2797173003/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:227: "This is used only once in the first merge session flow by the " On 2017/05/09 11:58:20, msramek (recovering) wrote: > Can we also update this for the external readers? When does the merge session > flow happen from the user's perspective? Is it something like on Chrome startup, > when an account is added, etc.? How about: "This is used at most once per lifetime of the application during the first merge session flow (the flow used to add an account for which Chrome has a valid OAuth 2 refresh token to the Gaia authentication cookies). The value of the first fetch is stored for future in RAM for future uses."
Thank you Mihai. Martin? https://codereview.chromium.org/2797173003/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:227: "This is used only once in the first merge session flow by the " On 2017/05/10 08:56:39, msarda wrote: > On 2017/05/09 11:58:20, msramek (recovering) wrote: > > Can we also update this for the external readers? When does the merge session > > flow happen from the user's perspective? Is it something like on Chrome > startup, > > when an account is added, etc.? > > How about: > "This is used at most once per lifetime of the application during the first > merge session flow (the flow used to add an account for which Chrome has a valid > OAuth 2 refresh token to the Gaia authentication cookies). The value of the > first fetch is stored for future in RAM for future uses." Done.
LGTM % nit. Thanks for rephrasing! https://codereview.chromium.org/2797173003/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:231: "is stored for future in RAM for future uses." nit: s/for future// (we say "for future uses" right after that)
Thanks Martin, comment addressed, landing. https://codereview.chromium.org/2797173003/diff/80001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2797173003/diff/80001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:231: "is stored for future in RAM for future uses." On 2017/05/12 18:56:41, msramek wrote: > nit: s/for future// > > (we say "for future uses" right after that) 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/2797173003/#ps100001 (title: "nits")
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": 100001, "attempt_start_ts": 1494666876904260,
"parent_rev": "93b356ad256b7bff7db894592f1e49fd7aa9aeb3", "commit_rev":
"66ec91a89ac74f8c45561b225b372ffd3e997359"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to gaia_cookie_manager_service. Network traffic annotation is added to network request of . components/signin/core/browser/gaia_cookie_manager_service.cc BUG=656607 ========== to ========== Network traffic annotation added to gaia_cookie_manager_service. Network traffic annotation is added to network request of . components/signin/core/browser/gaia_cookie_manager_service.cc BUG=656607 Review-Url: https://codereview.chromium.org/2797173003 Cr-Commit-Position: refs/heads/master@{#471591} Committed: https://chromium.googlesource.com/chromium/src/+/66ec91a89ac74f8c45561b225b37... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/66ec91a89ac74f8c45561b225b37... |
