-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
Changes from 4 commits
5972c8f
2fa50fa
b074831
6c4c96b
6a2eea6
d470e17
c0a4962
d7a0c7d
af2dfe2
600c5b2
2c3314d
b349b65
4640b16
5c3fcb7
2ef7ea5
d292fd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -188,22 +199,22 @@ 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(); | ||
protected abstract Properties getAvaticaConnectionPropertiesFailure(); | ||
|
||
@Test | ||
public void test_systemSchemaAccess_admin() throws Exception | ||
|
@@ -444,27 +455,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"); | ||
testAvaticaAuthFailure(properties, getRouterAvacticaUrl()); | ||
} | ||
|
||
@Test | ||
public void test_avaticaQueryWithContext_datasourceAndContextParamsUser_succeed() throws Exception | ||
{ | ||
final Properties properties = getAvaticaConnectionPropertiesForUser("datasourceAndContextParamsUser", "helloworld"); | ||
properties.setProperty("auth_test_ctx", "should-be-allowed"); | ||
testAvaticaAuthFailure(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( | ||
datasourceOnlyUserClient, | ||
query, | ||
ImmutableMap.of("auth_test_ctx", "should-be-allowed"), | ||
HttpResponseStatus.OK | ||
); | ||
} | ||
|
||
@Test | ||
|
@@ -501,6 +556,7 @@ protected void setupHttpClientsAndUsers() throws Exception | |
{ | ||
setupHttpClients(); | ||
setupDatasourceOnlyUser(); | ||
setupDatasourceAndContextParamsUser(); | ||
setupDatasourceAndSysTableUser(); | ||
setupDatasourceAndSysAndStateUser(); | ||
setupSysTableAndStateOnlyUser(); | ||
|
@@ -538,11 +594,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); | ||
|
@@ -557,11 +630,10 @@ protected void testAvaticaQuery(String url) | |
} | ||
} | ||
|
||
protected void testAvaticaAuthFailure(String url) throws Exception | ||
protected void testAvaticaAuthFailure(Properties connectionProperties, String url) throws Exception | ||
{ | ||
LOG.info("URL: " + url); | ||
try { | ||
Properties connectionProperties = getAvaticaConnectionPropertiesFailure(); | ||
Connection connection = DriverManager.getConnection(url, connectionProperties); | ||
Statement statement = connection.createStatement(); | ||
statement.setMaxRows(450); | ||
|
@@ -612,9 +684,20 @@ protected StatusResponseHolder makeSQLQueryRequest( | |
String query, | ||
HttpResponseStatus expectedStatus | ||
) throws Exception | ||
{ | ||
return makeSQLQueryRequest(httpClient, query, ImmutableMap.of(), expectedStatus); | ||
} | ||
|
||
protected StatusResponseHolder makeSQLQueryRequest( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Maybe rename to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -758,7 +841,6 @@ protected void setupHttpClients() throws Exception | |
setupTestSpecificHttpClients(); | ||
} | ||
|
||
|
||
protected void setupCommonHttpClients() | ||
{ | ||
adminClient = new CredentialedHttpClient( | ||
|
@@ -771,6 +853,11 @@ protected void setupCommonHttpClients() | |
httpClient | ||
); | ||
|
||
datasourceAndContextParamsClient = new CredentialedHttpClient( | ||
new BasicCredentials("datasourceAndContextParamsUser", "helloworld"), | ||
httpClient | ||
); | ||
|
||
datasourceAndSysUserClient = new CredentialedHttpClient( | ||
new BasicCredentials("datasourceAndSysUser", "helloworld"), | ||
httpClient | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.