Remove legacy public ACL's when setting objects private

Author: TimCsaky

app/src/components/utils.js

@@ -505,6 +505,23 @@ const utils = {
     else return '';
   },
 
+  /**
+   * @function stripResourcePath
+   * Yields a string `s` without trailing delimiters or wildcards. Returns an empty string if falsy.
+   * @param {string} s The input string
+   * @returns {string} The string `s` without trailing delimiters or wildcards, or an empty string.
+   */
+  stripResourcePath(s) {
+    switch (true) {
+      case s.endsWith(DELIMITER):
+        return utils.stripDelimit(s.slice(0, -1));
+      case s.endsWith(DELIMITER + '*'):
+        return utils.stripDelimit(s.slice(0, -2));
+      default:
+        return s;
+    }
+  },
+
   /**
    * @function toLowerKeys
    * Converts all key names for all objects in an array to lowercase

app/src/controllers/bucket.js

@@ -361,7 +361,7 @@ const controller = {
       };
       // Update S3 Policy
       await storageService.updatePublic(data).catch((e) => {
-        log.warn('Failed to apply permission changes to S3' + e, { function: 'togglePublic', ...data });
+        log.warn('Failed to apply permission changes to S3 ' + e, { function: 'togglePublic', ...data });
       });
 
       // Child bucket cannot be non-public when parent is public

app/src/controllers/sync.js

@@ -221,13 +221,15 @@ const controller = {
   async syncBucketPublic(key, bucketId, userId) {
     let isPublic = false;
     isPublic = await storageService.getPublic({ path: key, bucketId: bucketId });
-    bucketService.update({
-      bucketId: bucketId,
-      updatedBy: userId,
-      public: isPublic
-      // TODO: consider changing this to actual lastSyncDate
-      // lastSyncRequestedDate: now(),
-    });
+    // TODO: make sure this works correctly
+    console.log(`Syncing bucketId ${bucketId} public status to ${isPublic}`);
+    // bucketService.update({
+    //   bucketId: bucketId,
+    //   updatedBy: userId,
+    //   public: isPublic
+    //   // TODO: consider changing this to actual lastSyncDate
+    //   // lastSyncRequestedDate: now(),
+    // });
   },
 
   /**

app/src/services/storage.js

@@ -14,7 +14,6 @@ const {
   PutObjectAclCommand,
   GetBucketPolicyCommand,
   PutBucketPolicyCommand,
-  DeleteBucketPolicyCommand,
   PutObjectCommand,
   PutBucketEncryptionCommand,
   PutObjectTaggingCommand,
@@ -479,42 +478,74 @@ const objectStorageService = {
    * for an object 
    * will also remove this type of ACL set by other applications
    */
-  async removePublicACL(data, key) {
-    const existingAcl = await this._getS3Client(data).send(new GetObjectAclCommand({
-      Bucket: data.bucket,
-      Key: key
-    }));
-    const existingGrants = existingAcl.Grants;
-    const hasPublicAcl = existingGrants.some(grant => grant.Grantee?.URI === ALLUSERS && grant.Permission === 'READ');
-    if (hasPublicAcl) {
-      await this._getS3Client(data).send(new PutObjectAclCommand({
-        AccessControlPolicy: {
-          Grants: existingGrants
-            .filter(grant => grant.Grantee?.URI !== ALLUSERS && grant.Permission !== 'READ')
-            .map(grant => {
-              return {
-                Grantee: grant.Grantee,
-                Permission: grant.Permission,
-              };
-            }),
-          Owner: existingAcl.Owner
-        },
-        Bucket: data.bucket,
-        Key: key,
-      }));
-      return true;
+  async removePublicACL(bucketId, path, recursive = false) {
+    try {
+      const data = await utils.getBucket(bucketId);
+      const key = utils.stripDelimit(path);
+      let objects = [];
+
+      // list all objects at and below provided path (key)
+      if (recursive) {
+        const s3Response = await this.listAllObjectVersions({ filePath: key, bucketId: bucketId, precisePath: false });
+        `Found ${s3Response.Versions.length} object versions and ${s3Response.DeleteMarkers.length} 
+        delete markers in S3 for bucketId ${data.bucketId}`, { function: 'syncBucketRecursive' });
+        const s3Keys = [...new Set([
+          ...s3Response.DeleteMarkers.map(object => object.Key),
+          ...s3Response.Versions.map(object => object.Key),
+        ])];
+        objects = s3Keys
+      } else {
+        objects = [key];
+      }
+      log.info(`Removing ACL's from ${objects.length} objects`, { function: 'removePublicACL' });
+
+      // for each object update existing ACL to remove READ on ALLUSERS
+      for (const objKey of objects) {
+        const existingAcl = await this._getS3Client(data).send(new GetObjectAclCommand({
+          Bucket: data.bucket,
+          Key: objKey
+        }));
+        const existingGrants = existingAcl.Grants;
+        const hasPublicAcl = existingGrants.some(
+          grant => grant.Grantee?.URI === ALLUSERS && grant.Permission === 'READ'
+        );
+        if (hasPublicAcl) {
+          await this._getS3Client(data).send(new PutObjectAclCommand({
+            AccessControlPolicy: {
+              Grants: existingGrants
+                .filter(grant => grant.Grantee?.URI !== ALLUSERS && grant.Permission !== 'READ')
+                .map(grant => {
+                  return {
+                    Grantee: grant.Grantee,
+                    Permission: grant.Permission,
+                  };
+                }),
+              Owner: existingAcl.Owner
+            },
+            Bucket: data.bucket,
+            Key: objKey,
+          }));
+          return true;
+        }
+      }
+    } catch (error) {
+      console.log('Error in removePublicACL:', error);
+      throw error;
     }
   },
 
   /**
    * @function updatePublic
    * Updates the S3 Bucket Policies for the given resource
+   * any non-COMS Bucket Policies (Sid starts with `coms::`) are preserved
+   * NOTE: This function will remove legacy ACL's for all resources at or below path when setting to private
+   * to ensure objects are not inadvertently left public due to ACL settings
    * @param {string} path the path of the resource
    * @param {boolean} whether given resource should be set to public
    * @param {string} bucketId of COMS bucket for the resource
    * @returns {Promise<object>} The S3 PutBucketPolicyCommand response
    */
-  async updatePublic({ path, public: publicFlag = false, bucketId }) {
+  async updatePublic({ path, public: setAsPublic = false, bucketId }) {
     const data = await utils.getBucket(bucketId);
     const isPrefix = data.key + '/' === path;
     const resource = data.bucket + '/' + path;
@@ -529,21 +560,22 @@ const objectStorageService = {
       log.debug('No existing policy found', { function: 'updatePublic' });
     }
 
-    // If COMS policy found for any parent prefix, throw error. no exceptions to inherited policies permitted
+    // If COMS policy found for any parent prefix, throw error, exceptions from inherited policies are not permitted
     const parentPolicy = existingStatement.find(policy =>
-      `coms::${resource}`.startsWith(policy.Sid) && `coms::${resource}` !== policy.Sid);
+      `coms::${resource}`.startsWith(policy.Sid) &&
+      `coms::${resource}` !== policy.Sid);
     if (parentPolicy) {
+      console.log('Parent policy found, cannot override public status set on parent folder');
       throw new Error(`Unable to override Public status set on folder: ${parentPolicy.Resource}`);
     }
 
     // --- Update policy
-    // if making public add Allow rule for this resource
-    if (publicFlag) {
-
-      // include any existing non-coms-managed rules for this resource or below
-      const newStatement = existingStatement
-        .filter(policy => !policy.Sid.startsWith(`coms::${resource}`));
-
+    // keep non-coms and sibling policies
+    let newStatement = existingStatement.filter(policy => {
+      return !policy.Sid.startsWith(utils.stripResourcePath(`coms::${resource}`));
+    });
+    // if making public add Allow rule for this resource (and below)
+    if (setAsPublic) {
       const resourceKey = isPrefix ? resource + '*' : resource; // prefixes/need/trailing/wildcard/*
       newStatement
         .push({
@@ -553,28 +585,17 @@ const objectStorageService = {
           Principal: '*',
           Sid: 'coms::' + resource,
         });
-
-      // put new policy
-      if (newStatement.length > 0) {
-        s3Response = await this._getS3Client(data).send(new PutBucketPolicyCommand({
-          Bucket: data.bucket,
-          Policy: JSON.stringify({ Version: '2012-10-17', Statement: newStatement })
-        }));
-      }
     }
-
-    // else setting to private
-    // note: logic is confusing. But here, if setting to private, we are only deleting a COMS policy
-    else if (existingStatement.length > 0) {
-      await this._getS3Client(data).send(new DeleteBucketPolicyCommand({ Bucket: data.bucket }));
-      s3Response = [];
+    if (newStatement.length > 0) {
+      s3Response = await this._getS3Client(data).send(new PutBucketPolicyCommand({
+        Bucket: data.bucket,
+        Policy: JSON.stringify({ Version: '2012-10-17', Statement: newStatement })
+      }));
     }
 
-    // If updating a single file,
-    // Remove public ACL for file if present (phase out previous implementation)
-    if (isPrefix === false) {
-      await this.removePublicACL(data, path);
-    }
+    // -- remove all ACL's for this resource and below as precaution to user
+    // if updating a single object or setting a folder private, 
+    if (!isPrefix || !setAsPublic) await this.removePublicACL(bucketId, path, isPrefix);
 
     return s3Response;
   },