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.

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 "Login Failed: Unable to find a valid CSRF token. Please try again." #1717

Merged
merged 15 commits into from
Apr 23, 2024

Conversation

michaljurecko
Copy link
Contributor

Jira: https://keboola.atlassian.net/browse/PSGO-553

Changes:

  • FIxed bug, see Jira.
    • Added redirect to canonical host, to match cookies domain
  • Fixed minor issues, typos, ...

@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-553 branch from 1c679b2 to 0d74d66 Compare April 22, 2024 20:50
Copy link

Apps Proxy Kubernetes Diff [CI]

Between base 9c1d694 ⬅️ head 0d74d66.

Expand
--- /tmp/artifacts/test-k8s-state.old.json.processed.kv	2024-04-22 20:56:48.384645664 +0000
+++ /tmp/artifacts/test-k8s-state.new.json.processed.kv	2024-04-22 20:56:48.504646690 +0000
@@ -105 +105 @@
-<Deployment/apps-proxy>.spec.template.spec.containers[0].image = "docker.io/keboola/apps-proxy:9c1d694";
+<Deployment/apps-proxy>.spec.template.spec.containers[0].image = "docker.io/keboola/apps-proxy:0d74d66";
@@ -461,3 +461,3 @@
-<Pod/apps-proxy-<hash>>.spec.containers[0].image = "docker.io/keboola/apps-proxy:9c1d694";
-<Pod/apps-proxy-<hash>>.spec.containers[0].image = "docker.io/keboola/apps-proxy:9c1d694";
-<Pod/apps-proxy-<hash>>.spec.containers[0].image = "docker.io/keboola/apps-proxy:9c1d694";
+<Pod/apps-proxy-<hash>>.spec.containers[0].image = "docker.io/keboola/apps-proxy:0d74d66";
+<Pod/apps-proxy-<hash>>.spec.containers[0].image = "docker.io/keboola/apps-proxy:0d74d66";
+<Pod/apps-proxy-<hash>>.spec.containers[0].image = "docker.io/keboola/apps-proxy:0d74d66";
@@ -879 +879 @@
-<ReplicaSet/apps-proxy-<hash>>.spec.template.spec.containers[0].image = "docker.io/keboola/apps-proxy:9c1d694";
+<ReplicaSet/apps-proxy-<hash>>.spec.template.spec.containers[0].image = "docker.io/keboola/apps-proxy:0d74d66";


(see artifacts in the Github Action for more information)

@@ -0,0 +1,3 @@
package config

const InternalPrefix = "/_proxy"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it is only one place where /_proxy prefix is defined.

Comment on lines +66 to +79
// CookieDomain without port for cookies.
func (c AppConfig) CookieDomain(publicURL *url.URL) string {
return c.Domain() + "." + publicURL.Hostname()
}

// BaseURL of the app.
func (c AppConfig) BaseURL(publicURL *url.URL) *url.URL {
return &url.URL{
Scheme: publicURL.Scheme,
Host: c.Domain() + "." + publicURL.Host,
Path: "/",
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New helper methods.

@@ -21,6 +23,7 @@ import (
type appHandler struct {
manager *Manager
app api.AppConfig
baseURL *url.URL
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 baseURL field, so it is cached.

Comment on lines +130 to +136
// Redirect request to canonical host to match cookies domain
if req.Host != h.baseURL.Host {
w.Header().Set("Location", h.baseURL.ResolveReference(&url.URL{Path: req.URL.Path}).String())
w.WriteHeader(http.StatusPermanentRedirect)
return nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes:

image

image

Comment on lines +222 to +236
{
name: "redirect-to-canonical-host",
run: func(t *testing.T, client *http.Client, m []*mockoidc.MockOIDC, appServer *testutil.AppServer, service *testutil.DataAppsAPI, dnsServer *dnsmock.Server) {
// Redirect to the canonical URL (match cookies domain)
request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://foo-bar-123.hub.keboola.local/some/data/app/url?foo=bar", nil)
require.NoError(t, err)
response, err := client.Do(request)
require.NoError(t, err)
require.Equal(t, http.StatusPermanentRedirect, response.StatusCode)
location := response.Header.Get("Location")
assert.Equal(t, location, "https://public-123.hub.keboola.local/some/data/app/url")
},
expectedNotifications: map[string]int{},
expectedWakeUps: map[string]int{},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New test.

Comment on lines -61 to 62
loggerWriter := logging.NewLoggerWriter(d.Logger(), "info")
loggerWriter := logging.NewLoggerWriter(d.Logger().WithComponent("oauth2proxy"), "info")
oautproxylogger.SetOutput(loggerWriter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mark logs from the OAuth2Proxy lib.
image

Comment on lines -73 to -79
// Get style.css
rec = httptest.NewRecorder()
req = httptest.NewRequest("GET", "https://hub.keboola.local/_proxy/assets/style.css", nil)
handler.ServeHTTP(rec, req)
require.Equal(t, http.StatusOK, rec.Code)
assert.NotEmpty(t, rec.Body.String())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Petr, you could have deleted it yourself when you added new assets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't remove that, because I wanted to have some test for an asset that exists. Yes, I could use favicon.ico instead style.css

@@ -193,38 +193,6 @@ func TestAppProxyHandler(t *testing.T) {
}
],
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed style.css request.

Comment on lines +59 to +61
ctx, span := l.telemetry.Tracer().Start(ctx, "keboola.go.apps-proxy.appconfig.Loader.GetConfig")
defer span.End(&err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New span.

Comment on lines -6 to 7
args_bin = []
args_bin = ["--sandboxes-api-url", "http://localhost:1234", "--sandboxes-api-token", "my-token", "--api-public-url", "http://localhost:8000"]
cmd = "make build-apps-proxy"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimal config to run the proxy locally (without any sandboxes api).

@michaljurecko michaljurecko marked this pull request as ready for review April 23, 2024 05:19
Comment on lines -113 to 117
<h1>
{{.App.IDAndName}}
</h1>
<h1>Application {{.App.IDAndName}}</h1>
{{ if .App.ProjectID }} <h2>Project {{.App.ProjectID}}</h2>{{ end }}
{{ end }}
</header>
<main>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small improvements in templates, so it is temporary better, until the design will be fixed:

image

image

@michaljurecko michaljurecko merged commit d5adfca into main Apr 23, 2024
14 checks passed
@michaljurecko michaljurecko deleted the michaljurecko-PSGO-553 branch April 23, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants