You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by milleruntime <gi...@git.apache.org> on 2016/09/19 19:22:43 UTC
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
GitHub user milleruntime opened a pull request:
https://github.com/apache/accumulo/pull/154
ACCUMULO-4461: modified ClientOpts to prompt for missing password
Created promptUser() to prompt the user on the console for a password if one is missing. If System.console is null, it will return null causing getToken() to return null (like it does currently).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/milleruntime/accumulo ACCUMULO-4461
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/154.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 #154
----
commit 8d65fb5219cff62ce328eef99693c0bdc4c5c977
Author: milleruntime <mi...@gmail.com>
Date: 2016-09-16T19:58:49Z
ACCUMULO-4461: modified ClientOpts to prompt for missing password
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/154#discussion_r79623802
--- Diff: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java ---
@@ -142,11 +158,22 @@ public AuthenticationToken getToken() {
}
}
- if (securePassword != null)
- return new PasswordToken(securePassword.value);
-
- if (password != null)
- return new PasswordToken(password.value);
+ // other token types should have resolved by this point, so return PasswordToken
+ Password pass = null;
+ if (securePassword != null) {
+ pass = securePassword;
+ } else if (password != null) {
+ pass = password;
+ } else {
+ try {
+ pass = Password.promptUser();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ if (pass != null) {
--- End diff --
That's a weird test. I feel like the `fail()` is the expected execution path (e.g. it's asserting that `getPrincipal()` throw an exception) and the `assertNull()` is an afterthought?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/154#discussion_r79496803
--- Diff: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java ---
@@ -92,6 +94,20 @@ public Password(String dfault) {
public String toString() {
return new String(value, UTF_8);
}
+
+ /**
+ * Prompts user for a password
+ *
+ * @return user entered Password object, null if no console exists
+ */
+ public static Password promptUser() throws IOException {
+ if (System.console() == null) {
+ return null;
--- End diff --
I'm thinking that throwing an exception here would be more clear. This method cannot properly function as the caller expected, so they should get a clear exception in this case (not just a null return value).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on the issue:
https://github.com/apache/accumulo/pull/154
Similar to the test error, this change also forces the user to enter password even when running stop-all.sh. Which is not necessary and cumbersome.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on the issue:
https://github.com/apache/accumulo/pull/154
Ok cool. I ran mvn verify -Psunny locally but coudln't figure out why tests were failing on ShellServerIT trace https://github.com/apache/accumulo/blob/1.7/test/src/test/java/org/apache/accumulo/test/ShellServerIT.java#L1444
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/154#discussion_r79627769
--- Diff: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java ---
@@ -142,11 +158,22 @@ public AuthenticationToken getToken() {
}
}
- if (securePassword != null)
- return new PasswordToken(securePassword.value);
-
- if (password != null)
- return new PasswordToken(password.value);
+ // other token types should have resolved by this point, so return PasswordToken
+ Password pass = null;
+ if (securePassword != null) {
+ pass = securePassword;
+ } else if (password != null) {
+ pass = password;
+ } else {
+ try {
+ pass = Password.promptUser();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ if (pass != null) {
--- End diff --
I will keep the try catch but remove the check for null
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/154
This looks good to me. I will try to find some time to test and merge. Thanks Mike!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/154#discussion_r79626920
--- Diff: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java ---
@@ -142,11 +158,22 @@ public AuthenticationToken getToken() {
}
}
- if (securePassword != null)
- return new PasswordToken(securePassword.value);
-
- if (password != null)
- return new PasswordToken(password.value);
+ // other token types should have resolved by this point, so return PasswordToken
+ Password pass = null;
+ if (securePassword != null) {
+ pass = securePassword;
+ } else if (password != null) {
+ pass = password;
+ } else {
+ try {
+ pass = Password.promptUser();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ if (pass != null) {
--- End diff --
OK I like removing the null return. If I get rid of the try/catch, I'd have to make getToken() throw IOException which will touch a bunch of files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/154#discussion_r79642827
--- Diff: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java ---
@@ -92,6 +94,20 @@ public Password(String dfault) {
public String toString() {
return new String(value, UTF_8);
}
+
+ /**
+ * Prompts user for a password
+ *
+ * @return user entered Password object, null if no console exists
+ */
+ public static Password promptUser() throws IOException {
+ if (System.console() == null) {
+ return null;
--- End diff --
Fixed in 895591c
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/154
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on the issue:
https://github.com/apache/accumulo/pull/154
@joshelser check out 52e5444e1657d859a0c76d89c823f100e03d57ff I tested the Admin commands and I couldn't find any that required password or auth token. This looks to be the only call to getToken that checks for null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/154
Looks reasonable. Let's see what https://builds.apache.org/job/Accumulo-Pull-Requests/449/console does. It looks like it's doing the right thing. I tweaked this earlier this week to do a normal `mvn verify -Psunny`. Hopefully, I don't have to re-validate things by hand :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #154: ACCUMULO-4461: modified ClientOpts to prompt for...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/154#discussion_r79497362
--- Diff: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java ---
@@ -142,11 +158,22 @@ public AuthenticationToken getToken() {
}
}
- if (securePassword != null)
- return new PasswordToken(securePassword.value);
-
- if (password != null)
- return new PasswordToken(password.value);
+ // other token types should have resolved by this point, so return PasswordToken
+ Password pass = null;
+ if (securePassword != null) {
+ pass = securePassword;
+ } else if (password != null) {
+ pass = password;
+ } else {
+ try {
+ pass = Password.promptUser();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ if (pass != null) {
--- End diff --
This becomes simpler too. Either we throw an RTE from the attempt to get a password, or we create a `PasswordToken` with the results. This method always gets to return a non-null value (woo!).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/154
> I will try to find some time to test and merge
Started running a `verify -Psunny`. Will get this in today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/154
Uh oh. Looks like we have a failure in ReadWriteIT#sunnyDay()
```
Tests run: 8, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 160.031 sec <<< FAILURE! - in org.apache.accumulo.test.functional.ReadWriteIT
sunnyDay(org.apache.accumulo.test.functional.ReadWriteIT) Time elapsed: 14.882 sec <<< ERROR!
java.io.IOException: Failed to run `accumulo admin stopAll`
at org.apache.accumulo.test.functional.ReadWriteIT.sunnyDay(ReadWriteIT.java:169)
```
```
2016-09-21 12:18:57,587 [util.Admin] ERROR: java.io.IOException: Attempted to prompt user on the console when System.console = null
java.lang.RuntimeException: java.io.IOException: Attempted to prompt user on the console when System.console = null
at org.apache.accumulo.core.cli.ClientOpts.getToken(ClientOpts.java:171)
at org.apache.accumulo.server.util.Admin.execute(Admin.java:213)
at org.apache.accumulo.server.util.Admin.main(Admin.java:154)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at org.apache.accumulo.start.Main$2.run(Main.java:157)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.IOException: Attempted to prompt user on the console when System.console = null
at org.apache.accumulo.core.cli.ClientOpts$Password.promptUser(ClientOpts.java:105)
at org.apache.accumulo.core.cli.ClientOpts.getToken(ClientOpts.java:169)
... 8 more
```
Kind of seems like we don't have a good understanding of commands/tools which need a username + credentials (password) and which don't. Maybe that's how we need to address the problem?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #154: ACCUMULO-4461: modified ClientOpts to prompt for missin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/154
> coudln't figure out why tests were failing on ShellServerIT trace
Yeah, that one is flaky. Don't worry about it :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---