From 863ef2d203146ef102dfc92d3003040d491c69f2 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Thu, 12 Oct 2023 07:31:41 -0600 Subject: [PATCH] fix(core): Add a role extra layer constraint into the secrets view tool #25587 * Adding minor fixes reported via IQA feedback. * Using constants for characters * Implementing Code Review feedback. * Implementing SonarQube feedback. --- .../velocity/viewtools/SecretTool.java | 96 +++++++++++-------- .../com/dotmarketing/filters/CMSUrlUtil.java | 42 ++++++-- .../dotmarketing/filters/CMSUrlUtilTest.java | 27 +++++- 3 files changed, 114 insertions(+), 51 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/SecretTool.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/SecretTool.java index f7bbaf07c9f0..0df567956ecc 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/SecretTool.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/SecretTool.java @@ -2,25 +2,25 @@ import com.dotcms.api.web.HttpServletRequestThreadLocal; import com.dotcms.api.web.HttpServletResponseThreadLocal; +import com.dotcms.exception.ExceptionUtil; import com.dotcms.rendering.velocity.viewtools.secrets.DotVelocitySecretAppConfig; import com.dotmarketing.business.APILocator; import com.dotmarketing.business.Role; -import com.dotmarketing.business.UserAPI; import com.dotmarketing.business.web.WebAPILocator; +import com.dotmarketing.filters.CMSUrlUtil; import com.dotmarketing.portlets.contentlet.model.Contentlet; import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; import com.dotmarketing.util.VelocityUtil; import com.liferay.portal.model.User; +import com.liferay.util.StringPool; import org.apache.velocity.context.Context; import org.apache.velocity.context.InternalContextAdapterImpl; import org.apache.velocity.tools.view.context.ViewContext; import org.apache.velocity.tools.view.tools.ViewTool; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import java.util.HashMap; import java.util.Optional; /** @@ -30,8 +30,7 @@ */ public class SecretTool implements ViewTool { - private InternalContextAdapterImpl internalContextAdapter; - private Context context; + private Context context; private HttpServletRequest request; @Override @@ -57,8 +56,8 @@ public Object get(final String key) { canUserEvaluate(); - final HttpServletRequest request = HttpServletRequestThreadLocal.INSTANCE.getRequest(); - final Optional config = DotVelocitySecretAppConfig.config(request); + final HttpServletRequest requestFromThreadLocal = HttpServletRequestThreadLocal.INSTANCE.getRequest(); + final Optional config = DotVelocitySecretAppConfig.config(requestFromThreadLocal); return config.isPresent()? config.get().getStringOrNull(key) : null; } @@ -90,8 +89,8 @@ public Object getSystemSecret (final String key, public char[] getCharArray(final String key) { canUserEvaluate(); - final HttpServletRequest request = HttpServletRequestThreadLocal.INSTANCE.getRequest(); - final Optional config = DotVelocitySecretAppConfig.config(request); + final HttpServletRequest requestFromThreadLocal = HttpServletRequestThreadLocal.INSTANCE.getRequest(); + final Optional config = DotVelocitySecretAppConfig.config(requestFromThreadLocal); return config.isPresent()? config.get().getCharArrayOrNull(key) : null; } @@ -110,51 +109,72 @@ public char[] getCharArraySystemSecret (final String key, private static final boolean ENABLE_SCRIPTING = Config.getBooleanProperty("secrets.scripting.enabled", false); - /** - * Test 2 things. - * 1) see if the user has the scripting role - * 2) otherwise check if the last modified user has the scripting role - * @return boolean - */ + /** + * Checks for the existence of the mandatory Scripting Role based on the following criteria: + *
    + *
  1. The User that last modified the Contentlet rendering the Secrets has the Scripting + * Role.
    2. + *
  2. The User present in the HTTP Request has the Scripting Role assigned to it.
    3. + *
+ * + * @throws SecurityException The User that either added the Secrets ViewTool code or the User + * present in the HTTP Request does not have the required Role. + */ protected void canUserEvaluate() { - - if(!ENABLE_SCRIPTING) { - + final String disabledScriptingErrorMsg = "External scripting is disabled in your dotcms instance"; + if (!ENABLE_SCRIPTING) { Logger.warn(this.getClass(), "Scripting called and ENABLE_SCRIPTING set to false"); - throw new SecurityException("External scripting is disabled in your dotcms instance."); + throw new SecurityException(disabledScriptingErrorMsg); } try { - - boolean hasScriptingRole = false; final Role scripting = APILocator.getRoleAPI().loadRoleByKey(Role.SCRIPTING_DEVELOPER); - - this.internalContextAdapter = new InternalContextAdapterImpl(context); - final String fieldResourceName = this.internalContextAdapter.getCurrentTemplateName(); - if (UtilMethods.isSet(fieldResourceName)) { - final String contentletFileAssetInode = fieldResourceName.substring(fieldResourceName.indexOf("/") + 1, fieldResourceName.indexOf("_")); - final Contentlet contentlet = APILocator.getContentletAPI().find(contentletFileAssetInode, APILocator.systemUser(), true); - final User lastModifiedUser = APILocator.getUserAPI().loadUserById(contentlet.getModUser(), APILocator.systemUser(), true); - hasScriptingRole = APILocator.getRoleAPI().doesUserHaveRole(lastModifiedUser, scripting); - } - + boolean hasScriptingRole = checkRoleFromLastModUser(scripting); if (!hasScriptingRole) { final User user = WebAPILocator.getUserWebAPI().getUser(this.request); // try with the current user if (null != user) { - hasScriptingRole = APILocator.getRoleAPI().doesUserHaveRole(user, scripting); } } if (!hasScriptingRole) { - - throw new SecurityException("External scripting is disabled in your dotcms instance."); + throw new SecurityException(disabledScriptingErrorMsg); } - } catch(Exception e) { - - Logger.warn(this.getClass(), "Scripting called with error" + e); - throw new SecurityException("External scripting is disabled in your dotcms instance.", e); + } catch (final Exception e) { + Logger.warnAndDebug(this.getClass(), String.format("Failed to evaluate Scripting Role " + + "presence: %s", ExceptionUtil.getErrorMessage(e)), e); + throw new SecurityException(disabledScriptingErrorMsg, e); } } // canUserEvaluate. + + /** + * Checks whether the User who last edited the Contentlet rendering the Secrets has the + * specified dotCMS Role or not. + * + * @param role The {@link Role} that will be checked. + * + * @return If the User has the specified Role, returns {@code true}. + */ + private boolean checkRoleFromLastModUser(final Role role) { + final InternalContextAdapterImpl internalContextAdapter = new InternalContextAdapterImpl(context); + final String resourcePath = internalContextAdapter.getCurrentTemplateName(); + boolean hasRole = false; + if (UtilMethods.isSet(resourcePath)) { + String contentletInode = StringPool.BLANK; + try { + contentletInode = CMSUrlUtil.getInstance().getInodeFromUrlPath(resourcePath); + final Contentlet contentlet = APILocator.getContentletAPI().find(contentletInode, APILocator.systemUser(), true); + final User lastModifiedUser = APILocator.getUserAPI().loadUserById(contentlet.getModUser(), APILocator.systemUser(), true); + hasRole = APILocator.getRoleAPI().doesUserHaveRole(lastModifiedUser, role); + } catch (final Exception e) { + Logger.warnAndDebug(SecretTool.class, String.format("Failed to find last " + + "modification user from Retrieved ID '%s' in URL Path '%s': %s", + contentletInode, resourcePath, + ExceptionUtil.getErrorMessage(e)), e); + } + } + return hasRole; + } + } diff --git a/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java b/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java index fe66462b294a..4d2550116544 100644 --- a/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java +++ b/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java @@ -1,11 +1,5 @@ package com.dotmarketing.filters; -import static com.dotmarketing.business.PermissionAPI.PERMISSION_READ; -import static com.dotmarketing.filters.CMSFilter.CMS_INDEX_PAGE; -import static com.dotmarketing.filters.Constants.CMS_FILTER_QUERY_STRING_OVERRIDE; -import static com.dotmarketing.filters.Constants.CMS_FILTER_URI_OVERRIDE; -import static java.util.stream.Collectors.toSet; - import com.dotcms.contenttype.model.type.BaseContentType; import com.dotmarketing.beans.Host; import com.dotmarketing.beans.Identifier; @@ -31,6 +25,10 @@ import com.liferay.util.Xss; import io.vavr.Tuple; import io.vavr.Tuple2; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URI; @@ -42,9 +40,14 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; + +import static com.dotmarketing.business.PermissionAPI.PERMISSION_READ; +import static com.dotmarketing.filters.CMSFilter.CMS_INDEX_PAGE; +import static com.dotmarketing.filters.Constants.CMS_FILTER_QUERY_STRING_OVERRIDE; +import static com.dotmarketing.filters.Constants.CMS_FILTER_URI_OVERRIDE; +import static com.liferay.util.StringPool.FORWARD_SLASH; +import static com.liferay.util.StringPool.UNDERLINE; +import static java.util.stream.Collectors.toSet; /** * Utilitary class used by the CMS Filter @@ -576,4 +579,25 @@ public static String getCurrentURI(final HttpServletRequest request) { throw new DotRuntimeException(e); } } + + /** + * Tries to recover the Inode from the URL path. The URL could be a page, such as: + * {@code /LIVE/27e8f845c3bd21ad1c601b8fe005caa6/dotParser_1695072095296.container} , or a call + * to a resource, such as: {@code Content/27e8f845c3bd21ad1c601b8fe005caa6_1695072095296} + * + * @param urlPath The URL path from a Contentlet. + * + * @return The Inode of the Contentlet. + */ + public String getInodeFromUrlPath(final String urlPath) { + final PageMode[] modes = PageMode.values(); + for (final PageMode mode : modes) { + if (urlPath.startsWith(FORWARD_SLASH + mode.name() + FORWARD_SLASH)) { + final String urlPathWithoutMode = urlPath.substring(mode.name().length() + 2); + return urlPathWithoutMode.substring(0, urlPathWithoutMode.indexOf(FORWARD_SLASH)); + } + } + return urlPath.substring(urlPath.indexOf(FORWARD_SLASH) + 1, urlPath.indexOf(UNDERLINE)); + } + } \ No newline at end of file diff --git a/dotCMS/src/test/java/com/dotmarketing/filters/CMSUrlUtilTest.java b/dotCMS/src/test/java/com/dotmarketing/filters/CMSUrlUtilTest.java index e8dbc4b2ced7..eb313ffa6cd9 100644 --- a/dotCMS/src/test/java/com/dotmarketing/filters/CMSUrlUtilTest.java +++ b/dotCMS/src/test/java/com/dotmarketing/filters/CMSUrlUtilTest.java @@ -1,14 +1,15 @@ package com.dotmarketing.filters; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; + import static com.dotmarketing.filters.Constants.CMS_FILTER_URI_OVERRIDE; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import java.io.UnsupportedEncodingException; -import javax.servlet.http.HttpServletRequest; -import org.junit.Test; - /** * @author nollymar */ @@ -44,4 +45,22 @@ public void testGetURIFromRequestWhenFilterIsSet() { assertEquals("dotcms+test.txt", result); } + /** + * Method To Test: {@link CMSUrlUtil#getInodeFromUrlPath(String)} + * Given Scenario: Invoke with a page live url + * ExpectedResult: the contentlet identifier will be returned + */ + @Test + public void test_getIdentifierFromUrlPath() { + final String liveUrlPath = "/LIVE/27e8f845c3bd21ad1c601b8fe005caa6/dotParser_1695072095296.container"; + final String contentIdentifier = CMSUrlUtil.getInstance().getInodeFromUrlPath(liveUrlPath); + assertNotNull(contentIdentifier); + assertEquals("27e8f845c3bd21ad1c601b8fe005caa6", contentIdentifier); + + final String templateUrlPath = "CONTENT/27e8f845c3bd21ad1c601b8fe005caa6_1695072095296.content"; + final String contentIdentifier2 = CMSUrlUtil.getInstance().getInodeFromUrlPath(templateUrlPath); + assertNotNull(contentIdentifier2); + assertEquals("27e8f845c3bd21ad1c601b8fe005caa6", contentIdentifier2); + } + }