buzzfeed / sso

sso, aka S.S.Octopus, aka octoboi, is a single sign-on solution for securing internal services
MIT License
3.07k stars 187 forks source link

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

Open Jusshersmith opened 4 years ago

Jusshersmith commented 4 years ago

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:

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

codecov[bot] commented 4 years ago

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
Jusshersmith commented 4 years ago

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