Add authentication support to AttachmentUploaderImpl.

Add authentication support to AttachmentUploaderImpl.

Update AUI to request an access token and send an Authorization header
with each requests.  If the server responds with 401, AUI invalidates
its token, but does not retry.  Retries will be implemented in a future
CL, probably at a higher level.

BUG=371516
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275293

[maniscalco, 2014-06-02 18:05:28]

Hi Pavel, would you please review this change?

You may notice some red trybots.  Looks like they haven't caught up to the
OA2TSR patch I landed earlier this morning.  I'll kick them off again later
today.

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

Here is initial feedback. I looked at header and implementation. I'll review
unittests in the afternoon.

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:88:
base::WeakPtrFactory<UploadState> weak_ptr_factory_;
Why do you need weak_ptr_factory?

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:116:
scopes_.insert(GaiaConstants::kChromeSyncOAuth2Scope);
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?

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:145:
token_service_provider_, account_id_, scopes_, access_token_);
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.

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:189: void
AttachmentUploaderImpl::UploadState::GetToken() {
[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]

Thanks for the feedback.  I've addressed it in patch set #2.  PTAL!

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:88:
base::WeakPtrFactory<UploadState> weak_ptr_factory_;
Good catch.  Removed.  It was left over from a previous revision.

On 2014/06/03 17:38:54, pavely wrote:
> Why do you need weak_ptr_factory?

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:116:
scopes_.insert(GaiaConstants::kChromeSyncOAuth2Scope);
Good point.  Done.  The scope set is now part of AUI's interface.

On 2014/06/03 17:38:54, pavely wrote:
> 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?

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:145:
token_service_provider_, account_id_, scopes_, access_token_);
On 2014/06/03 17:38:54, pavely wrote:
> 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.

Done.

https://codereview.chromium.org/304253010/diff/30005/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:189: void
AttachmentUploaderImpl::UploadState::GetToken() {
Agreed.

On 2014/06/03 17:38:54, pavely wrote:
> [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.

[pavely, 2014-06-04 23:34:39]

lgtm with small comment.

https://codereview.chromium.org/304253010/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl_unittest.cc (right):

https://codereview.chromium.org/304253010/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:405:
std::string expected_header(kAuthorization);
expected_header is not used.

[maniscalco, 2014-06-05 00:12:12]

https://codereview.chromium.org/304253010/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl_unittest.cc (right):

https://codereview.chromium.org/304253010/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:405:
std::string expected_header(kAuthorization);
Removed!

On 2014/06/04 23:34:39, pavely wrote:
> expected_header is not used.

[maniscalco, 2014-06-05 21:15:15]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-06-05 21:16:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maniscalco@chromium.org/304253010/120001

[commit-bot: I haz the power, 2014-06-06 01:29:12]

Change committed as 275293