CLDSRV-1017: apply each key's own authorization verdict in multiObjectDelete

lib/api/multiObjectDelete.js

@@ -695,13 +695,6 @@ function multiObjectDelete(authInfo, request, log, callback) {
                             });
                             return next(errors.InternalError);
                         }
-                        // Convert authorization results into an easier to handle format
-                        const actionImplicitDenies = authorizationResults.reduce((acc, curr, idx) => {
-                            const apiMethod = authorizationResults[idx].action;
-                            // eslint-disable-next-line no-param-reassign
-                            acc[apiMethod] = curr.isImplicit;
-                            return acc;
-                        }, {});
                         for (let i = 0; i < authorizationResults.length; i++) {
                             const result = authorizationResults[i];
                             // result is { isAllowed: true,
@@ -726,7 +719,11 @@ function multiObjectDelete(authInfo, request, log, callback) {
                                 continue;
                             }
 
-                            // Evaluate against the bucket policies
+                            // Evaluate against the bucket policies with this
+                            // key's own IAM verdict: all results share the same
+                            // action, so a request-wide map keyed by action
+                            // would collapse to the last key's verdict
+                            const actionImplicitDenies = { [result.action]: result.isImplicit };
                             const areAllActionsAllowed = evaluateBucketPolicyWithIAM(
                                 bucketMD,
                                 Object.keys(actionImplicitDenies),

tests/unit/api/multiObjectDelete.js

@@ -560,3 +560,103 @@ describe('multiObjectDelete checkPolicies request context', () => {
         });
     });
 });
+
+describe('multiObjectDelete per-key authorization results', () => {
+    // IAM user within the bucket owner account, so that the request
+    // goes through the checkPolicies path
+    const userAuthInfo = makeAuthInfo(canonicalID, 'testuser');
+
+    function makeDeleteRequest(keys) {
+        const post = `<Delete>${keys.map(key => `<Object><Key>${key}</Key></Object>`).join('')}</Delete>`;
+        return new DummyRequest({
+            bucketName,
+            namespace,
+            parsedHost: 'localhost',
+            headers: {
+                'content-md5': crypto.createHash('md5').update(post, 'utf8').digest('base64'),
+            },
+            post,
+            url: `/${bucketName}`,
+            socket: {
+                remoteAddress: '127.0.0.1',
+            },
+        });
+    }
+
+    function putObject(key, cb) {
+        const putRequest = new DummyRequest(
+            {
+                bucketName,
+                namespace,
+                objectKey: key,
+                headers: {},
+                url: `/${bucketName}/${key}`,
+            },
+            postBody,
+        );
+        return objectPut(authInfo, putRequest, undefined, log, cb);
+    }
+
+    beforeEach(done => {
+        cleanup();
+        sinon.stub(auth.server, 'extractParams').returns({
+            params: {
+                version: 4,
+                data: {
+                    signatureVersion: 'AWS4-HMAC-SHA256',
+                    authType: 'REST-HEADER',
+                    signatureAge: 0,
+                },
+            },
+        });
+        // Mimic a Vault user policy that only allows deleting allow/*:
+        // explicit allow on allow/*, implicit deny on everything else
+        sinon.stub(vault, 'checkPolicies').callsFake((requestContextParams, arn, log, cb) => {
+            const results = requestContextParams.parameterize.specificResource.map(entry => ({
+                isAllowed: entry.key.startsWith('allow/'),
+                isImplicit: !entry.key.startsWith('allow/'),
+                arn: `arn:aws:s3:::${bucketName}/${entry.key}`,
+                action: 'objectDelete',
+                versionId: entry.versionId,
+            }));
+            return cb(null, results);
+        });
+        bucketPut(authInfo, testBucketPutRequest, log, done);
+    });
+
+    afterEach(() => {
+        sinon.restore();
+    });
+
+    it('should deny a denied key even when the last key of the request is allowed', done => {
+        putObject('deny/d', () =>
+            putObject('allow/d', () => {
+                const request = makeDeleteRequest(['deny/d', 'allow/d']);
+                multiObjectDelete.multiObjectDelete(userAuthInfo, request, log, (err, xml) => {
+                    assert.strictEqual(err, null);
+                    assert.strictEqual(xml.includes('<Error><Key>deny/d</Key><Code>AccessDenied</Code>'), true);
+                    assert.strictEqual(xml.includes('<Deleted><Key>allow/d</Key></Deleted>'), true);
+                    assert.strictEqual(metadata.keyMaps.get(bucketName).has('deny/d'), true);
+                    assert.strictEqual(metadata.keyMaps.get(bucketName).has('allow/d'), false);
+                    done();
+                });
+            }),
+        );
+    });
+
+    it('should allow an allowed key even when the last key of the request is denied', done => {
+        putObject('allow/e', () =>
+            putObject('deny/e', () => {
+                const request = makeDeleteRequest(['allow/e', 'deny/e']);
+                multiObjectDelete.multiObjectDelete(userAuthInfo, request, log, (err, xml) => {
+                    assert.strictEqual(err, null);
+                    assert.strictEqual(xml.includes('<Deleted><Key>allow/e</Key></Deleted>'), true);
+                    assert.strictEqual(xml.includes('<Error><Key>deny/e</Key><Code>AccessDenied</Code>'), true);
+                    assert.strictEqual(metadata.keyMaps.get(bucketName).has('allow/e'), false);
+                    assert.strictEqual(metadata.keyMaps.get(bucketName).has('deny/e'), true);
+                    done();
+                });
+            }),
+        );
+    });
+});