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

[Jusshersmith]

Problem

When certain TTL's expire we revalidate or refresh the session (e.g. https://github.com/buzzfeed/sso/blob/master/internal/proxy/oauthproxy.go#L759-L764), which then ends up directly calling the ValidateGroup provider method (https://github.com/buzzfeed/sso/blob/master/internal/proxy/providers/sso.go#L381).

Because here we're not using the validator abstractions, if group validation isn't being utilised, then this causes a 403 with user is no longer in valid groups to be returned, requiring the user to refresh the page to progress further.

Solution

The logic within ValidateGroup to return a success when an empty slice of groups is passed in was removed during some refactoring as, when using 'validator' abstractions this was deemed bad behaviour**, but until we stop calling ValidateGroup directly and use those abstractions everywhere we need to maintain this behaviour.

https://github.com/buzzfeed/sso/pull/275 fixes this in a more permanent, stable fashion by replacing any remaining direct calls to ValidateGroup with calls to the validator abstractions, however until that is merged this is one possible temporary fix.

Notes

**This was deemed bad behaviour because if the group validator is the only validator in use (and not email domains or addresses), then allowing ValidateGroup to return successful if an empty list of groups is passed becomes a loophole to allow the defining of no validators, something which we explicitly prevent (https://github.com/buzzfeed/sso/blob/master/internal/proxy/options.go#L205-L213)

[Jusshersmith]

Looking back through this, I have some new thoughts:

Allowing the only defined validator to be used with a wildcard isn't new to SSO, and also actually remains possible using the other email domain/address validators. Historically, this has probably been classed as less of a 'loophole' and more of a 'workaround'.

It probably makes sense to offer the functionality to say "as long as the request comes from a valid user in my {insert provider} domain, then allow them". But seeing as this does reduce the security footprint somewhat, we should offer this as an explicit configuration option that can be set, rather than allowing a workaround by defining a validator with a wildcard.

My proposal then becomes:

• ship this fix (https://github.com/buzzfeed/sso/pull/286/files#diff-a45368080c12e555b0d7596f6da0d7faR184-R186)
• fix the issues in https://github.com/buzzfeed/sso/pull/275
• prevent the use of wildcards within validators, and require at least one validator by default, though add a new explicit configuration option which allows the setting of no validators.

Maybe we can combine the first and last point together.

[codecov[bot]]

Codecov Report

Merging #286 into master will increase coverage by 0.09%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   61.85%   61.94%   +0.09%     
==========================================
  Files          57       57              
  Lines        4638     4649      +11     
==========================================
+ Hits         2869     2880      +11     
  Misses       1556     1556              
  Partials      213      213

Impacted Files	Coverage Δ
internal/pkg/options/email_address_validator.go	100% <ø> (ø)	:arrow_up:
internal/pkg/options/email_domain_validator.go	100% <100%> (ø)	:arrow_up:
internal/proxy/providers/sso.go	68.84% <100%> (+0.24%)	:arrow_up:
internal/proxy/options.go	86.27% <100%> (+1.6%)	:arrow_up: