PakhomovAlexander commented on code in PR #2339:
URL: https://github.com/apache/ignite-3/pull/2339#discussion_r1291290439
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectWithBasicAuthenticationCommandTest.java:
##########
@@ -99,4 +109,178 @@ void failToConnectWithWrongCredentials() {
// And prompt is still disconnected
assertThat(getPrompt()).isEqualTo("[disconnected]> ");
}
+
+ @Test
+ @DisplayName("Should connect to cluster with username/password")
+ void connectWithAuthenticationParameters() throws IOException {
+ // Given basic authentication is NOT 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", "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"
Review Comment:
`Check authentication configuration` were fine when we used a configuration
to provide usename/password but now a user passes username and password
explicitly with command line arguments. What "configuration" is supposed to
mean in this case?
I would say that we either divide these two cases (config provided creds and
explicit ones) or just rephrase the general error to commit "configuration"
word.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectWithBasicAuthenticationCommandTest.java:
##########
@@ -99,4 +109,178 @@ void failToConnectWithWrongCredentials() {
// And prompt is still disconnected
assertThat(getPrompt()).isEqualTo("[disconnected]> ");
}
+
+ @Test
+ @DisplayName("Should connect to cluster with username/password")
+ void connectWithAuthenticationParameters() throws IOException {
+ // Given basic authentication is NOT 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", "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", "--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() throws IOException
{
+ // 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]> ");
+
+ // And answer is "y"
+ bindAnswers("y");
+
+ // When connect with auth parameters
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputIs(
+ "Config saved" + System.lineSeparator() + "Connected
to http://localhost:10300"; + System.lineSeparator())
+ );
+
+ // 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"
Review Comment:
Ah, "Check authentication configuration" here is misleading information. In
fact, configuration is ok but provided options are not.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/rest/ApiClientFactory.java:
##########
@@ -98,27 +111,49 @@ private ApiClient getClientFromSettings(ApiClientSettings
settings) {
return apiClient;
}
- private ApiClientSettings settings(String path, boolean
enableBasicAuthentication) {
+ private ApiClientSettings settingsWithAuth(String path) {
+ ApiClientSettingsBuilder builder = settingsBuilder(path);
+ return setupAuthentication(builder).build();
+ }
+
+ private ApiClientSettings settingsWithAuth(String path, String username,
String password) {
+ ApiClientSettingsBuilder clientSettingsBuilder = settingsBuilder(path);
+ clientSettingsBuilder.basicAuthenticationUsername(username);
+ clientSettingsBuilder.basicAuthenticationPassword(password);
+ return clientSettingsBuilder.build();
+ }
+
+ private ApiClientSettings settingsWithoutAuth(String path) {
+ ApiClientSettingsBuilder builder = settingsBuilder(path);
+ return builder.build();
+ }
+
+ private ApiClientSettingsBuilder settingsBuilder(String path) {
ConfigManager configManager = configManagerProvider.get();
- ApiClientSettingsBuilder builder = ApiClientSettings.builder()
+ return ApiClientSettings.builder()
.basePath(path)
.keyStorePath(configManager.getCurrentProperty(REST_KEY_STORE_PATH.value()))
.keyStorePassword(configManager.getCurrentProperty(REST_KEY_STORE_PASSWORD.value()))
.trustStorePath(configManager.getCurrentProperty(REST_TRUST_STORE_PATH.value()))
.trustStorePassword(configManager.getCurrentProperty(REST_TRUST_STORE_PASSWORD.value()));
-
- if (enableBasicAuthentication) {
- builder
-
.basicAuthenticationUsername(configManager.getCurrentProperty(BASIC_AUTHENTICATION_USERNAME.value()))
-
.basicAuthenticationPassword(configManager.getCurrentProperty(BASIC_AUTHENTICATION_PASSWORD.value()));
- }
-
- return builder.build();
}
- public String basicAuthenticationUsername() {
+ private ApiClientSettingsBuilder
setupAuthentication(ApiClientSettingsBuilder builder) {
ConfigManager configManager = configManagerProvider.get();
- return
configManager.getCurrentProperty(BASIC_AUTHENTICATION_USERNAME.value());
+
+ // Use credentials from current session settings if exist
Review Comment:
```suggestion
// Use credentials from current session settings if exist.
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/questions/ConnectToClusterQuestion.java:
##########
@@ -109,6 +113,29 @@ 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
+ */
+ public void askQuestionToStoreCredentials(@Nullable String username,
@Nullable String password) {
+ if (!nullOrBlank(username) && !nullOrBlank(password)) {
+ String storedUsername =
configManagerProvider.get().getCurrentProperty(BASIC_AUTHENTICATION_USERNAME.value());
+ String storedPassword =
configManagerProvider.get().getCurrentProperty(BASIC_AUTHENTICATION_PASSWORD.value());
+
+ //Ask question only if cli config has different values
Review Comment:
```suggestion
// Ask question only if cli config has different values.
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/connect/ItConnectWithBasicAuthenticationCommandTest.java:
##########
@@ -99,4 +109,178 @@ void failToConnectWithWrongCredentials() {
// And prompt is still disconnected
assertThat(getPrompt()).isEqualTo("[disconnected]> ");
}
+
+ @Test
+ @DisplayName("Should connect to cluster with username/password")
+ void connectWithAuthenticationParameters() throws IOException {
+ // Given basic authentication is NOT 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", "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", "--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() throws IOException
{
+ // 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]> ");
+
+ // And answer is "y"
+ bindAnswers("y");
+
+ // When connect with auth parameters
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // Then
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputIs(
+ "Config saved" + System.lineSeparator() + "Connected
to http://localhost:10300"; + System.lineSeparator())
+ );
+
+ // 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 answer is "y"
+ bindAnswers("y");
+
+ // And connected
+ execute("connect", "--username", "admin", "--password", "password");
+
+ // And output is
+ assertAll(
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputIs(
+ "Config saved" + System.lineSeparator() + "Connected
to http://localhost:10300"; + System.lineSeparator())
+ );
+
+ // And prompt shows user name and node name
+ assertThat(getPrompt()).isEqualTo("[admin:" + nodeName() + "]> ");
+ }
+
+ @Test
+ @DisplayName("Should create correct api client even if user doesn't store
credentials in settings.")
+ void sessionListenersShouldBeInvokedWithCorrectCredentials() throws
IOException {
Review Comment:
I think yuo mix different levels of abstraction in test and naming. The test
itself is on the highest abstraction level and operated with entities like
"connect, prompt, disconnect, configuration, answer". But test name (both
method signature and DisplayName) operates with entities like: API client and
listeners. This is a lower level of abstraction.
I think it is better not to mix them. The test name should use the same
vocabulary that test uses.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/Options.java:
##########
@@ -266,5 +266,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 = "-p";
+
+ public static final String PASSWORD_OPTION_DESC = "Password to connect
to cluster";
+
+ public static final String USERNAME_OPTION = "--username";
+
+ public static final String USERNAME_OPTION_SHORT = "-u";
Review Comment:
We use -u and -p for URL and Profile thighs. Can you please create a ticket
for revising all short options and reworking this concept?
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/connect/ConnectCallInput.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.ignite.internal.cli.call.connect;
+
+import org.apache.ignite.internal.cli.core.call.CallInput;
+import org.jetbrains.annotations.Nullable;
+
+/** Input for the {@link ConnectCall} call. */
+public class ConnectCallInput implements CallInput {
+
+
Review Comment:
two empty lines
--
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]
