github-actions[bot] commented on code in PR #67672:
URL: https://github.com/apache/doris/pull/67672#discussion_r4420511712
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnFE.java:
##########
@@ -399,6 +401,12 @@ public Expression visitCurrentUser(CurrentUser
currentUser, ExpressionRewriteCon
return new VarcharLiteral(res);
}
+ @Override
Review Comment:
[P1] Preserve `session_is_narrowed()` in context-free folding.
`visitSessionIsNarrowed` ignores `foldContextDependentExpressions` and
dereferences `context.cascadesContext`; `evaluateWithoutContext` passes a null
context, so expressions such as a metadata predicate can throw or be folded to
FALSE for every normal session. Add the same guard as
`visitCurrentUser`/`visitSessionUser` and only evaluate it when context folding
is enabled.
##########
be/src/information_schema/schema_table_privileges_scanner.cpp:
##########
@@ -63,6 +63,9 @@ Status SchemaTablePrivilegesScanner::_get_new_table() {
}
if (nullptr != _param->common_param->current_user_ident) {
table_params.__set_current_user_ident(*(_param->common_param->current_user_ident));
+ if (!_param->common_param->current_roles.empty()) {
+
table_params.__set_current_roles(_param->common_param->current_roles);
Review Comment:
[P1] Install `current_roles` in every TGetTables metadata handler. These
scanners now send the SU role subset here, but `listTableMetadataNameIds`,
`listTablePrivilegeStatus`, `listSchemaPrivilegeStatus`, and
`listUserPrivilegeStatus` never call `withSessionRoleOverride`; their Auth
checks therefore run with no `ConnectContext` and resolve the target's full
role union. A narrowed session can regain excluded direct SHOW/SELECT/GRANT
rows, and the name/ID handler performs its check in a new executor thread where
a handler ThreadLocal would not propagate. Apply the override inside each
handler/task or pass the role set explicitly before checking privileges.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/SchemaScanNode.java:
##########
@@ -162,6 +164,13 @@ protected void toThrift(TPlanNode msg) {
TUserIdentity tCurrentUser =
ConnectContext.get().getCurrentUserIdentity().toThrift();
msg.schema_scan_node.setCurrentUserIdent(tCurrentUser);
+ // SU narrowing: carry the session's active role subset
(current_roles) in the plan so the
+ // BE->FE metadata RPCs
(getDbNames/getTableNames/listTableStatus/describeTables) narrow
+ // name visibility to the same set the session sees; null = normal
session (no narrowing).
Review Comment:
[P1] Gate BE metadata callbacks during rolling upgrades. This sends the SU
role subset to FE RPCs, but old FEs treat the field as reserved/unknown and
authorize the target's full role union. `SchemaScanNode` can call every alive
FE for several schema tables, and other metadata paths call the master; unlike
FE-to-master SQL forwarding, these calls have no same-build check. Route
narrowed metadata only to compatible FEs or fail closed until all peers
understand `current_roles`.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/FEOpExecutor.java:
##########
@@ -194,6 +195,13 @@ protected TMasterOpRequest buildStmtForwardParams() throws
AnalysisException {
params.setUserIp(ctx.getRemoteIP());
params.setStmtId(ctx.getStmtId());
params.setCurrentUserIdent(ctx.getCurrentUserIdentity().toThrift());
Review Comment:
[P2] Preserve the authenticated identity in every audit path. The forwarding
additions carry only roles here; the master proxy applies them but leaves
`authenticatedIdentity` null, so forwarded SU statements are audited with the
target as both users. The REST AFTER_QUERY builder also omits
`setAuthenticatedUser`, leaving the new column blank. Carry the switcher
identity in the forward request and populate the field in all AFTER_QUERY
emitters (or provide a safe common fallback).
##########
be/src/exec/operator/schema_scan_operator.cpp:
##########
@@ -113,6 +113,12 @@ Status SchemaScanOperatorX::init(const TPlanNode& tnode,
RuntimeState* state) {
}
}
+ // SU narrowing: carry the SU-narrowed role subset from the FE plan to the
scanners so the
+ // BE->FE metadata RPCs narrow name visibility (empty/unset = no
narrowing, prior behavior).
+ if (tnode.schema_scan_node.__isset.current_roles) {
+ _common_scanner_param->current_roles =
tnode.schema_scan_node.current_roles;
Review Comment:
[P1] Carry narrowing through SCH_USER as well. This stores
`TSchemaScanNode.current_roles`, but `SchemaUserScanner` still builds a
TShowUserRequest with only the user identity and `showUser` is not wrapped. If
the target has a direct GRANT/ADMIN role omitted from SU, `mysql.user` can
expose the target's full account list and privilege topology. Add a role field
to this request and apply/clear it in the handler (or use the common metadata
path).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/SuUserCommand.java:
##########
@@ -0,0 +1,143 @@
+// 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.nereids.trees.plans.commands;
+
+import org.apache.doris.analysis.StmtType;
+import org.apache.doris.analysis.UserIdentity;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.FeNameFormat;
+import org.apache.doris.mysql.privilege.Auth;
+import org.apache.doris.mysql.privilege.PrivPredicate;
+import org.apache.doris.nereids.trees.plans.PlanType;
+import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.StmtExecutor;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * SU 'user'[@'host'] WITH ROLES ('role1'[, ...]) [WORKLOAD GROUP 'wg']
+ *
+ * <p>Session-narrowed identity switch, MySQL's proxy-user model with a
mandatory role list: the
+ * session becomes the TARGET identity (audit, current_user(), user-bound row
policies, user
+ * properties) with a role set that REPLACES every role source, enforced at
the single choke point
+ * {@code Auth.getRolesByUserWithLdap}. Session-only state; nothing is
persisted.
+ *
+ * <p>Contract:
+ * <ul>
+ * <li>Allowed only for accounts holding {@code PROXY_PRIV} (or {@code
ADMIN_PRIV}, which implies
+ * it); a session that does not switch keeps the switching account's own
grants.</li>
+ * <li>Narrowing only: every requested role must already be GRANTED to the
target user, so the
+ * session can never exceed the person's real authority.</li>
+ * <li>One-shot: a switched session cannot SU again, and nothing widens it. A
connection reset
+ * reverts to the authenticated identity ({@link
ConnectContext#revertSessionNarrowing}).</li>
+ * </ul>
+ */
+public class SuUserCommand extends Command implements NoForward {
+ private static final Logger LOG =
LogManager.getLogger(SuUserCommand.class);
+
+ private final UserIdentity userIdentity;
+ private final List<String> roles;
+ private final String workloadGroup; // nullable
+
+ public SuUserCommand(UserIdentity userIdentity, List<String> roles, String
workloadGroup) {
+ super(PlanType.SU_USER_COMMAND);
+ Preconditions.checkArgument(!roles.isEmpty(), "SU requires at least
one role");
+ this.userIdentity = Objects.requireNonNull(userIdentity, "userIdentity
is null");
+ this.roles = Objects.requireNonNull(roles, "roles is null");
+ this.workloadGroup = workloadGroup;
+ }
+
+ @Override
+ public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
+ // a switched session must never widen or re-target itself
+ if (ctx.getAuthenticatedIdentity() != null) {
+ throw new AnalysisException("SU is not allowed in an
already-switched session");
+ }
+ if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx,
PrivPredicate.PROXY)) {
+ throw new AnalysisException("Access denied: SU requires PROXY_PRIV
(or ADMIN_PRIV)");
+ }
+
+ userIdentity.analyze();
+ for (String role : roles) {
+ FeNameFormat.checkRoleName(role, true /* can be admin */, "Invalid
role in SU");
+ }
+
+ Auth auth = Env.getCurrentEnv().getAuth();
+ Set<String> targetRoles = auth.getGrantedRoleNames(userIdentity);
+ if (targetRoles.isEmpty()) {
+ // every existing user holds at least its default role
+ throw new AnalysisException("SU target user does not exist: " +
userIdentity);
+ }
+ // narrowing only: the requested roles must be a subset of the
target's granted roles
+ for (String role : roles) {
+ if (!targetRoles.contains(role)) {
+ throw new AnalysisException("SU role '" + role + "' is not
granted to the target user"
+ + " -- SU can only narrow, never mint authority");
+ }
+ }
+
+ UserIdentity authenticated = ctx.getCurrentUserIdentity();
+ ctx.setAuthenticatedIdentity(authenticated);
+ ctx.setCurrentUserIdentity(userIdentity);
+ ctx.setSessionRoleOverride(ImmutableSet.copyOf(roles));
+ if (!Strings.isNullOrEmpty(workloadGroup)) {
+ // Placement only. USAGE on the session's workload group (this
clause, or the
+ // target's default_workload_group when omitted) is checked per
query against the
+ // narrowed set widened with the target's own granted roles
+ // (Auth.getRolesForWorkloadGroupCheck): the person's lane follows
the person.
+ ctx.getSessionVariable().setWorkloadGroup(workloadGroup);
Review Comment:
[P2] Recompute the target workload group when the clause is omitted. This
only sets `SessionVariable.workloadGroup` for an explicit clause;
`WorkloadGroupMgr` otherwise honors the switcher's already-set session value
before looking up the target's default. Thus `SET
workload_group='switcher_lane'; SU target ...` executes as the target in the
wrong lane, contrary to the documented omitted-clause behavior.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java:
##########
@@ -378,6 +386,41 @@ public void resetConnection() throws UserException {
insertResult = null;
command = MysqlCommand.COM_SLEEP;
returnRows = 0;
+ // A narrowed session reverts to the AUTHENTICATED identity on
connection reset —
+ // dropping only the override would widen the session to the target's
full roles.
Review Comment:
[P2] Restore the authenticated identity before resetting session defaults.
`resetConnection` calls `resetSessionVariable()` while `currentUserIdentity` is
still the SU target, so `applyUserSessionVariableDefaults()` loads the target's
per-user query/insert timeouts; `revertSessionNarrowing()` then switches back
without recalculating them. After COM_RESET_CONNECTION, the original account
runs with the target's settings. Revert first or reapply defaults after the
revert.
##########
be/src/information_schema/schema_workload_group_privileges.cpp:
##########
@@ -55,6 +55,9 @@ Status
SchemaWorkloadGroupPrivilegesScanner::_get_workload_group_privs_block_fro
schema_table_request_params.columns_name.emplace_back(_s_tbls_columns[i].name);
}
schema_table_request_params.__set_current_user_ident(*_param->common_param->current_user_ident);
+ if (!_param->common_param->current_roles.empty()) {
+
schema_table_request_params.__set_current_roles(_param->common_param->current_roles);
Review Comment:
[P1] Honor the narrowed roles when enumerating workload-group privileges.
The RPC now carries `current_roles` here and the handler installs it, but
`getUserRoleWorkloadGroupPrivs` later reads `userRoleManager.getRolesByUser`
directly and emits every target role's workload-group grants. A SU session
therefore sees excluded role privileges in
`information_schema.workload_group_privileges`; derive the filter from the
narrowed role set for this metadata enumeration while keeping the separate
placement-check widening.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/SuUserCommand.java:
##########
@@ -0,0 +1,143 @@
+// 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.nereids.trees.plans.commands;
+
+import org.apache.doris.analysis.StmtType;
+import org.apache.doris.analysis.UserIdentity;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.FeNameFormat;
+import org.apache.doris.mysql.privilege.Auth;
+import org.apache.doris.mysql.privilege.PrivPredicate;
+import org.apache.doris.nereids.trees.plans.PlanType;
+import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.StmtExecutor;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+/**
+ * SU 'user'[@'host'] WITH ROLES ('role1'[, ...]) [WORKLOAD GROUP 'wg']
+ *
+ * <p>Session-narrowed identity switch, MySQL's proxy-user model with a
mandatory role list: the
+ * session becomes the TARGET identity (audit, current_user(), user-bound row
policies, user
+ * properties) with a role set that REPLACES every role source, enforced at
the single choke point
+ * {@code Auth.getRolesByUserWithLdap}. Session-only state; nothing is
persisted.
+ *
+ * <p>Contract:
+ * <ul>
+ * <li>Allowed only for accounts holding {@code PROXY_PRIV} (or {@code
ADMIN_PRIV}, which implies
+ * it); a session that does not switch keeps the switching account's own
grants.</li>
+ * <li>Narrowing only: every requested role must already be GRANTED to the
target user, so the
+ * session can never exceed the person's real authority.</li>
+ * <li>One-shot: a switched session cannot SU again, and nothing widens it. A
connection reset
+ * reverts to the authenticated identity ({@link
ConnectContext#revertSessionNarrowing}).</li>
+ * </ul>
+ */
+public class SuUserCommand extends Command implements NoForward {
+ private static final Logger LOG =
LogManager.getLogger(SuUserCommand.class);
+
+ private final UserIdentity userIdentity;
+ private final List<String> roles;
+ private final String workloadGroup; // nullable
+
+ public SuUserCommand(UserIdentity userIdentity, List<String> roles, String
workloadGroup) {
+ super(PlanType.SU_USER_COMMAND);
+ Preconditions.checkArgument(!roles.isEmpty(), "SU requires at least
one role");
+ this.userIdentity = Objects.requireNonNull(userIdentity, "userIdentity
is null");
+ this.roles = Objects.requireNonNull(roles, "roles is null");
+ this.workloadGroup = workloadGroup;
+ }
+
+ @Override
+ public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
+ // a switched session must never widen or re-target itself
+ if (ctx.getAuthenticatedIdentity() != null) {
+ throw new AnalysisException("SU is not allowed in an
already-switched session");
+ }
+ if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx,
PrivPredicate.PROXY)) {
+ throw new AnalysisException("Access denied: SU requires PROXY_PRIV
(or ADMIN_PRIV)");
+ }
+
+ userIdentity.analyze();
+ for (String role : roles) {
+ FeNameFormat.checkRoleName(role, true /* can be admin */, "Invalid
role in SU");
+ }
+
+ Auth auth = Env.getCurrentEnv().getAuth();
+ Set<String> targetRoles = auth.getGrantedRoleNames(userIdentity);
Review Comment:
[P2] Snapshot target grants under Auth's read lock. `getGrantedRoleNames`
iterates `UserRoleManager`/LDAP state without acquiring `Auth.readLock`, while
SU invokes it outside a lock; a concurrent GRANT/REVOKE or LDAP refresh can
race the subset check and accept/reject inconsistently or throw during
iteration. Protect the snapshot with the existing Auth lock (and release before
any external work if needed).
--
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]
