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 5c96a5fc37e37c9c5b44c8c97fb6f7727f8997f9..c0738dd7bdb375f4049ba183a0057bb29a72ed40 100644
--- a/chrome/browser/safe_browsing/permission_reporter_unittest.cc
+++ b/chrome/browser/safe_browsing/permission_reporter_unittest.cc
@@ -5,6 +5,8 @@
 #include "chrome/browser/safe_browsing/permission_reporter.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/test/simple_test_clock.h"
+#include "base/time/time.h"
 #include "chrome/common/safe_browsing/permission_report.pb.h"
 #include "content/public/browser/permission_type.h"
 #include "net/url_request/report_sender.h"
@@ -20,10 +22,14 @@ 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 =
+const PermissionReport::PermissionType kDummyPermissionReportPermissionOne =

[raymes, 2016/06/29 07:09:17]

nit: maybe we just inline this? Sometimes it's a bit easier to read than having
a bunch of constants all defined at the top.

[stefanocs, 2016/06/30 00:24:51]

Done.

     PermissionReport::GEOLOCATION;
 const PermissionReport::Action kDummyPermissionReportAction =

[raymes, 2016/06/29 07:09:17]

nit: same here.

[stefanocs, 2016/06/30 00:24:51]

Done.

     PermissionReport::GRANTED;
@@ -55,31 +61,63 @@ 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_(
-            new PermissionReporter(base::WrapUnique(mock_report_sender_))) {}
+            new PermissionReporter(base::WrapUnique(mock_report_sender_),
+                                   base::WrapUnique(clock_))) {}
+
+  bool IsAllowedToSend(content::PermissionType permission, const GURL& origin) {
+    return permission_reporter_->IsAllowedToSend(permission, origin);
+  }
 
   // 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(kDummyPermissionReportPermissionOne,
+            permission_report.permission());
   EXPECT_EQ(kDummyPermissionReportAction, permission_report.action());
-  EXPECT_EQ(kDummyOrigin, permission_report.origin());
+  EXPECT_EQ(kDummyOriginOne, permission_report.origin());
 
   EXPECT_EQ(GURL(kPermissionActionReportingUploadUrl),
             mock_report_sender_->latest_report_uri());
 }
 
+// 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_TRUE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginOne)));
+
+  int reports_to_send = kMaximumReportsPerOriginPerPermissionPerMinute;
+  while (reports_to_send--)
+    permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
+                                     kDummyAction);
+
+  EXPECT_FALSE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginOne)));
+  EXPECT_TRUE(IsAllowedToSend(kDummyPermissionTwo, GURL(kDummyOriginOne)));
+  EXPECT_TRUE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginTwo)));
+
+  clock_->Advance(base::TimeDelta::FromMinutes(1));
+  EXPECT_FALSE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginOne)));
+
+  clock_->Advance(base::TimeDelta::FromMicroseconds(1));
+  EXPECT_TRUE(IsAllowedToSend(kDummyPermissionOne, GURL(kDummyOriginOne)));

[raymes, 2016/06/29 07:09:17]

Instead of calling IsAllowedToSend directly, I think it would be better to test
the public interface by just sending reports. We can use the MockReportSender to
check if the reports actually got sent (we don't need to check their contents
though).

[stefanocs, 2016/06/30 00:24:51]

Done.

+}
+
 }  // namespace safe_browsing