Add read-only access to POST data for webRequest's onBeforeRequest

chrome/browser/extensions/api/web_request/web_request_api_unittest.cc

Index: chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
diff --git a/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc b/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
index fc3dc17c564494ba300ae51a06d757d2e47148ad..eef3bfc961a0ca78ebd02f0d050fdd40774d50b3 100644
--- a/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
+++ b/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
@@ -73,6 +73,32 @@ bool Contains(const Collection& collection, const Key& key) {
       collection.end();
 }
 
+// Inspects the data attached to the |message| and tries to extract its
+// "postData" section into |post_data|.
+void GetPostData(IPC::Message* message, std::string* post_data, bool* success) {

[battre, 2012/07/16 17:39:45]

nit: return boolean instead of passing bool* success?

[vabr (Chromium), 2012/07/17 11:11:11]

That would not work, ASSERT_* cannot be used in non-void functions.

[battre, 2012/07/17 12:46:07]

You can replace all ASSERT_* statements with CHECK statements because they would
indicate that the input is invalid.

[vabr (Chromium), 2012/07/17 13:04:52]

Done.

+  ASSERT_TRUE(success != NULL);
+  *success = false;
+  ASSERT_EQ(message->type(), ExtensionMsg_MessageInvoke::ID);

[battre, 2012/07/16 17:39:45]

as message is provided and not generated by the tested code, I would use a CHECK
here.

[vabr (Chromium), 2012/07/17 11:11:11]

Done.
QUESTION:
Do you want me to change this also for the first line of TestIPCSender::Send() ?

+  ExtensionMsg_MessageInvoke::Param param;
+  Value* temp_value = NULL;
+  ASSERT_TRUE(ExtensionMsg_MessageInvoke::Read(message, &param));
+  ASSERT_GE(param.c.GetSize(), static_cast<size_t>(2));
+  ASSERT_TRUE(param.c.Get(1, &temp_value));
+  std::string args;
+  ASSERT_TRUE(temp_value->GetAsString(&args));

[battre, 2012/07/16 17:39:45]

nit: new line after 88?

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+  const char kPostDataHead[] = "\"postData\":{";
+  size_t post_data_start = args.find(kPostDataHead);
+  if (post_data_start == std::string::npos)
+    return;
+  post_data_start += strlen(kPostDataHead);

[battre, 2012/07/16 17:39:45]

nit: new line after 93?

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+  const size_t post_data_end = args.find("}", post_data_start);
+  ASSERT_NE(post_data_end, std::string::npos);
+  const size_t post_data_length = post_data_end - post_data_start;
+  *post_data = std::string(args, post_data_start, post_data_length);
+  *success = true;
+  return;
+}
+
 }  // namespace
 
 // A mock event router that responds to events with a pre-arranged queue of
@@ -133,6 +159,8 @@ class ExtensionWebRequestTest : public testing::Test {
     context_->Init();
   }
 
+  void FireURLRequestWithPostData(const char* content_type, const char* bytes);
+
   MessageLoopForIO message_loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread io_thread_;
@@ -398,7 +426,94 @@ TEST_F(ExtensionWebRequestTest, SimulateChancelWhileBlocked) {
   ExtensionWebRequestEventRouter::GetInstance()->RemoveEventListener(
       &profile_, extension_id, kEventName + "/1");
   ExtensionWebRequestEventRouter::GetInstance()->RemoveEventListener(
-        &profile_, extension_id, kEventName2 + "/1");
+      &profile_, extension_id, kEventName2 + "/1");
+}
+
+// Fires a URLRequest with the specified |content_type|. Method will be "POST"
+// and the data will be |bytes|.

[battre, 2012/07/16 17:39:45]

the comment goes to the declaration above.

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+void ExtensionWebRequestTest::FireURLRequestWithPostData(
+    const char* content_type,
+    const char* bytes) {
+  // The request URL can be arbitrary but must have a HTTP or HTTPS scheme.

[battre, 2012/07/16 17:39:45]

nit: s/a HTTP/an HTTP/

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+  GURL request_url("http://www.example.com");
+  net::URLRequest request(request_url, &delegate_, context_.get());
+  request.set_method("POST");
+  request.SetExtraRequestHeaderByName("Content-Type", content_type, true);
+  request.AppendBytesToUpload(bytes, strlen(bytes));
+  ipc_sender_.PushTask(base::Bind(&base::DoNothing));
+  request.Start();
+}

[battre, 2012/07/16 17:39:45]

nit: empty line

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+TEST_F(ExtensionWebRequestTest, AccessPostData) {
+  // We verify that POST data are accessible to OnBeforeRequest listeners.
+  // Construct the test data.
+#define kBoundary "THIS_IS_A_BOUNDARY"
+#define kBlock "--" kBoundary "\r\n" \
+    "Content-Disposition: form-data; name=\"text\"\r\n" \
+    "\r\n" \
+    "test text\r\n" \
+    "--" kBoundary "--"
+  // POST data input.
+  const char kMultipartBytes[] = kBlock;
+  // POST data output.
+  const char kResultString[] = "\"text\":[\"test text\"]";
+  // Header.
+  const char kMultipart[] = "multipart/form-data; boundary=" kBoundary;
+#undef kBlock
+#undef kBoundary
+  bool kSuccessExpected[] = {true, false};
+
+  // Set up a dummy extension name.
+  ExtensionWebRequestEventRouter::RequestFilter filter;
+  std::string extension_id("1");
+  int extra_info_spec_post =
+      ExtensionWebRequestEventRouter::ExtraInfoSpec::BLOCKING |
+      ExtensionWebRequestEventRouter::ExtraInfoSpec::REQUEST_POST_DATA;
+  int extra_info_spec_no_post =
+      ExtensionWebRequestEventRouter::ExtraInfoSpec::BLOCKING;
+
+  // Subscribe to OnBeforeRequest with POST data requirement.
+  const std::string kEventName(keys::kOnBeforeRequest);
+  base::WeakPtrFactory<TestIPCSender> ipc_sender_factory(&ipc_sender_);
+  ExtensionWebRequestEventRouter::GetInstance()->AddEventListener(
+    &profile_, extension_id, extension_id, kEventName, kEventName + "/1",

[battre, 2012/07/16 17:39:45]

nit: +2 spaces

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+    filter, extra_info_spec_post, ipc_sender_factory.GetWeakPtr());
+
+  FireURLRequestWithPostData(kMultipart, kMultipartBytes);
+
+  MessageLoop::current()->RunAllPending();
+
+  // Now remove the requirement of POST data.
+  ExtensionWebRequestEventRouter::GetInstance()->RemoveEventListener(
+      &profile_, extension_id, kEventName + "/1");
+  ExtensionWebRequestEventRouter::GetInstance()->AddEventListener(
+    &profile_, extension_id, extension_id, kEventName, kEventName + "/1",

[battre, 2012/07/16 17:39:45]

+2 spaces

[vabr (Chromium), 2012/07/17 11:11:11]

Done.

+    filter, extra_info_spec_no_post, ipc_sender_factory.GetWeakPtr());
+
+  FireURLRequestWithPostData(kMultipart, kMultipartBytes);
+
+  MessageLoop::current()->RunAllPending();
+
+  IPC::Message* message = NULL;
+  std::string post_data;
+  TestIPCSender::SentMessages::const_iterator i = ipc_sender_.sent_begin();
+  for (size_t test = 0; test < arraysize(kSuccessExpected); ++test) {
+    EXPECT_NE(i, ipc_sender_.sent_end());
+    message = (i++)->get();
+    bool post_data_found;
+    GetPostData(message, &post_data, &post_data_found);
+    if (kSuccessExpected[test]) {
+      EXPECT_TRUE(post_data_found);

[battre, 2012/07/16 17:39:45]

if you write
EXPECT_TRUE(post_data_found) << test;
an error message is slightly simpler to analyze.

[vabr (Chromium), 2012/07/17 11:11:11]

Done. Thanks for teaching me this. :)

+      EXPECT_EQ(kResultString, post_data);
+    } else {
+      EXPECT_FALSE(post_data_found);
+    }
+  }
+
+  EXPECT_TRUE(i == ipc_sender_.sent_end());

[battre, 2012/07/16 17:39:45]

Does EXPECT_EQ work?

[vabr (Chromium), 2012/07/17 11:11:11]

Done. It seems to work, just as the EXPECT_NE a couple of lines above. I don't
know why I started with EXPECT_TRUE.

+
+  // Clean-up.
+  ExtensionWebRequestEventRouter::GetInstance()->RemoveEventListener(
+      &profile_, extension_id, kEventName + "/1");
 }
 
 struct HeaderModificationTest_Header {
@@ -663,6 +778,10 @@ TEST_F(ExtensionWebRequestTest, InitFromValue) {
       "asyncBlocking",
       true,
       ExtensionWebRequestEventRouter::ExtraInfoSpec::ASYNC_BLOCKING);
+  TestInitFromValue(
+      "requestPostData",
+      true,
+      ExtensionWebRequestEventRouter::ExtraInfoSpec::REQUEST_POST_DATA);
 
   // Multiple valid values are bitwise-or'ed.
   TestInitFromValue(