Skip to content
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

fix context processor #21

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

kostrom
Copy link

@kostrom kostrom commented Jul 27, 2021

In some unusual cases (which are probably poor practice on their own) the context_processors in password_policies can raise an exception, preventing other handlers from handling the real exception.

Ideally, we don't want password_policies making that situation worse, so I added some safety checks, and have introduced a unit test for the context_processors.py to validate these changes.

@kostrom
Copy link
Author

kostrom commented Jul 27, 2021

I had a previous pull request that didn't have tests, because I wasn't able to recreate the same middleware behavior. This test simplifies the issue by simply calling the bad code directly. I lost the repo, so redid the pull request from scratch.

@mpasternak
Copy link
Member

Thank you for taking time to commit this patch and create unit test for it. I gave it some thought. I'm still not sure about this change. Which edge-cases did you hit exactly? The change looks legit and it has tests, but... Django usually sets request.user to something reasonable. Would you like to comment on this edge cases or so? Thanks!

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