Auth Token failure categorised as General Exception

[oatsoda]

If you authenticate with an account and then later change the password on that account - therefore invalidating the token - the HttpProvider does not interpret this as an Auth Error - instead it just treats it as a general problem.

This is the response from the API:

HTTP/1.1 400 Bad Request
Cache-Control: no-store
Pragma: no-cache
Content-Length: 210
Content-Type: application/json
Server: Microsoft-IIS/8.5
X-WLID-Error: 0x8004100C
X-Content-Type-Options: nosniff
Strict-Transport-Security: max-age=31536000
X-XSS-Protection: 1; mode=block
Date: Tue, 13 Dec 2016 07:57:45 GMT
Connection: close

{"error":"invalid_grant","error_description":"The user could not be authenticated or the grant is expired. The user must first sign in and if needed grant the client application access to the requested scope."}

And this is the exception detail/stack trace:

"Newtonsoft.Json.JsonSerializationException: Error converting value \"invalid_grant\" to type 'Microsoft.Graph.Error'. Path 'error', line 1, position 24. ---> System.ArgumentException: Could not cast or convert from System.String to Microsoft.Graph.Error.
   at Newtonsoft.Json.Utilities.ConvertUtils.EnsureTypeAssignable(Object value, Type initialType, Type targetType)
   at Newtonsoft.Json.Utilities.ConvertUtils.ConvertOrCast(Object initialValue, CultureInfo culture, Type targetType)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.EnsureType(JsonReader reader, Object value, CultureInfo culture, JsonContract contract, Type targetType)
   --- End of inner exception stack trace ---
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.EnsureType(JsonReader reader, Object value, CultureInfo culture, JsonContract contract, Type targetType)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConve
rter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize[T](JsonReader reader)
   at Microsoft.Graph.Serializer.DeserializeObject[T](Stream stream) in D:\\Repos\\microsoftgraph\\msgraph-sdk-dotnet\\src\\Microsoft.Graph.Core\\Serializat
ion\\Serializer.cs:line 56
   at Microsoft.Graph.HttpProvider.<ConvertErrorResponseAsync>d__22.MoveNext() in D:\\Repos\\microsoftgraph\\msgraph-sdk-dotnet\\src\\Microsoft.Graph.Core\\Requests\\HttpProvider.cs:line 297"

Because the detail of the original exception is lost, this makes it very difficult to detect that this is an authentication failure.

It should really be ensuring that it presents this problem in a ServiceException with the relevant Microsoft.Graph.Error object which can then be inspected to see the details.

[oatsoda]

Here is a unit test to prove the current behaviour:

    [TestMethod]
    [ExpectedException(typeof(ServiceException))]
    public async Task SendAsync_AuthInvalidGrant()
    {
        using (var httpRequestMessage = new HttpRequestMessage(HttpMethod.Get, "https://localhost"))
        using (var stringContent = new StringContent("{\"error\":\"invalid_grant\",\"error_description\":\"The user could not be authenticated or the grant is expired.The user must first sign in and if needed grant the client application access to the requested scope.\"}"))
        using (var httpResponseMessage = new HttpResponseMessage())
        {
            httpResponseMessage.StatusCode = HttpStatusCode.BadRequest;
            httpResponseMessage.RequestMessage = httpRequestMessage;
            httpResponseMessage.Content = stringContent;

            this.testHttpMessageHandler.AddResponseMapping(httpRequestMessage.RequestUri.ToString(), httpResponseMessage);

            try
            {
                var returnedResponseMessage = await this.httpProvider.SendAsync(httpRequestMessage);
            }
            catch (ServiceException exception)
            {
                Assert.IsTrue(exception.IsMatch(ErrorConstants.Codes.GeneralException), "Unexpected error code returned.");
                Assert.AreEqual(
                    ErrorConstants.Messages.UnexpectedExceptionResponse,
                    exception.Error.Message,
                    "Unexpected error message returned.");

                throw;
            }
        }
    }

[caitlinrussell]

Thanks for the unit test. I'm seeing it pass on the current version (1.2.0) of the library. Which package version are you running?

[oatsoda]

The unit test should pass - I wrote it to prove current behaviour. I'm not sure exactly what the SDK should do with the response. The OneDrive API documentation states:

The error response is a single JSON object that contains a single property named error. This object includes all of the details of the error.

But in this case of "invalid_grant" the response is clearly not in that format. So is it an API mistake? Otherwise the SDK needs to do something different.

Confusingly, the OneDrive SDK appears to have code to handle this "error" + "error_description" response body, but because the Graph SDK explodes, in this case it doesn't reach that code (see linked issue).

Like I say, is it an API problem or an SDK problem?

[oatsoda]

Ok, so the OAUTH docs specifies the "error" + "error_description" response...

If the request fails due to a missing, invalid, or mismatching redirection URI, or if the client identifier is missing or invalid, the authorization server SHOULD inform the resource owner of the error and MUST NOT automatically redirect the user-agent to the invalid redirection URI.

It would therefore suggest that the problem is a weakness in the Graph SDK as it doesn't handle deserialising these OAUTH authentication errors. i.e the code in HttpProvider currently will catch the deserialisation exception and ConvertErrorResponseAsync will return null.

Perhaps better behaviour would be to handle this and either:

1. Manually deserialise the OATUTH authentication error into the Error object to ensure the ServiceException is thrown containing a new ErrorConstant to indicate that it is an "oauthAuthFailure"

• or -

2. Create a new Exception type (like ServiceException) such as OauthException and throw that instead.

[oatsoda]

Pull request created for option 1.

[shiftylogic]

See comment on the PR.

Thanks for reporting this bug. The error coming from the service is leaking in a format it should not be (per Graph Documentation). The service should be wrapping the OAuth error in a format that calling code expects.

[oatsoda]

Looks like it is an API problem as I initially wondered.

[shiftylogic]

@oatsoda Can you provide the exact call you make to the graph when you see this OAuth error return in this form? We are trying to reproduce the bug but are not having a lot of luck.

[oatsoda]

Here is the code we are using. You first need to authenticate with OneDrive and have got a refreshToken. Then change your password with OneDrive. Then authenticate again with the refreshToken as per below:

public const string RETURN_URL = "https://login.live.com/oauth20_desktop.srf";
public const string CONSUMER_BASE_URL = "https://api.onedrive.com/v1.0";

private static readonly string[] s_ConsumerScopes =
{
	"onedrive.readwrite", // Grants read and write permission to all of a user's OneDrive files, including files shared with the user
	"wl.offline_access"   // Enables your app to work offline even when the user isn't active. Provides a refresh token that can be used to generate additional access tokens
};

private Sdk.IOneDriveClient m_OneDriveSdkClient;

private async Task<string> AuthenticateConsumerAsync(string clientApplicationId, string refreshToken)
{
	var msaAuthProvider = new MsaAuthenticationProvider(
		clientApplicationId,
		RETURN_URL,
		s_ConsumerScopes);

	if (!string.IsNullOrWhiteSpace(refreshToken))
	{
		msaAuthProvider.CurrentAccountSession = new AccountSession
		{
			ClientId = clientApplicationId,
			RefreshToken = refreshToken
		};
	}

	m_OneDriveSdkClient = new Sdk.OneDriveClient(CONSUMER_BASE_URL, msaAuthProvider);

	await msaAuthProvider.AuthenticateUserAsync();
	return msaAuthProvider.CurrentAccountSession.RefreshToken;
}

[shiftylogic]

I see what's going on here. I guess this isn't a bug in the service. This is a bug in the OneDrive SDK. This repo is for the Microsoft Graph SDK. I'm going to recreate you a new issue on the OneDrive SDK repo and tag you. I can explain to them what the problem is.

Sorry for the confusion.

[shiftylogic]

New issue filed here:
OneDrive/onedrive-sdk-dotnet-msa-auth-adapter#32