Skip to content

Commit e87ec7f

Browse files
gradutagraduta
authored
[OGUI-1877] Decouple TST detector from Release/Take ALL actions (#3310)
- increase restrictions on GET API endpoint for retrieving locks state to Role.Detector - decouple the TST detector from the Release/Take ALL actions - add documentation about locks business logic - extract constants for ALL and TST and re-use from separate file so that it can be reused in other places as well
1 parent 6928fad commit e87ec7f

File tree

7 files changed

+202
-33
lines changed

7 files changed

+202
-33
lines changed

‎Control/README.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
- [Requirements](#requirements)
99
- [Installation](#installation)
1010
- [Business logic for Developers to know](#business-logic-for-developers-to-know)
11+
- [Locks](#locks)
1112
- [Configuration](#configuration)
1213
- [O2Control gRPC](#o2control-grpc)
1314
- [Apricot gRPC](#apricot-grpc)
@@ -55,6 +56,7 @@ It communicates with [Control agent](https://github.com/AliceO2Group/Control) ov
5556
7. Open browser and navigate to http://localhost:8080
5657

5758
## [Business logic for Developers to know](./docs/BUSINESS_FOR_DEVELOPER_TO_KNOW.md)
59+
## [Locks](./docs/LOCKS.md)
5860
## Configuration
5961
### O2Control gRPC
6062
* `hostname` - gRPC hostname

‎Control/docs/LOCKS.md‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
# Locks
2+
3+
## Overview
4+
5+
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.
6+
7+
All LOCKs operations require an authenticated user session and moreover:
8+
- Lock Operations require a role of minimum `Role.DETECTOR`.
9+
- Lock Operations on `ALL` detectors require a minimum role of `Role.GLOBAL`.
10+
- Forced Lock Operations require a role of minimum `Role.GLOBAL`.
11+
- Forced Lock Operations on `ALL` detectors require a minimum role of `Role.ADMIN`.
12+
13+
## Lock Management
14+
15+
### Lock States
16+
17+
Each detector can be in one of the following lock states:
18+
- **Released**: The detector is available to be locked by any user
19+
- **Taken**: The detector is currently locked by a specific user
20+
21+
### Get Lock States
22+
23+
### Take Lock
24+
25+
Acquires a lock on a detector for the current user.
26+
27+
**Parameters**:
28+
- `detectorId`: The individual detector identifier or `ALL` to lock all detectors (excluding TST)
29+
- `action`: `TAKE`
30+
- `shouldForce` (optional): Boolean flag to force taking the lock even if held by another user
31+
32+
**Behavior**:
33+
- When `detectorId` is `ALL` and the user has a role of at least `GLOBAL`, locks all detectors **except** TST
34+
- If a lock is already held by another user and:
35+
- `shouldForce` is `false`, the request will fail.
36+
- `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
37+
38+
### Release Lock
39+
40+
Releases a lock on a detector.
41+
42+
**Parameters**:
43+
- `detectorId`: The detector identifier or `ALL` to release all locks (excluding TST)
44+
- `action`: `RELEASE`
45+
- `shouldForce` (optional): Boolean flag to force releasing the lock even if held by another user
46+
47+
**Behavior**:
48+
- When `detectorId` is `ALL` and the user has a role of at least `GLOBAL`, releases locks on all detectors **except** TST
49+
- If a lock is already held by another user and:
50+
- `shouldForce` is `false`, the request will fail.
51+
- `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
52+
53+
## Special Cases
54+
55+
### TST Detector
56+
57+
The TST (test) detector is treated specially:
58+
- When using `ALL` as the detector ID, TST is excluded from both TAKE and RELEASE operations
59+
- TST must be taken/released explicitly by using `TST` as the detector ID
60+
- Moreover, front-end pages are also:
61+
- excluding TST from being displayed if on `Global` page
62+
- displaying TST at the end with separator if on `+Create` or `Locks` pages
63+
64+
### Error Handling
65+
66+
The lock controller handles the following error cases:
67+
- Missing `detectorId` parameter → Returns `InvalidInputError`
68+
- Invalid `action` parameter → Returns `InvalidInputError`
69+
- Lock conflicts (when not forcing) → Error from LockService

‎Control/lib/api.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,10 @@ module.exports.setup = (http, ws) => {
247247
http.post('/execute/o2-roc-config', coreMiddleware, (req, res) => ctrlService.createAutoEnvironment(req, res));
248248

249249
// Lock Service
250-
http.get('/locks', lockController.getLocksStateHandler.bind(lockController));
250+
http.get('/locks',
251+
minimumRoleMiddleware(Role.DETECTOR),
252+
lockController.getLocksStateHandler.bind(lockController)
253+
);
251254

252255
http.put(`/locks/:action/${DetectorId.ALL}`,
253256
minimumRoleMiddleware(Role.GLOBAL),

‎Control/lib/common/detectorId.enum.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
*/
1818
const DetectorId = Object.freeze({
1919
ALL: 'ALL',
20+
TST: 'TST',
2021
});
2122

2223
exports.DetectorId = DetectorId;

‎Control/lib/controllers/Lock.controller.js‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@ const { LogManager, LogLevel } = require('@aliceo2/web-ui');
1616
const { updateAndSendExpressResponseFromNativeError, InvalidInputError } = require('@aliceo2/web-ui');
1717

1818
const { DetectorLockAction } = require('./../common/lock/detectorLockAction.enum.js');
19+
const { DetectorId } = require('./../common/detectorId.enum.js');
1920
const {User} = require('./../dtos/User.js');
2021

2122
const LOG_FACILITY = 'cog/log-ctrl';
22-
const DETECTOR_ALL = 'ALL';
2323

2424
/**
2525
* Controller for dealing with all API requests on actions and state of the locks used for detectors
@@ -70,27 +70,31 @@ class LockController {
7070
}
7171
const user = new User(username, name, personid, access);
7272
if (action.toLocaleUpperCase() === DetectorLockAction.TAKE) {
73-
if (detectorId === DETECTOR_ALL) {
74-
Object.keys(this._lockService.locksByDetector).forEach((detector) => {
75-
try {
76-
this._lockService.takeLock(detector, user, shouldForce);
77-
} catch (error) {
78-
console.error(error);
79-
}
80-
});
73+
if (detectorId === DetectorId.ALL) {
74+
Object.keys(this._lockService.locksByDetector)
75+
.filter((detector) => detector !== DetectorId.TST) // Skip TST detector when locking all
76+
.forEach((detector) => {
77+
try {
78+
this._lockService.takeLock(detector, user, shouldForce);
79+
} catch (error) {
80+
console.error(error);
81+
}
82+
});
8183
} else {
8284
this._lockService.takeLock(detectorId, user, shouldForce);
8385
}
8486
res.status(200).json(this._lockService.locksByDetectorToJSON());
8587
} else if (action.toLocaleUpperCase() === DetectorLockAction.RELEASE) {
86-
if (detectorId === DETECTOR_ALL) {
87-
Object.keys(this._lockService.locksByDetector).forEach((detector) => {
88-
try {
89-
this._lockService.releaseLock(detector, user, shouldForce);
90-
} catch (error) {
91-
console.error(error);
92-
}
93-
});
88+
if (detectorId === DetectorId.ALL) {
89+
Object.keys(this._lockService.locksByDetector)
90+
.filter((detector) => detector !== DetectorId.TST) // Skip TST detector when releasing all
91+
.forEach((detector) => {
92+
try {
93+
this._lockService.releaseLock(detector, user, shouldForce);
94+
} catch (error) {
95+
console.error(error);
96+
}
97+
});
9498
} else {
9599
this._lockService.releaseLock(detectorId, user, shouldForce);
96100
}

‎Control/test/api/lock/api-get-locks.test.js‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
*/
1515

1616
const request = require('supertest');
17-
const { ADMIN_TEST_TOKEN, TEST_URL } = require('../generateToken.js');
17+
const { ADMIN_TEST_TOKEN, GUEST_TEST_TOKEN, TEST_URL } = require('../generateToken.js');
1818

1919
describe(`'API - GET - /locks' test suite`, () => {
2020
before(async () => {
@@ -50,4 +50,14 @@ describe(`'API - GET - /locks' test suite`, () => {
5050
message: 'Invalid JWT token provided'
5151
});
5252
});
53+
54+
it('should return unauthorized error for insufficient role token requests', async () => {
55+
await request(`${TEST_URL}/api/locks`)
56+
.get(`/?token=${GUEST_TEST_TOKEN}`)
57+
.expect(403, {
58+
status: 403,
59+
message: 'Not enough permissions for this operation',
60+
title: 'Unauthorized Access',
61+
});
62+
});
5363
});

0 commit comments

Comments
 (0)