clintropolis commented on code in PR #17564:
URL: https://github.com/apache/druid/pull/17564#discussion_r1905972026
##########
server/src/main/java/org/apache/druid/server/security/Access.java:
##########
@@ -21,52 +21,98 @@
import com.google.common.base.Strings;
import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.query.policy.Policy;
+import javax.annotation.Nullable;
+import java.util.Optional;
+
+/**
+ * Represents the outcome of verifying permissions to perform an {@link
Action} on a {@link Resource}, along with any
+ * policy restrictions.
+ */
public class Access
{
public static final String DEFAULT_ERROR_MESSAGE = "Unauthorized";
+ public static final String DEFAULT_AUTHORIZED_MESSAGE = "Authorized";
- public static final Access OK = new Access(true);
- public static final Access DENIED = new Access(false);
+ public static final Access OK = allow();
+ public static final Access DENIED = deny("");
private final boolean allowed;
private final String message;
+ // A policy restriction on top of table-level read access. It should be
empty if there are no policy restrictions
+ // or if access is requested for an action other than reading the table.
+ private final Optional<Policy> policy;
Review Comment:
re other comments about lists, this should also be a list of `Policy`
##########
server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.collect.ImmutableMap;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.filter.TrueDimFilter;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Stream;
+
+public class AuthorizationResult
Review Comment:
i agree with thread that `getPermissionErrorMessage` seems a bit unintuitive
and leaves room for mistakes. I'm also partial to making it return a 3 state
thing, so that 'allowed but with policies' cannot be mistaken for 'allow'. It
feels a little strange to let the caller indicate if they care about policies
or not by passing the boolean, since if in the future we add additional types
of policies that might change. It seems like the authorizer should not return
policies if the caller doesn't need them, so it shouldn't be up to the caller
##########
server/src/main/java/org/apache/druid/server/security/AuthorizationResult.java:
##########
@@ -0,0 +1,287 @@
+/*
+ * 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 com.google.common.collect.ImmutableMap;
+import org.apache.druid.error.DruidException;
+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;
+import java.util.Set;
+
+/**
+ * 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(),
+ null,
+ null
+ );
+
+ /**
+ * Provides a default deny access result.
+ */
+ public static final AuthorizationResult DENY = new AuthorizationResult(
+ PERMISSION.DENY,
+ Access.DENIED.getMessage(),
+ Collections.emptyMap(),
+ null,
+ null
+ );
+
+ 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;
+
+ @Nullable
+ private final Set<ResourceAction> sqlResourceActions;
+
+ @Nullable
+ private final Set<ResourceAction> allResourceActions;
Review Comment:
What are these for? It seems a bit strange to me since everything has been
migrated to use `AuthorizationResult` instead of `Access`, and these,
particularly `sqlResourceActions`, seems pretty tailored towards authorization
of SQL statements.
Its starting to make me wonder if we should modify the `Authorizer`
interface itself to add an additional method for authorizing SQL statements and
make that return this new `AuthorizationResult`, instead of cobbling it
together out of a set of `Access`, but I haven't fully finished thinking this
through yet.. will think on it a bit more.
##########
processing/src/main/java/org/apache/druid/segment/RestrictedSegment.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.segment;
+
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.Interval;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Optional;
+
+/**
+ * A wrapped {@link SegmentReference} with a {@link DimFilter} restriction.
The restriction must be applied on the
+ * segment.
+ */
+public class RestrictedSegment implements SegmentReference
+{
+ protected final SegmentReference delegate;
+ @Nullable
+ private final DimFilter filter;
+
+ public RestrictedSegment(
+ SegmentReference delegate,
+ @Nullable DimFilter filter
+ )
+ {
+ this.delegate = delegate;
+ this.filter = filter;
+ }
+
+ @Override
+ public Optional<Closeable> acquireReferences()
+ {
+ return delegate.acquireReferences();
+ }
+
+ @Override
+ public SegmentId getId()
+ {
+ return delegate.getId();
+ }
+
+ @Override
+ public Interval getDataInterval()
+ {
+ return delegate.getDataInterval();
+ }
+
+ @Override
+ public CursorFactory asCursorFactory()
+ {
+ return new RestrictedCursorFactory(delegate.asCursorFactory(), filter);
+ }
+
+ @Nullable
+ @Override
+ public QueryableIndex asQueryableIndex()
+ {
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public <T> T as(@Nonnull Class<T> clazz)
Review Comment:
>Call as(QueryableIndex.class) first.
>If that returns null, try as(BypassRestrictedSegment.class) followed by
as(QueryableIndex.class)
I was thinking that maybe this should throw an exception targeted at
developers to indicate that they should call
`as(BypassRestrictedSegment.class)`, since imagining in the future if this were
rolled out with some policies then the .as/asQueryableIndex methods would go
from working to suddenly returning null.
Along with throwing an exception, would also maybe a helper method to
indicate if the segment supports `BypassRestrictedSegment`, though the same
could also work if we just return null from .as, so I don't feel too strongly
either way.
##########
server/src/main/java/org/apache/druid/server/security/Policy.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.collect.Iterables;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.filter.EqualityFilter;
+import org.apache.druid.query.filter.TrueDimFilter;
+import org.apache.druid.segment.column.ColumnType;
+
+import java.util.Collection;
+import java.util.Objects;
+
+public class Policy
Review Comment:
its kind of confusing that there are 2 `Policy` classes, in the very least
we need javadocs to distinguish the differences, but better still if we don't
need both
--
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]
