Skip to content

Enforce realm and asset scoping for rule alarms - #3459

Merged
ebariaux merged 20 commits into
mainfrom
bugfix/rules-alarm-facade-scoping
Oct 30, 2026
Merged

ebariaux merged 20 commits into
mainfrom
bugfix/rules-alarm-facade-scoping

Conversation

@ebariaux

@ebariaux ebariaux commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • Asset-scoped rules may target only the scoped asset or its descendants. Creation requires targets; linking requires existing links, with every existing and requested asset in scope.
  • Assignees remain optional and must belong to the alarm’s realm. Restricted assignees need explicit links to every target; unrestricted assignees need no asset links.
  • Global creation uses the supplied realm or infers it from targets, validating all targets against that realm. Global linking uses the existing alarm’s realm. Reusable global alarm definitions are preserved.
  • Automatic notifications for unassigned HIGH alarms exclude restricted users without access to every linked asset, including alarms without asset links.
  • Rejected requests leave alarms and links unchanged. Existing validation errors, duplicate-link handling, and link timestamps are preserved.

Part of the rules facade scoping review tracked in #2675.

@ebariaux ebariaux added the Bug Error or issue in the system label Oct 20, 2026
@ebariaux
ebariaux marked this pull request as ready for review October 21, 2026 08:47
@ebariaux
ebariaux requested a review from a team October 21, 2026 08:48
wborn
wborn previously requested changes Oct 27, 2026

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@ebariaux
ebariaux requested a review from wborn October 29, 2026 09:46

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous review feedback. The database-backed notification recipient queries are a good improvement, avoiding repeated Keycloak Admin API calls.

The realm and asset scoping validation looks sound, including restricted assignee handling, validation of existing asset links, and global ruleset support. The additional tests also provide good coverage.

The composite-role limitation of UserQuery is now tracked in #3524. I've left one non-blocking suggestion for extending that issue to cover a future improvement to assignee validation.

No further blocking issues found.

This review was AI-assisted.

@wborn
wborn dismissed their stale review October 30, 2026 15:04

AI found no further blocking issues.

@ebariaux
ebariaux enabled auto-merge October 30, 2026 15:44
@sonarqubecloud

Copy link
Copy Markdown

@ebariaux
ebariaux added this pull request to the merge queue Oct 30, 2026
Merged via the queue into main with commit 4ef7b99 Oct 30, 2026
24 checks passed
@ebariaux
ebariaux deleted the bugfix/rules-alarm-facade-scoping branch October 30, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Error or issue in the system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants