feedback: Only send bearer token if email is set.

Further, use the empty string for the email rather than
"anonymous_user", since the listnr UI treats an empty email field
specially and recognizes the report as being anonymous, but does not do
so for the string "anonymous_user".

BUG=chromium:1112012
TEST=browser tests and component tests
TEST=sent two feedback reports; one with email set and one without

Change-Id: I3fa1341151a8daae82a39d8b47b1276d6a7c1721
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335891
Commit-Queue: Miriam Zimmerman <mutexlox@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Ian Barkley-Yeung <iby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795285}

chrome/browser/extensions/api/feedback_private/feedback_browsertest.cc

@@ -139,8 +139,7 @@ IN_PROC_BROWSER_TEST_F(FeedbackTest, ShowLoginFeedback) {
   EXPECT_TRUE(bool_result);
 }
 
-// Tests that there's an option in the email drop down box with a value
-// 'anonymous_user'.
+// Tests that there's an option in the email drop down box with a value ''.
 IN_PROC_BROWSER_TEST_F(FeedbackTest, AnonymousUser) {
   WaitForExtensionViewsToLoad();
 
@@ -160,7 +159,7 @@ IN_PROC_BROWSER_TEST_F(FeedbackTest, AnonymousUser) {
       "  ((function() {"
       "      var options = $('user-email-drop-down').options;"
       "      for (var option in options) {"
-      "        if (options[option].value == 'anonymous_user')"
+      "        if (options[option].value == '')"
       "          return true;"
       "      }"
       "      return false;"

chrome/browser/resources/feedback/html/default.html

@@ -42,7 +42,7 @@
     <div id="user-email" class="text-field-container" hidden>
       <label id="user-email-label" i18n-content="userEmail"></label>
       <select id="user-email-drop-down" aria-labelledby="user-email-label">
-        <option id="anonymous-user-option" value="anonymous_user"
+        <option id="anonymous-user-option" value=""
                 i18n-content="anonymousUser"></option>
       </select>
     </div>

components/feedback/feedback_data.cc

@@ -136,7 +136,8 @@ void FeedbackData::SendReport() {
     PrepareReport(&feedback_data);
     auto post_body = std::make_unique<std::string>();
     feedback_data.SerializeToString(post_body.get());
-    uploader_->QueueReport(std::move(post_body));
+    uploader_->QueueReport(std::move(post_body),
+                           /*has_email=*/!user_email().empty());
   }
 }
 

components/feedback/feedback_data_unittest.cc

@@ -44,8 +44,19 @@ class MockUploader : public FeedbackUploader {
   // feedback::FeedbackUploader:
   void StartDispatchingReport() override { std::move(on_report_sent_).Run(); }
 
+  void QueueReport(std::unique_ptr<std::string> data, bool has_email) override {
+    report_had_email_ = has_email;
+    called_queue_report_ = true;
+    FeedbackUploader::QueueReport(std::move(data), has_email);
+  }
+
+  bool called_queue_report() const { return called_queue_report_; }
+  bool report_had_email() const { return report_had_email_; }
+
  private:
   base::OnceClosure on_report_sent_;
+  bool called_queue_report_ = false;
+  bool report_had_email_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(MockUploader);
 };
@@ -100,7 +111,23 @@ TEST_F(FeedbackDataTest, ReportSending) {
   data_->AttachAndCompressFileData(kFileData);
   Send();
   RunMessageLoop();
+  EXPECT_EQ(data_->user_email(), "");
+  EXPECT_TRUE(data_->IsDataComplete());
+  EXPECT_TRUE(uploader_.called_queue_report());
+  EXPECT_FALSE(uploader_.report_had_email());
+}
+
+TEST_F(FeedbackDataTest, ReportSendingWithEmail) {
+  data_->SetAndCompressHistograms(kHistograms);
+  data_->set_image(kImageData);
+  data_->AttachAndCompressFileData(kFileData);
+  data_->set_user_email("foo@bar.com");
+  Send();
+  RunMessageLoop();
+  EXPECT_EQ(data_->user_email(), "foo@bar.com");
   EXPECT_TRUE(data_->IsDataComplete());
+  EXPECT_TRUE(uploader_.called_queue_report());
+  EXPECT_TRUE(uploader_.report_had_email());
 }
 
 }  // namespace feedback

components/feedback/feedback_report.cc

@@ -13,6 +13,7 @@
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "components/feedback/proto/extension.pb.h"
 
 namespace feedback {
 
@@ -42,8 +43,10 @@ FeedbackReport::FeedbackReport(
     const base::FilePath& path,
     const base::Time& upload_at,
     std::unique_ptr<std::string> data,
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : reports_path_(path),
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    bool has_email)
+    : has_email_(has_email),
+      reports_path_(path),
       upload_at_(upload_at),
       data_(std::move(data)),
       reports_task_runner_(task_runner) {
@@ -61,8 +64,12 @@ FeedbackReport::FeedbackReport(
 FeedbackReport::FeedbackReport(
     base::FilePath path,
     std::unique_ptr<std::string> data,
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : file_(path), data_(std::move(data)), reports_task_runner_(task_runner) {}
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    bool has_email)
+    : file_(path),
+      has_email_(has_email),
+      data_(std::move(data)),
+      reports_task_runner_(task_runner) {}
 
 // static
 const char FeedbackReport::kCrashReportIdsKey[]  = "crash_report_ids";
@@ -88,9 +95,14 @@ void FeedbackReport::LoadReportsAndQueue(const base::FilePath& user_dir,
        name = enumerator.Next()) {
     auto data = std::make_unique<std::string>();
     if (ReadFileToString(name, data.get())) {
+      userfeedback::ExtensionSubmit parsed;
+      parsed.ParseFromString(*data);
+
+      bool has_email = parsed.common_data().has_user_email() &&
+                       !parsed.common_data().user_email().empty();
       callback.Run(base::MakeRefCounted<FeedbackReport>(
-          std::move(name), std::move(data),
-          base::ThreadTaskRunnerHandle::Get()));
+          std::move(name), std::move(data), base::ThreadTaskRunnerHandle::Get(),
+          has_email));
     }
   }
 }

components/feedback/feedback_report.h

@@ -32,13 +32,15 @@ class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> {
   FeedbackReport(const base::FilePath& path,
                  const base::Time& upload_at,
                  std::unique_ptr<std::string> data,
-                 scoped_refptr<base::SequencedTaskRunner> task_runner);
+                 scoped_refptr<base::SequencedTaskRunner> task_runner,
+                 bool has_email);
 
   // Creates a feedback report from an existing one on-disk at |path|, the
   // |upload_at| time should be set after construction.
   FeedbackReport(base::FilePath path,
                  std::unique_ptr<std::string> data,
-                 scoped_refptr<base::SequencedTaskRunner> task_runner);
+                 scoped_refptr<base::SequencedTaskRunner> task_runner,
+                 bool has_email);
 
   // The ID of the product specific data for the crash report IDs as stored by
   // the feedback server.
@@ -64,6 +66,7 @@ class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> {
   const base::Time& upload_at() const { return upload_at_; }
   void set_upload_at(const base::Time& time) { upload_at_ = time; }
   const std::string& data() const { return *data_; }
+  bool has_email() const { return has_email_; }
   scoped_refptr<base::SequencedTaskRunner> reports_task_runner() const {
     return reports_task_runner_;
   }
@@ -75,6 +78,9 @@ class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> {
   // Name of the file corresponding to this report.
   base::FilePath file_;
 
+  // True iff the report is being sent with an email.
+  const bool has_email_;
+
   base::FilePath reports_path_;
   base::Time upload_at_;  // Upload this report at or after this time.
   std::unique_ptr<std::string> data_;

components/feedback/feedback_uploader.cc

@@ -78,10 +78,11 @@ void FeedbackUploader::SetMinimumRetryDelayForTesting(base::TimeDelta delay) {
   g_minimum_retry_delay = delay;
 }
 
-void FeedbackUploader::QueueReport(std::unique_ptr<std::string> data) {
+void FeedbackUploader::QueueReport(std::unique_ptr<std::string> data,
+                                   bool has_email) {
   reports_queue_.emplace(base::MakeRefCounted<FeedbackReport>(
-      feedback_reports_path_, base::Time::Now(), std::move(data),
-      task_runner_));
+      feedback_reports_path_, base::Time::Now(), std::move(data), task_runner_,
+      has_email));
   UpdateUploadTimer();
 }
 
@@ -176,7 +177,9 @@ void FeedbackUploader::DispatchReport() {
                                  : variations::InIncognito::kNo,
       resource_request.get());
 
-  AppendExtraHeadersToUploadRequest(resource_request.get());
+  if (report_being_dispatched_->has_email()) {
+    AppendExtraHeadersToUploadRequest(resource_request.get());
+  }
 
   std::unique_ptr<network::SimpleURLLoader> simple_url_loader =
       network::SimpleURLLoader::Create(std::move(resource_request),

components/feedback/feedback_uploader.h

@@ -46,7 +46,10 @@ class FeedbackUploader : public KeyedService,
   static void SetMinimumRetryDelayForTesting(base::TimeDelta delay);
 
   // Queues a report for uploading.
-  void QueueReport(std::unique_ptr<std::string> data);
+  // |data|: The serialized userfeedback::ExtensionSubmit proto to send.
+  // |has_email|: True iff the user included their email address in the report.
+  // virtual for testing.
+  virtual void QueueReport(std::unique_ptr<std::string> data, bool has_email);
 
   // Re-queues an existing report from disk for uploading.
   void RequeueReport(scoped_refptr<FeedbackReport> report);

components/feedback/feedback_uploader_dispatch_unittest.cc

@@ -34,10 +34,28 @@ constexpr base::TimeDelta kTestRetryDelay =
 constexpr char kFeedbackPostUrl[] =
     "https://www.google.com/tools/feedback/chrome/__submit";
 
-void QueueReport(FeedbackUploader* uploader, const std::string& report_data) {
-  uploader->QueueReport(std::make_unique<std::string>(report_data));
+void QueueReport(FeedbackUploader* uploader,
+                 const std::string& report_data,
+                 bool has_email = true) {
+  uploader->QueueReport(std::make_unique<std::string>(report_data), has_email);
 }
 
+// Stand-in for the FeedbackUploaderChrome class that adds a fake bearer token
+// to the request.
+class MockFeedbackUploaderChrome : public FeedbackUploader {
+ public:
+  MockFeedbackUploaderChrome(
+      content::BrowserContext* context,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : FeedbackUploader(context, task_runner) {}
+
+  void AppendExtraHeadersToUploadRequest(
+      network::ResourceRequest* resource_request) override {
+    resource_request->headers.SetHeader(net::HttpRequestHeaders::kAuthorization,
+                                        "Bearer abcdefg");
+  }
+};
+
 }  // namespace
 
 class FeedbackUploaderDispatchTest : public ::testing::Test {
@@ -106,6 +124,36 @@ TEST_F(FeedbackUploaderDispatchTest, VariationHeaders) {
   variations::VariationsIdsProvider::GetInstance()->ResetForTesting();
 }
 
+// Test that the bearer token is present if there is an email address present in
+// the report.
+TEST_F(FeedbackUploaderDispatchTest, BearerTokenWithEmail) {
+  MockFeedbackUploaderChrome uploader(
+      context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+  uploader.set_url_loader_factory_for_test(shared_url_loader_factory());
+
+  test_url_loader_factory()->SetInterceptor(
+      base::BindLambdaForTesting([&](const network::ResourceRequest& request) {
+        EXPECT_TRUE(request.headers.HasHeader("Authorization"));
+      }));
+
+  QueueReport(&uploader, "test", /*has_email=*/true);
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(FeedbackUploaderDispatchTest, BearerTokenNoEmail) {
+  MockFeedbackUploaderChrome uploader(
+      context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+  uploader.set_url_loader_factory_for_test(shared_url_loader_factory());
+
+  test_url_loader_factory()->SetInterceptor(
+      base::BindLambdaForTesting([&](const network::ResourceRequest& request) {
+        EXPECT_FALSE(request.headers.HasHeader("Authorization"));
+      }));
+
+  QueueReport(&uploader, "test", /*has_email=*/false);
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(FeedbackUploaderDispatchTest, 204Response) {
   FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
   FeedbackUploader uploader(

components/feedback/feedback_uploader_unittest.cc

@@ -132,8 +132,8 @@ class FeedbackUploaderTest : public testing::Test {
         test_shared_loader_factory_, &context_);
   }
 
-  void QueueReport(const std::string& data) {
-    uploader_->QueueReport(std::make_unique<std::string>(data));
+  void QueueReport(const std::string& data, bool has_email = true) {
+    uploader_->QueueReport(std::make_unique<std::string>(data), has_email);
   }
 
   MockFeedbackUploader* uploader() const { return uploader_.get(); }