Network traffic annotation added to autofill_download_manager.

Network traffic annotation added to autofill_download_manager.

Network traffic annotation is added to network request of
components/autofill/core/browser/autofill_download_manager.cc.

BUG=656607
Review-Url: https://codereview.chromium.org/2740213003
Cr-Commit-Position: refs/heads/master@{#457407}
Committed: https://chromium.googlesource.com/chromium/src/+/a87dcc8636090e23c4301a26b4645324653d9a52

[Ramin Halavati, 2017-03-10 09:47:28]

rhalavati@chromium.org changed reviewers:
+ rogerm@chromium.org

[Ramin Halavati, 2017-03-10 09:47:29]

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 autofill_download_manager 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.

[Ramin Halavati, 2017-03-15 11:59:31]

On 2017/03/10 09:47:29, 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 autofill_download_manager 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.

Hi,

A friendly reminder on this.

[Roger McFarlane (Chromium), 2017-03-15 14:54:56]

Is this what you're looking for?

https://codereview.chromium.org/2740213003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_download_manager.cc (right):

https://codereview.chromium.org/2740213003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_download_manager.cc:255: })");
There are actuallly two different requests that can be generated here.
Identified by request_data.request_type.

if request_data.request_type == AutofillDownloadManager::REQUEST_QUERY

  net::NetworkTrafficAnnotationTag traffic_annotation =
      net::DefineNetworkTrafficAnnotation("autofill_query", R"(
        semantics {
          sender: "autofill"
          description:
            "Google Chrome can automatically fill in web forms. If the feature "
            "is enabled, Chrome will send a non-identifying description of the "
            "form to Google's servers, which will respond with the type of data
"
            "required by each of the forms field, if known. I.e., if a field
expects "
            "to receive a name, phone number, street address, etc."
          trigger: "User encounters a web form."
          data: "Hashed descriptions of the form, its fields, and action. User
data"
            "is not sent."
          destination: GOOGLE_OWNED_SERVICE
        }
        policy {
          cookies_allowed: false
          setting:
            "You can enable or disable this feature via 'Enable autofill to "
            "fill out web forms in a single click.' in Chrome's settings under "
            "Advanced. The feature is enabled by default."
          policy {
            AutofillEnabled {
                policy_options {mode: MANDATORY}
                AutofillEnabled: false
            }
          }
        })"

if request_data.request_type == AutofillDownloadManager::REQUEST_UPLOAD

  net::NetworkTrafficAnnotationTag traffic_annotation =
      net::DefineNetworkTrafficAnnotation("autofill_upload", R"(
        semantics {
          sender: "autofill"
          description:
            "Google Chrome relies on crowd-sourced field type classifications "
            " to help it automatically fill in web forms. If the feature is
enabled, "
            "Chrome will send a non-identifying description of the form to "
            "Google's servers along with the type of data Chrome observed "
            "being given to the form. I.e., if you entered your first name into
"
            "a form field, Chrome will 'vote' for that form field being a first
"
            "name field."
          trigger: "User submits a web form."
          data: "Hashed descriptions of the form and its fields along with "
            "type of data given to each field, if recognized from the user's "
            "profile(s). The user data is not sent."
          destination: GOOGLE_OWNED_SERVICE
        }
        policy {
          cookies_allowed: false
          setting:
            "You can enable or disable this feature via 'Enable autofill to "
            "fill out web forms in a single click.' in Chrome's settings under "
            "Advanced. The feature is enabled by default."
          policy {
            AutofillEnabled {
                policy_options {mode: MANDATORY}
                AutofillEnabled: false
            }
          }
        })"

[Ramin Halavati, 2017-03-15 15:25:34]

Thank you very much, annotations updated, please review.

As we cannot have default values and variables without values for annotations,
we cannot use a switch/case for generation of annotation and we need to ensure
all cases of the given parameter are annotated.
In this former CL (https://codereview.chromium.org/2703363002) we finally
reached to the solution that I have applied here. Please suggest if you prefer
otherwise.

[Roger McFarlane (Chromium), 2017-03-15 20:58:30]

lgtm

https://codereview.chromium.org/2740213003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_download_manager.cc (right):

https://codereview.chromium.org/2740213003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_download_manager.cc:48: "Hashed
descriptions of the form, its fields, and action. User data"
nit: maybe shorten to "Hashed descriptions of the form and its fields. User data
is not sent." to match the upload language.

https://codereview.chromium.org/2740213003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_download_manager.cc:65: } else {
nit: no need to indent in an else as the if case ended in return.

[Ramin Halavati, 2017-03-16 08:57:49]

rhalavati@chromium.org changed reviewers:
+ battre@chromium.org, msramek@chromium.org

[Ramin Halavati, 2017-03-16 08:57:50]

Thank you.

Martin, Dominic,
Do you have any comments?

https://codereview.chromium.org/2740213003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_download_manager.cc (right):

https://codereview.chromium.org/2740213003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_download_manager.cc:48: "Hashed
descriptions of the form, its fields, and action. User data"
On 2017/03/15 20:58:30, Roger McFarlane (Chromium) wrote:
> nit: maybe shorten to "Hashed descriptions of the form and its fields. User
data
> is not sent." to match the upload language.

Done.

https://codereview.chromium.org/2740213003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_download_manager.cc:65: } else {
On 2017/03/15 20:58:30, Roger McFarlane (Chromium) wrote:
> nit: no need to indent in an else as the if case ended in return.

Done.

[msramek, 2017-03-16 09:32:40]

LGTM

[Ramin Halavati, 2017-03-16 11:50:42]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-03-16 11:50:43]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org
Link to the patchset: https://codereview.chromium.org/2740213003/#ps40001
(title: "Annotation updated.")

[commit-bot: I haz the power, 2017-03-16 11:51:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 12:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489665042875950,
"parent_rev": "0b6891ca2968c288af67584dd2ce758ef12fd699", "commit_rev":
"a87dcc8636090e23c4301a26b4645324653d9a52"}

[commit-bot: I haz the power, 2017-03-16 12:38:29]

Description was changed from

==========
Network traffic annotation added to autofill_download_manager.

Network traffic annotation is added to network request of
components/autofill/core/browser/autofill_download_manager.cc.

BUG=656607
==========

to

==========
Network traffic annotation added to autofill_download_manager.

Network traffic annotation is added to network request of
components/autofill/core/browser/autofill_download_manager.cc.

BUG=656607

Review-Url: https://codereview.chromium.org/2740213003
Cr-Commit-Position: refs/heads/master@{#457407}
Committed:
https://chromium.googlesource.com/chromium/src/+/a87dcc8636090e23c4301a26b464...
==========

[commit-bot: I haz the power, 2017-03-16 12:38:31]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a87dcc8636090e23c4301a26b464...