CLDSRV-426: update ACL permission checks for implicitDeny logic

CLDSRV-426: add tests for ACL permission check updates

CLDSRV-426: add tests for ACL permission check updates

lib/api/apiUtils/authorization/permissionChecks.js

@@ -8,13 +8,32 @@ const { allAuthedUsersId, bucketOwnerActions, logId, publicId,
 const publicReadBuckets = process.env.ALLOW_PUBLIC_READ_BUCKETS ?
     process.env.ALLOW_PUBLIC_READ_BUCKETS.split(',') : [];
 
-function checkBucketAcls(bucket, requestType, canonicalID) {
+function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) {
+    // Same logic applies on the Versioned APIs, so let's simplify it.
+    const requestTypeParsed = requestType.endsWith('Version') ?
+        requestType.slice(0, -7) : requestType;
     if (bucket.getOwner() === canonicalID) {
         return true;
     }
+    // Backward compatibility
+    const arrayOfAllowed = [
+        'objectPutTagging',
+        'objectPutLegalHold',
+        'objectPutRetention',
+    ];
+    if (mainApiCall === 'objectGet') {
+        if (requestTypeParsed === 'objectGetTagging') {
+            return true;
+        }
+    }
+    if (mainApiCall === 'objectPut') {
+        if (arrayOfAllowed.includes(requestTypeParsed)) {
+            return true;
+        }
+    }
 
     const bucketAcl = bucket.getAcl();
-    if (requestType === 'bucketGet' || requestType === 'bucketHead') {
+    if (requestTypeParsed === 'bucketGet' || requestTypeParsed === 'bucketHead') {
         if (bucketAcl.Canned === 'public-read'
             || bucketAcl.Canned === 'public-read-write'
             || (bucketAcl.Canned === 'authenticated-read'
@@ -32,7 +51,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
             return true;
         }
     }
-    if (requestType === 'bucketGetACL') {
+    if (requestTypeParsed === 'bucketGetACL') {
         if ((bucketAcl.Canned === 'log-delivery-write'
             && canonicalID === logId)
             || bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1
@@ -48,7 +67,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
         }
     }
 
-    if (requestType === 'bucketPutACL') {
+    if (requestTypeParsed === 'bucketPutACL') {
         if (bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1
             || bucketAcl.WRITE_ACP.indexOf(canonicalID) > -1) {
             return true;
@@ -62,11 +81,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
         }
     }
 
-    if (requestType === 'bucketDelete' && bucket.getOwner() === canonicalID) {
+    if (requestTypeParsed === 'objectDelete' || requestTypeParsed === 'objectPut') {
-        return true;
-    }
-
-    if (requestType === 'objectDelete' || requestType === 'objectPut') {
         if (bucketAcl.Canned === 'public-read-write'
             || bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1
             || bucketAcl.WRITE.indexOf(canonicalID) > -1) {
@@ -86,11 +101,12 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
     // objectPutACL, objectGetACL, objectHead or objectGet, the bucket
     // authorization check should just return true so can move on to check
     // rights at the object level.
-    return (requestType === 'objectPutACL' || requestType === 'objectGetACL' ||
+    return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' ||
-        requestType === 'objectGet' || requestType === 'objectHead');
+    requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
 }
 
-function checkObjectAcls(bucket, objectMD, requestType, canonicalID) {
+function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser,
+    isUserUnauthenticated, mainApiCall) {
     const bucketOwner = bucket.getOwner();
     // acls don't distinguish between users and accounts, so both should be allowed
     if (bucketOwnerActions.includes(requestType)
@@ -100,6 +116,15 @@ function checkObjectAcls(bucket, objectMD, requestType, canonicalID) {
     if (objectMD['owner-id'] === canonicalID) {
         return true;
     }
+
+    // Backward compatibility
+    if (mainApiCall === 'objectGet') {
+        if ((isUserUnauthenticated || (requesterIsNotUser && bucketOwner === objectMD['owner-id']))
+            && requestType === 'objectGetTagging') {
+            return true;
+        }
+    }
+
     if (!objectMD.acl) {
         return false;
     }

tests/unit/auth/permissionChecks.js

@@ -0,0 +1,269 @@
+const assert = require('assert');
+const { checkBucketAcls, checkObjectAcls } = require('../../../lib/api/apiUtils/authorization/permissionChecks');
+const constants = require('../../../constants');
+
+const { bucketOwnerActions } = constants;
+
+describe('checkBucketAcls', () => {
+    const mockBucket = {
+        getOwner: () => 'ownerId',
+        getAcl: () => ({
+            Canned: '',
+            FULL_CONTROL: [],
+            READ: [],
+            READ_ACP: [],
+            WRITE: [],
+            WRITE_ACP: [],
+        }),
+    };
+
+    const testScenarios = [
+        {
+            description: 'should return true if bucket owner matches canonicalID',
+            input: {
+                bucketAcl: {}, requestType: 'anyType', canonicalID: 'ownerId', mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectGetTagging when mainApiCall is objectGet',
+            input: {
+                bucketAcl: {}, requestType: 'objectGetTagging', canonicalID: 'anyId', mainApiCall: 'objectGet',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectPutTagging when mainApiCall is objectPut',
+            input: {
+                bucketAcl: {}, requestType: 'objectPutTagging', canonicalID: 'anyId', mainApiCall: 'objectPut',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectPutLegalHold when mainApiCall is objectPut',
+            input: {
+                bucketAcl: {}, requestType: 'objectPutLegalHold', canonicalID: 'anyId', mainApiCall: 'objectPut',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectPutRetention when mainApiCall is objectPut',
+            input: {
+                bucketAcl: {}, requestType: 'objectPutRetention', canonicalID: 'anyId', mainApiCall: 'objectPut',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for bucketGet if canned acl is public-read-write',
+            input: {
+                bucketAcl: { Canned: 'public-read-write' },
+                requestType: 'bucketGet',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for bucketGet if canned acl is authenticated-read and id is not publicId',
+            input: {
+                bucketAcl: { Canned: 'authenticated-read' },
+                requestType: 'bucketGet',
+                canonicalID: 'anyIdNotPublic',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for bucketHead if canned acl is public-read',
+            input: {
+                bucketAcl: { Canned: 'public-read' },
+                requestType: 'bucketHead',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for bucketGet if canonicalID has FULL_CONTROL access',
+            input: {
+                bucketAcl: { FULL_CONTROL: ['anyId'], READ: [] },
+                requestType: 'bucketGet',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for bucketGetACL if canonicalID has FULL_CONTROL',
+            input: {
+                bucketAcl: { FULL_CONTROL: ['anyId'], READ_ACP: [] },
+                requestType: 'bucketGetACL',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectDelete if bucketAcl.Canned is public-read-write',
+            input: {
+                bucketAcl: { Canned: 'public-read-write' },
+                requestType: 'objectDelete',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for requestType ending with "Version"',
+            input: {
+                bucketAcl: {},
+                requestType: 'objectGetVersion',
+                canonicalID: 'anyId',
+                mainApiCall: 'objectGet',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectPutACL',
+            input: {
+                bucketAcl: {},
+                requestType: 'objectPutACL',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return true for objectGetACL',
+            input: {
+                bucketAcl: {},
+                requestType: 'objectGetACL',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: true,
+        },
+        {
+            description: 'should return false for unmatched scenarios',
+            input: {
+                bucketAcl: {},
+                requestType: 'unmatchedRequest',
+                canonicalID: 'anyId',
+                mainApiCall: 'anyApiCall',
+            },
+            expected: false,
+        },
+    ];
+
+    testScenarios.forEach(scenario => {
+        it(scenario.description, () => {
+            // Mock the bucket based on the test scenario's input
+            mockBucket.getAcl = () => scenario.input.bucketAcl;
+
+            const result = checkBucketAcls(mockBucket,
+                scenario.input.requestType, scenario.input.canonicalID, scenario.input.mainApiCall);
+            assert.strictEqual(result, scenario.expected);
+        });
+    });
+});
+
+describe('checkObjectAcls', () => {
+    const mockBucket = {
+        getOwner: () => 'bucketOwnerId',
+        getName: () => 'bucketName',
+        getAcl: () => ({ Canned: '' }),
+    };
+    const mockObjectMD = {
+        'owner-id': 'objectOwnerId',
+        'acl': {
+            Canned: '',
+            FULL_CONTROL: [],
+            READ: [],
+            READ_ACP: [],
+            WRITE: [],
+            WRITE_ACP: [],
+        },
+    };
+
+    it('should return true if request type is in bucketOwnerActions and bucket owner matches canonicalID', () => {
+        assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, bucketOwnerActions[0],
+            'bucketOwnerId', false, false, 'anyApiCall'), true);
+    });
+
+    it('should return true if objectMD owner matches canonicalID', () => {
+        assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, 'anyType',
+            'objectOwnerId', false, false, 'anyApiCall'), true);
+    });
+
+    it('should return true for objectGetTagging when mainApiCall is objectGet and conditions met', () => {
+        assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, 'objectGetTagging',
+            'anyIdNotPublic', true, true, 'objectGet'), true);
+    });
+
+    it('should return false if no acl provided in objectMD', () => {
+        const objMDWithoutAcl = Object.assign({}, mockObjectMD);
+        delete objMDWithoutAcl.acl;
+        assert.strictEqual(checkObjectAcls(mockBucket, objMDWithoutAcl, 'anyType',
+            'anyId', false, false, 'anyApiCall'), false);
+    });
+
+    const tests = [
+        {
+            acl: 'public-read', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true,
+        },
+        {
+            acl: 'public-read-write', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true,
+        },
+        {
+            acl: 'authenticated-read', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true,
+        },
+        {
+            acl: 'bucket-owner-read', reqType: 'objectGet', id: 'bucketOwnerId', expected: true,
+        },
+        {
+            acl: 'bucket-owner-full-control', reqType: 'objectGet', id: 'bucketOwnerId', expected: true,
+        },
+        {
+            aclList: ['someId', 'anyIdNotPublic'],
+            aclField: 'FULL_CONTROL',
+            reqType: 'objectGet',
+            id: 'anyIdNotPublic',
+            expected: true,
+        },
+        {
+            aclList: ['someId', 'anyIdNotPublic'],
+            aclField: 'READ',
+            reqType: 'objectGet',
+            id: 'anyIdNotPublic',
+            expected: true,
+        },
+        { reqType: 'objectPut', id: 'anyId', expected: true },
+        { reqType: 'objectDelete', id: 'anyId', expected: true },
+        {
+            aclList: ['anyId'], aclField: 'FULL_CONTROL', reqType: 'objectPutACL', id: 'anyId', expected: true,
+        },
+        {
+            aclList: ['anyId'], aclField: 'FULL_CONTROL', reqType: 'objectGetACL', id: 'anyId', expected: true,
+        },
+        {
+            acl: '', reqType: 'objectGet', id: 'randomId', expected: false,
+        },
+    ];
+
+    tests.forEach(test => {
+        it(`should return ${test.expected} for ${test.reqType} with ACL as ${test.acl
+            || (`${test.aclField}:${JSON.stringify(test.aclList)}`)}`, () => {
+            if (test.acl) {
+                mockObjectMD.acl.Canned = test.acl;
+            } else if (test.aclList && test.aclField) {
+                mockObjectMD.acl[test.aclField] = test.aclList;
+            }
+
+            assert.strictEqual(
+                checkObjectAcls(mockBucket, mockObjectMD, test.reqType, test.id, false, false, 'anyApiCall'),
+                test.expected,
+            );
+        });
+    });
+});
+