Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804613002:
Network traffic annotation added to cloud_print_url_fetcher. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 6 months ago CC:
chromium-reviews, msramek, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to cloud_print_url_fetcher.
Network traffic annotation is added to network request of .
chrome/service/cloud_print/cloud_print_url_fetcher.cc
BUG=656607
Closing in favor of: https://codereview.chromium.org/2888763004
Patch Set 1 #Patch Set 2 : Annotation updated. #
Total comments: 4
Patch Set 3 : Annotation updated. #
Total comments: 8
Messages
Total messages: 21 (7 generated)
rhalavati@chromium.org changed reviewers: + gene@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 cloud_print_url_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.
lgtm overall, please check with vitalybuka@ he might have more context here On 2017/04/05 11:09:50, 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 cloud_print_url_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.
On 2017/04/05 18:37:45, gene1 wrote: > lgtm overall, > please check with vitalybuka@ he might have more context here > > > > On 2017/04/05 11:09:50, 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 cloud_print_url_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. Sorry I wasn't quite clear, I was asking you to suggest values for all the missing data. They are either marked by '...' or are choices separated with '/'.
Best person to suggest values will be paolof@google.com (not part of chromium), he is leading Cloud Print now and have better idea of what they should be. On Wed, Apr 5, 2017 at 11:47 PM, <rhalavati@chromium.org> wrote: > On 2017/04/05 18:37:45, gene1 wrote: > > lgtm overall, > > please check with vitalybuka@ he might have more context here > > > > > > > > On 2017/04/05 11:09:50, 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 cloud_print_url_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_annotation.proto > > > for the definition of the annotations. > > > > > > You can find a sample annotation in: > > > > > > https://cs.chromium.org/chromium/src/components/ > spellcheck/browser/spelling_service_client.cc > > > > > > Entriprise policy options are here: > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/ > components/policy/proto/chrome_settings.proto > > > > > > 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. > > Sorry I wasn't quite clear, I was asking you to suggest values for all the > missing data. They are either marked by '...' or are choices separated > with '/'. > > https://codereview.chromium.org/2804613002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
rhalavati@chromium.org changed reviewers: + sanjeevr@chromium.org, vitalybuka@chromium.org
Vitaly, Sanjeev, Can you help with completing annotation of this network request? Or suggest someone familiar? The questions are: 1- What are the trigger cases? 2- What data is sent to the network? 3- Is destination only local network or Google also receives some data. 4- Does the specified policies disable the feature entirely? Or one of them do it without needing the other one? 5- Also more detailed description will be appreciated.
On 2017/04/10 05:27:33, Ramin Halavati wrote: > Vitaly, Sanjeev, > > Can you help with completing annotation of this network request? Or suggest > someone familiar? > > The questions are: > 1- What are the trigger cases? > 2- What data is sent to the network? > 3- Is destination only local network or Google also receives some data. > 4- Does the specified policies disable the feature entirely? Or one of them do > it without needing the other one? > 5- Also more detailed description will be appreciated. Vitaly, Sanjeev, A gentle reminder on this.
vitalybuka@chromium.org changed reviewers: + thestig@chromium.org - gene@chromium.org, sanjeevr@chromium.org
Description was changed from ========== Network traffic annotation added to cloud_print_url_fetcher. Network traffic annotation is added to network request of . chrome/service/cloud_print/cloud_print_url_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to cloud_print_url_fetcher. Network traffic annotation is added to network request of . chrome/service/cloud_print/cloud_print_url_fetcher.cc BUG=656607 ==========
vitalybuka@chromium.org changed reviewers: - vitalybuka@chromium.org
vitalybuka@chromium.org changed reviewers: + vitalybuka@chromium.org
lgtm
lgtm https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:276: cookies_allowed: true ths is incorrect it sets net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES just below
Please fix cookies part.
https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:268: "Google Cloud Print allows users to print to a corp printer " Drop the word corp? Lots of people use Cloud Print at home too. Maybe: Google Cloud Print allows users to print to printers from Chrome and Android without needing to install print drivers.
Comments addressed, please review. I have added some inline questions. https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:268: "Google Cloud Print allows users to print to a corp printer " On 2017/04/18 19:16:41, Lei Zhang wrote: > Drop the word corp? Lots of people use Cloud Print at home too. Maybe: > > Google Cloud Print allows users to print to printers from Chrome and Android > without needing to install print drivers. Done. https://codereview.chromium.org/2804613002/diff/20001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:276: cookies_allowed: true On 2017/04/18 17:32:44, Vitaly Buka (NO REVIEWS) wrote: > ths is incorrect > it sets net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES just below Done. https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:271: "Printing and running background Cloud Print services." Can "background" be elaborated? https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:272: data: "..." What is sent? https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:273: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL I assume it should be OTHER? https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:291: })"); Are both policies required to disable this request or either of them does that?
https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:271: "Printing and running background Cloud Print services." On 2017/04/19 05:59:07, Ramin Halavati wrote: > Can "background" be elaborated? This is part of the Cloud Print Connector that can run as a separate service process. Which is why we are in chrome/service here. The Connector can be standalone in the sense that it is separate from the entire browser/renderer/gpu/utility process hierarchy. https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:272: data: "..." On 2017/04/19 05:59:07, Ramin Halavati wrote: > What is sent? StartPostRequest() and StartGetRequest() both use this helper function. Tracing their callers, there are a total of 12 non-test callers. So as is, lots of different things. BTW, I didn't write any of this code, so I'm just looking this up as we go. You could do the same, but I'm not sure that's what you want to do. Just to set expectations, shall we continue this Q&A format? https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:273: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/04/19 05:59:07, Ramin Halavati wrote: > I assume it should be OTHER? That would be consistent with "cloud_print_privet_register" and friends. https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... chrome/service/cloud_print/cloud_print_url_fetcher.cc:291: })"); On 2017/04/19 05:59:07, Ramin Halavati wrote: > Are both policies required to disable this request or either of them does that? CloudPrintSubmitEnabled doesn't belong here. It's used in the browser process, not the service process.
On 2017/04/19 19:27:26, Lei Zhang wrote: > https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... > File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): > > https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... > chrome/service/cloud_print/cloud_print_url_fetcher.cc:271: "Printing and running > background Cloud Print services." > On 2017/04/19 05:59:07, Ramin Halavati wrote: > > Can "background" be elaborated? > > This is part of the Cloud Print Connector that can run as a separate service > process. Which is why we are in chrome/service here. The Connector can be > standalone in the sense that it is separate from the entire > browser/renderer/gpu/utility process hierarchy. > > https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... > chrome/service/cloud_print/cloud_print_url_fetcher.cc:272: data: "..." > On 2017/04/19 05:59:07, Ramin Halavati wrote: > > What is sent? > > StartPostRequest() and StartGetRequest() both use this helper function. Tracing > their callers, there are a total of 12 non-test callers. So as is, lots of > different things. > > BTW, I didn't write any of this code, so I'm just looking this up as we go. You > could do the same, but I'm not sure that's what you want to do. Just to set > expectations, shall we continue this Q&A format? > > https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... > chrome/service/cloud_print/cloud_print_url_fetcher.cc:273: destination: > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL > On 2017/04/19 05:59:07, Ramin Halavati wrote: > > I assume it should be OTHER? > > That would be consistent with "cloud_print_privet_register" and friends. > > https://codereview.chromium.org/2804613002/diff/40001/chrome/service/cloud_pr... > chrome/service/cloud_print/cloud_print_url_fetcher.cc:291: })"); > On 2017/04/19 05:59:07, Ramin Halavati wrote: > > Are both policies required to disable this request or either of them does > that? > > CloudPrintSubmitEnabled doesn't belong here. It's used in the browser process, > not the service process. Thank you very much for the comments. I think it's better if I move the annotations to the calling classes and add annotations there. This way we can have more precise specifications for the data and can ask the owners/writers of each file to help. We are working on a newer structure for partial annotations that I think will help in better annotation of this code. I pause this CL until we land partial annotations and will continue updating after that.
Description was changed from ========== Network traffic annotation added to cloud_print_url_fetcher. Network traffic annotation is added to network request of . chrome/service/cloud_print/cloud_print_url_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to cloud_print_url_fetcher. Network traffic annotation is added to network request of . chrome/service/cloud_print/cloud_print_url_fetcher.cc BUG=656607 Closing in favor of: https://codereview.chromium.org/2888763004 ========== |
