From a47970c37ecc466d269d8aeaf298459d431e98ac Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Wed, 24 Jan 2024 20:41:36 +0530 Subject: [PATCH 1/7] FIX Pac4j issue --- extensions-core/druid-pac4j/pom.xml | 4 +-- .../druid/security/pac4j/Pac4jFilter.java | 32 +++++++++++++++++-- licenses.yaml | 4 +-- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/extensions-core/druid-pac4j/pom.xml b/extensions-core/druid-pac4j/pom.xml index 620a926d26a6..39217c1b667f 100644 --- a/extensions-core/druid-pac4j/pom.xml +++ b/extensions-core/druid-pac4j/pom.xml @@ -38,8 +38,8 @@ 1.7 - 7.9 - 6.5 + 8.22.1 + 8.22 diff --git a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java index 452a22609460..7bf8e5eae4de 100644 --- a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java +++ b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java @@ -29,7 +29,11 @@ import org.pac4j.core.engine.DefaultCallbackLogic; import org.pac4j.core.engine.DefaultSecurityLogic; import org.pac4j.core.engine.SecurityLogic; +import org.pac4j.core.exception.TechnicalException; import org.pac4j.core.exception.http.HttpAction; +import org.pac4j.core.exception.http.RedirectionAction; +import org.pac4j.core.exception.http.WithContentAction; +import org.pac4j.core.exception.http.WithLocationAction; import org.pac4j.core.http.adapter.HttpActionAdapter; import org.pac4j.core.profile.UserProfile; @@ -48,7 +52,29 @@ public class Pac4jFilter implements Filter { private static final Logger LOGGER = new Logger(Pac4jFilter.class); - private static final HttpActionAdapter NOOP_HTTP_ACTION_ADAPTER = (HttpAction code, JEEContext ctx) -> null; + private static final HttpActionAdapter HTTP_ACTION_ADAPTER = (HttpAction action, JEEContext context) -> { + if (action instanceof RedirectionAction) { + int code = action.getCode(); + HttpServletResponse response = context.getNativeResponse(); + response.setStatus(code); + if (action instanceof WithLocationAction) { + WithLocationAction withLocationAction = (WithLocationAction) action; + context.setResponseHeader("Location", withLocationAction.getLocation()); + } else if (action instanceof WithContentAction) { + WithContentAction withContentAction = (WithContentAction) action; + String content = withContentAction.getContent(); + if (content != null) { + try { + response.getWriter().write(content); + } + catch (IOException var8) { + throw new TechnicalException(var8); + } + } + } + } + return null; + }; private final Config pac4jConfig; private final SecurityLogic securityLogic; @@ -95,7 +121,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo callbackLogic.perform( context, pac4jConfig, - NOOP_HTTP_ACTION_ADAPTER, + HTTP_ACTION_ADAPTER, "/", true, false, false, null); } else { @@ -110,7 +136,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo return profiles.iterator().next().getId(); } }, - NOOP_HTTP_ACTION_ADAPTER, + HTTP_ACTION_ADAPTER, null, null, null, null); if (uid != null) { diff --git a/licenses.yaml b/licenses.yaml index f1a4cba1159c..f3769470edff 100644 --- a/licenses.yaml +++ b/licenses.yaml @@ -809,7 +809,7 @@ name: com.nimbusds nimbus-jose-jwt license_category: binary module: extensions/druid-pac4j license_name: Apache License version 2.0 -version: 7.9 +version: 8.22.1 libraries: - com.nimbusds: nimbus-jose-jwt @@ -819,7 +819,7 @@ name: com.nimbusds oauth2-oidc-sdk license_category: binary module: extensions/druid-pac4j license_name: Apache License version 2.0 -version: 6.5 +version: 8.22 libraries: - com.nimbusds: oauth2-oidc-sdk From 4f55b093c1e98c39f2f653fe2631c984fc27a2bb Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Thu, 25 Jan 2024 12:37:58 +0530 Subject: [PATCH 2/7] Fix default Authorization --- .../druid/security/pac4j/Pac4jFilter.java | 40 ++++--------------- 1 file changed, 7 insertions(+), 33 deletions(-) diff --git a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java index 7bf8e5eae4de..23d38d7d6178 100644 --- a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java +++ b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java @@ -29,12 +29,8 @@ import org.pac4j.core.engine.DefaultCallbackLogic; import org.pac4j.core.engine.DefaultSecurityLogic; import org.pac4j.core.engine.SecurityLogic; -import org.pac4j.core.exception.TechnicalException; -import org.pac4j.core.exception.http.HttpAction; -import org.pac4j.core.exception.http.RedirectionAction; -import org.pac4j.core.exception.http.WithContentAction; -import org.pac4j.core.exception.http.WithLocationAction; import org.pac4j.core.http.adapter.HttpActionAdapter; +import org.pac4j.core.http.adapter.JEEHttpActionAdapter; import org.pac4j.core.profile.UserProfile; import javax.servlet.Filter; @@ -52,33 +48,11 @@ public class Pac4jFilter implements Filter { private static final Logger LOGGER = new Logger(Pac4jFilter.class); - private static final HttpActionAdapter HTTP_ACTION_ADAPTER = (HttpAction action, JEEContext context) -> { - if (action instanceof RedirectionAction) { - int code = action.getCode(); - HttpServletResponse response = context.getNativeResponse(); - response.setStatus(code); - if (action instanceof WithLocationAction) { - WithLocationAction withLocationAction = (WithLocationAction) action; - context.setResponseHeader("Location", withLocationAction.getLocation()); - } else if (action instanceof WithContentAction) { - WithContentAction withContentAction = (WithContentAction) action; - String content = withContentAction.getContent(); - if (content != null) { - try { - response.getWriter().write(content); - } - catch (IOException var8) { - throw new TechnicalException(var8); - } - } - } - } - return null; - }; + private static final HttpActionAdapter HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter(); private final Config pac4jConfig; - private final SecurityLogic securityLogic; - private final CallbackLogic callbackLogic; + private final SecurityLogic securityLogic; + private final CallbackLogic callbackLogic; private final SessionStore sessionStore; private final String name; @@ -125,7 +99,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo "/", true, false, false, null); } else { - String uid = securityLogic.perform( + Object uid = securityLogic.perform( context, pac4jConfig, (JEEContext ctx, Collection profiles, Object... parameters) -> { @@ -137,10 +111,10 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo } }, HTTP_ACTION_ADAPTER, - null, null, null, null); + null, "none", null, null); if (uid != null) { - AuthenticationResult authenticationResult = new AuthenticationResult(uid, authorizerName, name, null); + AuthenticationResult authenticationResult = new AuthenticationResult(uid.toString(), authorizerName, name, null); servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult); filterChain.doFilter(servletRequest, servletResponse); } From dfbab893d5a6241d86d2daf28d82e1f44abdeca6 Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Thu, 25 Jan 2024 14:44:05 +0530 Subject: [PATCH 3/7] Addressing review comment --- .../java/org/apache/druid/security/pac4j/Pac4jFilter.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java index 23d38d7d6178..c22129a9e65b 100644 --- a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java +++ b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java @@ -48,7 +48,8 @@ public class Pac4jFilter implements Filter { private static final Logger LOGGER = new Logger(Pac4jFilter.class); - private static final HttpActionAdapter HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter(); + // JEE_HTTP_ACTION_ADAPTER updates the response in the context according to the HTTPAction. + private static final HttpActionAdapter JEE_HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter(); private final Config pac4jConfig; private final SecurityLogic securityLogic; @@ -95,7 +96,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo callbackLogic.perform( context, pac4jConfig, - HTTP_ACTION_ADAPTER, + JEE_HTTP_ACTION_ADAPTER, "/", true, false, false, null); } else { @@ -110,7 +111,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo return profiles.iterator().next().getId(); } }, - HTTP_ACTION_ADAPTER, + JEE_HTTP_ACTION_ADAPTER, null, "none", null, null); if (uid != null) { From 9e91369ae52d3f430f324d1d14988a11740184c9 Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Mon, 29 Jan 2024 21:27:37 +0530 Subject: [PATCH 4/7] Fix build --- licenses.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/licenses.yaml b/licenses.yaml index f3769470edff..9446ae406e72 100644 --- a/licenses.yaml +++ b/licenses.yaml @@ -815,6 +815,16 @@ libraries: --- +name: com.nimbusds content-type +license_category: binary +module: extensions/druid-pac4j +license_name: Apache License version 2.0 +version: 2.1 +libraries: + - com.nimbusds: content-type + +--- + name: com.nimbusds oauth2-oidc-sdk license_category: binary module: extensions/druid-pac4j From 41f65eca981ff37ea1c2a1c4de90ea78c356267f Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Tue, 30 Jan 2024 10:12:39 +0530 Subject: [PATCH 5/7] adding test cases --- extensions-core/druid-pac4j/pom.xml | 5 ++ .../druid/security/pac4j/Pac4jFilterTest.java | 78 +++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java diff --git a/extensions-core/druid-pac4j/pom.xml b/extensions-core/druid-pac4j/pom.xml index 39217c1b667f..c71b877ec9c0 100644 --- a/extensions-core/druid-pac4j/pom.xml +++ b/extensions-core/druid-pac4j/pom.xml @@ -145,6 +145,11 @@ easymock test + + org.mockito + mockito-core + test + diff --git a/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java b/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java new file mode 100644 index 000000000000..cd3ad8794329 --- /dev/null +++ b/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.druid.security.pac4j; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; +import org.pac4j.core.context.JEEContext; +import org.pac4j.core.exception.http.ForbiddenAction; +import org.pac4j.core.exception.http.FoundAction; +import org.pac4j.core.exception.http.HttpAction; +import org.pac4j.core.exception.http.WithLocationAction; +import org.pac4j.core.http.adapter.JEEHttpActionAdapter; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import static org.mockito.ArgumentMatchers.any; + +@RunWith(MockitoJUnitRunner.class) +public class Pac4jFilterTest +{ + + private static final JEEHttpActionAdapter JEE_HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter(); + @Mock + private HttpServletRequest request; + @Mock + private HttpServletResponse response; + private JEEContext context; + + @Before + public void setUp() + { + context = new JEEContext(request, response); + } + + @Test + public void testActionAdapterForRedirection() + { + HttpAction httpAction = new FoundAction("testUrl"); + Mockito.doReturn(httpAction.getCode()).when(response).getStatus(); + Mockito.doReturn(((WithLocationAction) httpAction).getLocation()).when(response).getHeader(any()); + JEE_HTTP_ACTION_ADAPTER.adapt(httpAction, context); + Assert.assertEquals(response.getStatus(), 302); + Assert.assertEquals(response.getHeader("Location"), "testUrl"); + } + + @Test + public void testActionAdapterForForbidden() + { + HttpAction httpAction = ForbiddenAction.INSTANCE; + Mockito.doReturn(httpAction.getCode()).when(response).getStatus(); + JEE_HTTP_ACTION_ADAPTER.adapt(httpAction, context); + Assert.assertEquals(response.getStatus(), HttpServletResponse.SC_FORBIDDEN); + } + +} From 597922f5456b9c83d1cf616859a7a32edca774b8 Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Thu, 1 Feb 2024 00:06:08 +0530 Subject: [PATCH 6/7] addressing review comments --- extensions-core/druid-pac4j/pom.xml | 2 +- .../org/apache/druid/security/pac4j/Pac4jFilter.java | 12 +++++------- .../apache/druid/security/pac4j/Pac4jFilterTest.java | 5 ++--- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/extensions-core/druid-pac4j/pom.xml b/extensions-core/druid-pac4j/pom.xml index c71b877ec9c0..7e8530e02714 100644 --- a/extensions-core/druid-pac4j/pom.xml +++ b/extensions-core/druid-pac4j/pom.xml @@ -36,7 +36,7 @@ 4.5.7 - + 1.7 8.22.1 8.22 diff --git a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java index c22129a9e65b..0495242835c4 100644 --- a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java +++ b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java @@ -29,7 +29,6 @@ import org.pac4j.core.engine.DefaultCallbackLogic; import org.pac4j.core.engine.DefaultSecurityLogic; import org.pac4j.core.engine.SecurityLogic; -import org.pac4j.core.http.adapter.HttpActionAdapter; import org.pac4j.core.http.adapter.JEEHttpActionAdapter; import org.pac4j.core.profile.UserProfile; @@ -48,9 +47,6 @@ public class Pac4jFilter implements Filter { private static final Logger LOGGER = new Logger(Pac4jFilter.class); - // JEE_HTTP_ACTION_ADAPTER updates the response in the context according to the HTTPAction. - private static final HttpActionAdapter JEE_HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter(); - private final Config pac4jConfig; private final SecurityLogic securityLogic; private final CallbackLogic callbackLogic; @@ -96,7 +92,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo callbackLogic.perform( context, pac4jConfig, - JEE_HTTP_ACTION_ADAPTER, + JEEHttpActionAdapter.INSTANCE, "/", true, false, false, null); } else { @@ -111,9 +107,11 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo return profiles.iterator().next().getId(); } }, - JEE_HTTP_ACTION_ADAPTER, + JEEHttpActionAdapter.INSTANCE, null, "none", null, null); - + // Changed the Authorizer from null to "none". + // In the older version, if it is null, it simply grant access and returns authorized. + // But in the newer pac4j version, it uses CsrfAuthorizer as default, And because of this, It was returning 403 in API calls. if (uid != null) { AuthenticationResult authenticationResult = new AuthenticationResult(uid.toString(), authorizerName, name, null); servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult); diff --git a/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java b/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java index cd3ad8794329..0523c970178d 100644 --- a/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java +++ b/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jFilterTest.java @@ -42,7 +42,6 @@ public class Pac4jFilterTest { - private static final JEEHttpActionAdapter JEE_HTTP_ACTION_ADAPTER = new JEEHttpActionAdapter(); @Mock private HttpServletRequest request; @Mock @@ -61,7 +60,7 @@ public void testActionAdapterForRedirection() HttpAction httpAction = new FoundAction("testUrl"); Mockito.doReturn(httpAction.getCode()).when(response).getStatus(); Mockito.doReturn(((WithLocationAction) httpAction).getLocation()).when(response).getHeader(any()); - JEE_HTTP_ACTION_ADAPTER.adapt(httpAction, context); + JEEHttpActionAdapter.INSTANCE.adapt(httpAction, context); Assert.assertEquals(response.getStatus(), 302); Assert.assertEquals(response.getHeader("Location"), "testUrl"); } @@ -71,7 +70,7 @@ public void testActionAdapterForForbidden() { HttpAction httpAction = ForbiddenAction.INSTANCE; Mockito.doReturn(httpAction.getCode()).when(response).getStatus(); - JEE_HTTP_ACTION_ADAPTER.adapt(httpAction, context); + JEEHttpActionAdapter.INSTANCE.adapt(httpAction, context); Assert.assertEquals(response.getStatus(), HttpServletResponse.SC_FORBIDDEN); } From 5173f8b2836db3731ac03dd8bb492a01421c340f Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Thu, 1 Feb 2024 00:07:27 +0530 Subject: [PATCH 7/7] Correct indentation --- extensions-core/druid-pac4j/pom.xml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/extensions-core/druid-pac4j/pom.xml b/extensions-core/druid-pac4j/pom.xml index 7e8530e02714..aed4ab5a489c 100644 --- a/extensions-core/druid-pac4j/pom.xml +++ b/extensions-core/druid-pac4j/pom.xml @@ -145,11 +145,11 @@ easymock test - - org.mockito - mockito-core - test - + + org.mockito + mockito-core + test +