Add authentication support to AttachmentUploaderImpl.

sync/internal_api/attachments/attachment_uploader_impl.cc

Index: sync/internal_api/attachments/attachment_uploader_impl.cc
diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc
index 25630dcaba8b2a71f79192b91921497bb17587bd..3934dff072b945a82c0e5612478dbde7b808f613 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/non_thread_safe.h"
+#include "google_apis/gaia/gaia_constants.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
@@ -25,18 +26,22 @@ namespace syncer {
 
 // Encapsulates all the state associated with a single upload.
 class AttachmentUploaderImpl::UploadState : public net::URLFetcherDelegate,
+                                            public OAuth2TokenService::Consumer,
                                             public base::NonThreadSafe {
  public:
   // Construct an UploadState.
   //
   // |owner| is a pointer to the object that will own (and must outlive!) this
   // |UploadState.
-  UploadState(const GURL& upload_url,
-              const scoped_refptr<net::URLRequestContextGetter>&
-                  url_request_context_getter,
-              const Attachment& attachment,
-              const UploadCallback& user_callback,
-              AttachmentUploaderImpl* owner);
+  UploadState(
+      const GURL& upload_url,
+      const scoped_refptr<net::URLRequestContextGetter>&
+          url_request_context_getter,
+      const Attachment& attachment,
+      const UploadCallback& user_callback,
+      const std::string& account_id,
+      OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider,
+      AttachmentUploaderImpl* owner);
 
   virtual ~UploadState();
 
@@ -50,17 +55,37 @@ class AttachmentUploaderImpl::UploadState : public net::URLFetcherDelegate,
   // URLFetcher implementation.
   virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
 
+  // OAuth2TokenService::Consumer.
+  virtual void OnGetTokenSuccess(const OAuth2TokenService::Request* request,
+                                 const std::string& access_token,
+                                 const base::Time& expiration_time) OVERRIDE;
+  virtual void OnGetTokenFailure(const OAuth2TokenService::Request* request,
+                                 const GoogleServiceAuthError& error) OVERRIDE;
+
  private:
   typedef std::vector<UploadCallback> UploadCallbackList;
 
+  void GetToken();
+
+  void ReportResult(const UploadResult& result,
+                    const AttachmentId& attachment_id);
+
   GURL upload_url_;
   const scoped_refptr<net::URLRequestContextGetter>&
       url_request_context_getter_;
   Attachment attachment_;
   UploadCallbackList user_callbacks_;
   scoped_ptr<net::URLFetcher> fetcher_;
+  std::string account_id_;
+  std::string access_token_;
+  OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider_;
   // Pointer to the AttachmentUploaderImpl that owns this object.
+  OAuth2TokenService::ScopeSet scopes_;
   AttachmentUploaderImpl* owner_;
+  scoped_ptr<OAuth2TokenServiceRequest> access_token_request_;
+
+  // Must be last data member.
+  base::WeakPtrFactory<UploadState> weak_ptr_factory_;

[pavely, 2014/06/03 17:38:54]

Why do you need weak_ptr_factory?

[maniscalco, 2014/06/04 00:20:50]

Good catch.  Removed.  It was left over from a previous revision.

On 2014/06/03 17:38:54, pavely wrote:

 
   DISALLOW_COPY_AND_ASSIGN(UploadState);
 };
@@ -71,33 +96,25 @@ AttachmentUploaderImpl::UploadState::UploadState(
         url_request_context_getter,
     const Attachment& attachment,
     const UploadCallback& user_callback,
+    const std::string& account_id,
+    OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider,
     AttachmentUploaderImpl* owner)
-    : upload_url_(upload_url),
+    : OAuth2TokenService::Consumer("attachment-uploader-impl"),
+      upload_url_(upload_url),
       url_request_context_getter_(url_request_context_getter),
       attachment_(attachment),
       user_callbacks_(1, user_callback),
-      owner_(owner) {
-  DCHECK(url_request_context_getter_);
+      account_id_(account_id),
+      token_service_provider_(token_service_provider),
+      owner_(owner),
+      weak_ptr_factory_(this) {
   DCHECK(upload_url_.is_valid());
+  DCHECK(url_request_context_getter_);
+  DCHECK(!account_id_.empty());
+  DCHECK(token_service_provider_);
   DCHECK(owner_);
-  fetcher_.reset(
-      net::URLFetcher::Create(upload_url_, net::URLFetcher::POST, this));
-  fetcher_->SetRequestContext(url_request_context_getter_.get());
-  // 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
-  // necessary.
-  scoped_refptr<base::RefCountedMemory> memory = attachment.GetData();
-  const std::string upload_content(memory->front_as<char>(), memory->size());
-  fetcher_->SetUploadData(kContentType, upload_content);
-  // TODO(maniscalco): Add authentication support (bug 371516).
-  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).
-  fetcher_->Start();
+  scopes_.insert(GaiaConstants::kChromeSyncOAuth2Scope);

[pavely, 2014/06/03 17:38:54]

If this is managed user then scope should be different (see
ManagedUserSigninManagerWrapper::GetSyncScopeToUse).
That's the thing I was talking about the other day:
Typical pattern is: there is some coordinator that sets up everything and then
there is some workhorse that issues requests. All this workhorse cares about is
"GiveMeSomeTokenRightNow", it shouldn't know about scopes or account_ids, it is
not its single responsibility. Coordinator should know and care about all these
details.
That's the reason SyncAuthProvider interfaces is this minimalistic.

Anyway... could you change code to pass scope set through parameters along with
account_id. Or put TODO here to change it in future change?

[maniscalco, 2014/06/04 00:20:50]

Good point.  Done.  The scope set is now part of AUI's interface.

On 2014/06/03 17:38:54, pavely wrote:

+  GetToken();
 }
 
 AttachmentUploaderImpl::UploadState::~UploadState() {
@@ -117,20 +134,71 @@ const Attachment& AttachmentUploaderImpl::UploadState::GetAttachment() {
 void AttachmentUploaderImpl::UploadState::OnURLFetchComplete(
     const net::URLFetcher* source) {
   DCHECK(CalledOnValidThread());
-  // TODO(maniscalco): Once the protocol is better defined, deal with the
-  // various HTTP response code we may encounter.
   UploadResult result = UPLOAD_UNSPECIFIED_ERROR;
+  AttachmentId attachment_id = attachment_.GetId();
   if (source->GetResponseCode() == net::HTTP_OK) {
     result = UPLOAD_SUCCESS;
+    // TODO(maniscalco): Update the attachment id with server address
+    // information before passing it to the callback (bug 371522).
+  } else if (source->GetResponseCode() == net::HTTP_UNAUTHORIZED) {
+    OAuth2TokenServiceRequest::InvalidateToken(
+        token_service_provider_, account_id_, scopes_, access_token_);

[pavely, 2014/06/03 17:38:54]

This is likely transient error. It might mean that we got unlucky and token
expired before we managed to use it. We should just retry requesting access
token.
You can put TODO here and fix this in separate change.

[maniscalco, 2014/06/04 00:20:50]

Done.

+  } else {
+    // TODO(maniscalco): Once the protocol is better defined, deal with the
+    // various HTTP response codes we may encounter.
   }
-  // TODO(maniscalco): Update the attachment id with server address information
-  // before passing it to the callback (bug 371522).
-  AttachmentId updated_id = attachment_.GetId();
+  ReportResult(result, attachment_id);
+}
+
+void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess(
+    const OAuth2TokenService::Request* request,
+    const std::string& access_token,
+    const base::Time& expiration_time) {
+  DCHECK_EQ(access_token_request_.get(), request);
+  access_token_request_.reset();
+  access_token_ = access_token;
+  fetcher_.reset(
+      net::URLFetcher::Create(upload_url_, net::URLFetcher::POST, this));
+  fetcher_->SetRequestContext(url_request_context_getter_.get());
+  // 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
+  // necessary.
+  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);
+  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).
+  fetcher_->Start();
+}
+
+void AttachmentUploaderImpl::UploadState::OnGetTokenFailure(
+    const OAuth2TokenService::Request* request,
+    const GoogleServiceAuthError& error) {
+  DCHECK_EQ(access_token_request_.get(), request);
+  access_token_request_.reset();
+  ReportResult(UPLOAD_UNSPECIFIED_ERROR, attachment_.GetId());
+}
+
+void AttachmentUploaderImpl::UploadState::GetToken() {

[pavely, 2014/06/03 17:38:54]

[nothing to change here]. It is unfortunate that style guide prescribes order of
method definition to be the same as declaration. It would be nice to order
functions so that you can follow logic sequentially top to bottom. 

[maniscalco, 2014/06/04 00:20:50]

Agreed.

On 2014/06/03 17:38:54, pavely wrote:

+  access_token_request_ = OAuth2TokenServiceRequest::CreateAndStart(
+      token_service_provider_, account_id_, scopes_, this);
+}
+
+void AttachmentUploaderImpl::UploadState::ReportResult(
+    const UploadResult& result,
+    const AttachmentId& attachment_id) {
   UploadCallbackList::const_iterator iter = user_callbacks_.begin();
   UploadCallbackList::const_iterator end = user_callbacks_.end();
   for (; iter != end; ++iter) {
     base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(*iter, result, updated_id));
+        FROM_HERE, base::Bind(*iter, result, attachment_id));
   }
   // Destroy this object and return immediately.
   owner_->DeleteUploadStateFor(attachment_.GetId().GetProto().unique_id());
@@ -140,10 +208,16 @@ void AttachmentUploaderImpl::UploadState::OnURLFetchComplete(
 AttachmentUploaderImpl::AttachmentUploaderImpl(
     const std::string& url_prefix,
     const scoped_refptr<net::URLRequestContextGetter>&
-        url_request_context_getter)
+        url_request_context_getter,
+    const std::string& account_id,
+    scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
+        token_service_provider)
     : url_prefix_(url_prefix),
-      url_request_context_getter_(url_request_context_getter) {
+      url_request_context_getter_(url_request_context_getter),
+      account_id_(account_id),
+      token_service_provider_(token_service_provider.Pass()) {
   DCHECK(CalledOnValidThread());
+  DCHECK(token_service_provider_);
 }
 
 AttachmentUploaderImpl::~AttachmentUploaderImpl() {
@@ -159,8 +233,14 @@ void AttachmentUploaderImpl::UploadAttachment(const Attachment& attachment,
   StateMap::iterator iter = state_map_.find(unique_id);
   if (iter == state_map_.end()) {
     const GURL url = GetUploadURLForAttachmentId(attachment_id);
-    scoped_ptr<UploadState> upload_state(new UploadState(
-        url, url_request_context_getter_, attachment, callback, this));
+    scoped_ptr<UploadState> upload_state(
+        new UploadState(url,
+                        url_request_context_getter_,
+                        attachment,
+                        callback,
+                        account_id_,
+                        token_service_provider_.get(),
+                        this));
     state_map_.add(unique_id, upload_state.Pass());
   } else {
     DCHECK(