Merge pull request #9007 from tangledbytes/utkarsh/fix/public-access-block-anon

tangledbytes · web-flow · commit d703fc136c39 · 2025-05-09T15:56:06.000+05:30
Fix anonymous user access with public_access defined and fix public_access tests
diff --git a/src/api/bucket_api.js b/src/api/bucket_api.js
@@ -1321,6 +1321,9 @@ module.exports = {
                 },
                 cors_configuration_rules: {
                     $ref: 'common_api#/definitions/bucket_cors_configuration',
+                },
+                public_access_block: {
+                    $ref: 'common_api#/definitions/public_access_block',
                 }
             }
         },
diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js
@@ -230,14 +230,21 @@ async function authorize_request_policy(req) {
     if (req.op_name === 'put_bucket') return;
 
     // owner_account is { id: bucket.owner_account, email: bucket.bucket_owner };
-    const { s3_policy, system_owner, bucket_owner, owner_account } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket);
+    const {
+        s3_policy,
+        system_owner,
+        bucket_owner,
+        owner_account,
+        public_access_block,
+    } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket);
+
     const auth_token = req.object_sdk.get_auth_token();
     const arn_path = _get_arn_from_req_path(req);
     const method = _get_method_from_req(req);
 
     const is_anon = !(auth_token && auth_token.access_key);
     if (is_anon) {
-        await authorize_anonymous_access(s3_policy, method, arn_path, req);
+        await authorize_anonymous_access(s3_policy, method, arn_path, req, public_access_block);
         return;
     }
 
@@ -283,21 +290,20 @@ async function authorize_request_policy(req) {
     let permission_by_id;
     let permission_by_name;
 
-    const public_access_block_cfg = await req.object_sdk.get_public_access_block({ name: req.params.bucket });
     // In NC, we allow principal to be:
     // 1. account name (for backwards compatibility)
     // 2. account id
     // we start the permission check on account identifier intentionally
     if (account_identifier_id) {
         permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_id, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets);
+            s3_policy, account_identifier_id, method, arn_path, req, public_access_block?.restrict_public_buckets);
         dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
     }
     if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
 
     if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) {
         permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_name, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets
+            s3_policy, account_identifier_name, method, arn_path, req, public_access_block?.restrict_public_buckets
         );
         dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
@@ -307,11 +313,11 @@ async function authorize_request_policy(req) {
     throw new S3Error(S3Error.AccessDenied);
 }
 
-async function authorize_anonymous_access(s3_policy, method, arn_path, req) {
+async function authorize_anonymous_access(s3_policy, method, arn_path, req, public_access_block) {
     if (!s3_policy) throw new S3Error(S3Error.AccessDenied);
 
     const permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
-        s3_policy, undefined, method, arn_path, req);
+        s3_policy, undefined, method, arn_path, req, public_access_block?.restrict_public_buckets);
     if (permission === "ALLOW") return;
 
     throw new S3Error(S3Error.AccessDenied);
diff --git a/src/sdk/object_sdk.js b/src/sdk/object_sdk.js
@@ -219,6 +219,7 @@ class ObjectSDK {
             system_owner: bucket.system_owner, // note that bucketspace_fs currently doesn't return system_owner
             bucket_owner: bucket.bucket_owner,
             owner_account: bucket.owner_account, // in NC NSFS this is the account id that owns the bucket
+            public_access_block: bucket.public_access_block,
         };
         return policy_info;
     }
diff --git a/src/server/system_services/bucket_server.js b/src/server/system_services/bucket_server.js
@@ -716,6 +716,7 @@ async function read_bucket_sdk_info(req) {
             .then(get_bucket_info),
         notifications: bucket.notifications,
         cors_configuration_rules: bucket.cors_configuration_rules,
+        public_access_block: bucket.public_access_block,
     };
 
     if (bucket.namespace) {
diff --git a/src/test/unit_tests/test_public_access_block.js b/src/test/unit_tests/test_public_access_block.js
@@ -46,7 +46,7 @@ async function run_on_random_bucket(s3, bucket_prefix, cb) {
     try {
         await cb(bucket);
     } finally {
-        await s3.deleteBucket({ Bucket: bucket });
+        await s3.deleteBucket({ Bucket: bucket }).catch(() => { /* nothing */ });
     }
 }
 
@@ -110,29 +110,33 @@ mocha.describe('noobaa public access block test', function() {
 
     mocha.it('put_public_access_block must throw when acls are used', async function() {
         await run_on_random_bucket(s3_owner, bucket_prefix, async bucket => {
-            try {
-                await s3_owner.putPublicAccessBlock({
+            await assert.rejects(
+                s3_owner.putPublicAccessBlock({
                     Bucket: bucket,
                     PublicAccessBlockConfiguration: {
                         BlockPublicAcls: true,
                     }
-                });
-                assert.fail("expected to fail");
-            } catch (error) {
-                assert(error.Code === S3Error.AccessControlListNotSupported.code);
-            }
+                }),
+                error => {
+                    // @ts-ignore
+                    assert(error.Code === S3Error.AccessControlListNotSupported.code);
+                    return true;
+                }
+            );
 
-            try {
-                await s3_owner.putPublicAccessBlock({
+            await assert.rejects(
+                s3_owner.putPublicAccessBlock({
                     Bucket: bucket,
                     PublicAccessBlockConfiguration: {
                         IgnorePublicAcls: true,
                     }
-                });
-                assert.fail("expected to fail");
-            } catch (error) {
-                assert(error.Code === S3Error.AccessControlListNotSupported.code);
-            }
+                }),
+                error => {
+                    // @ts-ignore
+                    assert(error.Code === S3Error.AccessControlListNotSupported.code);
+                    return true;
+                }
+            );
         });
     });
 
@@ -146,15 +150,12 @@ mocha.describe('noobaa public access block test', function() {
             });
 
             // Ensure we cannot put a public policy on this bucket
-            try {
-                await s3_owner.putBucketPolicy({
+            await assert.rejects(
+                s3_owner.putBucketPolicy({
                     Bucket: bucket,
                     Policy: generate_public_policy(bucket),
-                });
-                assert.fail("expected to fail");
-            } catch {
-                assert.ok(true);
-            }
+                })
+            );
 
             await s3_owner.deletePublicAccessBlock({
                 Bucket: bucket,
@@ -163,7 +164,7 @@ mocha.describe('noobaa public access block test', function() {
     });
 
     mocha.it('public_access_block should work when restrict public buckets is used', async function() {
-        await run_on_random_bucket(s3_owner, bucket_prefix, async bucket => {
+        await run_on_random_bucket(s3_owner, bucket_prefix, async function(bucket) {
             const KEY = "key";
 
             await s3_owner.putObject({
@@ -191,16 +192,13 @@ mocha.describe('noobaa public access block test', function() {
                 }
             });
 
-            try {
-                // Ensure anon can no longer access
-                await s3_anon.getObject({
+            // Ensure anon can no longer access
+            await assert.rejects(
+                s3_anon.getObject({
                     Bucket: bucket,
                     Key: KEY,
-                });
-                assert.fail("expected to fail after PublicAccessBlock");
-            } catch {
-                assert.ok(true);
-            }
+                }),
+            );
 
             await s3_owner.deletePublicAccessBlock({
                 Bucket: bucket,

Original file line number	Diff line number	Diff line change
`@@ -1321,6 +1321,9 @@ module.exports = {`
`1321`	`1321`	`},`
`1322`	`1322`	`cors_configuration_rules: {`
`1323`	`1323`	`$ref: 'common_api#/definitions/bucket_cors_configuration',`
	`1324`	`+ },`
	`1325`	`+ public_access_block: {`
	`1326`	`+ $ref: 'common_api#/definitions/public_access_block',`
`1324`	`1327`	`}`
`1325`	`1328`	`}`
`1326`	`1329`	`},`
Original file line number	Diff line number	Diff line change
`@@ -219,6 +219,7 @@ class ObjectSDK {`
`219`	`219`	`system_owner: bucket.system_owner, // note that bucketspace_fs currently doesn't return system_owner`
`220`	`220`	`bucket_owner: bucket.bucket_owner,`
`221`	`221`	`owner_account: bucket.owner_account, // in NC NSFS this is the account id that owns the bucket`
	`222`	`+ public_access_block: bucket.public_access_block,`
`222`	`223`	`};`
`223`	`224`	`return policy_info;`
`224`	`225`	`}`