NC | NSFS | Bucket Policy Check (Jira Issue 1307)

Signed-off-by: shirady <[email protected]>
noobaa · Jan 14, 2025 · 94201b1 · 94201b1
1 parent c171502
commit 94201b1
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 12 deletions.
diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js
@@ -277,23 +277,27 @@ async function authorize_request_policy(req) {
         if (is_owner || is_iam_account_and_same_root_account_owner) return;
         throw new S3Error(S3Error.AccessDenied);
     }
-    let permission;
+    // in case we have bucket policy
+    let permission_by_id;
+    let permission_by_name;
     // 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 = await s3_bucket_policy_utils.has_bucket_policy_permission(
+        permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
             s3_policy, account_identifier_id, method, arn_path, req);
+        dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
     }
 
-    if ((!account_identifier_id || permission === "IMPLICIT_DENY") && account.owner === undefined) {
-        permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
+    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);
+        dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
 
-    if (permission === "DENY") throw new S3Error(S3Error.AccessDenied);
-    if (permission === "ALLOW" || is_owner) return;
+    if (permission_by_id === "DENY" || permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);
+    if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW") || is_owner) return;
 
     throw new S3Error(S3Error.AccessDenied);
 }

diff --git a/src/sdk/bucketspace_fs.js b/src/sdk/bucketspace_fs.js
@@ -805,8 +805,8 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             throw new Error('has_bucket_action_permission: action is required');
         }
 
-        let permission;
-        permission = await bucket_policy_utils.has_bucket_policy_permission(
+        let permission_by_name;
+        const permission_by_id = await bucket_policy_utils.has_bucket_policy_permission(
             bucket_policy,
             account._id,
             action,
@@ -815,8 +815,8 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
         );
         // we (currently) allow account identified to be both id and name,
         // so if by-id failed, try also name
-        if (account.owner === undefined && permission === 'IMPLICIT_DENY') {
-            permission = await bucket_policy_utils.has_bucket_policy_permission(
+        if (account.owner === undefined && permission_by_id !== 'DENY') {
+            permission_by_name = await bucket_policy_utils.has_bucket_policy_permission(
                 bucket_policy,
                 account.name.unwrap(),
                 action,
@@ -825,8 +825,8 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             );
         }
 
-        if (permission === 'DENY') return false;
-        return is_owner || permission === 'ALLOW';
+        if (permission_by_id === 'DENY' || permission_by_name === 'DENY') return false;
+        return is_owner || (permission_by_id === 'ALLOW' || permission_by_name === 'ALLOW');
     }
 
     async validate_fs_bucket_access(bucket, object_sdk) {