Change database download to always be authenticated

[robertbrignull]

This PR changes the behaviour when requesting to download a CodeQL database from GitHub to always require logging in.

I have only applied this change in behaviour to user-initiated command (either from the UI or command palette) and after this there will be no way to avoid logging in, but if there's extreme backlash we could implement an opt-out config option. There's also a flow when the extension starts up but I have avoided changing this to keep the changes smaller.

The existing behaviour of the extension is that it would require you to login and make the requests authenticated only if canary mode was enabled (or you were using GHEC-DR). If canary mode was disabled then it would authenticate if a token was available but otherwise it would make unauthenticated requests.

Here are some flowchats showing the change:

Old behaviour for user-initiated download:

New behaviour for user-initiated download:

[Copilot]

PR Overview

This PR enforces authentication when downloading a CodeQL database from GitHub, eliminating the fallback to unauthenticated requests.

• In the API module, error handling in listDatabases has been simplified to require a valid access token.
• Test mocks for unauthenticated AppOctokit instances have been removed and assertions updated.
• The database fetcher now directly retrieves an authenticated Octokit instance without a fallback.

Reviewed Changes

File	Description
extensions/ql-vscode/src/databases/github-databases/api.ts	Simplifies authentication logic and error handling for listing databases.
extensions/ql-vscode/test/vscode-tests/no-workspace/databases/github-databases/api.test.ts	Removes outdated mocks and assertions for unauthenticated flows.
extensions/ql-vscode/CHANGELOG.md	Documents the change in authentication behavior.
extensions/ql-vscode/src/databases/database-fetcher.ts	Removes the fallback to unauthenticated AppOctokit and always uses credentials.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

extensions/ql-vscode/src/databases/github-databases/api.ts:70

• The try-catch that handled 404 errors from unauthenticated requests was removed. Please double-check that this logic is fully handled upstream now that authentication is always required.

if (!hasAccessToken && !(await askForGitHubConnect(config))) {

extensions/ql-vscode/test/vscode-tests/no-workspace/databases/github-databases/api.test.ts:161

• Outdated assertions referencing AppOctokit have been removed in favor of the authenticated octokit flow. Ensure that test expectations are updated to only rely on the active mocks.

expect(appMockListCodeqlDatabases).toHaveBeenCalled();

Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more

[Copilot]

The fallback logic to instantiate an unauthenticated AppOctokit has been removed. Verify that this code path always has valid credentials to avoid potential runtime errors.

Suggested change

	if (!this.app.credentials) {
	throw new Error("Missing credentials: Unable to instantiate Octokit without valid credentials.");
	}

[robertbrignull]

That's not how this works. The type of this.app.credentials is not nullable. If the user isn't authenticated already then calling credentials.getOctokit() will prompt the user to log in.

[koesie10]

I haven't reviewed the code yet, but I'm wondering if always requiring the user to login is necessary. Could we instead use the session if the user is logged in, and make an unauthenticated request if the user is not logged in? I believe this will still allow the user to login manually from VS Code because the profile icon will allow you to login to the extension:

[robertbrignull]

I'm wondering if always requiring the user to login is necessary. Could we instead use the session if the user is logged in, and make an unauthenticated request if the user is not logged in? I believe this will still allow the user to login manually from VS Code because the profile icon will allow you to login to the extension:

None of it's really necessary. What you're suggesting would also work and could be a middle-ground if we don't want to go all the way to always requiring logging in.

The main goal of this change is to reduce the number of support requests where people can't download their database because it's a private repo. It's possible that if we removed the canary mode requirement but kept logging in as optional then either it would just work or people would realise this and log in to fix any problems. There's also a decent chance they're already logged in if they're using any other extensions, but if they're logged in already then both of these potential changes are equivalent and neither will negatively affect them.

So the crucial question is if someone isn't logged in and their download fails, will they try logging in (and thus not raise a support request)? Alternatively would they be offended at being asked to log in? If there's no downside to requiring logging in then we may as well do so. 🤷🏼

[aeisenberg]

The askForGitHubConnect function allows you to dismiss a dialog as Never, which means that github will not try to download a database.

Previously, it looks like askForGitHubConnect was only called if there was no token available. Now it looks like it is always called.

Does this mean that if Never is selected, then databases can't be downloaded even if a user explicitly requests it? I'm not exactly sure of the flow, so I may be missing something.

[robertbrignull]

There's two separate flows which are annoyingly distinct from each other in the code. The user-initiated flow is the one we mainly use and is barely changed. The other flow is triggered automatically when starting and tries to see if your repo has a database. If you answer "never" then it won't do the automatic flow anymore but you can still download databases manually.

But based on discussion later on maybe I'll revert the changes here so it'll be a lot easier to review and merge.

[koesie10]

If our main goal is to reduce support requests, could the following also work?

1. If there is no session, try downloading without an access token
2. When the repository is not found, show a dialog like "Database not found, this may be because the repository is private. Do you want to login and retry?" with a "Login" button.
3. Then, retry if the user chooses to login.

There's also a decent chance they're already logged in if they're using any other extensions, but if they're logged in already then both of these potential changes are equivalent and neither will negatively affect them.

I'm not sure if this is the case. I believe the authentication session is shared between extensions, but an extension only has access to it after a user explicitly logs in. This might also be because we require scopes that other extensions don't use (such as read:packages).

[robertbrignull]

If our main goal is to reduce support requests, could the following also work?

1. If there is no session, try downloading without an access token
2. When the repository is not found, show a dialog like "Database not found, this may be because the repository is private. Do you want to login and retry?" with a "Login" button.
3. Then, retry if the user chooses to login.

Yes, that would also work. That would essentially mean reverting the changes to the automatic download flow. I'm ok with that if it makes this PR less controversial. It's the user-initiated flow that's much more important for the support requests as that's the one where people are trying to download a database and it isn't behaving as expected.

[robertbrignull]

1. If there is no session, try downloading without an access token

Oh I think I misunderstood as for some reason I thought you meant only the automatic download flow. I'm happy to leave that as it is to make this PR smaller.

For the manual download flow I don't think I agree as adding extra steps to try unauthenticated and then retry adds quite a lot of complexity. In the case of a private repo it also makes the process longer. I wouldn't say there's much downside to prompting the user to log in when they've just asked to download a database so it's not worth making the feature more complex.