-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Add support for multiple authentication challenges in WWW-Authenticate header #9242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
cc3a7e2
to
525979e
Compare
@@ -8,3 +8,4 @@ class RestFrameworkConfig(AppConfig): | |||
def ready(self): | |||
# Add System checks | |||
from .checks import pagination_system_check # NOQA | |||
from .checks import www_authenticate_behavior_setting_check # NOQA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line necessary? In my local build I was able to trigger the new error without it; I merely copied the pattern from the line above in my PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question; have you been able to understand this further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is following this advice from the Django docs:
Checks should be registered in a file that’s loaded when your application is loaded; for example, in the AppConfig.ready() method.
When you say you were able to trigger the new error without this: did this happen on startup, or when you ran check
manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand this better now.
The @register
decorator causes the check function to be registered; all that's required beyond using the decorator is to arrange to load the module in which those functions live. That's why omitting this line still allows my check to be performed (on both startup, and when invoking the check
management command manually, as it happens): the previous line causes the whole module to load, which causes not just pagination_system_check
but my new check function to be registered as well.
I tested this theory by removing both lines; in that case, my check does not run. But this means that both lines can be replaced with just from . import checks
; that's sufficient to register all the check functions in that module.
I hope all this made sense 🙂. If it does, I'll plan to make that change and resolve this conversation.
I am not sure what benefit this will provide? |
DRF supports having multiple alternative authentication schemes (which is great), but is not announcing that in the 401 |
Essentially, the value is in fulfilling the RFC's description of |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@@ -78,6 +78,7 @@ | |||
# Authentication | |||
'UNAUTHENTICATED_USER': 'django.contrib.auth.models.AnonymousUser', | |||
'UNAUTHENTICATED_TOKEN': None, | |||
'WWW_AUTHENTICATE_BEHAVIOR': 'first', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this a boolean, e.g. WWW_AUTHENTICATE_ALL = False
(by default)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I was leaving the door open for other modes beyond 'first'
and 'all'
, but I think a boolean setting makes more sense here.
@@ -84,7 +84,7 @@ When an unauthenticated request is denied permission there are two different err | |||
* [HTTP 401 Unauthorized][http401] | |||
* [HTTP 403 Permission Denied][http403] | |||
|
|||
HTTP 401 responses must always include a `WWW-Authenticate` header, that instructs the client how to authenticate. HTTP 403 responses do not include the `WWW-Authenticate` header. | |||
HTTP 401 responses must always include a `WWW-Authenticate` header, that instructs the client how to authenticate. The `www_authenticate_behavior` setting controls how the header is generated: if set to `'first'` (the default), then only the text for the first scheme in the list will be used; if set to `'all'`, then a comma-separated list of the text for all the schemes will be used (see [MDN WWW-Authenticate](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate) for more details). HTTP 403 responses do not include the `WWW-Authenticate` header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting name should be uppercase
Determines whether a single or multiple challenges are presented in the `WWW-Authenticate` header. | ||
|
||
This should be set to `'first'` (the default value) or `'all'`. When set to `'first'`, the `WWW-Authenticate` header will be set to an appropriate challenge for the first authentication scheme in the list. | ||
When set to `'all'`, a comma-separated list of the challenge for all specified authentication schemes will be used instead (following the [syntax specification](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC 9110 also warns:
Some user agents do not recognize this form, however. As a result, sending a WWW-Authenticate field value with more than one member on the same field line might not be interoperable.
Perhaps we should have a similar warning somewhere, either here or in authentication.md
.
@@ -8,3 +8,4 @@ class RestFrameworkConfig(AppConfig): | |||
def ready(self): | |||
# Add System checks | |||
from .checks import pagination_system_check # NOQA | |||
from .checks import www_authenticate_behavior_setting_check # NOQA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question; have you been able to understand this further?
@@ -19,3 +19,22 @@ def pagination_system_check(app_configs, **kwargs): | |||
) | |||
) | |||
return errors | |||
|
|||
|
|||
@register(Tags.compatibility) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should be Tags.security
, see for example all the checks in https://github.com/django/django/blob/0ee06c04e0256094270db3ffe8b5dafa6a8457a3/django/core/checks/security/base.py
Those checks also use deploy=True
, which causes them to only run with check --deploy
. Not sure is this is a good idea. OTOH, it's done the same way for all the other validity checks for security settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with Tags.compatibility
largely because the other existing check is also looking to validate settings (which is all my check does, really). Tags.security
seems to have more to do with actual security checks (rather than mere misconfiguration). Apparently it is also possible to omit the tag entirely.
I'm happy to go with any of these options, just let me know what you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have no strong preference.
This adds a setting to enable emitting a comma-separated list of challenges in the
WWW-Authenticate
header that is returned with a 401 response.Fixes #7328 and resolves #7812.