github-actions[bot] commented on code in PR #61819: URL: https://github.com/apache/doris/pull/61819#discussion_r3000305002
########## fe/fe-authentication/fe-authentication-role-mapping/src/main/java/org/apache/doris/authentication/rolemapping/UnifiedRoleMappingCelEngine.java: ########## @@ -0,0 +1,495 @@ +// 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.authentication.rolemapping; + +import org.apache.doris.authentication.BasicPrincipal; +import org.apache.doris.authentication.Principal; + +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelFunctionDecl; +import dev.cel.common.CelOverloadDecl; +import dev.cel.common.CelValidationResult; +import dev.cel.common.types.CelType; +import dev.cel.common.types.SimpleType; +import dev.cel.compiler.CelCompiler; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.parser.CelStandardMacro; +import dev.cel.runtime.CelRuntime; +import dev.cel.runtime.CelRuntime.CelFunctionBinding; +import dev.cel.runtime.CelRuntimeFactory; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; + +/** + * Single-file prototype of a unified CEL-based role-mapping engine. + * + * <p>This intentionally does not depend on OIDC token parsing or FE session + * wiring. It only implements the core rule compilation and evaluation model + * described by the unified role-mapping design.</p> + */ +public final class UnifiedRoleMappingCelEngine { + + private static final int MAX_VARIADIC_ARGUMENTS = 8; + private static final ThreadLocal<EvaluationContext> CURRENT_CONTEXT = new ThreadLocal<>(); + private static final CelCompiler COMPILER = createCompiler(); + private static final CelRuntime RUNTIME = createRuntime(); + Review Comment: **[Thread Safety - Medium]** `ThreadLocal`-based context passing is fragile: 1. **Non-reentrancy**: If `evaluate()` is ever called recursively on the same thread (e.g., via future refactoring), the outer context is silently overwritten and the `finally` block removes it entirely. 2. **CEL runtime thread delegation**: If the CEL runtime internally delegates to a different thread, `CURRENT_CONTEXT.get()` returns `null`, causing `IllegalStateException`. If the CEL library supports passing context variables through the `eval()` activation map rather than `ThreadLocal`, that would be a cleaner and safer pattern. ########## fe/fe-core/src/main/java/org/apache/doris/authentication/RoleMappingMgr.java: ########## @@ -0,0 +1,277 @@ +// 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.authentication; + +import org.apache.doris.authentication.rolemapping.UnifiedRoleMappingCelEngine; +import org.apache.doris.catalog.Env; +import org.apache.doris.common.DdlException; +import org.apache.doris.common.io.Text; +import org.apache.doris.common.io.Writable; +import org.apache.doris.nereids.trees.plans.commands.CreateRoleMappingCommand; +import org.apache.doris.persist.DropRoleMappingOperationLog; +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.gson.annotations.SerializedName; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +/** + * Manager for ROLE MAPPING metadata. + */ +public class RoleMappingMgr implements Writable { + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true); + + @SerializedName(value = "nTrm") + private Map<String, RoleMappingMeta> nameToRoleMapping = new LinkedHashMap<>(); + + private transient Map<String, String> integrationToMappingName = new LinkedHashMap<>(); + + private void readLock() { + lock.readLock().lock(); + } + + private void readUnlock() { + lock.readLock().unlock(); + } + + private void writeLock() { + lock.writeLock().lock(); + } + + private void writeUnlock() { + lock.writeLock().unlock(); + } + + public void createRoleMapping(String mappingName, boolean ifNotExists, String integrationName, + List<CreateRoleMappingCommand.RoleMappingRule> rules, String comment, String createUser) + throws DdlException { + Env env = Objects.requireNonNull(Env.getCurrentEnv(), "currentEnv can not be null"); + AuthenticationIntegrationMgr integrationMgr = env.getAuthenticationIntegrationMgr(); + integrationMgr.readLock(); + try { + writeLock(); + try { Review Comment: **[Deadlock Risk - High]** Inconsistent lock ordering between `AuthenticationIntegrationMgr` and `RoleMappingMgr`: - `createRoleMapping` (here): acquires `IntegrationMgr.readLock` → then `RoleMappingMgr.writeLock` - `dropAuthenticationIntegration` (`AuthenticationIntegrationMgr.java:128,136`): acquires `IntegrationMgr.writeLock` → then calls `RoleMappingMgr.hasRoleMapping()` which acquires `RoleMappingMgr.readLock` This is a classic ABBA deadlock pattern. Thread 1 holds IntegrationMgr and waits for RoleMappingMgr; Thread 2 holds RoleMappingMgr and waits for IntegrationMgr. **Fix:** Establish a consistent lock ordering. Either always acquire IntegrationMgr lock first, or refactor `dropAuthenticationIntegration` to check the role mapping constraint before acquiring its own write lock. ########## fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4: ########## @@ -350,6 +354,7 @@ supportedDropStatement | DROP WORKLOAD GROUP (IF EXISTS)? name=identifierOrText (FOR computeGroup=identifierOrText)? #dropWorkloadGroup Review Comment: **[Compatibility - Medium]** The tokens `MAPPING`, `CEL`, and `RULE` are used here but are NOT listed in the `nonReserved` rule (lines 1985-2370). This makes them reserved keywords, meaning any existing user query that uses these words as unquoted identifiers (e.g., `SELECT mapping FROM t`, `CREATE TABLE rule (...)`) will break after this change. **Fix:** Add `MAPPING`, `CEL`, and `RULE` to the `nonReserved` rule in `DorisParser.g4`, following the alphabetical convention used there. ########## fe/fe-authentication/fe-authentication-role-mapping/src/main/java/org/apache/doris/authentication/rolemapping/UnifiedRoleMappingCelEngine.java: ########## @@ -0,0 +1,495 @@ +// 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.authentication.rolemapping; + +import org.apache.doris.authentication.BasicPrincipal; +import org.apache.doris.authentication.Principal; + +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelFunctionDecl; +import dev.cel.common.CelOverloadDecl; +import dev.cel.common.CelValidationResult; +import dev.cel.common.types.CelType; +import dev.cel.common.types.SimpleType; +import dev.cel.compiler.CelCompiler; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.parser.CelStandardMacro; +import dev.cel.runtime.CelRuntime; +import dev.cel.runtime.CelRuntime.CelFunctionBinding; +import dev.cel.runtime.CelRuntimeFactory; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; + +/** + * Single-file prototype of a unified CEL-based role-mapping engine. + * + * <p>This intentionally does not depend on OIDC token parsing or FE session + * wiring. It only implements the core rule compilation and evaluation model + * described by the unified role-mapping design.</p> + */ +public final class UnifiedRoleMappingCelEngine { + + private static final int MAX_VARIADIC_ARGUMENTS = 8; + private static final ThreadLocal<EvaluationContext> CURRENT_CONTEXT = new ThreadLocal<>(); + private static final CelCompiler COMPILER = createCompiler(); + private static final CelRuntime RUNTIME = createRuntime(); + + private final List<CompiledRule> compiledRules; + + public UnifiedRoleMappingCelEngine(List<Rule> rules) { + Objects.requireNonNull(rules, "rules is required"); + List<CompiledRule> newCompiledRules = new ArrayList<>(rules.size()); + for (Rule rule : rules) { + newCompiledRules.add(compileRule(rule)); + } + this.compiledRules = Collections.unmodifiableList(newCompiledRules); + } + + public Set<String> evaluate(EvaluationContext context) { + Objects.requireNonNull(context, "context is required"); + LinkedHashSet<String> grantedRoles = new LinkedHashSet<>(); + CURRENT_CONTEXT.set(context); + try { + for (CompiledRule rule : compiledRules) { + Object result = rule.program.eval(Collections.emptyMap()); + if (!(result instanceof Boolean)) { + throw new IllegalStateException("Role mapping condition must evaluate to boolean"); + } + if ((Boolean) result) { + grantedRoles.addAll(rule.grantedRoles); + } + } + } catch (Exception e) { + throw new IllegalStateException("Failed to evaluate role mapping rules", e); + } finally { + CURRENT_CONTEXT.remove(); + } + return Collections.unmodifiableSet(grantedRoles); + } + + public static void main(String[] args) { + DemoResult demoResult = runMultipleMatchDemo(); + System.out.println("=== unified role mapping demo ==="); + System.out.println("principal: " + demoResult.getContext().getName()); Review Comment: **[Code Quality - Medium]** `main()` method and `runMultipleMatchDemo()` along with the `DemoResult` class (lines 323-345) are left in production code. This is test/demo code that should not ship in a security-sensitive module. The test class `RoleMappingCelPlayground` already exists for this purpose. **Fix:** Remove `main()`, `runMultipleMatchDemo()`, and `DemoResult` from this production class. -- 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]
