-
Notifications
You must be signed in to change notification settings - Fork 4
Fix middleware exemption check #576
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
Conversation
2ca1b4f to
ebd0aef
Compare
ebd0aef to
2184b36
Compare
| return True | ||
| return is_basic_auth_exempt(view_func.__wrapped__) | ||
| except AttributeError: | ||
| return False |
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 makes sense to me and accounts for varying decorator implementations. login_not_required, for example, doesn't use wraps.
Another option might be (continuing your idea in the Slack thread of using __qualname__):
def view_func_identifier(view_func: Callable) -> str:
return f"{view_func.__module__}.{view_func.__qualname__}"
def basic_auth_exempt(view_func: Callable) -> Callable:
"""Mark a view function as exempt from BasicAuthMiddleware.
Uses a registry approach that is decorator-order independent.
"""
_basic_auth_exempt_views.add(view_func_identifier(view_func))
return view_func
def is_basic_auth_exempt(view_func: Callable) -> bool:
"""Check if a view function is exempt from BasicAuthMiddleware."""
return view_func_identifier(view_func) in _basic_auth_exempt_views
The needs some minor amendments to the assertions to get the existing tests to pass, and passes for your newly added test without modifications.
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'm happy to go with this suggestion. The one misgiving I had around the __wrapped__ solution was that it addressed one specific scenario whereas the module + function name detection seems a better general approach.
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.
@malcolmbaig I've amended to use view_func_identifier I've also tested locally with curl and basic auth is disabled for the notifications callback view.
2184b36 to
ec4ee10
Compare
ec4ee10 to
424d33d
Compare
malcolmbaig
left a comment
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.
LG 👍🏼 .
Description
https://screening-discovery.slack.com/archives/C08R0BT5HH6/p1760623960488199
Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11319
Review notes
@MatMoore @malcolmbaig after investigating this further the problem was as Mat mentioned in the Slack thread ie. how other decorators using
functools.wrapsreturn a wrapped function which can't then be matched usingview_func in _basic_auth_exempt_viewsbecause the function we add to that set is wrapped.There are other decorators which might need the same treatment but wanted to get some feedback on this approach to solving it.
[Edit] see discussion, @malcolmbaig 's suggestion seemed a better general approach to identifying the decorated function