DRILL-6088: MainLoginPageModel errors out when http.auth.mechanisms i…

…s not configured

DRILL-6088: Update based on review feedback

close #1092

distribution/src/resources/drill-override-example.conf

@@ -91,7 +91,16 @@ drill.exec: {
       allowedMethods: ["GET", "POST", "HEAD", "OPTIONS"],
       allowedHeaders: ["X-Requested-With", "Content-Type", "Accept", "Origin"],
       credentials: true
-    }
+    },
+    auth: {
+        # Http Auth mechanisms to configure. If not provided but user.auth is enabled
+        # then default value is FORM.
+        mechanisms: ["FORM", "SPNEGO"],
+        # Spnego principal to be used by WebServer when Spnego authentication is enabled.
+        spnego.principal: "HTTP://<localhost>"
+        # Location to keytab file for above spnego principal
+        spnego.keytab: "<keytab_file_location>";
+    },
   },
   # Below SSL parameters need to be set for custom transport layer settings.
   ssl: {

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogInLogOutResources.java

@@ -17,11 +17,12 @@
  */
 package org.apache.drill.exec.server.rest;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.drill.common.config.DrillConfig;
 import org.apache.drill.exec.ExecConstants;
-import org.apache.drill.exec.rpc.security.AuthStringUtil;
 import org.apache.drill.exec.server.rest.auth.AuthDynamicFeature;
+import org.apache.drill.exec.server.rest.auth.DrillHttpSecurityHandlerProvider;
 import org.apache.drill.exec.work.WorkManager;
 import org.eclipse.jetty.security.authentication.FormAuthenticator;
 import org.eclipse.jetty.util.security.Constraint;
@@ -57,8 +58,9 @@ public class LogInLogOutResources {
   /**
    * Update the destination URI to be redirect URI if specified in the request URL so that after the login is
    * successful, request is forwarded to redirect page.
+   *
    * @param redirect - Redirect parameter in the request URI
-   * @param request - Http Servlet Request
+   * @param request  - Http Servlet Request
    * @throws Exception
    */
   private void updateSessionRedirectInfo(String redirect, HttpServletRequest request) throws Exception {
@@ -100,8 +102,7 @@ public Viewable getSpnegoLogin(@Context HttpServletRequest request, @Context Htt
     }
 
     final String errorString = "Invalid SPNEGO credentials or SPNEGO is not configured";
-    final DrillConfig drillConfig = workManager.getContext().getConfig();
-    MainLoginPageModel model = new MainLoginPageModel(errorString, drillConfig);
+    final MainLoginPageModel model = new MainLoginPageModel(errorString);
     return ViewableWithPermissions.createMainLoginPage(model);
   }
 
@@ -132,24 +133,27 @@ public Viewable getMainLoginPage(@Context HttpServletRequest request, @Context H
                                    @Context SecurityContext sc, @Context UriInfo uriInfo,
                                    @QueryParam(WebServerConstants.REDIRECT_QUERY_PARM) String redirect) throws Exception {
     updateSessionRedirectInfo(redirect, request);
-    final DrillConfig drillConfig = workManager.getContext().getConfig();
-    MainLoginPageModel model = new MainLoginPageModel(null, drillConfig);
+    final MainLoginPageModel model = new MainLoginPageModel(null);
     return ViewableWithPermissions.createMainLoginPage(model);
   }
 
-  private class MainLoginPageModel {
+  /**
+   * This class should be public for it's method's to be accessible by mainLogin.ftl file
+   */
+  @VisibleForTesting
+  public class MainLoginPageModel {
 
     private final String error;
 
     private final boolean authEnabled;
 
     private final Set<String> configuredMechs;
 
-    MainLoginPageModel(String error, DrillConfig drillConfig) {
+    MainLoginPageModel(String error) {
       this.error = error;
-      authEnabled = drillConfig.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
-      configuredMechs = AuthStringUtil.asSet(
-          drillConfig.getStringList(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS));
+      final DrillConfig config = workManager.getContext().getConfig();
+      authEnabled = config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
+      configuredMechs = DrillHttpSecurityHandlerProvider.getHttpAuthMechanisms(config);
     }
 
     public boolean isSpnegoEnabled() {

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillHttpSecurityHandlerProvider.java

@@ -58,51 +58,46 @@ public DrillHttpSecurityHandlerProvider(DrillConfig config, DrillbitContext dril
       throws DrillbitStartupException {
 
     Preconditions.checkState(config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED));
-    final Set<String> configuredMechanisms = new HashSet<>();
+    final Set<String> configuredMechanisms = getHttpAuthMechanisms(config);
 
-    if (config.hasPath(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS)) {
-      configuredMechanisms.addAll(AuthStringUtil.asSet(config.getStringList(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS)));
-    } else { // for backward compatibility
-      configuredMechanisms.add(Constraint.__FORM_AUTH);
-    }
+    final ScanResult scan = drillContext.getClasspathScan();
+    final Collection<Class<? extends DrillHttpConstraintSecurityHandler>> factoryImpls =
+        scan.getImplementations(DrillHttpConstraintSecurityHandler.class);
+    logger.debug("Found DrillHttpConstraintSecurityHandler implementations: {}", factoryImpls);
 
-      final ScanResult scan = drillContext.getClasspathScan();
-      final Collection<Class<? extends DrillHttpConstraintSecurityHandler>> factoryImpls =
-          scan.getImplementations(DrillHttpConstraintSecurityHandler.class);
-      logger.debug("Found DrillHttpConstraintSecurityHandler implementations: {}", factoryImpls);
-      for (final Class<? extends DrillHttpConstraintSecurityHandler> clazz : factoryImpls) {
+    for (final Class<? extends DrillHttpConstraintSecurityHandler> clazz : factoryImpls) {
 
-        // If all the configured mechanisms handler is added then break out of this loop
-        if (configuredMechanisms.isEmpty()) {
-          break;
-        }
+      // If all the configured mechanisms handler is added then break out of this loop
+      if (configuredMechanisms.isEmpty()) {
+        break;
+      }
 
-        Constructor<? extends DrillHttpConstraintSecurityHandler> validConstructor = null;
-        for (final Constructor<?> c : clazz.getConstructors()) {
-          final Class<?>[] params = c.getParameterTypes();
-          if (params.length == 0) {
-            validConstructor = (Constructor<? extends DrillHttpConstraintSecurityHandler>) c; // unchecked
-            break;
-          }
+      Constructor<? extends DrillHttpConstraintSecurityHandler> validConstructor = null;
+      for (final Constructor<?> c : clazz.getConstructors()) {
+        final Class<?>[] params = c.getParameterTypes();
+        if (params.length == 0) {
+          validConstructor = (Constructor<? extends DrillHttpConstraintSecurityHandler>) c; // unchecked
+          break;
         }
+      }
 
-        if (validConstructor == null) {
-          logger.warn("Skipping DrillHttpConstraintSecurityHandler class {}. It must implement at least one" +
-              " constructor with signature [{}()]", clazz.getCanonicalName(), clazz.getName());
-          continue;
-        }
+      if (validConstructor == null) {
+        logger.warn("Skipping DrillHttpConstraintSecurityHandler class {}. It must implement at least one" +
+            " constructor with signature [{}()]", clazz.getCanonicalName(), clazz.getName());
+        continue;
+      }
 
-        try {
-          final DrillHttpConstraintSecurityHandler instance = validConstructor.newInstance();
-          if (configuredMechanisms.remove(instance.getImplName())) {
-            instance.doSetup(drillContext);
-            securityHandlers.put(instance.getImplName(), instance);
-          }
-        } catch (IllegalArgumentException | ReflectiveOperationException | DrillException e) {
-          logger.warn(String.format("Failed to create DrillHttpConstraintSecurityHandler of type '%s'",
-              clazz.getCanonicalName()), e);
+      try {
+        final DrillHttpConstraintSecurityHandler instance = validConstructor.newInstance();
+        if (configuredMechanisms.remove(instance.getImplName())) {
+          instance.doSetup(drillContext);
+          securityHandlers.put(instance.getImplName(), instance);
         }
+      } catch (IllegalArgumentException | ReflectiveOperationException | DrillException e) {
+        logger.warn(String.format("Failed to create DrillHttpConstraintSecurityHandler of type '%s'",
+            clazz.getCanonicalName()), e);
       }
+    }
 
     if (securityHandlers.size() == 0) {
       throw new DrillbitStartupException("Authentication is enabled for WebServer but none of the security mechanism " +
@@ -181,4 +176,26 @@ public boolean isSpnegoEnabled() {
   public boolean isFormEnabled() {
     return securityHandlers.containsKey(Constraint.__FORM_AUTH);
   }
+
+  /**
+   * Return's list of configured mechanisms for HTTP authentication. For backward
+   * compatibility if authentication is enabled it will include FORM mechanism by default.
+   * @param config - {@link DrillConfig}
+   * @return
+   */
+  public static Set<String> getHttpAuthMechanisms(DrillConfig config) {
+    final Set<String> configuredMechs = new HashSet<>();
+    final boolean authEnabled = config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
+
+    if (authEnabled) {
+      if (config.hasPath(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS)) {
+        configuredMechs.addAll(
+            AuthStringUtil.asSet(config.getStringList(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS)));
+      } else {
+        // For backward compatibility
+        configuredMechs.add(Constraint.__FORM_AUTH);
+      }
+    }
+    return configuredMechs;
+  }
 }

exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/TestMainLoginPageModel.java

@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest;
+
+import com.google.common.collect.Lists;
+import com.typesafe.config.ConfigValueFactory;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.server.DrillbitContext;
+import org.apache.drill.exec.server.rest.LogInLogOutResources.MainLoginPageModel;
+import org.apache.drill.exec.work.WorkManager;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test for {@link LogInLogOutResources.MainLoginPageModel} with various configurations done in DrillConfig
+ */
+public class TestMainLoginPageModel {
+
+  @Mock
+  WorkManager workManager;
+
+  @Mock
+  DrillbitContext context;
+
+  @InjectMocks
+  LogInLogOutResources logInLogOutResources = new LogInLogOutResources();
+
+  @Before
+  public void setup() {
+    MockitoAnnotations.initMocks(this);
+    when(workManager.getContext()).thenReturn(context);
+  }
+
+  /**
+   * Test when auth is disabled then both Form and Spnego authentication is disabled.
+   */
+  @Test
+  public void testAuthDisabled() {
+    final DrillConfig config = DrillConfig.create();
+    when(context.getConfig()).thenReturn(config);
+    final MainLoginPageModel model = logInLogOutResources.new MainLoginPageModel(null);
+    assertTrue(!model.isFormEnabled());
+    assertTrue(!model.isSpnegoEnabled());
+  }
+
+  /**
+   * Test when auth is enabled with no http.auth.mechanisms configured then by default Form authentication is
+   * enabled but Spnego is disabled.
+   */
+  @Test
+  public void testAuthEnabledWithNoMech() {
+    final DrillConfig config = new DrillConfig(DrillConfig.create()
+      .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED,
+        ConfigValueFactory.fromAnyRef(true)), false);
+    when(context.getConfig()).thenReturn(config);
+    final MainLoginPageModel model = logInLogOutResources.new MainLoginPageModel(null);
+    assertTrue(model.isFormEnabled());
+    assertTrue(!model.isSpnegoEnabled());
+  }
+
+  /**
+   * Test when auth is enabled with http.auth.mechanisms configured as Form then only Form authentication is
+   * enabled but Spnego is disabled.
+   */
+  @Test
+  public void testAuthEnabledWithForm() {
+    final DrillConfig config = new DrillConfig(DrillConfig.create()
+      .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED,
+        ConfigValueFactory.fromAnyRef(true))
+      .withValue(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS,
+        ConfigValueFactory.fromIterable(Lists.newArrayList("form"))), false);
+    when(context.getConfig()).thenReturn(config);
+    final MainLoginPageModel model = logInLogOutResources.new MainLoginPageModel(null);
+    assertTrue(model.isFormEnabled());
+    assertTrue(!model.isSpnegoEnabled());
+  }
+
+  /**
+   * Test when auth is enabled with http.auth.mechanisms configured as Spnego then only Spnego authentication is
+   * enabled but Form is disabled.
+   */
+  @Test
+  public void testAuthEnabledWithSpnego() {
+    final DrillConfig config = new DrillConfig(DrillConfig.create()
+      .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED,
+        ConfigValueFactory.fromAnyRef(true))
+      .withValue(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS,
+        ConfigValueFactory.fromIterable(Lists.newArrayList("spnego"))), false);
+    when(context.getConfig()).thenReturn(config);
+    final MainLoginPageModel model = logInLogOutResources.new MainLoginPageModel(null);
+    assertTrue(!model.isFormEnabled());
+    assertTrue(model.isSpnegoEnabled());
+  }
+
+  /**
+   * Test when auth is enabled with http.auth.mechanisms configured as Form, Spnego then both Form and Spnego
+   * authentication are enabled.
+   */
+  @Test
+  public void testAuthEnabledWithFormSpnego() {
+    final DrillConfig config = new DrillConfig(DrillConfig.create()
+      .withValue(ExecConstants.USER_AUTHENTICATION_ENABLED,
+        ConfigValueFactory.fromAnyRef(true))
+      .withValue(ExecConstants.HTTP_AUTHENTICATION_MECHANISMS,
+        ConfigValueFactory.fromIterable(Lists.newArrayList("form", "spnego"))), false);
+    when(context.getConfig()).thenReturn(config);
+    final MainLoginPageModel model = logInLogOutResources.new MainLoginPageModel(null);
+    assertTrue(model.isFormEnabled());
+    assertTrue(model.isSpnegoEnabled());
+  }
+}