From 3c016314e6e4752b825d9c3e3fb04d4148ce3c71 Mon Sep 17 00:00:00 2001 From: mikiher Date: Wed, 15 Jul 2026 20:34:02 +0300 Subject: [PATCH 1/3] fix(auth): prevent admin users from deleting the root account --- server/controllers/UserController.js | 9 +- server/models/User.js | 9 +- .../server/controllers/UserController.test.js | 177 ++++++++++++++++++ 3 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 test/server/controllers/UserController.test.js diff --git a/server/controllers/UserController.js b/server/controllers/UserController.js index 3ec10539e..c0cbfc747 100644 --- a/server/controllers/UserController.js +++ b/server/controllers/UserController.js @@ -363,7 +363,13 @@ class UserController { * @param {Response} res */ async delete(req, res) { - if (req.params.id === 'root') { + const user = req.reqUser + + if (user.isRoot && !req.user.isRoot) { + Logger.error(`[UserController] Admin user "${req.user.username}" attempted to delete root user`) + return res.sendStatus(403) + } + if (user.isRoot) { Logger.error('[UserController] Attempt to delete root user. Root user cannot be deleted') return res.sendStatus(400) } @@ -371,7 +377,6 @@ class UserController { Logger.error(`[UserController] User ${req.user.username} is attempting to delete self`) return res.sendStatus(400) } - const user = req.reqUser // Todo: check if user is logged in and cancel streams diff --git a/server/models/User.js b/server/models/User.js index f380f8e4f..0b9d49438 100644 --- a/server/models/User.js +++ b/server/models/User.js @@ -530,7 +530,14 @@ class User extends Model { }, { sequelize, - modelName: 'user' + modelName: 'user', + hooks: { + beforeDestroy(user) { + if (user.type === 'root') { + throw new Error('Root user cannot be deleted') + } + } + } } ) } diff --git a/test/server/controllers/UserController.test.js b/test/server/controllers/UserController.test.js new file mode 100644 index 000000000..629788d84 --- /dev/null +++ b/test/server/controllers/UserController.test.js @@ -0,0 +1,177 @@ +const { expect } = require('chai') +const { Sequelize } = require('sequelize') +const sinon = require('sinon') + +const Database = require('../../../server/Database') +const UserController = require('../../../server/controllers/UserController') +const Logger = require('../../../server/Logger') +const SocketAuthority = require('../../../server/SocketAuthority') + +function createFakeRes() { + return { + sendStatus: sinon.spy(), + status: sinon.stub().returnsThis(), + send: sinon.spy(), + json: sinon.spy() + } +} + +describe('UserController - delete root protection', () => { + let rootUser + let adminUser + let regularUser + + beforeEach(async () => { + global.ServerSettings = {} + Database.sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }) + Database.sequelize.uppercaseFirst = (str) => (str ? `${str[0].toUpperCase()}${str.substr(1)}` : '') + await Database.buildModels() + + sinon.stub(Logger, 'info') + sinon.stub(Logger, 'error') + sinon.stub(SocketAuthority, 'adminEmitter') + + rootUser = await Database.userModel.create({ + username: 'root', + pash: 'hashed_password_root', + type: 'root', + isActive: true + }) + + adminUser = await Database.userModel.create({ + username: 'admin', + pash: 'hashed_password_admin', + type: 'admin', + isActive: true + }) + + regularUser = await Database.userModel.create({ + username: 'regular', + pash: 'hashed_password_regular', + type: 'user', + isActive: true + }) + }) + + afterEach(async () => { + sinon.restore() + await Database.sequelize.sync({ force: true }) + }) + + it('should prevent admin from deleting root user by UUID (403)', async () => { + const fakeReq = { + user: adminUser, + reqUser: rootUser, + params: { id: rootUser.id } + } + const fakeRes = createFakeRes() + + await UserController.delete(fakeReq, fakeRes) + + expect(fakeRes.sendStatus.calledWith(403)).to.be.true + expect(fakeRes.json.called).to.be.false + + const existingRoot = await Database.userModel.findByPk(rootUser.id) + expect(existingRoot).to.not.be.null + expect(existingRoot.type).to.equal('root') + }) + + it('should prevent root from deleting root user (400)', async () => { + const fakeReq = { + user: rootUser, + reqUser: rootUser, + params: { id: rootUser.id } + } + const fakeRes = createFakeRes() + + await UserController.delete(fakeReq, fakeRes) + + expect(fakeRes.sendStatus.calledWith(400)).to.be.true + expect(fakeRes.json.called).to.be.false + + const existingRoot = await Database.userModel.findByPk(rootUser.id) + expect(existingRoot).to.not.be.null + }) + + it('should not block deletion when URL param is literal "root" but target is a different user', async () => { + const fakeReq = { + user: adminUser, + reqUser: regularUser, + params: { id: 'root' } + } + const fakeRes = createFakeRes() + + await UserController.delete(fakeReq, fakeRes) + + expect(fakeRes.json.calledWith({ success: true })).to.be.true + + const deletedUser = await Database.userModel.findByPk(regularUser.id) + expect(deletedUser).to.be.null + }) + + it('should allow admin to delete a regular user (200)', async () => { + const fakeReq = { + user: adminUser, + reqUser: regularUser, + params: { id: regularUser.id } + } + const fakeRes = createFakeRes() + + await UserController.delete(fakeReq, fakeRes) + + expect(fakeRes.json.calledWith({ success: true })).to.be.true + expect(SocketAuthority.adminEmitter.calledWith('user_removed')).to.be.true + + const deletedUser = await Database.userModel.findByPk(regularUser.id) + expect(deletedUser).to.be.null + }) + + it('should prevent admin from deleting self (400)', async () => { + const fakeReq = { + user: adminUser, + reqUser: adminUser, + params: { id: adminUser.id } + } + const fakeRes = createFakeRes() + + await UserController.delete(fakeReq, fakeRes) + + expect(fakeRes.sendStatus.calledWith(400)).to.be.true + expect(fakeRes.json.called).to.be.false + + const existingAdmin = await Database.userModel.findByPk(adminUser.id) + expect(existingAdmin).to.not.be.null + }) +}) + +describe('User model - beforeDestroy root protection', () => { + beforeEach(async () => { + global.ServerSettings = {} + Database.sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }) + Database.sequelize.uppercaseFirst = (str) => (str ? `${str[0].toUpperCase()}${str.substr(1)}` : '') + await Database.buildModels() + }) + + afterEach(async () => { + await Database.sequelize.sync({ force: true }) + }) + + it('should reject direct destroy of root user', async () => { + const rootUser = await Database.userModel.create({ + username: 'root', + pash: 'hashed_password_root', + type: 'root', + isActive: true + }) + + try { + await rootUser.destroy() + expect.fail('Expected destroy to throw') + } catch (error) { + expect(error.message).to.equal('Root user cannot be deleted') + } + + const existingRoot = await Database.userModel.findByPk(rootUser.id) + expect(existingRoot).to.not.be.null + }) +}) From bced3084626c7f2a16900e32a260e957c017c25e Mon Sep 17 00:00:00 2001 From: advplyr Date: Mon, 20 Jul 2026 17:31:03 -0500 Subject: [PATCH 2/3] Trim down UserController test --- .../server/controllers/UserController.test.js | 167 +----------------- 1 file changed, 8 insertions(+), 159 deletions(-) diff --git a/test/server/controllers/UserController.test.js b/test/server/controllers/UserController.test.js index 629788d84..add025d6e 100644 --- a/test/server/controllers/UserController.test.js +++ b/test/server/controllers/UserController.test.js @@ -1,177 +1,26 @@ const { expect } = require('chai') -const { Sequelize } = require('sequelize') const sinon = require('sinon') -const Database = require('../../../server/Database') const UserController = require('../../../server/controllers/UserController') const Logger = require('../../../server/Logger') -const SocketAuthority = require('../../../server/SocketAuthority') -function createFakeRes() { - return { - sendStatus: sinon.spy(), - status: sinon.stub().returnsThis(), - send: sinon.spy(), - json: sinon.spy() - } -} - -describe('UserController - delete root protection', () => { - let rootUser - let adminUser - let regularUser - - beforeEach(async () => { - global.ServerSettings = {} - Database.sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }) - Database.sequelize.uppercaseFirst = (str) => (str ? `${str[0].toUpperCase()}${str.substr(1)}` : '') - await Database.buildModels() - - sinon.stub(Logger, 'info') +describe('UserController - delete', () => { + beforeEach(() => { sinon.stub(Logger, 'error') - sinon.stub(SocketAuthority, 'adminEmitter') - - rootUser = await Database.userModel.create({ - username: 'root', - pash: 'hashed_password_root', - type: 'root', - isActive: true - }) - - adminUser = await Database.userModel.create({ - username: 'admin', - pash: 'hashed_password_admin', - type: 'admin', - isActive: true - }) - - regularUser = await Database.userModel.create({ - username: 'regular', - pash: 'hashed_password_regular', - type: 'user', - isActive: true - }) }) - afterEach(async () => { + afterEach(() => { sinon.restore() - await Database.sequelize.sync({ force: true }) }) - it('should prevent admin from deleting root user by UUID (403)', async () => { - const fakeReq = { - user: adminUser, - reqUser: rootUser, - params: { id: rootUser.id } - } - const fakeRes = createFakeRes() + it('rejects deleting the root user by UUID', async () => { + const rootUser = { id: 'root-uuid', isRoot: true, username: 'root' } + const adminUser = { id: 'admin-uuid', isRoot: false, username: 'admin' } + const fakeRes = { sendStatus: sinon.spy(), json: sinon.spy() } - await UserController.delete(fakeReq, fakeRes) + await UserController.delete({ user: adminUser, reqUser: rootUser, params: { id: rootUser.id } }, fakeRes) expect(fakeRes.sendStatus.calledWith(403)).to.be.true expect(fakeRes.json.called).to.be.false - - const existingRoot = await Database.userModel.findByPk(rootUser.id) - expect(existingRoot).to.not.be.null - expect(existingRoot.type).to.equal('root') - }) - - it('should prevent root from deleting root user (400)', async () => { - const fakeReq = { - user: rootUser, - reqUser: rootUser, - params: { id: rootUser.id } - } - const fakeRes = createFakeRes() - - await UserController.delete(fakeReq, fakeRes) - - expect(fakeRes.sendStatus.calledWith(400)).to.be.true - expect(fakeRes.json.called).to.be.false - - const existingRoot = await Database.userModel.findByPk(rootUser.id) - expect(existingRoot).to.not.be.null - }) - - it('should not block deletion when URL param is literal "root" but target is a different user', async () => { - const fakeReq = { - user: adminUser, - reqUser: regularUser, - params: { id: 'root' } - } - const fakeRes = createFakeRes() - - await UserController.delete(fakeReq, fakeRes) - - expect(fakeRes.json.calledWith({ success: true })).to.be.true - - const deletedUser = await Database.userModel.findByPk(regularUser.id) - expect(deletedUser).to.be.null - }) - - it('should allow admin to delete a regular user (200)', async () => { - const fakeReq = { - user: adminUser, - reqUser: regularUser, - params: { id: regularUser.id } - } - const fakeRes = createFakeRes() - - await UserController.delete(fakeReq, fakeRes) - - expect(fakeRes.json.calledWith({ success: true })).to.be.true - expect(SocketAuthority.adminEmitter.calledWith('user_removed')).to.be.true - - const deletedUser = await Database.userModel.findByPk(regularUser.id) - expect(deletedUser).to.be.null - }) - - it('should prevent admin from deleting self (400)', async () => { - const fakeReq = { - user: adminUser, - reqUser: adminUser, - params: { id: adminUser.id } - } - const fakeRes = createFakeRes() - - await UserController.delete(fakeReq, fakeRes) - - expect(fakeRes.sendStatus.calledWith(400)).to.be.true - expect(fakeRes.json.called).to.be.false - - const existingAdmin = await Database.userModel.findByPk(adminUser.id) - expect(existingAdmin).to.not.be.null - }) -}) - -describe('User model - beforeDestroy root protection', () => { - beforeEach(async () => { - global.ServerSettings = {} - Database.sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }) - Database.sequelize.uppercaseFirst = (str) => (str ? `${str[0].toUpperCase()}${str.substr(1)}` : '') - await Database.buildModels() - }) - - afterEach(async () => { - await Database.sequelize.sync({ force: true }) - }) - - it('should reject direct destroy of root user', async () => { - const rootUser = await Database.userModel.create({ - username: 'root', - pash: 'hashed_password_root', - type: 'root', - isActive: true - }) - - try { - await rootUser.destroy() - expect.fail('Expected destroy to throw') - } catch (error) { - expect(error.message).to.equal('Root user cannot be deleted') - } - - const existingRoot = await Database.userModel.findByPk(rootUser.id) - expect(existingRoot).to.not.be.null }) }) From f12abca3fce2ba4ffff47978d915fe86819bde6d Mon Sep 17 00:00:00 2001 From: advplyr Date: Mon, 20 Jul 2026 17:33:15 -0500 Subject: [PATCH 3/3] Simplify user delete checks --- server/controllers/UserController.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/server/controllers/UserController.js b/server/controllers/UserController.js index c0cbfc747..4a4da0366 100644 --- a/server/controllers/UserController.js +++ b/server/controllers/UserController.js @@ -365,18 +365,14 @@ class UserController { async delete(req, res) { const user = req.reqUser - if (user.isRoot && !req.user.isRoot) { - Logger.error(`[UserController] Admin user "${req.user.username}" attempted to delete root user`) - return res.sendStatus(403) - } - if (user.isRoot) { - Logger.error('[UserController] Attempt to delete root user. Root user cannot be deleted') - return res.sendStatus(400) - } if (req.user.id === req.params.id) { Logger.error(`[UserController] User ${req.user.username} is attempting to delete self`) return res.sendStatus(400) } + if (user.isRoot) { + Logger.error(`[UserController] Admin user "${req.user.username}" attempted to delete root user`) + return res.sendStatus(403) + } // Todo: check if user is logged in and cancel streams