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

fix: Modify the callback URL string in auth flow, to support custom base URLs in deployments #5192

Merged
merged 3 commits into from
Jun 13, 2024
Merged

fix: Modify the callback URL string in auth flow, to support custom base URLs in deployments #5192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cfdd43d
x
ddl-rliu Apr 5, 2024
a068bab
x
ddl-rliu Apr 10, 2024
48719d7
Merge remote-tracking branch 'origin' into rliu.redirect-url
eapolinario Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion flyteadmin/auth/auth_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const (
)

var (
callbackRelativeURL = config.MustParseURL("/callback")
callbackRelativeURL = config.MustParseURL("callback")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is necessary, because when Flyteadmin generates the callback URL, the logic looks like URL("foo.tech").ResolveReference(URL("/callback")) -> "foo.tech/callback"

In fact, the base URL can have any path after it, but the result is the same. URL("foo.tech/any/path").ResolveReference(URL("/callback")) -> "foo.tech/callback"

This causes issues when flyte is configured on a custom subpath. To fix this...

By adding foo.tech/custom-subpath/login as an AuthorizedURI, and changing the reference to "callback", the logic becomes URL("foo.tech/custom-subpath/login").ResolveReference(URL("callback")) -> "foo.tech/custom-subpath/callback"

Copy link
Contributor

Choose a reason for hiding this comment

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

Should any tests be updated in

flyte/flyteadmin/auth/handlers_test.go

Line 117 in 2528de7

func TestGetCallbackHandler(t *testing.T) {
to help verify this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test here https://github.com/flyteorg/flyte/pull/5192/files#diff-aa5ac252b2ec45c95649a3a874e727e436be5c932e3d93b5b2aadcfcc83d57feR12

Although I could also merge this into handlers_test.go, no strong preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what you did is better / clearer - thanks!

rootRelativeURL = config.MustParseURL("/")
)

Expand Down
51 changes: 51 additions & 0 deletions flyteadmin/auth/auth_context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package auth

import (
"testing"

"github.com/stretchr/testify/assert"
"golang.org/x/oauth2"

"github.com/flyteorg/flyte/flyteadmin/auth/config"
)

func TestOAuth2ClientConfig(t *testing.T) {
authCtx := Context{
oauth2Client: &oauth2.Config{},
}

type test struct {
name string
url string
expectedRedirectURL string
}
tests := []test{
{
name: "simple publicUrl",
url: "https://flyte.com",
expectedRedirectURL: "https://flyte.com/callback",
},
{
name: "custom subpath",
url: "https://flyte.com/custom-subpath/console",
expectedRedirectURL: "https://flyte.com/custom-subpath/callback",
},
{
name: "complex publicUrl",
url: "https://flyte.com/login?redirect_url=https://flyte.com/console/select-project",
expectedRedirectURL: "https://flyte.com/callback",
},
{
name: "complex publicUrl with custom subpath",
url: "https://flyte.com/custom-subpath/login?redirect_url=https://flyte.com/custom-subpath/console/select-project",
expectedRedirectURL: "https://flyte.com/custom-subpath/callback",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cfg := authCtx.OAuth2ClientConfig(config.MustParseURL(tt.url))
assert.Equal(t, tt.expectedRedirectURL, cfg.RedirectURL)
})
}
}
Loading