Merge pull request #264 from buzzfeed/jusshersmith-fix-proxy-validato…

…r-options sso_*: allow group validator to be used standalone
buzzfeed · Oct 30, 2019 · b76969f · b76969f
2 parents 90b4cc8 + 8be879b
commit b76969f
Show file tree

Hide file tree

Showing 6 changed files with 10 additions and 17 deletions.
diff --git a/internal/pkg/options/email_group_validator.go b/internal/pkg/options/email_group_validator.go
@@ -22,8 +22,6 @@ type EmailGroupValidator struct {
 
 // NewEmailGroupValidator takes in a Provider object and a list of groups, and returns a Validator object.
 // The validator can be used to validate that the session.Email:
-// - if an empty list is passed in in place of a list of groups, all session.Emails will be considered valid
-//   regardless of group membership with that particular Provider.
 // - according to the Provider that was passed in, is a member of one of the originally passed in groups.
 // If valid, nil is returned in place of an error.
 func NewEmailGroupValidator(provider providers.Provider, allowedGroups []string) EmailGroupValidator {
@@ -34,10 +32,6 @@ func NewEmailGroupValidator(provider providers.Provider, allowedGroups []string)
 }
 
 func (v EmailGroupValidator) Validate(session *sessions.SessionState) error {
-	if len(v.AllowedGroups) == 0 {
-		return nil
-	}
-
 	err := v.validate(session)
 	if err != nil {
 		return err

diff --git a/internal/proxy/options.go b/internal/proxy/options.go
@@ -199,13 +199,13 @@ func (o *Options) Validate() error {
 				o.TCPWriteTimeout = uc.Timeout
 			}
 
-			if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 {
+			if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 && len(uc.AllowedGroups) == 0 {
 				invalidUpstreams = append(invalidUpstreams, uc.Service)
 			}
 		}
 		if len(invalidUpstreams) != 0 {
 			msgs = append(msgs, fmt.Sprintf(
-				"missing setting: DEFAULT_ALLOWED_EMAIL_DOMAINS or DEFAULT_ALLOWED_EMAIL_ADDRESSES in either environment or upstream config in the following upstreams: %v",
+				"missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config in the following upstreams: %v",
 				invalidUpstreams))
 		}
 	}

diff --git a/internal/proxy/options_test.go b/internal/proxy/options_test.go
@@ -59,7 +59,7 @@ func TestNewOptions(t *testing.T) {
 		"missing setting: client-secret",
 		"missing setting: statsd-host",
 		"missing setting: statsd-port",
-		"missing setting: DEFAULT_ALLOWED_EMAIL_DOMAINS or DEFAULT_ALLOWED_EMAIL_ADDRESSES in either environment or upstream config in the following upstreams: [testService]",
+		"missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config in the following upstreams: [testService]",
 		"Invalid value for COOKIE_SECRET; must decode to 32 or 64 bytes, but decoded to 0 bytes",
 	})
 	testutil.Equal(t, expected, err.Error())

diff --git a/internal/proxy/providers/sso.go b/internal/proxy/providers/sso.go
@@ -181,9 +181,6 @@ func (p *SSOProvider) ValidateGroup(email string, allowedGroups []string, access
 
 	logger.WithUser(email).WithAllowedGroups(allowedGroups).Info("validating groups")
 	inGroups := []string{}
-	if len(allowedGroups) == 0 {
-		return inGroups, true, nil
-	}
 
 	userGroups, err := p.UserGroups(email, allowedGroups, accessToken)
 	if err != nil {

diff --git a/internal/proxy/providers/sso_test.go b/internal/proxy/providers/sso_test.go
@@ -144,11 +144,11 @@ func TestSSOProviderGroups(t *testing.T) {
 		ProfileStatus    int
 	}{
 		{
-			Name:             "valid when no group id set",
+			Name:             "invalid when no group id set",
 			Email:            "michael.bland@gsa.gov",
 			Groups:           []string{},
 			ProxyGroupIds:    []string{},
-			ExpectedValid:    true,
+			ExpectedValid:    false,
 			ExpectedInGroups: []string{},
 			ExpectError:      nil,
 		},
@@ -311,15 +311,15 @@ func TestSSOProviderValidateSessionState(t *testing.T) {
 		ExpectedValid    bool
 	}{
 		{
-			Name: "valid when no group id set",
+			Name: "invalid when no group id set",
 			SessionState: &sessions.SessionState{
 				AccessToken: "abc",
 				Email:       "michael.bland@gsa.gov",
 			},
 			ProviderResponse: http.StatusOK,
 			Groups:           []string{},
 			ProxyGroupIds:    []string{},
-			ExpectedValid:    true,
+			ExpectedValid:    false,
 		},
 		{
 			Name: "invalid when response is is not 200",

diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go
@@ -47,7 +47,9 @@ func New(opts *Options) (*SSOProxy, error) {
 			validators = append(validators, options.NewEmailDomainValidator(upstreamConfig.AllowedEmailDomains))
 		}
 
-		validators = append(validators, options.NewEmailGroupValidator(provider, upstreamConfig.AllowedGroups))
+		if len(upstreamConfig.AllowedGroups) != 0 {
+			validators = append(validators, options.NewEmailGroupValidator(provider, upstreamConfig.AllowedGroups))
+		}
 
 		optFuncs = append(optFuncs,
 			SetProvider(provider),