Repository navigation
Conversation
wborn
left a comment
There was a problem hiding this comment.
This is an initial AI-assisted review.
The scope validation and expanded integration tests provide good coverage of the intended behavior.
One concern remains: synchronous Keycloak role lookups during automatic alarm notification processing introduce an external dependency that can cause alarm creation to fail after the alarm has already been persisted. This should be addressed before merging.
| // Both notification channels include all linked assets, so partial access is insufficient. | ||
| users.removeIf( | ||
| user -> | ||
| Arrays.asList(identityProvider.getUserRealmRoles(alarm.getRealm(), user.getId())) |
There was a problem hiding this comment.
This introduces a synchronous Keycloak Admin API request for every potential notification recipient. These requests are serialized by KeycloakIdentityProvider.getRealms(), and a Keycloak failure can cause sendAlarm() to fail after the alarm has already been persisted.
Could recipient filtering avoid these synchronous external requests so that notification processing cannot cause an otherwise successful alarm operation to fail?
|



Alarm rules could create or extend alarms outside their asset scope and assign users without validating access. Global rules also failed because creation and linking required an engine realm.
This PR adds validation before alarms or links are persisted:
Part of the rules facade scoping review tracked in #2675.