forbidden exception handling + owner of share authorized

duhizjame · duhizjame · commit 38bd34736ca5 · 2024-02-09T18:12:50.000+01:00
diff --git a/server/app/src/main/java/io/whitefox/api/deltasharing/server/DeltaSharesApiImpl.java b/server/app/src/main/java/io/whitefox/api/deltasharing/server/DeltaSharesApiImpl.java
@@ -48,12 +48,10 @@ public DeltaSharesApiImpl(
   @Override
   public Response getShare(String share) {
     return wrapExceptions(
-        () -> optionalToNotFound(
-            shareService.getShare(share),
-            foundShare -> shareToForbidden(foundShare, s -> {
-              var resultShare = new Share().name(s.name()).id(s.id());
-              return Response.ok(resultShare).build();
-            })),
+        () -> optionalToNotFound(shareService.getShare(share), s -> {
+          var resultShare = new Share().name(s.name()).id(s.id());
+          return Response.ok(resultShare).build();
+        }),
         exceptionToResponse);
   }
 
@@ -126,24 +124,18 @@ share, schema, table, startingTimestamp, getRequestPrincipal()),
   public Response listALLTables(String share, Integer maxResults, String pageToken) {
     return wrapExceptions(
         () -> optionalToNotFound(
-            shareService.getShare(share),
-            foundShare -> shareToForbidden(
-                foundShare,
-                s -> optionalToNotFound(
-                    deltaSharesService.listTablesOfShare(
-                        share,
-                        parseToken(pageToken),
-                        Optional.ofNullable(maxResults),
-                        getRequestPrincipal()),
-                    c -> Response.ok(c.getToken()
-                            .map(
-                                t -> new ListTablesResponse()
-                                    .items(mapList(c.getContent(), DeltaMappers::table2api))
-                                    .nextPageToken(tokenEncoder.encodePageToken(t)))
-                            .orElse(
-                                new ListTablesResponse()
-                                    .items(mapList(c.getContent(), DeltaMappers::table2api))))
-                        .build()))),
+            deltaSharesService.listTablesOfShare(
+                share,
+                parseToken(pageToken),
+                Optional.ofNullable(maxResults),
+                getRequestPrincipal()),
+            c -> Response.ok(c.getToken()
+                    .map(t -> new ListTablesResponse()
+                        .items(mapList(c.getContent(), DeltaMappers::table2api))
+                        .nextPageToken(tokenEncoder.encodePageToken(t)))
+                    .orElse(new ListTablesResponse()
+                        .items(mapList(c.getContent(), DeltaMappers::table2api))))
+                .build()),
         exceptionToResponse);
   }
 
diff --git a/server/app/src/main/java/io/whitefox/api/server/ApiUtils.java b/server/app/src/main/java/io/whitefox/api/server/ApiUtils.java
@@ -3,10 +3,10 @@
 import io.quarkus.runtime.util.ExceptionUtil;
 import io.whitefox.api.deltasharing.model.v1.generated.CommonErrorResponse;
 import io.whitefox.core.Principal;
-import io.whitefox.core.Share;
 import io.whitefox.core.services.DeltaSharedTable;
 import io.whitefox.core.services.exceptions.AlreadyExists;
 import io.whitefox.core.services.exceptions.NotFound;
+import jakarta.ws.rs.ForbiddenException;
 import jakarta.ws.rs.core.Response;
 import java.sql.Timestamp;
 import java.time.OffsetDateTime;
@@ -44,6 +44,12 @@ public interface ApiUtils extends DeltaHeaders {
               .errorCode("BAD REQUEST - timestamp provided is not formatted correctly")
               .message(ExceptionUtil.generateStackTrace(t)))
           .build();
+    } else if (t instanceof ForbiddenException) {
+      return Response.status(Response.Status.FORBIDDEN)
+          .entity(new CommonErrorResponse()
+              .errorCode("FORBIDDEN ACCESS")
+              .message(ExceptionUtil.generateStackTrace(t)))
+          .build();
     } else {
       return Response.status(Response.Status.BAD_GATEWAY)
           .entity(new CommonErrorResponse()
@@ -67,13 +73,14 @@ default Response notFoundResponse() {
         .build();
   }
 
-  default <T> Response optionalToNotFound(Optional<T> opt, Function<T, Response> fn) {
-    return opt.map(fn).orElse(notFoundResponse());
+  default Response forbiddenResponse() {
+    return Response.status(Response.Status.FORBIDDEN)
+        .entity(new CommonErrorResponse().errorCode("2").message("UNAUTHORIZED ACCESS"))
+        .build();
   }
 
-  default Response shareToForbidden(Share value, Function<Share, Response> fn) {
-    if (value.recipients().contains(getRequestPrincipal())) return fn.apply(value);
-    else return Response.status(Response.Status.FORBIDDEN).build();
+  default <T> Response optionalToNotFound(Optional<T> opt, Function<T, Response> fn) {
+    return opt.map(fn).orElse(notFoundResponse());
   }
 
   default Principal getRequestPrincipal() {
diff --git a/server/app/src/test/java/io/whitefox/api/deltasharing/SampleTables.java b/server/app/src/test/java/io/whitefox/api/deltasharing/SampleTables.java
@@ -70,7 +70,23 @@ public static StorageManager createStorageManager() {
                     new SharedTable("icebergtable2", "default", "name", icebergtable2)),
                 "name")),
         testPrincipal,
-        0L)));
+        0L),
+            new io.whitefox.core.Share(
+                    "noauthShare",
+                    "key",
+                    Map.of(
+                            "default",
+                            new io.whitefox.core.Schema(
+                                    "default",
+                                    List.of(
+                                            new SharedTable("table1", "default", "name", deltaTable1),
+                                            new SharedTable(
+                                                    "table-with-history", "default", "name", deltaTableWithHistory1),
+                                            new SharedTable("icebergtable1", "default", "name", icebergtable1),
+                                            new SharedTable("icebergtable2", "default", "name", icebergtable2)),
+                                    "name")),
+                    new Principal("Mr. White"),
+                    0L)));
   }
 
   public static final ParquetMetadata deltaTable1Metadata = ParquetMetadata.builder()
diff --git a/server/app/src/test/java/io/whitefox/api/deltasharing/server/DeltaSharesApiImplTest.java b/server/app/src/test/java/io/whitefox/api/deltasharing/server/DeltaSharesApiImplTest.java
@@ -108,6 +108,16 @@ public void listNotFoundSchemas() {
         .body("message", is("NOT FOUND"));
   }
 
+  @Test
+  public void listSchemasNoAuth() {
+    given()
+            .when()
+            .filter(deltaFilter)
+            .get("delta-api/v1/shares/{share}/schemas", "noauthShare")
+            .then()
+            .statusCode(403);
+  }
+
   @Test
   public void listSchemas() {
     given()
diff --git a/server/app/src/test/resources/application.properties b/server/app/src/test/resources/application.properties
@@ -1 +1,2 @@
-quarkus.http.test-port=8080
+quarkus.http.test-port=8080
+quarkus.test.arg-line=-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005
diff --git a/server/core/src/main/java/io/whitefox/core/Share.java b/server/core/src/main/java/io/whitefox/core/Share.java
@@ -50,7 +50,7 @@ public Share(
         id,
         schemas,
         Optional.empty(),
-        Set.of(createPrincipal),
+        Set.of(),
         createTime,
         createPrincipal,
         createTime,
diff --git a/server/core/src/main/java/io/whitefox/core/WhitefoxAuthorization.java b/server/core/src/main/java/io/whitefox/core/WhitefoxAuthorization.java
@@ -13,7 +13,7 @@ class WhitefoxSimpleAuthorization implements WhitefoxAuthorization {
 
     @Override
     public Boolean authorize(Share share, Principal principal) {
-      return share.recipients().contains(principal);
+      return share.recipients().contains(principal) || share.owner().equals(principal);
     }
   }
 }
diff --git a/server/core/src/main/java/io/whitefox/core/services/DeltaSharesServiceImpl.java b/server/core/src/main/java/io/whitefox/core/services/DeltaSharesServiceImpl.java
@@ -73,7 +73,8 @@ public ContentAndToken<List<Share>> listShares(
     Optional<ContentAndToken.Token> optionalToken =
         end < pageContent.size() ? Optional.of(new ContentAndToken.Token(end)) : Optional.empty();
     var content = pageContent.result().stream()
-        .filter(s -> s.recipients().contains(currentPrincipal))
+        .filter(
+            s -> s.recipients().contains(currentPrincipal) || s.owner().equals(currentPrincipal))
         .collect(Collectors.toList());
     return optionalToken
         .map(t -> ContentAndToken.of(content, t))
diff --git a/server/core/src/test/java/io/whitefox/core/services/DeltaShareServiceTest.java b/server/core/src/test/java/io/whitefox/core/services/DeltaShareServiceTest.java
@@ -13,6 +13,7 @@
 import io.whitefox.core.services.exceptions.TableNotFound;
 import io.whitefox.persistence.StorageManager;
 import io.whitefox.persistence.memory.InMemoryStorageManager;
+import jakarta.ws.rs.ForbiddenException;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -31,6 +32,7 @@ public class DeltaShareServiceTest {
   WhitefoxAuthorization whitefoxAuthorization = new WhitefoxSimpleAuthorization();
 
   private static final Principal testPrincipal = new Principal("Mr. Fox");
+  private static final Principal badPrincipal = new Principal("Mr. White");
 
   private static Share createShare(String name, String key, Map<String, Schema> schemas) {
     return new Share(name, key, schemas, testPrincipal, 0L);
@@ -52,6 +54,21 @@ public void listShares() {
     Assertions.assertTrue(sharesWithNextToken.getToken().isEmpty());
   }
 
+  @Test
+  public void listSharesUnauthorized() {
+    var shares = List.of(new Share("name", "key", Collections.emptyMap(), badPrincipal, 0L));
+    StorageManager storageManager = new InMemoryStorageManager(shares);
+    DeltaSharesService deltaSharesService = new DeltaSharesServiceImpl(
+        storageManager,
+        defaultMaxResults,
+        tableLoaderFactory,
+        fileSignerFactory,
+        whitefoxAuthorization);
+    var sharesWithNextToken =
+        deltaSharesService.listShares(Optional.empty(), Optional.of(30), testPrincipal);
+    Assertions.assertEquals(0, sharesWithNextToken.getContent().size());
+  }
+
   @Test
   public void listSharesWithToken() {
     var shares = List.of(createShare("name", "key", Collections.emptyMap()));
@@ -81,6 +98,18 @@ public void listSchemasOfEmptyShare() {
     Assertions.assertTrue(resultSchemas.get().getToken().isEmpty());
   }
 
+  @Test
+  public void listSchemasNoAuth() {
+    var shares = List.of(new Share("name", "key", Collections.emptyMap(), badPrincipal, 0L));
+    StorageManager storageManager = new InMemoryStorageManager(shares);
+    DeltaSharesService deltaSharesService = new DeltaSharesServiceImpl(
+        storageManager, 100, tableLoaderFactory, fileSignerFactory, whitefoxAuthorization);
+    assertThrows(
+        ForbiddenException.class,
+        () -> deltaSharesService.listSchemas(
+            "name", Optional.empty(), Optional.empty(), testPrincipal));
+  }
+
   @Test
   public void listSchemas() {
     var shares = List.of(createShare(

Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
`1`		`-quarkus.http.test-port=8080`
	`1`	`+quarkus.http.test-port=8080`
	`2`	`+quarkus.test.arg-line=-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ class WhitefoxSimpleAuthorization implements WhitefoxAuthorization {`
`13`	`13`
`14`	`14`	`@Override`
`15`	`15`	`public Boolean authorize(Share share, Principal principal) {`
`16`		`- return share.recipients().contains(principal);`
	`16`	`+ return share.recipients().contains(principal) \|\| share.owner().equals(principal);`
`17`	`17`	`}`
`18`	`18`	`}`
`19`	`19`	`}`