feat(storage): migrate ACL samples and tests#4260
feat(storage): migrate ACL samples and tests#4260angelcaamal wants to merge 1 commit intoGoogleCloudPlatform:mainfrom
Conversation
product-auto-label
bot
added
api: storage
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Google Cloud Storage Node.js samples by providing a complete suite of examples for Access Control List (ACL) management. It enables developers to programmatically control permissions for buckets and files, covering operations like granting and revoking ownership, and retrieving current ACL configurations. The inclusion of dedicated system tests ensures the reliability and accuracy of these new ACL functionalities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a number of new Node.js samples for Google Cloud Storage ACL operations. The overall structure is good, but there is a critical race condition in all the new sample files: they call an async function without await from a synchronous main function, which can cause the script to exit prematurely. I've provided suggestions to fix this by making the main function async and simplifying the structure. I've also found some issues in the test and cleanup scripts, including redundant code and misleading test descriptions, and have suggested fixes for those as well.
| function main(bucketName = 'my-bucket', userEmail = 'jdobry@google.com') { | ||
| // [START storage_add_bucket_default_owner] | ||
| /** | ||
| * TODO(developer): Uncomment the following lines before running the sample. | ||
| */ | ||
| // The ID of your GCS bucket | ||
| // const bucketName = 'your-unique-bucket-name'; | ||
|
|
||
| // The email address of the user to add | ||
| // const userEmail = 'user-email-to-add'; | ||
|
|
||
| // Imports the Google Cloud client library | ||
| const {Storage} = require('@google-cloud/storage'); | ||
|
|
||
| // Creates a client | ||
| const storage = new Storage(); | ||
|
|
||
| async function addBucketDefaultOwner() { | ||
| try { | ||
| // Makes the user an owner in the default ACL of the bucket. You can use | ||
| // addAllUsers(), addDomain(), addProject(), addGroup(), and | ||
| // addAllAuthenticatedUsers() to grant access to different types of entities. | ||
| // You can also use "readers" and "writers" to grant different roles. | ||
| await storage.bucket(bucketName).acl.default.owners.addUser(userEmail); | ||
|
|
||
| console.log( | ||
| `Added user ${userEmail} as an owner on bucket ${bucketName}.` | ||
| ); | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error executing add bucket default owner ACL:', | ||
| error.message || error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| addBucketDefaultOwner(); | ||
| // [END storage_add_bucket_default_owner] | ||
| } |
There was a problem hiding this comment.
The main function is synchronous and calls the async function addBucketDefaultOwner without await. This creates a race condition where the script can terminate before the asynchronous operation completes. To fix this, main should be async and the logic can be simplified by removing the unnecessary inner function. Setting process.exitCode = 1 on error will also correctly signal failure to calling scripts.
async function main(bucketName = 'my-bucket', userEmail = 'jdobry@google.com') {
// [START storage_add_bucket_default_owner]
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// The ID of your GCS bucket
// const bucketName = 'your-unique-bucket-name';
// The email address of the user to add
// const userEmail = 'user-email-to-add';
// Imports the Google Cloud client library
const {Storage} = require('@google-cloud/storage');
// Creates a client
const storage = new Storage();
try {
// Makes the user an owner in the default ACL of the bucket. You can use
// addAllUsers(), addDomain(), addProject(), addGroup(), and
// addAllAuthenticatedUsers() to grant access to different types of entities.
// You can also use "readers" and "writers" to grant different roles.
await storage.bucket(bucketName).acl.default.owners.addUser(userEmail);
console.log(
`Added user ${userEmail} as an owner on bucket ${bucketName}.`
);
} catch (error) {
console.error(
'Error executing add bucket default owner ACL:',
error.message || error
);
process.exitCode = 1;
}
// [END storage_add_bucket_default_owner]
}| function main(bucketName = 'my-bucket', userEmail = 'jdobry@google.com') { | ||
| // [START storage_add_bucket_owner] | ||
| /** | ||
| * TODO(developer): Uncomment the following lines before running the sample. | ||
| */ | ||
| // The ID of your GCS bucket | ||
| // const bucketName = 'your-unique-bucket-name'; | ||
|
|
||
| // The email address of the user to add | ||
| // const userEmail = 'user-email-to-add'; | ||
|
|
||
| // Imports the Google Cloud client library | ||
| const {Storage} = require('@google-cloud/storage'); | ||
|
|
||
| // Creates a client | ||
| const storage = new Storage(); | ||
|
|
||
| async function addBucketOwner() { | ||
| try { | ||
| // Makes the user an owner of the bucket. You can use addAllUsers(), | ||
| // addDomain(), addProject(), addGroup(), and addAllAuthenticatedUsers() | ||
| // to grant access to different types of entities. You can also use "readers" | ||
| // and "writers" to grant different roles. | ||
| await storage.bucket(bucketName).acl.owners.addUser(userEmail); | ||
|
|
||
| console.log( | ||
| `Added user ${userEmail} as an owner on bucket ${bucketName}.` | ||
| ); | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error executing add bucket owner ACL:', | ||
| error.message || error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| addBucketOwner(); | ||
| // [END storage_add_bucket_owner] | ||
| } |
There was a problem hiding this comment.
The main function is synchronous and calls the async function addBucketOwner without await. This creates a race condition where the script can terminate before the asynchronous operation completes. To fix this, main should be async and the logic can be simplified by removing the unnecessary inner function. Setting process.exitCode = 1 on error will also correctly signal failure to calling scripts.
async function main(bucketName = 'my-bucket', userEmail = 'jdobry@google.com') {
// [START storage_add_bucket_owner]
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// The ID of your GCS bucket
// const bucketName = 'your-unique-bucket-name';
// The email address of the user to add
// const userEmail = 'user-email-to-add';
// Imports the Google Cloud client library
const {Storage} = require('@google-cloud/storage');
// Creates a client
const storage = new Storage();
try {
// Makes the user an owner of the bucket. You can use addAllUsers(),
// addDomain(), addProject(), addGroup(), and addAllAuthenticatedUsers()
// to grant access to different types of entities. You can also use "readers"
// and "writers" to grant different roles.
await storage.bucket(bucketName).acl.owners.addUser(userEmail);
console.log(
`Added user ${userEmail} as an owner on bucket ${bucketName}.`
);
} catch (error) {
console.error(
'Error executing add bucket owner ACL:',
error.message || error
);
process.exitCode = 1;
}
// [END storage_add_bucket_owner]
}| function main( | ||
| bucketName = 'my-bucket', | ||
| fileName = 'test.txt', | ||
| userEmail = 'jdobry@google.com' | ||
| ) { | ||
| // [START storage_add_file_owner] | ||
| /** | ||
| * TODO(developer): Uncomment the following lines before running the sample. | ||
| */ | ||
| // The ID of your GCS bucket | ||
| // const bucketName = 'your-unique-bucket-name'; | ||
|
|
||
| // The name of the file to access | ||
| // const fileName = 'file.txt'; | ||
|
|
||
| // The email address of the user to add | ||
| // const userEmail = 'user-email-to-add'; | ||
|
|
||
| // Imports the Google Cloud client library | ||
| const {Storage} = require('@google-cloud/storage'); | ||
|
|
||
| // Creates a client | ||
| const storage = new Storage(); | ||
|
|
||
| async function addFileOwner() { | ||
| try { | ||
| await storage | ||
| .bucket(bucketName) | ||
| .file(fileName) | ||
| .acl.owners.addUser(userEmail); | ||
|
|
||
| console.log(`Added user ${userEmail} as an owner on file ${fileName}.`); | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error executing add file owner ACL:', | ||
| error.message || error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| addFileOwner(); | ||
| // [END storage_add_file_owner] | ||
| } |
There was a problem hiding this comment.
The main function is synchronous and calls the async function addFileOwner without await. This creates a race condition where the script can terminate before the asynchronous operation completes. To fix this, main should be async and the logic can be simplified by removing the unnecessary inner function. Setting process.exitCode = 1 on error will also correctly signal failure to calling scripts.
async function main(
bucketName = 'my-bucket',
fileName = 'test.txt',
userEmail = 'jdobry@google.com'
) {
// [START storage_add_file_owner]
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// The ID of your GCS bucket
// const bucketName = 'your-unique-bucket-name';
// The name of the file to access
// const fileName = 'file.txt';
// The email address of the user to add
// const userEmail = 'user-email-to-add';
// Imports the Google Cloud client library
const {Storage} = require('@google-cloud/storage');
// Creates a client
const storage = new Storage();
try {
await storage
.bucket(bucketName)
.file(fileName)
.acl.owners.addUser(userEmail);
console.log(`Added user ${userEmail} as an owner on file ${fileName}.`);
} catch (error) {
console.error(
'Error executing add file owner ACL:',
error.message || error
);
process.exitCode = 1;
}
// [END storage_add_file_owner]
}| function main(bucketName = 'my-bucket') { | ||
| // [START storage_print_bucket_acl] | ||
| /** | ||
| * TODO(developer): Uncomment the following lines before running the sample. | ||
| */ | ||
| // The ID of your GCS bucket | ||
| // const bucketName = 'your-unique-bucket-name'; | ||
|
|
||
| // Imports the Google Cloud client library | ||
| const {Storage} = require('@google-cloud/storage'); | ||
|
|
||
| // Creates a client | ||
| const storage = new Storage(); | ||
|
|
||
| async function printBucketAcl() { | ||
| try { | ||
| // Gets the ACL for the bucket | ||
| const [acls] = await storage.bucket(bucketName).acl.get(); | ||
|
|
||
| acls.forEach(acl => { | ||
| console.log(`${acl.role}: ${acl.entity}`); | ||
| }); | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error executing print bucket ACL:', | ||
| error.message || error | ||
| ); | ||
| } | ||
| } | ||
| printBucketAcl(); | ||
| // [END storage_print_bucket_acl] | ||
| } |
There was a problem hiding this comment.
The main function is synchronous and calls the async function printBucketAcl without await. This creates a race condition where the script can terminate before the asynchronous operation completes. To fix this, main should be async and the logic can be simplified by removing the unnecessary inner function. Setting process.exitCode = 1 on error will also correctly signal failure to calling scripts.
async function main(bucketName = 'my-bucket') {
// [START storage_print_bucket_acl]
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// The ID of your GCS bucket
// const bucketName = 'your-unique-bucket-name';
// Imports the Google Cloud client library
const {Storage} = require('@google-cloud/storage');
// Creates a client
const storage = new Storage();
try {
// Gets the ACL for the bucket
const [acls] = await storage.bucket(bucketName).acl.get();
acls.forEach(acl => {
console.log(`${acl.role}: ${acl.entity}`);
});
} catch (error) {
console.error(
'Error executing print bucket ACL:',
error.message || error
);
process.exitCode = 1;
}
// [END storage_print_bucket_acl]
}| function main(bucketName = 'my-bucket', userEmail = 'jdobry@google.com') { | ||
| // [START storage_print_bucket_acl_for_user] | ||
| /** | ||
| * TODO(developer): Uncomment the following lines before running the sample. | ||
| */ | ||
| // The ID of your GCS bucket | ||
| // const bucketName = 'your-unique-bucket-name'; | ||
|
|
||
| // The email address of the user to check | ||
| // const userEmail = 'user-email-to-check'; | ||
|
|
||
| // Imports the Google Cloud client library | ||
| const {Storage} = require('@google-cloud/storage'); | ||
|
|
||
| // Creates a client | ||
| const storage = new Storage(); | ||
|
|
||
| async function printBucketAclForUser() { | ||
| try { | ||
| const options = { | ||
| // Specify the user | ||
| entity: `user-${userEmail}`, | ||
| }; | ||
|
|
||
| // Gets the user's ACL for the bucket | ||
| const [aclObject] = await storage.bucket(bucketName).acl.get(options); | ||
|
|
||
| console.log(`${aclObject.role}: ${aclObject.entity}`); | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error executing print bucket ACL for user:', | ||
| error.message || error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| printBucketAclForUser(); | ||
| // [END storage_print_bucket_acl_for_user] | ||
| } |
There was a problem hiding this comment.
The main function is synchronous and calls the async function printBucketAclForUser without await. This creates a race condition where the script can terminate before the asynchronous operation completes. To fix this, main should be async and the logic can be simplified by removing the unnecessary inner function. Setting process.exitCode = 1 on error will also correctly signal failure to calling scripts.
async function main(bucketName = 'my-bucket', userEmail = 'jdobry@google.com') {
// [START storage_print_bucket_acl_for_user]
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// The ID of your GCS bucket
// const bucketName = 'your-unique-bucket-name';
// The email address of the user to check
// const userEmail = 'user-email-to-check';
// Imports the Google Cloud client library
const {Storage} = require('@google-cloud/storage');
// Creates a client
const storage = new Storage();
try {
const options = {
// Specify the user
entity: `user-${userEmail}`,
};
// Gets the user's ACL for the bucket
const [aclObject] = await storage.bucket(bucketName).acl.get(options);
console.log(`${aclObject.role}: ${aclObject.entity}`);
} catch (error) {
console.error(
'Error executing print bucket ACL for user:',
error.message || error
);
process.exitCode = 1;
}
// [END storage_print_bucket_acl_for_user]
}| function main( | ||
| bucketName = 'my-bucket', | ||
| fileName = 'test.txt', | ||
| userEmail = 'jdobry@google.com' | ||
| ) { | ||
| // [START storage_remove_file_owner] | ||
| /** | ||
| * TODO(developer): Uncomment the following lines before running the sample. | ||
| */ | ||
| // The ID of your GCS bucket | ||
| // const bucketName = 'your-unique-bucket-name'; | ||
|
|
||
| // The ID of your GCS file | ||
| // const fileName = 'your-file-name'; | ||
|
|
||
| // The email address of the user to remove | ||
| // const userEmail = 'user-email-to-remove'; | ||
|
|
||
| // Imports the Google Cloud client library | ||
| const {Storage} = require('@google-cloud/storage'); | ||
|
|
||
| // Creates a client | ||
| const storage = new Storage(); | ||
|
|
||
| async function removeFileOwner() { | ||
| try { | ||
| // Removes the user from the access control list of the file. You can use | ||
| // deleteAllUsers(), deleteDomain(), deleteProject(), deleteGroup(), and | ||
| // deleteAllAuthenticatedUsers() to remove access for different types of entities. | ||
| await storage | ||
| .bucket(bucketName) | ||
| .file(fileName) | ||
| .acl.owners.deleteUser(userEmail); | ||
|
|
||
| console.log(`Removed user ${userEmail} from file ${fileName}.`); | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error executing remove file owner ACL:', | ||
| error.message || error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| removeFileOwner(); | ||
| // [END storage_remove_file_owner] | ||
| } |
There was a problem hiding this comment.
The main function is synchronous and calls the async function removeFileOwner without await. This creates a race condition where the script can terminate before the asynchronous operation completes. To fix this, main should be async and the logic can be simplified by removing the unnecessary inner function. Setting process.exitCode = 1 on error will also correctly signal failure to calling scripts.
async function main(
bucketName = 'my-bucket',
fileName = 'test.txt',
userEmail = 'jdobry@google.com'
) {
// [START storage_remove_file_owner]
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// The ID of your GCS bucket
// const bucketName = 'your-unique-bucket-name';
// The ID of your GCS file
// const fileName = 'your-file-name';
// The email address of the user to remove
// const userEmail = 'user-email-to-remove';
// Imports the Google Cloud client library
const {Storage} = require('@google-cloud/storage');
// Creates a client
const storage = new Storage();
try {
// Removes the user from the access control list of the file. You can use
// deleteAllUsers(), deleteDomain(), deleteProject(), deleteGroup(), and
// deleteAllAuthenticatedUsers() to remove access for different types of entities.
await storage
.bucket(bucketName)
.file(fileName)
.acl.owners.deleteUser(userEmail);
console.log(`Removed user ${userEmail} from file ${fileName}.`);
} catch (error) {
console.error(
'Error executing remove file owner ACL:',
error.message || error
);
process.exitCode = 1;
}
// [END storage_remove_file_owner]
}| return bucket.deleteFiles() | ||
| .then(() => bucket.deleteFiles(), console.error) | ||
| .then(() => { | ||
| console.log(`Deleting ${bucket.name}`); | ||
| return bucket.delete(); | ||
| }, console.error) | ||
| .catch(console.error); |
There was a problem hiding this comment.
The bucket deletion logic has a few issues: the deleteFiles() call is duplicated, and the error handling could lead to subsequent errors. If the first deleteFiles() fails, the error is logged, but the promise chain continues, which will cause the bucket.delete() call to fail on a non-empty bucket. The logic can be simplified and made more robust.
return bucket.deleteFiles({force: true})
.then(() => {
console.log(`Deleting ${bucket.name}`);
return bucket.delete();
})
.catch(err => {
console.error(`Failed to delete bucket ${bucket.name}:`, err);
});
| try { | ||
| await bucket.deleteFiles({force: true}); | ||
| } catch (err) { | ||
| // ignore error | ||
| } |
There was a problem hiding this comment.
This try-catch block containing bucket.deleteFiles({force: true}) is a duplicate of the one just above it. One call is sufficient to delete all files and their versions.
| await bucket.file(fileName).acl.readers.deleteUser(userEmail); | ||
| }); | ||
|
|
||
| it('should add a user as an owner on a bucket', () => { |
There was a problem hiding this comment.
The test description is misleading. This test case adds a user as an owner to a file, not a bucket.
| it('should add a user as an owner on a bucket', () => { | |
| it('should add a user as an owner on a file', () => { |
| ); | ||
| }); | ||
|
|
||
| it('should remove a user from a bucket', () => { |
There was a problem hiding this comment.
The test description is misleading. This test case removes a user from a file, not a bucket.
| it('should remove a user from a bucket', () => { | |
| it('should remove a user from a file', () => { |
1 participant
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.