Skip to content
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

Allow user to log into staging and get a new token #FF #8

Merged
merged 4 commits into from
Sep 6, 2019

Conversation

pepopowitz
Copy link
Contributor

This PR adds the ability to log into stagingapi.artsy.net. The new token is spit out to the console.

Usage

./bin/run login

Console in/out looks like this:

image

Caveats

  • It's currently only working for staging. There is an item in the backlog for being able to choose staging or prod.
  • It requires a client_id and client_secret set as environment variables. I've added .env to .gitignore so that you can store them in there. For now, I'm borrowing force's id and secret locally. I added a backlog item to get a proper client id and secret for this app.
  • No tests included. They should be. I just haven't gotten comfortable writing them against oclif yet.
  • I'm going to add a card for "token is automatically copied to clipboard for user". With all the output from oclif, it isn't reasonable to pipe to pbcopy. That isn't really a cross-platform solution anyway.

@@ -6,10 +6,30 @@ class Gravity {
production: "api.artsy.net",
}

async getAccessToken(credentials: Credentials) {
const gravityUrl = this.url("oauth2/access_token")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joeyAghion this seems like a reasonable way to authenticate, but let me know if there is a method that makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have to dig... is this where Force posts credentials to obtain an access token today?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is. And I imagine this cli is not tight to Force, as long as it's a valid access token generated by Gravity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - any valid id/secret will do.

async get(endpoint: string) {
const token: string = process.env.TOKEN! // temp until our auth/token plumbing is hooked up

const gravityUrl: string = this.url(endpoint)
const gravityUrl: string = this.url(`api/v1/${endpoint}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do a little bit of shuffling around of URLs, because the auth endpoint isn't under api/v1....

@pepopowitz pepopowitz added the ff Future Fursday/Friday label Aug 12, 2019
Copy link
Contributor

@joeyAghion joeyAghion left a comment

Choose a reason for hiding this comment

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

Rather than "automatically copying to clipboard," it would be great if this cli allowed itself to be composed with other command line utilities according to the unix philosophy of small, sharp tools. E.g., if run non-interactively, it could just write the access token to stdout (and any errors or other informational content to stderr). When run interactively, it could include the helpful ...Your access token... content. This would allow simple composition of this and other commands like ... login | pbcopy.

In Ruby, this can be accomplished by testing for STDOUT.tty? but I'm sure there are TS analogs.

@@ -6,10 +6,30 @@ class Gravity {
production: "api.artsy.net",
}

async getAccessToken(credentials: Credentials) {
const gravityUrl = this.url("oauth2/access_token")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have to dig... is this where Force posts credentials to obtain an access token today?

Copy link
Member

@jonallured jonallured left a comment

Choose a reason for hiding this comment

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

This looks great! FWIW, I agree with @joeyAghion that it would be great for this command to be composed with something like pbcopy rather than automagically doing that. Seems more portable to me that way too.

@pepopowitz
Copy link
Contributor Author

I'll modify this to allow piping to pbcopy before I'd consider this done. Thanks for the feedback!

pepopowitz and others added 4 commits September 6, 2019 08:39
This is nice because it means you can compose this command with pbcopy
or use it in a shell script or whatever.
@jonallured
Copy link
Member

Ok I cloned down this PR, rebased on top of master and then snipped out the extra log statements with c3dab16 so going to merge this. Hope that's ok @pepopowitz!! 🤗

@jonallured jonallured merged commit b726b02 into artsy:master Sep 6, 2019
@artsyit
Copy link
Contributor

artsyit commented Sep 6, 2019

🚀 PR was released in v0.0.3 🚀

@artsyit
Copy link
Contributor

artsyit commented Sep 6, 2019

🚀 PR was released in v0.0.3 🚀

2 similar comments
@artsyit
Copy link
Contributor

artsyit commented Sep 6, 2019

🚀 PR was released in v0.0.3 🚀

@artsyit
Copy link
Contributor

artsyit commented Sep 6, 2019

🚀 PR was released in v0.0.3 🚀

@pepopowitz
Copy link
Contributor Author

Thanks for moving this forward, @jonallured!

@pepopowitz pepopowitz deleted the auth branch September 6, 2019 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ff Future Fursday/Friday released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants