diff --git a/internal/auth/authenticator.go b/internal/auth/authenticator.go index 78b180e3..169709d1 100644 --- a/internal/auth/authenticator.go +++ b/internal/auth/authenticator.go @@ -12,6 +12,7 @@ import ( "github.com/buzzfeed/sso/internal/auth/providers" "github.com/buzzfeed/sso/internal/pkg/aead" log "github.com/buzzfeed/sso/internal/pkg/logging" + "github.com/buzzfeed/sso/internal/pkg/options" "github.com/buzzfeed/sso/internal/pkg/sessions" "github.com/buzzfeed/sso/internal/pkg/templates" @@ -20,7 +21,7 @@ import ( // Authenticator stores all the information associated with proxying the request. type Authenticator struct { - Validator func(string) bool + Validators []options.Validator EmailDomains []string ProxyRootDomains []string Host string @@ -225,11 +226,16 @@ func (p *Authenticator) authenticate(rw http.ResponseWriter, req *http.Request) } } - if !p.Validator(session.Email) { - logger.WithUser(session.Email).Error("invalid email user") + errors := options.RunValidators(p.Validators, session) + if len(errors) == len(p.Validators) { + logger.WithUser(session.Email).Info( + fmt.Sprintf("permission denied: unauthorized: %q", errors)) return nil, ErrUserNotAuthorized } + logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( + fmt.Sprintf("authentication: user passed validation")) + return session, nil } @@ -575,13 +581,24 @@ func (p *Authenticator) getOAuthCallback(rw http.ResponseWriter, req *http.Reque // Set cookie, or deny: The authenticator validates the session email and group // - for p.Validator see validator.go#newValidatorImpl for more info // - for p.provider.ValidateGroup see providers/google.go#ValidateGroup for more info - if !p.Validator(session.Email) { + + errors := options.RunValidators(p.Validators, session) + if len(errors) == len(p.Validators) { tags := append(tags, "error:invalid_email") p.StatsdClient.Incr("application_error", tags, 1.0) - logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Error( - "invalid_email", "permission denied; unauthorized user") - return "", HTTPError{Code: http.StatusForbidden, Message: "Invalid Account"} + logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( + fmt.Sprintf("oauth callback: unauthorized: %q", errors)) + + formattedErrors := make([]string, 0, len(errors)) + for _, err := range errors { + formattedErrors = append(formattedErrors, err.Error()) + } + errorMsg := fmt.Sprintf("We ran into some issues while validating your account: \"%s\"", + strings.Join(formattedErrors, ", ")) + return "", HTTPError{Code: http.StatusForbidden, Message: errorMsg} } + logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( + fmt.Sprintf("oauth callback: user passed validation")) logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info("authentication complete") err = p.sessionStore.SaveSession(rw, req, session) diff --git a/internal/auth/authenticator_test.go b/internal/auth/authenticator_test.go index 52755c05..998cb998 100644 --- a/internal/auth/authenticator_test.go +++ b/internal/auth/authenticator_test.go @@ -17,6 +17,7 @@ import ( "github.com/buzzfeed/sso/internal/auth/providers" "github.com/buzzfeed/sso/internal/pkg/aead" + "github.com/buzzfeed/sso/internal/pkg/options" "github.com/buzzfeed/sso/internal/pkg/sessions" "github.com/buzzfeed/sso/internal/pkg/templates" "github.com/buzzfeed/sso/internal/pkg/testutil" @@ -66,13 +67,6 @@ func setTestProvider(provider *providers.TestProvider) func(*Authenticator) erro } } -func setMockValidator(response bool) func(*Authenticator) error { - return func(a *Authenticator) error { - a.Validator = func(string) bool { return response } - return nil - } -} - func setRedirectURL(redirectURL *url.URL) func(*Authenticator) error { return func(a *Authenticator) error { a.redirectURL = redirectURL @@ -424,7 +418,7 @@ func TestSignIn(t *testing.T) { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) auth, err := NewAuthenticator(config, - setMockValidator(tc.validEmail), + SetValidators([]options.Validator{options.NewMockValidator(tc.validEmail)}), setMockSessionStore(tc.mockSessionStore), setMockTempl(), setMockRedirectURL(), @@ -571,7 +565,7 @@ func TestSignOutPage(t *testing.T) { provider.RevokeError = tc.RevokeError p, _ := NewAuthenticator(config, - setMockValidator(true), + SetValidators([]options.Validator{options.NewMockValidator(true)}), setMockSessionStore(tc.mockSessionStore), setMockTempl(), setTestProvider(provider), @@ -948,7 +942,7 @@ func TestGetProfile(t *testing.T) { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) p, _ := NewAuthenticator(config, - setMockValidator(true), + SetValidators([]options.Validator{options.NewMockValidator(true)}), ) u, _ := url.Parse("http://example.com") testProvider := providers.NewTestProvider(u) @@ -1050,7 +1044,7 @@ func TestRedeemCode(t *testing.T) { config := testConfiguration(t) proxy, _ := NewAuthenticator(config, - setMockValidator(true), + SetValidators([]options.Validator{options.NewMockValidator(true)}), ) testURL, err := url.Parse("example.com") @@ -1357,7 +1351,8 @@ func TestOAuthCallback(t *testing.T) { Value: "state", }, }, - expectedError: HTTPError{Code: http.StatusForbidden, Message: "Invalid Account"}, + expectedError: HTTPError{Code: http.StatusForbidden, + Message: "We ran into some issues while validating your account: \"MockValidator error\""}, }, { name: "valid email, invalid redirect", @@ -1438,7 +1433,7 @@ func TestOAuthCallback(t *testing.T) { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) proxy, _ := NewAuthenticator(config, - setMockValidator(tc.validEmail), + SetValidators([]options.Validator{options.NewMockValidator(tc.validEmail)}), setMockCSRFStore(tc.csrfResp), setMockSessionStore(tc.sessionStore), ) @@ -1559,7 +1554,7 @@ func TestOAuthStart(t *testing.T) { provider := providers.NewTestProvider(nil) proxy, _ := NewAuthenticator(config, setTestProvider(provider), - setMockValidator(true), + SetValidators([]options.Validator{options.NewMockValidator(true)}), setMockRedirectURL(), setMockCSRFStore(&sessions.MockCSRFStore{}), ) diff --git a/internal/auth/mux.go b/internal/auth/mux.go index 270315a7..5fe0c639 100644 --- a/internal/auth/mux.go +++ b/internal/auth/mux.go @@ -18,12 +18,11 @@ type AuthenticatorMux struct { func NewAuthenticatorMux(config Configuration, statsdClient *statsd.Client) (*AuthenticatorMux, error) { logger := log.NewLogEntry() - - var validator func(string) bool + validators := []options.Validator{} if len(config.AuthorizeConfig.EmailConfig.Addresses) != 0 { - validator = options.NewEmailAddressValidator(config.AuthorizeConfig.EmailConfig.Addresses) + validators = append(validators, options.NewEmailAddressValidator(config.AuthorizeConfig.EmailConfig.Addresses)) } else { - validator = options.NewEmailDomainValidator(config.AuthorizeConfig.EmailConfig.Domains) + validators = append(validators, options.NewEmailDomainValidator(config.AuthorizeConfig.EmailConfig.Domains)) } authenticators := []*Authenticator{} @@ -38,7 +37,7 @@ func NewAuthenticatorMux(config Configuration, statsdClient *statsd.Client) (*Au idpSlug := idp.Data().ProviderSlug authenticator, err := NewAuthenticator(config, - SetValidator(validator), + SetValidators(validators), SetProvider(idp), SetCookieStore(config.SessionConfig, idpSlug), SetStatsdClient(statsdClient), diff --git a/internal/auth/options.go b/internal/auth/options.go index 5e8d41b8..a7d5f0d9 100644 --- a/internal/auth/options.go +++ b/internal/auth/options.go @@ -9,6 +9,7 @@ import ( "github.com/buzzfeed/sso/internal/auth/providers" "github.com/buzzfeed/sso/internal/pkg/aead" "github.com/buzzfeed/sso/internal/pkg/groups" + "github.com/buzzfeed/sso/internal/pkg/options" "github.com/buzzfeed/sso/internal/pkg/sessions" "github.com/datadog/datadog-go/statsd" @@ -96,9 +97,9 @@ func SetRedirectURL(serverConfig ServerConfig, slug string) func(*Authenticator) } // SetValidator sets the email validator -func SetValidator(validator func(string) bool) func(*Authenticator) error { +func SetValidators(validators []options.Validator) func(*Authenticator) error { return func(a *Authenticator) error { - a.Validator = validator + a.Validators = validators return nil } } diff --git a/internal/pkg/options/email_address_validator.go b/internal/pkg/options/email_address_validator.go index 6272ca86..02c6195e 100644 --- a/internal/pkg/options/email_address_validator.go +++ b/internal/pkg/options/email_address_validator.go @@ -1,38 +1,72 @@ package options import ( + "errors" "fmt" "strings" + + "github.com/buzzfeed/sso/internal/pkg/sessions" ) -// NewEmailAddressValidator returns a function that checks whether a given email is valid based on a list -// of email addresses. The address "*" is a wild card that matches any non-empty email. -func NewEmailAddressValidator(emails []string) func(string) bool { - allowAll := false +var ( + _ Validator = EmailAddressValidator{} + + // These error message should be formatted in such a way that is appropriate + // for display to the end user. + ErrEmailAddressDenied = errors.New("Unauthorized Email Address") +) + +type EmailAddressValidator struct { + AllowedEmails []string +} + +// NewEmailAddressValidator takes in a list of email addresses and returns a Validator object. +// The validator can be used to validate that the session.Email: +// - 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 - for _, email := range emails { - if email == "*" { - allowAll = true - } + for _, email := range allowedEmails { emailAddress := fmt.Sprintf("%s", strings.ToLower(email)) emailAddresses = append(emailAddresses, emailAddress) } - if allowAll { - return func(email string) bool { return email != "" } + return EmailAddressValidator{ + AllowedEmails: emailAddresses, } +} - return func(email string) bool { - if email == "" { - return false - } - email = strings.ToLower(email) - for _, emailItem := range emailAddresses { - if email == emailItem { - return true - } +func (v EmailAddressValidator) Validate(session *sessions.SessionState) error { + if session.Email == "" { + return ErrInvalidEmailAddress + } + + if len(v.AllowedEmails) == 0 { + return ErrEmailAddressDenied + } + + if len(v.AllowedEmails) == 1 && v.AllowedEmails[0] == "*" { + return nil + } + + err := v.validate(session) + if err != nil { + return err + } + return nil +} + +func (v EmailAddressValidator) validate(session *sessions.SessionState) error { + email := strings.ToLower(session.Email) + for _, emailItem := range v.AllowedEmails { + if email == emailItem { + return nil } - return false } + return ErrEmailAddressDenied } diff --git a/internal/pkg/options/email_address_validator_test.go b/internal/pkg/options/email_address_validator_test.go index 193fc035..aef92f75 100644 --- a/internal/pkg/options/email_address_validator_test.go +++ b/internal/pkg/options/email_address_validator_test.go @@ -2,119 +2,148 @@ package options import ( "testing" + + "github.com/buzzfeed/sso/internal/pkg/sessions" ) func TestEmailAddressValidatorValidator(t *testing.T) { testCases := []struct { - name string - domains []string - email string - expectValid bool + name string + AllowedEmails []string + email string + expectedErr error + session *sessions.SessionState }{ { - name: "nothing should validate when address list is empty", - domains: []string(nil), - email: "foo@example.com", - expectValid: false, - }, - { - name: "single address validation", - domains: []string{"foo@example.com"}, - email: "foo@example.com", - expectValid: true, + name: "nothing should validate when address list is empty", + AllowedEmails: []string(nil), + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: ErrEmailAddressDenied, }, { - name: "substring matches are rejected", - domains: []string{"foo@example.com"}, - email: "foo@hackerexample.com", - expectValid: false, + name: "single address validation", + AllowedEmails: []string{"foo@example.com"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "no subdomain rollup happens", - domains: []string{"foo@example.com"}, - email: "foo@bar.example.com", - expectValid: false, + name: "substring matches are rejected", + AllowedEmails: []string{"foo@example.com"}, + session: &sessions.SessionState{ + Email: "foo@hackerexample.com", + }, + expectedErr: ErrEmailAddressDenied, }, { - name: "multiple address validation still rejects other addresses", - domains: []string{"foo@abc.com", "foo@xyz.com"}, - email: "foo@example.com", - expectValid: false, + name: "no subdomain rollup happens", + AllowedEmails: []string{"foo@example.com"}, + session: &sessions.SessionState{ + Email: "foo@bar.example.com", + }, + expectedErr: ErrEmailAddressDenied, }, { - name: "multiple address validation still accepts emails from either address", - domains: []string{"foo@abc.com", "foo@xyz.com"}, - email: "foo@abc.com", - expectValid: true, + name: "multiple address validation still rejects other addresses", + AllowedEmails: []string{"foo@abc.com", "foo@xyz.com"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: ErrEmailAddressDenied, }, { - name: "multiple address validation still rejects other addresses", - domains: []string{"foo@abc.com", "bar@xyz.com"}, - email: "bar@xyz.com", - expectValid: true, + name: "multiple address validation still accepts emails from either address", + AllowedEmails: []string{"foo@abc.com", "foo@xyz.com"}, + session: &sessions.SessionState{ + Email: "foo@abc.com", + }, + expectedErr: nil, }, { - name: "comparisons are case insensitive", - domains: []string{"Foo@Example.Com"}, - email: "foo@example.com", - expectValid: true, + name: "multiple address validation still rejects other addresses", + AllowedEmails: []string{"foo@abc.com", "bar@xyz.com"}, + session: &sessions.SessionState{ + Email: "bar@xyz.com", + }, + expectedErr: nil, }, { - name: "comparisons are case insensitive", - domains: []string{"Foo@Example.Com"}, - email: "foo@EXAMPLE.COM", - expectValid: true, + name: "comparisons are case insensitive-1", + AllowedEmails: []string{"Foo@Example.Com"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "comparisons are case insensitive", - domains: []string{"foo@example.com"}, - email: "foo@ExAmPlE.CoM", - expectValid: true, + name: "comparisons are case insensitive-2", + AllowedEmails: []string{"Foo@Example.Com"}, + session: &sessions.SessionState{ + Email: "foo@EXAMPLE.COM", + }, + expectedErr: nil, }, { - name: "single wildcard allows all", - domains: []string{"*"}, - email: "foo@example.com", - expectValid: true, + name: "comparisons are case insensitive-3", + AllowedEmails: []string{"foo@example.com"}, + session: &sessions.SessionState{ + Email: "foo@ExAmPLE.CoM", + }, + expectedErr: nil, }, { - name: "single wildcard allows all", - domains: []string{"*"}, - email: "bar@gmail.com", - expectValid: true, + name: "single wildcard allows all", + AllowedEmails: []string{"*"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "wildcard in list allows all", - domains: []string{"foo@example.com", "*"}, - email: "foo@example.com", - expectValid: true, + name: "single wildcard allows all", + AllowedEmails: []string{"*"}, + session: &sessions.SessionState{ + Email: "bar@gmail.com", + }, + expectedErr: nil, }, { - name: "wildcard in list allows all", - domains: []string{"foo@example.com", "*"}, - email: "foo@gmail.com", - expectValid: true, + name: "wildcard is ignored if other domains included", + AllowedEmails: []string{"foo@example.com", "*"}, + session: &sessions.SessionState{ + Email: "foo@gmail.com", + }, + expectedErr: ErrEmailAddressDenied, }, { - name: "empty email rejected", - domains: []string{"foo@example.com"}, - email: "", - expectValid: false, + name: "empty email rejected", + AllowedEmails: []string{"foo@example.com"}, + email: "", + session: &sessions.SessionState{ + Email: "", + }, + expectedErr: ErrInvalidEmailAddress, }, { - name: "wildcard still rejects empty emails", - domains: []string{"*"}, - email: "", - expectValid: false, + name: "wildcard still rejects empty emails", + AllowedEmails: []string{"*"}, + email: "", + session: &sessions.SessionState{ + Email: "", + }, + expectedErr: ErrInvalidEmailAddress, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - emailValidator := NewEmailAddressValidator(tc.domains) - valid := emailValidator(tc.email) - if valid != tc.expectValid { - t.Fatalf("expected %v, got %v", tc.expectValid, valid) + emailValidator := NewEmailAddressValidator(tc.AllowedEmails) + err := emailValidator.Validate(tc.session) + if err != tc.expectedErr { + t.Fatalf("expected %v, got %v", tc.expectedErr, err) } }) } diff --git a/internal/pkg/options/email_domain_validator.go b/internal/pkg/options/email_domain_validator.go index f3991ce3..cab3205c 100644 --- a/internal/pkg/options/email_domain_validator.go +++ b/internal/pkg/options/email_domain_validator.go @@ -1,38 +1,75 @@ package options import ( + "errors" "fmt" "strings" + + "github.com/buzzfeed/sso/internal/pkg/sessions" +) + +var ( + _ Validator = &EmailDomainValidator{} + + // These error message should be formatted in such a way that is appropriate + // for display to the end user. + ErrEmailDomainDenied = errors.New("Unauthorized Email Domain") ) -// NewEmailDomainValidator returns a function that checks whether a given email is valid based on a list -// of domains. The domain "*" is a wild card that matches any non-empty email. -func NewEmailDomainValidator(domains []string) func(string) bool { - allowAll := false - var emailDomains []string +type EmailDomainValidator struct { + AllowedDomains []string +} + +// NewEmailDomainValidator takes in a list of domains and returns a Validator object. +// The validator can be used to validate that the session.Email: +// - 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 domains { + for _, domain := range allowedDomains { if domain == "*" { - allowAll = true + 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, + } +} + +func (v *EmailDomainValidator) Validate(session *sessions.SessionState) error { + if session.Email == "" { + return ErrInvalidEmailAddress } - if allowAll { - return func(email string) bool { return email != "" } + if len(v.AllowedDomains) == 0 { + return ErrEmailDomainDenied } - return func(email string) bool { - if email == "" { - return false - } - email = strings.ToLower(email) - for _, domain := range emailDomains { - if strings.HasSuffix(email, domain) { - return true - } + if len(v.AllowedDomains) == 1 && v.AllowedDomains[0] == "*" { + return nil + } + + err := v.validate(session) + if err != nil { + return err + } + return nil +} + +func (v *EmailDomainValidator) validate(session *sessions.SessionState) error { + email := strings.ToLower(session.Email) + for _, domain := range v.AllowedDomains { + if strings.HasSuffix(email, domain) { + return nil } - return false } + return ErrEmailDomainDenied } diff --git a/internal/pkg/options/email_domain_validator_test.go b/internal/pkg/options/email_domain_validator_test.go index 9bae8fad..e2eb407f 100644 --- a/internal/pkg/options/email_domain_validator_test.go +++ b/internal/pkg/options/email_domain_validator_test.go @@ -2,119 +2,147 @@ package options import ( "testing" + + "github.com/buzzfeed/sso/internal/pkg/sessions" ) func TestEmailDomainValidatorValidator(t *testing.T) { testCases := []struct { - name string - domains []string - email string - expectValid bool + name string + allowedDomains []string + email string + expectedErr error + session *sessions.SessionState }{ { - name: "nothing should validate when domain list is empty", - domains: []string(nil), - email: "foo@example.com", - expectValid: false, - }, - { - name: "single domain validation", - domains: []string{"example.com"}, - email: "foo@example.com", - expectValid: true, + name: "nothing should validate when domain list is empty", + allowedDomains: []string(nil), + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: ErrEmailDomainDenied, }, { - name: "substring matches are rejected", - domains: []string{"example.com"}, - email: "foo@hackerexample.com", - expectValid: false, + name: "single domain validation", + allowedDomains: []string{"example.com"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "no subdomain rollup happens", - domains: []string{"example.com"}, - email: "foo@bar.example.com", - expectValid: false, + name: "substring matches are rejected", + allowedDomains: []string{"example.com"}, + session: &sessions.SessionState{ + Email: "foo@hackerexample.com", + }, + expectedErr: ErrEmailDomainDenied, }, { - name: "multiple domain validation still rejects other domains", - domains: []string{"abc.com", "xyz.com"}, - email: "foo@example.com", - expectValid: false, + name: "no subdomain rollup happens", + allowedDomains: []string{"example.com"}, + session: &sessions.SessionState{ + Email: "foo@bar.example.com", + }, + expectedErr: ErrEmailDomainDenied, }, { - name: "multiple domain validation still accepts emails from either domain", - domains: []string{"abc.com", "xyz.com"}, - email: "foo@abc.com", - expectValid: true, + name: "multiple domain validation still rejects other domains", + allowedDomains: []string{"abc.com", "xyz.com"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: ErrEmailDomainDenied, }, { - name: "multiple domain validation still rejects other domains", - domains: []string{"abc.com", "xyz.com"}, - email: "bar@xyz.com", - expectValid: true, + name: "multiple domain validation still accepts emails from either domain", + allowedDomains: []string{"abc.com", "xyz.com"}, + session: &sessions.SessionState{ + Email: "foo@abc.com", + }, + expectedErr: nil, }, { - name: "comparisons are case insensitive", - domains: []string{"Example.Com"}, - email: "foo@example.com", - expectValid: true, + name: "multiple domain validation still rejects other domains", + allowedDomains: []string{"abc.com", "xyz.com"}, + session: &sessions.SessionState{ + Email: "bar@xyz.com", + }, + expectedErr: nil, }, { - name: "comparisons are case insensitive", - domains: []string{"Example.Com"}, - email: "foo@EXAMPLE.COM", - expectValid: true, + name: "comparisons are case insensitive", + allowedDomains: []string{"Example.Com"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "comparisons are case insensitive", - domains: []string{"example.com"}, - email: "foo@ExAmPlE.CoM", - expectValid: true, + name: "comparisons are case insensitive", + allowedDomains: []string{"Example.Com"}, + session: &sessions.SessionState{ + Email: "foo@EXAMPLE.COM", + }, + expectedErr: nil, }, { - name: "single wildcard allows all", - domains: []string{"*"}, - email: "foo@example.com", - expectValid: true, + name: "comparisons are case insensitive", + allowedDomains: []string{"example.com"}, + session: &sessions.SessionState{ + Email: "foo@ExAmPLE.CoM", + }, + expectedErr: nil, }, { - name: "single wildcard allows all", - domains: []string{"*"}, - email: "bar@gmail.com", - expectValid: true, + name: "single wildcard allows all", + allowedDomains: []string{"*"}, + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "wildcard in list allows all", - domains: []string{"example.com", "*"}, - email: "foo@example.com", - expectValid: true, + name: "single wildcard allows all", + allowedDomains: []string{"*"}, + session: &sessions.SessionState{ + Email: "bar@gmail.com", + }, + expectedErr: nil, }, { - name: "wildcard in list allows all", - domains: []string{"example.com", "*"}, - email: "foo@gmail.com", - expectValid: true, + name: "wildcard is ignored if other domains are included", + allowedDomains: []string{"*", "example.com"}, + session: &sessions.SessionState{ + Email: "foo@gmal.com", + }, + expectedErr: ErrEmailDomainDenied, }, { - name: "empty email rejected", - domains: []string{"example.com"}, - email: "", - expectValid: false, + name: "empty email rejected", + allowedDomains: []string{"example.com"}, + email: "", + session: &sessions.SessionState{ + Email: "foo@example.com", + }, + expectedErr: nil, }, { - name: "wildcard still rejects empty emails", - domains: []string{"*"}, - email: "", - expectValid: false, + name: "wildcard still rejects empty emails", + allowedDomains: []string{"*"}, + session: &sessions.SessionState{ + Email: "", + }, + expectedErr: ErrInvalidEmailAddress, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - emailValidator := NewEmailDomainValidator(tc.domains) - valid := emailValidator(tc.email) - if valid != tc.expectValid { - t.Fatalf("expected %v, got %v", tc.expectValid, valid) + emailValidator := NewEmailDomainValidator(tc.allowedDomains) + err := emailValidator.Validate(tc.session) + if err != tc.expectedErr { + t.Fatalf("expected %v, got %v", tc.expectedErr, err) } }) } diff --git a/internal/pkg/options/email_group_validator.go b/internal/pkg/options/email_group_validator.go new file mode 100644 index 00000000..36b1d988 --- /dev/null +++ b/internal/pkg/options/email_group_validator.go @@ -0,0 +1,60 @@ +package options + +import ( + "errors" + + "github.com/buzzfeed/sso/internal/pkg/sessions" + "github.com/buzzfeed/sso/internal/proxy/providers" +) + +var ( + _ Validator = EmailGroupValidator{} + + // These error message should be formatted in such a way that is appropriate + // for display to the end user. + ErrGroupMembership = errors.New("Invalid Group Membership") +) + +type EmailGroupValidator struct { + Provider providers.Provider + AllowedGroups []string +} + +// 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 { + return EmailGroupValidator{ + Provider: provider, + AllowedGroups: allowedGroups, + } +} + +func (v EmailGroupValidator) Validate(session *sessions.SessionState) error { + if len(v.AllowedGroups) == 0 { + return nil + } + + err := v.validate(session) + if err != nil { + return err + } + return nil +} + +func (v EmailGroupValidator) validate(session *sessions.SessionState) error { + matchedGroups, valid, err := v.Provider.ValidateGroup(session.Email, v.AllowedGroups, session.AccessToken) + if err != nil { + return ErrValidationError + } + + if valid { + session.Groups = matchedGroups + return nil + } + + return ErrGroupMembership +} diff --git a/internal/pkg/options/mock_validator.go b/internal/pkg/options/mock_validator.go new file mode 100644 index 00000000..87739be0 --- /dev/null +++ b/internal/pkg/options/mock_validator.go @@ -0,0 +1,29 @@ +package options + +import ( + "errors" + + "github.com/buzzfeed/sso/internal/pkg/sessions" +) + +var ( + _ Validator = EmailAddressValidator{} +) + +type MockValidator struct { + Result bool +} + +func NewMockValidator(result bool) MockValidator { + return MockValidator{ + Result: result, + } +} + +func (v MockValidator) Validate(session *sessions.SessionState) error { + if v.Result { + return nil + } + + return errors.New("MockValidator error") +} diff --git a/internal/pkg/options/validators.go b/internal/pkg/options/validators.go new file mode 100644 index 00000000..0b14e48f --- /dev/null +++ b/internal/pkg/options/validators.go @@ -0,0 +1,32 @@ +package options + +import ( + "errors" + + "github.com/buzzfeed/sso/internal/pkg/sessions" +) + +var ( + // These error message should be formatted in such a way that is appropriate + // for display to the end user. + ErrInvalidEmailAddress = errors.New("Invalid Email Address In Session State") + ErrValidationError = errors.New("Error during validation") +) + +type Validator interface { + Validate(*sessions.SessionState) error +} + +// RunValidators runs each passed in validator and returns a slice of errors. If an +// empty slice is returned, it can be assumed all passed in validators were successful. +func RunValidators(validators []Validator, session *sessions.SessionState) []error { + validatorErrors := make([]error, 0, len(validators)) + + for _, validator := range validators { + err := validator.Validate(session) + if err != nil { + validatorErrors = append(validatorErrors, err) + } + } + return validatorErrors +} diff --git a/internal/proxy/oauthproxy.go b/internal/proxy/oauthproxy.go index f3f53781..7fc935f3 100644 --- a/internal/proxy/oauthproxy.go +++ b/internal/proxy/oauthproxy.go @@ -13,6 +13,7 @@ import ( "github.com/buzzfeed/sso/internal/pkg/aead" log "github.com/buzzfeed/sso/internal/pkg/logging" + "github.com/buzzfeed/sso/internal/pkg/options" "github.com/buzzfeed/sso/internal/pkg/sessions" "github.com/buzzfeed/sso/internal/proxy/providers" @@ -53,15 +54,12 @@ func (e *ErrOAuthProxyMisconfigured) Error() string { const statusInvalidHost = 421 -// EmailValidatorFn function type for validating email addresses. -type EmailValidatorFn func(string) bool - // OAuthProxy stores all the information associated with proxying the request. type OAuthProxy struct { - cookieSecure bool - emailValidator EmailValidatorFn - redirectURL *url.URL // the url to receive requests at - templates *template.Template + cookieSecure bool + Validators []options.Validator + redirectURL *url.URL // the url to receive requests at + templates *template.Template skipAuthPreflight bool passAccessToken bool @@ -152,9 +150,9 @@ func SetProxyHandler(handler http.Handler) func(*OAuthProxy) error { } // SetValidator sets the email validator as a functional option -func SetValidator(validator func(string) bool) func(*OAuthProxy) error { +func SetValidators(validators []options.Validator) func(*OAuthProxy) error { return func(op *OAuthProxy) error { - op.emailValidator = validator + op.Validators = validators return nil } } @@ -176,9 +174,9 @@ type StateParameter struct { // NewOAuthProxy creates a new OAuthProxy struct. func NewOAuthProxy(opts *Options, optFuncs ...func(*OAuthProxy) error) (*OAuthProxy, error) { p := &OAuthProxy{ - cookieSecure: opts.CookieSecure, - StatsdClient: opts.StatsdClient, - emailValidator: func(_ string) bool { return true }, + cookieSecure: opts.CookieSecure, + StatsdClient: opts.StatsdClient, + Validators: []options.Validator{}, redirectURL: &url.URL{Path: "/oauth2/callback"}, templates: getTemplates(), @@ -567,40 +565,29 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { } // We validate the user information, and check that this user has proper authorization - // for the resources requested. This can be set via the email address or any groups. + // for the resources requested. // // set cookie, or deny - if !p.emailValidator(session.Email) { - tags = append(tags, "error:invalid_email") + + errors := options.RunValidators(p.Validators, session) + if len(errors) == len(p.Validators) { + tags = append(tags, "error:validation_failed") p.StatsdClient.Incr("application_error", tags, 1.0) logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( - "permission denied: unauthorized") - p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", "Invalid Account") - return - } - - allowedGroups := p.upstreamConfig.AllowedGroups + fmt.Sprintf("permission denied: unauthorized: %q", errors)) - inGroups, validGroup, err := p.provider.ValidateGroup(session.Email, allowedGroups, session.AccessToken) - if err != nil { - tags = append(tags, "error:user_group_failed") - p.StatsdClient.Incr("provider_error", tags, 1.0) - logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( - "couldn't fetch user groups") - p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error", "Error validating group membership, please try again") - return - } - if !validGroup { - tags = append(tags, "error:unauthorized_email") - p.StatsdClient.Incr("provider_error", tags, 1.0) - logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).WithAllowedGroups( - allowedGroups).Info("permission denied: unauthorized") - p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", "Group membership required") + formattedErrors := make([]string, 0, len(errors)) + for _, err := range errors { + formattedErrors = append(formattedErrors, err.Error()) + } + errorMsg := fmt.Sprintf("We ran into some issues while validating your account: \"%s\"", + strings.Join(formattedErrors, ", ")) + p.ErrorPage(rw, req, http.StatusForbidden, "Permission Denied", errorMsg) return } - logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).WithInGroups(inGroups).Info( - "authentication complete") - session.Groups = inGroups + + logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).WithInGroups(session.Groups).Info( + fmt.Sprintf("oauth callback: user validated ")) // We store the session in a cookie and redirect the user back to the application err = p.sessionStore.SaveSession(rw, req, session) @@ -703,6 +690,9 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) { func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (err error) { logger := log.NewLogEntry().WithRemoteAddress(getRemoteAddr(req)) + remoteAddr := getRemoteAddr(req) + tags := []string{"action:authenticate"} + allowedGroups := p.upstreamConfig.AllowedGroups // Clear the session cookie if anything goes wrong. @@ -791,11 +781,18 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er } } - if !p.emailValidator(session.Email) { - logger.WithUser(session.Email).Error("not authorized") + errors := options.RunValidators(p.Validators, session) + if len(errors) == len(p.Validators) { + tags = append(tags, "error:validation_failed") + p.StatsdClient.Incr("application_error", tags, 1.0) + logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( + fmt.Sprintf("permission denied: unauthorized: %q", errors)) return ErrUserNotAuthorized } + logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info( + fmt.Sprintf("authentication: user validated")) + for key, val := range p.upstreamConfig.InjectRequestHeaders { req.Header.Set(key, val) } diff --git a/internal/proxy/oauthproxy_test.go b/internal/proxy/oauthproxy_test.go index b565d09d..d21aced8 100644 --- a/internal/proxy/oauthproxy_test.go +++ b/internal/proxy/oauthproxy_test.go @@ -20,6 +20,7 @@ import ( "github.com/mccutchen/go-httpbin/httpbin" "github.com/buzzfeed/sso/internal/pkg/aead" + "github.com/buzzfeed/sso/internal/pkg/options" "github.com/buzzfeed/sso/internal/pkg/sessions" "github.com/buzzfeed/sso/internal/pkg/testutil" "github.com/buzzfeed/sso/internal/proxy/providers" @@ -30,22 +31,6 @@ func init() { } -func testValidatorFunc(valid bool) func(*OAuthProxy) error { - return func(p *OAuthProxy) error { - p.emailValidator = func(string) bool { - return valid - } - return nil - } -} - -func setValidator(f func(string) bool) func(*OAuthProxy) error { - return func(p *OAuthProxy) error { - p.emailValidator = f - return nil - } -} - func setCSRFStore(s sessions.CSRFStore) func(*OAuthProxy) error { return func(p *OAuthProxy) error { p.csrfStore = s @@ -139,7 +124,7 @@ func testNewOAuthProxy(t *testing.T, optFuncs ...func(*OAuthProxy) error) (*OAut } standardOptFuncs := []func(*OAuthProxy) error{ - testValidatorFunc(true), + SetValidators([]options.Validator{options.NewMockValidator(true)}), SetProvider(provider), setSessionStore(&sessions.MockSessionStore{Session: testSession()}), SetUpstreamConfig(upstreamConfig), @@ -270,7 +255,7 @@ func TestAuthOnlyEndpoint(t *testing.T) { proxy, close := testNewOAuthProxy(t, setSessionStore(tc.sessionStore), - setValidator(func(_ string) bool { return tc.validEmail }), + SetValidators([]options.Validator{options.NewMockValidator(tc.validEmail)}), SetProvider(tp), ) defer close() diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index 9865f221..7aa2ee49 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -17,6 +17,7 @@ func New(opts *Options) (*SSOProxy, error) { var requestSigner *RequestSigner var err error + if opts.RequestSigningKey != "" { requestSigner, err = NewRequestSigner(opts.RequestSigningKey) if err != nil { @@ -37,17 +38,23 @@ func New(opts *Options) (*SSOProxy, error) { return nil, err } + validators := []options.Validator{} if len(upstreamConfig.AllowedEmailAddresses) != 0 { - optFuncs = append(optFuncs, SetValidator(options.NewEmailAddressValidator(upstreamConfig.AllowedEmailAddresses))) - } else if len(upstreamConfig.AllowedEmailDomains) != 0 { - optFuncs = append(optFuncs, SetValidator(options.NewEmailDomainValidator(upstreamConfig.AllowedEmailDomains))) + validators = append(validators, options.NewEmailAddressValidator(upstreamConfig.AllowedEmailAddresses)) + } + + if len(upstreamConfig.AllowedEmailDomains) != 0 { + validators = append(validators, options.NewEmailDomainValidator(upstreamConfig.AllowedEmailDomains)) } + validators = append(validators, options.NewEmailGroupValidator(provider, upstreamConfig.AllowedGroups)) + optFuncs = append(optFuncs, SetProvider(provider), SetCookieStore(opts), SetUpstreamConfig(upstreamConfig), SetProxyHandler(handler), + SetValidators(validators), ) oauthproxy, err := NewOAuthProxy(opts, optFuncs...)