refactor(s3): just use S3ServerSide, inject dependencies

S3LocalDev and S3ServerSide differ significantly only by the S3
API client, and the prefix for the short URL. Apply DRY to the two
classes so that the same code is exercised both in local and live
environments, injecting appropriate state into S3ServerSide at
runtime

- DependencyIds: Define new constants for s3Client and fileURLPrefix
- S3ServerSide: Use injectable ctor parameters instead of module-level
  constants
- inversify.config: Rework config so that only S3ServerSide inputs
  are determined by `NODE_ENV`, not the entire s3 dependency
- `api/user`: Use the s3 held by the container, not the methods
  it destructures into, so that we can properly refer to the object's
  state through `this`
- Rework `aws.test.ts` to account for changes
- Remove S3LocalDev, no longer used

src/server/api/user.ts

@@ -28,12 +28,7 @@ const { Public, Private } = FileVisibility
 
 const router = Express.Router()
 
-const {
-  buildFileLongUrl,
-  setS3ObjectACL,
-  uploadFileToS3,
-  getKeyFromLongUrl,
-} = container.get<S3Interface>(DependencyIds.s3)
+const s3 = container.get<S3Interface>(DependencyIds.s3)
 
 const fileUploadMiddleware = fileUpload({
   limits: {
@@ -152,14 +147,14 @@ router.post(
         const url = Url.create(
           {
             userId: user.id,
-            longUrl: file ? buildFileLongUrl(fileKey) : longUrl,
+            longUrl: file ? s3.buildFileLongUrl(fileKey) : longUrl,
             shortUrl,
             isFile: !!file,
           },
           { transaction: t },
         )
         if (file) {
-          await uploadFileToS3(file.data, fileKey, file.mimetype)
+          await s3.uploadFileToS3(file.data, fileKey, file.mimetype)
         }
         return url
       })
@@ -275,14 +270,14 @@ router.patch(
         if (!url.isFile) {
           await url.update({ longUrl }, { transaction: t })
         } else if (file) {
-          const oldKey = getKeyFromLongUrl(url.longUrl)
+          const oldKey = s3.getKeyFromLongUrl(url.longUrl)
           const newKey = addFileExtension(shortUrl, getFileExtension(file.name))
           await url.update(
-            { longUrl: buildFileLongUrl(newKey) },
+            { longUrl: s3.buildFileLongUrl(newKey) },
             { transaction: t },
           )
-          await setS3ObjectACL(oldKey, Private)
-          await uploadFileToS3(file.data, newKey, file.mimetype)
+          await s3.setS3ObjectACL(oldKey, Private)
+          await s3.uploadFileToS3(file.data, newKey, file.mimetype)
         }
       })
       res.ok(jsonMessage(`Short link "${shortUrl}" has been updated`))
@@ -328,8 +323,8 @@ router.patch('/url', validator.body(stateEditSchema), async (req, res) => {
       await url.update({ state }, { transaction: t })
       if (url.isFile) {
         // Toggle the ACL of the S3 object
-        await setS3ObjectACL(
-          getKeyFromLongUrl(url.longUrl),
+        await s3.setS3ObjectACL(
+          s3.getKeyFromLongUrl(url.longUrl),
           state === ACTIVE ? Public : Private,
         )
       }

src/server/constants.ts

@@ -8,6 +8,9 @@ export const DependencyIds = {
   mailer: Symbol.for('mailer'),
   cryptography: Symbol.for('cryptography'),
   s3: Symbol.for('s3'),
+  s3Bucket: Symbol.for('s3Bucket'),
+  s3Client: Symbol.for('s3Client'),
+  fileURLPrefix: Symbol.for('fileURLPrefix'),
 }
 
 export default DependencyIds

src/server/inversify.config.ts

@@ -1,3 +1,5 @@
+import AWS from 'aws-sdk'
+
 import { container } from './util/inversify'
 import { UrlCacheRedis } from './api/cache/url'
 import { UrlRepositorySequelize } from './api/repositories/url'
@@ -8,12 +10,14 @@ import { OtpCacheRedis } from './api/cache/otp'
 import { UserRepositorySequelize } from './api/repositories/user'
 import { MailerNode } from './util/email'
 import { CryptographyBcrypt } from './util/cryptography'
-import { DEV_ENV } from './config'
+import { DEV_ENV, accessEndpoint, bucketEndpoint, s3Bucket } from './config'
 import { MailerNoOp } from './util/emaildev'
 import { S3ServerSide } from './util/aws'
-import { S3LocalDev } from './util/localstack'
 
-function bindIfUnbound<T>(dependencyId: symbol, impl: { new (): T }) {
+function bindIfUnbound<T>(
+  dependencyId: symbol,
+  impl: { new (...args: any[]): T },
+) {
   if (!container.isBound(dependencyId)) {
     container.bind(dependencyId).to(impl)
   }
@@ -28,11 +32,28 @@ export default () => {
   bindIfUnbound(DependencyIds.userRepository, UserRepositorySequelize)
   bindIfUnbound(DependencyIds.cryptography, CryptographyBcrypt)
 
+  container.bind(DependencyIds.s3Bucket).toConstantValue(s3Bucket)
+
   if (DEV_ENV) {
+    const s3Client = new AWS.S3({
+      credentials: {
+        accessKeyId: 'foobar',
+        secretAccessKey: 'foobar',
+      },
+      endpoint: bucketEndpoint,
+      s3ForcePathStyle: true,
+    })
+
     bindIfUnbound(DependencyIds.mailer, MailerNoOp)
-    bindIfUnbound(DependencyIds.s3, S3LocalDev)
+    container
+      .bind(DependencyIds.fileURLPrefix)
+      .toConstantValue(`${accessEndpoint}/`)
+    container.bind(DependencyIds.s3Client).toConstantValue(s3Client)
   } else {
     bindIfUnbound(DependencyIds.mailer, MailerNode)
-    bindIfUnbound(DependencyIds.s3, S3ServerSide)
+    container.bind(DependencyIds.fileURLPrefix).toConstantValue('https://')
+    container.bind(DependencyIds.s3Client).toConstantValue(new AWS.S3())
   }
+
+  bindIfUnbound(DependencyIds.s3, S3ServerSide)
 }

src/server/util/aws.ts

@@ -1,16 +1,14 @@
 import { AWSError, S3 } from 'aws-sdk'
 import { PromiseResult } from 'aws-sdk/lib/request'
-import { injectable } from 'inversify'
-import { s3Bucket } from '../config'
+import { injectable, inject } from 'inversify'
+import DependencyIds from '../constants'
 
 // Enums for S3 object ACL toggling. Do not change string representations.
 export enum FileVisibility {
   Public = 'public-read',
   Private = 'private',
 }
 
-export const s3 = new S3()
-
 export interface S3Interface {
   setS3ObjectACL: (
     key: string,
@@ -29,16 +27,30 @@ export interface S3Interface {
 /* eslint class-methods-use-this: ["error", { "exceptMethods":
   ["setS3ObjectACL", "uploadFileToS3", "buildFileLongUrl", "getKeyFromLongUrl"] }] */
 export class S3ServerSide implements S3Interface {
+  private s3Client: S3
+  private s3Bucket: string
+  private fileURLPrefix: string
+
+  constructor(
+    @inject(DependencyIds.s3Client) s3Client: S3,
+    @inject(DependencyIds.s3Bucket) s3Bucket: string,
+    @inject(DependencyIds.fileURLPrefix) fileURLPrefix: string
+  ) {
+    this.s3Client = s3Client
+    this.s3Bucket = s3Bucket
+    this.fileURLPrefix = fileURLPrefix
+  }
+
   setS3ObjectACL(
     key: string,
     acl: FileVisibility,
   ): Promise<PromiseResult<S3.PutObjectAclOutput, AWSError>> {
     const params = {
-      Bucket: s3Bucket,
+      Bucket: this.s3Bucket,
       Key: key,
       ACL: acl,
     }
-    const result = s3.putObjectAcl(params).promise()
+    const result = this.s3Client.putObjectAcl(params).promise()
     return result
   }
 
@@ -49,17 +61,17 @@ export class S3ServerSide implements S3Interface {
   ): Promise<PromiseResult<S3.PutObjectOutput, AWSError>> {
     const params = {
       ContentType: fileType,
-      Bucket: s3Bucket,
+      Bucket: this.s3Bucket,
       Body: file,
       Key: key,
       ACL: FileVisibility.Public,
       CacheControl: `no-cache`,
     }
-    return s3.putObject(params).promise()
+    return this.s3Client.putObject(params).promise()
   }
 
   buildFileLongUrl(key: string): string {
-    return `https://${s3Bucket}/${key}`
+    return `${this.fileURLPrefix}${this.s3Bucket}/${key}`
   }
 
   getKeyFromLongUrl(longUrl: string): string {

test/server/util/aws.test.ts

@@ -1,18 +1,82 @@
 import { container } from '../../../src/server/util/inversify'
-import { S3Interface, S3ServerSide } from '../../../src/server/util/aws'
+import {
+  FileVisibility,
+  S3Interface,
+  S3ServerSide,
+} from '../../../src/server/util/aws'
 import { DependencyIds } from '../../../src/server/constants'
 
-describe('S3 link validator', () => {
+describe('S3ServerSide', () => {
+  const fileURLPrefix = 'https://'
+  const s3Bucket = 'file-staging.go.gov.sg'
+
+  const MockS3 = jest.fn(() => ({
+    putObjectAcl: jest.fn(() => ({ promise: jest.fn() })),
+    putObject: jest.fn(() => ({ promise: jest.fn() })),
+  }))
+  let mockS3Client = new MockS3()
+
   afterEach(() => {
     container.unbindAll()
   })
   beforeEach(() => {
+    mockS3Client = new MockS3()
+    container.bind(DependencyIds.fileURLPrefix).toConstantValue(fileURLPrefix)
+    container.bind(DependencyIds.s3Bucket).toConstantValue(s3Bucket)
+    container.bind(DependencyIds.s3Client).toConstantValue(mockS3Client)
     container.bind<S3Interface>(DependencyIds.s3).to(S3ServerSide)
   })
+
+  it('should call s3Client.putObjectAcl on setS3ObjectACL', () => {
+    const key = 'key'
+    const acl = FileVisibility.Private
+    const s3 = container.get<S3Interface>(DependencyIds.s3)
+    s3.setS3ObjectACL(key, acl)
+    expect(mockS3Client.putObjectAcl).toHaveBeenCalled()
+    expect(mockS3Client.putObjectAcl).toHaveBeenCalledWith({
+      Bucket: s3Bucket,
+      Key: key,
+      ACL: acl,
+    })
+  })
+
+  it('should call s3Client.putObject on uploadFileToS3', () => {
+    const file = Buffer.from([])
+    const key = 'key'
+    const fileType = 'type'
+
+    const s3 = container.get<S3Interface>(DependencyIds.s3)
+    s3.uploadFileToS3(file, key, fileType)
+    expect(mockS3Client.putObject).toHaveBeenCalled()
+    expect(mockS3Client.putObject).toHaveBeenCalledWith({
+      ContentType: fileType,
+      Bucket: s3Bucket,
+      Body: file,
+      Key: key,
+      ACL: FileVisibility.Public,
+      CacheControl: `no-cache`,
+    })
+  })
+
   it('should return correct file link when provided with a short url', () => {
     const shortUrl = 'test'
-    const validLink = `https://file-staging.go.gov.sg/${shortUrl}`
-    const { buildFileLongUrl } = container.get<S3Interface>(DependencyIds.s3)
-    expect(buildFileLongUrl(shortUrl)).toBe(validLink)
+    const validLink = `${fileURLPrefix}${s3Bucket}/${shortUrl}`
+    const s3 = container.get<S3Interface>(DependencyIds.s3)
+    expect(s3.buildFileLongUrl(shortUrl)).toBe(validLink)
+  })
+
+  describe('getKeyFromLongUrl', () => {
+    it('should return correct key when provided a long URL', () => {
+      const shortUrl = 'test'
+      const validLink = `${fileURLPrefix}${s3Bucket}/${shortUrl}`
+      const s3 = container.get<S3Interface>(DependencyIds.s3)
+      expect(s3.getKeyFromLongUrl(validLink)).toBe(shortUrl)
+    })
+
+    it('should throw when provided a long URL', () => {
+      const validLink = `${fileURLPrefix}${s3Bucket}/`
+      const s3 = container.get<S3Interface>(DependencyIds.s3)
+      expect(() => s3.getKeyFromLongUrl(validLink)).toThrow('Invalid URL')
+    })
   })
 })