From a21bb74fb02f8bed083e31b2568f3753888e9fa4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Mar 2026 14:03:15 -0400 Subject: [PATCH 1/7] Add general_access field on sharing doc to store a single access level for which resource is shared generally Signed-off-by: Craig Perkins --- .../opensearch/sample/resource/TestUtils.java | 60 +++++++-- .../multi_share/PubliclySharedDocTests.java | 123 +++++++++++++----- .../resources/ResourceAccessHandler.java | 2 +- .../ResourceSharingIndexHandler.java | 8 +- .../resources/sharing/Recipients.java | 14 +- .../resources/sharing/ResourceSharing.java | 42 +++--- .../security/resources/sharing/ShareWith.java | 58 +++++++-- 7 files changed, 218 insertions(+), 89 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java index 242fd238ad..4329c8a310 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java @@ -263,11 +263,25 @@ public static String putSharingInfoPayload( """.formatted(resourceId, resourceType, accessLevel, recipient.getName(), entity); } + public static String putGeneralAccessPayload(String resourceId, String resourceType, String accessLevel) { + return """ + { + "resource_id": "%s", + "resource_type": "%s", + "share_with": { + "general_access": "%s" + } + } + """.formatted(resourceId, resourceType, accessLevel); + } + public static class PatchSharingInfoPayloadBuilder { private String resourceId; private String resourceType; private final Map share = new HashMap<>(); private final Map revoke = new HashMap<>(); + private String setGeneralAccess; + private String revokeGeneralAccess; public PatchSharingInfoPayloadBuilder resourceId(String resourceId) { this.resourceId = resourceId; @@ -287,15 +301,19 @@ public void share(Recipients recipients, String accessLevel) { public void revoke(Recipients recipients, String accessLevel) { Recipients existing = revoke.getOrDefault(accessLevel, new Recipients(new HashMap<>())); - // intentionally share() is called here since we are building a shareWith object, this final object will be used to remove - // access - // think of it as currentShareWith.removeAll(revokeShareWith) existing.share(recipients); revoke.put(accessLevel, existing); } - private String buildJsonString(Map input) { + public void setGeneralAccess(String accessLevel) { + this.setGeneralAccess = accessLevel; + } + public void revokeGeneralAccess(String accessLevel) { + this.revokeGeneralAccess = accessLevel; + } + + private String buildJsonString(Map input) { List output = new ArrayList<>(); for (Map.Entry entry : input.entrySet()) { try { @@ -308,16 +326,22 @@ private String buildJsonString(Map input) { } catch (IOException e) { throw new RuntimeException(e); } - } - return String.join(",", output); - } public String build() { - String allShares = buildJsonString(share); - String allRevokes = buildJsonString(revoke); + String addGeneralAccess = setGeneralAccess != null ? "\"general_access\": \"" + setGeneralAccess + "\"" : ""; + String addNamedRecipients = buildJsonString(share); + String addSection = String.join(",", List.of(addGeneralAccess, addNamedRecipients).stream().filter(s -> !s.isBlank()).toList()); + + String revokeGeneralAccessStr = revokeGeneralAccess != null ? "\"general_access\": \"" + revokeGeneralAccess + "\"" : ""; + String revokeNamedRecipients = buildJsonString(revoke); + String revokeSection = String.join( + ",", + List.of(revokeGeneralAccessStr, revokeNamedRecipients).stream().filter(s -> !s.isBlank()).toList() + ); + return """ { "resource_id": "%s", @@ -329,7 +353,7 @@ public String build() { %s } } - """.formatted(resourceId, resourceType, allShares, allRevokes); + """.formatted(resourceId, resourceType, addSection, revokeSection); } } @@ -588,6 +612,22 @@ public TestRestClient.HttpResponse shareResource( } } + public TestRestClient.HttpResponse shareResourceGenerally(String resourceId, TestSecurityConfig.User user, String accessLevel) { + try (TestRestClient client = cluster.getRestClient(user)) { + return client.putJson(SECURITY_SHARE_ENDPOINT, putGeneralAccessPayload(resourceId, RESOURCE_TYPE, accessLevel)); + } + } + + public TestRestClient.HttpResponse revokeGeneralAccess(String resourceId, TestSecurityConfig.User user, String accessLevel) { + PatchSharingInfoPayloadBuilder patchBuilder = new PatchSharingInfoPayloadBuilder(); + patchBuilder.resourceType(RESOURCE_TYPE); + patchBuilder.resourceId(resourceId); + patchBuilder.revokeGeneralAccess(accessLevel); + try (TestRestClient client = cluster.getRestClient(user)) { + return client.patch(SECURITY_SHARE_ENDPOINT, patchBuilder.build()); + } + } + public TestRestClient.HttpResponse shareResourceGroup( String resourceId, TestSecurityConfig.User user, diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java index 26dc6ca2f5..140063e3b1 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java @@ -17,14 +17,15 @@ import org.junit.runner.RunWith; import org.opensearch.sample.resource.TestUtils; -import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.opensearch.sample.resource.TestUtils.FULL_ACCESS_USER; import static org.opensearch.sample.resource.TestUtils.LIMITED_ACCESS_USER; +import static org.opensearch.sample.resource.TestUtils.NO_ACCESS_USER; import static org.opensearch.sample.resource.TestUtils.SAMPLE_FULL_ACCESS; import static org.opensearch.sample.resource.TestUtils.SAMPLE_READ_ONLY; import static org.opensearch.sample.resource.TestUtils.newCluster; @@ -33,8 +34,9 @@ import static org.opensearch.test.framework.TestSecurityConfig.User.USER_ADMIN; /** - * Test resource access to a publicly shared resource at different access-levels. - * All tests are against USER_ADMIN's resource created during setup. + * Tests for general_access (public) sharing at a specific access level. + * Verifies that general_access grants everyone the specified level, + * while named recipients can hold higher levels independently. */ @RunWith(RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -49,7 +51,7 @@ public class PubliclySharedDocTests { @Before public void setup() { resourceId = api.createSampleResourceAs(USER_ADMIN); - api.awaitSharingEntry(resourceId); // wait until sharing entry is created + api.awaitSharingEntry(resourceId); } @After @@ -57,52 +59,109 @@ public void cleanup() { api.wipeOutResourceEntries(); } - private void assertNoAccessBeforeSharing(TestSecurityConfig.User user) throws Exception { - forbidden(() -> api.getResource(resourceId, user)); - forbidden(() -> api.updateResource(resourceId, user, "sampleUpdateAdmin")); - forbidden(() -> api.deleteResource(resourceId, user)); + @Test + public void generalAccess_readOnly_grantsEveryoneReadAccess() throws Exception { + // no access before sharing + forbidden(() -> api.getResource(resourceId, FULL_ACCESS_USER)); + forbidden(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); + forbidden(() -> api.getResource(resourceId, NO_ACCESS_USER)); - forbidden(() -> api.shareResource(resourceId, user, user, SAMPLE_FULL_ACCESS)); - forbidden(() -> api.revokeResource(resourceId, user, user, SAMPLE_FULL_ACCESS)); - } + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); - private void assertReadOnly() throws Exception { + // everyone can read TestRestClient.HttpResponse response = ok(() -> api.getResource(resourceId, FULL_ACCESS_USER)); assertThat(response.getBody(), containsString("sample")); - forbidden(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "sampleUpdateAdmin")); - forbidden(() -> api.deleteResource(resourceId, FULL_ACCESS_USER)); - forbidden(() -> api.shareResource(resourceId, FULL_ACCESS_USER, FULL_ACCESS_USER, SAMPLE_FULL_ACCESS)); - forbidden(() -> api.revokeResource(resourceId, FULL_ACCESS_USER, FULL_ACCESS_USER, SAMPLE_FULL_ACCESS)); + response = ok(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); + assertThat(response.getBody(), containsString("sample")); + + // but no one can write + forbidden(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "updated")); + forbidden(() -> api.deleteResource(resourceId, FULL_ACCESS_USER)); } - private void assertFullAccess() throws Exception { + @Test + public void generalAccess_fullAccess_grantsEveryoneFullAccess() throws Exception { + forbidden(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); + + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_FULL_ACCESS)); + TestRestClient.HttpResponse response = ok(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); assertThat(response.getBody(), containsString("sample")); - ok(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "sampleUpdateAdmin")); - ok(() -> api.shareResource(resourceId, LIMITED_ACCESS_USER, TestUtils.LIMITED_ACCESS_USER, SAMPLE_FULL_ACCESS)); - ok(() -> api.revokeResource(resourceId, LIMITED_ACCESS_USER, USER_ADMIN, SAMPLE_FULL_ACCESS)); + + ok(() -> api.updateResource(resourceId, LIMITED_ACCESS_USER, "updated")); ok(() -> api.deleteResource(resourceId, LIMITED_ACCESS_USER)); } @Test - public void readOnly() throws Exception { - assertNoAccessBeforeSharing(FULL_ACCESS_USER); - // 1. share at read-only for full-access user and at full-access for limited perms user - ok(() -> api.shareResource(resourceId, USER_ADMIN, new TestSecurityConfig.User("*"), SAMPLE_READ_ONLY)); + public void generalAccess_readOnly_namedRecipientCanWrite() throws Exception { + // share publicly at read-only, but grant FULL_ACCESS_USER write access explicitly + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + ok(() -> api.shareResource(resourceId, USER_ADMIN, FULL_ACCESS_USER, SAMPLE_FULL_ACCESS)); + + // everyone can read + TestRestClient.HttpResponse response = ok(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); + assertThat(response.getBody(), containsString("sample")); + + // only FULL_ACCESS_USER can write + ok(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "updated")); + forbidden(() -> api.updateResource(resourceId, LIMITED_ACCESS_USER, "updated")); + forbidden(() -> api.deleteResource(resourceId, LIMITED_ACCESS_USER)); + } + + @Test + public void revokeGeneralAccess_removesPublicAccess() throws Exception { + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + + // confirm access granted + ok(() -> api.getResource(resourceId, FULL_ACCESS_USER)); - // 2. check read-only access for full-access user - assertReadOnly(); + // revoke general access + ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + + // access should be gone + forbidden(() -> api.getResource(resourceId, FULL_ACCESS_USER)); } @Test - public void fullAccess() throws Exception { - assertNoAccessBeforeSharing(LIMITED_ACCESS_USER); - // 1. share at read-only for full-access user and at full-access for limited perms user - ok(() -> api.shareResource(resourceId, USER_ADMIN, new TestSecurityConfig.User("*"), SAMPLE_FULL_ACCESS)); + public void generalAccess_doesNotLeakSharingInfo() throws Exception { + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); - // 2. check read-only access for full-access user - assertFullAccess(); + // a user with only general read access cannot view or modify sharing info + forbidden(() -> api.shareResource(resourceId, FULL_ACCESS_USER, FULL_ACCESS_USER, SAMPLE_FULL_ACCESS)); + forbidden(() -> api.revokeGeneralAccess(resourceId, FULL_ACCESS_USER, SAMPLE_READ_ONLY)); } + @Test + public void generalAccess_upgradeLevel_replacesExistingGeneralAccess() throws Exception { + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + forbidden(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "updated")); + + // upgrade general access to full + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_FULL_ACCESS)); + + TestRestClient.HttpResponse response = ok(() -> api.getResource(resourceId, FULL_ACCESS_USER)); + assertThat(response.getBody(), containsString("sample")); + ok(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "updated")); + } + + @Test + public void generalAccess_sharingInfoResponse_containsGeneralAccessField() throws Exception { + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + + try (var client = cluster.getRestClient(USER_ADMIN)) { + TestRestClient.HttpResponse response = ok( + () -> client.get( + TestUtils.SECURITY_SHARE_ENDPOINT + + "?resource_id=" + + resourceId + + "&resource_type=" + + org.opensearch.sample.utils.Constants.RESOURCE_TYPE + ) + ); + assertThat(response.getBody(), containsString("general_access")); + assertThat(response.getBody(), containsString(SAMPLE_READ_ONLY)); + assertThat(response.getBody(), not(containsString("\"users\""))); + } + } } diff --git a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java index ce4634e55e..a47347e9c9 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java @@ -384,7 +384,7 @@ private Set getFlatPrincipals(User user) { // for users: Set users = new HashSet<>(); users.add(user.getName()); - users.add("*"); // for matching against publicly shared resource + users.add("public"); // for matching against publicly shared resources // return flattened principals to build the bool query return Stream.concat( diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index 77ea71eafc..87f546d668 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -649,9 +649,11 @@ public void share(String resourceId, String resourceIndex, ShareWith shareWith, } for (String accessLevel : shareWith.accessLevels()) { Recipients target = shareWith.atAccessLevel(accessLevel); - sharingInfo.share(accessLevel, target); } + if (shareWith.getGeneralAccess() != null) { + sharingInfo.setGeneralAccess(shareWith.getGeneralAccess()); + } String resourceSharingIndex = getSharingIndex(resourceIndex); try (ThreadContext.StoredContext ctx = threadPool.getThreadContext().stashContext()) { @@ -719,10 +721,10 @@ public void patchSharingInfo( // Apply patch and update the document sharingInfoListener.whenComplete(sharingInfo -> { if (add != null) { - sharingInfo.getShareWith().add(add); + sharingInfo.applyAdd(add); } if (revoke != null) { - sharingInfo.getShareWith().revoke(revoke); + sharingInfo.applyRevoke(revoke); } try (ThreadContext.StoredContext ctx = this.threadPool.getThreadContext().stashContext()) { diff --git a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java index 4db1b55850..1c2b81ddd7 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/Recipients.java +++ b/src/main/java/org/opensearch/security/resources/sharing/Recipients.java @@ -76,10 +76,6 @@ public void revoke(Recipients target) { } } - public boolean isPublic() { - return recipients.values().stream().anyMatch(recipients -> recipients.contains("*")); - } - public boolean isSharedWithAny(Recipient recipientType, Set targets) { return !Collections.disjoint(recipients.getOrDefault(recipientType, Collections.emptySet()), targets); } @@ -143,15 +139,7 @@ public static Recipients fromXContent( // Validate element value if validator provided // For "users" field, allow wildcard; for others, don't if (elementValidator != null) { - // Check if wildcard should be allowed based on recipient type - boolean allowWildcard = (recipient == Recipient.USERS); - if (allowWildcard) { - // Use wildcard-enabled validator for users - RequestContentValidator.principalValidator(true).validate(fieldName, value); - } else { - // Use standard validator for roles and backend_roles - elementValidator.validate(fieldName, value); - } + elementValidator.validate(fieldName, value); } values.add(value); diff --git a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java index 4e58d784f5..9e972d2858 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java @@ -105,15 +105,26 @@ public void share(String accessLevel, Recipients target) { return; } Recipients sharedWith = shareWith.atAccessLevel(accessLevel); - // sharedWith will be null when sharing at a new access-level if (sharedWith == null) { - // update the ShareWith object shareWith = shareWith.updateSharingInfo(accessLevel, target); } else { sharedWith.share(target); } } + public void setGeneralAccess(String generalAccess) { + ShareWith current = getShareWith(); + shareWith = new ShareWith(current.getSharingInfo(), generalAccess); + } + + public void applyAdd(ShareWith add) { + shareWith = getShareWith().add(add); + } + + public void applyRevoke(ShareWith revoke) { + shareWith = getShareWith().revoke(revoke); + } + public void revoke(String accessLevel, Recipients target) { if (shareWith == null) { log.warn("Cannot revoke access as resource {} is not shared with anyone", this.resourceId); @@ -292,16 +303,15 @@ public boolean isSharedWithEntity(Recipient recipientType, Set targets, * @return a {@link Set} of access level identifiers granted to the user, never {@code null}. */ public Set getAccessLevelsForUser(User user) { - Set userRoles = new HashSet<>(user.getSecurityRoles()); - Set userBackendRoles = new HashSet<>(user.getRoles()); + Set accessLevels = new HashSet<>(); - userRoles.add("*"); - userBackendRoles.add("*"); + if (shareWith != null && shareWith.getGeneralAccess() != null) { + accessLevels.add(shareWith.getGeneralAccess()); + } - Set accessLevels = new HashSet<>(); - accessLevels.addAll(fetchAccessLevels(Recipient.USERS, Set.of(user.getName(), "*"))); - accessLevels.addAll(fetchAccessLevels(Recipient.ROLES, userRoles)); - accessLevels.addAll(fetchAccessLevels(Recipient.BACKEND_ROLES, userBackendRoles)); + accessLevels.addAll(fetchAccessLevels(Recipient.USERS, Set.of(user.getName()))); + accessLevels.addAll(fetchAccessLevels(Recipient.ROLES, user.getSecurityRoles())); + accessLevels.addAll(fetchAccessLevels(Recipient.BACKEND_ROLES, user.getRoles())); return accessLevels; } @@ -320,14 +330,9 @@ public Set fetchAccessLevels(Recipient recipientType, Set entiti String accessLevel = entry.getKey(); Recipients recipients = entry.getValue(); - Set sharingRecipients = new HashSet<>(recipients.getRecipients().getOrDefault(recipientType, Set.of())); + Set sharingRecipients = recipients.getRecipients().getOrDefault(recipientType, Set.of()); - // if there’s a wildcard (i.e. the document is shared publicly at this access-level), or at least one entity in common, add the - // level to a final list of groups - boolean matchesWildcard = sharingRecipients.contains("*"); - boolean intersects = !Collections.disjoint(sharingRecipients, entities); - - if (matchesWildcard || intersects) { + if (!Collections.disjoint(sharingRecipients, entities)) { matchingGroups.add(accessLevel); } } @@ -350,6 +355,9 @@ public List getAllPrincipals() { // Add shared recipients if (shareWith != null) { + if (shareWith.isPublic()) { + principals.add("public"); + } // shared with at any access level for (Recipients recipients : shareWith.getSharingInfo().values()) { Map> recipientMap = recipients.getRecipients(); diff --git a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java index 69bf635ba7..e7502de208 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java @@ -48,21 +48,37 @@ public class ShareWith implements ToXContentFragment, NamedWriteable { */ private final Map sharingInfo; + /** + * The access level granted to everyone (general/public access). + * e.g. "read" means anyone can read; named recipients may hold higher levels. + * Null means the resource is not publicly accessible. + */ + private final String generalAccess; + public ShareWith(Map sharingInfo) { + this(sharingInfo, null); + } + + public ShareWith(Map sharingInfo, String generalAccess) { this.sharingInfo = sharingInfo; + this.generalAccess = generalAccess; } public ShareWith(StreamInput in) throws IOException { this.sharingInfo = in.readMap(StreamInput::readString, Recipients::new); + this.generalAccess = in.readOptionalString(); } public boolean isPublic() { - // TODO Contemplate following google doc model of link sharing which has single access level when link sharing is enabled - return sharingInfo.values().stream().anyMatch(Recipients::isPublic); + return generalAccess != null; + } + + public String getGeneralAccess() { + return generalAccess; } public boolean isPrivate() { - return sharingInfo == null || sharingInfo.isEmpty(); + return generalAccess == null && (sharingInfo == null || sharingInfo.isEmpty()); } public Set accessLevels() { @@ -88,11 +104,14 @@ public ShareWith updateSharingInfo(String accessLevel, Recipients target) { @Override public XContentBuilder toXContent(XContentBuilder b, Params params) throws IOException { b.startObject(); + if (generalAccess != null) { + b.field("general_access", generalAccess); + } for (Map.Entry e : this.sharingInfo.entrySet()) { Recipients norm = pruneRecipients(e.getValue()); if (norm == null) continue; // skip empty level b.field(e.getKey()); - norm.toXContent(b, params); // TODO ensure this skips empty arrays too + norm.toXContent(b, params); } return b.endObject(); } @@ -117,15 +136,23 @@ public static ShareWith fromXContent(XContentParser parser, RequestContentValida parser.nextToken(); } + String generalAccess = null; + XContentParser.Token token; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { // Each field in the object represents a SharedWithActionGroup if (token == XContentParser.Token.FIELD_NAME) { - String accessLevel = parser.currentName(); + String fieldName = parser.currentName(); + + if ("general_access".equals(fieldName)) { + parser.nextToken(); + generalAccess = parser.textOrNull(); + continue; + } // Validate access level if validator is provided if (accessLevelValidator != null) { - accessLevelValidator.validate("access_level", accessLevel); + accessLevelValidator.validate("access_level", fieldName); } parser.nextToken(); @@ -135,11 +162,11 @@ public static ShareWith fromXContent(XContentParser parser, RequestContentValida RequestContentValidator.ARRAY_SIZE_VALIDATOR, RequestContentValidator.principalValidator(false) ); - sharingInfo.put(accessLevel, recipients); + sharingInfo.put(fieldName, recipients); } } - return new ShareWith(sharingInfo); + return new ShareWith(sharingInfo, generalAccess); } @Override @@ -150,6 +177,7 @@ public String getWriteableName() { @Override public void writeTo(StreamOutput out) throws IOException { out.writeMap(sharingInfo, StreamOutput::writeString, (o, sw) -> sw.writeTo(o)); + out.writeOptionalString(generalAccess); } @Override @@ -161,7 +189,7 @@ public String toString() { * Returns a new ShareWith by merging this and another ShareWith (adding recipients). */ public ShareWith add(ShareWith other) { - if (other == null || other.isPrivate()) { + if (other == null) { return this; } for (var entry : other.sharingInfo.entrySet()) { @@ -172,14 +200,16 @@ public ShareWith add(ShareWith other) { return orig; }); } - return this; + // generalAccess in the patch overwrites the current value + String newGeneralAccess = other.generalAccess != null ? other.generalAccess : this.generalAccess; + return new ShareWith(this.sharingInfo, newGeneralAccess); } /** * Returns a new ShareWith by revoking recipients based on another ShareWith. */ public ShareWith revoke(ShareWith other) { - if (this.sharingInfo.isEmpty() || other == null || other.isPrivate()) { + if (other == null) { return this; } for (var entry : other.sharingInfo.entrySet()) { @@ -190,7 +220,9 @@ public ShareWith revoke(ShareWith other) { return orig; }); } - return this; + // clear generalAccess if the revoke patch specifies the same level + String newGeneralAccess = other.generalAccess != null && other.generalAccess.equals(this.generalAccess) ? null : this.generalAccess; + return new ShareWith(this.sharingInfo, newGeneralAccess); } /** Return a normalized ShareWith with no empty buckets and no empty action-groups. */ @@ -202,7 +234,7 @@ public ShareWith prune() { cleaned.put(e.getKey(), prunedRecipients); } } - return new ShareWith(cleaned); + return new ShareWith(cleaned, generalAccess); } private static Recipients pruneRecipients(Recipients r) { From 44644ef29b09499af5e3d2bf0c845144467384f7 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Mar 2026 15:00:05 -0400 Subject: [PATCH 2/7] Forbid sharing generally at full access Signed-off-by: Craig Perkins --- .../multi_share/PubliclySharedDocTests.java | 45 +++++++++++++++++++ .../ResourceSharingIndexHandler.java | 4 +- .../resources/api/share/ShareRequest.java | 22 +++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java index 140063e3b1..9f9a308da2 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java @@ -29,6 +29,7 @@ import static org.opensearch.sample.resource.TestUtils.SAMPLE_FULL_ACCESS; import static org.opensearch.sample.resource.TestUtils.SAMPLE_READ_ONLY; import static org.opensearch.sample.resource.TestUtils.newCluster; +import static org.opensearch.security.api.AbstractApiIntegrationTest.badRequest; import static org.opensearch.security.api.AbstractApiIntegrationTest.forbidden; import static org.opensearch.security.api.AbstractApiIntegrationTest.ok; import static org.opensearch.test.framework.TestSecurityConfig.User.USER_ADMIN; @@ -132,6 +133,12 @@ public void generalAccess_doesNotLeakSharingInfo() throws Exception { forbidden(() -> api.revokeGeneralAccess(resourceId, FULL_ACCESS_USER, SAMPLE_READ_ONLY)); } + @Test + public void generalAccess_fullAccess_doesNotGrantSharePermission() throws Exception { + // setting general_access to a level that includes share permission must be rejected at the API + badRequest(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_FULL_ACCESS)); + } + @Test public void generalAccess_upgradeLevel_replacesExistingGeneralAccess() throws Exception { ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); @@ -145,6 +152,44 @@ public void generalAccess_upgradeLevel_replacesExistingGeneralAccess() throws Ex ok(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "updated")); } + @Test + public void generalAccess_resourceAppearsInListAndSearch() throws Exception { + forbidden(() -> api.listResources(FULL_ACCESS_USER)); + forbidden(() -> api.searchResources(FULL_ACCESS_USER)); + forbidden(() -> api.searchResources(TestUtils.ApiHelper.searchAllPayload(), FULL_ACCESS_USER)); + + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + + // resource should now appear in list + TestRestClient.HttpResponse listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); + assertThat(listResponse.getBody(), containsString("sample")); + + // resource should appear in search (GET and POST) + TestRestClient.HttpResponse searchResponse = ok(() -> api.searchResources(FULL_ACCESS_USER)); + assertThat(searchResponse.getBody(), containsString("sample")); + + searchResponse = ok(() -> api.searchResources(TestUtils.ApiHelper.searchAllPayload(), FULL_ACCESS_USER)); + TestUtils.ApiHelper.assertSearchResponse(searchResponse, 1, "sample"); + + searchResponse = ok(() -> api.searchResources(TestUtils.ApiHelper.searchByNamePayload("sample"), FULL_ACCESS_USER)); + TestUtils.ApiHelper.assertSearchResponse(searchResponse, 1, "sample"); + } + + @Test + public void revokeGeneralAccess_resourceDisappearsFromListAndSearch() throws Exception { + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + + // confirm visible + TestRestClient.HttpResponse listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); + assertThat(listResponse.getBody(), containsString("sample")); + + ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + + // should no longer appear + forbidden(() -> api.listResources(FULL_ACCESS_USER)); + forbidden(() -> api.searchResources(FULL_ACCESS_USER)); + } + @Test public void generalAccess_sharingInfoResponse_containsGeneralAccessField() throws Exception { ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index 87f546d668..b97afe58b0 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -1175,10 +1175,8 @@ public boolean canUserShare(User user, boolean isAdmin, ResourceSharing resource if (isAdmin || resourceSharingRecord.isCreatedBy(user.getName())) return true; - if (resourceSharingRecord.isSharedWithEveryone()) return true; - var sw = resourceSharingRecord.getShareWith(); - if (sw == null || sw.getSharingInfo().isEmpty()) return false; + if (sw == null) return false; Set users = Set.of(user.getName()); Set roles = new HashSet<>(user.getSecurityRoles()); diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java index 29435e0547..36493719d6 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java @@ -253,6 +253,28 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou } } } + rejectSharePermissionOnGeneralAccess(resourcePluginInfo); + } + + /** + * Rejects any general_access value whose resolved actions include share permission. + * general_access is content access only — it must never grant policy-control (share) permission. + */ + private void rejectSharePermissionOnGeneralAccess(ResourcePluginInfo resourcePluginInfo) { + if (resourceType == null) return; + var flattened = resourcePluginInfo.flattenedForType(resourceType); + for (ShareWith sw : new ShareWith[] { shareWith, add }) { + if (sw == null || sw.getGeneralAccess() == null) continue; + String level = sw.getGeneralAccess(); + if (flattened.resolve(Set.of(level)).contains(ShareAction.NAME)) { + throw new IllegalArgumentException( + "general_access level ['" + + level + + "'] includes share permission, which is not allowed. " + + "general_access controls content access only." + ); + } + } } } From 98dcef367c90fdc5bd869648250df7be86222126 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Mar 2026 15:46:57 -0400 Subject: [PATCH 3/7] Fix tests Signed-off-by: Craig Perkins --- .../opensearch/sample/resource/TestUtils.java | 12 +++++++ .../multi_share/PubliclySharedDocTests.java | 32 ++++++++++++------- .../resources/ResourceAccessHandler.java | 9 ++---- .../resources/ResourceSharingDlsUtils.java | 2 +- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java index 4329c8a310..2394c3863f 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java @@ -746,5 +746,17 @@ public void awaitSharingEntry(String resourceId, String expectedString) { }); } } + + public void awaitResourceVisibility(String resourceId, String expectedPrincipal) { + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + Awaitility.await("Wait for all_shared_principals to contain " + expectedPrincipal + " for resource " + resourceId) + .pollInterval(Duration.ofMillis(500)) + .untilAsserted(() -> { + TestRestClient.HttpResponse response = client.get(RESOURCE_INDEX_NAME + "/_doc/" + resourceId); + response.assertStatusCode(200); + assertThat(response.getBody(), containsString(expectedPrincipal)); + }); + } + } } } diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java index 9f9a308da2..437ba0e94c 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java @@ -28,6 +28,7 @@ import static org.opensearch.sample.resource.TestUtils.NO_ACCESS_USER; import static org.opensearch.sample.resource.TestUtils.SAMPLE_FULL_ACCESS; import static org.opensearch.sample.resource.TestUtils.SAMPLE_READ_ONLY; +import static org.opensearch.sample.resource.TestUtils.SAMPLE_READ_WRITE; import static org.opensearch.sample.resource.TestUtils.newCluster; import static org.opensearch.security.api.AbstractApiIntegrationTest.badRequest; import static org.opensearch.security.api.AbstractApiIntegrationTest.forbidden; @@ -82,10 +83,10 @@ public void generalAccess_readOnly_grantsEveryoneReadAccess() throws Exception { } @Test - public void generalAccess_fullAccess_grantsEveryoneFullAccess() throws Exception { + public void generalAccess_readWrite_grantsEveryoneFullContentAccess() throws Exception { forbidden(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); - ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_FULL_ACCESS)); + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_WRITE)); TestRestClient.HttpResponse response = ok(() -> api.getResource(resourceId, LIMITED_ACCESS_USER)); assertThat(response.getBody(), containsString("sample")); @@ -144,8 +145,8 @@ public void generalAccess_upgradeLevel_replacesExistingGeneralAccess() throws Ex ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); forbidden(() -> api.updateResource(resourceId, FULL_ACCESS_USER, "updated")); - // upgrade general access to full - ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_FULL_ACCESS)); + // upgrade general access to read_write (full content access, no share permission) + ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_WRITE)); TestRestClient.HttpResponse response = ok(() -> api.getResource(resourceId, FULL_ACCESS_USER)); assertThat(response.getBody(), containsString("sample")); @@ -154,18 +155,22 @@ public void generalAccess_upgradeLevel_replacesExistingGeneralAccess() throws Ex @Test public void generalAccess_resourceAppearsInListAndSearch() throws Exception { - forbidden(() -> api.listResources(FULL_ACCESS_USER)); - forbidden(() -> api.searchResources(FULL_ACCESS_USER)); - forbidden(() -> api.searchResources(TestUtils.ApiHelper.searchAllPayload(), FULL_ACCESS_USER)); + // before sharing: list and search return 200 with empty results + TestRestClient.HttpResponse listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); + assertThat(listResponse.getBody(), not(containsString("sample"))); + + TestRestClient.HttpResponse searchResponse = ok(() -> api.searchResources(FULL_ACCESS_USER)); + assertThat(searchResponse.getBody(), not(containsString("sample"))); ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + api.awaitResourceVisibility(resourceId, "public"); // resource should now appear in list - TestRestClient.HttpResponse listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); + listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); assertThat(listResponse.getBody(), containsString("sample")); // resource should appear in search (GET and POST) - TestRestClient.HttpResponse searchResponse = ok(() -> api.searchResources(FULL_ACCESS_USER)); + searchResponse = ok(() -> api.searchResources(FULL_ACCESS_USER)); assertThat(searchResponse.getBody(), containsString("sample")); searchResponse = ok(() -> api.searchResources(TestUtils.ApiHelper.searchAllPayload(), FULL_ACCESS_USER)); @@ -178,16 +183,21 @@ public void generalAccess_resourceAppearsInListAndSearch() throws Exception { @Test public void revokeGeneralAccess_resourceDisappearsFromListAndSearch() throws Exception { ok(() -> api.shareResourceGenerally(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + api.awaitResourceVisibility(resourceId, "public"); // confirm visible TestRestClient.HttpResponse listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); assertThat(listResponse.getBody(), containsString("sample")); ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + api.awaitResourceVisibility(resourceId, "user:admin"); // user:* sentinel removed, only creator remains // should no longer appear - forbidden(() -> api.listResources(FULL_ACCESS_USER)); - forbidden(() -> api.searchResources(FULL_ACCESS_USER)); + listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); + assertThat(listResponse.getBody(), not(containsString("sample"))); + + TestRestClient.HttpResponse searchResponse = ok(() -> api.searchResources(FULL_ACCESS_USER)); + assertThat(searchResponse.getBody(), not(containsString("sample"))); } @Test diff --git a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java index a47347e9c9..f77788fe1b 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java @@ -12,7 +12,6 @@ package org.opensearch.security.resources; import java.util.Collections; -import java.util.HashSet; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -382,14 +381,10 @@ private void loadAllResourceSharingRecords(String resourceType, ActionListener getFlatPrincipals(User user) { // 1) collect all entities we’ll match against share_with arrays // for users: - Set users = new HashSet<>(); - users.add(user.getName()); - users.add("public"); // for matching against publicly shared resources - // return flattened principals to build the bool query return Stream.concat( - // users - users.stream().map(u -> "user:" + u), + // users, plus bare "public" sentinel for publicly shared resources + Stream.concat(Stream.of("user:" + user.getName(), "public"), Stream.empty()), // then roles and backend_roles Stream.concat(user.getSecurityRoles().stream().map(r -> "role:" + r), user.getRoles().stream().map(b -> "backend:" + b)) ).collect(Collectors.toSet()); diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingDlsUtils.java b/src/main/java/org/opensearch/security/resources/ResourceSharingDlsUtils.java index 86ac188b37..13fd2e3e80 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingDlsUtils.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingDlsUtils.java @@ -35,7 +35,7 @@ public static IndexToRuleMap resourceRestrictions( ) { List principals = new ArrayList<>(); - principals.add("user:*"); // Convention for publicly visible + principals.add("public"); // matches resources shared via general_access principals.add("user:" + user.getName()); // owner // Security roles (OpenSearch Security roles) From 6494503e8b084aa3537d24c0da51aaf8db62613d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 19 Mar 2026 16:36:12 -0400 Subject: [PATCH 4/7] Fix tests Signed-off-by: Craig Perkins --- .../security/resources/sharing/ShareWith.java | 2 +- .../sharing/ResourceSharingTests.java | 60 +++++++++++++++++-- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java index e7502de208..50e5173713 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java @@ -209,7 +209,7 @@ public ShareWith add(ShareWith other) { * Returns a new ShareWith by revoking recipients based on another ShareWith. */ public ShareWith revoke(ShareWith other) { - if (other == null) { + if (other == null || sharingInfo.isEmpty()) { return this; } for (var entry : other.sharingInfo.entrySet()) { diff --git a/src/test/java/org/opensearch/security/resources/sharing/ResourceSharingTests.java b/src/test/java/org/opensearch/security/resources/sharing/ResourceSharingTests.java index 1b505252f8..470863b632 100644 --- a/src/test/java/org/opensearch/security/resources/sharing/ResourceSharingTests.java +++ b/src/test/java/org/opensearch/security/resources/sharing/ResourceSharingTests.java @@ -195,11 +195,11 @@ public void isSharedWithEntity_handlesNullsAndDelegates() { } @Test - public void fetchAccessLevels_returnsLevelsWithWildcardOrIntersection() { + public void fetchAccessLevels_returnsLevelsWithIntersection() { CreatedBy cb = mockCreatedBy("o"); - // level:read has users {"u1", "*"} -> wildcard = match - Recipients rRead = mockRecipients(Map.of(Recipient.USERS, new HashSet<>(Set.of("u1", "*")))); + // level:read has users {"u1", "u2"} -> intersection with {"u2"} = match + Recipients rRead = mockRecipients(Map.of(Recipient.USERS, new HashSet<>(Set.of("u1", "u2")))); // level:write has roles {"roleA"} -> no match for {"roleB"} unless overlap Recipients rWrite = mockRecipients(Map.of(Recipient.ROLES, new HashSet<>(Set.of("roleA")))); // level:admin has backend {"br1","br2"} -> intersection with {"br2"} = match @@ -214,10 +214,13 @@ public void fetchAccessLevels_returnsLevelsWithWildcardOrIntersection() { ResourceSharing rs = ResourceSharing.builder().resourceId("r").createdBy(cb).shareWith(sw).build(); - // users, looking for anything in {"any"} -> wildcard on read makes it match - Set levelsUsers = rs.fetchAccessLevels(Recipient.USERS, Set.of("any")); + // users, looking for {"u2"} -> intersection with read makes it match + Set levelsUsers = rs.fetchAccessLevels(Recipient.USERS, Set.of("u2")); assertEquals(Set.of("read"), levelsUsers); + // users, looking for {"unknown"} -> no match + assertTrue(rs.fetchAccessLevels(Recipient.USERS, Set.of("unknown")).isEmpty()); + // roles, looking for {"roleB"} -> no match; {"roleA"} -> match write assertTrue(rs.fetchAccessLevels(Recipient.ROLES, Set.of("roleB")).isEmpty()); assertEquals(Set.of("write"), rs.fetchAccessLevels(Recipient.ROLES, Set.of("roleA"))); @@ -259,6 +262,53 @@ public void getAllPrincipals_includesCreatorAndAllRecipientsAcrossLevels() { assertEquals(new HashSet<>(principals).size(), principals.size()); } + @Test + public void getAccessLevelsForUser_includesNamedRecipientsAndGeneralAccess() { + CreatedBy cb = mockCreatedBy("owner"); + + Recipients readRecipients = mockRecipients(Map.of(Recipient.USERS, new HashSet<>(Set.of("alice")))); + Recipients writeRecipients = mockRecipients(Map.of(Recipient.ROLES, new HashSet<>(Set.of("editor")))); + + Map info = new HashMap<>(); + info.put("read", readRecipients); + info.put("write", writeRecipients); + + ShareWith sw = mock(ShareWith.class); + when(sw.getSharingInfo()).thenReturn(info); + when(sw.getGeneralAccess()).thenReturn("read"); + info.forEach((level, r) -> when(sw.atAccessLevel(level)).thenReturn(r)); + + org.opensearch.security.user.User user = new org.opensearch.security.user.User("bob").withSecurityRoles(Set.of("editor")); + + ResourceSharing rs = ResourceSharing.builder().resourceId("r").createdBy(cb).shareWith(sw).build(); + Set levels = rs.getAccessLevelsForUser(user); + + // general_access contributes "read"; role "editor" contributes "write" + assertEquals(Set.of("read", "write"), levels); + } + + @Test + public void getAccessLevelsForUser_noGeneralAccess_onlyNamedRecipients() { + CreatedBy cb = mockCreatedBy("owner"); + + Recipients readRecipients = mockRecipients(Map.of(Recipient.USERS, new HashSet<>(Set.of("alice")))); + + Map info = new HashMap<>(); + info.put("read", readRecipients); + + ShareWith sw = mock(ShareWith.class); + when(sw.getSharingInfo()).thenReturn(info); + when(sw.getGeneralAccess()).thenReturn(null); + when(sw.atAccessLevel("read")).thenReturn(readRecipients); + + org.opensearch.security.user.User user = new org.opensearch.security.user.User("alice"); + + ResourceSharing rs = ResourceSharing.builder().resourceId("r").createdBy(cb).shareWith(sw).build(); + Set levels = rs.getAccessLevelsForUser(user); + + assertEquals(Set.of("read"), levels); + } + @Test public void getAllPrincipals_handlesNullShareWith() { ResourceSharing rs = ResourceSharing.builder().resourceId("r").createdBy(mockCreatedBy("owner")).build(); From c6610cc889246ffa9f7eaced28bdf93943ca234a Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 20 Mar 2026 09:48:02 -0400 Subject: [PATCH 5/7] Change PATCH format Signed-off-by: Craig Perkins --- .../opensearch/sample/resource/TestUtils.java | 73 ++++++++----------- .../multi_share/PubliclySharedDocTests.java | 6 +- .../resources/ResourceAccessHandler.java | 36 +++++---- .../ResourceSharingIndexHandler.java | 5 ++ .../resources/api/share/ShareRequest.java | 42 +++++++++-- .../api/share/ShareTransportAction.java | 2 + 6 files changed, 98 insertions(+), 66 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java index 2394c3863f..1e926d06f7 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java @@ -10,7 +10,6 @@ import java.io.IOException; import java.time.Duration; -import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -280,8 +279,8 @@ public static class PatchSharingInfoPayloadBuilder { private String resourceType; private final Map share = new HashMap<>(); private final Map revoke = new HashMap<>(); - private String setGeneralAccess; - private String revokeGeneralAccess; + private boolean generalAccessPresent; + private String generalAccess; public PatchSharingInfoPayloadBuilder resourceId(String resourceId) { this.resourceId = resourceId; @@ -306,54 +305,42 @@ public void revoke(Recipients recipients, String accessLevel) { } public void setGeneralAccess(String accessLevel) { - this.setGeneralAccess = accessLevel; + this.generalAccessPresent = true; + this.generalAccess = accessLevel; } - public void revokeGeneralAccess(String accessLevel) { - this.revokeGeneralAccess = accessLevel; - } - - private String buildJsonString(Map input) { - List output = new ArrayList<>(); - for (Map.Entry entry : input.entrySet()) { - try { - XContentBuilder builder = XContentFactory.jsonBuilder(); - entry.getValue().toXContent(builder, ToXContent.EMPTY_PARAMS); - String recipJson = builder.toString(); - output.add(""" - "%s" : %s - """.formatted(entry.getKey(), recipJson)); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - return String.join(",", output); + public void revokeGeneralAccess() { + this.generalAccessPresent = true; + this.generalAccess = null; } public String build() { - String addGeneralAccess = setGeneralAccess != null ? "\"general_access\": \"" + setGeneralAccess + "\"" : ""; - String addNamedRecipients = buildJsonString(share); - String addSection = String.join(",", List.of(addGeneralAccess, addNamedRecipients).stream().filter(s -> !s.isBlank()).toList()); - - String revokeGeneralAccessStr = revokeGeneralAccess != null ? "\"general_access\": \"" + revokeGeneralAccess + "\"" : ""; - String revokeNamedRecipients = buildJsonString(revoke); - String revokeSection = String.join( - ",", - List.of(revokeGeneralAccessStr, revokeNamedRecipients).stream().filter(s -> !s.isBlank()).toList() - ); - + String generalAccessField = generalAccessPresent + ? (generalAccess != null ? "\"general_access\": \"" + generalAccess + "\"," : "\"general_access\": null,") + : ""; return """ { "resource_id": "%s", "resource_type": "%s", - "add": { - %s - }, - "revoke": { - %s - } + %s + "add": %s, + "revoke": %s } - """.formatted(resourceId, resourceType, addSection, revokeSection); + """.formatted(resourceId, resourceType, generalAccessField, buildSection(share), buildSection(revoke)); + } + + // Produces e.g.: {"sample_read_only":{"users":["alice"]}} + private String buildSection(Map named) { + try { + XContentBuilder b = XContentFactory.jsonBuilder().startObject(); + for (Map.Entry entry : named.entrySet()) { + b.field(entry.getKey()); + entry.getValue().toXContent(b, ToXContent.EMPTY_PARAMS); + } + return b.endObject().toString(); + } catch (IOException e) { + throw new RuntimeException(e); + } } } @@ -618,11 +605,11 @@ public TestRestClient.HttpResponse shareResourceGenerally(String resourceId, Tes } } - public TestRestClient.HttpResponse revokeGeneralAccess(String resourceId, TestSecurityConfig.User user, String accessLevel) { + public TestRestClient.HttpResponse revokeGeneralAccess(String resourceId, TestSecurityConfig.User user) { PatchSharingInfoPayloadBuilder patchBuilder = new PatchSharingInfoPayloadBuilder(); patchBuilder.resourceType(RESOURCE_TYPE); patchBuilder.resourceId(resourceId); - patchBuilder.revokeGeneralAccess(accessLevel); + patchBuilder.revokeGeneralAccess(); try (TestRestClient client = cluster.getRestClient(user)) { return client.patch(SECURITY_SHARE_ENDPOINT, patchBuilder.build()); } diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java index 437ba0e94c..952aff3277 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/feature/enabled/multi_share/PubliclySharedDocTests.java @@ -119,7 +119,7 @@ public void revokeGeneralAccess_removesPublicAccess() throws Exception { ok(() -> api.getResource(resourceId, FULL_ACCESS_USER)); // revoke general access - ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN)); // access should be gone forbidden(() -> api.getResource(resourceId, FULL_ACCESS_USER)); @@ -131,7 +131,7 @@ public void generalAccess_doesNotLeakSharingInfo() throws Exception { // a user with only general read access cannot view or modify sharing info forbidden(() -> api.shareResource(resourceId, FULL_ACCESS_USER, FULL_ACCESS_USER, SAMPLE_FULL_ACCESS)); - forbidden(() -> api.revokeGeneralAccess(resourceId, FULL_ACCESS_USER, SAMPLE_READ_ONLY)); + forbidden(() -> api.revokeGeneralAccess(resourceId, FULL_ACCESS_USER)); } @Test @@ -189,7 +189,7 @@ public void revokeGeneralAccess_resourceDisappearsFromListAndSearch() throws Exc TestRestClient.HttpResponse listResponse = ok(() -> api.listResources(FULL_ACCESS_USER)); assertThat(listResponse.getBody(), containsString("sample")); - ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN, SAMPLE_READ_ONLY)); + ok(() -> api.revokeGeneralAccess(resourceId, USER_ADMIN)); api.awaitResourceVisibility(resourceId, "user:admin"); // user:* sentinel removed, only creator remains // should no longer appear diff --git a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java index f77788fe1b..a607a70816 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java @@ -213,6 +213,8 @@ public void patchSharingInfo( @NonNull String resourceType, @Nullable ShareWith add, @Nullable ShareWith revoke, + boolean generalAccessPresent, + @Nullable String generalAccess, ActionListener listener ) { final UserSubjectImpl userSubject = (UserSubjectImpl) threadContext.getPersistent( @@ -249,19 +251,27 @@ public void patchSharingInfo( revoke ); - this.resourceSharingIndexHandler.patchSharingInfo(resourceId, resourceIndex, add, revoke, ActionListener.wrap(sharingInfo -> { - LOGGER.debug("Successfully patched sharing info for resource {} with add: {}, revoke: {}", resourceId, add, revoke); - listener.onResponse(sharingInfo); - }, e -> { - LOGGER.error( - "Failed to patched sharing info for resource {} with add: {}, revoke: {} : {}", - resourceId, - add, - revoke, - e.getMessage() - ); - listener.onFailure(e); - })); + this.resourceSharingIndexHandler.patchSharingInfo( + resourceId, + resourceIndex, + add, + revoke, + generalAccessPresent, + generalAccess, + ActionListener.wrap(sharingInfo -> { + LOGGER.debug("Successfully patched sharing info for resource {} with add: {}, revoke: {}", resourceId, add, revoke); + listener.onResponse(sharingInfo); + }, e -> { + LOGGER.error( + "Failed to patched sharing info for resource {} with add: {}, revoke: {} : {}", + resourceId, + add, + revoke, + e.getMessage() + ); + listener.onFailure(e); + }) + ); } diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index b97afe58b0..f29eb63a87 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -709,6 +709,8 @@ public void patchSharingInfo( String resourceIndex, ShareWith add, ShareWith revoke, + boolean generalAccessPresent, + String generalAccess, ActionListener listener ) { @@ -726,6 +728,9 @@ public void patchSharingInfo( if (revoke != null) { sharingInfo.applyRevoke(revoke); } + if (generalAccessPresent) { + sharingInfo.setGeneralAccess(generalAccess); + } try (ThreadContext.StoredContext ctx = this.threadPool.getThreadContext().stashContext()) { // update the record diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java index 36493719d6..e29c8cd38e 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareRequest.java @@ -43,6 +43,10 @@ public class ShareRequest extends ActionRequest implements DocRequest { private final ShareWith add; @JsonProperty("revoke") private final ShareWith revoke; + /** True if general_access was explicitly present in the patch request (even if null). */ + private final boolean generalAccessPresent; + @JsonProperty("general_access") + private final String generalAccess; private final RestRequest.Method method; @@ -56,6 +60,8 @@ private ShareRequest(Builder builder) { this.shareWith = builder.shareWith; this.add = builder.add; this.revoke = builder.revoke; + this.generalAccessPresent = builder.generalAccessPresent; + this.generalAccess = builder.generalAccess; this.method = builder.method; } @@ -68,6 +74,8 @@ public ShareRequest(StreamInput in) throws IOException { this.shareWith = in.readOptionalWriteable(ShareWith::new); this.add = in.readOptionalWriteable(ShareWith::new); this.revoke = in.readOptionalWriteable(ShareWith::new); + this.generalAccessPresent = in.readBoolean(); + this.generalAccess = in.readOptionalString(); } @Override @@ -79,6 +87,8 @@ public void writeTo(StreamOutput out) throws IOException { out.writeOptionalWriteable(shareWith); out.writeOptionalWriteable(add); out.writeOptionalWriteable(revoke); + out.writeBoolean(generalAccessPresent); + out.writeOptionalString(generalAccess); } @Override @@ -98,8 +108,11 @@ public ActionRequestValidationException validate() { arv.addValidationError("share_with is required"); throw arv; } - if ((method == RestRequest.Method.PATCH || method == RestRequest.Method.POST) && add == null && revoke == null) { - arv.addValidationError("either add or revoke must be present"); + if ((method == RestRequest.Method.PATCH || method == RestRequest.Method.POST) + && add == null + && revoke == null + && !generalAccessPresent) { + arv.addValidationError("at least one of add, revoke, or general_access must be present"); throw arv; } return null; @@ -117,6 +130,14 @@ public ShareWith getRevoke() { return revoke; } + public boolean isGeneralAccessPresent() { + return generalAccessPresent; + } + + public String getGeneralAccess() { + return generalAccess; + } + public RestRequest.Method getMethod() { return method; } @@ -156,6 +177,8 @@ public static class Builder { ShareWith shareWith; ShareWith add; ShareWith revoke; + boolean generalAccessPresent; + String generalAccess; RestRequest.Method method; public void resourceId(String resourceId) { @@ -247,6 +270,10 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou case "revoke": this.revoke(ShareWith.fromXContent(parser, accessLevelValidator)); break; + case "general_access": + this.generalAccessPresent = true; + this.generalAccess = parser.currentToken() == XContentParser.Token.VALUE_NULL ? null : parser.text(); + break; default: parser.skipChildren(); } @@ -262,11 +289,12 @@ public void parseContent(XContentParser xContentParser, ResourcePluginInfo resou */ private void rejectSharePermissionOnGeneralAccess(ResourcePluginInfo resourcePluginInfo) { if (resourceType == null) return; - var flattened = resourcePluginInfo.flattenedForType(resourceType); - for (ShareWith sw : new ShareWith[] { shareWith, add }) { - if (sw == null || sw.getGeneralAccess() == null) continue; - String level = sw.getGeneralAccess(); - if (flattened.resolve(Set.of(level)).contains(ShareAction.NAME)) { + var actionGroups = resourcePluginInfo.flattenedForType(resourceType); + // check top-level general_access (PATCH) and share_with.general_access (PUT) + String putLevel = shareWith != null ? shareWith.getGeneralAccess() : null; + for (String level : new String[] { generalAccess, putLevel }) { + if (level == null) continue; + if (actionGroups.resolve(Set.of(level)).contains(ShareAction.NAME)) { throw new IllegalArgumentException( "general_access level ['" + level diff --git a/src/main/java/org/opensearch/security/resources/api/share/ShareTransportAction.java b/src/main/java/org/opensearch/security/resources/api/share/ShareTransportAction.java index f60a61414a..016d9216b7 100644 --- a/src/main/java/org/opensearch/security/resources/api/share/ShareTransportAction.java +++ b/src/main/java/org/opensearch/security/resources/api/share/ShareTransportAction.java @@ -51,6 +51,8 @@ protected void doExecute(Task task, ShareRequest request, ActionListener Date: Fri, 20 Mar 2026 10:05:45 -0400 Subject: [PATCH 6/7] Update comments Signed-off-by: Craig Perkins --- .../java/org/opensearch/sample/resource/TestUtils.java | 5 ----- .../security/resources/sharing/ResourceSharing.java | 10 +++++----- .../security/resources/sharing/ShareWith.java | 8 +++++--- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java index 1e926d06f7..efd7d91013 100644 --- a/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java +++ b/sample-resource-plugin/src/integrationTest/java/org/opensearch/sample/resource/TestUtils.java @@ -304,11 +304,6 @@ public void revoke(Recipients recipients, String accessLevel) { revoke.put(accessLevel, existing); } - public void setGeneralAccess(String accessLevel) { - this.generalAccessPresent = true; - this.generalAccess = accessLevel; - } - public void revokeGeneralAccess() { this.generalAccessPresent = true; this.generalAccess = null; diff --git a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java index 9e972d2858..587aad6a30 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ResourceSharing.java @@ -30,11 +30,11 @@ /** * Represents a resource sharing configuration that manages access control for OpenSearch resources. - * This class holds information about shared resources including their source, creator, and sharing permissions. + * This class holds information about shared resources including their creator and sharing permissions. * The class maintains information about: *
    - *
  • The source index where the resource is defined
    • *
  • The unique identifier of the resource
    • + *
  • The type of the resource
    • *
  • The creator's information
    • *
  • The sharing permissions and recipients
    • *
@@ -260,9 +260,9 @@ public boolean isCreatedBy(String userName) { } /** - * Checks if the given resource is shared with everyone, i.e. the entity list is "*" + * Checks if the given resource is shared with everyone via general access. * - * @return True if the resource is shared with everyone, false otherwise. + * @return True if the resource has general access set (i.e. publicly accessible), false otherwise. */ public boolean isSharedWithEveryone() { return this.shareWith != null && this.shareWith.isPublic(); @@ -319,7 +319,7 @@ public Set getAccessLevelsForUser(User user) { * Fetches all access-levels where at-least 1 recipient matches the given set of targets * @param recipientType the type of recipient to be matched against * @param entities targets to look for - * @return set of access-levels which contain given nay of the targets + * @return set of access-levels which contain any of the targets */ public Set fetchAccessLevels(Recipient recipientType, Set entities) { if (shareWith == null) { diff --git a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java index 50e5173713..0cbfd51a03 100644 --- a/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java +++ b/src/main/java/org/opensearch/security/resources/sharing/ShareWith.java @@ -26,13 +26,15 @@ /** * This class contains information about whom a resource is shared with and what is the action-group associated with it. + * Optionally includes a {@code general_access} level that grants baseline access to all authenticated users. * *

Example usage: * 

  * "share_with": {
- *   "default": {
- *     "users": [],
- *     "roles": [],
+ *   "general_access": "read_only",
+ *   "read_write": {
+ *     "users": ["alice"],
+ *     "roles": ["editors"],
  *     "backend_roles": []
  *   }
  * }

From e82c1c4ca7bf1baa62481ce8dbd7181954d61519 Mon Sep 17 00:00:00 2001
From: Craig Perkins 
Date: Fri, 20 Mar 2026 11:01:40 -0400
Subject: [PATCH 7/7] Fix test

Signed-off-by: Craig Perkins 
---
 .../security/resources/ResourceAccessHandlerTests.java    | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java b/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java
index 8fdf5dd584..b6b42e1f5a 100644
--- a/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java
+++ b/src/test/java/org/opensearch/security/resources/ResourceAccessHandlerTests.java
@@ -280,13 +280,13 @@ public void testPatchSharingInfoSuccess() {
 
         ResourceSharing doc = mock(ResourceSharing.class);
         doAnswer(inv -> {
-            ActionListener l = inv.getArgument(4);
+            ActionListener l = inv.getArgument(6);
             l.onResponse(doc);
             return null;
-        }).when(sharingIndexHandler).patchSharingInfo(eq(RESOURCE_ID), eq(INDEX), eq(add), eq(revoke), any());
+        }).when(sharingIndexHandler).patchSharingInfo(eq(RESOURCE_ID), eq(INDEX), eq(add), eq(revoke), eq(false), eq(null), any());
 
         ActionListener listener = mock(ActionListener.class);
-        handler.patchSharingInfo(RESOURCE_ID, TYPE, add, revoke, listener);
+        handler.patchSharingInfo(RESOURCE_ID, TYPE, add, revoke, false, null, listener);
 
         verify(listener).onResponse(doc);
     }
@@ -295,7 +295,7 @@ public void testPatchSharingInfoSuccess() {
     public void testPatchSharingInfoFailsIfNoUser() {
         ShareWith x = new ShareWith(ImmutableMap.of());
         ActionListener listener = mock(ActionListener.class);
-        handler.patchSharingInfo(RESOURCE_ID, TYPE, x, x, listener);
+        handler.patchSharingInfo(RESOURCE_ID, TYPE, x, x, false, null, listener);
 
         verify(listener).onFailure(any(OpenSearchStatusException.class));
     }