Auth: Better exception for bad passwords (#2246)

Fixes #1879. In the case where a password is wrong, now indicates "WRONGPASS invalid username-password pair or user is disabled" rather than "NOAUTH Returned - connection has not authenticated" to help users diagnose their problem a bit easier.
StackExchange · Sep 6, 2022 · 5ceb775 · 5ceb775
1 parent 846e096
commit 5ceb775
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 8 deletions.
diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md
@@ -11,6 +11,7 @@
 - Fix [#1968](https://github.com/StackExchange/StackExchange.Redis/issues/1968): Improved handling of EVAL scripts during server restarts and failovers, detecting and re-sending the script for a retry when needed ([#2170 by martintmk](https://github.com/StackExchange/StackExchange.Redis/pull/2170))
 - Adds: `ConfigurationOptions.SslClientAuthenticationOptions` (`netcoreapp3.1`/`net5.0`+ only) to give more control over SSL/TLS authentication ([#2224 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2224))
 - Fix [#2240](https://github.com/StackExchange/StackExchange.Redis/pull/2241): Improve support for DNS-based IPv6 endpoints ([#2241 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2241))
+- Fix [#1879](https://github.com/StackExchange/StackExchange.Redis/issues/1879): Improve exception message when the wrong password is used ([#2246 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2246))
 - Fix [#2233](https://github.com/StackExchange/StackExchange.Redis/issues/2233): Repeated connection to Sentinel servers using the same ConfigurationOptions would fail ([#2242 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2242))
 
 

diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs
@@ -558,7 +558,7 @@ private static bool AllComplete(Task[] tasks)
         }
 
         internal Exception? AuthException { get; private set; }
-        internal void SetAuthSuspect(Exception authException) => AuthException = authException;
+        internal void SetAuthSuspect(Exception authException) => AuthException ??= authException;
 
         /// <summary>
         /// Creates a new <see cref="ConnectionMultiplexer"/> instance.

diff --git a/src/StackExchange.Redis/RedisLiterals.cs b/src/StackExchange.Redis/RedisLiterals.cs
@@ -24,6 +24,7 @@ public static readonly CommandBytes
             slave_read_only = "slave-read-only",
             timeout = "timeout",
             wildcard = "*",
+            WRONGPASS = "WRONGPASS",
             yes = "yes",
             zero = "0",
 

diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs
@@ -227,7 +227,14 @@ public virtual bool SetResult(PhysicalConnection connection, Message message, in
             }
             if (result.IsError)
             {
-                if (result.StartsWith(CommonReplies.NOAUTH)) bridge?.Multiplexer?.SetAuthSuspect(new RedisServerException("NOAUTH Returned - connection has not authenticated"));
+                if (result.StartsWith(CommonReplies.NOAUTH))
+                {
+                    bridge?.Multiplexer?.SetAuthSuspect(new RedisServerException("NOAUTH Returned - connection has not yet authenticated"));
+                }
+                else if (result.StartsWith(CommonReplies.WRONGPASS))
+                {
+                    bridge?.Multiplexer?.SetAuthSuspect(new RedisServerException(result.ToString()));
+                }
 
                 var server = bridge?.ServerEndPoint;
                 bool log = !message.IsInternalCall;

diff --git a/tests/StackExchange.Redis.Tests/Secure.cs b/tests/StackExchange.Redis.Tests/Secure.cs
@@ -50,16 +50,19 @@ public void CheckConfig()
     [Fact]
     public void Connect()
     {
-        using var server = Create();
+        using var conn = Create();
 
-        server.GetDatabase().Ping();
+        conn.GetDatabase().Ping();
     }
 
     [Theory]
-    [InlineData("wrong")]
-    [InlineData("")]
-    public async Task ConnectWithWrongPassword(string password)
+    [InlineData("wrong", "WRONGPASS invalid username-password pair or user is disabled.")]
+    [InlineData("", "NOAUTH Returned - connection has not yet authenticated")]
+    public async Task ConnectWithWrongPassword(string password, string exepctedMessage)
     {
+        using var checkConn = Create();
+        var checkServer = GetServer(checkConn);
+
         var config = ConfigurationOptions.Parse(GetConfiguration());
         config.Password = password;
         config.ConnectRetry = 0; // we don't want to retry on closed sockets in this case.
@@ -74,6 +77,16 @@ public async Task ConnectWithWrongPassword(string password)
             conn.GetDatabase().Ping();
         }).ConfigureAwait(false);
         Log("Exception: " + ex.Message);
-        Assert.StartsWith("It was not possible to connect to the redis server(s). There was an authentication failure; check that passwords (or client certificates) are configured correctly: (RedisServerException) NOAUTH Returned - connection has not authenticated", ex.Message);
+        Assert.StartsWith("It was not possible to connect to the redis server(s). There was an authentication failure; check that passwords (or client certificates) are configured correctly: (RedisServerException) ", ex.Message);
+
+        // This changed in some version...not sure which. For our purposes, splitting on v3 vs v6+
+        if (checkServer.Version >= RedisFeatures.v6_0_0)
+        {
+            Assert.EndsWith(exepctedMessage, ex.Message);
+        }
+        else
+        {
+            Assert.EndsWith("NOAUTH Returned - connection has not yet authenticated", ex.Message);
+        }
     }
 }