From ac4a1643e1bd55565288a6c7ffefd232607c46f7 Mon Sep 17 00:00:00 2001 From: evasques Date: Thu, 15 Jul 2021 17:45:22 +0100 Subject: [PATCH] PRODSEC-4422 - Scripts not in Data Dictionary can be executed by action (#596) * Added validation to the ScriptActionExecuter class to enforce the existing constraints on parameter script-ref (Repo has the constraint to only allow scripts in Data Dictionary / Scripts and AGS has the constraint to only allow scripts in Data Dictionary / Records Management / Records Management Scripts") by validating if the given scriptRef is in the allowed valued of the constraint set on that param * Added a new unit test for AGS to make sure that rmscript action still works as expected when the script is in the correct folder and fails when not * Added new case in ActionServiceImpl2Test#testExecuteScript to assert that the transaction fails when we execute the action with an invalid script * Moved test testActionResult from ActionServiceImplTest to class ActionServiceImpl2Test - Before it ran with a script not in Data Dictionary so with the added validation it started to fail. I moved the unit test do avoid duplicating the code to create the script in the correct location. --- .../action/ExecuteScriptActionTest.java | 171 ++++++++++++++++++ .../test/util/BaseRMTestCase.java | 3 + .../action/executer/ScriptActionExecuter.java | 24 ++- .../repo/action/ActionServiceImpl2Test.java | 113 ++++++++++-- .../repo/action/ActionServiceImplTest.java | 40 ---- 5 files changed, 294 insertions(+), 57 deletions(-) create mode 100644 amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/action/ExecuteScriptActionTest.java diff --git a/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/action/ExecuteScriptActionTest.java b/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/action/ExecuteScriptActionTest.java new file mode 100644 index 0000000000..56d7a6d593 --- /dev/null +++ b/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/action/ExecuteScriptActionTest.java @@ -0,0 +1,171 @@ +/* + * #%L + * Alfresco Records Management Module + * %% + * Copyright (C) 2005 - 2021 Alfresco Software Limited + * %% + * This file is part of the Alfresco software. + * - + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is + * provided under the following open source license terms: + * - + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * - + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * - + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + * #L% + */ + +package org.alfresco.module.org_alfresco_module_rm.test.legacy.action; + +import org.alfresco.model.ContentModel; +import org.alfresco.module.org_alfresco_module_rm.action.dm.ExecuteScriptAction; +import org.alfresco.module.org_alfresco_module_rm.test.util.BaseRMTestCase; +import org.alfresco.repo.content.MimetypeMap; +import org.alfresco.repo.security.authentication.AuthenticationUtil; +import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback; +import org.alfresco.service.cmr.action.Action; +import org.alfresco.service.cmr.repository.ContentWriter; +import org.alfresco.service.cmr.repository.NodeRef; +import org.alfresco.service.namespace.NamespaceService; +import org.alfresco.service.namespace.QName; + +/** + * RM Action Execute Script Unit test + * + * @author Eva Vasques + */ +public class ExecuteScriptActionTest extends BaseRMTestCase +{ + + @Override + protected boolean isCollaborationSiteTest() + { + return true; + } + + public void testExecuteScript() + { + + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); + String fileOriginalName = (String) nodeService.getProperty(dmDocument, ContentModel.PROP_NAME); + + // Valid Script + NodeRef validScriptRef = addTempScript("valid-rm-script.js", + "document.properties.name = \"Valid_\" + document.properties.name;\ndocument.save();"); + + // Invalid Script + NodeRef invalidScriptRef = addTempScript("invalid-rm-script.js", + "document.properties.name = \"Invalid_\" + document.properties.name;\ndocument.save();", dmFolder); + + // Attempt to execute a script not in RM Scripts folder should fail + doTestInTransaction(new FailureTest("Script outside proper Data Dictionary folder should not be executed", + IllegalStateException.class) + { + public void run() throws Exception + { + executeAction(invalidScriptRef, dmDocument); + } + }, dmCollaborator); + + // Scripts in correct data dictionary folder should be executed + doTestInTransaction(new Test() + { + @Override + public Void run() throws Exception + { + executeAction(validScriptRef, dmDocument); + return null; + } + + @Override + public void test(Void result) throws Exception + { + // Assert the script was executed and the document was renamed + String currentName = (String) nodeService.getProperty(dmDocument, ContentModel.PROP_NAME); + assertEquals(currentName, "Valid_" + fileOriginalName); + } + }, dmCollaborator); + + retryingTransactionHelper.doInTransaction(new RetryingTransactionCallback() + { + public Void execute() throws Throwable + { + // Set the name back to what it was + nodeService.setProperty(dmDocument, ContentModel.PROP_NAME, fileOriginalName); + return null; + } + }); + } + + private NodeRef addTempScript(final String scriptFileName, final String javaScript, final NodeRef parentRef) + { + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); + return retryingTransactionHelper.doInTransaction(new RetryingTransactionCallback() + { + public NodeRef execute() throws Throwable + { + + NodeRef script = nodeService.getChildByName(parentRef, ContentModel.ASSOC_CONTAINS, scriptFileName); + + if (script == null) + { + // Create the script node reference + script = nodeService.createNode(parentRef, ContentModel.ASSOC_CONTAINS, + QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, scriptFileName), ContentModel.TYPE_CONTENT) + .getChildRef(); + + nodeService.setProperty(script, ContentModel.PROP_NAME, scriptFileName); + + ContentWriter contentWriter = contentService.getWriter(script, ContentModel.PROP_CONTENT, true); + contentWriter.setMimetype(MimetypeMap.MIMETYPE_JAVASCRIPT); + contentWriter.setEncoding("UTF-8"); + contentWriter.putContent(javaScript); + + } + return script; + } + }); + } + + private NodeRef addTempScript(final String scriptFileName, final String javaScript) + { + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); + return retryingTransactionHelper.doInTransaction(new RetryingTransactionCallback() + { + public NodeRef execute() throws Throwable + { + + // get the company_home + NodeRef companyHomeRef = repositoryHelper.getCompanyHome(); + // get the Data Dictionary + NodeRef dataDictionaryRef = nodeService.getChildByName(companyHomeRef, ContentModel.ASSOC_CONTAINS, + "Data Dictionary"); + // get the Scripts Folder + NodeRef rmFolder = nodeService.getChildByName(dataDictionaryRef, ContentModel.ASSOC_CONTAINS, + "Records Management"); + NodeRef scriptsRef = nodeService.getChildByName(rmFolder, ContentModel.ASSOC_CONTAINS, + "Records Management Scripts"); + + return addTempScript(scriptFileName, javaScript, scriptsRef); + } + }); + } + + private void executeAction(NodeRef scriptRef, NodeRef nodeRef) + { + Action action = actionService.createAction("rmscript"); + action.setParameterValue(ExecuteScriptAction.PARAM_SCRIPTREF, scriptRef); + actionService.executeAction(action, nodeRef); + } + +} diff --git a/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java b/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java index 9ad32a185f..37422b6f7e 100644 --- a/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java +++ b/amps/ags/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java @@ -59,6 +59,7 @@ import org.alfresco.module.org_alfresco_module_rm.security.ExtendedSecurityServi import org.alfresco.module.org_alfresco_module_rm.security.FilePlanPermissionService; import org.alfresco.module.org_alfresco_module_rm.util.RMContainerCacheManager; import org.alfresco.module.org_alfresco_module_rm.vital.VitalRecordService; +import org.alfresco.repo.model.Repository; import org.alfresco.repo.policy.BehaviourFilter; import org.alfresco.repo.policy.PolicyComponent; import org.alfresco.repo.security.authentication.AuthenticationUtil; @@ -150,6 +151,7 @@ public abstract class BaseRMTestCase extends RetryingTransactionHelperTestCase protected OwnableService ownableService; protected VersionService versionService; protected DocumentLinkService documentLinkService; + protected Repository repositoryHelper; /** RM Services */ protected DispositionService dispositionService; @@ -405,6 +407,7 @@ public abstract class BaseRMTestCase extends RetryingTransactionHelperTestCase ownableService = (OwnableService)applicationContext.getBean("OwnableService"); versionService = (VersionService)applicationContext.getBean("VersionService"); documentLinkService = (DocumentLinkService)applicationContext.getBean("DocumentLinkService"); + repositoryHelper = (Repository)applicationContext.getBean("repositoryHelper"); // Get RM services dispositionService = (DispositionService)applicationContext.getBean("DispositionService"); diff --git a/repository/src/main/java/org/alfresco/repo/action/executer/ScriptActionExecuter.java b/repository/src/main/java/org/alfresco/repo/action/executer/ScriptActionExecuter.java index 648f966062..7c141a173d 100644 --- a/repository/src/main/java/org/alfresco/repo/action/executer/ScriptActionExecuter.java +++ b/repository/src/main/java/org/alfresco/repo/action/executer/ScriptActionExecuter.java @@ -34,7 +34,10 @@ import org.alfresco.repo.action.ParameterDefinitionImpl; import org.alfresco.repo.admin.SysAdminParams; import org.alfresco.repo.jscript.ScriptAction; import org.alfresco.service.ServiceRegistry; -import org.alfresco.service.cmr.action.Action; +import org.alfresco.service.cmr.action.Action; +import org.alfresco.service.cmr.action.ActionDefinition; +import org.alfresco.service.cmr.action.ActionService; +import org.alfresco.service.cmr.action.ParameterConstraint; import org.alfresco.service.cmr.action.ParameterDefinition; import org.alfresco.service.cmr.dictionary.DataTypeDefinition; import org.alfresco.service.cmr.repository.NodeRef; @@ -126,6 +129,10 @@ public class ScriptActionExecuter extends ActionExecuterAbstractBase if (nodeService.exists(actionedUponNodeRef)) { NodeRef scriptRef = (NodeRef)action.getParameterValue(PARAM_SCRIPTREF); + if(!isValidScriptRef(action)) + { + throw new IllegalStateException("Invalid script ref path: " + scriptRef); + } NodeRef spaceRef = this.serviceRegistry.getRuleService().getOwningNodeRef(action); if (spaceRef == null) { @@ -222,4 +229,19 @@ public class ScriptActionExecuter extends ActionExecuterAbstractBase return companyHomeRef; } + + private boolean isValidScriptRef(Action action) + { + NodeRef scriptRef = (NodeRef) action.getParameterValue(PARAM_SCRIPTREF); + ActionService actionService = this.serviceRegistry.getActionService(); + ActionDefinition actDef = actionService.getActionDefinition(action.getActionDefinitionName()); + ParameterDefinition parameterDef = actDef.getParameterDefintion(PARAM_SCRIPTREF); + String paramConstraintName = parameterDef.getParameterConstraintName(); + if (paramConstraintName != null) + { + ParameterConstraint paramConstraint = actionService.getParameterConstraint(paramConstraintName); + return paramConstraint.isValidValue(scriptRef.toString()); + } + return true; + } } diff --git a/repository/src/test/java/org/alfresco/repo/action/ActionServiceImpl2Test.java b/repository/src/test/java/org/alfresco/repo/action/ActionServiceImpl2Test.java index 9c7e3b8988..df9a8cc1aa 100644 --- a/repository/src/test/java/org/alfresco/repo/action/ActionServiceImpl2Test.java +++ b/repository/src/test/java/org/alfresco/repo/action/ActionServiceImpl2Test.java @@ -26,8 +26,8 @@ package org.alfresco.repo.action; -import static java.lang.Thread.sleep; import static junit.framework.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -39,6 +39,7 @@ import java.util.List; import java.util.Map; import org.alfresco.model.ContentModel; +import org.alfresco.repo.action.executer.ActionExecuter; import org.alfresco.repo.action.executer.ContentMetadataExtracter; import org.alfresco.repo.action.executer.CounterIncrementActionExecuter; import org.alfresco.repo.action.executer.ScriptActionExecuter; @@ -259,7 +260,7 @@ public class ActionServiceImpl2Test public void testExecuteScript() throws Exception { final NodeRef scriptToBeExecuted = addTempScript("changeFileNameTest.js", - "document.properties.name = \"Changed\" + \"_\" + document.properties.name;\ndocument.save();"); + "document.properties.name = \"Changed_\" + document.properties.name;\ndocument.save();"); assertNotNull("Failed to add the test script.", scriptToBeExecuted); // add a test file to the Site in order to change its name @@ -310,6 +311,73 @@ public class ActionServiceImpl2Test return null; } }); + + //Execute script not in Data Dictionary > Scripts + AuthenticationUtil.setFullyAuthenticatedUser(testSiteAndMemberInfo.siteManager); + NodeRef companyHomeRef = wellKnownNodes.getCompanyHome(); + NodeRef sharedFolderRef = nodeService.getChildByName(companyHomeRef, ContentModel.ASSOC_CONTAINS, + "Shared"); + final NodeRef invalidScriptRef = addTempScript("changeFileNameTest.js", + "document.properties.name = \"Invalid_Change.pdf\";\ndocument.save();",sharedFolderRef); + assertNotNull("Failed to add the test script.", scriptToBeExecuted); + transactionHelper.doInTransaction(new RetryingTransactionCallback() + { + public Void execute() throws Throwable + { + // Create the action + Action action = actionService.createAction(ScriptActionExecuter.NAME); + action.setParameterValue(ScriptActionExecuter.PARAM_SCRIPTREF, invalidScriptRef); + + try + { + // Execute the action + actionService.executeAction(action, testNode); + } + catch (Throwable th) + { + // do nothing + } + assertFalse("Scripts outside of Data Dictionary Scripts folder should not be executed", + ("Invalid_Change.pdf".equals(nodeService.getProperty(testNode, ContentModel.PROP_NAME)))); + + return null; + } + }); + } + + @Test + public void testActionResult() throws Exception + { + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); + transactionHelper.doInTransaction(new RetryingTransactionCallback() + { + public Void execute() throws Throwable + { + try + { + // Create the script node reference + NodeRef script = addTempScript("test-action-result-script.js", "\"VALUE\";"); + + // Create the action + Action action = actionService.createAction(ScriptActionExecuter.NAME); + action.setParameterValue(ScriptActionExecuter.PARAM_SCRIPTREF, script); + + // Execute the action + actionService.executeAction(action, testNode); + + // Get the result + String result = (String) action.getParameterValue(ActionExecuter.PARAM_RESULT); + assertNotNull(result); + assertEquals("VALUE", result); + } + finally + { + AuthenticationUtil.clearCurrentSecurityContext(); + } + + return null; + } + }); } @Test @@ -369,6 +437,32 @@ public class ActionServiceImpl2Test }); } + private NodeRef addTempScript(final String scriptFileName, final String javaScript, final NodeRef parentRef) + { + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); + return transactionHelper.doInTransaction(new RetryingTransactionCallback() + { + public NodeRef execute() throws Throwable + { + + // Create the script node reference + NodeRef script = nodeService.createNode(parentRef, ContentModel.ASSOC_CONTAINS, + QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, scriptFileName), + ContentModel.TYPE_CONTENT).getChildRef(); + + nodeService.setProperty(script, ContentModel.PROP_NAME, scriptFileName); + + ContentWriter contentWriter = contentService.getWriter(script, ContentModel.PROP_CONTENT, true); + contentWriter.setMimetype(MimetypeMap.MIMETYPE_JAVASCRIPT); + contentWriter.setEncoding("UTF-8"); + contentWriter.putContent(javaScript); + + tempNodes.addNodeRef(script); + return script; + } + }); + } + private NodeRef addTempScript(final String scriptFileName, final String javaScript) { AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); @@ -386,20 +480,7 @@ public class ActionServiceImpl2Test NodeRef scriptsRef = nodeService.getChildByName(dataDictionaryRef, ContentModel.ASSOC_CONTAINS, "Scripts"); - // Create the script node reference - NodeRef script = nodeService.createNode(scriptsRef, ContentModel.ASSOC_CONTAINS, - QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, scriptFileName), - ContentModel.TYPE_CONTENT).getChildRef(); - - nodeService.setProperty(script, ContentModel.PROP_NAME, scriptFileName); - - ContentWriter contentWriter = contentService.getWriter(script, ContentModel.PROP_CONTENT, true); - contentWriter.setMimetype(MimetypeMap.MIMETYPE_JAVASCRIPT); - contentWriter.setEncoding("UTF-8"); - contentWriter.putContent(javaScript); - - tempNodes.addNodeRef(script); - return script; + return addTempScript(scriptFileName, javaScript, scriptsRef); } }); } diff --git a/repository/src/test/java/org/alfresco/repo/action/ActionServiceImplTest.java b/repository/src/test/java/org/alfresco/repo/action/ActionServiceImplTest.java index 0d76dde851..f227199b14 100644 --- a/repository/src/test/java/org/alfresco/repo/action/ActionServiceImplTest.java +++ b/repository/src/test/java/org/alfresco/repo/action/ActionServiceImplTest.java @@ -805,46 +805,6 @@ public class ActionServiceImplTest extends BaseAlfrescoSpringTest assertEquals(action4, savedAction2.getAction(2)); } - /** - * Test the action result parameter - */ - @Test - public void testActionResult() - { - // We need to run this test as Administrator. The ScriptAction has to run as a full user (instead of as System) - // so that we can setup the Person object in the ScriptNode - AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); - try - { - // Create the script node reference - NodeRef script = this.nodeService.createNode( - this.folder, - ContentModel.ASSOC_CONTAINS, - QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, "testScript.js"), - ContentModel.TYPE_CONTENT).getChildRef(); - this.nodeService.setProperty(script, ContentModel.PROP_NAME, "testScript.js"); - ContentWriter contentWriter = this.contentService.getWriter(script, ContentModel.PROP_CONTENT, true); - contentWriter.setMimetype("text/plain"); - contentWriter.setEncoding("UTF-8"); - contentWriter.putContent("\"VALUE\";"); - - // Create the action - Action action1 = this.actionService.createAction(ScriptActionExecuter.NAME); - action1.setParameterValue(ScriptActionExecuter.PARAM_SCRIPTREF, script); - - // Execute the action - this.actionService.executeAction(action1, this.nodeRef); - - // Get the result - String result = (String)action1.getParameterValue(ActionExecuter.PARAM_RESULT); - assertNotNull(result); - assertEquals("VALUE", result); - } - finally - { - AuthenticationUtil.clearCurrentSecurityContext(); - } - } /** =================================================================================== * Test asynchronous actions