-
Notifications
You must be signed in to change notification settings - Fork 541
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
Upgrade Autodesk to v2 #764
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution - just some minor code style comments.
Have you verified that these changes work as expected in a real application with the Autodesk v2 API?
src/AspNet.Security.OAuth.Autodesk/AutodeskAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
src/AspNet.Security.OAuth.Autodesk/AutodeskAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
src/AspNet.Security.OAuth.Autodesk/AutodeskAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
src/AspNet.Security.OAuth.Autodesk/AutodeskAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
src/AspNet.Security.OAuth.Autodesk/AutodeskAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
Note: you forgot to update the address of the userinfo endpoint to use the v2 userinfo API. See https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/pull/788/files. |
@EMostafaAli I don't see this change in this PR: Are you sure you pushed the changes to the correct branch? (also, you'll likely need to update the claim mapping in the options class) |
My bad, I was looking at different code. I will try to make the changes quickly. |
@kevinchalet , updated the user info endpoint |
@EMostafaAli it looks like you didn't fix the points mentioned by @martincostello. We'll also need you to fix #764 (comment) before being able to merge this PR. |
@kevinchalet, I hope this new commit addresses your comments. |
The tests are failing. |
@martincostello, Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR!
I'll leave @martincostello decide what to do next, as this PR is technically a breaking behavior change, given the claims are changing 😃
using var requestMessage = new HttpRequestMessage(HttpMethod.Post, Options.TokenEndpoint); | ||
requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); | ||
var credentials = Convert.ToBase64String(Encoding.ASCII.GetBytes( | ||
$"{Options.ClientId}:{Options.ClientSecret}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the standard way of computing the Authorization: Basic [...]
header requires formURL-encoding the client identifier and client secret individually before computing their base64 representation. That said, I see in the Autodesk docs that they don't mention formURL-encoding so their implementation is probably not 100% compliant. Let's keep it this way.
Update Autodesk provider for v2 https://aps.autodesk.com/en/docs/oauth/v2/developers_guide/overview/
Issue: #760