From 18d10507892ef2b47b9f69086842321d540bf5c2 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 5 Feb 2026 15:43:37 +0100 Subject: [PATCH 1/6] Increase restriction on GET lock API to role-detector --- Control/lib/api.js | 5 ++++- Control/test/api/lock/api-get-locks.test.js | 12 +++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/Control/lib/api.js b/Control/lib/api.js index 56caeb28a..09b086181 100644 --- a/Control/lib/api.js +++ b/Control/lib/api.js @@ -247,7 +247,10 @@ module.exports.setup = (http, ws) => { http.post('/execute/o2-roc-config', coreMiddleware, (req, res) => ctrlService.createAutoEnvironment(req, res)); // Lock Service - http.get('/locks', lockController.getLocksStateHandler.bind(lockController)); + http.get('/locks', + minimumRoleMiddleware(Role.DETECTOR), + lockController.getLocksStateHandler.bind(lockController) + ); http.put(`/locks/:action/${DetectorId.ALL}`, minimumRoleMiddleware(Role.GLOBAL), diff --git a/Control/test/api/lock/api-get-locks.test.js b/Control/test/api/lock/api-get-locks.test.js index 6276ba311..1509b697d 100644 --- a/Control/test/api/lock/api-get-locks.test.js +++ b/Control/test/api/lock/api-get-locks.test.js @@ -14,7 +14,7 @@ */ const request = require('supertest'); -const { ADMIN_TEST_TOKEN, TEST_URL } = require('../generateToken.js'); +const { ADMIN_TEST_TOKEN, GUEST_TEST_TOKEN, TEST_URL } = require('../generateToken.js'); describe(`'API - GET - /locks' test suite`, () => { before(async () => { @@ -50,4 +50,14 @@ describe(`'API - GET - /locks' test suite`, () => { message: 'Invalid JWT token provided' }); }); + + it('should return unauthorized error for insufficient role token requests', async () => { + await request(`${TEST_URL}/api/locks`) + .get(`/?token=${GUEST_TEST_TOKEN}`) + .expect(403, { + status: 403, + message: 'Not enough permissions for this operation', + title: 'Unauthorized Access', + }); + }); }); From 523301e703a5c86723762f1b3d7ba4d0c3176669 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 5 Feb 2026 15:44:44 +0100 Subject: [PATCH 2/6] Add documentation on LOCKS --- Control/README.md | 2 ++ Control/docs/LOCKS.md | 69 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 Control/docs/LOCKS.md diff --git a/Control/README.md b/Control/README.md index 9bfe68f09..5982be415 100644 --- a/Control/README.md +++ b/Control/README.md @@ -8,6 +8,7 @@ - [Requirements](#requirements) - [Installation](#installation) - [Business logic for Developers to know](#business-logic-for-developers-to-know) + - [Locks](#locks) - [Configuration](#configuration) - [O2Control gRPC](#o2control-grpc) - [Apricot gRPC](#apricot-grpc) @@ -55,6 +56,7 @@ It communicates with [Control agent](https://github.com/AliceO2Group/Control) ov 7. Open browser and navigate to http://localhost:8080 ## [Business logic for Developers to know](./docs/BUSINESS_FOR_DEVELOPER_TO_KNOW.md) +## [Locks](./docs/LOCKS.md) ## Configuration ### O2Control gRPC * `hostname` - gRPC hostname diff --git a/Control/docs/LOCKS.md b/Control/docs/LOCKS.md new file mode 100644 index 000000000..62a4c18b8 --- /dev/null +++ b/Control/docs/LOCKS.md @@ -0,0 +1,69 @@ +# Locks + +## Overview + +The locks functionality in Control/ECS GUI provides a mechanism to ensure exclusive access to detectors during operations. This prevents conflicts when multiple users attempt to configure or control the same detector simultaneously. + +All LOCKs operations require an authenticated user session and moreover: +- Lock Operations require a role of minimum `Role.DETECTOR`. +- Lock Operations on `ALL` detectors require a minimum role of `Role.GLOBAL`. +- Forced Lock Operations require a role of minimum `Role.GLOBAL`. +- Forced Lock Operations on `ALL` detectors require a minimum role of `Role.ADMIN`. + +## Lock Management + +### Lock States + +Each detector can be in one of the following lock states: +- **Released**: The detector is available to be locked by any user +- **Taken**: The detector is currently locked by a specific user + +### Get Lock States + +### Take Lock + +Acquires a lock on a detector for the current user. + +**Parameters**: +- `detectorId`: The individual detector identifier or `ALL` to lock all detectors (excluding TST) +- `action`: `TAKE` +- `shouldForce` (optional): Boolean flag to force taking the lock even if held by another user + +**Behavior**: +- When `detectorId` is `ALL`, locks all detectors **except** TST +- If a lock is already held by another user and: + - `shouldForce` is `false`, the request will fail. + - `shouldForce` is `true` and the user has a role of at least `GLOBAL` for individual detector and at least `ADMIN` for detector `ALL`, the lock will be taken regardless of current ownership + +### Release Lock + +Releases a lock on a detector. + +**Parameters**: +- `detectorId`: The detector identifier or `ALL` to release all locks (excluding TST) +- `action`: `RELEASE` +- `shouldForce` (optional): Boolean flag to force releasing the lock even if held by another user + +**Behavior**: +- When `detectorId` is `ALL`, releases locks on all detectors **except** TST +- If a lock is already held by another user and: + - `shouldForce` is `false`, the request will fail. + - `shouldForce` is `true` and the user has a role of at least `GLOBAL` for individual detector and at least `ADMIN` for detector `ALL`, the lock will be released regardless of current ownership + +## Special Cases + +### TST Detector + +The TST (test) detector is treated specially: +- When using `ALL` as the detector ID, TST is excluded from both TAKE and RELEASE operations +- TST must be locked/unlocked explicitly by using `TST` as the detector ID +- Moreover, front-end pages are also: + - excluding TST from being displayed if on `Global` page + - displaying TST at the end with separator if on `+Create` or `Locks` pages + +### Error Handling + +The lock controller handles the following error cases: +- Missing `detectorId` parameter → Returns `InvalidInputError` +- Invalid `action` parameter → Returns `InvalidInputError` +- Lock conflicts (when not forcing) → Error from LockService From d8c032867406d746a311b517875c39eae70d5d0d Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 5 Feb 2026 15:45:57 +0100 Subject: [PATCH 3/6] Update logic for lock ALL --- Control/lib/common/detectorId.enum.js | 1 + Control/lib/controllers/Lock.controller.js | 14 ++- .../controllers/mocha-lock.controller.test.js | 108 +++++++++++++++--- 3 files changed, 104 insertions(+), 19 deletions(-) diff --git a/Control/lib/common/detectorId.enum.js b/Control/lib/common/detectorId.enum.js index f8541d847..9f8f1bc09 100644 --- a/Control/lib/common/detectorId.enum.js +++ b/Control/lib/common/detectorId.enum.js @@ -17,6 +17,7 @@ */ const DetectorId = Object.freeze({ ALL: 'ALL', + TST: 'TST', }); exports.DetectorId = DetectorId; diff --git a/Control/lib/controllers/Lock.controller.js b/Control/lib/controllers/Lock.controller.js index bc9b77a11..97d5fb190 100644 --- a/Control/lib/controllers/Lock.controller.js +++ b/Control/lib/controllers/Lock.controller.js @@ -16,10 +16,10 @@ const { LogManager, LogLevel } = require('@aliceo2/web-ui'); const { updateAndSendExpressResponseFromNativeError, InvalidInputError } = require('@aliceo2/web-ui'); const { DetectorLockAction } = require('./../common/lock/detectorLockAction.enum.js'); +const { DetectorId } = require('./../common/DetectorId.enum.js'); const {User} = require('./../dtos/User.js'); const LOG_FACILITY = 'cog/log-ctrl'; -const DETECTOR_ALL = 'ALL'; /** * Controller for dealing with all API requests on actions and state of the locks used for detectors @@ -70,8 +70,10 @@ class LockController { } const user = new User(username, name, personid, access); if (action.toLocaleUpperCase() === DetectorLockAction.TAKE) { - if (detectorId === DETECTOR_ALL) { - Object.keys(this._lockService.locksByDetector).forEach((detector) => { + if (detectorId === DetectorId.ALL) { + Object.keys(this._lockService.locksByDetector) + .filter((detector) => detector !== DetectorId.TST) // Skip TST detector when locking all + .forEach((detector) => { try { this._lockService.takeLock(detector, user, shouldForce); } catch (error) { @@ -83,8 +85,10 @@ class LockController { } res.status(200).json(this._lockService.locksByDetectorToJSON()); } else if (action.toLocaleUpperCase() === DetectorLockAction.RELEASE) { - if (detectorId === DETECTOR_ALL) { - Object.keys(this._lockService.locksByDetector).forEach((detector) => { + if (detectorId === DetectorId.ALL) { + Object.keys(this._lockService.locksByDetector) + .filter((detector) => detector !== DetectorId.TST) // Skip TST detector when releasing all + .forEach((detector) => { try { this._lockService.releaseLock(detector, user, shouldForce); } catch (error) { diff --git a/Control/test/lib/controllers/mocha-lock.controller.test.js b/Control/test/lib/controllers/mocha-lock.controller.test.js index 0b5377c03..776cea378 100644 --- a/Control/test/lib/controllers/mocha-lock.controller.test.js +++ b/Control/test/lib/controllers/mocha-lock.controller.test.js @@ -16,11 +16,12 @@ const assert = require('assert'); const sinon = require('sinon'); -const {LockController} = require('./../../../lib/controllers/Lock.controller.js'); -const {LockService} = require('./../../../lib/services/Lock.service.js'); -const {User} = require('./../../../lib/dtos/User.js'); -const {DetectorLockAction} = require('../../../lib/common/lock/detectorLockAction.enum.js'); -const {DetectorLockState} = require('../../../lib/common/lock/detectorLockState.enum.js'); +const { LockController } = require('./../../../lib/controllers/Lock.controller.js'); +const { LockService } = require('./../../../lib/services/Lock.service.js'); +const { User } = require('./../../../lib/dtos/User.js'); +const { DetectorLockAction } = require('../../../lib/common/lock/detectorLockAction.enum.js'); +const { DetectorLockState } = require('../../../lib/common/lock/detectorLockState.enum.js'); +const { DetectorId } = require('../../../lib/common/DetectorId.enum.js'); describe(`'LockController' test suite`, () => { const res = { @@ -41,7 +42,7 @@ describe(`'LockController' test suite`, () => { }); it('should successfully return the state of locks if service is enabled and with detectors', () => { - lockService.setLockStatesForDetectors(['ABC', 'XYZ']) + lockService.setLockStatesForDetectors(['ABC', 'XYZ', DetectorId.TST]) lockController.getLocksStateHandler({}, res); assert.ok(res.status.calledWith(200)); assert.ok(res.json.calledWith({ @@ -55,6 +56,11 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.FREE, + owner: undefined, + }, })); }); }); @@ -88,6 +94,11 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.FREE, + owner: undefined, + } })); }); @@ -152,6 +163,11 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.FREE, + owner: undefined, + }, })); }); @@ -209,12 +225,12 @@ describe(`'LockController' test suite`, () => { })); }); - it('should successfully lock all detectors available to lock', () => { + it(`should successfully lock all detectors available to lock but ${DetectorId.TST}`, () => { lockService.takeLock('ABC', new User('anonymous', 'Anonymous', 0, ['global'])); lockController.actionLockHandler({ params: { action: DetectorLockAction.TAKE, - detectorId: 'ALL' + detectorId: DetectorId.ALL }, session: { personid: 1, name: 'OneAnonymous', @@ -241,20 +257,38 @@ describe(`'LockController' test suite`, () => { username: 'oneanonymous', }, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.FREE, + owner: undefined, + }, })); }); - it('should successfully release all detectors available to lock', () => { + it(`should successfully release all detectors available to lock but ${DetectorId.TST}`, () => { + // take lock for TST to verify it is not released + lockController.actionLockHandler({ + params: { + action: DetectorLockAction.TAKE, + detectorId: DetectorId.TST, + }, session: { + personid: 1, + name: 'OneAnonymous', + username: 'oneanonymous', + } + }, res); + lockController.actionLockHandler({ params: { action: DetectorLockAction.RELEASE, - detectorId: 'ALL' + detectorId: DetectorId.ALL }, session: { personid: 1, name: 'OneAnonymous', username: 'oneanonymous', } }, res); + assert.ok(res.status.calledWith(200)); assert.ok(res.json.calledWith({ ABC: { @@ -271,6 +305,15 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.TAKEN, + owner: { + fullName: 'OneAnonymous', + personid: 1, + username: 'oneanonymous', + }, + }, })); }); }); @@ -304,6 +347,15 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.TAKEN, + owner: { + fullName: 'OneAnonymous', + personid: 1, + username: 'oneanonymous', + }, + }, })); res.status.resetHistory() res.json.resetHistory(); @@ -334,14 +386,23 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.TAKEN, + owner: { + fullName: 'OneAnonymous', + personid: 1, + username: 'oneanonymous', + }, + }, })); }); - it('should successfully respond to a request to take ALL locks by force', () => { + it('should successfully respond to a request to take ALL locks by force but TST', () => { lockController.actionForceLockHandler({ params: { action: DetectorLockAction.TAKE, - detectorId: 'ALL', + detectorId: DetectorId.ALL, }, session: { personid: 22, username: 'twotwoanonymous', @@ -369,15 +430,25 @@ describe(`'LockController' test suite`, () => { username: 'twotwoanonymous', } }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.TAKEN, + owner: { + fullName: 'OneAnonymous', + personid: 1, + username: 'oneanonymous', + }, + }, + })); }); - it('should successfully respond to a request to release ALL locks by force', () => { + it('should successfully respond to a request to release ALL locks by force but TST', () => { lockService.takeLock('ABC', new User('anonymous', 'Anonymous', 0, ['global']), true); // now one lock is under personid 0 lockController.actionForceLockHandler({ params: { action: DetectorLockAction.RELEASE, - detectorId: 'ALL', + detectorId: DetectorId.ALL, }, session: { personid: 22, username: 'twotwoanonymous', @@ -397,6 +468,15 @@ describe(`'LockController' test suite`, () => { state: DetectorLockState.FREE, owner: undefined, }, + TST: { + name: DetectorId.TST, + state: DetectorLockState.TAKEN, + owner: { + fullName: 'OneAnonymous', + personid: 1, + username: 'oneanonymous', + }, + }, })); }); }); From 62afb130e135f5ba159ade5e28d7994efa9930a2 Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 5 Feb 2026 15:50:17 +0100 Subject: [PATCH 4/6] Fix eslint reported errors --- Control/lib/controllers/Lock.controller.js | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/Control/lib/controllers/Lock.controller.js b/Control/lib/controllers/Lock.controller.js index 97d5fb190..d0132a728 100644 --- a/Control/lib/controllers/Lock.controller.js +++ b/Control/lib/controllers/Lock.controller.js @@ -74,12 +74,12 @@ class LockController { Object.keys(this._lockService.locksByDetector) .filter((detector) => detector !== DetectorId.TST) // Skip TST detector when locking all .forEach((detector) => { - try { - this._lockService.takeLock(detector, user, shouldForce); - } catch (error) { - console.error(error); - } - }); + try { + this._lockService.takeLock(detector, user, shouldForce); + } catch (error) { + console.error(error); + } + }); } else { this._lockService.takeLock(detectorId, user, shouldForce); } @@ -89,12 +89,12 @@ class LockController { Object.keys(this._lockService.locksByDetector) .filter((detector) => detector !== DetectorId.TST) // Skip TST detector when releasing all .forEach((detector) => { - try { - this._lockService.releaseLock(detector, user, shouldForce); - } catch (error) { - console.error(error); - } - }); + try { + this._lockService.releaseLock(detector, user, shouldForce); + } catch (error) { + console.error(error); + } + }); } else { this._lockService.releaseLock(detectorId, user, shouldForce); } From c8cf96d88cc9e1ae1d96e734afff2dd7f852230f Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 5 Feb 2026 15:53:08 +0100 Subject: [PATCH 5/6] Fix miss-spelled named import --- Control/lib/controllers/Lock.controller.js | 2 +- Control/test/lib/controllers/mocha-lock.controller.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Control/lib/controllers/Lock.controller.js b/Control/lib/controllers/Lock.controller.js index d0132a728..3cdc41ae6 100644 --- a/Control/lib/controllers/Lock.controller.js +++ b/Control/lib/controllers/Lock.controller.js @@ -16,7 +16,7 @@ const { LogManager, LogLevel } = require('@aliceo2/web-ui'); const { updateAndSendExpressResponseFromNativeError, InvalidInputError } = require('@aliceo2/web-ui'); const { DetectorLockAction } = require('./../common/lock/detectorLockAction.enum.js'); -const { DetectorId } = require('./../common/DetectorId.enum.js'); +const { DetectorId } = require('./../common/detectorId.enum.js'); const {User} = require('./../dtos/User.js'); const LOG_FACILITY = 'cog/log-ctrl'; diff --git a/Control/test/lib/controllers/mocha-lock.controller.test.js b/Control/test/lib/controllers/mocha-lock.controller.test.js index 776cea378..5fb8393ff 100644 --- a/Control/test/lib/controllers/mocha-lock.controller.test.js +++ b/Control/test/lib/controllers/mocha-lock.controller.test.js @@ -21,7 +21,7 @@ const { LockService } = require('./../../../lib/services/Lock.service.js'); const { User } = require('./../../../lib/dtos/User.js'); const { DetectorLockAction } = require('../../../lib/common/lock/detectorLockAction.enum.js'); const { DetectorLockState } = require('../../../lib/common/lock/detectorLockState.enum.js'); -const { DetectorId } = require('../../../lib/common/DetectorId.enum.js'); +const { DetectorId } = require('../../../lib/common/detectorId.enum.js'); describe(`'LockController' test suite`, () => { const res = { From e841b11989f6cf8870fed8dcb381de3aa98780ff Mon Sep 17 00:00:00 2001 From: George Raduta Date: Thu, 5 Feb 2026 15:57:10 +0100 Subject: [PATCH 6/6] Complement docs on LOCKS --- Control/docs/LOCKS.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Control/docs/LOCKS.md b/Control/docs/LOCKS.md index 62a4c18b8..958199e5f 100644 --- a/Control/docs/LOCKS.md +++ b/Control/docs/LOCKS.md @@ -30,7 +30,7 @@ Acquires a lock on a detector for the current user. - `shouldForce` (optional): Boolean flag to force taking the lock even if held by another user **Behavior**: -- When `detectorId` is `ALL`, locks all detectors **except** TST +- When `detectorId` is `ALL` and the user has a role of at least `GLOBAL`, locks all detectors **except** TST - If a lock is already held by another user and: - `shouldForce` is `false`, the request will fail. - `shouldForce` is `true` and the user has a role of at least `GLOBAL` for individual detector and at least `ADMIN` for detector `ALL`, the lock will be taken regardless of current ownership @@ -45,7 +45,7 @@ Releases a lock on a detector. - `shouldForce` (optional): Boolean flag to force releasing the lock even if held by another user **Behavior**: -- When `detectorId` is `ALL`, releases locks on all detectors **except** TST +- When `detectorId` is `ALL` and the user has a role of at least `GLOBAL`, releases locks on all detectors **except** TST - If a lock is already held by another user and: - `shouldForce` is `false`, the request will fail. - `shouldForce` is `true` and the user has a role of at least `GLOBAL` for individual detector and at least `ADMIN` for detector `ALL`, the lock will be released regardless of current ownership @@ -56,7 +56,7 @@ Releases a lock on a detector. The TST (test) detector is treated specially: - When using `ALL` as the detector ID, TST is excluded from both TAKE and RELEASE operations -- TST must be locked/unlocked explicitly by using `TST` as the detector ID +- TST must be taken/released explicitly by using `TST` as the detector ID - Moreover, front-end pages are also: - excluding TST from being displayed if on `Global` page - displaying TST at the end with separator if on `+Create` or `Locks` pages