search

IdentityServer / IdentityServer3.AccessTokenValidation

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

Upgrade question #82

Closed snothub closed 8 years ago

snothub commented 8 years ago

In the process of upgrading from Thinktecture.IdentityServer3.AccessTokenValidation v.1.2.2 to IdentityServer3.AccessTokenValidation v.2.8, I notice that lot of our legacy code/tests fail where checking for Thread.CurrentPrincipal. Before upgrade, this is set to the expected instance of our principal implementation, whereas after upgrade it is not. Is there a change in behaviour here somewhere?

Edit: this is causing the PrincipalPermission to fail as CurrentPrincipal is not populated. I'd like to understand where this was populated before upgrade and why it no longer is after, but I'm not seeing this.

brockallen commented 8 years ago

Are you using SuppressDefaultHostAuthentication anywhere?

snothub commented 8 years ago

I am definitely not.

brockallen commented 8 years ago

I'm not sure we changes anything that would affect this behavior (@leastprivilege would know better as that's something he worked on). Is there a version change in web api or any owin/katana component?

snothub commented 8 years ago

I'm not seeing any version changes for web api or katana, I just upgraded accesstokenvalidation middleware with the dependancy changes it brings in (like Microsoft.IdentityModel.Protocol.Extensions and IdentityModel).

In the IsAuthorized(HttpActionContext actionContext) override of AuthorizeAttribute I'm seeing GenericPrincipal on Thread.CurrentPrincipal while actionContext.RequestContext.Principal has correct value. These are both correct pre-upgrade.

snothub commented 8 years ago

I've just noticed that this only occurs in one repository, meaning behaviour is correct in another app/repo after similar upgrade. So I'll close and investigate on my end.

snothub commented 8 years ago

Last comment wasn't entirely correct, as the other repo where it worked wasn't using PrincipalPermission which seems to make a difference.

snothub commented 8 years ago

Any idea on this @leastprivilege? I'm getting nowhere with this upgrade.

leastprivilege commented 8 years ago

Sorry - no idea.

leastprivilege commented 8 years ago

Why is your scenario different to our client sample solution (and the Web API included there)?

As a side note - Thread.CurrentPrincipal is not really supported anymore in web api. Use the request context instead.

snothub commented 8 years ago

My current theory for the difference is the use of PrincipalPermissionAttribute that is no longer working. And yes, we use request context, but are still stuck with CP for legacy reasons :(

leastprivilege commented 8 years ago

PrincipalPermissionAttribute is not advisable. Replace it with [Authorize].

This is not an issue with our middleware. Why the behavior is different between the versions - i don't know. sorry.

snothub commented 8 years ago

That is a different debate I'm afraid. PrincipalPermissionAttribute and AuthorizeAttribute are not directly interchangeable and even if they were I'd be looking at literally hundreds of legacy changes which just about gives me shivers just thinking about it... :)