Add additional middleware checks for django.contrib.auth

[Jaap Roes]

The django.contrib.auth.checks module defines a ​check_middleware function. This function currently checks if LoginRequiredMiddleware is enabled, if so it checks if AuthenticationMiddleware is also enabled, and is placed before it.

This is nice, and something than happens at runtime for other middlewares in contrib.auth, for example:

​RemoteUserMiddleware raises a ImproperlyConfigured error if it's enabled but AuthenticationMiddleware isn't enabled, or is placed after it.

​AuthenticationMiddleware itself also raises a ImproperlyConfigured error if it's enabled but SessionMiddleware is not (or isn't executed before reaching it).

I can contribute a patch, unless there's any reason not to do this.

[Hisham Mahmood]

This ​discussion is related. Basically it wasn't added because there were no similar checks in LoginRequiredMixin and also to add no extra work on per request path.

[Jaap Roes]

Thanks, yes I agree with the reasoning. I'd like to see if it's possible to convert these other runtime checks to similar Django checks, so the behaviour is consistent.

[Tim Graham]

A disadvantage I see with the system check approach is that it makes it more difficult to use a custom middleware that sets request.user and doesn't inherit AuthenticationMiddleware. In that case, the system check error has to be silenced using SILENCED_SYSTEM_CHECKS.

[Jaap Roes]

I see that, but is that a common pattern? Even RemoteUserMiddleware, which sets request.user, requires AuthenticationMiddleware in front of it (that's partially what this ticket is about).

[Tim Graham]

I don't know if it would affect anyone but it reminds me of other cases (I can't think of a concrete example offhand) where we got complaints after changing a duck-typing (similar to hasattr(request, "user")) to something more specific like a class (or even subclass) check.

[Jaap Roes]

Are you arguing that the system check for LoginRequiredMiddleware should've been a runtime/duck-type check? Or just that the other middleware checks shouldn't be changed to do the same?

[Tim Graham]

I'm just pointing out a possibility in response to "unless there's any reason not to do this."

[Mariusz Felisiak]

I think it's not worth changing existing errors to system checks. We have had to relax such checks many times in the past, e.g. efeceba589974b95b35b2e25df86498c96315518.

[Natalia Bidart]

I tend to agree with Tim and Mariusz: Given Django's maturity, we prioritize changes that clearly address bugs or introduce new features rather than optional alterations.

If there is a compelling use case or scenario where implementing this change significantly improves functionality and justifies the potential risk to existing installations, we would be open to re-evaluating this issue.

Thank you anyways for your interest in making Django better!

[Jaap Roes]

Thanks Mariusz for highlighting that contribution! I wasn't aware of admin.E410. It's interesting, the current implementation of ​admin.E410 has a hint that almost mirrors the message of the runtime check in ​AuthenticationMiddleware.

The checks for AuthenticationMiddleware (​admin.E408 and ​auth.E013) have the same goal as the runtime check in ​RemoteUserMiddleware. They all want request.user to exist, i.e. for AuthenticationMiddleware to be enabled.

I do agree that there is a fundamental difference between a duck-type check and a subclass check. Currently Django inconsistently applies both to achieve the same goal.

In the cases of ​RemoteUserMiddleware and ​AuthenticationMiddleware, both error message point to the exact middleware class that is expected to be enabled. So while alternative implementations may be able to pass the current implementation, it isn't in really the true spirit of these checks.

As Tim identified, the major downside to adding these checks is that some user might have to add these checks to SILENCED_SYSTEM_CHECKS. The upside is that Django is more consistent (and helpful) when configuring contrib.auth. Currently contrib.auth takes some of that responsibility, but not every project uses that.

I've prepared a pull request on GitHub, to make it easier to see how similar all these cases are. Sadly this ticket was just closed as WONTFIX while doing this. I hope someone will still take the time to take a look at it.