diff --git a/internal/auth/authenticator.go b/internal/auth/authenticator.go index dc2c4d37..6a0f06a3 100644 --- a/internal/auth/authenticator.go +++ b/internal/auth/authenticator.go @@ -20,8 +20,6 @@ import ( // Authenticator stores all the information associated with proxying the request. type Authenticator struct { - Validator func(string) bool - EmailDomains []string ProxyRootDomains []string Host string Scheme string @@ -80,7 +78,6 @@ func NewAuthenticator(config Configuration, optionFuncs ...func(*Authenticator) p := &Authenticator{ ProxyClientID: config.ClientConfigs["proxy"].ID, ProxyClientSecret: config.ClientConfigs["proxy"].Secret, - EmailDomains: config.AuthorizeConfig.EmailConfig.Domains, Host: config.ServerConfig.Host, Scheme: config.ServerConfig.Scheme, @@ -152,10 +149,16 @@ func (p *Authenticator) SignInPage(rw http.ResponseWriter, req *http.Request, co // validateRedirectURI middleware already ensures that this is a valid URL destinationURL, _ := url.Parse(redirectURL.Query().Get("redirect_uri")) + emailDomainsFormValue := req.FormValue("email_domains") + emailDomains := []string{} + if emailDomainsFormValue != "" { + emailDomains = strings.Split(emailDomainsFormValue, ",") + } + t := signInResp{ ProviderName: p.provider.Data().ProviderName, ProviderSlug: p.provider.Data().ProviderSlug, - EmailDomains: p.EmailDomains, + EmailDomains: emailDomains, Redirect: redirectURL.String(), Destination: destinationURL.Host, Version: VERSION, @@ -225,11 +228,6 @@ func (p *Authenticator) authenticate(rw http.ResponseWriter, req *http.Request) } } - if !p.Validator(session.Email) { - logger.WithUser(session.Email).Error("invalid email user") - return nil, ErrUserNotAuthorized - } - return session, nil } @@ -569,17 +567,6 @@ func (p *Authenticator) getOAuthCallback(rw http.ResponseWriter, req *http.Reque return "", HTTPError{Code: http.StatusForbidden, Message: "Invalid Redirect URI"} } - // 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) { - 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("authentication complete") err = p.sessionStore.SaveSession(rw, req, session) if err != nil { diff --git a/internal/auth/authenticator_test.go b/internal/auth/authenticator_test.go index 52755c05..bf3d032f 100644 --- a/internal/auth/authenticator_test.go +++ b/internal/auth/authenticator_test.go @@ -66,13 +66,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 @@ -135,7 +128,6 @@ func TestSignIn(t *testing.T) { mockAuthCodeCipher *aead.MockCipher refreshResponse providerRefreshResponse providerValidToken bool - validEmail bool expectedSignInPage bool expectedDestinationURL string expectedCode int @@ -143,6 +135,9 @@ func TestSignIn(t *testing.T) { }{ { name: "err no cookie calls proxy oauth redirect, no params map redirects to sign in page", + paramsMap: map[string]string{ + "email_domains": "testone.com,testtwo.com", + }, mockSessionStore: &sessions.MockSessionStore{ LoadError: http.ErrNoCookie, }, @@ -156,7 +151,8 @@ func TestSignIn(t *testing.T) { LoadError: http.ErrNoCookie, }, paramsMap: map[string]string{ - "redirect_uri": "http://foo.example.com", + "email_domains": "testone.com,testtwo.com", + "redirect_uri": "http://foo.example.com", }, expectedSignInPage: true, expectedDestinationURL: "foo.example.com", @@ -175,6 +171,9 @@ func TestSignIn(t *testing.T) { }, { name: "expired lifetime of session clears session and redirects to sign in", + paramsMap: map[string]string{ + "email_domains": "testone.com,testtwo.com", + }, mockSessionStore: &sessions.MockSessionStore{ Session: &sessions.SessionState{ Email: "email", @@ -238,23 +237,6 @@ func TestSignIn(t *testing.T) { expectedCode: http.StatusInternalServerError, expectedErrorResponse: &errResponse{"save error"}, }, - { - name: "refresh period expired, successful refresh, invalid email", - mockSessionStore: &sessions.MockSessionStore{ - Session: &sessions.SessionState{ - Email: "email", - AccessToken: "accesstoken", - RefreshToken: "refresh", - LifetimeDeadline: time.Now().Add(time.Hour), - RefreshDeadline: time.Now().Add(-time.Hour), - }, - }, - refreshResponse: providerRefreshResponse{ - OK: true, - }, - expectedCode: http.StatusUnauthorized, - expectedErrorResponse: &errResponse{ErrUserNotAuthorized.Error()}, - }, { name: "valid session state, save session error", mockSessionStore: &sessions.MockSessionStore{ @@ -272,7 +254,7 @@ func TestSignIn(t *testing.T) { expectedErrorResponse: &errResponse{"save error"}, }, { - name: "invalid session state, invalid email", + name: "invalid session state", mockSessionStore: &sessions.MockSessionStore{ Session: &sessions.SessionState{ Email: "email", @@ -299,7 +281,6 @@ func TestSignIn(t *testing.T) { refreshResponse: providerRefreshResponse{ OK: true, }, - validEmail: true, expectedCode: http.StatusForbidden, expectedErrorResponse: &errResponse{"no state parameter supplied"}, }, @@ -320,7 +301,6 @@ func TestSignIn(t *testing.T) { refreshResponse: providerRefreshResponse{ OK: true, }, - validEmail: true, expectedCode: http.StatusForbidden, expectedErrorResponse: &errResponse{"no redirect_uri parameter supplied"}, }, @@ -342,7 +322,6 @@ func TestSignIn(t *testing.T) { refreshResponse: providerRefreshResponse{ OK: true, }, - validEmail: true, expectedCode: http.StatusBadRequest, expectedErrorResponse: &errResponse{"malformed redirect_uri parameter passed"}, }, @@ -367,7 +346,6 @@ func TestSignIn(t *testing.T) { mockAuthCodeCipher: &aead.MockCipher{ MarshalError: fmt.Errorf("error marshal"), }, - validEmail: true, expectedCode: http.StatusInternalServerError, expectedErrorResponse: &errResponse{"error marshal"}, }, @@ -392,7 +370,6 @@ func TestSignIn(t *testing.T) { mockAuthCodeCipher: &aead.MockCipher{ MarshalString: "abcdefg", }, - validEmail: true, expectedCode: http.StatusFound, }, { @@ -410,7 +387,6 @@ func TestSignIn(t *testing.T) { "state": "state", "redirect_uri": "http://foo.example.com", }, - validEmail: true, providerValidToken: true, mockAuthCodeCipher: &aead.MockCipher{ MarshalString: "abcdefg", @@ -424,7 +400,6 @@ func TestSignIn(t *testing.T) { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) auth, err := NewAuthenticator(config, - setMockValidator(tc.validEmail), setMockSessionStore(tc.mockSessionStore), setMockTempl(), setMockRedirectURL(), @@ -459,7 +434,7 @@ func TestSignIn(t *testing.T) { expectedSignInResp := &signInResp{ ProviderName: provider.Data().ProviderName, ProviderSlug: "test", - EmailDomains: auth.EmailDomains, + EmailDomains: []string{"testone.com", "testtwo.com"}, Redirect: u.String(), Destination: tc.expectedDestinationURL, Version: VERSION, @@ -571,7 +546,6 @@ func TestSignOutPage(t *testing.T) { provider.RevokeError = tc.RevokeError p, _ := NewAuthenticator(config, - setMockValidator(true), setMockSessionStore(tc.mockSessionStore), setMockTempl(), setTestProvider(provider), @@ -947,9 +921,7 @@ func TestGetProfile(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) - p, _ := NewAuthenticator(config, - setMockValidator(true), - ) + p, _ := NewAuthenticator(config) u, _ := url.Parse("http://example.com") testProvider := providers.NewTestProvider(u) testProvider.Groups = tc.groupEmails @@ -1049,9 +1021,7 @@ func TestRedeemCode(t *testing.T) { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) - proxy, _ := NewAuthenticator(config, - setMockValidator(true), - ) + proxy, _ := NewAuthenticator(config) testURL, err := url.Parse("example.com") if err != nil { @@ -1220,7 +1190,6 @@ func TestOAuthCallback(t *testing.T) { paramsMap map[string]string expectedError error testRedeemResponse testRedeemResponse - validEmail bool csrfResp *sessions.MockCSRFStore sessionStore *sessions.MockSessionStore expectedRedirect string @@ -1338,29 +1307,7 @@ func TestOAuthCallback(t *testing.T) { }, { - name: "invalid email address", - paramsMap: map[string]string{ - "code": "authCode", - "state": base64.URLEncoding.EncodeToString([]byte("state:http://www.example.com/something")), - }, - testRedeemResponse: testRedeemResponse{ - SessionState: &sessions.SessionState{ - Email: "example@email.com", - AccessToken: "accessToken", - RefreshDeadline: time.Now().Add(time.Hour), - RefreshToken: "refresh", - }, - }, - csrfResp: &sessions.MockCSRFStore{ - Cookie: &http.Cookie{ - Name: "something_csrf", - Value: "state", - }, - }, - expectedError: HTTPError{Code: http.StatusForbidden, Message: "Invalid Account"}, - }, - { - name: "valid email, invalid redirect", + name: "invalid redirect", paramsMap: map[string]string{ "code": "authCode", "state": base64.URLEncoding.EncodeToString([]byte("state:something")), @@ -1379,11 +1326,10 @@ func TestOAuthCallback(t *testing.T) { Value: "state", }, }, - validEmail: true, expectedError: HTTPError{Code: http.StatusForbidden, Message: "Invalid Redirect URI"}, }, { - name: "valid email, valid redirect, save error", + name: "valid redirect, save error", paramsMap: map[string]string{ "code": "authCode", "state": base64.URLEncoding.EncodeToString([]byte("state:http://www.example.com/something")), @@ -1405,11 +1351,10 @@ func TestOAuthCallback(t *testing.T) { sessionStore: &sessions.MockSessionStore{ SaveError: fmt.Errorf("saveError"), }, - validEmail: true, expectedError: HTTPError{Code: http.StatusInternalServerError, Message: "Internal Error"}, }, { - name: "valid email, valid redirect, valid save", + name: "valid redirect, valid save", paramsMap: map[string]string{ "code": "authCode", "state": base64.URLEncoding.EncodeToString([]byte("state:http://www.example.com/something")), @@ -1429,7 +1374,6 @@ func TestOAuthCallback(t *testing.T) { }, }, sessionStore: &sessions.MockSessionStore{}, - validEmail: true, expectedRedirect: "http://www.example.com/something", }, } @@ -1438,7 +1382,6 @@ func TestOAuthCallback(t *testing.T) { t.Run(tc.name, func(t *testing.T) { config := testConfiguration(t) proxy, _ := NewAuthenticator(config, - setMockValidator(tc.validEmail), setMockCSRFStore(tc.csrfResp), setMockSessionStore(tc.sessionStore), ) @@ -1559,7 +1502,6 @@ func TestOAuthStart(t *testing.T) { provider := providers.NewTestProvider(nil) proxy, _ := NewAuthenticator(config, setTestProvider(provider), - setMockValidator(true), setMockRedirectURL(), setMockCSRFStore(&sessions.MockCSRFStore{}), ) diff --git a/internal/auth/configuration.go b/internal/auth/configuration.go index bd111917..7630444b 100644 --- a/internal/auth/configuration.go +++ b/internal/auth/configuration.go @@ -54,8 +54,6 @@ import ( // SERVER_TIMEOUT_READ // // AUTHORIZE_PROXY_DOMAINS -// AUTHORIZE_EMAIL_DOMAINS -// AUTHORIZE_EMAIL_ADDRESSES // // METRICS_STATSD_PORT // METRICS_STATSD_HOST @@ -105,10 +103,6 @@ func DefaultAuthConfig() Configuration { }, // we provide no defaults for these right now AuthorizeConfig: AuthorizeConfig{ - EmailConfig: EmailConfig{ - Domains: []string{}, - Addresses: []string{}, - }, ProxyConfig: ProxyConfig{ Domains: []string{}, }, @@ -126,7 +120,6 @@ var ( _ Validator = ProviderConfig{} _ Validator = ClientConfig{} _ Validator = AuthorizeConfig{} - _ Validator = EmailConfig{} _ Validator = ProxyConfig{} _ Validator = ServerConfig{} _ Validator = MetricsConfig{} @@ -411,15 +404,10 @@ func (cc ClientConfig) Validate() error { } type AuthorizeConfig struct { - EmailConfig EmailConfig `mapstructure:"email"` ProxyConfig ProxyConfig `mapstructure:"proxy"` } func (ac AuthorizeConfig) Validate() error { - if err := ac.EmailConfig.Validate(); err != nil { - return xerrors.Errorf("invalid authorize.email config: %w", err) - } - if err := ac.ProxyConfig.Validate(); err != nil { return xerrors.Errorf("invalid authorize.proxy config: %w", err) } @@ -427,23 +415,6 @@ func (ac AuthorizeConfig) Validate() error { return nil } -type EmailConfig struct { - Domains []string `mapstructure:"domains"` - Addresses []string `mapstructure:"addresses"` -} - -func (ec EmailConfig) Validate() error { - if len(ec.Domains) > 0 && len(ec.Addresses) > 0 { - return xerrors.New("can not specify both email.domains and email.addesses") - } - - if len(ec.Domains) == 0 && len(ec.Addresses) == 0 { - return xerrors.New("must specify either email.domains or email.addresses") - } - - return nil -} - type ProxyConfig struct { Domains []string `mapstructure:"domains"` } diff --git a/internal/auth/configuration_test.go b/internal/auth/configuration_test.go index 0ce8a95b..91e6df6e 100644 --- a/internal/auth/configuration_test.go +++ b/internal/auth/configuration_test.go @@ -61,9 +61,6 @@ func testConfiguration(t *testing.T) Configuration { ProxyConfig: ProxyConfig{ Domains: []string{"proxy.local", "root.local", "example.com"}, }, - EmailConfig: EmailConfig{ - Domains: []string{"proxy.local"}, - }, }, } err := c.Validate() @@ -163,15 +160,6 @@ func TestEnvironmentOverridesConfiguration(t *testing.T) { assertEq("baz-client-id", baz.ClientConfig.ID, t) }, }, - { - Name: "Test ENV CSV Lists", - EnvOverrides: map[string]string{ - "AUTHORIZE_EMAIL_DOMAINS": "proxy.local,root.local", - }, - CheckFunc: func(c Configuration, t *testing.T) { - assertEq([]string{"proxy.local", "root.local"}, c.AuthorizeConfig.EmailConfig.Domains, t) - }, - }, } for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { @@ -247,9 +235,6 @@ func TestConfigValidate(t *testing.T) { ProxyConfig: ProxyConfig{ Domains: []string{"proxy.local", "root.local"}, }, - EmailConfig: EmailConfig{ - Domains: []string{"proxy.local"}, - }, }, }, ExpectedErr: nil, diff --git a/internal/auth/mux.go b/internal/auth/mux.go index 270315a7..fece9d8d 100644 --- a/internal/auth/mux.go +++ b/internal/auth/mux.go @@ -6,7 +6,6 @@ import ( "github.com/buzzfeed/sso/internal/pkg/hostmux" log "github.com/buzzfeed/sso/internal/pkg/logging" - "github.com/buzzfeed/sso/internal/pkg/options" "github.com/datadog/datadog-go/statsd" ) @@ -19,13 +18,6 @@ type AuthenticatorMux struct { func NewAuthenticatorMux(config Configuration, statsdClient *statsd.Client) (*AuthenticatorMux, error) { logger := log.NewLogEntry() - var validator func(string) bool - if len(config.AuthorizeConfig.EmailConfig.Addresses) != 0 { - validator = options.NewEmailAddressValidator(config.AuthorizeConfig.EmailConfig.Addresses) - } else { - validator = options.NewEmailDomainValidator(config.AuthorizeConfig.EmailConfig.Domains) - } - authenticators := []*Authenticator{} idpMux := http.NewServeMux() @@ -38,7 +30,6 @@ func NewAuthenticatorMux(config Configuration, statsdClient *statsd.Client) (*Au idpSlug := idp.Data().ProviderSlug authenticator, err := NewAuthenticator(config, - SetValidator(validator), SetProvider(idp), SetCookieStore(config.SessionConfig, idpSlug), SetStatsdClient(statsdClient), diff --git a/internal/auth/options.go b/internal/auth/options.go index 5e8d41b8..9d2708bf 100644 --- a/internal/auth/options.go +++ b/internal/auth/options.go @@ -95,14 +95,6 @@ func SetRedirectURL(serverConfig ServerConfig, slug string) func(*Authenticator) } } -// SetValidator sets the email validator -func SetValidator(validator func(string) bool) func(*Authenticator) error { - return func(a *Authenticator) error { - a.Validator = validator - return nil - } -} - // SetCookieStore sets the cookie store to use a miscreant cipher func SetCookieStore(sessionConfig SessionConfig, providerSlug string) func(*Authenticator) error { return func(a *Authenticator) error { diff --git a/internal/proxy/oauthproxy.go b/internal/proxy/oauthproxy.go index 9b6e8038..1dc51856 100644 --- a/internal/proxy/oauthproxy.go +++ b/internal/proxy/oauthproxy.go @@ -472,8 +472,8 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request, tags p.ErrorPage(rw, req, http.StatusInternalServerError, "Internal Error", err.Error()) return } - - signinURL := p.provider.GetSignInURL(callbackURL, encryptedState) + allowedDomains := p.upstreamConfig.AllowedEmailDomains + signinURL := p.provider.GetSignInURL(callbackURL, encryptedState, allowedDomains) logger.WithSignInURL(signinURL).Info("starting OAuth flow") http.Redirect(rw, req, signinURL.String(), http.StatusFound) } diff --git a/internal/proxy/providers/providers.go b/internal/proxy/providers/providers.go index 226dc16f..6b01a080 100644 --- a/internal/proxy/providers/providers.go +++ b/internal/proxy/providers/providers.go @@ -14,7 +14,7 @@ type Provider interface { ValidateGroup(string, []string, string) ([]string, bool, error) UserGroups(string, []string, string) ([]string, error) ValidateSessionState(*sessions.SessionState, []string) bool - GetSignInURL(redirectURL *url.URL, finalRedirect string) *url.URL + GetSignInURL(redirectURL *url.URL, finalRedirect string, allowedDomains []string) *url.URL GetSignOutURL(redirectURL *url.URL) *url.URL RefreshSession(*sessions.SessionState, []string) (bool, error) } diff --git a/internal/proxy/providers/singleflight_middleware.go b/internal/proxy/providers/singleflight_middleware.go index 40e6fcf7..afc871de 100644 --- a/internal/proxy/providers/singleflight_middleware.go +++ b/internal/proxy/providers/singleflight_middleware.go @@ -131,8 +131,8 @@ func (p *SingleFlightProvider) RefreshSession(s *sessions.SessionState, allowedG } // GetSignInURL calls the GetSignInURL for the provider, which will return the sign in url -func (p *SingleFlightProvider) GetSignInURL(redirectURI *url.URL, finalRedirect string) *url.URL { - return p.provider.GetSignInURL(redirectURI, finalRedirect) +func (p *SingleFlightProvider) GetSignInURL(redirectURI *url.URL, finalRedirect string, allowedDomains []string) *url.URL { + return p.provider.GetSignInURL(redirectURI, finalRedirect, allowedDomains) } // GetSignOutURL calls the GetSignOutURL for the provider, which will return the sign out url diff --git a/internal/proxy/providers/sso.go b/internal/proxy/providers/sso.go index ad2e3be5..536c4e11 100644 --- a/internal/proxy/providers/sso.go +++ b/internal/proxy/providers/sso.go @@ -409,7 +409,7 @@ func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGrou } // GetSignInURL with typical oauth parameters -func (p *SSOProvider) GetSignInURL(redirectURL *url.URL, state string) *url.URL { +func (p *SSOProvider) GetSignInURL(redirectURL *url.URL, state string, allowedDomains []string) *url.URL { a := *p.Data().SignInURL now := time.Now() rawRedirect := redirectURL.String() @@ -421,6 +421,8 @@ func (p *SSOProvider) GetSignInURL(redirectURL *url.URL, state string) *url.URL params.Add("state", state) params.Set("ts", fmt.Sprint(now.Unix())) params.Set("sig", p.signRedirectURL(rawRedirect, now)) + params.Set("email_domains", strings.Join(allowedDomains, ",")) + a.RawQuery = params.Encode() return &a } diff --git a/internal/proxy/providers/test_provider.go b/internal/proxy/providers/test_provider.go index 35494786..64f1c401 100644 --- a/internal/proxy/providers/test_provider.go +++ b/internal/proxy/providers/test_provider.go @@ -77,7 +77,7 @@ func (tp *TestProvider) GetSignOutURL(redirectURL *url.URL) *url.URL { } // GetSignInURL mocks GetSignInURL -func (tp *TestProvider) GetSignInURL(redirectURL *url.URL, state string) *url.URL { +func (tp *TestProvider) GetSignInURL(redirectURL *url.URL, state string, allowedDomains []string) *url.URL { a := *tp.Data().SignInURL params, _ := url.ParseQuery(a.RawQuery) params.Add("state", state)