Skip to content

Allow to use PKCE #23

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

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Allow to use PKCE #23

merged 2 commits into from
Sep 30, 2024

Conversation

sbrunner
Copy link
Contributor

@sbrunner sbrunner commented Aug 21, 2024

Proof Key for Code Exchange
Defined by RFC7636
https://www.rfc-editor.org/rfc/rfc7636

To be use as:

import pkce

client = OpenidClient.from_issuer_url(
    url="https://provider.example.com/openid",
    authentication_redirect_uri="https://myapp.example.com/login-callback",
    client_id="client-id",
)
code_verifier, code_challenge = pkce.generate_pkce_pair()

def on_login():
    # this method should be called when the user wants to log in
    # it returns an HTTP redirect to the Openid provider
    return HttpRedirect(to=client.authorization_code_flow.start_authentication(
                    code_challenge=code_challenge,
                    code_challenge_method="S256"
    ))

def on_login_callback(current_url):
    token_response = client.authorization_code_flow.handle_authentication_result(
        current_url,
        code_verifier=code_verifier,
        code_challenge=code_challenge,
        code_challenge_method="S256",
    )
    # token_response now contains access and id tokens
    ...

@sbrunner sbrunner marked this pull request as ready for review August 21, 2024 09:57
Copy link
Member

@lilioid lilioid left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

Are you willing to work on this more or is this more of a pointer to what's missing and how to implement it?

I have not tested this PR in depth but the added functionality is definitely a good thing to have.
A few things though:

  • I have marked two sections of code with some minor details. Please address those.
  • I would very much like to document this a bit better. This could be as an example in the documentations Usage Section or as part of the docstrings.
  • In your PR description you use pkce.generate_pkce_pair(). Even though generating pkce pairs is not very complex, I would like to provide that functionality as part of this library. So if you've implemented that part already, please commit the pkce.py file.

Proof Key for Code Exchange
Defined by RFC7636
https://www.rfc-editor.org/rfc/rfc7636
@sbrunner
Copy link
Contributor Author

Thanks for the review, updated :-)
The pkce code comes from this package: https://pypi.org/project/pkce/
It's true that's relay simple, but I'm not sure that good to copy this code, other proposition it to add it as a dependency or an extra dependency, what do you want me to do?

@sbrunner sbrunner requested a review from lilioid August 29, 2024 15:54
@lilioid
Copy link
Member

lilioid commented Sep 8, 2024

Thanks for the review, updated :-) The pkce code comes from this package: pypi.org/project/pkce It's true that's relay simple, but I'm not sure that good to copy this code, other proposition it to add it as a dependency or an extra dependency, what do you want me to do?

I'm not really a fan of micro-packages so I would prefer copying the relevant PKCE code over into our source tree

@sbrunner
Copy link
Contributor Author

sbrunner commented Sep 9, 2024

@ftsell done :-)

Copy link
Member

@lilioid lilioid left a comment

Choose a reason for hiding this comment

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

Thanks. Especially for mentioning where the pkce code was taken from.
I'd eventually like to also add tests and convert the pkce docstrings to our format but for now we can merge this PR if you want

@lilioid lilioid merged commit c19aa3d into fsinfuhh:main Sep 30, 2024
17 checks passed
@sbrunner sbrunner deleted the allows-pkce branch September 30, 2024 11:21
@sbrunner sbrunner restored the allows-pkce branch October 7, 2024 15:28
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