feat: enable GitHub OAuth2 login by default on new deployments #16662
Conversation
66e0aff to
2788eb5
Compare
|
Note: before merging, I need to update the embedded GitHub Client ID. I'm still waiting for the app to be properly set up on our side. |
@hugodutka how will users using this embedded client ID? Each GitHub app needs a unique callback URL as far as I understand, so I'm not sure how this will work. EDIT: nevermind, seems like the redirect URI can be customised. |
|
@dannykopping we’ll be using the device flow here, which I implemented support for a couple of days ago: #16585. This doesn’t use a callback URL. |
codersdk/deployment.go
Outdated
| Name: "OAuth2 GitHub Default Provider", | ||
| Description: "Enable the default GitHub OAuth2 provider managed by Coder.", | ||
| Flag: "oauth2-github-default-provider", | ||
| Env: "CODER_OAUTH2_GITHUB_DEFAULT_PROVIDER", |
There was a problem hiding this comment.
nit: for consistency, this should have the _ENABLE suffix. This is not a blocking change though.
cli/server_test.go
Outdated
| t.Run("NewDeployment", func(t *testing.T) { | ||
| runGitHubProviderTest(t, testCase{ | ||
| githubEnabled: true, | ||
| createUserPreStart: false, | ||
| createUserPostRestart: true, | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ExistingDeployment", func(t *testing.T) { | ||
| runGitHubProviderTest(t, testCase{ | ||
| githubEnabled: false, | ||
| createUserPreStart: true, | ||
| createUserPostRestart: false, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Can you also add a test case as follows:
- GIVEN an existing deployment with 0 users and
CODER_OAUTH2_GITHUB_{CLIENT_ID,CLIENT_SECRET}set - WHEN coder starts
- THEN the new "default" GitHub OAuth2 provider is not enabled
There was a problem hiding this comment.
I think there's also an argument to be made against enabling this by default if OIDC login is enabled.
There was a problem hiding this comment.
I'd rather not make this logic dependent on OIDC config. I think it's not intuitive that setting one up would disable the other.
There was a problem hiding this comment.
:D It sounds like there's an argument against then. Let's leave it out of this PR so!
cli/server.go
Outdated
| params.clientID = "Iv23liSBHklRMBNx5lk9" | ||
| params.allowEveryone = true | ||
| params.deviceFlow = true |
There was a problem hiding this comment.
Can this be extracted to a highly visible variable?
There was a problem hiding this comment.
What do you consider highly visible? Package level global constant? Just asking to avoid further back and forth on this.
There was a problem hiding this comment.
Something like that, yeah. We should need to go searching to find the default client ID.
I assume OAuth2 device flow doesn't require a client secret?
There was a problem hiding this comment.
That's right, it doesn't require a client secret.
What do you mean by not accessible? As in the deployment is airgapped? |
Not necessarily; there have been multiple instances in the past where state actors have blocked GitHub. Or it could be blocked at the corporate network level. What would you say is the best course of action in this case? |
|
@johnstcn I don't think this would be very common. I think just allowing the admins to disable the default via an env variable is enough in these cases. |
eb41f65 to
032150d
Compare
1a67ae7 to
dfd49d0
Compare
9a15225 to
407cdd3
Compare
|
Addressed the feedback and updated the embedded client ID. Should be ready to merge once the base branch is merged. |
dfd49d0 to
3b36025
Compare
407cdd3 to
071d6cc
Compare
071d6cc to
1720441
Compare
1720441 to
297e90b
Compare
Third and final PR to address #16230.
This PR enables GitHub OAuth2 login by default on new deployments. Combined with #16629, this will allow the first admin user to sign up with GitHub rather than email and password.
We take care not to enable the default on deployments that would upgrade to a Coder version with this change.
To disable the default provider an admin can set the
CODER_OAUTH2_GITHUB_DEFAULT_PROVIDERenv variable to false.