PakhomovAlexander commented on code in PR #2339:
URL: https://github.com/apache/ignite-3/pull/2339#discussion_r1270466167
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectWithBasicAuthenticationCommandTest.java:
##########
@@ -99,4 +103,155 @@ void failToConnectWithWrongCredentials() {
// And prompt is still disconnected
assertThat(getPrompt()).isEqualTo("[disconnected]> ");
}
+
+ @Test
+ @DisplayName("Should connect to cluster with username/password")
+ void connectWithAuthenticationParameters() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ //execute("connect", nodeName(), "--username", "admin1", "--password",
"password1");
+
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to
http://localhost:10300";)
+ );
+
+ // And prompt shows user name and node name
+ assertThat(getPrompt()).isEqualTo("[admin:" + nodeName() + "]> ");
+ }
+
+ @Test
+ @DisplayName("Should NOT connect to cluster with incorrect password")
+ void connectWithWrongAuthenticationParameters() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ execute("connect", "--username", "admin", "--password",
"wrong-password");
+
+ // Then
+ assertAll(
+ this::assertOutputIsEmpty,
+ () -> assertErrOutputIs("Authentication error" +
System.lineSeparator()
+ + "Could not connect to node with URL
http://localhost:10300. Check authentication configuration"
+ + System.lineSeparator())
+ );
+ // And prompt is still disconnected
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+ }
+
+ @Test
+ void connectFailIfPasswordNotDefined() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ //execute("connect", nodeName(), "--username", "admin1", "--password",
"password1");
Review Comment:
commented line
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/ssl/ItSslTest.java:
##########
@@ -36,7 +36,7 @@ public class ItSslTest extends
CliSslNotInitializedIntegrationTestBase {
* wouldn't help because it will start to ask questions.
*/
private void connect(String url) {
- Flows.from(new UrlCallInput(url))
+ Flows.from(new ConnectCallInput(url, null, null))
Review Comment:
Could you create Builder for ConnectCallInput? So, you won't pass nulls to
the constructor.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/connect/ConnectOptions.java:
##########
@@ -0,0 +1,39 @@
+package org.apache.ignite.internal.cli.commands.connect;
+
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.PASSWORD_OPTION;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.PASSWORD_OPTION_DESC;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.PASSWORD_OPTION_SHORT;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.USERNAME_OPTION;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.USERNAME_OPTION_DESC;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.USERNAME_OPTION_SHORT;
+
+import org.jetbrains.annotations.Nullable;
+import picocli.CommandLine.Option;
+
+/**
+ * Mixin class for connect command options.
+ */
+public class ConnectOptions {
+
+
+ @Option(names = {USERNAME_OPTION, USERNAME_OPTION_SHORT}, description =
USERNAME_OPTION_DESC)
+ private String username;
+
+ @Option(names = {PASSWORD_OPTION, PASSWORD_OPTION_SHORT}, description =
PASSWORD_OPTION_DESC)
+ private String password;
Review Comment:
These options should be grouped together. Check
https://picocli.info/#_argument_groups
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/Options.java:
##########
@@ -258,5 +258,23 @@ public static final class Constants {
public static final String CLUSTER_CONFIG_FILE_OPTION_SHORT = "-cfgf";
public static final String CLUSTER_CONFIG_FILE_OPTION_DESC = "Path to
cluster configuration file";
+
+ public static final String PASSWORD_OPTION = "--password";
+
+ public static final String PASSWORD_OPTION_SHORT = "--pwd";
Review Comment:
Short option can not be a multi-letter. `CLUSTER_CONFIG_FILE_OPTION_SHORT =
"-cfgf";` is a wrong variant and will be re-worked.
For password CLI use -p and for username -u.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectWithBasicAuthenticationCommandTest.java:
##########
@@ -99,4 +103,155 @@ void failToConnectWithWrongCredentials() {
// And prompt is still disconnected
assertThat(getPrompt()).isEqualTo("[disconnected]> ");
}
+
+ @Test
+ @DisplayName("Should connect to cluster with username/password")
+ void connectWithAuthenticationParameters() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ //execute("connect", nodeName(), "--username", "admin1", "--password",
"password1");
Review Comment:
Why does this comment exist?
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/questions/ConnectToClusterQuestion.java:
##########
@@ -108,6 +112,25 @@ public FlowBuilder<Void, String>
askQuestionIfConnected(String nodeUrl) {
}
return Flows.from(nodeUrl);
}
+ /**
+ * Ask if the user wants to store credentials in config.
+ *
+ * @param username username.
+ * @param password password
+ * @return {@link FlowBuilder} instance which provides result.
+ */
+ public void askQuestionToStoreCredentials(@Nullable String username,
@Nullable String password) {
+ if (!nullOrBlank(username) && !nullOrBlank(password)) {
+ QuestionUiComponent question = fromYesNoQuestion(
+ "Do you want to store username and password in cli
configuration config?"
Review Comment:
I think a question like: "Remember current credentials?" is shorter and
less-implementation-specific.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/connect/ConnectOptions.java:
##########
@@ -0,0 +1,39 @@
+package org.apache.ignite.internal.cli.commands.connect;
+
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.PASSWORD_OPTION;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.PASSWORD_OPTION_DESC;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.PASSWORD_OPTION_SHORT;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.USERNAME_OPTION;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.USERNAME_OPTION_DESC;
+import static
org.apache.ignite.internal.cli.commands.Options.Constants.USERNAME_OPTION_SHORT;
+
+import org.jetbrains.annotations.Nullable;
+import picocli.CommandLine.Option;
+
+/**
+ * Mixin class for connect command options.
+ */
+public class ConnectOptions {
+
+
Review Comment:
two empty lines
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCall.java:
##########
@@ -90,41 +100,41 @@ public CallOutput<String> execute(UrlCallInput input) {
}
}
- private String fetchNodeName(String nodeUrl) throws ApiException {
- return new
NodeManagementApi(clientFactory.getClient(nodeUrl)).nodeState().getName();
- }
-
- /**
- * Deduces whether the cluster has the authentication turned on and
returns a name of successfully authenticated user or {@code null}.
- *
- * @param nodeUrl Node URL.
- * @return Username if authentication was successful or {@code null} if
not or if the cluster has no authentication turned on.
- * @throws ApiException If fails to call an API.
- */
@Nullable
- private String getAuthenticatedUsername(String nodeUrl) throws
ApiException {
- // Try without authentication first to check whether the
authentication is enabled on the cluster.
- String username = clientFactory.basicAuthenticationUsername();
- if (!nullOrBlank(username)) {
- try {
- new
NodeConfigurationApi(clientFactory.getClientWithoutBasicAuthentication(nodeUrl)).getNodeConfiguration();
- return null;
- } catch (ApiException e) {
- if (e.getCause() == null) {
- Problem problem =
IgniteCliApiExceptionHandler.extractProblem(e);
- if (problem.getStatus() ==
HttpStatus.UNAUTHORIZED.getCode()) {
- new
NodeConfigurationApi(clientFactory.getClient(nodeUrl)).getNodeConfiguration();
- return username;
- }
+ private SessionInfo connectWithoutAuthentication(String nodeUrl) throws
ApiException {
+ try {
+ ApiClient apiClient =
clientFactory.getClientWithoutBasicAuthentication(nodeUrl);
+ return constructSessionInfo(apiClient, nodeUrl, null);
+ } catch (ApiException e) {
+ if (e.getCause() == null) {
+ Problem problem =
IgniteCliApiExceptionHandler.extractProblem(e);
+ if (problem.getStatus() == HttpStatus.UNAUTHORIZED.getCode()) {
Review Comment:
You can get the response code from an exception object. So, you do not need
to deserialize the body.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCallInput.java:
##########
@@ -0,0 +1,34 @@
+package org.apache.ignite.internal.cli.call.connect;
+
+import javax.annotation.Nullable;
+import org.apache.ignite.internal.cli.core.call.CallInput;
+
+/** Input for the {@link ConnectCall} call. */
+public class ConnectCallInput implements CallInput {
+
+ private final String url;
+ @Nullable
+ private final String username;
+ @Nullable
+ private final String password;
+
+ public ConnectCallInput(String url, @Nullable String username, @Nullable
String password) {
+ this.url = url;
+ this.username = username;
+ this.password = password;
+ }
+
+ String getUrl() {
Review Comment:
We prefer not to use `get` prefix for getters. Record-style seems to be a
better choice.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCall.java:
##########
@@ -47,41 +50,48 @@
* Call for connect to Ignite 3 node. As a result {@link Session} will hold a
valid node-url.
*/
@Singleton
-public class ConnectCall implements Call<UrlCallInput, String> {
- private final Session session;
+public class ConnectCall implements Call<ConnectCallInput, String> {
+ @Inject
+ private Session session;
- private final StateConfigProvider stateConfigProvider;
+ @Inject
+ private StateConfigProvider stateConfigProvider;
- private final ApiClientFactory clientFactory;
+ @Inject
+ private ApiClientFactory clientFactory;
- private final JdbcUrlFactory jdbcUrlFactory;
+ @Inject
+ private JdbcUrlFactory jdbcUrlFactory;
+
+ @Inject
+ private ConfigManagerProvider configManagerProvider;
/**
* Constructor.
*/
- public ConnectCall(Session session, StateConfigProvider
stateConfigProvider, ApiClientFactory clientFactory,
Review Comment:
There is no reason to change the common approach for calls: "construction
injection". We do have some places where we use field injection but they are
special and I would not like to spread this practice over the code base.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectWithBasicAuthenticationCommandTest.java:
##########
@@ -99,4 +103,155 @@ void failToConnectWithWrongCredentials() {
// And prompt is still disconnected
assertThat(getPrompt()).isEqualTo("[disconnected]> ");
}
+
+ @Test
+ @DisplayName("Should connect to cluster with username/password")
+ void connectWithAuthenticationParameters() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ //execute("connect", nodeName(), "--username", "admin1", "--password",
"password1");
+
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to
http://localhost:10300";)
+ );
+
+ // And prompt shows user name and node name
+ assertThat(getPrompt()).isEqualTo("[admin:" + nodeName() + "]> ");
+ }
+
+ @Test
+ @DisplayName("Should NOT connect to cluster with incorrect password")
+ void connectWithWrongAuthenticationParameters() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ execute("connect", "--username", "admin", "--password",
"wrong-password");
+
+ // Then
+ assertAll(
+ this::assertOutputIsEmpty,
+ () -> assertErrOutputIs("Authentication error" +
System.lineSeparator()
+ + "Could not connect to node with URL
http://localhost:10300. Check authentication configuration"
+ + System.lineSeparator())
+ );
+ // And prompt is still disconnected
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+ }
+
+ @Test
+ void connectFailIfPasswordNotDefined() {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ //execute("connect", nodeName(), "--username", "admin1", "--password",
"password1");
+
+ execute("connect", "--username", "admin", "--password", "");
+
+ // Then
+ assertAll(
+ this::assertOutputIsEmpty,
+ () -> assertErrOutputIs("Authentication error" +
System.lineSeparator()
+ + "Could not connect to node with URL
http://localhost:10300. Check authentication configuration"
+ + System.lineSeparator())
+ );
+ // And prompt is still disconnected
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+ }
+
+ @Test
+ @DisplayName("Should connect to cluster with incorrect password in config
but correct in command")
+ void connectWithWrongAuthenticationParametersInConfig() {
+ // Given basic authentication is configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig(),
createJdbcTestsBasicSecretConfig());
+ // And wrong password is in config
+
configManagerProvider.configManager.setProperty(CliConfigKeys.Constants.BASIC_AUTHENTICATION_PASSWORD,
"wrong-password");
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Connected to
http://localhost:10300";)
+ );
+
+ // And prompt shows user name and node name
+ assertThat(getPrompt()).isEqualTo("[admin:" + nodeName() + "]> ");
+ }
+
+ @Test
+ @DisplayName("Should restore initial values in config in case of connect
failed")
+ void connectWithWrongAuthenticationParametersRestorePreviousCredentials() {
+ // Given basic authentication is configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig(),
createJdbcTestsBasicSecretConfig());
+
+ // Given prompt before connect
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+
+ // When connect with auth parameters
+ execute("connect", "--username", "admin", "--password",
"wrong-password");
+
+ // Then
+ assertAll(
+ this::assertOutputIsEmpty,
+ () -> assertErrOutputIs("Authentication error" +
System.lineSeparator()
+ + "Could not connect to node with URL
http://localhost:10300. Check authentication configuration"
+ + System.lineSeparator())
+ );
+ // And prompt is still disconnected
+ assertThat(getPrompt()).isEqualTo("[disconnected]> ");
+ //Previous correct values restored in config
+ assertEquals("admin",
configManagerProvider.get().getCurrentProperty(Constants.BASIC_AUTHENTICATION_USERNAME));
+ assertEquals("password",
configManagerProvider.get().getCurrentProperty(Constants.BASIC_AUTHENTICATION_PASSWORD));
+ }
+
+ @Test
+ @DisplayName("Should ask to store credentials")
+ void shouldAskToStoreCredentials() throws IOException {
+ // Given basic authentication is NOT configured in config file
+ configManagerProvider.setConfigFile(createIntegrationTestsConfig());
+ // Given prompt before connect
+ String promptBefore = getPrompt();
+ assertThat(promptBefore).isEqualTo("[disconnected]> ");
+
+ // And connected
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // And output is
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputIs("Connected to http://localhost:10300"; +
System.lineSeparator())
+ );
+
+ // And answer is "y"
+ bindAnswers("y");
+
+ // And disconnect
+ resetOutput();
+ execute("disconnect");
+
+ // And prompt is changed to another node
+ String promptAfter = getPrompt();
+ assertThat(promptAfter).isEqualTo("[" + CLUSTER_NODES.get(1).name() +
"]> ");
Review Comment:
I think it should be
```java
// And prompt shows user name and node name
assertThat(getPrompt()).isEqualTo("[admin:" + nodeName() + "]> ");
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/connect/ConnectReplCommand.java:
##########
@@ -50,10 +57,11 @@ public class ConnectReplCommand extends BaseCommand
implements Runnable {
@Override
public void run() {
question.askQuestionIfConnected(nodeNameOrUrl.stringUrl())
- .map(UrlCallInput::new)
+ .map(url -> new ConnectCallInput(url,
connectOptions.username(), connectOptions.password()))
.then(Flows.fromCall(connectCall))
.verbose(verbose)
.print()
+ .onSuccess(() ->
question.askQuestionToStoreCredentials(connectOptions.username(),
connectOptions.password()))
Review Comment:
I did not manage to be asked whatever I tried.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
