Skip to content

Handle early Responses #35

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

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

Conversation

willstott101
Copy link
Contributor

@willstott101 willstott101 commented Nov 4, 2021

Responses can be returned by middleware before reaching AuthenticationMiddleware - in these cases there is no request.user attribute.

This happens with Django's default MIDDLEWARE ordering if the current host is disallowed (not in ALLOWED_HOSTS) for instance.

Responses can be returned by middleware before reaching AuthenticationMiddleware - in these cases there is no request.user attribute. This happnes with the default Django MIDDLEWARE ordering if the current host is disallowed (not in ALLOWED_HOSTS)
@btimby
Copy link
Contributor

btimby commented Nov 4, 2021

Looks good, can you think of a way to add a unit test? I skimmed over the tests and did not see anything similar. This may require a new TestCase that tests the middleware class directly.

@btimby
Copy link
Contributor

btimby commented Nov 4, 2021

For reference, these views are used by some of the tests:

https://github.com/smartfile/django-session-jwt/blob/master/django_session_jwt/views.py

And the tests themselves are here, at the top of the file are some low-level tests (mocks and method calls). Tests for the middleware would fit roughly into this category. The second section uses the views above.

https://github.com/smartfile/django-session-jwt/blob/master/django_session_jwt/tests.py

@willstott101
Copy link
Contributor Author

Added a test that fails before this patch

@willstott101
Copy link
Contributor Author

Moved the test to the existing ViewTestCase as that's the level it's operating at right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants