Network traffic annotation added to password_protection_request

components/safe_browsing/password_protection/password_protection_request.cc

Index: components/safe_browsing/password_protection/password_protection_request.cc
diff --git a/components/safe_browsing/password_protection/password_protection_request.cc b/components/safe_browsing/password_protection/password_protection_request.cc
index 4a1f36f352ad4ddc8bd76b7ce303a3cdcd19356d..b0e4c16606864487ebb905a7edac63fd7298bca8 100644
--- a/components/safe_browsing/password_protection/password_protection_request.cc
+++ b/components/safe_browsing/password_protection/password_protection_request.cc
@@ -12,6 +12,7 @@
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
 #include "net/http/http_status_code.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
 
 using content::BrowserThread;
 
@@ -126,10 +127,47 @@ void PasswordProtectionRequest::SendRequest() {
 
   // In case the request take too long, we set a timer to cancel this request.
   StartTimeout();
-
+  net::NetworkTrafficAnnotationTag traffic_annotation =
+      net::DefineNetworkTrafficAnnotation("password_protection_request", R"(
+        semantics {
+          sender: "Safe Browsing"
+          description:
+            "When the user is about to log in to a new, uncommon site, Chrome "
+            "will send a request to Safe Browsing to determine if the page is "
+            "phishing. It'll then show a warning if the page is bad."

[msramek, 2017/05/12 19:10:19]

nit: "bad" is a subjective term.

Maybe "...if the page poses a risk of phishing."?

[Jialiu Lin, 2017/05/12 19:59:33]

Agree, "...if the page poses a risk of phishing." sounds better.

[Ramin Halavati, 2017/05/14 13:21:33]

Done.

+          trigger:
+            "When the user has opted-in to Safe Browsing extended reporting, "
+            "focuses on a password field on a page that isn't popular or known "

[msramek, 2017/05/12 19:10:19]

nit: I would rephrase a bit for readability.

"When a user focuses on a password field on a page that they haven't visited
before and that isn't popular or known to be safe."

We can leave the SBER part for "setting" to make this shorter.

[Jialiu Lin, 2017/05/12 19:59:33]

SGTM

[Ramin Halavati, 2017/05/14 13:21:33]

Done.

+            "to be safe, and they haven't visited."
+          data:
+            "URL and referrer of the current page, password form action, and "
+            "iframe structure."
+          destination: GOOGLE_OWNED_SERVICE
+        }
+        policy {
+          cookies_allowed: true
+          cookies_store: "Safe Browsing Cookie Store"
+          setting:
+            "Users can control this feature via 'Protect you and your device "
+            "from dangerous sites' or 'Automatically report details of "
+            "possible security incidents to Google' setting under 'Privacy'. "

[msramek, 2017/05/12 19:10:19]

Do we normally list both SB and SBER as the setting for SBER features? I think
we normally only list SBER.

But I also vaguely remember that we used to explain the difference between SBER1
and SBER2.

[Jialiu Lin, 2017/05/12 19:59:33]

SBER condition is temporary, eventually this feature will be extended to all SB
users.  The current setting text looks fine for me. 

And this feature applies to both SBER1 and SBER2. Do you think we still need to
explain the differences?

[msramek, 2017/05/15 12:57:28]

I'm just saying that all SBER features can be disabled by switching off either
SBER or SB, but we don't describe that in other annotations.

I'm not sure that future plans play a role here. First of all, the annotation
should be correct for the current implemention. Secondly, the next step in the
rollout plan includes history sync users, and we don't mention that either. And
finally, it's not yet clear whether this will be launched for SB as a whole -
that depends on your metrics.

That said, the current wording is technically correct, so I'm fine with it if
you think it should stay this way.
No. But I think we do that in some annotations, so I'm aiming for consistency.

[Ramin Halavati, 2017/05/15 13:37:18]

We usually set the most fine grained policy that disables the feature. So I
think we can just have 'SafeBrowsingExtendedReportingOptInAllowed'.
What do you suggest Jialio?

+            "By default, the first setting is enabled and the second is not."
+          chrome_policy {
+            SafeBrowsingEnabled {
+              policy_options {mode: MANDATORY}
+              SafeBrowsingEnabled: false
+            }
+          }
+          chrome_policy {
+            SafeBrowsingExtendedReportingOptInAllowed {
+              policy_options {mode: MANDATORY}
+              SafeBrowsingExtendedReportingOptInAllowed: false
+            }
+          }
+        })");
   fetcher_ = net::URLFetcher::Create(
       0, PasswordProtectionService::GetPasswordProtectionRequestUrl(),
-      net::URLFetcher::POST, this);
+      net::URLFetcher::POST, this, traffic_annotation);
   data_use_measurement::DataUseUserData::AttachToFetcher(
       fetcher_.get(), data_use_measurement::DataUseUserData::SAFE_BROWSING);
   fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);