Include hash on sync attachment uploads to ensure data integrity.

This change is one in a series of changes designed to ensure end-to-end
data integrity of attachments.  See referenced bug for details.

Simplify AttachmentUploaderImplTest by moving duplicated test code into
private UploadAndRespondWith method.

Update TODO comment to reflect the fact that URLFetcher automatically
adds a Content-Length header.  Update test cases accordingly.

TBR=kinuko

BUG=418649,371521

Review URL: https://codereview.chromium.org/611113002

Cr-Commit-Position: refs/heads/master@{#298274}

6 files changed, 107 insertions, 64 deletions

diff --git a/sync/BUILD.gn b/sync/BUILD.gn
index d32575b..9b187cf 100644
--- a/sync/BUILD.gn
+++ b/sync/BUILD.gn
@@ -399,6 +399,7 @@ source_set("sync_core") {
     "//google_apis",
     "//net",
     "//sql",
+    "//third_party/leveldatabase",
     "//third_party/zlib",
     "//url",
     "//sync/protocol",
diff --git a/sync/internal_api/attachments/DEPS b/sync/internal_api/attachments/DEPS
index 50828ba..6fbaa4d 100644
--- a/sync/internal_api/attachments/DEPS
+++ b/sync/internal_api/attachments/DEPS
@@ -1,3 +1,4 @@
 include_rules = [
   "+sync/api/attachments",
+  "+third_party/leveldatabase",
 ]
diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc
index 5ca3304..9c7276f 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl.cc
@@ -4,9 +4,14 @@
 
 #include "sync/internal_api/public/attachments/attachment_uploader_impl.h"
 
+#include "base/base64.h"
 #include "base/bind.h"
+#include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_piece.h"
+#include "base/strings/stringprintf.h"
+#include "base/sys_byteorder.h"
 #include "base/threading/non_thread_safe.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "net/base/load_flags.h"
@@ -15,6 +20,7 @@
 #include "net/url_request/url_fetcher_delegate.h"
 #include "sync/api/attachments/attachment.h"
 #include "sync/protocol/sync.pb.h"
+#include "third_party/leveldatabase/src/util/crc32c.h"
 
 namespace {
 
@@ -201,12 +207,15 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess(
   fetcher_->SetUploadData(kContentType, upload_content);
   const std::string auth_header("Authorization: Bearer " + access_token_);
   fetcher_->AddExtraRequestHeader(auth_header);
+  // TODO(maniscalco): Consider computing the hash once and storing the value as
+  // a new field in the Attachment object to avoid recomputing when an upload
+  // fails and is retried (bug 417794).
+  fetcher_->AddExtraRequestHeader(ComputeHashHeader(memory));
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
                          net::LOAD_DO_NOT_SEND_COOKIES |
                          net::LOAD_DISABLE_CACHE);
-  // TODO(maniscalco): Set an appropriate headers (User-Agent, Content-type, and
-  // Content-length) on the request and include the content's MD5,
-  // AttachmentId's unique_id and the "sync birthday" (bug 371521).
+  // TODO(maniscalco): Set appropriate headers (e.g. User-Agent) on the request
+  // and include the "sync birthday" (bug 371521).
   fetcher_->Start();
 }
 
@@ -334,4 +343,18 @@ void AttachmentUploaderImpl::OnUploadStateStopped(const UniqueId& unique_id) {
   }
 }
 
+std::string AttachmentUploaderImpl::ComputeHashHeader(
+    const scoped_refptr<base::RefCountedMemory>& memory) {
+  // Generate an X-Goog-Hash header containing the object's crc32c, big-endian,
+  // base64 encoded.  See also
+  // https://cloud.google.com/storage/docs/reference-headers#xgooghash
+  const uint32_t crc32c_big_endian = base::HostToNet32(
+      leveldb::crc32c::Value(memory->front_as<char>(), memory->size()));
+  const base::StringPiece raw(reinterpret_cast<const char*>(&crc32c_big_endian),
+                              sizeof(crc32c_big_endian));
+  std::string encoded;
+  base::Base64Encode(raw, &encoded);
+  return base::StringPrintf("X-Goog-Hash: crc32c=%s", encoded.c_str());
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
index e682253..9bfbb4e75 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
@@ -33,6 +33,10 @@ const char kAttachmentData[] = "some data";
 const char kAccountId[] = "some-account-id";
 const char kAccessToken[] = "some-access-token";
 const char kAuthorization[] = "Authorization";
+const char kContentLength[] = "Content-Length";
+const char kContentType[] = "Content-Type";
+const char kContentTypeValue[] = "application/octet-stream";
+const char kXGoogHash[] = "X-Goog-Hash";
 const char kAttachments[] = "/attachments/";
 
 }  // namespace
@@ -184,6 +188,11 @@ class AttachmentUploaderImplTest : public testing::Test,
   // Run the message loop until UploadDone has been invoked |num_uploads| times.
   void RunAndWaitFor(int num_uploads);
 
+  // Upload an attachment and have the server respond with |status_code|.
+  //
+  // Returns the attachment that was uploaded.
+  Attachment UploadAndRespondWith(const net::HttpStatusCode& status_code);
+
   scoped_ptr<AttachmentUploader>& uploader();
   const AttachmentUploader::UploadCallback& upload_callback() const;
   std::vector<HttpRequest>& http_requests_received();
@@ -297,6 +306,17 @@ void AttachmentUploaderImplTest::RunAndWaitFor(int num_uploads) {
   }
 }
 
+Attachment AttachmentUploaderImplTest::UploadAndRespondWith(
+    const net::HttpStatusCode& status_code) {
+  token_service().AddAccount(kAccountId);
+  request_handler().SetStatusCode(status_code);
+  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
+  some_data->data() = kAttachmentData;
+  Attachment attachment = Attachment::Create(some_data);
+  uploader()->UploadAttachment(attachment, upload_callback());
+  return attachment;
+}
+
 scoped_ptr<AttachmentUploader>& AttachmentUploaderImplTest::uploader() {
   return uploader_;
 }
@@ -419,13 +439,7 @@ TEST_F(AttachmentUploaderImplTest, GetURLForAttachmentId_PathAndSlash) {
 // Token is requested, token is returned, HTTP request is made, attachment is
 // received by server.
 TEST_F(AttachmentUploaderImplTest, UploadAttachment_HappyCase) {
-  token_service().AddAccount(kAccountId);
-  request_handler().SetStatusCode(net::HTTP_OK);
-
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kAttachmentData;
-  Attachment attachment = Attachment::Create(some_data);
-  uploader()->UploadAttachment(attachment, upload_callback());
+  Attachment attachment = UploadAndRespondWith(net::HTTP_OK);
 
   // Run until the done callback is invoked.
   RunAndWaitFor(1);
@@ -445,23 +459,45 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_HappyCase) {
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
   EXPECT_EQ(kAttachmentData, http_request.content);
-  const std::string header_name(kAuthorization);
-  const std::string header_value(std::string("Bearer ") + kAccessToken);
+}
+
+// Verify the request contains the appropriate headers.
+TEST_F(AttachmentUploaderImplTest, UploadAttachment_Headers) {
+  Attachment attachment = UploadAndRespondWith(net::HTTP_OK);
+
+  // Run until the done callback is invoked.
+  RunAndWaitFor(1);
+
+  // See that the done callback was invoked with the right arguments.
+  ASSERT_EQ(1U, upload_results().size());
+  EXPECT_EQ(AttachmentUploader::UPLOAD_SUCCESS, upload_results()[0]);
+  ASSERT_EQ(1U, attachment_ids().size());
+  EXPECT_EQ(attachment.GetId(), attachment_ids()[0]);
+
+  // See that the HTTP server received one request.
+  ASSERT_EQ(1U, http_requests_received().size());
+  const HttpRequest& http_request = http_requests_received().front();
+
+  const std::string auth_header_name(kAuthorization);
+  const std::string auth_header_value(std::string("Bearer ") + kAccessToken);
+
+  EXPECT_THAT(
+      http_request.headers,
+      testing::Contains(testing::Pair(kAuthorization, auth_header_value)));
+  EXPECT_THAT(http_request.headers,
+              testing::Contains(testing::Key(kContentLength)));
+  EXPECT_THAT(
+      http_request.headers,
+      testing::Contains(testing::Pair(kContentType, kContentTypeValue)));
   EXPECT_THAT(http_request.headers,
-              testing::Contains(testing::Pair(header_name, header_value)));
+              testing::Contains(testing::Key(kXGoogHash)));
 }
 
 // Verify two overlapping calls to upload the same attachment result in only one
 // HTTP request.
 TEST_F(AttachmentUploaderImplTest, UploadAttachment_Collapse) {
-  token_service().AddAccount(kAccountId);
-  request_handler().SetStatusCode(net::HTTP_OK);
-
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kAttachmentData;
-  Attachment attachment1 = Attachment::Create(some_data);
+  Attachment attachment1 = UploadAndRespondWith(net::HTTP_OK);
   Attachment attachment2 = attachment1;
-  uploader()->UploadAttachment(attachment1, upload_callback());
   uploader()->UploadAttachment(attachment2, upload_callback());
 
   // Wait for upload_callback() to be invoked twice.
@@ -474,17 +510,12 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_Collapse) {
 // uplaod finishes.  We do this by issuing two non-overlapping uploads for the
 // same attachment and see that it results in two HTTP requests.
 TEST_F(AttachmentUploaderImplTest, UploadAttachment_CleanUpAfterUpload) {
-  token_service().AddAccount(kAccountId);
-  request_handler().SetStatusCode(net::HTTP_OK);
-
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kAttachmentData;
-  Attachment attachment1 = Attachment::Create(some_data);
-  Attachment attachment2 = attachment1;
-  uploader()->UploadAttachment(attachment1, upload_callback());
+  Attachment attachment1 = UploadAndRespondWith(net::HTTP_OK);
 
   // Wait for upload_callback() to be invoked before starting the second upload.
   RunAndWaitFor(1);
+
+  Attachment attachment2 = attachment1;
   uploader()->UploadAttachment(attachment2, upload_callback());
 
   // Wait for upload_callback() to be invoked a second time.
@@ -519,13 +550,7 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_FailToGetToken) {
 
 // Verify behavior when the server returns "503 Service Unavailable".
 TEST_F(AttachmentUploaderImplTest, UploadAttachment_ServiceUnavilable) {
-  token_service().AddAccount(kAccountId);
-  request_handler().SetStatusCode(net::HTTP_SERVICE_UNAVAILABLE);
-
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kAttachmentData;
-  Attachment attachment = Attachment::Create(some_data);
-  uploader()->UploadAttachment(attachment, upload_callback());
+  Attachment attachment = UploadAndRespondWith(net::HTTP_SERVICE_UNAVAILABLE);
 
   RunAndWaitFor(1);
 
@@ -544,11 +569,6 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_ServiceUnavilable) {
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
   EXPECT_EQ(kAttachmentData, http_request.content);
-  std::string expected_header(kAuthorization);
-  const std::string header_name(kAuthorization);
-  const std::string header_value(std::string("Bearer ") + kAccessToken);
-  EXPECT_THAT(http_request.headers,
-              testing::Contains(testing::Pair(header_name, header_value)));
 
   // See that we did not invalidate the token.
   ASSERT_EQ(0, token_service().num_invalidate_token());
@@ -556,13 +576,7 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_ServiceUnavilable) {
 
 // Verify that we "403 Forbidden" as a non-transient error.
 TEST_F(AttachmentUploaderImplTest, UploadAttachment_Forbidden) {
-  token_service().AddAccount(kAccountId);
-  request_handler().SetStatusCode(net::HTTP_FORBIDDEN);
-
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kAttachmentData;
-  Attachment attachment = Attachment::Create(some_data);
-  uploader()->UploadAttachment(attachment, upload_callback());
+  Attachment attachment = UploadAndRespondWith(net::HTTP_FORBIDDEN);
 
   RunAndWaitFor(1);
 
@@ -581,11 +595,6 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_Forbidden) {
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
   EXPECT_EQ(kAttachmentData, http_request.content);
-  std::string expected_header(kAuthorization);
-  const std::string header_name(kAuthorization);
-  const std::string header_value(std::string("Bearer ") + kAccessToken);
-  EXPECT_THAT(http_request.headers,
-              testing::Contains(testing::Pair(header_name, header_value)));
 
   // See that we did not invalidate the token.
   ASSERT_EQ(0, token_service().num_invalidate_token());
@@ -594,13 +603,7 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_Forbidden) {
 // Verify that when we receive an "401 Unauthorized" we invalidate the access
 // token.
 TEST_F(AttachmentUploaderImplTest, UploadAttachment_BadToken) {
-  token_service().AddAccount(kAccountId);
-  request_handler().SetStatusCode(net::HTTP_UNAUTHORIZED);
-
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kAttachmentData;
-  Attachment attachment = Attachment::Create(some_data);
-  uploader()->UploadAttachment(attachment, upload_callback());
+  Attachment attachment = UploadAndRespondWith(net::HTTP_UNAUTHORIZED);
 
   RunAndWaitFor(1);
 
@@ -619,16 +622,23 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_BadToken) {
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
   EXPECT_EQ(kAttachmentData, http_request.content);
-  std::string expected_header(kAuthorization);
-  const std::string header_name(kAuthorization);
-  const std::string header_value(std::string("Bearer ") + kAccessToken);
-  EXPECT_THAT(http_request.headers,
-              testing::Contains(testing::Pair(header_name, header_value)));
 
   // See that we invalidated the token.
   ASSERT_EQ(1, token_service().num_invalidate_token());
 }
 
+TEST_F(AttachmentUploaderImplTest, ComputeHashHeader) {
+  scoped_refptr<base::RefCountedString> empty(new base::RefCountedString);
+  empty->data() = "";
+  EXPECT_EQ("X-Goog-Hash: crc32c=AAAAAA==",
+            AttachmentUploaderImpl::ComputeHashHeader(empty));
+
+  scoped_refptr<base::RefCountedString> hello_world(new base::RefCountedString);
+  hello_world->data() = "hello world";
+  EXPECT_EQ("X-Goog-Hash: crc32c=yZRlqg==",
+            AttachmentUploaderImpl::ComputeHashHeader(hello_world));
+}
+
 // TODO(maniscalco): Add test case for when we are uploading an attachment that
 // already exists.  409 Conflict? (bug 379825)
 
diff --git a/sync/internal_api/public/attachments/attachment_uploader_impl.h b/sync/internal_api/public/attachments/attachment_uploader_impl.h
index a88c967..674443f 100644
--- a/sync/internal_api/public/attachments/attachment_uploader_impl.h
+++ b/sync/internal_api/public/attachments/attachment_uploader_impl.h
@@ -6,6 +6,7 @@
 #define SYNC_INTERNAL_API_PUBLIC_ATTACHMENTS_ATTACHMENT_UPLOADER_IMPL_H_
 
 #include "base/containers/scoped_ptr_hash_map.h"
+#include "base/gtest_prod_util.h"
 #include "base/threading/non_thread_safe.h"
 #include "google_apis/gaia/oauth2_token_service_request.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -51,12 +52,18 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader,
                                     const AttachmentId& attachment_id);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(AttachmentUploaderImplTest, ComputeHashHeader);
+
   class UploadState;
   typedef std::string UniqueId;
   typedef base::ScopedPtrHashMap<UniqueId, UploadState> StateMap;
 
   void OnUploadStateStopped(const UniqueId& unique_id);
 
+  // Returns an X-Goog-Hash header for |memory|.  Potentially expensive.
+  static std::string ComputeHashHeader(
+      const scoped_refptr<base::RefCountedMemory>& memory);
+
   GURL sync_service_url_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   std::string account_id_;
diff --git a/sync/sync.gyp b/sync/sync.gyp
index c5532be..55ad858 100644
--- a/sync/sync.gyp
+++ b/sync/sync.gyp
@@ -48,6 +48,7 @@
         '../google_apis/google_apis.gyp:google_apis',
         '../net/net.gyp:net',
         '../sql/sql.gyp:sql',
+        '../third_party/leveldatabase/leveldatabase.gyp:leveldatabase',
         '../third_party/protobuf/protobuf.gyp:protobuf_lite',
         '../third_party/zlib/zlib.gyp:zlib',
         '../url/url.gyp:url_lib',