Introducing ClientRequestFilter.java: A New Plugin for Applying Request Headers in the Authentication Filter Class #23380

SthuthiGhosh9400 · 2024-08-05T12:04:17Z

Description

Created an interface (RequestModifier.java) in Presto to add additional headers in request.
Setting extra credential in request header is a use case.

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes

* Add  `RequestModifier.java` interface in Presto-spi :pr:`23380`
* Improve Request Headers in the Authentication Filter Class :pr:`23380`

…ng the plugin in authentication filter.

tdcmeehan

This PR needs some test cases.

steveburnett · 2024-08-26T14:31:07Z

Nit rephrasing of release note entries following the Order of changes in the Release Notes Guidelines.

== RELEASE NOTES ==

General Changes

* Add  `RequestModifier.java` interface in Presto-spi :pr:`23380`
* Improve Request Headers in the Authentication Filter Class :pr:`23380`

SthuthiGhosh9400 · 2024-08-30T04:34:18Z

@tdcmeehan

I have added a test case to verify setting values in the request header and committed it.
Could you have a check and confirm ?
Thanks.

tdcmeehan · 2024-08-30T13:03:29Z

presto-main/pom.xml

@@ -499,6 +499,11 @@
            <artifactId>ratis-common</artifactId>
            <optional>true</optional>
        </dependency>
+        <dependency>


We do not use Mockito in this project. Can you please refactor your code to avoid its use?

@tdcmeehan
Instead of using Mockito to create mock objects and define their behavior, can I manually write simple classes (stubs) that simulate the behavior of the actual classes?

Do you have any suggestion here in order to verify the codebase if value correctly passed to request header?

Yes, that is how we go about this.

@tdcmeehan
I have refactored the code that was added for the test case.

tdcmeehan · 2024-09-05T19:38:53Z

presto-main/src/test/java/com/facebook/presto/TestRequestHeaderModifier.java

+        assertEquals("CustomValue", wrappedRequest.getHeader("X-Custom-Header"));
+    }
+
+    abstract static class HttpServletRequestAdapter


There's already a MockHttpServletRequest, can you use that?

@tdcmeehan
I have refactored the code to use MockHttpServletRequest. Could you check it?
Thanks.

tdcmeehan · 2024-09-06T14:06:40Z

While we don't use Mockito, the stubs in the test are a little out of control. We need to reduce the size of this simple test. I have a couple of suggestions that you can research to make the testing simpler.

Can you look at TestHttpRemoteTask and examine how they implemented an in-memory endpoint for the purposes of testing? Perhaps you can build something similar.
TestingPrestoServer has all sorts of helper methods to retrieve individual classes from Presto. Perhaps you can add a new getter to retrieve the RequestModifierManager, register a simple new request modifier that just adds a dummy header, and make a client call that proves the header is added.

SthuthiGhosh9400 · 2024-09-08T13:22:51Z

TestingPrestoServer has all sorts of helper methods to retrieve individual classes from Presto. Perhaps you can add a new getter to retrieve the RequestModifierManager, register a simple new request modifier that just adds a dummy header, and make a client call that proves the header is added.

@tdcmeehan
I have attempted to improve the test case based on the suggestion. Could you have a check?
Thanks.

tdcmeehan · 2024-09-09T13:34:40Z

presto-main/src/main/java/com/facebook/presto/server/testing/TestingPrestoServer.java

@@ -825,4 +830,25 @@ private static int driftServerPort(DriftServer server)
    {
        return ((DriftNettyServerTransport) server.getServerTransport()).getPort();
    }
+
+    public RequestModifierManager getRequestModifierManager()


Please, just add a method to return the RequestModifierManager, and let users inject whatever custom modifier they wish.

tdcmeehan · 2024-09-09T13:36:44Z

presto-main/src/main/java/com/facebook/presto/RequestModifierManager.java

+
+public class RequestModifierManager
+{
+    private final List<RequestModifier> requestModifiers;


This needs to be thread safe. I recommend using a CopyOnWriteArrayList.

tdcmeehan · 2024-09-09T14:08:20Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

            // authentication succeeded
-            nextFilter.doFilter(withPrincipal(request, principal), response);
+            CustomHttpServletRequestWrapper wrappedRequest = withPrincipal(request, principal);


Instead of modifying the authentication filter, this should just go in its own filter that is applied after the authentication filter.

tdcmeehan · 2024-09-09T14:08:37Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+            this.customHeaders = new HashMap<>();
+        }
+
+        public void addHeader(String name, String value)


tdcmeehan · 2024-09-09T14:09:24Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+    public static class CustomHttpServletRequestWrapper
+            extends HttpServletRequestWrapper
+    {
+        private final Map<String, String> customHeaders;


Please use a concurrent map

tdcmeehan · 2024-09-09T14:09:59Z

presto-spi/src/main/java/com/facebook/presto/spi/RequestModifier.java

+import java.util.Map;
+import java.util.Optional;
+
+public interface RequestModifier


Let's call this something more descriptive, such as ClientRequestFilter.

- Improved code flow based on the review comments

SthuthiGhosh9400 · 2024-09-10T04:38:48Z

@tdcmeehan
I have incorporated the review comments. Could you please verify them?
Thanks.

tdcmeehan · 2024-09-10T12:57:00Z

presto-main/src/main/java/com/facebook/presto/ClientRequestFilterManager.java

+
+public class ClientRequestFilterManager
+{
+    private final CopyOnWriteArrayList<ClientRequestFilter> clientRequestFilters;


Suggested change

private final CopyOnWriteArrayList<ClientRequestFilter> clientRequestFilters;

private final List<ClientRequestFilter> clientRequestFilters;

tdcmeehan · 2024-09-10T12:57:21Z

presto-main/src/main/java/com/facebook/presto/ClientRequestFilterManager.java

+    private final CopyOnWriteArrayList<ClientRequestFilter> clientRequestFilters;
+    public ClientRequestFilterManager()
+    {
+        this.clientRequestFilters = new CopyOnWriteArrayList<>();
+    }


Suggested change

private final CopyOnWriteArrayList<ClientRequestFilter> clientRequestFilters;

public ClientRequestFilterManager()

{

this.clientRequestFilters = new CopyOnWriteArrayList<>();

}

private final List<ClientRequestFilter> clientRequestFilters = new CopyOnWriteArrayList<>();

tdcmeehan · 2024-09-10T12:57:50Z

presto-main/src/main/java/com/facebook/presto/ClientRequestFilterManager.java

+
+    public List<ClientRequestFilter> getClientRequestFilters()
+    {
+        return Collections.unmodifiableList(clientRequestFilters);


Suggested change

return Collections.unmodifiableList(clientRequestFilters);

return ImmutableList.copyOf(clientRequestFilters);

tdcmeehan · 2024-09-10T13:09:42Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+                    extraHeaderValueMap.ifPresent(map -> {
+                        for (Map.Entry<String, String> extraHeaderEntry : map.entrySet()) {
+                            if (request.getHeader(extraHeaderEntry.getKey()) == null) {
+                                extraHeadersMap.putIfAbsent(extraHeaderEntry.getKey(), extraHeaderEntry.getValue());


I don't think we should silently not put the header if there's an existing header. I think we should consider the following:

The SPI contract mandates that any potential header is known upfront.

The runtime will validate that the headers that are requested to be modified match with what is actually returned.

There is a blocklist of headers that the plugin should not modify (such as query ID).

The runtime will validate that all registered filters only return a disjoint set of headers being added. If there is a conflict, this is a fatal error that will require an administrator to fix (but un-registering one of these plugins).

If there is a conflict here, then an exception is thrown.

@tdcmeehan
I have gone through your comments and added a few validations in the current implementation. I can verify that with you. However, I have to know about the headers in the blocklist that should not be modified.
Could you help?

Let's start with the query ID, trace ID and transaction ID in PrestoHeaders.java.

I have incorporated the changes mentioned in your comment. Could you please review them and share your suggestions?

tdcmeehan · 2024-09-10T13:10:12Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+        private final Map<String, String> customHeaders;
+
+        public CustomHttpServletRequestWrapper(HttpServletRequest request)
+        {
+            super(request);
+            this.customHeaders = new ConcurrentHashMap<>();
+        }


Suggested change

private final Map<String, String> customHeaders;

public CustomHttpServletRequestWrapper(HttpServletRequest request)

{

super(request);

this.customHeaders = new ConcurrentHashMap<>();

}

private final Map<String, String> customHeaders = new ConcurrentHashMap<>();

tdcmeehan

Some feedback is still unadressed.

tdcmeehan · 2024-09-25T18:06:15Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+                        for (Map.Entry<String, String> extraHeaderEntry : map.entrySet()) {
+                            String headerKey = extraHeaderEntry.getKey();
+                            if (headersBlockList.contains(headerKey)) {
+                                throw new RuntimeException("Modification attempt detected: The header " + headerKey + " is present in the blocked headers list.");


Please throw a PrestoException and introduce a new error code for this. It should be a system error.

tdcmeehan · 2024-09-25T18:06:34Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+            while (originalHeaderNames.hasMoreElements()) {
+                headerNames.add(originalHeaderNames.nextElement());
+            }
+            return Collections.enumeration(headerNames);


Please use Immutable collections.

SthuthiGhosh9400 · 2024-09-26T15:27:23Z

Some feedback is still unadressed.

@tdcmeehan Could you help me identify which feedback is still pending? I believe I have addressed feedback items 2, 3, 4, and 5 from the list above.

tdcmeehan · 2024-09-26T15:55:23Z

Some feedback is still unadressed.

@tdcmeehan Could you help me identify which feedback is still pending? I believe I have addressed feedback items 2, 3, 4, and 5 from the list above.

Please scroll up in this PR and examine feedback on the code that is still remaining.

SthuthiGhosh9400 · 2024-09-26T16:20:52Z

Please scroll up in this PR and examine feedback on the code that is still remaining.

Yeah. @tdcmeehan I have incorporated all feedback items now. Kindly review them and share your suggestions?

Thanks.

tdcmeehan · 2024-10-07T19:08:25Z

presto-main/src/test/java/com/facebook/presto/TestClientRequestFilterPlugin.java

+        ClientRequestFilter customModifier = new ClientRequestFilter()
+        {
+            @Override
+            public List<String> getHeaderNames()
+            {
+                return Collections.singletonList("X-Custom-Header");
+            }
+            @Override
+            public <T> Optional<Map<String, String>> getExtraHeaders(T additionalInfo)
+            {
+                Map<String, String> headers = new HashMap<>();
+                headers.put("X-Custom-Header", "CustomValue_1");
+                return Optional.of(headers);
+            }
+        };
+
+        ClientRequestFilter customModifierConflict = new ClientRequestFilter()
+        {
+            @Override
+            public List<String> getHeaderNames()
+            {
+                return Collections.singletonList("X-Custom-Header");
+            }
+            @Override
+            public <T> Optional<Map<String, String>> getExtraHeaders(T additionalInfo)
+            {
+                Map<String, String> headers = new HashMap<>();
+                headers.put("X-Custom-Header", "CustomValue_2");
+                return Optional.of(headers);
+            }


Please reduce duplicate code. Consider an inner helper class.

tdcmeehan · 2024-10-07T19:09:12Z

presto-main/src/test/java/com/facebook/presto/TestClientRequestFilterPlugin.java

+    }
+
+    static class ConcreteHttpServletRequest
+            extends MockHttpServletRequest


Why not modify MockHttpServletRequest directly?

In ConcreteHttpServletRequest, I am storing additional headers in the customHeaders map and overriding the getHeaders method to manage them. This functionality isn't natively provided by MockHttpServletRequest, so extending it makes sense.

SthuthiGhosh9400 · 2024-10-08T20:06:55Z

@tdcmeehan
I have incorporated the review comments. Kindly review them.
If it looks good, I can squash the commits and edit the commit message according to the guidelines.

tdcmeehan

Please refactor tests to use the code under test.

tdcmeehan · 2024-10-09T14:54:41Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+                        for (Map.Entry<String, String> extraHeaderEntry : map.entrySet()) {
+                            String headerKey = extraHeaderEntry.getKey();
+                            if (headersBlockList.contains(headerKey)) {
+                                throw new PrestoException(HEADER_MODIFICATION_ATTEMPT, "Modification attempt detected: The header " + headerKey + " is present in the blocked headers list.");


Instead of mentioning a "blocked headers list", just list out the headers that are not allowed to be modified.

tdcmeehan · 2024-10-09T14:54:55Z

presto-main/src/main/java/com/facebook/presto/server/security/AuthenticationFilter.java

+                                throw new PrestoException(HEADER_MODIFICATION_ATTEMPT, "Modification attempt detected: The header " + headerKey + " is present in the blocked headers list.");
+                            }
+                            if (globallyAddedHeaders.contains(headerKey)) {
+                                throw new RuntimeException("Header conflict detected: " + headerKey + " already added by another filter.");


Do not throw RuntimeException, throw PrestoException.

tdcmeehan · 2024-10-09T14:58:04Z

presto-main/src/test/java/com/facebook/presto/TestClientRequestFilterPlugin.java

+                    for (Map.Entry<String, String> extraHeaderEntry : map.entrySet()) {
+                        String headerKey = extraHeaderEntry.getKey();
+                        if (headersBlockList.contains(headerKey)) {
+                            throw new RuntimeException("Modification attempt detected: The header " + headerKey + " is present in the blocked headers list.");


Why are we duplicating code here? I am very confused about how this test works.

SthuthiGhosh9400 · 2024-10-10T23:26:10Z

@tdcmeehan
I have refactored the test cases and used a separate method for adding headers to the request in the Authentication filter.
Could you review it and share your suggestions?

Added new interface for the plugin - Incorporated the changes for usi…

55c4278

…ng the plugin in authentication filter.

SthuthiGhosh9400 requested a review from a team as a code owner August 5, 2024 12:04

SthuthiGhosh9400 requested a review from presto-oss August 5, 2024 12:04

SthuthiGhosh9400 mentioned this pull request Aug 5, 2024

Introducing RequestModifier.java: A New Plugin for Modifying Request Headers in the Authentication Filter Class #23136

Closed

6 tasks

tdcmeehan self-assigned this Aug 5, 2024

tdcmeehan requested changes Aug 15, 2024

View reviewed changes

- Add a test case to check the modification of the request header.

fb5d812

tdcmeehan requested changes Aug 30, 2024

View reviewed changes

SthuthiGhosh9400 added 2 commits September 6, 2024 00:54

- Modified the test case to avoid the use of Mockito

0b6e62c

- Removed Mockito dependency and added mockwebserver

a3d875d

tdcmeehan requested changes Sep 5, 2024

View reviewed changes

- used MockHttpServletRequest

87c12ec

- Refactored the code according to the review comment

1506807

tdcmeehan requested changes Sep 9, 2024

View reviewed changes

SthuthiGhosh9400 added 2 commits September 10, 2024 10:02

- Renamed RequestModifier to ClientRequestFilter

0def3a4

- Improved code flow based on the review comments

Merge branch 'master' into request-modifier-plugin

5639435

SthuthiGhosh9400 changed the title ~~Introducing RequestModifier.java: A New Plugin for Modifying Request Headers in the Authentication Filter Class~~ Introducing ClientRequestFilter.java: A New Plugin for Applying Request Headers in the Authentication Filter Class Sep 10, 2024

tdcmeehan requested changes Sep 10, 2024

View reviewed changes

Incorporated review comments

57d0d17

tdcmeehan requested changes Sep 25, 2024

View reviewed changes

Incorporated review comments

7c28ec3

tdcmeehan requested changes Oct 7, 2024

View reviewed changes

incorporated review comments

dc15a70

Merge branch 'prestodb:master' into request-modifier-plugin

09a5031

tdcmeehan requested changes Oct 9, 2024

View reviewed changes

SthuthiGhosh9400 added 4 commits October 11, 2024 04:22

Refactored code for test cases

0c2092e

Renamed a method

3a2e4ed

Merge branch 'master' into request-modifier-plugin

dd1dd36

corrected checkstyle

77e4df0

	private final CopyOnWriteArrayList<ClientRequestFilter> clientRequestFilters;
	private final List<ClientRequestFilter> clientRequestFilters;

	return Collections.unmodifiableList(clientRequestFilters);
	return ImmutableList.copyOf(clientRequestFilters);

Introducing ClientRequestFilter.java: A New Plugin for Applying Request Headers in the Authentication Filter Class #23380

Are you sure you want to change the base?

Introducing ClientRequestFilter.java: A New Plugin for Applying Request Headers in the Authentication Filter Class #23380

Conversation

SthuthiGhosh9400 commented Aug 5, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

tdcmeehan left a comment

Choose a reason for hiding this comment

steveburnett commented Aug 26, 2024

SthuthiGhosh9400 commented Aug 30, 2024

Choose a reason for hiding this comment

SthuthiGhosh9400 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan commented Sep 6, 2024

SthuthiGhosh9400 commented Sep 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SthuthiGhosh9400 commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SthuthiGhosh9400 commented Sep 26, 2024

tdcmeehan commented Sep 26, 2024

SthuthiGhosh9400 commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SthuthiGhosh9400 commented Oct 8, 2024

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SthuthiGhosh9400 commented Oct 10, 2024

SthuthiGhosh9400 commented Aug 5, 2024 •

edited

Loading

SthuthiGhosh9400 Sep 4, 2024 •

edited

Loading