github-actions[bot] commented on code in PR #66770:
URL: https://github.com/apache/doris/pull/66770#discussion_r3793532514
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -450,31 +806,31 @@ public boolean checkPrivByAuthInfo(ConnectContext ctx,
AuthorizationInfo authInf
return true;
}
- public Map<String, Optional<DataMaskPolicy>>
evalDataMaskPolicies(UserIdentity currentUser, String
- ctl, String db, String tbl, Set<String> cols) {
- Map<String, Optional<DataMaskPolicy>> res = Maps.newHashMap();
- for (String col : cols) {
- res.put(col, evalDataMaskPolicy(currentUser, ctl, db, tbl, col));
- }
- return res;
- }
-
- public Optional<DataMaskPolicy> evalDataMaskPolicy(UserIdentity
currentUser, String
+ public Optional<DataMaskSpec> evalDataMaskPolicy(UserIdentity currentUser,
String
ctl, String db, String tbl, String col) {
Objects.requireNonNull(currentUser, "require currentUser object");
Objects.requireNonNull(ctl, "require ctl object");
Objects.requireNonNull(db, "require db object");
Objects.requireNonNull(tbl, "require tbl object");
Objects.requireNonNull(col, "require col object");
- return
getAccessControllerOrDefault(ctl).evalDataMaskPolicy(currentUser, ctl, db, tbl,
col.toLowerCase());
+ // Sources are asked about columns in lower case, which is how the
ones that store policies per
+ // column have them written.
+ String column = col.toLowerCase();
+ AuthorizedResource.Table table = AuthorizedResource.table(ctl, db,
tbl);
+ return Optional.ofNullable(controllerOf(table)
+ .getDataMasks(AccessTranslation.subjectOf(currentUser), table,
Review Comment:
[P2] Batch mask lookups before crossing the plugin boundary
The new SPI explicitly accepts all columns at once so a network-backed
source is not called once per column, but this wrapper always sends a
singleton. LogicalCheckPolicy invokes it inside the output-slot loop, and
SQL-cache validation does the same per cached column, so the production path
still makes O(columns) plugin callbacks (and cache hits repeat them). Please
restore a plural manager path, call it once per table, and group cache
revalidation by table; a recording-plugin test should assert one callback for a
multi-column relation.
##########
docker/thirdparties/run-thirdparties-docker.sh:
##########
@@ -1736,11 +1736,36 @@ start_polaris() {
fi
}
+# The Doris plugin jars and service definition used to be curl'ed from inside
+# ranger-admin, with the bucket patched into the tracked scripts by `sed -i`.
+# That both broke on BSD sed and left the working tree dirty, and one flaky
+# download killed the container's `set -e` entrypoint. Fetch them here instead,
+# into the gitignored cache/ dir that the container bind mounts read-only.
+download_ranger_artifacts() {
+ local dest="${ROOT}/docker-compose/ranger/cache"
+ local
url_prefix="https://${s3BucketName}.${s3Endpoint}/regression/docker/ranger-plugins";
+ local name
+
+ mkdir -p "${dest}"
+ for name in ranger-servicedef-doris.json \
+ mysql-connector-java-8.0.25.jar \
+ ranger-doris-plugin-3.0.0-SNAPSHOT.jar; do
+ if [[ -s "${dest}/${name}" ]]; then
+ echo "ranger artifact cached: ${name}"
+ continue
+ fi
+ echo "downloading ${url_prefix}/${name}"
+ curl -fsSL --retry 10 --retry-all-errors --retry-delay 5 \
+ --connect-timeout 30 --speed-limit 1024 --speed-time 120 \
+ -o "${dest}/${name}.part" "${url_prefix}/${name}"
+ mv "${dest}/${name}.part" "${dest}/${name}"
+ done
+}
+
start_ranger() {
echo "RUN_RANGER"
export CONTAINER_UID=${CONTAINER_UID}
- find "${ROOT}/docker-compose/ranger/script" -type f -exec sed -i
"s/s3Endpoint/${s3Endpoint}/g" {} \;
- find "${ROOT}/docker-compose/ranger/script" -type f -exec sed -i
"s/s3BucketName/${s3BucketName}/g" {} \;
+ download_ranger_artifacts
Review Comment:
[P2] Keep Ranger shutdown independent of artifact downloads
`start_ranger` is also the handler for `-c ranger --stop`, but this new call
runs before both `compose_down_stack` and the existing `STOP != 1` gate. With
an empty cache, stopping Ranger now performs three long-retry network downloads
and exits without bringing the stack down whenever the artifact bucket or
network is unavailable. Please move the download into the start-only branch
(before compose up) so shutdown remains a local, reliable operation.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/LegacyAccessControllerPlugin.java:
##########
@@ -0,0 +1,170 @@
+// 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.doris.mysql.privilege;
+
+import org.apache.doris.analysis.UserIdentity;
+import org.apache.doris.authorization.AccessContext;
+import org.apache.doris.authorization.AccessDeniedException;
+import org.apache.doris.authorization.AccessRequirement;
+import org.apache.doris.authorization.AuthorizedResource;
+import org.apache.doris.authorization.AuthorizedSubject;
+import org.apache.doris.authorization.DataMaskSpec;
+import org.apache.doris.authorization.RowFilterSpec;
+import org.apache.doris.authorization.spi.AuthorizationPlugin;
+import org.apache.doris.common.AuthorizationException;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+
+/**
+ * Presents an access controller written against the older, per-scope
interface as an authorization source.
+ *
+ * <p>That interface asks a separate question per kind of object and answers
each with a boolean; this one
+ * asks a single question about a typed resource and answers by refusing or
not. The translation is the whole
+ * of this class, and it is not a temporary shim: {@code
CatalogAccessController} is what a catalog's
+ * {@code access_controller.class} names, so implementations of it exist
outside this repository and keep
+ * working unchanged.
+ */
+public class LegacyAccessControllerPlugin implements AuthorizationPlugin {
+
+ private final String name;
+ private final CatalogAccessController controller;
+
+ public LegacyAccessControllerPlugin(String name, CatalogAccessController
controller) {
+ this.name = Objects.requireNonNull(name, "name is required");
+ this.controller = Objects.requireNonNull(controller, "controller is
required");
+ }
+
+ /** The controller this presents, for where the controller itself is the
question rather than its answers. */
+ public CatalogAccessController getController() {
+ return controller;
+ }
+
+ @Override
+ public String name() {
+ return name;
+ }
+
+ @Override
+ public void checkPrivilege(AuthorizedSubject subject, AuthorizedResource
resource,
+ AccessRequirement requirement, AccessContext context) throws
AccessDeniedException {
+ UserIdentity currentUser = AccessTranslation.userIdentityOf(subject);
+ PrivPredicate wanted = AccessTranslation.privPredicateOf(requirement);
+ switch (resource.getKind()) {
+ case GLOBAL:
+ refuseUnless(controller.checkGlobalPriv(currentUser, wanted),
subject, resource, requirement);
+ return;
+ case CATALOG:
+ refuseUnless(controller.checkCtlPriv(currentUser,
+ ((AuthorizedResource.Catalog) resource).getCatalog(),
wanted),
+ subject, resource, requirement);
+ return;
+ case DATABASE: {
+ AuthorizedResource.Database database =
(AuthorizedResource.Database) resource;
+ refuseUnless(controller.checkDbPriv(currentUser,
database.getCatalog(),
Review Comment:
[P1] Preserve global grants in the legacy adapter
Before this change the manager computed the instance source's global verdict
and called boolean legacy overloads that returned immediately for a global
grant. This adapter calls only the scoped methods, so an unchanged legacy
source that returns false here now denies a user with global SELECT/ADMIN;
catalog, table, and column checks lose the same override. Legacy sources cannot
opt back in through AuthorizationContext, so please reproduce the old global
short-circuit and cover the globally-allowed/locally-denied upgrade case.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -295,13 +560,111 @@ public Auth getAuth() {
return this.auth;
}
+ /**
+ * Answers whether {@code subject} may act on {@code resource} as {@code
requirement} demands.
+ *
+ * <p>This is the one place a check is routed. Which source is asked
follows from the resource alone -
+ * system-wide objects and catalog-level grants go to the source {@code
access_controller_type}
+ * installs, everything inside a catalog goes to the source that catalog
is bound to - and whatever it
+ * answers is the answer. Combining two sources, or granting anything
before asking, would have to
+ * happen here, and deliberately does not.
+ *
+ * <p>Columns are not decided here: see {@link #decideColumns}.
+ */
+ public boolean decide(UserIdentity subject, AuthorizedResource resource,
AccessRequirement requirement) {
+ if (resource.getKind() == ResourceKind.COLUMNS) {
+ throw new IllegalArgumentException("column access is decided by
decideColumns(), which"
+ + " reports which column was refused instead of a yes or
no");
+ }
+ try {
+ ask(subject, resource, requirement);
+ return true;
+ } catch (AccessDeniedException e) {
+ // The reason travels no further for now: every caller of the
boolean facades phrases its own
+ // error message. It is carried this far so that the day one of
them stops doing so, there is
+ // something to phrase it from.
+ return false;
+ }
+ }
+
+ /**
+ * Checks access to named columns, reporting the column that was refused
rather than a yes or no.
+ *
+ * <p>Kept apart from {@link #decide} because the answer has a different
shape, not because the routing
+ * differs: it is the same source the table itself would be asked about.
+ */
+ public void decideColumns(UserIdentity subject, AuthorizedResource.Columns
columns,
+ AccessRequirement requirement) throws AuthorizationException {
+ try {
+ ask(subject, columns, requirement);
+ } catch (AccessDeniedException e) {
+ throw new AuthorizationException(e.getMessage());
+ }
+ }
+
+ private void ask(UserIdentity subject, AuthorizedResource resource,
AccessRequirement requirement)
+ throws AccessDeniedException {
+
controllerOf(resource).checkPrivilege(AccessTranslation.subjectOf(subject),
resource, requirement,
Review Comment:
[P1] Pin the plugin classloader for runtime callbacks
Only factory creation runs with the plugin loader as TCCL; this query-time
callback runs under the FE thread's app TCCL, as do row filters, masks, and
close. A supported directory plugin that lazily uses
ServiceLoader/Class.forName (or starts a worker here) therefore cannot see its
bundled provider or can bind a parent copy that is incompatible with the
child-loaded interface. The connector plugin paths already wrap every such
boundary for this reason. Please use one try/finally TCCL helper for all
authorization-plugin callbacks and test lazy provider discovery from the plugin
jar.
##########
fe/fe-core/src/main/java/org/apache/doris/policy/RowPolicy.java:
##########
@@ -221,15 +229,39 @@ public boolean isInvalid() {
return (wherePredicate == null);
}
- @Override
- public Expression getFilterExpression() throws AnalysisException {
+ /**
+ * The predicate as SQL text, which is the form the authorization layer
hands to the planner.
+ *
+ * <p>It is the text the administrator wrote, recovered from the stored
statement - not a rendering of
+ * the parsed predicate. Rendering would not survive the round trip:
{@code toSql()} on a compound
+ * predicate produces the diagnostic form {@code AND[a,b]}, which does not
parse back, so any policy
+ * combining two conditions would break.</p>
+ */
+ public String getFilterSql() throws AnalysisException {
if (wherePredicate == null) {
throw new AnalysisException("Invalid row policy [" +
getPolicyIdent() + "], " + getOriginStmt());
}
- return wherePredicate;
+ if (wherePredicateSql == null) {
+ wherePredicateSql = parseWherePredicateSql();
+ }
+ return wherePredicateSql;
+ }
+
+ private String parseWherePredicateSql() throws AnalysisException {
+ try {
+ CreatePolicyCommand command = (CreatePolicyCommand) new
NereidsParser().parseSingle(getOriginStmt());
Review Comment:
[P2] Honor stmtIdx when recovering row-policy SQL
RowPolicy persists both the full `OriginStatement` text and `stmtIdx`, but
this reparses the entire text as one statement and ignores the index. For a
supported full-text-plus-index execution with CREATE ROW POLICY at index 1,
creation has a valid predicate, yet the first governed query reaches
`getFilterSql()`, `parseSingle` fails, and the query is rejected; before this
patch the fresh policy used the already-parsed expression. Please parse all
statements and select `stmtIdx` here and in `gsonPostProcess` (or seed the
exact SQL for fresh objects), and cover index 1 before and after JSON replay.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalCheckPolicy.java:
##########
@@ -180,10 +179,10 @@ public RelatedPolicy findPolicy(LogicalPlan logicalPlan,
CascadesContext cascade
Optional<SqlCacheContext> sqlCacheContext =
statementContext.getSqlCacheContext();
boolean hasDataMask = false;
for (Slot slot : logicalPlan.getOutput()) {
- Optional<DataMaskPolicy> dataMaskPolicy =
accessManager.evalDataMaskPolicy(
+ Optional<DataMaskSpec> dataMaskPolicy =
accessManager.evalDataMaskPolicy(
Review Comment:
[P2] Let the selected plugin decide admin policy exemptions
The root/admin early return above prevents both data-policy callbacks from
ever seeing those subjects, even though the new SPI makes administrator
exemptions the selected source's choice. A plugin can allow the table read
while still returning a tenant filter or sensitive-column mask for these
subjects, but the planner silently drops both and reads unrestricted data.
Please remove or make this exemption plugin-owned, and add a directory-plugin
case that returns a root/admin row filter and mask.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -295,13 +560,111 @@ public Auth getAuth() {
return this.auth;
}
+ /**
+ * Answers whether {@code subject} may act on {@code resource} as {@code
requirement} demands.
+ *
+ * <p>This is the one place a check is routed. Which source is asked
follows from the resource alone -
+ * system-wide objects and catalog-level grants go to the source {@code
access_controller_type}
+ * installs, everything inside a catalog goes to the source that catalog
is bound to - and whatever it
+ * answers is the answer. Combining two sources, or granting anything
before asking, would have to
+ * happen here, and deliberately does not.
+ *
+ * <p>Columns are not decided here: see {@link #decideColumns}.
+ */
+ public boolean decide(UserIdentity subject, AuthorizedResource resource,
AccessRequirement requirement) {
+ if (resource.getKind() == ResourceKind.COLUMNS) {
+ throw new IllegalArgumentException("column access is decided by
decideColumns(), which"
+ + " reports which column was refused instead of a yes or
no");
+ }
+ try {
+ ask(subject, resource, requirement);
+ return true;
+ } catch (AccessDeniedException e) {
+ // The reason travels no further for now: every caller of the
boolean facades phrases its own
+ // error message. It is carried this far so that the day one of
them stops doing so, there is
+ // something to phrase it from.
+ return false;
+ }
+ }
+
+ /**
+ * Checks access to named columns, reporting the column that was refused
rather than a yes or no.
+ *
+ * <p>Kept apart from {@link #decide} because the answer has a different
shape, not because the routing
+ * differs: it is the same source the table itself would be asked about.
+ */
+ public void decideColumns(UserIdentity subject, AuthorizedResource.Columns
columns,
+ AccessRequirement requirement) throws AuthorizationException {
+ try {
+ ask(subject, columns, requirement);
+ } catch (AccessDeniedException e) {
+ throw new AuthorizationException(e.getMessage());
+ }
+ }
+
+ private void ask(UserIdentity subject, AuthorizedResource resource,
AccessRequirement requirement)
+ throws AccessDeniedException {
+
controllerOf(resource).checkPrivilege(AccessTranslation.subjectOf(subject),
resource, requirement,
+ ConnectionAccessContext.current());
Review Comment:
[P2] Preserve an explicit connection's access context
Every ConnectContext overload drops that object and this method rebuilds
context only from the thread local. That loses a known client IP when
authorization happens before the context is installed (HTTP basic/cookie auth
does this) or on an executor thread (metadata listing does this), so an
IP-dependent plugin receives AccessContext.NONE and can refuse a valid client
request. Please thread an explicit AccessContext through decisions, build it
from supplied contexts/request data, and use the thread-local lookup only as a
fallback; cover a supplied non-current context in tests.
--
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]
