From 35e1dfc7a6ad48a69718c45b8043556a939762b3 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:07:49 +0200 Subject: [PATCH 01/14] Add test validating AlarmFacade rule scoping, currently failing --- .../test/rules/AlarmFacadeTest.groovy | 311 ++++++++++++++++++ 1 file changed, 311 insertions(+) create mode 100644 test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy new file mode 100644 index 0000000000..cb2b695217 --- /dev/null +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -0,0 +1,311 @@ +/* + * Copyright 2026, OpenRemote Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package org.openremote.test.rules + +import org.openremote.manager.alarm.AlarmService +import org.openremote.manager.asset.AssetStorageService +import org.openremote.manager.notification.NotificationService +import org.openremote.manager.rules.RulesEngineId +import org.openremote.manager.rules.facade.AlarmFacade +import org.openremote.manager.security.ManagerIdentityService +import org.openremote.manager.setup.SetupService +import org.openremote.model.alarm.Alarm +import org.openremote.model.alarm.SentAlarm +import org.openremote.model.asset.UserAssetLink +import org.openremote.model.asset.impl.ThingAsset +import org.openremote.model.notification.Notification +import org.openremote.model.security.User +import org.openremote.model.util.UniqueIdentifierGenerator +import org.openremote.setup.integration.KeycloakTestSetup +import org.openremote.test.ManagerContainerTrait +import spock.lang.Shared +import spock.lang.Specification + +import static org.openremote.model.Constants.RESTRICTED_USER_REALM_ROLE + +/** Exercises creation against real asset paths, user roles, user links and alarm persistence. */ +class AlarmFacadeTest extends Specification implements ManagerContainerTrait { + + private static final String SOURCE_ID = "4242" + + @Shared + AlarmService alarmService + + @Shared + AssetStorageService assetStorageService + + @Shared + NotificationService originalNotificationService + + @Shared + Map testAssets = [:] + + @Shared + Map testUsers = [:] + + @Shared + String realm + + @Shared + String otherRealm + + String alarmTitle + List createdUserLinks = [] + List queuedNotifications = [] + + def setupSpec() { + def container = startContainer(defaultConfig(), defaultServices()) + alarmService = container.getService(AlarmService.class) + assetStorageService = container.getService(AssetStorageService.class) + originalNotificationService = container.getService(NotificationService.class) + def identityProvider = container.getService(ManagerIdentityService.class).identityProvider + def keycloakSetup = container.getService(SetupService.class).getTaskOfType(KeycloakTestSetup.class) + realm = keycloakSetup.realmBuilding.name + otherRealm = keycloakSetup.realmCity.name + + testAssets.ancestor = createAsset("Ancestor", realm) + testAssets.scoped = createAsset("Scoped", realm, testAssets.ancestor.id) + testAssets.child = createAsset("Child", realm, testAssets.scoped.id) + testAssets.grandchild = createAsset("Grandchild", realm, testAssets.child.id) + testAssets.sibling = createAsset("Sibling", realm, testAssets.ancestor.id) + testAssets.unrelated = createAsset("Unrelated", realm) + testAssets.otherRealm = createAsset("Other realm", otherRealm) + + testUsers.unrestricted = identityProvider.getUser(keycloakSetup.testuser2Id) + testUsers.restricted = identityProvider.getUser(keycloakSetup.testuser3Id) + testUsers.otherRealm = identityProvider.getUser(keycloakSetup.testuser1Id) + assert testUsers.unrestricted.realm == realm + assert testUsers.restricted.realm == realm + assert testUsers.otherRealm.realm != realm + assert !identityProvider.getUserRealmRoles(realm, testUsers.unrestricted.id).contains(RESTRICTED_USER_REALM_ROLE) + assert identityProvider.getUserRealmRoles(realm, testUsers.restricted.id).contains(RESTRICTED_USER_REALM_ROLE) + } + + def setup() { + alarmTitle = "Alarm facade ${UniqueIdentifierGenerator.generateId()}" + // Capture the synchronous enqueue boundary, so absence checks do not race the notification + // worker. Nothing from this specification is delivered to email or push providers. + alarmService.notificationService = Stub(NotificationService) { + sendNotificationAsync(_ as Notification, _ as Notification.Source, _ as String) >> { + Notification notification, Notification.Source source, String sourceId -> + queuedNotifications.add(notification) + } + } + } + + def cleanup() { + alarmService.notificationService = originalNotificationService + // Look up by this iteration's unique title, including alarms persisted by a failing rejection + // test. Do not depend on the facade returning the ID before an assertion or exception fails. + createdAlarms().each { alarmService.removeAlarm(it) } + if (!createdUserLinks.isEmpty()) { + assetStorageService.deleteUserAssetLinks(createdUserLinks) + } + } + + def cleanupSpec() { + if (assetStorageService != null && !testAssets.isEmpty()) { + assert assetStorageService.delete(testAssets.values().collect { it.id }) + } + } + + def "#scope scope creating an alarm for #targetName is allowed: #allowed"() { + given: + def facade = facadeFor(scope) + def targetId = testAssets[targetName].id + def alarm = newAlarm() + + when: + def alarmId = facade.create(alarm, [targetId]) + + then: + if (allowed) { + assertCreated(alarmId, scope, null, [targetId]) + } else { + verifyAll { + alarmId == null + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + } + } + + where: + scope | targetName | allowed + "realm" | "ancestor" | true + "realm" | "sibling" | true + "realm" | "unrelated" | true + "asset" | "scoped" | true + "asset" | "child" | true + "asset" | "grandchild" | true + "asset" | "ancestor" | false + "asset" | "sibling" | false + "asset" | "unrelated" | false + } + + def "#scope scope rejects a foreign-realm asset before creating the alarm"() { + when: + facadeFor(scope).create(newAlarm(), [testAssets.otherRealm.id]) + + then: + thrown(AlarmService.AlarmAssetLinkRealmMismatchException) + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + + where: + scope << ["realm", "asset"] + } + + def "asset scope rejects the complete mixed target list #targetNames before notifying"() { + given: + def alarm = newAlarm(testUsers.unrestricted.id, Alarm.Severity.HIGH) + def targetIds = targetNames.collect { testAssets[it].id } + + when: + def alarmId = facadeFor("asset").create(alarm, targetIds) + + then: + verifyAll { + alarmId == null + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + } + + where: + targetNames << [["child", "sibling"], ["sibling", "child"]] + } + + def "#scope scope accepts duplicate allowed asset IDs and keeps one link per asset"() { + given: + def targetId = testAssets.child.id + + when: + def alarmId = facadeFor(scope).create(newAlarm(), [targetId, targetId]) + + then: + assertCreated(alarmId, scope, null, [targetId]) + queuedNotifications.isEmpty() + + where: + scope << ["realm", "asset"] + } + + def "#scope scope overrides the supplied alarm realm with the engine realm"() { + given: + def alarm = newAlarm().setRealm(otherRealm) + + when: + def alarmId = facadeFor(scope).create(alarm, [testAssets.child.id]) + + then: + assertCreated(alarmId, scope, null, [testAssets.child.id]) + + where: + scope << ["realm", "asset"] + } + + def "#scope scope assigning #assignee to #targetNames with links #linkedNames is allowed: #allowed"() { + given: + def user = testUsers[assignee] + def targetIds = targetNames.collect { testAssets[it].id } + linkedNames.each { name -> + createdUserLinks.add(new UserAssetLink(realm, user.id, testAssets[name].id)) + } + assetStorageService.storeUserAssetLinks(createdUserLinks) + // These are newly created assets: verify precisely which targets the assignee can access. + targetNames.each { name -> + assert assetStorageService.isUserAsset(user.id, testAssets[name].id) == linkedNames.contains(name) + } + def alarm = newAlarm(user.id, Alarm.Severity.HIGH) + + when: + def alarmId = facadeFor(scope).create(alarm, targetIds) + + then: + if (allowed) { + assertCreated(alarmId, scope, user.id, targetIds) + // Positive control: the capture is connected to the real alarm service's notification path. + assert queuedNotifications.size() == 2 + assert queuedNotifications.any { notification -> + notification.targets?.any { it.type == Notification.TargetType.USER && it.id == user.id } + } + } else { + verifyAll { + alarmId == null + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + } + } + + where: + [scope, assignee, targetNames, linkedNames, allowed] << ["realm", "asset"].collectMany { scope -> + assigneeCases().collect { row -> [scope] + row } + } + } + + private static List assigneeCases() { + [ + // Unrestricted users need no links, even when they have links elsewhere. + ["unrestricted", ["child"], [], true], + ["unrestricted", ["child"], ["unrelated"], true], + ["unrestricted", ["child", "grandchild"], [], true], + // Restricted users require explicit links to every target, not just a scope/ancestor link. + ["restricted", ["scoped"], ["scoped"], true], + ["restricted", ["child"], ["child"], true], + ["restricted", ["child", "grandchild"], ["child", "grandchild"], true], + ["restricted", ["child"], [], false], + ["restricted", ["child"], ["scoped"], false], + ["restricted", ["child"], ["sibling"], false], + ["restricted", ["child", "grandchild"], ["child"], false], + ["otherRealm", ["child"], [], false] + ] + } + + private ThingAsset createAsset(String name, String assetRealm, String parentId = null) { + assetStorageService.merge(new ThingAsset("Alarm facade ${name}").setRealm(assetRealm).setParentId(parentId)) + } + + private AlarmFacade facadeFor(String scope) { + def engineId = scope == "asset" ? new RulesEngineId(realm, testAssets.scoped.id) : new RulesEngineId(realm) + new AlarmFacade(engineId, alarmService) + } + + private Alarm newAlarm(String assigneeId = null, Alarm.Severity severity = Alarm.Severity.MEDIUM) { + new Alarm(alarmTitle, "Alarm facade scope regression", severity, assigneeId, realm) + .setSourceId(SOURCE_ID) + } + + private List createdAlarms() { + alarmService.getAlarms(realm, null, null, null).findAll { it.title == alarmTitle } + } + + private void assertCreated(Long alarmId, String scope, String assigneeId, List targetIds) { + assert alarmId != null + def alarms = createdAlarms() + assert alarms.size() == 1 + def alarm = alarms.first() + assert alarm.id == alarmId + assert alarm.realm == realm + assert alarm.source == (scope == "asset" ? Alarm.Source.ASSET_RULESET : Alarm.Source.REALM_RULESET) + assert alarm.sourceId == SOURCE_ID + assert alarm.assigneeId == assigneeId + def links = alarmService.getAssetLinks(alarmId, realm) + assert links.size() == targetIds.toSet().size() + assert links.collect { it.id.assetId }.toSet() == targetIds.toSet() + } +} From c8f2b65d88e22448bdf1b1c1fd47f87bf929da44 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:29:19 +0200 Subject: [PATCH 02/14] Enforce scope and assignee access when rules create alarms --- .../manager/alarm/AlarmService.java | 5 +- .../openremote/manager/rules/RulesEngine.java | 2 +- .../manager/rules/facade/AlarmFacade.java | 70 ++++++++++++++++++- .../org/openremote/model/rules/Alarms.java | 9 +++ .../test/rules/AlarmFacadeTest.groovy | 30 ++++++-- .../test/rules/AlarmRuleTest.groovy | 43 +++++++++++- 6 files changed, 145 insertions(+), 14 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java index 6cb90b129f..be7229eabb 100644 --- a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java +++ b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java @@ -169,9 +169,10 @@ protected void validateAssetIds(Collection assetIds) { /** * Throws an {@link EntityNotFoundException} if any asset does not exist, or an {@link - * AlarmAssetLinkRealmMismatchException} if any asset does not belong to {@code realm}. + * AlarmAssetLinkRealmMismatchException} if any asset does not belong to {@code realm}. Callers + * are responsible for enforcing realm authorization. */ - protected void validateAssetsInRealm(String realm, Collection assetIds) { + public void validateAssetsInRealm(String realm, Collection assetIds) { validateAssetIds(assetIds); Set uniqueAssetIds = new HashSet<>(assetIds); Map assetRealms = assetStorageService.getAssetRealms(uniqueAssetIds); diff --git a/manager/src/main/java/org/openremote/manager/rules/RulesEngine.java b/manager/src/main/java/org/openremote/manager/rules/RulesEngine.java index 990cc1c43f..2aedb32d91 100644 --- a/manager/src/main/java/org/openremote/manager/rules/RulesEngine.java +++ b/manager/src/main/java/org/openremote/manager/rules/RulesEngine.java @@ -179,7 +179,7 @@ public RulesEngine( new UsersFacade<>(id, assetStorageService, notificationService, identityService); this.notificationFacade = new NotificationsFacade<>(id, notificationService); this.webhooksFacade = new WebhooksFacade<>(id, webhookService); - this.alarmsFacade = new AlarmFacade<>(id, alarmService); + this.alarmsFacade = new AlarmFacade<>(id, alarmService, assetStorageService, identityService); this.historicFacade = new HistoricFacade<>(id, assetStorageService, assetDatapointService); this.predictedFacade = new PredictedFacade<>(id, assetStorageService, assetPredictedDatapointService); diff --git a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java index 17aa22d2b3..eaaa0d678b 100644 --- a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java +++ b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java @@ -18,26 +18,90 @@ */ package org.openremote.manager.rules.facade; +import static org.openremote.model.Constants.RESTRICTED_USER_REALM_ROLE; + +import java.util.Arrays; import java.util.List; +import java.util.logging.Logger; import org.openremote.manager.alarm.AlarmService; +import org.openremote.manager.asset.AssetStorageService; import org.openremote.manager.rules.RulesEngineId; +import org.openremote.manager.security.ManagerIdentityService; import org.openremote.model.alarm.Alarm; import org.openremote.model.alarm.Alarm.Source; import org.openremote.model.rules.Alarms; +import org.openremote.model.rules.AssetRuleset; import org.openremote.model.rules.GlobalRuleset; import org.openremote.model.rules.RealmRuleset; import org.openremote.model.rules.Ruleset; +import org.openremote.model.security.User; public class AlarmFacade extends Alarms { + private static final Logger LOG = Logger.getLogger(AlarmFacade.class.getName()); + protected final RulesEngineId rulesEngineId; protected final AlarmService alarmService; + protected final AssetStorageService assetStorageService; + protected final ManagerIdentityService identityService; - public AlarmFacade(RulesEngineId rulesEngineId, AlarmService alarmService) { + public AlarmFacade( + RulesEngineId rulesEngineId, + AlarmService alarmService, + AssetStorageService assetStorageService, + ManagerIdentityService identityService) { this.rulesEngineId = rulesEngineId; this.alarmService = alarmService; + this.assetStorageService = assetStorageService; + this.identityService = identityService; } + @Override public Long create(Alarm alarm, List assetIds) { + String realm = rulesEngineId.getRealm().orElseThrow(); + List targetIds = assetIds == null ? List.of() : assetIds.stream().distinct().toList(); + + if (!targetIds.isEmpty()) { + // Preserve the existing validation errors for missing and foreign-realm assets. + alarmService.validateAssetsInRealm(realm, targetIds); + if (rulesEngineId.getScope() == AssetRuleset.class) { + for (String assetId : targetIds) { + if (!FacadeHelper.doesRuleEngineScopeAllowAccess( + rulesEngineId, assetId, assetStorageService)) { + LOG.warning( + "Cannot create alarm for asset outside engine scope: " + + assetId + + ", " + + rulesEngineId); + return null; + } + } + } + } + + String assigneeId = alarm.getAssigneeId(); + if (assigneeId != null) { + User assignee = identityService.getIdentityProvider().getUser(assigneeId); + if (assignee == null || !realm.equals(assignee.getRealm())) { + LOG.warning( + "Cannot create alarm for missing or foreign-realm assignee: " + + assigneeId + + ", " + + rulesEngineId); + return null; + } + boolean restricted = + Arrays.asList(identityService.getIdentityProvider().getUserRealmRoles(realm, assigneeId)) + .contains(RESTRICTED_USER_REALM_ROLE); + if (restricted && !assetStorageService.isUserAssets(assigneeId, targetIds)) { + LOG.warning( + "Cannot create alarm for restricted assignee without links to all target assets: " + + assigneeId + + ", " + + rulesEngineId); + return null; + } + } + Alarm.Source source; if (rulesEngineId.getScope() == GlobalRuleset.class) { @@ -48,10 +112,10 @@ public Long create(Alarm alarm, List assetIds) { source = Source.ASSET_RULESET; } - alarm.setRealm(rulesEngineId.getRealm().orElseThrow()); + alarm.setRealm(realm); alarm.setSource(source); - return alarmService.sendAlarm(alarm, assetIds).getId(); + return alarmService.sendAlarm(alarm, targetIds).getId(); } public void linkAssets(List assetIds, Long alarmId) { diff --git a/model/src/main/java/org/openremote/model/rules/Alarms.java b/model/src/main/java/org/openremote/model/rules/Alarms.java index e0cab6d7eb..2feb0632f9 100644 --- a/model/src/main/java/org/openremote/model/rules/Alarms.java +++ b/model/src/main/java/org/openremote/model/rules/Alarms.java @@ -25,6 +25,15 @@ /** Facade for sending {@link Alarm}s from rules RHS. */ @TsIgnore public abstract class Alarms { + /** + * Creates an alarm in the engine's realm for assets within its scope. An assignee is optional; + * when provided, the user must belong to that realm. Restricted assignees must have explicit + * links to every target asset. The caller must set the alarm's source ID to identify the ruleset. + * + * @return the created alarm ID, or {@code null} if an asset is outside the engine's asset scope + * or the assignee cannot access the targets + * @throws IllegalArgumentException if an asset ID is invalid or belongs to a different realm + */ public abstract Long create(Alarm alarm, List assetIds); public abstract void linkAssets(List assetIds, Long alarmId); diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index cb2b695217..a8f39940ba 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -50,6 +50,9 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { @Shared AssetStorageService assetStorageService + @Shared + ManagerIdentityService identityService + @Shared NotificationService originalNotificationService @@ -74,7 +77,8 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { alarmService = container.getService(AlarmService.class) assetStorageService = container.getService(AssetStorageService.class) originalNotificationService = container.getService(NotificationService.class) - def identityProvider = container.getService(ManagerIdentityService.class).identityProvider + identityService = container.getService(ManagerIdentityService.class) + def identityProvider = identityService.identityProvider def keycloakSetup = container.getService(SetupService.class).getTaskOfType(KeycloakTestSetup.class) realm = keycloakSetup.realmBuilding.name otherRealm = keycloakSetup.realmCity.name @@ -102,9 +106,8 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { // Capture the synchronous enqueue boundary, so absence checks do not race the notification // worker. Nothing from this specification is delivered to email or push providers. alarmService.notificationService = Stub(NotificationService) { - sendNotificationAsync(_ as Notification, _ as Notification.Source, _ as String) >> { - Notification notification, Notification.Source source, String sourceId -> - queuedNotifications.add(notification) + sendNotificationAsync(_ as Notification, _ as Notification.Source, _ as String) >> { Notification notification, Notification.Source source, String sourceId -> + queuedNotifications.add(notification) } } } @@ -258,6 +261,22 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { } } + def "#scope scope rejects a missing assignee before creating or notifying"() { + when: + def alarmId = facadeFor(scope).create( + newAlarm(UniqueIdentifierGenerator.generateId(), Alarm.Severity.HIGH), [testAssets.child.id]) + + then: + verifyAll { + alarmId == null + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + } + + where: + scope << ["realm", "asset"] + } + private static List assigneeCases() { [ // Unrestricted users need no links, even when they have links elsewhere. @@ -267,6 +286,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { // Restricted users require explicit links to every target, not just a scope/ancestor link. ["restricted", ["scoped"], ["scoped"], true], ["restricted", ["child"], ["child"], true], + ["restricted", ["child", "child"], ["child"], true], ["restricted", ["child", "grandchild"], ["child", "grandchild"], true], ["restricted", ["child"], [], false], ["restricted", ["child"], ["scoped"], false], @@ -282,7 +302,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { private AlarmFacade facadeFor(String scope) { def engineId = scope == "asset" ? new RulesEngineId(realm, testAssets.scoped.id) : new RulesEngineId(realm) - new AlarmFacade(engineId, alarmService) + new AlarmFacade(engineId, alarmService, assetStorageService, identityService) } private Alarm newAlarm(String assigneeId = null, Alarm.Severity severity = Alarm.Severity.MEDIUM) { diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmRuleTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmRuleTest.groovy index c853b923f0..087a32b88e 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmRuleTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmRuleTest.groovy @@ -35,6 +35,8 @@ import org.openremote.model.notification.NotificationSendResult import org.openremote.model.query.RulesetQuery import org.openremote.model.rules.json.JsonRulesetDefinition import org.openremote.model.rules.json.RuleActionAlarm +import org.openremote.model.rules.json.RuleAction +import org.openremote.model.rules.json.RuleActionWriteAttribute import org.openremote.model.security.User import org.openremote.model.util.ValueUtil import org.openremote.setup.integration.KeycloakTestSetup @@ -128,13 +130,18 @@ class AlarmRuleTest extends Specification implements ManagerContainerTrait { } @Shared - createAlarmRule = { severity, assigneeId -> + createAlarmRule = { severity, assigneeId, resetAfterAlarm = false -> when: "a ruleset with a notification action is created in the building realm" def rulesStr = getClass().getResource("/org/openremote/test/rules/CO2AlarmRule.json").text JsonRulesetDefinition rulesetDef = ValueUtil.JSON.readValue(rulesStr, JsonRulesetDefinition.class) rulesetDef.rules[0].when.groups.get(0).items.get(0).assets.ids(managerTestSetup.apartment2LivingroomId) ((RuleActionAlarm) rulesetDef.rules[0].then[0]).alarm.setSeverity(severity) ((RuleActionAlarm) rulesetDef.rules[0].then[0]).assigneeId = assigneeId + if (resetAfterAlarm) { + // A second action lets rejection tests wait for the alarm action to finish. + rulesetDef.rules[0].then = (rulesetDef.rules[0].then.toList() + + new RuleActionWriteAttribute(attributeName: "co2Level", value: 4000)) as RuleAction[] + } rulesStr = ValueUtil.JSON.writeValueAsString(rulesetDef) def realmRuleset = new RealmRuleset(managerTestSetup.realmBuildingName, "CO2 Alarm Rule", Ruleset.Lang.JSON, rulesStr) realmRuleset = rulesetStorageService.merge(realmRuleset) @@ -221,14 +228,44 @@ Value: 6000 where: severity | assigneeId | emailNotifications Alarm.Severity.LOW | null | 0 - Alarm.Severity.LOW | keycloakTestSetup.testuser1Id | 0 + Alarm.Severity.LOW | keycloakTestSetup.testuser2Id | 0 Alarm.Severity.MEDIUM | null | 0 Alarm.Severity.MEDIUM | keycloakTestSetup.testuser2Id | 0 Alarm.Severity.HIGH | null | 1 - Alarm.Severity.HIGH | keycloakTestSetup.testuser3Id | 1 + Alarm.Severity.HIGH | keycloakTestSetup.buildingUserId | 1 Alarm.Severity.HIGH | alarmsReadWriteUserId | 1 } + def "rule rejects alarm with inaccessible assignee '#assigneeId' and severity '#severity'"() { + given: "the target starts below the alarm threshold" + def conditions = new PollingConditions(timeout: 10, delay: 0.2) + assetProcessingService.sendAttributeEvent(new AttributeEvent(managerTestSetup.apartment2LivingroomId, "co2Level", 3000)) + conditions.eventually { + def asset = assetStorageService.find(managerTestSetup.apartment2LivingroomId, true) + assert asset.getAttribute("co2Level").flatMap { it.getValue() }.orElse(null) == 3000 + } + createAlarmRule(severity, assigneeId, true) + + when: "the alarm condition is triggered" + assetProcessingService.sendAttributeEvent(new AttributeEvent(managerTestSetup.apartment2LivingroomId, "co2Level", 6000)) + + then: "the action after the alarm runs, confirming the rejection did not stop the rule" + conditions.eventually { + def asset = assetStorageService.find(managerTestSetup.apartment2LivingroomId, true) + assert asset.getAttribute("co2Level").flatMap { it.getValue() }.orElse(null) == 4000 + } + + and: "no alarm or email notification was created" + alarmService.getAlarms(managerTestSetup.realmBuildingName, null, null, null).isEmpty() + notificationMessages.isEmpty() + + where: + severity | assigneeId + Alarm.Severity.LOW | keycloakTestSetup.testuser1Id + Alarm.Severity.HIGH | keycloakTestSetup.testuser1Id + Alarm.Severity.HIGH | keycloakTestSetup.testuser3Id + } + def "rule does not create alarm when rules service is restarted"() { def realmRuleset = createAlarmRule(Alarm.Severity.HIGH, null) From ebe1878d7df52778480fb7b8ffeec569c9cd59ec Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:40:17 +0200 Subject: [PATCH 03/14] Add regression tests for alarm facade asset linking scope, not passing yet --- .../test/rules/AlarmFacadeTest.groovy | 279 +++++++++++++++++- 1 file changed, 275 insertions(+), 4 deletions(-) diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index a8f39940ba..b017c61ee7 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -18,6 +18,9 @@ */ package org.openremote.test.rules +import jakarta.persistence.EntityNotFoundException +import java.time.Instant +import java.util.concurrent.TimeUnit import org.openremote.manager.alarm.AlarmService import org.openremote.manager.asset.AssetStorageService import org.openremote.manager.notification.NotificationService @@ -39,7 +42,7 @@ import spock.lang.Specification import static org.openremote.model.Constants.RESTRICTED_USER_REALM_ROLE -/** Exercises creation against real asset paths, user roles, user links and alarm persistence. */ +/** Exercises creation and linking against real asset paths, user roles, user links and alarm persistence. */ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { private static final String SOURCE_ID = "4242" @@ -116,7 +119,9 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { alarmService.notificationService = originalNotificationService // Look up by this iteration's unique title, including alarms persisted by a failing rejection // test. Do not depend on the facade returning the ID before an assertion or exception fails. - createdAlarms().each { alarmService.removeAlarm(it) } + [realm, otherRealm].each { alarmRealm -> + createdAlarms(alarmRealm).each { alarmService.removeAlarm(it) } + } if (!createdUserLinks.isEmpty()) { assetStorageService.deleteUserAssetLinks(createdUserLinks) } @@ -277,6 +282,226 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { scope << ["realm", "asset"] } + def "#scope scope linking targets #targetNames is allowed: #allowed"() { + given: "an alarm already linked to the scope asset" + def alarm = createExistingAlarm(["scoped"]) + def previousLinks = linkTimestamps(alarm) + def targetIds = assetIdsFor(targetNames) + + when: + facadeFor(scope).linkAssets(targetIds, alarm.id) + + then: "the complete request is accepted or all existing links are preserved" + noExceptionThrown() + assertLinkState(alarm, allowed ? assetIdsFor(["scoped"]) + targetIds : assetIdsFor(["scoped"]), previousLinks) + + where: + scope | targetNames | allowed + "realm" | ["ancestor"] | true + "realm" | ["sibling"] | true + "realm" | ["unrelated"] | true + "asset" | ["scoped"] | true + "asset" | ["child"] | true + "asset" | ["grandchild"] | true + "asset" | ["ancestor"] | false + "asset" | ["sibling"] | false + "asset" | ["unrelated"] | false + "asset" | ["child", "sibling"] | false + "asset" | ["sibling", "child"] | false + } + + def "#scope scope linking to an alarm with existing targets #existingNames is allowed: #allowed"() { + given: "an alarm whose current links determine its scope" + def alarm = createExistingAlarm(existingNames) + def previousLinks = linkTimestamps(alarm) + def targetIds = assetIdsFor(["grandchild"]) + + when: "the requested target is inside the engine's subtree" + facadeFor(scope).linkAssets(targetIds, alarm.id) + + then: "every existing link must also be in scope, and asset scope cannot claim an unlinked alarm" + noExceptionThrown() + assertLinkState(alarm, allowed ? assetIdsFor(existingNames) + targetIds : assetIdsFor(existingNames), previousLinks) + + where: + scope | existingNames | allowed + "realm" | ["ancestor"] | true + "realm" | ["sibling"] | true + "realm" | ["unrelated"] | true + "realm" | ["child", "sibling"] | true + "realm" | [] | true + "asset" | ["scoped"] | true + "asset" | ["child"] | true + "asset" | ["scoped", "child"] | true + "asset" | ["ancestor"] | false + "asset" | ["sibling"] | false + "asset" | ["unrelated"] | false + "asset" | ["child", "sibling"] | false + "asset" | ["sibling", "child"] | false + "asset" | [] | false + } + + def "#scope scope linking #targetNames with #assignee assignee linked to #linkedNames is allowed: #allowed"() { + given: + def user = testUsers[assignee] + linkedNames.each { name -> + createdUserLinks.add(new UserAssetLink(realm, user.id, testAssets[name].id)) + } + assetStorageService.storeUserAssetLinks(createdUserLinks) + (["scoped"] + targetNames).unique().each { name -> + assert assetStorageService.isUserAsset(user.id, testAssets[name].id) == linkedNames.contains(name) + } + def alarm = createExistingAlarm(["scoped"], user.id) + def previousLinks = linkTimestamps(alarm) + def targetIds = assetIdsFor(targetNames) + + when: + facadeFor(scope).linkAssets(targetIds, alarm.id) + + then: "restricted assignees need access to the union of current and requested targets" + noExceptionThrown() + assertLinkState(alarm, allowed ? assetIdsFor(["scoped"]) + targetIds : assetIdsFor(["scoped"]), previousLinks) + + where: + [scope, assignee, targetNames, linkedNames, allowed] << ["realm", "asset"].collectMany { scope -> + linkAssigneeCases().collect { row -> [scope] + row } + } + } + + def "#scope scope rejects linking foreign-realm assets in #targetNames without changing existing links"() { + given: + def alarm = createExistingAlarm(["scoped"]) + def previousLinks = linkTimestamps(alarm) + + when: + facadeFor(scope).linkAssets(assetIdsFor(targetNames), alarm.id) + + then: + thrown(AlarmService.AlarmAssetLinkRealmMismatchException) + assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) + + where: + [scope, targetNames] << ["realm", "asset"].collectMany { scope -> + [["otherRealm"], ["child", "otherRealm"], ["otherRealm", "child"]].collect { targets -> + [scope, targets] + } + } + } + + def "#scope scope rejects linking to a foreign-realm alarm without changing its links"() { + given: + def alarm = createExistingAlarm(["otherRealm"], null, otherRealm) + def previousLinks = linkTimestamps(alarm) + + when: + facadeFor(scope).linkAssets(assetIdsFor(["child"]), alarm.id) + + then: + thrown(AlarmService.AlarmAssetLinkRealmMismatchException) + assertLinkState(alarm, assetIdsFor(["otherRealm"]), previousLinks) + alarmService.getAssetLinks(alarm.id, realm).isEmpty() + + where: + scope << ["realm", "asset"] + } + + def "#scope scope linking duplicate or existing targets #targetNames is idempotent"() { + given: + def alarm = createExistingAlarm(["scoped"]) + def previousLinks = linkTimestamps(alarm) + def targetIds = assetIdsFor(targetNames) + + when: + facadeFor(scope).linkAssets(targetIds, alarm.id) + + then: "one link per asset is retained and the original link timestamp is preserved" + assertLinkState(alarm, assetIdsFor(["scoped", "child"]), previousLinks) + + when: "the same request is repeated at a later time" + def firstLinks = linkTimestamps(alarm) + advancePseudoClock(1, TimeUnit.SECONDS) + facadeFor(scope).linkAssets(targetIds, alarm.id) + + then: + assertLinkState(alarm, assetIdsFor(["scoped", "child"]), firstLinks) + + where: + [scope, targetNames] << ["realm", "asset"].collectMany { scope -> + [["child", "child"], ["scoped", "child", "child"]].collect { targets -> [scope, targets] } + } + } + + def "#scope scope linking empty input #targetIds remains a no-op"() { + given: + def alarm = createExistingAlarm(["scoped"]) + def previousLinks = linkTimestamps(alarm) + def facade = facadeFor(scope) + + when: + facade.linkAssets(targetIds, alarm.id) + facade.linkAssets(targetIds, null) + + then: "empty requests do not change links or require a valid alarm ID" + noExceptionThrown() + assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) + + where: + scope | targetIds + "realm" | null + "realm" | [] + "asset" | null + "asset" | [] + } + + def "#scope scope rejects invalid target #invalidTarget without partially linking"() { + given: + def alarm = createExistingAlarm(["scoped"]) + def previousLinks = linkTimestamps(alarm) + def invalidId = invalidTarget == "missing" ? UniqueIdentifierGenerator.generateId() : invalidTarget + + when: + facadeFor(scope).linkAssets([testAssets.child.id, invalidId], alarm.id) + + then: + thrown(exceptionType) + assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) + + where: + [scope, invalidTarget, exceptionType] << ["realm", "asset"].collectMany { scope -> + [ + [null, IllegalArgumentException], + ["", IllegalArgumentException], + ["missing", EntityNotFoundException] + ].collect { row -> + [scope] + row + } + } + } + + def "#scope scope retains validation of alarm ID #invalidAlarmId when linking"() { + given: + def alarm = createExistingAlarm(["scoped"]) + def previousLinks = linkTimestamps(alarm) + + when: + facadeFor(scope).linkAssets(assetIdsFor(["child"]), invalidAlarmId) + + then: + thrown(exceptionType) + assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) + + where: + [scope, invalidAlarmId, exceptionType] << ["realm", "asset"].collectMany { scope -> + [ + [null, IllegalArgumentException], + [-1L, IllegalArgumentException], + [Long.MAX_VALUE, EntityNotFoundException] + ].collect { row -> + [scope] + row + } + } + } + private static List assigneeCases() { [ // Unrestricted users need no links, even when they have links elsewhere. @@ -296,6 +521,52 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { ] } + private static List linkAssigneeCases() { + [ + ["unrestricted", ["child", "grandchild"], [], true], + ["unrestricted", ["child"], ["unrelated"], true], + ["restricted", ["child", "grandchild"], ["scoped", "child", "grandchild"], true], + // Access to current targets does not grant access to descendants being added. + ["restricted", ["child"], ["scoped"], false], + // Access to new targets is insufficient if access to an existing target was revoked. + ["restricted", ["child"], ["child"], false], + ["restricted", ["child", "grandchild"], ["scoped", "child"], false], + ["restricted", ["grandchild", "child"], ["scoped", "child"], false], + ["restricted", ["child"], ["sibling"], false], + ["restricted", ["scoped", "child", "child"], ["scoped", "child"], true], + ["otherRealm", ["child"], [], false] + ] + } + + private List assetIdsFor(List names) { + names.collect { testAssets[it].id } + } + + private SentAlarm createExistingAlarm(List assetNames, String assigneeId = null, String alarmRealm = realm) { + // Existing alarms may originate from another entry point or predate the facade checks. + def alarm = alarmService.sendAlarm( + newAlarm(assigneeId).setRealm(alarmRealm).setSource(Alarm.Source.REALM_RULESET), assetIdsFor(assetNames)) + // Ensure replacing an existing link would produce an observably different timestamp. + advancePseudoClock(1, TimeUnit.SECONDS) + alarm + } + + private Map linkTimestamps(SentAlarm alarm) { + alarmService.getAssetLinks(alarm.id, alarm.realm).collectEntries { link -> + [(link.id.assetId): link.createdOn] + } + } + + private void assertLinkState(SentAlarm alarm, List expectedIds, Map previousLinks) { + def links = alarmService.getAssetLinks(alarm.id, alarm.realm) + assert links.size() == expectedIds.toSet().size() + assert links.collect { it.id.assetId }.toSet() == expectedIds.toSet() + previousLinks.each { assetId, createdOn -> + assert links.find { it.id.assetId == assetId }.createdOn == createdOn + } + assert queuedNotifications.isEmpty() + } + private ThingAsset createAsset(String name, String assetRealm, String parentId = null) { assetStorageService.merge(new ThingAsset("Alarm facade ${name}").setRealm(assetRealm).setParentId(parentId)) } @@ -310,8 +581,8 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { .setSourceId(SOURCE_ID) } - private List createdAlarms() { - alarmService.getAlarms(realm, null, null, null).findAll { it.title == alarmTitle } + private List createdAlarms(String alarmRealm = realm) { + alarmService.getAlarms(alarmRealm, null, null, null).findAll { it.title == alarmTitle } } private void assertCreated(Long alarmId, String scope, String assigneeId, List targetIds) { From 8e6a8b7fc2561d2f676ebdda62f372cb63c73c2e Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:51:05 +0200 Subject: [PATCH 04/14] Enforce scope and assignee access when linking alarm assets --- .../manager/rules/facade/AlarmFacade.java | 121 ++++++++++++------ .../org/openremote/model/rules/Alarms.java | 12 ++ 2 files changed, 91 insertions(+), 42 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java index eaaa0d678b..775cc2c5eb 100644 --- a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java +++ b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java @@ -23,12 +23,14 @@ import java.util.Arrays; import java.util.List; import java.util.logging.Logger; +import java.util.stream.Stream; import org.openremote.manager.alarm.AlarmService; import org.openremote.manager.asset.AssetStorageService; import org.openremote.manager.rules.RulesEngineId; import org.openremote.manager.security.ManagerIdentityService; import org.openremote.model.alarm.Alarm; import org.openremote.model.alarm.Alarm.Source; +import org.openremote.model.alarm.SentAlarm; import org.openremote.model.rules.Alarms; import org.openremote.model.rules.AssetRuleset; import org.openremote.model.rules.GlobalRuleset; @@ -63,43 +65,10 @@ public Long create(Alarm alarm, List assetIds) { if (!targetIds.isEmpty()) { // Preserve the existing validation errors for missing and foreign-realm assets. alarmService.validateAssetsInRealm(realm, targetIds); - if (rulesEngineId.getScope() == AssetRuleset.class) { - for (String assetId : targetIds) { - if (!FacadeHelper.doesRuleEngineScopeAllowAccess( - rulesEngineId, assetId, assetStorageService)) { - LOG.warning( - "Cannot create alarm for asset outside engine scope: " - + assetId - + ", " - + rulesEngineId); - return null; - } - } - } } - - String assigneeId = alarm.getAssigneeId(); - if (assigneeId != null) { - User assignee = identityService.getIdentityProvider().getUser(assigneeId); - if (assignee == null || !realm.equals(assignee.getRealm())) { - LOG.warning( - "Cannot create alarm for missing or foreign-realm assignee: " - + assigneeId - + ", " - + rulesEngineId); - return null; - } - boolean restricted = - Arrays.asList(identityService.getIdentityProvider().getUserRealmRoles(realm, assigneeId)) - .contains(RESTRICTED_USER_REALM_ROLE); - if (restricted && !assetStorageService.isUserAssets(assigneeId, targetIds)) { - LOG.warning( - "Cannot create alarm for restricted assignee without links to all target assets: " - + assigneeId - + ", " - + rulesEngineId); - return null; - } + if (!areAssetsInScope(targetIds) + || !isAssigneeAllowed(realm, alarm.getAssigneeId(), targetIds)) { + return null; } Alarm.Source source; @@ -118,13 +87,81 @@ public Long create(Alarm alarm, List assetIds) { return alarmService.sendAlarm(alarm, targetIds).getId(); } + @Override public void linkAssets(List assetIds, Long alarmId) { - if (assetIds != null && !assetIds.isEmpty()) { - String realmId = - rulesEngineId - .getRealm() - .orElseThrow(() -> new IllegalStateException("Realm ruleset must have a realm ID")); - alarmService.linkAssets(assetIds, realmId, alarmId); + if (assetIds == null || assetIds.isEmpty()) { + return; + } + + String realm = + rulesEngineId + .getRealm() + .orElseThrow(() -> new IllegalStateException("Realm ruleset must have a realm ID")); + SentAlarm alarm = alarmService.getAlarm(alarmId); + if (!realm.equals(alarm.getRealm())) { + throw new AlarmService.AlarmAssetLinkRealmMismatchException( + "Alarm '" + alarmId + "' is not in realm '" + realm + "'"); + } + + List targetIds = assetIds.stream().distinct().toList(); + alarmService.validateAssetsInRealm(realm, targetIds); + List existingAssetIds = + alarmService.getAssetLinks(alarmId, realm).stream() + .map(link -> link.getId().getAssetId()) + .toList(); + if (rulesEngineId.getScope() == AssetRuleset.class && existingAssetIds.isEmpty()) { + LOG.warning( + "Cannot establish asset scope for unlinked alarm: " + alarmId + ", " + rulesEngineId); + return; + } + + List allAssetIds = + Stream.concat(existingAssetIds.stream(), targetIds.stream()).distinct().toList(); + if (!areAssetsInScope(allAssetIds) + || !isAssigneeAllowed(realm, alarm.getAssigneeId(), allAssetIds)) { + return; + } + + alarmService.linkAssets(targetIds, realm, alarmId); + } + + private boolean areAssetsInScope(List assetIds) { + if (rulesEngineId.getScope() == AssetRuleset.class) { + for (String assetId : assetIds) { + if (!FacadeHelper.doesRuleEngineScopeAllowAccess( + rulesEngineId, assetId, assetStorageService)) { + LOG.warning("Alarm asset is outside engine scope: " + assetId + ", " + rulesEngineId); + return false; + } + } + } + return true; + } + + private boolean isAssigneeAllowed(String realm, String assigneeId, List assetIds) { + if (assigneeId == null) { + return true; + } + User assignee = identityService.getIdentityProvider().getUser(assigneeId); + if (assignee == null || !realm.equals(assignee.getRealm())) { + LOG.warning( + "Alarm assignee is missing or belongs to another realm: " + + assigneeId + + ", " + + rulesEngineId); + return false; + } + boolean restricted = + Arrays.asList(identityService.getIdentityProvider().getUserRealmRoles(realm, assigneeId)) + .contains(RESTRICTED_USER_REALM_ROLE); + if (restricted && !assetStorageService.isUserAssets(assigneeId, assetIds)) { + LOG.warning( + "Restricted alarm assignee lacks links to all target assets: " + + assigneeId + + ", " + + rulesEngineId); + return false; } + return true; } } diff --git a/model/src/main/java/org/openremote/model/rules/Alarms.java b/model/src/main/java/org/openremote/model/rules/Alarms.java index 2feb0632f9..015a40b28c 100644 --- a/model/src/main/java/org/openremote/model/rules/Alarms.java +++ b/model/src/main/java/org/openremote/model/rules/Alarms.java @@ -36,5 +36,17 @@ public abstract class Alarms { */ public abstract Long create(Alarm alarm, List assetIds); + /** + * Adds asset links to an alarm in the engine's realm. Asset-scoped engines require at least one + * existing link, and all existing and requested assets must be within the engine's subtree. + * Assignees must belong to the alarm's realm; restricted assignees must have explicit links to + * every existing and requested asset. + * + *

No links are changed if scope or assignee access is denied. Null or empty asset lists are + * ignored. Duplicate and existing links are accepted without changing their timestamps. + * + * @throws IllegalArgumentException if the alarm ID or an asset ID is invalid, or the alarm or a + * requested asset belongs to a different realm + */ public abstract void linkAssets(List assetIds, Long alarmId); } From d21e2594ee932848c57b0ba8165414d2e02f9491 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:26:16 +0200 Subject: [PATCH 05/14] Fix recipient scoping for unassigned high-severity alarms --- .../manager/alarm/AlarmService.java | 18 +- .../test/alarm/AlarmResourceTest.groovy | 4 +- .../test/rules/AlarmFacadeTest.groovy | 155 +++++++++++++++++- 3 files changed, 172 insertions(+), 5 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java index be7229eabb..ad1428e0f4 100644 --- a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java +++ b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java @@ -236,11 +236,12 @@ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { /** * Sends e-mail and push notifications for an alarm to the alarm assignee. If an assignee is not - * set all users having the {@link Constants#READ_ADMIN_ROLE} or {@link - * Constants#WRITE_ALARMS_ROLE} are notified. + * set, users in the alarm's realm having the {@link Constants#SUPER_USER_REALM_ROLE} or {@link + * Constants#WRITE_ALARMS_ROLE} are notified. Restricted users must have explicit links to every + * linked asset; they are not notified for alarms without linked assets. * * @param alarm the alarm to send e-mail and push notifications for - * @param excludeUserIds users matching these user IDs will are excluded from the notifications + * @param excludeUserIds users matching these user IDs are excluded from the notifications */ protected void sendAssigneeNotification(SentAlarm alarm, Set excludeUserIds) { List users = getAlarmNotificationUsers(alarm); @@ -299,6 +300,17 @@ private List getAlarmNotificationUsers(SentAlarm alarm) { .realmRoles(new StringPredicate(Constants.SUPER_USER_REALM_ROLE)) .serviceUsers(false); users.addAll(Arrays.asList(identityProvider.queryUsers(userQuery))); + List assetIds = + getAssetLinks(alarm.getId(), alarm.getRealm()).stream() + .map(link -> link.getId().getAssetId()) + .distinct() + .toList(); + // Both notification channels include all linked assets, so partial access is insufficient. + users.removeIf( + user -> + Arrays.asList(identityProvider.getUserRealmRoles(alarm.getRealm(), user.getId())) + .contains(Constants.RESTRICTED_USER_REALM_ROLE) + && !assetStorageService.isUserAssets(user.getId(), assetIds)); } else { users.add(identityProvider.getUser(alarm.getAssigneeId())); } diff --git a/test/src/test/groovy/org/openremote/test/alarm/AlarmResourceTest.groovy b/test/src/test/groovy/org/openremote/test/alarm/AlarmResourceTest.groovy index bcabf16463..a95d566f87 100644 --- a/test/src/test/groovy/org/openremote/test/alarm/AlarmResourceTest.groovy +++ b/test/src/test/groovy/org/openremote/test/alarm/AlarmResourceTest.groovy @@ -190,7 +190,9 @@ class AlarmResourceTest extends Specification implements ManagerContainerTrait { "Medium Unassigned Alarm" | "Another Description" | Severity.MEDIUM | null | 0 "Medium Assigned 1 Alarm" | "Assigned Description" | Severity.MEDIUM | keycloakTestSetup.testuser1Id | 0 "Medium Assigned 2 Alarm" | "Assigned Description" | Severity.MEDIUM | alarmsReadWriteUserId | 0 - "High Unassigned Alarm" | "Critical Description" | Severity.HIGH | null | 1 + // This alarm has no linked assets. The building realm's unrestricted writer has no email; + // its restricted writers must not receive automatic notifications for an unlinked alarm. + "High Unassigned Alarm" | "Critical Description" | Severity.HIGH | null | 0 "High Assigned 1 Alarm" | "Critical Description" | Severity.HIGH | keycloakTestSetup.testuser1Id | 0 "High Assigned 2 Alarm" | "Critical Description" | Severity.HIGH | alarmsReadWriteUserId | 1 } diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index b017c61ee7..2621d83884 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -32,7 +32,10 @@ import org.openremote.model.alarm.Alarm import org.openremote.model.alarm.SentAlarm import org.openremote.model.asset.UserAssetLink import org.openremote.model.asset.impl.ThingAsset +import org.openremote.model.notification.EmailNotificationMessage import org.openremote.model.notification.Notification +import org.openremote.model.notification.PushNotificationMessage +import org.openremote.model.security.ClientRole import org.openremote.model.security.User import org.openremote.model.util.UniqueIdentifierGenerator import org.openremote.setup.integration.KeycloakTestSetup @@ -42,7 +45,7 @@ import spock.lang.Specification import static org.openremote.model.Constants.RESTRICTED_USER_REALM_ROLE -/** Exercises creation and linking against real asset paths, user roles, user links and alarm persistence. */ +/** Exercises creation, linking and notification recipients against real assets, roles and persistence. */ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { private static final String SOURCE_ID = "4242" @@ -97,6 +100,10 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { testUsers.unrestricted = identityProvider.getUser(keycloakSetup.testuser2Id) testUsers.restricted = identityProvider.getUser(keycloakSetup.testuser3Id) testUsers.otherRealm = identityProvider.getUser(keycloakSetup.testuser1Id) + testUsers.otherAlarmWriter = identityProvider.getUser(keycloakSetup.testuser4Id) + testUsers.notificationWriter = keycloakSetup.createUser( + realm, "alarm-facade-writer", UniqueIdentifierGenerator.generateId(), "Alarm", "Writer", + "alarm-facade-writer@openremote.local", true, [ClientRole.WRITE_ALARMS] as ClientRole[]) assert testUsers.unrestricted.realm == realm assert testUsers.restricted.realm == realm assert testUsers.otherRealm.realm != realm @@ -128,6 +135,9 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { } def cleanupSpec() { + if (testUsers.notificationWriter != null) { + identityService.identityProvider.deleteUser(realm, testUsers.notificationWriter.id) + } if (assetStorageService != null && !testAssets.isEmpty()) { assert assetStorageService.delete(testAssets.values().collect { it.id }) } @@ -282,6 +292,102 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { scope << ["realm", "asset"] } + def "unassigned HIGH alarm on #operation notifies a restricted user linked to #linkedNames for targets #targetNames: #allowed"() { + given: + linkNotificationRecipient(linkedNames) + def expectedRecipients = unrestrictedNotificationRecipients() + if (allowed) { + expectedRecipients.add(testUsers.restricted) + } + + when: + def alarmId = createOrUpdateUnassignedHighAlarm(operation, targetNames) + + then: + alarmService.getAlarm(alarmId).assigneeId == null + assertNotificationRecipients(expectedRecipients) + + where: + [operation, targetNames, linkedNames, allowed] << ["realm create", "asset create", "update"].collectMany { operation -> + [ + [["child"], [], false], + [["child"], ["scoped"], false], + [["child"], ["sibling"], false], + [["child"], ["child"], true], + [["child", "grandchild"], ["child"], false], + [["grandchild", "child"], ["child"], false], + [["child", "grandchild"], ["child", "grandchild"], true], + [["child", "child"], ["child"], true] + ].collect { row -> [operation] + row } + } + } + + def "unassigned HIGH alarm with no asset links on #operation notifies only unrestricted users"() { + given: "a restricted recipient has an unrelated asset link" + linkNotificationRecipient(["child"]) + + when: + def alarmId = createOrUpdateUnassignedHighAlarm(operation, targetNames) + + then: + alarmService.getAssetLinks(alarmId, realm).isEmpty() + assertNotificationRecipients(unrestrictedNotificationRecipients()) + + where: + operation | targetNames + "realm create" | null + "realm create" | [] + "update" | [] + } + + def "updates recheck restricted recipient access after an asset link is revoked"() { + given: + linkNotificationRecipient(["child", "grandchild"]) + def alarmId = facadeFor("asset").create(newAlarm(null, Alarm.Severity.HIGH), assetIdsFor(["child", "grandchild"])) + assertNotificationRecipients(unrestrictedNotificationRecipients() + testUsers.restricted) + queuedNotifications.clear() + def revokedLink = createdUserLinks.find { it.id.assetId == testAssets.grandchild.id } + assetStorageService.deleteUserAssetLinks([revokedLink]) + createdUserLinks.remove(revokedLink) + + when: + def oldAlarm = alarmService.getAlarm(alarmId) + def updatedAlarm = alarmService.getAlarm(alarmId) + updatedAlarm.content = "Updated after access was revoked" + alarmService.updateAlarm(oldAlarm, updatedAlarm) + + then: + assertNotificationRecipients(unrestrictedNotificationRecipients()) + } + + def "automatic recipient filtering preserves the exclusion of a manual alarm's creator"() { + given: + linkNotificationRecipient(["child"]) + def alarm = newAlarm(null, Alarm.Severity.HIGH) + .setSource(Alarm.Source.MANUAL).setSourceId(testUsers.notificationWriter.id) + + when: + alarmService.sendAlarm(alarm, assetIdsFor(["child"])) + + then: + assertNotificationRecipients([testUsers.otherAlarmWriter, testUsers.restricted]) + } + + def "automatic recipient filtering preserves the exclusion of the previous assignee"() { + given: + linkNotificationRecipient(["child"]) + def oldAlarm = createExistingAlarm(["child"], testUsers.restricted.id) + def updatedAlarm = alarmService.getAlarm(oldAlarm.id) + updatedAlarm.assigneeId = null + updatedAlarm.severity = Alarm.Severity.HIGH + + when: + alarmService.updateAlarm(oldAlarm, updatedAlarm) + + then: + assertNotificationRecipients(unrestrictedNotificationRecipients()) + } + def "#scope scope linking targets #targetNames is allowed: #allowed"() { given: "an alarm already linked to the scope asset" def alarm = createExistingAlarm(["scoped"]) @@ -502,6 +608,53 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { } } + private void linkNotificationRecipient(List assetNames) { + createdUserLinks.addAll(assetNames.collect { name -> + new UserAssetLink(realm, testUsers.restricted.id, testAssets[name].id) + }) + if (!createdUserLinks.isEmpty()) { + assetStorageService.storeUserAssetLinks(createdUserLinks) + } + } + + private List unrestrictedNotificationRecipients() { + // The other fixture users either lack the alarm role, belong to another realm, or are + // restricted and have no links to these new assets. Neither writer needs any asset links. + [testUsers.notificationWriter, testUsers.otherAlarmWriter] + } + + private Long createOrUpdateUnassignedHighAlarm(String operation, List assetNames) { + if (operation == "update") { + def oldAlarm = createExistingAlarm(assetNames) + assert queuedNotifications.isEmpty() + def updatedAlarm = alarmService.getAlarm(oldAlarm.id) + updatedAlarm.severity = Alarm.Severity.HIGH + alarmService.updateAlarm(oldAlarm, updatedAlarm) + return oldAlarm.id + } + facadeFor(operation == "asset create" ? "asset" : "realm") + .create(newAlarm(null, Alarm.Severity.HIGH), assetNames == null ? null : assetIdsFor(assetNames)) + } + + private void assertNotificationRecipients(List expectedUsers) { + assert queuedNotifications.size() == 2 + def push = queuedNotifications.find { it.message instanceof PushNotificationMessage } + def email = queuedNotifications.find { it.message instanceof EmailNotificationMessage } + assert push != null + assert email != null + verifyAll { + push.targets.every { it.type == Notification.TargetType.USER } + push.targets.collect { it.id }.toSet() == expectedUsers.collect { it.id }.toSet() + email.message.to.collect { + it.address + }.toSet() == expectedUsers.findAll { + it.email + }.collect { + it.email + }.toSet() + } + } + private static List assigneeCases() { [ // Unrestricted users need no links, even when they have links elsewhere. From 8a4a0bbd3522913172b68432dea0cc83c1f188e7 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:35:06 +0200 Subject: [PATCH 06/14] Add failing tests for asset-scoped alarms without target assets --- .../test/rules/AlarmFacadeTest.groovy | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index 2621d83884..fb01e3f4ed 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -176,6 +176,46 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { "asset" | "unrelated" | false } + def "#scope scope creating an alarm without targets #targetIds for #assignee is allowed: #allowed"() { + given: + def user = assignee == "none" ? null : testUsers[assignee] + if (assignee == "restricted") { + // Access to the engine's asset does not make it an implicit alarm target. + linkNotificationRecipient(["scoped"]) + } + def alarm = newAlarm(user?.id, Alarm.Severity.HIGH) + + when: + def alarmId = facadeFor(scope).create(alarm, targetIds) + + then: + if (allowed) { + assertCreated(alarmId, scope, user?.id, []) + assertNotificationRecipients(user == null ? unrestrictedNotificationRecipients() : [user]) + } else { + verifyAll { + alarmId == null + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + } + } + + where: + scope | targetIds | assignee | allowed + "asset" | null | "none" | false + "asset" | [] | "none" | false + "asset" | null | "unrestricted" | false + "asset" | [] | "unrestricted" | false + "asset" | null | "restricted" | false + "asset" | [] | "restricted" | false + "realm" | null | "none" | true + "realm" | [] | "none" | true + "realm" | null | "unrestricted" | true + "realm" | [] | "unrestricted" | true + "realm" | null | "restricted" | false + "realm" | [] | "restricted" | false + } + def "#scope scope rejects a foreign-realm asset before creating the alarm"() { when: facadeFor(scope).create(newAlarm(), [testAssets.otherRealm.id]) From 083b1a0ee3f97ac88fb7589b6d457b71a9917e42 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:46:07 +0200 Subject: [PATCH 07/14] Require target assets when creating asset-scoped alarms --- .../manager/rules/facade/AlarmFacade.java | 5 +++++ .../java/org/openremote/model/rules/Alarms.java | 14 +++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java index 775cc2c5eb..4f41bcc12a 100644 --- a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java +++ b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java @@ -62,6 +62,11 @@ public Long create(Alarm alarm, List assetIds) { String realm = rulesEngineId.getRealm().orElseThrow(); List targetIds = assetIds == null ? List.of() : assetIds.stream().distinct().toList(); + if (rulesEngineId.getScope() == AssetRuleset.class && targetIds.isEmpty()) { + LOG.warning("Cannot create an asset-scoped alarm without target assets: " + rulesEngineId); + return null; + } + if (!targetIds.isEmpty()) { // Preserve the existing validation errors for missing and foreign-realm assets. alarmService.validateAssetsInRealm(realm, targetIds); diff --git a/model/src/main/java/org/openremote/model/rules/Alarms.java b/model/src/main/java/org/openremote/model/rules/Alarms.java index 015a40b28c..be368c2bd8 100644 --- a/model/src/main/java/org/openremote/model/rules/Alarms.java +++ b/model/src/main/java/org/openremote/model/rules/Alarms.java @@ -26,12 +26,16 @@ @TsIgnore public abstract class Alarms { /** - * Creates an alarm in the engine's realm for assets within its scope. An assignee is optional; - * when provided, the user must belong to that realm. Restricted assignees must have explicit - * links to every target asset. The caller must set the alarm's source ID to identify the ruleset. + * Creates an alarm in the engine's realm for assets within its scope. Asset-scoped engines + * require at least one target, and every target must be the scoped asset itself or a descendant. + * Realm-scoped engines may omit targets. * - * @return the created alarm ID, or {@code null} if an asset is outside the engine's asset scope - * or the assignee cannot access the targets + *

An assignee is optional; when provided, the user must belong to that realm. Restricted + * assignees must have explicit links to every target asset. The caller must set the alarm's + * source ID to identify the ruleset. + * + * @return the created alarm ID, or {@code null} if an asset-scoped engine has no targets, an + * asset is outside the engine's asset scope, or the assignee cannot access the targets * @throws IllegalArgumentException if an asset ID is invalid or belongs to a different realm */ public abstract Long create(Alarm alarm, List assetIds); From 1f27b0a4670a2df712ed4e0729e9bedb93ca8e71 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:59:36 +0200 Subject: [PATCH 08/14] Add failing tests for global-rule alarm creation --- .../test/rules/AlarmFacadeTest.groovy | 213 +++++++++++++++++- 1 file changed, 207 insertions(+), 6 deletions(-) diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index fb01e3f4ed..ad0bfffeef 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -332,6 +332,206 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { scope << ["realm", "asset"] } + def "global scope creates in #expectedRealm with declared realm #declaredRealm and targets #targetNames"() { + given: + def targetIds = targetNames == null ? null : assetIdsFor(targetNames) + def alarm = newAlarm().setRealm(declaredRealm).setSource(Alarm.Source.ASSET_RULESET) + + when: + def alarmId = facadeFor("global").create(alarm, targetIds) + + then: + assertCreated(alarmId, "global", null, targetIds ?: [], expectedRealm) + queuedNotifications.isEmpty() + + where: + declaredRealm | targetNames | expectedRealm + realm | ["child"] | realm + otherRealm | ["otherRealm"] | otherRealm + realm | null | realm + realm | [] | realm + otherRealm | [] | otherRealm + null | ["child", "sibling"] | realm + null | ["otherRealm"] | otherRealm + null | ["child", "child"] | realm + "" | ["child"] | realm + } + + def "global scope rejects conflicting realms for declared realm #declaredRealm and targets #targetNames"() { + given: + def alarm = newAlarm(null, Alarm.Severity.HIGH).setRealm(declaredRealm) + + when: + facadeFor("global").create(alarm, assetIdsFor(targetNames)) + + then: + thrown(AlarmService.AlarmAssetLinkRealmMismatchException) + [realm, otherRealm].every { createdAlarms(it).isEmpty() } + queuedNotifications.isEmpty() + + where: + declaredRealm | targetNames + realm | ["otherRealm"] + otherRealm | ["child"] + realm | ["child", "otherRealm"] + realm | ["otherRealm", "child"] + null | ["child", "otherRealm"] + null | ["otherRealm", "child"] + } + + def "global scope rejects missing realm #declaredRealm and missing targets #targetIds"() { + given: "an assignee's realm cannot substitute for an explicit realm or target assets" + def alarm = newAlarm(testUsers.unrestricted.id, Alarm.Severity.HIGH).setRealm(declaredRealm) + + when: + def alarmId = facadeFor("global").create(alarm, targetIds) + + then: + verifyAll { + alarmId == null + [realm, otherRealm].every { createdAlarms(it).isEmpty() } + queuedNotifications.isEmpty() + } + + where: + declaredRealm | targetIds + null | null + null | [] + "" | null + "" | [] + } + + def "global scope with #realmMode realm assigning #assignee to #targetNames with links #linkedNames is allowed: #allowed"() { + given: + def user = testUsers[assignee] + if (assignee == "restricted") { + linkNotificationRecipient(linkedNames) + } + def targetIds = assetIdsFor(targetNames) + def alarm = newAlarm(user.id, Alarm.Severity.HIGH).setRealm(realmMode == "explicit" ? realm : null) + + when: + def alarmId = facadeFor("global").create(alarm, targetIds) + + then: + if (allowed) { + assertCreated(alarmId, "global", user.id, targetIds) + assertNotificationRecipients([user]) + } else { + verifyAll { + alarmId == null + [realm, otherRealm].every { createdAlarms(it).isEmpty() } + queuedNotifications.isEmpty() + } + } + + where: + [realmMode, assignee, targetNames, linkedNames, allowed] << ["explicit", "inferred"].collectMany { realmMode -> + [ + ["unrestricted", ["child", "sibling"], [], true], + ["restricted", ["child"], ["child"], true], + ["restricted", ["child", "child"], ["child"], true], + ["restricted", ["child", "grandchild"], ["child", "grandchild"], true], + ["restricted", ["child"], [], false], + ["restricted", ["child"], ["scoped"], false], + ["restricted", ["child", "grandchild"], ["child"], false], + ["otherRealm", ["child"], [], false] + ].collect { row -> [realmMode] + row } + } + } + + def "global scope without targets #targetIds assigning #assignee is allowed: #allowed"() { + given: + def user = testUsers[assignee] + if (assignee == "restricted") { + linkNotificationRecipient(["scoped"]) + } + + when: + def alarmId = facadeFor("global").create(newAlarm(user.id, Alarm.Severity.HIGH), targetIds) + + then: + if (allowed) { + assertCreated(alarmId, "global", user.id, []) + assertNotificationRecipients([user]) + } else { + verifyAll { + alarmId == null + [realm, otherRealm].every { createdAlarms(it).isEmpty() } + queuedNotifications.isEmpty() + } + } + + where: + targetIds | assignee | allowed + null | "unrestricted" | true + [] | "unrestricted" | true + null | "restricted" | false + [] | "restricted" | false + } + + def "global scope with #realmMode realm rejects a missing assignee"() { + given: + def alarm = newAlarm(UniqueIdentifierGenerator.generateId(), Alarm.Severity.HIGH) + .setRealm(realmMode == "explicit" ? realm : null) + + when: + def alarmId = facadeFor("global").create(alarm, assetIdsFor(["child"])) + + then: + verifyAll { + alarmId == null + [realm, otherRealm].every { createdAlarms(it).isEmpty() } + queuedNotifications.isEmpty() + } + + where: + realmMode << ["explicit", "inferred"] + } + + def "global scope with #realmMode realm rejects invalid target #invalidTarget before creating or notifying"() { + given: + def alarm = newAlarm(null, Alarm.Severity.HIGH).setRealm(realmMode == "explicit" ? realm : null) + def invalidId = invalidTarget == "missing" ? UniqueIdentifierGenerator.generateId() : invalidTarget + + when: + facadeFor("global").create(alarm, [testAssets.child.id, invalidId]) + + then: + thrown(exceptionType) + [realm, otherRealm].every { createdAlarms(it).isEmpty() } + queuedNotifications.isEmpty() + + where: + [realmMode, invalidTarget, exceptionType] << ["explicit", "inferred"].collectMany { realmMode -> + [ + [null, IllegalArgumentException], + ["", IllegalArgumentException], + ["missing", EntityNotFoundException] + ].collect { row -> [realmMode] + row } + } + } + + def "global scope infers the realm again when an alarm definition is reused"() { + given: "JSON actions can reuse the same alarm definition across executions" + def facade = facadeFor("global") + def alarm = newAlarm().setRealm(null) + + when: + def firstId = facade.create(alarm, assetIdsFor(["child"])) + + then: + assertCreated(firstId, "global", null, assetIdsFor(["child"]), realm) + + when: "a later execution targets another realm" + def secondId = facade.create(alarm, assetIdsFor(["otherRealm"])) + + then: + assertCreated(secondId, "global", null, assetIdsFor(["otherRealm"]), otherRealm) + secondId != firstId + queuedNotifications.isEmpty() + } + def "unassigned HIGH alarm on #operation notifies a restricted user linked to #linkedNames for targets #targetNames: #allowed"() { given: linkNotificationRecipient(linkedNames) @@ -765,7 +965,8 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { } private AlarmFacade facadeFor(String scope) { - def engineId = scope == "asset" ? new RulesEngineId(realm, testAssets.scoped.id) : new RulesEngineId(realm) + def engineId = scope == "global" ? new RulesEngineId() : + (scope == "asset" ? new RulesEngineId(realm, testAssets.scoped.id) : new RulesEngineId(realm)) new AlarmFacade(engineId, alarmService, assetStorageService, identityService) } @@ -778,17 +979,17 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { alarmService.getAlarms(alarmRealm, null, null, null).findAll { it.title == alarmTitle } } - private void assertCreated(Long alarmId, String scope, String assigneeId, List targetIds) { + private void assertCreated(Long alarmId, String scope, String assigneeId, List targetIds, String alarmRealm = realm) { assert alarmId != null - def alarms = createdAlarms() + def alarms = createdAlarms(alarmRealm) assert alarms.size() == 1 def alarm = alarms.first() assert alarm.id == alarmId - assert alarm.realm == realm - assert alarm.source == (scope == "asset" ? Alarm.Source.ASSET_RULESET : Alarm.Source.REALM_RULESET) + assert alarm.realm == alarmRealm + assert alarm.source == [asset: Alarm.Source.ASSET_RULESET, realm: Alarm.Source.REALM_RULESET, global: Alarm.Source.GLOBAL_RULESET][scope] assert alarm.sourceId == SOURCE_ID assert alarm.assigneeId == assigneeId - def links = alarmService.getAssetLinks(alarmId, realm) + def links = alarmService.getAssetLinks(alarmId, alarmRealm) assert links.size() == targetIds.toSet().size() assert links.collect { it.id.assetId }.toSet() == targetIds.toSet() } From 4664f3bc82aafd41e26753177896e06a960d113f Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:17:28 +0200 Subject: [PATCH 09/14] Fix realm resolution and validation for global-rule alarm creation --- .../manager/rules/facade/AlarmFacade.java | 36 ++++++++++++++++++- .../org/openremote/model/rules/Alarms.java | 21 ++++++----- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java index 4f41bcc12a..8306e1348e 100644 --- a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java +++ b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java @@ -20,6 +20,7 @@ import static org.openremote.model.Constants.RESTRICTED_USER_REALM_ROLE; +import jakarta.persistence.EntityNotFoundException; import java.util.Arrays; import java.util.List; import java.util.logging.Logger; @@ -37,6 +38,8 @@ import org.openremote.model.rules.RealmRuleset; import org.openremote.model.rules.Ruleset; import org.openremote.model.security.User; +import org.openremote.model.util.TextUtil; +import org.openremote.model.util.ValueUtil; public class AlarmFacade extends Alarms { private static final Logger LOG = Logger.getLogger(AlarmFacade.class.getName()); @@ -59,7 +62,6 @@ public AlarmFacade( @Override public Long create(Alarm alarm, List assetIds) { - String realm = rulesEngineId.getRealm().orElseThrow(); List targetIds = assetIds == null ? List.of() : assetIds.stream().distinct().toList(); if (rulesEngineId.getScope() == AssetRuleset.class && targetIds.isEmpty()) { @@ -67,6 +69,13 @@ public Long create(Alarm alarm, List assetIds) { return null; } + String realm = resolveCreationRealm(alarm, targetIds); + if (realm == null) { + LOG.warning( + "Cannot create a global alarm without a realm or target assets: " + rulesEngineId); + return null; + } + if (!targetIds.isEmpty()) { // Preserve the existing validation errors for missing and foreign-realm assets. alarmService.validateAssetsInRealm(realm, targetIds); @@ -79,6 +88,8 @@ public Long create(Alarm alarm, List assetIds) { Alarm.Source source; if (rulesEngineId.getScope() == GlobalRuleset.class) { + // JSON actions can reuse this definition with targets in another realm on the next execution. + alarm = ValueUtil.clone(alarm); source = Source.GLOBAL_RULESET; } else if (rulesEngineId.getScope() == RealmRuleset.class) { source = Source.REALM_RULESET; @@ -130,6 +141,29 @@ public void linkAssets(List assetIds, Long alarmId) { alarmService.linkAssets(targetIds, realm, alarmId); } + private String resolveCreationRealm(Alarm alarm, List targetIds) { + if (rulesEngineId.getScope() != GlobalRuleset.class) { + return rulesEngineId.getRealm().orElseThrow(); + } + if (!TextUtil.isNullOrEmpty(alarm.getRealm())) { + return alarm.getRealm(); + } + if (targetIds.isEmpty()) { + return null; + } + + String firstAssetId = targetIds.getFirst(); + if (TextUtil.isNullOrEmpty(firstAssetId)) { + throw new IllegalArgumentException("Missing asset ID"); + } + // Creation validates the complete target list against this realm before persisting anything. + String realm = assetStorageService.getAssetRealms(List.of(firstAssetId)).get(firstAssetId); + if (realm == null) { + throw new EntityNotFoundException("Asset '" + firstAssetId + "' does not exist"); + } + return realm; + } + private boolean areAssetsInScope(List assetIds) { if (rulesEngineId.getScope() == AssetRuleset.class) { for (String assetId : assetIds) { diff --git a/model/src/main/java/org/openremote/model/rules/Alarms.java b/model/src/main/java/org/openremote/model/rules/Alarms.java index be368c2bd8..d0665f7fea 100644 --- a/model/src/main/java/org/openremote/model/rules/Alarms.java +++ b/model/src/main/java/org/openremote/model/rules/Alarms.java @@ -26,16 +26,21 @@ @TsIgnore public abstract class Alarms { /** - * Creates an alarm in the engine's realm for assets within its scope. Asset-scoped engines - * require at least one target, and every target must be the scoped asset itself or a descendant. - * Realm-scoped engines may omit targets. + * Creates an alarm for assets within the engine's scope. Realm- and asset-scoped engines use + * their own realm. Asset-scoped engines require at least one target, and every target must be the + * scoped asset itself or a descendant. Realm-scoped engines may omit targets. * - *

An assignee is optional; when provided, the user must belong to that realm. Restricted - * assignees must have explicit links to every target asset. The caller must set the alarm's - * source ID to identify the ruleset. + *

Global engines use the supplied alarm realm, or infer it from the target assets when it is + * null or blank. All targets must belong to the resolved realm. The supplied alarm definition is + * preserved for global engines so it can be reused with targets in another realm. * - * @return the created alarm ID, or {@code null} if an asset-scoped engine has no targets, an - * asset is outside the engine's asset scope, or the assignee cannot access the targets + *

An assignee is optional; when provided, the user must belong to the alarm's realm. + * Restricted assignees require at least one target and explicit links to every target asset. The + * caller must set the alarm's source ID to identify the ruleset. + * + * @return the created alarm ID, or {@code null} if the realm cannot be resolved, an asset-scoped + * engine has no targets, an asset is outside the engine's asset scope, or the assignee cannot + * access the targets * @throws IllegalArgumentException if an asset ID is invalid or belongs to a different realm */ public abstract Long create(Alarm alarm, List assetIds); From c90e6431bac1492074ab3a66a18ee29f387ce766 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:27:55 +0200 Subject: [PATCH 10/14] Add failing tests for global-rule alarm asset linking --- .../test/rules/AlarmFacadeTest.groovy | 99 ++++++++++++++++++- 1 file changed, 94 insertions(+), 5 deletions(-) diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index ad0bfffeef..7fbec7e98e 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -96,6 +96,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { testAssets.sibling = createAsset("Sibling", realm, testAssets.ancestor.id) testAssets.unrelated = createAsset("Unrelated", realm) testAssets.otherRealm = createAsset("Other realm", otherRealm) + testAssets.otherRealmChild = createAsset("Other realm child", otherRealm, testAssets.otherRealm.id) testUsers.unrestricted = identityProvider.getUser(keycloakSetup.testuser2Id) testUsers.restricted = identityProvider.getUser(keycloakSetup.testuser3Id) @@ -628,6 +629,92 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { assertNotificationRecipients(unrestrictedNotificationRecipients()) } + def "global scope links targets #targetNames using the existing alarm realm #alarmRealm with links #existingNames"() { + given: + def alarm = createExistingAlarm(existingNames, null, alarmRealm) + def previousLinks = linkTimestamps(alarm) + def targetIds = assetIdsFor(targetNames) + + when: + facadeFor("global").linkAssets(targetIds, alarm.id) + + then: "global engines can link anywhere in the alarm's realm, including an initially unlinked alarm" + noExceptionThrown() + assertLinkState(alarm, assetIdsFor(existingNames) + targetIds, previousLinks) + alarmService.getAlarm(alarm.id).realm == alarmRealm + + where: + alarmRealm | existingNames | targetNames + realm | ["scoped"] | ["ancestor", "child", "sibling", "unrelated"] + realm | ["child", "sibling"] | ["grandchild"] + realm | [] | ["child", "sibling"] + otherRealm | ["otherRealm"] | ["otherRealmChild"] + otherRealm | [] | ["otherRealm", "otherRealmChild"] + } + + def "global scope rejects targets #targetNames outside the existing alarm realm #alarmRealm with links #existingNames"() { + given: + def alarm = createExistingAlarm(existingNames, null, alarmRealm) + def previousLinks = linkTimestamps(alarm) + + when: + facadeFor("global").linkAssets(assetIdsFor(targetNames), alarm.id) + + then: "the alarm's realm is authoritative even when it has no current links" + thrown(AlarmService.AlarmAssetLinkRealmMismatchException) + assertLinkState(alarm, assetIdsFor(existingNames), previousLinks) + alarmService.getAlarm(alarm.id).realm == alarmRealm + alarmService.getAssetLinks(alarm.id, alarmRealm == realm ? otherRealm : realm).isEmpty() + + where: + alarmRealm | existingNames | targetNames + otherRealm | ["otherRealm"] | ["child"] + otherRealm | ["otherRealm"] | ["otherRealmChild", "child"] + otherRealm | ["otherRealm"] | ["child", "otherRealmChild"] + realm | [] | ["otherRealm"] + otherRealm | [] | ["child"] + } + + def "global scope linking an unlinked alarm with #assignee assignee linked to #linkedNames is allowed: #allowed"() { + given: + def user = testUsers[assignee] + if (assignee == "restricted") { + linkNotificationRecipient(linkedNames) + } + def alarm = createExistingAlarm([], user.id) + def previousLinks = linkTimestamps(alarm) + def targetIds = assetIdsFor(["child", "sibling"]) + + when: + facadeFor("global").linkAssets(targetIds, alarm.id) + + then: + noExceptionThrown() + assertLinkState(alarm, allowed ? targetIds : [], previousLinks) + + where: + assignee | linkedNames | allowed + "unrestricted" | [] | true + "restricted" | ["child", "sibling"] | true + "restricted" | [] | false + "restricted" | ["child"] | false + "restricted" | ["ancestor"] | false + "otherRealm" | [] | false + } + + def "global scope checks the assignee against the existing alarm's realm"() { + given: "an unrestricted assignee from a different realm than the alarm and its targets" + def alarm = createExistingAlarm(["otherRealm"], testUsers.unrestricted.id, otherRealm) + def previousLinks = linkTimestamps(alarm) + + when: + facadeFor("global").linkAssets(assetIdsFor(["otherRealmChild"]), alarm.id) + + then: + noExceptionThrown() + assertLinkState(alarm, assetIdsFor(["otherRealm"]), previousLinks) + } + def "#scope scope linking targets #targetNames is allowed: #allowed"() { given: "an alarm already linked to the scope asset" def alarm = createExistingAlarm(["scoped"]) @@ -709,7 +796,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { assertLinkState(alarm, allowed ? assetIdsFor(["scoped"]) + targetIds : assetIdsFor(["scoped"]), previousLinks) where: - [scope, assignee, targetNames, linkedNames, allowed] << ["realm", "asset"].collectMany { scope -> + [scope, assignee, targetNames, linkedNames, allowed] << ["realm", "asset", "global"].collectMany { scope -> linkAssigneeCases().collect { row -> [scope] + row } } } @@ -727,7 +814,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) where: - [scope, targetNames] << ["realm", "asset"].collectMany { scope -> + [scope, targetNames] << ["realm", "asset", "global"].collectMany { scope -> [["otherRealm"], ["child", "otherRealm"], ["otherRealm", "child"]].collect { targets -> [scope, targets] } @@ -772,7 +859,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { assertLinkState(alarm, assetIdsFor(["scoped", "child"]), firstLinks) where: - [scope, targetNames] << ["realm", "asset"].collectMany { scope -> + [scope, targetNames] << ["realm", "asset", "global"].collectMany { scope -> [["child", "child"], ["scoped", "child", "child"]].collect { targets -> [scope, targets] } } } @@ -797,6 +884,8 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { "realm" | [] "asset" | null "asset" | [] + "global" | null + "global" | [] } def "#scope scope rejects invalid target #invalidTarget without partially linking"() { @@ -813,7 +902,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) where: - [scope, invalidTarget, exceptionType] << ["realm", "asset"].collectMany { scope -> + [scope, invalidTarget, exceptionType] << ["realm", "asset", "global"].collectMany { scope -> [ [null, IllegalArgumentException], ["", IllegalArgumentException], @@ -837,7 +926,7 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { assertLinkState(alarm, assetIdsFor(["scoped"]), previousLinks) where: - [scope, invalidAlarmId, exceptionType] << ["realm", "asset"].collectMany { scope -> + [scope, invalidAlarmId, exceptionType] << ["realm", "asset", "global"].collectMany { scope -> [ [null, IllegalArgumentException], [-1L, IllegalArgumentException], From 44c0ac4c854319f4da30d94ee3e5bc1f8bc7886b Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Tue, 20 Oct 2026 17:35:28 +0200 Subject: [PATCH 11/14] Fix realm resolution for global-rule alarm asset linking --- .../openremote/manager/rules/facade/AlarmFacade.java | 10 ++++++---- .../main/java/org/openremote/model/rules/Alarms.java | 12 ++++++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java index 8306e1348e..cb488b092d 100644 --- a/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java +++ b/manager/src/main/java/org/openremote/manager/rules/facade/AlarmFacade.java @@ -109,11 +109,13 @@ public void linkAssets(List assetIds, Long alarmId) { return; } - String realm = - rulesEngineId - .getRealm() - .orElseThrow(() -> new IllegalStateException("Realm ruleset must have a realm ID")); SentAlarm alarm = alarmService.getAlarm(alarmId); + String realm = + rulesEngineId.getScope() == GlobalRuleset.class + ? alarm.getRealm() + : rulesEngineId + .getRealm() + .orElseThrow(() -> new IllegalStateException("Realm ruleset must have a realm ID")); if (!realm.equals(alarm.getRealm())) { throw new AlarmService.AlarmAssetLinkRealmMismatchException( "Alarm '" + alarmId + "' is not in realm '" + realm + "'"); diff --git a/model/src/main/java/org/openremote/model/rules/Alarms.java b/model/src/main/java/org/openremote/model/rules/Alarms.java index d0665f7fea..f708cca25a 100644 --- a/model/src/main/java/org/openremote/model/rules/Alarms.java +++ b/model/src/main/java/org/openremote/model/rules/Alarms.java @@ -46,10 +46,14 @@ public abstract class Alarms { public abstract Long create(Alarm alarm, List assetIds); /** - * Adds asset links to an alarm in the engine's realm. Asset-scoped engines require at least one - * existing link, and all existing and requested assets must be within the engine's subtree. - * Assignees must belong to the alarm's realm; restricted assignees must have explicit links to - * every existing and requested asset. + * Adds asset links to an existing alarm. Realm- and asset-scoped engines require the alarm to + * belong to the engine's realm. Global engines use the existing alarm's realm. Every requested + * asset must belong to the alarm's realm. + * + *

Asset-scoped engines require at least one existing link, and all existing and requested + * assets must be within the engine's subtree. An assignee is optional; when provided, the user + * must belong to the alarm's realm. Restricted assignees must have explicit links to every + * existing and requested asset. * *

No links are changed if scope or assignee access is denied. Null or empty asset lists are * ignored. Duplicate and existing links are accepted without changing their timestamps. From d846d9dea4a03301bec1fa7e6dad09e8c4a3e00e Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:07:57 +0100 Subject: [PATCH 12/14] Resolve alarm notification recipients before persisting changes --- .../manager/alarm/AlarmService.java | 88 +++++++++---- .../test/rules/AlarmFacadeTest.groovy | 124 ++++++++++++++++++ 2 files changed, 184 insertions(+), 28 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java index ad1428e0f4..05c245b264 100644 --- a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java +++ b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java @@ -30,9 +30,9 @@ import java.text.DateFormat; import java.time.Instant; import java.util.*; +import java.util.function.Supplier; import java.util.logging.Logger; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.apache.camel.builder.RouteBuilder; import org.hibernate.Session; import org.openremote.container.message.MessageBrokerService; @@ -189,7 +189,11 @@ public void validateAssetsInRealm(String realm, Collection assetIds) { } } - /** Sends an alarm. Callers are responsible for enforcing realm authorization. */ + /** + * Sends an alarm. Notification recipients and their permissions are resolved before persistence; + * notification submission happens afterward. Callers are responsible for enforcing realm + * authorization. + */ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { Objects.requireNonNull(alarm, "Alarm cannot be null"); Objects.requireNonNull(alarm.getRealm(), "Alarm realm cannot be null"); @@ -202,6 +206,14 @@ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { validateAssetsInRealm(alarm.getRealm(), assetIds); } + List notificationUsers = + getAlarmNotificationUsers( + alarm.getSeverity(), + alarm.getRealm(), + alarm.getAssigneeId(), + () -> assetIds, + alarm.getSource() == MANUAL ? Set.of(alarm.getSourceId()) : Set.of()); + Instant timestamp = Instant.ofEpochMilli(timerService.getCurrentTimeMillis()); SentAlarm sentAlarm = persistenceService.doReturningTransaction( @@ -226,26 +238,20 @@ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { clientEventService.publishEvent( new AlarmEvent(alarm.getRealm(), PersistenceEvent.Cause.CREATE)); if (alarm.getSeverity() == Alarm.Severity.HIGH) { - Set excludeUserIds = - alarm.getSource() == MANUAL ? Set.of(alarm.getSourceId()) : Set.of(); - sendAssigneeNotification(sentAlarm, excludeUserIds); + sendAssigneeNotification(sentAlarm, notificationUsers); } return sentAlarm; } /** - * Sends e-mail and push notifications for an alarm to the alarm assignee. If an assignee is not - * set, users in the alarm's realm having the {@link Constants#SUPER_USER_REALM_ROLE} or {@link - * Constants#WRITE_ALARMS_ROLE} are notified. Restricted users must have explicit links to every - * linked asset; they are not notified for alarms without linked assets. + * Submits e-mail and push notifications to recipients resolved before the alarm was persisted. + * Submission failures do not roll back the already persisted alarm. * * @param alarm the alarm to send e-mail and push notifications for - * @param excludeUserIds users matching these user IDs are excluded from the notifications + * @param users eligible recipients, with exclusions and asset-access checks already applied */ - protected void sendAssigneeNotification(SentAlarm alarm, Set excludeUserIds) { - List users = getAlarmNotificationUsers(alarm); - users.removeIf(user -> excludeUserIds.contains(user.getId())); + protected void sendAssigneeNotification(SentAlarm alarm, List users) { if (users.isEmpty()) { LOG.fine("No matching users to send alarm notification"); return; @@ -289,30 +295,43 @@ protected void sendAssigneeNotification(SentAlarm alarm, Set excludeUser notificationService.sendNotificationAsync(email, Notification.Source.INTERNAL, "alarms"); } - private List getAlarmNotificationUsers(SentAlarm alarm) { + /** + * Resolves recipients for HIGH alarms before any alarm write. Target assets are loaded only for + * unassigned HIGH alarms. Restricted recipients require explicit links to every target asset; + * alarms without targets exclude restricted recipients. + */ + private List getAlarmNotificationUsers( + Alarm.Severity severity, + String realm, + String assigneeId, + Supplier> assetIdsSupplier, + Set excludeUserIds) { + if (severity != Alarm.Severity.HIGH) return List.of(); + ManagerIdentityProvider identityProvider = identityService.getIdentityProvider(); List users = new ArrayList<>(); - if (alarm.getAssigneeId() == null) { + if (assigneeId == null) { UserQuery userQuery = new UserQuery() - .realm(new RealmPredicate(alarm.getRealm())) + .realm(new RealmPredicate(realm)) .clientRoles(new StringPredicate(Constants.WRITE_ALARMS_ROLE)) .realmRoles(new StringPredicate(Constants.SUPER_USER_REALM_ROLE)) .serviceUsers(false); users.addAll(Arrays.asList(identityProvider.queryUsers(userQuery))); - List assetIds = - getAssetLinks(alarm.getId(), alarm.getRealm()).stream() - .map(link -> link.getId().getAssetId()) - .distinct() - .toList(); + users.removeIf(user -> excludeUserIds.contains(user.getId())); + List assetIds = assetIdsSupplier.get(); + List uniqueAssetIds = + assetIds == null ? List.of() : assetIds.stream().distinct().toList(); // Both notification channels include all linked assets, so partial access is insufficient. users.removeIf( user -> - Arrays.asList(identityProvider.getUserRealmRoles(alarm.getRealm(), user.getId())) + Arrays.asList(identityProvider.getUserRealmRoles(realm, user.getId())) .contains(Constants.RESTRICTED_USER_REALM_ROLE) - && !assetStorageService.isUserAssets(user.getId(), assetIds)); + && !assetStorageService.isUserAssets(user.getId(), uniqueAssetIds)); } else { - users.add(identityProvider.getUser(alarm.getAssigneeId())); + if (!excludeUserIds.contains(assigneeId)) { + users.add(identityProvider.getUser(assigneeId)); + } } return users; } @@ -372,11 +391,26 @@ private String getAlarmNotificationText(Map content) { return sb.toString(); } - /** Updates an existing alarm. Callers are responsible for enforcing realm authorization. */ + /** + * Updates an existing alarm. Notification recipients and their permissions are resolved before + * persistence; notification submission happens afterward. Callers are responsible for enforcing + * realm authorization. + */ public void updateAlarm(SentAlarm oldAlarm, SentAlarm newAlarm) { String oldAssigneeId = oldAlarm.getAssigneeId(); String newAssigneeId = newAlarm.getAssigneeId(); + List notificationUsers = + getAlarmNotificationUsers( + newAlarm.getSeverity(), + oldAlarm.getRealm(), + newAssigneeId, + () -> + getAssetLinks(oldAlarm.getId(), oldAlarm.getRealm()).stream() + .map(link -> link.getId().getAssetId()) + .toList(), + oldAssigneeId == null ? Set.of() : Set.of(oldAssigneeId)); + persistenceService.doTransaction( entityManager -> entityManager @@ -399,9 +433,7 @@ public void updateAlarm(SentAlarm oldAlarm, SentAlarm newAlarm) { new AlarmEvent(newAlarm.getRealm(), PersistenceEvent.Cause.UPDATE)); if (newAlarm.getSeverity() == Alarm.Severity.HIGH) { - Set excludeUserIds = - Stream.of(oldAssigneeId).filter(Objects::nonNull).collect(Collectors.toSet()); - sendAssigneeNotification(getAlarm(oldAlarm.getId()), excludeUserIds); + sendAssigneeNotification(getAlarm(oldAlarm.getId()), notificationUsers); } } diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index 7fbec7e98e..921d635ef4 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -35,6 +35,7 @@ import org.openremote.model.asset.impl.ThingAsset import org.openremote.model.notification.EmailNotificationMessage import org.openremote.model.notification.Notification import org.openremote.model.notification.PushNotificationMessage +import org.openremote.model.query.UserQuery import org.openremote.model.security.ClientRole import org.openremote.model.security.User import org.openremote.model.util.UniqueIdentifierGenerator @@ -581,6 +582,129 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { "update" | [] } + def "notification #failureAt failure leaves no new alarm or links for targets #targetNames"() { + given: + def failure = new IllegalStateException("Notification ${failureAt} unavailable") + def assigneeId = failureAt == "assignee lookup" ? testUsers.unrestricted.id : null + def alarm = newAlarm(assigneeId, Alarm.Severity.HIGH).setSource(Alarm.Source.REALM_RULESET) + failNotificationPreparation(failureAt, failure) + + when: + alarmService.sendAlarm(alarm, assetIdsFor(targetNames)) + + then: + def thrownFailure = thrown(IllegalStateException) + thrownFailure.is(failure) + createdAlarms().isEmpty() + queuedNotifications.isEmpty() + + cleanup: + alarmService.identityService = identityService + alarmService.assetStorageService = assetStorageService + + where: + [failureAt, targetNames] << [ + ["recipient lookup", "role lookup", "asset access", "assignee lookup"], + [[], ["child", "grandchild"]] + ].combinations() + } + + def "notification #failureAt failure leaves an existing alarm unchanged for targets #targetNames"() { + given: + def oldAlarm = createExistingAlarm(targetNames) + def previousLinks = linkTimestamps(oldAlarm) + def updatedAlarm = alarmService.getAlarm(oldAlarm.id) + updatedAlarm.content = "Changed content" + updatedAlarm.status = Alarm.Status.ACKNOWLEDGED + updatedAlarm.severity = Alarm.Severity.HIGH + updatedAlarm.assigneeId = failureAt == "assignee lookup" ? testUsers.unrestricted.id : null + def failure = new IllegalStateException("Notification ${failureAt} unavailable") + failNotificationPreparation(failureAt, failure) + + when: + alarmService.updateAlarm(oldAlarm, updatedAlarm) + + then: + def thrownFailure = thrown(IllegalStateException) + thrownFailure.is(failure) + def stored = alarmService.getAlarm(oldAlarm.id) + verifyAll { + stored.content == oldAlarm.content + stored.status == oldAlarm.status + stored.severity == oldAlarm.severity + stored.assigneeId == oldAlarm.assigneeId + stored.lastModified == oldAlarm.lastModified + } + assertLinkState(stored, assetIdsFor(targetNames), previousLinks) + + cleanup: + alarmService.identityService = identityService + alarmService.assetStorageService = assetStorageService + + where: + [failureAt, targetNames] << [ + ["recipient lookup", "role lookup", "asset access", "assignee lookup"], + [[], ["child", "grandchild"]] + ].combinations() + } + + def "notification submission failure after #operation preserves the persisted alarm"() { + given: "recipient preparation succeeds, but notification submission fails after persistence" + def oldAlarm = operation == "update" ? createExistingAlarm(["child"]) : null + def failure = new IllegalStateException("Notification submission unavailable") + alarmService.notificationService = Stub(NotificationService) { + sendNotificationAsync(_ as Notification, _ as Notification.Source, _ as String) >> { + throw failure + } + } + + when: + if (oldAlarm == null) { + createOrUpdateUnassignedHighAlarm(operation, ["child"]) + } else { + def updatedAlarm = alarmService.getAlarm(oldAlarm.id) + updatedAlarm.severity = Alarm.Severity.HIGH + alarmService.updateAlarm(oldAlarm, updatedAlarm) + } + + then: + def thrownFailure = thrown(IllegalStateException) + thrownFailure.is(failure) + def alarms = createdAlarms() + alarms.size() == 1 + alarms[0].severity == Alarm.Severity.HIGH + if (oldAlarm != null) { + assert alarms[0].id == oldAlarm.id + } + assertLinkState(alarms[0], assetIdsFor(["child"]), [:]) + + where: + operation << ["realm create", "update"] + } + + private void failNotificationPreparation(String failureAt, RuntimeException failure) { + def provider = Spy(identityService.identityProvider) + def storage = Spy(assetStorageService) + switch (failureAt) { + case "recipient lookup": + provider.queryUsers(_ as UserQuery) >> { throw failure } + break + case "role lookup": + provider.getUserRealmRoles(_ as String, _ as String) >> { throw failure } + break + case "asset access": + storage.isUserAssets(_ as String, _ as List) >> { throw failure } + break + case "assignee lookup": + provider.getUser(_ as String) >> { throw failure } + break + } + alarmService.identityService = Stub(ManagerIdentityService) { + getIdentityProvider() >> provider + } + alarmService.assetStorageService = storage + } + def "updates recheck restricted recipient access after an asset link is revoked"() { given: linkNotificationRecipient(["child", "grandchild"]) From 92f650255686bf2fcc7db1d7fd864fc9d575278c Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Fri, 30 Oct 2026 10:32:34 +0100 Subject: [PATCH 13/14] Revert "Resolve alarm notification recipients before persisting changes" This reverts commit d846d9dea4a03301bec1fa7e6dad09e8c4a3e00e. --- .../manager/alarm/AlarmService.java | 88 ++++--------- .../test/rules/AlarmFacadeTest.groovy | 124 ------------------ 2 files changed, 28 insertions(+), 184 deletions(-) diff --git a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java index 05c245b264..ad1428e0f4 100644 --- a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java +++ b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java @@ -30,9 +30,9 @@ import java.text.DateFormat; import java.time.Instant; import java.util.*; -import java.util.function.Supplier; import java.util.logging.Logger; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.apache.camel.builder.RouteBuilder; import org.hibernate.Session; import org.openremote.container.message.MessageBrokerService; @@ -189,11 +189,7 @@ public void validateAssetsInRealm(String realm, Collection assetIds) { } } - /** - * Sends an alarm. Notification recipients and their permissions are resolved before persistence; - * notification submission happens afterward. Callers are responsible for enforcing realm - * authorization. - */ + /** Sends an alarm. Callers are responsible for enforcing realm authorization. */ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { Objects.requireNonNull(alarm, "Alarm cannot be null"); Objects.requireNonNull(alarm.getRealm(), "Alarm realm cannot be null"); @@ -206,14 +202,6 @@ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { validateAssetsInRealm(alarm.getRealm(), assetIds); } - List notificationUsers = - getAlarmNotificationUsers( - alarm.getSeverity(), - alarm.getRealm(), - alarm.getAssigneeId(), - () -> assetIds, - alarm.getSource() == MANUAL ? Set.of(alarm.getSourceId()) : Set.of()); - Instant timestamp = Instant.ofEpochMilli(timerService.getCurrentTimeMillis()); SentAlarm sentAlarm = persistenceService.doReturningTransaction( @@ -238,20 +226,26 @@ public SentAlarm sendAlarm(Alarm alarm, List assetIds) { clientEventService.publishEvent( new AlarmEvent(alarm.getRealm(), PersistenceEvent.Cause.CREATE)); if (alarm.getSeverity() == Alarm.Severity.HIGH) { - sendAssigneeNotification(sentAlarm, notificationUsers); + Set excludeUserIds = + alarm.getSource() == MANUAL ? Set.of(alarm.getSourceId()) : Set.of(); + sendAssigneeNotification(sentAlarm, excludeUserIds); } return sentAlarm; } /** - * Submits e-mail and push notifications to recipients resolved before the alarm was persisted. - * Submission failures do not roll back the already persisted alarm. + * Sends e-mail and push notifications for an alarm to the alarm assignee. If an assignee is not + * set, users in the alarm's realm having the {@link Constants#SUPER_USER_REALM_ROLE} or {@link + * Constants#WRITE_ALARMS_ROLE} are notified. Restricted users must have explicit links to every + * linked asset; they are not notified for alarms without linked assets. * * @param alarm the alarm to send e-mail and push notifications for - * @param users eligible recipients, with exclusions and asset-access checks already applied + * @param excludeUserIds users matching these user IDs are excluded from the notifications */ - protected void sendAssigneeNotification(SentAlarm alarm, List users) { + protected void sendAssigneeNotification(SentAlarm alarm, Set excludeUserIds) { + List users = getAlarmNotificationUsers(alarm); + users.removeIf(user -> excludeUserIds.contains(user.getId())); if (users.isEmpty()) { LOG.fine("No matching users to send alarm notification"); return; @@ -295,43 +289,30 @@ protected void sendAssigneeNotification(SentAlarm alarm, List users) { notificationService.sendNotificationAsync(email, Notification.Source.INTERNAL, "alarms"); } - /** - * Resolves recipients for HIGH alarms before any alarm write. Target assets are loaded only for - * unassigned HIGH alarms. Restricted recipients require explicit links to every target asset; - * alarms without targets exclude restricted recipients. - */ - private List getAlarmNotificationUsers( - Alarm.Severity severity, - String realm, - String assigneeId, - Supplier> assetIdsSupplier, - Set excludeUserIds) { - if (severity != Alarm.Severity.HIGH) return List.of(); - + private List getAlarmNotificationUsers(SentAlarm alarm) { ManagerIdentityProvider identityProvider = identityService.getIdentityProvider(); List users = new ArrayList<>(); - if (assigneeId == null) { + if (alarm.getAssigneeId() == null) { UserQuery userQuery = new UserQuery() - .realm(new RealmPredicate(realm)) + .realm(new RealmPredicate(alarm.getRealm())) .clientRoles(new StringPredicate(Constants.WRITE_ALARMS_ROLE)) .realmRoles(new StringPredicate(Constants.SUPER_USER_REALM_ROLE)) .serviceUsers(false); users.addAll(Arrays.asList(identityProvider.queryUsers(userQuery))); - users.removeIf(user -> excludeUserIds.contains(user.getId())); - List assetIds = assetIdsSupplier.get(); - List uniqueAssetIds = - assetIds == null ? List.of() : assetIds.stream().distinct().toList(); + List assetIds = + getAssetLinks(alarm.getId(), alarm.getRealm()).stream() + .map(link -> link.getId().getAssetId()) + .distinct() + .toList(); // Both notification channels include all linked assets, so partial access is insufficient. users.removeIf( user -> - Arrays.asList(identityProvider.getUserRealmRoles(realm, user.getId())) + Arrays.asList(identityProvider.getUserRealmRoles(alarm.getRealm(), user.getId())) .contains(Constants.RESTRICTED_USER_REALM_ROLE) - && !assetStorageService.isUserAssets(user.getId(), uniqueAssetIds)); + && !assetStorageService.isUserAssets(user.getId(), assetIds)); } else { - if (!excludeUserIds.contains(assigneeId)) { - users.add(identityProvider.getUser(assigneeId)); - } + users.add(identityProvider.getUser(alarm.getAssigneeId())); } return users; } @@ -391,26 +372,11 @@ private String getAlarmNotificationText(Map content) { return sb.toString(); } - /** - * Updates an existing alarm. Notification recipients and their permissions are resolved before - * persistence; notification submission happens afterward. Callers are responsible for enforcing - * realm authorization. - */ + /** Updates an existing alarm. Callers are responsible for enforcing realm authorization. */ public void updateAlarm(SentAlarm oldAlarm, SentAlarm newAlarm) { String oldAssigneeId = oldAlarm.getAssigneeId(); String newAssigneeId = newAlarm.getAssigneeId(); - List notificationUsers = - getAlarmNotificationUsers( - newAlarm.getSeverity(), - oldAlarm.getRealm(), - newAssigneeId, - () -> - getAssetLinks(oldAlarm.getId(), oldAlarm.getRealm()).stream() - .map(link -> link.getId().getAssetId()) - .toList(), - oldAssigneeId == null ? Set.of() : Set.of(oldAssigneeId)); - persistenceService.doTransaction( entityManager -> entityManager @@ -433,7 +399,9 @@ public void updateAlarm(SentAlarm oldAlarm, SentAlarm newAlarm) { new AlarmEvent(newAlarm.getRealm(), PersistenceEvent.Cause.UPDATE)); if (newAlarm.getSeverity() == Alarm.Severity.HIGH) { - sendAssigneeNotification(getAlarm(oldAlarm.getId()), notificationUsers); + Set excludeUserIds = + Stream.of(oldAssigneeId).filter(Objects::nonNull).collect(Collectors.toSet()); + sendAssigneeNotification(getAlarm(oldAlarm.getId()), excludeUserIds); } } diff --git a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy index 921d635ef4..7fbec7e98e 100644 --- a/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy +++ b/test/src/test/groovy/org/openremote/test/rules/AlarmFacadeTest.groovy @@ -35,7 +35,6 @@ import org.openremote.model.asset.impl.ThingAsset import org.openremote.model.notification.EmailNotificationMessage import org.openremote.model.notification.Notification import org.openremote.model.notification.PushNotificationMessage -import org.openremote.model.query.UserQuery import org.openremote.model.security.ClientRole import org.openremote.model.security.User import org.openremote.model.util.UniqueIdentifierGenerator @@ -582,129 +581,6 @@ class AlarmFacadeTest extends Specification implements ManagerContainerTrait { "update" | [] } - def "notification #failureAt failure leaves no new alarm or links for targets #targetNames"() { - given: - def failure = new IllegalStateException("Notification ${failureAt} unavailable") - def assigneeId = failureAt == "assignee lookup" ? testUsers.unrestricted.id : null - def alarm = newAlarm(assigneeId, Alarm.Severity.HIGH).setSource(Alarm.Source.REALM_RULESET) - failNotificationPreparation(failureAt, failure) - - when: - alarmService.sendAlarm(alarm, assetIdsFor(targetNames)) - - then: - def thrownFailure = thrown(IllegalStateException) - thrownFailure.is(failure) - createdAlarms().isEmpty() - queuedNotifications.isEmpty() - - cleanup: - alarmService.identityService = identityService - alarmService.assetStorageService = assetStorageService - - where: - [failureAt, targetNames] << [ - ["recipient lookup", "role lookup", "asset access", "assignee lookup"], - [[], ["child", "grandchild"]] - ].combinations() - } - - def "notification #failureAt failure leaves an existing alarm unchanged for targets #targetNames"() { - given: - def oldAlarm = createExistingAlarm(targetNames) - def previousLinks = linkTimestamps(oldAlarm) - def updatedAlarm = alarmService.getAlarm(oldAlarm.id) - updatedAlarm.content = "Changed content" - updatedAlarm.status = Alarm.Status.ACKNOWLEDGED - updatedAlarm.severity = Alarm.Severity.HIGH - updatedAlarm.assigneeId = failureAt == "assignee lookup" ? testUsers.unrestricted.id : null - def failure = new IllegalStateException("Notification ${failureAt} unavailable") - failNotificationPreparation(failureAt, failure) - - when: - alarmService.updateAlarm(oldAlarm, updatedAlarm) - - then: - def thrownFailure = thrown(IllegalStateException) - thrownFailure.is(failure) - def stored = alarmService.getAlarm(oldAlarm.id) - verifyAll { - stored.content == oldAlarm.content - stored.status == oldAlarm.status - stored.severity == oldAlarm.severity - stored.assigneeId == oldAlarm.assigneeId - stored.lastModified == oldAlarm.lastModified - } - assertLinkState(stored, assetIdsFor(targetNames), previousLinks) - - cleanup: - alarmService.identityService = identityService - alarmService.assetStorageService = assetStorageService - - where: - [failureAt, targetNames] << [ - ["recipient lookup", "role lookup", "asset access", "assignee lookup"], - [[], ["child", "grandchild"]] - ].combinations() - } - - def "notification submission failure after #operation preserves the persisted alarm"() { - given: "recipient preparation succeeds, but notification submission fails after persistence" - def oldAlarm = operation == "update" ? createExistingAlarm(["child"]) : null - def failure = new IllegalStateException("Notification submission unavailable") - alarmService.notificationService = Stub(NotificationService) { - sendNotificationAsync(_ as Notification, _ as Notification.Source, _ as String) >> { - throw failure - } - } - - when: - if (oldAlarm == null) { - createOrUpdateUnassignedHighAlarm(operation, ["child"]) - } else { - def updatedAlarm = alarmService.getAlarm(oldAlarm.id) - updatedAlarm.severity = Alarm.Severity.HIGH - alarmService.updateAlarm(oldAlarm, updatedAlarm) - } - - then: - def thrownFailure = thrown(IllegalStateException) - thrownFailure.is(failure) - def alarms = createdAlarms() - alarms.size() == 1 - alarms[0].severity == Alarm.Severity.HIGH - if (oldAlarm != null) { - assert alarms[0].id == oldAlarm.id - } - assertLinkState(alarms[0], assetIdsFor(["child"]), [:]) - - where: - operation << ["realm create", "update"] - } - - private void failNotificationPreparation(String failureAt, RuntimeException failure) { - def provider = Spy(identityService.identityProvider) - def storage = Spy(assetStorageService) - switch (failureAt) { - case "recipient lookup": - provider.queryUsers(_ as UserQuery) >> { throw failure } - break - case "role lookup": - provider.getUserRealmRoles(_ as String, _ as String) >> { throw failure } - break - case "asset access": - storage.isUserAssets(_ as String, _ as List) >> { throw failure } - break - case "assignee lookup": - provider.getUser(_ as String) >> { throw failure } - break - } - alarmService.identityService = Stub(ManagerIdentityService) { - getIdentityProvider() >> provider - } - alarmService.assetStorageService = storage - } - def "updates recheck restricted recipient access after an asset link is revoked"() { given: linkNotificationRecipient(["child", "grandchild"]) From fe81673febcdcb9eaa0650b8a9d4bf2815e17d89 Mon Sep 17 00:00:00 2001 From: Eric Bariaux <375613+ebariaux@users.noreply.github.com> Date: Fri, 30 Oct 2026 11:41:18 +0100 Subject: [PATCH 14/14] Batch restricted alarm recipient lookup with UserQuery --- .../manager/alarm/AlarmService.java | 16 +- .../alarm/AlarmServiceNotificationTest.groovy | 181 ++++++++++++++++++ 2 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 manager/src/test/groovy/org/openremote/manager/alarm/AlarmServiceNotificationTest.groovy diff --git a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java index ad1428e0f4..0ef71fe331 100644 --- a/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java +++ b/manager/src/main/java/org/openremote/manager/alarm/AlarmService.java @@ -300,6 +300,19 @@ private List getAlarmNotificationUsers(SentAlarm alarm) { .realmRoles(new StringPredicate(Constants.SUPER_USER_REALM_ROLE)) .serviceUsers(false); users.addAll(Arrays.asList(identityProvider.queryUsers(userQuery))); + if (users.isEmpty()) { + return users; + } + // UserQuery matches directly assigned roles; composite roles are not expanded. + UserQuery restrictedUsersQuery = + new UserQuery() + .realm(new RealmPredicate(alarm.getRealm())) + .ids(users.stream().map(User::getId).toArray(String[]::new)) + .realmRoles(new StringPredicate(Constants.RESTRICTED_USER_REALM_ROLE)); + Set restrictedUserIds = + Arrays.stream(identityProvider.queryUsers(restrictedUsersQuery)) + .map(User::getId) + .collect(Collectors.toSet()); List assetIds = getAssetLinks(alarm.getId(), alarm.getRealm()).stream() .map(link -> link.getId().getAssetId()) @@ -308,8 +321,7 @@ private List getAlarmNotificationUsers(SentAlarm alarm) { // Both notification channels include all linked assets, so partial access is insufficient. users.removeIf( user -> - Arrays.asList(identityProvider.getUserRealmRoles(alarm.getRealm(), user.getId())) - .contains(Constants.RESTRICTED_USER_REALM_ROLE) + restrictedUserIds.contains(user.getId()) && !assetStorageService.isUserAssets(user.getId(), assetIds)); } else { users.add(identityProvider.getUser(alarm.getAssigneeId())); diff --git a/manager/src/test/groovy/org/openremote/manager/alarm/AlarmServiceNotificationTest.groovy b/manager/src/test/groovy/org/openremote/manager/alarm/AlarmServiceNotificationTest.groovy new file mode 100644 index 0000000000..12cf73aca7 --- /dev/null +++ b/manager/src/test/groovy/org/openremote/manager/alarm/AlarmServiceNotificationTest.groovy @@ -0,0 +1,181 @@ +/* + * Copyright 2026, OpenRemote Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package org.openremote.manager.alarm + +import java.time.Instant +import org.openremote.manager.asset.AssetStorageService +import org.openremote.manager.notification.NotificationService +import org.openremote.manager.security.ManagerIdentityProvider +import org.openremote.manager.security.ManagerIdentityService +import org.openremote.model.Container +import org.openremote.model.alarm.Alarm +import org.openremote.model.alarm.AlarmAssetLink +import org.openremote.model.alarm.SentAlarm +import org.openremote.model.notification.EmailNotificationMessage +import org.openremote.model.notification.Notification +import org.openremote.model.notification.PushNotificationMessage +import org.openremote.model.query.UserQuery +import org.openremote.model.security.User +import spock.lang.Specification + +import static org.openremote.model.Constants.RESTRICTED_USER_REALM_ROLE +import static org.openremote.model.Constants.SUPER_USER_REALM_ROLE +import static org.openremote.model.Constants.WRITE_ALARMS_ROLE + +class AlarmServiceNotificationTest extends Specification { + + private static final String REALM = "building" + + def "unassigned alarm recipients use one restricted-user query for assets #assetIds"() { + given: + def unrestricted = user("unrestricted") + def fullyLinked = user("fully-linked") + def partlyLinked = user("partly-linked") + def unlinked = user("unlinked") + def candidates = [unrestricted, fullyLinked, partlyLinked, unlinked] + def restricted = [fullyLinked, partlyLinked, unlinked] + List queries = [] + def provider = Stub(ManagerIdentityProvider) { + queryUsers(_ as UserQuery) >> { UserQuery query -> + queries.add(query) + (query.realmRoles*.value == [RESTRICTED_USER_REALM_ROLE] ? restricted : candidates) as User[] + } + getUserRealmRoles(_, _) >> { + throw new AssertionError("Recipient filtering must not request roles separately for each user") + } + } + def storage = Mock(AssetStorageService) + List notifications = [] + def alarm = alarm() + def service = service(provider, storage, alarm, assetIds, notifications) + + when: + service.sendAssigneeNotification(alarm, [] as Set) + + then: "role detection is batched and limited to recipients in this alarm's realm" + queries.size() == 2 + verifyAll(queries[0]) { + realmPredicate.name == REALM + clientRoles*.value == [WRITE_ALARMS_ROLE] + realmRoles*.value == [SUPER_USER_REALM_ROLE] + serviceUsers == false + } + verifyAll(queries[1]) { + realmPredicate.name == REALM + ids.toSet() == candidates*.id.toSet() + realmRoles*.value == [RESTRICTED_USER_REALM_ROLE] + !realmRoles[0].negate + clientRoles == null + assets == null + } + + and: "only restricted users need links to every distinct asset" + 1 * storage.isUserAssets(fullyLinked.id, assetIds.unique(false)) >> !assetIds.empty + 1 * storage.isUserAssets(partlyLinked.id, assetIds.unique(false)) >> (assetIds.unique(false).size() == 1) + 1 * storage.isUserAssets(unlinked.id, assetIds.unique(false)) >> false + 0 * storage.isUserAssets(unrestricted.id, _) + def expected = [unrestricted] + (assetIds.empty ? [] : [fullyLinked]) + + (assetIds.unique(false).size() == 1 ? [partlyLinked] : []) + assertRecipients(notifications, expected) + + where: + assetIds << [[], ["child"], ["child", "grandchild"], ["child", "child"]] + } + + def "unassigned alarm with no candidates does not query restricted users or send notifications"() { + given: + def provider = Mock(ManagerIdentityProvider) + def storage = Mock(AssetStorageService) + List notifications = [] + def alarm = alarm() + def service = service(provider, storage, alarm, [], notifications) + + when: + service.sendAssigneeNotification(alarm, [] as Set) + + then: + 1 * provider.queryUsers(_ as UserQuery) >> new User[0] + 0 * provider._ + 0 * storage._ + notifications.empty + } + + def "assigned alarm keeps the assignee lookup without querying restricted users"() { + given: + def assignee = user("assignee") + def provider = Mock(ManagerIdentityProvider) + def storage = Mock(AssetStorageService) + List notifications = [] + def alarm = alarm().setAssigneeId(assignee.id) + def service = service(provider, storage, alarm, ["child"], notifications) + + when: + service.sendAssigneeNotification(alarm, [] as Set) + + then: + 1 * provider.getUser(assignee.id) >> assignee + 0 * provider._ + 0 * storage._ + assertRecipients(notifications, [assignee]) + } + + private AlarmService service(ManagerIdentityProvider provider, AssetStorageService storage, + SentAlarm alarm, List assetIds, List notifications) { + def links = assetIds.collect { new AlarmAssetLink(REALM, alarm.id, it) } + def service = Spy(AlarmService) { + getAssetLinks(alarm.id, REALM) >> links + } + def container = Stub(Container) { + getConfig() >> [:] + } + def identityService = Stub(ManagerIdentityService) { + getIdentityProvider() >> provider + } + def notificationService = Stub(NotificationService) { + sendNotificationAsync(_ as Notification, _ as Notification.Source, _ as String) >> { Notification notification, Notification.Source source, String sourceId -> + notifications.add(notification) + } + } + [container: container, identityService: identityService, assetStorageService: storage, + notificationService: notificationService].each { name, value -> + def field = AlarmService.getDeclaredField(name) + field.accessible = true + field.set(service, value) + } + service + } + + private static User user(String id) { + new User().setId(id).setRealm(REALM).setFirstName(id).setEmail("${id}@example.com") + } + + private static SentAlarm alarm() { + new SentAlarm().setId(42L).setRealm(REALM).setTitle("Test alarm").setContent("Test content") + .setSeverity(Alarm.Severity.HIGH).setStatus(Alarm.Status.OPEN) + .setSource(Alarm.Source.CLIENT).setCreatedOn(Instant.parse("2026-10-30T10:00:00Z")) + } + + private void assertRecipients(List notifications, List expected) { + assert notifications.size() == 2 + def push = notifications.find { it.message instanceof PushNotificationMessage } + def email = notifications.find { it.message instanceof EmailNotificationMessage } + assert push.targets*.id.toSet() == expected*.id.toSet() + assert email.message.to*.address.toSet() == expected*.email.toSet() + } +}