From ded85a83495e0d6c13bfbf8b9a006aeeb68d5c85 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 9 Sep 2024 10:29:34 +0200 Subject: [PATCH] add tests for web/AuthorizationFilter (#4639) --- .../org/opengrok/web/AuthorizationFilter.java | 41 +++--- .../java/org/opengrok/web/PageConfig.java | 8 +- .../opengrok/web/AuthorizationFilterTest.java | 137 ++++++++++++++++++ 3 files changed, 163 insertions(+), 23 deletions(-) create mode 100644 opengrok-web/src/test/java/org/opengrok/web/AuthorizationFilterTest.java diff --git a/opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java b/opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java index adf31b67a75..a52ac7f85ea 100644 --- a/opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java +++ b/opengrok-web/src/main/java/org/opengrok/web/AuthorizationFilter.java @@ -18,12 +18,13 @@ */ /* - * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved. * Portions Copyright (c) 2020, Chris Fraire . */ package org.opengrok.web; import java.io.IOException; +import java.util.Objects; import java.util.logging.Level; import java.util.logging.Logger; @@ -35,6 +36,7 @@ import jakarta.servlet.ServletResponse; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.opengrok.indexer.configuration.IncludeFiles; import org.opengrok.indexer.configuration.Project; import org.opengrok.indexer.logger.LoggerFactory; import org.opengrok.indexer.web.Laundromat; @@ -46,7 +48,7 @@ public class AuthorizationFilter implements Filter { @Override public void init(FilterConfig fc) { - // Empty since there is No specific init configuration. + // Empty since there is no specific init configuration. } /** @@ -56,49 +58,50 @@ public void init(FilterConfig fc) { * so does not have to be exempted here. */ @Override - public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc) throws IOException, ServletException { - HttpServletRequest httpReq = (HttpServletRequest) sr; - HttpServletResponse httpRes = (HttpServletResponse) sr1; + public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain fc) + throws IOException, ServletException { - if (httpReq.getServletPath().startsWith(RestApp.API_PATH)) { + HttpServletRequest httpRequest = (HttpServletRequest) servletRequest; + HttpServletResponse httpResponse = (HttpServletResponse) servletResponse; + + if (httpRequest.getServletPath().startsWith(RestApp.API_PATH)) { if (LOGGER.isLoggable(Level.FINER)) { LOGGER.log(Level.FINER, "Allowing request to {0} in {1}", - new Object[] {Laundromat.launderLog(httpReq.getServletPath()), + new Object[] {Laundromat.launderLog(httpRequest.getServletPath()), AuthorizationFilter.class.getName()}); } - fc.doFilter(sr, sr1); + fc.doFilter(servletRequest, servletResponse); return; } - PageConfig config = PageConfig.get(httpReq); - + PageConfig config = PageConfig.get(httpRequest); Project p = config.getProject(); if (p != null && !config.isAllowed(p)) { if (LOGGER.isLoggable(Level.INFO)) { - if (httpReq.getRemoteUser() != null) { + if (httpRequest.getRemoteUser() != null) { LOGGER.log(Level.INFO, "Access denied for user ''{0}'' for URI: {1}", - new Object[] {Laundromat.launderLog(httpReq.getRemoteUser()), - Laundromat.launderLog(httpReq.getRequestURI())}); + new Object[] {Laundromat.launderLog(httpRequest.getRemoteUser()), + Laundromat.launderLog(httpRequest.getRequestURI())}); } else { LOGGER.log(Level.INFO, "Access denied for URI: {0}", - Laundromat.launderLog(httpReq.getRequestURI())); + Laundromat.launderLog(httpRequest.getRequestURI())); } } - if (!config.getEnv().getIncludeFiles().getForbiddenIncludeFileContent(false).isEmpty()) { - sr.getRequestDispatcher("/eforbidden").forward(sr, sr1); + IncludeFiles includeFiles = config.getEnv().getIncludeFiles(); + if (Objects.nonNull(includeFiles) && !includeFiles.getForbiddenIncludeFileContent(false).isEmpty()) { + servletRequest.getRequestDispatcher("/eforbidden").forward(servletRequest, servletResponse); return; } - httpRes.sendError(HttpServletResponse.SC_FORBIDDEN, "Access forbidden"); + httpResponse.sendError(HttpServletResponse.SC_FORBIDDEN, "Access forbidden"); return; } - fc.doFilter(sr, sr1); + fc.doFilter(servletRequest, servletResponse); } @Override public void destroy() { // Empty since there is No specific destroy configuration. } - } diff --git a/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java b/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java index d2be6e97f44..a821834eb4c 100644 --- a/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java +++ b/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java @@ -113,12 +113,12 @@ * pages (how content gets generated) consistent and to document the request * parameters used. *

- * General contract for this class (i.e. if not explicitly documented): no - * method of this class changes neither the request nor the response. + * General contract for this class (i.e. if not explicitly documented): + * no method of this class changes neither the request nor the response. * * @author Jens Elkner */ -public final class PageConfig { +public class PageConfig { private static final Logger LOGGER = LoggerFactory.getLogger(PageConfig.class); @@ -166,7 +166,7 @@ public final class PageConfig { */ private final Scripts scripts = new Scripts(); - private static final String ATTR_NAME = PageConfig.class.getCanonicalName(); + static final String ATTR_NAME = PageConfig.class.getCanonicalName(); private HttpServletRequest req; private final ExecutorService executor; diff --git a/opengrok-web/src/test/java/org/opengrok/web/AuthorizationFilterTest.java b/opengrok-web/src/test/java/org/opengrok/web/AuthorizationFilterTest.java new file mode 100644 index 00000000000..b4e1e528b95 --- /dev/null +++ b/opengrok-web/src/test/java/org/opengrok/web/AuthorizationFilterTest.java @@ -0,0 +1,137 @@ +/* + * CDDL HEADER START + * + * The contents of this file are subject to the terms of the + * Common Development and Distribution License (the "License"). + * You may not use this file except in compliance with the License. + * + * See LICENSE.txt included in this distribution for the specific + * language governing permissions and limitations under the License. + * + * When distributing Covered Code, include this CDDL HEADER in each + * file and include the License file at LICENSE.txt. + * If applicable, add the following below this CDDL HEADER, with the + * fields enclosed by brackets "[]" replaced with your own identifying + * information: Portions Copyright [yyyy] [name of copyright owner] + * + * CDDL HEADER END + */ + +/* + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. + */ +package org.opengrok.web; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.FilterConfig; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.commons.lang3.tuple.Pair; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import org.opengrok.indexer.configuration.Project; +import org.opengrok.indexer.configuration.RuntimeEnvironment; +import org.opengrok.indexer.web.DummyHttpServletRequest; +import org.opengrok.indexer.web.Prefix; +import org.opengrok.web.api.v1.RestApp; + +import java.io.IOException; +import java.util.stream.Stream; + +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Provides coverage for the {@link AuthorizationFilter} class. + */ +class AuthorizationFilterTest { + /** + * Test that requests that start with API path are let through. + */ + @Test + void testApiPath() throws ServletException, IOException { + AuthorizationFilter filter = new AuthorizationFilter(); + HttpServletRequest request = new DummyHttpServletRequest() { + @Override + public String getServletPath() { + return RestApp.API_PATH + "foo"; + } + }; + HttpServletResponse response = mock(HttpServletResponse.class); + + FilterChain chain = mock(FilterChain.class); + // The init() method is currently empty, however it does not hurt to exercise it in case that changes. + FilterConfig filterConfig = mock(FilterConfig.class); + filter.init(filterConfig); + filter.doFilter(request, response, chain); + + verify(chain).doFilter(request, response); + verify(response, never()).sendError(anyInt()); + verify(response, never()).sendError(anyInt(), anyString()); + } + + private static Stream> getParamsForTestIsAllowed() { + return Stream.of(Pair.of(true, null), + Pair.of(true, "user"), + Pair.of(false, null), + Pair.of(false, "user")); + } + + @ParameterizedTest + @MethodSource("getParamsForTestIsAllowed") + void testIsAllowed(Pair pair) throws ServletException, IOException { + AuthorizationFilter filter = new AuthorizationFilter(); + PageConfig pageConfig = mock(PageConfig.class); + Project project = mock(Project.class); + when(pageConfig.getProject()).thenReturn(project); + when(pageConfig.isAllowed(project)).thenReturn(pair.getLeft()); + when(pageConfig.getEnv()).thenReturn(RuntimeEnvironment.getInstance()); + HttpServletRequest request = new DummyHttpServletRequest() { + @Override + public String getServletPath() { + return Prefix.DOWNLOAD_P.toString(); + } + + @Override + public String getRemoteUser() { + return pair.getRight(); + } + + @Override + public String getRequestURI() { + return "URI"; + } + + @Override + public Object getAttribute(String s) { + if (s.equals(PageConfig.ATTR_NAME)) { + return pageConfig; + } + + return "X"; + } + }; + HttpServletResponse response = mock(HttpServletResponse.class); + + FilterChain chain = mock(FilterChain.class); + // The init() method is currently empty, however it does not hurt to exercise it in case that changes. + FilterConfig filterConfig = mock(FilterConfig.class); + filter.init(filterConfig); + filter.doFilter(request, response, chain); + + if (pair.getLeft()) { + verify(chain).doFilter(request, response); + verify(response, never()).sendError(anyInt()); + verify(response, never()).sendError(anyInt(), anyString()); + } else { + verify(chain, never()).doFilter(request, response); + verify(response).sendError(HttpServletResponse.SC_FORBIDDEN, "Access forbidden"); + } + } +} \ No newline at end of file