diff --git a/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java b/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java index 5414ed1891..7f710f43e2 100644 --- a/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java +++ b/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java @@ -270,77 +270,20 @@ public List getTargets( target -> { Notification.TargetType targetType = target.getType(); String targetId = target.getId(); - AssetQuery assetQuery = - new AssetQuery() - .select(new AssetQuery.Select().excludeAttributes()) - .types(ConsoleAsset.class) - .attributes( - new AttributePredicate( - ConsoleAsset.CONSOLE_PROVIDERS, - new ValueEmptyPredicate().negate(true), - false, - new NameValuePredicate.Path( - PushNotificationMessage.TYPE, "data", "token"))); - - switch (targetType) { - case REALM -> - // Any console assets in the target realm - assetQuery.realm(new RealmPredicate(targetId)); - case USER -> - // Any console assets linked to the target user - assetQuery.userIds(targetId); - case ASSET -> { - if (rulesRealm == null) { - // Any users linked to this asset and then their linked console assets - assetQuery.userIds( - assetStorageService.findUserAssetLinks(null, null, targetId).stream() - .map(ual -> ual.getId().getUserId()) - .distinct() - .toArray(String[]::new)); - } else { - // Resolve regular, non-system users in the rule's realm before expanding their - // consoles. - assetQuery.userIds( - Arrays.stream( - managerIdentityService - .getIdentityProvider() - .queryUsers( - new UserQuery() - .assets(targetId) - .realm(new RealmPredicate(rulesRealm)) - .serviceUsers(false) - .attributes( - new UserQuery.AttributeValuePredicate( - true, - new StringPredicate( - User.SYSTEM_ACCOUNT_ATTRIBUTE))))) - .filter( - user -> - source != Notification.Source.ASSET_RULESET - || NotificationScope.isUserInAssetScope( - user.getRealm(), - user.getId(), - sourceId, - managerIdentityService.getIdentityProvider(), - assetStorageService)) - .map(User::getId) - .distinct() - .toArray(String[]::new)); - } - } - } - - if (rulesRealm != null) { - // Delivery consoles must share the rule's realm, but need not be in its asset - // subtree. - assetQuery.realm(new RealmPredicate(rulesRealm)); - } - + // Prefer consoles that still have an FCM token so a normal send is unchanged. + // #3113: when none do, map FCM consoles anyway so the send records + // "No FCM token found" instead of a missing-targets error. Browser consoles + // (push version "web") stay out of both queries. Rules-realm scoping applies to both. List consoleAssetIds = - assetStorageService.findAll(assetQuery).stream() - .map(Asset::getId) - .distinct() - .toList(); + consoleIds( + consoleAssetsLinkedTo(targetType, targetId, rulesRealm, source, sourceId) + .attributes(nonEmptyFcmToken())); + if (consoleAssetIds.isEmpty()) { + consoleAssetIds = + consoleIds( + consoleAssetsLinkedTo(targetType, targetId, rulesRealm, source, sourceId) + .attributes(fcmProviderVersion())); + } if (!consoleAssetIds.isEmpty()) { // Special handling if target type is user (don't need to find all linked users) @@ -438,6 +381,25 @@ public void sendMessage( AbstractNotificationMessage message) throws Exception { + Notification.TargetType targetType = target.getType(); + String targetId = target.getId(); + + if (targetType != Notification.TargetType.ASSET + && targetType != Notification.TargetType.CUSTOM) { + String msg = "Target type not supported: " + targetType; + LOG.warning(msg); + throw new Exception(msg); + } + + // A console without a token must fail even when FCM itself is not configured. Otherwise the + // dev-mode skip below hides the missing token and the API reports success. + String fcmToken = consoleFCMTokenMap.get(targetId); + if (targetType == Notification.TargetType.ASSET && TextUtil.isNullOrEmpty(fcmToken)) { + String msg = "No FCM token found for console: " + targetId; + LOG.finer(msg); + throw new Exception(msg); + } + if (devMode && !valid) { // FCM not configured; log the notification instead of sending so the sent record is still // created @@ -451,19 +413,6 @@ public void sendMessage( return; } - Notification.TargetType targetType = target.getType(); - String targetId = target.getId(); - - if (targetType != Notification.TargetType.ASSET - && targetType != Notification.TargetType.CUSTOM) { - String msg = "Target type not supported: " + targetType; - LOG.warning(msg); - throw new Exception(msg); - } - - // Check this asset has an FCM token (i.e. it is registered for push notifications) - String fcmToken = consoleFCMTokenMap.get(targetId); - if (TextUtil.isNullOrEmpty(fcmToken)) { String msg = "No FCM token found for console: " + targetId; LOG.finer(msg); @@ -650,6 +599,91 @@ protected static Message buildFCMMessage(long id, PushNotificationMessage pushMe return builder.build(); } + private AssetQuery consoleAssetsLinkedTo( + Notification.TargetType targetType, + String targetId, + String rulesRealm, + Notification.Source source, + String sourceId) { + AssetQuery assetQuery = + new AssetQuery() + .select(new AssetQuery.Select().excludeAttributes()) + .types(ConsoleAsset.class); + + switch (targetType) { + case REALM -> + // Any console assets in the target realm + assetQuery.realm(new RealmPredicate(targetId)); + case USER -> + // Any console assets linked to the target user + assetQuery.userIds(targetId); + case ASSET -> { + if (rulesRealm == null) { + // Any users linked to this asset and then their linked console assets + assetQuery.userIds( + assetStorageService.findUserAssetLinks(null, null, targetId).stream() + .map(ual -> ual.getId().getUserId()) + .distinct() + .toArray(String[]::new)); + } else { + // Resolve regular, non-system users in the rule's realm before expanding their + // consoles. + assetQuery.userIds( + Arrays.stream( + managerIdentityService + .getIdentityProvider() + .queryUsers( + new UserQuery() + .assets(targetId) + .realm(new RealmPredicate(rulesRealm)) + .serviceUsers(false) + .attributes( + new UserQuery.AttributeValuePredicate( + true, + new StringPredicate(User.SYSTEM_ACCOUNT_ATTRIBUTE))))) + .filter( + user -> + source != Notification.Source.ASSET_RULESET + || NotificationScope.isUserInAssetScope( + user.getRealm(), + user.getId(), + sourceId, + managerIdentityService.getIdentityProvider(), + assetStorageService)) + .map(User::getId) + .distinct() + .toArray(String[]::new)); + } + } + } + + if (rulesRealm != null) { + // Delivery consoles must share the rule's realm, but need not be in its asset subtree. + assetQuery.realm(new RealmPredicate(rulesRealm)); + } + return assetQuery; + } + + private static AttributePredicate nonEmptyFcmToken() { + return new AttributePredicate( + ConsoleAsset.CONSOLE_PROVIDERS, + new ValueEmptyPredicate().negate(true), + false, + new NameValuePredicate.Path(PushNotificationMessage.TYPE, "data", "token")); + } + + private static AttributePredicate fcmProviderVersion() { + return new AttributePredicate( + ConsoleAsset.CONSOLE_PROVIDERS, + new StringPredicate(FCM_PROVIDER_NAME), + false, + new NameValuePredicate.Path(PushNotificationMessage.TYPE, "version")); + } + + private List consoleIds(AssetQuery assetQuery) { + return assetStorageService.findAll(assetQuery).stream().map(Asset::getId).distinct().toList(); + } + protected static boolean isLinkedToFcmProvider(ConsoleAsset asset) { return asset .getConsoleProviders() diff --git a/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy b/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy index 6a824f9ceb..49710f9ee0 100644 --- a/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy +++ b/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy @@ -414,6 +414,34 @@ class NotificationTest extends Specification implements ManagerContainerTrait { assert !notificationTargetIds.contains(testuser3Console1.id) } + when: "all linked consoles for a user lack an FCM token" + notificationIds.clear() + notificationTargetIds.clear() + def testUser3Console2Asset = assetStorageService.find(testuser3Console2.id) as ConsoleAsset + testUser3Console2Asset.getConsoleProviders().map { + it.get(PushNotificationMessage.TYPE) + }.get().getData().put("token", null) + assetStorageService.merge(testUser3Console2Asset) + conditions.eventually { + assert pushNotificationHandler.consoleFCMTokenMap.get(testUser3Console2Asset.id) == null + } + notification.targets = [new Notification.Target(Notification.TargetType.USER, keycloakTestSetup.testuser3Id)] + advancePseudoClock(1, TimeUnit.HOURS, container) + adminNotificationResource.sendNotification(null, notification) + + then: "sent notifications record the missing FCM token instead of missing targets" + ex = thrown() + ex.response.withCloseable { r -> + assert r.status == 400 + return true + } + conditions.eventually { + def console1Notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) + def console2Notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console2.id, null, null, null, null, null) + assert console1Notifications.any { it.error?.contains("No FCM token found for console") } + assert console2Notifications.any { it.error?.contains("No FCM token found for console") } + } + when: "a notification handler throws an exception" throwPushHandlerException = true advancePseudoClock(1, TimeUnit.HOURS, container) @@ -441,7 +469,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { conditions.eventually { assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser2Console.id, null, null, null, null, null).length == 2 notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) - assert notifications.length == 5 + assert notifications.length == 7 assert notifications.every {n -> PushNotificationMessage pushMessage = n.message as PushNotificationMessage pushMessage.getTitle() == "Test Action" && @@ -450,7 +478,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { n.deliveredOn == null && n.acknowledgedOn == null } - assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console2.id, null, null, null, null, null).length == 7 + assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console2.id, null, null, null, null, null).length == 8 assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, anonymousConsole.id, null, null, null, null, null).length == 3 } @@ -462,7 +490,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { then: "the notification should have been updated" conditions.eventually { notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) - assert notifications.length == 5 + assert notifications.length == 7 assert notifications.count {n -> n.deliveredOn != null} == 1 } @@ -474,7 +502,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { then: "the notification should have been updated" conditions.eventually { notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) - assert notifications.length == 5 + assert notifications.length == 7 assert notifications.count {n -> n.deliveredOn != null && n.acknowledgedOn != null && n.acknowledgement == "\"dismissed\"" } == 1 @@ -591,6 +619,19 @@ class NotificationTest extends Specification implements ManagerContainerTrait { when: "a repeat frequency is set and a notification is sent" // Clear exception flag throwPushHandlerException = false + // The missing-token scenario cleared both consoles. Later sends still need a token. + def fcmToken = "23123213ad2313b0897efd" + [testuser3Console1, testuser3Console2].each { console -> + def asset = assetStorageService.find(console.id) as ConsoleAsset + asset.getConsoleProviders().map { + it.get(PushNotificationMessage.TYPE) + }.get().getData().put("token", fcmToken) + assetStorageService.merge(asset) + } + conditions.eventually { + assert pushNotificationHandler.consoleFCMTokenMap.get(testuser3Console1.id) == fcmToken + assert pushNotificationHandler.consoleFCMTokenMap.get(testuser3Console2.id) == fcmToken + } // Move clock to beginning of next hour def advancement = Instant.ofEpochMilli(getClockTimeOf(container)) .truncatedTo(ChronoUnit.HOURS)