feat(samples): auth samples #1444
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Here is the summary of changes. You are about to add 6 region tags.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
size: l
piaxc
suggested changes
Aug 24, 2022
| * Uses a service account (SA1) to impersonate as another service account (SA2) and obtain id token for the impersonated account. | ||
| * To obtain token for SA2, SA1 should have the "roles/iam.serviceAccountTokenCreator" permission on SA2. | ||
| * | ||
| * @param {string} scope - The scope that you might need to request to access Google APIs, |
There was a problem hiding this comment.
This is all outside of the region tags. Wouldn't we want users to be able to see it from the docs?
| * @param {string} url - The url or target audience to obtain the ID token for. | ||
| */ | ||
| function main(url) { | ||
| // [START auth_cloud_idtoken_metadata_server] |
There was a problem hiding this comment.
Again, please include the comments in the region tag
Sita04
reviewed
Aug 25, 2022
Sita04
reviewed
Aug 25, 2022
| * @param {string} targetAudience - The url or target audience to obtain the ID token for. | ||
| */ | ||
| function main(targetAudience, jsonCredentialsPath) { | ||
| // [START auth_cloud_idtoken_service_account] |
There was a problem hiding this comment.
Can we move the region tag to line 14 to include the comment above?
Sita04
reviewed
Aug 25, 2022
| * logical identifier of an API service, such as "iap.googleapis.com". | ||
| */ | ||
| function main(idToken, expectedAudience) { | ||
| // [START auth_cloud_verify_google_idtoken] |
FrodoTheTrue
force-pushed
the
add-auth-samples
branch
from
b9c6e70
to
9e05ef5
Compare
|
Hi @piaxc @Sita04, thanks for the review, just sent the updates Regarding moving comments for variables - unfortunately everywhere in js (and in Go) samples we have this structure, so user can jump directly to the github code if it is really needed. I moved important comments to the region tags, but for variables I prefer to keep the structure consistent with other languages. |
|
@bcoe could you please help with reviewing this from node.js side? |
bcoe
added
the
do not merge
danielbankhead
requested changes
Sep 14, 2022
|
Please use the */external/* links as currently written; they are used to
allow for future updates of that documentation.
…On Tue, Sep 13, 2022 at 7:58 PM Daniel Bankhead ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In samples/authenticateExplicit.js
<#1444 (comment)>
:
> +// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * Lists storage buckets by authenticating with ADC.
+ */
+function main() {
+ // [START auth_cloud_explicit_adc]
+ /**
+ * TODO(developer):
+ * 1. Set up ADC as described in https://cloud.google.com/docs/authentication/external/set-up-adc
Should this link be
https://cloud.google.com/docs/authentication/provide-credentials-adc?
⬇️ Suggested change
- * 1. Set up ADC as described in https://cloud.google.com/docs/authentication/external/set-up-adc
+ * 1. Set up ADC as described in https://cloud.google.com/docs/authentication/provide-credentials-adc
------------------------------
In samples/authenticateExplicit.js
<#1444 (comment)>
:
> +//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * Lists storage buckets by authenticating with ADC.
+ */
+function main() {
+ // [START auth_cloud_explicit_adc]
+ /**
+ * TODO(developer):
+ * 1. Set up ADC as described in https://cloud.google.com/docs/authentication/external/set-up-adc
+ * 2. Make sure you have the necessary permission to list storage buckets "storage.buckets.list"
A link would help here. E.g.
https://cloud.google.com/storage/docs/access-control/iam-permissions#bucket_permissions
------------------------------
In samples/authenticateExplicit.js
<#1444 (comment)>
:
> +
+ const storageOptions = {
+ projectId,
+ authClient: credential,
+ };
+
+ // Construct the Storage client.
+ const storage = new Storage(storageOptions);
+ const [buckets] = await storage.getBuckets();
+ console.log('Buckets:');
+
+ for (const bucket of buckets) {
+ console.log(bucket.name);
+ }
+
+ console.log('Listed all storage buckets.');
I don't think this line is helpful outside of the test
------------------------------
In samples/authenticateExplicit.js
<#1444 (comment)>
:
> + // const googleAuth = new GoogleAuth({ scopes: scope });
+ // For more information on scopes to use,
+ // see: https://developers.google.com/identity/protocols/oauth2/scopes
+
+ const storageOptions = {
+ projectId,
+ authClient: credential,
+ };
+
+ // Construct the Storage client.
+ const storage = new Storage(storageOptions);
+ const [buckets] = await storage.getBuckets();
+ console.log('Buckets:');
+
+ for (const bucket of buckets) {
+ console.log(bucket.name);
We can make the output easier to read.
⬇️ Suggested change
- console.log(bucket.name);
+ console.log(`- ${bucket.name}`);
------------------------------
In samples/authenticateImplicitWithAdc.js
<#1444 (comment)>
:
> +// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * Shows credentials auto-detections in the intercation with GCP libraries
+ *
+ * @param {string} projectId - Project ID or project number of the Cloud project you want to use.
+ */
+function main(projectId) {
+ // [START auth_cloud_implicit_adc]
+ /**
+ * TODO(developer):
+ * 1. Uncomment and replace these variables before running the sample.
+ * 2. Set up ADC as described in https://cloud.google.com/docs/authentication/external/set-up-adc
⬇️ Suggested change
- * 2. Set up ADC as described in https://cloud.google.com/docs/authentication/external/set-up-adc
+ * 2. Set up ADC as described in https://cloud.google.com/docs/authentication/provide-credentials-adc?
------------------------------
In samples/authenticateImplicitWithAdc.js
<#1444 (comment)>
:
> +/**
+ * Shows credentials auto-detections in the intercation with GCP libraries
+ *
+ * @param {string} projectId - Project ID or project number of the Cloud project you want to use.
+ */
+function main(projectId) {
+ // [START auth_cloud_implicit_adc]
+ /**
+ * TODO(developer):
+ * 1. Uncomment and replace these variables before running the sample.
+ * 2. Set up ADC as described in https://cloud.google.com/docs/authentication/external/set-up-adc
+ * 3. Make sure that the user account or service account that you are using
+ * has the required permissions. For this sample, you must have "compute.instances.list".
+ */
+ // const projectId = 'YOUR_PROJECT_ID';
+ // const zone = 'us-central1-a';
Unused
⬇️ Suggested change
- // const zone = 'us-central1-a';
------------------------------
In samples/authenticateImplicitWithAdc.js
<#1444 (comment)>
:
> +
+ const {Storage} = ***@***.***/storage');
+
+ async function authenticateImplicitWithAdc() {
+ // This snippet demonstrates how to list buckets.
+ // NOTE: Replace the client created below with the client required for your application.
+ // Note that the credentials are not specified when constructing the client.
+ // The client library finds your credentials using ADC.
+ const storage = new Storage({
+ projectId,
+ });
+ const [buckets] = await storage.getBuckets();
+ console.log('Buckets:');
+
+ for (const bucket of buckets) {
+ console.log(bucket.name);
⬇️ Suggested change
- console.log(bucket.name);
+ console.log(`- ${bucket.name}`);
------------------------------
In samples/authenticateImplicitWithAdc.js
<#1444 (comment)>
:
> + async function authenticateImplicitWithAdc() {
+ // This snippet demonstrates how to list buckets.
+ // NOTE: Replace the client created below with the client required for your application.
+ // Note that the credentials are not specified when constructing the client.
+ // The client library finds your credentials using ADC.
+ const storage = new Storage({
+ projectId,
+ });
+ const [buckets] = await storage.getBuckets();
+ console.log('Buckets:');
+
+ for (const bucket of buckets) {
+ console.log(bucket.name);
+ }
+
+ console.log('Listed all storage buckets.');
⬇️ Suggested change
- console.log('Listed all storage buckets.');
------------------------------
In samples/authenticateImplicitWithAdc.js
<#1444 (comment)>
:
> + // NOTE: Replace the client created below with the client required for your application.
+ // Note that the credentials are not specified when constructing the client.
+ // The client library finds your credentials using ADC.
⬇️ Suggested change
- // NOTE: Replace the client created below with the client required for your application.
- // Note that the credentials are not specified when constructing the client.
- // The client library finds your credentials using ADC.
------------------------------
In samples/authenticateImplicitWithAdc.js
<#1444 (comment)>
:
> + * 3. Make sure that the user account or service account that you are using
+ * has the required permissions. For this sample, you must have "compute.instances.list".
Inaccurate line, should be something like Make sure you have the
necessary permission to list storage buckets "storage.buckets.list" along
with a helpful link
https://cloud.google.com/storage/docs/access-control/iam-permissions#bucket_permissions
------------------------------
In samples/idTokenFromImpersonatedCredentials.js
<#1444 (comment)>
:
> + // Create the impersonated credential.
+ const impersonatedCredentials = new Impersonated({
+ sourceClient: credential,
+ delegates,
+ targetPrincipal: impersonatedServiceAccount,
+ targetScopes: [scope],
+ lifetime: 300,
+ });
+
+ // Get the ID token.
+ // Once you've obtained the ID token, you can use it to make an authenticated call
+ // to the target audience.
+ await impersonatedCredentials.fetchIdToken(targetAudience, {
+ includeEmail: true,
+ });
+ console.log('Generated ID token.');
⬇️ Suggested change
- console.log('Generated ID token.');
------------------------------
In samples/idTokenFromImpersonatedCredentials.js
<#1444 (comment)>
:
> + await impersonatedCredentials.fetchIdToken(targetAudience, {
+ includeEmail: true,
+ });
It would be helpful to do something with this result
------------------------------
In samples/idTokenFromImpersonatedCredentials.js
<#1444 (comment)>
:
> + // delegates: The chained list of delegates required to grant the final accessToken.
+ // For more information, see:
+ // https://cloud.google.com/iam/docs/create-short-lived-credentials-direct#sa-credentials-permissions
+ // Delegate is NOT USED here.
+ const delegates = [];
Maybe leave this out since it's not used?
------------------------------
In samples/idTokenFromMetadataServer.js
<#1444 (comment)>
:
> + await client.fetchIdToken(url);
+ console.log('Generated ID token.');
It would be more helpful to do something with this token rather than to
log this message
------------------------------
In samples/idTokenFromServiceAccount.js
<#1444 (comment)>
:
> + // Get the ID token.
+ // Once you've obtained the ID token, use it to make an authenticated call
+ // to the target audience.
+ await client.fetchIdToken(targetAudience);
+ console.log('Generated ID token.');
It would be more helpful to do something with this token rather than to
log this message
------------------------------
In samples/test/auth.test.js
<#1444 (comment)>
:
> +const execSync = (cmd, opts) => {
+ return cp.execSync(cmd, Object.assign({encoding: 'utf-8'}, opts));
+};
execFile or execFileSync would be cleaner rather than dealing with
spacing, escaping, etc. (I'm assuming this was copied from tests from a few
years back). See:
-
https://nodejs.org/api/child_process.html#child_processexecfilesyncfile-args-options
------------------------------
In samples/verifyGoogleIdToken.js
<#1444 (comment)>
:
> + * so verifying ID tokens before making Google API calls is usually unnecessary.
+ *
+ * @param {string} idToken - The Google ID token to verify.
+ * and use IAM to narrow the permissions: https://cloud.google.com/docs/authentication#authorization_for_services
+ * @param {string} expectedAudience - The service name for which the id token is requested. Service name refers to the
+ * logical identifier of an API service, such as "iap.googleapis.com".
+ */
+function main(idToken, expectedAudience) {
+ // [START auth_cloud_verify_google_idtoken]
+ /**
+ * TODO(developer):
+ * 1. Uncomment and replace these variables before running the sample.
+ */
+ // const idToken = 'id-token';
+ // const targetAudience = 'pubsub.googleapis.com';
+ // const jwkUrk = 'https://www.googleapis.com/oauth2/v3/certs';
⬇️ Suggested change
- // const jwkUrk = 'https://www.googleapis.com/oauth2/v3/certs';
------------------------------
In samples/verifyGoogleIdToken.js
<#1444 (comment)>
:
> + // Verify that the token contains subject and email claims.
+ // Get the User id.
+ if (result.payload['sub']) {
+ console.log(`User id: ${result.payload['sub']}`);
+ }
+
+ // Optionally, if "includeEmail" was set in the token options, check if the
+ // email was verified
+ if (result.payload['email_verified']) {
+ console.log(`Email verified: ${result.payload['email_verified']}`);
+ }
+
+ console.log('ID token verified.');
We could print this out:
⬇️ Suggested change
- // Verify that the token contains subject and email claims.
- // Get the User id.
- if (result.payload['sub']) {
- console.log(`User id: ${result.payload['sub']}`);
- }
-
- // Optionally, if "includeEmail" was set in the token options, check if the
- // email was verified
- if (result.payload['email_verified']) {
- console.log(`Email verified: ${result.payload['email_verified']}`);
- }
-
- console.log('ID token verified.');
+ // The verified payload
+ console.dir(result.payload)
—
Reply to this email directly, view it on GitHub
<#1444 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEGJFRTRMOYCIH6EBAYOXJDV6E5LPANCNFSM57I32QIQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
--
Pia Chamberlain | Senior Technical Writer | she/her | Cloud Security Docs
|
|
@danielbankhead thanks a lot for the review! updated the PR also we have already published canonical samples for Java (and Python): https://github.com/googleapis/google-auth-library-java/tree/main/samples/snippets/src/main/java, so most of the ideas and comments moved here (I commented) |
danielbankhead
approved these changes
Sep 15, 2022
|
I just moved the same code: GoogleCloudPlatform/nodejs-docs-samples#2759 |
I like these here for integration purposes, say an unexpected breaking change was made in the codebase, having them here would make it easy to catch in a PR rather than in another repo. In either case, approved. |
FrodoTheTrue
force-pushed
the
add-auth-samples
branch
from
5c892f7
to
0a80ddb
Compare
FrodoTheTrue
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
bcoe
removed
the
do not merge
|
defer to @danielbankhead's review. |
danielbankhead
approved these changes
Sep 29, 2022
gcf-merge-on-green bot
pushed a commit
that referenced
this pull request
Oct 17, 2022
🤖 I have created a release *beep* *boop* --- ## [8.6.0](https://togithub.com/googleapis/google-auth-library-nodejs/compare/v8.5.2...v8.6.0) (2022-10-06) ### Features * Adding validation for psc endpoints ([#1473](https://togithub.com/googleapis/google-auth-library-nodejs/issues/1473)) ([4bbd13f](https://togithub.com/googleapis/google-auth-library-nodejs/commit/4bbd13fbf9081e004209d0ffc336648cff0c529e)) * **samples:** Auth samples ([#1444](https://togithub.com/googleapis/google-auth-library-nodejs/issues/1444)) ([137883a](https://togithub.com/googleapis/google-auth-library-nodejs/commit/137883aff56c9e847abb6445c89a76a27536fe26)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕