Change database download to always be authenticated

[robertbrignull]

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

I have applied this change in behaviour to user-initiated command (either from the UI or command palette) as well as to the automatic behaviour whenever the extension starts up. Currently there's no way to avoid logging in, but if there's extreme backlash we could implement an opt-out config option.

The existing behaviour of the extension was a bit complicated:

• When using the user-initiated flow, 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.
• For the automatic flow on startup, if would use a token if one was available but otherwise it would make an unauthenticated request. If the unauthenticated request failed it would ask if you wanted to log in and try again.

The new behaviour is overall simpler, though I've tried to keep the style of the automated flow around informing the user of the operation outcome. I think the best way to understand the behaviour change is through flowchats where I've tried to capture all of the possible options:

Old behaviour for user-initiated download:

New behaviour for user-initiated download:

Old behaviour for automatic download:

New behaviour for automatic 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.

[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).