Move AnonymizerTool from Single*LogSource to LogSourceAccessManager

Instead of each log source having to handle anonymization, just let the
feedbackPrivate API handle it.

Bug: 787554
Change-Id: I8e25235fe2aa2f01aeed4e32c4f23aa925712367
Reviewed-on: https://chromium-review.googlesource.com/806681
Commit-Queue: Simon Que <sque@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521508}

chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.cc

@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
-#include "components/feedback/anonymizer_tool.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace system_logs {
@@ -65,10 +64,8 @@ void SingleDebugDaemonLogSource::OnFetchComplete(
   // DebugDaemonClient, which does not use the SystemLogsResponse alias.
   auto response = std::make_unique<SystemLogsResponse>();
   // Return an empty result if the call to GetLog() failed.
-  if (result.has_value()) {
-    response->emplace(log_name,
-                      feedback::AnonymizerTool().Anonymize(result.value()));
-  }
+  if (result.has_value())
+    response->emplace(log_name, result.value());
 
   callback.Run(std::move(response));
 }

chrome/browser/chromeos/system_logs/single_log_file_log_source.cc

@@ -169,8 +169,7 @@ void SingleLogFileLogSource::ReadFile(size_t num_rotations_allowed,
   num_bytes_read_ += size_read;
 
   // Pass it back to the callback.
-  AppendToSystemLogsResponse(result, source_name(),
-                             anonymizer_.Anonymize(result_string));
+  AppendToSystemLogsResponse(result, source_name(), result_string);
 
   // If the file was rotated, close the file handle and call this function
   // again, to read from the new file.

chrome/browser/chromeos/system_logs/single_log_file_log_source.h

@@ -11,7 +11,6 @@
 #include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "components/feedback/anonymizer_tool.h"
 #include "components/feedback/system_logs/system_logs_source.h"
 
 namespace base {
@@ -97,9 +96,6 @@ class SingleLogFileLogSource : public SystemLogsSource {
   // was originally opened for reading.
   ino_t file_inode_;
 
-  // For removing PII from log results.
-  feedback::AnonymizerTool anonymizer_;
-
   base::WeakPtrFactory<SingleLogFileLogSource> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SingleLogFileLogSource);

chrome/browser/chromeos/system_logs/single_log_file_log_source_unittest.cc

@@ -270,32 +270,6 @@ TEST_F(SingleLogFileLogSourceTest, IncompleteLines) {
   EXPECT_EQ("Goodbye world\n", latest_response());
 }
 
-TEST_F(SingleLogFileLogSourceTest, Anonymize) {
-  InitializeSource(SingleLogFileLogSource::SupportedSource::kUiLatest);
-
-  EXPECT_TRUE(AppendToFile(base::FilePath("ui/ui.LATEST"),
-                           "My MAC address is: 11:22:33:44:55:66\n"));
-  FetchFromSource();
-
-  EXPECT_EQ(1, num_callback_calls());
-  EXPECT_EQ("My MAC address is: 11:22:33:00:00:01\n", latest_response());
-
-  // Suppose the write operation is not atomic, and the MAC address is written
-  // across two separate writes.
-  EXPECT_TRUE(AppendToFile(base::FilePath("ui/ui.LATEST"),
-                           "Your MAC address is: AB:88:C"));
-  FetchFromSource();
-
-  EXPECT_EQ(2, num_callback_calls());
-  EXPECT_EQ("", latest_response());
-
-  EXPECT_TRUE(AppendToFile(base::FilePath("ui/ui.LATEST"), "D:99:EF:77\n"));
-  FetchFromSource();
-
-  EXPECT_EQ(3, num_callback_calls());
-  EXPECT_EQ("Your MAC address is: ab:88:cd:00:00:02\n", latest_response());
-}
-
 TEST_F(SingleLogFileLogSourceTest, HandleLogFileRotation) {
   InitializeSource(SingleLogFileLogSource::SupportedSource::kMessages);
 

extensions/browser/api/feedback_private/feedback_private_api_chromeos_unittest.cc

@@ -186,6 +186,31 @@ TEST_F(FeedbackPrivateApiUnittest, ReadLogSourceIncremental) {
   EXPECT_NE("", RunReadLogSourceFunctionWithError(params));
 }
 
+TEST_F(FeedbackPrivateApiUnittest, Anonymize) {
+  const TimeDelta timeout(TimeDelta::FromMilliseconds(0));
+  LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);
+
+  ReadLogSourceParams params;
+  params.source = api::feedback_private::LOG_SOURCE_MESSAGES;
+  params.incremental = true;
+
+  int result_reader_id = 0;
+  std::string result_string;
+  // Skip over all the alphabetic results, to test anonymization of the
+  // subsequent MAC address.
+  for (int i = 0; i < 26; ++i) {
+    EXPECT_TRUE(
+        RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+    EXPECT_GT(result_reader_id, 0);
+    params.reader_id = std::make_unique<int>(result_reader_id);
+  }
+
+  EXPECT_TRUE(
+      RunReadLogSourceFunction(params, &result_reader_id, &result_string));
+  EXPECT_EQ(*params.reader_id, result_reader_id);
+  EXPECT_EQ("11:22:33:00:00:01", result_string);
+}
+
 TEST_F(FeedbackPrivateApiUnittest, ReadLogSourceMultipleSources) {
   const TimeDelta timeout(TimeDelta::FromMilliseconds(0));
   LogSourceAccessManager::SetRateLimitingTimeoutForTesting(&timeout);

extensions/browser/api/feedback_private/feedback_private_api_unittest_base_chromeos.cc

@@ -28,6 +28,9 @@ using api::feedback_private::LogSource;
 using system_logs::SystemLogsResponse;
 using system_logs::SystemLogsSource;
 
+// A fake MAC address used to test anonymization.
+const char kDummyMacAddress[] = "11:22:33:44:55:66";
+
 std::unique_ptr<KeyedService> ApiResourceManagerTestFactory(
     content::BrowserContext* context) {
   return std::make_unique<ApiResourceManager<LogSourceResource>>(context);
@@ -43,15 +46,12 @@ class TestSingleLogSource : public SystemLogsSource {
   ~TestSingleLogSource() override = default;
 
   // Fetch() will return a single different string each time, in the following
-  // sequence: "a", " bb", "  ccc", until 25 spaces followed by 26 z's. Will
-  // never return an empty result.
+  // sequence: "a", " bb", "  ccc", until 25 spaces followed by 26 z's. After
+  // that, it returns |kDummyMacAddress| before repeating the entire process.
+  // It will never return an empty result.
   void Fetch(const system_logs::SysLogsSourceCallback& callback) override {
-    int count_modulus = call_count_ % kNumCharsToIterate;
-    std::string result =
-        std::string(count_modulus, ' ') +
-        std::string(count_modulus + 1, kInitialChar + count_modulus);
+    std::string result = GetNextLogResult();
     DCHECK_GT(result.size(), 0U);
-    ++call_count_;
 
     auto result_map = std::make_unique<SystemLogsResponse>();
     result_map->emplace("", result);
@@ -64,6 +64,18 @@ class TestSingleLogSource : public SystemLogsSource {
   }
 
  private:
+  std::string GetNextLogResult() {
+    if (call_count_ == kNumCharsToIterate) {
+      call_count_ = 0;
+      return kDummyMacAddress;
+    }
+    std::string result =
+        std::string(call_count_, ' ') +
+        std::string(call_count_ + 1, kInitialChar + call_count_);
+    ++call_count_;
+    return result;
+  }
+
   // Iterate over the whole lowercase alphabet, starting from 'a'.
   const int kNumCharsToIterate = 26;
   const char kInitialChar = 'a';

extensions/browser/api/feedback_private/log_source_access_manager.cc

@@ -57,6 +57,7 @@ void GetLogLinesFromSystemLogsResponse(const SystemLogsResponse& response,
 LogSourceAccessManager::LogSourceAccessManager(content::BrowserContext* context)
     : context_(context),
       tick_clock_(std::make_unique<base::DefaultTickClock>()),
+      anonymizer_(std::make_unique<feedback::AnonymizerTool>()),
       weak_factory_(this) {}
 
 LogSourceAccessManager::~LogSourceAccessManager() {}
@@ -121,6 +122,10 @@ void LogSourceAccessManager::OnFetchComplete(
   result.reader_id = delete_resource ? kInvalidResourceId : resource_id;
 
   GetLogLinesFromSystemLogsResponse(*response, &result.log_lines);
+
+  for (std::string& line : result.log_lines)
+    line = anonymizer_->Anonymize(line);
+
   if (delete_resource) {
     // This should also remove the entry from |sources_|.
     ApiResourceManager<LogSourceResource>::Get(context_)->Remove(extension_id,

extensions/browser/api/feedback_private/log_source_access_manager.h

@@ -16,6 +16,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
+#include "components/feedback/anonymizer_tool.h"
 #include "components/feedback/system_logs/system_logs_source.h"
 #include "content/public/browser/browser_context.h"
 #include "extensions/browser/api/feedback_private/access_rate_limiter.h"
@@ -150,6 +151,9 @@ class LogSourceAccessManager {
   // Can override the default clock for testing.
   std::unique_ptr<base::TickClock> tick_clock_;
 
+  // For removing PII from log strings from log sources.
+  std::unique_ptr<feedback::AnonymizerTool> anonymizer_;
+
   base::WeakPtrFactory<LogSourceAccessManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(LogSourceAccessManager);