Auth: Always call prior handlers during Challenge

Author: HaoK

src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs

@@ -311,6 +311,11 @@ protected virtual Task HandleSignOutAsync(SignOutContext context)
             return TaskCache.CompletedTask;
         }
 
+        /// <summary>
+        /// Override this method to deal with a challenge that is forbidden.
+        /// </summary>
+        /// <param name="context"></param>
+        /// <returns>The returned boolean is ignored.</returns>
         protected virtual Task<bool> HandleForbiddenAsync(ChallengeContext context)
         {
             Response.StatusCode = 403;
@@ -323,7 +328,7 @@ protected virtual Task<bool> HandleForbiddenAsync(ChallengeContext context)
         /// changing the 401 result to 302 of a login page or external sign-in location.)
         /// </summary>
         /// <param name="context"></param>
-        /// <returns>True if no other handlers should be called</returns>
+        /// <returns>The returned boolean is no longer used.</returns>
         protected virtual Task<bool> HandleUnauthorizedAsync(ChallengeContext context)
         {
             Response.StatusCode = 401;
@@ -333,7 +338,6 @@ protected virtual Task<bool> HandleUnauthorizedAsync(ChallengeContext context)
         public async Task ChallengeAsync(ChallengeContext context)
         {
             ChallengeCalled = true;
-            var handled = false;
             if (ShouldHandleScheme(context.AuthenticationScheme, Options.AutomaticChallenge))
             {
                 switch (context.Behavior)
@@ -347,18 +351,18 @@ public async Task ChallengeAsync(ChallengeContext context)
                         }
                         goto case ChallengeBehavior.Unauthorized;
                     case ChallengeBehavior.Unauthorized:
-                        handled = await HandleUnauthorizedAsync(context);
+                        await HandleUnauthorizedAsync(context);
                         Logger.AuthenticationSchemeChallenged(Options.AuthenticationScheme);
                         break;
                     case ChallengeBehavior.Forbidden:
-                        handled = await HandleForbiddenAsync(context);
+                        await HandleForbiddenAsync(context);
                         Logger.AuthenticationSchemeForbidden(Options.AuthenticationScheme);
                         break;
                 }
                 context.Accept();
             }
 
-            if (!handled && PriorHandler != null)
+            if (PriorHandler != null)
             {
                 await PriorHandler.ChallengeAsync(context);
             }

test/Microsoft.AspNetCore.Authentication.Test/AuthenticationHandlerFacts.cs

@@ -75,6 +75,49 @@ public async Task AuthHandlerAuthenticateCachesTicket(string scheme)
             Assert.Equal(1, handler.AuthCount);
         }
 
+        // Prior to https://github.com/aspnet/Security/issues/930 we wouldn't call prior if handled
+        [Fact]
+        public async Task AuthHandlerChallengeAlwaysCallsPriorHandler()
+        {
+            var handler = await TestHandler.Create("Alpha");
+            var previous = new PreviousHandler();
+
+            handler.PriorHandler = previous;
+            await handler.ChallengeAsync(new ChallengeContext("Alpha"));
+            Assert.True(previous.ChallengeCalled);
+        }
+
+        private class PreviousHandler : IAuthenticationHandler
+        {
+            public bool ChallengeCalled = false;
+
+            public Task AuthenticateAsync(AuthenticateContext context)
+            {
+                throw new NotImplementedException();
+            }
+
+            public Task ChallengeAsync(ChallengeContext context)
+            {
+                ChallengeCalled = true;
+                return Task.FromResult(0);
+            }
+
+            public void GetDescriptions(DescribeSchemesContext context)
+            {
+                throw new NotImplementedException();
+            }
+
+            public Task SignInAsync(SignInContext context)
+            {
+                throw new NotImplementedException();
+            }
+
+            public Task SignOutAsync(SignOutContext context)
+            {
+                throw new NotImplementedException();
+            }
+        }
+
         private class CountOptions : AuthenticationOptions { }
 
         private class CountHandler : AuthenticationHandler<CountOptions>
@@ -109,6 +152,8 @@ private class TestHandler : AuthenticationHandler<TestOptions>
         {
             private TestHandler() { }
 
+            public AuthenticateResult Result = AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), new AuthenticationProperties(), "whatever"));
+
             public static async Task<TestHandler> Create(string scheme)
             {
                 var handler = new TestHandler();
@@ -124,7 +169,7 @@ await handler.InitializeAsync(
 
             protected override Task<AuthenticateResult> HandleAuthenticateAsync()
             {
-                return Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(), new AuthenticationProperties(), "whatever")));
+                return Task.FromResult(Result);
             }
         }
 
@@ -220,7 +265,6 @@ public int StatusCode
 
                 set
                 {
-                    throw new NotImplementedException();
                 }
             }