Skip to content

Conversation

@peombwa
Copy link
Member

@peombwa peombwa commented Nov 22, 2018

Changes proposed in this pull request

  • Adds AuthenticationHandler middleware which authenticates requests using a specified AuthenticationProvider service of type IAuthenticationProvider. It also handles http's status code 401 for non-streamed content by getting a new token from the AuthenticationProvider and re-issues the request.
  • AuthenticationHandler implements DelegatingHandler and overrides SendAsync
  • Moves IsBuffered to Helpers/ContentHelper Static Class to make it reusable

Other links

darrelmiller
darrelmiller previously approved these changes Nov 27, 2018
/// <param name="httpResponseMessage">The <see cref="HttpResponseMessage"/>to send.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>to send.</param>
/// <returns></returns>
private async Task<HttpResponseMessage> SendRetryAsync(HttpResponseMessage httpResponseMessage, CancellationToken cancellationToken)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to consider the following behavior where an Authorization header is not expected per a workload:
https://developer.microsoft.com/en-us/graph/docs/api-reference/v1.0/api/driveitem_createuploadsession#remarks
SendRetryAsync will cause an error if a Authorization header is provided in this scenario. Other than that, this PR looks good.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't be an issue as we stap the Authorization header only for graph.microsoft.com issues.

@peombwa peombwa merged commit 0d34c89 into dev Nov 27, 2018
@peombwa peombwa deleted the AuthenticationHandler branch January 14, 2019 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants