sso
sso copied to clipboard
sso_proxy: reduce direct calls to ValidateGroup() and clean up logic
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
andwithinGracePeriod
to be part of the sessions package, instead of the providers package. (In fact, a version ofextendDeadline
already exists in the session package. We now use that instead) - Changes direct calls to
ValidateGroup
withininternal/proxy/providers/sso.go
to options/validator package calls withininternal/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.
- Introduces the
- Renames the
ValidateSessionState
method toValidateSessionToken
, 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 tovalidators
to better represent its responsibility.
Codecov Report
Merging #275 into master will decrease coverage by
0.2%
. The diff coverage is47.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 |
@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.