Skip to content

fix: Allow Github.com auth when github-enterprise.uri is set #7002

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

Merged
merged 3 commits into from
Jun 23, 2025

Conversation

dyhagho
Copy link
Contributor

@dyhagho dyhagho commented Jun 2, 2025

Problem

Since version 0.110.0, setting the following configuration:

"github-enterprise.uri": "https://someghe.com"

causes the GitHub.com authentication flow to be skipped entirely. This results in:

  • No prompt to authenticate with github.com
  • Pull Request extension for Github.com repos breaking silently

For many developers (including myself), contributing to both github.com and a company-hosted GitHub Enterprise is common.

Even though the config can be set per workspace, it can be tedious to maintain across multiple repositories.
And, in practice, setting github-enterprise.uri in user settings makes more sense. But this intentionally disables github.com functionality with the current logic.

What does change?

This PR updates doCreate so that GitHub.com is initialized even though the enterprise URI is configured.
This mirrors pre-0.110.0 behavior.

Before

before

After

after

@dyhagho dyhagho marked this pull request as draft June 20, 2025 11:27
@dyhagho dyhagho marked this pull request as ready for review June 20, 2025 11:27
@dyhagho
Copy link
Contributor Author

dyhagho commented Jun 20, 2025

@alexr00 could I get a review? :)

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I understand the motivation, but I think we need one change.

@@ -221,6 +221,7 @@ export class CredentialStore extends Disposable {
}

private async doCreate(options: vscode.AuthenticationGetSessionOptions, additionalScopes: boolean = false): Promise<AuthResult> {
const github = await this.initialize(AuthProvider.github, options, additionalScopes ? SCOPES_WITH_ADDITIONAL : undefined, additionalScopes);
Copy link
Member

Choose a reason for hiding this comment

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

This will cause a badge to be shown on the Accounts menu, prompting for github.com auth, even when enterprise auth is successful. If enterprise auth has succeeded, the options should include silent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

What do you think of this approach?

const githubOptions = { ...options };
if (enterprise && !enterprise.canceled && this.isAuthenticated(AuthProvider.githubEnterprise)) {
  githubOptions.silent = true;
}
const github = await this.initialize(AuthProvider.github, githubOptions, additionalScopes ? SCOPES_WITH_ADDITIONAL : undefined, additionalScopes);

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that the isAuthenticated check is needed, but otherwise it looks good.

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

@dyhagho, sorry to hurry things along after you waited patiently, but it looks like we need this fix for #6971! I have pushed your proposed change.

@alexr00 alexr00 enabled auto-merge (squash) June 23, 2025 13:16
@vs-code-engineering vs-code-engineering bot added this to the June 2025 milestone Jun 23, 2025
@alexr00
Copy link
Member

alexr00 commented Jun 23, 2025

/azp run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@alexr00 alexr00 merged commit ad89f17 into microsoft:main Jun 23, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants