Conversation
Steve-Mcl
changed the title
andypalmi
left a comment
andypalmi
left a comment
There was a problem hiding this comment.
Following up on the open question in the description about application scope. I think it's worth closing for a reason beyond the UI: application:device-group:update is checked against the group's own application, so with granular RBAC enabled a caller authorized only on Application A could still set this group's target snapshot to one owned by Application B in the same team, where they may hold no permission at all. Since an Application always belongs to a single Team, scoping to application is a strict tightening over the current team check, so it shouldn't affect any currently-valid same-application usage (checked this against the existing happy-path/update-snapshot tests, both already use a same-application snapshot). Suggestion inline, plus a couple of tests in the same style as the cross-team one just above.
| // ensure the snapshot belongs to the same team as the device group's | ||
| // application - never trust a payload-supplied snapshot id to be in-team | ||
| const snapshotTeamId = await snapshot.getTeamId() | ||
| const application = await deviceGroup.getApplication() | ||
| if (!snapshotTeamId || !application || snapshotTeamId !== application.TeamId) { | ||
| throw new ValidationError('Snapshot does not belong to the same team') | ||
| } | ||
| snapshotId = snapshot.id |
There was a problem hiding this comment.
| // ensure the snapshot belongs to the same team as the device group's | |
| // application - never trust a payload-supplied snapshot id to be in-team | |
| const snapshotTeamId = await snapshot.getTeamId() | |
| const application = await deviceGroup.getApplication() | |
| if (!snapshotTeamId || !application || snapshotTeamId !== application.TeamId) { | |
| throw new ValidationError('Snapshot does not belong to the same team') | |
| } | |
| snapshotId = snapshot.id | |
| // ensure the snapshot belongs to the same application as the device | |
| // group - never trust a payload-supplied snapshot id to be in-scope. | |
| // Scoped to application (not just team) because granular RBAC can | |
| // authorize a caller for this application without authorizing them | |
| // for another application in the same team. | |
| const application = await deviceGroup.getApplication() | |
| let snapshotApplicationId = null | |
| if (snapshot.ownerType === 'instance') { | |
| const project = await snapshot.getProject() | |
| snapshotApplicationId = project?.ApplicationId | |
| } else if (snapshot.ownerType === 'device') { | |
| const device = await snapshot.getDevice() | |
| snapshotApplicationId = device?.ApplicationId || (device?.ProjectId ? (await device.getProject())?.ApplicationId : null) | |
| } | |
| if (!application || !snapshotApplicationId || snapshotApplicationId !== application.id) { | |
| throw new ValidationError('Snapshot does not belong to the same application') | |
| } | |
| snapshotId = snapshot.id |
There was a problem hiding this comment.
Applying this locally and running tests i see
1) Application Device Groups API
Update Device Group
Updates targetSnapshot for group and devices:
AssertionError: expected 400 to be 200
+ expected - actual
-400
+200
at Assertion.fail (node_modules\should\cjs\should.js:275:17)
at Assertion.value (node_modules\should\cjs\should.js:356:19)
at Context.<anonymous> (test\unit\forge\ee\routes\api\applicationDeviceGroups_spec.js:456:40)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
All other 41 tests passed.
The existing test Updates targetSnapshot for group and devices sets the group's target to a snapshot on TestObjects.instance, which lives in TestObjects.application - a different application in the same BTeam. Today that returns 200. Applying the app scope change flips it to 400, so the old test fails.
Reading the name of the test that failed "Updates targetSnapshot for group and devices" does suggest is was a "happy path" test and that it was true, it just did not consider application scope. It reuses the pre-existing TestObjects.instance (which lives in TestObjects.application) purely as a convenient source of a valid snapshot - it was never intended to assert that a cross-application snapshot is allowed, that was just an incidental side effect of the fixture. Its real intent is the happy path: a valid target snapshot gets set on the group and propagates to the devices.
So I don't think this is a behaviour we meant to guarantee - it's a fixture that needs correcting rather than an expectation to flip. If I changed the test to expect 400 we'd lose our only happy-path coverage for setting a target snapshot.
My suggestion -
- We adopt the application scope check as suggested (agree on the RBAC reasoning - a caller authorised for this application isn't necessarily authorised for another in the same team, so team-scope leaves that gap open).
- Fix the Updates targetSnapshot for group and devices fixture so the snapshot comes from the group's own application (create an instance under the group's application and snapshot that), keeping it as a 200 happy path test.
- Add your two new tests for the same-team/different-application rejection (covering both the instance-owned and device-owned branches).
There was a problem hiding this comment.
Matches what I see too — newSnapshot in that test comes from TestObjects.instance, which lives in TestObjects.application, not the application the group/devices belong to in prepare(). Cross-application fixture that happened to pass before this change, not an intended guarantee. Agreed on all three points — suggestion for the fixture fix below, and a reply with the updated test 2 for parity.
There was a problem hiding this comment.
(GitHub won't let me anchor a suggestion on the happy-path test itself since that line isn't part of this PR's diff, so here's the fixture fix as a plain snippet instead — same idea as the suggestion above, applied to the Updates targetSnapshot for group and devices test around line 447-448):
// create a new snapshot on an instance in the group's own application -
// TestObjects.instance lives in a different BTeam application and would
// now be rejected by the application-scope check
const ownInstance = await factory.createInstance({ name: generateName('instance') }, application, app.stack, app.template, app.projectType, { start: false })
const newSnapshot = await factory.createSnapshot({ name: generateName('snapshot') }, ownInstance, TestObjects.bob)|
|
||
| // check no devices got an update command | ||
| app.comms.devices.sendCommand.callCount.should.equal(0) | ||
| }) |
There was a problem hiding this comment.
Superseded, see the updated suggestion in the reply below (adds the device assertions to test 2 per the parity nit).
There was a problem hiding this comment.
Minor nit: Test 2 only asserts the group is unchanged, not the member devices (Test 1 checks both). Worth adding the device assertions for parity, but not essential.
There was a problem hiding this comment.
Good catch, folded in the device assertions for test 2, this combined suggestion now supersedes the one above:
| }) | |
| }) | |
| // Bob (BTeam) should not be able to point his BTeam device group at a same-team | |
| // snapshot that belongs to a different BTeam application. | |
| it('Rejects a targetSnapshot that belongs to a different application in the same team (400)', async function () { | |
| const { sid, application, deviceGroup, device1of2, device2of2, snapshot } = await prepare() | |
| // Create a second BTeam application (same team, different application) with its own instance snapshot | |
| const otherApplication = await factory.createApplication({ name: generateName('other-app') }, TestObjects.BTeam) | |
| const otherInstance = await factory.createInstance({ name: generateName('other-instance') }, otherApplication, app.stack, app.template, app.projectType, { start: false }) | |
| const otherInstanceSnapshot = await factory.createSnapshot({ name: generateName('other-instance-snapshot') }, otherInstance, TestObjects.bob) | |
| const response = await callUpdate(sid, application, deviceGroup, { | |
| targetSnapshotId: otherInstanceSnapshot.hashid | |
| }) | |
| response.statusCode.should.equal(400) | |
| response.json().should.have.property('code', 'invalid_input') | |
| const updatedDeviceGroup = await app.db.models.DeviceGroup.byId(deviceGroup.hashid) | |
| const updatedDevice1 = await app.db.models.Device.byId(device1of2.hashid) | |
| const updatedDevice2 = await app.db.models.Device.byId(device2of2.hashid) | |
| updatedDeviceGroup.should.have.property('targetSnapshotId', snapshot.id) | |
| updatedDevice1.should.have.property('targetSnapshotId', snapshot.id) | |
| updatedDevice2.should.have.property('targetSnapshotId', snapshot.id) | |
| app.comms.devices.sendCommand.callCount.should.equal(0) | |
| }) | |
| // Same scenario, but the foreign snapshot belongs to an application-owned device | |
| // rather than an instance - exercises the device-owned branch of the fix. | |
| it('Rejects a device-owned targetSnapshot that belongs to a different application in the same team (400)', async function () { | |
| const { sid, application, deviceGroup, device1of2, device2of2, snapshot } = await prepare() | |
| const otherApplication = await factory.createApplication({ name: generateName('other-app') }, TestObjects.BTeam) | |
| const otherDevice = await factory.createDevice({ name: generateName('other-device') }, TestObjects.BTeam, null, otherApplication) | |
| const otherDeviceSnapshot = await app.db.models.ProjectSnapshot.create({ | |
| name: generateName('other-device-snapshot'), | |
| DeviceId: otherDevice.id, | |
| settings: {}, | |
| flows: {} | |
| }) | |
| const response = await callUpdate(sid, application, deviceGroup, { | |
| targetSnapshotId: otherDeviceSnapshot.hashid | |
| }) | |
| response.statusCode.should.equal(400) | |
| response.json().should.have.property('code', 'invalid_input') | |
| const updatedDeviceGroup = await app.db.models.DeviceGroup.byId(deviceGroup.hashid) | |
| const updatedDevice1 = await app.db.models.Device.byId(device1of2.hashid) | |
| const updatedDevice2 = await app.db.models.Device.byId(device2of2.hashid) | |
| updatedDeviceGroup.should.have.property('targetSnapshotId', snapshot.id) | |
| updatedDevice1.should.have.property('targetSnapshotId', snapshot.id) | |
| updatedDevice2.should.have.property('targetSnapshotId', snapshot.id) | |
| app.comms.devices.sendCommand.callCount.should.equal(0) | |
| }) | |
|
Recap of what's needed to land this:
|
2 participants
Description
Fix device group snapshot assignment F5 - see linked issue for details
Related Issue(s)
see linked issue
Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
area:migrationlabel