Java/acarbo valkey 493 password update (#425)

* Update tests Signed-off-by: Andrew Carbonetto <[email protected]> * Add cluster-mode tests Signed-off-by: Andrew Carbonetto <[email protected]> * Spotless Signed-off-by: Andrew Carbonetto <[email protected]> * Fixes for self-review Signed-off-by: Andrew Carbonetto <[email protected]> --------- Signed-off-by: Andrew Carbonetto <[email protected]>
Bit-Quill · Nov 19, 2024 · 0f3b334 · 0f3b334
1 parent 58ffca6
commit 0f3b334
Show file tree

Hide file tree

Showing 9 changed files with 286 additions and 92 deletions.
diff --git a/glide-core/redis-rs/redis/src/types.rs b/glide-core/redis-rs/redis/src/types.rs
@@ -153,6 +153,10 @@ pub enum ErrorKind {
 
     /// Not all slots are covered by the cluster
     NotAllSlotsCovered,
+
+    /// Used when an error occurs on when user perform wrong usage of management operation.
+    /// E.g. not allowed configuration change.
+    UserOperationError,
 }
 
 #[derive(PartialEq, Debug)]
@@ -900,6 +904,7 @@ impl RedisError {
             ErrorKind::RESP3NotSupported => "resp3 is not supported by server",
             ErrorKind::ParseError => "parse error",
             ErrorKind::NotAllSlotsCovered => "not all slots are covered",
+            ErrorKind::UserOperationError => "Wrong usage of management operation",
         }
     }
 
@@ -1095,6 +1100,7 @@ impl RedisError {
             ErrorKind::NotAllSlotsCovered => RetryMethod::NoRetry,
             ErrorKind::FatalReceiveError => RetryMethod::Reconnect,
             ErrorKind::FatalSendError => RetryMethod::ReconnectAndRetry,
+            ErrorKind::UserOperationError => RetryMethod::NoRetry,
         }
     }
 }

diff --git a/glide-core/src/client/mod.rs b/glide-core/src/client/mod.rs
@@ -479,28 +479,63 @@ impl Client {
 
     /// Update the password used to authenticate with the servers.
     /// If None is passed, the password will be removed.
-    /// If `re_auth` is true, the new password will be used to re-authenticate with all of the nodes.
+    /// If `immediate_auth` is true, the password will be used to authenticate with the servers immediately using the `AUTH` command.
+    /// The default behavior is to update the password without authenticating immediately.
+    /// If the password is empty or None, and `immediate_auth` is true, the password will be updated and an error will be returned.
     pub async fn update_connection_password(
         &mut self,
         password: Option<String>,
-        re_auth: bool,
+        immediate_auth: bool,
     ) -> RedisResult<Value> {
-        if re_auth {
-            let routing = RoutingInfo::MultiNode((
-                MultipleNodeRoutingInfo::AllNodes,
-                Some(ResponsePolicy::AllSucceeded),
-            ));
-            let mut cmd = redis::cmd("AUTH");
-            cmd.arg(&password);
-            self.send_command(&cmd, Some(routing)).await?;
+        let timeout = self.request_timeout;
+        // The password update operation is wrapped in a timeout to prevent it from blocking indefinitely.
+        // If the operation times out, an error is returned.
+        // Since the password update operation is not a command that go through the regular command pipeline,
+        // it is not have the regular timeout handling, as such we need to handle it separately.
+        match tokio::time::timeout(timeout, async {
+            match self.internal_client {
+                ClientWrapper::Standalone(ref mut client) => {
+                    client.update_connection_password(password.clone()).await
+                }
+                ClientWrapper::Cluster { ref mut client } => {
+                    client.update_connection_password(password.clone()).await
+                }
+            }
+        })
+            .await
+        {
+            Ok(result) => {
+                if immediate_auth {
+                    self.send_immediate_auth(password).await
+                } else {
+                    result
+                }
+            }
+            Err(_elapsed) => Err(RedisError::from((
+                ErrorKind::IoError,
+                "Password update operation timed out, please check the connection",
+            ))),
         }
+    }
 
-        match self.internal_client {
-            ClientWrapper::Standalone(ref mut client) => {
-                client.update_connection_password(password).await
-            }
-            ClientWrapper::Cluster { ref mut client } => {
-                client.update_connection_password(password).await
+    async fn send_immediate_auth(&mut self, password: Option<String>) -> RedisResult<Value> {
+        match &password {
+            Some(pw) if pw.is_empty() => Err(RedisError::from((
+                ErrorKind::UserOperationError,
+                "Empty password provided for authentication",
+            ))),
+            None => Err(RedisError::from((
+                ErrorKind::UserOperationError,
+                "No password provided for authentication",
+            ))),
+            Some(password) => {
+                let routing = RoutingInfo::MultiNode((
+                    MultipleNodeRoutingInfo::AllNodes,
+                    Some(ResponsePolicy::AllSucceeded),
+                ));
+                let mut cmd = redis::cmd("AUTH");
+                cmd.arg(password);
+                self.send_command(&cmd, Some(routing)).await
             }
         }
     }

diff --git a/glide-core/src/protobuf/command_request.proto b/glide-core/src/protobuf/command_request.proto
@@ -510,7 +510,7 @@ message ClusterScan {
 
 message UpdateConnectionPassword {
     optional string password = 1;
-    bool re_auth = 2;
+    bool immediate_auth = 2;
 }
 
 message CommandRequest {

diff --git a/glide-core/src/socket_listener.rs b/glide-core/src/socket_listener.rs
@@ -529,7 +529,7 @@ fn handle_request(request: CommandRequest, mut client: Client, writer: Rc<Writer
                             update_connection_password_command
                                 .password
                                 .map(|chars| chars.to_string()),
-                            update_connection_password_command.re_auth,
+                            update_connection_password_command.immediate_auth,
                         )
                         .await
                         .map_err(|err| err.into()),

diff --git a/java/client/src/main/java/glide/api/BaseClient.java b/java/client/src/main/java/glide/api/BaseClient.java
@@ -218,7 +218,6 @@
 import glide.api.models.commands.LInsertOptions.InsertPosition;
 import glide.api.models.commands.LPosOptions;
 import glide.api.models.commands.ListDirection;
-import glide.api.models.commands.PasswordUpdateMode;
 import glide.api.models.commands.RangeOptions;
 import glide.api.models.commands.RangeOptions.LexRange;
 import glide.api.models.commands.RangeOptions.RangeQuery;
@@ -787,10 +786,15 @@ protected Map<String, Object> handleLcsIdxResponse(Map<String, Object> response)
      * that the internal reconnection mechanism can handle reconnection seamlessly, preventing the
      * loss of in-flight commands.
      *
+     * @param immediateAuth A <code>boolean</code> flag. If <code>true</code>, the client will
+     *     authenticate immediately with the new password against all connections, Using <code>AUTH
+     *     </code> command. <br>
+     *     If password supplied is an empty string, the client will not perform auth and a warning
+     *     will be returned. <br>
+     *     The default is `false`.
      * @apiNote This method updates the client's internal password configuration and does not perform
      *     password rotation on the server side.
      * @param password A new password to set.
-     * @param mode Password update mode, see {@link PasswordUpdateMode}.
      * @return <code>"OK"</code>.
      * @example
      *     <pre>{@code
@@ -799,9 +803,9 @@ protected Map<String, Object> handleLcsIdxResponse(Map<String, Object> response)
      * }</pre>
      */
     public CompletableFuture<String> updateConnectionPassword(
-            @NonNull String password, @NonNull PasswordUpdateMode mode) {
+            @NonNull String password, boolean immediateAuth) {
         return commandManager.submitPasswordUpdate(
-                Optional.of(password), mode, this::handleStringResponse);
+                Optional.of(password), immediateAuth, this::handleStringResponse);
     }
 
     /**
@@ -815,16 +819,22 @@ public CompletableFuture<String> updateConnectionPassword(
      *
      * @apiNote This method updates the client's internal password configuration and does not perform
      *     password rotation on the server side.
-     * @param mode Password update mode, see {@link PasswordUpdateMode}.
+     * @param immediateAuth A <code>boolean</code> flag. If <code>true</code>, the client will
+     *     authenticate immediately with the new password against all connections, Using <code>AUTH
+     *     </code> command. <br>
+     *     If password supplied is an empty string, the client will not perform auth and a warning
+     *     will be returned. <br>
+     *     The default is `false`.
      * @return <code>"OK"</code>.
      * @example
      *     <pre>{@code
-     * String response = client.resetConnectionPassword(RE_AUTHENTICATE).get();
+     * String response = client.resetConnectionPassword(true).get();
      * assert response.equals("OK");
      * }</pre>
      */
-    public CompletableFuture<String> updateConnectionPassword(@NonNull PasswordUpdateMode mode) {
-        return commandManager.submitPasswordUpdate(Optional.empty(), mode, this::handleStringResponse);
+    public CompletableFuture<String> updateConnectionPassword(boolean immediateAuth) {
+        return commandManager.submitPasswordUpdate(
+                Optional.empty(), immediateAuth, this::handleStringResponse);
     }
 
     @Override

diff --git a/java/client/src/main/java/glide/managers/CommandManager.java b/java/client/src/main/java/glide/managers/CommandManager.java
@@ -17,7 +17,6 @@
 import glide.api.models.GlideString;
 import glide.api.models.Script;
 import glide.api.models.Transaction;
-import glide.api.models.commands.PasswordUpdateMode;
 import glide.api.models.commands.scan.ClusterScanCursor;
 import glide.api.models.commands.scan.ScanOptions;
 import glide.api.models.configuration.RequestRoutingConfiguration.ByAddressRoute;
@@ -224,16 +223,16 @@ public <T> CompletableFuture<T> submitClusterScan(
      * Submit a password update request to GLIDE core.
      *
      * @param password A new password to set or empty value to remove the password.
-     * @param mode Password update mode.
+     * @param immediateAuth immediately perform auth.
      * @param responseHandler A response handler.
      * @return A request promise.
      * @param <T> Type of the response.
      */
     public <T> CompletableFuture<T> submitPasswordUpdate(
             Optional<String> password,
-            PasswordUpdateMode mode,
+            boolean immediateAuth,
             GlideExceptionCheckedFunction<Response, T> responseHandler) {
-        var builder = UpdateConnectionPassword.newBuilder().setReAuth(mode.getValue());
+        var builder = UpdateConnectionPassword.newBuilder().setImmediateAuth(immediateAuth);
         password.ifPresent(builder::setPassword);
 
         var command = CommandRequest.newBuilder().setUpdateConnectionPassword(builder.build());

diff --git a/java/integTest/src/test/java/glide/SharedClientTests.java b/java/integTest/src/test/java/glide/SharedClientTests.java
@@ -27,11 +27,12 @@
 import net.bytebuddy.utility.RandomString;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Timeout;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
-// @Timeout(35) // seconds
+@Timeout(35) // seconds
 public class SharedClientTests {
 
     private static GlideClient standaloneClient = null;