You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by prasadns14 <gi...@git.apache.org> on 2017/10/18 01:00:58 UTC
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
GitHub user prasadns14 opened a pull request:
https://github.com/apache/drill/pull/998
DRILL-5887: Display process user/groups info in Drill UI
Display process user and process user groups in Drill UI
@paul-rogers please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/prasadns14/drill DRILL-5887
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/998.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 #998
----
commit 9f0719423980dfb5d825e4f03a2a450c915ada7c
Author: Prasad Nagaraj Subramanya <pr...@gmail.com>
Date: 2017-10-18T00:49:11Z
DRILL-5887: Display process user/groups info in Drill UI
----
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145584179
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,21 +86,33 @@ public ClusterInfo getClusterInfoJSON() {
// 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);
+ final String processUser = isUserLoggedIn ?
+ ImpersonationUtil.getProcessUserName() : null;
--- End diff --
From security perspective it would be good to display process user only if an admin user is logged in. Removed the check here as the condition is handled in html.
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145569034
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,21 +86,33 @@ public ClusterInfo getClusterInfoJSON() {
// 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);
+ final String processUser = isUserLoggedIn ?
+ ImpersonationUtil.getProcessUserName() : null;
--- End diff --
Do we want to display the process user only if the user is logged in? Here, are we assuming logged in means authenticated? Why not display the process user otherwise?
Rather than null, should we just use a tag such as "<process user>"?
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145570135
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -112,6 +112,14 @@
<td>Admin User Groups</td>
<td class="list-value">${model.getAdminUserGroups()}</td>
</tr>
+ <tr>
+ <td>Process User</td>
+ <td class="list-value">${model.getProcessUser()}</td>
--- End diff --
Please look at the generated HTML to ensure that the angle brackets in "<none>" are escaped. Should happen [by default](http://freemarker.org/docs/dgui_misc_autoescaping.html) if we've set up Freemarker correctly.
---
[GitHub] drill issue #998: DRILL-5887: Display process user/groups info in Drill UI
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/998
+1, LGTM.
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r146145515
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,19 +86,24 @@ public ClusterInfo getClusterInfoJSON() {
// 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;
+ final String processUser = ImpersonationUtil.getProcessUserName();
+ final String processUserGroups = Joiner.on(", ").join(ImpersonationUtil.getProcessUserGroupNames());
+ String adminUsers = ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+ String adminUserGroups = ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
// separate groups by comma + space
- if (adminUsers != null) {
+ if (adminUsers.length() == 0) {
--- End diff --
@arina-ielchiieva Thanks for the suggestion.
Any particular reason to move these manipulations to freemarker? Do we have any advantage moving this if block over to freemarker?
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145584184
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,21 +86,33 @@ public ClusterInfo getClusterInfoJSON() {
// 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);
+ final String processUser = isUserLoggedIn ?
+ ImpersonationUtil.getProcessUserName() : null;
+ final String processUserGroups = isUserLoggedIn ?
+ Joiner.on(",").join(ImpersonationUtil.getProcessUserGroupNames()) : null;
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 = Joiner.on(", ").join(groups);
+ if (adminUsers.length() == 0) {
+ adminUsers = "<empty>";
--- End diff --
yes freemarker handles this
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145569670
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,21 +86,33 @@ public ClusterInfo getClusterInfoJSON() {
// 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);
+ final String processUser = isUserLoggedIn ?
+ ImpersonationUtil.getProcessUserName() : null;
+ final String processUserGroups = isUserLoggedIn ?
+ Joiner.on(",").join(ImpersonationUtil.getProcessUserGroupNames()) : null;
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 = Joiner.on(", ").join(groups);
+ if (adminUsers.length() == 0) {
+ adminUsers = "<empty>";
--- End diff --
This string uses angle brackets, which have meaning to HTML. When we emit these strings into the template, do we call the Freemarker function to escape special HTML characters?
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145584186
--- Diff: exec/java-exec/src/main/resources/rest/index.ftl ---
@@ -112,6 +112,14 @@
<td>Admin User Groups</td>
<td class="list-value">${model.getAdminUserGroups()}</td>
</tr>
+ <tr>
+ <td>Process User</td>
+ <td class="list-value">${model.getProcessUser()}</td>
--- End diff --
yes freemarker handles this
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145686885
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,19 +86,24 @@ public ClusterInfo getClusterInfoJSON() {
// 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;
+ final String processUser = ImpersonationUtil.getProcessUserName();
+ final String processUserGroups = Joiner.on(", ").join(ImpersonationUtil.getProcessUserGroupNames());
+ String adminUsers = ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+ String adminUserGroups = ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
// separate groups by comma + space
- if (adminUsers != null) {
+ if (adminUsers.length() == 0) {
--- End diff --
Frankly saying, I would suggest to do all these manipulations in freemarker. There are many built-in freemarker functions that can help (i.e. split, join, check for emptiness etc [1]).
[1] http://freemarker.org/docs/ref_builtins_string.html#ref_builtin_split
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145569370
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,21 +86,33 @@ public ClusterInfo getClusterInfoJSON() {
// 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);
+ final String processUser = isUserLoggedIn ?
+ ImpersonationUtil.getProcessUserName() : null;
+ final String processUserGroups = isUserLoggedIn ?
+ Joiner.on(",").join(ImpersonationUtil.getProcessUserGroupNames()) : null;
--- End diff --
Are we guaranteed that the list contains no spaces around the commas? (When reading from ZK, the string is user entered, and the string might be "fred , bob". Do we have to worry about that here?) If the string might contain extra spaces, we might want to use code that Karthik provided to split, strip spaces, and combine the names.
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r146222565
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,19 +86,24 @@ public ClusterInfo getClusterInfoJSON() {
// 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;
+ final String processUser = ImpersonationUtil.getProcessUserName();
+ final String processUserGroups = Joiner.on(", ").join(ImpersonationUtil.getProcessUserGroupNames());
+ String adminUsers = ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+ String adminUserGroups = ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
// separate groups by comma + space
- if (adminUsers != null) {
+ if (adminUsers.length() == 0) {
--- End diff --
This would be adherence to the MVC pattern which separates model from the view. Model is generated on the server side while freemarker is responsible for representation layer.
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r147018186
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,19 +86,24 @@ public ClusterInfo getClusterInfoJSON() {
// 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;
+ final String processUser = ImpersonationUtil.getProcessUserName();
+ final String processUserGroups = Joiner.on(", ").join(ImpersonationUtil.getProcessUserGroupNames());
+ String adminUsers = ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
+ String adminUserGroups = ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
// separate groups by comma + space
- if (adminUsers != null) {
+ if (adminUsers.length() == 0) {
--- End diff --
Made the changes. @arina-ielchiieva please review
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/998#discussion_r145584181
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java ---
@@ -85,21 +86,33 @@ public ClusterInfo getClusterInfoJSON() {
// 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);
+ final String processUser = isUserLoggedIn ?
+ ImpersonationUtil.getProcessUserName() : null;
+ final String processUserGroups = isUserLoggedIn ?
+ Joiner.on(",").join(ImpersonationUtil.getProcessUserGroupNames()) : null;
--- End diff --
This list is obtained from hadoop.security.UserGroupInformation and is not the user option. hence we need not worry about the space
---
[GitHub] drill pull request #998: DRILL-5887: Display process user/groups info in Dri...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/998
---