Open Jusshersmith opened 5 years ago
Merging #261 into master will decrease coverage by
0.01%
. The diff coverage is65.9%
.
@@ Coverage Diff @@
## master #261 +/- ##
=========================================
- Coverage 62.51% 62.5% -0.02%
=========================================
Files 54 55 +1
Lines 4199 4211 +12
=========================================
+ Hits 2625 2632 +7
- Misses 1385 1386 +1
- Partials 189 193 +4
Impacted Files | Coverage Δ | |
---|---|---|
internal/proxy/providers/providers.go | 0% <ø> (ø) |
:arrow_up: |
internal/proxy/providers/sso.go | 69.14% <0%> (+0.53%) |
:arrow_up: |
internal/proxy/providers/test_provider.go | 0% <0%> (ø) |
:arrow_up: |
...nternal/proxy/providers/singleflight_middleware.go | 0% <0%> (ø) |
:arrow_up: |
internal/pkg/templates/templates.go | 100% <100%> (ø) |
:arrow_up: |
internal/proxy/static_files.go | 73.33% <73.33%> (ø) |
|
internal/auth/authenticator.go | 83.85% <84.21%> (-2.33%) |
:arrow_down: |
internal/proxy/oauthproxy.go | 59.15% <86.27%> (+4.71%) |
:arrow_up: |
Marked as ready for review, but see Notes in description.
Problem
The current setup of html sign in and sign out pages causes some extra friction with implementing https://github.com/buzzfeed/sso/pull/252 - this change gives us some extra flexibility here. Also, having
sso_proxy
first send requests tosso_auth
, which subsequently renders the sign in page (and redirects back) results in extra requests and extra complexity (for example, an extra layer of nested redirects needs to be specified).Solution
Move the sign_in and sign_out HTML pages to be rendered by sso_proxy instead of sso_auth. In doing so, simplify the flow in part between
sso_proxy
andsso_authenticator
.Notes
This is a big change, so I'm working to add some more detailed descriptions of the changes (and going over any TODO's) to aid in better understanding them, as well as reviewing the PR in general.