sso_proxy: reduce direct calls to ValidateGroup() and clean up logic

[Jusshersmith]

Problem

We are still calling ValidateGroup() directly within sso_proxy, but using the options/validator package elsewhere in the same logic path (originally partially due to circular imports). This makes it increasingly difficult to make sure we were running the right validations at the right time, and certain methods were growing in complexity and responsibility.

Solution

Attempt to reunite some of the most problematic portions of code in related to the above.

High level overview of included changes:

• Moves extendDeadline and withinGracePeriod to be part of the sessions package, instead of the providers package. (In fact, a version of extendDeadline already exists in the session package. We now use that instead)
• Changes direct calls to ValidateGroup within internal/proxy/providers/sso.go to options/validator package calls within internal/proxy/providers/oauthproxy.go.
  • Introduces the runValidatorsWithGracePeriod helper method here to help handle cases where we want to check if the auth provider is unavailable instead of explicitly denying authentication.
• Renames the ValidateSessionState method to ValidateSessionToken, which seemed to better fit its responsibility.
• Modify tests to explicitly test new logic
• Some formatting simplification/changes of the validator errors, and other general refactoring

Notes

The perhaps less obvious change is that within the Authenticate() method we'll now only run validators when the refresh or validation period has expired.
This is instead of running group validations when the refresh or validation period has expired, and domain/email validations on all proxied requests.

---------- EDIT -----------

Additional changes

• This also now includes some logic to prevent the copying + use of a cookie authorised with upstream 'foo' with upstream 'bar'. A new AuthorisedUpstream value has been added to the session which is checked against the request host.
  For the time being, when caught this check will re-trigger the start of the oauth flow, primarily to help introduce this additional check in a graceful manner.

• options package now renamed to validators to better represent its responsibility.

[codecov]

Codecov Report

Merging #275 into master will decrease coverage by 0.2%.
The diff coverage is 47.82%.

@@            Coverage Diff            @@
##           master    #275      +/-   ##
=========================================
- Coverage   62.51%   62.3%   -0.21%     
=========================================
  Files          54      54              
  Lines        4199    4197       -2     
=========================================
- Hits         2625    2615      -10     
- Misses       1385    1394       +9     
+ Partials      189     188       -1

Impacted Files	Coverage Δ
internal/pkg/validators/validators.go	0% <ø> (ø)
internal/pkg/validators/email_domain_validator.go	100% <ø> (ø)
internal/pkg/validators/email_address_validator.go	100% <ø> (ø)
internal/proxy/providers/providers.go	0% <ø> (ø)	⬆️
internal/pkg/validators/email_group_validator.go	0% <0%> (ø)
internal/proxy/providers/test_provider.go	0% <0%> (ø)	⬆️
...nternal/proxy/providers/singleflight_middleware.go	0% <0%> (ø)	⬆️
internal/proxy/proxy.go	20% <0%> (ø)	⬆️
internal/pkg/validators/mock_validator.go	0% <0%> (ø)
internal/auth/authenticator.go	86.18% <100%> (ø)	⬆️
... and 6 more

[jphines]

Can we put the allowed groups in the validator error message instead of pulling them out this way? I think it could make the logline easier to understand which set of validators rejected a user?

[Jusshersmith]

Added the allowed groups into the group validator error message. Below is an example of the error message displayed to users (the same formatting is used for the log lines).

Also removed a chunk of formatting logic around the errors as it was becoming over-engineered and unnecessary. We could also add some extra context to errors coming from the domain/email address validators, but as is the error message would become pretty bloated if multiple validators returned errors - so perhaps worth addressing that separately?

[jphines]

A few comment nits -- but once those are cleared up I think this is good to go.

[Jusshersmith]

@jphines - ready for re-review. Main new logic changes consist of the change to the raised error if an unauthorised upstream is being requested to better handle the graceful introduction of this check, and formatting of the errors returned by the validators.

Other than that, it's largely comment changes/additions.