sso icon indicating copy to clipboard operation
sso copied to clipboard

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

Open Jusshersmith opened this issue 5 years ago • 2 comments

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.

Jusshersmith avatar Dec 06 '19 16:12 Jusshersmith

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% <ø> (ø) :arrow_up:
internal/pkg/validators/email_group_validator.go 0% <0%> (ø)
internal/proxy/providers/test_provider.go 0% <0%> (ø) :arrow_up:
...nternal/proxy/providers/singleflight_middleware.go 0% <0%> (ø) :arrow_up:
internal/proxy/proxy.go 20% <0%> (ø) :arrow_up:
internal/pkg/validators/mock_validator.go 0% <0%> (ø)
internal/auth/authenticator.go 86.18% <100%> (ø) :arrow_up:
... and 6 more

codecov[bot] avatar Dec 12 '19 12:12 codecov[bot]

@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.

Jusshersmith avatar Jan 28 '20 12:01 Jusshersmith