feat: add getCredentials method #180

bantini · 2017-11-09T05:43:20Z

Solves #169
Created getCredentials method to get credentials from the metadata server.

The details from JSON file is left for the user to implement.

-Added metadata return for GCP

-Added test case

…entials

…e-auth-library-nodejs into 169-create-get-credentials

coveralls · 2017-11-09T05:56:16Z

Coverage decreased (-0.4%) to 93.179% when pulling d04e52c on bantini:169-create-get-credentials into b39c16f on google:master.

bantini · 2017-11-09T05:58:02Z

@stephenplusplus Does it address your requirements?
@saicheems FYI.

stephenplusplus · 2017-11-09T16:07:22Z

Yes, thank you, this LGTM!

stephenplusplus · 2017-11-09T16:14:14Z

Maybe it's possible to have this method be more generic, so that it would return the JSON key file contents, if they exist, otherwise the metadata from GCP?

Also, do you know what the response from GCP looks like that this new method would return?

bantini · 2017-11-09T19:37:58Z

@stephenplusplus The reason I have not added the functionality to return the contents from the JSON is that the user can retrieve the content from the JSON file directly if they are not on GCP.

The response from the server looks like this:

        {
        "default":
        {
          "aliases":["default"],
          "email":"default@test-creds.iam.gserviceaccount.com",
          "scopes":["https://www.googleapis.com/auth/cloud-platform"]
        },
        "test-creds@test-creds.iam.gserviceaccount.com":
        {
          "aliases":["test-creds"],
          "email":"test-creds@test-creds.iam.gserviceaccount.com",
          "scopes":["https://www.googleapis.com/auth/cloud-platform"]}
        }

stephenplusplus · 2017-11-09T20:06:34Z

The point of the method that I'm looking for is basically: "Get the user's client_email and private_key". If the private key isn't available, just the email is fine. If they are both unavailable, that would be an error.

That way, instead of each application writing:

if GCP
  - call metadata server
  - detect email from response
else
  - read JSON file
  - parse out client_email and private_key

They only have to write:

authClient.getCredentials(function(err, credentials) {
  console.log(credentials.client_email)
})

lukesneeringer · 2017-11-14T17:10:39Z

@bantini Would that work for you?

jasonpolites · 2017-11-14T18:17:47Z

@stephenplusplus Are we sure we want to make the PK accessible? For use cases like generating a signed URL you don't actually need the keyfile as you can defer to the signBlob API.

@danoscarmike

stephenplusplus · 2017-11-14T18:22:24Z

Either way is fine with me. I would lean towards returning as much as possible, otherwise we are basically just creating a "getClientEmail()" method (which is also fine with me).

…ni/google-auth-library-nodejs into 169-create-get-credentials

bantini · 2017-11-14T18:59:12Z

Test case for checking the environment variable and returning an error if it is not set is failing due to a time-out.

saicheems · 2017-11-16T01:15:55Z

Nilayan, I experimented in another PR to fix the build:

From Cannot read property '0' of undefined npm/npm#17858 (comment)

Run
```
npx npmc install
rm package-lock.json
npx npmc install
```
Recommit package-lock.json.
You'll have to do one of the following:

In .travis.yml delete the cache item.
```
-cache:
-  directories:
-  - node_modules/
-
```
^ the cached modules are corrupted and causing npm install to fail.

The alternative requires admin access to Travis. You'd have to clear the build cache for this PR. I think that's an option accessible from More options in the Travis interface.

bantini · 2017-11-16T01:26:06Z

@saicheems The changes worked. Thanks!

lukesneeringer · 2017-11-16T18:45:45Z

@bantini Can this be merged?

bantini · 2017-11-16T19:17:50Z

@lukesneeringer Yes. The tests are all passing. And it seems @stephenplusplus got it working in his code.

stephenplusplus · 2017-11-17T15:09:18Z

@bantini can you merge, or do we need someone else?

bantini · 2017-11-17T15:20:44Z

@stephenplusplus I don't have permission to merge.

stephenplusplus · 2017-11-17T15:26:10Z

@ofrobots ?

src/auth/googleauth.ts

+  // To save the contents of the JSON credential file
+  private _jsonContent: JWTInput;
+
+  get jsonContent(): JWTInput {


src/auth/googleauth.ts

+              } else {
+                // Callback with the body
+                const credential:
+                    CredentialBody = {client_email: body['default']['email']};


src/auth/googleauth.ts

+                }
+              }
+            });
+      } else if (this.jsonContent) {


src/auth/googleauth.ts

+      } else if (this.jsonContent) {
+        const credential: CredentialBody = {
+          client_email: this.jsonContent.client_email,
+          private_key: this.jsonContent.private_key


src/auth/googleauth.ts

+        }
+      } else {
+        if (callback) {
+          callback(new Error('Could not find credential file.'), undefined);


src/auth/googleauth.ts

+        if (callback) {
+          callback(null, credential);
+        }
+      } else if (err) {


ofrobots

Looking good. Left a few more comments.

src/auth/googleauth.ts

+          if (callback) {
+            callback(err, undefined);
+          }
+        } else if (gce) {


src/auth/googleauth.ts

+
+
+  /**
+   * Returns the contents of the metadata of GC instance


src/auth/googleauth.ts

+              },
+              (err, body, res) => {
+                if (err || !res || res.statusCode !== 200 || !body ||
+                    !body.default || !body.default.email) {


ofrobots

Almost there.

src/auth/googleauth.ts

+   * If it doesn't exist, and the environment is on GCE, it gets the
+   * client_email from the cloud metadata server.
+   * @param {function} callback Callback.
+   * @return object representation of the service account JSON


ofrobots

LGTM, go ahead and land once you have addressed the two minor nits.

Note that I changed the PR title to follow conventionalcommits.org. That helps figuring out the semver-ness of commits going forward.

src/auth/googleauth.ts

-          if (callback) {
-            callback(err, undefined);
-          }
+          callback(err, undefined);


src/auth/googleauth.ts

-                    if (callback) {
-                      callback(null, credential);
-                    }
+          callback(new Error('Unknown error.'), undefined);


danoscarmike · 2017-11-20T18:03:30Z

@bantini is this one good to merge? And this fixes #169 still, correct?

bantini · 2017-11-20T18:17:26Z

@danoscarmike Yes. It fixes #169.

stephenplusplus · 2017-11-20T18:17:46Z

Thanks for this @bantini! Would you let me know when the release is out?

bantini · 2017-11-20T18:25:16Z

@stephenplusplus I have no information about the release process. But I will surely let you know if I hear something.

stephenplusplus · 2017-11-20T18:30:13Z

@ofrobots do you know of someone we could ping to send a release?

ofrobots · 2017-11-20T18:46:07Z

I can do it. Need a bit of time to move #173 off to a separate branch for 1.0.

stephenplusplus · 2017-11-20T20:56:12Z

Cool, thanks!

ofrobots · 2017-11-20T21:14:25Z

google-auth-library 0.12.0 published 🎉

bantini and others added 12 commits October 20, 2017 10:07

Method to get credentials

a0b2a2b

-Removed local service_account param return

caf2967

-Added metadata return for GCP

-Added method signature

b41c5aa

-Added test case

Encapsulated withing checkGCE

0fc0cfb

Merge remote-tracking branch 'google/master' into 169-create-get-cred…

13044c1

…entials

Removed test and simplified method

6c78413

Merge branch '169-create-get-credentials' of github.com:bantini/googl…

01082f9

…e-auth-library-nodejs into 169-create-get-credentials

Using method instead of variable

0d5ae83

Introduced callback

f724370

Commit with test cases

9717944

Removed console.log

17365fa

Removed commented method

6d57c4f

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 9, 2017

bantini added 2 commits November 8, 2017 21:50

Merge branch 'master' into 169-create-get-credentials

80cb4e6

Merge branch 'master' into 169-create-get-credentials

d04e52c

bantini requested a review from ofrobots November 9, 2017 05:57

ofrobots requested a review from lukesneeringer November 9, 2017 23:09

stephenplusplus mentioned this pull request Nov 13, 2017

Expose a getCredentials method #169

Closed

bantini added 2 commits November 14, 2017 10:32

Returning client email and private key

2462961

Merge branch '169-create-get-credentials' of https://github.com/banti…

5eda92f

…ni/google-auth-library-nodejs into 169-create-get-credentials

stephenplusplus mentioned this pull request Nov 16, 2017

Use signBlob API for getSignedPolicy() and getSignedUrl() googleapis/nodejs-storage#91

Merged

3 tasks

Removed cache and added package-lock.json

5e4b149

stephenplusplus mentioned this pull request Nov 17, 2017

Bump version to 0.14.3 googleapis/gax-nodejs#156

Closed

ofrobots reviewed Nov 17, 2017

View reviewed changes

Comments fixes

6704a16

ofrobots changed the title ~~169 create get credentials~~ feat: add getCredentials method Nov 17, 2017

ofrobots reviewed Nov 17, 2017

View reviewed changes

Check for non-gce first and added additional tests

671c462

JustinBeckwith added this to the 1.0 milestone Nov 19, 2017

ofrobots reviewed Nov 19, 2017

View reviewed changes

JSdoc changes and making callback mandatory

fa381c8

ofrobots approved these changes Nov 20, 2017

View reviewed changes

Removed undefined for credentials from return body for error

cf0056a

bantini merged commit 8eddbc9 into googleapis:master Nov 20, 2017

bantini deleted the 169-create-get-credentials branch November 20, 2017 18:17

feat: add getCredentials method #180

feat: add getCredentials method #180

Conversation

bantini commented Nov 9, 2017

coveralls commented Nov 9, 2017 • edited Loading

bantini commented Nov 9, 2017

stephenplusplus commented Nov 9, 2017

stephenplusplus commented Nov 9, 2017

bantini commented Nov 9, 2017

stephenplusplus commented Nov 9, 2017

lukesneeringer commented Nov 14, 2017

jasonpolites commented Nov 14, 2017 • edited Loading

stephenplusplus commented Nov 14, 2017

bantini commented Nov 14, 2017

saicheems commented Nov 16, 2017 • edited Loading

bantini commented Nov 16, 2017

lukesneeringer commented Nov 16, 2017

bantini commented Nov 16, 2017

stephenplusplus commented Nov 17, 2017

bantini commented Nov 17, 2017

stephenplusplus commented Nov 17, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

ofrobots left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

danoscarmike commented Nov 20, 2017

bantini commented Nov 20, 2017

stephenplusplus commented Nov 20, 2017

bantini commented Nov 20, 2017

stephenplusplus commented Nov 20, 2017

ofrobots commented Nov 20, 2017

stephenplusplus commented Nov 20, 2017

ofrobots commented Nov 20, 2017

coveralls commented Nov 9, 2017 •

edited

Loading

jasonpolites commented Nov 14, 2017 •

edited

Loading

saicheems commented Nov 16, 2017 •

edited

Loading