[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143910336
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java 
---
@@ -75,6 +80,29 @@ public ClusterInfo getClusterInfoJSON() {
 final DrillConfig config = dbContext.getConfig();
 final boolean userEncryptionEnabled = 
config.getBoolean(ExecConstants.USER_ENCRYPTION_SASL_ENABLED);
 final boolean bitEncryptionEnabled = 
config.getBoolean(ExecConstants.BIT_ENCRYPTION_SASL_ENABLED);
+// If the user is logged in and is admin user then show the admin user 
info
+// For all other cases the user info need-not or should-not be 
displayed
+OptionManager optionManager = work.getContext().getOptionManager();
+final boolean isUserLoggedIn = AuthDynamicFeature.isUserLoggedIn(sc);
+String adminUsers = isUserLoggedIn ?
+
ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager) : null;
+String adminUserGroups = isUserLoggedIn ?
+
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager) : 
null;
+
+// separate groups by comma + space
+if (adminUsers != null) {
+  String[] groups = adminUsers.split(",");
+  adminUsers = DrillStringUtils.join(groups, ", ");
+}
+
+// separate groups by comma + space
+if (adminUserGroups != null) {
+  String[] groups = adminUserGroups.split(",");
+  adminUserGroups = DrillStringUtils.join(groups, ", ");
--- End diff --

What if the user provided the list with spaces: "a, b,  c"? Actually, in 
the above, it is fine; HTML will compress the run of spaces to a single one for 
display...

But, what about where we do the real check? Do the unit tests cover this 
case?


---

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143910033
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptionsAuthEnabled.java
 ---
@@ -115,4 +124,59 @@ private void setOptHelper() throws Exception {
   test(String.format("ALTER SYSTEM SET `%s` = %d;", 
ExecConstants.SLICE_TARGET, ExecConstants.SLICE_TARGET_DEFAULT));
 }
   }
+
+  @Test
+  public void testAdminUserOptions() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder();
+
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+  OptionManager optionManager = 
cluster.drillbit().getContext().getOptionManager();
+
+  // Admin Users Tests
+  // config file should have the 'fake' default admin user and it 
should be returned
+  // by the option manager if the option has not been set by the user
+  String configAdminUser =  
optionManager.getOption(ExecConstants.ADMIN_USERS_VALIDATOR);;
--- End diff --

one semicolon should be enough...


---

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143910797
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptionsAuthEnabled.java
 ---
@@ -115,4 +124,59 @@ private void setOptHelper() throws Exception {
   test(String.format("ALTER SYSTEM SET `%s` = %d;", 
ExecConstants.SLICE_TARGET, ExecConstants.SLICE_TARGET_DEFAULT));
 }
   }
+
+  @Test
+  public void testAdminUserOptions() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder();
+
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+  OptionManager optionManager = 
cluster.drillbit().getContext().getOptionManager();
+
+  // Admin Users Tests
+  // config file should have the 'fake' default admin user and it 
should be returned
+  // by the option manager if the option has not been set by the user
+  String configAdminUser =  
optionManager.getOption(ExecConstants.ADMIN_USERS_VALIDATOR);;
+  assertEquals(configAdminUser, 
ExecConstants.ADMIN_USERS_VALIDATOR.DEFAULT_ADMIN_USERS);
+
+  // Option accessor should never return the 'fake' default from the 
config
+  String adminUser1 = 
ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+  assertNotEquals(adminUser1, 
ExecConstants.ADMIN_USERS_VALIDATOR.DEFAULT_ADMIN_USERS);
+
+  // Change TEST_ADMIN_USER if necessary
+  String TEST_ADMIN_USER = "ronswanson";
+  if (adminUser1.equals(TEST_ADMIN_USER)) {
+TEST_ADMIN_USER += "thefirst";
+  }
+  // Check if the admin option accessor honors a user-supplied values
+  String sql = String.format("ALTER SYSTEM SET `%s`='%s'", 
ExecConstants.ADMIN_USERS_KEY, TEST_ADMIN_USER);
+  client.queryBuilder().sql(sql).run();
+  String adminUser2 = 
ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+  assertEquals(adminUser2, TEST_ADMIN_USER);
+
+  // Admin User Groups Tests
+
+  // config file should have the 'fake' default admin user and it 
should be returned
+  // by the option manager if the option has not been set by the user
+  String configAdminUserGroups =  
optionManager.getOption(ExecConstants.ADMIN_USER_GROUPS_VALIDATOR);
+  assertEquals(configAdminUserGroups, 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.DEFAULT_ADMIN_USER_GROUPS);
+
+  // Option accessor should never return the 'fake' default from the 
config
+  String adminUserGroups1 = 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
+  assertNotEquals(adminUserGroups1, 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.DEFAULT_ADMIN_USER_GROUPS);
+
+  // Change TEST_ADMIN_USER_GROUPS if necessary
+  String TEST_ADMIN_USER_GROUPS = "yakshavers";
+  if (adminUserGroups1.equals(TEST_ADMIN_USER_GROUPS)) {
+TEST_ADMIN_USER_GROUPS += ",wormracers";
+  }
+  // Check if the admin option accessor honors a user-supplied values
+  sql = String.format("ALTER SYSTEM SET `%s`='%s'", 
ExecConstants.ADMIN_USER_GROUPS_KEY, TEST_ADMIN_USER_GROUPS);
+  client.queryBuilder().sql(sql).run();
+  String adminUserGroups2 = 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
+  assertEquals(adminUserGroups2, TEST_ADMIN_USER_GROUPS);
--- End diff --

Should the tests include actually checking if the user and/or group 
matches? This will check for problems such as spaces before/after the names, 
etc.


---

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143902334
  
--- Diff: 
common/src/main/java/org/apache/drill/common/util/DrillStringUtils.java ---
@@ -1,203 +1,258 @@
-/**
- * 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.common.util;
-
-import io.netty.buffer.ByteBuf;
-
-import org.apache.commons.lang3.StringEscapeUtils;
-import org.apache.commons.lang3.StringUtils;
-
-public class DrillStringUtils {
-
-  /**
-   * Converts the long number into more human readable string.
-   */
-  public static String readable(long bytes) {
-int unit = 1024;
-long absBytes = Math.abs(bytes);
-if (absBytes < unit) {
-  return bytes + " B";
-}
-int exp = (int) (Math.log(absBytes) / Math.log(unit));
-char pre = ("KMGTPE").charAt(exp-1);
-return String.format("%s%.1f %ciB", (bytes == absBytes ? "" : "-"), 
absBytes / Math.pow(unit, exp), pre);
-  }
-
-
-  /**
-   * Unescapes any Java literals found in the {@code String}.
-   * For example, it will turn a sequence of {@code '\'} and
-   * {@code 'n'} into a newline character, unless the {@code '\'}
-   * is preceded by another {@code '\'}.
-   *
-   * @param input  the {@code String} to unescape, may be null
-   * @return a new unescaped {@code String}, {@code null} if null string 
input
-   */
-  public static final String unescapeJava(String input) {
-return StringEscapeUtils.unescapeJava(input);
-  }
-
-  /**
-   * Escapes the characters in a {@code String} according to Java string 
literal
-   * rules.
-   *
-   * Deals correctly with quotes and control-chars (tab, backslash, cr, ff,
-   * etc.) so, for example, a tab becomes the characters {@code '\\'} and
-   * {@code 't'}.
-   *
-   * Example:
-   * 
-   * input string: He didn't say, "Stop!"
-   * output string: He didn't say, \"Stop!\"
-   * 
-   *
-   * @param input  String to escape values in, may be null
-   * @return String with escaped values, {@code null} if null string input
-   */
-  public static final String escapeJava(String input) {
-return StringEscapeUtils.escapeJava(input);
-  }
-
-  public static final String escapeNewLines(String input) {
-if (input == null) {
-  return null;
-}
-StringBuilder result = new StringBuilder();
-boolean sawNewline = false;
-for (int i = 0; i < input.length(); i++) {
-  char curChar = input.charAt(i);
-  if (curChar == '\r' || curChar == '\n') {
-if (sawNewline) {
-  continue;
-}
-sawNewline = true;
-result.append("\\n");
-  } else {
-sawNewline = false;
-result.append(curChar);
-  }
-}
-return result.toString();
-  }
-
-  /**
-   * Copied form commons-lang 2.x code as common-lang 3.x has this API 
removed.
-   * 
(http://commons.apache.org/proper/commons-lang/article3_0.html#StringEscapeUtils.escapeSql)
-   * @param str
-   * @return
-   */
-  public static String escapeSql(String str) {
-return (str == null) ? null : StringUtils.replace(str, "'", "''");
-  }
-
-  /**
-   * Return a printable representation of a byte buffer, escaping the 
non-printable
-   * bytes as '\\xNN' where NN is the hexadecimal representation of such 
bytes.
-   *
-   * This function does not modify  the {@code readerIndex} and {@code 
writerIndex}
-   * of the byte buffer.
-   */
-  public static String toBinaryString(ByteBuf buf, int strStart, int 
strEnd) {
-StringBuilder result = new StringBuilder();
-for (int i = strStart; i < strEnd ; ++i) {
-  appendByte(result, buf.getByte(i));
-}
-return result.toString();
-  }
-
-  /**
-   * Return a printable representation of a byte array, 

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143910090
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptionsAuthEnabled.java
 ---
@@ -115,4 +124,59 @@ private void setOptHelper() throws Exception {
   test(String.format("ALTER SYSTEM SET `%s` = %d;", 
ExecConstants.SLICE_TARGET, ExecConstants.SLICE_TARGET_DEFAULT));
 }
   }
+
+  @Test
+  public void testAdminUserOptions() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder();
+
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+  OptionManager optionManager = 
cluster.drillbit().getContext().getOptionManager();
+
+  // Admin Users Tests
+  // config file should have the 'fake' default admin user and it 
should be returned
+  // by the option manager if the option has not been set by the user
+  String configAdminUser =  
optionManager.getOption(ExecConstants.ADMIN_USERS_VALIDATOR);;
+  assertEquals(configAdminUser, 
ExecConstants.ADMIN_USERS_VALIDATOR.DEFAULT_ADMIN_USERS);
+
+  // Option accessor should never return the 'fake' default from the 
config
+  String adminUser1 = 
ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+  assertNotEquals(adminUser1, 
ExecConstants.ADMIN_USERS_VALIDATOR.DEFAULT_ADMIN_USERS);
+
+  // Change TEST_ADMIN_USER if necessary
+  String TEST_ADMIN_USER = "ronswanson";
+  if (adminUser1.equals(TEST_ADMIN_USER)) {
+TEST_ADMIN_USER += "thefirst";
+  }
+  // Check if the admin option accessor honors a user-supplied values
+  String sql = String.format("ALTER SYSTEM SET `%s`='%s'", 
ExecConstants.ADMIN_USERS_KEY, TEST_ADMIN_USER);
+  client.queryBuilder().sql(sql).run();
--- End diff --

`client.alterSystem(key, value)`


---

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143910606
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptionsAuthEnabled.java
 ---
@@ -115,4 +124,59 @@ private void setOptHelper() throws Exception {
   test(String.format("ALTER SYSTEM SET `%s` = %d;", 
ExecConstants.SLICE_TARGET, ExecConstants.SLICE_TARGET_DEFAULT));
 }
   }
+
+  @Test
+  public void testAdminUserOptions() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder();
+
+try (ClusterFixture cluster = builder.build();
--- End diff --

No big deal, but if you set no options, then a shorthand is:

```
cluster = ClusterFixture.standardCluster();
```


---

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/983#discussion_r143910712
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptionsAuthEnabled.java
 ---
@@ -115,4 +124,59 @@ private void setOptHelper() throws Exception {
   test(String.format("ALTER SYSTEM SET `%s` = %d;", 
ExecConstants.SLICE_TARGET, ExecConstants.SLICE_TARGET_DEFAULT));
 }
   }
+
+  @Test
+  public void testAdminUserOptions() throws Exception {
+FixtureBuilder builder = ClusterFixture.builder();
+
+try (ClusterFixture cluster = builder.build();
+ ClientFixture client = cluster.clientFixture()) {
+  OptionManager optionManager = 
cluster.drillbit().getContext().getOptionManager();
+
+  // Admin Users Tests
+  // config file should have the 'fake' default admin user and it 
should be returned
+  // by the option manager if the option has not been set by the user
+  String configAdminUser =  
optionManager.getOption(ExecConstants.ADMIN_USERS_VALIDATOR);;
+  assertEquals(configAdminUser, 
ExecConstants.ADMIN_USERS_VALIDATOR.DEFAULT_ADMIN_USERS);
+
+  // Option accessor should never return the 'fake' default from the 
config
+  String adminUser1 = 
ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+  assertNotEquals(adminUser1, 
ExecConstants.ADMIN_USERS_VALIDATOR.DEFAULT_ADMIN_USERS);
+
+  // Change TEST_ADMIN_USER if necessary
+  String TEST_ADMIN_USER = "ronswanson";
+  if (adminUser1.equals(TEST_ADMIN_USER)) {
+TEST_ADMIN_USER += "thefirst";
+  }
+  // Check if the admin option accessor honors a user-supplied values
+  String sql = String.format("ALTER SYSTEM SET `%s`='%s'", 
ExecConstants.ADMIN_USERS_KEY, TEST_ADMIN_USER);
+  client.queryBuilder().sql(sql).run();
+  String adminUser2 = 
ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+  assertEquals(adminUser2, TEST_ADMIN_USER);
+
+  // Admin User Groups Tests
+
+  // config file should have the 'fake' default admin user and it 
should be returned
+  // by the option manager if the option has not been set by the user
+  String configAdminUserGroups =  
optionManager.getOption(ExecConstants.ADMIN_USER_GROUPS_VALIDATOR);
+  assertEquals(configAdminUserGroups, 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.DEFAULT_ADMIN_USER_GROUPS);
+
+  // Option accessor should never return the 'fake' default from the 
config
+  String adminUserGroups1 = 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
+  assertNotEquals(adminUserGroups1, 
ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.DEFAULT_ADMIN_USER_GROUPS);
+
+  // Change TEST_ADMIN_USER_GROUPS if necessary
+  String TEST_ADMIN_USER_GROUPS = "yakshavers";
+  if (adminUserGroups1.equals(TEST_ADMIN_USER_GROUPS)) {
+TEST_ADMIN_USER_GROUPS += ",wormracers";
--- End diff --

Usually ALL_CAPS is reserved for constants, which means we can't alter them 
as we do here... I suspect you mean `testAdminUserGroups`...


---

[GitHub] drill pull request #983: MD-2769: DRILL-5819: Default value of security.admi...

[bitblender]

GitHub user bitblender opened a pull request:

https://github.com/apache/drill/pull/983

MD-2769: DRILL-5819: Default value of security.admin.user_groups and 
security.admin.users is true

The values for admin user/groups in the config file was incorrectly set to 
"true". 

These options have no meaningful static default values. At runtime, the 
process user/groups should be used as the default admin user/groups. New 
accessors have been added for these options and these have to be used instead 
of the usual option manager accessors. Dummy default strings are used in the 
config file, which when returned by the usual options manager, indicate that 
the user has not changed these options. 

This change implements the above-mentioned items and also adds a UI section 
to see the current admin user/groups. This UI section is only shown to 
authenticated admin users.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bitblender/drill DRILL-5819

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/983.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #983


commit 3ba5401a3f6780dbbffd36ef2a9f67b12089a9ef
Author: karthik 
Date:   2017-10-04T18:23:04Z

MD-2769: DRILL-5819: Default value of security.admin.user_groups and 
security.admin.users is true




---