aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

The redirection middleware doesn't play well with in memory functional tests #288

Closed javiercn closed 6 years ago

javiercn commented 6 years ago

Apparently there are no server addresses feature on the TestServer and that causes the middleware to blow up.

https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.HttpsPolicy/HttpsRedirectionMiddleware.cs#L32

@Tratcher Is there something reasonable we can do here for tests? Like allow configuring the addresses on the test server for tests

javiercn commented 6 years ago

I've confirmed that the issue is due to the lack of server addresses. It works fine when I added them myself.

Tratcher commented 6 years ago

Justin made this work in his tests by adding the feature manually.

The server does have a base address property we could use to auto populate the feature, but you'd never get more than one address. That might be enough for these tests.

Tratcher commented 6 years ago

Nevermind, the middleware should allow the feature to be null and fall back to 443.

javiercn commented 6 years ago

Yeah, good point.

It's failing at injection time, so we might just do

app.ServerFeatures.GetFeaure<IServerAddressesFeature>() ?? new NoAddressesFeature()

That way we use the class as a hint to the middleware to make DI work and have the middleware redirect to 443

Tratcher commented 6 years ago

Does it need a value at all?

jkotalik commented 6 years ago

Right now I throw if the ServerAddress feature is null https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.HttpsPolicy/HttpsRedirectionMiddleware.cs#L40. From feedback, I'll go ahead and change the behavior to not throw if the feature isn't set and return 443.

javiercn commented 6 years ago

@Tratcher The value is just for DI to be happy. The problem is we are passing down null and that makes activator utilities blow up. The NoAddressesFeature is a special value to signal the middleware that it neesds to do 443. Or if everything just works with an empty ServerAddressesFeature, just use that.

javiercn commented 6 years ago

@jkotalik Ah, good point. I actually didn't dig deep enough, and its just constructor throwing. Nevermind anything I said, just make it work with the null value by assuming 443

jkotalik commented 6 years ago

@javiercn I was mistaken, this also fails at injection time. I think we will have to do something like:

app.ServerFeatures.GetFeaure<IServerAddressesFeature>() ?? new NoAddressesFeature()

or just add a second constructor to HttpsMiddleware that doesn't take an IServerAddressFeature.

javiercn commented 6 years ago

Done 726035e