From 5b04192945aa068c0f7979deb5088e6ce0017e95 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Mon, 30 Nov 2020 09:23:12 -0500 Subject: [PATCH 1/3] Run TestSupervisorLogin only on valid HTTP/HTTPS supervisor addresses We were assuming that env.SupervisorHTTPAddress was set, but it might not be depending on the environment on which the integration tests are being run. For example, in our acceptance environments, we don't currently set env.SupervisorHTTPAddress. I tried to follow the pattern from TestSupervisorOIDCDiscovery here. Signed-off-by: Andrew Keesler --- test/integration/supervisor_login_test.go | 135 ++++++++++++---------- 1 file changed, 76 insertions(+), 59 deletions(-) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index ac7c3d02..cb3c71db 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -32,70 +32,87 @@ func TestSupervisorLogin(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - // Create downstream OIDC provider (i.e., update supervisor with OIDC provider). - scheme := "http" - addr := env.SupervisorHTTPAddress - caBundle := "" - path := "/some/path" - issuer := fmt.Sprintf("https://%s%s", addr, path) - _, _ = requireCreatingOIDCProviderCausesDiscoveryEndpointsToAppear( - ctx, - t, - scheme, - addr, - caBundle, - issuer, - client, - ) - - // Create HTTP client. - httpClient := newHTTPClient(t, caBundle, nil) - httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { - // Don't follow any redirects right now, since we simply want to validate that our auth endpoint - // redirects us. - return http.ErrUseLastResponse + tests := []struct { + Scheme string + Address string + CABundle string + }{ + {Scheme: "http", Address: env.SupervisorHTTPAddress}, + {Scheme: "https", Address: env.SupervisorHTTPSIngressAddress, CABundle: env.SupervisorHTTPSIngressCABundle}, } - // Declare the downstream auth endpoint url we will use. - downstreamAuthURL := makeDownstreamAuthURL(t, scheme, addr, path) + for _, test := range tests { + scheme := test.Scheme + addr := test.Address + caBundle := test.CABundle - // Make request to auth endpoint - should fail, since we have no upstreams. - req, err := http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) - require.NoError(t, err) - rsp, err := httpClient.Do(req) - require.NoError(t, err) - defer rsp.Body.Close() - require.Equal(t, http.StatusUnprocessableEntity, rsp.StatusCode) + if addr == "" { + // Both cases are not required, so when one is empty skip it. + continue + } - // Create upstream OIDC provider. - spec := idpv1alpha1.UpstreamOIDCProviderSpec{ - Issuer: env.SupervisorTestUpstream.Issuer, - TLS: &idpv1alpha1.TLSSpec{ - CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorTestUpstream.CABundle)), - }, - Client: idpv1alpha1.OIDCClient{ - SecretName: makeTestClientCredsSecret(t, env.SupervisorTestUpstream.ClientID, env.SupervisorTestUpstream.ClientSecret).Name, - }, + // Create downstream OIDC provider (i.e., update supervisor with OIDC provider). + path := "/some/path" + issuer := fmt.Sprintf("https://%s%s", addr, path) + _, _ = requireCreatingOIDCProviderCausesDiscoveryEndpointsToAppear( + ctx, + t, + scheme, + addr, + caBundle, + issuer, + client, + ) + + // Create HTTP client. + httpClient := newHTTPClient(t, caBundle, nil) + httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + // Don't follow any redirects right now, since we simply want to validate that our auth endpoint + // redirects us. + return http.ErrUseLastResponse + } + + // Declare the downstream auth endpoint url we will use. + downstreamAuthURL := makeDownstreamAuthURL(t, scheme, addr, path) + + // Make request to auth endpoint - should fail, since we have no upstreams. + req, err := http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) + require.NoError(t, err) + rsp, err := httpClient.Do(req) + require.NoError(t, err) + defer rsp.Body.Close() + require.Equal(t, http.StatusUnprocessableEntity, rsp.StatusCode) + + // Create upstream OIDC provider. + spec := idpv1alpha1.UpstreamOIDCProviderSpec{ + Issuer: env.SupervisorTestUpstream.Issuer, + TLS: &idpv1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorTestUpstream.CABundle)), + }, + Client: idpv1alpha1.OIDCClient{ + SecretName: makeTestClientCredsSecret(t, env.SupervisorTestUpstream.ClientID, env.SupervisorTestUpstream.ClientSecret).Name, + }, + } + upstream := makeTestUpstream(t, spec, idpv1alpha1.PhaseReady) + + upstreamRedirectURI := fmt.Sprintf("https://%s/some/path/callback/%s", env.SupervisorHTTPAddress, upstream.Name) + + // Make request to authorize endpoint - should pass, since we now have an upstream. + req, err = http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) + require.NoError(t, err) + rsp, err = httpClient.Do(req) + require.NoError(t, err) + defer rsp.Body.Close() + require.Equal(t, http.StatusFound, rsp.StatusCode) + requireValidRedirectLocation( + ctx, + t, + upstream.Spec.Issuer, + env.SupervisorTestUpstream.ClientID, + upstreamRedirectURI, + rsp.Header.Get("Location"), + ) } - upstream := makeTestUpstream(t, spec, idpv1alpha1.PhaseReady) - - upstreamRedirectURI := fmt.Sprintf("https://%s/some/path/callback/%s", env.SupervisorHTTPAddress, upstream.Name) - - // Make request to authorize endpoint - should pass, since we now have an upstream. - req, err = http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) - require.NoError(t, err) - rsp, err = httpClient.Do(req) - require.NoError(t, err) - defer rsp.Body.Close() - require.Equal(t, http.StatusFound, rsp.StatusCode) - requireValidRedirectLocation( - ctx, - t, - upstream.Spec.Issuer, - env.SupervisorTestUpstream.ClientID, - upstreamRedirectURI, - rsp.Header.Get("Location"), - ) } func makeDownstreamAuthURL(t *testing.T, scheme, addr, path string) string { From 5be46d0bb74a5b688dc2fd0213cf597d023b0eac Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Mon, 30 Nov 2020 09:58:08 -0500 Subject: [PATCH 2/3] test/integration: get downstream issuer path from upstream redirect See comment in the code. Signed-off-by: Andrew Keesler --- test/integration/supervisor_login_test.go | 35 ++++++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index cb3c71db..0ce937fb 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -11,6 +11,8 @@ import ( "fmt" "net/http" "net/url" + "path" + "strings" "testing" "time" @@ -52,7 +54,7 @@ func TestSupervisorLogin(t *testing.T) { } // Create downstream OIDC provider (i.e., update supervisor with OIDC provider). - path := "/some/path" + path := getDownstreamIssuerPathFromUpstreamRedirectURI(t, env.SupervisorTestUpstream.CallbackURL) issuer := fmt.Sprintf("https://%s%s", addr, path) _, _ = requireCreatingOIDCProviderCausesDiscoveryEndpointsToAppear( ctx, @@ -95,8 +97,6 @@ func TestSupervisorLogin(t *testing.T) { } upstream := makeTestUpstream(t, spec, idpv1alpha1.PhaseReady) - upstreamRedirectURI := fmt.Sprintf("https://%s/some/path/callback/%s", env.SupervisorHTTPAddress, upstream.Name) - // Make request to authorize endpoint - should pass, since we now have an upstream. req, err = http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthURL, nil) require.NoError(t, err) @@ -109,12 +109,39 @@ func TestSupervisorLogin(t *testing.T) { t, upstream.Spec.Issuer, env.SupervisorTestUpstream.ClientID, - upstreamRedirectURI, + env.SupervisorTestUpstream.CallbackURL, rsp.Header.Get("Location"), ) } } +func getDownstreamIssuerPathFromUpstreamRedirectURI(t *testing.T, upstreamRedirectURI string) string { + // We need to construct the downstream issuer path from the upstream redirect URI since the two + // are related, and the upstream redirect URI is supplied via a static test environment + // variable. The upstream redirect URI should be something like + // https://supervisor.com/some/supervisor/path/callback + // and therefore the downstream issuer should be something like + // https://supervisor.com/some/supervisor/path + // since the /callback endpoint is placed at the root of the downstream issuer path. + upstreamRedirectURL, err := url.Parse(upstreamRedirectURI) + require.NoError(t, err) + + redirectURIPathWithoutLastSegment, lastUpstreamRedirectURIPathSegment := path.Split(upstreamRedirectURL.Path) + require.Equalf( + t, + "callback", + lastUpstreamRedirectURIPathSegment, + "expected upstream redirect URI (%q) to follow supervisor callback path conventions (i.e., end in /callback)", + upstreamRedirectURI, + ) + + if strings.HasSuffix(redirectURIPathWithoutLastSegment, "/") { + redirectURIPathWithoutLastSegment = redirectURIPathWithoutLastSegment[:len(redirectURIPathWithoutLastSegment)-1] + } + + return redirectURIPathWithoutLastSegment +} + func makeDownstreamAuthURL(t *testing.T, scheme, addr, path string) string { t.Helper() downstreamOAuth2Config := oauth2.Config{ From eae6d355f8ef35ea446839af76bed7a96b072fbb Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Mon, 30 Nov 2020 10:01:31 -0500 Subject: [PATCH 3/3] test/integration: skip TestSupervisorLogin until new callback logic is on main Signed-off-by: Andrew Keesler --- test/integration/supervisor_login_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 0ce937fb..ca5c2787 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -28,6 +28,8 @@ import ( ) func TestSupervisorLogin(t *testing.T) { + t.Skip("waiting on new callback path logic to get merged in from the callback endpoint work") + env := library.IntegrationEnv(t) client := library.NewSupervisorClientset(t) @@ -115,6 +117,7 @@ func TestSupervisorLogin(t *testing.T) { } } +//nolint:unused func getDownstreamIssuerPathFromUpstreamRedirectURI(t *testing.T, upstreamRedirectURI string) string { // We need to construct the downstream issuer path from the upstream redirect URI since the two // are related, and the upstream redirect URI is supplied via a static test environment @@ -142,6 +145,7 @@ func getDownstreamIssuerPathFromUpstreamRedirectURI(t *testing.T, upstreamRedire return redirectURIPathWithoutLastSegment } +//nolint:unused func makeDownstreamAuthURL(t *testing.T, scheme, addr, path string) string { t.Helper() downstreamOAuth2Config := oauth2.Config{ @@ -163,6 +167,7 @@ func makeDownstreamAuthURL(t *testing.T, scheme, addr, path string) string { ) } +//nolint:unused func generateAuthRequestParams(t *testing.T) (state.State, nonce.Nonce, pkce.Code) { t.Helper() state, err := state.Generate() @@ -174,6 +179,7 @@ func generateAuthRequestParams(t *testing.T) (state.State, nonce.Nonce, pkce.Cod return state, nonce, pkce } +//nolint:unused func requireValidRedirectLocation( ctx context.Context, t *testing.T,