Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for multiple authentication challenges in WWW-Authenticate header #9242

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1cd663c
Add a setting to control WWW-Authenticate header behavior
waxlamp Jan 26, 2024
0a53bb0
Implement alternative WWW-Authenticate generation behaviors
waxlamp Jan 26, 2024
8c23de2
Add documentation for new setting and behavior
waxlamp Jan 26, 2024
525979e
Add a system check for WWW_AUTHENTICATE_BEHAVIOR setting
waxlamp Jan 29, 2024
ea612e2
Fix isort error
waxlamp Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rest_framework/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Author

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.

21 changes: 20 additions & 1 deletion rest_framework/checks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.core.checks import Tags, Warning, register
from django.core.checks import Tags, Error, Warning, register


@register(Tags.compatibility)
Expand All @@ -19,3 +19,22 @@ def pagination_system_check(app_configs, **kwargs):
)
)
return errors


@register(Tags.compatibility)
Copy link
Collaborator

@peterthomassen peterthomassen Apr 28, 2025 •
edited
Loading

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.

Copy link
Author

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.

Copy link
Collaborator

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.

def www_authenticate_behavior_setting_check(app_configs, **kwargs):
errors = []
# WWW_AUTHENTICATE_BEHAVIOR setting must be 'first' or 'all'
from rest_framework.settings import api_settings
setting = api_settings.WWW_AUTHENTICATE_BEHAVIOR
if setting not in ['first', 'all']:
errors.append(
Error(
"The rest_framework setting WWW_AUTHENTICATE_BEHAVIOR must be either "
f"'first' or 'all' (it is currently set to '{setting}').",
hint="Set WWW_AUTHENTICATE_BEHAVIOR to either 'first' or 'all', "
"or leave it unset (the default value is 'first').",
id="rest_framework.E001",
)
)
return errors