Merge pull request #1713 from NuGet/anurse/1684-endlessprompting

Fixed #1684 by returning a 403 when API key is present but invalid

src/NuGetGallery/Authentication/ApiKeyAuthenticationHandler.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Globalization;
 using System.Linq;
 using System.Security.Claims;
 using System.Text;
@@ -48,8 +49,19 @@ internal string GetChallengeMessage()
                 {
                     // Had an API key, but it wasn't valid
                     message = Strings.ApiKeyNotAuthorized;
-                }
 
+                    // Return a 403, since the user is attempting to authenticating but failing.
+                    // 401 indicates that authentication was not attempted but is required.
+                    Response.StatusCode = 403;
+                }
+                else
+                {
+                    // Keep the 401, but add the authentication information
+                    Response.Headers.Append("WWW-Authenticate", String.Format(
+                        CultureInfo.InvariantCulture,
+                        "ApiKey realm=\"{0}\"",
+                        Request.Uri.Host));
+                }
             }
             return message;
         }

tests/NuGetGallery.Facts/Authentication/ApiKeyAuthenticationHandlerFacts.cs

@@ -99,7 +99,7 @@ public async Task GivenA401ResponseInPassiveModeWithMatchingAuthenticationTypeAn
             }
 
             [Fact]
-            public async Task GivenA401ResponseInActiveModeAndNoHeader_ItReturnsApiKeyRequired()
+            public async Task GivenA401ResponseInActiveModeAndNoHeader_ItReturns401ApiKeyRequired()
             {
                 // Arrange
                 var handler = await TestableApiKeyAuthenticationHandler.CreateAsync(new ApiKeyAuthenticationOptions()
@@ -108,6 +108,7 @@ public async Task GivenA401ResponseInActiveModeAndNoHeader_ItReturnsApiKeyRequir
                     AuthenticationType = "blarg"
                 });
                 handler.OwinContext.Response.StatusCode = 401;
+                handler.OwinContext.Response.Headers.Set("WWW-Authenticate", "existing");
                 handler.OwinContext.Authentication.AuthenticationResponseChallenge = 
                     new AuthenticationResponseChallenge(new [] { "blarg" }, new AuthenticationProperties());
 
@@ -116,10 +117,19 @@ public async Task GivenA401ResponseInActiveModeAndNoHeader_ItReturnsApiKeyRequir
 
                 // Assert
                 Assert.Equal(Strings.ApiKeyRequired, message);
+                Assert.Equal(401, handler.OwinContext.Response.StatusCode);
+
+                var authenticateValues = handler.OwinContext.Response.Headers.GetCommaSeparatedValues("WWW-Authenticate");
+                Assert.Contains(
+                    "ApiKey realm=\"nuget.local\"", 
+                    authenticateValues);
+                Assert.Contains(
+                    "existing",
+                    authenticateValues);
             }
 
             [Fact]
-            public async Task GivenA401ResponseInActiveModeAndHeader_ItReturnsApiKeyNotAuthorized()
+            public async Task GivenA401ResponseInActiveModeAndHeader_ItReturns403ApiKeyNotAuthorized()
             {
                 // Arrange
                 var handler = await TestableApiKeyAuthenticationHandler.CreateAsync(new ApiKeyAuthenticationOptions()
@@ -137,6 +147,7 @@ public async Task GivenA401ResponseInActiveModeAndHeader_ItReturnsApiKeyNotAutho
 
                 // Assert
                 Assert.Equal(Strings.ApiKeyNotAuthorized, message);
+                Assert.Equal(403, handler.OwinContext.Response.StatusCode);
             }
         }
 

tests/NuGetGallery.Facts/Framework/Fakes.cs

@@ -107,6 +107,8 @@ public static IOwinContext CreateOwinContext()
         {
             var ctx = new OwinContext();
 
+            ctx.Request.SetUrl("http://nuget.local/");
+
             // Fill in some values that cause exceptions if not present
             ctx.Set<Action<Action<object>, object>>("server.OnSendingHeaders", (_, __) => { });