Fix backwards compat on "domain" config + remove "domains" config

Fixes #48

README.md

@@ -4,7 +4,6 @@
 
 A minimal forward authentication service that provides Google oauth based login and authentication for the [traefik](https://github.com/containous/traefik) reverse proxy/load balancer.
 
-
 ## Why?
 
 - Seamlessly overlays any http service with a single endpoint (see: `url-path` in [Configuration](#configuration))
@@ -16,6 +15,7 @@ A minimal forward authentication service that provides Google oauth based login
 
 # Contents
 
+- [Releases](#releases)
 - [Usage](#usage)
   - [Simple](#simple)
   - [Advanced](#advanced)
@@ -32,6 +32,16 @@ A minimal forward authentication service that provides Google oauth based login
 - [Copyright](#copyright)
 - [License](#license)
 
+## Releases
+
+We recommend using the `2` tag on docker hub.
+
+You can also use the latest incremental releases found on [docker hub](https://hub.docker.com/r/thomseddon/traefik-forward-auth/tags) and [github](https://github.com/thomseddon/traefik-forward-auth/releases).
+
+#### Upgrade Guide
+
+v2 was released in June 2019, whilst this is fully backwards compatible, a number of configuration options were modified, please see the [upgrade guide](https://github.com/thomseddon/traefik-forward-auth/wiki/v2-Upgrade-Guide) to prevent warnings on startup and ensure you are using the current configuration.
+
 ## Usage
 
 #### Simple:
@@ -96,10 +106,6 @@ Create a new project then search for and select "Credentials" in the search bar.
 
 Click "Create Credentials" > "OAuth client ID". Select "Web Application", fill in the name of your app, skip "Authorized JavaScript origins" and fill "Authorized redirect URIs" with all the domains you will allow authentication from, appended with the `url-path` (e.g. https://app.test.com/_oauth)
 
-#### Upgrade Guide
-
-v2 was released in April 2019, whilst this is fully backwards compatible, a number of configuration options were modified, please see the [upgrade guide](https://github.com/thomseddon/traefik-forward-auth/wiki/v2-Upgrade-Guide) to prevent warnings on startup and ensure you are using the current configuration.
-
 ## Configuration
 
 ### Overview

internal/config.go

@@ -31,7 +31,7 @@ type Config struct {
 	CookieName     string               `long:"cookie-name" env:"COOKIE_NAME" default:"_forward_auth" description:"Cookie Name"`
 	CSRFCookieName string               `long:"csrf-cookie-name" env:"CSRF_COOKIE_NAME" default:"_forward_auth_csrf" description:"CSRF Cookie Name"`
 	DefaultAction  string               `long:"default-action" env:"DEFAULT_ACTION" default:"auth" choice:"auth" choice:"allow" description:"Default action"`
-	Domains        []string             `long:"domain" env:"DOMAIN" description:"Only allow given email domains, can be set multiple times"`
+	Domains        CommaSeparatedList   `long:"domain" env:"DOMAIN" description:"Only allow given email domains, can be set multiple times"`
 	LifetimeString int                  `long:"lifetime" env:"LIFETIME" default:"43200" description:"Lifetime in seconds"`
 	Path           string               `long:"url-path" env:"URL_PATH" default:"/_oauth" description:"Callback URL Path"`
 	SecretString   string               `long:"secret" env:"SECRET" description:"Secret used for signing (required)" json:"-"`
@@ -45,13 +45,12 @@ type Config struct {
 	Lifetime time.Duration
 
 	// Legacy
-	CookieDomainsLegacy CookieDomains      `long:"cookie-domains" env:"COOKIE_DOMAINS" description:"DEPRECATED - Use \"cookie-domain\""`
+	CookieDomainsLegacy CookieDomains `long:"cookie-domains" env:"COOKIE_DOMAINS" description:"DEPRECATED - Use \"cookie-domain\""`
-	CookieSecretLegacy  string             `long:"cookie-secret" env:"COOKIE_SECRET" description:"DEPRECATED - Use \"secret\""  json:"-"`
+	CookieSecretLegacy  string        `long:"cookie-secret" env:"COOKIE_SECRET" description:"DEPRECATED - Use \"secret\""  json:"-"`
-	CookieSecureLegacy  string             `long:"cookie-secure" env:"COOKIE_SECURE" description:"DEPRECATED - Use \"insecure-cookie\""`
+	CookieSecureLegacy  string        `long:"cookie-secure" env:"COOKIE_SECURE" description:"DEPRECATED - Use \"insecure-cookie\""`
-	DomainsLegacy       CommaSeparatedList `long:"domains" env:"DOMAINS" description:"DEPRECATED - Use \"domain\""`
+	ClientIdLegacy      string        `long:"client-id" env:"CLIENT_ID" group:"DEPs" description:"DEPRECATED - Use \"providers.google.client-id\""`
-	ClientIdLegacy      string             `long:"client-id" env:"CLIENT_ID" group:"DEPs" description:"DEPRECATED - Use \"providers.google.client-id\""`
+	ClientSecretLegacy  string        `long:"client-secret" env:"CLIENT_SECRET" description:"DEPRECATED - Use \"providers.google.client-id\""  json:"-"`
-	ClientSecretLegacy  string             `long:"client-secret" env:"CLIENT_SECRET" description:"DEPRECATED - Use \"providers.google.client-id\""  json:"-"`
+	PromptLegacy        string        `long:"prompt" env:"PROMPT" description:"DEPRECATED - Use \"providers.google.prompt\""`
-	PromptLegacy        string             `long:"prompt" env:"PROMPT" description:"DEPRECATED - Use \"providers.google.prompt\""`
 }
 
 func NewGlobalConfig() Config {
@@ -125,10 +124,6 @@ func NewConfig(args []string) (Config, error) {
 		fmt.Println("cookie-domains config option is deprecated, please use cookie-domain")
 		c.CookieDomains = append(c.CookieDomains, c.CookieDomainsLegacy...)
 	}
-	if len(c.DomainsLegacy) > 0 {
-		fmt.Println("domains config option is deprecated, please use domain")
-		c.Domains = append(c.Domains, c.DomainsLegacy...)
-	}
 
 	// Transformations
 	if len(c.Path) > 0 && c.Path[0] != '/' {
@@ -176,16 +171,15 @@ func (c *Config) parseUnknownFlag(option string, arg flags.SplitArgument, args [
 	// Parse rules in the format "rule.<name>.<param>"
 	parts := strings.Split(option, ".")
 	if len(parts) == 3 && parts[0] == "rule" {
-		// Get or create rule
+		// Ensure there is a name
-		rule, ok := c.Rules[parts[1]]
+		name := parts[1]
-		if !ok {
+		if len(name) == 0 {
-			rule = NewRule()
+			return args, errors.New("route name is required")
-			c.Rules[parts[1]] = rule
 		}
 
 		// Get value, or pop the next arg
 		val, ok := arg.Value()
-		if !ok {
+		if !ok && len(args) > 1 {
 			val = args[0]
 			args = args[1:]
 		}
@@ -204,6 +198,13 @@ func (c *Config) parseUnknownFlag(option string, arg flags.SplitArgument, args [
 			}
 		}
 
+		// Get or create rule
+		rule, ok := c.Rules[name]
+		if !ok {
+			rule = NewRule()
+			c.Rules[name] = rule
+		}
+
 		// Add param value to rule
 		switch parts[2] {
 		case "action":
@@ -250,7 +251,7 @@ func (c *Config) Validate() {
 	}
 
 	if c.Providers.Google.ClientId == "" || c.Providers.Google.ClientSecret == "" {
-		log.Fatal("google.providers.client-id, google.providers.client-secret must be set")
+		log.Fatal("providers.google.client-id, providers.google.client-secret must be set")
 	}
 
 	// Check rules

internal/config_test.go

@@ -98,6 +98,28 @@ func TestConfigParseUnknownFlags(t *testing.T) {
 	}
 }
 
+func TestConfigParseRuleError(t *testing.T) {
+	assert := assert.New(t)
+
+	// Rule without name
+	_, err := NewConfig([]string{
+		"--rule..action=auth",
+	})
+	if assert.Error(err) {
+		assert.Equal("route name is required", err.Error())
+	}
+
+	// Rule without value
+	c, err := NewConfig([]string{
+		"--rule.one.action=",
+	})
+	if assert.Error(err) {
+		assert.Equal("route param value is required", err.Error())
+	}
+	// Check rules
+	assert.Equal(map[string]*Rule{}, c.Rules)
+}
+
 func TestConfigFlagBackwardsCompatability(t *testing.T) {
 	assert := assert.New(t)
 	c, err := NewConfig([]string{
@@ -109,7 +131,7 @@ func TestConfigFlagBackwardsCompatability(t *testing.T) {
 		"--cookie-secure=false",
 		"--cookie-domains=test1.com,example.org",
 		"--cookie-domain=another1.net",
-		"--domains=test2.com,example.org",
+		"--domain=test2.com,example.org",
 		"--domain=another2.net",
 		"--whitelist=test3.com,example.org",
 		"--whitelist=another3.net",
@@ -124,7 +146,7 @@ func TestConfigFlagBackwardsCompatability(t *testing.T) {
 	}
 	assert.Equal(expected1, c.CookieDomains, "should read legacy comma separated list cookie-domains")
 
-	expected2 := []string{"another2.net", "test2.com", "example.org"}
+	expected2 := CommaSeparatedList{"test2.com", "example.org", "another2.net"}
 	assert.Equal(expected2, c.Domains, "should read legacy comma separated list domains")
 
 	expected3 := CommaSeparatedList{"test3.com", "example.org", "another3.net"}
@@ -199,6 +221,70 @@ func TestConfigParseEnvironment(t *testing.T) {
 
 	assert.Equal("env_cookie_name", c.CookieName, "variable should be read from environment")
 	assert.Equal("env_client_id", c.Providers.Google.ClientId, "namespace variable should be read from environment")
+
+	os.Unsetenv("COOKIE_NAME")
+	os.Unsetenv("PROVIDERS_GOOGLE_CLIENT_ID")
+}
+
+func TestConfigParseEnvironmentBackwardsCompatability(t *testing.T) {
+	assert := assert.New(t)
+	vars := map[string]string{
+		"CLIENT_ID":      "clientid",
+		"CLIENT_SECRET":  "verysecret",
+		"PROMPT":         "prompt",
+		"COOKIE_SECRET":  "veryverysecret",
+		"LIFETIME":       "200",
+		"COOKIE_SECURE":  "false",
+		"COOKIE_DOMAINS": "test1.com,example.org",
+		"COOKIE_DOMAIN":  "another1.net",
+		"DOMAIN":        "test2.com,example.org",
+		"WHITELIST":      "test3.com,example.org",
+	}
+	for k, v := range vars {
+		os.Setenv(k, v)
+	}
+	c, err := NewConfig([]string{})
+	require.Nil(t, err)
+
+	// The following used to be passed as comma separated list
+	expected1 := []CookieDomain{
+		*NewCookieDomain("another1.net"),
+		*NewCookieDomain("test1.com"),
+		*NewCookieDomain("example.org"),
+	}
+	assert.Equal(expected1, c.CookieDomains, "should read legacy comma separated list cookie-domains")
+
+	expected2 := CommaSeparatedList{"test2.com", "example.org"}
+	assert.Equal(expected2, c.Domains, "should read legacy comma separated list domains")
+
+	expected3 := CommaSeparatedList{"test3.com", "example.org"}
+	assert.Equal(expected3, c.Whitelist, "should read legacy comma separated list whitelist")
+
+	// Name changed
+	assert.Equal([]byte("veryverysecret"), c.Secret)
+
+	// Google provider params used to be top level
+	assert.Equal("clientid", c.ClientIdLegacy)
+	assert.Equal("clientid", c.Providers.Google.ClientId, "--client-id should set providers.google.client-id")
+	assert.Equal("verysecret", c.ClientSecretLegacy)
+	assert.Equal("verysecret", c.Providers.Google.ClientSecret, "--client-secret should set providers.google.client-secret")
+	assert.Equal("prompt", c.PromptLegacy)
+	assert.Equal("prompt", c.Providers.Google.Prompt, "--prompt should set providers.google.promot")
+
+	// "cookie-secure" used to be a standard go bool flag that could take
+	// true, TRUE, 1, false, FALSE, 0 etc. values.
+	// Here we're checking that format is still suppoted
+	assert.Equal("false", c.CookieSecureLegacy)
+	assert.True(c.InsecureCookie, "--cookie-secure=false should set insecure-cookie true")
+
+	c, err = NewConfig([]string{"--cookie-secure=TRUE"})
+	assert.Nil(err)
+	assert.Equal("TRUE", c.CookieSecureLegacy)
+	assert.False(c.InsecureCookie, "--cookie-secure=TRUE should set insecure-cookie false")
+
+	for k := range vars {
+		os.Unsetenv(k)
+	}
 }
 
 func TestConfigTransformation(t *testing.T) {