Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proxy: transition idps ux flow #218

Merged
merged 2 commits into from
Jun 26, 2019
Merged

proxy: transition idps ux flow #218

merged 2 commits into from
Jun 26, 2019

Conversation

jphines
Copy link
Contributor

@jphines jphines commented Jun 25, 2019

Problem

If a user accesses an upstream for which the identity provider has changed, the user will get a very confusing and potentially concerning 500 Internal Server Error. We can fix this ux flow so the user can be transparently authenticated with the new provider

Solution

If a user is are already authenticated, we can transparently re-auth the user by clearing the existing cookie and restarting the authentication flow. If they aren't authenticated, this same process starts new auth flow at the authenticator.

Notes

In order to make this work, we must add new fields to the session object which includes what provider slug/type information for the session. This adds some potential length to this cookie which is already starting to get big.

@jphines jphines added the enhancement New feature or request label Jun 25, 2019
@jphines jphines self-assigned this Jun 25, 2019
@jphines jphines changed the title Sso transition idps proxy: transition idps ux flow Jun 25, 2019
Jusshersmith
Jusshersmith previously approved these changes Jun 25, 2019
Copy link
Contributor

@Jusshersmith Jusshersmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@codecov
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #218 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #218   +/-   ##
=======================================
+ Coverage   61.23%   62.23%   +1%     
=======================================
  Files          50       50           
  Lines        4073     4067    -6     
=======================================
+ Hits         2494     2531   +37     
+ Misses       1393     1349   -44     
- Partials      186      187    +1
Impacted Files Coverage Δ
internal/pkg/sessions/session_state.go 85% <ø> (+36.42%) ⬆️
internal/proxy/providers/sso.go 69.76% <100%> (+0.23%) ⬆️
internal/proxy/oauthproxy.go 50.98% <100%> (+7.33%) ⬆️
internal/proxy/reverse_proxy.go 90.43% <0%> (+1.73%) ⬆️

Copy link
Contributor

@Jusshersmith Jusshersmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

teststeststests! 🔥

@jphines jphines merged commit 5819e69 into master Jun 26, 2019
@jphines jphines deleted the sso-transition-idps branch June 26, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants