Skip to content

Commit

Permalink
Code review from ctubbsii for #3440
Browse files Browse the repository at this point in the history 
* Use protected method and override in test instead of injecting static
  implementation as a public static field that allows any thread to
  modify the shell's authentication behavior
* Restore previous context-specific prompts by adding passwordPrompt
  string to the getAuthenticationToken method
* Tweak token variable name to minimize diff and for formatting
* Use existing client config file in ShellAuthenticatorIT
  • Loading branch information
@ctubbsii
ctubbsii committed Jun 8, 2023
1 parent 8f71e5c commit 2db232f
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 66 deletions.
42 changes: 16 additions & 26 deletions shell/src/main/java/org/apache/accumulo/shell/Shell.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,15 +190,6 @@
@AutoService(KeywordExecutable.class)
public class Shell extends ShellOptions implements KeywordExecutable {

public static interface Authenticator {
default boolean authenticateUser(AccumuloClient client, String principal,
AuthenticationToken token) throws AccumuloException, AccumuloSecurityException {
return client.securityOperations().authenticateUser(principal, token);
}
}

public static Authenticator AUTHENTICATOR = new Authenticator() {};

public static final Logger log = LoggerFactory.getLogger(Shell.class);
private static final Logger audit = LoggerFactory.getLogger(Shell.class.getName() + ".audit");

Expand Down Expand Up @@ -267,29 +258,29 @@ public Shell(LineReader reader) {
this.writer = terminal.writer();
}

private boolean authenticateUser(AccumuloClient client, AuthenticationToken token)
// this is visible only for FateCommandTest, otherwise, should be private or inline
protected boolean authenticateUser(AccumuloClient client, AuthenticationToken token)
throws AccumuloException, AccumuloSecurityException {
return AUTHENTICATOR.authenticateUser(client, client.whoami(), token);
return client.securityOperations().authenticateUser(client.whoami(), token);
}

private AuthenticationToken getAuthenticationToken(String principal,
String authenticationString) {
private AuthenticationToken getAuthenticationToken(String principal, String password,
String passwordPrompt) {
AuthenticationToken token = null;
if (authenticationString == null
&& clientProperties.containsKey(ClientProperty.AUTH_TOKEN.getKey())
if (password == null && clientProperties.containsKey(ClientProperty.AUTH_TOKEN.getKey())
&& principal.equals(ClientProperty.AUTH_PRINCIPAL.getValue(clientProperties))) {
token = ClientProperty.getAuthenticationToken(clientProperties);
}
if (token == null) {
// Read password if the user explicitly asked for it, or didn't specify anything at all
if (PasswordConverter.STDIN.equals(authenticationString) || authenticationString == null) {
authenticationString = reader.readLine("Password: ", '*');
if (PasswordConverter.STDIN.equals(password) || password == null) {
password = reader.readLine(passwordPrompt, '*');
}
if (authenticationString == null) {
if (password == null) {
// User cancel, e.g. Ctrl-D pressed
throw new ParameterException("No password or token option supplied");
} else {
token = new PasswordToken(authenticationString);
token = new PasswordToken(password);
}
}
return token;
Expand Down Expand Up @@ -369,13 +360,12 @@ public boolean config(String... args) throws IOException {
exitCode = 1;
return false;
}
String authenticationString = options.getPassword();
final AuthenticationToken authToken = getAuthenticationToken(principal, authenticationString);
String password = options.getPassword();
final AuthenticationToken token = getAuthenticationToken(principal, password, "Password: ");
try {
this.setTableName("");
accumuloClient =
Accumulo.newClient().from(clientProperties).as(principal, authToken).build();
authenticateUser(accumuloClient, authToken);
accumuloClient = Accumulo.newClient().from(clientProperties).as(principal, token).build();
authenticateUser(accumuloClient, token);
context = (ClientContext) accumuloClient;
} catch (Exception e) {
printException(e);
Expand Down Expand Up @@ -756,8 +746,8 @@ public void execCommand(String input, boolean ignoreAuthTimeout, boolean echoPro
writer.println("Shell has been idle for too long. Please re-authenticate.");
boolean authFailed = true;
do {
final AuthenticationToken authToken =
getAuthenticationToken(accumuloClient.whoami(), null);
final AuthenticationToken authToken = getAuthenticationToken(accumuloClient.whoami(),
null, "Enter current password for '" + accumuloClient.whoami() + "': ");
try {
authFailed = !authenticateUser(accumuloClient, authToken);
} catch (Exception e) {
Expand Down
16 changes: 6 additions & 10 deletions shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@
import java.util.List;

import org.apache.accumulo.core.client.AccumuloClient;
import org.apache.accumulo.core.client.AccumuloException;
import org.apache.accumulo.core.client.AccumuloSecurityException;
import org.apache.accumulo.core.client.security.tokens.AuthenticationToken;
import org.apache.accumulo.core.clientImpl.ClientContext;
import org.apache.accumulo.core.fate.AdminUtil;
Expand Down Expand Up @@ -134,13 +132,6 @@ public void reset() {
public static void setup() {
zk = createMock(ZooReaderWriter.class);
managerLockPath = createMock(ServiceLockPath.class);
Shell.AUTHENTICATOR = new Shell.Authenticator() {
@Override
public boolean authenticateUser(AccumuloClient client, String principal,
AuthenticationToken token) throws AccumuloException, AccumuloSecurityException {
return true;
}
};
}

@Test
Expand Down Expand Up @@ -331,7 +322,12 @@ private Shell createShell(TestOutputStream output) throws IOException {
Terminal terminal = new DumbTerminal(new FileInputStream(FileDescriptor.in), output);
terminal.setSize(new Size(80, 24));
LineReader reader = LineReaderBuilder.builder().terminal(terminal).build();
Shell shell = new Shell(reader);
Shell shell = new Shell(reader) {
@Override
protected boolean authenticateUser(AccumuloClient client, AuthenticationToken token) {
return true;
}
};
shell.setLogErrorsToConsole();
return shell;
}
Expand Down
33 changes: 3 additions & 30 deletions test/src/main/java/org/apache/accumulo/test/shell/ShellAuthenticatorIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,10 @@
*/
package org.apache.accumulo.test.shell;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.nio.file.Files;
import java.util.Properties;
import java.util.TimeZone;

import org.apache.accumulo.harness.SharedMiniClusterBase;
Expand Down Expand Up @@ -59,7 +54,6 @@ public static void teardown() {
private StringInputStream input;
private TestOutputStream output;
private Shell shell;
private File config;
public LineReader reader;
public Terminal terminal;

Expand All @@ -71,17 +65,6 @@ public void setupShell() throws IOException {
terminal = new DumbTerminal(input, output);
terminal.setSize(new Size(80, 24));
reader = LineReaderBuilder.builder().terminal(terminal).build();
if (config != null) {
if (!config.delete()) {
throw new IllegalStateException("failed to delete: " + config.getAbsolutePath());
}
}
config = Files.createTempFile(null, null).toFile();
try (FileWriter writer = new FileWriter(config, UTF_8)) {
Properties p = SharedMiniClusterBase.getClientProps();
p.store(writer, null);
}
config.deleteOnExit();
}

@AfterEach
Expand All @@ -95,7 +78,7 @@ public void tearDownShell() {
public void testClientPropertiesFile() throws IOException {
shell = new Shell(reader);
shell.setLogErrorsToConsole();
assertTrue(shell.config("--config-file", config.toString()));
assertTrue(shell.config("--config-file", getCluster().getClientPropsPath()));
}

@Test
Expand All @@ -122,20 +105,10 @@ public void testAuthTimeoutPropertiesFile() throws IOException, InterruptedExcep
terminal = new DumbTerminal(input, output);
terminal.setSize(new Size(80, 24));
reader = LineReaderBuilder.builder().terminal(terminal).build();
if (config != null) {
if (!config.delete()) {
throw new IllegalStateException("failed to delete: " + config.getAbsolutePath());
}
}
config = Files.createTempFile(null, null).toFile();
try (FileWriter writer = new FileWriter(config, UTF_8)) {
Properties p = SharedMiniClusterBase.getClientProps();
p.store(writer, null);
}
config.deleteOnExit();
shell = new Shell(reader);
shell.setLogErrorsToConsole();
assertTrue(shell.config("--auth-timeout", "1", "--config-file", config.toString()));
assertTrue(
shell.config("--auth-timeout", "1", "--config-file", getCluster().getClientPropsPath()));
Thread.sleep(90000);
shell.execCommand("whoami", false, false);
assertTrue(output.get().contains("root"));
Expand Down

0 comments on commit 2db232f

Please sign in to comment.