-
Notifications
You must be signed in to change notification settings - Fork 5
[CP Auth 4/5]: add the CP Auth object and redux logic #1767
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
Conversation
| import { CPAUTH_USER_EXPIRED, CPAUTH_USER_LOAD } from 'stores/cpauth/constants'; | ||
| import { getCPAuthUser } from 'stores/cpauth/selectors'; | ||
|
|
||
| export function cpAuthMiddleware(cpAuth: CPAuth): Middleware { |
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.
This middleware is used to renew user information, if a user is expired.
In general, the user will be renewed automatically, but the flow will fail if there's a race condition between renewal and a separate request. This prevents that.
There is also the case where the token expires when the user while the user is not logged in, so automatic renew is not possible
This can also prevent actions from getting dispatched, if the user expired, if we want to use this auth method as a primary one.
| import { defaultConfig } from 'lib/CPAuth/config'; | ||
| import OAuth2 from 'lib/OAuth2/OAuth2'; | ||
|
|
||
| class CPAuth extends OAuth2 { |
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.
This inherits from the OAuth2 class, so we could have this as a singleton, but not limit the number of OAuth2 providers to a single one.
| SignUp: '/signup/:token', | ||
| OAuthCallback: '/oauth/callback', | ||
| CPAccess: '/cp-access', | ||
| CPAccessCallback: '/cp-access/callback', |
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.
This is the path that the user is redirected to, after logging in
| authority, | ||
| clientId: 'fFlz7lckhWA0kIaW3fLIl8chFSs2wvW6', | ||
| clientSecret: | ||
| 'PoioOqWKUndxVnbcRzlv59EgvwPVJQIdIlved143Uko0SjGJ7OprnecZQbab3WhH', |
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.
I think we really gotta figure out a way to do this without a client secret...
We kinda need dex to support PKCE: dexidp/dex#1652
That's the flow we're using with auth0 I think
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.
Great find with that PR!
But can we move forward with this PR until we get that sorted?
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.
Btw, I don't think this is a huge problem right now, as users still need to have access to the control plane they try to login on.
Take for example a customer installation. Try authenticating with the kubectl plugin. Dex will allow the authentication request, since the client ID and Secret are correct, but you won't actually be able to log in, since you're not registered in the customer's authentication configuration (e.g. active directory).
oponder
left a comment
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.
LGTM but not feeling 100% about the client secret, lets keep an eye on dex and improve that when the opportunity presents itself.
|
Presenting Opportunity ;o) |
Part of the larger #1547
This PR adds the dex configuration, and CP Authentication redux logic