[GitHub] [pulsar] BewareMyPower commented on a change in pull request #14044: [broker][authentication]Support pass http auth status

[GitBox <gi...@apache.org>]

BewareMyPower commented on a change in pull request #14044:
URL: https://github.com/apache/pulsar/pull/14044#discussion_r800574348



##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -363,4 +369,59 @@ public boolean isExpired() {
             return expiration < System.currentTimeMillis();
         }
     }
+
+    private static final class TokenAuthenticationHttpState implements AuthenticationState {
+
+        private final AuthenticationProviderToken provider;
+        private AuthenticationDataSource authenticationDataSource;
+        private Jwt<?, Claims> jwt;
+        private long expiration;
+
+        TokenAuthenticationHttpState(AuthenticationProviderToken provider, HttpServletRequest request)
+                throws AuthenticationException {
+            this.provider = provider;
+            String httpHeaderValue = request.getHeader(HTTP_HEADER_NAME);
+            if (httpHeaderValue == null || !httpHeaderValue.startsWith(HTTP_HEADER_VALUE_PREFIX)) {
+                throw new AuthenticationException("Invalid HTTP Authorization header");
+            }
+
+            // Remove prefix
+            String token = httpHeaderValue.substring(HTTP_HEADER_VALUE_PREFIX.length());
+            this.jwt = provider.authenticateToken(token);
+            this.authenticationDataSource = new AuthenticationDataHttps(request);
+            if (jwt.getBody().getExpiration() != null) {
+                this.expiration = jwt.getBody().getExpiration().getTime();
+            } else {
+                // Disable expiration
+                this.expiration = Long.MAX_VALUE;
+            }
+        }
+
+        @Override
+        public String getAuthRole() throws AuthenticationException {
+            return provider.getPrincipal(jwt);
+        }
+
+        @Override
+        public AuthenticationDataSource getAuthDataSource() {
+            return authenticationDataSource;
+        }
+
+        @Override
+        public AuthData authenticate(AuthData authData) throws AuthenticationException {
+            return null;

Review comment:
       Could you explain why does `authenticate` return null here? If it's because `authenticate` could never be called, it's better to throw an exception here and explain the reason in comments.

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -363,4 +369,59 @@ public boolean isExpired() {
             return expiration < System.currentTimeMillis();
         }
     }
+
+    private static final class TokenAuthenticationHttpState implements AuthenticationState {
+
+        private final AuthenticationProviderToken provider;
+        private AuthenticationDataSource authenticationDataSource;
+        private Jwt<?, Claims> jwt;
+        private long expiration;

Review comment:
       ```suggestion
           private final AuthenticationDataSource authenticationDataSource;
           private final Jwt<?, Claims> jwt;
           private final long expiration;
   ```

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java
##########
@@ -76,8 +77,15 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
                 // not sasl type, return role directly.
                 String role = authenticationService.authenticateHttpRequest((HttpServletRequest) request);
                 request.setAttribute(AuthenticatedRoleAttributeName, role);
-                request.setAttribute(AuthenticatedDataAttributeName,
-                    new AuthenticationDataHttps((HttpServletRequest) request));
+                String authMethodName = httpRequest.getHeader("X-Pulsar-Auth-Method-Name");
+                if (authMethodName != null && authenticationService.getAuthenticationProvider(authMethodName) != null) {
+                    AuthenticationState authenticationState = authenticationService
+                            .getAuthenticationProvider(authMethodName).newHttpAuthState(httpRequest);
+                    request.setAttribute(AuthenticatedDataAttributeName, authenticationState.getAuthDataSource());

Review comment:
       @gaoran10 I think it's because here `request.setAttribute` accepts an `AuthenticationDataSource` rather than a `AuthenticationDataHttps`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org