Merge pull request #1564 from influxdata/fix/sourceless_logout

Fix logout menu item functionality & provide logoutLink only when using auth

CHANGELOG.md

@@ -3,6 +3,7 @@
 ### Bug Fixes
 1. [#1530](https://github.com/influxdata/chronograf/pull/1530): Update query config field ordering to always match input query
 1. [#1535](https://github.com/influxdata/chronograf/pull/1535): Fix add field functions to existing Kapacitor rules
+1. [#1564](https://github.com/influxdata/chronograf/pull/1564): Fix regression of logout menu item functionality
 1. [#1562](https://github.com/influxdata/chronograf/pull/1562): Fix InfluxQL parsing with multiple tag values for a tag key
 
 ### Features

server/mux.go

@@ -179,30 +179,34 @@ func NewMux(opts MuxOpts, service Service) http.Handler {
 
 	router.PUT("/chronograf/v1/sources/:id/dbs/:dbid/rps/:rpid", service.UpdateRetentionPolicy)
 	router.DELETE("/chronograf/v1/sources/:id/dbs/:dbid/rps/:rpid", service.DropRetentionPolicy)
-	var authRoutes AuthRoutes
+
+	allRoutes := &AllRoutes{
+		Logger: opts.Logger,
+	}
+
+	router.Handler("GET", "/chronograf/v1/", allRoutes)
+
 	var out http.Handler
 
-	/* Authentication */
-	logout := "/oauth/logout"
 	basepath := ""
 	if opts.PrefixRoutes {
 		basepath = opts.Basepath
 	}
+
+	/* Authentication */
 	if opts.UseAuth {
 		// Encapsulate the router with OAuth2
 		var auth http.Handler
-		auth, authRoutes = AuthAPI(opts, router)
+		auth, allRoutes.AuthRoutes = AuthAPI(opts, router)
+		allRoutes.LogoutLink = "/oauth/logout"
 
-		// Create middleware to redirect to the appropriate provider logout
-		targetURL := "/"
-		router.GET(logout, Logout(targetURL, basepath, authRoutes))
+		// 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))
 	} else {
 		out = Logger(opts.Logger, PrefixedRedirect(opts.Basepath, router))
 	}
 
-	router.GET("/chronograf/v1/", AllRoutes(authRoutes, path.Join(opts.Basepath, logout), opts.Logger))
-
 	return out
 }
 

server/routes.go

@@ -38,26 +38,33 @@ type getRoutesResponse struct {
 	Logout     *string     `json:"logout,omitempty"` // Location of the logout route for all auth routes
 }
 
-// AllRoutes returns all top level routes within chronograf
-func AllRoutes(authRoutes []AuthRoute, logout string, logger chronograf.Logger) http.HandlerFunc {
+// AllRoutes is a handler that returns all links to resources in Chronograf server.
+// Optionally, routes for authentication can be returned.
+type AllRoutes struct {
+	AuthRoutes []AuthRoute // Location of all auth routes. If no auth, this can be empty.
+	LogoutLink string      // Location of the logout route for all auth routes. If no auth, this can be empty.
+	Logger     chronograf.Logger
+}
+
+// ServeHTTP returns all top level routes within chronograf
+func (a *AllRoutes) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	routes := getRoutesResponse{
 		Sources:    "/chronograf/v1/sources",
 		Layouts:    "/chronograf/v1/layouts",
 		Me:         "/chronograf/v1/me",
 		Mappings:   "/chronograf/v1/mappings",
 		Dashboards: "/chronograf/v1/dashboards",
-		Auth:       make([]AuthRoute, len(authRoutes)),
-	}
-	if logout != "" {
-		routes.Logout = &logout
+		Auth:       make([]AuthRoute, len(a.AuthRoutes)), // We want to return at least an empty array, rather than null
 	}
 
-	for i, route := range authRoutes {
+	// The JSON response will have no field present for the LogoutLink if there is no logout link.
+	if a.LogoutLink != "" {
+		routes.Logout = &a.LogoutLink
+	}
+
+	for i, route := range a.AuthRoutes {
 		routes.Auth[i] = route
 	}
 
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		encodeJSON(w, http.StatusOK, routes, logger)
-		return
-	})
+	encodeJSON(w, http.StatusOK, routes, a.Logger)
 }

server/routes_test.go

@@ -11,10 +11,12 @@ import (
 
 func TestAllRoutes(t *testing.T) {
 	logger := log.New(log.DebugLevel)
-	handler := AllRoutes([]AuthRoute{}, "", logger)
+	handler := &AllRoutes{
+		Logger: logger,
+	}
 	req := httptest.NewRequest("GET", "http://docbrowns-inventions.com", nil)
 	w := httptest.NewRecorder()
-	handler(w, req)
+	handler.ServeHTTP(w, req)
 
 	resp := w.Result()
 	body, err := ioutil.ReadAll(resp.Body)
@@ -27,4 +29,47 @@ func TestAllRoutes(t *testing.T) {
 	if err := json.Unmarshal(body, &routes); err != nil {
 		t.Error("TestAllRoutes not able to unmarshal JSON response")
 	}
+	want := `{"layouts":"/chronograf/v1/layouts","mappings":"/chronograf/v1/mappings","sources":"/chronograf/v1/sources","me":"/chronograf/v1/me","dashboards":"/chronograf/v1/dashboards","auth":[]}
+`
+	if want != string(body) {
+		t.Errorf("TestAllRoutes\nwanted\n*%s*\ngot\n*%s*", want, string(body))
+	}
+
+}
+
+func TestAllRoutesWithAuth(t *testing.T) {
+	logger := log.New(log.DebugLevel)
+	handler := &AllRoutes{
+		AuthRoutes: []AuthRoute{
+			{
+				Name:     "github",
+				Label:    "GitHub",
+				Login:    "/oauth/github/login",
+				Logout:   "/oauth/github/logout",
+				Callback: "/oauth/github/callback",
+			},
+		},
+		LogoutLink: "/oauth/logout",
+		Logger:     logger,
+	}
+	req := httptest.NewRequest("GET", "http://docbrowns-inventions.com", nil)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	resp := w.Result()
+	body, err := ioutil.ReadAll(resp.Body)
+	defer resp.Body.Close()
+
+	if err != nil {
+		t.Error("TestAllRoutesWithAuth not able to retrieve body")
+	}
+	var routes getRoutesResponse
+	if err := json.Unmarshal(body, &routes); err != nil {
+		t.Error("TestAllRoutesWithAuth not able to unmarshal JSON response")
+	}
+	want := `{"layouts":"/chronograf/v1/layouts","mappings":"/chronograf/v1/mappings","sources":"/chronograf/v1/sources","me":"/chronograf/v1/me","dashboards":"/chronograf/v1/dashboards","auth":[{"name":"github","label":"GitHub","login":"/oauth/github/login","logout":"/oauth/github/logout","callback":"/oauth/github/callback"}],"logout":"/oauth/logout"}
+`
+	if want != string(body) {
+		t.Errorf("TestAllRoutesWithAuth\nwanted\n*%s*\ngot\n*%s*", want, string(body))
+	}
 }

ui/src/side_nav/components/NavItems.js

@@ -2,7 +2,7 @@ import React, {PropTypes} from 'react'
 import {Link} from 'react-router'
 import classnames from 'classnames'
 
-const {node, string} = PropTypes
+const {bool, node, string} = PropTypes
 
 const NavListItem = React.createClass({
   propTypes: {
@@ -16,7 +16,10 @@ const NavListItem = React.createClass({
     const isActive = location.startsWith(link)
 
     return (
-      <Link className={classnames('sidebar__menu-item', {active: isActive})} to={link}>
+      <Link
+        className={classnames('sidebar__menu-item', {active: isActive})}
+        to={link}
+      >
         {children}
       </Link>
     )
@@ -27,13 +30,20 @@ const NavHeader = React.createClass({
   propTypes: {
     link: string,
     title: string,
+    useAnchor: bool,
   },
   render() {
-    return (
-      <Link className="sidebar__menu-route" to={this.props.link}>
-        <h3 className="sidebar__menu-heading">{this.props.title}</h3>
+    const {link, title, useAnchor} = this.props
+
+    // Some nav items, such as Logout, need to hit an external link rather
+    // than simply route to an internal page. Anchor tags serve that purpose.
+    return useAnchor
+      ? <a className="sidebar__menu-route" href={link}>
+          <h3 className="sidebar__menu-heading">{title}</h3>
+        </a>
+      : <Link className="sidebar__menu-route" to={link}>
+          <h3 className="sidebar__menu-heading">{title}</h3>
         </Link>
-    )
   },
 })
 
@@ -63,7 +73,9 @@ const NavBlock = React.createClass({
     })
 
     return (
-      <div className={classnames('sidebar__square', className, {active: isActive})}>
+      <div
+        className={classnames('sidebar__square', className, {active: isActive})}
+      >
         {this.renderLink()}
         <div className={wrapperClassName || 'sidebar__menu-wrapper'}>
           <div className="sidebar__menu">

ui/src/side_nav/containers/SideNav.js

@@ -19,16 +19,12 @@ const SideNav = React.createClass({
     location: shape({
       pathname: string.isRequired,
     }).isRequired,
-    me: shape({
-      email: string,
-    }),
     isHidden: bool.isRequired,
     logoutLink: string,
   },
 
   render() {
     const {
-      me,
       params: {sourceID},
       location: {pathname: location},
       isHidden,
@@ -37,7 +33,7 @@ const SideNav = React.createClass({
 
     const sourcePrefix = `/sources/${sourceID}`
     const dataExplorerLink = `${sourcePrefix}/chronograf/data-explorer`
-    const showLogout = !!(me && me.name)
+    const showLogout = !!logoutLink
 
     return isHidden
       ? null
@@ -81,7 +77,7 @@ const SideNav = React.createClass({
           </NavBlock>
           {showLogout
             ? <NavBlock icon="user-outline" className="sidebar__square-last">
-                <NavHeader link={logoutLink} title="Logout" />
+                <NavHeader useAnchor={true} link={logoutLink} title="Logout" />
               </NavBlock>
             : null}
         </NavBar>
@@ -89,10 +85,9 @@ const SideNav = React.createClass({
 })
 
 const mapStateToProps = ({
-  auth: {me, logoutLink},
+  auth: {logoutLink},
   app: {ephemeral: {inPresentationMode}},
 }) => ({
-  me,
   isHidden: inPresentationMode,
   logoutLink,
 })