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..958199e5f --- /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` 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 + +### 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` 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 + +## 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 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 + +### 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 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/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..3cdc41ae6 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,27 +70,31 @@ 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) => { - try { - this._lockService.takeLock(detector, user, shouldForce); - } catch (error) { - console.error(error); - } - }); + 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) { + console.error(error); + } + }); } else { this._lockService.takeLock(detectorId, user, shouldForce); } res.status(200).json(this._lockService.locksByDetectorToJSON()); } else if (action.toLocaleUpperCase() === DetectorLockAction.RELEASE) { - if (detectorId === DETECTOR_ALL) { - Object.keys(this._lockService.locksByDetector).forEach((detector) => { - try { - this._lockService.releaseLock(detector, user, shouldForce); - } catch (error) { - console.error(error); - } - }); + 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) { + console.error(error); + } + }); } else { this._lockService.releaseLock(detectorId, user, shouldForce); } 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', + }); + }); }); diff --git a/Control/test/lib/controllers/mocha-lock.controller.test.js b/Control/test/lib/controllers/mocha-lock.controller.test.js index 0b5377c03..5fb8393ff 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', + }, + }, })); }); });