Skip to content

Commit

Permalink
[enhance](auth)The priority of attributes is higher than session vari…
Browse files Browse the repository at this point in the history
…able (apache#47185)

### What problem does this PR solve?
The current priority is that session variable is higher than user
property, which is incorrect because session variable can be set freely
by ordinary users and will bypass the restrictions set by administrators

Scope of Impact:
- getInsertTimeoutS
- getQueryTimeoutS
- getMaxExecMemByte
  • Loading branch information
zddr authored and lzyy2024 committed Feb 21, 2025
1 parent 4288cf3 commit 659159a
Show file tree
Hide file tree
Showing 5 changed files with 191 additions and 8 deletions.
65 changes: 63 additions & 2 deletions fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import io.netty.util.concurrent.FastThreadLocal;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.json.JSONObject;
Expand Down Expand Up @@ -1025,15 +1026,75 @@ public String getCurrentConnectedFEIp() {
public int getExecTimeout() {
if (executor != null && executor.isSyncLoadKindStmt()) {
// particular for insert stmt, we can expand other type of timeout in the same way
return Math.max(sessionVariable.getInsertTimeoutS(), sessionVariable.getQueryTimeoutS());
return Math.max(getInsertTimeoutS(), getQueryTimeoutS());
} else if (executor != null && executor.isAnalyzeStmt()) {
return sessionVariable.getAnalyzeTimeoutS();
} else {
// normal query stmt
return sessionVariable.getQueryTimeoutS();
return getQueryTimeoutS();
}
}

/**
* First, retrieve from the user's attributes. If not, retrieve from the session variable
*
* @return insertTimeoutS
*/
public int getInsertTimeoutS() {
int userInsertTimeout = getInsertTimeoutSFromProperty();
if (userInsertTimeout > 0) {
return userInsertTimeout;
}
return sessionVariable.getInsertTimeoutS();
}

private int getInsertTimeoutSFromProperty() {
if (env == null || env.getAuth() == null || StringUtils.isEmpty(getQualifiedUser())) {
return 0;
}
return env.getAuth().getInsertTimeout(getQualifiedUser());
}

/**
* First, retrieve from the user's attributes. If not, retrieve from the session variable
*
* @return queryTimeoutS
*/
public int getQueryTimeoutS() {
int userQueryTimeout = getQueryTimeoutSFromProperty();
if (userQueryTimeout > 0) {
return userQueryTimeout;
}
return sessionVariable.getQueryTimeoutS();
}

private int getQueryTimeoutSFromProperty() {
if (env == null || env.getAuth() == null || StringUtils.isEmpty(getQualifiedUser())) {
return 0;
}
return env.getAuth().getQueryTimeout(getQualifiedUser());
}

/**
* First, retrieve from the user's attributes. If not, retrieve from the session variable
*
* @return maxExecMemByte
*/
public long getMaxExecMemByte() {
long userLimit = getMaxExecMemByteFromProperty();
if (userLimit > 0) {
return userLimit;
}
return sessionVariable.getMaxExecMemByte();
}

private long getMaxExecMemByteFromProperty() {
if (env == null || env.getAuth() == null || StringUtils.isEmpty(getQualifiedUser())) {
return 0L;
}
return env.getAuth().getExecMemLimit(getQualifiedUser());
}

public void setResultAttachedInfo(Map<String, String> resultAttachedInfo) {
this.resultAttachedInfo = resultAttachedInfo;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,7 @@ private void setFromUserProperty(ConnectContext connectContext) {
this.queryOptions.setResourceLimit(resourceLimit);
}
// set exec mem limit
long maxExecMemByte = connectContext.getSessionVariable().getMaxExecMemByte();
long memLimit = maxExecMemByte > 0 ? maxExecMemByte :
Env.getCurrentEnv().getAuth().getExecMemLimit(qualifiedUser);
long memLimit = connectContext.getMaxExecMemByte();
if (memLimit > 0) {
// overwrite the exec_mem_limit from session variable;
this.queryOptions.setMemLimit(memLimit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,7 @@ private static void setOptionsFromUserProperty(ConnectContext connectContext, TQ
queryOptions.setResourceLimit(resourceLimit);
}
// set exec mem limit
long maxExecMemByte = connectContext.getSessionVariable().getMaxExecMemByte();
long memLimit = maxExecMemByte > 0 ? maxExecMemByte :
Env.getCurrentEnv().getAuth().getExecMemLimit(qualifiedUser);
long memLimit = connectContext.getMaxExecMemByte();
if (memLimit > 0) {
// overwrite the exec_mem_limit from session variable;
queryOptions.setMemLimit(memLimit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.doris.mysql.privilege.Auth;
import org.apache.doris.thrift.TUniqueId;

import mockit.Expectations;
import mockit.Mocked;
import org.junit.Assert;
import org.junit.Before;
Expand Down Expand Up @@ -202,4 +203,73 @@ public void testThreadLocal() {
Assert.assertNotNull(ConnectContext.get());
Assert.assertEquals(ctx, ConnectContext.get());
}

@Test
public void testGetMaxExecMemByte() {
ConnectContext context = new ConnectContext();
context.setQualifiedUser("a");
context.setEnv(env);
long sessionValue = 2097153L;
long propertyValue = 2097154L;
// only session
context.getSessionVariable().setMaxExecMemByte(sessionValue);
long result = context.getMaxExecMemByte();
Assert.assertEquals(sessionValue, result);
// has property
new Expectations() {
{
auth.getExecMemLimit(anyString);
minTimes = 0;
result = propertyValue;
}
};
result = context.getMaxExecMemByte();
Assert.assertEquals(propertyValue, result);
}

@Test
public void testGetQueryTimeoutS() {
ConnectContext context = new ConnectContext();
context.setQualifiedUser("a");
context.setEnv(env);
int sessionValue = 1;
int propertyValue = 2;
// only session
context.getSessionVariable().setQueryTimeoutS(sessionValue);
long result = context.getQueryTimeoutS();
Assert.assertEquals(sessionValue, result);
// has property
new Expectations() {
{
auth.getQueryTimeout(anyString);
minTimes = 0;
result = propertyValue;
}
};
result = context.getQueryTimeoutS();
Assert.assertEquals(propertyValue, result);
}

@Test
public void testInsertQueryTimeoutS() {
ConnectContext context = new ConnectContext();
context.setQualifiedUser("a");
context.setEnv(env);
int sessionValue = 1;
int propertyValue = 2;
// only session
context.getSessionVariable().setInsertTimeoutS(sessionValue);
long result = context.getInsertTimeoutS();
Assert.assertEquals(sessionValue, result);
// has property
new Expectations() {
{
auth.getInsertTimeout(anyString);
minTimes = 0;
result = propertyValue;
}
};
result = context.getInsertTimeoutS();
Assert.assertEquals(propertyValue, result);
}
}
56 changes: 56 additions & 0 deletions regression-test/suites/account_p0/test_property_session.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// 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.

import org.junit.Assert;

suite("test_property_session") {
String suiteName = "test_property_session"
String userName = "${suiteName}_user"
String pwd = 'C123_567p'
sql """drop user if exists ${userName}"""
sql """CREATE USER '${userName}' IDENTIFIED BY '${pwd}'"""

//cloud-mode
if (isCloudMode()) {
def clusters = sql " SHOW CLUSTERS; "
assertTrue(!clusters.isEmpty())
def validCluster = clusters[0][0]
sql """GRANT USAGE_PRIV ON CLUSTER `${validCluster}` TO ${userName}""";
}
sql """GRANT select_PRIV ON *.*.* TO ${userName}""";
connect(user=userName, password="${pwd}", url=context.config.jdbcUrl) {
sql """
set query_timeout=1;
"""
test {
sql """
select sleep(3);
"""
exception "timeout"
}
}

// the priority of property should be higher than session
sql """set property for '${userName}' 'query_timeout' = '10';"""
connect(user=userName, password="${pwd}", url=context.config.jdbcUrl) {
sql """
select sleep(3);
"""
}

sql """drop user if exists ${userName}"""
}

0 comments on commit 659159a

Please sign in to comment.