diff options
author | maniscalco <maniscalco@chromium.org> | 2014-11-17 17:52:26 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-11-18 01:52:51 +0000 |
commit | 41b0acac5f6ae101df41208dd721b0a7a647c17d (patch) | |
tree | 272d311e7d17e59fd913c3e685c1df7c1d79afe6 /sync | |
parent | 0755060246e22371c9ac15c497b9278694e4a6e9 (diff) | |
download | chromium_src-41b0acac5f6ae101df41208dd721b0a7a647c17d.zip chromium_src-41b0acac5f6ae101df41208dd721b0a7a647c17d.tar.gz chromium_src-41b0acac5f6ae101df41208dd721b0a7a647c17d.tar.bz2 |
Send birthday on sync attachment uploads and downloads.
Refactor common URLFetcher settings into shared static method
(ConfigureURLFetcherCommon) called by both uploader and downloader.
Include User-Agent on uploads and downloads to aid in troubleshooting.
Use header constants from net::HttpRequestHeaders.
Make ReadTransaction's GetAttachmentIdsToUpload const.
BUG=371521, 433898
Review URL: https://codereview.chromium.org/729363005
Cr-Commit-Position: refs/heads/master@{#304541}
Diffstat (limited to 'sync')
10 files changed, 114 insertions, 65 deletions
diff --git a/sync/internal_api/attachments/attachment_downloader.cc b/sync/internal_api/attachments/attachment_downloader.cc index a3c780f..fb60201 100644 --- a/sync/internal_api/attachments/attachment_downloader.cc +++ b/sync/internal_api/attachments/attachment_downloader.cc @@ -21,13 +21,11 @@ scoped_ptr<AttachmentDownloader> AttachmentDownloader::Create( const std::string& account_id, const OAuth2TokenService::ScopeSet scopes, const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>& - token_service_provider) { - return scoped_ptr<AttachmentDownloader>( - new AttachmentDownloaderImpl(sync_service_url, - url_request_context_getter, - account_id, - scopes, - token_service_provider)); + token_service_provider, + const std::string& store_birthday) { + return scoped_ptr<AttachmentDownloader>(new AttachmentDownloaderImpl( + sync_service_url, url_request_context_getter, account_id, scopes, + token_service_provider, store_birthday)); } } // namespace syncer diff --git a/sync/internal_api/attachments/attachment_downloader_impl.cc b/sync/internal_api/attachments/attachment_downloader_impl.cc index 232b99e..c4edcec 100644 --- a/sync/internal_api/attachments/attachment_downloader_impl.cc +++ b/sync/internal_api/attachments/attachment_downloader_impl.cc @@ -47,17 +47,20 @@ AttachmentDownloaderImpl::AttachmentDownloaderImpl( const std::string& account_id, const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>& - token_service_provider) + token_service_provider, + const std::string& store_birthday) : OAuth2TokenService::Consumer("attachment-downloader-impl"), sync_service_url_(sync_service_url), url_request_context_getter_(url_request_context_getter), account_id_(account_id), oauth2_scopes_(scopes), - token_service_provider_(token_service_provider) { + token_service_provider_(token_service_provider), + raw_store_birthday_(store_birthday) { + DCHECK(url_request_context_getter_.get()); DCHECK(!account_id.empty()); DCHECK(!scopes.empty()); DCHECK(token_service_provider_.get()); - DCHECK(url_request_context_getter_.get()); + DCHECK(!raw_store_birthday_.empty()); } AttachmentDownloaderImpl::~AttachmentDownloaderImpl() { @@ -182,15 +185,9 @@ scoped_ptr<net::URLFetcher> AttachmentDownloaderImpl::CreateFetcher( const std::string& access_token) { scoped_ptr<net::URLFetcher> url_fetcher( net::URLFetcher::Create(GURL(url), net::URLFetcher::GET, this)); - url_fetcher->SetAutomaticallyRetryOn5xx(false); - const std::string auth_header("Authorization: Bearer " + access_token); - url_fetcher->AddExtraRequestHeader(auth_header); - url_fetcher->SetRequestContext(url_request_context_getter_.get()); - url_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, what else?) on - // the request (bug 371521). + AttachmentUploaderImpl::ConfigureURLFetcherCommon( + url_fetcher.get(), access_token, raw_store_birthday_, + url_request_context_getter_.get()); return url_fetcher.Pass(); } diff --git a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc index f36fbad..bd45627 100644 --- a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc +++ b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc @@ -28,6 +28,7 @@ const char kAccountId[] = "attachments@gmail.com"; const char kAccessToken[] = "access.token"; const char kAttachmentServerUrl[] = "http://attachments.com/"; const char kAttachmentContent[] = "attachment.content"; +const char kStoreBirthday[] = "z00000000-0000-007b-0000-0000000004d2"; // MockOAuth2TokenService remembers last request for access token and verifies // that only one request is active at a time. @@ -202,12 +203,9 @@ void AttachmentDownloaderImplTest::SetUp() { OAuth2TokenService::ScopeSet scopes; scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope); - attachment_downloader_ = - AttachmentDownloader::Create(GURL(kAttachmentServerUrl), - url_request_context_getter_, - kAccountId, - scopes, - token_service_provider); + attachment_downloader_ = AttachmentDownloader::Create( + GURL(kAttachmentServerUrl), url_request_context_getter_, kAccountId, + scopes, token_service_provider, std::string(kStoreBirthday)); } void AttachmentDownloaderImplTest::TearDown() { diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc index 2f14263..7edbf94 100644 --- a/sync/internal_api/attachments/attachment_uploader_impl.cc +++ b/sync/internal_api/attachments/attachment_uploader_impl.cc @@ -25,6 +25,7 @@ namespace { const char kContentType[] = "application/octet-stream"; const char kAttachments[] = "attachments/"; +const char kSyncStoreBirthday[] = "X-Sync-Store-Birthday"; } // namespace @@ -52,6 +53,7 @@ class AttachmentUploaderImpl::UploadState : public net::URLFetcherDelegate, const std::string& account_id, const OAuth2TokenService::ScopeSet& scopes, OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider, + const std::string& raw_store_birthday, const base::WeakPtr<AttachmentUploaderImpl>& owner); ~UploadState() override; @@ -98,6 +100,7 @@ class AttachmentUploaderImpl::UploadState : public net::URLFetcherDelegate, std::string account_id_; OAuth2TokenService::ScopeSet scopes_; std::string access_token_; + std::string raw_store_birthday_; OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider_; // Pointer to the AttachmentUploaderImpl that owns this object. base::WeakPtr<AttachmentUploaderImpl> owner_; @@ -115,6 +118,7 @@ AttachmentUploaderImpl::UploadState::UploadState( const std::string& account_id, const OAuth2TokenService::ScopeSet& scopes, OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider, + const std::string& raw_store_birthday, const base::WeakPtr<AttachmentUploaderImpl>& owner) : OAuth2TokenService::Consumer("attachment-uploader-impl"), is_stopped_(false), @@ -124,6 +128,7 @@ AttachmentUploaderImpl::UploadState::UploadState( user_callbacks_(1, user_callback), account_id_(account_id), scopes_(scopes), + raw_store_birthday_(raw_store_birthday), token_service_provider_(token_service_provider), owner_(owner) { DCHECK(upload_url_.is_valid()); @@ -131,6 +136,7 @@ AttachmentUploaderImpl::UploadState::UploadState( DCHECK(!account_id_.empty()); DCHECK(!scopes_.empty()); DCHECK(token_service_provider_); + DCHECK(!raw_store_birthday_.empty()); GetToken(); } @@ -195,8 +201,13 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess( access_token_ = access_token; fetcher_.reset( net::URLFetcher::Create(upload_url_, net::URLFetcher::POST, this)); - fetcher_->SetAutomaticallyRetryOn5xx(false); - fetcher_->SetRequestContext(url_request_context_getter_.get()); + ConfigureURLFetcherCommon(fetcher_.get(), access_token_, raw_store_birthday_, + url_request_context_getter_.get()); + + const uint32_t crc32c = attachment_.GetCrc32c(); + fetcher_->AddExtraRequestHeader(base::StringPrintf( + "X-Goog-Hash: crc32c=%s", FormatCrc32cHash(crc32c).c_str())); + // TODO(maniscalco): Is there a better way? Copying the attachment data into // a string feels wrong given how large attachments may be (several MBs). If // we may end up switching from URLFetcher to URLRequest, this copy won't be @@ -204,16 +215,7 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess( scoped_refptr<base::RefCountedMemory> memory = attachment_.GetData(); const std::string upload_content(memory->front_as<char>(), memory->size()); fetcher_->SetUploadData(kContentType, upload_content); - const std::string auth_header("Authorization: Bearer " + access_token_); - fetcher_->AddExtraRequestHeader(auth_header); - const uint32_t crc32c = attachment_.GetCrc32c(); - fetcher_->AddExtraRequestHeader(base::StringPrintf( - "X-Goog-Hash: crc32c=%s", FormatCrc32cHash(crc32c).c_str())); - fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES | - net::LOAD_DO_NOT_SEND_COOKIES | - net::LOAD_DISABLE_CACHE); - // TODO(maniscalco): Set appropriate headers (e.g. User-Agent) on the request - // and include the "sync birthday" (bug 371521). + fetcher_->Start(); } @@ -264,17 +266,20 @@ AttachmentUploaderImpl::AttachmentUploaderImpl( const std::string& account_id, const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>& - token_service_provider) + token_service_provider, + const std::string& store_birthday) : sync_service_url_(sync_service_url), url_request_context_getter_(url_request_context_getter), account_id_(account_id), scopes_(scopes), token_service_provider_(token_service_provider), + raw_store_birthday_(store_birthday), weak_ptr_factory_(this) { DCHECK(CalledOnValidThread()); DCHECK(!account_id.empty()); DCHECK(!scopes.empty()); DCHECK(token_service_provider_.get()); + DCHECK(!raw_store_birthday_.empty()); } AttachmentUploaderImpl::~AttachmentUploaderImpl() { @@ -304,14 +309,9 @@ void AttachmentUploaderImpl::UploadAttachment(const Attachment& attachment, const GURL url = GetURLForAttachmentId(sync_service_url_, attachment_id); scoped_ptr<UploadState> upload_state( - new UploadState(url, - url_request_context_getter_, - attachment, - callback, - account_id_, - scopes_, - token_service_provider_.get(), - weak_ptr_factory_.GetWeakPtr())); + new UploadState(url, url_request_context_getter_, attachment, callback, + account_id_, scopes_, token_service_provider_.get(), + raw_store_birthday_, weak_ptr_factory_.GetWeakPtr())); state_map_.add(unique_id, upload_state.Pass()); } @@ -350,4 +350,27 @@ std::string AttachmentUploaderImpl::FormatCrc32cHash(uint32_t crc32c) { return encoded; } +void AttachmentUploaderImpl::ConfigureURLFetcherCommon( + net::URLFetcher* fetcher, + const std::string& access_token, + const std::string& raw_store_birthday, + net::URLRequestContextGetter* request_context_getter) { + DCHECK(request_context_getter); + DCHECK(fetcher); + fetcher->SetAutomaticallyRetryOn5xx(false); + fetcher->SetRequestContext(request_context_getter); + fetcher->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES | + net::LOAD_DO_NOT_SEND_COOKIES | + net::LOAD_DISABLE_CACHE); + fetcher->AddExtraRequestHeader(base::StringPrintf( + "%s: Bearer %s", net::HttpRequestHeaders::kAuthorization, + access_token.c_str())); + // Encode the birthday. Birthday is opaque so we assume it could contain + // anything. Encode it so that it's safe to pass as an HTTP header value. + std::string encoded_store_birthday; + base::Base64Encode(raw_store_birthday, &encoded_store_birthday); + fetcher->AddExtraRequestHeader(base::StringPrintf( + "%s: %s", kSyncStoreBirthday, encoded_store_birthday.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 d31fc18..2df6719 100644 --- a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc +++ b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc @@ -4,6 +4,7 @@ #include "sync/internal_api/public/attachments/attachment_uploader_impl.h" +#include "base/base64.h" #include "base/bind.h" #include "base/callback.h" #include "base/memory/ref_counted.h" @@ -18,6 +19,7 @@ #include "google_apis/gaia/fake_oauth2_token_service.h" #include "google_apis/gaia/gaia_constants.h" #include "google_apis/gaia/oauth2_token_service_request.h" +#include "net/http/http_status_code.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/http_request.h" #include "net/test/embedded_test_server/http_response.h" @@ -33,12 +35,11 @@ namespace { 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/"; +const char kStoreBirthday[] = "z00000000-0000-007b-0000-0000000004d2"; +const char kSyncStoreBirthdayHeader[] = "X-Sync-Store-Birthday"; } // namespace @@ -284,11 +285,9 @@ void AttachmentUploaderImplTest::SetUp() { OAuth2TokenService::ScopeSet scopes; scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope); - uploader().reset(new AttachmentUploaderImpl(url, - url_request_context_getter_, - kAccountId, - scopes, - token_service_provider)); + uploader().reset(new AttachmentUploaderImpl( + url, url_request_context_getter_, kAccountId, scopes, + token_service_provider, std::string(kStoreBirthday))); upload_callback_ = base::Bind(&AttachmentUploaderImplTest::UploadDone, base::Unretained(this)); @@ -479,19 +478,27 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_Headers) { 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( + net::HttpRequestHeaders::kAuthorization, auth_header_value))); EXPECT_THAT( http_request.headers, - testing::Contains(testing::Pair(kAuthorization, auth_header_value))); + testing::Contains(testing::Key(net::HttpRequestHeaders::kContentLength))); + EXPECT_THAT(http_request.headers, + testing::Contains(testing::Pair( + net::HttpRequestHeaders::kContentType, kContentTypeValue))); EXPECT_THAT(http_request.headers, - testing::Contains(testing::Key(kContentLength))); + testing::Contains(testing::Key(kXGoogHash))); EXPECT_THAT( http_request.headers, - testing::Contains(testing::Pair(kContentType, kContentTypeValue))); + testing::Contains(testing::Key(net::HttpRequestHeaders::kUserAgent))); + std::string encoded_store_birthday; + base::Base64Encode(kStoreBirthday, &encoded_store_birthday); EXPECT_THAT(http_request.headers, - testing::Contains(testing::Key(kXGoogHash))); + testing::Contains(testing::Pair(kSyncStoreBirthdayHeader, + encoded_store_birthday))); } // Verify two overlapping calls to upload the same attachment result in only one diff --git a/sync/internal_api/public/attachments/attachment_downloader.h b/sync/internal_api/public/attachments/attachment_downloader.h index 12db422..8b7180c 100644 --- a/sync/internal_api/public/attachments/attachment_downloader.h +++ b/sync/internal_api/public/attachments/attachment_downloader.h @@ -51,6 +51,8 @@ class SYNC_EXPORT AttachmentDownloader { // |scopes| is the set of scopes to use for downloads. // // |token_service_provider| provides an OAuth2 token service. + // + // |store_birthday| is the raw, sync store birthday. static scoped_ptr<AttachmentDownloader> Create( const GURL& sync_service_url, const scoped_refptr<net::URLRequestContextGetter>& @@ -58,7 +60,8 @@ class SYNC_EXPORT AttachmentDownloader { const std::string& account_id, const OAuth2TokenService::ScopeSet scopes, const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>& - token_service_provider); + token_service_provider, + const std::string& store_birthday); }; } // namespace syncer diff --git a/sync/internal_api/public/attachments/attachment_downloader_impl.h b/sync/internal_api/public/attachments/attachment_downloader_impl.h index 2e75ce2..d6fa533 100644 --- a/sync/internal_api/public/attachments/attachment_downloader_impl.h +++ b/sync/internal_api/public/attachments/attachment_downloader_impl.h @@ -38,6 +38,8 @@ class AttachmentDownloaderImpl : public AttachmentDownloader, // |scopes| is the set of scopes to use for downloads. // // |token_service_provider| provides an OAuth2 token service. + // + // |store_birthday| is the raw, sync store birthday. AttachmentDownloaderImpl( const GURL& sync_service_url, const scoped_refptr<net::URLRequestContextGetter>& @@ -45,7 +47,8 @@ class AttachmentDownloaderImpl : public AttachmentDownloader, const std::string& account_id, const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>& - token_service_provider); + token_service_provider, + const std::string& store_birthday); ~AttachmentDownloaderImpl() override; // AttachmentDownloader implementation. @@ -101,6 +104,7 @@ class AttachmentDownloaderImpl : public AttachmentDownloader, scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider> token_service_provider_; scoped_ptr<OAuth2TokenService::Request> access_token_request_; + std::string raw_store_birthday_; StateMap state_map_; // |requests_waiting_for_access_token_| only keeps references to DownloadState diff --git a/sync/internal_api/public/attachments/attachment_uploader_impl.h b/sync/internal_api/public/attachments/attachment_uploader_impl.h index d2279ef..9ef7bcd 100644 --- a/sync/internal_api/public/attachments/attachment_uploader_impl.h +++ b/sync/internal_api/public/attachments/attachment_uploader_impl.h @@ -33,6 +33,8 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, // |scopes| is the set of scopes to use for uploads. // // |token_service_provider| provides an OAuth2 token service. + // + // |store_birthday| is the raw, sync store birthday. AttachmentUploaderImpl( const GURL& sync_service_url, const scoped_refptr<net::URLRequestContextGetter>& @@ -40,7 +42,8 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, const std::string& account_id, const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider>& - token_service_provider); + token_service_provider, + const std::string& store_birthday); ~AttachmentUploaderImpl() override; // AttachmentUploader implementation. @@ -58,6 +61,14 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, // (https://cloud.google.com/storage/docs/reference-headers#xgooghash). static std::string FormatCrc32cHash(uint32_t crc32c); + // Apply common settings to |fetcher|, suitable for both uploads and + // downloads. + static void ConfigureURLFetcherCommon( + net::URLFetcher* fetcher, + const std::string& auth_token, + const std::string& raw_store_birthday, + net::URLRequestContextGetter* request_context_getter); + private: class UploadState; typedef std::string UniqueId; @@ -71,6 +82,7 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, OAuth2TokenService::ScopeSet scopes_; scoped_refptr<OAuth2TokenServiceRequest::TokenServiceProvider> token_service_provider_; + std::string raw_store_birthday_; StateMap state_map_; // Must be last data member. diff --git a/sync/internal_api/public/read_transaction.h b/sync/internal_api/public/read_transaction.h index 2084b9a..c75d16a 100644 --- a/sync/internal_api/public/read_transaction.h +++ b/sync/internal_api/public/read_transaction.h @@ -46,9 +46,12 @@ class SYNC_EXPORT ReadTransaction : public BaseTransaction { void GetDataTypeContext(ModelType type, sync_pb::DataTypeContext* context) const; - // Clears |id_set| and fills it with the ids of attachments that need to be + // Clear |id_set| and fill it with the ids of attachments that need to be // uploaded to the sync server. - void GetAttachmentIdsToUpload(ModelType type, AttachmentIdSet* id_set); + void GetAttachmentIdsToUpload(ModelType type, AttachmentIdSet* id_set) const; + + // Return the current (opaque) store birthday. + std::string GetStoreBirthday() const; private: void* operator new(size_t size); // Transaction is meant for stack use only. diff --git a/sync/internal_api/read_transaction.cc b/sync/internal_api/read_transaction.cc index 7d44fe4..872818f 100644 --- a/sync/internal_api/read_transaction.cc +++ b/sync/internal_api/read_transaction.cc @@ -48,10 +48,14 @@ void ReadTransaction::GetDataTypeContext( } void ReadTransaction::GetAttachmentIdsToUpload(ModelType type, - AttachmentIdSet* id_set) { + AttachmentIdSet* id_set) const { DCHECK(id_set); transaction_->directory()->GetAttachmentIdsToUpload( transaction_, type, id_set); } +std::string ReadTransaction::GetStoreBirthday() const { + return transaction_->directory()->store_birthday(); +} + } // namespace syncer |