Add getPermissionErrorMessage method to AuthorizationResult, and use …

…it as the default interface for checking permission
apache · Dec 18, 2024 · 925b01f · 925b01f
1 parent 3eb0b66
commit 925b01f
Show file tree

Hide file tree

Showing 27 changed files with 140 additions and 137 deletions.
diff --git a/extensions-contrib/grpc-query/src/main/java/org/apache/druid/grpc/server/QueryDriver.java b/extensions-contrib/grpc-query/src/main/java/org/apache/druid/grpc/server/QueryDriver.java
@@ -148,7 +148,7 @@ private QueryResponse runNativeQuery(QueryRequest request, AuthenticationResult
     try {
       queryLifecycle.initialize(query);
       AuthorizationResult authorizationResult = queryLifecycle.authorize(authResult);
-      if (!authorizationResult.isAllowed()) {
+      if (!authorizationResult.getPermissionErrorMessage(true).isPresent()) {
         throw new ForbiddenException(Access.DEFAULT_ERROR_MESSAGE);
       }
       queryResponse = queryLifecycle.execute();

diff --git a/...c-security/src/main/java/org/apache/druid/security/basic/BasicSecurityResourceFilter.java b/...c-security/src/main/java/org/apache/druid/security/basic/BasicSecurityResourceFilter.java
@@ -60,14 +60,14 @@ public ContainerRequest filter(ContainerRequest request)
         getAuthorizerMapper()
     );
 
-    if (!authResult.isAllowed()) {
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
       throw new WebApplicationException(
           Response.status(Response.Status.FORBIDDEN)
                   .type(MediaType.TEXT_PLAIN)
-                  .entity(StringUtils.format("Access-Check-Result: %s", authResult.getFailureMessage()))
+                  .entity(StringUtils.format("Access-Check-Result: %s", error))
                   .build()
       );
-    }
+    });
 
     return request;
   }

diff --git a/...sions-core/druid-catalog/src/main/java/org/apache/druid/catalog/http/CatalogResource.java b/...sions-core/druid-catalog/src/main/java/org/apache/druid/catalog/http/CatalogResource.java
@@ -59,7 +59,6 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
 import java.util.stream.Collectors;
 
 /**
@@ -582,9 +581,9 @@ private void authorizeTable(
   private void authorize(String resource, String key, Action action, HttpServletRequest request)
   {
     final AuthorizationResult authResult = authorizeAccess(resource, key, action, request);
-    if (!authResult.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
   }
 
   private AuthorizationResult authorizeAccess(String resource, String key, Action action, HttpServletRequest request)

diff --git a/...-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java b/...-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java
@@ -175,7 +175,8 @@ public GetQueriesResponse doGetRunningQueries(
     queries.sort(Comparator.comparing(DartQueryInfo::getStartTime).thenComparing(DartQueryInfo::getDartQueryId));
 
     final GetQueriesResponse response;
-    if (stateReadAccess.isAllowed()) {
+    boolean hasFullPermission = !stateReadAccess.getPermissionErrorMessage(true).isPresent();
+    if (hasFullPermission) {
       // User can READ STATE, so they can see all running queries, as well as authentication details.
       response = new GetQueriesResponse(queries);
     } else {
@@ -245,9 +246,9 @@ public Response cancelQuery(
       return Response.status(Response.Status.ACCEPTED).build();
     }
 
-    final AuthorizationResult access = authorizeCancellation(req, cancelables);
+    final AuthorizationResult authResult = authorizeCancellation(req, cancelables);
 
-    if (access.isAllowed()) {
+    if (!authResult.getPermissionErrorMessage(true).isPresent()) {
       sqlLifecycleManager.removeAll(sqlQueryId, cancelables);
 
       // Don't call cancel() on the cancelables. That just cancels native queries, which is useless here. Instead,

diff --git a/...ti-stage-query/src/main/java/org/apache/druid/msq/dart/controller/sql/DartQueryMaker.java b/...ti-stage-query/src/main/java/org/apache/druid/msq/dart/controller/sql/DartQueryMaker.java
@@ -52,6 +52,7 @@
 import org.apache.druid.msq.sql.MSQTaskQueryMaker;
 import org.apache.druid.segment.column.ColumnType;
 import org.apache.druid.server.QueryResponse;
+import org.apache.druid.server.security.ForbiddenException;
 import org.apache.druid.sql.calcite.planner.PlannerContext;
 import org.apache.druid.sql.calcite.rel.DruidQuery;
 import org.apache.druid.sql.calcite.run.QueryMaker;
@@ -127,6 +128,9 @@ public DartQueryMaker(
   @Override
   public QueryResponse<Object[]> runQuery(DruidQuery druidQuery)
   {
+    plannerContext.getAuthorizationResult().getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
     final MSQSpec querySpec = MSQTaskQueryMaker.makeQuerySpec(
         null,
         druidQuery,

diff --git a/...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/rpc/MSQResourceUtils.java b/...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/rpc/MSQResourceUtils.java
@@ -27,7 +27,6 @@
 
 import javax.servlet.http.HttpServletRequest;
 import java.util.List;
-import java.util.Objects;
 
 /**
  * Utility methods for MSQ resources such as {@link ControllerResource}.
@@ -42,15 +41,15 @@ public static void authorizeAdminRequest(
   {
     final List<ResourceAction> resourceActions = permissionMapper.getAdminPermissions();
 
-    AuthorizationResult access = AuthorizationUtils.authorizeAllResourceActions(
+    AuthorizationResult authResult = AuthorizationUtils.authorizeAllResourceActions(
         request,
         resourceActions,
         authorizerMapper
     );
 
-    if (!access.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(access.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
   }
 
   public static void authorizeQueryRequest(
@@ -62,14 +61,14 @@ public static void authorizeQueryRequest(
   {
     final List<ResourceAction> resourceActions = permissionMapper.getQueryPermissions(queryId);
 
-    AuthorizationResult access = AuthorizationUtils.authorizeAllResourceActions(
+    AuthorizationResult authResult = AuthorizationUtils.authorizeAllResourceActions(
         request,
         resourceActions,
         authorizerMapper
     );
 
-    if (!access.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(access.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
   }
 }
diff --git a/...ions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java b/...ions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java
@@ -54,6 +54,7 @@
 import org.apache.druid.segment.column.ColumnType;
 import org.apache.druid.server.QueryResponse;
 import org.apache.druid.server.lookup.cache.LookupLoadingSpec;
+import org.apache.druid.server.security.ForbiddenException;
 import org.apache.druid.sql.calcite.parser.DruidSqlIngest;
 import org.apache.druid.sql.calcite.parser.DruidSqlInsert;
 import org.apache.druid.sql.calcite.parser.DruidSqlReplace;
@@ -116,6 +117,9 @@ public class MSQTaskQueryMaker implements QueryMaker
   @Override
   public QueryResponse<Object[]> runQuery(final DruidQuery druidQuery)
   {
+    plannerContext.getAuthorizationResult().getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
     Hook.QUERY_PLAN.run(druidQuery.getQuery());
     plannerContext.dispatchHook(DruidHook.NATIVE_PLAN, druidQuery.getQuery());
 

diff --git a/...ti-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java b/...ti-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java
@@ -675,21 +675,21 @@ private MSQControllerTask getMSQControllerTaskAndCheckPermission(
       return msqControllerTask;
     }
 
-    AuthorizationResult access = AuthorizationUtils.authorizeAllResourceActions(
+    AuthorizationResult authResult = AuthorizationUtils.authorizeAllResourceActions(
         authenticationResult,
         Collections.singletonList(new ResourceAction(Resource.STATE_RESOURCE, forAction)),
         authorizerMapper
     );
 
-    if (access.isAllowed()) {
-      return msqControllerTask;
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(StringUtils.format(
+          "The current user[%s] cannot view query id[%s] since the query is owned by another user",
+          currentUser,
+          queryId
+      ));
+    });
 
-    throw new ForbiddenException(StringUtils.format(
-        "The current user[%s] cannot view query id[%s] since the query is owned by another user",
-        currentUser,
-        queryId
-    ));
+    return msqControllerTask;
   }
 
   /**

diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTaskUtils.java b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/IndexTaskUtils.java
@@ -43,7 +43,6 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 
 public class IndexTaskUtils
 {
@@ -80,12 +79,11 @@ public static AuthorizationResult datasourceAuthorizationCheck(
         action
     );
 
-    AuthorizationResult access = AuthorizationUtils.authorizeResourceAction(req, resourceAction, authorizerMapper);
-    if (!access.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(access.getFailureMessage()));
-    }
-
-    return access;
+    AuthorizationResult authResult = AuthorizationUtils.authorizeResourceAction(req, resourceAction, authorizerMapper);
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
+    return authResult;
   }
 
   public static void setTaskDimensions(final ServiceMetricEvent.Builder metricBuilder, final Task task)

diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
@@ -101,7 +101,6 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 
 /**
@@ -183,10 +182,9 @@ public Response taskPost(
         resourceActions,
         authorizerMapper
     );
-
-    if (!authResult.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
 
     return asLeaderWith(
         taskMaster.getTaskQueue(),
@@ -615,17 +613,18 @@ public Response getTasks(
           resourceAction,
           authorizerMapper
       );
-      if (!authResult.isAllowed()) {
+
+      authResult.getPermissionErrorMessage(true).ifPresent(error -> {
         throw new WebApplicationException(
             Response.status(Response.Status.FORBIDDEN)
                     .type(MediaType.TEXT_PLAIN)
                     .entity(StringUtils.format(
                         "Access-Check-Result: %s",
-                        Objects.requireNonNull(authResult.getFailureMessage())
+                        error
                     ))
                     .build()
         );
-      }
+      });
     }
 
     return asLeaderWith(
@@ -667,9 +666,9 @@ public Response killPendingSegments(
         authorizerMapper
     );
 
-    if (!authResult.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
 
     if (overlord.isLeader()) {
       try {

diff --git a/.../main/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilter.java b/.../main/java/org/apache/druid/indexing/overlord/http/security/SupervisorResourceFilter.java
@@ -41,7 +41,6 @@
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.PathSegment;
 import javax.ws.rs.core.Response;
-import java.util.Objects;
 
 public class SupervisorResourceFilter extends AbstractResourceFilter
 {
@@ -104,9 +103,9 @@ public boolean apply(PathSegment input)
         getAuthorizerMapper()
     );
 
-    if (!authResult.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
 
     return request;
   }

diff --git a/...ce/src/main/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilter.java b/...ce/src/main/java/org/apache/druid/indexing/overlord/http/security/TaskResourceFilter.java
@@ -40,7 +40,6 @@
 import javax.ws.rs.WebApplicationException;
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
-import java.util.Objects;
 
 /**
  * Use this ResourceFilter when the datasource information is present after "task" segment in the request Path
@@ -99,9 +98,9 @@ public ContainerRequest filter(ContainerRequest request)
         getAuthorizerMapper()
     );
 
-    if (!authResult.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
 
     return request;
   }

diff --git a/...ing-service/src/main/java/org/apache/druid/indexing/overlord/sampler/SamplerResource.java b/...ing-service/src/main/java/org/apache/druid/indexing/overlord/sampler/SamplerResource.java
@@ -40,7 +40,6 @@
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
 import java.util.HashSet;
-import java.util.Objects;
 import java.util.Set;
 
 @Path("/druid/indexer/v1/sampler")
@@ -79,9 +78,9 @@ public SamplerResponse post(final SamplerSpec sampler, @Context final HttpServle
         authorizerMapper
     );
 
-    if (!authResult.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-    }
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
     return sampler.sample();
   }
 }
diff --git a/...rvice/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java b/...rvice/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java
@@ -148,9 +148,9 @@ public Response specPost(final SupervisorSpec spec, @Context final HttpServletRe
               authorizerMapper
           );
 
-          if (!authResult.isAllowed()) {
-            throw new ForbiddenException(Objects.requireNonNull(authResult.getFailureMessage()));
-          }
+          authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+            throw new ForbiddenException(error);
+          });
 
           manager.createOrUpdateAndStartSupervisor(spec);
 

diff --git a/server/src/main/java/org/apache/druid/segment/realtime/ChatHandlers.java b/server/src/main/java/org/apache/druid/segment/realtime/ChatHandlers.java
@@ -29,7 +29,6 @@
 import org.apache.druid.server.security.ResourceType;
 
 import javax.servlet.http.HttpServletRequest;
-import java.util.Objects;
 
 public class ChatHandlers
 {
@@ -50,11 +49,11 @@ public static AuthorizationResult authorizationCheck(
         action
     );
 
-    AuthorizationResult access = AuthorizationUtils.authorizeResourceAction(req, resourceAction, authorizerMapper);
-    if (!access.isAllowed()) {
-      throw new ForbiddenException(Objects.requireNonNull(access.getFailureMessage()));
-    }
+    AuthorizationResult authResult = AuthorizationUtils.authorizeResourceAction(req, resourceAction, authorizerMapper);
+    authResult.getPermissionErrorMessage(true).ifPresent(error -> {
+      throw new ForbiddenException(error);
+    });
 
-    return access;
+    return authResult;
   }
 }