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

Return authorization URL response on authorize call, return BadReques… #428

Merged
merged 6 commits into from
Apr 30, 2019

Conversation

feordin
Copy link
Contributor

@feordin feordin commented Apr 19, 2019

Forward errors from authorization URL to caller, and return BadRequest error on missing params

Description

Updated authorize route in proxy to return the result from the redirect. For other errors with missing params, return a BadRequest result.

Related issues

Addresses [issue AB#68689].

Testing

Unit test created and run

@feordin feordin requested a review from brandonpollett April 19, 2019 23:59
@hansenms
Copy link
Contributor

I am not convinced that we need to add src/Microsoft.Health.Fhir.Api.UnitTests/Controllers/openid-configuration.json, could we not just in the test provide the https://login.microsoftonline.com/common endpoint as authority and the controller would pull the openid-configuration as normal?

@brandonpollett
Copy link
Contributor

I am not convinced that we need to add src/Microsoft.Health.Fhir.Api.UnitTests/Controllers/openid-configuration.json, could we not just in the test provide the https://login.microsoftonline.com/common endpoint as authority and the controller would pull the openid-configuration as normal?

I think that would make it more of an integration test. We may be able to just mock out the returned configuration instead of adding it.

@feordin
Copy link
Contributor Author

feordin commented Apr 22, 2019

I am not convinced that we need to add src/Microsoft.Health.Fhir.Api.UnitTests/Controllers/openid-configuration.json, could we not just in the test provide the https://login.microsoftonline.com/common endpoint as authority and the controller would pull the openid-configuration as normal?

I think that would make it more of an integration test. We may be able to just mock out the returned configuration instead of adding it.

As Brandon said, I was trying to avoid having the test call out to any external resources by adding the file. As it is now, the file is used as the mock response which the HttpClient sends back. It was just a long enough string it seemed easier to place it in a separate file rather than embed it as a string in the C# file.

@feordin feordin force-pushed the personal/jaerwin/Aad500Error branch from 523e772 to 987a0da Compare April 26, 2019 19:00
@feordin feordin merged commit 08cc609 into master Apr 30, 2019
@feordin feordin deleted the personal/jaerwin/Aad500Error branch April 30, 2019 23:00
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