-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Persist Account keys on login #375
Conversation
@@ -12,18 +12,6 @@ const clientSecret = | |||
const REDIRECT_URI = `http://127.0.0.1`; | |||
|
|||
class OAuthClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will jsdoc this in another pr, this one is getting big
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the JSDoc PR can you add the annotation //@ts-check
to the top of the file? it'll turn on in-IDE static analysis and should help you generate good types.
@@ -12,18 +12,6 @@ const clientSecret = | |||
const REDIRECT_URI = `http://127.0.0.1`; | |||
|
|||
class OAuthClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the JSDoc PR can you add the annotation //@ts-check
to the top of the file? it'll turn on in-IDE static analysis and should help you generate good types.
this.server.on("listening", () => { | ||
this.port = this.server.address().port; | ||
this.server.emit("ready"); | ||
}); | ||
this.server.listen(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this.server.listening
set when you emit ready
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check was a workaround for that builtYargs.argv weirdness, so not strictly necessary but yes by the time i send "ready" it's already true
headers.append("Authorization", `Bearer ${accountKey}`); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on abstracting out something with an API like makeFaunaRequest
on the next draft of this PR? that'll let you centralize error handling and make these methods really short & similar.
Ticket(s): FE-5875
Problem
We aren't persisting account keys
Solution
Build basic handler to manage creds files
access_keys
orsecret_keys
file.profile
flag can be used to index the stored account keys.default
is otherwise used.Result
Basic persistence of creds
Made a simple list databases handler
fauna db list
that uses the stored account key. Will build that command and tests out later.Testing