diff --git a/CHANGELOG.md b/CHANGELOG.md index f71bd11955..83265d179f 100644 --- a/CHANGELOG.md +++ b/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] diff --git a/server/mux.go b/server/mux.go index f8b3160927..cecf7a99b6 100644 --- a/server/mux.go +++ b/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)) - out = Logger(opts.Logger, PrefixedRedirect(opts.Basepath, auth)) + router.GET("/oauth/logout", Logout("/", opts.Basepath, allRoutes.AuthRoutes)) + 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" - logoutPath := "/oauth/logout" - - if opts.PrefixRoutes { - rootPath = path.Join(opts.Basepath, rootPath) - logoutPath = path.Join(opts.Basepath, logoutPath) - } + rootPath := path.Join(opts.Basepath, "/chronograf/v1") + logoutPath := path.Join(opts.Basepath, "/oauth/logout") tokenMiddleware := AuthorizedToken(opts.Auth, opts.Logger, router) // Wrap the API with token validation middleware. diff --git a/server/prefixing_redirector.go b/server/prefixing_redirector.go index 2c4652d870..0317ceb399 100644 --- a/server/prefixing_redirector.go +++ b/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) { - if status >= 300 && status < 400 { - 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) +func (f *flushingResponseWriter) WriteHeader(status int) { + f.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() { - if i.Flusher != nil { - i.Flusher.Flush() +func (f *flushingResponseWriter) Flush() { + if flusher, ok := f.ResponseWriter.(http.Flusher); ok { + flusher.Flush() } } -// PrefixedRedirect alters the Location header of downstream http.Handlers -// to include a specified prefix -func PrefixedRedirect(prefix string, next http.Handler) http.Handler { +// FlushingHandler may not actually do anything, but it was ostensibly +// implemented to flush response writers that can be flushed for the +// 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) }) diff --git a/server/prefixing_redirector_test.go b/server/prefixing_redirector_test.go deleted file mode 100644 index 728c906047..0000000000 --- a/server/prefixing_redirector_test.go +++ /dev/null @@ -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) - } - }) - } -} diff --git a/server/server.go b/server/server.go index c783e34f2e..9d7f44e5ac 100644 --- a/server/server.go +++ b/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"` - 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"` + 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"` 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 basepath != "" && s.PrefixRoutes == false { + if !validBasepath(s.Basepath) { + 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, - PrefixRoutes: s.PrefixRoutes, + Basepath: s.Basepath, 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, "") == "" +} diff --git a/server/server_test.go b/server/server_test.go index 3f6c11b160..f6dd40c423 100644 --- a/server/server_test.go +++ b/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) + } + }) + } +}