feat: add pkce-openid-backend - #255
Conversation
|
@omar-nelc could you take a look 😬 |
e29b8ba to
33c6ee0
Compare
This add a generic backend based in ConfigurableOpenIdConnectAuth but with PKCE. This backend is inspired in the social-core way to implement PKCE. There is a current PR in working, but for the moment, that class is not merged and accesible. So after that is finished this has it code for `code_challenge` and `code_challenge_method`implementation. PR: python-social-auth/social-core#856
33c6ee0 to
1584426
Compare
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Thanks @johanv26. I have added a suggestion to refactor the BaseOAuth2PKCEMixin and use it so we can refactor later and use the BaseOAuth2PKCE base class once it lands on upstream.
This usually helps with the release upgrade.
Co-authored-by: Omar Al-Ithawi @ NELC <134635705+omar-nelc@users.noreply.github.com> chore: suggestions from code review Co-authored-by: Omar Al-Ithawi @ NELC <134635705+omar-nelc@users.noreply.github.com>
d0a7b2c to
6664e76
Compare
| PKCE_DEFAULT_CODE_CHALLENGE_METHOD = "s256" | ||
| PKCE_DEFAULT_CODE_VERIFIER_LENGTH = 32 | ||
| USE_PKCE = True |
There was a problem hiding this comment.
Thanks @johanv26!! Looks much better now!
Those aren't needed, because they're inherited from the mixin.
andrey-canon
left a comment
andrey-canon
left a comment
There was a problem hiding this comment.
LGTM, I just left some comments related with the code's style that I would have implemented , however I know this is a copy of the social-core pr so feel free to ignore these I just wanted to point them out
| code_verifier = self.strategy.session_get(name) | ||
| return code_verifier |
There was a problem hiding this comment.
| code_verifier = self.strategy.session_get(name) | |
| return code_verifier | |
| code_verifier = self.strategy.session_get(name) | |
| return code_verifier |
| return code_challenge | ||
| elif method == "plain": |
There was a problem hiding this comment.
| return code_challenge | |
| elif method == "plain": | |
| return code_challenge | |
| if method == "plain": |
|
|
||
| PKCE_DEFAULT_CODE_CHALLENGE_METHOD = "s256" | ||
| PKCE_DEFAULT_CODE_VERIFIER_LENGTH = 32 | ||
| USE_PKCE = True |
There was a problem hiding this comment.
I don't understand why is this necessary, I mean the obvious answer is because you can disable that behavior on sub-classes, but if I don't want that behavior on sub-classes I could inherit from BaseOAuth2 instead of this one
|
@andrey-canon I updated to the version is approved in social_core. But taking advantage of that PR is not merged yet, I added some of your review to that PR. |
0f5ecbc to
837a4ae
Compare
Co-authored-by: Andrey Cañon <andrey.canon@edunext.co>
837a4ae to
5a4fc33
Compare
* feat: add pkce-openid-backend This adds a generic backend based on ConfigurableOpenIdConnectAuth but with PKCE. This backend is inspired in the social-core way to implement PKCE. There is a current PR in work, but for the moment, that class is not merged and accessible. So after that is finished this has it code for `code_challenge` and `code_challenge_method`implementation. PR: python-social-auth/social-core#856 * refactor: avoid repeated pkce conf definition * feat: update with small changes social_core * refactor: suggestions from code review Co-authored-by: Omar Al-Ithawi @ NELC <134635705+omar-nelc@users.noreply.github.com> Co-authored-by: Andrey Cañon <andrey.canon@edunext.co>
feat: add pkce-openid-backend (#255)
2 participants
Description
This adds a generic backend based on ConfigurableOpenIdConnectAuth but
with PKCE.
This backend is inspired by the social-core way to implement PKCE.
There is a current PR in work, but for the moment, that class is not merged and accessible.
So after that is finished this has its code for
code_challengeandcode_challenge_methodimplementation.PR: python-social-auth/social-core#856
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Additional information
Jira story
https://edunext.atlassian.net/browse/FUTUREX-606
Checklist for Merge