Onboards notifications plugin to centralized resource authz#1237
Conversation
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…ncipals - Use adminClient() (cert-based) for security REST API calls and _cat/indices in tests, since resource sharing restricts the password-based admin user - Add all_shared_principals field to notifications-config-mapping (schema v3) for resource sharing framework queries - Add protected_types setting for notification_config in test cluster Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Aligns with the security plugin rename (opensearch-project/security#6018). Also marks notifications_read_only as the default access level for migration API usage. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Add resolution strategy for tools.jackson dependencies to resolve conflict between security plugin (3.2.1) and compileOnly (3.2.0) - Update test assertions from schema_version 2 to 3 Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Use single version (versions.jackson3) for all tools.jackson modules to resolve SNAPSHOT drift between opensearch-core and remote-metadata-sdk - Keep security plugin zip conditional on securityEnabled since it's only needed for integration test clusters Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
These tests exercise the legacy RBAC backend-roles filtering path which is intentionally bypassed when resource sharing is enabled. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Only run ResourceSharingNotificationIT when resource sharing is enabled; exclude all other ITs since they use the admin user which cannot access protected resource types - Use addPatchUserRolesMapping instead of createUserRolesMapping to avoid overwriting the all_access role mapping Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Add leading slash to path field per RFC 6902 ("/users" not "users").
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Add verifyResourceAccess() to UserAccessManager that calls ResourceSharingClient.verifyAccess() for GET, UPDATE, DELETE ops - Add suspendUntilCallback utility for bridging ActionListener to coroutines - Use adminClient() for refreshAllIndices since protected resource indices restrict the admin user - Fix ResourceSharingNotificationIT to use proper non-admin roles and verify access control enforcement Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
| */ | ||
| suspend fun create(request: CreateNotificationConfigRequest, user: User?): CreateNotificationConfigResponse { | ||
| log.info("$LOG_PREFIX:NotificationConfig-create") | ||
| userAccess.validateUser(user) |
There was a problem hiding this comment.
do we have a replacement for create?
There was a problem hiding this comment.
No explicit replacement needed for create. The security plugin's ResourceIndexListener.postIndex() automatically intercepts the index write and registers the resource with the creator's identity. So when a user creates a config, the security plugin sees the index event and creates the corresponding sharing record — no plugin-side code required for this path.
| return when (request.configIds.size) { | ||
| 0 -> getAll(request, user) | ||
| 1 -> info(request.configIds.first(), user) | ||
| else -> info(request.configIds, user) |
There was a problem hiding this comment.
How about getAll() and info with multiple configIds
There was a problem hiding this comment.
For single-ID info() and single-ID delete(): the security plugin's ResourceAccessEvaluator handles it automatically at the transport layer once the request class implements DocRequest (companion PR: opensearch-project/common-utils#980).
For getAll() (search): the security plugin applies a DLS filter on all_shared_principals to restrict search results to resources the user owns or has been shared with. No plugin-side filtering needed.
For multi-ID cases: DocRequest.id() returns null when there are multiple IDs, which skips the transport-level single-resource check. The security plugin will still apply DLS on any search-based path. For direct multi-get, we'd need to either iterate with individual checks or rely on the plugin-level doesUserHaveAccess (which currently passes through when resource sharing is enabled). This is an area we can tighten in a follow-up if needed.
There was a problem hiding this comment.
Addressed in the latest push:
- Single-ID GET/DELETE: handled by security plugin's
ResourceAccessEvaluatorat the transport layer viaDocRequest(companion PR: Implement DocRequest on notification config and alerting request classes common-utils#980) - Multi-ID GET/DELETE:
DocRequest.id()returnsnull, so transport-level check is skipped. Added explicitverifyResourceAccess()calls per-ID for these paths. - getAll() (search): DLS filter on
all_shared_principalshandles this automatically — no plugin-side code needed.
…cess for multi-ID ops - Extract RESOURCE_TYPE constant in NotificationsResourceSharingExtension and reference it from UserAccessManager - Add verifyResourceAccess for multi-ID info() and multi-ID delete() - For getAll(): DLS filter applied by security plugin handles search filtering automatically - For create: security plugin's ResourceIndexListener.postIndex() registers resource ownership automatically on index write Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…est-based authz Direct resource access control is handled automatically by the security plugin's ResourceAccessEvaluator when request classes implement DocRequest. The verifyAccess client method is only needed for transitive access checks. Request classes in common-utils need to implement DocRequest as a follow-up to enable transport-level enforcement. Simplified integration tests to validate framework registration and basic CRUD with resource sharing enabled. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Add verifyResourceAccess calls for multi-ID info() and delete() since DocRequest.id() returns null for these (transport-level check skipped) - Update tests to exercise security plugin's transport-level interception: owner access, non-owner denial, and share-then-access flow - Use adminClient() for refreshAllIndices (protected index restriction) Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
CI will fail until opensearch-project/common-utils#980 is merged and a new artifact is available. |
Covers the full access level hierarchy with extensive assertions: - Owner access: create and retrieve own resources - Unshared: full cluster role is insufficient without resource-level access - read_only: can GET; cannot UPDATE, DELETE, or SHARE - read_write: can GET, UPDATE, DELETE; cannot SHARE; owner sees updates - full_access: can GET, UPDATE, DELETE, SHARE; share chain works All users have identical cluster-level permissions to prove that resource-level access control (via resource-access-levels.yml) is the enforcing layer. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Tests cover the full access level hierarchy: - Owner: can GET, UPDATE, SHARE, DELETE own resources - Non-owner (no sharing): cannot GET, UPDATE, DELETE, or SHARE even with full cluster-level permissions - read_only level: can GET; cannot UPDATE, DELETE, SHARE; config unchanged after failed update attempt - read_write level: can GET, UPDATE, DELETE; cannot SHARE; owner sees updates made by shared user; non-owner can delete - full_access level: can GET, UPDATE, DELETE, SHARE; share chain works (Alice->Bob->Charlie); Charlie at read_only cannot delete TODOs for follow-up (require PluginClient integration): - List isolation test (DLS-based search filtering) - Revoke access test (revoke API contract) Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Implement PluginClient (FilterClient that runs as plugin's subject) for DLS-based search filtering in future - NotificationPlugin now implements IdentityAwarePlugin and receives PluginSubject for the PluginClient - Add revoke access test: verifies PATCH /_plugins/_security/api/resource/share with "revoke" field removes previously granted access - 6 comprehensive tests now pass: owner CRUD, non-owner denial, read_only, read_write, full_access, and revoke TODO: Wire PluginClient into search path for DLS-based list isolation Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
- Create a separate SdkClient backed by PluginClient for search operations, so the security plugin sees the user identity and applies DLS filtering on all_shared_principals - NotificationConfigIndex.getAllNotificationConfigs now uses searchSdkClient which preserves user context for DLS - Add list isolation test: users only see their own configs and configs explicitly shared with them All 7 resource sharing integration tests pass: - Owner CRUD + share - Non-owner denial - read_only access (GET only) - read_write access (GET/UPDATE/DELETE, no share) - full_access (all ops + share chain) - Revoke access - List isolation via DLS Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Description
Implements resource-access-control for notification_config.
Related Issues
Resolves #1245
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.