Account with NotPrincipal should explicitly provide permission

The is the bucket policy with below combination

Effect : DENY,
Action: $OPERATION,
NotPrincipal: $ACCOUNT

The permission on the "Action" for the $ACCOUNT mentioned in the
"NotPrincipal" should be explicitly given.

Example: For "Action: get_object", if we want to DENY permission
for * (all accounts) and we want to give explicit permission to
any one or few accounts, then we can include that account as
part of part of "NotPrincipal"

Check DFBUGS-1519 for steps to reproduce the issue

Fixes: https://issues.redhat.com/browse/DFBUGS-1519

Signed-off-by: Vinayakswami Hariharmath <[email protected]>

Author: vh05

src/endpoint/s3/s3_rest.js

@@ -293,16 +293,24 @@ async function authorize_request_policy(req) {
             s3_policy, account_identifier_id, method, arn_path, req, public_access_block_cfg?.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) {
+    if (account_identifier_name && permission_by_id !== "ALLOW") {
+        // we check the name only if the id check is not ALLOW
+        // this is to avoid double checking the same permission
+        // and to make sure we are not checking the same permission twice
+        // in case of NC account (which is not unique)
         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_cfg?.public_access_block?.restrict_public_buckets);
         dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
-    if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);
-    if ((permission_by_id === "ALLOW" || permission_by_name === "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);
 }

src/test/unit_tests/test_s3_bucket_policy.js

@@ -1645,6 +1645,52 @@ mocha.describe('s3_bucket_policy', function() {
             Bucket: BKT,
             Key: KEY,
         }));
+
+    });
+
+    mocha.it('should be able to use notPrincipal with Effect Deny', async function() {
+        const self = this; // eslint-disable-line no-invalid-this
+        self.timeout(15000);
+        const auth_put_policy = {
+        Version: '2012-10-17',
+        Statement: [
+            {
+                Effect: 'Allow',
+                Principal: { AWS: [user_a, user_b] },
+                Action: ['s3:*'],
+                Resource: [`arn:aws:s3:::${BKT}/*`]
+            },
+            {
+                Effect: 'Deny',
+                NotPrincipal: { AWS: user_a },
+                Action: ['s3:GetObject'],
+                Resource: [`arn:aws:s3:::${BKT}/*`]
+            }
+        ]};
+        let res = await s3_owner.putBucketPolicy({
+            Bucket: BKT,
+            Policy: JSON.stringify(auth_put_policy)
+        });
+        assert.equal(res.$metadata.httpStatusCode, 200);
+
+        res = await s3_a.putObject({
+            Body: BODY,
+            Bucket: BKT,
+            Key: KEY,
+        });
+        assert.equal(res.$metadata.httpStatusCode, 200);
+
+        res = await s3_a.getObject({
+            Bucket: BKT,
+            Key: KEY,
+        });
+        assert.equal(res.$metadata.httpStatusCode, 200);
+        assert.equal(res.Body.toString(), BODY);
+
+        await assert_throws_async(s3_b.getObject({
+            Bucket: BKT,
+            Key: KEY,
+        }));
     });
 
     mocha.it('should be able to use notResource', async function() {