From fe5b85b1d56ddb9613792b682351b8f9b02c9036 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Tue, 13 Oct 2026 15:35:46 -0700 Subject: [PATCH 1/3] Add provider-admin share entities and CRUD, served by Armeria. --- .../java/io/opensharing/Transactions.java | 17 ++ .../java/io/opensharing/auth/UserContext.java | 8 + .../io/opensharing/http/ListResponse.java | 20 +++ .../io/opensharing/runtime/OpenSharing.java | 30 +++- .../opensharing/share/CreateShareRequest.java | 7 + .../io/opensharing/share/ShareEntity.java | 96 ++++++++++ .../io/opensharing/share/ShareResponse.java | 35 ++++ .../io/opensharing/share/ShareService.java | 79 ++++++++ .../java/io/opensharing/share/ShareStore.java | 137 ++++++++++++++ .../opensharing/share/UpdateShareRequest.java | 7 + .../opensharing/share/ShareServiceTest.java | 48 +++++ server/opensharing-server/pom.xml | 28 +++ .../io/opensharing/OpenSharingServer.java | 98 +++++++--- .../auth/ProviderTokenDecorator.java | 66 +++++++ .../opensharing/config/ServerProperties.java | 1 + .../persistence/JpaTransactions.java | 50 ++++++ .../opensharing/share/ShareAdminService.java | 63 +++++++ .../src/main/resources/hibernate.properties | 14 ++ .../src/main/resources/local-catalog.yml | 7 + .../src/main/resources/server.properties | 3 + .../test/java/io/opensharing/TestServer.java | 94 ++++++++++ .../auth/ProviderTokenDecoratorTest.java | 143 +++++++++++++++ .../share/ShareAdminServiceTest.java | 170 ++++++++++++++++++ server/pom.xml | 35 ++++ 24 files changed, 1229 insertions(+), 27 deletions(-) create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/Transactions.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ListResponse.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/CreateShareRequest.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/ShareResponse.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateShareRequest.java create mode 100644 server/opensharing-server-core/src/test/java/io/opensharing/share/ShareServiceTest.java create mode 100644 server/opensharing-server/src/main/java/io/opensharing/auth/ProviderTokenDecorator.java create mode 100644 server/opensharing-server/src/main/java/io/opensharing/persistence/JpaTransactions.java create mode 100644 server/opensharing-server/src/main/java/io/opensharing/share/ShareAdminService.java create mode 100644 server/opensharing-server/src/main/resources/hibernate.properties create mode 100644 server/opensharing-server/src/test/java/io/opensharing/TestServer.java create mode 100644 server/opensharing-server/src/test/java/io/opensharing/auth/ProviderTokenDecoratorTest.java create mode 100644 server/opensharing-server/src/test/java/io/opensharing/share/ShareAdminServiceTest.java diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/Transactions.java b/server/opensharing-server-core/src/main/java/io/opensharing/Transactions.java new file mode 100644 index 000000000..69d077690 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/Transactions.java @@ -0,0 +1,17 @@ +package io.opensharing; + +import jakarta.persistence.EntityManager; +import java.util.function.Function; + +/** + * Runs store work in one database transaction. Each host supplies its own, backed by its own + * persistence setup; OpenSharing's entities must be registered with it. + */ +public interface Transactions { + + /** + * Runs {@code work} in a new transaction and commits it, or rolls it back when {@code work} + * throws. Entities returned from {@code work} are detached once it returns. + */ + T inTransaction(boolean readOnly, Function work); +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java b/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java index a3bd13e71..683a1c066 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java @@ -1,5 +1,7 @@ package io.opensharing.auth; +import io.opensharing.http.ApiException; + /** * End-user identity for a catalog call. * @@ -30,4 +32,10 @@ public String toString() { + userName + "]"; } + + public void requireOwner(String ownerId, String what) { + if (userId == null || !ownerId.equals(userId)) { + throw ApiException.permissionDenied("user '" + userName + "' does not own " + what); + } + } } diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ListResponse.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ListResponse.java new file mode 100644 index 000000000..460857cb8 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ListResponse.java @@ -0,0 +1,20 @@ +package io.opensharing.http; + +import com.fasterxml.jackson.annotation.JsonInclude; +import java.util.List; + +/** + * Paginated list envelope shared by list endpoints. {@code nextPageToken} is omitted on the last + * page: + * + * 
{@code
+ * {"items": [, ...], "nextPageToken": ""}
+ * }
+ */ +@JsonInclude(JsonInclude.Include.NON_NULL) +public record ListResponse(List items, String nextPageToken) { + + public static ListResponse of(List items) { + return new ListResponse<>(items, 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 d699b200e..9607b525c 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 @@ -1,6 +1,11 @@ package io.opensharing.runtime; +import io.opensharing.Transactions; import io.opensharing.catalog.CatalogConnector; +import io.opensharing.share.ShareEntity; +import io.opensharing.share.ShareService; +import io.opensharing.share.ShareStore; +import java.util.List; /** * OpenSharing assembled from what its host supplies. The standalone server builds one from its @@ -10,22 +15,34 @@ public final class OpenSharing { private final CatalogConnector catalog; + private final ShareService shares; - private OpenSharing(CatalogConnector catalog) { + private OpenSharing(CatalogConnector catalog, Transactions tx) { this.catalog = catalog; + this.shares = new ShareService(catalog, new ShareStore(tx)); } public static Builder builder() { return new Builder(); } + /** The entity classes the host's {@link Transactions} must register. */ + public static List> entities() { + return List.of(ShareEntity.class); + } + public CatalogConnector catalog() { return catalog; } + public ShareService shares() { + return shares; + } + public static final class Builder { private CatalogConnector catalog; + private Transactions transactions; private Builder() {} @@ -35,11 +52,20 @@ public Builder catalog(CatalogConnector catalog) { return this; } + /** The host's database transactions (required). */ + public Builder transactions(Transactions transactions) { + this.transactions = transactions; + return this; + } + public OpenSharing build() { if (catalog == null) { throw new IllegalStateException("OpenSharing requires a CatalogConnector from the host"); } - return new OpenSharing(catalog); + if (transactions == null) { + throw new IllegalStateException("OpenSharing requires Transactions from the host"); + } + return new OpenSharing(catalog, transactions); } } } diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/CreateShareRequest.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/CreateShareRequest.java new file mode 100644 index 000000000..3be6cff75 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/CreateShareRequest.java @@ -0,0 +1,7 @@ +package io.opensharing.share; + +import java.util.Map; + +/** POST body to create a share. The authenticated caller becomes the owner. */ +public record CreateShareRequest( + String name, String displayName, String comment, Map properties) {} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java new file mode 100644 index 000000000..083f1a6bd --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java @@ -0,0 +1,96 @@ +package io.opensharing.share; + +import io.opensharing.BaseEntity; +import io.opensharing.ObjectNames; +import jakarta.persistence.CollectionTable; +import jakarta.persistence.Column; +import jakarta.persistence.ElementCollection; +import jakarta.persistence.Entity; +import jakarta.persistence.FetchType; +import jakarta.persistence.JoinColumn; +import jakarta.persistence.MapKeyColumn; +import jakarta.persistence.Table; +import jakarta.persistence.UniqueConstraint; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; + +/** Persisted share row. {@code name} is always stored lowercase. */ +@Entity +@Table( + name = "os_shares", + uniqueConstraints = @UniqueConstraint(name = "uk_shares_name", columnNames = "name")) +public class ShareEntity extends BaseEntity { + + @Column(nullable = false, length = 255) + private String name; + + @Column(name = "display_name", length = 255) + private String displayName; + + @Column(length = 8192) + private String comment; + + /** + * The catalog's user id for whoever created this share. Not a foreign key: there is no principal + * table. + */ + @Column(name = "owner_id", nullable = false, length = 255) + private String ownerId; + + /** + * User-defined key-value properties, one row per key in {@code os_share_properties}. Loaded + * eagerly because the share is read after the store's transaction has closed. + */ + @ElementCollection(fetch = FetchType.EAGER) + @CollectionTable( + name = "os_share_properties", + joinColumns = @JoinColumn(name = "share_id"), + uniqueConstraints = + @UniqueConstraint( + name = "uk_share_properties", + columnNames = {"share_id", "property_key"})) + @MapKeyColumn(name = "property_key", length = 255) + @Column(name = "property_value", length = 1000) + private Map properties = new LinkedHashMap<>(); + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = ObjectNames.normalize(name); + } + + public String getDisplayName() { + return displayName; + } + + public void setDisplayName(String displayName) { + this.displayName = displayName; + } + + public String getComment() { + return comment; + } + + public void setComment(String comment) { + this.comment = comment; + } + + public Map getProperties() { + return Collections.unmodifiableMap(properties); + } + + public void setProperties(Map properties) { + this.properties = properties == null ? new LinkedHashMap<>() : new LinkedHashMap<>(properties); + } + + public String getOwnerId() { + return ownerId; + } + + public void setOwnerId(String ownerId) { + this.ownerId = ownerId; + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareResponse.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareResponse.java new file mode 100644 index 000000000..67d1db2fb --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareResponse.java @@ -0,0 +1,35 @@ +package io.opensharing.share; + +import com.fasterxml.jackson.annotation.JsonInclude; +import java.util.Map; + +/** + * A share as the protocol reports it. Fields without a value are omitted: + * + * 
{@code
+ * {
+ *   "id": "",
+ *   "name": "",
+ *   "displayName": "",
+ *   "comment": "",
+ *   "properties": {"": ""}
+ * }
+ * }
+ */ +@JsonInclude(JsonInclude.Include.NON_NULL) +public record ShareResponse( + String id, + String name, + String displayName, + String comment, + Map properties) { + + public static ShareResponse from(ShareEntity share) { + return new ShareResponse( + share.getId(), + share.getName(), + share.getDisplayName(), + share.getComment(), + Map.copyOf(share.getProperties())); + } +} 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 new file mode 100644 index 000000000..01bf17107 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareService.java @@ -0,0 +1,79 @@ +package io.opensharing.share; + +import io.opensharing.ObjectNames; +import io.opensharing.auth.AuthContext; +import io.opensharing.auth.Privilege; +import io.opensharing.auth.UserContext; +import io.opensharing.catalog.CatalogConnector; +import io.opensharing.http.ApiException; +import io.opensharing.http.ListResponse; +import java.util.Map; + +/** + * 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. + */ +public class ShareService { + + private final CatalogConnector catalog; + private final ShareStore shares; + + public ShareService(CatalogConnector catalog, ShareStore shares) { + this.catalog = catalog; + this.shares = shares; + } + + /** Creates a share owned by the caller, who needs CREATE_SHARE from the catalog. */ + public ShareResponse create(UserContext user, CreateShareRequest request) { + // The host has already authenticated the caller. A host that also checks CREATE_SHARE at its + // endpoint can make this a no-op. + catalog.authorize(AuthContext.of(user), Privilege.CREATE_SHARE); + return ShareResponse.from( + shares.create( + user, + ObjectNames.validateShareName(request.name()), + request.displayName(), + request.comment(), + validateProperties(request.properties()))); + } + + /** Lists every share by name, unpaged. */ + public ListResponse list() { + // TODO: page with maxResults and pageToken. + // TODO: check USE_SHARE through catalog.authorize. + return ListResponse.of(shares.list().stream().map(ShareResponse::from).toList()); + } + + /** Gets a share by name in any case. */ + public ShareResponse get(String share) { + return ShareResponse.from(shares.require(share)); + } + + /** Updates the fields set in {@code request}. Owner only. */ + public ShareResponse update(UserContext user, String share, UpdateShareRequest request) { + return ShareResponse.from( + shares.update( + user, + share, + request.displayName(), + request.comment(), + validateProperties(request.properties()))); + } + + /** Deletes the share. Owner only. */ + public void delete(UserContext user, String share) { + shares.delete(share, user); + } + + private static Map validateProperties(Map properties) { + if (properties != null) { + for (Map.Entry property : properties.entrySet()) { + if (property.getValue() == null) { + throw ApiException.invalidParameter( + "property '" + property.getKey() + "' must not be null"); + } + } + } + return properties; + } +} 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 new file mode 100644 index 000000000..4e937375f --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareStore.java @@ -0,0 +1,137 @@ +package io.opensharing.share; + +import io.opensharing.ObjectNames; +import io.opensharing.Transactions; +import io.opensharing.auth.UserContext; +import io.opensharing.http.ApiException; +import jakarta.persistence.EntityManager; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +/** + * Storage for shares. Names are stored lowercase and looked up case-insensitively. Each method runs + * in its own transaction, so returned entities are detached and must be fully loaded. Each loaded + * share also runs {@code SELECT * FROM os_share_properties WHERE share_id = ?}. + */ +public class ShareStore { + + private final Transactions tx; + + public ShareStore(Transactions tx) { + this.tx = tx; + } + + /** + * Creates a share owned by {@code author}. {@code name} must already be validated and lowercase. + * Fails with already-exists when the name is taken. + */ + public ShareEntity create( + UserContext author, + String name, + String displayName, + String comment, + Map properties) { + return tx.inTransaction( + false, + em -> { + if (exists(em, name)) { + throw ApiException.alreadyExists("share '" + name + "' already exists"); + } + ShareEntity share = new ShareEntity(); + share.setName(name); + share.setDisplayName(displayName); + share.setComment(comment); + share.setProperties(properties); + share.setOwnerId(author.userId()); + em.persist(share); + return share; + }); + } + + /** Only non-null fields are applied. Only the owner may update the share. */ + public ShareEntity update( + UserContext user, + String name, + String displayName, + String comment, + Map properties) { + return tx.inTransaction( + false, + em -> { + ShareEntity share = requireOwned(em, name, user); + if (displayName != null) { + share.setDisplayName(displayName); + } + if (comment != null) { + share.setComment(comment); + } + if (properties != null) { + share.setProperties(properties); + } + return share; + }); + } + + /** Looks up a share by name in any case. */ + public Optional find(String name) { + return tx.inTransaction(true, em -> find(em, name)); + } + + /** Like {@link #find}, but fails with not-found when the share does not exist. */ + public ShareEntity require(String name) { + return tx.inTransaction(true, em -> require(em, name)); + } + + /** Like {@link #require}, but also fails unless {@code user} owns the share. */ + public ShareEntity requireOwned(String name, UserContext user) { + return tx.inTransaction(true, em -> requireOwned(em, name, user)); + } + + /** {@code SELECT * FROM os_shares ORDER BY name ASC}: every share, regardless of owner. */ + public List list() { + return tx.inTransaction( + true, + em -> + em.createQuery("select s from ShareEntity s order by s.name", ShareEntity.class) + .getResultList()); + } + + /** Deletes a share. Only the owner may delete it. */ + public void delete(String name, UserContext user) { + tx.inTransaction( + false, + em -> { + em.remove(requireOwned(em, name, user)); + return null; + }); + } + + /** {@code SELECT id FROM os_shares WHERE name = ? LIMIT 1} */ + private static boolean exists(EntityManager em, String name) { + return !em.createQuery("select s.id from ShareEntity s where s.name = :name", String.class) + .setParameter("name", name) + .setMaxResults(1) + .getResultList() + .isEmpty(); + } + + /** {@code SELECT * FROM os_shares WHERE name = ?} */ + private static Optional find(EntityManager em, String name) { + return em.createQuery("select s from ShareEntity s where s.name = :name", ShareEntity.class) + .setParameter("name", ObjectNames.normalize(name)) + .getResultStream() + .findFirst(); + } + + private static ShareEntity require(EntityManager em, String name) { + return find(em, name) + .orElseThrow(() -> ApiException.notFound("share '" + name + "' does not exist")); + } + + private static ShareEntity requireOwned(EntityManager em, String name, UserContext user) { + ShareEntity share = require(em, name); + user.requireOwner(share.getOwnerId(), "share '" + share.getName() + "'"); + return share; + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateShareRequest.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateShareRequest.java new file mode 100644 index 000000000..a1c4266fe --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/UpdateShareRequest.java @@ -0,0 +1,7 @@ +package io.opensharing.share; + +import java.util.Map; + +/** PATCH body for share metadata. Null fields are left unchanged. */ +public record UpdateShareRequest( + String displayName, String comment, Map properties) {} 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 new file mode 100644 index 000000000..a74366482 --- /dev/null +++ b/server/opensharing-server-core/src/test/java/io/opensharing/share/ShareServiceTest.java @@ -0,0 +1,48 @@ +package io.opensharing.share; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.opensharing.Transactions; +import io.opensharing.auth.Privilege; +import io.opensharing.auth.UserContext; +import io.opensharing.catalog.CatalogConnector; +import io.opensharing.exception.CatalogAuthorizationException; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; + +class ShareServiceTest { + + @Test + void createsAShareOnlyWithCreateShareFromTheCatalog() { + List privileges = new ArrayList<>(); + CatalogConnector catalog = + proxy( + CatalogConnector.class, + (proxy, method, args) -> { + privileges.add((Privilege) args[1]); + throw new CatalogAuthorizationException("no CREATE_SHARE"); + }); + Transactions unused = + proxy( + Transactions.class, + (proxy, method, args) -> { + throw new UnsupportedOperationException(); + }); + ShareService shares = new ShareService(catalog, new ShareStore(unused)); + UserContext alice = UserContext.fromUserIdAndName("alice-id", "alice"); + + assertThrows( + CatalogAuthorizationException.class, + () -> shares.create(alice, new CreateShareRequest("sales", null, null, null))); + assertEquals(List.of(Privilege.CREATE_SHARE), privileges); + } + + private static T proxy(Class type, InvocationHandler handler) { + return type.cast( + Proxy.newProxyInstance(type.getClassLoader(), new Class[] {type}, handler)); + } +} diff --git a/server/opensharing-server/pom.xml b/server/opensharing-server/pom.xml index f5f18da67..3cc62b778 100644 --- a/server/opensharing-server/pom.xml +++ b/server/opensharing-server/pom.xml @@ -31,6 +31,34 @@ jackson-dataformat-yaml + + org.hibernate.orm + hibernate-core + + + org.hibernate.orm + hibernate-hikaricp + + + com.zaxxer + HikariCP + + + com.h2database + h2 + runtime + + + org.postgresql + postgresql + runtime + + + com.mysql + mysql-connector-j + runtime + + ch.qos.logback logback-classic diff --git a/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java b/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java index 864f6db0c..e4dd6c7d0 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java +++ b/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java @@ -4,13 +4,21 @@ import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.HttpStatusException; import com.linecorp.armeria.server.Server; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.server.annotation.JacksonRequestConverterFunction; +import com.linecorp.armeria.server.annotation.JacksonResponseConverterFunction; +import io.opensharing.auth.ProviderTokenDecorator; import io.opensharing.catalog.CatalogConnector; import io.opensharing.catalog.local.LocalCatalogConnector; import io.opensharing.catalog.local.LocalCatalogLoader; import io.opensharing.config.ServerProperties; import io.opensharing.config.ServerProperties.Property; import io.opensharing.http.ApiExceptionHandler; +import io.opensharing.http.ApiJson; +import io.opensharing.persistence.JpaTransactions; import io.opensharing.runtime.OpenSharing; +import io.opensharing.share.ShareAdminService; +import java.util.List; import java.util.Locale; /** @@ -20,17 +28,18 @@ public final class OpenSharingServer implements AutoCloseable { private final Server server; + private final JpaTransactions transactions; private final OpenSharing openSharing; - private OpenSharingServer(Server server, OpenSharing openSharing) { + private OpenSharingServer(Server server, JpaTransactions transactions, OpenSharing openSharing) { this.server = server; + this.transactions = transactions; this.openSharing = openSharing; } /** - * Starts the server with the settings in {@code server.properties}, overridden by JVM system - * properties and environment variables, and blocks until the JVM shuts down. It takes no program - * arguments. + * Starts the server from {@code server.properties}, where JVM system properties and environment + * variables override keys, and serves until the JVM shuts down. It takes no program arguments. */ public static void main(String[] args) throws InterruptedException { if (args.length > 0) { @@ -39,32 +48,54 @@ public static void main(String[] args) throws InterruptedException { + " environment variables"); } OpenSharingServer server = start(new ServerProperties()); - server.server.closeOnJvmShutdown(); + Runtime.getRuntime().addShutdownHook(new Thread(server::close)); server.server.blockUntilShutdown(); } /** - * Builds the server from {@code properties} and returns once it is listening. The catalog is - * loaded first, so an invalid catalog fails startup before the port is bound. + * Starts a server configured by {@code properties} and returns once it is listening. Provider + * services are served under {@code provider.base-path}, and any other path gets a 404 in the + * protocol's error format. If startup fails, the database connections are closed. */ public static OpenSharingServer start(ServerProperties properties) { - OpenSharing openSharing = OpenSharing.builder().catalog(catalog(properties)).build(); - HttpService notFound = - (ctx, req) -> { - throw HttpStatusException.of(HttpStatus.NOT_FOUND); - }; - Server server = - Server.builder() - .http(properties.getPort()) - .serviceUnder("/", notFound.decorate(new ApiExceptionHandler())) - .build(); + CatalogConnector catalog = catalog(properties); + JpaTransactions transactions = new JpaTransactions(OpenSharing.entities()); try { - server.start().join(); - } catch (RuntimeException e) { - server.close(); + OpenSharing openSharing = + OpenSharing.builder().catalog(catalog).transactions(transactions).build(); + ApiExceptionHandler errors = new ApiExceptionHandler(); + HttpService notFound = + (ctx, req) -> { + throw HttpStatusException.of(HttpStatus.NOT_FOUND); + }; + ServerBuilder builder = + Server.builder() + .http(properties.getPort()) + .annotatedServiceExtensions( + List.of(new JacksonRequestConverterFunction(ApiJson.MAPPER)), + List.of(new JacksonResponseConverterFunction(ApiJson.MAPPER)), + List.of(errors)) + .serviceUnder("/", notFound.decorate(errors)); + String providerPath = properties.get(Property.PROVIDER_BASE_PATH); + ProviderTokenDecorator providerToken = new ProviderTokenDecorator(catalog); + mountProvider( + builder, + providerPath, + providerToken, + errors, + new ShareAdminService(openSharing.shares())); + Server server = builder.build(); + try { + server.start().join(); + } catch (RuntimeException e) { + server.close(); + throw e; + } + return new OpenSharingServer(server, transactions, openSharing); + } catch (RuntimeException | Error e) { + transactions.close(); throw e; } - return new OpenSharingServer(server, openSharing); } /** The port the server listens on, which it picks itself when configured with 0. */ @@ -72,20 +103,37 @@ public int port() { return server.activeLocalPort(); } - /** The OpenSharing instance built from the server's settings. */ + /** The core services behind the HTTP endpoints, for callers such as tests that need them. */ public OpenSharing openSharing() { return openSharing; } - /** Stops the server and blocks until it has stopped. */ + /** Stops the server, then closes the database connections its requests used. */ @Override public void close() { server.stop().join(); + transactions.close(); + } + + // Serves an annotated provider service under the provider path, behind the provider token check. + // The token check must sit inside the error decorator, so that its 401s get the protocol body. + private static void mountProvider( + ServerBuilder builder, + String providerPath, + ProviderTokenDecorator providerToken, + ApiExceptionHandler errors, + Object service) { + builder + .annotatedService() + .pathPrefix(providerPath) + .decorator(providerToken) + .decorator(errors) + .build(service); } /** - * The catalog named by {@code catalog.type}, matched ignoring case and surrounding spaces. Only - * the local catalog ships with this build; it reads {@code catalog.local.file}. + * Builds the catalog connector named by {@code catalog.type}, in any case. This build ships only + * {@code local}, and fails on any other type. */ static CatalogConnector catalog(ServerProperties properties) { String type = properties.get(Property.CATALOG_TYPE); diff --git a/server/opensharing-server/src/main/java/io/opensharing/auth/ProviderTokenDecorator.java b/server/opensharing-server/src/main/java/io/opensharing/auth/ProviderTokenDecorator.java new file mode 100644 index 000000000..6f8c6fad8 --- /dev/null +++ b/server/opensharing-server/src/main/java/io/opensharing/auth/ProviderTokenDecorator.java @@ -0,0 +1,66 @@ +package io.opensharing.auth; + +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.auth.OAuth2Token; +import com.linecorp.armeria.server.DecoratingHttpServiceFunction; +import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.ServiceRequestContext; +import com.linecorp.armeria.server.auth.AuthTokenExtractors; +import io.netty.util.AttributeKey; +import io.opensharing.catalog.CatalogConnector; +import io.opensharing.http.ApiException; + +/** + * Authenticates a provider request through the catalog, which checks the bearer token and returns + * the caller; services read it with {@link #user}. Missing or rejected tokens get a 401. + */ +public final class ProviderTokenDecorator implements DecoratingHttpServiceFunction { + + private static final AttributeKey USER = + AttributeKey.valueOf(ProviderTokenDecorator.class, "user"); + + private final CatalogConnector catalog; + + public ProviderTokenDecorator(CatalogConnector catalog) { + this.catalog = catalog; + } + + @Override + public HttpResponse serve(HttpService delegate, ServiceRequestContext ctx, HttpRequest req) { + OAuth2Token token = AuthTokenExtractors.oAuth2().apply(req.headers()); + if (token == null || token.accessToken().isBlank()) { + throw ApiException.unauthenticated("bearer token is required"); + } + // A catalog may call out to a remote service, which must not block the event loop. + return HttpResponse.of( + () -> { + try { + ctx.setAttr(USER, authenticate(token.accessToken())); + return delegate.serve(ctx, req); + } catch (Exception e) { + return HttpResponse.ofFailure(e); + } + }, + ctx.blockingTaskExecutor()); + } + + /** + * Returns the caller this decorator authenticated for the request. Only services mounted behind + * this decorator may call it; anywhere else there is no caller, so it fails with a 401 rather + * than letting the service run anonymously. + */ + public static UserContext user(ServiceRequestContext ctx) { + UserContext user = ctx.attr(USER); + if (user == null) { + throw ApiException.unauthenticated("this endpoint requires an authenticated user"); + } + return user; + } + + // Asks the catalog who owns the token, with no privilege so that only the token is checked. The + // catalog's userId becomes the caller's identity, such as the owner of the shares they create. + private UserContext authenticate(String token) { + return catalog.authorize(new AuthContext(null, new UserContext(null, token, null)), null); + } +} diff --git a/server/opensharing-server/src/main/java/io/opensharing/config/ServerProperties.java b/server/opensharing-server/src/main/java/io/opensharing/config/ServerProperties.java index c374fbe99..633f5719c 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/config/ServerProperties.java +++ b/server/opensharing-server/src/main/java/io/opensharing/config/ServerProperties.java @@ -22,6 +22,7 @@ public class ServerProperties { public enum Property { PORT("server.port", "8080"), + PROVIDER_BASE_PATH("provider.base-path", "/api/1.0/opensharing/provider"), CATALOG_TYPE("catalog.type", "local"), LOCAL_CATALOG_FILE("catalog.local.file", "classpath:local-catalog.yml"); diff --git a/server/opensharing-server/src/main/java/io/opensharing/persistence/JpaTransactions.java b/server/opensharing-server/src/main/java/io/opensharing/persistence/JpaTransactions.java new file mode 100644 index 000000000..5f4b37123 --- /dev/null +++ b/server/opensharing-server/src/main/java/io/opensharing/persistence/JpaTransactions.java @@ -0,0 +1,50 @@ +package io.opensharing.persistence; + +import io.opensharing.Transactions; +import jakarta.persistence.EntityManager; +import java.util.List; +import java.util.function.Function; +import org.hibernate.Session; +import org.hibernate.SessionFactory; +import org.hibernate.Transaction; +import org.hibernate.cfg.Configuration; + +/** + * Runs core store work in Hibernate transactions, one session each. Hibernate reads its settings + * from {@code hibernate.properties} on the classpath. + */ +public final class JpaTransactions implements Transactions, AutoCloseable { + + private final SessionFactory sessions; + + public JpaTransactions(List> entities) { + Configuration configuration = new Configuration(); + entities.forEach(configuration::addAnnotatedClass); + this.sessions = configuration.buildSessionFactory(); + } + + @Override + public T inTransaction(boolean readOnly, Function work) { + try (Session session = sessions.openSession()) { + session.setDefaultReadOnly(readOnly); + Transaction transaction = session.beginTransaction(); + try { + T result = work.apply(session); + transaction.commit(); + return result; + } catch (RuntimeException | Error e) { + if (transaction.isActive()) { + transaction.rollback(); + } + throw e; + } + } + } + + @Override + public void close() { + if (sessions.isOpen()) { + sessions.close(); + } + } +} 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 new file mode 100644 index 000000000..478f4dcec --- /dev/null +++ b/server/opensharing-server/src/main/java/io/opensharing/share/ShareAdminService.java @@ -0,0 +1,63 @@ +package io.opensharing.share; + +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.server.ServiceRequestContext; +import com.linecorp.armeria.server.annotation.Blocking; +import com.linecorp.armeria.server.annotation.Delete; +import com.linecorp.armeria.server.annotation.Get; +import com.linecorp.armeria.server.annotation.Param; +import com.linecorp.armeria.server.annotation.Patch; +import com.linecorp.armeria.server.annotation.Post; +import com.linecorp.armeria.server.annotation.ProducesJson; +import com.linecorp.armeria.server.annotation.StatusCode; +import io.opensharing.auth.ProviderTokenDecorator; +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. + */ +@Blocking +@ProducesJson +public final class ShareAdminService { + + private final ShareService shares; + + public ShareAdminService(ShareService shares) { + this.shares = shares; + } + + /** {@code POST /shares}: creates a share owned by the caller. Needs CREATE_SHARE. */ + @Post("/shares") + @StatusCode(201) + public ShareResponse create(ServiceRequestContext ctx, CreateShareRequest request) { + return shares.create(ProviderTokenDecorator.user(ctx), request); + } + + /** {@code GET /shares}: lists every share by name, unpaged. */ + @Get("/shares") + public ListResponse list() { + return shares.list(); + } + + /** {@code GET /shares/{share}}: gets a share by name in any case. */ + @Get("/shares/{share}") + public ShareResponse get(@Param String share) { + return shares.get(share); + } + + /** {@code PATCH /shares/{share}}: updates the fields set in the body. Owner only. */ + @Patch("/shares/{share}") + public ShareResponse update( + ServiceRequestContext ctx, @Param String share, UpdateShareRequest request) { + return shares.update(ProviderTokenDecorator.user(ctx), share, request); + } + + /** {@code DELETE /shares/{share}}: deletes the share. Owner only. */ + @Delete("/shares/{share}") + public HttpResponse delete(ServiceRequestContext ctx, @Param String share) { + shares.delete(ProviderTokenDecorator.user(ctx), share); + return HttpResponse.of(HttpStatus.NO_CONTENT); + } +} diff --git a/server/opensharing-server/src/main/resources/hibernate.properties b/server/opensharing-server/src/main/resources/hibernate.properties new file mode 100644 index 000000000..0bf1b59fa --- /dev/null +++ b/server/opensharing-server/src/main/resources/hibernate.properties @@ -0,0 +1,14 @@ +## OpenSharing standalone server database settings, read by Hibernate from the classpath. +## A JVM system property with the same name as a key overrides it. + +# The in-memory database lives while the server's pool holds connections to it, so every server +# starts empty and loses its data on shutdown. The driver is derived from the URL, so pointing this +# at a file, Postgres or MySQL is the only change needed to keep the data. +hibernate.connection.url=jdbc:h2:mem:opensharing +hibernate.connection.username=sa +hibernate.connection.password= +# Hibernate's built-in pool is not for production. +hibernate.connection.provider_class=org.hibernate.hikaricp.internal.HikariCPConnectionProvider +# Creates missing tables and columns to match the entities on startup. Production should use +# validate and manage the schema with a migration tool. +hibernate.hbm2ddl.auto=update diff --git a/server/opensharing-server/src/main/resources/local-catalog.yml b/server/opensharing-server/src/main/resources/local-catalog.yml index 920e65480..005f27b68 100644 --- a/server/opensharing-server/src/main/resources/local-catalog.yml +++ b/server/opensharing-server/src/main/resources/local-catalog.yml @@ -32,3 +32,10 @@ assets: format: delta shareableBy: - catalog-alice-id + + - identifier: main.sales.orders + type: TABLE + attributes: + subtype: MANAGED + storageLocation: s3://test/main.sales.orders/ + format: delta diff --git a/server/opensharing-server/src/main/resources/server.properties b/server/opensharing-server/src/main/resources/server.properties index 8882e4c57..e6e5c2a0d 100644 --- a/server/opensharing-server/src/main/resources/server.properties +++ b/server/opensharing-server/src/main/resources/server.properties @@ -6,6 +6,9 @@ # HTTP port the server listens on; 0 picks a free one. server.port=8080 +# Path prefix of the provider API, whose bearer tokens the catalog authenticates. +provider.base-path=/api/1.0/opensharing/provider + # Which CatalogConnector the server uses; local reads assets from a YAML file. catalog.type=local # The YAML file the local catalog reads: a classpath: resource or a file path. diff --git a/server/opensharing-server/src/test/java/io/opensharing/TestServer.java b/server/opensharing-server/src/test/java/io/opensharing/TestServer.java new file mode 100644 index 000000000..ec381911f --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/TestServer.java @@ -0,0 +1,94 @@ +package io.opensharing; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.common.RequestHeadersBuilder; +import com.linecorp.armeria.common.ResponseHeaders; +import io.opensharing.config.ServerProperties; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.Properties; + +/** A standalone server on a free port, and a client that calls it as a provider. */ +public final class TestServer implements AutoCloseable { + + private static final ObjectMapper JSON = new ObjectMapper(); + + private final OpenSharingServer server; + private final WebClient client; + + private TestServer(OpenSharingServer server) { + this.server = server; + this.client = WebClient.of("http://127.0.0.1:" + server.port()); + } + + public static TestServer start() { + Properties props = new Properties(); + props.setProperty("server.port", "0"); + return new TestServer(OpenSharingServer.start(new ServerProperties(props))); + } + + public OpenSharingServer server() { + return server; + } + + public Response get(String path, String token) { + return call(HttpMethod.GET, path, token, null); + } + + public Response post(String path, String token, String json) { + return call(HttpMethod.POST, path, token, json); + } + + public Response patch(String path, String token, String json) { + return call(HttpMethod.PATCH, path, token, json); + } + + public Response delete(String path, String token) { + return call(HttpMethod.DELETE, path, token, null); + } + + /** Calls {@code path} with {@code token} as the bearer token, unless it is null. */ + public Response call(HttpMethod method, String path, String token, String json) { + RequestHeadersBuilder headers = RequestHeaders.builder(method, path); + if (token != null) { + headers.add(HttpHeaderNames.AUTHORIZATION, "Bearer " + token); + } + HttpRequest request = + json == null + ? HttpRequest.of(headers.build()) + : HttpRequest.of( + headers.contentType(MediaType.JSON_UTF_8).build(), HttpData.ofUtf8(json)); + return Response.of(client.execute(request).aggregate().join()); + } + + @Override + public void close() { + server.close(); + } + + /** A response with its body parsed as JSON, or a missing node when it has none. */ + public record Response(int status, ResponseHeaders headers, JsonNode body) { + + static Response of(AggregatedHttpResponse response) { + try { + return new Response( + response.status().code(), response.headers(), JSON.readTree(response.contentUtf8())); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + public String errorCode() { + return body.path("errorCode").asText(); + } + } +} diff --git a/server/opensharing-server/src/test/java/io/opensharing/auth/ProviderTokenDecoratorTest.java b/server/opensharing-server/src/test/java/io/opensharing/auth/ProviderTokenDecoratorTest.java new file mode 100644 index 000000000..9550b9642 --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/auth/ProviderTokenDecoratorTest.java @@ -0,0 +1,143 @@ +package io.opensharing.auth; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.RequestHeaders; +import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.ServiceRequestContext; +import io.opensharing.catalog.CatalogConnector; +import io.opensharing.exception.CatalogAuthenticationException; +import io.opensharing.exception.CatalogException; +import io.opensharing.http.ApiExceptionHandler; +import io.opensharing.http.ErrorCodes; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; +import org.junit.jupiter.api.Test; + +class ProviderTokenDecoratorTest { + + private static final UserContext ALICE = UserContext.fromUserIdAndName("alice-id", "alice"); + + private final ObjectMapper objectMapper = new ObjectMapper(); + + @Test + void rejectsTokenTheCatalogCannotAuthenticate() throws Exception { + AggregatedHttpResponse response = + serve(failing(new CatalogAuthenticationException("invalid bearer token")), "token"); + + assertEquals(401, response.status().code()); + assertEquals("Bearer", response.headers().get("WWW-Authenticate")); + assertEquals(ErrorCodes.UNAUTHENTICATED, body(response).get("errorCode").asText()); + } + + @Test + void mapsCatalogFailureToBadGateway() throws Exception { + AggregatedHttpResponse response = + serve(failing(new CatalogException("catalog unreachable")), "token"); + + assertEquals(502, response.status().code()); + assertNull(response.headers().get("WWW-Authenticate")); + JsonNode body = body(response); + assertEquals(ErrorCodes.CATALOG_ERROR, body.get("errorCode").asText()); + assertEquals("catalog unreachable", body.get("message").asText()); + } + + @Test + void mapsUnexpectedFailureToInternalError() throws Exception { + AggregatedHttpResponse response = serve(failing(new IllegalStateException("boom")), "token"); + + assertEquals(500, response.status().code()); + assertEquals(ErrorCodes.INTERNAL_ERROR, body(response).get("errorCode").asText()); + } + + @Test + void rejectsMissingTokenWithoutCallingTheCatalog() { + List privileges = new ArrayList<>(); + + AggregatedHttpResponse response = serve(authorizing(ALICE, privileges), null); + + assertEquals(401, response.status().code()); + assertTrue(privileges.isEmpty()); + } + + @Test + void onlyAuthenticatesAndHandsTheCallerToTheService() { + List privileges = new ArrayList<>(); + AtomicReference served = new AtomicReference<>(); + HttpService service = + (ctx, req) -> { + served.set(ProviderTokenDecorator.user(ctx)); + return HttpResponse.of(HttpStatus.NO_CONTENT); + }; + + AggregatedHttpResponse response = + serve(service, authorizing(ALICE, privileges), HttpMethod.POST, "token"); + + assertEquals(204, response.status().code()); + assertSame(ALICE, served.get()); + assertEquals(Arrays.asList((Privilege) null), privileges); + } + + private static AggregatedHttpResponse serve(CatalogConnector catalog, String token) { + return serve((ctx, req) -> HttpResponse.of(HttpStatus.OK), catalog, HttpMethod.GET, token); + } + + private static AggregatedHttpResponse serve( + HttpService service, CatalogConnector catalog, HttpMethod method, String token) { + RequestHeaders headers = + token == null + ? RequestHeaders.of(method, "/shares") + : RequestHeaders.of(method, "/shares", "authorization", "Bearer " + token); + HttpRequest request = HttpRequest.of(headers); + HttpService decorated = + service + .decorate(new ProviderTokenDecorator(catalog)) + .decorate(new ApiExceptionHandler()); + try { + return decorated.serve(ServiceRequestContext.of(request), request).aggregate().join(); + } catch (Exception e) { + throw new AssertionError(e); + } + } + + private static CatalogConnector authorizing(UserContext user, List privileges) { + return catalog( + args -> { + privileges.add((Privilege) args[1]); + return user; + }); + } + + private static CatalogConnector failing(RuntimeException failure) { + return catalog( + args -> { + throw failure; + }); + } + + private static CatalogConnector catalog(Function authorize) { + return (CatalogConnector) + Proxy.newProxyInstance( + CatalogConnector.class.getClassLoader(), + new Class[] {CatalogConnector.class}, + (proxy, method, args) -> authorize.apply(args)); + } + + private JsonNode body(AggregatedHttpResponse response) throws Exception { + return objectMapper.readTree(response.contentUtf8()); + } +} diff --git a/server/opensharing-server/src/test/java/io/opensharing/share/ShareAdminServiceTest.java b/server/opensharing-server/src/test/java/io/opensharing/share/ShareAdminServiceTest.java new file mode 100644 index 000000000..c50bfb44b --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/share/ShareAdminServiceTest.java @@ -0,0 +1,170 @@ +package io.opensharing.share; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +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 ShareAdminServiceTest { + + private static final String SHARES = "/api/1.0/opensharing/provider/shares"; + + private static TestServer server; + + @BeforeAll + static void startServer() { + server = TestServer.start(); + } + + @AfterAll + static void stopServer() { + server.close(); + } + + @Test + void requiresACatalogAuthorizedPrincipal() { + // Missing bearer token is unauthenticated. + Response missing = server.get(SHARES, null); + assertEquals(401, missing.status()); + assertEquals(ErrorCodes.UNAUTHENTICATED, missing.errorCode()); + + // A token the catalog does not know is unauthenticated, and names the Bearer scheme. + Response unknown = server.get(SHARES, "nobody-token"); + assertEquals(401, unknown.status()); + assertEquals("Bearer", unknown.headers().get("WWW-Authenticate")); + assertEquals(ErrorCodes.UNAUTHENTICATED, unknown.errorCode()); + } + + @Test + void rejectsADuplicateShareNameInAnyCase() { + // Owner creates a share. + assertEquals(201, create("alice-token", "{\"name\":\"taxes\"}").status()); + + // The same name in another case already exists, even for another caller. + Response duplicate = create("bob-token", "{\"name\":\"TAXES\"}"); + assertEquals(409, duplicate.status()); + assertEquals(ErrorCodes.RESOURCE_ALREADY_EXISTS, duplicate.errorCode()); + } + + @Test + void rejectsInvalidShareNames() { + // Blank names fail request validation. + assertInvalid(create("alice-token", "{\"name\":\"\"}")); + + // Names with a space or a forward slash are rejected. + assertInvalid(create("alice-token", "{\"name\":\"bad name\"}")); + assertInvalid(create("alice-token", "{\"name\":\"bad/name\"}")); + } + + @Test + void rejectsNullPropertyValues() { + // Creating with a null property value is rejected. + assertInvalid(create("alice-token", "{\"name\":\"nulls\",\"properties\":{\"team\":null}}")); + + // Updating with a null property value is rejected too. + assertEquals(201, create("alice-token", "{\"name\":\"nulls\"}").status()); + assertInvalid( + server.patch(SHARES + "/nulls", "alice-token", "{\"properties\":{\"team\":null}}")); + } + + @Test + void updatesOnlyTheFieldsInTheBody() { + // Owner creates a share with every field set. + Response created = + create( + "alice-token", + "{\"name\":\"travel\",\"displayName\":\"Travel\",\"comment\":\"v1\"," + + "\"properties\":{\"team\":\"finance\"}}"); + assertEquals(201, created.status()); + + // Owner updates only the comment; the other fields are kept. + Response updated = server.patch(SHARES + "/travel", "alice-token", "{\"comment\":\"v2\"}"); + assertEquals(200, updated.status()); + assertEquals("v2", updated.body().path("comment").asText()); + assertEquals("Travel", updated.body().path("displayName").asText()); + assertEquals("finance", updated.body().path("properties").path("team").asText()); + } + + @Test + void onlyTheOwnerDeletesAShare() { + // Owner creates a share. + assertEquals(201, create("alice-token", "{\"name\":\"vendors\"}").status()); + + // Non-owner cannot delete. + Response denied = server.delete(SHARES + "/vendors", "bob-token"); + assertEquals(403, denied.status()); + assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); + + // The share is still there. + assertEquals(200, server.get(SHARES + "/vendors", "bob-token").status()); + } + + @Test + void answersMissingSharesWithNotFound() { + // Updating a missing share is 404. + Response update = server.patch(SHARES + "/missing", "alice-token", "{\"comment\":\"x\"}"); + assertEquals(404, update.status()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, update.errorCode()); + + // Deleting a missing share is 404. + Response delete = server.delete(SHARES + "/missing", "alice-token"); + assertEquals(404, delete.status()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, delete.errorCode()); + } + + @Test + void createsListsUpdatesAndDeletesAShare() { + // Owner creates a share; name is persisted lowercase. + Response created = create("alice-token", "{\"name\":\"Sales\",\"comment\":\"orders\"}"); + assertEquals(201, created.status()); + assertEquals("sales", created.body().path("name").asText()); + assertFalse(created.body().path("id").isMissingNode()); + + // Owner lists shares. + Response list = server.get(SHARES, "alice-token"); + assertEquals(200, list.status()); + assertEquals( + 1, + list.body().path("items").findValuesAsText("name").stream() + .filter("sales"::equals) + .count()); + + // Get by name is case-insensitive and allowed for any catalog principal. + Response fetched = server.get(SHARES + "/SALES", "bob-token"); + assertEquals(200, fetched.status()); + assertEquals("sales", fetched.body().path("name").asText()); + assertEquals("orders", fetched.body().path("comment").asText()); + + // Non-owner cannot update. + Response stolen = server.patch(SHARES + "/sales", "bob-token", "{\"comment\":\"stolen\"}"); + assertEquals(403, stolen.status()); + assertEquals(ErrorCodes.PERMISSION_DENIED, stolen.errorCode()); + + // Owner updates the comment. + Response updated = server.patch(SHARES + "/sales", "alice-token", "{\"comment\":\"updated\"}"); + assertEquals(200, updated.status()); + assertEquals("updated", updated.body().path("comment").asText()); + + // Owner deletes the share. + assertEquals(204, server.delete(SHARES + "/sales", "alice-token").status()); + + // Deleted share is 404. + Response deleted = server.get(SHARES + "/sales", "alice-token"); + assertEquals(404, deleted.status()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, deleted.errorCode()); + } + + private static Response create(String token, String body) { + return server.post(SHARES, token, body); + } + + private static void assertInvalid(Response response) { + assertEquals(400, response.status()); + assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, response.errorCode()); + } +} diff --git a/server/pom.xml b/server/pom.xml index 287199aa4..40db2c2cf 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -32,6 +32,11 @@ 4.1.126.Final 2.19.1 3.1.0 + 6.6.18.Final + 6.3.0 + 2.3.232 + 42.7.7 + 9.2.0 2.0.17 1.5.18 5.12.2 @@ -81,6 +86,36 @@ jakarta.persistence-api ${jakarta-persistence.version} + + org.hibernate.orm + hibernate-core + ${hibernate.version} + + + org.hibernate.orm + hibernate-hikaricp + ${hibernate.version} + + + com.zaxxer + HikariCP + ${hikaricp.version} + + + com.h2database + h2 + ${h2.version} + + + org.postgresql + postgresql + ${postgresql.version} + + + com.mysql + mysql-connector-j + ${mysql.version} + org.slf4j slf4j-api From 2df60b36dd1e5e7e9719fd7639c219f82b5ee14e Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 19 Oct 2026 14:01:37 -0700 Subject: [PATCH 2/3] Lock a share's row for owner changes, so concurrent changes to one share run one at a time. --- .../java/io/opensharing/share/ShareStore.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) 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 4e937375f..b5a10ec0f 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 @@ -5,6 +5,7 @@ import io.opensharing.auth.UserContext; import io.opensharing.http.ApiException; import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; import java.util.List; import java.util.Map; import java.util.Optional; @@ -75,17 +76,17 @@ public ShareEntity update( /** Looks up a share by name in any case. */ public Optional find(String name) { - return tx.inTransaction(true, em -> find(em, name)); + return tx.inTransaction(true, em -> find(em, name, LockModeType.NONE)); } /** Like {@link #find}, but fails with not-found when the share does not exist. */ public ShareEntity require(String name) { - return tx.inTransaction(true, em -> require(em, name)); + return tx.inTransaction(true, em -> require(em, name, LockModeType.NONE)); } /** Like {@link #require}, but also fails unless {@code user} owns the share. */ public ShareEntity requireOwned(String name, UserContext user) { - return tx.inTransaction(true, em -> requireOwned(em, name, user)); + return tx.inTransaction(true, em -> requireOwner(require(em, name, LockModeType.NONE), user)); } /** {@code SELECT * FROM os_shares ORDER BY name ASC}: every share, regardless of owner. */ @@ -116,21 +117,29 @@ private static boolean exists(EntityManager em, String name) { .isEmpty(); } - /** {@code SELECT * FROM os_shares WHERE name = ?} */ - private static Optional find(EntityManager em, String name) { + /** {@code SELECT * FROM os_shares WHERE name = ?}, plus {@code FOR UPDATE} under a lock. */ + private static Optional find(EntityManager em, String name, LockModeType lock) { return em.createQuery("select s from ShareEntity s where s.name = :name", ShareEntity.class) .setParameter("name", ObjectNames.normalize(name)) + .setLockMode(lock) .getResultStream() .findFirst(); } - private static ShareEntity require(EntityManager em, String name) { - return find(em, name) + private static ShareEntity require(EntityManager em, String name, LockModeType lock) { + return find(em, name, lock) .orElseThrow(() -> ApiException.notFound("share '" + name + "' does not exist")); } + /** + * Locks the row of a share the caller owns, so changes to one share run one at a time and each + * sees what the one before it wrote. + */ private static ShareEntity requireOwned(EntityManager em, String name, UserContext user) { - ShareEntity share = require(em, name); + return requireOwner(require(em, name, LockModeType.PESSIMISTIC_WRITE), user); + } + + private static ShareEntity requireOwner(ShareEntity share, UserContext user) { user.requireOwner(share.getOwnerId(), "share '" + share.getName() + "'"); return share; } From 800e6854475acdbcfd2adeef1131a4d471666d37 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 19 Oct 2026 16:45:56 -0700 Subject: [PATCH 3/3] Move the owner check into Ownership, deny a missing owner id, and simplify store comments. --- .../java/io/opensharing/auth/Ownership.java | 16 +++++++++ .../java/io/opensharing/auth/UserContext.java | 8 ----- .../io/opensharing/runtime/OpenSharing.java | 6 +++- .../io/opensharing/share/ShareEntity.java | 4 +-- .../io/opensharing/share/ShareService.java | 2 +- .../java/io/opensharing/share/ShareStore.java | 33 +++++++++-------- .../io/opensharing/auth/OwnershipTest.java | 36 +++++++++++++++++++ 7 files changed, 76 insertions(+), 29 deletions(-) create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/auth/Ownership.java create mode 100644 server/opensharing-server-core/src/test/java/io/opensharing/auth/OwnershipTest.java diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/auth/Ownership.java b/server/opensharing-server-core/src/main/java/io/opensharing/auth/Ownership.java new file mode 100644 index 000000000..482a0e3db --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/auth/Ownership.java @@ -0,0 +1,16 @@ +package io.opensharing.auth; + +import io.opensharing.http.ApiException; + +/** Ownership checks for objects OpenSharing stores. */ +public final class Ownership { + + private Ownership() {} + + /** Passes only when {@code user} has the id {@code ownerId}; a missing id never matches. */ + public static void check(UserContext user, String ownerId, String what) { + if (user.userId() == null || ownerId == null || !ownerId.equals(user.userId())) { + throw ApiException.permissionDenied("user '" + user.userName() + "' does not own " + what); + } + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java b/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java index 683a1c066..a3bd13e71 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/auth/UserContext.java @@ -1,7 +1,5 @@ package io.opensharing.auth; -import io.opensharing.http.ApiException; - /** * End-user identity for a catalog call. * @@ -32,10 +30,4 @@ public String toString() { + userName + "]"; } - - public void requireOwner(String ownerId, String what) { - if (userId == null || !ownerId.equals(userId)) { - throw ApiException.permissionDenied("user '" + userName + "' does not own " + what); - } - } } 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 9607b525c..9d88eedef 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 @@ -52,7 +52,11 @@ public Builder catalog(CatalogConnector catalog) { return this; } - /** The host's database transactions (required). */ + /** + * How OpenSharing reaches the host's database. OpenSharing keeps its own tables there and runs + * every read and write through these transactions, so the host's persistence setup must + * register the classes from {@link OpenSharing#entities}. Must be set before {@link #build}. + */ public Builder transactions(Transactions transactions) { this.transactions = transactions; return this; diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java index 083f1a6bd..16640028e 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/share/ShareEntity.java @@ -32,8 +32,8 @@ public class ShareEntity extends BaseEntity { private String comment; /** - * The catalog's user id for whoever created this share. Not a foreign key: there is no principal - * table. + * The catalog's user id for whoever created this share. It is not kept in sync with the catalog, + * so it goes stale if the catalog removes the user. */ @Column(name = "owner_id", nullable = false, length = 255) private String ownerId; 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 01bf17107..fc1c4d70d 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 @@ -46,7 +46,7 @@ public ListResponse list() { /** Gets a share by name in any case. */ public ShareResponse get(String share) { - return ShareResponse.from(shares.require(share)); + return ShareResponse.from(shares.getOrThrow(share)); } /** Updates the fields set in {@code request}. Owner only. */ 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 b5a10ec0f..93210f470 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 @@ -2,6 +2,7 @@ import io.opensharing.ObjectNames; import io.opensharing.Transactions; +import io.opensharing.auth.Ownership; import io.opensharing.auth.UserContext; import io.opensharing.http.ApiException; import jakarta.persistence.EntityManager; @@ -12,8 +13,8 @@ /** * Storage for shares. Names are stored lowercase and looked up case-insensitively. Each method runs - * in its own transaction, so returned entities are detached and must be fully loaded. Each loaded - * share also runs {@code SELECT * FROM os_share_properties WHERE share_id = ?}. + * in its own transaction and returns shares with their properties loaded, so callers can read them + * after it ends. */ public class ShareStore { @@ -60,7 +61,7 @@ public ShareEntity update( return tx.inTransaction( false, em -> { - ShareEntity share = requireOwned(em, name, user); + ShareEntity share = getOwnedOrThrow(em, name, user); if (displayName != null) { share.setDisplayName(displayName); } @@ -80,16 +81,16 @@ public Optional find(String name) { } /** Like {@link #find}, but fails with not-found when the share does not exist. */ - public ShareEntity require(String name) { - return tx.inTransaction(true, em -> require(em, name, LockModeType.NONE)); + public ShareEntity getOrThrow(String name) { + return tx.inTransaction(true, em -> getOrThrow(em, name, LockModeType.NONE)); } - /** Like {@link #require}, but also fails unless {@code user} owns the share. */ - public ShareEntity requireOwned(String name, UserContext user) { - return tx.inTransaction(true, em -> requireOwner(require(em, name, LockModeType.NONE), user)); + /** Like {@link #getOrThrow}, but also fails unless {@code user} owns the share. */ + public ShareEntity getOwnedOrThrow(String name, UserContext user) { + return tx.inTransaction(true, em -> checkOwner(getOrThrow(em, name, LockModeType.NONE), user)); } - /** {@code SELECT * FROM os_shares ORDER BY name ASC}: every share, regardless of owner. */ + /** Every share, regardless of owner, ordered by name. */ public List list() { return tx.inTransaction( true, @@ -103,12 +104,11 @@ public void delete(String name, UserContext user) { tx.inTransaction( false, em -> { - em.remove(requireOwned(em, name, user)); + em.remove(getOwnedOrThrow(em, name, user)); return null; }); } - /** {@code SELECT id FROM os_shares WHERE name = ? LIMIT 1} */ private static boolean exists(EntityManager em, String name) { return !em.createQuery("select s.id from ShareEntity s where s.name = :name", String.class) .setParameter("name", name) @@ -117,7 +117,6 @@ private static boolean exists(EntityManager em, String name) { .isEmpty(); } - /** {@code SELECT * FROM os_shares WHERE name = ?}, plus {@code FOR UPDATE} under a lock. */ private static Optional find(EntityManager em, String name, LockModeType lock) { return em.createQuery("select s from ShareEntity s where s.name = :name", ShareEntity.class) .setParameter("name", ObjectNames.normalize(name)) @@ -126,7 +125,7 @@ private static Optional find(EntityManager em, String name, LockMod .findFirst(); } - private static ShareEntity require(EntityManager em, String name, LockModeType lock) { + private static ShareEntity getOrThrow(EntityManager em, String name, LockModeType lock) { return find(em, name, lock) .orElseThrow(() -> ApiException.notFound("share '" + name + "' does not exist")); } @@ -135,12 +134,12 @@ private static ShareEntity require(EntityManager em, String name, LockModeType l * Locks the row of a share the caller owns, so changes to one share run one at a time and each * sees what the one before it wrote. */ - private static ShareEntity requireOwned(EntityManager em, String name, UserContext user) { - return requireOwner(require(em, name, LockModeType.PESSIMISTIC_WRITE), user); + private static ShareEntity getOwnedOrThrow(EntityManager em, String name, UserContext user) { + return checkOwner(getOrThrow(em, name, LockModeType.PESSIMISTIC_WRITE), user); } - private static ShareEntity requireOwner(ShareEntity share, UserContext user) { - user.requireOwner(share.getOwnerId(), "share '" + share.getName() + "'"); + private static ShareEntity checkOwner(ShareEntity share, UserContext user) { + Ownership.check(user, share.getOwnerId(), "share '" + share.getName() + "'"); return share; } } diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/auth/OwnershipTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/auth/OwnershipTest.java new file mode 100644 index 000000000..6a7d0216d --- /dev/null +++ b/server/opensharing-server-core/src/test/java/io/opensharing/auth/OwnershipTest.java @@ -0,0 +1,36 @@ +package io.opensharing.auth; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.opensharing.http.ApiException; +import org.junit.jupiter.api.Test; + +class OwnershipTest { + + private static final UserContext ALICE = UserContext.fromUserIdAndName("alice-id", "alice"); + + @Test + void passesForTheOwner() { + assertDoesNotThrow(() -> Ownership.check(ALICE, "alice-id", "share 's'")); + } + + @Test + void deniesAnotherUser() { + ApiException e = + assertThrows(ApiException.class, () -> Ownership.check(ALICE, "bob-id", "share 's'")); + + assertEquals(403, e.getStatus()); + assertEquals("user 'alice' does not own share 's'", e.getMessage()); + } + + @Test + void deniesWhenEitherIdIsMissing() { + UserContext anonymous = UserContext.fromUserIdAndName(null, "anonymous"); + + assertThrows(ApiException.class, () -> Ownership.check(ALICE, null, "share 's'")); + assertThrows(ApiException.class, () -> Ownership.check(anonymous, "alice-id", "share 's'")); + assertThrows(ApiException.class, () -> Ownership.check(anonymous, null, "share 's'")); + } +}