From 6f86efb594721bc577c56b284f5f2499e563c45c Mon Sep 17 00:00:00 2001
From: Patrick O'Doherty <p@trickod.com>
Date: Mon, 23 May 2016 17:56:15 +0100
Subject: [PATCH] Don't allow wide-open Google or Github configs
Fail loudly if either the google_opts domain value or github_opts organization
values are not set in the configuration. The lack of these values means that
a) in the Google case any @gmail.com address will be allowed
b) the Github case any Github user will be allowed.
This was previously documented but left as a foot-gun in the code.
Future commits will allow for explicit wildcards to be set.
---
cmd/cashierd/main.go | 8 ++++++--
server/auth/github/github.go | 11 ++++++-----
server/auth/github/github_test.go | 16 +++++++++++++---
server/auth/google/google.go | 9 +++++++--
server/auth/google/google_test.go | 19 +++++++++++++++----
5 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/cmd/cashierd/main.go b/cmd/cashierd/main.go
index e482ddec..61461a74 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 1c62d9b9..192cd9d2 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 383642f3..f50d1344 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 cf780f3b..0328d428 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 c6a3def6..4d41986b 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)
}
--
GitLab