Skip to content

Configstore#40

Merged
Caledrith merged 5 commits intomasterfrom
configstore
Jun 26, 2020
Merged

Configstore#40
Caledrith merged 5 commits intomasterfrom
configstore

Conversation

@Caledrith
Copy link
Collaborator

replacement of keytar with configstore

@Caledrith Caledrith requested a review from jmango22 June 23, 2020 10:29
: (['domoapps']);

return Promise.all([
keytar.getPassword(`domoapps-oauth-access-${allScopes.join('-')}`, `${instance}-${proxyId}`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't these just return the same thing now? Before it was the OAuth access token and OAuth refresh token. Now there doesn't seem like a unique way to distinguish between them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've resolved this. However, I'm just realizing that there's a "scopes" part of the service itself (what it's named in the credential manager). What function does that serve? I've only used the domoapps scope, but I'm curious without keytar if I need to account for another scope or if it can just be saved as is

Copy link
Contributor

@jmango22 jmango22 Jun 26, 2020

Choose a reason for hiding this comment

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

Good catch. When you request a token and refresh token they are tied to the scopes you have requested. We don't want a token with the wrong scopes returned, because if we have the wrong scopes some requests will be blocked. We need a way store different tokens for the same instance but with different scopes. Let me know if this needs further clarification.

Copy link
Contributor

@jmango22 jmango22 left a comment

Choose a reason for hiding this comment

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

Looks good!

@Caledrith Caledrith merged commit c80afe3 into master Jun 26, 2020
@Caledrith Caledrith deleted the configstore branch June 26, 2020 16:52
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