From 6f86efb594721bc577c56b284f5f2499e563c45c Mon Sep 17 00:00:00 2001 From: Patrick O'Doherty Date: Mon, 23 May 2016 17:56:15 +0100 Subject: 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. --- 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 +++++++++++++++---- 4 files changed, 41 insertions(+), 14 deletions(-) (limited to 'server') 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) } -- cgit v1.2.3