From 5d6c2edab0f45dbbec0a62e5b8786e4530036da6 Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Fri, 31 May 2019 15:43:00 -0700 Subject: [PATCH] Require ClaimsPrincipal.Identity.IsAuthenticated for SignIn by default (#9482) --- ...thentication.Abstractions.netcoreapp3.0.cs | 1 + .../src/AuthenticationOptions.cs | 6 +++ ...tCore.Authentication.Core.netcoreapp3.0.cs | 3 +- .../src/AuthenticationService.cs | 22 ++++++++- .../test/AuthenticationServiceTests.cs | 45 +++++++++++++++---- .../AuthMiddlewareAndFilterTestBase.cs | 10 ++++- .../Controllers/LoginController.cs | 4 +- .../Authentication/test/CookieTests.cs | 8 ++-- .../Authentication/test/PolicyTests.cs | 10 ++--- .../test/SharedAuthenticationTests.cs | 10 ++--- 10 files changed, 91 insertions(+), 28 deletions(-) diff --git a/src/Http/Authentication.Abstractions/ref/Microsoft.AspNetCore.Authentication.Abstractions.netcoreapp3.0.cs b/src/Http/Authentication.Abstractions/ref/Microsoft.AspNetCore.Authentication.Abstractions.netcoreapp3.0.cs index 07b96e1378..dd9628c89a 100644 --- a/src/Http/Authentication.Abstractions/ref/Microsoft.AspNetCore.Authentication.Abstractions.netcoreapp3.0.cs +++ b/src/Http/Authentication.Abstractions/ref/Microsoft.AspNetCore.Authentication.Abstractions.netcoreapp3.0.cs @@ -51,6 +51,7 @@ namespace Microsoft.AspNetCore.Authentication public string DefaultScheme { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public string DefaultSignInScheme { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public string DefaultSignOutScheme { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } + public bool RequireAuthenticatedSignIn { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public System.Collections.Generic.IDictionary SchemeMap { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public System.Collections.Generic.IEnumerable Schemes { get { throw null; } } public void AddScheme(string name, System.Action configureBuilder) { } diff --git a/src/Http/Authentication.Abstractions/src/AuthenticationOptions.cs b/src/Http/Authentication.Abstractions/src/AuthenticationOptions.cs index 2781a35757..2fc213d718 100644 --- a/src/Http/Authentication.Abstractions/src/AuthenticationOptions.cs +++ b/src/Http/Authentication.Abstractions/src/AuthenticationOptions.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Security.Claims; using Microsoft.AspNetCore.Http; namespace Microsoft.AspNetCore.Authentication @@ -89,5 +90,10 @@ namespace Microsoft.AspNetCore.Authentication /// Used as the default scheme by . /// public string DefaultForbidScheme { get; set; } + + /// + /// If true, SignIn should throw if attempted with a ClaimsPrincipal.Identity.IsAuthenticated = false. + /// + public bool RequireAuthenticatedSignIn { get; set; } = true; } } diff --git a/src/Http/Authentication.Core/ref/Microsoft.AspNetCore.Authentication.Core.netcoreapp3.0.cs b/src/Http/Authentication.Core/ref/Microsoft.AspNetCore.Authentication.Core.netcoreapp3.0.cs index 26a6b29d96..f12e27bcf2 100644 --- a/src/Http/Authentication.Core/ref/Microsoft.AspNetCore.Authentication.Core.netcoreapp3.0.cs +++ b/src/Http/Authentication.Core/ref/Microsoft.AspNetCore.Authentication.Core.netcoreapp3.0.cs @@ -33,8 +33,9 @@ namespace Microsoft.AspNetCore.Authentication } public partial class AuthenticationService : Microsoft.AspNetCore.Authentication.IAuthenticationService { - public AuthenticationService(Microsoft.AspNetCore.Authentication.IAuthenticationSchemeProvider schemes, Microsoft.AspNetCore.Authentication.IAuthenticationHandlerProvider handlers, Microsoft.AspNetCore.Authentication.IClaimsTransformation transform) { } + public AuthenticationService(Microsoft.AspNetCore.Authentication.IAuthenticationSchemeProvider schemes, Microsoft.AspNetCore.Authentication.IAuthenticationHandlerProvider handlers, Microsoft.AspNetCore.Authentication.IClaimsTransformation transform, Microsoft.Extensions.Options.IOptions options) { } public Microsoft.AspNetCore.Authentication.IAuthenticationHandlerProvider Handlers { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } + public Microsoft.AspNetCore.Authentication.AuthenticationOptions Options { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public Microsoft.AspNetCore.Authentication.IAuthenticationSchemeProvider Schemes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public Microsoft.AspNetCore.Authentication.IClaimsTransformation Transform { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } [System.Diagnostics.DebuggerStepThroughAttribute] diff --git a/src/Http/Authentication.Core/src/AuthenticationService.cs b/src/Http/Authentication.Core/src/AuthenticationService.cs index 3e46df2f24..ce6328ae75 100644 --- a/src/Http/Authentication.Core/src/AuthenticationService.cs +++ b/src/Http/Authentication.Core/src/AuthenticationService.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Authentication { @@ -20,11 +21,13 @@ namespace Microsoft.AspNetCore.Authentication /// The . /// The . /// The . - public AuthenticationService(IAuthenticationSchemeProvider schemes, IAuthenticationHandlerProvider handlers, IClaimsTransformation transform) + /// The . + public AuthenticationService(IAuthenticationSchemeProvider schemes, IAuthenticationHandlerProvider handlers, IClaimsTransformation transform, IOptions options) { Schemes = schemes; Handlers = handlers; Transform = transform; + Options = options.Value; } /// @@ -42,6 +45,11 @@ namespace Microsoft.AspNetCore.Authentication /// public IClaimsTransformation Transform { get; } + /// + /// The . + /// + public AuthenticationOptions Options { get; } + /// /// Authenticate for the specified authentication scheme. /// @@ -146,6 +154,18 @@ namespace Microsoft.AspNetCore.Authentication throw new ArgumentNullException(nameof(principal)); } + if (Options.RequireAuthenticatedSignIn) + { + if (principal.Identity == null) + { + throw new InvalidOperationException("SignInAsync when principal.Identity == null is not allowed when AuthenticationOptions.RequireAuthenticatedSignIn is true."); + } + if (!principal.Identity.IsAuthenticated) + { + throw new InvalidOperationException("SignInAsync when principal.Identity.IsAuthenticated is false is not allowed when AuthenticationOptions.RequireAuthenticatedSignIn is true."); + } + } + if (scheme == null) { var defaultScheme = await Schemes.GetDefaultSignInSchemeAsync(); diff --git a/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs b/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs index e21ea40d51..b62db22077 100644 --- a/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs +++ b/src/Http/Authentication.Core/test/AuthenticationServiceTests.cs @@ -57,6 +57,35 @@ namespace Microsoft.AspNetCore.Authentication Assert.Contains("base", ex.Message); } + [Fact] + public async Task CanOnlySignInWithIsAuthenticated() + { + var services = new ServiceCollection().AddOptions().AddAuthenticationCore(o => + { + o.AddScheme("signin", "whatever"); + }).BuildServiceProvider(); + var context = new DefaultHttpContext(); + context.RequestServices = services; + + var ex = await Assert.ThrowsAsync(() => context.SignInAsync("signin", new ClaimsPrincipal(), null)); + await context.SignInAsync("signin", new ClaimsPrincipal(new ClaimsIdentity("whatever")), null); + } + + [Fact] + public async Task CanSignInWithoutIsAuthenticated() + { + var services = new ServiceCollection().AddOptions().AddAuthenticationCore(o => + { + o.AddScheme("signin", "whatever"); + o.RequireAuthenticatedSignIn = false; + }).BuildServiceProvider(); + var context = new DefaultHttpContext(); + context.RequestServices = services; + + await context.SignInAsync("signin", new ClaimsPrincipal(), null); + await context.SignInAsync("signin", new ClaimsPrincipal(new ClaimsIdentity("whatever")), null); + } + [Fact] public async Task CanOnlySignInIfSupported() { @@ -70,12 +99,12 @@ namespace Microsoft.AspNetCore.Authentication var context = new DefaultHttpContext(); context.RequestServices = services; - await context.SignInAsync("uber", new ClaimsPrincipal(), null); - var ex = await Assert.ThrowsAsync(() => context.SignInAsync("base", new ClaimsPrincipal(), null)); + await context.SignInAsync("uber", new ClaimsPrincipal(new ClaimsIdentity("whatever")), null); + var ex = await Assert.ThrowsAsync(() => context.SignInAsync("base", new ClaimsPrincipal(new ClaimsIdentity("whatever")), null)); Assert.Contains("uber", ex.Message); Assert.Contains("signin", ex.Message); - await context.SignInAsync("signin", new ClaimsPrincipal(), null); - ex = await Assert.ThrowsAsync(() => context.SignInAsync("signout", new ClaimsPrincipal(), null)); + await context.SignInAsync("signin", new ClaimsPrincipal(new ClaimsIdentity("whatever")), null); + ex = await Assert.ThrowsAsync(() => context.SignInAsync("signout", new ClaimsPrincipal(new ClaimsIdentity("whatever")), null)); Assert.Contains("uber", ex.Message); Assert.Contains("signin", ex.Message); } @@ -117,7 +146,7 @@ namespace Microsoft.AspNetCore.Authentication await context.ForbidAsync(); var ex = await Assert.ThrowsAsync(() => context.SignOutAsync()); Assert.Contains("cannot be used for SignOutAsync", ex.Message); - ex = await Assert.ThrowsAsync(() => context.SignInAsync(new ClaimsPrincipal())); + ex = await Assert.ThrowsAsync(() => context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever")))); Assert.Contains("cannot be used for SignInAsync", ex.Message); } @@ -136,7 +165,7 @@ namespace Microsoft.AspNetCore.Authentication await context.ChallengeAsync(); await context.ForbidAsync(); await context.SignOutAsync(); - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); } [Fact] @@ -154,7 +183,7 @@ namespace Microsoft.AspNetCore.Authentication await context.ChallengeAsync(); await context.ForbidAsync(); await context.SignOutAsync(); - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); } [Fact] @@ -172,7 +201,7 @@ namespace Microsoft.AspNetCore.Authentication await context.ChallengeAsync(); await context.ForbidAsync(); await context.SignOutAsync(); - var ex = await Assert.ThrowsAsync(() => context.SignInAsync(new ClaimsPrincipal())); + var ex = await Assert.ThrowsAsync(() => context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever")))); Assert.Contains("cannot be used for SignInAsync", ex.Message); } diff --git a/src/Mvc/test/Mvc.FunctionalTests/AuthMiddlewareAndFilterTestBase.cs b/src/Mvc/test/Mvc.FunctionalTests/AuthMiddlewareAndFilterTestBase.cs index 7f4c205109..148a2fd3ab 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/AuthMiddlewareAndFilterTestBase.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/AuthMiddlewareAndFilterTestBase.cs @@ -66,7 +66,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests request.Headers.Add("Cookie", authCookie); response = await Client.SendAsync(request); - await AssertAuthorizeResponse(response); + await AssertForbiddenResponse(response); authCookie = await GetAuthCookieAsync("LoginClaimAB"); request = new HttpRequestMessage(HttpMethod.Get, action); @@ -243,7 +243,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests request.Headers.Add("Cookie", authCookie); response = await Client.SendAsync(request); - await AssertAuthorizeResponse(response); + await AssertForbiddenResponse(response); authCookie = await GetAuthCookieAsync("LoginClaimAB"); request = new HttpRequestMessage(HttpMethod.Get, url); @@ -259,6 +259,12 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal("/Account/Login", response.Headers.Location.LocalPath); } + private async Task AssertForbiddenResponse(HttpResponseMessage response) + { + await response.AssertStatusCodeAsync(HttpStatusCode.Redirect); + Assert.Equal("/Account/AccessDenied", response.Headers.Location.LocalPath); + } + private async Task GetAuthCookieAsync(string action) { var response = await Client.PostAsync($"Login/{action}", null); diff --git a/src/Mvc/test/WebSites/SecurityWebSite/Controllers/LoginController.cs b/src/Mvc/test/WebSites/SecurityWebSite/Controllers/LoginController.cs index e5b80fc48d..044cc39d49 100644 --- a/src/Mvc/test/WebSites/SecurityWebSite/Controllers/LoginController.cs +++ b/src/Mvc/test/WebSites/SecurityWebSite/Controllers/LoginController.cs @@ -24,7 +24,7 @@ namespace SecurityWebSite.Controllers [HttpPost] public async Task LoginClaimA() { - var identity = new ClaimsIdentity(new[] { new Claim("ClaimA", "Value") }); + var identity = new ClaimsIdentity(new[] { new Claim("ClaimA", "Value") }, CookieAuthenticationDefaults.AuthenticationScheme); await HttpContext.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, new ClaimsPrincipal(identity)); return Ok(); } @@ -32,7 +32,7 @@ namespace SecurityWebSite.Controllers [HttpPost] public async Task LoginClaimAB() { - var identity = new ClaimsIdentity(new[] { new Claim("ClaimA", "Value"), new Claim("ClaimB", "Value") }); + var identity = new ClaimsIdentity(new[] { new Claim("ClaimA", "Value"), new Claim("ClaimB", "Value") }, CookieAuthenticationDefaults.AuthenticationScheme); await HttpContext.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, new ClaimsPrincipal(identity)); return Ok(); } diff --git a/src/Security/Authentication/test/CookieTests.cs b/src/Security/Authentication/test/CookieTests.cs index 4a72567844..7e66070b48 100644 --- a/src/Security/Authentication/test/CookieTests.cs +++ b/src/Security/Authentication/test/CookieTests.cs @@ -1001,7 +1001,7 @@ namespace Microsoft.AspNetCore.Authentication.Cookies .Configure(app => { app.UseAuthentication(); - app.Run(context => context.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, new ClaimsPrincipal(new ClaimsIdentity()))); + app.Run(context => context.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, new ClaimsPrincipal(new ClaimsIdentity("whatever")))); }) .ConfigureServices(services => { @@ -1024,7 +1024,7 @@ namespace Microsoft.AspNetCore.Authentication.Cookies .Configure(app => { app.UseAuthentication(); - app.Run(context => context.SignInAsync("Cookie1", new ClaimsPrincipal(new ClaimsIdentity()))); + app.Run(context => context.SignInAsync("Cookie1", new ClaimsPrincipal(new ClaimsIdentity("whatever")))); }) .ConfigureServices(services => { @@ -1048,7 +1048,7 @@ namespace Microsoft.AspNetCore.Authentication.Cookies { app.UseAuthentication(); app.Map("/notlogin", signoutApp => signoutApp.Run(context => context.SignInAsync("Cookies", - new ClaimsPrincipal()))); + new ClaimsPrincipal(new ClaimsIdentity("whatever"))))); }) .ConfigureServices(services => services.AddAuthentication().AddCookie(o => o.LoginPath = new PathString("/login"))); var server = new TestServer(builder); @@ -1065,7 +1065,7 @@ namespace Microsoft.AspNetCore.Authentication.Cookies .Configure(app => { app.UseAuthentication(); - app.Map("/login", signoutApp => signoutApp.Run(context => context.SignInAsync("Cookies", new ClaimsPrincipal()))); + app.Map("/login", signoutApp => signoutApp.Run(context => context.SignInAsync("Cookies", new ClaimsPrincipal(new ClaimsIdentity("whatever"))))); }) .ConfigureServices(services => services.AddAuthentication().AddCookie(o => o.LoginPath = new PathString("/login"))); diff --git a/src/Security/Authentication/test/PolicyTests.cs b/src/Security/Authentication/test/PolicyTests.cs index 60e6c41be6..77d764e14b 100644 --- a/src/Security/Authentication/test/PolicyTests.cs +++ b/src/Security/Authentication/test/PolicyTests.cs @@ -101,7 +101,7 @@ namespace Microsoft.AspNetCore.Authentication Assert.Equal(1, handler1.SignOutCount); Assert.Equal(0, handler2.SignOutCount); - await context.SignInAsync("forward", new ClaimsPrincipal()); + await context.SignInAsync("forward", new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, handler1.SignInCount); Assert.Equal(0, handler2.SignInCount); } @@ -156,7 +156,7 @@ namespace Microsoft.AspNetCore.Authentication Assert.Equal(1, handler1.SignOutCount); Assert.Equal(0, handler2.SignOutCount); - await context.SignInAsync("forward", new ClaimsPrincipal()); + await context.SignInAsync("forward", new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, handler1.SignInCount); Assert.Equal(0, handler2.SignInCount); } @@ -216,7 +216,7 @@ namespace Microsoft.AspNetCore.Authentication Assert.Equal(1, handler1.SignOutCount); Assert.Equal(0, handler2.SignOutCount); - await context.SignInAsync("forward", new ClaimsPrincipal()); + await context.SignInAsync("forward", new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, handler1.SignInCount); Assert.Equal(0, handler2.SignInCount); } @@ -268,7 +268,7 @@ namespace Microsoft.AspNetCore.Authentication Assert.Equal(1, handler1.SignOutCount); Assert.Equal(0, handler2.SignOutCount); - await context.SignInAsync("forward", new ClaimsPrincipal()); + await context.SignInAsync("forward", new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, handler1.SignInCount); Assert.Equal(0, handler2.SignInCount); } @@ -325,7 +325,7 @@ namespace Microsoft.AspNetCore.Authentication Assert.Equal(1, handler1.SignOutCount); Assert.Equal(0, handler2.SignOutCount); - await context.SignInAsync("forward", new ClaimsPrincipal()); + await context.SignInAsync("forward", new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(0, handler1.SignInCount); Assert.Equal(1, handler2.SignInCount); } diff --git a/src/Security/Authentication/test/SharedAuthenticationTests.cs b/src/Security/Authentication/test/SharedAuthenticationTests.cs index 4590c4915d..36956f8374 100644 --- a/src/Security/Authentication/test/SharedAuthenticationTests.cs +++ b/src/Security/Authentication/test/SharedAuthenticationTests.cs @@ -70,7 +70,7 @@ namespace Microsoft.AspNetCore.Authentication if (SupportsSignIn) { - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, forwardDefault.SignInCount); } else @@ -107,7 +107,7 @@ namespace Microsoft.AspNetCore.Authentication var context = new DefaultHttpContext(); context.RequestServices = sp; - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, specific.SignInCount); Assert.Equal(0, specific.AuthenticateCount); Assert.Equal(0, specific.ForbidCount); @@ -330,7 +330,7 @@ namespace Microsoft.AspNetCore.Authentication if (SupportsSignIn) { - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, selector.SignInCount); } else @@ -399,7 +399,7 @@ namespace Microsoft.AspNetCore.Authentication if (SupportsSignIn) { - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, forwardDefault.SignInCount); } else @@ -473,7 +473,7 @@ namespace Microsoft.AspNetCore.Authentication if (SupportsSignIn) { - await context.SignInAsync(new ClaimsPrincipal()); + await context.SignInAsync(new ClaimsPrincipal(new ClaimsIdentity("whatever"))); Assert.Equal(1, specific.SignInCount); } else