sso_*:clean up some validation calls

internal/pkg/options/email_group_validator.go

@@ -42,7 +42,7 @@ func (v EmailGroupValidator) Validate(session *sessions.SessionState) error {
 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
+		return err
 	}
 
 	if valid {

internal/pkg/options/validators.go

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

internal/pkg/sessions/session_state.go

@@ -45,6 +45,14 @@ func (s *SessionState) ValidationPeriodExpired() bool {
 	return isExpired(s.ValidDeadline)
 }
 
+// IsWithinGracePeriod returns true if the session is still within the grace period
+func (s *SessionState) IsWithinGracePeriod(gracePeriodTTL time.Duration) bool {
+	if s.GracePeriodStart.IsZero() {
+		s.GracePeriodStart = time.Now()
+	}
+	return s.GracePeriodStart.Add(gracePeriodTTL).After(time.Now())
+}
+
 func isExpired(t time.Time) bool {
 	if t.Before(time.Now()) {
 		return true

internal/proxy/oauthproxy.go

@@ -359,7 +359,7 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, req *http.Request, code i
 	p.templates.ExecuteTemplate(rw, "error.html", t)
 }
 
-// IsWhitelistedRequest cheks that proxy host exists and checks the SkipAuthRegex
+// IsWhitelistedRequest checks that proxy host exists and checks the SkipAuthRegex
 func (p *OAuthProxy) IsWhitelistedRequest(req *http.Request) bool {
 	if p.skipAuthPreflight && req.Method == "OPTIONS" {
 		return true
@@ -375,6 +375,28 @@ func (p *OAuthProxy) IsWhitelistedRequest(req *http.Request) bool {
 	return false
 }
 
+// runValidatorsWithGracePeriod runs all validators and upon finding errors, checks to see if the
+// auth provider is explicity denying authentication or if it's merely unavailable. If it's unavailable,
+// we check whether the session is within the grace period or not to determine whether to allow the request
+// at this point.
+func (p *OAuthProxy) runValidatorsWithGracePeriod(session *sessions.SessionState) (err error) {
+	logger := log.NewLogEntry()
+	errors := options.RunValidators(p.Validators, session)
+	if len(errors) == len(p.Validators) {
+		for _, err := range errors {
+			// Check to see if the auth provider is explicity denying authentication, or if it is merely unavailable.
+			if err == providers.ErrAuthProviderUnavailable && session.IsWithinGracePeriod(p.provider.Data().GracePeriodTTL) {
+				return err
+			}
+		}
+		allowedGroups := p.upstreamConfig.AllowedGroups
+		logger.WithUser(session.Email).WithAllowedGroups(allowedGroups).Info(
+			"no longer authorized after validation period: %q", errors)
+		return ErrUserNotAuthorized
+	}
+	return nil
+}
+
 func (p *OAuthProxy) isXHR(req *http.Request) bool {
 	return req.Header.Get("X-Requested-With") == "XMLHttpRequest"
 }
@@ -693,8 +715,6 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 	remoteAddr := getRemoteAddr(req)
 	tags := []string{"action:authenticate"}
 
-	allowedGroups := p.upstreamConfig.AllowedGroups
-
 	// Clear the session cookie if anything goes wrong.
 	defer func() {
 		if err != nil {
@@ -705,7 +725,7 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 	session, err := p.sessionStore.LoadSession(req)
 	if err != nil {
 		// We loaded a cookie but it wasn't valid, clear it, and reject the request
-		logger.Error(err, "error authenticating user")
+		logger.Error(err, "invalid session loaded")
 		return err
 	}
 
@@ -728,7 +748,23 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 	} else if session.RefreshPeriodExpired() {
 		// Refresh period is the period in which the access token is valid. This is ultimately
 		// controlled by the upstream provider and tends to be around 1 hour.
-		ok, err := p.provider.RefreshSession(session, allowedGroups)
+		// If it has expired we:
+		// - run email domain, email address, and email group validations against the session (if defined).
+		// - attempt to refresh the session
+
+		err := p.runValidatorsWithGracePeriod(session)
+		if err != nil {
+			switch err {
+			case providers.ErrAuthProviderUnavailable:
+				tags = append(tags, "action:refresh_session", "error:validation_failed")
+				p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
+				session.RefreshDeadline = sessions.ExtendDeadline(p.provider.Data().SessionValidTTL)
+			default:
+				return ErrUserNotAuthorized
+			}
+		}
+
+		ok, err := p.provider.RefreshSession(session)
 		// We failed to refresh the session successfully
 		// clear the cookie and reject the request
 		if err != nil {
@@ -757,9 +793,23 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 	} else if session.ValidationPeriodExpired() {
 		// Validation period has expired, this is the shortest interval we use to
 		// check for valid requests. This should be set to something like a minute.
-		// This calls up the provider chain to validate this user is still active
-		// and hasn't been de-authorized.
-		ok := p.provider.ValidateSessionState(session, allowedGroups)
+		// In this case we:
+		// - run any defined email domain, email address, and email group validators against the session
+		// - call up the provider chain to validate this user is still active and hasn't been de-authorized.
+
+		err := p.runValidatorsWithGracePeriod(session)
+		if err != nil {
+			switch err {
+			case providers.ErrAuthProviderUnavailable:
+				tags = append(tags, "action:refresh_session", "error:validation_failed")
+				p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
+				session.RefreshDeadline = sessions.ExtendDeadline(p.provider.Data().SessionValidTTL)
+			default:
+				return ErrUserNotAuthorized
+			}
+		}
+
+		ok := p.provider.ValidateSessionToken(session)
 		if !ok {
 			// This user is now no longer authorized, or we failed to
 			// validate the user.
@@ -781,25 +831,6 @@ func (p *OAuthProxy) Authenticate(rw http.ResponseWriter, req *http.Request) (er
 		}
 	}
 
-	// We revalidate group membership whenever the session is refreshed or revalidated
-	// just above in the call to ValidateSessionState and RefreshSession.
-	// To reduce strain on upstream identity providers we only revalidate email domains and
-	// addresses on each request here.
-	for _, v := range p.Validators {
-		_, EmailGroupValidator := v.(options.EmailGroupValidator)
-
-		if !EmailGroupValidator {
-			err := v.Validate(session)
-			if err != nil {
-				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", err))
-				return ErrUserNotAuthorized
-			}
-		}
-	}
-
 	logger.WithRemoteAddress(remoteAddr).WithUser(session.Email).Info(
 		fmt.Sprintf("authentication: user validated"))
 

internal/proxy/providers/providers.go

@@ -13,10 +13,10 @@ type Provider interface {
 	Redeem(string, string) (*sessions.SessionState, error)
 	ValidateGroup(string, []string, string) ([]string, bool, error)
 	UserGroups(string, []string, string) ([]string, error)
-	ValidateSessionState(*sessions.SessionState, []string) bool
+	ValidateSessionToken(*sessions.SessionState) bool
 	GetSignInURL(redirectURL *url.URL, finalRedirect string) *url.URL
 	GetSignOutURL(redirectURL *url.URL) *url.URL
-	RefreshSession(*sessions.SessionState, []string) (bool, error)
+	RefreshSession(*sessions.SessionState) (bool, error)
 }
 
 // New returns a new sso Provider

internal/proxy/providers/singleflight_middleware.go

@@ -94,10 +94,10 @@ func (p *SingleFlightProvider) UserGroups(email string, groups []string, accessT
 	return groups, nil
 }
 
-// ValidateSessionState calls the provider's ValidateSessionState function and returns the response
-func (p *SingleFlightProvider) ValidateSessionState(s *sessions.SessionState, allowedGroups []string) bool {
-	response, err := p.do("ValidateSessionState", s.AccessToken, func() (interface{}, error) {
-		valid := p.provider.ValidateSessionState(s, allowedGroups)
+// ValidateSessionToken calls the provider's ValidateSessionToken function and returns the response
+func (p *SingleFlightProvider) ValidateSessionToken(s *sessions.SessionState) bool {
+	response, err := p.do("ValidateSessionToken", s.AccessToken, func() (interface{}, error) {
+		valid := p.provider.ValidateSessionToken(s)
 		return valid, nil
 	})
 	if err != nil {
@@ -112,11 +112,11 @@ func (p *SingleFlightProvider) ValidateSessionState(s *sessions.SessionState, al
 	return valid
 }
 
-// RefreshSession takes in a SessionState and allowedGroups and
+// RefreshSession takes in a SessionState and
 // returns false if the session is not refreshed and true if it is.
-func (p *SingleFlightProvider) RefreshSession(s *sessions.SessionState, allowedGroups []string) (bool, error) {
+func (p *SingleFlightProvider) RefreshSession(s *sessions.SessionState) (bool, error) {
 	response, err := p.do("RefreshSession", s.RefreshToken, func() (interface{}, error) {
-		return p.provider.RefreshSession(s, allowedGroups)
+		return p.provider.RefreshSession(s)
 	})
 	if err != nil {
 		return false, err

internal/proxy/providers/sso.go

@@ -98,17 +98,6 @@ func isProviderUnavailable(statusCode int) bool {
 	return statusCode == http.StatusTooManyRequests || statusCode == http.StatusServiceUnavailable
 }
 
-func extendDeadline(ttl time.Duration) time.Time {
-	return time.Now().Add(ttl).Truncate(time.Second)
-}
-
-func (p *SSOProvider) withinGracePeriod(s *sessions.SessionState) bool {
-	if s.GracePeriodStart.IsZero() {
-		s.GracePeriodStart = time.Now()
-	}
-	return s.GracePeriodStart.Add(p.GracePeriodTTL).After(time.Now())
-}
-
 // Redeem takes a redirectURL and code and redeems the SessionState
 func (p *SSOProvider) Redeem(redirectURL, code string) (*sessions.SessionState, error) {
 	if code == "" {
@@ -165,9 +154,9 @@ func (p *SSOProvider) Redeem(redirectURL, code string) (*sessions.SessionState,
 		AccessToken:  jsonResponse.AccessToken,
 		RefreshToken: jsonResponse.RefreshToken,
 
-		RefreshDeadline:  extendDeadline(time.Duration(jsonResponse.ExpiresIn) * time.Second),
-		LifetimeDeadline: extendDeadline(p.SessionLifetimeTTL),
-		ValidDeadline:    extendDeadline(p.SessionValidTTL),
+		RefreshDeadline:  sessions.ExtendDeadline(time.Duration(jsonResponse.ExpiresIn) * time.Second),
+		LifetimeDeadline: sessions.ExtendDeadline(p.SessionLifetimeTTL),
+		ValidDeadline:    sessions.ExtendDeadline(p.SessionValidTTL),
 
 		Email: jsonResponse.Email,
 		User:  user,
@@ -245,9 +234,9 @@ func (p *SSOProvider) UserGroups(email string, groups []string, accessToken stri
 	return jsonResponse.Groups, nil
 }
 
-// RefreshSession takes a SessionState and allowedGroups and refreshes the session access token,
+// RefreshSession takes a SessionState and refreshes the session access token,
 // returns `true` on success, and `false` on error
-func (p *SSOProvider) RefreshSession(s *sessions.SessionState, allowedGroups []string) (bool, error) {
+func (p *SSOProvider) RefreshSession(s *sessions.SessionState) (bool, error) {
 	logger := log.NewLogEntry()
 
 	if s.RefreshToken == "" {
@@ -259,35 +248,17 @@ func (p *SSOProvider) RefreshSession(s *sessions.SessionState, allowedGroups []s
 		// When we detect that the auth provider is not explicitly denying
 		// authentication, and is merely unavailable, we refresh and continue
 		// as normal during the "grace period"
-		if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) {
+		if err == ErrAuthProviderUnavailable && s.IsWithinGracePeriod(p.GracePeriodTTL) {
 			tags := []string{"action:refresh_session", "error:redeem_token_failed"}
 			p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
-			s.RefreshDeadline = extendDeadline(p.SessionValidTTL)
+			s.RefreshDeadline = sessions.ExtendDeadline(p.SessionValidTTL)
 			return true, nil
 		}
 		return false, err
 	}
 
-	inGroups, validGroup, err := p.ValidateGroup(s.Email, allowedGroups, newToken)
-	if err != nil {
-		// When we detect that the auth provider is not explicitly denying
-		// authentication, and is merely unavailable, we refresh and continue
-		// as normal during the "grace period"
-		if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) {
-			tags := []string{"action:refresh_session", "error:user_groups_failed"}
-			p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
-			s.RefreshDeadline = extendDeadline(p.SessionValidTTL)
-			return true, nil
-		}
-		return false, err
-	}
-	if !validGroup {
-		return false, errors.New("Group membership revoked")
-	}
-	s.Groups = inGroups
-
 	s.AccessToken = newToken
-	s.RefreshDeadline = extendDeadline(duration)
+	s.RefreshDeadline = sessions.ExtendDeadline(duration)
 	s.GracePeriodStart = time.Time{}
 	logger.WithUser(s.Email).WithRefreshDeadline(s.RefreshDeadline).Info("refreshed session access token")
 	return true, nil
@@ -340,16 +311,16 @@ func (p *SSOProvider) redeemRefreshToken(refreshToken string) (token string, exp
 	return
 }
 
-// ValidateSessionState takes a sessionState and allowedGroups and validates the session state
-func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGroups []string) bool {
+// ValidateSessionToken takes a sessionState and validates the session token
+func (p *SSOProvider) ValidateSessionToken(s *sessions.SessionState) bool {
 	logger := log.NewLogEntry()
 
 	// we validate the user's access token is valid
 	params := url.Values{}
 	params.Add("client_id", p.ClientID)
 	req, err := p.newRequest("GET", fmt.Sprintf("%s?%s", p.ValidateURL.String(), params.Encode()), nil)
 	if err != nil {
-		logger.WithUser(s.Email).Error(err, "error validating session state")
+		logger.WithUser(s.Email).Error(err, "error validating session access token")
 		return false
 	}
 
@@ -358,52 +329,29 @@ func (p *SSOProvider) ValidateSessionState(s *sessions.SessionState, allowedGrou
 
 	resp, err := httpClient.Do(req)
 	if err != nil {
-		logger.WithUser(s.Email).Error("error making request to validate access token")
+		logger.WithUser(s.Email).Error("error making request to validate session access token")
 		return false
 	}
 
 	if resp.StatusCode != 200 {
 		// When we detect that the auth provider is not explicitly denying
 		// authentication, and is merely unavailable, we validate and continue
 		// as normal during the "grace period"
-		if isProviderUnavailable(resp.StatusCode) && p.withinGracePeriod(s) {
+		if isProviderUnavailable(resp.StatusCode) && s.IsWithinGracePeriod(p.GracePeriodTTL) {
 			tags := []string{"action:validate_session", "error:validation_failed"}
 			p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
-			s.ValidDeadline = extendDeadline(p.SessionValidTTL)
+			s.ValidDeadline = sessions.ExtendDeadline(p.SessionValidTTL)
 			return true
 		}
 		logger.WithUser(s.Email).WithHTTPStatus(resp.StatusCode).Info(
-			"could not validate user access token")
-		return false
-	}
-
-	// check the user is in the proper group(s)
-	inGroups, validGroup, err := p.ValidateGroup(s.Email, allowedGroups, s.AccessToken)
-	if err != nil {
-		// When we detect that the auth provider is not explicitly denying
-		// authentication, and is merely unavailable, we validate and continue
-		// as normal during the "grace period"
-		if err == ErrAuthProviderUnavailable && p.withinGracePeriod(s) {
-			tags := []string{"action:validate_session", "error:user_groups_failed"}
-			p.StatsdClient.Incr("provider_error_fallback", tags, 1.0)
-			s.ValidDeadline = extendDeadline(p.SessionValidTTL)
-			return true
-		}
-		logger.WithUser(s.Email).Error(err, "error fetching group memberships")
-		return false
-	}
-
-	if !validGroup {
-		logger.WithUser(s.Email).WithAllowedGroups(allowedGroups).Info(
-			"user is no longer in valid groups")
+			"could not validate session access token")
 		return false
 	}
-	s.Groups = inGroups
 
-	s.ValidDeadline = extendDeadline(p.SessionValidTTL)
+	s.ValidDeadline = sessions.ExtendDeadline(p.SessionValidTTL)
 	s.GracePeriodStart = time.Time{}
 
-	logger.WithUser(s.Email).WithSessionValid(s.ValidDeadline).Info("validated session")
+	logger.WithUser(s.Email).WithSessionValid(s.ValidDeadline).Info("validated session access token")
 
 	return true
 }