[druid] branch master updated: Revert PreResponseAuthorizationCheckFilter (#13813)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

gian pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git


The following commit(s) were added to refs/heads/master by this push:
     new e9fed1445f Revert PreResponseAuthorizationCheckFilter (#13813)
e9fed1445f is described below

commit e9fed1445f2124b94fdba5976a90b7b52cd5baff
Author: imply-cheddar <86...@users.noreply.github.com>
AuthorDate: Fri May 19 10:16:43 2023 +0900

    Revert PreResponseAuthorizationCheckFilter (#13813)
    
    Make it permissive like it used to be again so that we
    ensure that validation errors make it out.
---
 .../PreResponseAuthorizationCheckFilter.java       | 19 ++++-----------
 .../PreResponseAuthorizationCheckFilterTest.java   | 28 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/server/src/main/java/org/apache/druid/server/security/PreResponseAuthorizationCheckFilter.java b/server/src/main/java/org/apache/druid/server/security/PreResponseAuthorizationCheckFilter.java
index d0f543a06d..97d3e05a5f 100644
--- a/server/src/main/java/org/apache/druid/server/security/PreResponseAuthorizationCheckFilter.java
+++ b/server/src/main/java/org/apache/druid/server/security/PreResponseAuthorizationCheckFilter.java
@@ -179,21 +179,10 @@ public class PreResponseAuthorizationCheckFilter implements Filter
 
   private static boolean statusShouldBeHidden(int status)
   {
-    // We allow 404s (not found) to not be rewritten to forbidden because consistently returning 404s is a way to leak
-    // less information when something wasn't able to be done anyway.  I.e. if we pretend that the thing didn't exist
-    // when the authorization fails, then there is no information about whether the thing existed.  If we return
-    // a 403 when authorization fails and a 404 when authorization succeeds, but it doesn't exist, then we have
-    // leaked that it could maybe exist, if the authentication credentials were good.
-    //
-    // We also allow 307s (temporary redirect) to not be hidden as they are used to redirect to the leader.
-    switch (status) {
-      case HttpServletResponse.SC_FORBIDDEN:
-      case HttpServletResponse.SC_NOT_FOUND:
-      case HttpServletResponse.SC_TEMPORARY_REDIRECT:
-        return false;
-      default:
-        return true;
-    }
+    // Hide any 200s because a 200 response could contain stuff that we don't want seen, so we hide that.  It's also
+    // possible that errors can leak information, but that's something we cannot truly fix here.  We choose to let
+    // those error messages through because this filter values giving the user good feedback instead.
+    return status / 100 == 2;
   }
 
   public static void sendJsonError(HttpServletResponse resp, int error, String errorJson, OutputStream outputStream)
diff --git a/server/src/test/java/org/apache/druid/server/http/security/PreResponseAuthorizationCheckFilterTest.java b/server/src/test/java/org/apache/druid/server/http/security/PreResponseAuthorizationCheckFilterTest.java
index b8b2f21fc4..2cb4361bc2 100644
--- a/server/src/test/java/org/apache/druid/server/http/security/PreResponseAuthorizationCheckFilterTest.java
+++ b/server/src/test/java/org/apache/druid/server/http/security/PreResponseAuthorizationCheckFilterTest.java
@@ -106,6 +106,34 @@ public class PreResponseAuthorizationCheckFilterTest
     Assert.assertEquals(403, resp.getStatus());
   }
 
+  @Test
+  public void testMissingAuthorizationCheck401ResponseAndNotCommitted() throws ServletException, IOException
+  {
+    EmittingLogger.registerEmitter(new NoopServiceEmitter());
+
+    AuthenticationResult authenticationResult = new AuthenticationResult("so-very-valid", "so-very-valid", null, null);
+
+    MockHttpServletRequest req = new MockHttpServletRequest();
+    req.requestUri = "uri";
+    req.method = "GET";
+    req.remoteAddr = "1.2.3.4";
+    req.remoteHost = "aHost";
+
+    MockHttpServletResponse resp = new MockHttpServletResponse();
+    resp.setStatus(401);
+
+    req.attributes.put(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+
+    PreResponseAuthorizationCheckFilter filter = new PreResponseAuthorizationCheckFilter(
+        authenticators,
+        new DefaultObjectMapper()
+    );
+    filter.doFilter(req, resp, (request, response) -> {
+    });
+
+    Assert.assertEquals(401, resp.getStatus());
+  }
+
   @Test
   public void testMissingAuthorizationCheckWithForbidden() throws Exception
   {


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org