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

add PKCE to SessionRefresh middleware and refactor #512

Conversation

escattone
Copy link
Contributor

@escattone escattone commented Dec 23, 2023

This PR addresses #397 (comment).

@escattone escattone force-pushed the consider-PKCE-in-session-refresh-middleware branch 13 times, most recently from 5d138cc to e3f1e26 Compare January 3, 2024 21:17
@escattone escattone marked this pull request as ready for review January 3, 2024 22:20
@escattone escattone requested a review from akatsoulas January 3, 2024 22:20
@escattone escattone force-pushed the consider-PKCE-in-session-refresh-middleware branch 2 times, most recently from 75bf68b to 7ee4bc5 Compare January 4, 2024 23:04
@escattone escattone force-pushed the consider-PKCE-in-session-refresh-middleware branch from 7ee4bc5 to ae4ca3c Compare January 5, 2024 23:14
@@ -159,3 +162,88 @@ def add_state_and_verifier_and_nonce_to_session(
"nonce": nonce,
"added_on": time.time(),
}


class AuthorizationCodeRequestMixin:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it makes a lot of sense to introduce this mixin and use it in both places, I am concerned that introducing this change will also introduce complexity and it will make it harder to modify/debug/override code. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akatsoulas and I discussed the pros and cons of this via video. Closing this in favor of #515.

@escattone escattone closed this Jan 9, 2024
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