Workspace aware sharing records

[DarshitChanpura]

Description

Category: Enhancement

Makes the resource-sharing framework workspace-aware, so a resource's visibility and access can be driven by the workspaces it belongs to. This is the compatibility prerequisite for graduating resource sharing (RP) out of experimental — workspaces converge onto the RP ownership model rather than the reverse.

Design doc (grounded in a full trace of the shipped code): https://pippin.amazon.dev/architect/zVSJIXo0GB4A?artifact=94G3FCSuz7dt

What changed

• Model: ResourceSharing gains an optional, set-valued workspaces field (a resource may belong to multiple workspaces). Threaded through the builder, XContent (omitted when empty → byte-identical for existing records), and transport serialization. Also adds the previously-missing StreamInput reader + NamedWriteable registration so ShareResponse round-trips.
• Read path (DLS): introduces a workspace:<id> principal namespace. Workspace IDs on a resource are denormalized into all_shared_principals; a user's accessible workspaces are added to the DLS terms filter — so visibility via workspace membership works through the existing intersection with no new query shape.
• Write path: generalizes the single-parent access recursion in hasPermission into a container fan-out (hierarchical parent + workspaces). Workspaces are batched in a single mget (avoids N+1 GETs on the privilege hot path) and evaluated in-memory; an ancestor-scoped cycle guard prevents unbounded recursion.
• Ingestion: ResourceIndexListener reads the multi-valued workspaces field off the indexed doc; migrate API is workspace-aware and adds an idempotent backfill path for already-migrated records (transition support for workspaces that predate RP).
• SPI: new ResourceSharingExtension.resolveWorkspacesForUser(...) extension point for trusted, server-set workspace membership. Default returns empty (safe by default); the contract explicitly requires the source to be non-user-assertable and I/O-free.

Old vs. new behavior: non-workspace resources are unaffected — records and DLS are byte-identical, and the workspace resolver is disabled unless a plugin opts in. New behavior only activates for workspace-associated resources when a trusted resolver is registered.

Issues Resolved

Relates to #6348 (graduate resource sharing out of experimental) and #6119 (backend workspace collaborator management). No issue fully closed by this PR — see the open items comment below.

Not a backport.

No new static front-end permissions introduced.

Testing

• Unit tests across ResourceSharing, ResourceAccessHandler, ResourcePluginInfo, ResourceSharingIndexHandler (incl. the mget + backfill paths), and the migrate extraction.
• Integration tests on live clusters in sample-resource-plugin: migrate workspace-backfill (idempotent) and live-index ingestion (which empirically confirms the workspace field is read at index time on the default mapping).
• Full resources unit package green; Codecov patch passing.

Check List

• New functionality includes testing
• New functionality has been documented (design doc linked)
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• 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]

PR Code Analyzer ❗

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

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
sample-resource-plugin/src/main/java/org/opensearch/sample/resource/actions/rest/create/CreateResourceRestAction.java	99	low	Workspace IDs are accepted directly from user-supplied request body without validation or membership verification. A user could tag a newly created resource with an arbitrary workspace ID (including workspaces they are not members of), causing the resource to appear in that workspace's DLS all_shared_principals and become visible to all members of that workspace. While no privilege escalation to the creator occurs, it may allow unsolicited resource injection into workspaces. Whether creation-time workspace tagging is intentionally unrestricted warrants explicit design review.

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]

PR Reviewer Guide 🔍

(Review updated until commit 31fde47)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: SPI + core ResourceSharing workspace field additions Relevant files: spi/src/main/java/org/opensearch/security/spi/resources/ResourceProvider.java spi/src/main/java/org/opensearch/security/spi/resources/ResourceSharingExtension.java src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java Sub-PR theme: Migration path backfill of workspaces on existing records Relevant files: src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java src/test/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiActionTests.java src/test/java/org/opensearch/security/resources/ResourceSharingIndexHandlerTests.java Sub-PR theme: Container-inheritance permission checks and DLS workspace principals Relevant files: src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java src/main/java/org/opensearch/security/resources/ResourceSharingDlsUtils.java src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java

⚡ Recommended focus areas for review Wire-format compatibility (ResourceSharing) ResourceSharing#writeTo appends writeOptionalStringCollection(workspaces) and the new StreamInput constructor reads it unconditionally, with no version guard. The code comment states no guard is needed because the resource-sharing feature is not yet GA. However, ResourceSharing implements NamedWriteable and is now registered in getNamedWriteables() — it is sent over the transport (e.g. via ShareResponse#readNamedWriteable(ResourceSharing.class)). In a mixed-version cluster during a rolling upgrade, a new node writing this format to an older peer (or vice versa) will misparse the stream because the older node does not read the trailing writeOptionalStringCollection. If any pre-existing release already serializes ResourceSharing on the wire, this is a hard BWC break: it should be guarded with out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) using the first unreleased version. Please verify whether an older released version can be on the wire; if yes, add symmetric version guards. public void writeTo(StreamOutput out) throws IOException { out.writeString(resourceId); out.writeString(resourceType); out.writeOptionalString(tenant); out.writeOptionalString(parentType); out.writeOptionalString(parentId); createdBy.writeTo(out); if (shareWith != null) { out.writeBoolean(true); shareWith.writeTo(out); } else { out.writeBoolean(false); } // No version guard needed: workspaces ships within the resource-sharing feature (introduced in 3.3), // which is not yet GA, so there is no older node that speaks the old wire format without this field. // The symmetric read lives in the ResourceSharing(StreamInput) constructor, registered as the // resource_sharing NamedWriteable in OpenSearchSecurityPlugin#getNamedWriteables. out.writeOptionalStringCollection(workspaces == null ? null : new ArrayList<>(workspaces)); } Hardcoded workspace type WORKSPACE_RESOURCE_TYPE is hardcoded to the literal string "workspace", and checkContainers relies on resourcePluginInfo.indexByType("workspace") to find the workspace index. The javadoc explicitly notes this is a spike placeholder and the real type is defined by the workspace provider via SPI. If a workspace-owning plugin registers its provider under a different type name, workspace inheritance silently degrades to "no workspace grants access" (safe but incorrect). Consider resolving the workspace type name from the SPI (or a settings/registry lookup) before merging, or document it as a known limitation. /** * SPIKE placeholder for the workspace resource type name. The authoritative value comes from the workspace * provider registered through the resource-sharing SPI (issue #6119). */ private static final String WORKSPACE_RESOURCE_TYPE = "workspace"; Extension resolver invoked without null guard resolveWorkspacesForUser reads user.getSecurityRoles()/user.getRoles() and passes them to every registered extension. The default SPI contract specifies securityRoles and backendRoles are never null, but the sample plugin's SampleResourceExtension#resolveWorkspacesForUser still has if (securityRoles == null || securityRoles.isEmpty()) return Collections.emptySet();, suggesting callers may not always guarantee non-null. That is fine here since we replace null with emptySet(), but any third-party extension that dereferences backendRoles will NPE when user.getRoles() returns null and we've substituted emptySet() — actually safe. Lower-severity: consider documenting that the aggregator normalizes nulls so extension authors can rely on it. public Set<String> resolveWorkspacesForUser(org.opensearch.security.user.User user) { lock.readLock().lock(); try { if (resourceSharingExtensions.isEmpty()) { return java.util.Collections.emptySet(); } Set<String> securityRoles = user.getSecurityRoles() == null ? java.util.Collections.emptySet() : user.getSecurityRoles(); Set<String> backendRoles = user.getRoles() == null ? java.util.Collections.emptySet() : user.getRoles(); Set<String> merged = new HashSet<>(); for (ResourceSharingExtension extension : resourceSharingExtensions) { Set<String> contributed = extension.resolveWorkspacesForUser(user.getName(), securityRoles, backendRoles); if (contributed != null && !contributed.isEmpty()) { merged.addAll(contributed); } } return merged; } finally { lock.readLock().unlock(); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 31fde47

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add version guard for new wire field The workspaces field is added to the wire format without any version guard. Even though the resource-sharing feature is described as not yet GA, if any node running an earlier build of this feature exists in a mixed cluster, it will send/receive the old format without the trailing workspaces field, causing EOF on read or leftover bytes on write. Add a version guard (if (in.getVersion().onOrAfter(V))) on both sides to be safe against rolling upgrades within pre-GA versions. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [119-129] public ResourceSharing(StreamInput in) throws IOException { this.resourceId = in.readString(); this.resourceType = in.readString(); this.tenant = in.readOptionalString(); this.parentType = in.readOptionalString(); this.parentId = in.readOptionalString(); this.createdBy = new CreatedBy(in); this.shareWith = in.readBoolean() ? new ShareWith(in) : null; - List<String> ws = in.readOptionalStringList(); - this.workspaces = ws == null ? null : new HashSet<>(ws); + if (in.getVersion().onOrAfter(WORKSPACES_VERSION)) { + List<String> ws = in.readOptionalStringList(); + this.workspaces = ws == null ? null : new HashSet<>(ws); + } } Suggestion importance[1-10]: 5 __ Why: Valid concern for wire compatibility during rolling upgrades even pre-GA, but the PR author explicitly documents the rationale for omitting the version guard. Impact is limited given the feature's pre-GA status.	Low
General	Use record's own type for action resolution The recordGrantsAction call uses resourceType from the method parameter, but the sharing record was fetched by resourceId and might have a different resourceType than the caller-supplied one. Consider using sharingInfo.getResourceType() to look up the correct action group registry, otherwise flattenedForType may resolve against the wrong type and grant/deny incorrectly. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [207-213] -if (recordGrantsAction(sharingInfo, resourceType, user, action)) { +if (recordGrantsAction(sharingInfo, sharingInfo.getResourceType(), user, action)) { listener.onResponse(true); return; } // resource itself does not grant the action: fall back to its containers (parent and/or workspaces) checkContainers(sharingInfo, action, visitedAncestors, listener); Suggestion importance[1-10]: 4 __ Why: The suggestion is speculative: in the calling context, resourceType is the type the caller is authorizing against, and the sharing record's stored type is typically the same. The change could actually mask bugs. Minor impact.	Low
	Avoid double-restore of thread context ctx.restore() is called inside listener callbacks, but ctx is already managed by try-with-resources which will auto-close (restore) when the try block exits. Since the listener may run asynchronously after the try block has already closed the context, calling restore() again here is either a no-op or restores an already-restored context. Remove the explicit ctx.restore() calls or restructure to not use try-with-resources. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [203-215] client.update(ur, ActionListener.wrap(updateResponse -> { - ctx.restore(); // Refresh the resource doc's principals from the now-workspace-aware record. updateResourceVisibility( resourceId, resourceIndex, existing.getAllPrincipals(), ActionListener.wrap(r -> listener.onResponse(true), listener::onFailure) ); - }, e -> { - ctx.restore(); - listener.onFailure(e); - })); + }, listener::onFailure)); Suggestion importance[1-10]: 3 __ Why: The pattern of explicit ctx.restore() inside async callbacks is used consistently across this file (see the indexResourceSharing method), because the try-with-resources close happens before the async callback runs. The suggestion misdiagnoses the pattern.	Low

---

Previous suggestions

Suggestions up to commit 533c69a

Category	Suggestion	Impact
Possible issue	Resolve workspace type via SPI, not hardcoded Hardcoding the workspace resource type as "workspace" will silently fail (deny access) for any workspace plugin that registers a different type name. This should be resolved via the SPI (e.g. a marker on ResourceProvider or a dedicated registration mechanism) rather than a magic string, otherwise the workspace inheritance feature is effectively inoperative for real workspace plugins. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [325] +// TODO: resolve workspace resource type via SPI rather than a hardcoded constant private static final String WORKSPACE_RESOURCE_TYPE = "workspace"; Suggestion importance[1-10]: 6 __ Why: Valid concern: hardcoding "workspace" as the type name limits interoperability with real workspace plugins that may register different type names, though the PR's own comments already flag this as a spike placeholder.	Low
General	Guard ancestor set against concurrent access visitedAncestors is a plain HashSet mutated across async callbacks in checkContainers/checkParent. If any listener is invoked on a different thread than the initial call (which is typical for async index handlers), concurrent access to the set is unsafe. Consider using a thread-safe set or ensuring the removal happens on the same thread as insertion; alternatively, pass a fresh copy down each recursion branch. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [172-180] final String visitKey = resourceType + ":" + resourceId; if (!visitedAncestors.add(visitKey)) { LOGGER.debug("Skipping resource '{}' of type '{}' already on the parent chain to avoid a cycle", resourceId, resourceType); outerListener.onResponse(false); return; } -// Keep the guard scoped to the current ancestor chain: remove the key when this node resolves so sibling -// branches (and later, unrelated walks sharing the set) are not falsely denied. -final ActionListener<Boolean> listener = ActionListener.runBefore(outerListener, () -> visitedAncestors.remove(visitKey)); +final ActionListener<Boolean> listener = ActionListener.runBefore(outerListener, () -> { + synchronized (visitedAncestors) { visitedAncestors.remove(visitKey); } +}); Suggestion importance[1-10]: 5 __ Why: Reasonable concern about thread-safety of the HashSet used across async callbacks; however, since the set tracks a DFS ancestor chain and typical execution is sequential per branch, the risk may be limited. Still, a valid defensive point.	Low
	Verify null-entry semantics for migration counters The entry != null branch is entered on successful creation, but indexResourceSharing invokes the listener with the sharing info on both new-creation and version-conflict paths depending on implementation. Verify that entry is truly null only when the record already existed (VersionConflictEngineException swallowed); otherwise migrated and skippedExisting/backfilledExisting counts will be miscategorized. src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java [422-428] migratedCount.getAndIncrement(); migrationStatsLatch.countDown(); } else if (docWorkspaces != null && !docWorkspaces.isEmpty()) { - // A record already exists (create was a no-op) but the source doc has workspace membership. - // Backfill the workspaces field + refresh all_shared_principals so the pre-existing record is - // not left workspace-blind. Idempotent: a no-op if the workspaces are already present. sharingIndexHandler.backfillWorkspacesOnExisting( Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify semantics rather than proposing a concrete fix, and improved_code doesn't materially change the logic.	Low
	Verify stream read/write field order symmetry The stream constructor reads fields in a different order than writeTo writes them. writeTo writes createdBy before shareWith, and workspaces last — this matches. However, verify the read/write order matches exactly; any mismatch will corrupt the wire format. Also ensure the field order stays symmetric across future changes. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [119-129] +public ResourceSharing(StreamInput in) throws IOException { + this.resourceId = in.readString(); + this.resourceType = in.readString(); + this.tenant = in.readOptionalString(); + this.parentType = in.readOptionalString(); + this.parentId = in.readOptionalString(); + this.createdBy = new CreatedBy(in); + this.shareWith = in.readBoolean() ? new ShareWith(in) : null; + List<String> ws = in.readOptionalStringList(); + this.workspaces = ws == null ? null : new HashSet<>(ws); +} - Suggestion importance[1-10]: 2 __ Why: The suggestion merely asks to verify the read/write order, and improved_code is identical to existing_code. Low actionable value.	Low

Suggestions up to commit 48ccf99

Category	Suggestion	Impact
General	Skip empty mget on hot path When workspaceIds is empty but workspaceIndex is non-null (no workspace memberships on this resource), the code still issues an empty mget. Skip the batch fetch when workspaceIds is empty and go straight to checkParent to avoid an unnecessary round-trip on the privilege hot path. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [287-299] -if (workspaceIndex != null) { +if (workspaceIndex != null && !workspaceIds.isEmpty()) { resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { for (ResourceSharing wsRecord : records.values()) { if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, user, action)) { listener.onResponse(true); return; } } checkParent(sharingInfo, action, visitedAncestors, listener); }, listener::onFailure)); } else { checkParent(sharingInfo, action, visitedAncestors, listener); } Suggestion importance[1-10]: 5 __ Why: Valid minor optimization; however, workspaceIndex is already computed as null when workspaceIds.isEmpty(), so the empty mget case is already guarded. The suggestion is redundant but harmless.	Low
	Document visited-set cleanup invariant If admin-DN check or authenticated-user check returned early above, visitedAncestors was already mutated with visitKey in prior nested calls, but here the guard is added before the admin short-circuit runs — meaning the admin path also does the add, but that path returns without triggering the runBefore cleanup (it calls listener.onResponse(true) on the wrapped listener). Actually the wrapped listener does run cleanup — verify the admin/sharingInfo == null early returns all go through listener (the wrapped one), not outerListener, so the visited-set key is properly released. Currently the admin path uses listener.onResponse(true) which is correct, but this is fragile; consider always using the wrapped listener and asserting cleanup coverage. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [172-180] -// Ancestor-cycle guard: block only if this resource is already on the current parent chain (an actual -// cycle, e.g. A -> parent B -> parent A). This is a DFS-path guard, not a global visited set: a resource -// seen and released on one branch must stay evaluable on another, so the key is removed once this node's -// evaluation completes (via the runBefore wrapper below). Denying a true on-path repeat is safe — the -// ancestor that first introduced it is still being evaluated and will contribute its own grant. final String visitKey = resourceType + ":" + resourceId; if (!visitedAncestors.add(visitKey)) { LOGGER.debug("Skipping resource '{}' of type '{}' already on the parent chain to avoid a cycle", resourceId, resourceType); outerListener.onResponse(false); return; } +// All subsequent early returns MUST go through `listener` (not `outerListener`) so the visited-set +// key is released via runBefore below. +final ActionListener<Boolean> listener = ActionListener.runBefore(outerListener, () -> visitedAncestors.remove(visitKey)); Suggestion importance[1-10]: 4 __ Why: The suggestion mostly asks the author to verify existing behavior and add a comment; it acknowledges the current code is correct. Low impact.	Low
	Ensure stable collection wire format writeOptionalStringCollection accepts a Collection, but workspaces is a Set. More importantly, the read side uses readOptionalStringList which expects a list written by writeOptionalStringCollection — verify iteration order stability. Additionally, if this sample resource is used across mixed-version nodes, the wire format has changed without a version guard; ensure both ends are always upgraded together (as done in ResourceSharing), otherwise deserialization will fail on older nodes. sample-resource-plugin/src/main/java/org/opensearch/sample/SampleResource.java [121-122] user.writeTo(out); // Symmetric with the StreamInput ctor. Passing null when unset keeps mixed-caller compatibility. -out.writeOptionalStringCollection(workspaces); +out.writeOptionalStringCollection(workspaces == null ? null : new java.util.ArrayList<>(workspaces)); Suggestion importance[1-10]: 3 __ Why: writeOptionalStringCollection accepts any Collection<String> including Set, so the conversion is unnecessary. The BWC concern is somewhat valid but the suggested change doesn't address it.	Low
	Clarify unreachable fallback branch getAllPrincipals() already includes "user:" + createdBy.getUsername() when createdBy is non-null, so the isEmpty() fallback branch is effectively dead code for the normal creation flow. If createdBy were ever null, both branches would fail; consider dropping the fallback (or asserting createdBy != null) to make the intent explicit and avoid confusion. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [334-337] List<String> initialPrincipals = new ArrayList<>(sharingInfo.getAllPrincipals()); -if (initialPrincipals.isEmpty()) { - initialPrincipals.add("user:" + createdBy.getUsername()); -} +assert !initialPrincipals.isEmpty() : "sharingInfo must have at least a creator principal"; Suggestion importance[1-10]: 3 __ Why: Minor code-clarity observation. Replacing a defensive fallback with an assert could weaken robustness in production (assertions are disabled by default), so the improvement is marginal.	Low

Suggestions up to commit 3115d4c

Category	Suggestion	Impact
Possible issue	Prevent latch hang on sync throw The entry != null branch increments migratedCount but the else if branch dispatches an async backfill call. If backfillWorkspacesOnExisting invokes its listener synchronously on the current thread (e.g. for the empty-set fast path), the latch countdown ordering is fine, but if it throws synchronously before invoking the listener, the latch will never be decremented and the migration will hang. Wrap the call in try/catch to guarantee countdown. src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java [424-444] } else if (docWorkspaces != null && !docWorkspaces.isEmpty()) { - // A record already exists (create was a no-op) but the source doc has workspace membership. - // Backfill the workspaces field + refresh all_shared_principals so the pre-existing record is - // not left workspace-blind. Idempotent: a no-op if the workspaces are already present. - sharingIndexHandler.backfillWorkspacesOnExisting( - sourceInfo.sourceIndex, - resourceId, - docWorkspaces, - ActionListener.wrap(changed -> { + try { + sharingIndexHandler.backfillWorkspacesOnExisting( + sourceInfo.sourceIndex, + resourceId, + docWorkspaces, + ActionListener.wrap(changed -> { + if (Boolean.TRUE.equals(changed)) { + backfilledExisting.getAndIncrement(); + } else { + skippedExisting.getAndIncrement(); + } + migrationStatsLatch.countDown(); + }, e -> { + LOGGER.warn("Failed to backfill workspaces for existing record [{}]: {}", resourceId, e.getMessage()); + failureCount.getAndIncrement(); + migrationStatsLatch.countDown(); + }) + ); + } catch (Exception ex) { + LOGGER.warn("Synchronous failure backfilling workspaces for [{}]: {}", resourceId, ex.getMessage()); + failureCount.getAndIncrement(); + migrationStatsLatch.countDown(); + } Suggestion importance[1-10]: 6 __ Why: Valid concern: a synchronous exception from backfillWorkspacesOnExisting before invoking the listener would leave the latch undecremented, causing the migration to hang. Wrapping in try/catch is a reasonable defensive measure.	Low
General	Avoid double-restore of thread context ctx.restore() is called inside both success and failure callbacks, but the try-with-resources block will also invoke close() on the stashed context when execution leaves the try block. Calling restore() and then having the try-with-resources close it can lead to double-restore or thread-context leaks if the callbacks execute after the try block exits. Reconsider whether the try-with-resources should be used here (async pattern usually stashes without try-with-resources and restores in listener). src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [198-216] -client.update(ur, ActionListener.wrap(updateResponse -> { +ThreadContext.StoredContext ctx = this.threadPool.getThreadContext().stashContext(); +try { + UpdateRequest ur = client.prepareUpdate(resourceSharingIndex, resourceId) + .setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) + .setDoc(Map.of("workspaces", merged)) + .request(); + client.update(ur, ActionListener.wrap(updateResponse -> { + ctx.restore(); + updateResourceVisibility( + resourceId, + resourceIndex, + existing.getAllPrincipals(), + ActionListener.wrap(r -> listener.onResponse(true), listener::onFailure) + ); + }, e -> { + ctx.restore(); + listener.onFailure(e); + })); +} catch (Exception ex) { ctx.restore(); - // Refresh the resource doc's principals from the now-workspace-aware record. - updateResourceVisibility( - resourceId, - resourceIndex, - existing.getAllPrincipals(), - ActionListener.wrap(r -> listener.onResponse(true), listener::onFailure) - ); -}, e -> { - ctx.restore(); - listener.onFailure(e); -})); + throw ex; +} Suggestion importance[1-10]: 4 __ Why: The concern about double-restore is partially valid, but StoredContext.restore() is typically idempotent and the pattern is used elsewhere in the codebase. The suggested improvement doesn't clearly fix the raised issue.	Low
	Log missing workspace sharing records The workspace records returned by the batched mget may be missing entries (records that failed to parse or don't exist), and iterating records.values() skips them silently. Consider logging or handling the missing-record case explicitly to avoid confusing "silent denial" behavior when a workspace id has no sharing record; at minimum, log at debug level for observability. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [287-299] if (workspaceIndex != null) { resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { - for (ResourceSharing wsRecord : records.values()) { + for (String wsId : workspaceIds) { + ResourceSharing wsRecord = records.get(wsId); + if (wsRecord == null) { + LOGGER.debug("No sharing record found for workspace '{}'; skipping", wsId); + continue; + } if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, user, action)) { listener.onResponse(true); return; } } checkParent(sharingInfo, action, visitedAncestors, listener); }, listener::onFailure)); } else { checkParent(sharingInfo, action, visitedAncestors, listener); } Suggestion importance[1-10]: 3 __ Why: Adding debug logging for missing workspace records improves observability but has minimal functional impact. The current code correctly handles missing entries by iterating only existing values.	Low
	Document wire-format ordering contract The stream constructor's read order must be identical to writeTo's write order for round-trips to work. Verify that writeTo writes createdBy before shareWith and workspaces last — the current implementation does, but any future reordering will silently break wire compatibility. Consider adding a comment referencing writeTo to prevent divergence. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [119-129] +// NOTE: field order MUST mirror writeTo(). See writeTo() below. public ResourceSharing(StreamInput in) throws IOException { this.resourceId = in.readString(); this.resourceType = in.readString(); this.tenant = in.readOptionalString(); this.parentType = in.readOptionalString(); this.parentId = in.readOptionalString(); this.createdBy = new CreatedBy(in); this.shareWith = in.readBoolean() ? new ShareWith(in) : null; List<String> ws = in.readOptionalStringList(); this.workspaces = ws == null ? null : new HashSet<>(ws); } Suggestion importance[1-10]: 2 __ Why: This is a minor documentation/comment suggestion that only asks to add a comment. The existing implementation is already correct.	Low

Suggestions up to commit 19af1df

Category	Suggestion	Impact
Possible issue	Fix double restore of thread context Calling ctx.restore() inside the async callback conflicts with the try-with-resources on ctx, which will also restore on scope exit — the second restore on a already-closed StoredContext can throw or corrupt the thread context. Remove the manual ctx.restore() calls inside the listener since the try-with-resources handles it, or drop the try-with-resources and restore manually. This same pattern appears in fetchSharingInfoForIds. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [203-215] client.update(ur, ActionListener.wrap(updateResponse -> { - ctx.restore(); - // Refresh the resource doc's principals from the now-workspace-aware record. updateResourceVisibility( resourceId, resourceIndex, existing.getAllPrincipals(), ActionListener.wrap(r -> listener.onResponse(true), listener::onFailure) ); -}, e -> { - ctx.restore(); - listener.onFailure(e); -})); +}, listener::onFailure)); Suggestion importance[1-10]: 7 __ Why: Calling ctx.restore() explicitly inside the async callback while the try-with-resources will also close the StoredContext on scope exit can lead to double-restore issues. This is a legitimate concern about correctness of thread-context management, though the exact behavior depends on StoredContext idempotency.	Medium
General	Log when workspace provider is missing The workspace branch is only taken when workspaceIndex != null, but workspaceIndex is set to null when workspaceIds.isEmpty(). If workspaceIds is non-empty but no provider is registered for the workspace type (indexByType returns null), workspaces are silently ignored without logging. Add a debug/warn log when the workspace type is unregistered so operators can diagnose why workspace-based access is being skipped. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [287-299] if (workspaceIndex != null) { resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { for (ResourceSharing wsRecord : records.values()) { if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, user, action)) { listener.onResponse(true); return; } } checkParent(sharingInfo, action, visitedAncestors, listener); }, listener::onFailure)); } else { + if (!workspaceIds.isEmpty()) { + LOGGER.debug("Resource has workspaces {} but no provider is registered for type '{}'; skipping workspace inheritance", workspaceIds, WORKSPACE_RESOURCE_TYPE); + } checkParent(sharingInfo, action, visitedAncestors, listener); } Suggestion importance[1-10]: 4 __ Why: Adding a diagnostic log when workspace ids are present but no provider is registered is a reasonable observability improvement, but low-impact and not a correctness issue.	Low
	Avoid hard-denying on cycle detection Returning false on an on-path cycle short-circuits the entire evaluation with a denial, even though other branches (siblings/workspaces) at the ancestor level might still grant access under OR semantics. Instead of denying, skip this branch by returning without contributing a grant, letting the caller's other branches decide. Consider signaling "no decision from this branch" rather than a hard deny. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [173-180] if (!visitedAncestors.add(visitKey)) { LOGGER.debug("Skipping resource '{}' of type '{}' already on the parent chain to avoid a cycle", resourceId, resourceType); + // Do not contribute a grant from this cyclic branch; let sibling branches decide. outerListener.onResponse(false); return; } -// Keep the guard scoped to the current ancestor chain: remove the key when this node resolves so sibling -// branches (and later, unrelated walks sharing the set) are not falsely denied. final ActionListener<Boolean> listener = ActionListener.runBefore(outerListener, () -> visitedAncestors.remove(visitKey)); Suggestion importance[1-10]: 3 __ Why: The suggestion is largely a no-op — the improved_code still calls outerListener.onResponse(false) for the cyclic branch, just as the existing code does. Under OR semantics returning false from one branch already lets other branches decide, so the concern is somewhat misplaced.	Low
	Verify stream read/write symmetry writeTo uses writeOptionalStringCollection which writes a boolean followed by the size and elements, while readOptionalStringList must match that exact format. Verify the pairing is correct — if any older node exists that serializes without the workspaces field, deserialization will fail with an unexpected EOF or corrupt subsequent reads. Since the comment claims no BWC concerns, ensure this constructor is only ever used against writers that include the field. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [119-129] public ResourceSharing(StreamInput in) throws IOException { this.resourceId = in.readString(); this.resourceType = in.readString(); this.tenant = in.readOptionalString(); this.parentType = in.readOptionalString(); this.parentId = in.readOptionalString(); this.createdBy = new CreatedBy(in); this.shareWith = in.readBoolean() ? new ShareWith(in) : null; + // Must remain symmetric with writeTo's writeOptionalStringCollection(workspaces). List<String> ws = in.readOptionalStringList(); this.workspaces = ws == null ? null : new HashSet<>(ws); } Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify symmetry and adds a comment; the existing code already correctly pairs writeOptionalStringCollection with readOptionalStringList. The PR's stream round-trip tests confirm this works.	Low

Suggestions up to commit 5333eba

Category	Suggestion	Impact
General	Verify correct resource type used for workspace action-group resolution In checkContainers, recordGrantsAction is called with WORKSPACE_RESOURCE_TYPE as the resource type for all workspace records. However, recordGrantsAction calls resourcePluginInfo.flattenedForType(resourceType) to resolve action groups. If the workspace type's action groups are not registered or differ from the original resource type, this will silently use the wrong action group resolution. The workspace resource type should be verified to have its action groups registered, or the original resource type should be passed when checking workspace-inherited access. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [288-293] -private void checkContainers( - ResourceSharing sharingInfo, - String action, - Set<String> visitedAncestors, - ActionListener<Boolean> listener -) { - final User user = getAuthenticatedUser(); - if (user == null) { - listener.onResponse(false); - return; - } - ... - if (workspaceIndex != null) { - resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { - for (ResourceSharing wsRecord : records.values()) { - if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, user, action)) { +if (workspaceIndex != null) { + resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { + for (ResourceSharing wsRecord : records.values()) { + // Use the original resourceType for action-group resolution since workspace containers + // grant access to the child resource's actions, not workspace-specific actions. + if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, user, action)) { Suggestion importance[1-10]: 5 __ Why: This raises a valid concern about using WORKSPACE_RESOURCE_TYPE for action-group resolution when checking workspace-inherited access — the workspace type may have different or missing action groups. However, the improved_code is identical to the existing_code, making it only a verification suggestion rather than an actual fix.	Low
	Remove unreachable fallback that masks potential null creator bug getAllPrincipals() always adds "user:" + createdBy.getUsername() when createdBy is non-null, so initialPrincipals will never be empty in normal cases. The fallback if (initialPrincipals.isEmpty()) block is dead code and could mask a bug where createdBy is null (which would cause a NullPointerException in getAllPrincipals anyway). The fallback should be removed or the null case should be handled explicitly. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [273-276] List<String> initialPrincipals = new ArrayList<>(sharingInfo.getAllPrincipals()); -if (initialPrincipals.isEmpty()) { - initialPrincipals.add("user:" + createdBy.getUsername()); -} +// getAllPrincipals() always includes the creator; no fallback needed. +updateResourceVisibility(resourceId, resourceIndex, initialPrincipals, ActionListener.wrap((updateResponse) -> { Suggestion importance[1-10]: 4 __ Why: The observation that getAllPrincipals() always adds the creator principal (making the fallback dead code) is correct and valid. However, the improved_code snippet is incomplete — it cuts off mid-statement without showing the full replacement — reducing its actionability.	Low
	Ensure symmetric null/empty handling in stream deserialization The writeTo method writes workspaces using writeOptionalStringCollection, which writes null when workspaces is null. However, when workspaces is an empty set (non-null), it writes an empty list. The readOptionalStringList call will return an empty list in that case, but the constructor stores it as a non-null empty HashSet. This asymmetry is fine, but more critically: when workspaces is null on write, readOptionalStringList returns null, and getWorkspaces() then returns emptySet(). This is consistent, but the equals method compares getWorkspaces() (which normalizes null to empty), while hashCode also uses getWorkspaces(). Ensure the stream round-trip is truly symmetric by always writing a non-null collection. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [127-128] List<String> ws = in.readOptionalStringList(); -this.workspaces = ws == null ? null : new HashSet<>(ws); +this.workspaces = (ws == null || ws.isEmpty()) ? null : new HashSet<>(ws); Suggestion importance[1-10]: 3 __ Why: The suggestion proposes normalizing empty lists to null during deserialization, but the current behavior is already consistent: getWorkspaces() normalizes null to empty, and both equals and hashCode use getWorkspaces(). The improved_code is functionally equivalent for the equals/hashCode contract, making this a minor style preference rather than a real bug fix.	Low

[github-actions]

Persistent review updated to latest commit 7412d76

[codecov]

Codecov Report

❌ Patch coverage is 78.17460% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.73%. Comparing base (d1b600b) to head (31fde47).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ecurity/resources/ResourceSharingIndexHandler.java	77.77%	11 Missing and 5 partials ⚠️
...arch/security/resources/ResourceAccessHandler.java	80.48%	6 Missing and 2 partials ⚠️
...i/migrate/MigrateResourceSharingInfoApiAction.java	78.94%	4 Missing and 4 partials ⚠️
...ch/security/resources/sharing/ResourceSharing.java	82.35%	4 Missing and 2 partials ⚠️
...ain/java/org/opensearch/sample/SampleResource.java	66.66%	4 Missing and 1 partial ⚠️
...nsearch/security/resources/ResourcePluginInfo.java	75.00%	2 Missing and 3 partials ⚠️
.../actions/rest/create/CreateResourceRestAction.java	72.72%	1 Missing and 2 partials ⚠️
...org/opensearch/sample/SampleResourceExtension.java	75.00%	1 Missing and 1 partial ⚠️
...arch/security/resources/ResourceIndexListener.java	85.71%	0 Missing and 1 partial ⚠️
...ch/security/resources/ResourceSharingDlsUtils.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6374      +/-   ##
==========================================
+ Coverage   75.51%   75.73%   +0.22%     
==========================================
  Files         456      457       +1     
  Lines       30282    30628     +346     
  Branches     4574     4649      +75     
==========================================
+ Hits        22866    23197     +331     
- Misses       5284     5287       +3     
- Partials     2132     2144      +12     

Files with missing lines	Coverage Δ
...earch/security/spi/resources/ResourceProvider.java	83.33% <100.00%> (+3.33%)	⬆️
...curity/spi/resources/ResourceSharingExtension.java	100.00% <100.00%> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java	84.17% <ø> (ø)
...search/security/configuration/DlsFlsValveImpl.java	72.89% <ø> (+4.88%)	⬆️
...arch/security/resources/ResourceIndexListener.java	88.50% <85.71%> (-0.78%)	⬇️
...ch/security/resources/ResourceSharingDlsUtils.java	74.07% <75.00%> (+0.16%)	⬆️
...org/opensearch/sample/SampleResourceExtension.java	88.23% <75.00%> (-11.77%)	⬇️
.../actions/rest/create/CreateResourceRestAction.java	77.27% <72.72%> (-1.52%)	⬇️
...ain/java/org/opensearch/sample/SampleResource.java	68.18% <66.66%> (-2.19%)	⬇️
...nsearch/security/resources/ResourcePluginInfo.java	82.78% <75.00%> (-1.19%)	⬇️
... and 4 more

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit 0a7486f

[github-actions]

Persistent review updated to latest commit 5333eba

[github-actions]

Persistent review updated to latest commit 19af1df

[github-actions]

Persistent review updated to latest commit 3115d4c

[github-actions]

Persistent review updated to latest commit 31fde47

[DarshitChanpura]

Open items / scope notes for reviewers

In-repo, tracked as follow-ups (not blocking this PR's mechanics):

• A few changed lines are covered by integration tests only (e.g. the ResourceSharingDlsUtils workspace-principal loop, which runs under DLS integration, and SampleResource's transport round-trip). Called out for visibility.
• WORKSPACE_RESOURCE_TYPE in the write-path fan-out is a placeholder constant; the authoritative type name comes from the workspace provider once it's registered (see cross-repo below). Degrades safely — if no provider registers that type, the workspace branch denies cleanly.

Cross-repo dependencies (cannot land here):

• OpenSearch-Dashboards / opensearch-workspaces backend plugin (per Backend workspace collaborator management via security plugin resource sharing #6119): must register the saved-object index as a protected resource, declare its workspacesField(), register the workspace resource type, and implement the trusted resolveWorkspacesForUser SPI. Until then the read-path resolver is inert by design.
• Materializing workspace collaborator records from the existing frontend-managed ACLs (the permissions field on the workspace saved object) — the collaborator data source lives in OSD; this PR provides the record-creation + backfill machinery.
• security-dashboards-plugin: workspace share UI wiring.

Decisions for the team (not code):

• The exact graduation gate for Graduate resource sharing feature out of experimental #6348 — is workspace compatibility (this PR) sufficient, or is full end-to-end enforcement required first?
• MDS scope: there is currently no MDS/datasource handling in RP code; whether it's in scope for graduation is open.

Happy to split any of the above into tracking issues if preferred.

[DarshitChanpura]

Decision needed: where does the OpenSearch-side workspace backend live?

The ResourceSharingExtension SPI (registering the saved-object index as a protected resource, declaring workspacesField(), registering the workspace resource type, and providing the trusted resolveWorkspacesForUser) must be implemented by an OpenSearch-side (Java) plugin. None exists today — workspaces currently live entirely in the OSD Node.js layer — so new backend code is required regardless. The open question is where:

• Option A — new opensearch-workspaces backend plugin (per Backend workspace collaborator management via security plugin resource sharing #6119): cleanest separation; owns workspace resource-type + membership resolution.
• Option B — fold into an existing OpenSearch-side module (e.g. core's opensearch-dashboards module): avoids a new plugin/repo but couples workspace backend logic there.

Not the security plugin's responsibility either way. Owner: workspaces team. This gates the read-path resolver going live (it is inert until implemented) and ties into the #6348 graduation gate.

[DarshitChanpura]

Follow-up: dynamic workspace membership updates (associate/dissociate)

OSD shares an object to another workspace by mutating its workspaces array via an update (POST /api/workspaces/_associate; _dissociate reverses it — verified in OpenSearch-Dashboards on 2.19 and main).

ResourceIndexListener.postIndex extracts workspaces only on the create branch. On update (!result.isCreated()) it calls fetchAndUpdateResourceVisibility, which recomputes all_shared_principals from the sharing record, not from the doc's changed workspaces field (this PR, ResourceIndexListener L94 + L136/195). So:

• associating a resource to a new workspace after creation does not add its workspace:<id> principal, and
• dissociating does not remove the stale one (over-sharing).

Not addressed in this PR (create + migrate-backfill are). Fix: on update, re-extract the object's workspaces and recompute principals (adds and removals). This is naturally the OpenSearch-side workspace backend's responsibility (see the "where does the backend live" decision). Flagging so it's tracked before enforcement is turned on.