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

feat: Add explicit refresh token callable. #1230 #1555

Merged
merged 11 commits into from
Aug 3, 2022
Merged

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Jul 25, 2022

Adds force_token_refresh callable to q.auth. Upon calling, the q.auth.access_token is updated, and waved session is synced.

Since this is server-to-server communication, no cookies are stored (in comparison to server-to-browser) so I had to keep the session ID in the app to make sure I could identify and sync the correct session during refresh. Open to better ideas/suggestions.

Open question: Shall we purge the session in case of an unsuccessful refresh attempt?

Closes #1230

@mturoci mturoci requested a review from lo5 as a code owner July 25, 2022 07:50
@mturoci mturoci requested review from geomodular and zoido July 25, 2022 07:50
@mturoci mturoci changed the title feat: Add implicit refresh token callable. #1230 feat: Add explicit refresh token callable. #1230 Jul 25, 2022
Copy link
Contributor

@zoido zoido left a comment

Choose a reason for hiding this comment

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

Thank you. Just some minor comments with discussion potential and one question:

What are potential performance implications?

Apps that will required this will need to call this with every outgoing request. This is IMHO ok for the internal channel to waved. But I am not sure what TokenSource implementation is effectively used so my concern is whether there is a potential of spamming the identity with a lot of token requests.

py/h2o_wave/server.py Show resolved Hide resolved
py/h2o_wave/server.py Outdated Show resolved Hide resolved
auth.go Show resolved Hide resolved
py/h2o_wave/server.py Outdated Show resolved Hide resolved
py/h2o_wave/server.py Outdated Show resolved Hide resolved
@mturoci
Copy link
Collaborator Author

mturoci commented Aug 2, 2022

But I am not sure what TokenSource implementation is effectively used so my concern is whether there is a potential of spamming the identity with a lot of token requests.

Good point. According to docs - TokenSource returns a TokenSource that returns t until t expires, automatically refreshing it as necessary using the provided context. which seems good to me, but feel free to correct me if I am missing something.

Open question: Shall we purge the session in case of an unsuccessful refresh attempt?

wdyt @zoido?

@mturoci mturoci requested a review from zoido August 2, 2022 07:45
@zoido
Copy link
Contributor

zoido commented Aug 2, 2022

Good point. According to docs - TokenSource returns a TokenSource that returns t until t expires, automatically refreshing it as necessary using the provided context. which seems good to me, but feel free to correct me if I am missing something.

Agree. Clicked trough it once again and got to the same conclusion. Thanks.

Open question: Shall we purge the session in case of an unsuccessful refresh attempt?

wdyt @zoido?

I don't think it's necessary for the explicit endpoint. Reasoning: you explicitly call something that can fail. If it fails you should be able to recover.

But I do not see deep into the interaction between the app and waved so I cannot foresee all of the potential side-effects.

zoido
zoido previously approved these changes Aug 2, 2022
Copy link
Contributor

@zoido zoido left a comment

Choose a reason for hiding this comment

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

LGTM 🥗

Thanks.

auth.go Outdated Show resolved Hide resolved
@mturoci
Copy link
Collaborator Author

mturoci commented Aug 2, 2022

Thanks for the review @zoido!

Copy link
Member

@lo5 lo5 left a comment

Choose a reason for hiding this comment

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

Thank you @mturoci, @zoido!

@mturoci mturoci merged commit 20f74be into master Aug 3, 2022
@mturoci mturoci deleted the feat/issue-1230 branch August 3, 2022 06:48
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.

Provide a way to get fresh access tokens even "in request"
3 participants