Adding minor fixes reported via IQA feedback.

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<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;
 	}
 
@@ -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;
 	}
 
@@ -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;
+	}
+
 }

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));
+	}
+
 }

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);
+    }
+
 }