Use plain S3 url to get public objects

Add `GetObjectVersion` permission to `public` bucket policies
Handle sql transaction during syncBucketRecords

Author: TimCsaky

app/src/components/utils.js

@@ -275,6 +275,28 @@ const utils = {
     return array.find(obj => (obj.key === key && obj.value === value));
   },
 
+
+  /**
+ * @function getS3Url
+ * Constructs the S3 URL for a given file in a specified bucket.
+ * @param {object} data An object containing the necessary information to construct the S3 URL.
+ * @param {string} data.bucketId The ID of the S3 bucket.
+ * @param {string} data.filePath The path of the file within the bucket.
+ * @param {string} [data.s3VersionId] An optional S3 version ID for the file.
+ * @returns {Promise<string>} A promise that resolves to the constructed S3 URL.
+ * @throws {Error} If there is an issue retrieving the bucket details.
+ */
+  async getS3Url(data) {
+    // get bucket details
+    const { read } = require('../services/bucket');
+    const bucket = await read(data.bucketId);
+    let url = `${bucket.endpoint}/${bucket.bucket}/${data.filePath.replace(/^\/|\/$/g, '')}`;
+    if (data.s3VersionId) {
+      url += `?versionId=${data.s3VersionId}`;
+    }
+    return url;
+  },
+
   /**
    * @function getS3VersionId
    * Gets the s3VersionId from database using given internal COMS version id

app/src/controllers/object.js

@@ -21,6 +21,7 @@ const {
   getCurrentIdentity,
   getKeyValue,
   getMetadata,
+  getS3Url,
   getS3VersionId,
   joinPath,
   isTruthy,
@@ -879,18 +880,27 @@ const controller = {
           response.Body.pipe(res); // Stream body content directly to response
           res.status(200);
         }
-      } else {
-        const signedUrl = await storageService.readSignedUrl({
-          expiresIn: req.query.expiresIn,
-          ...data
-        });
+      }
+      else {
+        let s3Url;
+        // if object is public, construct S3 url manually
+        if (req.currentObject.public) {
+          s3Url = await getS3Url(data);
+        }
+        // else get pre-signed S3 url
+        else {
+          s3Url = await storageService.readSignedUrl({
+            expiresIn: req.query.expiresIn,
+            ...data
+          });
+        }
 
-        // Present download url link
+        // if request was for a url, present download url link
         if (req.query.download && req.query.download === DownloadMode.URL) {
-          res.status(201).json(signedUrl);
+          res.status(201).json(s3Url);
           // Download via HTTP redirect
         } else {
-          res.status(302).set('Location', signedUrl).end();
+          res.status(302).set('Location', s3Url).end();
         }
       }
 

app/src/controllers/sync.js

@@ -113,14 +113,15 @@ const controller = {
       const bucket = await bucketService.read(bucketId);
       const userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER), SYSTEM_USER);
 
-      // sync bucket.public flag
-      await this.syncBucketPublic(bucket.key, bucket.bucketId, userId);
 
       const s3Objects = await storageService.listAllObjectVersions({ bucketId: bucketId, filterLatest: true });
       log.info(`Found ${s3Objects.Versions.length} object versions and ${s3Objects.DeleteMarkers.length} 
         delete markers in S3 for bucketId ${bucketId}`, { function: 'syncBucketSingle' });
 
       const response = await utils.trxWrapper(async (trx) => {
+        // sync bucket.public flag
+        await this.syncBucketPublic(bucket, userId, trx);
+        // queue objects
         return this.queueObjectRecords([bucket], s3Objects, userId, trx);
       });
 
@@ -207,7 +208,7 @@ const controller = {
             trx
           );
           // --- Sync S3 Bucket Policies applied by COMS
-          await this.syncBucketPublic(bucket.key, bucket.bucketId, userId);
+          await this.syncBucketPublic(bucket, userId, trx);
         })
       );
       return dbBuckets;
@@ -218,16 +219,27 @@ const controller = {
     }
   },
 
-  async syncBucketPublic(key, bucketId, userId) {
+  /**
+   * Synchronizes the public status of a bucket with the storage service.
+   * @async
+   * @function syncBucketPublic
+   * @param {Object} bucket - The bucket object to synchronize
+   * @param {string} userId - The ID of the user performing the sync operation
+   * @param {Object} trx - The database transaction object
+   * @returns {Promise<void>}
+   * @description Fetches the public status from the storage service for the given bucket
+   * and updates the bucket record with the retrieved public status and the user who performed the update.
+   */
+  async syncBucketPublic(bucket, userId, trx) {
     let isPublic = false;
-    isPublic = await storageService.getPublic({ path: key, bucketId: bucketId });
-    bucketService.update({
-      bucketId: bucketId,
+    isPublic = await storageService.getPublic({ path: bucket.key, bucket: bucket }, trx);
+    await bucketService.update({
+      bucketId: bucket.bucketId,
       updatedBy: userId,
       public: isPublic
       // TODO: consider changing this to actual lastSyncDate
       // lastSyncRequestedDate: now(),
-    });
+    }, trx);
   },
 
   /**

app/src/services/storage.js

@@ -24,7 +24,7 @@ const { Upload } = require('@aws-sdk/lib-storage');
 const { getSignedUrl } = require('@aws-sdk/s3-request-presigner');
 const config = require('config');
 
-const { ALLUSERS, MetadataDirective, TaggingDirective } = require('../components/constants');
+const { ALLUSERS, DEFAULTREGION, MetadataDirective, TaggingDirective } = require('../components/constants');
 const log = require('../components/log')(module.filename);
 const utils = require('../components/utils');
 
@@ -582,7 +582,7 @@ const objectStorageService = {
       const resourceKey = isPrefix ? resource + '*' : resource; // prefixes/need/trailing/wildcard/*
       newPolicies
         .push({
-          Action: 's3:GetObject',
+          Action: ['s3:GetObject', 's3:GetObjectVersion'],
           Resource: resourceKey,
           Effect: 'Allow',
           Principal: '*',
@@ -604,17 +604,18 @@ const objectStorageService = {
 
   /**
    * @function getPublic
-   * checks for a Bucket Policy or ACL that will make the given resource public
-   * @param {string} path the path of the resource
-   * @param {string} bucketId of COMS bucket for the resource
-   * @returns {Promise<boolean>} whether the given resource is public
+   * Checks for a Bucket Policy or ACL that will make the given resource public
+   * @param {string} options.path The path of the resource to check
+   * @param {string} [options.bucketId] Optional bucketId to retrieve bucket configuration
+   * @param {object} [options.bucket] Optional bucket object containing bucketId (alternative to bucketId)
+   * @returns {Promise<boolean>} True if the resource is public via policy or ACL, false otherwise
    */
-  async getPublic({ path, bucketId }) {
-    const data = await utils.getBucket(bucketId);
-    const resource = data.bucket + '/' + path;
-    const hasPublicPolicy = await this.hasEffectivePublicPolicy(resource, data);
+  async getPublic({ path, bucketId = undefined, bucket = undefined }) {
+    const bucketData = bucket ? { ...bucket, region: DEFAULTREGION } : await utils.getBucket(bucketId);
+    const resource = bucketData.bucket + '/' + path;
+    const hasPublicPolicy = await this.hasEffectivePublicPolicy(resource, bucketData);
     // if resource is an object, check for public ACL's (ACL's cannot apply to prefixes)
-    const hasPublicAcl = data.key !== resource ? await this.hasPublicAcl(data, path) : false;
+    const hasPublicAcl = bucketData.key !== resource ? await this.hasPublicAcl(bucketData, path) : false;
     // Check for COMS Bucket Policy for this resource
     return hasPublicAcl || hasPublicPolicy;
   },
@@ -623,11 +624,12 @@ const objectStorageService = {
    * @function hasEffectivePublicPolicy
    * check for a Bucket Policy that will make the given resource public
    * @param {*} resource
-   * @param {*} data
+   * @param {*} bucketData
    */
-  async hasEffectivePublicPolicy(resource, data) {
+  async hasEffectivePublicPolicy(resource, bucketData) {
     try {
-      const existingPolicy = await this._getS3Client(data).send(new GetBucketPolicyCommand({ Bucket: data.bucket }));
+      const existingPolicy = await this._getS3Client(bucketData)
+        .send(new GetBucketPolicyCommand({ Bucket: bucketData.bucket }));
       const statement = JSON.parse(existingPolicy.Policy).Statement;
       // A Deny policy on resource or above, which override Allow policies will set public status to false
       const denyPolicies = statement
@@ -651,7 +653,7 @@ const objectStorageService = {
         return (allowPolicies.length > 0) ? true : false;
       }
     } catch (e) {
-      log.debug('No existing effective policies found', { function: 'getPublic' });
+      log.debug('No existing effective policies found', { function: 'hasEffectivePublicPolicy' });
       return false;
     }
   },