[Django] #35971: RemoteUserMiddleware needs a get_username method

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+-----------------------------------------
Reporter: adk-swisstopo | Type: Uncategorized
Status: new | Component: contrib.auth
Version: dev | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+-----------------------------------------
As currently implemented, the only way to customise how
RemoteUserMiddleware gets the username is through the "header" variable.
This is then used in __call__ and __acall__ methods like this:

{{{
username = request.META[self.header]
}}}

It would be convenient to move that logic into a separate method that
could be overridden. For example:
{{{
get_username(self):
return request.META[self.header]
}}}

Specific use case: the proxy I have in front of Django always sets two
specific headers (say "X-Username" and "X-Authenticated"). The value of
"X-Username" is only valid if "X-Authenticated" is "true", otherwise it
should be ignored (typically it ends up being a single space character). I
use PersistentRemoteMiddleware to use X-Username but the only way I found
to ignore it when X-Authenticated is not true is to override __call__ /
__acall__ or clean_username. Both seem rather fragile while a small change
to RemoteUserMiddleware would make for a much more robust, flexible and
maintainable solution.

With the proposed change, in my child class I could just say
{{{
def get_username(self):
if request.META["X-Authenticated"].lower() != "true":
raise KeyError
else:
return request.META[self.header]
}}}

I am happy to propose a patch if we can agree this change is desirable.
--
Ticket URL: <https://code.djangoproject.com/ticket/35971>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+--------------------------------------

Reporter: adk-swisstopo | Owner: (none)
Type: New feature | Status: new
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:

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

Changes (by adk-swisstopo):

* type: Uncategorized => New feature

--
Ticket URL: <https://code.djangoproject.com/ticket/35971#comment:1>

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+--------------------------------------

Reporter: Adrien Kunysz | Owner: (none)

Type: New feature | Status: new

Component: contrib.auth | Version: 5.0

Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Changes (by Adrien Kunysz):

* version: dev => 5.0

--
Ticket URL: <https://code.djangoproject.com/ticket/35971#comment:2>

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+--------------------------------------
Reporter: Adrien Kunysz | Owner: (none)
Type: New feature | Status: new
Component: contrib.auth | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Description changed by Adrien Kunysz:

Old description:

> As currently implemented, the only way to customise how
> RemoteUserMiddleware gets the username is through the "header" variable.
> This is then used in __call__ and __acall__ methods like this:
>
> {{{
> username = request.META[self.header]
> }}}
>
> It would be convenient to move that logic into a separate method that
> could be overridden. For example:
> {{{
> get_username(self):
> return request.META[self.header]
> }}}
>
> Specific use case: the proxy I have in front of Django always sets two
> specific headers (say "X-Username" and "X-Authenticated"). The value of
> "X-Username" is only valid if "X-Authenticated" is "true", otherwise it
> should be ignored (typically it ends up being a single space character).
> I use PersistentRemoteMiddleware to use X-Username but the only way I
> found to ignore it when X-Authenticated is not true is to override
> __call__ / __acall__ or clean_username. Both seem rather fragile while a
> small change to RemoteUserMiddleware would make for a much more robust,
> flexible and maintainable solution.
>
> With the proposed change, in my child class I could just say
> {{{
> def get_username(self):
> if request.META["X-Authenticated"].lower() != "true":
> raise KeyError
> else:
> return request.META[self.header]
> }}}
>
> I am happy to propose a patch if we can agree this change is desirable.

New description:

As currently implemented, the only way to customise how
RemoteUserMiddleware gets the username is through the "header" variable.
This is then used in __call__ and __acall__ methods like this:

{{{
username = request.META[self.header]
}}}

It would be convenient to move that logic into a separate method that
could be overridden. For example:
{{{

get_username(self, header_name):
return request.META[header_name]

}}}

Specific use case: the proxy I have in front of Django always sets two
specific headers (say "X-Username" and "X-Authenticated"). The value of
"X-Username" is only valid if "X-Authenticated" is "true", otherwise it
should be ignored (typically it ends up being a single space character). I
use PersistentRemoteMiddleware to use X-Username but the only way I found
to ignore it when X-Authenticated is not true is to override __call__ /

__acall__ , which seems rather fragile while a small change to

RemoteUserMiddleware would make for a much more robust, flexible and
maintainable solution.

With the proposed change, in my child class I could just say
{{{

def get_username(self, header_name):

if request.META["X-Authenticated"].lower() != "true":
raise KeyError

return request.META[header_name]

}}}

I am happy to propose a patch if we can agree this change is desirable.

The analysis above is for the latest version on github. I have marked this
feature request as 5.0 because that's the version I currently use and
backporting the proposed change seems easy enough.

--
--
Ticket URL: <https://code.djangoproject.com/ticket/35971#comment:3>

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+--------------------------------------
Reporter: Adrien Kunysz | Owner: (none)
Type: New feature | Status: new
Component: contrib.auth | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Description changed by Adrien Kunysz:

Old description:

> As currently implemented, the only way to customise how
> RemoteUserMiddleware gets the username is through the "header" variable.
> This is then used in __call__ and __acall__ methods like this:
>
> {{{
> username = request.META[self.header]
> }}}
>
> It would be convenient to move that logic into a separate method that
> could be overridden. For example:
> {{{

get_username(self, request, header_name):

return request.META[header_name]
}}}

Specific use case: the proxy I have in front of Django always sets two
specific headers (say "X-Username" and "X-Authenticated"). The value of
"X-Username" is only valid if "X-Authenticated" is "true", otherwise it
should be ignored (typically it ends up being a single space character). I
use PersistentRemoteMiddleware to use X-Username but the only way I found
to ignore it when X-Authenticated is not true is to override __call__ /
__acall__ , which seems rather fragile while a small change to
RemoteUserMiddleware would make for a much more robust, flexible and
maintainable solution.

With the proposed change, in my child class I could just say
{{{

def get_username(self, request, header_name):

if request.META["X-Authenticated"].lower() != "true":
raise KeyError
return request.META[header_name]
}}}

I am happy to propose a patch if we can agree this change is desirable.

The analysis above is for the latest version on github. I have marked this
feature request as 5.0 because that's the version I currently use and
backporting the proposed change seems easy enough.

--
--

Ticket URL: <https://code.djangoproject.com/ticket/35971#comment:4>

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+--------------------------------------
Reporter: Adrien Kunysz | Owner: (none)

Type: New feature | Status: closed
Component: contrib.auth | Version: 5.0
Severity: Normal | Resolution: wontfix

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

Changes (by Sarah Boyce):

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

Comment:

I think given that it's probably not too difficult to achieve this by
overriding the call method, I'm not sure it's worth adding a hook
(below is untested)
{{{#!python
class MyRemoteUserMiddleware(RemoteUserMiddleware):
def __call__(self, request):
if request.META.get(self.header) and

request.META["X-Authenticated"].lower() != "true":

if self.force_logout_if_no_header and
request.user.is_authenticated:
self._remove_invalid_user(request)
return self.get_response(request)
return super().__call__(request)
}}}

If you want it to be added, you can propose and discuss the idea on the
[https://forum.djangoproject.com/c/internals/5 Django Forum], where you'll
reach a broader audience and receive additional feedback.

I'll close the ticket for now, but if the community agrees with the
proposal, please return to this ticket and reference the forum discussion
so we can re-open it. For more information, please refer to
[https://docs.djangoproject.com/en/stable/internals/contributing/bugs-and-
features/#requesting-features the documented guidelines for requesting
features].
--
Ticket URL: <https://code.djangoproject.com/ticket/35971#comment:5>

[Django]

#35971: RemoteUserMiddleware needs a get_username method
-------------------------------+--------------------------------------
Reporter: Adrien Kunysz | Owner: (none)
Type: New feature | Status: closed
Component: contrib.auth | Version: 5.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Adrien Kunysz):

Thank you. I don't think your proposal resolves my problem but it gave me
another idea. I can just delete the entry from request.META before calling
the parent __call__. In case anyone else runs into this issue, this is
what I am doing (5.0 has process_request instead of __call__):

{{{
def process_request(self, request):
authenticated = request.META.get("X-Authenticated", "false").lower()
== "true"
if not authenticated and self.header in request.META:
del request.META[self.header]
return super().process_request(request)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35971#comment:6>