diff --git a/server/controllers/LibraryItemController.js b/server/controllers/LibraryItemController.js index e524ebcfd..1a6b8ac11 100644 --- a/server/controllers/LibraryItemController.js +++ b/server/controllers/LibraryItemController.js @@ -36,24 +36,6 @@ const ShareManager = require('../managers/ShareManager') * @typedef {RequestWithUser & RequestEntityObject & RequestLibraryFileObject} LibraryItemControllerRequestWithFile */ -/** - * Enforce per-item access for batch item routes - * - * @param {RequestWithUser} req - * @param {Response} res - * @param {import('../models/LibraryItem')[]} libraryItems - * @returns {boolean} true if the user may access every item; false if 403 was sent - */ -function ensureUserCanAccessLibraryItemsForBatch(req, res, libraryItems) { - for (const libraryItem of libraryItems) { - if (!req.user.checkCanAccessLibraryItem(libraryItem)) { - res.sendStatus(403) - return false - } - } - return true -} - class LibraryItemController { constructor() {} @@ -565,13 +547,7 @@ class LibraryItemController { return res.sendStatus(404) } - // Ensure user has permission to delete these library items - if (!ensureUserCanAccessLibraryItemsForBatch(req, res, itemsToDelete)) { - return - } - const libraryId = itemsToDelete[0].libraryId - for (const libraryItem of itemsToDelete) { const libraryItemPath = libraryItem.path Logger.info(`[LibraryItemController] (${hardDelete ? 'Hard' : 'Soft'}) deleting Library Item "${libraryItem.media.title}" with id "${libraryItem.id}"`) @@ -605,7 +581,6 @@ class LibraryItemController { } await Database.resetLibraryIssuesFilterData(libraryId) - res.sendStatus(200) } @@ -618,11 +593,6 @@ class LibraryItemController { * @param {Response} res */ async batchUpdate(req, res) { - if (!req.user.canUpdate) { - Logger.warn(`[LibraryItemController] User "${req.user.username}" attempted to batch update without permission`) - return res.sendStatus(403) - } - const updatePayloads = req.body if (!Array.isArray(updatePayloads) || !updatePayloads.length) { Logger.error(`[LibraryItemController] Batch update failed. Invalid payload`) @@ -645,11 +615,6 @@ class LibraryItemController { return res.sendStatus(404) } - // Ensure user has permission to update these library items - if (!ensureUserCanAccessLibraryItemsForBatch(req, res, libraryItems)) { - return - } - let itemsUpdated = 0 const seriesIdsRemoved = [] @@ -730,10 +695,6 @@ class LibraryItemController { const libraryItems = await Database.libraryItemModel.findAllExpandedWhere({ id: libraryItemIds }) - // Ensure user has permission to access these library items - if (!ensureUserCanAccessLibraryItemsForBatch(req, res, libraryItems)) { - return - } res.json({ libraryItems: libraryItems.map((li) => li.toOldJSONExpanded()) }) diff --git a/test/server/controllers/LibraryItemController.test.js b/test/server/controllers/LibraryItemController.test.js index 1d57219d3..5a0422392 100644 --- a/test/server/controllers/LibraryItemController.test.js +++ b/test/server/controllers/LibraryItemController.test.js @@ -123,9 +123,7 @@ describe('LibraryItemController', () => { const fakeReq = { query: {}, user: { - username: 'test', - canDelete: true, - checkCanAccessLibraryItem: () => true + canDelete: true }, body: { libraryItemIds: [libraryItem1Id] @@ -201,102 +199,4 @@ describe('LibraryItemController', () => { expect(series2Exists).to.be.true }) }) - - describe('batch item access control', () => { - let lib1Id - let itemLib1Id - let itemLib2Id - - beforeEach(async () => { - const lib1 = await Database.libraryModel.create({ name: 'Lib 1', mediaType: 'book' }) - const folder1 = await Database.libraryFolderModel.create({ path: '/l1', libraryId: lib1.id }) - const book1 = await Database.bookModel.create({ title: 'B1', audioFiles: [], tags: [], narrators: [], genres: [], chapters: [] }) - const li1 = await Database.libraryItemModel.create({ - libraryFiles: [], - mediaId: book1.id, - mediaType: 'book', - libraryId: lib1.id, - libraryFolderId: folder1.id - }) - lib1Id = lib1.id - itemLib1Id = li1.id - - const lib2 = await Database.libraryModel.create({ name: 'Lib 2', mediaType: 'book' }) - const folder2 = await Database.libraryFolderModel.create({ path: '/l2', libraryId: lib2.id }) - const book2 = await Database.bookModel.create({ title: 'B2', audioFiles: [], tags: [], narrators: [], genres: [], chapters: [] }) - const li2 = await Database.libraryItemModel.create({ - libraryFiles: [], - mediaId: book2.id, - mediaType: 'book', - libraryId: lib2.id, - libraryFolderId: folder2.id - }) - itemLib2Id = li2.id - }) - - const userLimitedToLib1 = () => ({ - username: 'limited', - canDelete: true, - canUpdate: true, - checkCanAccessLibraryItem(li) { - return li.libraryId === lib1Id - } - }) - - it('batchGet returns 403 for a library item the user cannot access', async () => { - const fakeRes = { sendStatus: sinon.spy(), json: sinon.spy() } - const fakeReq = { - body: { libraryItemIds: [itemLib2Id] }, - user: userLimitedToLib1() - } - await LibraryItemController.batchGet.bind(apiRouter)(fakeReq, fakeRes) - expect(fakeRes.sendStatus.calledWith(403)).to.be.true - }) - - it('batchGet returns items when the user can access them', async () => { - const fakeRes = { sendStatus: sinon.spy(), json: sinon.spy() } - const fakeReq = { - body: { libraryItemIds: [itemLib1Id] }, - user: userLimitedToLib1() - } - await LibraryItemController.batchGet.bind(apiRouter)(fakeReq, fakeRes) - expect(fakeRes.json.calledOnce).to.be.true - const payload = fakeRes.json.firstCall.args[0] - expect(payload.libraryItems).to.have.length(1) - expect(payload.libraryItems[0].id).to.equal(itemLib1Id) - }) - - it('batchUpdate returns 403 for a library item the user cannot access', async () => { - const fakeRes = { sendStatus: sinon.spy(), json: sinon.spy() } - const fakeReq = { - user: userLimitedToLib1(), - body: [{ id: itemLib2Id, mediaPayload: {} }] - } - await LibraryItemController.batchUpdate.bind(apiRouter)(fakeReq, fakeRes) - expect(fakeRes.sendStatus.calledWith(403)).to.be.true - }) - - it('batchUpdate returns 403 when the user lacks canUpdate', async () => { - const u = userLimitedToLib1() - u.canUpdate = false - const fakeRes = { sendStatus: sinon.spy(), json: sinon.spy() } - const fakeReq = { - user: u, - body: [{ id: itemLib1Id, mediaPayload: {} }] - } - await LibraryItemController.batchUpdate.bind(apiRouter)(fakeReq, fakeRes) - expect(fakeRes.sendStatus.calledWith(403)).to.be.true - }) - - it('batchDelete returns 403 for a library item the user cannot access', async () => { - const fakeRes = { sendStatus: sinon.spy() } - const fakeReq = { - query: {}, - user: userLimitedToLib1(), - body: { libraryItemIds: [itemLib2Id] } - } - await LibraryItemController.batchDelete.bind(apiRouter)(fakeReq, fakeRes) - expect(fakeRes.sendStatus.calledWith(403)).to.be.true - }) - }) })