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

feature: support forward OAuth2 authorization header #130

Merged
merged 1 commit into from
Oct 30, 2024
Merged

feature: support forward OAuth2 authorization header #130

merged 1 commit into from
Oct 30, 2024

Conversation

kalil-pelissier
Copy link
Contributor

Steps:

  • Add ForwardAuthorizationHeader to configuration.
  • Add ForwardAuthorizationHeader to Conn struct.
  • Retrieve access token from request and forward it in HTTP headers.

Note: ForwardAuthorizationHeader will overwrite AccessToken if specified.

close #129

@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 17, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

nineinchnick
nineinchnick reviewed Oct 18, 2024
View reviewed changes
trino/trino.go Outdated Show resolved Hide resolved
trino/trino.go Outdated Show resolved Hide resolved
trino/trino.go Outdated Show resolved Hide resolved
@kalil-pelissier kalil-pelissier force-pushed the feature/support-forward-authorization-header branch from 706b0ef to b816483 Compare October 21, 2024 07:44
@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 21, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@kalil-pelissier kalil-pelissier mentioned this pull request Oct 21, 2024
Merged
@cla-bot cla-bot bot added the cla-signed label Oct 23, 2024
@kalil-pelissier kalil-pelissier force-pushed the feature/support-forward-authorization-header branch 3 times, most recently from 841ca11 to 45fcf51 Compare October 24, 2024 08:50
@nineinchnick
Copy link
Member

The CI failure with the unknown transaction is a known issue, you can ignore that. I'll make sure everything is green before and after merging this.

@kalil-pelissier
Copy link
Contributor Author

Nice thanks @nineinchnick for the feedback.
I'm done working on this PR so it's ready for reviews!

nineinchnick
nineinchnick reviewed Oct 24, 2024
View reviewed changes
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

LGTM except a few nitpicks. Please squash all commits, and remove the prefix from the commit message.

trino/trino.go Show resolved Hide resolved
trino/trino.go Outdated Show resolved Hide resolved
trino/trino.go Outdated Show resolved Hide resolved
@kalil-pelissier kalil-pelissier force-pushed the feature/support-forward-authorization-header branch from 45fcf51 to fa22903 Compare October 25, 2024 10:54
nineinchnick
nineinchnick reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

Can you add a test, or update some existing ones?

trino/trino.go Outdated Show resolved Hide resolved
trino/trino.go Outdated Show resolved Hide resolved
@kalil-pelissier kalil-pelissier force-pushed the feature/support-forward-authorization-header branch 2 times, most recently from 5d5165e to b10bae8 Compare October 29, 2024 08:06
@kalil-pelissier
Copy link
Contributor Author

Hi @nineinchnick,
I have added tests to check the configuration and execution of the new AuthorizationHeaderFeature.

@kalil-pelissier kalil-pelissier self-assigned this Oct 30, 2024
@kalil-pelissier
Copy link
Contributor Author

kalil-pelissier commented Oct 30, 2024 •
edited
Loading

thanks for approving @nineinchnick,
what is the flow/time for merging and releasing changes to trino-go-client ?

@nineinchnick
Copy link
Member

As soon as I get a response to this one last comment: #130 (comment)

@kalil-pelissier kalil-pelissier force-pushed the feature/support-forward-authorization-header branch from b10bae8 to 54e6474 Compare October 30, 2024 09:26
@kalil-pelissier
Copy link
Contributor Author

Done, I had missed that last comment!

@nineinchnick nineinchnick added the enhancement New feature or request label Oct 30, 2024
@nineinchnick nineinchnick merged commit 70bd4d7 into trinodb:master Oct 30, 2024
5 checks passed
@nineinchnick
Copy link
Member

Released: https://github.com/trinodb/trino-go-client/releases/tag/v0.320.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nineinchnick nineinchnick nineinchnick approved these changes

Assignees

@kalil-pelissier kalil-pelissier

Labels
cla-signed enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow forwarding OAuth2 Authorization Headers at query runtime
2 participants
@kalil-pelissier @nineinchnick