Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800653002:
Network traffic annotation added to common/cloud. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to policy/core/common/cloud.
Network traffic annotation is added to network request of
components/policy/core/common/cloud/device_management_service.cc
components/policy/core/common/cloud/external_policy_data_fetcher.cc
components/policy/core/common/cloud/user_info_fetcher.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2800653002
Cr-Commit-Position: refs/heads/master@{#468999}
Committed: https://chromium.googlesource.com/chromium/src/+/9cde810d897d01c57b889a39977b4f39b68cdfb1
Patch Set 1 #
Total comments: 91
Patch Set 2 : Annotations Updated. #Patch Set 3 : Annotation updated. #Patch Set 4 : Annotations updated. #
Total comments: 2
Patch Set 5 : Annotation updated. #
Messages
Total messages: 25 (7 generated)
Description was changed from ========== Network traffic annotation added to common/cloud. Network traffic annotation is added to network request of components/policy/core/common/cloud/device_management_service.cc components/policy/core/common/cloud/external_policy_data_fetcher.cc components/policy/core/common/cloud/user_info_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to policy/core/common/cloud. Network traffic annotation is added to network request of components/policy/core/common/cloud/device_management_service.cc components/policy/core/common/cloud/external_policy_data_fetcher.cc components/policy/core/common/cloud/user_info_fetcher.cc BUG=656607 ==========
rhalavati@chromium.org changed reviewers: + pastarmovj@chromium.org
Julian, I've added annotation templates to 3 files in policy folder, please suggest values for them. Here are the resources if required: 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.
pastarmovj@chromium.org changed reviewers: + atwilson@chromium.org, emaxx@chromium.org
I put some of the values I am fairly sure as comments. Where I was not so sure there is a question mark Drew, Maxim please verify and confirm. Some I outright don't know and left for you to fill in. Please see original comment from the author on this CL for explanation what is expected to go in there. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:596: sender: "..." Cloud Policy https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:600: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:603: cookies_allowed: false/true true? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:604: cookies_store: "..." system? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:605: setting: "..." NA? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:606: chrome_policy { empty. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:612: policy_exception_justification: "..." This request is part of the policy fetcher itself. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:184: sender: "..." Cloud Policy https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:188: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:191: cookies_allowed: false/true true? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:192: cookies_store: "..." system? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:194: chrome_policy { empty? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:200: policy_exception_justification: "..." This request is part of the policy fetcher itself. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:48: sender: "..." Cloud Policy https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:52: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:55: cookies_allowed: false/true true? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:64: policy_exception_justification: "..." The request is part of the policy fetcher itself.
On 2017/04/07 09:58:46, pastarmovj wrote: > I put some of the values I am fairly sure as comments. Where I was not so sure > there is a question mark Drew, Maxim please verify and confirm. Some I outright > don't know and left for you to fill in. Please see original comment from the > author on this CL for explanation what is expected to go in there. > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > File components/policy/core/common/cloud/device_management_service.cc (right): > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:596: sender: > "..." > Cloud Policy > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:600: > destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > GOOGLE_OWNED_SERVICE > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:603: > cookies_allowed: false/true > true? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:604: > cookies_store: "..." > system? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:605: setting: > "..." > NA? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:606: > chrome_policy { > empty. > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/device_management_service.cc:612: > policy_exception_justification: "..." > This request is part of the policy fetcher itself. > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > File components/policy/core/common/cloud/external_policy_data_fetcher.cc > (right): > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/external_policy_data_fetcher.cc:184: sender: > "..." > Cloud Policy > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/external_policy_data_fetcher.cc:188: > destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > GOOGLE_OWNED_SERVICE > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/external_policy_data_fetcher.cc:191: > cookies_allowed: false/true > true? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/external_policy_data_fetcher.cc:192: > cookies_store: "..." > system? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/external_policy_data_fetcher.cc:194: > chrome_policy { > empty? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/external_policy_data_fetcher.cc:200: > policy_exception_justification: "..." > This request is part of the policy fetcher itself. > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > File components/policy/core/common/cloud/user_info_fetcher.cc (right): > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/user_info_fetcher.cc:48: sender: "..." > Cloud Policy > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/user_info_fetcher.cc:52: destination: > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > GOOGLE_OWNED_SERVICE > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/user_info_fetcher.cc:55: cookies_allowed: > false/true > true? > > https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... > components/policy/core/common/cloud/user_info_fetcher.cc:64: > policy_exception_justification: "..." > The request is part of the policy fetcher itself. Drew, Maxim, A friendly reminder on this.
https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:597: description: "..." Communication with the Cloud Policy backend - used to check for the existence of cloud policy for the signed-in account, and to load/update cloud policy if it exists. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:598: trigger: "..." Sign in to Chrome, then also periodic refreshes. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:599: data: "..." Encoded policy settings. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:603: cookies_allowed: false/true On 2017/04/07 09:58:45, pastarmovj wrote: > true? Yes, true, although we don't really use them. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:604: cookies_store: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > system? Yes system https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:605: setting: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > NA? Cannot be controlled by user settings. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:607: [POLICY_NAME] { SigninAllowed https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:608: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} MANDATORY https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:609: [POLICY_NAME]: ... //(value to disable it) SigninAllowed: false https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:185: description: "..." Used to fetch policy for extensions, policy-controlled wallpaper, and custom terms of service. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:186: trigger: "..." Periodically loaded when a managed user is signed in to Chrome. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:187: data: "..." Various data, supplied by the admin for the managed user. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:191: cookies_allowed: false/true On 2017/04/07 09:58:46, pastarmovj wrote: > true? true https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:192: cookies_store: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > system? system https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:193: setting: "..." none https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:194: chrome_policy { On 2017/04/07 09:58:46, pastarmovj wrote: > empty? empty https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:49: description: "..." Calls to the Google Account service to check if the signed-in user is managed. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:50: trigger: "..." User signing in to Chrome. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:51: data: "..." Information about the user (specifically whether they are managed, along with basic profile information from their Google Account). https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:55: cookies_allowed: false/true On 2017/04/07 09:58:46, pastarmovj wrote: > true? true https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:56: cookies_store: "..." system https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:57: setting: "..." none https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:59: [POLICY_NAME] { SigninAllowed https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:60: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} MANDATORY https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:61: [POLICY_NAME]: ... //(value to disable it) SigninAllowed: false
Thank you very much, annotations updated, please review. I've also added some inline questions. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:596: sender: "..." On 2017/04/07 09:58:45, pastarmovj wrote: > Cloud Policy Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:597: description: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > Communication with the Cloud Policy backend - used to check for the existence of > cloud policy for the signed-in account, and to load/update cloud policy if it > exists. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:598: trigger: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > Sign in to Chrome, then also periodic refreshes. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:599: data: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > Encoded policy settings. Could you please elaborate on this, +insisting on user data. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:603: cookies_allowed: false/true On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:45, pastarmovj wrote: > > true? > > Yes, true, although we don't really use them. If you don't use it, I can add a CL to disable it. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:604: cookies_store: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:46, pastarmovj wrote: > > system? > > Yes system Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:605: setting: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:46, pastarmovj wrote: > > NA? > Cannot be controlled by user settings. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:606: chrome_policy { On 2017/04/07 09:58:46, pastarmovj wrote: > empty. Acknowledged. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:607: [POLICY_NAME] { On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > SigninAllowed Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:608: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > MANDATORY Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:609: [POLICY_NAME]: ... //(value to disable it) On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > SigninAllowed: false Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:612: policy_exception_justification: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > This request is part of the policy fetcher itself. Acknowledged. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:184: sender: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > Cloud Policy Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:185: description: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > Used to fetch policy for extensions, policy-controlled wallpaper, and custom > terms of service. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:186: trigger: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > Periodically loaded when a managed user is signed in to Chrome. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:187: data: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > Various data, supplied by the admin for the managed user. Please elaborate. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:188: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/07 09:58:46, pastarmovj wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:191: cookies_allowed: false/true On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:46, pastarmovj wrote: > > true? > > true Load flags disabled SAVE and SEND. Are they just loaded? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:192: cookies_store: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:46, pastarmovj wrote: > > system? > > system Acknowledged. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:193: setting: "..." On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > none Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:194: chrome_policy { On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:46, pastarmovj wrote: > > empty? > > empty Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:200: policy_exception_justification: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > This request is part of the policy fetcher itself. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:48: sender: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > Cloud Policy Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:49: description: "..." On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > Calls to the Google Account service to check if the signed-in user is managed. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:50: trigger: "..." On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > User signing in to Chrome. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:51: data: "..." On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > Information about the user (specifically whether they are managed, along with > basic profile information from their Google Account). Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:52: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/07 09:58:46, pastarmovj wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:55: cookies_allowed: false/true On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > On 2017/04/07 09:58:46, pastarmovj wrote: > > true? > > true SAVE and SEND seem to be disabled in load flags. Are they just loaded? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:56: cookies_store: "..." On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > system Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:57: setting: "..." On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > none Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:59: [POLICY_NAME] { On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > SigninAllowed Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:60: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > MANDATORY Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:61: [POLICY_NAME]: ... //(value to disable it) On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > SigninAllowed: false Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:64: policy_exception_justification: "..." On 2017/04/07 09:58:46, pastarmovj wrote: > The request is part of the policy fetcher itself. Done.
lgtm if you're comfortable with the data provided. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:599: data: "..." On 2017/04/19 05:21:57, Ramin Halavati wrote: > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > Encoded policy settings. > > Could you please elaborate on this, +insisting on user data. I'm not certain what should be described here - is there a document somewhere describing what should be in each field? In general, this network traffic contains various protobufs that implement the cloud policy management protocol, used to deliver policy from the google cloud to chrome and chrome OS clients, for managed users. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:603: cookies_allowed: false/true On 2017/04/19 05:21:57, Ramin Halavati wrote: > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > On 2017/04/07 09:58:45, pastarmovj wrote: > > > true? > > > > Yes, true, although we don't really use them. > > If you don't use it, I can add a CL to disable it. Happy to review such a CL and/or revert it if it turns out we *do* use cookies :) https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:187: data: "..." On 2017/04/19 05:21:57, Ramin Halavati wrote: > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > Various data, supplied by the admin for the managed user. > > Please elaborate. This is used to fetch policy for extensions, which is part of the cloud policy protocol for managed users. The data is generally freeform json based with no fixed schema. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:191: cookies_allowed: false/true On 2017/04/19 05:21:58, Ramin Halavati wrote: > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > On 2017/04/07 09:58:46, pastarmovj wrote: > > > true? > > > > true > > Load flags disabled SAVE and SEND. Are they just loaded? I don't think we use them here so feel free to mark this as false and/or disable cookies in the associated request. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:55: cookies_allowed: false/true On 2017/04/19 05:21:58, Ramin Halavati wrote: > On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > > On 2017/04/07 09:58:46, pastarmovj wrote: > > > true? > > > > true > > SAVE and SEND seem to be disabled in load flags. Are they just loaded? Not intentionally. So feel free to mark this as false and/or block cookies for this request.
Thank you Andrew. I disabled the cookies in another CL, made some small changes, and left some inline comments, please review. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:599: data: "..." On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > Encoded policy settings. > > > > Could you please elaborate on this, +insisting on user data. > > I'm not certain what should be described here - is there a document somewhere > describing what should be in each field? In general, this network traffic > contains various protobufs that implement the cloud policy management protocol, > used to deliver policy from the google cloud to chrome and chrome OS clients, > for managed users. There isn't a well defined document, we will define more precise guideline after the first review of annotations. Generally we should give a clear (but brief) view of what is SENT by this request, specially regarding user data. It seems to me that 'Encoded policy settings' is what this request receives and what is sent is auth data. Am I wrong? https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:600: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/07 09:58:46, pastarmovj wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:603: cookies_allowed: false/true On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > On 2017/04/07 09:58:45, pastarmovj wrote: > > > > true? > > > > > > Yes, true, although we don't really use them. > > > > If you don't use it, I can add a CL to disable it. > > Happy to review such a CL and/or revert it if it turns out we *do* use cookies > :) Done in https://codereview.chromium.org/2837123005 https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:187: data: "..." On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > Various data, supplied by the admin for the managed user. > > > > Please elaborate. > > This is used to fetch policy for extensions, which is part of the cloud policy > protocol for managed users. The data is generally freeform json based with no > fixed schema. Again, please confirm this is what this request sends. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:191: cookies_allowed: false/true On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > On 2017/04/19 05:21:58, Ramin Halavati wrote: > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > On 2017/04/07 09:58:46, pastarmovj wrote: > > > > true? > > > > > > true > > > > Load flags disabled SAVE and SEND. Are they just loaded? > > I don't think we use them here so feel free to mark this as false and/or disable > cookies in the associated request. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/user_info_fetcher.cc:55: cookies_allowed: false/true On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > On 2017/04/19 05:21:58, Ramin Halavati wrote: > > On 2017/04/18 12:01:32, Andrew T Wilson (Slow) wrote: > > > On 2017/04/07 09:58:46, pastarmovj wrote: > > > > true? > > > > > > true > > > > SAVE and SEND seem to be disabled in load flags. Are they just loaded? > > Not intentionally. So feel free to mark this as false and/or block cookies for > this request. Done.
Thanks again for providing all this information on our behalf. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:599: data: "..." On 2017/04/26 05:28:06, Ramin Halavati wrote: > On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > > Encoded policy settings. > > > > > > Could you please elaborate on this, +insisting on user data. > > > > I'm not certain what should be described here - is there a document somewhere > > describing what should be in each field? In general, this network traffic > > contains various protobufs that implement the cloud policy management > protocol, > > used to deliver policy from the google cloud to chrome and chrome OS clients, > > for managed users. > > There isn't a well defined document, we will define more precise guideline after > the first review of annotations. Generally we should give a clear (but brief) > view of what is SENT by this request, specially regarding user data. > > It seems to me that 'Encoded policy settings' is what this request receives and > what is sent is auth data. Am I wrong? Ah, OK, I get it. In theory, all kinds of data could get sent up, depending on what piece of the protocol is being executed. During initial signin or device enrollment, auth data is sent up as part of registration. After initial signin/enrollment, if the session or device is managed, a unique device or profile ID is sent with every future request. On Chrome OS, other diagnostic information can be sent up for managed sessions, including things like which users have used the device, device hardware status, connected networks, CPU usage, etc. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:187: data: "..." On 2017/04/26 05:28:06, Ramin Halavati wrote: > On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > > Various data, supplied by the admin for the managed user. > > > > > > Please elaborate. > > > > This is used to fetch policy for extensions, which is part of the cloud policy > > protocol for managed users. The data is generally freeform json based with no > > fixed schema. > > Again, please confirm this is what this request sends. This request sends no additional data - it is used to load external resources provided by the admin via a unique URL.
Thank you Andrew. Annotation updated, please review. Also that would be awesome if you rephrase |data| in user_info_fetcher.cc to make it more formal. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/device_management_service.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/device_management_service.cc:599: data: "..." On 2017/04/26 09:34:11, Andrew T Wilson (Slow) wrote: > On 2017/04/26 05:28:06, Ramin Halavati wrote: > > On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > > > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > > > Encoded policy settings. > > > > > > > > Could you please elaborate on this, +insisting on user data. > > > > > > I'm not certain what should be described here - is there a document > somewhere > > > describing what should be in each field? In general, this network traffic > > > contains various protobufs that implement the cloud policy management > > protocol, > > > used to deliver policy from the google cloud to chrome and chrome OS > clients, > > > for managed users. > > > > There isn't a well defined document, we will define more precise guideline > after > > the first review of annotations. Generally we should give a clear (but brief) > > view of what is SENT by this request, specially regarding user data. > > > > It seems to me that 'Encoded policy settings' is what this request receives > and > > what is sent is auth data. Am I wrong? > > Ah, OK, I get it. In theory, all kinds of data could get sent up, depending on > what piece of the protocol is being executed. > > During initial signin or device enrollment, auth data is sent up as part of > registration. > After initial signin/enrollment, if the session or device is managed, a unique > device or profile ID is sent with every future request. > On Chrome OS, other diagnostic information can be sent up for managed sessions, > including things like which users have used the device, device hardware status, > connected networks, CPU usage, etc. Done. https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... File components/policy/core/common/cloud/external_policy_data_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/1/components/policy/core/comm... components/policy/core/common/cloud/external_policy_data_fetcher.cc:187: data: "..." On 2017/04/26 09:34:11, Andrew T Wilson (Slow) wrote: > On 2017/04/26 05:28:06, Ramin Halavati wrote: > > On 2017/04/25 13:48:01, Andrew T Wilson (Slow) wrote: > > > On 2017/04/19 05:21:57, Ramin Halavati wrote: > > > > On 2017/04/18 12:01:31, Andrew T Wilson (Slow) wrote: > > > > > Various data, supplied by the admin for the managed user. > > > > > > > > Please elaborate. > > > > > > This is used to fetch policy for extensions, which is part of the cloud > policy > > > protocol for managed users. The data is generally freeform json based with > no > > > fixed schema. > > > > Again, please confirm this is what this request sends. > This request sends no additional data - it is used to load external resources > provided by the admin via a unique URL. Done.
https://codereview.chromium.org/2800653002/diff/60001/components/policy/core/... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/60001/components/policy/core/... components/policy/core/common/cloud/user_info_fetcher.cc:55: "managed, along with basic profile information from their Google " Sorry, this is incorrect. The information we send up is auth information (i.e. oauth token). The information returned is basic profile information and whether the user is managed. When I wrote the original text i didn't realize that |data| just was *sent* data.
Thank you very much, comment addressed, please review. https://codereview.chromium.org/2800653002/diff/60001/components/policy/core/... File components/policy/core/common/cloud/user_info_fetcher.cc (right): https://codereview.chromium.org/2800653002/diff/60001/components/policy/core/... components/policy/core/common/cloud/user_info_fetcher.cc:55: "managed, along with basic profile information from their Google " On 2017/04/27 12:15:31, Andrew T Wilson (Slow) wrote: > Sorry, this is incorrect. The information we send up is auth information (i.e. > oauth token). The information returned is basic profile information and whether > the user is managed. When I wrote the original text i didn't realize that |data| > just was *sent* data. Thank you, Done.
still lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you very much. Dominic, Martin, Any comments?
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...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493824866137120,
"parent_rev": "0abd2199c67e65774e8fff05a31068d17b2ae805", "commit_rev":
"9cde810d897d01c57b889a39977b4f39b68cdfb1"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to policy/core/common/cloud. Network traffic annotation is added to network request of components/policy/core/common/cloud/device_management_service.cc components/policy/core/common/cloud/external_policy_data_fetcher.cc components/policy/core/common/cloud/user_info_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to policy/core/common/cloud. Network traffic annotation is added to network request of components/policy/core/common/cloud/device_management_service.cc components/policy/core/common/cloud/external_policy_data_fetcher.cc components/policy/core/common/cloud/user_info_fetcher.cc BUG=656607 Review-Url: https://codereview.chromium.org/2800653002 Cr-Commit-Position: refs/heads/master@{#468999} Committed: https://chromium.googlesource.com/chromium/src/+/9cde810d897d01c57b889a39977b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9cde810d897d01c57b889a39977b...
Message was sent while issue was closed.
From what I can see, the annotation is missing details about the forced re-enrollment protocol (FRE). This happens during OOBE and it can't be turned off by "signing out of Chrome".
Message was sent while issue was closed.
On 2017/06/07 16:47:40, Thiemo Nagel wrote: > From what I can see, the annotation is missing details about the forced > re-enrollment protocol (FRE). This happens during OOBE and it can't be turned > off by "signing out of Chrome". Thank you Thiemo. Could you please be more specific about the required changes? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
