Skip to content

Use credentials for database download in non-canary mode#3072

Merged
koesie10 merged 4 commits intomainfrom
koesie10/download-github-database-authentication
Nov 20, 2023
Merged

Use credentials for database download in non-canary mode#3072
koesie10 merged 4 commits intomainfrom
koesie10/download-github-database-authentication

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Nov 14, 2023

We want users to be able to download databases from private/internal repositories without using canary mode. This will change the prompt to ask for credentials in non-canary mode as well. This means there are now 2 different prompts:

  • This repository has an origin (GitHub) that may have one or more CodeQL databases. Connect to GitHub and download any existing databases?
    • Connect
    • No
    • Never
  • This repository has an origin (GitHub) that has Ruby and JavaScript CodeQL databases. Download any existing databases from GitHub?
    • Download
    • No
    • Never

When you click on "Connect" in the first notification, we'll prompt for credentials and then for the language (if necessary). In the second one, we'll immediately prompt for the language.

Based on #3071

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 force-pushed the koesie10/download-github-database branch from b190c35 to 636f8f1 Compare November 17, 2023 13:46
We want users to be able to download databases from private/internal
repositories without using canary mode. This will change the prompt to
ask for credentials in non-canary mode as well.
@koesie10 koesie10 force-pushed the koesie10/download-github-database-authentication branch from 8bd794e to 5d42cbc Compare November 17, 2023 13:48
@koesie10 koesie10 marked this pull request as ready for review November 20, 2023 08:46
@koesie10 koesie10 requested a review from a team as a code owner November 20, 2023 08:46
@koesie10 koesie10 requested a review from starcke November 20, 2023 08:46
Base automatically changed from koesie10/download-github-database to main November 20, 2023 10:08
Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

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

Mostly looks good, although it would be good to see if the logic could be made a bit simpler. I also wonder about public repos.

if (databases.length === 0) {
// If the user didn't have an access token, they have already been prompted,
// so we should give feedback.
if (!hasAccessToken) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the behavior is correct, but I found the logic a bit hard to follow. It also seems a bit odd that there is a database prompt (the one to connect) in here instead of in github-database-prompt.ts.

I also wonder, what if the repo is public - do we then ask for a non-needed token?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The prompt for connecting is defined in this file because it is common to both downloading and updating databases. This should make sense in #3079.

If the repo is public, we do ask for an unnecessary token. I can change this to always make a request first, and then only prompt if we get a 404. However, that will make this logic even harder to follow.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

However, that will make this logic even harder to follow.

That is probably right, although I am not super sure. I think what makes it complicated
is that we dont want to ask the user to connect and then ask them again to download the DB. If we asked
both questions then it could probably be written something like this:

databases = getDatabases(current_credentials)

if (databases.length === 0 && no_credentials) {
  // Ask the user for credentials and try again
  databases = getDatabases(ask_for_credentials)
}

downloadDatabaseFromGitHub(databases)

What do you think? How much do we want to prevent the second question?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think I've found a somewhat simpler solution which will make this easier to follow and also skip the prompt for credentials when the repo is public. I'll create a new function listDatabases which will:

  1. Try retrieving the databases, either with or without credentials depending on what is available
  2. If that fails with a 404 and they don't have an access token, ask the user whether they want to connect to GitHub
  3. If they do, prompt for credentials and retry the request

This function can then return a result like { databases: CodeqlDatabase[], promptedForCredentials: boolean }. The logic in the module will then be a little bit simpler since the name is promptedForCredentials instead of hasAccessToken, more closely matching what the intention is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That sounds good, thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've just pushed this change, the logic for that is mostly inside github-database-api.ts and the listDatabases function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks better and also covers the public repo case. Thanks!

Comment thread extensions/ql-vscode/src/databases/github-database-prompt.ts
Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

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

👍

@koesie10 koesie10 merged commit 01d24e0 into main Nov 20, 2023
@koesie10 koesie10 deleted the koesie10/download-github-database-authentication branch November 20, 2023 13:40
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.

2 participants