Improve AllowedRoute test table formatting

didinahmadi · Oct 7, 2020 · b7b7ade · b7b7ade
1 parent 89a8ac8
commit b7b7ade
Showing 4 changed files with 87 additions and 68 deletions.
diff --git a/oauthproxy.go b/oauthproxy.go
@@ -76,7 +76,7 @@ type OAuthProxy struct {
 	AuthOnlyPath      string
 	UserInfoPath      string
 
-	allowedRoutes           []*allowedRoute
+	allowedRoutes           []allowedRoute
 	redirectURL             *url.URL // the url to receive requests at
 	whitelistDomains        []string
 	provider                providers.Provider
@@ -285,16 +285,16 @@ func buildSignInMessage(opts *options.Options) string {
 // buildRoutesAllowlist builds an []allowedRoute  list from either the legacy
 // SkipAuthRegex option (paths only support) or newer SkipAuthRoutes option
 // (method=path support)
-func buildRoutesAllowlist(opts *options.Options) ([]*allowedRoute, error) {
-	routes := make([]*allowedRoute, 0, len(opts.SkipAuthRegex)+len(opts.SkipAuthRoutes))
+func buildRoutesAllowlist(opts *options.Options) ([]allowedRoute, error) {
+	routes := make([]allowedRoute, 0, len(opts.SkipAuthRegex)+len(opts.SkipAuthRoutes))
 
 	for _, path := range opts.SkipAuthRegex {
 		compiledRegex, err := regexp.Compile(path)
 		if err != nil {
 			return nil, err
 		}
 		logger.Printf("Skipping auth - Method: ALL | Path: %s", path)
-		routes = append(routes, &allowedRoute{
+		routes = append(routes, allowedRoute{
 			method:    "",
 			pathRegex: compiledRegex,
 		})
@@ -306,21 +306,21 @@ func buildRoutesAllowlist(opts *options.Options) ([]*allowedRoute, error) {
 			path   string
 		)
 
-		parts := strings.Split(methodPath, "=")
+		parts := strings.SplitN(methodPath, "=", 2)
 		if len(parts) == 1 {
 			method = ""
 			path = parts[0]
 		} else {
 			method = strings.ToUpper(parts[0])
-			path = strings.Join(parts[1:], "=")
+			path = parts[1]
 		}
 
 		compiledRegex, err := regexp.Compile(path)
 		if err != nil {
 			return nil, err
 		}
 		logger.Printf("Skipping auth - Method: %s | Path: %s", method, path)
-		routes = append(routes, &allowedRoute{
+		routes = append(routes, allowedRoute{
 			method:    method,
 			pathRegex: compiledRegex,
 		})

diff --git a/oauthproxy_test.go b/oauthproxy_test.go
@@ -2242,21 +2242,24 @@ func TestTrustedIPs(t *testing.T) {
 }
 
 func Test_buildRoutesAllowlist(t *testing.T) {
+	type expectedAllowedRoute struct {
+		method      string
+		regexString string
+	}
+
 	testCases := []struct {
-		name            string
-		skipAuthRegex   []string
-		skipAuthRoutes  []string
-		expectedMethods []string
-		expectedRegexes []string
-		shouldError     bool
+		name           string
+		skipAuthRegex  []string
+		skipAuthRoutes []string
+		expectedRoutes []expectedAllowedRoute
+		shouldError    bool
 	}{
 		{
-			name:            "No skip auth configured",
-			skipAuthRegex:   []string{},
-			skipAuthRoutes:  []string{},
-			expectedMethods: []string{},
-			expectedRegexes: []string{},
-			shouldError:     false,
+			name:           "No skip auth configured",
+			skipAuthRegex:  []string{},
+			skipAuthRoutes: []string{},
+			expectedRoutes: []expectedAllowedRoute{},
+			shouldError:    false,
 		},
 		{
 			name: "Only skipAuthRegex configured",
@@ -2265,13 +2268,15 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 				"^/baz/[0-9]+/thing",
 			},
 			skipAuthRoutes: []string{},
-			expectedMethods: []string{
-				"",
-				"",
-			},
-			expectedRegexes: []string{
-				"^/foo/bar",
-				"^/baz/[0-9]+/thing",
+			expectedRoutes: []expectedAllowedRoute{
+				{
+					method:      "",
+					regexString: "^/foo/bar",
+				},
+				{
+					method:      "",
+					regexString: "^/baz/[0-9]+/thing",
+				},
 			},
 			shouldError: false,
 		},
@@ -2285,19 +2290,27 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 				"WEIRD=^/methods/are/allowed",
 				"PATCH=/second/equals?are=handled&just=fine",
 			},
-			expectedMethods: []string{
-				"GET",
-				"POST",
-				"",
-				"WEIRD",
-				"PATCH",
-			},
-			expectedRegexes: []string{
-				"^/foo/bar",
-				"^/baz/[0-9]+/thing",
-				"^/all/methods$",
-				"^/methods/are/allowed",
-				"/second/equals?are=handled&just=fine",
+			expectedRoutes: []expectedAllowedRoute{
+				{
+					method:      "GET",
+					regexString: "^/foo/bar",
+				},
+				{
+					method:      "POST",
+					regexString: "^/baz/[0-9]+/thing",
+				},
+				{
+					method:      "",
+					regexString: "^/all/methods$",
+				},
+				{
+					method:      "WEIRD",
+					regexString: "^/methods/are/allowed",
+				},
+				{
+					method:      "PATCH",
+					regexString: "/second/equals?are=handled&just=fine",
+				},
 			},
 			shouldError: false,
 		},
@@ -2312,19 +2325,27 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 				"POST=^/baz/[0-9]+/thing",
 				"^/all/methods$",
 			},
-			expectedMethods: []string{
-				"",
-				"",
-				"GET",
-				"POST",
-				"",
-			},
-			expectedRegexes: []string{
-				"^/foo/bar/regex",
-				"^/baz/[0-9]+/thing/regex",
-				"^/foo/bar",
-				"^/baz/[0-9]+/thing",
-				"^/all/methods$",
+			expectedRoutes: []expectedAllowedRoute{
+				{
+					method:      "",
+					regexString: "^/foo/bar/regex",
+				},
+				{
+					method:      "",
+					regexString: "^/baz/[0-9]+/thing/regex",
+				},
+				{
+					method:      "GET",
+					regexString: "^/foo/bar",
+				},
+				{
+					method:      "POST",
+					regexString: "^/baz/[0-9]+/thing",
+				},
+				{
+					method:      "",
+					regexString: "^/all/methods$",
+				},
 			},
 			shouldError: false,
 		},
@@ -2335,10 +2356,9 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 				"^/baz/[0-9]+/thing",
 				"(bad[regex",
 			},
-			skipAuthRoutes:  []string{},
-			expectedMethods: []string{},
-			expectedRegexes: []string{},
-			shouldError:     true,
+			skipAuthRoutes: []string{},
+			expectedRoutes: []expectedAllowedRoute{},
+			shouldError:    true,
 		},
 		{
 			name:          "Invalid skipAuthRoutes entry",
@@ -2349,9 +2369,8 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 				"^/all/methods$",
 				"PUT=(bad[regex",
 			},
-			expectedMethods: []string{},
-			expectedRegexes: []string{},
-			shouldError:     true,
+			expectedRoutes: []expectedAllowedRoute{},
+			shouldError:    true,
 		},
 	}
 
@@ -2369,10 +2388,9 @@ func Test_buildRoutesAllowlist(t *testing.T) {
 			assert.NoError(t, err)
 
 			for i, route := range routes {
-				assert.Greater(t, len(tc.expectedMethods), i)
-				assert.Equal(t, route.method, tc.expectedMethods[i])
-				assert.Greater(t, len(tc.expectedRegexes), i)
-				assert.Equal(t, route.pathRegex.String(), tc.expectedRegexes[i])
+				assert.Greater(t, len(tc.expectedRoutes), i)
+				assert.Equal(t, route.method, tc.expectedRoutes[i].method)
+				assert.Equal(t, route.pathRegex.String(), tc.expectedRoutes[i].regexString)
 			}
 		})
 	}

diff --git a/pkg/validation/allowlist.go b/pkg/validation/allowlist.go
@@ -6,8 +6,8 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/ip"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip"
 )
 
 func validateAllowlists(o *options.Options) []string {
@@ -32,11 +32,11 @@ func validateRoutes(o *options.Options) []string {
 	msgs := []string{}
 	for _, route := range o.SkipAuthRoutes {
 		var regex string
-		parts := strings.Split(route, "=")
+		parts := strings.SplitN(route, "=", 2)
 		if len(parts) == 1 {
 			regex = parts[0]
 		} else {
-			regex = strings.Join(parts[1:], "=")
+			regex = parts[1]
 		}
 		_, err := regexp.Compile(regex)
 		if err != nil {

diff --git a/pkg/validation/allowlist_test.go b/pkg/validation/allowlist_test.go
@@ -1,10 +1,11 @@
 package validation
 
 import (
-	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
+
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
 )
 
 var _ = Describe("Allowlist", func() {