* 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"; + } +}