Skip to content

Commit

Permalink
fix(core): Add a role extra layer constraint into the secrets view tool
Browse files Browse the repository at this point in the history 
#25587

* Adding minor fixes reported via IQA feedback.

* Using constants for characters

* Implementing Code Review feedback.

* Implementing SonarQube feedback.
  • Loading branch information
@jcastro-dotcms @dsolistorres
jcastro-dotcms authored and dsolistorres committed Nov 6, 2023
1 parent 1406b9f commit 863ef2d
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 51 deletions.
96 changes: 58 additions & 38 deletions dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/SecretTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -30,8 +30,7 @@
*/
public class SecretTool implements ViewTool {

private InternalContextAdapterImpl internalContextAdapter;
private Context context;
private Context context;
private HttpServletRequest request;

@Override
Expand All @@ -57,8 +56,8 @@ public Object get(final String key) {

canUserEvaluate();

final HttpServletRequest request = HttpServletRequestThreadLocal.INSTANCE.getRequest();
final Optional<DotVelocitySecretAppConfig> config = DotVelocitySecretAppConfig.config(request);
final HttpServletRequest requestFromThreadLocal = HttpServletRequestThreadLocal.INSTANCE.getRequest();
final Optional<DotVelocitySecretAppConfig> config = DotVelocitySecretAppConfig.config(requestFromThreadLocal);
return config.isPresent()? config.get().getStringOrNull(key) : null;
}

Expand Down Expand Up @@ -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<DotVelocitySecretAppConfig> config = DotVelocitySecretAppConfig.config(request);
final HttpServletRequest requestFromThreadLocal = HttpServletRequestThreadLocal.INSTANCE.getRequest();
final Optional<DotVelocitySecretAppConfig> config = DotVelocitySecretAppConfig.config(requestFromThreadLocal);
return config.isPresent()? config.get().getCharArrayOrNull(key) : null;
}

Expand All @@ -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:
* <ol>
* <li>The User that last modified the Contentlet rendering the Secrets has the Scripting
* Role.</li>
* <li>The User present in the HTTP Request has the Scripting Role assigned to it.</li>
* </ol>
*
* @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;
}

}
42 changes: 33 additions & 9 deletions dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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));
}

}
27 changes: 23 additions & 4 deletions dotCMS/src/test/java/com/dotmarketing/filters/CMSUrlUtilTest.java
View file
Original file line number Diff line number Diff line change
@@ -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
*/
Expand Down Expand Up @@ -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);
}

}

0 comments on commit 863ef2d

Please sign in to comment.