search

jupyterhub / oauthenticator

OAuth + JupyterHub Authenticator = OAuthenticator
https://oauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
413 stars 364 forks source link

Drop next_url from authorize_redirect state param #671

Closed johnpmayer closed 1 year ago

johnpmayer commented 1 year ago

This addresses #659

I still want to add tests for the callback handler

I also dropped the log warnings when the url was "cleaned up" before saving to the cookie.

EDIT: fixes #659 - by writing this, the issue is closed when this PR is merged

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

minrk commented 1 year ago

Nice! Thanks for the PR. Let us know if you would like any help with the additional tests. It would be great to have something that exercises that the next_url logic in the callback is still working. It certainly looks right.

We could also encrypt the serialized state with tornado's create_signed_value (the serialization/encryption used in set_secure_cookie).

johnpmayer commented 1 year ago

I think this covers it. Note that I just mocked get_secure_cookie. Anything more "real" and I feel like the tests probably ought to leverage tornado.testing.AsyncHTTPTestCase, but I didn't see any prior art for that in this repository.

johnpmayer commented 1 year ago

@minrk are you able to take a look at this?

welcome[bot] commented 1 year ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart: