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.

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 — sharing records are byte-identical (the workspaces field is omitted when empty), and the workspace resolver is disabled unless a plugin opts in. The DLS filter's shape changes from a single terms to a bool.should (min_should_match=1); with an empty resolver the workspace clause is dropped, so it stays semantically equivalent (same documents match) though the query JSON differs. 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 649432f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Race in Registration Validation The conflict check for workspacesField uses putIfAbsent while iterating providers, but a provider that declares workspacesField=null (opt-out) will not be recorded in the map. If another provider on the same index declares a non-null field, the null-declaring provider still contributes to that index for ingestion (via its own workspacesField() returning null, skipping the branch) — this is fine — but workspacesFieldForIndex returns the first provider's declared field regardless of iteration order. Because providers can be iterated in any order (Set), a provider with null and a provider with a real field on the same index will always work, but the assertion "the value is unambiguous regardless of map iteration order" only holds because null values are skipped. This is subtle and worth an explicit comment/test that mixing null and non-null on the same index is permitted intentionally. String workspacesField = rp.workspacesField(); if (workspacesField != null) { String existing = indexToWorkspacesField.putIfAbsent(rp.resourceIndexName(), workspacesField); if (existing != null && !existing.equals(workspacesField)) { throw new OpenSearchSecurityException( String.format( "Conflicting workspaces fields declared for resource index [%s]: [%s] and [%s]. All providers sharing" + " an index must declare the same workspaces field (or null to opt out).", rp.resourceIndexName(), existing, workspacesField ) ); } } Possible Data Loss on Reconcile Failure In reconcileWorkspacesAttempt, when the sharing record is missing after WORKSPACE_RECONCILE_MAX_ATTEMPTS retries (~500ms total), the method logs an error and calls listener.onResponse(false). In ResourceIndexListener.postIndex, the listener only logs a warning on failure. If the sharing record creation is delayed beyond ~500ms (e.g., under load, slow refresh, or during recovery), the workspace membership silently never gets stamped onto the record. The suggested remediation (re-run migrate API) requires operator intervention. Consider increasing retry budget or using an exponential backoff scheme, since this affects write-path authorization correctness. if (attempt < WORKSPACE_RECONCILE_MAX_ATTEMPTS) { threadPool.schedule( () -> reconcileWorkspacesAttempt(resourceSharingIndex, resourceId, target, sourceSeqNo, attempt + 1, listener), WORKSPACE_RECONCILE_RETRY_DELAY, ThreadPool.Names.GENERIC ); } else { // Fail loud: the record never appeared, so the write-path record may be out of step with the // resource's workspaces. Re-run the migrate API to repair once the record exists. LOGGER.error( "Sharing record [{}] still missing after {} attempts; workspaces left unreconciled. Re-run " + "POST _plugins/_security/api/resources/migrate to repair.", resourceId, attempt ); listener.onResponse(false); } return; Possible Issue The update flow now issues a GetRequest to fetch existing workspaces before indexing. If the GET fails (e.g. transient issue), the entire update fails — a regression from the prior behavior where updates always succeeded. Additionally, this introduces a TOCTOU window: between the GET and the subsequent index, a concurrent workspace association could be lost because the index overwrites the doc entirely with sample.toXContent(...). Consider whether the sample plugin should model this via a partial update (script/doc merge) instead, or document that sample-plugin behavior is illustrative. pluginClient.get(new GetRequest(RESOURCE_INDEX_NAME, resourceId), ActionListener.wrap(getResponse -> { try { Set<String> existingWorkspaces = new HashSet<>(); if (getResponse.isExists() && getResponse.getSource() != null && getResponse.getSource().get("workspaces") instanceof Collection<?> c) { for (Object o : c) { if (o != null) { existingWorkspaces.add(o.toString()); } } } sample.setWorkspaces(existingWorkspaces.isEmpty() ? null : existingWorkspaces); try (XContentBuilder builder = jsonBuilder()) { sample.toXContent(builder, ToXContent.EMPTY_PARAMS); // because some plugins seem to treat update API calls as index request IndexRequest ir = new IndexRequest(RESOURCE_INDEX_NAME).id(resourceId) .setRefreshPolicy(WriteRequest.RefreshPolicy.WAIT_UNTIL) // WAIT_UNTIL because we don't want tests to fail, as they // execute search right after update .source(builder); log.debug("Update Request: {}", ir.toString()); pluginClient.index( ir, ActionListener.wrap( updateResponse -> listener.onResponse( new CreateResourceResponse("Resource " + sample.getName() + " updated successfully.") ), listener::onFailure ) ); } } catch (Exception e) { log.error(() -> new ParameterizedMessage("Failed to update resource: {}", resourceId), e); listener.onFailure(e); } }, listener::onFailure)); Missing Cycle Guard The PR description states "an ancestor-scoped cycle guard prevents unbounded recursion" in the container fan-out, but checkParent recurses into hasPermission(parentId, parentType, action, listener) unconditionally without tracking visited ancestors. If a resource has a parent chain with a cycle (parent A -> parent B -> parent A), this will recurse indefinitely. The workspace test testHasPermission_workspaceIsLeafEvaluatedNoRecursion proves workspaces are not recursed, but the parent chain has no such guard visible here. /** * Resolves access inherited from the single hierarchical parent (if any), recursing via {@link #hasPermission} so * grandparent chains continue to work. Denies when there is no parent. */ private void checkParent(ResourceSharing sharingInfo, String action, ActionListener<Boolean> listener) { if (sharingInfo.getParentId() != null) { hasPermission(sharingInfo.getParentId(), sharingInfo.getParentType(), action, listener); } else { listener.onResponse(false); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 649432f

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Propagate user instead of re-resolving checkContainers re-reads the authenticated user from the thread context inside a callback chain triggered by an async fetchSharingInfo result. Depending on the executing thread the persistent context may not be the caller's, and even if it is, hasPermission already used the user to compute grants — the container fan-out should use that same user consistently. Pass the user (or a resolved principal set) down from hasPermission rather than re-resolving here, so the workspace check cannot silently fall back to null (denying legitimate access) or to a different user context. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [216-224] -private void checkContainers(ResourceSharing sharingInfo, String action, ActionListener<Boolean> listener) { - final User user = getAuthenticatedUser(); +private void checkContainers(ResourceSharing sharingInfo, User user, String action, ActionListener<Boolean> listener) { if (user == null) { listener.onResponse(false); return; } final List<String> workspaceIds = new ArrayList<>(sharingInfo.getWorkspaces()); final String workspaceIndex = workspaceIds.isEmpty() ? null : resourcePluginInfo.indexByType(WORKSPACE_RESOURCE_TYPE); Suggestion importance[1-10]: 6 __ Why: Passing the user through avoids potential inconsistency if the thread context changes between the initial hasPermission call and the async callback. Reasonable defensive improvement, though in practice the context is typically preserved.	Low
Possible issue	Version-guard new wire-format fields The ResourceSharing wire format was extended with two new trailing fields (workspaces and workspacesSeqNo) without a version guard. In a mixed-version cluster during rolling upgrade, an older node that still uses the pre-change reader will EOF on these trailing bytes (new writer -> old reader), and an older writer's payload will leave the new reader trying to read past end-of-stream (old writer -> new reader). Guard both the write and read sides symmetrically with out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) using the first unreleased version constant that introduces this change. If the claim in the comment that "there is no older node that speaks the old wire format" holds, document/verify it against the actual released versions; otherwise this will break BWC. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [129-140] 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); - this.workspacesSeqNo = in.readZLong(); + if (in.getVersion().onOrAfter(V_WORKSPACES)) { + List<String> ws = in.readOptionalStringList(); + this.workspaces = ws == null ? null : new HashSet<>(ws); + this.workspacesSeqNo = in.readZLong(); + } else { + this.workspaces = null; + this.workspacesSeqNo = SequenceNumbers.UNASSIGNED_SEQ_NO; + } } Suggestion importance[1-10]: 5 __ Why: The comment explicitly states the feature is not yet GA so no older nodes speak the old wire format; the suggestion raises a valid BWC concern but the PR author has already reasoned about it. Moderate importance if the assumption is incorrect.	Low
	Guard write side of new fields Mirror-side of the same wire-format concern: the writer unconditionally appends two new fields. Guard with out.getVersion().onOrAfter(V) so peers before that version continue to receive the pre-change layout, and only newer peers receive the extra fields. Even if the feature is behind a flag, mixed-version clusters can still exchange these NamedWriteables during a rolling upgrade. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [304-309] -// 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)); -out.writeZLong(workspacesSeqNo); +if (out.getVersion().onOrAfter(V_WORKSPACES)) { + out.writeOptionalStringCollection(workspaces == null ? null : new ArrayList<>(workspaces)); + out.writeZLong(workspacesSeqNo); +} Suggestion importance[1-10]: 5 __ Why: Mirror of the previous suggestion for the write side. Same reasoning: the author documented the assumption that no older nodes exist, but symmetric version-guarding would be more defensive.	Low
	Version-guard SampleResource new field SampleResource is a NamedWriteable whose writeTo/StreamInput constructor added a trailing workspaces field with no version guard. If any older peer can still send or receive SampleResource on the wire (e.g., transport-action request/response), an older reader will fail to parse the extra bytes and an older writer's payload will short the new reader. Add symmetric getVersion().onOrAfter(V) guards on both sides. If SampleResource is genuinely only reachable within a single-version boundary this is fine, but that must be verified — the sample plugin is used in integration tests exercising real transport actions. sample-resource-plugin/src/main/java/org/opensearch/sample/SampleResource.java [114-122] public void writeTo(StreamOutput out) throws IOException { out.writeString(name); out.writeOptionalString(description); out.writeOptionalString(groupId); out.writeMap(attributes, StreamOutput::writeString, StreamOutput::writeString); user.writeTo(out); - // Symmetric with the StreamInput ctor. Passing null when unset keeps mixed-caller compatibility. - out.writeOptionalStringCollection(workspaces); + if (out.getVersion().onOrAfter(V_WORKSPACES)) { + out.writeOptionalStringCollection(workspaces); + } } Suggestion importance[1-10]: 3 __ Why: SampleResource is a sample/test plugin class, so BWC concerns are less critical than for production security code. The suggestion is still technically valid but low impact.	Low

---

Previous suggestions

Suggestions up to commit 8f62005

Category	Suggestion	Impact
Possible issue	Do not silently ignore mget failures Silently skipping failed mget items on the authorization hot path is dangerous: a transient shard failure for a workspace container record will look identical to "record doesn't exist", causing checkContainers to falsely deny (or, worse, if you later change the semantics, falsely allow). Propagate an error when items fail so callers do not authorize on partial data. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [710-714] client.multiGet(mget, ActionListener.wrap(mres -> { ctx.restore(); Map<String, ResourceSharing> records = new HashMap<>(); for (MultiGetItemResponse item : mres.getResponses()) { - if (item == null || item.isFailed()) continue; + if (item == null) continue; + if (item.isFailed()) { + listener.onFailure(item.getFailure().getFailure()); + return; + } Suggestion importance[1-10]: 7 __ Why: Silently skipping failed mget items on the authorization path can conflate transient failures with missing records, leading to incorrect deny/allow decisions. Propagating failures is a reasonable safety improvement.	Medium
	Version-guard added stream fields The ResourceSharing wire format adds two trailing fields (workspaces and workspacesSeqNo) with no version guard. Even if the feature is not GA, once shipped these bytes travel between mixed-version nodes during rolling upgrade; a newer writer sending to an older reader (or vice versa) will EOF/corrupt the stream. Wrap both read and write in an onOrAfter(V) check against the first release containing this change, on both sides symmetrically. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [130-141] 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); - this.workspacesSeqNo = in.readZLong(); + if (in.getVersion().onOrAfter(Version.V_WORKSPACE_AWARE_SHARING)) { + List<String> ws = in.readOptionalStringList(); + this.workspaces = ws == null ? null : new HashSet<>(ws); + this.workspacesSeqNo = in.readZLong(); + } else { + this.workspaces = null; + this.workspacesSeqNo = SequenceNumbers.UNASSIGNED_SEQ_NO; + } } Suggestion importance[1-10]: 3 __ Why: The PR explicitly documents that no version guard is needed because the feature is not GA and ships within 3.3. The suggestion is defensive but contradicts the stated design rationale in the code comment.	Low
	Version-guard SampleResource workspaces field SampleResource is a NamedWriteable whose wire format gains a trailing workspaces field with no version guard. If any older peer can read/write this type during a rolling upgrade, the new writer sending to an old reader will leave the trailing bytes unread, and an old writer to a new reader will EOF at readOptionalStringList(). Guard both writeTo and the stream constructor symmetrically with onOrAfter(V). sample-resource-plugin/src/main/java/org/opensearch/sample/SampleResource.java [54-62] public SampleResource(StreamInput in) throws IOException { this.name = in.readString(); this.description = in.readOptionalString(); this.groupId = in.readOptionalString(); this.attributes = in.readMap(StreamInput::readString, StreamInput::readString); this.user = new User(in); - List<String> ws = in.readOptionalStringList(); - this.workspaces = ws == null ? null : new HashSet<>(ws); + if (in.getVersion().onOrAfter(Version.V_WORKSPACE_AWARE_SHARING)) { + List<String> ws = in.readOptionalStringList(); + this.workspaces = ws == null ? null : new HashSet<>(ws); + } } Suggestion importance[1-10]: 2 __ Why: SampleResource is a sample/test plugin resource, not production wire protocol, so BWC concerns are limited. The concern is minor for a sample plugin.	Low
General	Deduplicate workspace type constant WORKSPACE_RESOURCE_TYPE is hard-coded to the literal string "workspace", but Constants.WORKSPACE_TYPE comments say the value "must equal ResourceAccessHandler.WORKSPACE_RESOURCE_TYPE". Two independent literals will silently drift; if a plugin registers a differently named workspace type or if either constant is renamed, container fan-out breaks with no compile-time signal. Expose one canonical constant and reference it from both places. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [264-265] final List<String> workspaceIds = new ArrayList<>(sharingInfo.getWorkspaces()); final String workspaceIndex = workspaceIds.isEmpty() ? null : resourcePluginInfo.indexByType(WORKSPACE_RESOURCE_TYPE); +// WORKSPACE_RESOURCE_TYPE is the single source of truth; sample plugin's Constants.WORKSPACE_TYPE must reference it. -// Evaluate workspaces (batched) first; fall back to the single parent (recursive) only if no workspace grants. -if (workspaceIndex != null) { - resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { - for (ResourceSharing wsRecord : records.values()) { - Suggestion importance[1-10]: 5 __ Why: Duplicating the "workspace" literal in two independent constants creates a real drift risk noted in the code comment itself; consolidating to a single canonical constant is a valid maintainability improvement.	Low

Suggestions up to commit 81f704c

Category	Suggestion	Impact
Possible issue	Version-guard new wire fields on both sides ResourceSharing is a NamedWriteable exchanged over the transport wire (e.g. in ShareResponse). Adding two trailing fields (workspaces, workspacesSeqNo) with no version guard breaks mixed-version clusters: a new-version writer sending to an older-version reader will have the older reader's stream constructor stop after shareWith, leaving these bytes in the stream and corrupting subsequent reads. Guard both write and read with out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) using the first unreleased version constant that includes this change, so peers before(V) neither read nor write the new fields. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [305-310] -// 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)); -out.writeZLong(workspacesSeqNo); +if (out.getVersion().onOrAfter(Version.V_3_3_0)) { + out.writeOptionalStringCollection(workspaces == null ? null : new ArrayList<>(workspaces)); + out.writeZLong(workspacesSeqNo); +} Suggestion importance[1-10]: 7 __ Why: The author's inline comment acknowledges this decision, arguing the feature is not GA so no old node exists. If accurate, the suggestion is unnecessary; but if BWC across minor versions is expected, appending unguarded fields to a NamedWriteable is a legitimate risk. Moderate impact due to disputed premise.	Medium
	Version-guard added StreamOutput field SampleResource is a NamedWriteable; appending workspaces to the wire format without a version guard breaks mixed-version transport: a new writer sending to an old reader will leave these bytes unread and corrupt subsequent stream data. Guard both writeTo and the StreamInput constructor symmetrically with out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) using the first unreleased version constant. sample-resource-plugin/src/main/java/org/opensearch/sample/SampleResource.java [114-122] public void writeTo(StreamOutput out) throws IOException { out.writeString(name); out.writeOptionalString(description); out.writeOptionalString(groupId); out.writeMap(attributes, StreamOutput::writeString, StreamOutput::writeString); user.writeTo(out); - // Symmetric with the StreamInput ctor. Passing null when unset keeps mixed-caller compatibility. - out.writeOptionalStringCollection(workspaces); + if (out.getVersion().onOrAfter(Version.V_3_3_0)) { + out.writeOptionalStringCollection(workspaces); + } } Suggestion importance[1-10]: 3 __ Why: This is a sample plugin used mostly for integration tests, so the mixed-version concern is less impactful here than in production code. The suggestion is valid in principle but of low importance in this file.	Low
General	Handle unassigned seq_no in guard comparison Using <= against UNASSIGNED_SEQ_NO (which is -2) means a legitimate sourceSeqNo of 0 (the very first primary write on a fresh shard) will pass, but if a record was ever stamped with UNASSIGNED_SEQ_NO explicitly, 0 <= -2 is false and reconcile proceeds — fine. However, if two concurrent reconciles arrive with the same sourceSeqNo (e.g. retries after a version conflict), the second is rejected as stale even though it targets the same write. Consider < when sourceSeqNo equals a real stored seq_no to allow idempotent retries, or clearly document why equal is treated as stale. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [230-237] -Map<String, Object> source = getResponse.getSourceAsMap(); -long storedSeqNo = source.get(WORKSPACES_SEQ_NO_FIELD) instanceof Number n - ? n.longValue() - : SequenceNumbers.UNASSIGNED_SEQ_NO; -// Monotonic guard: an older source operation must never overwrite state written by a newer one. -if (sourceSeqNo <= storedSeqNo) { +if (storedSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO && sourceSeqNo <= storedSeqNo) { listener.onResponse(false); return; } Suggestion importance[1-10]: 3 __ Why: The suggestion's own reasoning admits the current code handles the UNASSIGNED_SEQ_NO case correctly (0 > -2 passes). The proposed "improvement" doesn't materially change behavior since UNASSIGNED_SEQ_NO is negative. Low impact.	Low
	Prevent self-reference in container fan-out If a workspace record itself contains workspaces (malformed or intentional nesting), the leaf-evaluation path is fine, but if checkContainers is ever entered via workspace-record fetching in future refactors, this could recurse. More immediately, sharingInfo.getWorkspaces() may include the resource's own id if a caller misconfigures self-reference; consider filtering out the resource's own id from workspaceIds to avoid a self-fetch on the workspace index that would either miss or, worse, evaluate the resource itself as its own container. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [216-224] -private void checkContainers(ResourceSharing sharingInfo, String action, ActionListener<Boolean> listener) { - final User user = getAuthenticatedUser(); - if (user == null) { - listener.onResponse(false); - return; - } +final List<String> workspaceIds = new ArrayList<>(sharingInfo.getWorkspaces()); +workspaceIds.remove(sharingInfo.getResourceId()); +final String workspaceIndex = workspaceIds.isEmpty() ? null : resourcePluginInfo.indexByType(WORKSPACE_RESOURCE_TYPE); - final List<String> workspaceIds = new ArrayList<>(sharingInfo.getWorkspaces()); - final String workspaceIndex = workspaceIds.isEmpty() ? null : resourcePluginInfo.indexByType(WORKSPACE_RESOURCE_TYPE); - Suggestion importance[1-10]: 3 __ Why: The self-reference scenario is speculative and the current leaf-evaluation approach (no recursion into workspace records) already prevents infinite loops, as tested in testHasPermission_workspaceIsLeafEvaluatedNoRecursion. Marginal defensive improvement.	Low

Suggestions up to commit 7c5cf51

Category	Suggestion	Impact
General	Return validated workspaces field deterministically The javadoc claims conflicts are rejected at registration so the value is unambiguous, but setResourceSharingExtensions only tracks the first non-null field per index in indexToWorkspacesField; a provider that declares null after a non-null provider is accepted (correct), yet a provider that declares non-null after a null-only provider is also accepted without validation because indexToWorkspacesField was never populated. That's fine, but this lookup then depends on typeToProvider.values() iteration order — which is a HashMap. Switch to LinkedHashMap or explicitly return the field stored in indexToWorkspacesField to guarantee determinism. src/main/java/org/opensearch/security/resources/ResourcePluginInfo.java [382-394] public String workspacesFieldForIndex(String index) { lock.readLock().lock(); try { - for (ResourceProvider provider : typeToProvider.values()) { - if (provider.resourceIndexName().equals(index) && provider.workspacesField() != null) { - return provider.workspacesField(); - } - } - return null; + return indexToWorkspacesField.get(index); } finally { lock.readLock().unlock(); } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a determinism issue with iterating over a HashMap's values. The indexToWorkspacesField map is local to setResourceSharingExtensions and not a field, so the exact improved_code won't compile as-is, but the underlying concern about determinism is valid.	Low
	Reject updates to non-existent resources getResponse.getSource() is the deprecated/renamed API; more importantly, an update on a non-existent resource ID will silently succeed as an index create with no workspaces preserved (since existingWorkspaces stays empty and gets set to null). Consider failing the update when the source doc does not exist, so callers cannot create workspace-less resources via the update path — otherwise this is a subtle way to bypass the trusted-write contract for workspaces. sample-resource-plugin/src/main/java/org/opensearch/sample/resource/actions/transport/UpdateResourceTransportAction.java [73-78] pluginClient.get(new GetRequest(RESOURCE_INDEX_NAME, resourceId), ActionListener.wrap(getResponse -> { try { + if (!getResponse.isExists()) { + listener.onFailure(new IllegalArgumentException("Resource " + resourceId + " does not exist")); + return; + } Set<String> existingWorkspaces = new HashSet<>(); - if (getResponse.isExists() - && getResponse.getSource() != null + if (getResponse.getSource() != null && getResponse.getSource().get("workspaces") instanceof Collection<?> c) { Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a subtle edge case where an update on a non-existent resource could bypass the trusted-write contract. However, this is in a sample plugin (test fixture), so impact is limited.	Low
	Clarify workspace index variable naming indexByType(WORKSPACE_RESOURCE_TYPE) returns the resource index (e.g. .sample_resource), but fetchSharingInfoForIds internally calls getSharingIndex(resourceIndex) which appends -sharing. If a caller passes the sharing index name here it will be double-suffixed. Confirm the argument passed to fetchSharingInfoForIds is the source resource index (not the sharing index) — the current code appears correct, but the naming workspaceIndex is ambiguous and should be renamed to workspaceResourceIndex to avoid future regressions. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [223-224] final List<String> workspaceIds = new ArrayList<>(sharingInfo.getWorkspaces()); -final String workspaceIndex = workspaceIds.isEmpty() ? null : resourcePluginInfo.indexByType(WORKSPACE_RESOURCE_TYPE); +final String workspaceResourceIndex = workspaceIds.isEmpty() ? null : resourcePluginInfo.indexByType(WORKSPACE_RESOURCE_TYPE); Suggestion importance[1-10]: 2 __ Why: This is a minor naming/readability suggestion. The current code is correct, and the rename provides marginal clarity improvement.	Low
Possible issue	Version-guard new wire fields on both sides The new workspaces and workspacesSeqNo fields are appended to the wire format of ResourceSharing without a getVersion() guard. If any node running a prior build of this feature branch (before these two fields existed) is on the wire — e.g. during rolling upgrade within pre-GA — the old reader will not consume these trailing bytes and will corrupt the stream. Guard both the write and the symmetric read with out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) using the first unreleased version that contains this change. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [305-310] -// 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)); -out.writeZLong(workspacesSeqNo); +if (out.getVersion().onOrAfter(WORKSPACES_ADDED_VERSION)) { + out.writeOptionalStringCollection(workspaces == null ? null : new ArrayList<>(workspaces)); + out.writeZLong(workspacesSeqNo); +} Suggestion importance[1-10]: 4 __ Why: The suggestion raises a legitimate concern about wire compatibility, but the PR author explicitly documents that the resource-sharing feature is pre-GA and no older node speaks the old wire format. The concern may still be valid for rolling upgrades within pre-GA branches, but the impact is limited.	Low

Suggestions up to commit eca8bdf

Category	Suggestion	Impact
Possible issue	Prevent workspace revert via optimistic concurrency There is a TOCTOU race between the GET-then-INDEX: a concurrent authorized associate/dissociate can land between the GET and the subsequent full-document IndexRequest, which then overwrites workspaces back to the value seen at read-time. Use optimistic concurrency by passing the GET response's seq_no/primary_term on the IndexRequest so a concurrent membership change causes a version conflict rather than a silent revert. sample-resource-plugin/src/main/java/org/opensearch/sample/resource/actions/transport/UpdateResourceTransportAction.java [73-85] pluginClient.get(new GetRequest(RESOURCE_INDEX_NAME, resourceId), ActionListener.wrap(getResponse -> { try { Set<String> existingWorkspaces = new HashSet<>(); if (getResponse.isExists() && getResponse.getSource() != null && getResponse.getSource().get("workspaces") instanceof Collection<?> c) { for (Object o : c) { if (o != null) { existingWorkspaces.add(o.toString()); } } } sample.setWorkspaces(existingWorkspaces.isEmpty() ? null : existingWorkspaces); + final long seqNo = getResponse.getSeqNo(); + final long primaryTerm = getResponse.getPrimaryTerm(); + // ... then set ir.setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm) Suggestion importance[1-10]: 7 __ Why: Correctly identifies a genuine TOCTOU race between GET and IndexRequest where a concurrent authorized workspace membership change could be silently overwritten; using seq_no/primary_term optimistic concurrency is a valid fix for the sample plugin's trusted-write contract.	Medium
	Reject unassigned seq_no in monotonic guard sourceSeqNo <= storedSeqNo will reject a valid initial reconcile when the source op's seq_no is 0 and storedSeqNo defaults to SequenceNumbers.UNASSIGNED_SEQ_NO (-2), which is correct, but it also rejects sourceSeqNo == 0 after any earlier reconcile stored 0. More critically, when sourceSeqNo == UNASSIGNED_SEQ_NO (e.g. certain replica/retry paths) this comparison silently applies as newer. Guard against unassigned sourceSeqNo explicitly to avoid corrupting the monotonic invariant. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [183-192] public void reconcileWorkspaces( String resourceIndex, String resourceId, Set<String> workspaces, long sourceSeqNo, ActionListener<Boolean> listener ) { + if (sourceSeqNo == SequenceNumbers.UNASSIGNED_SEQ_NO) { + listener.onResponse(false); + return; + } Set<String> target = workspaces == null ? Set.of() : new HashSet<>(workspaces); reconcileWorkspacesAttempt(getSharingIndex(resourceIndex), resourceId, target, sourceSeqNo, 1, listener); } Suggestion importance[1-10]: 6 __ Why: Guarding against UNASSIGNED_SEQ_NO in the monotonic guard is a reasonable defensive check to prevent stale/replica ops from corrupting the invariant, though its practical impact depends on how reconcileWorkspaces is invoked in existing callers.	Low
	Capture user before async callback getAuthenticatedUser() is called once at the start of checkContainers, but the async mget callback runs on a different thread where the threadContext may not carry the same user. Capture the user explicitly at the callsite in hasPermission and pass it down, or ensure the listener restores the caller's context; otherwise recordGrantsAction on the workspace records may be evaluated against a null/different user. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [227-239] if (workspaceIndex != null) { + final User capturedUser = user; resourceSharingIndexHandler.fetchSharingInfoForIds(workspaceIndex, workspaceIds, ActionListener.wrap(records -> { for (ResourceSharing wsRecord : records.values()) { -... - if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, user, action)) { + if (recordGrantsAction(wsRecord, WORKSPACE_RESOURCE_TYPE, capturedUser, action)) { listener.onResponse(true); return; } } checkParent(sharingInfo, action, listener); }, listener::onFailure)); Suggestion importance[1-10]: 3 __ Why: The user is already captured as a final local variable before the async call, so passing user in the lambda closure already works; the suggestion offers minor stylistic clarity rather than a real fix.	Low
General	Deduplicate seeded principals on creation sharingInfo.getAllPrincipals() may include the literal string "public" when share_with.isPublic() is true. Stamping "public" into all_shared_principals here is intentional per the existing DLS query, but ensure that duplicates and ordering are consistent with prior behavior; previously only the creator was seeded, so downstream logic that expects an initial single-principal set could regress. Consider deduplicating and verifying that seeding all recipients at creation time matches the intended write-path semantics. src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java [386-401] ActionListener<IndexResponse> irListener = ActionListener.wrap(idxResponse -> { ctx.restore(); LOGGER.info("Successfully created {} entry for resource {} in index {}.", resourceSharingIndex, resourceId, resourceIndex); - // Seed all_shared_principals from getAllPrincipals() (creator + any share recipients); fall back to - // the creator when empty. - List<String> initialPrincipals = new ArrayList<>(sharingInfo.getAllPrincipals()); + List<String> initialPrincipals = new ArrayList<>(new java.util.LinkedHashSet<>(sharingInfo.getAllPrincipals())); if (initialPrincipals.isEmpty()) { initialPrincipals.add("user:" + createdBy.getUsername()); } Suggestion importance[1-10]: 3 __ Why: getAllPrincipals() already returns a List built with reasonable semantics; deduplication is a minor defensive nicety and the suggestion mostly asks the author to verify behavior rather than fixing a concrete bug.	Low

Suggestions up to commit 1577069

Category	Suggestion	Impact
Possible issue	Version-guard new wire-format field symmetrically Adding an unguarded field to ResourceSharing's wire format changes the byte layout for all peers. If any node running a prior build of the resource-sharing feature exists in a mixed-version cluster (e.g. rolling upgrade from an earlier pre-GA snapshot), the new writer -> old reader and old writer -> new reader paths will fail because the trailing writeOptionalStringCollection byte is absent/extra. Add a Version guard using out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) on both sides with the same constant V of the first release containing this change. src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java [290-294] -// 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)); +if (out.getVersion().onOrAfter(Version.V_3_3_0)) { + out.writeOptionalStringCollection(workspaces == null ? null : new ArrayList<>(workspaces)); +} Suggestion importance[1-10]: 5 __ Why: The PR author explicitly commented that no version guard is needed because the feature is pre-GA. While version guards are generally best practice for wire format changes, the suggestion may not apply given the stated context.	Low
	Use consistent user reference in async check The user is re-fetched via getAuthenticatedUser() inside checkContainers, but the outer hasPermission uses this.user captured in the constructor/context. If the thread context has been stashed/restored differently between the initial call and the async callback, getAuthenticatedUser() may return null and deny incorrectly. Reuse the same user reference used in recordGrantsAction (pass it in) to keep authorization consistent across the async chain. src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java [217-223] private void checkContainers(ResourceSharing sharingInfo, String action, ActionListener<Boolean> listener) { - final User user = getAuthenticatedUser(); + final User user = this.user; if (user == null) { listener.onResponse(false); return; } final List<String> workspaceIds = new ArrayList<>(sharingInfo.getWorkspaces()); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a plausible concern about async user context consistency, but without visibility into how this.user is defined in the class, it's speculative. The improved_code is nearly identical to existing_code, only changing the source of user.	Low
General	Ensure latch decrement on all failure paths The reconcile is only invoked when entry == null (existing record path). If the outer indexResourceSharing call fails synchronously in an unexpected way, the latch may not be counted down for that document, causing migrationStatsLatch.await() to block forever. Ensure the outer catch (Exception e) also decrements the latch for every submitted doc. src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java [430-441] +ActionListener.wrap(changed -> { + if (Boolean.TRUE.equals(changed)) { + backfilledExisting.getAndIncrement(); + } else { + skippedExisting.getAndIncrement(); + } + migrationStatsLatch.countDown(); +}, e -> { + LOGGER.warn("Failed to reconcile workspaces for existing record [{}]: {}", resourceId, e.getMessage()); + failureCount.getAndIncrement(); + migrationStatsLatch.countDown(); +}) - Suggestion importance[1-10]: 3 __ Why: The concern about latch not being decremented on synchronous exceptions is valid, but the improved_code is identical to existing_code, providing no actual fix. The suggestion text mentions the outer catch block, but no change is shown there.	Low

[github-actions]

Persistent review updated to latest commit 7412d76

[codecov]

Codecov Report

❌ Patch coverage is 77.57009% with 96 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.93%. Comparing base (a9930af) to head (649432f).

Files with missing lines	Patch %	Lines
...ecurity/resources/ResourceSharingIndexHandler.java	73.07%	34 Missing and 15 partials ⚠️
...ch/security/resources/sharing/ResourceSharing.java	78.04%	6 Missing and 3 partials ⚠️
...i/migrate/MigrateResourceSharingInfoApiAction.java	82.50%	4 Missing and 3 partials ⚠️
...tions/transport/UpdateResourceTransportAction.java	77.77%	3 Missing and 3 partials ⚠️
...ain/java/org/opensearch/sample/SampleResource.java	66.66%	4 Missing and 1 partial ⚠️
...nsearch/security/resources/ResourcePluginInfo.java	85.71%	0 Missing and 5 partials ⚠️
...ch/security/resources/ResourceSharingDlsUtils.java	71.42%	1 Missing and 3 partials ⚠️
.../actions/rest/create/CreateResourceRestAction.java	72.72%	1 Missing and 2 partials ⚠️
...arch/security/resources/ResourceAccessHandler.java	90.62%	2 Missing and 1 partial ⚠️
...arch/security/resources/ResourceIndexListener.java	78.57%	0 Missing and 3 partials ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6374      +/-   ##
==========================================
+ Coverage   75.92%   75.93%   +0.01%     
==========================================
  Files         459      461       +2     
  Lines       30591    30892     +301     
  Branches     4591     4657      +66     
==========================================
+ Hits        23226    23458     +232     
- Misses       5251     5280      +29     
- Partials     2114     2154      +40     

Files with missing lines	Coverage Δ
...rg/opensearch/sample/SampleWorkspaceExtension.java	100.00% <100.00%> (ø)
...in/java/org/opensearch/sample/utils/Constants.java	0.00% <ø> (ø)
...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.12% <ø> (-0.10%)	⬇️
...search/security/configuration/DlsFlsValveImpl.java	72.75% <ø> (ø)
...org/opensearch/sample/SampleResourceExtension.java	87.50% <71.42%> (-12.50%)	⬇️
.../actions/rest/create/CreateResourceRestAction.java	77.27% <72.72%> (-1.52%)	⬇️
...arch/security/resources/ResourceAccessHandler.java	76.28% <90.62%> (+1.99%)	⬆️
...arch/security/resources/ResourceIndexListener.java	87.23% <78.57%> (-2.06%)	⬇️
... and 7 more

... and 7 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.

[cwperks]

If I'm reading this correctly, the migrate API can now be called multiple times? One from a version before this PR and one afterwards to get the workspace info?

[cwperks]

@DarshitChanpura With multi-tenancy, with a request destined for a tenant there is a securitytenant header. What is the equivalent with workspaces? If a request is for a particular workspace then there would only be a single workspace to filter by for security.

[DarshitChanpura]

Deliberately no securitytenant-style header for workspaces — access is membership-scoped, not request-scoped.

The models differ:

• securitytenant works as a request selector because a saved object lives in exactly one tenant, and the tenant is the security boundary — the header picks which tenant's index you operate in.
• A workspace here is a container a resource belongs to, and a resource can be in multiple workspaces (1:N). The security boundary is the user's membership across all their accessible workspaces, resolved server-side via the trusted resolveWorkspacesForUser — not something the request asserts.

So on the read path, DLS filters the resource's workspaces field against the user's whole accessible-workspace set (a terms over the set), not a single header-selected workspace.

A "current workspace" (the workspace the UI is focused on) is the closest analogue to securitytenant, but it's a UX/narrowing concern, not a security one: such a header could only narrow results to that workspace (intersected with membership) — it can never grant, since membership is the enforced floor. DLS already works with no header, so it's an optional fast-follow if the UI wants server-side scoping to the active workspace.

Worth flagging: because membership is 1:N (unlike 1:1 tenant), "a single workspace to filter by" isn't quite the model even when a request targets one workspace — security still authorizes against the user's full membership; a single-workspace header would be an extra filter on top.

[cwperks]

But for requests like GET Report Definition, how does that request ferry along which workspace the user is toggled to in dashboards to the backend?

[DarshitChanpura]

Thanks @cwperks — addressed in e2f4225:

• ResourceAccessHandler ("overly complicated", cycle can't happen): agreed. Dropped the ancestor cycle guard entirely — workspaces are leaf-evaluated (never recursed into), and parent inheritance is the single chain the pre-existing code already recursed without a guard, so the visited-set + path-scoping was guarding a case that can't occur. hasPermission is back to one method; the container fallback is just "any workspace grants (batched mget) OR the single parent grants (recursion)".
  • Re your DM about ResourceAccessEvaluator: that file is unchanged in this PR (no diff) — the write-path changes are all in ResourceAccessHandler, and should read much simpler now. Happy to walk through it.
• ResourceProvider.workspacesField() default: now defaults to "workspaces"; a doc without the field just belongs to no workspace. Removed the redundant override in the sample plugin.
• ResourceProvider javadoc (still accurate?): no — updated. It described the old all_shared_principals denormalization, which is gone; it now describes storing on the sharing record (write path) + DLS filtering the field (read path).
• Comment nits (MigrateApiTests, ResourceSharing): trimmed; removed references to prior revisions.
• Migrate called multiple times (pre/post this PR)? Yes — that's intended and safe. backfillWorkspacesOnExisting is idempotent: re-running merges workspace IDs onto the existing record and only writes when something's new, so an old (pre-PR) migrate followed by a new one picks up workspace info without duplicating or clobbering created_by/share_with.
• Workspace request-scoping header (like securitytenant): good question — this is the one open design item. There's no workspace equivalent wired yet; membership comes from the trusted resolveWorkspacesForUser SPI (resolved from server-set identity, not a request header). If workspaces should be request-scoped (a "current workspace" header, analogous to tenant), that's a distinct mechanism we'd define with the OpenSearch-side workspace backend (§11 in the design doc). I'll capture it there as an open question — want it request-scoped, membership-scoped, or both?

[github-actions]

Persistent review updated to latest commit e2f4225

[DarshitChanpura]

On request-scoping (your securitytenant question) — recommendation: membership-scoped as the enforced authorization, request-scoping only as an optional narrowing filter later (not instead).

• Membership = what you're allowed to see (authz); a "current workspace" header = what you want to see now (UX). A request header must only ever narrow, never widen — otherwise omitting/forging it could over-share.
• The securitytenant analogy breaks: tenants are disjoint + single-active, whereas workspaces are multi-membership and non-disjoint (a resource can be in several). Your "realm" framing is itself membership-based.
• Backend enforcement must hold for direct .kibana/API calls that carry no OSD workspace context, so membership has to be the floor regardless.

No code change needed — the current DLS already filters the workspaces field against resolveWorkspacesForUser (membership), with no request header. If the UI later needs a single-workspace view, we add a request filter that intersects with membership — tracked in design doc §11.5(4) as a fast-follow with the OpenSearch-side backend.

[github-actions]

Persistent review updated to latest commit d86d9c6

[github-actions]

Persistent review updated to latest commit 725dfb3

[github-actions]

Persistent review updated to latest commit 361ec6b

[github-actions]

Persistent review updated to latest commit 1577069

[github-actions]

Persistent review updated to latest commit eca8bdf

[github-actions]

Persistent review updated to latest commit 7c5cf51

[github-actions]

Persistent review updated to latest commit 81f704c

[github-actions]

Persistent review updated to latest commit 8f62005

[github-actions]

Persistent review updated to latest commit 649432f

[cwperks]

I think we need to figure this out. With multi-tenancy, we set user.getRequestedTenant to know which tenant a request is destined for.

[cwperks]

I think security-dashboards-plugin carries the tenant info via https://github.com/opensearch-project/security-dashboards-plugin/blob/d4db9d33dde68e8c62565bfbd7bea66e980399eb/server/auth/types/authentication_type.ts#L175-L193