From 0cfa9a7be2657febdccc9c40acf0c9f1ef67b08e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Apr 2024 10:17:21 -0500 Subject: [PATCH 1/3] chore: verify pass through external auth query params Unit test added to verify behavior of query params set in the auth url for external apps. This behavior is intended to specifically support Auth0 audience query param. --- coderd/externalauth/externalauth_test.go | 78 ++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 84fbe4ff5de35..c3710750b3da9 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -3,9 +3,11 @@ package externalauth_test import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -13,6 +15,7 @@ import ( "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" "golang.org/x/xerrors" @@ -417,6 +420,77 @@ func TestConvertYAML(t *testing.T) { }) } +// TestConstantQueryParams verifies a constant query parameter can be set in the +// "authenticate" url for external auth applications, and it will be carried forward +// to actual auth requests. +// This unit test was specifically created for Auth0 which can set an +// audience query parameter in it's /authorize endpoint. +func TestConstantQueryParams(t *testing.T) { + t.Parallel() + const constantQueryParamKey = "audience" + const constantQueryParamValue = "foobar" + constantQueryParam := fmt.Sprintf("%s=%s", constantQueryParamKey, constantQueryParamValue) + fake, config, _ := setupOauth2Test(t, testConfig{ + FakeIDPOpts: []oidctest.FakeIDPOpt{ + oidctest.WithMiddlewares(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + if strings.Contains(request.URL.Path, "authorize") { + // Assert has the audience query param + assert.Equal(t, request.URL.Query().Get(constantQueryParamKey), constantQueryParamValue) + } + next.ServeHTTP(writer, request) + }) + }), + }, + CoderOIDCConfigOpts: []func(cfg *coderd.OIDCConfig){ + func(cfg *coderd.OIDCConfig) { + // Include a constant query parameter. + authURL, err := url.Parse(cfg.OAuth2Config.(*oauth2.Config).Endpoint.AuthURL) + require.NoError(t, err) + + authURL.RawQuery = url.Values{constantQueryParamKey: []string{constantQueryParamValue}}.Encode() + cfg.OAuth2Config.(*oauth2.Config).Endpoint.AuthURL = authURL.String() + require.Contains(t, cfg.OAuth2Config.(*oauth2.Config).Endpoint.AuthURL, constantQueryParam) + }, + }, + }) + + callbackCalled := false + fake.SetCoderdCallbackHandler(func(writer http.ResponseWriter, request *http.Request) { + // Just record the callback was hit, and the auth succeeded. + callbackCalled = true + }) + + // Verify the AuthURL endpoint contains the constant query parameter and is a valid URL. + // It should look something like: + // http://127.0.0.1:>/oauth2/authorize? + // audience=foobar& + // client_id=d& + // redirect_uri=& + // response_type=code& + // scope=openid+email+profile& + // state=state + const state = "state" + rawAuthURL := config.AuthCodeURL(state) + // Parsing the url is not perfect. It allows imperfections like the query + // params having 2 question marks '?a=foo?b=bar'. + // So use it to validate, then verify the raw url is as expected. + authURL, err := url.Parse(rawAuthURL) + require.NoError(t, err) + require.Equal(t, authURL.Query().Get(constantQueryParamKey), constantQueryParamValue) + // We are not using a real server, so it fakes https://coder.com + require.Equal(t, authURL.Scheme, "https") + // Validate the raw URL. + // Double check only 1 '?' exists. Url parsing allows multiple '?' in the query string. + require.Equal(t, strings.Count(rawAuthURL, "?"), 1) + + // Actually run an auth request. Although it says OIDC, the flow is the same + // for oauth2. + cli, resp := fake.OIDCCallback(t, state, jwt.MapClaims{}) + require.True(t, callbackCalled) + +} + type testConfig struct { FakeIDPOpts []oidctest.FakeIDPOpt CoderOIDCConfigOpts []func(cfg *coderd.OIDCConfig) @@ -433,6 +507,10 @@ type testConfig struct { func setupOauth2Test(t *testing.T, settings testConfig) (*oidctest.FakeIDP, *externalauth.Config, database.ExternalAuthLink) { t.Helper() + if settings.ExternalAuthOpt == nil { + settings.ExternalAuthOpt = func(_ *externalauth.Config) {} + } + const providerID = "test-idp" fake := oidctest.NewFakeIDP(t, append([]oidctest.FakeIDPOpt{}, settings.FakeIDPOpts...)..., From cfc78f22dc415ea550cc66888f7d6a779e941f99 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Apr 2024 10:25:01 -0500 Subject: [PATCH 2/3] remove excess error return --- coderd/coderdtest/oidctest/idp.go | 4 ++-- coderd/coderdtest/oidctest/idp_test.go | 5 ++--- coderd/externalauth/externalauth_test.go | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index ff42e31997235..c0b95619d46b7 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -604,7 +604,7 @@ func (f *FakeIDP) CreateAuthCode(t testing.TB, state string) string { // something. // Essentially this is used to fake the Coderd side of the exchange. // The flow starts at the user hitting the OIDC login page. -func (f *FakeIDP) OIDCCallback(t testing.TB, state string, idTokenClaims jwt.MapClaims) (*http.Response, error) { +func (f *FakeIDP) OIDCCallback(t testing.TB, state string, idTokenClaims jwt.MapClaims) *http.Response { t.Helper() if f.serve { panic("cannot use OIDCCallback with WithServing. This is only for the in memory usage") @@ -625,7 +625,7 @@ func (f *FakeIDP) OIDCCallback(t testing.TB, state string, idTokenClaims jwt.Map _ = resp.Body.Close() } }) - return resp, nil + return resp } // ProviderJSON is the .well-known/configuration JSON diff --git a/coderd/coderdtest/oidctest/idp_test.go b/coderd/coderdtest/oidctest/idp_test.go index 519635b067916..ce88f84273756 100644 --- a/coderd/coderdtest/oidctest/idp_test.go +++ b/coderd/coderdtest/oidctest/idp_test.go @@ -54,12 +54,11 @@ func TestFakeIDPBasicFlow(t *testing.T) { token = oauthToken }) - resp, err := fake.OIDCCallback(t, expectedState, jwt.MapClaims{}) - require.NoError(t, err) + resp := fake.OIDCCallback(t, expectedState, jwt.MapClaims{}) require.Equal(t, http.StatusOK, resp.StatusCode) // Test the user info - _, err = cfg.Provider.UserInfo(ctx, oauth2.StaticTokenSource(token)) + _, err := cfg.Provider.UserInfo(ctx, oauth2.StaticTokenSource(token)) require.NoError(t, err) // Now test it can refresh diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index c3710750b3da9..0834b93a4b740 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -486,9 +486,9 @@ func TestConstantQueryParams(t *testing.T) { // Actually run an auth request. Although it says OIDC, the flow is the same // for oauth2. - cli, resp := fake.OIDCCallback(t, state, jwt.MapClaims{}) + resp := fake.OIDCCallback(t, state, jwt.MapClaims{}) require.True(t, callbackCalled) - + require.Equal(t, http.StatusOK, resp.StatusCode) } type testConfig struct { From b8e83330353b1fba2136af68b880056764e07d7a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 10 Apr 2024 11:05:22 -0500 Subject: [PATCH 3/3] linting --- coderd/coderdtest/oidctest/idp_test.go | 1 + coderd/externalauth/externalauth_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/coderd/coderdtest/oidctest/idp_test.go b/coderd/coderdtest/oidctest/idp_test.go index ce88f84273756..7706834785960 100644 --- a/coderd/coderdtest/oidctest/idp_test.go +++ b/coderd/coderdtest/oidctest/idp_test.go @@ -54,6 +54,7 @@ func TestFakeIDPBasicFlow(t *testing.T) { token = oauthToken }) + //nolint:bodyclose resp := fake.OIDCCallback(t, expectedState, jwt.MapClaims{}) require.Equal(t, http.StatusOK, resp.StatusCode) diff --git a/coderd/externalauth/externalauth_test.go b/coderd/externalauth/externalauth_test.go index 0834b93a4b740..88f3b7a3b59e9 100644 --- a/coderd/externalauth/externalauth_test.go +++ b/coderd/externalauth/externalauth_test.go @@ -486,6 +486,7 @@ func TestConstantQueryParams(t *testing.T) { // Actually run an auth request. Although it says OIDC, the flow is the same // for oauth2. + //nolint:bodyclose resp := fake.OIDCCallback(t, state, jwt.MapClaims{}) require.True(t, callbackCalled) require.Equal(t, http.StatusOK, resp.StatusCode)