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.

Sign up for GitHub

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

Jump to bottom

Fix refresh tokens flow #284

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Fix refresh tokens flow #284

merged 1 commit into from
Nov 18, 2022

Conversation

sinkuladis
Copy link
Contributor

@sinkuladis sinkuladis commented Nov 9, 2022 •
edited
Loading

Description

Fixes refresh token issue caused by Error: header info didn't have x_redirect_server. Trino coordinator with enabled refresh token after access token expiration will respond with only x_token_server field in authentication header, if refresh token has expired then coordinator restarts the flow.

Non-technical explanation

Fix refresh flow in case Trino cluster has refresh tokens enabled.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

@cla-bot
Copy link

cla-bot bot commented Nov 9, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: me98yk.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@sinkuladis sinkuladis changed the title Support refresh tokens flow Fix refresh tokens flow Nov 9, 2022
@cla-bot cla-bot bot added the cla-signed label Nov 9, 2022
lukasz-walkiewicz
lukasz-walkiewicz reviewed Nov 15, 2022
View reviewed changes
tests/unit/test_client.py

post_statement_callback = PostStatementCallback(redirect_server, token_server, [token], sample_post_response_data)

# bind post statement
Copy link
Member

Choose a reason for hiding this comment

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

Is first post statement (binding + actual request) testing anything more than test_oauth2_authentication_flow? If not then I'd remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

trino/auth.py Outdated
@@ -273,7 +273,8 @@ class _OAuth2TokenBearer(AuthBase):
_BEARER_PREFIX = re.compile(r"bearer", flags=re.IGNORECASE)

def __init__(self, redirect_auth_url_handler: Callable[[str], None]):
self._redirect_auth_url = redirect_auth_url_handler
self._redirect_auth_url = None
Copy link
Member

Choose a reason for hiding this comment

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

Why would you want to store it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. We don't need to store it. I'm removing this part

trino/auth.py Outdated
@@ -322,15 +323,18 @@ def _attempt_oauth(self, response, **kwargs):
auth_info_headers = parse_dict_header(_OAuth2TokenBearer._BEARER_PREFIX.sub("", auth_info, count=1))

auth_server = auth_info_headers.get('x_redirect_server')
if auth_server is None:
if auth_server is None and self._redirect_auth_url is None:
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this condition. If x_redirect_server is optional then it's optional. Previous value is not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is no need to check if x_redirect_server is present in this case. I've also updated tests cases that were failing because of the missing x_redirect_server in the response

lukasz-walkiewicz
lukasz-walkiewicz approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@lukasz-walkiewicz lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

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

LGTM but I think you need to sign CLA first.

cc: @hashhar

tests/unit/test_client.py Outdated
uri=token_server,
body=get_token_callback)

redirect_handler = RedirectHandler()
Copy link
Member

Choose a reason for hiding this comment

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

nit: you could use a redirect handler which would produce an error when called to ensure that it's not being used when redirect_uri is not returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I've updated this part.

@hashhar
Copy link
Member

hashhar commented Nov 16, 2022

Thanks for reviewing the important parts @lukasz-walkiewicz.

@sinkuladis I'll take a look by tomorrow.

@sinkuladis
Copy link
Contributor Author

@lukasz-walkiewicz CLA is signed. My first push was signed-off with the wrong username and email, that's why bot was complaining. I've corrected commit author already 👍

@hashhar hashhar self-requested a review November 16, 2022 09:37
hashhar
hashhar approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % style nit

tests/unit/test_client.py Outdated
('Bearer"', 'Error: header info didn\'t have x_token_server'),
('x_redirect_server="redirect_server", x_token_server="token_server"',
'Error: header info didn\'t match x_redirect_server="redirect_server", x_token_server="token_server"'),
# noqa: E501
Copy link
Member

Choose a reason for hiding this comment

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

the noqa was here to prevent wrapping across lines. Revert the wrapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed that bit. Thanks!

@sinkuladis
Fix refresh tokens flow
3cf5b80 
Signed-off-by: sinkuladis <sink.vlad@gmail.com>
hashhar
hashhar approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar 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.

@hashhar hashhar merged commit 8dcb594 into trinodb:master Nov 18, 2022
@hashhar
Copy link
Member

hashhar commented Nov 18, 2022

@sinkuladis Can you please see if the release note at #283 (comment) captures the issue correctly.

i.e. without this fix if the access token had expired the refresh token flow didn't work correctly and instead failed with Error: header info didn't have x_redirect_server?

@sinkuladis
Copy link
Contributor Author

Yes, the release note captures it well. Hey, many thanks for reviewing my PR! :D

@bitsondatadev bitsondatadev mentioned this pull request Nov 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar hashhar approved these changes

@lukasz-walkiewicz lukasz-walkiewicz lukasz-walkiewicz approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sinkuladis @hashhar @lukasz-walkiewicz