Network traffic annotation added to drive/base_requests.

Network traffic annotation added to drive/base_requests.

Network traffic annotation is added to network request of
drive/base_requests.

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

[Ramin Halavati, 2017-03-06 08:09:45]

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

[Ramin Halavati, 2017-03-06 08:09:46]

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 drive/base_requests.cc 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.

[kinaba, 2017-03-06 08:32:19]

kinaba@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinaba, 2017-03-06 08:32:20]

+kinuko,

To me it looks we should have separate annotation for
* Google Drive integration for Chrome OS, and
* Sync FileSystem
(since for instance the former is controlled by DriveDisabled policy but the
other is not, etc.)

What do you think?
Having DriveAPIService constructor to take this tag argument may be one way to
do that.

[Ramin Halavati, 2017-03-06 08:58:52]

On 2017/03/06 08:32:20, kinaba wrote:
> +kinuko,
> 
> To me it looks we should have separate annotation for
> * Google Drive integration for Chrome OS, and
> * Sync FileSystem
> (since for instance the former is controlled by DriveDisabled policy but the
> other is not, etc.)
> 
> What do you think?
> Having DriveAPIService constructor to take this tag argument may be one way to
> do that.

Thank you, I am not sure if I am clear enough on what DriveAPIService does, but
based on my current understanding:
Is it sufficient if annotation would be added to DriveAPIService constructor, or
we may need different annotations for its different functions like 'Search',
'GetChangeList', 'GetFileResouce', etc.?
I mean if a DriveAPIService object is only used for one task, annotation can be
passed to it based on the task, otherwise we need different annotations for
different functions.

[kinaba, 2017-03-06 11:44:26]

On 2017/03/06 08:58:52, Ramin Halavati wrote:
> Is it sufficient if annotation would be added to DriveAPIService constructor,

That's what I was thinking of.
Adding the annotation to DriveAPIService constructor, which forwards it to the
constructor
of RequestSender, which in turn holds the annotation object instance and
provides a getter
method, which is used from the location of the patch set 1.

> I mean if a DriveAPIService object is only used for one task,

It depends on your definition of "task".
DriveAPIService itself is a wrapper of Google Drive REST API. Each method
corresponds to one API call
so in a certain sense it corresponds to a "task", but on the other hand, single
API call does not usually
have any definitive meaning, because actual user visible feature almost always
corresponds to the combination
of multiple API calls. Each of the 3 existing instances of DriveAPIService
represents different domain
and thus can have clear annotations.

[Ramin Halavati, 2017-03-06 14:14:22]

NetworkTrafficAnnotaionTag moved to DeviceAPIService constructor and is passed
to URLFetcher::Create through RequestSender object.

I also modified all unittests that were affected and added 
TRAFFIC_ANNOTATION_FOR_TESTS to them, stating that they don't need annotation.

I just found one usage for DeviceAPIService in sync_engine.cc and added
annotation template there, but it seems that annotation should become a
parameter of function SyncEngine::DriveServiceFactory::CreateDriveService, am I
correct?

[kinaba, 2017-03-06 23:33:54]

There are two more callers
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/drive/drive_inte...
https://cs.chromium.org/chromium/src/chrome/browser/apps/drive/drive_service_...

[Ramin Halavati, 2017-03-07 10:27:04]

Comments addressed, please review and provide annotation for sync_engine.cc

[kinaba, 2017-03-07 11:17:26]

kinaba@chromium.org changed reviewers:
+ tzik@chromium.org

[kinaba, 2017-03-07 11:17:27]

+kinuko, +tzik, could you add comments for sync_engine.cc?


LGTM for everywhere else, where I should have ownership.
(I assume you'll have separate patches for NOT_ANNOTATED_YET after this?)

[kinaba, 2017-03-07 11:22:06]

The CQ bit was checked by kinaba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 11:22:28]

Dry run: 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-07 11:37:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 11:37:24]

Dry run: 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_...)

[kinaba, 2017-03-07 11:43:02]

eh, compilation error, so it's meant to be NO_TRAFFIC_ANNOTATION_YET ?

[Ramin Halavati, 2017-03-07 13:26:27]

On 2017/03/07 11:43:02, kinaba wrote:
> eh, compilation error, so it's meant to be NO_TRAFFIC_ANNOTATION_YET ?

Sorry, thanks, corrected.

[Ramin Halavati, 2017-03-07 13:27:23]

On 2017/03/07 13:26:27, Ramin Halavati wrote:
> On 2017/03/07 11:43:02, kinaba wrote:
> > eh, compilation error, so it's meant to be NO_TRAFFIC_ANNOTATION_YET ?
> 
> Sorry, thanks, corrected.

We will have annotations for Android and ChromeOS in next phase. Priority is now
Chromium.

[kinaba, 2017-03-07 13:29:24]

The CQ bit was checked by kinaba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 13:29:42]

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

[kinaba, 2017-03-07 13:29:57]

On 2017/03/07 13:27:23, Ramin Halavati wrote:
> On 2017/03/07 13:26:27, Ramin Halavati wrote:
> > On 2017/03/07 11:43:02, kinaba wrote:
> > > eh, compilation error, so it's meant to be NO_TRAFFIC_ANNOTATION_YET ?
> > 
> > Sorry, thanks, corrected.
> 
> We will have annotations for Android and ChromeOS in next phase. Priority is
now
> Chromium.

Got it. Thanks.

[tzik, 2017-03-07 13:37:43]

lgtm

[kinaba, 2017-03-07 13:39:23]

On 2017/03/07 13:37:43, tzik wrote:
> lgtm

Hey, you are asked to fill the "..." in the CL, not just approving it :)
PTAL at the first comment by Ramin.

[commit-bot: I haz the power, 2017-03-07 15:36:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:36:40]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-03-08 05:30:34]

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:71:
net::DefineNetworkTrafficAnnotation("...", R"(
sync_file_system

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:73: sender: "..."
Sync FileSystem Chrome API

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:74: description:
"..."
Sync FileSystem API provides an isolated FileSystem to Chrome Apps. The contents
of the FileSystem are automatically synced among application instances through a
hidden folder on Google Drive. This traffic is uploading or downloading these
files for synchronization.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:75: trigger: "..."
When a Chrome App uses Sync FileSystem API, or when a file on Google Drive is
modified.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:76: data: "..."
Files that created by Chrome Apps via Sync FileSystem API.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:77: destination:
WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
GOOGLE_OWNED_SERVICE

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:80:
cookies_allowed: false/true
false

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:81: cookies_store:
"..."
This feature cannot be disabled in settings.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:89:
policy_exception_justification: "..."
Not implemented.

[Ramin Halavati, 2017-03-08 09:41:20]

Annotation updated, please review.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:71:
net::DefineNetworkTrafficAnnotation("...", R"(
On 2017/03/08 05:30:34, tzik wrote:
> sync_file_system

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:73: sender: "..."
On 2017/03/08 05:30:34, tzik wrote:
> Sync FileSystem Chrome API

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:74: description:
"..."
On 2017/03/08 05:30:34, tzik wrote:
> Sync FileSystem API provides an isolated FileSystem to Chrome Apps. The
contents
> of the FileSystem are automatically synced among application instances through
a
> hidden folder on Google Drive. This traffic is uploading or downloading these
> files for synchronization.

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:75: trigger: "..."
On 2017/03/08 05:30:34, tzik wrote:
> When a Chrome App uses Sync FileSystem API, or when a file on Google Drive is
> modified.

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:76: data: "..."
On 2017/03/08 05:30:34, tzik wrote:
> Files that created by Chrome Apps via Sync FileSystem API.

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:77: destination:
WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
On 2017/03/08 05:30:34, tzik wrote:
> GOOGLE_OWNED_SERVICE

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:80:
cookies_allowed: false/true
On 2017/03/08 05:30:34, tzik wrote:
> false

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:81: cookies_store:
"..."
On 2017/03/08 05:30:34, tzik wrote:
> This feature cannot be disabled in settings.

Done.

https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:89:
policy_exception_justification: "..."
On 2017/03/08 05:30:34, tzik wrote:
> Not implemented.

Done.

[Ramin Halavati, 2017-03-14 11:46:20]

On 2017/03/08 09:41:20, Ramin Halavati wrote:
> Annotation updated, please review.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:71:
> net::DefineNetworkTrafficAnnotation("...", R"(
> On 2017/03/08 05:30:34, tzik wrote:
> > sync_file_system
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:73: sender: "..."
> On 2017/03/08 05:30:34, tzik wrote:
> > Sync FileSystem Chrome API
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:74: description:
> "..."
> On 2017/03/08 05:30:34, tzik wrote:
> > Sync FileSystem API provides an isolated FileSystem to Chrome Apps. The
> contents
> > of the FileSystem are automatically synced among application instances
through
> a
> > hidden folder on Google Drive. This traffic is uploading or downloading
these
> > files for synchronization.
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:75: trigger:
"..."
> On 2017/03/08 05:30:34, tzik wrote:
> > When a Chrome App uses Sync FileSystem API, or when a file on Google Drive
is
> > modified.
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:76: data: "..."
> On 2017/03/08 05:30:34, tzik wrote:
> > Files that created by Chrome Apps via Sync FileSystem API.
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:77: destination:
> WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
> On 2017/03/08 05:30:34, tzik wrote:
> > GOOGLE_OWNED_SERVICE
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:80:
> cookies_allowed: false/true
> On 2017/03/08 05:30:34, tzik wrote:
> > false
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:81:
cookies_store:
> "..."
> On 2017/03/08 05:30:34, tzik wrote:
> > This feature cannot be disabled in settings.
> 
> Done.
> 
>
https://codereview.chromium.org/2728323002/diff/100001/chrome/browser/sync_fi...
> chrome/browser/sync_file_system/drive_backend/sync_engine.cc:89:
> policy_exception_justification: "..."
> On 2017/03/08 05:30:34, tzik wrote:
> > Not implemented.
> 
> Done.

A friendly reminder on this.

[tzik, 2017-03-14 11:55:03]

lgtm

[Ramin Halavati, 2017-03-14 12:00:06]

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

[Ramin Halavati, 2017-03-14 12:00:07]

Thank you.

battre@, msramek@,
Do you have any comments?

[msramek, 2017-03-14 22:38:38]

LGTM

[Ramin Halavati, 2017-03-15 06:38:33]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-03-15 06:38:34]

The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/2728323002/#ps120001
(title: "Annoatation udpated.")

[commit-bot: I haz the power, 2017-03-15 06:38:57]

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-15 07:58:21]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489559913695620,
"parent_rev": "00be4e9a8303846ea830a6229e5533feb1ce4ca3", "commit_rev":
"74805ef21d813e8745a18ec0722f772e39515ec4"}

[commit-bot: I haz the power, 2017-03-15 07:59:31]

Description was changed from

==========
Network traffic annotation added to drive/base_requests.

Network traffic annotation is added to network request of
drive/base_requests.

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

to

==========
Network traffic annotation added to drive/base_requests.

Network traffic annotation is added to network request of
drive/base_requests.

BUG=656607

Review-Url: https://codereview.chromium.org/2728323002
Cr-Commit-Position: refs/heads/master@{#457028}
Committed:
https://chromium.googlesource.com/chromium/src/+/74805ef21d813e8745a18ec0722f...
==========

[commit-bot: I haz the power, 2017-03-15 07:59:33]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/74805ef21d813e8745a18ec0722f...