Skip to content

Add support for composite external auth redirect handler#10248

Merged
kokosing merged 1 commit intotrinodb:masterfrom
wendigo:serafin/add-composite-redirect-handler
Feb 3, 2022
Merged

Add support for composite external auth redirect handler#10248
kokosing merged 1 commit intotrinodb:masterfrom
wendigo:serafin/add-composite-redirect-handler

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Dec 9, 2021

No description provided.

@cla-bot cla-bot bot added the cla-signed label Dec 9, 2021
@wendigo wendigo added WIP and removed cla-signed labels Dec 9, 2021
@wendigo wendigo requested a review from s2lomon December 9, 2021 11:55
@wendigo wendigo force-pushed the serafin/add-composite-redirect-handler branch from 0beb0d5 to 58140f9 Compare December 9, 2021 14:12
@cla-bot cla-bot bot added the cla-signed label Dec 9, 2021
nineinchnick
nineinchnick approved these changes Dec 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've tested this on my macOS 12, and it works. I've got a use case where the JVM is running in headless mode (out of my control) and the current method of opening the browser doesn't work.

@wendigo wendigo requested review from ebyhr and hashhar December 14, 2021 12:00
hashhar
hashhar reviewed Dec 16, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Some general comments/questions. Looks ok to me. Add some context to the commit message why this is needed (most likely because the JVM is running in headless mode and the java.awt.Desktop doesn't work there).

I'll defer to @lukasz-walkiewicz and @kokosing since they are more familiar with this part.

kokosing
kokosing reviewed Dec 16, 2021
View reviewed changes
client/trino-client/src/main/java/io/trino/client/auth/external/SystemOpenRedirectHandler.java Outdated
Copy link
Copy Markdown
Member

@kokosing kokosing Dec 16, 2021

Choose a reason for hiding this comment

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

log?

Copy link
Copy Markdown
Contributor Author

@wendigo wendigo Dec 16, 2021

Choose a reason for hiding this comment

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

We are not using logger in trino-client afair

@wendigo wendigo force-pushed the serafin/add-composite-redirect-handler branch from e755a13 to 2cbb6c3 Compare January 18, 2022 10:23
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Jan 18, 2022
@wendigo wendigo force-pushed the serafin/add-composite-redirect-handler branch 2 times, most recently from c83b845 to 41869ff Compare February 3, 2022 10:10
kokosing
kokosing approved these changes Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comments

@wendigo wendigo force-pushed the serafin/add-composite-redirect-handler branch 2 times, most recently from 826d811 to a130363 Compare February 3, 2022 11:56
kokosing
kokosing reviewed Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Please provide release notes and documentation changes

s2lomon
s2lomon approved these changes Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@s2lomon s2lomon left a comment

Choose a reason for hiding this comment

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

% comment about tests

@wendigo wendigo force-pushed the serafin/add-composite-redirect-handler branch from a130363 to f410d33 Compare February 3, 2022 13:04
@wendigo wendigo removed the WIP label Feb 3, 2022
@wendigo wendigo force-pushed the serafin/add-composite-redirect-handler branch from f410d33 to 7b98764 Compare February 3, 2022 13:18
@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Feb 3, 2022

CI hit: #10932

@kokosing kokosing merged commit 050d089 into trinodb:master Feb 3, 2022
@kokosing kokosing mentioned this pull request Feb 3, 2022
Closed
@mosabua mosabua mentioned this pull request Feb 3, 2022
Merged
@github-actions github-actions bot added this to the 370 milestone Feb 3, 2022
@wendigo wendigo deleted the serafin/add-composite-redirect-handler branch January 21, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hashhar hashhar hashhar left review comments

@kokosing kokosing kokosing approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

+3 more reviewers

@nineinchnick nineinchnick nineinchnick approved these changes

@s2lomon s2lomon s2lomon approved these changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed jdbc Relates to Trino JDBC driver

Milestone

370

Development

Successfully merging this pull request may close these issues.

6 participants

@wendigo @nineinchnick @kokosing @s2lomon @hashhar @lukasz-walkiewicz