Add throttling to permission reporter

chrome/browser/safe_browsing/permission_reporter_unittest.cc

Index: chrome/browser/safe_browsing/permission_reporter_unittest.cc
diff --git a/chrome/browser/safe_browsing/permission_reporter_unittest.cc b/chrome/browser/safe_browsing/permission_reporter_unittest.cc
index ddb88731cf09d545bfc39415fabc71bcb4e40666..ac3e7e0baec436d8a6485976c566a45641a45ba6 100644
--- a/chrome/browser/safe_browsing/permission_reporter_unittest.cc
+++ b/chrome/browser/safe_browsing/permission_reporter_unittest.cc
@@ -7,6 +7,8 @@
 #include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/test/simple_test_clock.h"
+#include "base/time/time.h"
 #include "chrome/common/safe_browsing/permission_report.pb.h"
 #include "components/variations/active_field_trials.h"
 #include "content/public/browser/permission_type.h"
@@ -24,13 +26,13 @@ const char kPermissionActionReportingUploadUrl[] =
     "http://safebrowsing.googleusercontent.com/safebrowsing/clientreport/"
     "permission-action";
 
-const char kDummyOrigin[] = "http://example.test/";
-const PermissionType kDummyPermission = PermissionType::GEOLOCATION;
+const int kMaximumReportsPerOriginPerPermissionPerMinute = 5;
+
+const char kDummyOriginOne[] = "http://example.test/";
+const char kDummyOriginTwo[] = "http://example2.test/";
+const PermissionType kDummyPermissionOne = PermissionType::GEOLOCATION;
+const PermissionType kDummyPermissionTwo = PermissionType::NOTIFICATIONS;
 const PermissionAction kDummyAction = GRANTED;
-const PermissionReport::PermissionType kDummyPermissionReportPermission =
-    PermissionReport::GEOLOCATION;
-const PermissionReport::Action kDummyPermissionReportAction =
-    PermissionReport::GRANTED;
 
 const char kDummyTrialOne[] = "trial one";
 const char kDummyGroupOne[] = "group one";
@@ -50,21 +52,32 @@ struct base::Feature kFeatureOffByDefault {
 // A mock ReportSender that keeps track of the last report sent.
 class MockReportSender : public net::ReportSender {
  public:
-  MockReportSender() : net::ReportSender(nullptr, DO_NOT_SEND_COOKIES) {}
+  MockReportSender() : net::ReportSender(nullptr, DO_NOT_SEND_COOKIES) {
+    number_of_new_reports_ = 0;
+  }
+
   ~MockReportSender() override {}
 
   void Send(const GURL& report_uri, const std::string& report) override {
     latest_report_uri_ = report_uri;
     latest_report_ = report;
+    number_of_new_reports_++;
   }
 
   const GURL& latest_report_uri() { return latest_report_uri_; }
 
   const std::string& latest_report() { return latest_report_; }
 
+  int NumberOfNewReports() {

[raymes, 2016/06/30 01:30:19]

nit: GetAndResetNumberReports

[stefanocs, 2016/06/30 04:14:27]

Done.

+    int number_of_new_reports = number_of_new_reports_;
+    number_of_new_reports_ = 0;
+    return number_of_new_reports;
+  }
+
  private:
   GURL latest_report_uri_;
   std::string latest_report_;
+  int number_of_new_reports_;
 
   DISALLOW_COPY_AND_ASSIGN(MockReportSender);
 };
@@ -74,28 +87,33 @@ class MockReportSender : public net::ReportSender {
 class PermissionReporterTest : public ::testing::Test {
  protected:
   PermissionReporterTest()
-      : mock_report_sender_(new MockReportSender()),
+      : mock_report_sender_(new MockReportSender),
+        clock_(new base::SimpleTestClock),
         permission_reporter_(

[raymes, 2016/06/30 01:30:19]

We should probably create/reset this in a SetUp method to ensure the state gets
reset for every test.

[stefanocs, 2016/06/30 04:14:27]

Done.

-            new PermissionReporter(base::WrapUnique(mock_report_sender_))) {}
+            new PermissionReporter(base::WrapUnique(mock_report_sender_),
+                                   base::WrapUnique(clock_))) {}
 
   // Owned by |permission_reporter_|.
   MockReportSender* mock_report_sender_;
 
+  // Owned by |permission_reporter_|.
+  base::SimpleTestClock* clock_;
+
   std::unique_ptr<PermissionReporter> permission_reporter_;
 };
 
 // Test that PermissionReporter::SendReport sends a serialized report string to
 // SafeBrowsing CSD servers.
 TEST_F(PermissionReporterTest, SendReport) {
-  permission_reporter_->SendReport(GURL(kDummyOrigin), kDummyPermission,
+  permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
                                    kDummyAction);
 
   PermissionReport permission_report;
   ASSERT_TRUE(
       permission_report.ParseFromString(mock_report_sender_->latest_report()));
-  EXPECT_EQ(kDummyPermissionReportPermission, permission_report.permission());
-  EXPECT_EQ(kDummyPermissionReportAction, permission_report.action());
-  EXPECT_EQ(kDummyOrigin, permission_report.origin());
+  EXPECT_EQ(PermissionReport::GEOLOCATION, permission_report.permission());
+  EXPECT_EQ(PermissionReport::GRANTED, permission_report.action());
+  EXPECT_EQ(kDummyOriginOne, permission_report.origin());
 #if defined(OS_ANDROID)
   EXPECT_EQ(PermissionReport::ANDROID_PLATFORM,
             permission_report.platform_type());
@@ -140,7 +158,7 @@ TEST_F(PermissionReporterTest, SendReportWithFieldTrials) {
   EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_one->trial_name()));
   EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_two->trial_name()));
 
-  permission_reporter_->SendReport(GURL(kDummyOrigin), kDummyPermission,
+  permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
                                    kDummyAction);
 
   PermissionReport permission_report;
@@ -162,4 +180,39 @@ TEST_F(PermissionReporterTest, SendReportWithFieldTrials) {
   EXPECT_EQ(0U, expected_group_ids.size());
 }
 
+// Test that PermissionReporter::IsAllowedToSend returns true only when the
+// number of reports sent in the last one minute per origin per permission is
+// under a threshold.
+TEST_F(PermissionReporterTest, IsAllowedToSend) {
+  EXPECT_EQ(0, mock_report_sender_->NumberOfNewReports());
+
+  int reports_to_send = kMaximumReportsPerOriginPerPermissionPerMinute;
+  while (reports_to_send--)
+    permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
+                                     kDummyAction);
+  EXPECT_EQ(5, mock_report_sender_->NumberOfNewReports());
+
+  permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
+                                   kDummyAction);
+  EXPECT_EQ(0, mock_report_sender_->NumberOfNewReports());
+
+  permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionTwo,
+                                   kDummyAction);
+  EXPECT_EQ(1, mock_report_sender_->NumberOfNewReports());
+
+  permission_reporter_->SendReport(GURL(kDummyOriginTwo), kDummyPermissionOne,
+                                   kDummyAction);
+  EXPECT_EQ(1, mock_report_sender_->NumberOfNewReports());
+
+  clock_->Advance(base::TimeDelta::FromMinutes(1));
+  permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
+                                   kDummyAction);
+  EXPECT_EQ(0, mock_report_sender_->NumberOfNewReports());
+
+  clock_->Advance(base::TimeDelta::FromMicroseconds(1));
+  permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
+                                   kDummyAction);
+  EXPECT_EQ(1, mock_report_sender_->NumberOfNewReports());

[raymes, 2016/06/30 01:30:19]

Maybe have one more test where we add the reports at different times over a
minute and test that only some of them get evicted when advancing time?

[stefanocs, 2016/06/30 04:14:27]

Done.

+}
+
 }  // namespace safe_browsing