Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should a revoked refresh token result in an AuthException? #232

Open
jonsagara opened this issue May 4, 2021 · 5 comments
Open

Should a revoked refresh token result in an AuthException? #232

jonsagara opened this issue May 4, 2021 · 5 comments
Labels

Comments

@jonsagara
Copy link

Describe the bug

In testing a revoked refresh token used when calling Files.ListFolderAsync, the Dropbox SDK threw a generic HttpRequestException with status 400 Bad Request. There is no indication of what failed or why.

Here is the sample code that I ran in LINQPad:

var dropbox = new DropboxClient(
    oauth2RefreshToken: "my revoked refresh token",
    appKey: "my app key",
    appSecret: "my app secret"
    );

var listFolderResult = await dropbox.Files.ListFolderAsync("");

Here is the stack trace of the HttpRequestException from the request to https://api.dropbox.com/oauth2/token:

   at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode()
   at Dropbox.Api.DropboxRequestHandler.RefreshAccessToken(String[] scopeList)
   at Dropbox.Api.DropboxRequestHandler.CheckAndRefreshAccessToken()
   at Dropbox.Api.DropboxRequestHandler.RequestJsonStringWithRetry(String host, String routeName, String auth, RouteStyle routeStyle, String requestArg, Stream body)
   at Dropbox.Api.DropboxRequestHandler.Dropbox.Api.Stone.ITransport.SendRpcRequestAsync[TRequest,TResponse,TError](TRequest request, String host, String route, String auth, IEncoder`1 requestEncoder, IDecoder`1 responseDecoder, IDecoder`1 errorDecoder)
   at UserQuery.Main(), line 10

Here is the raw response returned by Fiddler:

{
    "error_description": "refresh token is invalid or revoked",
    "error": "invalid_grant"
}

If I make a similar request using an old-style long-lived access token, I get an AuthException whose Message is invalid_access_token/.... This I can use to alert the user that my app can no longer communicate with Dropbox on their behalf.

Here is the sample code:

var dropbox = new DropboxClient(oauth2Token: "my revoked access token");

var listFolderResult = await dropbox.Files.ListFolderAsync("");

Here is the stack trace of the AuthException from the request to https://api.dropboxapi.com/2/files/list_folder:

   at Dropbox.Api.DropboxRequestHandler.RequestJsonString(String host, String routeName, String auth, RouteStyle routeStyle, String requestArg, Stream body)
   at Dropbox.Api.DropboxRequestHandler.RequestJsonStringWithRetry(String host, String routeName, String auth, RouteStyle routeStyle, String requestArg, Stream body)
   at Dropbox.Api.DropboxRequestHandler.RequestJsonStringWithRetry(String host, String routeName, String auth, RouteStyle routeStyle, String requestArg, Stream body)
   at Dropbox.Api.DropboxRequestHandler.Dropbox.Api.Stone.ITransport.SendRpcRequestAsync[TRequest,TResponse,TError](TRequest request, String host, String route, String auth, IEncoder`1 requestEncoder, IDecoder`1 responseDecoder, IDecoder`1 errorDecoder)
   at UserQuery.Main(), line 10

Here is the raw response returned by Fiddler:

{
    "error_summary": "invalid_access_token/...",
    "error": {
        ".tag": "invalid_access_token"
    }
}

To Reproduce

  1. Obtain a short-lived refresh token by having the user authorize the application via OAuth.
  2. Have the user disconnect the app from Connected Apps in their Dropbox.com settings.
  3. Use the refresh token as described in the code snippet above to try to list folders.

Expected Behavior

When trying to use a revoked refresh token, I expect the SDK to throw an AuthException telling me that the refresh token is invalid or has been revoked.

Actual Behavior

The SDK throws a generic HttpRequestException with no details as to what caused the failure.

I believe the issue is in DropboxRequestHandler.cs in the RefreshAccessToken method:

var response = await this.defaultHttpClient.PostAsync(url, bodyContent).ConfigureAwait(false);
// if response is an invalid grant, we want to throw this exception rather than the one thrown in
// response.EnsureSuccessStatusCode();
if (response.StatusCode == HttpStatusCode.Unauthorized)
{
var reason = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
if (reason == "invalid_grant")
{
throw AuthException.Decode(reason, () => new AuthException(this.GetRequestId(response)));
}
}
response.EnsureSuccessStatusCode();
if (response.IsSuccessStatusCode)
{
var json = JObject.Parse(await response.Content.ReadAsStringAsync());
string accessToken = json["access_token"].ToString();
DateTime tokenExpiration = DateTime.Now.AddSeconds(json["expires_in"].ToObject<int>());
this.options.OAuth2AccessToken = accessToken;
this.options.OAuth2AccessTokenExpiresAt = tokenExpiration;
return true;
}
return false;

At line 279, it handles an Unauthorized response, but it doesn't handle the 400 Bad Request returned by the API. The subsequent call to response.EnsureSuccessStatusCode(); on line 288 causes the generic HttpRequestException to be thrown.

Would it be possible to add error handling before line 288 to throw an AuthException if it detects an invalid or revoked refresh token?

Versions

  • What version of the SDK are you using? Dropbox.Api 6.4.0
  • What version of the language are you using? C# 9.0
  • What platform are you using? (if applicable)
    • ASP.NET Core 5.0
    • .NET SDK 5.0.202
    • Windows 10 / whatever version of Windows Azure App Service uses

Thank you,

Jon

@jonsagara jonsagara added the bug label May 4, 2021
@greg-db
Copy link
Contributor

greg-db commented May 5, 2021

Thanks for the detailed write up! I'm raising this with the team to see if we can fix up this behavior.

@bellissimo
Copy link

Another vote for this. Without being able to inform users that the link is broken, you can end up in a scenario where things like auto backups and/or syncing silently stop working, potentially resulting in data loss for the user.

@greg-db
Copy link
Contributor

greg-db commented May 17, 2021

@bellissimo Thanks for the feedback!

@jonsagara
Copy link
Author

Hi all, has there been further discussion on how to proceed with this?

@greg-db
Copy link
Contributor

greg-db commented Jun 2, 2021

@jonsagara This is still open with engineering, but I don't have an update on it yet. I'll follow up here once I do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants