Merge pull request #3284 from influxdata/bugfix/basepath-logout

Fix logout when using basepath & simplify basepath usage

CHANGELOG.md

@@ -16,6 +16,7 @@
 1.  [#3252](https://github.com/influxdata/chronograf/pull/3252): Allows users to select tickscript editor with mouse
 1.  [#3279](https://github.com/influxdata/chronograf/pull/3279): Change color when value is equal to or greater than threshold value
 1.  [#3281](https://github.com/influxdata/chronograf/pull/3281): Fix base path for kapacitor logs
+1.  [#3284](https://github.com/influxdata/chronograf/pull/3284): Fix logout when using basepath & simplify basepath usage (deprecates `PREFIX_ROUTES`)
 
 ## v1.4.4.1 [2018-04-16]
 

server/mux.go

@@ -25,7 +25,6 @@ type MuxOpts struct {
 	Logger        chronograf.Logger
 	Develop       bool                 // Develop loads assets from filesystem instead of bindata
 	Basepath      string               // URL path prefix under which all chronograf routes will be mounted
-	PrefixRoutes  bool                 // Mounts all backend routes under route specified by the Basepath
 	UseAuth       bool                 // UseAuth turns on Github OAuth and JWT
 	Auth          oauth2.Authenticator // Auth is used to authenticate and authorize
 	ProviderFuncs []func(func(oauth2.Provider, oauth2.Mux))
@@ -44,7 +43,7 @@ func NewMux(opts MuxOpts, service Service) http.Handler {
 	})
 
 	// Prefix any URLs found in the React assets with any configured basepath
-	prefixedAssets := NewDefaultURLPrefixer(basepath, assets, opts.Logger)
+	prefixedAssets := NewDefaultURLPrefixer(opts.Basepath, assets, opts.Logger)
 
 	// Compress the assets with gzip if an accepted encoding
 	compressed := gziphandler.GzipHandler(prefixedAssets)
@@ -57,7 +56,7 @@ func NewMux(opts MuxOpts, service Service) http.Handler {
 	var router chronograf.Router = hr
 
 	// Set route prefix for all routes if basepath is present
-	if opts.PrefixRoutes {
+	if opts.Basepath != "" {
 		router = &MountableRouter{
 			Prefix:   opts.Basepath,
 			Delegate: hr,
@@ -313,11 +312,6 @@ func NewMux(opts MuxOpts, service Service) http.Handler {
 
 	var out http.Handler
 
-	basepath := ""
-	if opts.PrefixRoutes {
-		basepath = opts.Basepath
-	}
-
 	/* Authentication */
 	if opts.UseAuth {
 		// Encapsulate the router with OAuth2
@@ -326,10 +320,10 @@ func NewMux(opts MuxOpts, service Service) http.Handler {
 		allRoutes.LogoutLink = path.Join(opts.Basepath, "/oauth/logout")
 
 		// Create middleware that redirects to the appropriate provider logout
-		router.GET(allRoutes.LogoutLink, Logout("/", basepath, allRoutes.AuthRoutes))
+		router.GET("/oauth/logout", Logout("/", opts.Basepath, allRoutes.AuthRoutes))
-		out = Logger(opts.Logger, PrefixedRedirect(opts.Basepath, auth))
+		out = Logger(opts.Logger, FlushingHandler(auth))
 	} else {
-		out = Logger(opts.Logger, PrefixedRedirect(opts.Basepath, router))
+		out = Logger(opts.Logger, FlushingHandler(router))
 	}
 
 	return out
@@ -363,13 +357,8 @@ func AuthAPI(opts MuxOpts, router chronograf.Router) (http.Handler, AuthRoutes)
 		})
 	}
 
-	rootPath := "/chronograf/v1"
+	rootPath := path.Join(opts.Basepath, "/chronograf/v1")
-	logoutPath := "/oauth/logout"
+	logoutPath := path.Join(opts.Basepath, "/oauth/logout")
-
-	if opts.PrefixRoutes {
-		rootPath = path.Join(opts.Basepath, rootPath)
-		logoutPath = path.Join(opts.Basepath, logoutPath)
-	}
 
 	tokenMiddleware := AuthorizedToken(opts.Auth, opts.Logger, router)
 	// Wrap the API with token validation middleware.

server/prefixing_redirector.go

@@ -2,49 +2,32 @@ package server
 
 import (
 	"net/http"
-	"net/url"
-	"path"
-	"strings"
 )
 
-type interceptingResponseWriter struct {
+type flushingResponseWriter struct {
 	http.ResponseWriter
-	Flusher http.Flusher
-	Prefix  string
 }
 
-func (i *interceptingResponseWriter) WriteHeader(status int) {
+func (f *flushingResponseWriter) WriteHeader(status int) {
-	if status >= 300 && status < 400 {
+	f.ResponseWriter.WriteHeader(status)
-		location := i.ResponseWriter.Header().Get("Location")
-		if u, err := url.Parse(location); err == nil && !u.IsAbs() {
-			hasPrefix := strings.HasPrefix(u.Path, i.Prefix)
-			if !hasPrefix || (hasPrefix && !strings.HasPrefix(u.Path[len(i.Prefix):], i.Prefix)) {
-				i.ResponseWriter.Header().Set("Location", path.Join(i.Prefix, location)+"/")
-			}
-		}
-	}
-	i.ResponseWriter.WriteHeader(status)
 }
 
 // Flush is here because the underlying HTTP chunked transfer response writer
 // to implement http.Flusher.  Without it data is silently buffered.  This
 // was discovered when proxying kapacitor chunked logs.
-func (i *interceptingResponseWriter) Flush() {
+func (f *flushingResponseWriter) Flush() {
-	if i.Flusher != nil {
+	if flusher, ok := f.ResponseWriter.(http.Flusher); ok {
-		i.Flusher.Flush()
+		flusher.Flush()
 	}
 }
 
-// PrefixedRedirect alters the Location header of downstream http.Handlers
+// FlushingHandler may not actually do anything, but it was ostensibly
-// to include a specified prefix
+// implemented to flush response writers that can be flushed for the
-func PrefixedRedirect(prefix string, next http.Handler) http.Handler {
+// purposes in the comment above.
+func FlushingHandler(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		iw := &interceptingResponseWriter{
+		iw := &flushingResponseWriter{
 			ResponseWriter: w,
-			Prefix:         prefix,
-		}
-		if flusher, ok := w.(http.Flusher); ok {
-			iw.Flusher = flusher
 		}
 		next.ServeHTTP(iw, r)
 	})

server/prefixing_redirector_test.go

@@ -1,87 +0,0 @@
-package server
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"strings"
-	"testing"
-)
-
-var prefixingRedirectTests = []struct {
-	CaseName       string
-	RedirectTarget string
-	Prefix         string
-	Expected       string
-}{
-	{
-		"ChronografBasepath",
-		"/chronograf/v1/",
-		"/chronograf",
-		"/chronograf/chronograf/v1/",
-	},
-	{
-		"DifferentBasepath",
-		"/chronograf/v1/",
-		"/delorean",
-		"/delorean/chronograf/v1/",
-	},
-	{
-		"TrailingSlashPrefix",
-		"/chronograf/v1/",
-		"/delorean/",
-		"/delorean/chronograf/v1/",
-	},
-	{
-		"NoPrefix",
-		"/chronograf/v1/",
-		"",
-		"/chronograf/v1/",
-	},
-	{
-		"SlashPrefix",
-		"/chronograf/v1/",
-		"/",
-		"/chronograf/v1/",
-	},
-	{
-		"AlreadyPrefixed",
-		"/chronograf/chronograf/v1/",
-		"/chronograf",
-		"/chronograf/chronograf/v1/",
-	},
-}
-
-func Test_PrefixingRedirector(t *testing.T) {
-	t.Parallel()
-	for _, p := range prefixingRedirectTests {
-		t.Run(p.CaseName, func(subt *testing.T) {
-			hf := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-				rw.Header().Set("Location", p.RedirectTarget)
-				rw.WriteHeader(http.StatusTemporaryRedirect)
-			})
-			pr := PrefixedRedirect(p.Prefix, hf)
-
-			ts := httptest.NewServer(pr)
-			defer ts.Close()
-
-			hc := http.Client{
-				CheckRedirect: func(req *http.Request, via []*http.Request) error {
-					return http.ErrUseLastResponse
-				},
-			}
-
-			mockBody := strings.NewReader("")
-			req, _ := http.NewRequest("GET", ts.URL, mockBody)
-
-			resp, err := hc.Do(req)
-			if err != nil {
-				subt.Fatal("Unexpected http err:", err)
-			}
-
-			expected := p.Expected
-			if loc := resp.Header.Get("Location"); loc != expected {
-				subt.Fatal("Unexpected redirected location. Expected:", expected, "Actual:", loc)
-			}
-		})
-	}
-}

server/server.go

@@ -3,6 +3,7 @@ package server
 import (
 	"context"
 	"crypto/tls"
+	"fmt"
 	"log"
 	"math/rand"
 	"net"
@@ -10,6 +11,7 @@ import (
 	"net/url"
 	"os"
 	"path"
+	"regexp"
 	"runtime"
 	"strconv"
 	"time"
@@ -27,7 +29,6 @@ import (
 
 var (
 	startTime time.Time
-	basepath  string
 )
 
 func init() {
@@ -96,8 +97,7 @@ type Server struct {
 
 	ReportingDisabled bool   `short:"r" long:"reporting-disabled" description:"Disable reporting of usage stats (os,arch,version,cluster_id,uptime) once every 24hr" env:"REPORTING_DISABLED"`
 	LogLevel          string `short:"l" long:"log-level" value-name:"choice" choice:"debug" choice:"info" choice:"error" default:"info" description:"Set the logging level" env:"LOG_LEVEL"`
-	Basepath          string `short:"p" long:"basepath" description:"A URL path prefix under which all chronograf routes will be mounted" env:"BASE_PATH"`
+	Basepath          string `short:"p" long:"basepath" description:"A URL path prefix under which all chronograf routes will be mounted. (Note: PREFIX_ROUTES has been deprecated. Now, if basepath is set, all routes will be prefixed with it.)" env:"BASE_PATH"`
-	PrefixRoutes      bool   `long:"prefix-routes" description:"Force chronograf server to require that all requests to it are prefixed with the value set in --basepath" env:"PREFIX_ROUTES"`
 	ShowVersion       bool   `short:"v" long:"version" description:"Show Chronograf version info"`
 	BuildInfo         chronograf.BuildInfo
 	Listener          net.Listener
@@ -344,11 +344,13 @@ func (s *Server) Serve(ctx context.Context) error {
 		return err
 	}
 
-	basepath = s.Basepath
+	if !validBasepath(s.Basepath) {
-	if basepath != "" && s.PrefixRoutes == false {
+		err := fmt.Errorf("Invalid basepath, must follow format \"/mybasepath\"")
 		logger.
 			WithField("component", "server").
-			Info("Note: you may want to use --prefix-routes with --basepath. Try `./chronograf --help` for more info.")
+			WithField("basepath", "invalid").
+			Error(err)
+		return err
 	}
 
 	providerFuncs := []func(func(oauth2.Provider, oauth2.Mux)){}
@@ -366,8 +368,7 @@ func (s *Server) Serve(ctx context.Context) error {
 		Logger:        logger,
 		UseAuth:       s.useAuth(),
 		ProviderFuncs: providerFuncs,
-		Basepath:      basepath,
+		Basepath:      s.Basepath,
-		PrefixRoutes:  s.PrefixRoutes,
 		StatusFeedURL: s.StatusFeedURL,
 		CustomLinks:   s.CustomLinks,
 	}, service)
@@ -537,3 +538,8 @@ func clientUsage(values client.Values) *client.Usage {
 		},
 	}
 }
+
+func validBasepath(basepath string) bool {
+	re := regexp.MustCompile(`(\/{1}\w+)+`)
+	return re.ReplaceAllLiteralString(basepath, "") == ""
+}

server/server_test.go

@@ -3,10 +3,12 @@ package server
 import (
 	"context"
 	"net/http"
+	"testing"
 
 	"github.com/bouk/httprouter"
 )
 
+// WithContext is a helper function to cut down on boilerplate in server test files
 func WithContext(ctx context.Context, r *http.Request, kv map[string]string) *http.Request {
 	params := make(httprouter.Params, 0, len(kv))
 	for k, v := range kv {
@@ -17,3 +19,50 @@ func WithContext(ctx context.Context, r *http.Request, kv map[string]string) *ht
 	}
 	return r.WithContext(httprouter.WithParams(ctx, params))
 }
+
+func Test_validBasepath(t *testing.T) {
+	type args struct {
+		basepath string
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "Basepath can be empty",
+			args: args{
+				basepath: "",
+			},
+			want: true,
+		},
+		{
+			name: "Basepath is not empty and valid",
+			args: args{
+				basepath: "/russ",
+			},
+			want: true,
+		},
+		{
+			name: "Basepath is not empty and invalid - no slashes",
+			args: args{
+				basepath: "russ",
+			},
+			want: false,
+		},
+		{
+			name: "Basepath is not empty and invalid - extra slashes",
+			args: args{
+				basepath: "//russ//",
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := validBasepath(tt.args.basepath); got != tt.want {
+				t.Errorf("validBasepath() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}