search

IdentityServer / IdentityServer3.AccessTokenValidation

OWIN Middleware to validate access tokens from IdentityServer3
Apache License 2.0
91 stars 149 forks source link

Prevent NullReferenceException in UseIdentityServerBearerTokenAuthentication constructor #128

Closed fluffynuts closed 7 years ago

fluffynuts commented 7 years ago

Issue occurs when IdentityServer is set up before setting up IdentityServer Bearer Token Authentication.

I ran into this issue myself after a refactoring that happened to move lines about in a way which didn't seem as if it should break things. The issue is also discussed here: http://stackoverflow.com/questions/31977522/owin-middlware-with-identityserver3-accesstokenvalidation-in-asp-net-5-returns#31989005 with the workaround being to move usage of UseIdentityServerBearerTokenAuthentication above usage of UseIdentityServer. I'm not advocating this as the only possible way to solve the issue. This is just one way and it has the following benefits:

I'm not sure if there's a good reason why UseIdentityServerBearerTokenAuthentication must be called before UseIdentityServer when embedding IdentityServer. I'm open to correction on this PR.

dnfclas commented 7 years ago

Hi @fluffynuts, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas commented 7 years ago

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

leastprivilege commented 7 years ago

Isn't there a default logger/factory in Katana itself. Why not simple use that when the logger has not been setup by the host?

fluffynuts commented 7 years ago

As noted in the PR: this isn't necessarily the /best/ way to solve the problem. It's just one way. But considering that the problem comes from bad OWIN behaviour, it might be a valid choice to eschew OWIN here. Alternatively, I'd welcome solid, working OWIN-based behaviour which circumvents the erroneous outcome, so whatever works. I confess to not knowing enough about OWIN to whip up an alternative PR aligned with your suggestion though. tl;dr I'd just really like to see the issue fixed and I'm not too concerned about how, though at the juncture that got us here, tbh, I don't trust OWIN. It already misbehaved once, imo.

leastprivilege commented 7 years ago

I don't have the time to look into that right now.

But if you do - check the OWIN logging source. IIRC there is a default implementation and you could set that when needed.

This PR is against master - we would need a new one against dev.

leastprivilege commented 7 years ago

see here

https://github.com/IdentityServer/IdentityServer3.AccessTokenValidation/blob/master/source/AccessTokenValidation.Tests/Util/PipelineFactory.cs#L16