#270, #357 - AdfsRefreshMiddleware & AdfsAuthCodeRefreshBackend #374
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge in latest changes
- Adfs refresh/access_tokens only stored when middlware activated
sdev95
changed the title
sdev95
changed the title
sdev95
changed the title
|
@sdev95 I'm not going to have time to review this for quite a while. If you don't hear from me and it's August, please feel free to @/mention me again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
As mentioned in #270, #357, this would be a great addition to the library and the other PR ( #343 ) seems stale or hopefully he is having a great long holiday :). I will attempt to continue the effort from @Dejiah and @daggaz in this PR.
I tried to solve your feedback on the previous PR @tim-schilling. The PR is not ready yet, but since I like quick feedback loops I decided to create it already. I have been working with Django for 6 months, so any general feedback besides this feature is greatly appreciated.
Feedback
Security concerns about storing token data in the session:
My current approach is to check the value of SESSION_ENGINE, and it will throw an assert message that says that it is not recommended.
Prevent the library from storing tokens in the session:
I agree with you, we should not make a choice for all users of the library to store token data in the session. I saw feedback from @daggaz in the other PR and I like the idea of having a seperate backend class that has al the logic for doing the auth code flow with refreshing.
Move as much of the new changes to the middleware instead of the backend:
I prefer a solution where we have a new backendclass.
There needs to be some documentation/tests:
I will pick this up once the general direction is agreed upon.
Kind regards and thank you for any effort in reviewing, I will have time to act upon feedback upcoming weeks 👍