Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: me98yk.
|
sinkuladis
force-pushed
the
feat/refresh-token
branch
from
348700f to
ba4537a
Compare
sinkuladis
force-pushed
the
feat/refresh-token
branch
from
ba4537a to
1408c75
Compare
|
|
||
| post_statement_callback = PostStatementCallback(redirect_server, token_server, [token], sample_post_response_data) | ||
|
|
||
| # bind post statement |
There was a problem hiding this comment.
Is first post statement (binding + actual request) testing anything more than test_oauth2_authentication_flow? If not then I'd remove it.
trino/auth.py
Outdated
|
|
||
| def __init__(self, redirect_auth_url_handler: Callable[[str], None]): | ||
| self._redirect_auth_url = redirect_auth_url_handler | ||
| self._redirect_auth_url = None |
There was a problem hiding this comment.
Why would you want to store it?
There was a problem hiding this comment.
You are right. We don't need to store it. I'm removing this part
trino/auth.py
Outdated
|
|
||
| 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: |
There was a problem hiding this comment.
I think we can remove this condition. If x_redirect_server is optional then it's optional. Previous value is not important.
There was a problem hiding this comment.
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
sinkuladis
force-pushed
the
feat/refresh-token
branch
from
1408c75 to
3b6d80f
Compare
lukasz-walkiewicz
left a comment
lukasz-walkiewicz
left a comment
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good idea. I've updated this part.
sinkuladis
force-pushed
the
feat/refresh-token
branch
from
3b6d80f to
70543d3
Compare
|
Thanks for reviewing the important parts @lukasz-walkiewicz. @sinkuladis I'll take a look by tomorrow. |
|
@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 👍 |
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 |
There was a problem hiding this comment.
the noqa was here to prevent wrapping across lines. Revert the wrapping.
There was a problem hiding this comment.
missed that bit. Thanks!
Signed-off-by: sinkuladis <sink.vlad@gmail.com>
sinkuladis
force-pushed
the
feat/refresh-token
branch
from
70543d3 to
3cf5b80
Compare
|
@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 |
|
Yes, the release note captures it well. Hey, many thanks for reviewing my PR! :D |
3 participants
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 onlyx_token_serverfield 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: