Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738243003:
Network traffic annotation added to device_description_fetcher. (Closed)
|
Created:
3 years, 9 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, battre Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to device_description_fetcher.
Network traffic annotation is added to network request of
chrome/browser/extensions/api/dial/device_description_fetcher.cc.
BUG=656607
Review-Url: https://codereview.chromium.org/2738243003
Cr-Commit-Position: refs/heads/master@{#457413}
Committed: https://chromium.googlesource.com/chromium/src/+/73d4f0e4ea80b895ee7f1e68d7b6db58509b6bc8
Patch Set 1 #
Total comments: 24
Patch Set 2 : Annotation updated. #Patch Set 3 : Annotation updated. #Messages
Total messages: 26 (14 generated)
rhalavati@chromium.org changed reviewers: + imcheng@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 device_description_fetcher 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.
imcheng@chromium.org changed reviewers: + mfoltz@chromium.org
Adding mfoltz@ to take a look as well. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:54: net::DefineNetworkTrafficAnnotation("...", R"( dial_get_device_description https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:56: sender: "..." dial https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:57: description: "..." Chromium sends a request to a device (such as a smart TV) discovered via the DIAL (Disovery and Launch) protocol to obtain its device description. Chromium then uses the device description to determine the capabilities of the device to be used as a target for casting media content. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:58: trigger: "..." A new or updated device has been discovered via DIAL in the local network. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:59: data: "..." A HTTP GET request. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:60: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER I suppose this should be WEBSITE as the destination is an IP address of a device on the local network. It returns a XML response to a HTTP GET request. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:63: cookies_allowed: false/true false https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:65: setting: "..." This (and the Media Router) can be disabled via the EnableMediaRouter policy. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:67: [POLICY_NAME] { EnableMediaRouter https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:68: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} MANDATORY https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:69: [POLICY_NAME]: ... //(value to disable it) false
Annotation updated, please review. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:54: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/13 19:59:42, imcheng wrote: > dial_get_device_description Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:56: sender: "..." On 2017/03/13 19:59:42, imcheng wrote: > dial Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:57: description: "..." On 2017/03/13 19:59:42, imcheng wrote: > Chromium sends a request to a device (such as a smart TV) discovered via the > DIAL (Disovery and Launch) protocol to obtain its device description. Chromium > then uses the device description to determine the capabilities of the device to > be used as a target for casting media content. Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:58: trigger: "..." On 2017/03/13 19:59:42, imcheng wrote: > A new or updated device has been discovered via DIAL in the local network. Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:59: data: "..." On 2017/03/13 19:59:42, imcheng wrote: > A HTTP GET request. Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:60: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/03/13 19:59:42, imcheng wrote: > I suppose this should be WEBSITE as the destination is an IP address of a device > on the local network. It returns a XML response to a HTTP GET request. I think it's OTHER, as it is not a website that user interacts with. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:63: cookies_allowed: false/true On 2017/03/13 19:59:42, imcheng wrote: > false Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:65: setting: "..." On 2017/03/13 19:59:42, imcheng wrote: > This (and the Media Router) can be disabled via the EnableMediaRouter policy. So it cannot be disabled by settings? https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:67: [POLICY_NAME] { On 2017/03/13 19:59:42, imcheng wrote: > EnableMediaRouter Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:68: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/03/13 19:59:42, imcheng wrote: > MANDATORY Done. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:69: [POLICY_NAME]: ... //(value to disable it) On 2017/03/13 19:59:42, imcheng wrote: > false Done.
https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:65: setting: "..." On 2017/03/13 21:44:30, Ramin Halavati wrote: > On 2017/03/13 19:59:42, imcheng wrote: > > This (and the Media Router) can be disabled via the EnableMediaRouter policy. > > So it cannot be disabled by settings? You can disable it via the media-router flag but not through settings.
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Annotation updated, please review. https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2738243003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/device_description_fetcher.cc:65: setting: "..." On 2017/03/14 22:48:19, imcheng wrote: > On 2017/03/13 21:44:30, Ramin Halavati wrote: > > On 2017/03/13 19:59:42, imcheng wrote: > > > This (and the Media Router) can be disabled via the EnableMediaRouter > policy. > > > > So it cannot be disabled by settings? > > You can disable it via the media-router flag but not through settings. Thank you.
LGTM
The CQ bit was checked by rhalavati@chromium.org
The CQ bit was unchecked by rhalavati@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was checked by rhalavati@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: This issue passed the CQ dry run.
The CQ bit was checked by rhalavati@chromium.org
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": 40001, "attempt_start_ts": 1489669664108760,
"parent_rev": "93e959d18bd91a0ebd267561d44d8f29cb2561d9", "commit_rev":
"73d4f0e4ea80b895ee7f1e68d7b6db58509b6bc8"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to device_description_fetcher. Network traffic annotation is added to network request of chrome/browser/extensions/api/dial/device_description_fetcher.cc. BUG=656607 ========== to ========== Network traffic annotation added to device_description_fetcher. Network traffic annotation is added to network request of chrome/browser/extensions/api/dial/device_description_fetcher.cc. BUG=656607 Review-Url: https://codereview.chromium.org/2738243003 Cr-Commit-Position: refs/heads/master@{#457413} Committed: https://chromium.googlesource.com/chromium/src/+/73d4f0e4ea80b895ee7f1e68d7b6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/73d4f0e4ea80b895ee7f1e68d7b6... |
