sso_proxy: fix session revalidation/refresh when group validation isn't being used

internal/pkg/options/email_address_validator.go

@@ -25,8 +25,6 @@ type EmailAddressValidator struct {
 // - is non-empty
 // - matches one of the originally passed in email addresses
 //   (case insensitive)
-// - if the originally passed in list of emails consists only of "*", then all emails
-//   are considered valid based on their domain.
 // If valid, nil is returned in place of an error.
 func NewEmailAddressValidator(allowedEmails []string) EmailAddressValidator {
 	var emailAddresses []string
@@ -50,10 +48,6 @@ func (v EmailAddressValidator) Validate(session *sessions.SessionState) error {
 		return ErrEmailAddressDenied
 	}
 
-	if len(v.AllowedEmails) == 1 && v.AllowedEmails[0] == "*" {
-		return nil
-	}
-
 	err := v.validate(session)
 	if err != nil {
 		return err

internal/pkg/options/email_address_validator_test.go

@@ -95,28 +95,28 @@ func TestEmailAddressValidatorValidator(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name:          "single wildcard allows all",
+			name:          "single wildcard is denied",
 			AllowedEmails: []string{"*"},
 			session: &sessions.SessionState{
 				Email: "[email protected]",
 			},
-			expectedErr: nil,
+			expectedErr: ErrEmailAddressDenied,
 		},
 		{
-			name:          "single wildcard allows all",
-			AllowedEmails: []string{"*"},
+			name:          "wildcard is ignored if other domains included are invalid",
+			AllowedEmails: []string{"[email protected]", "*"},
 			session: &sessions.SessionState{
-				Email: "bar@gmail.com",
+				Email: "foo@gmail.com",
 			},
-			expectedErr: nil,
+			expectedErr: ErrEmailAddressDenied,
 		},
 		{
-			name:          "wildcard is ignored if other domains included",
+			name:          "wildcard is ignored if other domains included are valid",
 			AllowedEmails: []string{"[email protected]", "*"},
 			session: &sessions.SessionState{
-				Email: "foo@gmail.com",
+				Email: "foo@example.com",
 			},
-			expectedErr: ErrEmailAddressDenied,
+			expectedErr: nil,
 		},
 		{
 			name:          "empty email rejected",

internal/pkg/options/email_domain_validator.go

@@ -25,19 +25,13 @@ type EmailDomainValidator struct {
 // - is non-empty
 // - the domain of the email address matches one of the originally passed in domains.
 //   (case insensitive)
-// - if the originally passed in list of domains consists only of "*", then all emails
-//   are considered valid based on their domain.
 // If valid, nil is returned in place of an error.
 func NewEmailDomainValidator(allowedDomains []string) EmailDomainValidator {
 	emailDomains := make([]string, 0, len(allowedDomains))
 
 	for _, domain := range allowedDomains {
-		if domain == "*" {
-			emailDomains = append(emailDomains, domain)
-		} else {
-			emailDomain := fmt.Sprintf("@%s", strings.ToLower(domain))
-			emailDomains = append(emailDomains, emailDomain)
-		}
+		emailDomain := fmt.Sprintf("@%s", strings.ToLower(domain))
+		emailDomains = append(emailDomains, emailDomain)
 	}
 	return EmailDomainValidator{
 		AllowedDomains: emailDomains,
@@ -53,10 +47,6 @@ func (v EmailDomainValidator) Validate(session *sessions.SessionState) error {
 		return ErrEmailDomainDenied
 	}
 
-	if len(v.AllowedDomains) == 1 && v.AllowedDomains[0] == "*" {
-		return nil
-	}
-
 	err := v.validate(session)
 	if err != nil {
 		return err

internal/pkg/options/email_domain_validator_test.go

@@ -95,28 +95,28 @@ func TestEmailDomainValidatorValidator(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name:           "single wildcard allows all",
+			name:           "single wildcard is denied",
 			allowedDomains: []string{"*"},
 			session: &sessions.SessionState{
 				Email: "[email protected]",
 			},
-			expectedErr: nil,
+			expectedErr: ErrEmailDomainDenied,
 		},
 		{
-			name:           "single wildcard allows all",
-			allowedDomains: []string{"*"},
+			name:           "wildcard is ignored if other domains are included are invalid",
+			allowedDomains: []string{"*", "example.com"},
 			session: &sessions.SessionState{
-				Email: "bar@gmail.com",
+				Email: "foo@gmal.com",
 			},
-			expectedErr: nil,
+			expectedErr: ErrEmailDomainDenied,
 		},
 		{
-			name:           "wildcard is ignored if other domains are included",
+			name:           "wildcard is ignored if other domains are included are valid",
 			allowedDomains: []string{"*", "example.com"},
 			session: &sessions.SessionState{
-				Email: "foo@gmal.com",
+				Email: "foo@example.com",
 			},
-			expectedErr: ErrEmailDomainDenied,
+			expectedErr: nil,
 		},
 		{
 			name:           "empty email rejected",

internal/proxy/options.go

@@ -64,6 +64,7 @@ type Options struct {
 	DefaultAllowedEmailDomains   []string `envconfig:"DEFAULT_ALLOWED_EMAIL_DOMAINS"`
 	DefaultAllowedEmailAddresses []string `envconfig:"DEFAULT_ALLOWED_EMAIL_ADDRESSES"`
 	DefaultAllowedGroups         []string `envconfig:"DEFAULT_ALLOWED_GROUPS"`
+	AllowNoValidators            bool     `envconfig:"ALLOW_NO_VALIDATORS" default:"false"`
 
 	ClientID     string `envconfig:"CLIENT_ID"`
 	ClientSecret string `envconfig:"CLIENT_SECRET"`
@@ -196,20 +197,42 @@ func (o *Options) Validate() error {
 	}
 
 	if o.upstreamConfigs != nil {
-		invalidUpstreams := []string{}
+		noValidatorsDefined := []string{}
+		wildcardUsed := []string{}
 		for _, uc := range o.upstreamConfigs {
 			if uc.Timeout > o.TCPWriteTimeout {
 				o.TCPWriteTimeout = uc.Timeout
 			}
 
-			if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 && len(uc.AllowedGroups) == 0 {
-				invalidUpstreams = append(invalidUpstreams, uc.Service)
+			// If we don't explicity accept 'no validators' then ensure defined validator values aren't len(0) or len(1) with a wildcard.
+			// Not accepting wildcards is a new restriction, so we largely test against this for now to raise awareness and avoid unexpected outcomes.
+			if !o.AllowNoValidators {
+				if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 && len(uc.AllowedGroups) == 0 {
+					noValidatorsDefined = append(noValidatorsDefined, uc.Service)
+				} else {
+					validatorValues := make(map[string][]string)
+					validatorValues["Addresses"] = uc.AllowedEmailAddresses
+					validatorValues["Domains"] = uc.AllowedEmailDomains
+					validatorValues["Groups"] = uc.AllowedGroups
+					for _, v := range validatorValues {
+						if len(v) != 0 && v[0] == "*" {
+							wildcardUsed = append(wildcardUsed, uc.Service)
+							break
+						}
+					}
+				}
 			}
 		}
-		if len(invalidUpstreams) != 0 {
+		if len(noValidatorsDefined) != 0 {
 			msgs = append(msgs, fmt.Sprintf(
-				"missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config in the following upstreams: %v",
-				invalidUpstreams))
+				`missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config.
+				If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: %v`,
+				noValidatorsDefined))
+		}
+		if len(wildcardUsed) != 0 {
+			msgs = append(msgs, fmt.Sprintf(
+				"invalid setting: a wildcard cannot be used within validators. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected upstreams: %v",
+				wildcardUsed))
 		}
 	}
 

internal/proxy/options_test.go

@@ -16,7 +16,7 @@ func testOptions() *Options {
 	o.CookieSecret = testEncodedCookieSecret
 	o.ClientID = "bazquux"
 	o.ClientSecret = "xyzzyplugh"
-	o.DefaultAllowedEmailDomains = []string{"*"}
+	o.AllowNoValidators = true
 	o.DefaultProviderSlug = "idp"
 	o.ProviderURLString = "https://www.example.com"
 	o.UpstreamConfigsFile = "testdata/upstream_configs.yml"
@@ -50,6 +50,7 @@ func TestNewOptions(t *testing.T) {
 	err := o.Validate()
 	testutil.NotEqual(t, nil, err)
 
+	//TODO: invalid setting: wildcards used in validators not tested
 	expected := errorMsg([]string{
 		"missing setting: cluster",
 		"missing setting: provider-url",
@@ -59,7 +60,7 @@ func TestNewOptions(t *testing.T) {
 		"missing setting: client-secret",
 		"missing setting: statsd-host",
 		"missing setting: statsd-port",
-		"missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in 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.\n\t\t\t\tIf no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: [testService]",
 		"Invalid value for COOKIE_SECRET; must decode to 32 or 64 bytes, but decoded to 0 bytes",
 	})
 	testutil.Equal(t, expected, err.Error())
@@ -70,6 +71,80 @@ func TestInitializedOptions(t *testing.T) {
 	testutil.Equal(t, nil, o.Validate())
 }
 
+func TestValidatorOptions(t *testing.T) {
+	testCases := []struct {
+		name              string
+		upstreamConfig    *UpstreamConfig
+		AllowNoValidators bool
+		expectedErrors    []string
+	}{
+		{
+			name: "no validators error",
+			upstreamConfig: &UpstreamConfig{
+				Service:               "testService",
+				AllowedEmailDomains:   []string{},
+				AllowedEmailAddresses: []string{},
+				AllowedGroups:         []string{},
+			},
+			expectedErrors: []string{
+				"missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config.\n\t\t\t\tIf no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: [testService]",
+			},
+		},
+		{
+			name: "wildcard error",
+			upstreamConfig: &UpstreamConfig{
+				Service:               "testService",
+				AllowedEmailDomains:   []string{"*"},
+				AllowedEmailAddresses: []string{"*"},
+				AllowedGroups:         []string{"*"},
+			},
+			expectedErrors: []string{
+				"invalid setting: a wildcard cannot be used within validators. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected upstreams: [testService]",
+			},
+		},
+		{
+			name: "validators defined, one using wildcard returns error",
+			upstreamConfig: &UpstreamConfig{
+				Service:               "testService",
+				AllowedEmailDomains:   []string{""},
+				AllowedEmailAddresses: []string{"*"},
+				AllowedGroups:         []string{"foo"},
+			},
+			expectedErrors: []string{
+				"invalid setting: a wildcard cannot be used within validators. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected upstreams: [testService]",
+			},
+		},
+		{
+			name: "AllowNoValidators is true. No validators defined returns success",
+			upstreamConfig: &UpstreamConfig{
+				Service:               "testService",
+				AllowedEmailDomains:   []string{},
+				AllowedEmailAddresses: []string{},
+				AllowedGroups:         []string{},
+			},
+			AllowNoValidators: true,
+			expectedErrors:    []string{},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			o := testOptions()
+			o.AllowNoValidators = tc.AllowNoValidators
+			o.UpstreamConfigsFile = ""
+			if !tc.AllowNoValidators {
+				o.upstreamConfigs = []*UpstreamConfig{tc.upstreamConfig}
+			}
+
+			err := o.Validate()
+			// Above we override the o.UpstreamConfigsFile setting to ensure we test these upstream configs with a clean slate,
+			// but this means we always get the below 'missing setting: upstream-configs' error.
+			tc.expectedErrors = append([]string{"missing setting: upstream-configs"}, tc.expectedErrors...)
+			testutil.Equal(t, errorMsg(tc.expectedErrors), err.Error())
+		})
+	}
+}
+
 func TestProviderURLValidation(t *testing.T) {
 	testCases := []struct {
 		name                              string

internal/proxy/providers/sso.go

@@ -181,6 +181,9 @@ 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 {

internal/proxy/providers/sso_test.go

@@ -144,11 +144,11 @@ func TestSSOProviderGroups(t *testing.T) {
 		ProfileStatus    int
 	}{
 		{
-			Name:             "invalid when no group id set",
+			Name:             "valid when no group id set",
 			Email:            "[email protected]",
 			Groups:           []string{},
 			ProxyGroupIds:    []string{},
-			ExpectedValid:    false,
+			ExpectedValid:    true,
 			ExpectedInGroups: []string{},
 			ExpectError:      nil,
 		},
@@ -319,7 +319,7 @@ func TestSSOProviderValidateSessionState(t *testing.T) {
 			ProviderResponse: http.StatusOK,
 			Groups:           []string{},
 			ProxyGroupIds:    []string{},
-			ExpectedValid:    false,
+			ExpectedValid:    true,
 		},
 		{
 			Name: "invalid when response is is not 200",