Refactor: optimize AntPathMatcher usage, eliminate code duplication and improve logging

jmix-localfs/localfs/src/main/java/io/jmix/localfs/LocalFileStorage.java

@@ -238,7 +238,7 @@ public InputStream openStream(FileRef reference) {
             Path path = root.resolve(relativePath);
 
             if (!path.toFile().exists()) {
-                log.error("File " + path + " not found");
+                log.error("File {} not found", path);
                 continue;
             }
 
@@ -250,7 +250,7 @@ public InputStream openStream(FileRef reference) {
 
                 inputStream = Files.newInputStream(path);
             } catch (IOException e) {
-                log.error("Error opening input stream for " + path, e);
+                log.error("Error opening input stream for {}", path, e);
             }
         }
 

jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/AbstractReportBeforeInvocationEventListener.java

@@ -0,0 +1,84 @@
+/*
+ * Copyright 2021 Haulmont.
+ *
+ * Licensed 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 io.jmix.reportsrest.security.event;
+
+import io.jmix.core.AccessManager;
+import io.jmix.core.security.SecurityContextHelper;
+import io.jmix.reportsrest.security.accesscontext.ReportRestAccessContext;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.http.HttpServletRequest;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.http.HttpStatus;
+import org.springframework.security.core.Authentication;
+import org.springframework.util.AntPathMatcher;
+
+/**
+ * Base class for report REST API access listeners that checks "reports.rest.enabled" specific policy
+ * for /rest/reports/** requests. If the current user doesn't have this policy then the FORBIDDEN error is thrown.
+ */
+public abstract class AbstractReportBeforeInvocationEventListener {
+
+    protected static final String REPORT_AUTHORIZED_URL = "/rest/reports/**";
+    protected static final AntPathMatcher ANT_PATH_MATCHER = new AntPathMatcher();
+
+    @Autowired
+    protected AccessManager accessManager;
+
+    /**
+     * Checks if the request should be validated and applies access constraints.
+     *
+     * @param request        the servlet request
+     * @param authentication the authentication to check permissions for
+     * @return {@code true} if access is permitted, {@code false} otherwise
+     */
+    protected boolean checkAccess(ServletRequest request, Authentication authentication) {
+        if (!shouldCheckRequest(request)) {
+            return true;
+        }
+
+        ReportRestAccessContext reportRestAccessContext = new ReportRestAccessContext();
+        Authentication currentAuthentication = SecurityContextHelper.getAuthentication();
+        try {
+            SecurityContextHelper.setAuthentication(authentication);
+            accessManager.applyRegisteredConstraints(reportRestAccessContext);
+        } finally {
+            SecurityContextHelper.setAuthentication(currentAuthentication);
+        }
+
+        return reportRestAccessContext.isPermitted();
+    }
+
+    /**
+     * Returns the HTTP status code for forbidden access.
+     *
+     * @return the forbidden status code
+     */
+    protected int getForbiddenErrorCode() {
+        return HttpStatus.FORBIDDEN.value();
+    }
+
+    /**
+     * Checks if the request URI matches the report REST API pattern.
+     *
+     * @param request the servlet request
+     * @return {@code true} if the request should be checked, {@code false} otherwise
+     */
+    protected boolean shouldCheckRequest(ServletRequest request) {
+        String requestURI = ((HttpServletRequest) request).getRequestURI();
+        return ANT_PATH_MATCHER.match(REPORT_AUTHORIZED_URL, requestURI);
+    }
+}

jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportAsResourceServerBeforeInvocationEventListener.java

@@ -17,51 +17,20 @@
 package io.jmix.reportsrest.security.event;
 
 import io.jmix.authserver.event.AsResourceServerBeforeInvocationEvent;
-import io.jmix.core.AccessManager;
-import io.jmix.core.security.SecurityContextHelper;
-import io.jmix.reportsrest.security.accesscontext.ReportRestAccessContext;
-import jakarta.servlet.ServletRequest;
-import jakarta.servlet.http.HttpServletRequest;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.event.EventListener;
-import org.springframework.http.HttpStatus;
-import org.springframework.security.core.Authentication;
-import org.springframework.util.AntPathMatcher;
 
 /**
  * A listener for {@link AsResourceServerBeforeInvocationEvent} that checks "reports.rest.enabled" specific policy
  * for /rest/reports/** requests, managed by resource server of the Authorization Server add-on. If the current user
  * doesn't have this policy then the FORBIDDEN error is thrown.
  */
-public class ReportAsResourceServerBeforeInvocationEventListener {
-
-    private static final String REPORT_AUTHORIZED_URL = "/rest/reports/**";
-
-    @Autowired
-    protected AccessManager accessManager;
+public class ReportAsResourceServerBeforeInvocationEventListener extends AbstractReportBeforeInvocationEventListener {
 
     @EventListener(AsResourceServerBeforeInvocationEvent.class)
     public void doListen(AsResourceServerBeforeInvocationEvent event) {
-        if (shouldCheckRequest(event.getRequest())) {
-            ReportRestAccessContext reportRestAccessContext = new ReportRestAccessContext();
-            Authentication currentAuthentication = SecurityContextHelper.getAuthentication();
-            try {
-                SecurityContextHelper.setAuthentication(event.getAuthentication());
-                accessManager.applyRegisteredConstraints(reportRestAccessContext);
-            } finally {
-                SecurityContextHelper.setAuthentication(currentAuthentication);
-            }
-
-            if (!reportRestAccessContext.isPermitted()) {
-                event.preventInvocation();
-                event.setErrorCode(HttpStatus.FORBIDDEN.value());
-            }
+        if (!checkAccess(event.getRequest(), event.getAuthentication())) {
+            event.preventInvocation();
+            event.setErrorCode(getForbiddenErrorCode());
         }
     }
-
-    protected boolean shouldCheckRequest(ServletRequest request) {
-        String requestURI = ((HttpServletRequest) request).getRequestURI();
-        AntPathMatcher antPathMatcher = new AntPathMatcher();
-        return antPathMatcher.match(REPORT_AUTHORIZED_URL, requestURI);
-    }
-}
+}

jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportOidcResourceServerBeforeInvocationEventListener.java

@@ -16,53 +16,21 @@
 
 package io.jmix.reportsrest.security.event;
 
-import io.jmix.core.AccessManager;
-import io.jmix.core.security.SecurityContextHelper;
 import io.jmix.oidc.resourceserver.OidcResourceServerBeforeInvocationEvent;
-import io.jmix.reportsrest.security.accesscontext.ReportRestAccessContext;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.event.EventListener;
-import org.springframework.http.HttpStatus;
-import org.springframework.security.core.Authentication;
-import org.springframework.util.AntPathMatcher;
-
-import jakarta.servlet.ServletRequest;
-import jakarta.servlet.http.HttpServletRequest;
 
 /**
- * A copy of {@link ReportAsResourceServerBeforeInvocationEventListener} that works with OIDC add-on events.
- *
- * TODO get rid of code duplication
+ * A listener for {@link OidcResourceServerBeforeInvocationEvent} that checks "reports.rest.enabled" specific policy
+ * for /rest/reports/** requests, managed by resource server of the OIDC add-on. If the current user
+ * doesn't have this policy then the FORBIDDEN error is thrown.
  */
-public class ReportOidcResourceServerBeforeInvocationEventListener {
-
-    private static final String REPORT_AUTHORIZED_URL = "/rest/reports/**";
-
-    @Autowired
-    protected AccessManager accessManager;
+public class ReportOidcResourceServerBeforeInvocationEventListener extends AbstractReportBeforeInvocationEventListener {
 
     @EventListener(OidcResourceServerBeforeInvocationEvent.class)
     public void doListen(OidcResourceServerBeforeInvocationEvent event) {
-        if (shouldCheckRequest(event.getRequest())) {
-            ReportRestAccessContext reportRestAccessContext = new ReportRestAccessContext();
-            Authentication currentAuthentication = SecurityContextHelper.getAuthentication();
-            try {
-                SecurityContextHelper.setAuthentication(event.getAuthentication());
-                accessManager.applyRegisteredConstraints(reportRestAccessContext);
-            } finally {
-                SecurityContextHelper.setAuthentication(currentAuthentication);
-            }
-
-            if (!reportRestAccessContext.isPermitted()) {
-                event.preventInvocation();
-                event.setErrorCode(HttpStatus.FORBIDDEN.value());
-            }
+        if (!checkAccess(event.getRequest(), event.getAuthentication())) {
+            event.preventInvocation();
+            event.setErrorCode(getForbiddenErrorCode());
         }
     }
-
-    protected boolean shouldCheckRequest(ServletRequest request) {
-        String requestURI = ((HttpServletRequest) request).getRequestURI();
-        AntPathMatcher antPathMatcher = new AntPathMatcher();
-        return antPathMatcher.match(REPORT_AUTHORIZED_URL, requestURI);
-    }
-}
+}

jmix-reports/reports/src/main/java/io/jmix/reports/impl/ReportExecutionHistoryRecorderImpl.java

@@ -236,7 +236,7 @@ private void deleteFiles(List<FileRef> fileRefs) {
             try {
                 getFileStorage().removeFile(path);
             } catch (FileStorageException e) {
-                log.error("Failed to remove document from storage " + path, e);
+                log.error("Failed to remove document from storage {}", path, e);
             }
         }
     }

jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/HtmlFormatter.java

@@ -145,13 +145,13 @@ protected void loadFontsFromDirectory(HtmlToPdfConverter converter, File fontsDi
                         }
                     }
                 } else {
-                    log.debug("Fonts directory is empty: " + fontsDir.getPath());
+                    log.debug("Fonts directory is empty: {}", fontsDir.getPath());
                 }
             } else {
                 log.warn(format("File %s is not a directory", fontsDir.getAbsolutePath()));
             }
         } else {
-            log.debug("Fonts directory does not exist: " + fontsDir.getPath());
+            log.debug("Fonts directory does not exist: {}", fontsDir.getPath());
         }
     }
 

jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/JavaProcessManager.java

@@ -33,7 +33,7 @@ public List<Long> findPid(String host, int port) {
 
     @Override
     public void kill(Process process, List<Long> pids) {
-        log.info("Java office process manager is going to kill following processes " + pids);
+        log.info("Java office process manager is going to kill following processes {}", pids);
         if (process != null)
             process.destroy();
     }

jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/LinuxProcessManager.java

@@ -62,7 +62,7 @@ public List<Long> findPid(String host, int port) {
     }
 
     public void kill(Process process, List<Long> pids) {
-        log.info("Linux office process manager is going to kill following processes " + pids);
+        log.info("Linux office process manager is going to kill following processes {}", pids);
         for (Long pid : pids) {
             try {
                 if (PID_UNKNOWN != pid) {
@@ -72,7 +72,7 @@ public void kill(Process process, List<Long> pids) {
                     super.kill(process, Collections.singletonList(pid));
                 }
             } catch (Exception e) {
-                log.error(String.format("An error occurred while killing process %d in linux system. Process.destroy() will be called.", pid), e);
+                log.error("An error occurred while killing process {} in linux system. Process.destroy() will be called.", pid, e);
                 super.kill(process, Collections.singletonList(pid));
             }
         }

jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/OOServer.java

@@ -216,9 +216,9 @@ protected void deleteProfileDir() {
                 File oldProfileDir = new File(instanceProfileDir.getParentFile(),
                         instanceProfileDir.getName() + ".old." + System.currentTimeMillis());
                 if (instanceProfileDir.renameTo(oldProfileDir)) {
-                    log.warn("could not delete profileDir: " + ioException.getMessage() + "; renamed it to " + oldProfileDir);
+                    log.warn("Could not delete profileDir: {}; renamed it to {}", ioException.getMessage(), oldProfileDir);
                 } else {
-                    log.error("could not delete profileDir: " + ioException.getMessage());
+                    log.error("Could not delete profileDir: {}", ioException.getMessage());
                 }
             }
         }

jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/WinProcessManager.java

@@ -61,7 +61,7 @@ public List<Long> findPid(String host, int port) {
 
     @Override
     public void kill(Process process, List<Long> pids) {
-        log.info("Windows office process manager is going to kill following processes " + pids);
+        log.info("Windows office process manager is going to kill following processes {}", pids);
         for (Long pid : pids) {
             try {
                 if (PID_UNKNOWN != pid) {
@@ -72,7 +72,7 @@ public void kill(Process process, List<Long> pids) {
                     super.kill(process, Collections.singletonList(pid));
                 }
             } catch (IOException e) {
-                log.error(String.format("An error occurred while killing process %d in windows system. Process.destroy() will be called.", pid), e);
+                log.error("An error occurred while killing process {} in windows system. Process.destroy() will be called.", pid, e);
                 super.kill(process, Collections.singletonList(pid));
             }
         }

jmix-rest/rest/src/main/java/io/jmix/rest/impl/config/RestJsonTransformations.java

@@ -149,7 +149,7 @@ protected void init() {
                     IOUtils.closeQuietly(stream);
                 }
             } else {
-                log.warn("Resource " + location + " not found, ignore it");
+                log.warn("Resource {} not found, ignore it", location);
             }
         }
     }

jmix-rest/rest/src/main/java/io/jmix/rest/security/impl/RestAuthorizedUrlsRequestMatcher.java

@@ -28,6 +28,8 @@
 @Component("rest_RestAuthorizedUrlsRequestMatcher")
 public class RestAuthorizedUrlsRequestMatcher {
 
+    private static final AntPathMatcher ANT_PATH_MATCHER = new AntPathMatcher();
+
     private final List<String> restAuthorizedUrls;
 
     public RestAuthorizedUrlsRequestMatcher(RestProperties restProperties) {
@@ -48,10 +50,9 @@ public RestAuthorizedUrlsRequestMatcher(RestProperties restProperties) {
     public boolean isAuthorizedUrl(ServletRequest request) {
         String requestURI = ((HttpServletRequest) request).getRequestURI();
         String contextPath = ((HttpServletRequest) request).getContextPath();
-        AntPathMatcher antPathMatcher = new AntPathMatcher();
 
         for (String urlPattern : restAuthorizedUrls) {
-            if (antPathMatcher.match(contextPath + urlPattern, requestURI)) {
+            if (ANT_PATH_MATCHER.match(contextPath + urlPattern, requestURI)) {
                 return true;
             }
         }