msgraph-sdk-dotnet icon indicating copy to clipboard operation
msgraph-sdk-dotnet copied to clipboard

[Client bug]: Helper class `AsyncMonitor` doesn't work in conjunction with v5 SDK

Open olivermue opened this issue 1 year ago • 2 comments

Describe the bug Unfortunately there is no official code example or documentation available on how to use the SDK to copy a DriveItem and check the current process of copying. The latest given example here look okay, but doesn't work and lacks some problems.

Within the class AsyncMonitor the given client is used to send the request. This will lead to the fact, that the request will use the provided authentication provider to attach an authentication token to this request. But the Url of the copy operation is anonymous and returns a 401 status if a auth header is added. The response won't be checked for a failing request and waits endless, cause the response body contains an error message, that prevents reaching the throw statement.

If a service client without authentication is used like this:

var unauthorizedClient = new GraphServiceClient(_httpClientFactory.CreateClient(string.Empty));
var asyncMonitor = new AsyncMonitor<???>(unauthorizedClient, progressUrl);

the response gets a 200 status and contains something like this:

{
  "@odata.context": "https://myDomain.sharepoint.com/sites/myGroup/_api/v2.0/$metadata#oneDrive.asynchronousOperationStatus",
  "percentageComplete": 100,
  "resourceId": "01AIKISTH5XQ6VIEFNDNG34GGK3SAXOJBY",
  "status": "completed"
}

In that case it will be deserialized using the JsonConverter and the given generic type of AsyncMonitor<T>. Unfortunately this type (would be in my case guessed as being DriveItem) doesn't match the given response json of being an oneDrive.asynchronousOperationStatus. But even if we would use an AsyncMonitor<AsynchronousOperationStatus> it wouldn't work, cause the class is using the Kiota mechanism und can't be used in conjunction with JsonSerializer. So the deserialized object has everything set to null and doesn't contain any values.

Expected behavior Try to copy a file using the current Graph SDK and try to watch the copy progress by something like this:

var nativeResponseHandler = new NativeResponseHandler();
await graphServiceClient.Drives["driveId"].Items["sourceItemId"].Copy.PostAsync(new CopyPostRequestBody
{
    Name = sourceItem.Name,
    ParentReference = reference,
}, 
requestConfiguration => requestConfiguration.Options.Add(new ResponseHandlerOption(){ 
ResponseHandler = nativeResponseHandler }));

var responseMessage = nativeResponseHandler.Value as HttpResponseMessage;
var locationHeader = responseMessage.Headers.Location;
var asyncMonitor = new AsyncMonitor<DriveItem>(graphServiceClient, locationHeader.OriginalString);
var progress = new Progress<AsyncOperationStatus>(status => 
{
    Console.WriteLine($"Status: {status.Status}");
    Console.WriteLine($"Percentage complete: {status.PercentageComplete}");
    Console.WriteLine($"Operation: {status.Operation}");
});
var result = await asyncMonitor.PollForOperationCompletionAsync(progress, CancellationToken.None);

Client version v5.3.0

olivermue avatar Mar 31 '23 12:03 olivermue

Thanks for raising this @olivermue

Looks like the issue really is caused by the fact that the complete state is returned in 200 status code as compared to the documented expectation of a 202. Causing the model deserialization into the DriveItem type rather than the AsynchronousOperationStatus.

https://learn.microsoft.com/en-us/graph/long-running-actions-overview?tabs=http#retrieve-a-completed-status-report-from-the-monitor-url

Aside from this, it should be possible deserialize the appropiate models correctly. We'll update the implementation to capture the possibility of the AsynchronousOperationStatus coming back in a 200 response.

For the auth token issue, any chance you can confirm the way you are initializing the graph client? I believe the client now has a AllowedHostsValidator that should not set the bearer token if its not in the allowed list.

https://github.com/microsoftgraph/msgraph-sdk-dotnet-core/blob/24d519214038b8c6073177ad65ba64e2b91b7f40/src/Microsoft.Graph.Core/Authentication/AzureIdentityAccessTokenProvider.cs#L19

andrueastman avatar Apr 05 '23 10:04 andrueastman

After further investigation about the authentication issue I found the reason, why our graph client adds the auth token to the request. In this special case we create the instance by calling the constructor GraphServiceClient(IAuthenticationProvider authenticationProvider, string baseUrl = null) and providing our own authentication provider. This is done, because we use a behalf-of token in this code part and our auth provider is dumb and authenticates all requests without host check. I improved our own provider to instantiate an Microsoft.Graph.Authentication.AzureIdentityAccessTokenProvider and ask the public available AllowedHostsValidator to check the request url before applying the token by ourself. By this way these calls are without bearer token now and due to using the allowed hosts validator out of Microsoft.Graph.Core, we stay in sync with the Url supported by the library.

By implementing this workaround for our own provider it feels a little awkward to instantiate the graph token provider to get an instance of AllowedHostsValidator that knows the list of valid domains. As an additional enhancement it would be great if this string array of domains could be made public available within the library. In that case we could create an instance more easily by something like

namespace Microsoft.Graph.Authentication;

public static class GraphDomains
{
    public static readonly string[] Allowed = new string[] { "graph.microsoft.com", "graph.microsoft.us", "dod-graph.microsoft.us", "graph.microsoft.de", "microsoftgraph.chinacloudapi.cn", "canary.graph.microsoft.com" };
} 

// Somewhere in code
var hostsValidator = new AllowedHostsValidator(Microsoft.Graph.Autentication.GraphDomains.Allowed);

I guess the choosen names are not ideal 😊, but I think you get the idea.

olivermue avatar Apr 11 '23 10:04 olivermue