feat: adds keyring credentials provider for accounts #1483
Conversation
tdstein
requested review from
dotNomad,
kgartland-rstudio,
mmarchetti and
sagerb
as code owners
| r.Handle(ToPath("credentials"), GetCredentialsHandlerFunc(log)). | ||
| Methods(http.MethodGet) | ||
|
|
There was a problem hiding this comment.
I'll probably delete this. But, you can call it to see the list of credentials on the keyring.
tdstein
force-pushed
the
tdstein/credentials
branch
from
9e3e268 to
3e534f3
Compare
tdstein
force-pushed
the
tdstein/credentials
branch
4 times, most recently
from
9a9a54a to
bf2d11f
Compare
|
@kgartland-rstudio - I would like to get this tested on Windows before I merge. I need to verify that go-keyring works as expected. Adding a credential using the VSCode UI will exercise the necessary codepaths. |
tdstein
force-pushed
the
tdstein/credentials
branch
3 times, most recently
from
21d328d to
3ed4c94
Compare
| apiKey: string; | ||
| }; | ||
|
|
||
| export type Credentials = Map<string, Credential>; |
There was a problem hiding this comment.
Looks like this isn't used.
| // 200 - success | ||
| // 400 - bad request | ||
| // 500 - internal server error | ||
| createOrUpdate(cred: Credential) { |
There was a problem hiding this comment.
Nit: a lot of our other resources take parameters rather than a single object. I think a single object is totally fine here since they are all required, and past 3 parameters its much clearer to use an object. Just something I noticed
| @@ -83,28 +83,142 @@ export class CredentialsTreeDataProvider | |||
| isEmpty ? "empty" : "notEmpty", | |||
| ); | |||
| } | |||
|
|
|||
| public add = async () => { | |||
| const name = await getServerNameFromInputBox(); | |||
There was a problem hiding this comment.
Calling each of these in succession is effectively a multi step input. We have several multiStepInputs that also have the validate function examples rather than using notifications.
There was a problem hiding this comment.
Since this works I'd be fine with both of these (multistep input, and validation) being follow-ups
There was a problem hiding this comment.
Sounds good. I wasn't sure if it was needed at the outset, but now that this is complete, it makes sense to use the abstraction.
| export class CredentialsTreeItem extends TreeItem { | ||
| contextValue = "posit.publisher.credentials.tree.item"; | ||
|
|
||
| constructor(account: Account) { | ||
| super(account.name); | ||
| this.tooltip = this.getTooltip(account); | ||
| this.iconPath = new ThemeIcon("key"); | ||
| this.description = `${account.url}`; |
tdstein
force-pushed
the
tdstein/credentials
branch
2 times, most recently
from
a63b8e2 to
9e29e6e
Compare
| input = input.trim(); | ||
| if (input.trim() === "") { | ||
| return { | ||
| message: "Oops! It seems like your forgot something.", |
There was a problem hiding this comment.
Honestly I really like playful messages like this in software.
Co-authored-by: Jordan Jensen <[email protected]>
tdstein
force-pushed
the
tdstein/credentials
branch
from
a966593 to
3160925
Compare
Intent
Adds support for adding credentials via the VSCode credentials pane.
Type of Change
Approach
A new API endpoint (POST /credentials) accepts an API Key credential. No other authentication types are supported at this time. This endpoint uses the go-keyring library to write to the OS specific credentials store. This is the "Keychain Access" application on macOS.
A '+' button is added to the credentials pane, which triggers a multi-step form accepting a name, URL, and API Key. This then POST to the aforementioned endpoint.
A new "Account Provider" is added on the backend, which reads the credentials and then maps them to the existing "Account" structure.
Automated Tests
I added a few backend tests.
Directions for Reviewers
Try it out. I was able to deploy successfully using an API key on the keyring.
We should test on Windows and LInux to verify that the go-keyring library works as advertised.
Checklist