-
Notifications
You must be signed in to change notification settings - Fork 81
[POC] Add AnsibleBaseCsrfViewMiddleware and update SessionAuthentication to use get_setting #742
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
[POC] Add AnsibleBaseCsrfViewMiddleware and update SessionAuthentication to use get_setting #742
Conversation
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.
Pull Request Overview
This PR introduces dynamic configuration support for CSRF by implementing a custom middleware that leverages ansible_base’s get_setting, and updates session authentication to use this functionality.
- Introduces AnsibleBaseCsrfViewMiddleware and AnsibleBaseCSRFCheck to dynamically load CSRF_TRUSTED_ORIGINS.
- Updates SessionAuthentication.enforce_csrf() to integrate the new middleware for CSRF validation.
- Adds comprehensive tests to verify the new dynamic behavior for CSRF handling.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
test_app/tests/authentication/test_middleware.py | Added tests for verifying the new CSRF middleware and check behavior. |
ansible_base/authentication/session.py | Introduced AnsibleBaseCSRFCheck and updated SessionAuthentication to use it. |
ansible_base/authentication/middleware.py | Added AnsibleBaseCsrfViewMiddleware to dynamically load CSRF_TRUSTED_ORIGINS. |
if "*" in origin | ||
): | ||
allowed_origin_subdomains[parsed.scheme].append( | ||
parsed.netloc.lstrip("*") |
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.
[nitpick] Consider extracting the logic for stripping wildcard characters from the netloc into a helper function to reduce duplication and improve readability.
parsed.netloc.lstrip("*") | |
self._strip_wildcard_from_netloc(parsed.netloc) |
Copilot uses AI. Check for mistakes.
Django settings. | ||
This allows the setting to be dynamically loaded from various sources | ||
as configured by the ANSIBLE_BASE_SETTINGS_FUNCTION setting. |
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 like the composability (is this a word?) here
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.
Pass it through the linter, make a Jira issue able to be picked up via DVCS, and it'll have my approval.
… use get_setting - Add AnsibleBaseCsrfViewMiddleware that reads CSRF_TRUSTED_ORIGINS using ansible_base.lib.utils.settings.get_setting instead of directly from Django settings - Override all three cached properties (csrf_trusted_origins_hosts, allowed_origins_exact, allowed_origin_subdomains) to use get_setting for dynamic configuration - Add AnsibleBaseCSRFCheck class that inherits from AnsibleBaseCsrfViewMiddleware - Modify SessionAuthentication.enforce_csrf to use AnsibleBaseCSRFCheck instead of Django's default CSRFCheck - Add comprehensive tests for both middleware and session authentication CSRF functionality - Enables CSRF_TRUSTED_ORIGINS to be dynamically loaded from various sources via ANSIBLE_BASE_SETTINGS_FUNCTION while maintaining backward compatibility
4071571
to
bdfc0e2
Compare
DVCS PR Check Results: Could not find JIRA key(s) in PR title, branch name, or commit messages |
|
Is there a Jira work item for this work? |
This seems very related to #744 |
Note: this is In direct conflict with #755. Started with this one, started trying different implementations for doing the settings hack, 755 is the result. |
Closing in favor of #755 |
Description
This PR adds dynamic configuration support for
CSRF_TRUSTED_ORIGINS
by implementing a custom CSRF middleware that usesansible_base.lib.utils.settings.get_setting
instead of directly accessing Django settings.What is being changed?
AnsibleBaseCsrfViewMiddleware
class that inherits from Django'sCsrfViewMiddleware
AnsibleBaseCSRFCheck
class for session authentication CSRF validationSessionAuthentication.enforce_csrf()
to use the new custom CSRF middlewareWhy is this change needed?
CSRF_TRUSTED_ORIGINS
to be dynamically loaded from various sources viaANSIBLE_BASE_SETTINGS_FUNCTION
How does this change address the issue?
CsrfViewMiddleware
that accesssettings.CSRF_TRUSTED_ORIGINS
Type of Change
Self-Review Checklist
Testing Instructions
Prerequisites
ANSIBLE_BASE_SETTINGS_FUNCTION
configurationSteps to Test
Test Dynamic CSRF Configuration:
Test Session Authentication CSRF:
Run the test suite:
Expected Results
CSRF_TRUSTED_ORIGINS
should be read usingget_setting()
instead of direct Django settings accessget_setting
AnsibleBaseCSRFCheck
inherits fromAnsibleBaseCsrfViewMiddleware
PermissionDenied
exceptionsAdditional Context
Implementation Details
The implementation addresses the memory from previous interactions about Django accessing
settings.CSRF_TRUSTED_ORIGINS
directly in multiple places. Instead of just overriding a single property, this solution:Overrides all three cached properties that access
CSRF_TRUSTED_ORIGINS
:csrf_trusted_origins_hosts
- strips*
from netloc for host validationallowed_origins_exact
- filters origins without wildcardsallowed_origin_subdomains
- groups wildcard origins by schemeMaintains exact Django logic - each overridden method replicates Django's original implementation exactly, just using
get_setting('CSRF_TRUSTED_ORIGINS', [])
instead ofsettings.CSRF_TRUSTED_ORIGINS
Integrates with session authentication - creates
AnsibleBaseCSRFCheck
that DRF'sSessionAuthentication
can use for CSRF validationPerformance Considerations
@cached_property
decorators to cache results and avoid repeatedget_setting
callsget_setting
call per propertySecurity Impact
Required Actions