HBASE-26553. Address taklwu's comments

anmolnar · anmolnar · commit e8e41d9f1f3b · 2022-01-04T15:26:06.000+01:00
diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslClientCallbackHandlerTest.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslClientCallbackHandlerTest.java
@@ -68,7 +68,7 @@ public void testWithZeroTokens() {
     assertEquals(IOException.class, e.getCause().getClass());
   }
 
-  @Test()
+  @Test
   public void testWithPotentiallyMultipleTokens() throws Exception {
     OAuthBearerSaslClientAuthenticationProvider.OAuthBearerSaslClientCallbackHandler handler =
       createCallbackHandler();
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/exceptions/IllegalSaslStateException.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/exceptions/IllegalSaslStateException.java
@@ -36,5 +36,4 @@ public IllegalSaslStateException(String message) {
   public IllegalSaslStateException(String message, Throwable cause) {
     super(message, cause);
   }
-
 }
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/auth/AuthenticateCallbackHandler.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/auth/AuthenticateCallbackHandler.java
@@ -45,9 +45,4 @@ public interface AuthenticateCallbackHandler extends CallbackHandler {
    */
   default void configure(
     Configuration configs, String saslMechanism, Map<String, String> saslProps) {}
-
-  /**
-   * Closes this instance.
-   */
-  default void close() {}
 }
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerExtensionsValidatorCallback.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerExtensionsValidatorCallback.java
@@ -17,7 +17,7 @@
  */
 package org.apache.hadoop.hbase.security.oauthbearer;
 
-import static org.apache.hadoop.hbase.security.oauthbearer.Utils.subtractMap;
+import static org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerStringUtils.subtractMap;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerStringUtils.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerStringUtils.java
@@ -23,7 +23,7 @@
 import org.apache.yetus.audience.InterfaceAudience;
 
 @InterfaceAudience.Public
-public final class Utils {
+public final class OAuthBearerStringUtils {
   /**
    *  Converts a {@code Map} class into a string, concatenating keys and values
    *  Example:
@@ -76,16 +76,7 @@ public static <K, V> Map<K, V> subtractMap(Map<? extends K, ? extends V> minuend
       .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
   }
 
-  /**
-   * Checks if a string is null, empty or whitespace only.
-   * @param str a string to be checked
-   * @return true if the string is null, empty or whitespace only; otherwise, return false.
-   */
-  public static boolean isBlank(String str) {
-    return str == null || str.trim().isEmpty();
-  }
-
-  private Utils() {
+  private OAuthBearerStringUtils() {
     // empty
   }
 }
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallback.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallback.java
@@ -30,7 +30,7 @@
  * 2.0 Authorization Framework</a>. Callback handlers should communicate other
  * problems by raising an {@code IOException}.
  * <p>
- * This class was introduced in 2.0.0 and, while it feels stable, it could
+ * This class was introduced in 3.0.0 and, while it feels stable, it could
  * evolve. We will try to evolve the API in a compatible manner, but we reserve
  * the right to make breaking changes in minor releases, if necessary. We will
  * update the {@code InterfaceStability} annotation and this notice once the API
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerClientInitialResponse.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerClientInitialResponse.java
@@ -24,7 +24,7 @@
 import java.util.regex.Pattern;
 import javax.security.sasl.SaslException;
 import org.apache.hadoop.hbase.security.auth.SaslExtensions;
-import org.apache.hadoop.hbase.security.oauthbearer.Utils;
+import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerStringUtils;
 import org.apache.yetus.audience.InterfaceAudience;
 
 /**
@@ -64,7 +64,7 @@ public OAuthBearerClientInitialResponse(byte[] response) throws SaslException {
     String authzid = matcher.group("authzid");
     this.authorizationId = authzid == null ? "" : authzid;
     String kvPairs = matcher.group("kvpairs");
-    Map<String, String> properties = Utils.parseMap(kvPairs, "=", SEPARATOR);
+    Map<String, String> properties = OAuthBearerStringUtils.parseMap(kvPairs, "=", SEPARATOR);
     String auth = properties.get(AUTH_KEY);
     if (auth == null) {
       throw new SaslException("Invalid OAUTHBEARER client first message: 'auth' not specified");
@@ -207,6 +207,6 @@ public static void validateExtensions(SaslExtensions extensions) throws SaslExce
    * Converts the SASLExtensions to an OAuth protocol-friendly string
    */
   private String extensionsMessage() {
-    return Utils.mkString(saslExtensions.map(), "", "", "=", SEPARATOR);
+    return OAuthBearerStringUtils.mkString(saslExtensions.map(), "", "", "=", SEPARATOR);
   }
 }
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerSaslServer.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerSaslServer.java
@@ -36,7 +36,7 @@
 import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerExtensionsValidatorCallback;
 import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerToken;
 import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerValidatorCallback;
-import org.apache.hadoop.hbase.security.oauthbearer.Utils;
+import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerStringUtils;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -103,7 +103,7 @@ public byte[] evaluateResponse(byte[] response)
       return process(clientResponse.tokenValue(), clientResponse.authorizationId(),
         clientResponse.extensions());
     } catch (SaslAuthenticationException e) {
-      LOG.error("SASL authentication error: {}", e.getMessage());
+      LOG.error("SASL authentication error", e);
       throw e;
     } catch (Exception e) {
       LOG.error("SASL server problem", e);
@@ -215,7 +215,7 @@ private Map<String, String> processExtensions(OAuthBearerToken token, SaslExtens
     if (!extensionsCallback.invalidExtensions().isEmpty()) {
       String errorMessage = String.format("Authentication failed: %d extensions are invalid! "
           + "They are: %s", extensionsCallback.invalidExtensions().size(),
-        Utils.mkString(extensionsCallback.invalidExtensions(), "", "", ": ", "; "));
+        OAuthBearerStringUtils.mkString(extensionsCallback.invalidExtensions(), "", "", ": ", "; "));
       LOG.debug(errorMessage);
       throw new SaslAuthenticationException(errorMessage);
     }
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/knox/OAuthBearerSignedJwt.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/knox/OAuthBearerSignedJwt.java
@@ -37,8 +37,8 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerToken;
-import org.apache.hadoop.hbase.security.oauthbearer.Utils;
 import org.apache.yetus.audience.InterfaceAudience;
 
 /**
@@ -145,7 +145,7 @@ public OAuthBearerSignedJwt validate(){
       }
       lifetime = expirationTimeSeconds.toInstant().toEpochMilli();
       String principalName = claims.getSubject();
-      if (Utils.isBlank(principalName)) {
+      if (StringUtils.isBlank(principalName)) {
         throw new OAuthBearerIllegalTokenException(OAuthBearerValidationResult
           .newFailure("No principal name in JWT claim"));
       }
@@ -165,13 +165,13 @@ private JWTClaimsSet validateToken(String jwtToken)
     JWTClaimsSet.Builder jwtClaimsSetBuilder = new JWTClaimsSet.Builder();
 
     // Audience
-    if (!Utils.isBlank(requiredAudience)) {
+    if (!StringUtils.isBlank(requiredAudience)) {
       requiredClaims.add("aud");
       jwtClaimsSetBuilder.audience(requiredAudience);
     }
 
     // Issuer
-    if (!Utils.isBlank(requiredIssuer)) {
+    if (!StringUtils.isBlank(requiredIssuer)) {
       requiredClaims.add("iss");
       jwtClaimsSetBuilder.issuer(requiredIssuer);
     }
diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/knox/OAuthBearerSignedJwtValidatorCallbackHandler.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/knox/OAuthBearerSignedJwtValidatorCallbackHandler.java
@@ -27,11 +27,11 @@
 import java.util.Objects;
 import javax.security.auth.callback.Callback;
 import javax.security.auth.callback.UnsupportedCallbackException;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.security.auth.AuthenticateCallbackHandler;
 import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerExtensionsValidatorCallback;
 import org.apache.hadoop.hbase.security.oauthbearer.OAuthBearerValidatorCallback;
-import org.apache.hadoop.hbase.security.oauthbearer.Utils;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -91,7 +91,7 @@ public class OAuthBearerSignedJwtValidatorCallbackHandler implements Authenticat
   public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
     if (!configured) {
       throw new RuntimeException(
-        "OAuthBearerSignedJwtValidatorCallbackHandler handler be configured first.");
+        "OAuthBearerSignedJwtValidatorCallbackHandler must be configured first.");
     }
 
     for (Callback callback : callbacks) {
@@ -171,7 +171,7 @@ private int allowableClockSkewSeconds() {
       ALLOWABLE_CLOCK_SKEW_SECONDS_OPTION);
     int allowableClockSkewSeconds = 0;
     try {
-      allowableClockSkewSeconds = Utils.isBlank(allowableClockSkewSecondsValue)
+      allowableClockSkewSeconds = StringUtils.isBlank(allowableClockSkewSecondsValue)
         ? 0 : Integer.parseInt(allowableClockSkewSecondsValue.trim());
     } catch (NumberFormatException e) {
       throw new OAuthBearerConfigException(e.getMessage(), e);
@@ -188,12 +188,12 @@ private void loadJwkSet() throws IOException, ParseException {
     String jwksFile = hBaseConfiguration.get(JWKS_FILE);
     String jwksUrl = hBaseConfiguration.get(JWKS_URL);
 
-    if (Utils.isBlank(jwksFile) && Utils.isBlank(jwksUrl)) {
+    if (StringUtils.isBlank(jwksFile) && StringUtils.isBlank(jwksUrl)) {
       throw new RuntimeException("Failed to initialize JWKS db. "
         + JWKS_FILE + " or " + JWKS_URL + " must be specified in the config.");
     }
 
-    if (!Utils.isBlank(jwksFile)) {
+    if (!StringUtils.isBlank(jwksFile)) {
       this.jwkSet = JWKSet.load(new File(jwksFile));
       LOG.debug("JWKS db initialized from file: {}", jwksFile);
       return;
diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallbackTest.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallbackTest.java
@@ -20,9 +20,20 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.ClassRule;
 import org.junit.Test;
+import org.junit.experimental.categories.Category;
 
+@Category({ MiscTests.class, SmallTests.class})
 public class OAuthBearerTokenCallbackTest {
+
+  @ClassRule
+  public static final HBaseClassTestRule CLASS_RULE =
+    HBaseClassTestRule.forClass(OAuthBearerTokenCallbackTest.class);
+
   private static final OAuthBearerToken TOKEN = new OAuthBearerToken() {
     @Override
     public String value() {
diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerValidatorCallbackTest.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerValidatorCallbackTest.java
@@ -22,6 +22,7 @@
 import static org.junit.Assert.assertSame;
 import org.junit.Test;
 
+
 public class OAuthBearerValidatorCallbackTest {
   private static final OAuthBearerToken TOKEN = new OAuthBearerToken() {
     @Override
diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerSaslClientTest.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerSaslClientTest.java
@@ -83,10 +83,6 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
         }
       }
     }
-
-    @Override
-    public void close() {
-    }
   }
 
   @Test
diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/util/ClassLoaderTestHelper.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/util/ClassLoaderTestHelper.java
@@ -19,7 +19,6 @@
 
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
-
 import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileInputStream;
@@ -35,7 +34,6 @@
 import javax.tools.JavaFileObject;
 import javax.tools.StandardJavaFileManager;
 import javax.tools.ToolProvider;
-
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
 import org.slf4j.Logger;
diff --git a/hbase-examples/src/main/java/org/apache/hadoop/hbase/jwt/client/example/JwtClientExample.java b/hbase-examples/src/main/java/org/apache/hadoop/hbase/jwt/client/example/JwtClientExample.java
@@ -50,12 +50,17 @@
 @InterfaceAudience.Private
 public class JwtClientExample extends Configured implements Tool {
   private static final Logger LOG = LoggerFactory.getLogger(JwtClientExample.class);
-  private static final String jwt = "<base64_encoded_jwt_token>";
+  private static final String JWT_TOKEN = "<base64_encoded_jwt_token>";
 
   private static final byte[] FAMILY = Bytes.toBytes("d");
 
   public JwtClientExample() {
-    setConf(HBaseConfiguration.create());
+    Configuration conf = HBaseConfiguration.create();
+    conf.set("hbase.client.sasl.provider.class",
+      "org.apache.hadoop.hbase.security.provider.OAuthBearerSaslProviderSelector");
+    conf.set("hbase.client.sasl.provider.extras",
+      "org.apache.hadoop.hbase.security.provider.OAuthBearerSaslClientAuthenticationProvider");
+    setConf(conf);
   }
 
   @Override public int run(String[] args) throws Exception {
@@ -66,7 +71,7 @@ public JwtClientExample() {
     UserProvider provider = UserProvider.instantiate(conf);
     User user = provider.getCurrent();
 
-    OAuthBearerTokenUtil.addTokenForUser(user, jwt);
+    OAuthBearerTokenUtil.addTokenForUser(user, JWT_TOKEN);
     LOG.info("JWT token added");
 
     try (final Connection conn = ConnectionFactory.createConnection(conf, user)) {

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ public void testWithZeroTokens() {`
`68`	`68`	`assertEquals(IOException.class, e.getCause().getClass());`
`69`	`69`	`}`
`70`	`70`
`71`		`- @Test()`
	`71`	`+ @Test`
`72`	`72`	`public void testWithPotentiallyMultipleTokens() throws Exception {`
`73`	`73`	`OAuthBearerSaslClientAuthenticationProvider.OAuthBearerSaslClientCallbackHandler handler =`
`74`	`74`	`createCallbackHandler();`
Original file line number	Diff line number	Diff line change
`@@ -36,5 +36,4 @@ public IllegalSaslStateException(String message) {`
`36`	`36`	`public IllegalSaslStateException(String message, Throwable cause) {`
`37`	`37`	`super(message, cause);`
`38`	`38`	`}`
`39`		`-`
`40`	`39`	`}`