Use stdin for supplying auth to CodeQL#1964
Conversation
This will supply the GitHub access token to certain CodeQL CLI commands such that private packages can be resolved. It will only do so if the user has an existing auth session. If they don't, they will now get a prompt to login. However, this will only happen for commands which actually use authentication, which is limited to packaging commands.
| const accessToken = await credentials.getExistingAccessToken(); | ||
|
|
||
| const extraArgs = accessToken ? ["--github-auth-stdin"] : []; |
There was a problem hiding this comment.
Why get the accessToken here at all if it's not going to be used below? Should it be the case that if we have an accessToken already, then we use it below and don't prompt?
There was a problem hiding this comment.
Though, I wonder if this will be a poor user experience. Most users will only be downloading public packages. They shouldn't need to log in for that.
Do you think it would be better if:
- Try to get an existing token.
- If found, use that.
- If not found, use no token (as before)
- If we get an error because the package is private (and no token was provided) prompt the user to log in.
- If we get an error because the package is private (and a token was provided) clearly explain that this token doesn't have proper permissions so the user can re-authenticate with a different token.
There was a problem hiding this comment.
The only reason for getting the access token here is to check whether the user has an existing access token. Essentially, we're already doing steps 1-3. I think steps 4 and 5 can be implemented separately in the future if private packages are used more widely.
We're not using the access token retrieved here because there can be quite some time between retrieving this access token and the command being run because the command can be queued. By re-retrieving the access token when the CLI actually asks for it, we'll always have an up-to-date access token and don't need to worry about the rare race condition of the user logging out/invalidating their access token while the command is in the queue.
There was a problem hiding this comment.
Ok. Thanks for the explanation. So, it looks like there are two race conditions that aren't handled:
- User logs in after invoking this command, but before the command is run. In this case, the extension will assume that the user is not logged in (this is fine, I think).
- User logs out after invoking this command, but before the command is run. In this case, the extension will assume the user is still logged in, try to get the credentials, but
credentials.getAccessToken()returns undefined. In this case, will the command hang? Or will it fail since the valueundefinedis passed to stdin as the string "undefined"?
This race condition is probably rare enough that it's not worth spending too much effort on and when it does happen, it would probably be obvious to the user what happened.
There was a problem hiding this comment.
credentials.getAccessToken() should never return undefined. The difference between getAccessToken and getExistingAccessToken is that getAccessToken will ask the user to login. If the user doesn't login, the promise will reject.
We weren't handling the rejection of the promise and this would permanently hang the CLI server, so I've now pushed a commit that handles this case by giving the accessToken that we retrieved previously. This could be an invalid access token, but should still allow the command to continue.
| progressReporter, | ||
| async (line) => { | ||
| if (line.startsWith("Enter value for --github-auth-stdin")) { | ||
| return credentials.getAccessToken(); |
There was a problem hiding this comment.
Is this what we want to do?
| return credentials.getAccessToken(); | |
| return accessToken; |
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Code looks ok to me. Code doing this kind of job of starting other processes is always quite complicated.
I'll let @aeisenberg complete his review and approve, but I'm happy whenever he's happy.
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Thanks for the explanation. I think it is good, but I would like to see an updated jsdoc before merging.
| const accessToken = await credentials.getExistingAccessToken(); | ||
|
|
||
| const extraArgs = accessToken ? ["--github-auth-stdin"] : []; |
There was a problem hiding this comment.
Ok. Thanks for the explanation. So, it looks like there are two race conditions that aren't handled:
- User logs in after invoking this command, but before the command is run. In this case, the extension will assume that the user is not logged in (this is fine, I think).
- User logs out after invoking this command, but before the command is run. In this case, the extension will assume the user is still logged in, try to get the credentials, but
credentials.getAccessToken()returns undefined. In this case, will the command hang? Or will it fail since the valueundefinedis passed to stdin as the string "undefined"?
This race condition is probably rare enough that it's not worth spending too much effort on and when it does happen, it would probably be obvious to the user what happened.
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Thanks for the changes. It makes a lot of sense now.
3 participants
This will supply the GitHub access token to certain CodeQL CLI commands such that private packages can be resolved. It will only do so if the user has an existing auth session. If they don't, they will now get a prompt to login. However, this will only happen for commands which actually use authentication, which is limited to packaging commands.
Checklist
ready-for-doc-reviewlabel there.