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

Adds support for accessToken in config. #3

Closed
wants to merge 2 commits into from

Conversation

mbleigh
Copy link

@mbleigh mbleigh commented Jun 24, 2016

There are situations where an application will already have an access token (see issue). In my case, I have a command-line client that uses an OAuth flow to gain credentials instead of a service account.

This PR adds support for config.accessToken which, if specified, will short-circuit the more complex behavior and simply return the token provided. This will allow gcloud to support access token based authentication.

@@ -0,0 +1 @@
node_modules
Copy link
Owner

Choose a reason for hiding this comment

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

This should just be in your global .gitignore (https://help.github.com/articles/ignoring-files).

Copy link
Author

Choose a reason for hiding this comment

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

Happy to remove this and not start a nitpick battle, but I think it's a good practice for an open source project to maintain its own .gitignore for files that are generated in the course of development on it. 😄

Consider it gone.

@stephenplusplus
Copy link
Owner

Thank you for this, but I'm not sure I like having this here, since someone who has an access token wouldn't be using this library. Let's just find a place for it to go in gcloud-node, where it won't use this module at all if a token is provided.

@mbleigh
Copy link
Author

mbleigh commented Jun 24, 2016

@stephenplusplus that's what I thought at first too, but authorizeRequest is a sugar layer that is theoretically useful even if you're not otherwise using auto auth. This is the cleanest place to add the change, doing it in gcloud will mean essentially duplicating authorizeRequest and adding a bunch of unfortunate forking code paths.

If you're not convinced, I can certainly go down the path of adding it directly to gcloud.

@stephenplusplus
Copy link
Owner

I see your point, however I think this module should just solve the "automatic auth" part. Thinking outside the scope of gcloud-node, the utility function to decorate the request is quite small, I would recommend a user just write it instead of pulling down this library:

Object.assign(reqOpts.headers, { Authorization: 'Bearer {token}' });

Also, some functionality would be lost, such as #getAuthClient. This is going to complicate some of gcloud-node's code that relies on having a google-auth-library auth client instance: https://github.com/GoogleCloudPlatform/gcloud-node/search?p=1&q=authclient&utf8=%E2%9C%93

Our APIs that use gRPC (Datastore, Logging, Pub/Sub) require that auth client. I'm not sure if gRPC has a way to simply provide a token.

Then there are other libraries gcloud-node uses-- gce-images & gcs-resumable-upload-- that would need to be updated to accept just an access token. That shouldn't be a big deal.

@mbleigh
Copy link
Author

mbleigh commented Jun 24, 2016

I guess that's more or less exactly why I wanted to make the change here -- now we're talking about making updates to 3+ separate libraries with unknown ramifications for gRPC calls and so on and so forth. The grpc code seems to maybe just have a similar token authentication to REST, but it's wrapped in so many layers of abstraction it's hard to tell.

I'm open to suggestions and willing to do some work, but this was intended to be a quick couple-hour PR and it's sounding more and more like a multi-day effort. 😞 We're probably just going to have to use REST directly and skip gcloud for the moment.

@stephenplusplus
Copy link
Owner

Yeah, complicated indeed. Thanks for helping out so far. If time allows, feel free to keep digging and share your findings. I'll do the same.

@kwent
Copy link

kwent commented Mar 14, 2018

@mbleigh @stephenplusplus A PR has been sitting here since June 24, 2016. How can we help to make this happen ? Looks like a lot of us need this.

@stephenplusplus
Copy link
Owner

I started a new PR for this: #48.

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.

3 participants