Monday, 19 October 2020

The Mysterious Case of Safari and the Empty Download

TL;DR: Safari wants a Content-Type header in responses. Even if the response is Content-Length: 0. Without this, Safari can attempt to trigger an empty download. Don't argue; just go with it; some browsers are strange.

The longer version

Every now and then a mystery presents itself. A puzzle which just doesn't make sense and yet stubbornly continues to exist. I happened upon one of these the other day and to say it was frustrating does it no justice at all.

It all came back to the default iOS and Mac browser; Safari. When our users log into our application, they are redirected to a shared login provider which, upon successful authentication, hands over a cookie containing auth details and redirects back to our application. A middleware in our app reads what it needs from the cookie and then creates a cookie of its own which is to be used throughout the session. As soon as the cookie is set, the page refreshes and the app boots up in an authenticated state.

That's the background. This mechanism had long been working fine with Chrome (which the majority of our users browse with), Edge, Firefox and Internet Explorer. But we started to get reports from Safari users that, once they'd supplied their credentials, they'd not be authenticated and redirected back to our application. Instead they'd be prompted to download an empty document and the redirect would not take place.

As a team we could not fathom why this should be the case; it just didn't make sense. There followed hours of experimentation before Hennie noticed something. It was at the point when the redirect back to our app from the login provider took place. Specifically the initial response that came back which contained our custom cookie and a Refresh: 0 header to trigger a refresh in the browser. There was no content in the response, save for headers. It was Content-Length: 0 all the way.

Hennie noticed that there was no Content-Type set and wondered if that was significant. It didn't seem like it would be a necessary header given there was no content. But Safari reckons not with logic. As an experiment we tried setting the response header to Content-Type: text/html. It worked! No mystery download, no failed redirect (which it turned out was actually a successful redirect which wasn't being surfaced in Safari's network request tab).

It appears that always providing a Content-Type header in your responses is wise if only for the case of Safari. In fact, it's generally unlikely that this won't be set anyway, but it can happen as we have experienced. Hopefully we've suffered so you don't have to.

Friday, 2 October 2020

Autofac 6, integration tests and .NET generic hosting

I blogged a little while ago around to support integration tests using Autofac. This was specific to Autofac but documented a workaround for a long standing issue with ConfigureTestContainer that was introduced into .NET core 3.0 which affects all third-party containers that use ConfigureTestContainer in their tests.

I'll not repeat the contents of the previous post - it all still stands. However, with Autofac 6 the approach documented there will cease to work. This is because the previous approach relied upon ContainerBuilder not being sealed. As of Autofac 6 it is.

Happily the tremendous Alistair Evans came up with an alternative approach which is listed below:

    /// <summary>
    /// Based upon https://github.com/dotnet/AspNetCore.Docs/tree/master/aspnetcore/test/integration-tests/samples/3.x/IntegrationTestsSample
    /// </summary>
    /// <typeparam name="TStartup"></typeparam>
    public class AutofacWebApplicationFactory<TStartup> : WebApplicationFactory<TStartup> where TStartup : class
    {
        protected override IHost CreateHost(IHostBuilder builder)
        {
            builder.UseServiceProviderFactory<ContainerBuilder>(new CustomServiceProviderFactory());
            return base.CreateHost(builder);
        }
    }

    /// <summary>
    /// Based upon https://github.com/dotnet/aspnetcore/issues/14907#issuecomment-620750841 - only necessary because of an issue in ASP.NET Core
    /// </summary>
    public class CustomServiceProviderFactory : IServiceProviderFactory<ContainerBuilder>
    {
        private AutofacServiceProviderFactory _wrapped;
        private IServiceCollection _services;

        public CustomServiceProviderFactory()
        {
            _wrapped = new AutofacServiceProviderFactory();
        }

        public ContainerBuilder CreateBuilder(IServiceCollection services)
        {
            // Store the services for later.
            _services = services;

            return _wrapped.CreateBuilder(services);
        }

        public IServiceProvider CreateServiceProvider(ContainerBuilder containerBuilder)
        {
            var sp = _services.BuildServiceProvider();
#pragma warning disable CS0612 // Type or member is obsolete
            var filters = sp.GetRequiredService<IEnumerable<IStartupConfigureContainerFilter<ContainerBuilder>>>();
#pragma warning restore CS0612 // Type or member is obsolete

            foreach (var filter in filters)
            {
                filter.ConfigureContainer(b => { })(containerBuilder);
            }

            return _wrapped.CreateServiceProvider(containerBuilder);
        }        
    }

Using this in place of the previous approach should allow you continue running your integration tests with Autofac 6. Thanks Alistair!

Concern for third-party containers

Whilst this gets us back up and running, Alistair pointed out that this approach depends upon a deprecated interface. This is the IStartupConfigureContainerFilter which has been marked as Obsolete since mid 2019. What this means is, at some point, this approach will stop working.

The marvellous David Fowler has said that ConfigureTestContainer issue should be resolved in .NET. However it's worth noting that this has been an issue since .NET Core 3 shipped and unfortunately the wonderful Chris Ross has advised that it's not likely to be fixed for .NET 5.

I'm very keen this does get resolved in .NET. Building tests upon an Obsolete attribute doesn't fill me with confidence. I'm a long time user of Autofac and I'd like to continue to be. Here's hoping that's made possible by a fix landing in .NET. If this is something you care about, it may be worth upvoting / commenting on the issue in GitHub so the team are aware of desire around this being resolved.