IdentityServer / IdentityServer4.AccessTokenValidation

IdentityServer Access Token Validation for ASP.NET Core
Apache License 2.0
544 stars 214 forks source link

Adding NET452 conditional code to force HttpClient to support secure connections with TLS. #49

Closed obosha closed 7 years ago

obosha commented 7 years ago

Encountered this bug when targeting NET452 using both IdentityServer4 and IdentityServer4.AccessTokenValidation.

However the problem goes beyond this use case, as the OIDC discovery document retrieval would also fail when served from an IS4 service targeting 452, and called by a service that is targeting 452.

The line of code I added resolves this bug.

(NOTE: Sorry about the tab issue on the line of code following my additions. I'm not sure why that is there because it is not locally. Might be encoding used in the source file I guess.)

dnfclas commented 7 years ago

Hi @obosha, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

leastprivilege commented 7 years ago

Thanks for figuring that out. The concern I have here is that ServicePointManager is a global - so if set something on the MW - it might conflict with other settings in the application.

So ultimately this is a host concert - you could put that code in your startup for fix the problem, right?

obosha commented 7 years ago

This is true, a valid point, and actually what I have done on my end. So, I suppose one of two roads could be taken:

  1. Change nothing.
  2. Add more specific error message for this concern when targeting 452.

Thoughts?

On Fri, Feb 3, 2017 at 2:02 AM, Dominick Baier notifications@github.com wrote:

Thanks for figuring that out. The concern I have here is that ServicePointManager is a global - so if set something on the MW - it might conflict with other settings in the application.

So ultimately this is a host concert - you could put that code in your startup for fix the problem, right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/IdentityServer/IdentityServer4.AccessTokenValidation/pull/49#issuecomment-277187058, or mute the thread https://github.com/notifications/unsubscribe-auth/AKFlui-Hfz7worFdkUSdgh-ldHf8l46Qks5rYt8EgaJpZM4L1qaM .

leastprivilege commented 7 years ago

What error message would you add?

leastprivilege commented 7 years ago

Could you please add a comment to the docs

thanks!

obosha commented 7 years ago

Apologies, yes, I will get to that today. I was buried yesterday.

On Tue, Feb 7, 2017 at 7:23 AM, Dominick Baier notifications@github.com wrote:

Could you please add a comment to the docs

  • the error behavior you saw
  • how you fixed it

thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/IdentityServer/IdentityServer4.AccessTokenValidation/pull/49#issuecomment-277998283, or mute the thread https://github.com/notifications/unsubscribe-auth/AKFlummoMmEki7GHIVFBdqi0Ml-5zasWks5raHBjgaJpZM4L1qaM .

dnfclas commented 7 years ago

@obosha, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, DNFBOT;