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

added support for oauth2 audience #164

Merged
merged 1 commit into from
Jan 24, 2024
Merged

added support for oauth2 audience #164

merged 1 commit into from
Jan 24, 2024

Conversation

andythsu
Copy link
Member

No description provided.

@cla-bot cla-bot
Copy link

cla-bot bot commented Jan 17, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Andy Su (Apps).
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

@cla-bot cla-bot bot added the cla-signed label Jan 17, 2024
willmostly
willmostly requested changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

Looks good, one improvement requested to the test setup then I think this is good to go

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestLbTokenUtil.java Outdated
@TestInstance(Lifecycle.PER_CLASS)
public class TestLbTokenUtil
{
private final String idToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJ0ZXN0Iiwic3ViIjoidGVzdCIsImF1ZCI6InRlc3QuY29tIn0.YVdTAuEnn-UHsZf5AKA8avaBTLQvdQ3Dd7LeaXKSjlq0atRcmp_IzE8vnJYplYwBs6VNsGpgrfeIMXUsZFKTcfIBinAh05Fj5M7zXVoOPLEdYH5QJ_sfe5-KFPuJs32epC7GE50-umLKTHDZ14dfiXUNK4yAE0bmiNh1lhKcEP35ZAaBt8ldngXJ7BlNbpLpS30-Z4vA6Nht-WjIlluVHd3B5LCJO5FKRcjpuV7E_XZID9c8p0_7Wl7yEvaQB0yJd-VKgT2MQAZse17YqBTb3ix51F5N05s98w7w5zX8NI_afp97YWwNPKTN0dVZFVF8abZ-35O2CAlM0hzbt7NKdw";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use JWT.create() to build the token in setup()? That would make these tests easier to understand and maintain in the future if new features are desired.

@andythsu andythsu force-pushed the oauth2_aud branch 2 times, most recently from 5ceeebb to 5bd6f60 Compare January 17, 2024 21:15
@mosabua mosabua mentioned this pull request Jan 18, 2024
Closed
willmostly
willmostly approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

LGTM!

mosabua
mosabua approved these changes Jan 24, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good now.

@mosabua mosabua merged commit 0ad2dbf into trinodb:main Jan 24, 2024
2 checks passed
@mosabua mosabua mentioned this pull request Jan 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua approved these changes

@willmostly willmostly willmostly approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@andythsu @mosabua @willmostly