Merge pull request #1235 from govau/sessioncleanup

Allow all environment variables to be set in common way

controllers/root_test.go

@@ -8,14 +8,15 @@ import (
 	"github.com/cloudfoundry-community/go-cfenv"
 
 	"github.com/18F/cg-dashboard/controllers"
+	"github.com/18F/cg-dashboard/helpers"
 	. "github.com/18F/cg-dashboard/helpers/testhelpers"
 	. "github.com/18F/cg-dashboard/helpers/testhelpers/docker"
 )
 
 func TestPing(t *testing.T) {
 	response, request := NewTestRequest("GET", "/ping", nil)
 	env, _ := cfenv.Current()
-	router, _, err := controllers.InitApp(GetMockCompleteEnvVars(), env)
+	router, _, err := controllers.InitApp(helpers.NewEnvVarsFromPath(NewEnvLookupFromMap(GetMockCompleteEnvVars())), env)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -39,11 +40,11 @@ func TestPingWithRedis(t *testing.T) {
 	defer cleanUpRedis()
 	// Override the mock env vars to use redis for session backend.
 	envVars := GetMockCompleteEnvVars()
-	envVars.SessionBackend = "redis"
+	envVars[helpers.SessionBackendEnvVar] = "redis"
 	env, _ := cfenv.Current()
 
 	// Setup router.
-	router, _, err := controllers.InitApp(envVars, env)
+	router, _, err := controllers.InitApp(helpers.NewEnvVarsFromPath(NewEnvLookupFromMap(envVars)), env)
 	if err != nil {
 		t.Fatal(err)
 	}

controllers/routes.go

@@ -70,7 +70,7 @@ func InitRouter(settings *helpers.Settings, templates *helpers.Templates, mailer
 }
 
 // InitApp takes in envars and sets up the router and settings that will be used for the unstarted server.
-func InitApp(envVars helpers.EnvVars, env *cfenv.App) (*web.Router, *helpers.Settings, error) {
+func InitApp(envVars *helpers.EnvVars, env *cfenv.App) (*web.Router, *helpers.Settings, error) {
 	// Initialize the settings.
 	settings := helpers.Settings{}
 	if err := settings.InitSettings(envVars, env); err != nil {

controllers/routes_test.go

@@ -13,7 +13,7 @@ import (
 
 type initAppTest struct {
 	testName          string
-	envVars           helpers.EnvVars
+	envVars           map[string]string
 	returnRouterNil   bool
 	returnSettingsNil bool
 	returnErrorNil    bool
@@ -29,7 +29,7 @@ var initAppTests = []initAppTest{
 	},
 	{
 		testName:          "Blank EnvVars",
-		envVars:           helpers.EnvVars{},
+		envVars:           map[string]string{},
 		returnRouterNil:   true,
 		returnSettingsNil: true,
 		returnErrorNil:    false,
@@ -39,7 +39,7 @@ var initAppTests = []initAppTest{
 func TestInitApp(t *testing.T) {
 	for _, test := range initAppTests {
 		env, _ := cfenv.Current()
-		router, settings, err := controllers.InitApp(test.envVars, env)
+		router, settings, err := controllers.InitApp(helpers.NewEnvVarsFromPath(NewEnvLookupFromMap(test.envVars)), env)
 		if (router == nil) != test.returnRouterNil {
 			t.Errorf("Test %s did not return correct router value. Expected %t, Actual %t", test.testName, test.returnRouterNil, (router == nil))
 		} else if (settings == nil) != test.returnSettingsNil {

controllers/secure_test.go

@@ -81,7 +81,7 @@ func TestPrivilegedProxy(t *testing.T) {
 	for _, test := range proxyTests {
 		// We can only get this after the server has started.
 		testServer := CreateExternalServerForPrivileged(t, test)
-		test.EnvVars.UAAURL = testServer.URL
+		test.EnvVars[helpers.UAAURLEnvVar] = testServer.URL
 		// Construct full url for the proxy.
 		fullURL := fmt.Sprintf("%s%s", testServer.URL, test.RequestPath)
 		c := &controllers.SecureContext{Context: &controllers.Context{}}

helpers/env_vars.go

@@ -1,5 +1,13 @@
 package helpers
 
+import (
+	"fmt"
+	"log"
+	"strconv"
+
+	cfenv "github.com/cloudfoundry-community/go-cfenv"
+)
+
 var (
 	// ClientIDEnvVar is the environment variable key that represents the registered
 	// Client ID for this web app.
@@ -53,27 +61,100 @@ var (
 	TICSecretEnvVar = "TIC_SECRET"
 )
 
-// EnvVars holds all the environment variable values that a non-test server should have.
+// EnvVars provides a convenient method to access environment variables
 type EnvVars struct {
-	ClientID        string
-	ClientSecret    string
-	Hostname        string
-	LoginURL        string
-	UAAURL          string
-	APIURL          string
-	LogURL          string
-	PProfEnabled    string
-	BuildInfo       string
-	NewRelicLicense string
-	SecureCookies   string
-	LocalCF         string
-	SessionBackend  string
-	SessionKey      string
-	BasePath        string
-	SMTPHost        string
-	SMTPPort        string
-	SMTPUser        string
-	SMTPPass        string
-	SMTPFrom        string
-	TICSecret       string
+	path []EnvLookup
+}
+
+// String returns value for key if present, else returns defaultVal if not found
+func (el *EnvVars) String(key, defaultVal string) string {
+	rv, found := el.load(key)
+	if !found {
+		return defaultVal
+	}
+	return rv
+}
+
+// MustString will panic if value is not set, otherwise it returns the value.
+func (el *EnvVars) MustString(key string) string {
+	rv, found := el.load(key)
+	if !found {
+		panic(&ErrMissingEnvVar{Name: key})
+	}
+	return rv
+}
+
+// Bool looks for the key, and if found, parses it using strconv.ParseBool and returns
+// the result. If not found, returns false. If found and won't parse, panics.
+func (el *EnvVars) Bool(key string) bool {
+	val, found := el.load(key)
+	if !found {
+		return false
+	}
+
+	rv, err := strconv.ParseBool(val)
+	if err != nil {
+		// invalid values will now return an error
+		// previous behavior defaulted to false
+		panic(err)
+	}
+
+	return rv
+}
+
+// load is an internal method that looks for a given key within
+// all elements in the path, and if none found, returns "", false.
+func (el *EnvVars) load(key string) (string, bool) {
+	for _, env := range el.path {
+		rv, found := env(key)
+		if found {
+			return rv, true
+		}
+	}
+	return "", false
+}
+
+// NewEnvVarsFromPath create an EnvVars object, where the elements in the path
+// are searched in order to load a given variable.
+func NewEnvVarsFromPath(path ...EnvLookup) *EnvVars {
+	return &EnvVars{path: path}
+}
+
+// EnvLookup must return the value for the given key and whether it was found or not
+type EnvLookup func(key string) (string, bool)
+
+// ErrMissingEnvVar is panicked if a MustGet fails.
+type ErrMissingEnvVar struct {
+	// Name of the key that was not found
+	Name string
+}
+
+// Error returns an error string
+func (err *ErrMissingEnvVar) Error() string {
+	return fmt.Sprintf("missing env variable: %s", err.Name)
+}
+
+// NewEnvLookupFromCFAppNamedService looks for a CloudFoundry bound service
+// with the given name, and will allow sourcing of environment variables
+// from there. If no service is found, a warning is printed, but no error thrown.
+func NewEnvLookupFromCFAppNamedService(cfApp *cfenv.App, namedService string) EnvLookup {
+	service, err := cfApp.Services.WithName(namedService)
+	if err != nil {
+		log.Printf("Warning: No bound service found with name: %s, will not be used for sourcing env variables.\n", namedService)
+	}
+	return func(name string) (string, bool) {
+		if service == nil { // no service
+			return "", false
+		}
+		serviceVar, found := service.Credentials[name]
+		if !found {
+			return "", false
+		}
+		serviceVarAsString, ok := serviceVar.(string)
+		if !ok {
+			log.Printf("Warning: variable found in service for %s, but unable to cast as string, so ignoring.\n", name)
+			return "", false
+		}
+		return serviceVarAsString, true
+	}
 }

helpers/settings.go

@@ -90,50 +90,32 @@ func (s *Settings) CreateContext() context.Context {
 
 // InitSettings attempts to populate all the fields of the Settings struct. It will return an error if it fails,
 // otherwise it returns nil for success.
-func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
-	if len(envVars.ClientID) == 0 {
-		return errors.New("Unable to find '" + ClientIDEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.ClientSecret) == 0 {
-		return errors.New("Unable to find '" + ClientSecretEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.Hostname) == 0 {
-		return errors.New("Unable to find '" + HostnameEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.LoginURL) == 0 {
-		return errors.New("Unable to find '" + LoginURLEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.UAAURL) == 0 {
-		return errors.New("Unable to find '" + UAAURLEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.APIURL) == 0 {
-		return errors.New("Unable to find '" + APIURLEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.LogURL) == 0 {
-		return errors.New("Unable to find '" + LogURLEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.SessionKey) == 0 {
-		return errors.New("Unable to find '" + SessionKeyEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.SMTPFrom) == 0 {
-		return errors.New("Unable to find '" + SMTPFromEnvVar + "' in environment. Exiting.\n")
-	}
-	if len(envVars.SMTPHost) == 0 {
-		return errors.New("Unable to find '" + SMTPHostEnvVar + "' in environment. Exiting.\n")
-	}
+func (s *Settings) InitSettings(envVars *EnvVars, env *cfenv.App) (retErr error) {
+	defer func() {
+		// While .MustString() is convenient in readability below, we'd prefer to convert this
+		// to an error for upstream callers.
+		if r := recover(); r != nil {
+			missingErr, ok := r.(*ErrMissingEnvVar)
+			if !ok {
+				// We don't know what this is, re-panic
+				panic(r)
+			}
 
-	s.BasePath = envVars.BasePath
-	s.AppURL = envVars.Hostname
-	s.ConsoleAPI = envVars.APIURL
-	s.LoginURL = envVars.LoginURL
-	s.UaaURL = envVars.UAAURL
-	s.LogURL = envVars.LogURL
-	s.PProfEnabled = ((envVars.PProfEnabled == "true") || (envVars.PProfEnabled == "1"))
-	if s.BuildInfo = envVars.BuildInfo; len(s.BuildInfo) == 0 {
-		s.BuildInfo = "developer-build"
-	}
-	s.LocalCF = ((envVars.LocalCF == "true") || (envVars.LocalCF == "1"))
-	s.SecureCookies = ((envVars.SecureCookies == "true") || (envVars.SecureCookies == "1"))
+			// Set return code to the actual error
+			retErr = missingErr
+		}
+	}()
+
+	s.BasePath = envVars.String(BasePathEnvVar, "")
+	s.AppURL = envVars.MustString(HostnameEnvVar)
+	s.ConsoleAPI = envVars.MustString(APIURLEnvVar)
+	s.LoginURL = envVars.MustString(LoginURLEnvVar)
+	s.UaaURL = envVars.MustString(UAAURLEnvVar)
+	s.LogURL = envVars.MustString(LogURLEnvVar)
+	s.PProfEnabled = envVars.Bool(PProfEnabledEnvVar)
+	s.BuildInfo = envVars.String(BuildInfoEnvVar, "developer-build")
+	s.LocalCF = envVars.Bool(LocalCFEnvVar)
+	s.SecureCookies = envVars.Bool(SecureCookiesEnvVar)
 	// Safe guard: shouldn't run with insecure cookies if we are
 	// in a non-development environment (i.e. production)
 	if s.LocalCF == false && s.SecureCookies == false {
@@ -142,13 +124,13 @@ func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
 
 	// Setup OAuth2 Client Service.
 	s.OAuthConfig = &oauth2.Config{
-		ClientID:     envVars.ClientID,
-		ClientSecret: envVars.ClientSecret,
+		ClientID:     envVars.MustString(ClientIDEnvVar),
+		ClientSecret: envVars.MustString(ClientSecretEnvVar),
 		RedirectURL:  s.AppURL + "/oauth2callback",
 		Scopes:       []string{"cloud_controller.read", "cloud_controller.write", "cloud_controller.admin", "scim.read", "openid"},
 		Endpoint: oauth2.Endpoint{
-			AuthURL:  envVars.LoginURL + "/oauth/authorize",
-			TokenURL: envVars.UAAURL + "/oauth/token",
+			AuthURL:  envVars.MustString(LoginURLEnvVar) + "/oauth/authorize",
+			TokenURL: envVars.MustString(UAAURLEnvVar) + "/oauth/token",
 		},
 	}
 
@@ -157,7 +139,7 @@ func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
 	}
 
 	// Initialize Sessions.
-	switch envVars.SessionBackend {
+	switch envVars.String(SessionBackendEnvVar, "") {
 	case "redis":
 		address, password, err := getRedisSettings(env)
 		if err != nil {
@@ -193,7 +175,7 @@ func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
 			},
 		}
 		// create our redis pool.
-		store, err := redistore.NewRediStoreWithPool(redisPool, []byte(envVars.SessionKey))
+		store, err := redistore.NewRediStoreWithPool(redisPool, []byte(envVars.MustString(SessionKeyEnvVar)))
 		if err != nil {
 			return err
 		}
@@ -205,7 +187,7 @@ func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
 			Secure:   s.SecureCookies,
 		}
 		s.Sessions = store
-		s.SessionBackend = envVars.SessionBackend
+		s.SessionBackend = "redis"
 
 		// Use health check function where we do a PING.
 		s.SessionBackendHealthCheck = func() bool {
@@ -219,7 +201,7 @@ func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
 			return true
 		}
 	default:
-		store := sessions.NewFilesystemStore("", []byte(envVars.SessionKey))
+		store := sessions.NewFilesystemStore("", []byte(envVars.MustString(SessionKeyEnvVar)))
 		store.MaxLength(4096 * 4)
 		store.Options = &sessions.Options{
 			HttpOnly: true,
@@ -238,18 +220,18 @@ func (s *Settings) InitSettings(envVars EnvVars, env *cfenv.App) error {
 	gob.Register(oauth2.Token{})
 
 	s.HighPrivilegedOauthConfig = &clientcredentials.Config{
-		ClientID:     envVars.ClientID,
-		ClientSecret: envVars.ClientSecret,
+		ClientID:     envVars.MustString(ClientIDEnvVar),
+		ClientSecret: envVars.MustString(ClientSecretEnvVar),
 		Scopes:       []string{"scim.invite", "cloud_controller.admin", "scim.read"},
-		TokenURL:     envVars.UAAURL + "/oauth/token",
+		TokenURL:     envVars.MustString(UAAURLEnvVar) + "/oauth/token",
 	}
 
-	s.SMTPFrom = envVars.SMTPFrom
-	s.SMTPHost = envVars.SMTPHost
-	s.SMTPPass = envVars.SMTPPass
-	s.SMTPPort = envVars.SMTPPort
-	s.SMTPUser = envVars.SMTPUser
-	s.TICSecret = envVars.TICSecret
+	s.SMTPFrom = envVars.MustString(SMTPFromEnvVar)
+	s.SMTPHost = envVars.MustString(SMTPHostEnvVar)
+	s.SMTPPass = envVars.String(SMTPPassEnvVar, "")
+	s.SMTPPort = envVars.String(SMTPPortEnvVar, "")
+	s.SMTPUser = envVars.String(SMTPUserEnvVar, "")
+	s.TICSecret = envVars.String(TICSecretEnvVar, "")
 	return nil
 }