[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4331: HIVE-27352: Support both LDAP and Kerberos Auth in HS2

["dengzhhu653 (via GitHub)" <gi...@apache.org>]

dengzhhu653 commented on code in PR #4331:
URL: https://github.com/apache/hive/pull/4331#discussion_r1206869688


##########
service/src/java/org/apache/hive/service/auth/AuthType.java:
##########
@@ -18,55 +18,78 @@
 
 package org.apache.hive.service.auth;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
+
 import org.apache.commons.lang3.EnumUtils;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.conf.HiveServer2TransportMode;
 
-import java.util.Arrays;
+import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * AuthType is used to parse and verify
  * {@link org.apache.hadoop.hive.conf.HiveConf.ConfVars#HIVE_SERVER2_AUTHENTICATION}.
  * Throws an exception if the config value is not allowed.
  */
 public class AuthType {
-  static final Set<HiveAuthConstants.AuthTypes> PASSWORD_BASED_TYPES = new HashSet<>(Arrays.asList(
-      HiveAuthConstants.AuthTypes.LDAP, HiveAuthConstants.AuthTypes.CUSTOM, HiveAuthConstants.AuthTypes.PAM));
+  static final Set<HiveAuthConstants.AuthTypes> PASSWORD_BASED_TYPES = ImmutableSet.of(HiveAuthConstants.AuthTypes.LDAP,
+      HiveAuthConstants.AuthTypes.CUSTOM, HiveAuthConstants.AuthTypes.PAM, HiveAuthConstants.AuthTypes.NONE);
   private final BitSet typeBits;
+  private final List<HiveAuthConstants.AuthTypes> authTypes;
+  private final HiveServer2TransportMode mode;
 
-  public AuthType(String authTypes) throws Exception {
+  @VisibleForTesting
+  public AuthType(String authTypes, HiveServer2TransportMode mode) {
+    this.authTypes = new ArrayList<>();
+    this.mode = mode;
     typeBits = new BitSet();
     parseTypes(authTypes);
     verifyTypes(authTypes);
   }
 
-  private void parseTypes(String authTypes) throws Exception {
+  private void parseTypes(String authTypes) {
     String[] types = authTypes.split(",");
     for (String type : types) {
       if (!EnumUtils.isValidEnumIgnoreCase(HiveAuthConstants.AuthTypes.class, type)) {
-        throw new Exception(type + " is not a valid authentication type.");
+        throw new IllegalArgumentException(type + " is not a valid authentication type.");
       }
-      typeBits.set(EnumUtils.getEnumIgnoreCase(HiveAuthConstants.AuthTypes.class, type).ordinal());
+      HiveAuthConstants.AuthTypes authType = EnumUtils.getEnumIgnoreCase(HiveAuthConstants.AuthTypes.class, type);
+      this.authTypes.add(authType);
+      typeBits.set(authType.ordinal());
     }
   }
 
-  private void verifyTypes(String authTypes) throws Exception {
+  private void verifyTypes(String authTypes) {
     if (typeBits.cardinality() == 1) {
       // single authentication type has no conflicts
       return;
     }
-    if ((typeBits.get(HiveAuthConstants.AuthTypes.SAML.ordinal()) || typeBits.get(HiveAuthConstants.AuthTypes.JWT.ordinal())) &&
-        !typeBits.get(HiveAuthConstants.AuthTypes.NOSASL.ordinal()) &&
-        !typeBits.get(HiveAuthConstants.AuthTypes.KERBEROS.ordinal()) &&
-        !typeBits.get(HiveAuthConstants.AuthTypes.NONE.ordinal()) &&
-        (!areAnyEnabled(PASSWORD_BASED_TYPES) || isExactlyOneEnabled(PASSWORD_BASED_TYPES))) {
-      // SAML can be enabled with another password based authentication types
-      return;
+    if (typeBits.get(HiveAuthConstants.AuthTypes.NOSASL.ordinal())) {
+      throw new UnsupportedOperationException("NOSASL can't be along with other auth methods: " + authTypes);
+    }
+
+    if (typeBits.get(HiveAuthConstants.AuthTypes.NONE.ordinal())) {
+      throw new UnsupportedOperationException("None can't be along with other auth methods: " + authTypes);
+    }
+
+    if (areAnyEnabled(PASSWORD_BASED_TYPES) && !isExactlyOneEnabled(PASSWORD_BASED_TYPES)) {

Review Comment:
   It's hard to tell which password-based type the client is using, as in HiveConnection we don't care about the specific type, instead it transmits the username/password directly to the server.
   binary: https://github.com/apache/hive/blob/master/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java#L982-L987
   http: https://github.com/apache/hive/blob/master/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java#L620-L627



-- 
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: gitbox-unsubscribe@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org