Skip to content

Register event listeners for default system access control #11632

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

Merged
kokosing merged 2 commits into from
Mar 23, 2022
Merged

Register event listeners for default system access control #11632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8bde31
Rename setSystemAccessControl to more apt loadSystemAccessControl
gaurav8297 Mar 23, 2022
eb0be1d
Register event listeners for default system access control
gaurav8297 Mar 23, 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: 5 additions & 2 deletions core/trino-main/src/main/java/io/trino/security/AccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public void loadSystemAccessControl()
List<File> configFiles = this.configFiles;
if (configFiles.isEmpty()) {
if (!CONFIG_FILE.exists()) {
setSystemAccessControl(defaultAccessControlName, ImmutableMap.of());
loadSystemAccessControl(defaultAccessControlName, ImmutableMap.of());
log.info("Using system access control: %s", defaultAccessControlName);
return;
}
Expand Down Expand Up @@ -187,7 +187,7 @@ private SystemAccessControl createSystemAccessControl(File configFile)
}

@VisibleForTesting
protected void setSystemAccessControl(String name, Map<String, String> properties)
public void loadSystemAccessControl(String name, Map<String, String> properties)
Comment on lines 189 to +190
Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Mar 23, 2022

Choose a reason for hiding this comment

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

If it's public, does the @VisibleForTesting still make sense?

Copy link
Copy Markdown
Member Author

@gaurav8297 gaurav8297 Mar 23, 2022

Choose a reason for hiding this comment

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

I think so. It's being used only in tests other than this class itself

{
requireNonNull(name, "name is null");
requireNonNull(properties, "properties is null");
Expand All @@ -200,6 +200,9 @@ protected void setSystemAccessControl(String name, Map<String, String> propertie
systemAccessControl = factory.create(ImmutableMap.copyOf(properties));
}

systemAccessControl.getEventListeners()
.forEach(eventListenerManager::addEventListener);
Comment on lines +203 to +204
Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Mar 23, 2022

Choose a reason for hiding this comment

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

I would move it to the no-argument loadSystemAccessControl(), mostly because this is also where the even listeners are registered for the non-default access controls. This ways it's clearer why this has to be done.

But then you'll have to change TestingAccessControlManager to call loadSystemAccessControl() instead (and orchestrate all the configuration set-up I presume). Do you think this is feasible?

Copy link
Copy Markdown
Member Author

@gaurav8297 gaurav8297 Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But then we would need to move out factory.create(..) from setSystemAccessControl(String name, Map<String, String> properties) to loadSystemAccessControl() because without initializing systemAccessControl, we can't register event listeners.

Also in a way setSystemAccessControl(String name...) is loading system access control from the name so I think it's fine

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Mar 23, 2022

Choose a reason for hiding this comment

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

OK then. This was my thought from looking around the code, as I wanted to understand the context of this change.


setSystemAccessControls(ImmutableList.of(systemAccessControl));
}

Expand Down
5 changes: 0 additions & 5 deletions core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,6 @@ public TestingAccessControlManager(TransactionManager transactionManager, EventL
this(transactionManager, eventListenerManager, new AccessControlConfig());
}

public void loadSystemAccessControl(String name, Map<String, String> properties)
{
setSystemAccessControl(name, properties);
}

public static TestingPrivilege privilege(String entityName, TestingPrivilegeType type)
{
return new TestingPrivilege(Optional.empty(), entityName, type);
Expand Down
68 changes: 56 additions & 12 deletions core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public void testInitializing()
public void testNoneSystemAccessControl()
{
AccessControlManager accessControlManager = createAccessControlManager(createTestTransactionManager());
accessControlManager.setSystemAccessControl(AllowAllSystemAccessControl.NAME, ImmutableMap.of());
accessControlManager.loadSystemAccessControl(AllowAllSystemAccessControl.NAME, ImmutableMap.of());
accessControlManager.checkCanSetUser(Optional.empty(), USER_NAME);
}

Expand All @@ -102,7 +102,7 @@ public void testReadOnlySystemAccessControl()
TransactionManager transactionManager = createTestTransactionManager();
AccessControlManager accessControlManager = createAccessControlManager(transactionManager);

accessControlManager.setSystemAccessControl(ReadOnlySystemAccessControl.NAME, ImmutableMap.of());
accessControlManager.loadSystemAccessControl(ReadOnlySystemAccessControl.NAME, ImmutableMap.of());
accessControlManager.checkCanSetUser(Optional.of(PRINCIPAL), USER_NAME);
accessControlManager.checkCanSetSystemSessionProperty(identity, "property");

Expand Down Expand Up @@ -139,7 +139,7 @@ public void testSetAccessControl()

TestSystemAccessControlFactory accessControlFactory = new TestSystemAccessControlFactory("test");
accessControlManager.addSystemAccessControlFactory(accessControlFactory);
accessControlManager.setSystemAccessControl("test", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("test", ImmutableMap.of());

accessControlManager.checkCanSetUser(Optional.of(PRINCIPAL), USER_NAME);
assertEquals(accessControlFactory.getCheckedUserName(), USER_NAME);
Expand All @@ -154,7 +154,7 @@ public void testNoCatalogAccessControl()

TestSystemAccessControlFactory accessControlFactory = new TestSystemAccessControlFactory("test");
accessControlManager.addSystemAccessControlFactory(accessControlFactory);
accessControlManager.setSystemAccessControl("test", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("test", ImmutableMap.of());

transaction(transactionManager, accessControlManager)
.execute(transactionId -> {
Expand All @@ -171,7 +171,7 @@ public void testDenyCatalogAccessControl()

TestSystemAccessControlFactory accessControlFactory = new TestSystemAccessControlFactory("test");
accessControlManager.addSystemAccessControlFactory(accessControlFactory);
accessControlManager.setSystemAccessControl("test", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("test", ImmutableMap.of());

queryRunner.createCatalog("catalog", MockConnectorFactory.create(), ImmutableMap.of());
accessControlManager.addCatalogAccessControl(new CatalogName("catalog"), new DenyConnectorAccessControl());
Expand Down Expand Up @@ -218,7 +218,7 @@ public void checkCanSetSystemSessionProperty(SystemSecurityContext context, Stri
};
}
});
accessControlManager.setSystemAccessControl("test", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("test", ImmutableMap.of());

queryRunner.createCatalog("catalog", MockConnectorFactory.create(), ImmutableMap.of());
accessControlManager.addCatalogAccessControl(new CatalogName("catalog"), new ConnectorAccessControl()
Expand Down Expand Up @@ -263,7 +263,7 @@ public void testDenySystemAccessControl()

TestSystemAccessControlFactory accessControlFactory = new TestSystemAccessControlFactory("test");
accessControlManager.addSystemAccessControlFactory(accessControlFactory);
accessControlManager.setSystemAccessControl("test", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("test", ImmutableMap.of());

queryRunner.createCatalog("catalog", MockConnectorFactory.create(), ImmutableMap.of());
accessControlManager.addCatalogAccessControl(new CatalogName("connector"), new DenyConnectorAccessControl());
Expand All @@ -289,7 +289,7 @@ public void testDenyExecuteProcedureBySystem()

TestSystemAccessControlFactory accessControlFactory = new TestSystemAccessControlFactory("deny-all");
accessControlManager.addSystemAccessControlFactory(accessControlFactory);
accessControlManager.setSystemAccessControl("deny-all", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("deny-all", ImmutableMap.of());

assertDenyExecuteProcedure(transactionManager, accessControlManager, "Access Denied: Cannot execute procedure connector.schema.procedure");
}
Expand All @@ -300,7 +300,7 @@ public void testDenyExecuteProcedureByConnector()
try (LocalQueryRunner queryRunner = LocalQueryRunner.create(TEST_SESSION)) {
TransactionManager transactionManager = queryRunner.getTransactionManager();
AccessControlManager accessControlManager = createAccessControlManager(transactionManager);
accessControlManager.setSystemAccessControl("allow-all", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("allow-all", ImmutableMap.of());

queryRunner.createCatalog("connector", MockConnectorFactory.create(), ImmutableMap.of());
accessControlManager.addCatalogAccessControl(new CatalogName("connector"), new DenyConnectorAccessControl());
Expand All @@ -315,7 +315,7 @@ public void testAllowExecuteProcedure()
try (LocalQueryRunner queryRunner = LocalQueryRunner.create(TEST_SESSION)) {
TransactionManager transactionManager = queryRunner.getTransactionManager();
AccessControlManager accessControlManager = createAccessControlManager(transactionManager);
accessControlManager.setSystemAccessControl("allow-all", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("allow-all", ImmutableMap.of());

queryRunner.createCatalog("connector", MockConnectorFactory.create(), ImmutableMap.of());
accessControlManager.addCatalogAccessControl(new CatalogName("connector"), new AllowAllAccessControl());
Expand All @@ -327,6 +327,45 @@ public void testAllowExecuteProcedure()
}
}

@Test
public void testRegisterSingleEventListenerForDefaultAccessControl()
{
EventListener expectedListener = new EventListener() {};

String defaultAccessControlName = "event-listening-default-access-control";
TestingEventListenerManager eventListenerManager = emptyEventListenerManager();
AccessControlManager accessControlManager = createAccessControlManager(
eventListenerManager,
defaultAccessControlName);
accessControlManager.addSystemAccessControlFactory(
eventListeningSystemAccessControlFactory(defaultAccessControlName, expectedListener));

accessControlManager.loadSystemAccessControl();

assertThat(eventListenerManager.getConfiguredEventListeners())
.contains(expectedListener);
}

@Test
public void testRegisterMultipleEventListenerForDefaultAccessControl()
{
EventListener firstListener = new EventListener() {};
EventListener secondListener = new EventListener() {};

String defaultAccessControlName = "event-listening-default-access-control";
TestingEventListenerManager eventListenerManager = emptyEventListenerManager();
AccessControlManager accessControlManager = createAccessControlManager(
eventListenerManager,
defaultAccessControlName);
accessControlManager.addSystemAccessControlFactory(
eventListeningSystemAccessControlFactory(defaultAccessControlName, firstListener, secondListener));

accessControlManager.loadSystemAccessControl();

assertThat(eventListenerManager.getConfiguredEventListeners())
.contains(firstListener, secondListener);
}

@Test
public void testRegisterSingleEventListener()
throws IOException
Expand Down Expand Up @@ -384,7 +423,7 @@ public void testDenyExecuteFunctionBySystemAccessControl()

TestSystemAccessControlFactory accessControlFactory = new TestSystemAccessControlFactory("deny-all");
accessControlManager.addSystemAccessControlFactory(accessControlFactory);
accessControlManager.setSystemAccessControl("deny-all", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("deny-all", ImmutableMap.of());

transaction(transactionManager, accessControlManager)
.execute(transactionId -> {
Expand All @@ -403,7 +442,7 @@ public void testAllowExecuteFunction()
CatalogManager catalogManager = new CatalogManager();
TransactionManager transactionManager = createTestTransactionManager(catalogManager);
AccessControlManager accessControlManager = createAccessControlManager(transactionManager);
accessControlManager.setSystemAccessControl("allow-all", ImmutableMap.of());
accessControlManager.loadSystemAccessControl("allow-all", ImmutableMap.of());

transaction(transactionManager, accessControlManager)
.execute(transactionId -> {
Expand Down Expand Up @@ -436,6 +475,11 @@ private AccessControlManager createAccessControlManager(EventListenerManager eve
return new AccessControlManager(createTestTransactionManager(), eventListenerManager, config, DefaultSystemAccessControl.NAME);
}

private AccessControlManager createAccessControlManager(EventListenerManager eventListenerManager, String defaultAccessControlName)
{
return new AccessControlManager(createTestTransactionManager(), eventListenerManager, new AccessControlConfig(), defaultAccessControlName);
}

private SystemAccessControlFactory eventListeningSystemAccessControlFactory(String name, EventListener... eventListeners)
{
return new SystemAccessControlFactory()
Expand Down
6 changes: 3 additions & 3 deletions core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public void testDocsExample()
{
TransactionManager transactionManager = createTestTransactionManager();
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);
accessControlManager.setSystemAccessControl(
accessControlManager.loadSystemAccessControl(
FileBasedSystemAccessControl.NAME,
ImmutableMap.of("security.config-file", new File("../../docs/src/main/sphinx/security/user-impersonation.json").getAbsolutePath()));

Expand Down Expand Up @@ -785,7 +785,7 @@ public void testRefreshing()
configFile.deleteOnExit();
copy(new File(getResourcePath("catalog.json")), configFile);

accessControlManager.setSystemAccessControl(FileBasedSystemAccessControl.NAME, ImmutableMap.of(
accessControlManager.loadSystemAccessControl(FileBasedSystemAccessControl.NAME, ImmutableMap.of(
SECURITY_CONFIG_FILE, configFile.getAbsolutePath(),
SECURITY_REFRESH_PERIOD, "1ms"));

Expand Down Expand Up @@ -842,7 +842,7 @@ private AccessControlManager newAccessControlManager(TransactionManager transact
{
AccessControlManager accessControlManager = new AccessControlManager(transactionManager, emptyEventListenerManager(), new AccessControlConfig(), DefaultSystemAccessControl.NAME);

accessControlManager.setSystemAccessControl(FileBasedSystemAccessControl.NAME, ImmutableMap.of("security.config-file", getResourcePath(resourceName)));
accessControlManager.loadSystemAccessControl(FileBasedSystemAccessControl.NAME, ImmutableMap.of("security.config-file", getResourcePath(resourceName)));

return accessControlManager;
}
Expand Down