Skip to content

Fix device group snapshot assignment (F5) #7901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Steve-Mcl wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −0
Open

Fix device group snapshot assignment (F5) #7901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions forge/ee/db/controllers/DeviceGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ module.exports = {
if (!snapshot) {
throw new ValidationError('Snapshot does not exist')
}
// 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
Comment on lines +107 to 114

@andypalmi andypalmi Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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

@Steve-Mcl Steve-Mcl Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@andypalmi andypalmi Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andypalmi andypalmi Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

}

Expand Down
31 changes: 31 additions & 0 deletions test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,37 @@ describe('Application Device Groups API', function () {
app.comms.devices.sendCommand.callCount.should.equal(0) // no devices should have been sent an update
})

// Bob (BTeam owner) should not be able to point his BTeam device group at an ATeam snapshot.
it('Rejects a targetSnapshot that belongs to another team (400)', async function () {
const { sid, application, deviceGroup, device1of2, device2of2, snapshot } = await prepare()

// Create an application, instance and snapshot owned by ATeam (foreign to bob's BTeam group)
const foreignApplication = await factory.createApplication({ name: generateName('foreign-app') }, TestObjects.ATeam)
const foreignInstance = await factory.createInstance({ name: generateName('foreign-instance') }, foreignApplication, app.stack, app.template, app.projectType, { start: false })
const foreignSnapshot = await factory.createSnapshot({ name: generateName('foreign-snapshot') }, foreignInstance, TestObjects.alice)

// now call the API to point the group at the foreign snapshot
const response = await callUpdate(sid, application, deviceGroup, {
targetSnapshotId: foreignSnapshot.hashid
})

// should fail
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)

// should not have updated the group or devices - still the original in-team snapshot
updatedDeviceGroup.should.have.property('targetSnapshotId', snapshot.id)
updatedDevice1.should.have.property('targetSnapshotId', snapshot.id)
updatedDevice2.should.have.property('targetSnapshotId', snapshot.id)

// check no devices got an update command
app.comms.devices.sendCommand.callCount.should.equal(0)
})

@andypalmi andypalmi Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded, see the updated suggestion in the reply below (adds the device assertions to test 2 per the parity nit).

@Steve-Mcl Steve-Mcl Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andypalmi andypalmi Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, folded in the device assertions for test 2, this combined suggestion now supersedes the one above:

Suggested change
})
})
// 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)
})


it('Cannot update a device group with empty name', async function () {
const sid = await login('bob', 'bbPassword')
const application = await factory.createApplication({ name: generateName('app') }, TestObjects.BTeam)
Expand Down
Loading