Skip to content

Implement DocRequest on notification config and alerting request classes#980

Merged
cwperks merged 3 commits into
opensearch-project:mainfrom
DarshitChanpura:notifications-doc-request
Jul 21, 2026
Merged

Implement DocRequest on notification config and alerting request classes#980
cwperks merged 3 commits into
opensearch-project:mainfrom
DarshitChanpura:notifications-doc-request

Conversation

@DarshitChanpura

@DarshitChanpura DarshitChanpura commented Jul 21, 2026

Copy link
Copy Markdown
Member

Description

Implements DocRequest on notification-config and alerting request classes so the security plugin's ResourceAccessEvaluator can automatically intercept and authorize direct-access operations (GET, UPDATE, DELETE, ACK) on protected resources at the transport layer.

Companion PRs:

Notifications changes

  • GetNotificationConfigRequest — returns id() for single-ID requests, null for search/multi-ID (DLS handles those).
  • UpdateNotificationConfigRequest — returns configId.
  • DeleteNotificationConfigRequest — returns id() for single-ID deletes, null for multi-ID.
  • CreateNotificationConfigRequest — returns configId (may be null for auto-generated IDs).
  • Added CONFIG_INDEX_NAME constant to NotificationConstants.

Alerting changes

Adds DocRequest to the alerting request classes whose transport actions target a registered resource (`monitor`) or a subordinate of one (alerts/findings/comments).

Primary resource operations — index/id point at the resource itself:

  • `GetMonitorRequest`, `IndexMonitorRequest`, `DeleteMonitorRequest` → `SCHEDULED_JOBS_INDEX` / monitor id
  • `GetWorkflowRequest`, `IndexWorkflowRequest`, `DeleteWorkflowRequest` → `SCHEDULED_JOBS_INDEX` / workflow id
  • `DocLevelMonitorFanOutRequest` → `SCHEDULED_JOBS_INDEX` / monitor id

Subordinate operations — index/id point at the parent resource so the ActionFilter gates access transitively:

  • `AcknowledgeAlertRequest` → `SCHEDULED_JOBS_INDEX` / monitor id
  • `AcknowledgeChainedAlertRequest` → `SCHEDULED_JOBS_INDEX` / workflow id
  • `GetAlertsRequest`, `GetFindingsRequest` → `SCHEDULED_JOBS_INDEX` / single monitor id (else `null`, subordinate DLS handles the rest)
  • `GetWorkflowAlertsRequest` → `SCHEDULED_JOBS_INDEX` / single workflow id (else `null`)

Comment operations — index/id point at the comment itself; alerting still gates transitively via the underlying alert fetch:

  • `IndexCommentRequest`, `DeleteCommentRequest` → `ALL_COMMENTS_INDEX_PATTERN` / comment id

Also introduces `AlertingConstants` with `ALL_ALERT_INDEX_PATTERN` and `ALL_COMMENTS_INDEX_PATTERN` for shared reuse.

Related Issues

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 260fa4f)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Implement DocRequest on notification config request classes

Relevant files:

  • src/main/kotlin/org/opensearch/commons/notifications/NotificationConstants.kt
  • src/main/kotlin/org/opensearch/commons/notifications/action/DeleteNotificationConfigRequest.kt
  • src/main/kotlin/org/opensearch/commons/notifications/action/GetNotificationConfigRequest.kt
  • src/main/kotlin/org/opensearch/commons/notifications/action/UpdateNotificationConfigRequest.kt

Sub-PR theme: Implement DocRequest on alerting monitor/workflow/alert request classes

Relevant files:

  • src/main/kotlin/org/opensearch/commons/alerting/action/AcknowledgeAlertRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/AcknowledgeChainedAlertRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/DeleteMonitorRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/DeleteWorkflowRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/GetAlertsRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/GetFindingsRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/GetMonitorRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/GetWorkflowAlertsRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/GetWorkflowRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/IndexMonitorRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/IndexWorkflowRequest.kt

Sub-PR theme: Implement DocRequest on alerting comment request classes and add AlertingConstants

Relevant files:

  • src/main/kotlin/org/opensearch/commons/alerting/action/DeleteCommentRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt
  • src/main/kotlin/org/opensearch/commons/alerting/util/AlertingConstants.kt

⚡ Recommended focus areas for review

Possible Issue

id() returns commentId.ifBlank { null } for IndexCommentRequest. Per the class docs, commentId is blank for create operations and only populated for updates. However, this same request handles both create and update; when creating a new comment, the security evaluator will receive a null id and may treat it as a bulk/search operation. Confirm this is the intended fallback path (i.e., that null correctly signals "no existing resource to authorize" for create) rather than bypassing authorization unintentionally.

override fun id(): String? {
    // For updates, the target is the existing comment; for creates, no comment id exists yet.
    return commentId.ifBlank { null }
}
Inconsistent id() Logic

GetWorkflowAlertsRequest.id() returns only workflowIds?.singleOrNull() and ignores monitorIds, while the analogous GetAlertsRequest and GetFindingsRequest fall back to monitorId ?: monitorIds?.singleOrNull(). If a caller queries workflow alerts by a single monitorId (with no workflowId), authorization will fall through to search-level DLS instead of being gated on the specific monitor. Verify this asymmetry is intentional.

override fun id(): String? {
    // Access is gated on the workflow when a single workflowId is provided; otherwise fall back to search-level DLS.
    return workflowIds?.singleOrNull()
}

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 260fa4f

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Include single monitorId fallback for id

The returned id here is a workflowId, but the declared index is SCHEDULED_JOBS_INDEX
which is fine, however the request is fetching alerts. More importantly, this
ignores the case where alerts are being queried by monitorIds too. Consider also
returning monitorIds?.singleOrNull() as a fallback, mirroring GetAlertsRequest, so
single-monitor lookups can be authorized similarly.

src/main/kotlin/org/opensearch/commons/alerting/action/GetWorkflowAlertsRequest.kt [79-82]

 override fun id(): String? {
-    // Access is gated on the workflow when a single workflowId is provided; otherwise fall back to search-level DLS.
-    return workflowIds?.singleOrNull()
+    // Access is gated on the workflow/monitor when a single id is provided; otherwise fall back to search-level DLS.
+    return workflowIds?.singleOrNull() ?: monitorIds?.singleOrNull()
 }
Suggestion importance[1-10]: 5

__

Why: Reasonable suggestion to mirror GetAlertsRequest behavior by including monitorIds?.singleOrNull() as fallback. However, since the index is SCHEDULED_JOBS_INDEX and workflow alerts are workflow-centric, the authorization semantics may differ; the improvement is plausible but not clearly critical.

Low
Fallback to entityId on create

On create requests the target document authorization should be against the parent
entityId (the alert), not the not-yet-existing commentId. Consider returning
entityId when commentId is blank so permission checks reference the parent entity
rather than null.

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt [85-88]

 override fun id(): String? {
-    // For updates, the target is the existing comment; for creates, no comment id exists yet.
-    return commentId.ifBlank { null }
+    // For updates, the target is the existing comment; for creates, authorize against the parent entity.
+    return commentId.ifBlank { entityId.ifBlank { null } }
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about create-time authorization, but the declared index() returns the comments index, so referencing an alert id (from another index) may not be semantically correct either. The improvement is debatable.

Low

Previous suggestions

Suggestions up to commit 6d67985
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid runtime TODO in overridden methods

The index() and id() methods throw NotImplementedError at runtime via TODO(), which
will cause any consumer of DocRequest (e.g., security plugin) to crash when handling
this request. Return proper values (e.g., the comments index pattern and commentId
or entityId) or a safe null fallback instead of leaving TODO.

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt [80-87]

-// TODO maybe this is not needed
 override fun index(): String? {
-    TODO("Not yet implemented")
+    return org.opensearch.commons.alerting.util.AlertingConstants.ALL_COMMENTS_INDEX_PATTERN
 }
 
 override fun id(): String? {
-    TODO("Not yet implemented")
+    return commentId.ifBlank { null }
 }
Suggestion importance[1-10]: 9

__

Why: The TODO() calls will throw NotImplementedError at runtime whenever index() or id() is invoked, which is a real crash risk. Providing a safe implementation is a critical correctness fix.

High
Return the correct index for monitor deletion

DeleteMonitorRequest targets a monitor document in the scheduled jobs index, not an
alert index. Returning ALL_ALERT_INDEX_PATTERN will mislead permission checks and
DLS filtering. It should return ScheduledJob.SCHEDULED_JOBS_INDEX instead, matching
other monitor-related requests.

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteMonitorRequest.kt [38-44]

 override fun index(): String? {
-    return ALL_ALERT_INDEX_PATTERN
+    return ScheduledJob.SCHEDULED_JOBS_INDEX
 }
 
 override fun id(): String? {
     return monitorId
 }
Suggestion importance[1-10]: 8

__

Why: Monitor documents live in the scheduled jobs index, so returning the alert index pattern misidentifies the resource for security/DLS. This aligns with other monitor requests in the PR.

Medium
Return correct index for workflow deletion

A workflow document lives in the scheduled jobs index, not in the alerts index
pattern. Returning ALL_ALERT_INDEX_PATTERN misidentifies the resource for
authorization purposes; use ScheduledJob.SCHEDULED_JOBS_INDEX to be consistent with
other workflow requests.

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteWorkflowRequest.kt [42-48]

 override fun index(): String? {
-    return ALL_ALERT_INDEX_PATTERN
+    return ScheduledJob.SCHEDULED_JOBS_INDEX
 }
 
 override fun id(): String? {
     return workflowId
 }
Suggestion importance[1-10]: 8

__

Why: Workflow documents are stored in the scheduled jobs index; returning the alert index pattern misrepresents the resource for authorization checks. Consistent with other workflow requests.

Medium
Use scheduled jobs index for monitor fan-out

The fan-out request operates against a monitor definition (which is stored in the
scheduled jobs index), not the alerts index. Returning ALL_ALERT_INDEX_PATTERN will
result in incorrect resource identification for security/DLS checks; use
ScheduledJob.SCHEDULED_JOBS_INDEX instead.

src/main/kotlin/org/opensearch/commons/alerting/action/DocLevelMonitorFanOutRequest.kt [104-110]

 override fun index(): String? {
-    return ALL_ALERT_INDEX_PATTERN
+    return ScheduledJob.SCHEDULED_JOBS_INDEX
 }
 
 override fun id(): String? {
     return monitor.id
 }
Suggestion importance[1-10]: 7

__

Why: The fan-out request operates on a monitor definition; using the alerts index pattern is inconsistent with other monitor requests and may impact security identification, though this is an internal request.

Medium
Suggestions up to commit 6a86018
CategorySuggestion                                                                                                                                    Impact
Security
Handle multi-ID case for DocRequest id

Returning null from id() when multiple config IDs are present may cause
resource-based authorization/permission checks to be bypassed or misapplied, since
callers cannot enforce per-document authorization on a multi-doc delete. Consider
either enforcing a single-ID contract via validation, or returning identifiers that
let downstream authorization iterate over all target documents.

src/main/kotlin/org/opensearch/commons/notifications/action/DeleteNotificationConfigRequest.kt [120-122]

+override fun id(): String? {
+    return if (configIds.size == 1) configIds.first() else null
+}
 
-
Suggestion importance[1-10]: 2

__

Why: The suggestion raises a potentially valid concern about authorization semantics for multi-ID delete requests, but the improved_code is identical to existing_code, providing no actionable fix. It reads more as a comment/question than a concrete improvement.

Low
General
Clarify id semantics for empty/multi requests

configIds can be empty (the constructor allows it and only validate() checks it
later), so configIds.first() is only reached when size == 1, but returning null for
the empty/search-all case may lead consumers of DocRequest to treat the request as
unresourced. Confirm this is the intended semantic, or return a sentinel/throw so
callers don't inadvertently skip authorization on a "list all" query.

src/main/kotlin/org/opensearch/commons/notifications/action/GetNotificationConfigRequest.kt [179-181]

+override fun id(): String? {
+    return if (configIds.size == 1) configIds.first() else null
+}
 
-
Suggestion importance[1-10]: 2

__

Why: Similar to the first suggestion, the improved_code is identical to existing_code, offering no concrete change. It only asks to confirm intended semantics, which limits its impact.

Low
Suggestions up to commit b1d4440
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid runtime crash from TODO overrides

The index() and id() overrides throw NotImplementedError at runtime via TODO(),
which will crash any caller that invokes them (including security/permission checks
that rely on DocRequest). Return concrete values (e.g. the comments index pattern
and commentId) or return null instead of throwing.

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt [80-87]

-// TODO maybe this is not needed
 override fun index(): String? {
-    TODO("Not yet implemented")
+    return AlertingConstants.ALL_COMMENTS_INDEX_PATTERN
 }
 
 override fun id(): String? {
-    TODO("Not yet implemented")
+    return commentId.ifBlank { null }
 }
Suggestion importance[1-10]: 9

__

Why: The TODO() calls throw NotImplementedError at runtime, which would crash any consumer invoking these overrides for authorization checks. Returning concrete values is essential.

High
Return correct index for monitor deletion

DeleteMonitorRequest targets the scheduled jobs index (where monitors are stored),
not the alert index. Returning ALL_ALERT_INDEX_PATTERN here will cause permission
checks to be evaluated against the wrong index, mismatching every other
monitor-scoped request in this PR (e.g. IndexMonitorRequest, GetMonitorRequest).

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteMonitorRequest.kt [38-44]

 override fun index(): String? {
-    return ALL_ALERT_INDEX_PATTERN
+    return ScheduledJob.SCHEDULED_JOBS_INDEX
 }
 
 override fun id(): String? {
     return monitorId
 }
Suggestion importance[1-10]: 8

__

Why: Monitors are stored in the scheduled jobs index, so returning ALL_ALERT_INDEX_PATTERN misroutes permission checks and is inconsistent with other monitor requests. This looks like a genuine bug.

Medium
Return correct index for workflow deletion

Workflows are stored in the scheduled jobs index, not the alert index. Using
ALL_ALERT_INDEX_PATTERN will misroute authorization/DLS checks for workflow
deletion. Align with GetWorkflowRequest/IndexWorkflowRequest which correctly return
ScheduledJob.SCHEDULED_JOBS_INDEX.

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteWorkflowRequest.kt [42-48]

 override fun index(): String? {
-    return ALL_ALERT_INDEX_PATTERN
+    return ScheduledJob.SCHEDULED_JOBS_INDEX
 }
 
 override fun id(): String? {
     return workflowId
 }
Suggestion importance[1-10]: 8

__

Why: Workflows live in the scheduled jobs index; using the alert index pattern is inconsistent with IndexWorkflowRequest/GetWorkflowRequest and would cause incorrect authorization routing.

Medium
General
Use object and add trailing newline

The file is missing a trailing newline and defines a class with only a companion
object. Prefer a Kotlin object for a constants container and add the terminating
newline to satisfy standard style/lint checks.

src/main/kotlin/org/opensearch/commons/alerting/util/AlertingConstants.kt [3-9]

-class AlertingConstants {
-    companion object {
-        const val ALL_ALERT_INDEX_PATTERN = ".opendistro-alerting-alert*"
+object AlertingConstants {
+    const val ALL_ALERT_INDEX_PATTERN = ".opendistro-alerting-alert*"
 
-        const val ALL_COMMENTS_INDEX_PATTERN = ".opensearch-alerting-comments*"
-    }
+    const val ALL_COMMENTS_INDEX_PATTERN = ".opensearch-alerting-comments*"
 }
Suggestion importance[1-10]: 3

__

Why: Converting to an object is a minor stylistic improvement, but changing it would require updating all import sites that use AlertingConstants.Companion.*. Low impact.

Low
Suggestions up to commit 2876330
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid throwing from DocRequest overrides

The index() and id() methods throw NotImplementedError at runtime via TODO(), which
will break any caller (e.g., security plugin resolving DocRequest) that invokes
these methods on an IndexCommentRequest. Return proper values (the comments index
pattern and commentId, or null when creating a new comment) instead of throwing.

src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt [80-87]

-// TODO maybe this is not needed
 override fun index(): String? {
-    TODO("Not yet implemented")
+    return ALL_COMMENTS_INDEX_PATTERN
 }
 
 override fun id(): String? {
-    TODO("Not yet implemented")
+    return commentId.ifBlank { null }
 }
Suggestion importance[1-10]: 9

__

Why: The TODO() calls will throw NotImplementedError at runtime when the security plugin invokes these methods, which is a critical bug. Providing sensible defaults is essential.

High
Return the correct index for deletes

DeleteMonitorRequest targets a monitor document, not an alert document, so the
returned index should be the scheduled-jobs index
(ScheduledJob.SCHEDULED_JOBS_INDEX) rather than the alerts index pattern. Otherwise
permission checks will be evaluated against the wrong index. The same concern
applies to DeleteWorkflowRequest.

src/main/kotlin/org/opensearch/commons/alerting/action/DeleteMonitorRequest.kt [38-44]

 override fun index(): String? {
-    return ALL_ALERT_INDEX_PATTERN
+    return ScheduledJob.SCHEDULED_JOBS_INDEX
 }
 
 override fun id(): String? {
     return monitorId
 }
Suggestion importance[1-10]: 8

__

Why: DeleteMonitorRequest deletes a monitor document stored in the scheduled-jobs index, so returning the alert index pattern would misdirect permission checks. This is a correctness issue.

Medium
General
Simplify constants holder and add newline

The file has no trailing newline and uses a class with a companion object purely to
hold constants; converting to a Kotlin object (or top-level const vals) is idiomatic
and avoids the awkward AlertingConstants.Companion.X access from Java callers. Also
add a final newline to comply with typical style checks.

src/main/kotlin/org/opensearch/commons/alerting/util/AlertingConstants.kt [3-9]

-class AlertingConstants {
-    companion object {
-        const val ALL_ALERT_INDEX_PATTERN = ".opendistro-alerting-alert*"
+package org.opensearch.commons.alerting.util
 
-        const val ALL_COMMENTS_INDEX_PATTERN = ".opensearch-alerting-comments*"
-    }
+object AlertingConstants {
+    const val ALL_ALERT_INDEX_PATTERN = ".opendistro-alerting-alert*"
+    const val ALL_COMMENTS_INDEX_PATTERN = ".opensearch-alerting-comments*"
 }
Suggestion importance[1-10]: 3

__

Why: Converting to a Kotlin object is more idiomatic but the change is stylistic and does not affect functionality; the callers already use Companion accessors.

Low
Verify id/index pairing resolves correctly

The docs mention gating by workflow, but the index returned is SCHEDULED_JOBS_INDEX
and the id() is a workflow id — this is consistent, however
GetAlertsRequest/GetFindingsRequest prefer monitorId over the list. Consider also
returning a single alertIds-derived id only when appropriate; more importantly
ensure that returning a workflow id against the scheduled-jobs index is intentional
for permission resolution (workflows are stored there). If not, the id/index pair
will not resolve to a real document.

src/main/kotlin/org/opensearch/commons/alerting/action/GetWorkflowAlertsRequest.kt [79-82]

+override fun id(): String? {
+    // Access is gated on the workflow when a single workflowId is provided; otherwise fall back to search-level DLS.
+    return workflowIds?.singleOrNull()
+}
 
-
Suggestion importance[1-10]: 2

__

Why: The suggestion merely asks to verify correctness without proposing an actual change (improved_code equals existing_code), providing limited value.

Low
Suggestions up to commit 9de6440
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against null set access

configIds may be null for a GetNotificationConfigRequest (e.g., when listing all
configs with filters). Calling .size on a nullable set will throw a
NullPointerException. Use a safe call to handle the null case.

src/main/kotlin/org/opensearch/commons/notifications/action/GetNotificationConfigRequest.kt [178-180]

 override fun id(): String? {
-    return if (configIds.size == 1) configIds.first() else null
+    return if (configIds?.size == 1) configIds.first() else null
 }
Suggestion importance[1-10]: 2

__

Why: The configIds field is declared as Set<String> (non-nullable) based on the diff context, so calling .size should not throw a NullPointerException. The suggestion appears to be based on an incorrect assumption about nullability.

Low

Enables the security plugin's ResourceAccessEvaluator to automatically
intercept and authorize GET, UPDATE, DELETE operations on protected
notification_config resources at the transport layer.

- GetNotificationConfigRequest: returns id() for single-ID requests,
  null for search/multi-ID (DLS handles those)
- UpdateNotificationConfigRequest: returns configId
- DeleteNotificationConfigRequest: returns id() for single-ID deletes,
  null for multi-ID
- Added CONFIG_INDEX_NAME constant to NotificationConstants

Note: CreateNotificationConfigRequest is NOT marked as DocRequest since
create operations don't access existing resources — the security plugin's
ResourceIndexListener handles ownership registration automatically on
index write.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
DarshitChanpura force-pushed the notifications-doc-request branch from 3785c80 to 9de6440 Compare July 21, 2026 04:17
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9de6440

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b1d4440.

PathLineSeverityDescription
src/main/kotlin/org/opensearch/commons/alerting/action/IndexCommentRequest.kt79lowBoth index() and id() methods contain TODO("Not yet implemented") which throw NotImplementedError at runtime. If the DocRequest interface is exercised by the security plugin for this class, it will cause an unhandled exception. The inline comment acknowledges uncertainty about whether these methods are needed, suggesting incomplete implementation was shipped.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2876330

@DarshitChanpura DarshitChanpura changed the title Implement DocRequest on notification config request classes Implement DocRequest on notification config and alerting request classes Jul 21, 2026
@DarshitChanpura
DarshitChanpura force-pushed the notifications-doc-request branch from 2876330 to b1d4440 Compare July 21, 2026 04:53
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b1d4440

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6a86018

Enables the security plugin's ResourceAccessEvaluator to automatically
intercept and authorize operations on protected resources at the
transport layer.

Notifications:
- GetNotificationConfigRequest: implements DocRequest with id() for
  single-ID requests, null for search/multi-ID (DLS handles those)
- UpdateNotificationConfigRequest: implements DocRequest with configId
- DeleteNotificationConfigRequest: implements DocRequest with id() for
  single-ID deletes, null for multi-ID
- All override type() to return "notification_config"
- Added CONFIG_INDEX_NAME and CONFIG_RESOURCE_TYPE constants

Alerting:
- DocRequest implemented on Get, Delete, Index, and Acknowledge request
  classes for monitors, workflows, comments, alerts, and findings

Note: Create requests that generate new resources (without accessing
existing ones) do NOT implement DocRequest.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@DarshitChanpura
DarshitChanpura force-pushed the notifications-doc-request branch from 6a86018 to 6d67985 Compare July 21, 2026 05:14
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6d67985

- DeleteMonitorRequest, DeleteWorkflowRequest, DocLevelMonitorFanOutRequest:
  point at SCHEDULED_JOBS_INDEX (where monitors/workflows live) rather
  than the alerts index pattern. The previous mapping would make the
  security plugin's ActionFilter look up the resource in the wrong index
  and fail to gate access.
- IndexCommentRequest: replace TODO stubs with the comments index and
  the (possibly-blank) commentId, matching DeleteCommentRequest.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 260fa4f

@cwperks
cwperks merged commit 3946eb8 into opensearch-project:main Jul 21, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants