fix: AssetPacks router

src/Asset/Asset.model.ts

@@ -32,11 +32,37 @@ export class Asset extends Model<AssetAttributes> {
     return counts[0].count > 0
   }
 
-  static findByIds(ids: string[]) {
-    return this.query<AssetAttributes>(SQL`
-    SELECT *
+  static findByIds(ids: string[], ethAddresses?: string[]) {
+    const query = SQL`
+    SELECT a.* FROM ${SQL.raw(this.tableName)} a`
+
+    if (ethAddresses) {
+      query.append(SQL`
+      INNER JOIN ${SQL.raw(AssetPack.tableName)} ap ON a.asset_pack_id = ap.id
+      WHERE a.id = ANY(${ids})
+        AND ap.eth_address = ANY(${ethAddresses})
+        AND ap.is_deleted = FALSE`)
+    } else {
+      query.append(SQL`
+      INNER JOIN ${SQL.raw(AssetPack.tableName)} ap ON a.asset_pack_id = ap.id
+      WHERE a.id = ANY(${ids})
+        AND ap.is_deleted = FALSE`)
+    }
+
+    return this.query<AssetAttributes>(query)
+  }
+
+  static async existsAnyWithADifferentAssetPackId(
+    ids: string[],
+    assetPackId: string
+  ): Promise<boolean> {
+    const counts = await this.query(SQL`
+    SELECT COUNT(*) as count
       FROM ${SQL.raw(this.tableName)}
-      WHERE id = ANY(${ids})`)
+      WHERE id = ANY(${ids})
+        AND asset_pack_id != ${assetPackId}`)
+
+    return counts[0].count > 0
   }
 
   static findByAssetPackId(assetPackId: string, limit: number | null = null) {

src/Asset/Asset.router.spec.ts

@@ -0,0 +1,149 @@
+import { ExpressApp } from '../common/ExpressApp'
+import { AssetRouter } from './Asset.router'
+import { Asset } from './Asset.model'
+import { getDefaultEthAddress } from '../AssetPack/utils'
+
+jest.mock('./Asset.model')
+jest.mock('../AssetPack/utils')
+
+const anOwnerAddress = 'anOwnerAddress'
+
+const anAsset = {
+  id: 'anAssetId',
+  asset_pack_id: 'anAssetPackId',
+  name: 'anAsset',
+  model: 'model.glb',
+  category: 'decorations',
+  contents: {},
+  tags: [],
+  metrics: {
+    triangles: 0,
+    materials: 0,
+    textures: 0,
+    meshes: 0,
+    bodies: 0,
+    entities: 0,
+  },
+}
+
+const anotherAsset = {
+  id: 'anotherAssetId',
+  asset_pack_id: 'anotherAssetPackId',
+  name: 'anotherAsset',
+  model: 'another.glb',
+  category: 'decorations',
+  contents: {},
+  tags: [],
+  metrics: {
+    triangles: 0,
+    materials: 0,
+    textures: 0,
+    meshes: 0,
+    bodies: 0,
+    entities: 0,
+  },
+}
+
+describe('Asset router', () => {
+  let router: AssetRouter
+  let req: {
+    params: Record<string, string>
+    query: Record<string, unknown>
+    auth: { ethAddress: string }
+  }
+
+  beforeEach(() => {
+    router = new AssetRouter(new ExpressApp())
+    req = { params: {}, query: {}, auth: { ethAddress: anOwnerAddress } }
+  })
+
+  afterEach(() => {
+    jest.resetAllMocks()
+  })
+
+  describe('when getting a single asset', () => {
+    beforeEach(() => {
+      req.params = { id: anAsset.id }
+    })
+
+    describe('when the asset belongs to the caller', () => {
+      beforeEach(() => {
+        ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([anAsset])
+      })
+
+      it('should call findByIds with the caller and default addresses', async () => {
+        await (router as any).getAsset(req)
+        expect(Asset.findByIds).toHaveBeenCalledWith(
+          [anAsset.id],
+          [anOwnerAddress, getDefaultEthAddress()]
+        )
+      })
+
+      it('should return the asset', async () => {
+        const result = await (router as any).getAsset(req)
+        expect(result).toEqual(anAsset)
+      })
+    })
+
+    describe('when the asset does not belong to the caller or the default address', () => {
+      beforeEach(() => {
+        ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([])
+      })
+
+      it('should return null', async () => {
+        const result = await (router as any).getAsset(req)
+        expect(result).toBeNull()
+      })
+    })
+  })
+
+  describe('when getting multiple assets', () => {
+    beforeEach(() => {
+      req.query = { id: [anAsset.id, anotherAsset.id] }
+    })
+
+    describe('when all assets belong to the caller or the default address', () => {
+      beforeEach(() => {
+        ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([
+          anAsset,
+          anotherAsset,
+        ])
+      })
+
+      it('should call findByIds with the caller and default addresses', async () => {
+        await (router as any).getAssets(req)
+        expect(Asset.findByIds).toHaveBeenCalledWith(
+          [anAsset.id, anotherAsset.id],
+          [anOwnerAddress, getDefaultEthAddress()]
+        )
+      })
+
+      it('should return all visible assets', async () => {
+        const result = await (router as any).getAssets(req)
+        expect(result).toEqual([anAsset, anotherAsset])
+      })
+    })
+
+    describe('when some assets belong to another user', () => {
+      beforeEach(() => {
+        ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([anAsset])
+      })
+
+      it('should return only the visible assets', async () => {
+        const result = await (router as any).getAssets(req)
+        expect(result).toEqual([anAsset])
+      })
+    })
+
+    describe('when all assets belong to another user', () => {
+      beforeEach(() => {
+        ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([])
+      })
+
+      it('should return an empty array', async () => {
+        const result = await (router as any).getAssets(req)
+        expect(result).toEqual([])
+      })
+    })
+  })
+})

src/Asset/Asset.router.ts

@@ -10,8 +10,11 @@
 } from '../middleware'
 import { getUploader, S3Content } from '../S3'
 import { AssetPack } from '../AssetPack'
+import { getDefaultEthAddress } from '../AssetPack/utils'
 import { Asset } from './Asset.model'
-import { withAuthentication } from '../middleware/authentication'
+import { withAuthentication, AuthRequest } from '../middleware/authentication'
+
+const DEFAULT_ETH_ADDRESS = getDefaultEthAddress()
 
 export class AssetRouter extends Router {
   mount() {
@@ -54,14 +57,20 @@
     this.router.get(
       '/assets/:id',
       withCors,
+      withAuthentication,
       withAssetExists,
       server.handleRequest(this.getAsset)
     )
 
     /**
      * Get a multiple assets
      */
-    this.router.get('/assets', withCors, server.handleRequest(this.getAssets))
+    this.router.get(
+      '/assets',
+      withCors,
+      withAuthentication,
+      server.handleRequest(this.getAssets)
+    )
   }
 
   async assetBelongsToPackMiddleware(req: Request) {
@@ -80,14 +89,20 @@
     // This handler is only here so `server.handleRequest` has a valid callback and it can return the appropiate formated response
   }
 
-  private getAsset(req: Request) {
+  private async getAsset(req: AuthRequest) {
     const id = server.extractFromReq(req, 'id')
-    return Asset.findOne(id)
+    const eth_address = req.auth.ethAddress
+    const [asset] = await Asset.findByIds(
+      [id],
+      [eth_address, DEFAULT_ETH_ADDRESS]
+    )
+    return asset ?? null
   }
 
-  private getAssets(req: Request) {
+  private async getAssets(req: AuthRequest) {
     const reqIds = server.extractFromReq<string | string[]>(req, 'id')
     const ids: string[] = Array.isArray(reqIds) ? reqIds : [reqIds]
-    return Asset.findByIds(ids)
+    const eth_address = req.auth.ethAddress
+    return Asset.findByIds(ids, [eth_address, DEFAULT_ETH_ADDRESS])
   }
 }

src/AssetPack/AssetPack.router.spec.ts

@@ -2,9 +2,11 @@ import { ILoggerComponent } from '@well-known-components/interfaces'
 import { ExpressApp } from '../common/ExpressApp'
 import { AssetPackRouter } from './AssetPack.router'
 import { AssetPack } from './AssetPack.model'
+import { Asset } from '../Asset'
 import { getDefaultEthAddress } from './utils'
 
 jest.mock('./AssetPack.model')
+jest.mock('../Asset/Asset.model')
 
 const anAssetPack = {
   id: 'anId',
@@ -375,4 +377,85 @@ describe('AssetPack router', () => {
       })
     })
   })
+
+  describe('when upserting an asset pack', () => {
+    const anAssetPackId = '8c251928-fb34-40e9-86d2-868a60d2fa78'
+    const anotherAssetPackId = '49c9ae13-6779-4ced-b208-dd352c9b7541'
+    const anOwnerAddress = 'anOwnerAddress'
+    let upsertReq: any
+    let assetUpsertSpy: jest.SpyInstance
+    let upsertedAssetAttrs: any[]
+
+    beforeEach(() => {
+      upsertedAssetAttrs = []
+      const mockUpsert = jest.fn().mockResolvedValue({})
+      ;((Asset as unknown) as jest.Mock).mockImplementation((attrs: any) => {
+        upsertedAssetAttrs.push(attrs)
+        return { upsert: mockUpsert, attributes: attrs }
+      })
+      assetUpsertSpy = mockUpsert
+      ;(AssetPack.count as jest.Mock).mockResolvedValue(0)
+      ;(Asset.existsAnyWithADifferentEthAddress as jest.Mock).mockResolvedValue(
+        false
+      )
+      ;(Asset.existsAnyWithADifferentAssetPackId as jest.Mock).mockResolvedValue(
+        false
+      )
+      ;(AssetPack.findOneWithAssets as jest.Mock).mockResolvedValue(null)
+      ;((AssetPack as unknown) as jest.Mock).mockImplementation(
+        (attrs: any) => ({
+          upsert: jest.fn().mockResolvedValue(attrs),
+          attributes: attrs,
+        })
+      )
+    })
+
+    describe('when an asset already belongs to a different asset pack', () => {
+      beforeEach(() => {
+        ;(Asset.existsAnyWithADifferentAssetPackId as jest.Mock).mockResolvedValue(
+          true
+        )
+        upsertReq = {
+          params: { id: anAssetPackId },
+          body: {
+            assetPack: {
+              id: anAssetPackId,
+              title: 'an-asset-pack',
+              assets: [
+                {
+                  id: '1e27cbda-5582-4219-8f83-2db817344cc1',
+                  asset_pack_id: anotherAssetPackId,
+                  name: 'an-asset',
+                  model: 'model.glb',
+                  category: 'decorations',
+                  contents: {},
+                  tags: ['test'],
+                  metrics: {
+                    triangles: 0,
+                    materials: 0,
+                    textures: 0,
+                    meshes: 0,
+                    bodies: 0,
+                    entities: 0,
+                  },
+                },
+              ],
+            },
+          },
+          auth: { ethAddress: anOwnerAddress },
+        }
+      })
+
+      it('should throw an error', async () => {
+        await expect(router.upsertAssetPack(upsertReq)).rejects.toThrow(
+          "One of the assets you're trying to upload is invalid"
+        )
+      })
+
+      it('should not upsert any assets', async () => {
+        await expect(router.upsertAssetPack(upsertReq)).rejects.toThrow()
+        expect(assetUpsertSpy).not.toHaveBeenCalled()
+      })
+    })
+  })
 })

src/AssetPack/AssetPack.router.ts

@@ -279,8 +279,17 @@ export class AssetPackRouter extends Router {
     const assetIds = assets.map((asset) => asset.id)
     if (await Asset.existsAnyWithADifferentEthAddress(assetIds, eth_address)) {
       throw new HTTPError(
-        "One of the assets you're trying to upload belongs to a different address. Check the ids",
-        { eth_address, assetIds }
+        "One of the assets you're trying to upload is invalid",
+        {},
+        STATUS_CODES.badRequest
+      )
+    }
+
+    if (await Asset.existsAnyWithADifferentAssetPackId(assetIds, id)) {
+      throw new HTTPError(
+        "One of the assets you're trying to upload is invalid",
+        {},
+        STATUS_CODES.badRequest
       )
     }