From 08412fd07220f46a944fc38b6b180994b28d9665 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Tue, 13 Oct 2026 16:38:04 -0700 Subject: [PATCH 1/4] Add provider-admin share permissions, served by Armeria. --- .../opensharing/recipient/RecipientStore.java | 7 +- .../io/opensharing/runtime/OpenSharing.java | 16 ++- .../share/SharePermissionEntity.java | 66 ++++++++++ .../share/SharePermissionResponse.java | 38 ++++++ .../share/SharePermissionStore.java | 87 +++++++++++++ .../io/opensharing/share/SharePrivilege.java | 6 + .../io/opensharing/share/ShareService.java | 66 +++++++++- .../java/io/opensharing/share/ShareStore.java | 6 +- .../share/UpdateSharePermissionsRequest.java | 36 ++++++ .../opensharing/share/ShareServiceTest.java | 8 +- .../opensharing/share/ShareAdminService.java | 20 ++- .../SharePermissionAdminServiceTest.java | 115 ++++++++++++++++++ .../SharePermissionUpdateTransactionTest.java | 93 ++++++++++++++ 13 files changed, 553 insertions(+), 11 deletions(-) create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionEntity.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionResponse.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/SharePrivilege.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateSharePermissionsRequest.java create mode 100644 server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionAdminServiceTest.java create mode 100644 server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionUpdateTransactionTest.java diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java b/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java index 223b88972..e8d841dce 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java @@ -97,7 +97,7 @@ public List list() { .getResultList()); } - /** Deletes a recipient and its tokens. Only the owner may delete it. */ + /** Deletes a recipient, its tokens and its permissions. Only the owner may delete it. */ public void delete(String name, UserContext user) { tx.inTransaction( false, @@ -106,6 +106,11 @@ public void delete(String name, UserContext user) { em.createQuery("delete from RecipientTokenEntity t where t.recipient.id = :recipientId") .setParameter("recipientId", recipient.getId()) .executeUpdate(); + // DELETE FROM os_share_permissions WHERE recipient_id = ? + em.createQuery( + "delete from SharePermissionEntity p where p.recipient.id = :recipientId") + .setParameter("recipientId", recipient.getId()) + .executeUpdate(); em.remove(recipient); return null; }); diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/runtime/OpenSharing.java b/server/opensharing-server-core/src/main/java/io/opensharing/runtime/OpenSharing.java index f3bd4d146..34fc31e85 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/runtime/OpenSharing.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/runtime/OpenSharing.java @@ -9,6 +9,8 @@ import io.opensharing.recipient.RecipientStore; import io.opensharing.recipient.RecipientTokenEntity; import io.opensharing.share.ShareEntity; +import io.opensharing.share.SharePermissionEntity; +import io.opensharing.share.SharePermissionStore; import io.opensharing.share.ShareService; import io.opensharing.share.ShareStore; import java.util.List; @@ -26,9 +28,16 @@ public final class OpenSharing { private OpenSharing(CatalogConnector catalog, Transactions tx) { this.catalog = catalog; + RecipientStore recipientStore = new RecipientStore(tx); this.shares = - new ShareService(catalog, tx, new ShareStore(tx), new SharedDataObjectStore(tx)); - this.recipients = new RecipientService(catalog, new RecipientStore(tx)); + new ShareService( + catalog, + tx, + new ShareStore(tx), + new SharedDataObjectStore(tx), + new SharePermissionStore(tx), + recipientStore); + this.recipients = new RecipientService(catalog, recipientStore); } public static Builder builder() { @@ -41,7 +50,8 @@ public static List> entities() { ShareEntity.class, SharedDataObjectEntity.class, RecipientEntity.class, - RecipientTokenEntity.class); + RecipientTokenEntity.class, + SharePermissionEntity.class); } public CatalogConnector catalog() { diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionEntity.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionEntity.java new file mode 100644 index 000000000..2b3eebc66 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionEntity.java @@ -0,0 +1,66 @@ +package io.opensharing.share; + +import io.opensharing.BaseEntity; +import io.opensharing.recipient.RecipientEntity; +import jakarta.persistence.Column; +import jakarta.persistence.Entity; +import jakarta.persistence.EnumType; +import jakarta.persistence.Enumerated; +import jakarta.persistence.FetchType; +import jakarta.persistence.JoinColumn; +import jakarta.persistence.ManyToOne; +import jakarta.persistence.Table; +import jakarta.persistence.UniqueConstraint; + +/** + * One privilege a recipient holds on a share. A recipient holds each privilege on a share at most + * once. {@code ShareStore} and {@code RecipientStore} delete the rows with their share or + * recipient. + */ +@Entity +@Table( + name = "os_share_permissions", + uniqueConstraints = + @UniqueConstraint( + name = "uk_share_permission", + columnNames = {"share_id", "recipient_id", "privilege"})) +public class SharePermissionEntity extends BaseEntity { + + /** Loaded eagerly because responses read the share's name after the store returns. */ + @ManyToOne(fetch = FetchType.EAGER, optional = false) + @JoinColumn(name = "share_id", nullable = false) + private ShareEntity share; + + /** Loaded eagerly because responses read the recipient's name after the store returns. */ + @ManyToOne(fetch = FetchType.EAGER, optional = false) + @JoinColumn(name = "recipient_id", nullable = false) + private RecipientEntity recipient; + + @Enumerated(EnumType.STRING) + @Column(nullable = false, length = 32) + private SharePrivilege privilege; + + public ShareEntity getShare() { + return share; + } + + public void setShare(ShareEntity share) { + this.share = share; + } + + public RecipientEntity getRecipient() { + return recipient; + } + + public void setRecipient(RecipientEntity recipient) { + this.recipient = recipient; + } + + public SharePrivilege getPrivilege() { + return privilege; + } + + public void setPrivilege(SharePrivilege privilege) { + this.privilege = privilege; + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionResponse.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionResponse.java new file mode 100644 index 000000000..46baff6ff --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionResponse.java @@ -0,0 +1,38 @@ +package io.opensharing.share; + +import com.fasterxml.jackson.annotation.JsonInclude; +import java.time.Instant; + +/** + * A privilege a recipient holds on a share. {@code grantedAt} is when it was first granted. + * + * 
{@code
+ * {
+ *   "shareId": "",
+ *   "shareName": "",
+ *   "recipientId": "",
+ *   "recipientName": "",
+ *   "privilege": "SELECT",
+ *   "grantedAt": ""
+ * }
+ * }
+ */ +@JsonInclude(JsonInclude.Include.NON_NULL) +public record SharePermissionResponse( + String shareId, + String shareName, + String recipientId, + String recipientName, + SharePrivilege privilege, + Instant grantedAt) { + + public static SharePermissionResponse from(SharePermissionEntity permission) { + return new SharePermissionResponse( + permission.getShare().getId(), + permission.getShare().getName(), + permission.getRecipient().getId(), + permission.getRecipient().getName(), + permission.getPrivilege(), + permission.getCreatedAt()); + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java new file mode 100644 index 000000000..3e3b6b8b1 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java @@ -0,0 +1,87 @@ +package io.opensharing.share; + +import io.opensharing.Transactions; +import io.opensharing.http.ApiException; +import io.opensharing.recipient.RecipientEntity; +import jakarta.persistence.EntityManager; +import java.util.List; +import java.util.Optional; + +/** + * Storage for privileges recipients hold on shares. Callers check share ownership before granting + * or revoking. Grants and revokes run in the caller's transaction, so a batch of them commits or + * rolls back together; {@link #list} runs in its own. Each loaded permission also loads its share + * and recipient. + */ +public class SharePermissionStore { + + private final Transactions tx; + + public SharePermissionStore(Transactions tx) { + this.tx = tx; + } + + /** Granting a privilege the recipient already holds leaves the original row untouched. */ + public void grant( + EntityManager em, ShareEntity share, RecipientEntity recipient, SharePrivilege privilege) { + if (find(em, share, recipient, privilege).isEmpty()) { + SharePermissionEntity permission = new SharePermissionEntity(); + permission.setShare(em.getReference(ShareEntity.class, share.getId())); + permission.setRecipient(em.getReference(RecipientEntity.class, recipient.getId())); + permission.setPrivilege(privilege); + em.persist(permission); + } + } + + /** Revokes a privilege. Fails with not-found when the recipient does not hold it. */ + public void revoke( + EntityManager em, ShareEntity share, RecipientEntity recipient, SharePrivilege privilege) { + SharePermissionEntity permission = + find(em, share, recipient, privilege) + .orElseThrow( + () -> + ApiException.notFound( + "recipient '" + + recipient.getName() + + "' does not hold " + + privilege + + " on share '" + + share.getName() + + "'")); + em.remove(permission); + } + + /** + * {@code SELECT p.* FROM os_share_permissions p JOIN os_recipients r ON r.id = p.recipient_id + * WHERE p.share_id = ? ORDER BY r.name} + */ + public List list(ShareEntity share) { + return tx.inTransaction( + true, + em -> + em.createQuery( + "select p from SharePermissionEntity p join fetch p.share" + + " join fetch p.recipient r where p.share.id = :shareId" + + " order by r.name", + SharePermissionEntity.class) + .setParameter("shareId", share.getId()) + .getResultList()); + } + + /** + * {@code SELECT * FROM os_share_permissions WHERE share_id = ? AND recipient_id = ? AND + * privilege = ?} + */ + private static Optional find( + EntityManager em, ShareEntity share, RecipientEntity recipient, SharePrivilege privilege) { + return em.createQuery( + "select p from SharePermissionEntity p where p.share.id = :shareId" + + " and p.recipient.id = :recipientId and p.privilege = :privilege", + SharePermissionEntity.class) + .setParameter("shareId", share.getId()) + .setParameter("recipientId", recipient.getId()) + .setParameter("privilege", privilege) + .getResultStream() + .findFirst(); + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePrivilege.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePrivilege.java new file mode 100644 index 000000000..ca3298140 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePrivilege.java @@ -0,0 +1,6 @@ +package io.opensharing.share; + +/** What a recipient may do with a share. Reading is all there is so far. */ +public enum SharePrivilege { + SELECT +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java index d8074cf4b..ec9dd204c 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java @@ -13,14 +13,19 @@ import io.opensharing.exception.CatalogException; import io.opensharing.http.ApiException; import io.opensharing.http.ListResponse; +import io.opensharing.recipient.RecipientEntity; +import io.opensharing.recipient.RecipientStore; import jakarta.persistence.EntityManager; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.stream.Stream; /** - * Provider share CRUD for a caller the host has authenticated. Creating a share needs CREATE_SHARE - * from the catalog; any caller may read shares, only the owner may change them. + * Provider share CRUD and recipient grants for a caller the host has authenticated. Creating a + * share needs CREATE_SHARE from the catalog; any caller may read shares, only the owner may change + * them. */ public class ShareService { @@ -28,16 +33,22 @@ public class ShareService { private final Transactions tx; private final ShareStore shares; private final SharedDataObjectStore objects; + private final SharePermissionStore permissions; + private final RecipientStore recipients; public ShareService( CatalogConnector catalog, Transactions tx, ShareStore shares, - SharedDataObjectStore objects) { + SharedDataObjectStore objects, + SharePermissionStore permissions, + RecipientStore recipients) { this.catalog = catalog; this.tx = tx; this.shares = shares; this.objects = objects; + this.permissions = permissions; + this.recipients = recipients; } /** Creates a share owned by the caller, who needs CREATE_SHARE from the catalog. */ @@ -124,6 +135,52 @@ public void delete(UserContext user, String share) { shares.delete(share, user); } + /** Lists who holds which privilege on the share, by recipient name, unpaged. */ + public ListResponse listPermissions(String share) { + // TODO: page with maxResults and pageToken. + return ListResponse.of( + permissions.list(shares.require(share)).stream() + .map(SharePermissionResponse::from) + .toList()); + } + + /** + * Applies the changes in order, each revoking then granting, all in one transaction: either every + * change commits or none does. Returns the share's permissions. Owner only. + */ + public ListResponse updatePermissions( + UserContext user, String share, UpdateSharePermissionsRequest request) { + shares.requireOwned(share, user); + List changes = new ArrayList<>(); + for (UpdateSharePermissionsRequest.Change change : request.changes()) { + if (change == null) { + throw ApiException.invalidParameter("changes must not contain null"); + } + if (Stream.concat(change.add().stream(), change.remove().stream()) + .anyMatch(Objects::isNull)) { + throw ApiException.invalidParameter("changes.add and changes.remove must not contain null"); + } + RecipientEntity recipient = + recipients.require(requireText(change.recipientName(), "changes.recipientName")); + changes.add(new RecipientChange(recipient, change)); + } + tx.inTransaction( + false, + em -> { + ShareEntity entity = shares.requireOwned(em, share, user); + for (RecipientChange change : changes) { + for (SharePrivilege privilege : change.change().remove()) { + permissions.revoke(em, entity, change.recipient(), privilege); + } + for (SharePrivilege privilege : change.change().add()) { + permissions.grant(em, entity, change.recipient(), privilege); + } + } + return null; + }); + return listPermissions(share); + } + // Resolves the object in the catalog on behalf of the caller and checks the declared type. private Addition addition(UserContext user, UpdateShareRequest.DataObject dataObject) { String name = requireText(dataObject.name(), "dataObject.name"); @@ -207,6 +264,9 @@ private record Alias(String schema, String table) {} private record Addition(String name, AssetType type, ResolvedAsset resolved, Alias alias) {} + private record RecipientChange( + RecipientEntity recipient, UpdateSharePermissionsRequest.Change change) {} + // Exactly one of alias and name is set. private record Removal(AssetType type, Alias alias, String name) { diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java index 5dcffc3fa..9b0b198e2 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java @@ -92,7 +92,7 @@ public List list() { .getResultList()); } - /** Deletes a share and the objects in it. Only the owner may delete it. */ + /** Deletes a share, the objects in it and its permissions. Only the owner may delete it. */ public void delete(String name, UserContext user) { tx.inTransaction( false, @@ -101,6 +101,10 @@ public void delete(String name, UserContext user) { em.createQuery("delete from SharedDataObjectEntity o where o.share.id = :shareId") .setParameter("shareId", share.getId()) .executeUpdate(); + // DELETE FROM os_share_permissions WHERE share_id = ? + em.createQuery("delete from SharePermissionEntity p where p.share.id = :shareId") + .setParameter("shareId", share.getId()) + .executeUpdate(); em.remove(share); return null; }); diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateSharePermissionsRequest.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateSharePermissionsRequest.java new file mode 100644 index 000000000..d701e1874 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateSharePermissionsRequest.java @@ -0,0 +1,36 @@ +package io.opensharing.share; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * Grants and revokes privileges on one share for one or more recipients. Changes are applied in + * order, and either all of them take effect or none do. + * + * 
{@code
+ * PATCH {provider base path}/shares//permissions
+ * {
+ *   "changes": [
+ *     {"recipientName": "", "add": ["SELECT"]},
+ *     {"recipientName": "", "remove": ["SELECT"]}
+ *   ]
+ * }
+ * }
+ */ +public record UpdateSharePermissionsRequest(List changes) { + + public UpdateSharePermissionsRequest { + changes = changes == null ? List.of() : Collections.unmodifiableList(new ArrayList<>(changes)); + } + + /** Privileges to revoke from and grant to one recipient; removes are applied first. */ + public record Change( + String recipientName, List add, List remove) { + + public Change { + add = add == null ? List.of() : Collections.unmodifiableList(new ArrayList<>(add)); + remove = remove == null ? List.of() : Collections.unmodifiableList(new ArrayList<>(remove)); + } + } +} diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/share/ShareServiceTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/share/ShareServiceTest.java index b1070f99a..bd70ea26e 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/share/ShareServiceTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/share/ShareServiceTest.java @@ -9,6 +9,7 @@ import io.opensharing.auth.UserContext; import io.opensharing.catalog.CatalogConnector; import io.opensharing.exception.CatalogAuthorizationException; +import io.opensharing.recipient.RecipientStore; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Proxy; import java.util.ArrayList; @@ -35,7 +36,12 @@ void createsAShareOnlyWithCreateShareFromTheCatalog() { }); ShareService shares = new ShareService( - catalog, unused, new ShareStore(unused), new SharedDataObjectStore(unused)); + catalog, + unused, + new ShareStore(unused), + new SharedDataObjectStore(unused), + new SharePermissionStore(unused), + new RecipientStore(unused)); UserContext alice = UserContext.fromUserIdAndName("alice-id", "alice"); assertThrows( diff --git a/server/opensharing-server/src/main/java/io/opensharing/share/ShareAdminService.java b/server/opensharing-server/src/main/java/io/opensharing/share/ShareAdminService.java index 00a26fa58..a0cf52e35 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/share/ShareAdminService.java +++ b/server/opensharing-server/src/main/java/io/opensharing/share/ShareAdminService.java @@ -16,8 +16,8 @@ import io.opensharing.http.ListResponse; /** - * Provider HTTP API for share CRUD, behind {@link ProviderTokenDecorator}. Any caller may read - * shares; only the owner may change them. + * Provider HTTP API for share CRUD and recipient grants, behind {@link ProviderTokenDecorator}. Any + * caller may read shares; only the owner may change them. */ @Blocking @ProducesJson @@ -69,4 +69,20 @@ public HttpResponse delete(ServiceRequestContext ctx, @Param String share) { shares.delete(ProviderTokenDecorator.user(ctx), share); return HttpResponse.of(HttpStatus.NO_CONTENT); } + + /** {@code GET /shares/{share}/permissions}: lists who holds which privilege on the share. */ + @Get("/shares/{share}/permissions") + public ListResponse listPermissions(@Param String share) { + return shares.listPermissions(share); + } + + /** + * {@code PATCH /shares/{share}/permissions}: grants and revokes privileges in order, all or + * nothing, then returns the share's permissions. Owner only. + */ + @Patch("/shares/{share}/permissions") + public ListResponse updatePermissions( + ServiceRequestContext ctx, @Param String share, UpdateSharePermissionsRequest request) { + return shares.updatePermissions(ProviderTokenDecorator.user(ctx), share, request); + } } diff --git a/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionAdminServiceTest.java b/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionAdminServiceTest.java new file mode 100644 index 000000000..897878b6e --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionAdminServiceTest.java @@ -0,0 +1,115 @@ +package io.opensharing.share; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.opensharing.TestServer; +import io.opensharing.TestServer.Response; +import io.opensharing.http.ErrorCodes; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +class SharePermissionAdminServiceTest { + + private static final String SHARES = "/api/1.0/opensharing/provider/shares"; + private static final String RECIPIENTS = "/api/1.0/opensharing/provider/recipients"; + + private static TestServer server; + + @BeforeAll + static void startServer() { + server = TestServer.start(); + } + + @AfterAll + static void stopServer() { + server.close(); + } + + @Test + void grantsAndRevokesSelect() { + // Create the share that will be granted. + assertEquals(201, server.post(SHARES, "alice-token", "{\"name\":\"sales\"}").status()); + + // Create the recipient that will receive SELECT. + assertEquals( + 201, + server + .post(RECIPIENTS, "alice-token", "{\"name\":\"Acme\",\"authenticationType\":\"TOKEN\"}") + .status()); + + // Non-owner cannot grant. + Response denied = + permissions( + "sales", + "bob-token", + "{\"changes\":[{\"recipientName\":\"acme\",\"add\":[\"SELECT\"]}]}"); + assertEquals(403, denied.status()); + assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); + + // Owner grants SELECT (recipient name is case-insensitive). + Response granted = change("sales", "Acme", "add"); + assertEquals(200, granted.status()); + assertEquals(1, granted.body().path("items").size()); + assertEquals("acme", granted.body().path("items").get(0).path("recipientName").asText()); + assertEquals("sales", granted.body().path("items").get(0).path("shareName").asText()); + assertEquals("SELECT", granted.body().path("items").get(0).path("privilege").asText()); + + // Granting the same privilege again is idempotent. + Response again = change("sales", "acme", "add"); + assertEquals(200, again.status()); + assertEquals(1, again.body().path("items").size()); + + // Listing permissions is allowed for any catalog principal. + Response listed = server.get(SHARES + "/sales/permissions", "bob-token"); + assertEquals(200, listed.status()); + assertEquals("SELECT", listed.body().path("items").get(0).path("privilege").asText()); + + // Owner revokes SELECT. + Response revoked = change("sales", "acme", "remove"); + assertEquals(200, revoked.status()); + assertEquals(0, revoked.body().path("items").size()); + + // Revoking a privilege that is not granted is 404. + Response missing = change("sales", "acme", "remove"); + assertEquals(404, missing.status()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); + } + + @Test + void rejectsNullChangesAndPrivileges() { + assertEquals(201, server.post(SHARES, "alice-token", "{\"name\":\"guarded\"}").status()); + + // A null change is rejected. + Response nullChange = permissions("guarded", "alice-token", "{\"changes\":[null]}"); + assertEquals(400, nullChange.status()); + assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, nullChange.errorCode()); + assertEquals("changes must not contain null", nullChange.body().path("message").asText()); + + // A null privilege is rejected in add and in remove. + for (String list : new String[] {"add", "remove"}) { + Response nullPrivilege = + permissions( + "guarded", + "alice-token", + "{\"changes\":[{\"recipientName\":\"acme\",\"" + list + "\":[null]}]}"); + assertEquals(400, nullPrivilege.status()); + assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, nullPrivilege.errorCode()); + assertEquals( + "changes.add and changes.remove must not contain null", + nullPrivilege.body().path("message").asText()); + } + } + + private static Response permissions(String share, String token, String body) { + return server.patch(SHARES + "/" + share + "/permissions", token, body); + } + + // Adds or removes SELECT for one recipient as the owner. + private static Response change(String share, String recipient, String list) { + return permissions( + share, + "alice-token", + "{\"changes\":[{\"recipientName\":\"" + recipient + "\",\"" + list + "\":[\"SELECT\"]}]}"); + } +} diff --git a/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionUpdateTransactionTest.java b/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionUpdateTransactionTest.java new file mode 100644 index 000000000..e4b6ec323 --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionUpdateTransactionTest.java @@ -0,0 +1,93 @@ +package io.opensharing.share; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.opensharing.TestServer; +import io.opensharing.TestServer.Response; +import java.util.List; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** A permissions update applies all of its grants and revokes, or none of them. */ +class SharePermissionUpdateTransactionTest { + + private static final String SHARES = "/api/1.0/opensharing/provider/shares"; + private static final String RECIPIENTS = "/api/1.0/opensharing/provider/recipients"; + + private static TestServer server; + + @BeforeAll + static void startServer() { + server = TestServer.start(); + } + + @AfterAll + static void stopServer() { + server.close(); + } + + @Test + void rollsBackEveryChangeWhenARevokeFails() { + createShareGrantedTo("atomic", "atomic-a", "atomic-b", "atomic-c"); + + // Revoking from a and granting to b succeed, but c holds nothing to revoke. + Response failed = + permissions( + "atomic", + """ + {"changes": [ + {"recipientName": "atomic-a", "remove": ["SELECT"]}, + {"recipientName": "atomic-b", "add": ["SELECT"]}, + {"recipientName": "atomic-c", "remove": ["SELECT"]} + ]} + """); + assertEquals(404, failed.status(), () -> failed.body().toString()); + + // a still holds SELECT and b was not granted it. + assertEquals(List.of("atomic-a"), holders("atomic")); + } + + @Test + void rollsBackWhenARecipientDoesNotExist() { + createShareGrantedTo("missing", "missing-a", "missing-b"); + + Response failed = + permissions( + "missing", + """ + {"changes": [ + {"recipientName": "missing-a", "remove": ["SELECT"]}, + {"recipientName": "missing-b", "add": ["SELECT"]}, + {"recipientName": "nobody", "add": ["SELECT"]} + ]} + """); + assertEquals(404, failed.status(), () -> failed.body().toString()); + + assertEquals(List.of("missing-a"), holders("missing")); + } + + // Creates the share and recipients, and grants SELECT to the first recipient only. + private static void createShareGrantedTo(String share, String... recipients) { + assertEquals(201, server.post(SHARES, "alice-token", "{\"name\":\"" + share + "\"}").status()); + for (String recipient : recipients) { + String body = "{\"name\":\"" + recipient + "\",\"authenticationType\":\"TOKEN\"}"; + assertEquals(201, server.post(RECIPIENTS, "alice-token", body).status()); + } + Response granted = + permissions( + share, + "{\"changes\":[{\"recipientName\":\"" + recipients[0] + "\",\"add\":[\"SELECT\"]}]}"); + assertEquals(200, granted.status(), () -> granted.body().toString()); + } + + private static Response permissions(String share, String body) { + return server.patch(SHARES + "/" + share + "/permissions", "alice-token", body); + } + + private static List holders(String share) { + Response response = server.get(SHARES + "/" + share + "/permissions", "alice-token"); + assertEquals(200, response.status()); + return response.body().path("items").findValuesAsText("recipientName"); + } +} From 23f828a3e41bdd63cd79bac5e272ae6c9015f314 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 19 Oct 2026 14:06:48 -0700 Subject: [PATCH 2/4] Test concurrent permission updates on one share. --- .../share/SharePermissionConcurrencyTest.java | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionConcurrencyTest.java diff --git a/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionConcurrencyTest.java b/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionConcurrencyTest.java new file mode 100644 index 000000000..58d68f44e --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/share/SharePermissionConcurrencyTest.java @@ -0,0 +1,96 @@ +package io.opensharing.share; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.opensharing.TestServer; +import io.opensharing.TestServer.Response; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** Concurrent permission updates on one share run one at a time, so each sees the other's. */ +class SharePermissionConcurrencyTest { + + private static final String SHARES = "/api/1.0/opensharing/provider/shares"; + private static final String RECIPIENTS = "/api/1.0/opensharing/provider/recipients"; + private static final int RUNS = 5; + + private static TestServer server; + private static ExecutorService pool; + + @BeforeAll + static void startServer() { + server = TestServer.start(); + pool = Executors.newFixedThreadPool(2); + } + + @AfterAll + static void stopServer() { + pool.shutdownNow(); + server.close(); + } + + @Test + void concurrentGrantsOfTheSamePrivilegeBothSucceed() throws Exception { + for (int i = 0; i < RUNS; i++) { + String share = "racing-grants-" + i; + createShareAndRecipient(share); + + List results = race(() -> change(share, "add"), () -> change(share, "add")); + + assertEquals(List.of(200, 200), statuses(results)); + } + } + + @Test + void concurrentRevokesOfTheSamePrivilegeFindItOnce() throws Exception { + for (int i = 0; i < RUNS; i++) { + String share = "racing-revokes-" + i; + createShareAndRecipient(share); + assertEquals(200, change(share, "add").status()); + + List results = race(() -> change(share, "remove"), () -> change(share, "remove")); + + assertEquals(List.of(200, 404), statuses(results)); + } + } + + // The recipient is named after the share. + private static void createShareAndRecipient(String share) { + assertEquals(201, server.post(SHARES, "alice-token", "{\"name\":\"" + share + "\"}").status()); + String body = "{\"name\":\"" + share + "\",\"authenticationType\":\"TOKEN\"}"; + assertEquals(201, server.post(RECIPIENTS, "alice-token", body).status()); + } + + private static Response change(String share, String action) { + return server.patch( + SHARES + "/" + share + "/permissions", + "alice-token", + "{\"changes\":[{\"recipientName\":\"" + share + "\",\"" + action + "\":[\"SELECT\"]}]}"); + } + + private static List statuses(List results) { + return results.stream().map(Response::status).sorted().toList(); + } + + private static List race(Callable first, Callable second) + throws Exception { + CountDownLatch start = new CountDownLatch(1); + Future a = pool.submit(() -> awaitThen(start, first)); + Future b = pool.submit(() -> awaitThen(start, second)); + start.countDown(); + return List.of(a.get(), b.get()); + } + + private static Response awaitThen(CountDownLatch start, Callable call) + throws Exception { + start.await(); + return call.call(); + } +} From f73d0ed46d77e08aa92465eedadfc2a60e808fbc Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 19 Oct 2026 16:53:35 -0700 Subject: [PATCH 3/4] Use getOrThrow and getOwnedOrThrow for share permissions. --- .../src/main/java/io/opensharing/share/ShareService.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java index ec9dd204c..849840b9a 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java @@ -139,7 +139,7 @@ public void delete(UserContext user, String share) { public ListResponse listPermissions(String share) { // TODO: page with maxResults and pageToken. return ListResponse.of( - permissions.list(shares.require(share)).stream() + permissions.list(shares.getOrThrow(share)).stream() .map(SharePermissionResponse::from) .toList()); } @@ -150,7 +150,7 @@ public ListResponse listPermissions(String share) { */ public ListResponse updatePermissions( UserContext user, String share, UpdateSharePermissionsRequest request) { - shares.requireOwned(share, user); + shares.getOwnedOrThrow(share, user); List changes = new ArrayList<>(); for (UpdateSharePermissionsRequest.Change change : request.changes()) { if (change == null) { @@ -161,13 +161,13 @@ public ListResponse updatePermissions( throw ApiException.invalidParameter("changes.add and changes.remove must not contain null"); } RecipientEntity recipient = - recipients.require(requireText(change.recipientName(), "changes.recipientName")); + recipients.getOrThrow(requireText(change.recipientName(), "changes.recipientName")); changes.add(new RecipientChange(recipient, change)); } tx.inTransaction( false, em -> { - ShareEntity entity = shares.requireOwned(em, share, user); + ShareEntity entity = shares.getOwnedOrThrow(em, share, user); for (RecipientChange change : changes) { for (SharePrivilege privilege : change.change().remove()) { permissions.revoke(em, entity, change.recipient(), privilege); From 9116fe9823eb4c655d60bd21b205c55cff3253ab Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 19 Oct 2026 16:59:49 -0700 Subject: [PATCH 4/4] Describe store queries in prose rather than SQL. --- .../java/io/opensharing/recipient/RecipientStore.java | 1 - .../java/io/opensharing/share/SharePermissionStore.java | 9 +-------- .../src/main/java/io/opensharing/share/ShareStore.java | 1 - 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java b/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java index e8d841dce..63b6d5a3c 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/recipient/RecipientStore.java @@ -106,7 +106,6 @@ public void delete(String name, UserContext user) { em.createQuery("delete from RecipientTokenEntity t where t.recipient.id = :recipientId") .setParameter("recipientId", recipient.getId()) .executeUpdate(); - // DELETE FROM os_share_permissions WHERE recipient_id = ? em.createQuery( "delete from SharePermissionEntity p where p.recipient.id = :recipientId") .setParameter("recipientId", recipient.getId()) diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java index 3e3b6b8b1..c8674487d 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/SharePermissionStore.java @@ -51,10 +51,7 @@ public void revoke( em.remove(permission); } - /** - * {@code SELECT p.* FROM os_share_permissions p JOIN os_recipients r ON r.id = p.recipient_id - * WHERE p.share_id = ? ORDER BY r.name} - */ + /** The share's permissions, ordered by recipient name. */ public List list(ShareEntity share) { return tx.inTransaction( true, @@ -68,10 +65,6 @@ public List list(ShareEntity share) { .getResultList()); } - /** - * {@code SELECT * FROM os_share_permissions WHERE share_id = ? AND recipient_id = ? AND - * privilege = ?} - */ private static Optional find( EntityManager em, ShareEntity share, RecipientEntity recipient, SharePrivilege privilege) { return em.createQuery( diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java index 9b0b198e2..7e2f489b5 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java @@ -101,7 +101,6 @@ public void delete(String name, UserContext user) { em.createQuery("delete from SharedDataObjectEntity o where o.share.id = :shareId") .setParameter("shareId", share.getId()) .executeUpdate(); - // DELETE FROM os_share_permissions WHERE share_id = ? em.createQuery("delete from SharePermissionEntity p where p.share.id = :shareId") .setParameter("shareId", share.getId()) .executeUpdate();