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

Removing existing authorization token #674

Merged

Conversation

davesmits
Copy link
Contributor

fixing bug #673

I tried to leave it as much intact as is with the string format but personally i like this syntax more but not sure if I am missing something with the string format

request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", "token");

Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @davesmits
This looks good.
I've provided a suggestion for improvement.

@@ -30,6 +30,12 @@ public TokenAcquisitionCredentialProvider(ITokenAcquisition tokenAcquisition, IE
/// <returns>A Task (as this is an async method).</returns>
public async Task AuthenticateRequestAsync(HttpRequestMessage request)
{
// In case of a retry there is already an authorization header; replace it with new one to avoid using expired token
if (request.Headers.Contains(Constants.Authorization))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the following?

request.Headers[Constants.Authorization] = string.Format(
                    CultureInfo.InvariantCulture,
                    "{0} {1}",
                    Constants.Bearer,
                    await _tokenAcquisition.GetAccessTokenForUserAsync(_initialScopes).ConfigureAwait(false)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the HttpHeaders doesn't have a indexer

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Thanks @davesmits

@jmprieur jmprieur merged commit 3411f8a into AzureAD:master Oct 10, 2020
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.

2 participants