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 theCODER_OAUTH2_GITHUB_DEFAULT_PROVIDER env variable to false.

github-actionsbot assignedhugodutka

Feb 22, 2025

hugodutka force-pushed thehugodutka/github-oauth2-default branch 7 times, most recently from66e0aff to2788eb5Compare

February 22, 2025 16:30

hugodutka mentioned this pull request

Feb 23, 2025

docs: update the quickstart page#16666

Merged

hugodutka marked this pull request as ready for review

February 23, 2025 18:08

hugodutka requested a review fromdannykopping

February 23, 2025 18:08

Copy link

ContributorAuthor

hugodutka commentedFeb 23, 2025

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.

Copy link

Contributor

dannykopping commentedFeb 24, 2025•
edited
Loading

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 theredirect URI can be customised.

dannykopping requested a review fromjohnstcn

February 24, 2025 07:52

Copy link

ContributorAuthor

hugodutka commentedFeb 24, 2025•
edited
Loading

@dannykopping we’ll be using thedevice flow here, which I implemented support for a couple of days ago:#16585. This doesn’t use a callback URL.

johnstcn reviewed

Feb 24, 2025

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

The change looks OK to me mechanically, and the device flow is convenient. What happens to new deployments where GitHub is not accessible though? It looks like this will be enabled and useless to the admin.

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",

Copy link

Member

johnstcnFeb 24, 2025•
edited
Loading

Choose a reason for hiding this comment

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

nit: for consistency, this should have the_ENABLE suffix. This isnot a blocking change though.

cli/server_test.go Outdated

Comment on lines 391 to 405

		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,
		})
		})

Copy link

Member

johnstcnFeb 24, 2025

Choose a reason for hiding this comment

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

Can you also add a test case as follows:

GIVEN an existing deployment with 0 users andCODER_OAUTH2_GITHUB_{CLIENT_ID,CLIENT_SECRET} set
WHEN coder starts
THEN the new "default" GitHub OAuth2 provider isnot enabled

Copy link

Member

johnstcnFeb 24, 2025•
edited
Loading

Choose a reason for hiding this comment

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

I think there's also an argument to be made against enabling this by default if OIDC login is enabled.

Copy link

ContributorAuthor

hugodutkaFeb 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link

Member

johnstcnFeb 24, 2025

Choose a reason for hiding this comment

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

:D It sounds like there's an argument against then. Let's leave it out of this PR so!

cli/server.go Outdated

Comment on lines 1896 to 1898

		params.clientID="Iv23liSBHklRMBNx5lk9"
		params.allowEveryone=true
		params.deviceFlow=true

Copy link

Member

johnstcnFeb 24, 2025

Choose a reason for hiding this comment

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

Can this be extracted to ahighly visible variable?

Copy link

ContributorAuthor

hugodutkaFeb 24, 2025

Choose a reason for hiding this comment

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

What do you consider highly visible? Package level global constant? Just asking to avoid further back and forth on this.

Copy link

Member

johnstcnFeb 24, 2025

Choose a reason for hiding this comment

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

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?

Copy link

ContributorAuthor

hugodutkaFeb 24, 2025

Choose a reason for hiding this comment

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

That's right, it doesn't require a client secret.

Copy link

ContributorAuthor

hugodutka commentedFeb 24, 2025

@johnstcn

What happens to new deployments where GitHub is not accessible though?

What do you mean by not accessible? As in the deployment is airgapped?

Copy link

Member

johnstcn commentedFeb 24, 2025

@johnstcn
What happens to new deployments where GitHub is not accessible though?
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?

Copy link

ContributorAuthor

hugodutka commentedFeb 24, 2025

@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.