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 index 482a0e3d..67cc7fc9 100644 --- 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 @@ -7,9 +7,14 @@ public final class Ownership { private Ownership() {} - /** Passes only when {@code user} has the id {@code ownerId}; a missing id never matches. */ + /** Whether {@code user} has the id {@code ownerId}; a missing id never matches. */ + public static boolean owns(UserContext user, String ownerId) { + return user.userId() != null && user.userId().equals(ownerId); + } + + /** Passes only when {@link #owns} does. */ public static void check(UserContext user, String ownerId, String what) { - if (user.userId() == null || ownerId == null || !ownerId.equals(user.userId())) { + if (!owns(user, ownerId)) { throw ApiException.permissionDenied("user '" + user.userName() + "' does not own " + what); } } diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/auth/Privilege.java b/server/opensharing-server-core/src/main/java/io/opensharing/auth/Privilege.java index c0f0b07d..a1088bbe 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/auth/Privilege.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/auth/Privilege.java @@ -3,5 +3,7 @@ /** Catalog-wide privileges OpenSharing checks; each connector maps them to its own catalog's. */ public enum Privilege { CREATE_SHARE, - CREATE_RECIPIENT + CREATE_RECIPIENT, + /** Lists and reads every share, not only the caller's own. */ + USE_SHARE } 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 4edb3860..04072186 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 @@ -4,12 +4,14 @@ import io.opensharing.Transactions; import io.opensharing.asset.SharedDataObjectStore; import io.opensharing.auth.AuthContext; +import io.opensharing.auth.Ownership; import io.opensharing.auth.Privilege; import io.opensharing.auth.UserContext; import io.opensharing.catalog.Asset; import io.opensharing.catalog.AssetType; import io.opensharing.catalog.CatalogConnector; import io.opensharing.catalog.ResolvedAsset; +import io.opensharing.exception.CatalogAuthorizationException; import io.opensharing.exception.CatalogException; import io.opensharing.http.ApiException; import io.opensharing.http.ListResponse; @@ -25,8 +27,9 @@ /** * 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. + * share needs CREATE_SHARE from the catalog, and only the owner may change it. The owner and + * callers with USE_SHARE from the catalog may read a share; listing without USE_SHARE returns only + * the caller's own shares. */ public class ShareService { @@ -66,15 +69,21 @@ public ShareResponse create(UserContext user, CreateShareRequest request) { validateProperties(request.properties()))); } - /** Lists one page of shares by name. */ - public ListResponse list(Integer maxResults, String pageToken) { - // TODO: check USE_SHARE through catalog.authorize. - return Pagination.page(maxResults, pageToken, shares::list, ShareResponse::from); + /** Lists one page of shares by name: every share with USE_SHARE, otherwise the caller's own. */ + public ListResponse list(UserContext user, Integer maxResults, String pageToken) { + Pagination.Slice slice = + holds(user, Privilege.USE_SHARE) + ? shares::list + : (offset, limit) -> shares.listOwnedBy(user.userId(), offset, limit); + return Pagination.page(maxResults, pageToken, slice, ShareResponse::from); } - /** Gets a share by name in any case, with the objects in it when {@code includeSharedData}. */ - public ShareResponse get(String share, boolean includeSharedData) { - ShareEntity entity = shares.getOrThrow(share); + /** + * Gets a share by name in any case, with the objects in it when {@code includeSharedData}. Owner + * or USE_SHARE. + */ + public ShareResponse get(UserContext user, String share, boolean includeSharedData) { + ShareEntity entity = requireReadable(user, share); return includeSharedData ? ShareResponse.from(entity, objects.list(entity)) : ShareResponse.from(entity); @@ -135,10 +144,17 @@ public void delete(UserContext user, String share) { shares.delete(share, user); } - /** Lists one page of who holds which privilege on the share, by recipient name. */ + /** + * Lists one page of who holds which privilege on the share, by recipient name. Owner or + * USE_SHARE. + */ public ListResponse listPermissions( - String share, Integer maxResults, String pageToken) { - ShareEntity entity = shares.getOrThrow(share); + UserContext user, String share, Integer maxResults, String pageToken) { + return permissionPage(requireReadable(user, share), maxResults, pageToken); + } + + private ListResponse permissionPage( + ShareEntity entity, Integer maxResults, String pageToken) { return Pagination.page( maxResults, pageToken, @@ -180,7 +196,24 @@ public ListResponse updatePermissions( } return null; }); - return listPermissions(share, null, null); + return permissionPage(shares.getOrThrow(share), null, null); + } + + private ShareEntity requireReadable(UserContext user, String share) { + ShareEntity entity = shares.getOrThrow(share); + if (!Ownership.owns(user, entity.getOwnerId())) { + catalog.authorize(AuthContext.of(user), Privilege.USE_SHARE); + } + return entity; + } + + private boolean holds(UserContext user, Privilege privilege) { + try { + catalog.authorize(AuthContext.of(user), privilege); + return true; + } catch (CatalogAuthorizationException e) { + return false; + } } // Resolves the object in the catalog on behalf of the caller and checks the declared type. 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 52df108e..4028b98b 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 @@ -94,6 +94,20 @@ public List list(int offset, int limit) { .getResultList()); } + /** Shares owned by {@code ownerId}, ordered by name. */ + public List listOwnedBy(String ownerId, int offset, int limit) { + return tx.inTransaction( + true, + em -> + em.createQuery( + "select s from ShareEntity s where s.ownerId = :ownerId order by s.name", + ShareEntity.class) + .setParameter("ownerId", ownerId) + .setFirstResult(offset) + .setMaxResults(limit) + .getResultList()); + } + /** 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( diff --git a/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java b/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java index ce8360c0..018a72d2 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java +++ b/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java @@ -11,6 +11,7 @@ import io.opensharing.catalog.AssetType; import io.opensharing.catalog.CatalogConnector; import io.opensharing.exception.CatalogAuthenticationException; +import io.opensharing.exception.CatalogAuthorizationException; import io.opensharing.exception.CatalogException; import io.opensharing.catalog.CloudProvider; import io.opensharing.catalog.CredentialRequest; @@ -205,13 +206,16 @@ public UserContext authorize(AuthContext auth, Privilege privilege) { if (token == null || token.isBlank()) { throw new CatalogAuthenticationException("missing bearer token"); } - return principals.stream() - .filter(principal -> token.equals(principal.bearerToken())) - .findFirst() - .map( - principal -> - new UserContext(principal.userId(), principal.bearerToken(), principal.userName())) - .orElseThrow(() -> new CatalogAuthenticationException("invalid bearer token")); + LocalCatalogFile.Principal principal = + principals.stream() + .filter(candidate -> token.equals(candidate.bearerToken())) + .findFirst() + .orElseThrow(() -> new CatalogAuthenticationException("invalid bearer token")); + if (privilege != null && !principal.privileges().contains(privilege)) { + throw new CatalogAuthorizationException( + "user '" + principal.userId() + "' does not hold " + privilege); + } + return new UserContext(principal.userId(), principal.bearerToken(), principal.userName()); } private static boolean covers(LocalCatalogFile.Asset asset, String location) { diff --git a/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogFile.java b/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogFile.java index 8783844c..8cdda32c 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogFile.java +++ b/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogFile.java @@ -1,5 +1,6 @@ package io.opensharing.catalog.local; +import io.opensharing.auth.Privilege; import io.opensharing.catalog.AssetType; import io.opensharing.catalog.CloudProvider; import io.opensharing.catalog.DataSourceFormat; @@ -25,13 +26,15 @@ public enum CredentialMode { STATIC } - /** Provider identity the local catalog accepts for admin APIs. */ - public record Principal(String bearerToken, String userId, String userName) { + /** Provider identity the local catalog accepts for admin APIs, holding only its privileges. */ + public record Principal( + String bearerToken, String userId, String userName, List privileges) { public Principal { if (userId == null || userId.isBlank()) { throw new IllegalArgumentException("local catalog principal is missing 'userId'"); } + privileges = privileges == null ? List.of() : List.copyOf(privileges); } } 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 973c2b98..8bf8b78c 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 @@ -17,8 +17,9 @@ import io.opensharing.http.ListResponse; /** - * Provider HTTP API for share CRUD and recipient grants, 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}. + * Only the owner may change a share. The owner and callers with USE_SHARE may read it; listing + * without USE_SHARE returns only the caller's own shares. */ @Blocking @ProducesJson @@ -37,22 +38,28 @@ public ShareResponse create(ServiceRequestContext ctx, CreateShareRequest reques return shares.create(ProviderTokenDecorator.user(ctx), request); } - /** {@code GET /shares}: lists one page of shares by name. */ + /** + * {@code GET /shares}: lists one page of shares by name: every share with USE_SHARE, otherwise + * the caller's own. + */ @Get("/shares") public ListResponse list( - @Param @Nullable Integer maxResults, @Param @Nullable String pageToken) { - return shares.list(maxResults, pageToken); + ServiceRequestContext ctx, + @Param @Nullable Integer maxResults, + @Param @Nullable String pageToken) { + return shares.list(ProviderTokenDecorator.user(ctx), maxResults, pageToken); } /** * {@code GET /shares/{share}}: gets a share by name in any case, with the objects in it when - * {@code include_shared_data} is true. + * {@code include_shared_data} is true. Owner or USE_SHARE. */ @Get("/shares/{share}") public ShareResponse get( + ServiceRequestContext ctx, @Param String share, @Param("include_shared_data") @Default("false") boolean includeSharedData) { - return shares.get(share, includeSharedData); + return shares.get(ProviderTokenDecorator.user(ctx), share, includeSharedData); } /** @@ -74,14 +81,15 @@ public HttpResponse delete(ServiceRequestContext ctx, @Param String share) { /** * {@code GET /shares/{share}/permissions}: lists one page of who holds which privilege on the - * share. + * share. Owner or USE_SHARE. */ @Get("/shares/{share}/permissions") public ListResponse listPermissions( + ServiceRequestContext ctx, @Param String share, @Param @Nullable Integer maxResults, @Param @Nullable String pageToken) { - return shares.listPermissions(share, maxResults, pageToken); + return shares.listPermissions(ProviderTokenDecorator.user(ctx), share, maxResults, pageToken); } /** diff --git a/server/opensharing-server/src/main/resources/local-catalog.yml b/server/opensharing-server/src/main/resources/local-catalog.yml index 005f27b6..28d73a21 100644 --- a/server/opensharing-server/src/main/resources/local-catalog.yml +++ b/server/opensharing-server/src/main/resources/local-catalog.yml @@ -5,13 +5,20 @@ credentials: mode: FAKE ttlSeconds: 3600 +# A principal holds only the privileges it lists. principals: - bearerToken: alice-token userId: catalog-alice-id userName: alice + privileges: [CREATE_SHARE, CREATE_RECIPIENT, USE_SHARE] - bearerToken: bob-token userId: catalog-bob-id userName: bob + privileges: [CREATE_SHARE, CREATE_RECIPIENT, USE_SHARE] + - bearerToken: carol-token + userId: catalog-carol-id + userName: carol + privileges: [CREATE_SHARE, CREATE_RECIPIENT] assets: - identifier: main.sales diff --git a/server/opensharing-server/src/test/java/io/opensharing/share/ShareReadPrivilegeTest.java b/server/opensharing-server/src/test/java/io/opensharing/share/ShareReadPrivilegeTest.java new file mode 100644 index 00000000..36f733e1 --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/share/ShareReadPrivilegeTest.java @@ -0,0 +1,60 @@ +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 java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** In the local catalog, alice and bob hold USE_SHARE and carol does not. */ +class ShareReadPrivilegeTest { + + private static final String SHARES = "/api/1.0/opensharing/provider/shares"; + + private static TestServer server; + + @BeforeAll + static void startServer() { + server = TestServer.start(); + assertEquals(201, server.post(SHARES, "alice-token", "{\"name\":\"alice_share\"}").status()); + assertEquals(201, server.post(SHARES, "carol-token", "{\"name\":\"carol_share\"}").status()); + } + + @AfterAll + static void stopServer() { + server.close(); + } + + @Test + void listsOnlyOwnSharesWithoutUseShare() { + assertEquals(List.of("carol_share"), names(server.get(SHARES, "carol-token"))); + assertEquals(List.of("alice_share", "carol_share"), names(server.get(SHARES, "bob-token"))); + } + + @Test + void readsOthersSharesOnlyWithUseShare() { + assertEquals(200, server.get(SHARES + "/carol_share", "carol-token").status()); + assertEquals(200, server.get(SHARES + "/carol_share/permissions", "carol-token").status()); + assertEquals(200, server.get(SHARES + "/carol_share", "bob-token").status()); + assertEquals(200, server.get(SHARES + "/carol_share/permissions", "bob-token").status()); + + Response share = server.get(SHARES + "/alice_share?include_shared_data=true", "carol-token"); + assertEquals(403, share.status()); + assertEquals(ErrorCodes.PERMISSION_DENIED, share.errorCode()); + Response permissions = server.get(SHARES + "/alice_share/permissions", "carol-token"); + assertEquals(403, permissions.status()); + assertEquals(ErrorCodes.PERMISSION_DENIED, permissions.errorCode()); + } + + private static List names(Response response) { + assertEquals(200, response.status()); + List names = new ArrayList<>(); + response.body().path("items").forEach(item -> names.add(item.path("name").asText())); + return names; + } +}