[Django] #35555: Add additional middleware checks for django.contrib.auth

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: | Version: dev
contrib.auth | Keywords: auth session
Severity: Normal | middleware checks
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The `django.contrib.auth.checks` module defines a
[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/checks.py#L240-L262
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:

[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L91-L120
RemoveUserMiddleware] raises a `ImproperlyConfigured` error if it's
enabled but `AuthenticationMiddleware` isn't enabled, or is placed after
it.

[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L27-L38
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Hisham Mahmood):

This [https://github.com/django/django/pull/17792#discussion_r1581447116
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:1>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:2>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:3>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:4>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jaap Roes:

Old description:

> The `django.contrib.auth.checks` module defines a
> [https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/checks.py#L240-L262
> 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:
>
> [https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L91-L120
> RemoveUserMiddleware] raises a `ImproperlyConfigured` error if it's
> enabled but `AuthenticationMiddleware` isn't enabled, or is placed after
> it.
>
> [https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L27-L38
> 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.

New description:

The `django.contrib.auth.checks` module defines a
[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/checks.py#L240-L262
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:

[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L91-L120

RemoteUserMiddleware] raises a `ImproperlyConfigured` error if it's

enabled but `AuthenticationMiddleware` isn't enabled, or is placed after
it.

[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L27-L38
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.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:5>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:6>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:7>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I'm just pointing out a possibility in response to "unless there's any
reason not to do this."
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:8>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:9>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody

Type: | Status: closed

Cleanup/optimization |
Component: contrib.auth | Version: dev

Severity: Normal | Resolution: wontfix

Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* resolution: => wontfix
* status: new => closed

Comment:

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!
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:10>

[Django]

#35555: Add additional middleware checks for django.contrib.auth
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: auth session | Triage Stage:
middleware checks | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jaap Roes):

Thanks Mariusz for highlighting that contribution! I wasn't aware of
`admin.E410`. It's interesting, the current implementation of
[https://github.com/django/django/blob/e56a32b89bb7fadffdfaa2cdf12b4863ccd5af9b/django/contrib/admin/checks.py#L158-L173
admin.E410] has a hint that almost mirrors the message of the runtime
check in
[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L27-L38
AuthenticationMiddleware].

The checks for `AuthenticationMiddleware`
([https://github.com/django/django/blob/e56a32b89bb7fadffdfaa2cdf12b4863ccd5af9b/django/contrib/admin/checks.py#L138-L147
admin.E408] and
[https://github.com/django/django/blob/e56a32b89bb7fadffdfaa2cdf12b4863ccd5af9b/django/contrib/auth/checks.py#L240-L262
auth.E013]) have the same goal as the runtime check in
[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L91-L120
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
[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L91-L120
RemoteUserMiddleware] and
[https://github.com/django/django/blob/f1705c8780c0a7587654fc736542d55fe4a7f29b/django/contrib/auth/middleware.py#L27-L38
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35555#comment:11>