-
Notifications
You must be signed in to change notification settings - Fork 706
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
Oauth logout login 2 #1315
Oauth logout login 2 #1315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, thanks!
"loginURI": "/oauth2/start", | ||
"logoutURI": "/oauth2/sign_out" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I think about this, maybe it's a bit confusing to call this loginURI
(or logoutURI
) since kubeapps has alredy URIs for the login (/login
), maybe it's better to call it oauthLoginURI
? Feel free to do that in a different PR or ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will do.
@@ -70,6 +70,7 @@ spec: | |||
- -skip-auth-regex=^\/config\.json$ | |||
- -skip-auth-regex=^\/favicon.*\.png$ | |||
- -skip-auth-regex=^\/static\/ | |||
- -skip-auth-regex=^\/$ | |||
{{- range .Values.authProxy.additionalFlags }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to skip the /login
route as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - at least, not currently, because we're using a hash-based url scheme. So the /login
route is actually /#/login
, which as far as oauth2proxy is concerned, is the /
path (like all the app routes), even on a refresh.
</a>{" "} | ||
for more info about using authentication providers with Kubeapps. | ||
</p> | ||
<a href={this.props.loginURI} className="button button-accent"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this a link and not a button? (it's okay if it's the same, I am just curious)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean why, rather than how? It's a plain link both in function (ie. just linking to a URI not handled by the app, without the /#/...
hash uri). But yes, the class ensures it's the same in appearance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, well, I was just surprised that a link with the button classes looks exactly the same than the button
Description of the change
Enables oauth login to use our normal Kubeapps login page (rather than the rather blank oauth2-proxy default).
Benefits
A unified login view for Kubeapps, regardless of whether token login or oauth/oidc is being used.
Also ensures we don't see a flash of the login screen after auth'ing.
Applicable issues
Additional information
Enables the root route to pass through the proxy, so that our own login form can trigger the authentication as it currently does for token auth also.