Reuse failed auth results in AuthN (#43269)

Kahbazi · web-flow · commit 1b15d0e92f13 · 2022-08-15T16:30:35.000-07:00
diff --git a/src/Security/Authentication/Certificate/src/AuthenticateResults.cs b/src/Security/Authentication/Certificate/src/AuthenticateResults.cs
@@ -0,0 +1,11 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.Certificate;
+
+internal static class AuthenticateResults
+{
+    internal static AuthenticateResult NoSelfSigned = AuthenticateResult.Fail("Options do not allow self signed certificates.");
+    internal static AuthenticateResult NoChainedCertificates = AuthenticateResult.Fail("Options do not allow chained certificates.");
+    internal static AuthenticateResult InvalidClientCertificate = AuthenticateResult.Fail("Client certificate failed validation.");
+}
diff --git a/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs b/src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs
@@ -126,7 +126,7 @@ private async Task<AuthenticateResult> ValidateCertificateAsync(X509Certificate2
             !Options.AllowedCertificateTypes.HasFlag(CertificateTypes.SelfSigned))
         {
             Logger.CertificateRejected("Self signed", clientCertificate.Subject);
-            return AuthenticateResult.Fail("Options do not allow self signed certificates.");
+            return AuthenticateResults.NoSelfSigned;
         }
 
         // If we have a chained cert, and they're not allowed, exit early and not bother with
@@ -135,7 +135,7 @@ private async Task<AuthenticateResult> ValidateCertificateAsync(X509Certificate2
             !Options.AllowedCertificateTypes.HasFlag(CertificateTypes.Chained))
         {
             Logger.CertificateRejected("Chained", clientCertificate.Subject);
-            return AuthenticateResult.Fail("Options do not allow chained certificates.");
+            return AuthenticateResults.NoChainedCertificates;
         }
 
         var chainPolicy = BuildChainPolicy(clientCertificate, isCertificateSelfSigned);
@@ -153,7 +153,7 @@ private async Task<AuthenticateResult> ValidateCertificateAsync(X509Certificate2
                 chainErrors.Add($"{validationFailure.Status} {validationFailure.StatusInformation}");
             }
             Logger.CertificateFailedValidation(clientCertificate.Subject, chainErrors);
-            return AuthenticateResult.Fail("Client certificate failed validation.");
+            return AuthenticateResults.InvalidClientCertificate;
         }
 
         var certificateValidatedContext = new CertificateValidatedContext(Context, Scheme, Options)
diff --git a/src/Security/Authentication/Cookies/src/AuthenticateResults.cs b/src/Security/Authentication/Cookies/src/AuthenticateResults.cs
@@ -0,0 +1,13 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.Cookies;
+
+internal static class AuthenticateResults
+{
+    internal static AuthenticateResult FailedUnprotectingTicket = AuthenticateResult.Fail("Unprotect ticket failed");
+    internal static AuthenticateResult MissingSessionId = AuthenticateResult.Fail("SessionId missing");
+    internal static AuthenticateResult MissingIdentityInSession = AuthenticateResult.Fail("Identity missing in session store");
+    internal static AuthenticateResult ExpiredTicket = AuthenticateResult.Fail("Ticket expired");
+    internal static AuthenticateResult NoPrincipal = AuthenticateResult.Fail("No principal.");
+}
diff --git a/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs b/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs
@@ -147,21 +147,21 @@ private async Task<AuthenticateResult> ReadCookieTicket()
         var ticket = Options.TicketDataFormat.Unprotect(cookie, GetTlsTokenBinding());
         if (ticket == null)
         {
-            return AuthenticateResult.Fail("Unprotect ticket failed");
+            return AuthenticateResults.FailedUnprotectingTicket;
         }
 
         if (Options.SessionStore != null)
         {
             var claim = ticket.Principal.Claims.FirstOrDefault(c => c.Type.Equals(SessionIdClaim));
             if (claim == null)
             {
-                return AuthenticateResult.Fail("SessionId missing");
+                return AuthenticateResults.MissingSessionId;
             }
             // Only store _sessionKey if it matches an existing session. Otherwise we'll create a new one.
             ticket = await Options.SessionStore.RetrieveAsync(claim.Value, Context, Context.RequestAborted);
             if (ticket == null)
             {
-                return AuthenticateResult.Fail("Identity missing in session store");
+                return AuthenticateResults.MissingIdentityInSession;
             }
             _sessionKey = claim.Value;
         }
@@ -178,7 +178,7 @@ private async Task<AuthenticateResult> ReadCookieTicket()
                 // Clear out the session key if its expired, so renew doesn't try to use it
                 _sessionKey = null;
             }
-            return AuthenticateResult.Fail("Ticket expired");
+            return AuthenticateResults.ExpiredTicket;
         }
 
         // Finally we have a valid ticket
@@ -204,7 +204,7 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
 
         if (context.Principal == null)
         {
-            return AuthenticateResult.Fail("No principal.");
+            return AuthenticateResults.NoPrincipal;
         }
 
         if (context.ShouldRenew)
diff --git a/src/Security/Authentication/Core/src/HandleRequestResult.cs b/src/Security/Authentication/Core/src/HandleRequestResult.cs
@@ -8,6 +8,10 @@ namespace Microsoft.AspNetCore.Authentication;
 /// </summary>
 public class HandleRequestResult : AuthenticateResult
 {
+    private static readonly HandleRequestResult _noResult = new() { None = true };
+    private static readonly HandleRequestResult _skippedResult = new() { Skipped = true };
+    private static readonly HandleRequestResult _handledResult = new() { Handled = true };
+
     /// <summary>
     /// Indicates that stage of authentication was directly handled by
     /// user intervention and no further processing should be attempted.
@@ -77,26 +81,17 @@ public class HandleRequestResult : AuthenticateResult
     /// The caller is responsible for generating the full response.
     /// </summary>
     /// <returns>The result.</returns>
-    public static HandleRequestResult Handle()
-    {
-        return new HandleRequestResult() { Handled = true };
-    }
+    public static HandleRequestResult Handle() => _handledResult;
 
     /// <summary>
     /// Discontinue processing the request in the current handler.
     /// </summary>
     /// <returns>The result.</returns>
-    public static HandleRequestResult SkipHandler()
-    {
-        return new HandleRequestResult() { Skipped = true };
-    }
+    public static HandleRequestResult SkipHandler() => _skippedResult;
 
     /// <summary>
     /// Indicates that there were no results produced during authentication.
     /// </summary>
     /// <returns>The result.</returns>
-    public static new HandleRequestResult NoResult()
-    {
-        return new HandleRequestResult() { None = true };
-    }
+    public static new HandleRequestResult NoResult() => _noResult;
 }
diff --git a/src/Security/Authentication/JwtBearer/src/AuthenticateResults.cs b/src/Security/Authentication/JwtBearer/src/AuthenticateResults.cs
@@ -0,0 +1,9 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.JwtBearer;
+
+internal static class AuthenticateResults
+{
+    internal static AuthenticateResult ValidatorNotFound = AuthenticateResult.Fail("No SecurityTokenValidator available for token.");
+}
diff --git a/src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs b/src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs
@@ -178,7 +178,7 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
                 return AuthenticateResult.Fail(authenticationFailedContext.Exception);
             }
 
-            return AuthenticateResult.Fail("No SecurityTokenValidator available for token.");
+            return AuthenticateResults.ValidatorNotFound;
         }
         catch (Exception ex)
         {
diff --git a/src/Security/Authentication/OAuth/src/HandleRequestResults.cs b/src/Security/Authentication/OAuth/src/HandleRequestResults.cs
@@ -0,0 +1,9 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.OAuth;
+
+internal static class HandleRequestResults
+{
+    internal static HandleRequestResult InvalidState = HandleRequestResult.Fail("The oauth state was missing or invalid.");
+}
diff --git a/src/Security/Authentication/OAuth/src/OAuthHandler.cs b/src/Security/Authentication/OAuth/src/OAuthHandler.cs
@@ -61,7 +61,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
 
         if (properties == null)
         {
-            return HandleRequestResult.Fail("The oauth state was missing or invalid.");
+            return HandleRequestResults.InvalidState;
         }
 
         // OAuth2 10.12 CSRF
diff --git a/src/Security/Authentication/OpenIdConnect/src/HandleRequestResults.cs b/src/Security/Authentication/OpenIdConnect/src/HandleRequestResults.cs
@@ -0,0 +1,10 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;
+
+internal static class HandleRequestResults
+{
+    internal static HandleRequestResult UnexpectedParams = HandleRequestResult.Fail("An OpenID Connect response cannot contain an identity token or an access token when using response_mode=query");
+    internal static HandleRequestResult NoMessage = HandleRequestResult.Fail("No message.");
+}
diff --git a/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs b/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs
@@ -522,8 +522,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
                     // Not for us?
                     return HandleRequestResult.SkipHandler();
                 }
-                return HandleRequestResult.Fail("An OpenID Connect response cannot contain an " +
-                        "identity token or an access token when using response_mode=query");
+                return HandleRequestResults.UnexpectedParams;
             }
         }
         // assumption: if the ContentType is "application/x-www-form-urlencoded" it should be safe to read as it is small.
@@ -548,7 +547,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
                 // Not for us?
                 return HandleRequestResult.SkipHandler();
             }
-            return HandleRequestResult.Fail("No message.");
+            return HandleRequestResults.NoMessage;
         }
 
         AuthenticationProperties? properties = null;
diff --git a/src/Security/Authentication/Twitter/src/HandleRequestResults.cs b/src/Security/Authentication/Twitter/src/HandleRequestResults.cs
@@ -0,0 +1,9 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.Twitter;
+
+internal static class HandleRequestResults
+{
+    internal static HandleRequestResult InvalidStateCookie = HandleRequestResult.Fail("Invalid state cookie.");
+}
diff --git a/src/Security/Authentication/Twitter/src/TwitterHandler.cs b/src/Security/Authentication/Twitter/src/TwitterHandler.cs
@@ -55,7 +55,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
 
         if (requestToken == null)
         {
-            return HandleRequestResult.Fail("Invalid state cookie.");
+            return HandleRequestResults.InvalidStateCookie;
         }
 
         var properties = requestToken.Properties;
diff --git a/src/Security/Authentication/WsFederation/src/HandleRequestResults.cs b/src/Security/Authentication/WsFederation/src/HandleRequestResults.cs
@@ -0,0 +1,10 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Authentication.WsFederation;
+
+internal static class HandleRequestResults
+{
+    internal static HandleRequestResult NoMessage = HandleRequestResult.Fail("No message.");
+    internal static HandleRequestResult UnsolicitedLoginsNotAllowed  = HandleRequestResult.Fail("Unsolicited logins are not allowed.");
+}
diff --git a/src/Security/Authentication/WsFederation/src/WsFederationHandler.cs b/src/Security/Authentication/WsFederation/src/WsFederationHandler.cs
@@ -161,7 +161,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
                 return HandleRequestResult.SkipHandler();
             }
 
-            return HandleRequestResult.Fail("No message.");
+            return HandleRequestResults.NoMessage;
         }
 
         try
@@ -175,7 +175,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync
             {
                 if (!Options.AllowUnsolicitedLogins)
                 {
-                    return HandleRequestResult.Fail("Unsolicited logins are not allowed.");
+                    return HandleRequestResults.UnsolicitedLoginsNotAllowed;
                 }
             }
             else

Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ private async Task<AuthenticateResult> ValidateCertificateAsync(X509Certificate2`
`126`	`126`	`!Options.AllowedCertificateTypes.HasFlag(CertificateTypes.SelfSigned))`
`127`	`127`	`{`
`128`	`128`	`Logger.CertificateRejected("Self signed", clientCertificate.Subject);`
`129`		`- return AuthenticateResult.Fail("Options do not allow self signed certificates.");`
	`129`	`+ return AuthenticateResults.NoSelfSigned;`
`130`	`130`	`}`
`131`	`131`
`132`	`132`	`// If we have a chained cert, and they're not allowed, exit early and not bother with`
`@@ -135,7 +135,7 @@ private async Task<AuthenticateResult> ValidateCertificateAsync(X509Certificate2`
`135`	`135`	`!Options.AllowedCertificateTypes.HasFlag(CertificateTypes.Chained))`
`136`	`136`	`{`
`137`	`137`	`Logger.CertificateRejected("Chained", clientCertificate.Subject);`
`138`		`- return AuthenticateResult.Fail("Options do not allow chained certificates.");`
	`138`	`+ return AuthenticateResults.NoChainedCertificates;`
`139`	`139`	`}`
`140`	`140`
`141`	`141`	`var chainPolicy = BuildChainPolicy(clientCertificate, isCertificateSelfSigned);`
`@@ -153,7 +153,7 @@ private async Task<AuthenticateResult> ValidateCertificateAsync(X509Certificate2`
`153`	`153`	`chainErrors.Add($"{validationFailure.Status} {validationFailure.StatusInformation}");`
`154`	`154`	`}`
`155`	`155`	`Logger.CertificateFailedValidation(clientCertificate.Subject, chainErrors);`
`156`		`- return AuthenticateResult.Fail("Client certificate failed validation.");`
	`156`	`+ return AuthenticateResults.InvalidClientCertificate;`
`157`	`157`	`}`
`158`	`158`
`159`	`159`	`var certificateValidatedContext = new CertificateValidatedContext(Context, Scheme, Options)`
Original file line number	Diff line number	Diff line change
`@@ -147,21 +147,21 @@ private async Task<AuthenticateResult> ReadCookieTicket()`
`147`	`147`	`var ticket = Options.TicketDataFormat.Unprotect(cookie, GetTlsTokenBinding());`
`148`	`148`	`if (ticket == null)`
`149`	`149`	`{`
`150`		`- return AuthenticateResult.Fail("Unprotect ticket failed");`
	`150`	`+ return AuthenticateResults.FailedUnprotectingTicket;`
`151`	`151`	`}`
`152`	`152`
`153`	`153`	`if (Options.SessionStore != null)`
`154`	`154`	`{`
`155`	`155`	`var claim = ticket.Principal.Claims.FirstOrDefault(c => c.Type.Equals(SessionIdClaim));`
`156`	`156`	`if (claim == null)`
`157`	`157`	`{`
`158`		`- return AuthenticateResult.Fail("SessionId missing");`
	`158`	`+ return AuthenticateResults.MissingSessionId;`
`159`	`159`	`}`
`160`	`160`	`// Only store _sessionKey if it matches an existing session. Otherwise we'll create a new one.`
`161`	`161`	`ticket = await Options.SessionStore.RetrieveAsync(claim.Value, Context, Context.RequestAborted);`
`162`	`162`	`if (ticket == null)`
`163`	`163`	`{`
`164`		`- return AuthenticateResult.Fail("Identity missing in session store");`
	`164`	`+ return AuthenticateResults.MissingIdentityInSession;`
`165`	`165`	`}`
`166`	`166`	`_sessionKey = claim.Value;`
`167`	`167`	`}`
`@@ -178,7 +178,7 @@ private async Task<AuthenticateResult> ReadCookieTicket()`
`178`	`178`	`// Clear out the session key if its expired, so renew doesn't try to use it`
`179`	`179`	`_sessionKey = null;`
`180`	`180`	`}`
`181`		`- return AuthenticateResult.Fail("Ticket expired");`
	`181`	`+ return AuthenticateResults.ExpiredTicket;`
`182`	`182`	`}`
`183`	`183`
`184`	`184`	`// Finally we have a valid ticket`
`@@ -204,7 +204,7 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()`
`204`	`204`
`205`	`205`	`if (context.Principal == null)`
`206`	`206`	`{`
`207`		`- return AuthenticateResult.Fail("No principal.");`
	`207`	`+ return AuthenticateResults.NoPrincipal;`
`208`	`208`	`}`
`209`	`209`
`210`	`210`	`if (context.ShouldRenew)`
Original file line number	Diff line number	Diff line change
`@@ -178,7 +178,7 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()`
`178`	`178`	`return AuthenticateResult.Fail(authenticationFailedContext.Exception);`
`179`	`179`	`}`
`180`	`180`
`181`		`- return AuthenticateResult.Fail("No SecurityTokenValidator available for token.");`
	`181`	`+ return AuthenticateResults.ValidatorNotFound;`
`182`	`182`	`}`
`183`	`183`	`catch (Exception ex)`
`184`	`184`	`{`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ protected override async Task<HandleRequestResult> HandleRemoteAuthenticateAsync`
`61`	`61`
`62`	`62`	`if (properties == null)`
`63`	`63`	`{`
`64`		`- return HandleRequestResult.Fail("The oauth state was missing or invalid.");`
	`64`	`+ return HandleRequestResults.InvalidState;`
`65`	`65`	`}`
`66`	`66`
`67`	`67`	`// OAuth2 10.12 CSRF`