feat: Private Key Auth #159
Conversation
bmingles
force-pushed
the
32-private-key
branch
from
8e9c8dd
to
c114d7e
Compare
End-to-end Test Summary
Detailed Test Results
Failed Test SummaryNo failed tests ✨Flaky Test SummaryNo flaky tests detected. ✨ |
|
The code should be ready for review. I do plan to update the DH npm package versions to non-alpha ones once https://github.com/deephaven-ent/iris/pull/2386 has merged and npm packages have been published, but there shouldn't be any api changes from that. |
| const username = await vscode.window.showInputBox({ | ||
| placeHolder: 'Username', | ||
| prompt: 'Enter your Deephaven username', | ||
| const credentials = await runUserLoginWorkflow({ |
There was a problem hiding this comment.
One thing that's weird about this is that if we're already authenticated, it requests our username/password again. We should at least fill in the lastLogin here so we don't need to type in the user name again.
Ideally we'd use the same username, and just ask for the password again (like how other apps require re-authentication when performing security operations). It's somewhat odd having to enter the username again.
That being said, after using it - why do we have the user explicitly ask to generate a keypair? Is there any reason they'd want to create multiple key pairs? We're essentially generating this key pair so the user doesn't need to re-enter their password every time, correct? In which case, I'd expect the flow to be more like Login -> enter username/password, with an option to "Remember Me", rather than having an action "Generate DHE Keypair". Thoughts?
There was a problem hiding this comment.
Users could in theory create different keys for different usernames, so we would at least need to have a way for them to create a key for a different user. This could be facilitated by requiring logout and then login as the other user (related to your other comment of why we need to prompt user/password if there exists a key)
The remember me option is interesting depending on how much we want to expose to the user about the method of how things are stored. As-is, the user has to have some understanding they are opting for key pair encryption vs hiding that nuance. It does seem simpler that way but seems like something we'd want to run by Charles since it differs from current key pair usage.
There was a problem hiding this comment.
Assuming Charles is ok with hiding the private / key pair behind a "remember me" feature, I guess we could change the workflow to something like:
- Prompt for username (remember last used)
- If private key exists for selected user, use it. If not, prompt for password
- Somewhere along the way present option to "remember me" and generate keys if not exists, or delete them if they do and user opts out
There was a problem hiding this comment.
Also, if we didn't want to make such a change on this PR, I could at least default the keygen username to remember the last username. That was just an oversight on my part
There was a problem hiding this comment.
I fixed it to remember the username
| @@ -13,6 +14,9 @@ export class ByURLAsyncCache<TValue> | |||
| this._promiseMap = new URLMap<Promise<TValue>>(); | |||
| } | |||
|
|
|||
| private readonly _onDidInvalidate = new vscode.EventEmitter<URL>(); | |||
| readonly onDidInvalidate = this._onDidInvalidate.event; | |||
There was a problem hiding this comment.
I'm a little confused why you're adding this event/listener when it isn't being used anywhere (that I can see). Seems to be a pattern you do on a few services (e.g. DhService.onDidDisconnect). Any reason why?
There was a problem hiding this comment.
I had added this and then ended up swapping the instance using it for a SerializedKeyMap where I added the onDidChange. I just left it since it seemed something less to implement later if needed. I'm fine to remove it if you'd prefer though.
src/types/uiTypes.d.ts
Outdated
|
|
||
| export type SeparatorPickItem = { | ||
| label: string; | ||
| kind: vscode.QuickPickItemKind.Separator; | ||
| }; | ||
|
|
||
| export type AuthenticationMethodPickItem = |
There was a problem hiding this comment.
Why bother prompting the user if they already have a key? Why not just login automatically?
There was a problem hiding this comment.
Use case would be to be able to login as a different user that doesn't have a private key.
Co-authored-by: Mike Bender <mikebender@deephaven.io>
DHE server context right-click context menu actions
Login via keypair - If a keypair has been generated, clicking on a DHE server node will allow picking username / pwd or key login
Discard connection action - shows up as trashcan on connected server node. Will discard DHE connection + any worker connections
Secret storage