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

Ajax Call [AuthorizeForScopes] Attributed Action Produce CORS error when MSAL exception is thrown #665

Merged
merged 4 commits into from
Oct 8, 2020

Conversation

creativebrother
Copy link
Contributor

Controller Action decorated with [AuthorizeForScopes] Attribute called by ajax method cause user agent CORS error when MSAL exception is thrown.

Suggest enhancement
(1) in [AuthorizeForScopes] to provide a return url extract from ajax custom header for user agent to return to after the challenge is satisfied.
(2) in OpenIdConnentHandler, after the redirect url for Identity Provider is calculated, for ajax request, the handler will return a 401 with this redirect url in the header location.

Added a test project to test modified AuthorizeForScopes Attribute to handle Ajax call. It depends on a locally built microsoft.aspnetcore.authentication.openidconnect dev nupkg within which associate change on ajax call redirection is handled differently (OpenIdConnectHandler need to be updated at Asp.net Core side).

…d by ajax method cause use agent CORS error.

Suggest enhancement
(1) in [AuthorizeForScopes] to provide a return url extract from ajax custom header for user agent to return to after the challenge is satisfied.
(2) in OpenIdConnentHandler,  after the redirect url for Identity Provider is calculated, for ajax request, the handler will return a 401 with this redirect url in the header location.

Added a test project to test modified AuthorizeForScopes Attribute to handle Ajax call. It depends on a locally built microsoft.aspnetcore.authentication.openidconnect dev nupkg within which associate change on ajax call redirection is handled differently (OpenIdConnectHandler need to be updated at Asp.net Core side).
@ghost
Copy link

ghost commented Oct 7, 2020

CLA assistant check
All CLA requirements met.

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 7, 2020

@creativebrother thanks for the PR. we will take a look. Could you move the test project under the "tests" folder? thanks.

@creativebrother
Copy link
Contributor Author

@creativebrother thanks for the PR. we will take a look. Could you move the test project under the "tests" folder? thanks.

Hi, @jennyf19 I believe the test project is under tests folder. Do you mean I do not need an extra folder underneath tests folder, just put the test project directly underneath the tests folder?

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 8, 2020

@creativebrother like tests\AjaxCallActionsWithDynamicConsent or whatever you'd like to call the subfolder under tests. looking at the commit in GitHub (I haven't pulled your fork yet, but will), it looks like it's in the root of the project. thanks again.

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 8, 2020

thanks @creativebrother

@creativebrother
Copy link
Contributor Author

creativebrother commented Oct 8, 2020

@jennyf19 I made another PR in Aspnetcore project side. dotnet/aspnetcore#26689 to address the issue.

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 8, 2020

@creativebrother thanks again. I pulled your fork and tested it out. looks great. there are some things we would need to change, just small things on our end. I think it's easiest to merge your PR, and then we can update the test app as needed. this is really great. thank you. @jmprieur


In reply to: 705536463 [](ancestors = 705536463)

/// <param name="resource">Resource to obtain access token for</param>
/// <returns></returns>
private async Task<string> GetAccessTokenforResource(string scope)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we'll probably remove this, but after we merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I was using an older version of our app as template and it is in the resource concept era....

Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

@jennyf19 jennyf19 added this to the 1.1.1 milestone Oct 8, 2020
@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 8, 2020

#603 & #642

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 8, 2020

@creativebrother going to merge this and then i'll do another PR fixing the comments i have above. Thanks again for this AWESOME contribution. The team really appreciates it!

@jennyf19 jennyf19 merged commit 5eb4683 into AzureAD:master Oct 8, 2020
@creativebrother
Copy link
Contributor Author

@creativebrother going to merge this and then i'll do another PR fixing the comments i have above. Thanks again for this AWESOME contribution. The team really appreciates it!

@jennyf19 Love to contribute to Open Source Project. Your guys/ladies are awesome!

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 9, 2020

@creativebrother we love it too! less work for us! lol! idk when we would have gotten around to this. feel free to contribute as much as you like ;)

@ITRON-kpalmberg
Copy link

Thanks for the addition @creativebrother. I am very happy to see this incoming to 1.2.0 and I'm sure many others in the future will appreciate it as well.

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.

3 participants