Network traffic annotation added to sync.

components/sync/driver/sync_stopped_reporter.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync/driver/sync_stopped_reporter.h"
 
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
 
 namespace {
 
 const char kEventEndpoint[] = "event";
 
 // The request is tiny, so even on poor connections 10 seconds should be
 // plenty of time. Since sync is off when this request is started, we don't
 // want anything sync-related hanging around for very long from a human
 // perspective either. This seems like a good compromise.
 const int kRequestTimeoutSeconds = 10;
@@ skipping 28 matching lines @@
   // Make the request proto with the GUID identifying this client.
   sync_pb::EventRequest event_request;
   sync_pb::SyncDisabledEvent* sync_disabled_event =
       event_request.mutable_sync_disabled();
   sync_disabled_event->set_cache_guid(cache_guid);
   sync_disabled_event->set_store_birthday(birthday);
 
   std::string msg;
   event_request.SerializeToString(&msg);
 
-  fetcher_ =
-      net::URLFetcher::Create(sync_event_url_, net::URLFetcher::POST, this);
+  net::NetworkTrafficAnnotationTag traffic_annotation =
+      net::DefineNetworkTrafficAnnotation("...", R"(

[Nicolas Zea, 2017/02/23 22:15:54]

It would be nice if we didn't have to repeat this literal across all these
files. Perhaps hard code it somewhere?

[Ramin Halavati, 2017/02/24 07:53:03]

Please elaborate me if I didn't understand your comment correctly.

The main annotation text will be dropped off during compile time and the
hardcoded |unique_id| will be kept and a pointer to that will be passed to other
functions.

As we want the annotation text not to be part of binary and run-time, we need it
to be in the same compilation unit as where it is used, to extract it using
clang tools.

So if there are two usage sites that need to have similar annotation, might we
have a function that creates URLFetcher for both of them and assigns the
annotation to it there?

[Nicolas Zea, 2017/02/24 20:37:38]

I was just thinking have the R"..." string be saved into a header (maybe within
net/traffic_annotation?) as a constant, and then use that constant here and in
the other sync callsites (or any other places with the same semantics/policy.
I'm mainly thinking about readability/maintainability of the code.

That said, I'm not sure how you're planning to use these annotations. If we had
say a GOOGLE_OWNED constant (or maybe even macro?) used at the sync callsites,
would that defeat what you're trying to accomplish?

[Ramin Halavati, 2017/02/27 09:23:16]

Current plan is extraction of R"..." strings using clang tools in separate
reporting and auditing tools and use them there. Clang tool considers the R"..."
boundary and does not apply macros or constants to it.
I think serialized protobuf is quite readable in code and the reports will be
created after parsing the protobufs. A presubmit tool will be added shortly to
check it the format is correct to help maintainability.

battre@:
Any comments?

+        semantics {
+          sender: "..."

[Nicolas Zea, 2017/03/03 22:39:55]

sender: "chrome sync"
description: "A network request to inform Chrome Sync that sync has been
disabled for this device."
trigger: "User disabled sync."
data: "the sync device id and store birthday"
cookies: false
cookies_store: false
settings: "NA"
policy: UNSET
policy exception: "This network request is only performed after the user has
opted in to sync and then disables sync"

[Ramin Halavati, 2017/03/06 06:55:45]

Thanks, could you elaborate on "Store Birthday"

Can't we use 'SyncDisabled' as a policy here as well? By preventing enabling
Sync, disabling it will also be prevented.

[Nicolas Zea, 2017/03/06 17:49:20]

Store birthday is the identifier for the lifetime of the account data that gets
reset every time the user clears their sync data via the google dashboard.
Perhaps we can just refer to this as "sync device identifier and metadata"?
That's true. I wasn't sure if the policy was meant to be tied to the specific
request or not. I'm fine with repeating SyncDsabled here.

[Ramin Halavati, 2017/03/07 08:53:47]

Done.

+          description: "..."
+          trigger: "..."
+          data: "..."
+          destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
+        }
+        policy {
+          cookies_allowed: false/true
+          cookies_store: "..."
+          setting: "..."
+          policy {
+            [POLICY_NAME] {
+              policy_options {mode: MANDATORY/RECOMMENDED/UNSET}
+              value: ...
+            }
+          }
+          policy_exception_justification: "..."
+        })");
+  fetcher_ = net::URLFetcher::Create(sync_event_url_, net::URLFetcher::POST,
+                                     this, traffic_annotation);
   fetcher_->AddExtraRequestHeader(base::StringPrintf(
       "%s: Bearer %s", net::HttpRequestHeaders::kAuthorization,
       access_token.c_str()));
   fetcher_->AddExtraRequestHeader(base::StringPrintf(
       "%s: %s", net::HttpRequestHeaders::kUserAgent, user_agent_.c_str()));
   fetcher_->SetRequestContext(request_context_.get());
   fetcher_->SetUploadData("application/octet-stream", msg);
   fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
                          net::LOAD_DO_NOT_SAVE_COOKIES |
                          net::LOAD_DO_NOT_SEND_COOKIES);
@@ skipping 34 matching lines @@
   replacements.SetPathStr(path);
   return sync_service_url.ReplaceComponents(replacements);
 }
 
 void SyncStoppedReporter::SetTimerTaskRunnerForTest(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
   timer_.SetTaskRunner(task_runner);
 }
 
 }  // namespace syncer