-
Notifications
You must be signed in to change notification settings - Fork 1k
chore: move /gitauth
to /externalauth
on the frontend
#9954
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
This actually took a lot more jank than anticipated, so I wanted to split this up before adding the ability to embed new providers.
Is the goal to add more oauth2 providers? |
@Emyrk the goal is to enable any OAuth2 or OIDC provider to become a "Click to Login" during workspace creation. |
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.
Pretty sure I commented on the only actual Go code changes that are not just a rename refactor. If that is all this PR is (A rename refactor), then green check the Go code.
I have not looked at all the existing tests for external auth, but if we are going to refactor/add/modify them, lets use the oidctest
package and do it right.
@@ -628,7 +628,7 @@ func TestCreateWithGitAuth(t *testing.T) { | |||
clitest.Start(t, inv) | |||
|
|||
pty.ExpectMatch("You must authenticate with GitHub to create a workspace") | |||
resp := coderdtest.RequestGitAuthCallback(t, "github", client) | |||
resp := coderdtest.RequestExternalAuthCallback(t, "github", client) |
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.
We should really switch these to the oidctest
package. OIDC is a super set of oauth2 right?
Will be fun... more test refactoring 😄
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.
Yeah, we def should. When I make the actual changes I will look into this
This actually took a lot more jank than anticipated, so I wanted to split this up before adding the ability to embed new providers.