Merge pull request #4 from patrickod/patrickod/fail-open-open-config

Don't allow wide-open Google or Github configs

5 files changed, 47 insertions, 16 deletions

diff --git a/cmd/cashierd/main.go b/cmd/cashierd/main.go
index e482dde..61461a7 100644
--- a/cmd/cashierd/main.go
+++ b/cmd/cashierd/main.go
@@ -212,13 +212,17 @@ func main() {
 	var authprovider auth.Provider
 	switch config.Auth.Provider {
 	case "google":
-		authprovider = google.New(&config.Auth)
+		authprovider, err = google.New(&config.Auth)
 	case "github":
-		authprovider = github.New(&config.Auth)
+		authprovider, err = github.New(&config.Auth)
 	default:
 		log.Fatalln("Unknown provider %s", config.Auth.Provider)
 	}
 
+	if err != nil {
+		log.Fatal(err)
+	}
+
 	ctx := &appContext{
 		cookiestore:  sessions.NewCookieStore([]byte(config.Server.CookieSecret)),
 		authprovider: authprovider,
diff --git a/server/auth/github/github.go b/server/auth/github/github.go
index 1c62d9b..192cd9d 100644
--- a/server/auth/github/github.go
+++ b/server/auth/github/github.go
@@ -1,6 +1,7 @@
 package github
 
 import (
+	"errors"
 	"net/http"
 
 	"github.com/nsheridan/cashier/server/auth"
@@ -23,7 +24,10 @@ type Config struct {
 }
 
 // New creates a new Github provider from a configuration.
-func New(c *config.Auth) auth.Provider {
+func New(c *config.Auth) (auth.Provider, error) {
+	if c.ProviderOpts["organization"] == "" {
+		return nil, errors.New("github_opts organization must not be empty")
+	}
 	return &Config{
 		config: &oauth2.Config{
 			ClientID:     c.OauthClientID,
@@ -36,7 +40,7 @@ func New(c *config.Auth) auth.Provider {
 			},
 		},
 		organization: c.ProviderOpts["organization"],
-	}
+	}, nil
 }
 
 // A new oauth2 http client.
@@ -54,9 +58,6 @@ func (c *Config) Valid(token *oauth2.Token) bool {
 	if !token.Valid() {
 		return false
 	}
-	if c.organization == "" {
-		return true
-	}
 	client := githubapi.NewClient(c.newClient(token))
 	member, _, err := client.Organizations.IsMember(c.organization, c.Username(token))
 	if err != nil {
diff --git a/server/auth/github/github_test.go b/server/auth/github/github_test.go
index 383642f..f50d134 100644
--- a/server/auth/github/github_test.go
+++ b/server/auth/github/github_test.go
@@ -19,7 +19,7 @@ var (
 func TestNew(t *testing.T) {
 	a := assert.New(t)
 
-	p := newGithub()
+	p, _ := newGithub()
 	g := p.(*Config)
 	a.Equal(g.config.ClientID, oauthClientID)
 	a.Equal(g.config.ClientSecret, oauthClientSecret)
@@ -27,10 +27,20 @@ func TestNew(t *testing.T) {
 	a.Equal(g.organization, organization)
 }
 
+func TestNewEmptyOrganization(t *testing.T) {
+	organization = ""
+	a := assert.New(t)
+
+	_, err := newGithub()
+	a.EqualError(err, "github_opts organization must not be empty")
+
+	organization = "exampleorg"
+}
+
 func TestStartSession(t *testing.T) {
 	a := assert.New(t)
 
-	p := newGithub()
+	p, _ := newGithub()
 	s := p.StartSession("test_state")
 	a.Equal(s.State, "test_state")
 	a.Contains(s.AuthURL, "github.com/login/oauth/authorize")
@@ -38,7 +48,7 @@ func TestStartSession(t *testing.T) {
 	a.Contains(s.AuthURL, fmt.Sprintf("client_id=%s", oauthClientID))
 }
 
-func newGithub() auth.Provider {
+func newGithub() (auth.Provider, error) {
 	c := &config.Auth{
 		OauthClientID:     oauthClientID,
 		OauthClientSecret: oauthClientSecret,
diff --git a/server/auth/google/google.go b/server/auth/google/google.go
index cf780f3..0328d42 100644
--- a/server/auth/google/google.go
+++ b/server/auth/google/google.go
@@ -1,6 +1,7 @@
 package google
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -26,7 +27,11 @@ type Config struct {
 }
 
 // New creates a new Google provider from a configuration.
-func New(c *config.Auth) auth.Provider {
+func New(c *config.Auth) (auth.Provider, error) {
+	if c.ProviderOpts["domain"] == "" {
+		return nil, errors.New("google_opts domain must not be empty")
+	}
+
 	return &Config{
 		config: &oauth2.Config{
 			ClientID:     c.OauthClientID,
@@ -36,7 +41,7 @@ func New(c *config.Auth) auth.Provider {
 			Scopes:       []string{googleapi.UserinfoEmailScope, googleapi.UserinfoProfileScope},
 		},
 		domain: c.ProviderOpts["domain"],
-	}
+	}, nil
 }
 
 // A new oauth2 http client.
diff --git a/server/auth/google/google_test.go b/server/auth/google/google_test.go
index c6a3def..4d41986 100644
--- a/server/auth/google/google_test.go
+++ b/server/auth/google/google_test.go
@@ -19,7 +19,7 @@ var (
 func TestNew(t *testing.T) {
 	a := assert.New(t)
 
-	p := newGoogle()
+	p, _ := newGoogle()
 	g := p.(*Config)
 	a.Equal(g.config.ClientID, oauthClientID)
 	a.Equal(g.config.ClientSecret, oauthClientSecret)
@@ -27,10 +27,22 @@ func TestNew(t *testing.T) {
 	a.Equal(g.domain, domain)
 }
 
+func TestNewWithoutDomain(t *testing.T) {
+	a := assert.New(t)
+
+	domain = ""
+
+	_, err := newGoogle()
+	a.EqualError(err, "google_opts domain must not be empty")
+
+	domain = "example.com"
+}
+
 func TestStartSession(t *testing.T) {
 	a := assert.New(t)
 
-	p := newGoogle()
+	p, err := newGoogle()
+	a.NoError(err)
 	s := p.StartSession("test_state")
 	a.Equal(s.State, "test_state")
 	a.Contains(s.AuthURL, "accounts.google.com/o/oauth2/auth")
@@ -39,13 +51,12 @@ func TestStartSession(t *testing.T) {
 	a.Contains(s.AuthURL, fmt.Sprintf("client_id=%s", oauthClientID))
 }
 
-func newGoogle() auth.Provider {
+func newGoogle() (auth.Provider, error) {
 	c := &config.Auth{
 		OauthClientID:     oauthClientID,
 		OauthClientSecret: oauthClientSecret,
 		OauthCallbackURL:  oauthCallbackURL,
 		ProviderOpts:      map[string]string{"domain": domain},
 	}
-	c.ProviderOpts["domain"] = domain
 	return New(c)
 }