Skip to content

SNOW-2194055: Separate server and redirect URIs in AuthHttpServer #2609

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
Merged
merged 9 commits into from
Nov 4, 2025
Merged

SNOW-2194055: Separate server and redirect URIs in AuthHttpServer #2609

merged 9 commits into from
Nov 4, 2025

Conversation

@sfc-gh-turbaszek
Copy link
Contributor

@sfc-gh-turbaszek sfc-gh-turbaszek commented Oct 28, 2025 •
edited
Loading

Fixes support for redirect_uri. Copy of #2400

@sfc-gh-turbaszek sfc-gh-turbaszek requested a review from a team as a code owner October 28, 2025 12:54
@sfc-gh-turbaszek sfc-gh-turbaszek force-pushed the turbaszek-SNOW-2194055-split-server-redirect-uris branch from a57bbe5 to 00665af Compare October 28, 2025 12:55
sfc-gh-snoonan
sfc-gh-snoonan requested changes Oct 29, 2025
View reviewed changes
sfc-gh-fpawlowski
sfc-gh-fpawlowski reviewed Oct 29, 2025
View reviewed changes
Copy link
Contributor

@sfc-gh-fpawlowski sfc-gh-fpawlowski left a comment

Choose a reason for hiding this comment

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

Left minor comment regarding the flow of configuration

sfc-gh-snoonan
sfc-gh-snoonan reviewed Nov 3, 2025
View reviewed changes
npeshkov
npeshkov approved these changes Nov 3, 2025
View reviewed changes
Copy link
Contributor

@npeshkov npeshkov left a comment

Choose a reason for hiding this comment

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

I tested the changes locally and in a Cloud Dev Env (custom redirect URI), and both work 👍

sfc-gh-fpawlowski
sfc-gh-fpawlowski approved these changes Nov 3, 2025
View reviewed changes
Copy link
Contributor

@sfc-gh-fpawlowski sfc-gh-fpawlowski left a comment

Choose a reason for hiding this comment

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

lgtm

@sfc-gh-turbaszek sfc-gh-turbaszek force-pushed the turbaszek-SNOW-2194055-split-server-redirect-uris branch from 1651005 to 80afdb9 Compare November 3, 2025 15:08
npeshkov
npeshkov reviewed Nov 4, 2025
View reviewed changes
Copy link
Contributor

@npeshkov npeshkov left a comment

Choose a reason for hiding this comment

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

Previously, without any configuration on user side, effective redirect uri would have a port added to it. This no longer happens, which will break setups in which the identity provider expects the uri to end with a port

npeshkov
npeshkov reviewed Nov 4, 2025
View reviewed changes
Copy link
Contributor

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

In a previous iteration of this PR, it was possible to specify the port and the host of the local auth server using environment variables. I do not see an easy way to do this anymore, and it is relevant when the local server port needs to match a part of the redirect uri (in cloud environments).

Edit: I did not see the oauth_socket_uri connection param

@sfc-gh-turbaszek sfc-gh-turbaszek added the DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector label Nov 4, 2025
@sfc-gh-turbaszek sfc-gh-turbaszek merged commit 1a0290d into main Nov 4, 2025
106 of 113 checks passed
@sfc-gh-turbaszek sfc-gh-turbaszek deleted the turbaszek-SNOW-2194055-split-server-redirect-uris branch November 4, 2025 15:37
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sfc-gh-fpawlowski sfc-gh-fpawlowski sfc-gh-fpawlowski approved these changes

@sfc-gh-snoonan sfc-gh-snoonan sfc-gh-snoonan approved these changes

+1 more reviewer

@npeshkov npeshkov npeshkov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DO_NOT_PORT_CHANGES_TO_SP Add this label when changes in this PR do not need to be port to SP connector

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sfc-gh-turbaszek @npeshkov @sfc-gh-fpawlowski @sfc-gh-snoonan