microsoftgraph / msgraph-sdk-android

Microsoft Graph SDK for Android! https://graph.microsoft.io
Other
51 stars 43 forks source link

Authorization header incorrectly included in OneDrive Consumer resumeable upload sessions #45

Closed caitlinrussell closed 7 years ago

caitlinrussell commented 7 years ago

OneDrive for Business ignores the access token, OneDrive for Consumer returns an error if it is sent. This blocks users from being able to upload large files to ODC, instead throwing a 401 error.

We need to write a SessionHttpProvider that extends DefaultHttpProvider, skipping the authorization step. We then need to update ChunkedUploadRequest to call an overridden request handler which targets SessionHttpProvider.

markuskreusch commented 7 years ago

Because we want to switch to version 1.3.2 I will add my workaround (#44) on top of 1.3.2. Because this is not a simple merge (code formatter broke the possiblity to get a clean diff) i would have to spend some time on this anyway.

I may try to implement it the way suggested here, but would be happy to get some further instructions so that a potential CR will get accepted.

caitlinrussell commented 7 years ago

The main thing to remove when creating a new HttpProvider is not to call the authentication provider when making the request:

if (mAuthenticationProvider != null) {
   mAuthenticationProvider.authenticateRequest(request);
}

Otherwise, the functionality will be the same.

Or, if you prefer, we could just add a method to BaseRequest.java to removeHeader(HeaderOption) and call that in ChunkedUploadRequest since this is handwritten code already.

markuskreusch commented 7 years ago

Had time to look at this and try it out. I am not sure how to continue here.

The approach with a custom IHttpProvider will not work properly because ChunkedUploadRequest obtains the http provider though a call to the client. The client itself is created by the user of the library. The IHttpProvider can either be not specified, which leads to contruction of the DefaultHttpProvider or the user can provide a custom implementation of IHttpProvider. Because the IHttpProvider in use is provided by the user it is just not possible to use a custom SessionHttpProvider when sending ChunkedUploadRequests. The user of the library would have to know that he needs to create two different IHttpProviders one which includes and one which omits the call to the authentication provider.

So I had a look at the second solution you suggested. The problem here is that the sole removal of the header will not be enough. The header is added to the request during the call to IHttpProvider#send. So doing a removal before that would not help. This is the reason I came up with the solution present in my PR. It was a way to letting the request know that an authorization header added lateron shall not be included. So needed is not just a removeHeader(HeaderOption) method but a keepHeaderRemoved(HeaderOption) method which prevents the addition of a specific header in the future. Is that what you had in mind?

To make things simpler a boolean property "skipAuthentication" could be introduced in IHttpRequest. The DefaultHttpProvider (and other implementations) or the IAuthenticationProvider could respect this flag and skip the authentication if the request whishes that.

caitlinrussell commented 7 years ago

This got resolved on the server side, so OneDrive now ignores the authorization header for both business and personal accounts. If you are still having issues, feel free to reopen