Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for authorizing query context params #12396

Merged
merged 16 commits into from
Apr 21, 2022
Merged

Add support for authorizing query context params #12396

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5972c8f
Add support for authorizing query context params
jihoonson Apr 4, 2022
2fa50fa
fix npe in test
jihoonson Apr 5, 2022
b074831
more test
jihoonson Apr 5, 2022
6c4c96b
unused import
jihoonson Apr 5, 2022
6a2eea6
fix integration test
jihoonson Apr 13, 2022
d470e17
context in QueryPlus
jihoonson Apr 13, 2022
c0a4962
context in query
jihoonson Apr 13, 2022
d7a0c7d
fix ci
jihoonson Apr 14, 2022
af2dfe2
redundant import
jihoonson Apr 14, 2022
600c5b2
fix ldap test
jihoonson Apr 15, 2022
2c3314d
Merge branch 'master' of github.com:apache/druid into context-param-a…
jihoonson Apr 15, 2022
b349b65
fix unit test
jihoonson Apr 15, 2022
4640b16
address comments
jihoonson Apr 19, 2022
5c3fcb7
fix auth test for extensions
jihoonson Apr 20, 2022
2ef7ea5
address more comments
jihoonson Apr 20, 2022
d292fd5
suppress unused warning
jihoonson Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...election/src/main/java/org/apache/druid/query/materializedview/MaterializedViewQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.druid.query.BaseQuery;
import org.apache.druid.query.DataSource;
import org.apache.druid.query.Query;
import org.apache.druid.query.QueryContext;
import org.apache.druid.query.QueryRunner;
import org.apache.druid.query.QuerySegmentWalker;
import org.apache.druid.query.filter.DimFilter;
Expand Down Expand Up @@ -145,6 +146,12 @@ public Map<String, Object> getContext()
return query.getContext();
}

@Override
public QueryContext getQueryContext()
{
return query.getQueryContext();
}

@Override
public <ContextType> ContextType getContextValue(String key)
{
Expand Down
12 changes: 6 additions & 6 deletions ...uery/aggregation/datasketches/quantiles/sql/DoublesSketchApproxQuantileSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
import org.apache.calcite.sql.type.ReturnTypes;
import org.apache.calcite.sql.type.SqlTypeFamily;
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.druid.java.util.common.Numbers;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.query.QueryContext;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.datasketches.quantiles.DoublesSketchAggregatorFactory;
import org.apache.druid.query.aggregation.datasketches.quantiles.DoublesSketchToQuantilePostAggregator;
Expand All @@ -50,7 +50,6 @@

import javax.annotation.Nullable;
import java.util.List;
import java.util.Map;

public class DoublesSketchApproxQuantileSqlAggregator implements SqlAggregator
{
Expand Down Expand Up @@ -200,11 +199,12 @@ public Aggregation toDruidAggregation(
);
}

@Nullable
static Long getMaxStreamLengthFromQueryContext(Map<String, Object> queryContext)
static long getMaxStreamLengthFromQueryContext(QueryContext queryContext)
{
final Object val = queryContext.get(CTX_APPROX_QUANTILE_DS_MAX_STREAM_LENGTH);
return val == null ? null : Numbers.parseLong(val);
return queryContext.getAsLong(
CTX_APPROX_QUANTILE_DS_MAX_STREAM_LENGTH,
DoublesSketchAggregatorFactory.DEFAULT_MAX_STREAM_LENGTH
);
}

private static class DoublesSketchApproxQuantileSqlAggFunction extends SqlAggFunction
Expand Down
1 change: 1 addition & 0 deletions integration-tests/docker/environment-configs/common
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ druid_auth_authenticator_basic_type=basic
druid_auth_authenticatorChain=["basic"]
druid_auth_authorizer_basic_type=basic
druid_auth_authorizers=["basic"]
druid_auth_authorizeQueryContextParams=true
druid_client_https_certAlias=druid
druid_client_https_keyManagerPassword=druid123
druid_client_https_keyStorePassword=druid123
Expand Down
1 change: 1 addition & 0 deletions integration-tests/docker/environment-configs/common-ldap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ druid_auth_authorizer_ldapauth_initialAdminUser=admin
druid_auth_authorizer_ldapauth_initialAdminRole=admin
druid_auth_authorizer_ldapauth_roleProvider_type=ldap
druid_auth_authorizers=["ldapauth"]
druid_auth_authorizeQueryContextParams=true
druid_client_https_certAlias=druid
druid_client_https_keyManagerPassword=druid123
druid_client_https_keyStorePassword=druid123
Expand Down
18 changes: 18 additions & 0 deletions integration-tests/docker/ldap-configs/bootstrap.ldif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,21 @@ objectClass: groupOfUniqueNames
cn: datasourceWithSysGroup
description: datasourceWithSysGroup users
uniqueMember: uid=datasourceAndSysUser,ou=Users,dc=example,dc=org

dn: uid=datasourceAndContextParamsUser,ou=Users,dc=example,dc=org
uid: datasourceAndContextParamsUser
cn: datasourceAndContextParamsUser
sn: datasourceAndContextParamsUser
objectClass: top
objectClass: posixAccount
objectClass: inetOrgPerson
homeDirectory: /home/datasourceAndContextParamsUser
uidNumber: 9
gidNumber: 9
userPassword: helloworld

dn: cn=datasourceAndContextParamsGroup,ou=Groups,dc=example,dc=org
objectClass: groupOfUniqueNames
cn: datasourceAndContextParamsGroup
description: datasourceAndContextParamsGroup users
uniqueMember: uid=datasourceAndContextParamsUser,ou=Users,dc=example,dc=org
3 changes: 1 addition & 2 deletions integration-tests/src/main/java/org/apache/druid/testing/utils/HttpUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.apache.druid.java.util.http.client.Request;
import org.apache.druid.java.util.http.client.response.StatusResponseHandler;
import org.apache.druid.java.util.http.client.response.StatusResponseHolder;
import org.apache.druid.testing.clients.AbstractQueryResourceTestClient;
import org.jboss.netty.handler.codec.http.HttpMethod;
import org.jboss.netty.handler.codec.http.HttpResponseStatus;

Expand All @@ -36,7 +35,7 @@

public class HttpUtil
{
private static final Logger LOG = new Logger(AbstractQueryResourceTestClient.class);
private static final Logger LOG = new Logger(HttpUtil.class);
private static final StatusResponseHandler RESPONSE_HANDLER = StatusResponseHandler.getInstance();

static final int NUM_RETRIES = 30;
Expand Down
129 changes: 114 additions & 15 deletions ...on-tests/src/test/java/org/apache/druid/tests/security/AbstractAuthConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,17 @@ public abstract class AbstractAuthConfigurationTest
)
);

protected static final List<ResourceAction> DATASOURCE_QUERY_CONTEXT_PERMISSIONS = ImmutableList.of(
new ResourceAction(
new Resource("auth_test", ResourceType.DATASOURCE),
Action.READ
),
new ResourceAction(
new Resource("auth_test_ctx", ResourceType.QUERY_CONTEXT),
Action.WRITE
)
);

/**
* create a ResourceAction set of permissions that can only read 'auth_test' + partial SYSTEM_TABLE, for Authorizer
* implementations which use ResourceAction pattern matching
Expand Down Expand Up @@ -188,22 +199,23 @@ public abstract class AbstractAuthConfigurationTest

protected HttpClient adminClient;
protected HttpClient datasourceOnlyUserClient;
protected HttpClient datasourceAndContextParamsClient;
protected HttpClient datasourceAndSysUserClient;
protected HttpClient datasourceWithStateUserClient;
protected HttpClient stateOnlyUserClient;
protected HttpClient internalSystemClient;


protected abstract void setupDatasourceOnlyUser() throws Exception;
protected abstract void setupDatasourceAndContextParamsUser() throws Exception;
protected abstract void setupDatasourceAndSysTableUser() throws Exception;
protected abstract void setupDatasourceAndSysAndStateUser() throws Exception;
protected abstract void setupSysTableAndStateOnlyUser() throws Exception;
protected abstract void setupTestSpecificHttpClients() throws Exception;
protected abstract String getAuthenticatorName();
protected abstract String getAuthorizerName();
protected abstract String getExpectedAvaticaAuthError();
protected abstract Properties getAvaticaConnectionProperties();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:
Maybe we should just add a new testAvaticaQuery(properties, url)and leave the existingtestAvaticaQuery as is?
Otherwise, we are forced to pass the properties to testAvaticaQuery in every downstream test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my bad that I removed this method before. I missed that implementations of this class can have a different implementation for this method. I added them back.

for testAvaticaQuery, I think it's OK to change the method signature as the new signature makes it clearer what user's credentials are used for the test.

protected abstract Properties getAvaticaConnectionPropertiesFailure();
protected abstract String getExpectedAvaticaAuthzError();

@Test
public void test_systemSchemaAccess_admin() throws Exception
Expand Down Expand Up @@ -444,27 +456,71 @@ public void test_internalSystemUser_hasNodeAccess()
@Test
public void test_avaticaQuery_broker()
{
testAvaticaQuery(getBrokerAvacticaUrl());
testAvaticaQuery(StringUtils.maybeRemoveTrailingSlash(getBrokerAvacticaUrl()));
final Properties properties = getAvaticaConnectionPropertiesForAdmin();
testAvaticaQuery(properties, getBrokerAvacticaUrl());
testAvaticaQuery(properties, StringUtils.maybeRemoveTrailingSlash(getBrokerAvacticaUrl()));
}

@Test
public void test_avaticaQuery_router()
{
testAvaticaQuery(getRouterAvacticaUrl());
testAvaticaQuery(StringUtils.maybeRemoveTrailingSlash(getRouterAvacticaUrl()));
final Properties properties = getAvaticaConnectionPropertiesForAdmin();
testAvaticaQuery(properties, getRouterAvacticaUrl());
testAvaticaQuery(properties, StringUtils.maybeRemoveTrailingSlash(getRouterAvacticaUrl()));
}

@Test
public void test_avaticaQueryAuthFailure_broker() throws Exception
{
testAvaticaAuthFailure(getBrokerAvacticaUrl());
final Properties properties = getAvaticaConnectionPropertiesForWrongAdmin();
testAvaticaAuthFailure(properties, getBrokerAvacticaUrl());
}

@Test
public void test_avaticaQueryAuthFailure_router() throws Exception
{
testAvaticaAuthFailure(getRouterAvacticaUrl());
final Properties properties = getAvaticaConnectionPropertiesForWrongAdmin();
testAvaticaAuthFailure(properties, getRouterAvacticaUrl());
}

@Test
public void test_avaticaQueryWithContext_datasourceOnlyUser_fail() throws Exception
{
final Properties properties = getAvaticaConnectionPropertiesForUser("datasourceOnlyUser", "helloworld");
properties.setProperty("auth_test_ctx", "should-be-denied");
testAvaticaAuthzFailure(properties, getRouterAvacticaUrl());
}

@Test
public void test_avaticaQueryWithContext_datasourceAndContextParamsUser_succeed()
{
final Properties properties = getAvaticaConnectionPropertiesForUser("datasourceAndContextParamsUser", "helloworld");
properties.setProperty("auth_test_ctx", "should-be-allowed");
testAvaticaQuery(properties, getRouterAvacticaUrl());
}

@Test
public void test_sqlQueryWithContext_datasourceOnlyUser_fail() throws Exception
{
final String query = "select count(*) from auth_test";
StatusResponseHolder responseHolder = makeSQLQueryRequest(
datasourceOnlyUserClient,
query,
ImmutableMap.of("auth_test_ctx", "should-be-denied"),
HttpResponseStatus.FORBIDDEN
);
}

@Test
public void test_sqlQueryWithContext_datasourceAndContextParamsUser_succeed() throws Exception
{
final String query = "select count(*) from auth_test";
StatusResponseHolder responseHolder = makeSQLQueryRequest(
datasourceAndContextParamsClient,
query,
ImmutableMap.of("auth_test_ctx", "should-be-allowed"),
HttpResponseStatus.OK
);
}

@Test
Expand Down Expand Up @@ -501,6 +557,7 @@ protected void setupHttpClientsAndUsers() throws Exception
{
setupHttpClients();
setupDatasourceOnlyUser();
setupDatasourceAndContextParamsUser();
setupDatasourceAndSysTableUser();
setupDatasourceAndSysAndStateUser();
setupSysTableAndStateOnlyUser();
Expand Down Expand Up @@ -538,11 +595,28 @@ protected void checkUnsecuredCoordinatorLoadQueuePath(HttpClient client)
HttpUtil.makeRequest(client, HttpMethod.GET, config.getCoordinatorUrl() + "/druid/coordinator/v1/loadqueue", null);
}

protected void testAvaticaQuery(String url)
private Properties getAvaticaConnectionPropertiesForAdmin()
{
return getAvaticaConnectionPropertiesForUser("admin", "priest");
}

private Properties getAvaticaConnectionPropertiesForWrongAdmin()
{
return getAvaticaConnectionPropertiesForUser("admin", "wrongpassword");
}

private Properties getAvaticaConnectionPropertiesForUser(String id, String password)
{
Properties connectionProperties = new Properties();
connectionProperties.setProperty("user", id);
connectionProperties.setProperty("password", password);
return connectionProperties;
}

protected void testAvaticaQuery(Properties connectionProperties, String url)
{
LOG.info("URL: " + url);
try {
Properties connectionProperties = getAvaticaConnectionProperties();
Connection connection = DriverManager.getConnection(url, connectionProperties);
Statement statement = connection.createStatement();
statement.setMaxRows(450);
Expand All @@ -557,11 +631,21 @@ protected void testAvaticaQuery(String url)
}
}

protected void testAvaticaAuthFailure(String url) throws Exception
protected void testAvaticaAuthFailure(Properties connectionProperties, String url) throws Exception
{
testAvaticaAuthFailure(connectionProperties, url, getExpectedAvaticaAuthError());
}

protected void testAvaticaAuthzFailure(Properties connectionProperties, String url) throws Exception
{
testAvaticaAuthFailure(connectionProperties, url, getExpectedAvaticaAuthzError());
}

protected void testAvaticaAuthFailure(Properties connectionProperties, String url, String expectedError)
throws Exception
{
LOG.info("URL: " + url);
try {
Properties connectionProperties = getAvaticaConnectionPropertiesFailure();
Connection connection = DriverManager.getConnection(url, connectionProperties);
Statement statement = connection.createStatement();
statement.setMaxRows(450);
Expand All @@ -571,7 +655,7 @@ protected void testAvaticaAuthFailure(String url) throws Exception
catch (AvaticaSqlException ase) {
Assert.assertEquals(
ase.getErrorMessage(),
getExpectedAvaticaAuthError()
expectedError
);
return;
}
Expand Down Expand Up @@ -612,9 +696,20 @@ protected StatusResponseHolder makeSQLQueryRequest(
String query,
HttpResponseStatus expectedStatus
) throws Exception
{
return makeSQLQueryRequest(httpClient, query, ImmutableMap.of(), expectedStatus);
}

protected StatusResponseHolder makeSQLQueryRequest(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Maybe rename to something like makeSqlRequestAndVerifyStatus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually not a new method, but I added a new parameter for this method. I would like to not rename it as this PR is already quite big.

HttpClient httpClient,
String query,
Map<String, Object> context,
HttpResponseStatus expectedStatus
) throws Exception
{
Map<String, Object> queryMap = ImmutableMap.of(
"query", query
"query", query,
"context", context
);
return HttpUtil.makeRequestWithExpectedStatus(
httpClient,
Expand Down Expand Up @@ -758,7 +853,6 @@ protected void setupHttpClients() throws Exception
setupTestSpecificHttpClients();
}


protected void setupCommonHttpClients()
{
adminClient = new CredentialedHttpClient(
Expand All @@ -771,6 +865,11 @@ protected void setupCommonHttpClients()
httpClient
);

datasourceAndContextParamsClient = new CredentialedHttpClient(
new BasicCredentials("datasourceAndContextParamsUser", "helloworld"),
httpClient
);

datasourceAndSysUserClient = new CredentialedHttpClient(
new BasicCredentials("datasourceAndSysUser", "helloworld"),
httpClient
Expand Down
Loading