clintropolis commented on code in PR #17564:
URL: https://github.com/apache/druid/pull/17564#discussion_r1907953254
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -214,8 +214,8 @@ public Response doPost(
return
io.getResponseWriter().buildNonOkResponse(qe.getFailType().getExpectedStatus(),
qe);
}
- if (!authResult.isAllowed()) {
- throw new ForbiddenException(authResult.toString());
+ if (!authResult.isUserWithNoRestriction()) {
+ throw new ForbiddenException(authResult.getErrorMessage());
Review Comment:
does this mean only credentials with no restrictions can issue native
queries? that doesn't seem correct, isn't `QueryLifecycle` handling rewriting
the query to apply the policies?
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -152,14 +152,14 @@ public Response cancelQuery(@PathParam("id") String
queryId, @Context final Http
datasources = new TreeSet<>();
}
- Access authResult = AuthorizationUtils.authorizeAllResourceActions(
+ AuthorizationResult authResult =
AuthorizationUtils.authorizeAllResourceActions(
req,
Iterables.transform(datasources,
AuthorizationUtils.DATASOURCE_WRITE_RA_GENERATOR),
authorizerMapper
);
- if (!authResult.isAllowed()) {
- throw new ForbiddenException(authResult.toString());
+ if (!authResult.isUserWithNoRestriction()) {
+ throw new ForbiddenException(authResult.getErrorMessage());
Review Comment:
I'm not sure this is correct, since it would prevent users with policies
from cancelling their own queries
##########
server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java:
##########
@@ -0,0 +1,235 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.security;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.policy.NoRestrictionPolicy;
+import org.apache.druid.query.policy.Policy;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+/**
+ * Represents the outcoming of performing authorization check on required
resource accesses on a query or http requests.
+ * It contains:
+ * <ul>
+ * <li>a boolean allow or deny access results for checking permissions on a
list of resource actions.
+ * <li>a failure message if deny access. It's null when access is allowed.
+ * <li>a map of table name with optional {@link Policy} restriction. An
empty value means there's no restriction
+ * enforced on the table.
+ * </ul>
+ */
+public class AuthorizationResult
+{
+ /**
+ * Provides access with no restrictions to all resources.This should be
limited to Druid internal systems or
+ * superusers, except in cases where granular ACL considerations are not a
priority.
+ */
+ public static final AuthorizationResult ALLOW_NO_RESTRICTION = new
AuthorizationResult(
+ PERMISSION.ALLOW_NO_RESTRICTION,
+ null,
+ Collections.emptyMap()
+ );
+
+ /**
+ * Provides a default deny access result.
+ */
+ public static final AuthorizationResult DENY = new AuthorizationResult(
+ PERMISSION.DENY,
+ Access.DENIED.getMessage(),
+ Collections.emptyMap()
+ );
+
+ enum PERMISSION
+ {
+ ALLOW_NO_RESTRICTION,
+ ALLOW_WITH_RESTRICTION,
+ DENY
+ }
+
+ private final PERMISSION permission;
+
+ @Nullable
+ private final String failureMessage;
+
+ private final Map<String, Optional<Policy>> policyRestrictions;
+
+ AuthorizationResult(
+ PERMISSION permission,
+ @Nullable String failureMessage,
+ Map<String, Optional<Policy>> policyRestrictions
+ )
+ {
+ this.permission = permission;
+ this.failureMessage = failureMessage;
+ this.policyRestrictions = policyRestrictions;
+
+ // sanity check
+ switch (permission) {
+ case DENY:
+ validateFailureMessageIsSet();
+ validatePolicyRestrictionEmpty();
+ return;
+ case ALLOW_WITH_RESTRICTION:
+ validateFailureMessageNull();
+ validatePolicyRestrictionNonEmpty();
+ return;
+ case ALLOW_NO_RESTRICTION:
+ validateFailureMessageNull();
+ validatePolicyRestrictionEmpty();
+ return;
+ default:
+ throw DruidException.defensive("unreachable");
+ }
+ }
+
+ public static AuthorizationResult deny(@Nonnull String failureMessage)
+ {
+ return new AuthorizationResult(PERMISSION.DENY, failureMessage,
Collections.emptyMap());
+ }
+
+ public static AuthorizationResult allowWithRestriction(Map<String,
Optional<Policy>> policyRestrictions)
+ {
+ if (policyRestrictions.isEmpty()) {
+ return ALLOW_NO_RESTRICTION;
+ }
+ return new AuthorizationResult(PERMISSION.ALLOW_WITH_RESTRICTION, null,
policyRestrictions);
+ }
+
+ /**
+ * Returns true if user has basic access.
+ */
+ public boolean allowBasicAccess()
+ {
+ return PERMISSION.ALLOW_NO_RESTRICTION.equals(permission) ||
PERMISSION.ALLOW_WITH_RESTRICTION.equals(permission);
+ }
+
+ /**
+ * Returns true if user has all required permission, and the policy
restrictions indicates one of the following:
+ * <li> no policy found
+ * <li> the user has a no-restriction policy
+ */
+ public boolean isUserWithNoRestriction()
Review Comment:
naming nit, there is no such thing as a "user" in Druid core, that is a
construct that may or may not exist in specific auth extensions.
Beyond that, I'm not quite sure this is the correct model, is it overly
restrictive since many of the former callers of `isAllowed` are calling this,
it seems like if there are any policies involved at all then the only thing
that will be allowed is query calls, every other API would be disallowed, which
doesn't seem the best behavior to me.
##########
processing/src/main/java/org/apache/druid/query/RestrictedDataSource.java:
##########
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableList;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.planning.DataSourceAnalysis;
+import org.apache.druid.query.policy.Policy;
+import org.apache.druid.segment.RestrictedSegment;
+import org.apache.druid.segment.SegmentReference;
+import org.apache.druid.utils.JvmUtils;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * Reperesents a TableDataSource with row-level restriction.
+ * <p>
+ * A RestrictedDataSource means the base TableDataSource has restriction
imposed. A table without any restriction should
+ * never be transformed to a RestrictedDataSource. Druid internal system and
admin users would have a null rowFilter,
+ * while external users would have a rowFilter based on the applied
restriction.
+ */
+public class RestrictedDataSource implements DataSource
+{
+ private final TableDataSource base;
+ private final Policy policy;
Review Comment:
I still think this should be a List, otherwise auth extensions are going to
have to some work to what might logically be multiple policies (in the general
sense at least). Like imagine an auth setup like ldap or something where there
are users and groups that allows defining policies at both levels, if we don't
allow a list here than that extension would have to combine all of that stuff
into a single Policy.
Maybe that is fine I guess, but it does seem like it would make extensions
have to do more work instead of just mapping different logical types of
policies such as row filtering and column access into a single
`CursorBuildSpec` transformation. I guess the benefit of a single policy would
be that if there were multiple row filters in place for example it could be
combined into a flat `and` statement instead of like `a = 'x' && (b = 'y' && c
= 'z')`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
