From 15c6b42cc1d359cb2ce4838dda0b4853e4217cae Mon Sep 17 00:00:00 2001 From: VladimirKozulin Date: Fri, 16 Jan 2026 18:56:24 +0300 Subject: [PATCH] Refactor: optimize AntPathMatcher usage, eliminate code duplication, and improve logging - Task 1: Optimize AntPathMatcher instantiation - Extract AntPathMatcher to static final field in RestAuthorizedUrlsRequestMatcher - Extract AntPathMatcher to static final field in ReportAsResourceServerBeforeInvocationEventListener - Extract AntPathMatcher to static final field in ReportOidcResourceServerBeforeInvocationEventListener - Task 2: Eliminate code duplication in reports-rest module - Create AbstractReportBeforeInvocationEventListener base class with common logic - Refactor ReportAsResourceServerBeforeInvocationEventListener to extend base class - Refactor ReportOidcResourceServerBeforeInvocationEventListener to extend base class - Task 3: Improve logging by using SLF4J parameterized messages - RestJsonTransformations: use {} placeholder instead of string concatenation - ReportExecutionHistoryRecorderImpl: use {} placeholder for path logging - LocalFileStorage: use {} placeholders for file path logging - HtmlFormatter: use {} placeholders for fonts directory logging - JavaProcessManager: use {} placeholder for process list logging - LinuxProcessManager: use {} placeholder for process list and error logging - WinProcessManager: use {} placeholder for process list and error logging - OOServer: use {} placeholders for profile directory logging --- .../io/jmix/localfs/LocalFileStorage.java | 4 +- ...ctReportBeforeInvocationEventListener.java | 84 +++++++++++++++++++ ...ceServerBeforeInvocationEventListener.java | 41 ++------- ...ceServerBeforeInvocationEventListener.java | 48 ++--------- .../ReportExecutionHistoryRecorderImpl.java | 2 +- .../yarg/formatters/impl/HtmlFormatter.java | 4 +- .../doc/connector/JavaProcessManager.java | 2 +- .../doc/connector/LinuxProcessManager.java | 4 +- .../impl/doc/connector/OOServer.java | 4 +- .../impl/doc/connector/WinProcessManager.java | 4 +- .../impl/config/RestJsonTransformations.java | 2 +- .../RestAuthorizedUrlsRequestMatcher.java | 5 +- 12 files changed, 113 insertions(+), 91 deletions(-) create mode 100644 jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/AbstractReportBeforeInvocationEventListener.java diff --git a/jmix-localfs/localfs/src/main/java/io/jmix/localfs/LocalFileStorage.java b/jmix-localfs/localfs/src/main/java/io/jmix/localfs/LocalFileStorage.java index e72d6c5c7c..a0bc44f7b1 100644 --- a/jmix-localfs/localfs/src/main/java/io/jmix/localfs/LocalFileStorage.java +++ b/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); } } diff --git a/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/AbstractReportBeforeInvocationEventListener.java b/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/AbstractReportBeforeInvocationEventListener.java new file mode 100644 index 0000000000..3aaf14e662 --- /dev/null +++ b/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); + } +} diff --git a/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportAsResourceServerBeforeInvocationEventListener.java b/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportAsResourceServerBeforeInvocationEventListener.java index 1fe7068cbc..740e516b98 100644 --- a/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportAsResourceServerBeforeInvocationEventListener.java +++ b/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); - } -} \ No newline at end of file +} diff --git a/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportOidcResourceServerBeforeInvocationEventListener.java b/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportOidcResourceServerBeforeInvocationEventListener.java index f0740144bf..16dac632a2 100644 --- a/jmix-reports/reports-rest/src/main/java/io/jmix/reportsrest/security/event/ReportOidcResourceServerBeforeInvocationEventListener.java +++ b/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); - } -} \ No newline at end of file +} diff --git a/jmix-reports/reports/src/main/java/io/jmix/reports/impl/ReportExecutionHistoryRecorderImpl.java b/jmix-reports/reports/src/main/java/io/jmix/reports/impl/ReportExecutionHistoryRecorderImpl.java index 48778a7303..957902638e 100644 --- a/jmix-reports/reports/src/main/java/io/jmix/reports/impl/ReportExecutionHistoryRecorderImpl.java +++ b/jmix-reports/reports/src/main/java/io/jmix/reports/impl/ReportExecutionHistoryRecorderImpl.java @@ -236,7 +236,7 @@ private void deleteFiles(List 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); } } } diff --git a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/HtmlFormatter.java b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/HtmlFormatter.java index 4a87edd8a5..121e27a8ce 100644 --- a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/HtmlFormatter.java +++ b/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()); } } diff --git a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/JavaProcessManager.java b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/JavaProcessManager.java index 66d9e899d4..3079911a64 100644 --- a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/JavaProcessManager.java +++ b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/JavaProcessManager.java @@ -33,7 +33,7 @@ public List findPid(String host, int port) { @Override public void kill(Process process, List 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(); } diff --git a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/LinuxProcessManager.java b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/LinuxProcessManager.java index c7401ad356..a0ad32ee48 100644 --- a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/LinuxProcessManager.java +++ b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/LinuxProcessManager.java @@ -62,7 +62,7 @@ public List findPid(String host, int port) { } public void kill(Process process, List 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 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)); } } diff --git a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/OOServer.java b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/OOServer.java index 5789b09d21..7c21892d5c 100644 --- a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/OOServer.java +++ b/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()); } } } diff --git a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/WinProcessManager.java b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/WinProcessManager.java index 76f1fe1a91..605fd96245 100644 --- a/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/WinProcessManager.java +++ b/jmix-reports/reports/src/main/java/io/jmix/reports/yarg/formatters/impl/doc/connector/WinProcessManager.java @@ -61,7 +61,7 @@ public List findPid(String host, int port) { @Override public void kill(Process process, List 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 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)); } } diff --git a/jmix-rest/rest/src/main/java/io/jmix/rest/impl/config/RestJsonTransformations.java b/jmix-rest/rest/src/main/java/io/jmix/rest/impl/config/RestJsonTransformations.java index b1f491a401..aad00c9f48 100644 --- a/jmix-rest/rest/src/main/java/io/jmix/rest/impl/config/RestJsonTransformations.java +++ b/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); } } } diff --git a/jmix-rest/rest/src/main/java/io/jmix/rest/security/impl/RestAuthorizedUrlsRequestMatcher.java b/jmix-rest/rest/src/main/java/io/jmix/rest/security/impl/RestAuthorizedUrlsRequestMatcher.java index 9099857e4b..bf53ea2de0 100644 --- a/jmix-rest/rest/src/main/java/io/jmix/rest/security/impl/RestAuthorizedUrlsRequestMatcher.java +++ b/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 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; } }