This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch UNOMI-976-groovy-action-authz in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 0cfc4170279a4dbe1e2b572e3cda51409b47960b Author: Serge Huber <[email protected]> AuthorDate: Thu Aug 13 14:24:07 2026 +0200 UNOMI-976: Restrict Groovy action upload to system administrators Uploading a Groovy action puts code into the server process. That is a host-level operation rather than something confined to one tenant's data plane, so GroovyActionsEndPoint now requires UnomiRoles.ADMINISTRATOR on every path, including the multipart upload. GroovyActionsServiceImpl also compiles through GroovyClassLoader.parseClass(codeSource, false) rather than GroovyShell.parse. The distinction matters because GroovyShell.parse instantiates the script, and instantiating a Groovy script is what evaluates its @Field initializers. Saving an action should compile it and nothing more; evaluating any part of a script's body belongs to dispatch, not to storage. The emitted bytecode is otherwise identical, which was checked rather than assumed. testSaveCompilesWithoutInstantiating carries its own positive control: the same script is first run through a plain GroovyShell and must set the marker. Without that step a probe that silently failed to set it would make the real assertion pass while proving nothing. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../karaf-kar/src/main/feature/feature.xml | 2 + extensions/groovy-actions/rest/pom.xml | 10 ++ .../groovy/actions/rest/GroovyActionsEndPoint.java | 5 + .../rest/GroovyActionsEndPointRoleTest.java | 37 +++++++ .../services/impl/GroovyActionsServiceImpl.java | 23 ++++- .../impl/GroovyActionsServiceImplTest.java | 49 +++++++++ .../cxs/actions/fieldInitializerAction.groovy | 27 +++++ .../test/java/org/apache/unomi/itests/AllITs.java | 1 + .../apache/unomi/itests/CorePersistenceITs.java | 1 + .../GroovyActionsEndpointRoleSecurityIT.java | 113 +++++++++++++++++++++ 10 files changed, 265 insertions(+), 3 deletions(-) diff --git a/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml b/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml index 5d527b1e8..c16201eda 100644 --- a/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml +++ b/extensions/groovy-actions/karaf-kar/src/main/feature/feature.xml @@ -20,6 +20,8 @@ <details>${project.description}</details> <feature>wrap</feature> <feature>unomi-services</feature> + <!-- REST endpoints use @RequiresRole from org.apache.unomi.rest.security --> + <feature>unomi-rest-api</feature> <configfile finalname="/etc/org.apache.unomi.groovy.actions.cfg">mvn:org.apache.unomi/unomi-groovy-actions-services/${project.version}/cfg/groovyactionscfg</configfile> <bundle start="false">mvn:org.apache.unomi/unomi-groovy-actions-services/${project.version}</bundle> <bundle start="false">mvn:org.apache.unomi/unomi-groovy-actions-rest/${project.version}</bundle> diff --git a/extensions/groovy-actions/rest/pom.xml b/extensions/groovy-actions/rest/pom.xml index 3bedcc985..b6aef5cd3 100644 --- a/extensions/groovy-actions/rest/pom.xml +++ b/extensions/groovy-actions/rest/pom.xml @@ -57,6 +57,11 @@ <artifactId>unomi-groovy-actions-services</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>org.apache.unomi</groupId> + <artifactId>unomi-rest</artifactId> + <scope>provided</scope> + </dependency> <dependency> <groupId>org.osgi</groupId> @@ -106,6 +111,11 @@ <artifactId>slf4j-api</artifactId> <scope>provided</scope> </dependency> + <dependency> + <groupId>org.junit.jupiter</groupId> + <artifactId>junit-jupiter</artifactId> + <scope>test</scope> + </dependency> </dependencies> <build> <plugins> diff --git a/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java b/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java index 1f41cf6f3..635dfe3d4 100644 --- a/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java +++ b/extensions/groovy-actions/rest/src/main/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPoint.java @@ -21,7 +21,9 @@ import org.apache.commons.io.IOUtils; import org.apache.cxf.jaxrs.ext.multipart.Attachment; import org.apache.cxf.jaxrs.ext.multipart.Multipart; import org.apache.cxf.rs.security.cors.CrossOriginResourceSharing; +import org.apache.unomi.api.security.UnomiRoles; import org.apache.unomi.groovy.actions.services.GroovyActionsService; +import org.apache.unomi.rest.security.RequiresRole; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; import org.slf4j.Logger; @@ -35,6 +37,7 @@ import java.io.IOException; @Produces(MediaType.APPLICATION_JSON + ";charset=UTF-8") @CrossOriginResourceSharing(allowAllOrigins = true, allowCredentials = true) @Path("/groovyActions") +@RequiresRole(UnomiRoles.ADMINISTRATOR) @Component(service = GroovyActionsEndPoint.class, property = "osgi.jaxrs.resource=true") public class GroovyActionsEndPoint { @@ -55,6 +58,7 @@ public class GroovyActionsEndPoint { * Uploads a Groovy action script and registers a matching action type. * <p> * The multipart field {@code file} must be a {@code .groovy} file; the action id is derived from the filename. + * Restricted to system administrators (JAAS); tenant administrators cannot upload scripts. * * @param file the Groovy script upload * @return an empty success response @@ -79,6 +83,7 @@ public class GroovyActionsEndPoint { /** * Deletes the Groovy action and its action type entry. + * Restricted to system administrators (JAAS). * * @param actionId the action identifier * @api.status 204 empty Action deleted. diff --git a/extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java b/extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java new file mode 100644 index 000000000..92ffea28d --- /dev/null +++ b/extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java @@ -0,0 +1,37 @@ +/* + * 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.unomi.groovy.actions.rest; + +import org.apache.unomi.api.security.UnomiRoles; +import org.apache.unomi.rest.security.RequiresRole; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Regression: Groovy upload/delete must stay restricted to system administrators. + */ +class GroovyActionsEndPointRoleTest { + + @Test + void endpointRequiresSystemAdministratorRole() { + RequiresRole requiresRole = GroovyActionsEndPoint.class.getAnnotation(RequiresRole.class); + assertNotNull(requiresRole, "GroovyActionsEndPoint must declare @RequiresRole"); + assertArrayEquals(new String[]{UnomiRoles.ADMINISTRATOR}, requiresRole.value()); + } +} diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index 40c02a7ae..09307e038 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -301,7 +301,7 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im // Extract Action annotation and register the ActionType try { synchronized(compilationLock) { - Action actionAnnotation = compilationShell.parse(groovyCodeSource).getClass().getMethod("execute").getAnnotation(Action.class); + Action actionAnnotation = compileToClass(groovyCodeSource).getMethod("execute").getAnnotation(Action.class); if (actionAnnotation != null) { contextManager.executeAsSystem(() -> { saveActionType(actionAnnotation); @@ -407,7 +407,7 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im try { GroovyCodeSource groovyCodeSource = new GroovyCodeSource(script, actionName, "/groovy/script"); synchronized(compilationLock) { - compilationShell.parse(groovyCodeSource).getClass().getMethod("execute"); + compileToClass(groovyCodeSource).getMethod("execute"); } // Note: We don't extract or save the ActionType here } catch (NoSuchMethodException e) { @@ -471,16 +471,33 @@ public class GroovyActionsServiceImpl extends AbstractMultiTypeCachingService im } } + /** * Thread-safe script compilation using synchronized shared shell. */ private Class<? extends Script> compileScript(String actionName, String scriptContent) { GroovyCodeSource codeSource = new GroovyCodeSource(scriptContent, actionName, "/groovy/script"); synchronized(compilationLock) { - return compilationShell.parse(codeSource).getClass(); + return compileToClass(codeSource); } } + + /** + * Compiles a script to its Class without instantiating it. + * <p> + * Deliberately {@code parseClass} and not {@code GroovyShell#parse}: {@code parse} returns a + * {@code Script} <em>instance</em>, and constructing that instance runs the script's field + * initializers. An uploaded script carrying a Groovy {@code @Field} initializer would therefore + * execute at upload/compile time, before any rule ever dispatches it. Every caller here only + * needs the compiled Class (to read the {@code @Action} annotation or check for {@code execute}), + * so nothing needs to be instantiated until the action is actually run. + */ + @SuppressWarnings("unchecked") + private Class<? extends Script> compileToClass(GroovyCodeSource codeSource) { + return (Class<? extends Script>) compilationShell.getClassLoader().parseClass(codeSource, false); + } + /** * Compiles a script and creates metadata with timing information. */ diff --git a/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java b/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java index 94536a296..557ff0649 100644 --- a/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java +++ b/extensions/groovy-actions/services/src/test/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImplTest.java @@ -16,6 +16,7 @@ */ package org.apache.unomi.groovy.actions.services.impl; +import groovy.lang.GroovyShell; import groovy.lang.Script; import org.apache.unomi.api.Event; import org.apache.unomi.api.ExecutionContext; @@ -47,6 +48,7 @@ import org.osgi.framework.wiring.BundleWiring; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Paths; import java.util.*; @@ -59,6 +61,8 @@ import static org.mockito.Mockito.when; */ public class GroovyActionsServiceImplTest { + private static final String FIELD_INITIALIZER_MARKER = "unomi.test.groovyFieldInitializerRan"; + private GroovyActionsServiceImpl groovyActionsService; private TenantService tenantService; private PersistenceService persistenceService; @@ -172,6 +176,51 @@ public class GroovyActionsServiceImplTest { }); } + /** + * Saving an action must compile it without instantiating it. A Groovy {@code @Field} initializer + * runs at instantiation, so an implementation that instantiates while saving would run whatever + * the script's author put in that initializer at save time, before any rule dispatches the action. + * <p> + * The positive control matters as much as the assertion. The same script is first run through a + * plain {@link GroovyShell}, which must set the marker; without that step a script that silently + * failed to set it would make the real assertion pass while proving nothing. + */ + @Test + public void testSaveCompilesWithoutInstantiating() throws Exception { + String groovyScript = loadGroovyScript( + "/META-INF/cxs/actions/fieldInitializerAction.groovy", + "Could not find the field-initializer test Groovy action file"); + System.clearProperty(FIELD_INITIALIZER_MARKER); + try { + // Positive control: instantiating the script does run the @Field initializer. + new GroovyShell().parse(stripActionAnnotation(groovyScript)); + assertEquals("positive control failed: the @Field initializer did not run even via " + + "GroovyShell#parse, so the assertion below would prove nothing", + "true", System.getProperty(FIELD_INITIALIZER_MARKER)); + System.clearProperty(FIELD_INITIALIZER_MARKER); + + // The assertion: saving the very same script must not instantiate it. + contextManager.executeAsTenant(TENANT_1, () -> { + groovyActionsService.save("fieldInitializerAction", groovyScript); + }); + + assertNull("Saving a Groovy action must compile it without instantiating it, so a @Field " + + "initializer must not run at save time", + System.getProperty(FIELD_INITIALIZER_MARKER)); + } finally { + System.clearProperty(FIELD_INITIALIZER_MARKER); + } + } + + /** + * Drops the {@code @Action} line so the positive control compiles under a bare {@link GroovyShell}, + * which has neither the service's ImportCustomizer nor its script base class. The {@code @Field} + * initializer - the only part under test - is untouched. + */ + private static String stripActionAnnotation(String script) { + return script.replaceAll("(?m)^@Action\\(.*\\)$", ""); + } + @Test public void testRemoveGroovyAction() throws Exception { // First save an action diff --git a/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy b/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy new file mode 100644 index 000000000..530b36e4f --- /dev/null +++ b/extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy @@ -0,0 +1,27 @@ +/* + * 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 groovy.transform.Field + +// A Groovy @Field initializer runs when the script class is *instantiated*, not when it is +// compiled. Saving this action must compile it without instantiating it, so this marker must not +// be set afterwards. Setting a system property keeps the probe inert. +@Field def sideEffect = { System.setProperty("unomi.test.groovyFieldInitializerRan", "true") }() + +@Action(id = "fieldInitializerAction", actionExecutor = "groovy:fieldInitializerAction") +def execute() { + return EventService.NO_CHANGE +} diff --git a/itests/src/test/java/org/apache/unomi/itests/AllITs.java b/itests/src/test/java/org/apache/unomi/itests/AllITs.java index 41351e5b0..973b6d313 100644 --- a/itests/src/test/java/org/apache/unomi/itests/AllITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/AllITs.java @@ -60,6 +60,7 @@ import org.junit.runners.Suite.SuiteClasses; RuleServiceIT.class, PrivacyServiceIT.class, GroovyActionsServiceIT.class, + GroovyActionsEndpointRoleSecurityIT.class, GraphQLEventIT.class, GraphQLListIT.class, GraphQLProfileIT.class, diff --git a/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java b/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java index 6cc692a0d..99483bdf3 100644 --- a/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/CorePersistenceITs.java @@ -61,6 +61,7 @@ import org.junit.runners.Suite.SuiteClasses; RuleServiceIT.class, PrivacyServiceIT.class, GroovyActionsServiceIT.class, + GroovyActionsEndpointRoleSecurityIT.class, GraphQLEventIT.class, GraphQLListIT.class, GraphQLProfileIT.class, diff --git a/itests/src/test/java/org/apache/unomi/itests/GroovyActionsEndpointRoleSecurityIT.java b/itests/src/test/java/org/apache/unomi/itests/GroovyActionsEndpointRoleSecurityIT.java new file mode 100644 index 000000000..5802faf26 --- /dev/null +++ b/itests/src/test/java/org/apache/unomi/itests/GroovyActionsEndpointRoleSecurityIT.java @@ -0,0 +1,113 @@ +/* + * 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.unomi.itests; + +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpDelete; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.entity.ByteArrayEntity; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.ops4j.pax.exam.junit.PaxExam; +import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; +import org.ops4j.pax.exam.spi.reactors.PerSuite; + +import java.nio.charset.StandardCharsets; + +/** + * HTTP-level checks that system-admin-only REST endpoints reject tenant private keys + * (including multipart upload / oneshot paths). + */ +@RunWith(PaxExam.class) +@ExamReactorStrategy(PerSuite.class) +public class GroovyActionsEndpointRoleSecurityIT extends BaseIT { + + + + + + @Test + public void groovyActions_requiresSystemAdministrator() throws Exception { + String path = getFullUrl("/cxs/groovyActions/rest-role-security-it-missing-action"); + + try (CloseableHttpResponse tenantAdmin = executeHttpRequest(new HttpDelete(path), AuthType.PRIVATE_KEY)) { + Assert.assertEquals("Tenant private key must not delete groovy actions", + 403, tenantAdmin.getStatusLine().getStatusCode()); + } + + try (CloseableHttpResponse jaasAdmin = executeHttpRequest(new HttpDelete(path), AuthType.JAAS_ADMIN)) { + int status = jaasAdmin.getStatusLine().getStatusCode(); + Assert.assertTrue("JAAS admin delete should be allowed (got " + status + ")", + status == 200 || status == 204 || status == 404); + } + } + + @Test + public void groovyActions_upload_requiresSystemAdministrator() throws Exception { + String script = "// GroovyActionsEndpointRoleSecurityIT probe\nvoid execute() {}\n"; + HttpPost upload = multipartPost(getFullUrl("/cxs/groovyActions/"), + "----UnomiGroovyBoundary", + filePart("file", "RestRoleSecurityITProbe.groovy", "text/plain", script)); + + try (CloseableHttpResponse tenantAdmin = executeHttpRequest(upload, AuthType.PRIVATE_KEY)) { + Assert.assertEquals("Tenant private key must not upload groovy actions", + 403, tenantAdmin.getStatusLine().getStatusCode()); + } + + HttpPost uploadJaas = multipartPost(getFullUrl("/cxs/groovyActions/"), + "----UnomiGroovyBoundaryJaas", + filePart("file", "RestRoleSecurityITProbe.groovy", "text/plain", script)); + try (CloseableHttpResponse jaasAdmin = executeHttpRequest(uploadJaas, AuthType.JAAS_ADMIN)) { + Assert.assertEquals("JAAS admin should be allowed to upload groovy actions", + 200, jaasAdmin.getStatusLine().getStatusCode()); + } + + try (CloseableHttpResponse cleanup = executeHttpRequest( + new HttpDelete(getFullUrl("/cxs/groovyActions/RestRoleSecurityITProbe")), AuthType.JAAS_ADMIN)) { + int status = cleanup.getStatusLine().getStatusCode(); + Assert.assertTrue(status == 200 || status == 204 || status == 404); + } + } + + private static HttpPost multipartPost(String url, String boundary, String... parts) { + HttpPost post = new HttpPost(url); + StringBuilder body = new StringBuilder(); + for (String part : parts) { + body.append("--").append(boundary).append("\r\n").append(part); + } + body.append("--").append(boundary).append("--\r\n"); + post.setHeader("Content-Type", "multipart/form-data; boundary=" + boundary); + post.setEntity(new ByteArrayEntity(body.toString().getBytes(StandardCharsets.UTF_8))); + return post; + } + + private static String part(String name, String contentType, String value) { + return "Content-Disposition: form-data; name=\"" + name + "\"\r\n" + + "Content-Type: " + contentType + "\r\n\r\n" + + value + "\r\n"; + } + + private static String filePart(String name, String filename, String contentType, String value) { + return "Content-Disposition: form-data; name=\"" + name + "\"; filename=\"" + filename + "\"\r\n" + + "Content-Type: " + contentType + "\r\n\r\n" + + value + "\r\n"; + } +}
