aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
967 stars 334 forks source link

Importance of UseStageMarker in Authentication Middleware in Owin #173

Closed Kahbazi closed 6 years ago

Kahbazi commented 6 years ago

In most of Authentication Middlewares which often are used in mode of AuthenticationMode.Active like Cookie, OAuthBearer, IdentityServer3 after registering the middleware to pipeline we have the code

app.UseStageMarker(PipelineStage.Authenticate)

I understand that it will cause IIS to change its integrated pipeline context to authenticate as part of Application Life Cycle.

But is it really necessary to add app.UseStageMarker(PipelineStage.Authenticate) and Why? What would happen if I don't add that?

I'm writing a Custom Authentication Middleware and I'm not sure if I should add app.UseStageMarker(PipelineStage.Authenticate) after registering my middleware.

because changing it would cause me some problems like I lost CultureInfo.CurrentCulture when IIS Context is changed.

app.Use(async (context, next) =>
{
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> en-US
    CultureInfo.CurrentCulture = new CultureInfo("fa-IR");
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> fa-IR
    await next();
});
app.UseStageMarker(PipelineStage.Authenticate);
app.Use(async (context, next) =>
{
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> en-US
    CultureInfo.CurrentCulture = new CultureInfo("fa-IR");
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> fa-IR
    await next();
});

I try to disable app.UseStageMarker(PipelineStage.Authenticate) and I did it by removing the method that it calls from AppBuilder.Properties

app.Properties.Remove("integratedpipeline.StageMarker")

And it works, but I'm not sure that this is a good move! I'm worry that it might cause problem somewhere else.

Is it a big deal if I run all of my owin middlewares even authentication in PreHandlerExecute which is the default stage?

Tratcher commented 6 years ago

The stage markers are primarily about making sure middleware have run before you take any action that may depend on them. E.g. the authentication middleware should run and produce a ClaimsPrincipal before the authorization code runs and inspects that principal. If your app works without the stage markers then that's fine.

Kahbazi commented 6 years ago

@Tratcher thanks for the info. I want to ask this specific question.

CultureInfo.CurrentCulture would reset after changing pipelineStage in IIS. Do you consider this a bug?

app.Use(async (context, next) =>
{
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> en-US
    CultureInfo.CurrentCulture = new CultureInfo("fa-IR");
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> fa-IR
    await next();
});
app.UseStageMarker(PipelineStage.Authenticate);
app.Use(async (context, next) =>
{ 
    Debug.WriteLine(CultureInfo.CurrentCulture);// --> en-US
    await next();
});
Tratcher commented 6 years ago

That does seem problematic.

Kahbazi commented 6 years ago

I know this is IIS problem, since it doesn't have any problem in self-host. So I guess there's nothing wrong with katana. To Which team I should report this issue? Do you think this will get fixed?

Tratcher commented 6 years ago

It might be something we could address in Katana, and if not we'll send it to the correct team internally. Either way a fix won't be available for some time.

Kahbazi commented 6 years ago

@Tratcher Thank you. Right now I use this workaround which ignore UseStageMarker and I guess it only works correctly if the application doesn't use any IIS HttpModule (beside Owin of course) which need to be run on certain stages, because with this workaround all of owin application even authentications middleware run on PreHandlerExecute stage.

I use this code app.Properties.Remove("integratedpipeline.StageMarker") which remove the method that UseStageMarker calls.

I like to know your thought about this workaround?

Tratcher commented 6 years ago

Creative. If your app works then go right ahead.

muratg commented 6 years ago

We don't plan to fix this issue.