From 0cfc4170279a4dbe1e2b572e3cda51409b47960b Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Thu, 13 Aug 2026 14:24:07 +0200 Subject: [PATCH] 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) --- .../karaf-kar/src/main/feature/feature.xml | 2 + extensions/groovy-actions/rest/pom.xml | 10 ++ .../actions/rest/GroovyActionsEndPoint.java | 5 + .../rest/GroovyActionsEndPointRoleTest.java | 37 ++++++ .../impl/GroovyActionsServiceImpl.java | 23 +++- .../impl/GroovyActionsServiceImplTest.java | 49 ++++++++ .../cxs/actions/fieldInitializerAction.groovy | 27 +++++ .../java/org/apache/unomi/itests/AllITs.java | 1 + .../unomi/itests/CorePersistenceITs.java | 1 + .../GroovyActionsEndpointRoleSecurityIT.java | 113 ++++++++++++++++++ 10 files changed, 265 insertions(+), 3 deletions(-) create mode 100644 extensions/groovy-actions/rest/src/test/java/org/apache/unomi/groovy/actions/rest/GroovyActionsEndPointRoleTest.java create mode 100644 extensions/groovy-actions/services/src/test/resources/META-INF/cxs/actions/fieldInitializerAction.groovy create mode 100644 itests/src/test/java/org/apache/unomi/itests/GroovyActionsEndpointRoleSecurityIT.java 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 5d527b1e85..c16201eda6 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 @@
${project.description}
wrap unomi-services + + unomi-rest-api mvn:org.apache.unomi/unomi-groovy-actions-services/${project.version}/cfg/groovyactionscfg mvn:org.apache.unomi/unomi-groovy-actions-services/${project.version} mvn:org.apache.unomi/unomi-groovy-actions-rest/${project.version} diff --git a/extensions/groovy-actions/rest/pom.xml b/extensions/groovy-actions/rest/pom.xml index 3bedcc985e..b6aef5cd37 100644 --- a/extensions/groovy-actions/rest/pom.xml +++ b/extensions/groovy-actions/rest/pom.xml @@ -57,6 +57,11 @@ unomi-groovy-actions-services provided + + org.apache.unomi + unomi-rest + provided + org.osgi @@ -106,6 +111,11 @@ slf4j-api provided + + org.junit.jupiter + junit-jupiter + test + 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 1f41cf6f3e..635dfe3d45 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.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 @@ @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 void setGroovyActionsService(GroovyActionsService groovyActionsService) { * Uploads a Groovy action script and registers a matching action type. *

* 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 Response save(@Multipart(value = "file") Attachment file) { /** * 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 0000000000..92ffea28d4 --- /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 40c02a7aeb..09307e0380 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 @@ private GroovyAction processGroovyScript(BundleContext bundleContext, URL url, I // 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 @@ private void processGroovyActionForCache(GroovyAction groovyAction) { 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 @@ private void validateNotEmpty(String value, String parameterName) { } } + /** * Thread-safe script compilation using synchronized shared shell. */ private Class 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. + *

+ * Deliberately {@code parseClass} and not {@code GroovyShell#parse}: {@code parse} returns a + * {@code Script} instance, 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 compileToClass(GroovyCodeSource codeSource) { + return (Class) 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 94536a296f..557ff06499 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 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 @@ */ 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 void testSaveGroovyAction() throws Exception { }); } + /** + * 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. + *

+ * 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 0000000000..530b36e4f2 --- /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 41351e5b02..973b6d3135 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 @@ 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 6cc692a0d1..99483bdf37 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 @@ 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 0000000000..5802faf26b --- /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"; + } +}