From 1d239a30ca0bf741316fe73f2b307876eaad0264 Mon Sep 17 00:00:00 2001 From: Maarten Date: Tue, 4 Jul 2023 09:37:13 +0200 Subject: [PATCH] Fix broken CookieAuthenticationRedirect caused by PR #14036 for non-api requests (#14399) * Fix broken CookieAuthenticationRedirect caused by PR #14036 when not in an API controller * Added Integration Tests for the MemberAuthorizationFilter * Fix merge conflict --------- Co-authored-by: Elitsa --- .../Filters/UmbracoMemberAuthorizeFilter.cs | 6 +- .../Security/ConfigureMemberCookieOptions.cs | 13 +- .../Security/MemberAuthorizeTests.cs | 126 ++++++++++++++++++ 3 files changed, 141 insertions(+), 4 deletions(-) create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Web.Website/Security/MemberAuthorizeTests.cs diff --git a/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs b/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs index 95c4ae5cec..2f56cdb51f 100644 --- a/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs +++ b/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs @@ -60,14 +60,14 @@ public class UmbracoMemberAuthorizeFilter : IAsyncAuthorizationFilter { context.HttpContext.SetReasonPhrase( "Resource restricted: the member is not of a permitted type or group."); + context.HttpContext.Response.StatusCode = 403; context.Result = new ForbidResult(); } } else { - context.HttpContext.SetReasonPhrase( - "Resource restricted: the member is not logged in."); - context.Result = new UnauthorizedResult(); + context.HttpContext.Response.StatusCode = 401; + context.Result = new ForbidResult(); } } diff --git a/src/Umbraco.Web.Common/Security/ConfigureMemberCookieOptions.cs b/src/Umbraco.Web.Common/Security/ConfigureMemberCookieOptions.cs index b8c2874641..1ba9a52526 100644 --- a/src/Umbraco.Web.Common/Security/ConfigureMemberCookieOptions.cs +++ b/src/Umbraco.Web.Common/Security/ConfigureMemberCookieOptions.cs @@ -1,10 +1,12 @@ using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Web.Common.Controllers; using Umbraco.Extensions; namespace Umbraco.Cms.Web.Common.Security; @@ -58,7 +60,16 @@ public sealed class ConfigureMemberCookieOptions : IConfigureNamedOptions { - ctx.Response.StatusCode = StatusCodes.Status403Forbidden; + // When the controller is an UmbracoAPIController, we want to return a StatusCode instead of a redirect. + // All other cases should use the default Redirect of the CookieAuthenticationEvent. + var controllerDescriptor = ctx.HttpContext.GetEndpoint()?.Metadata + .OfType() + .FirstOrDefault(); + + if (!controllerDescriptor?.ControllerTypeInfo.IsSubclassOf(typeof(UmbracoApiController)) ?? false) + { + new CookieAuthenticationEvents().OnRedirectToAccessDenied(ctx); + } return Task.CompletedTask; }, diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Web.Website/Security/MemberAuthorizeTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Web.Website/Security/MemberAuthorizeTests.cs new file mode 100644 index 0000000000..0fc1dfa85d --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Web.Website/Security/MemberAuthorizeTests.cs @@ -0,0 +1,126 @@ +using System.Net; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Logging; +using Umbraco.Cms.Core.Routing; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Core.Web; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Tests.Integration.TestServerTest; +using Umbraco.Cms.Web.Common.Controllers; +using Umbraco.Cms.Web.Common.Filters; +using Umbraco.Cms.Web.Common.Security; +using Umbraco.Cms.Web.Website.Controllers; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Web.Website.Security +{ + public class MemberAuthorizeTests : UmbracoTestServerTestBase + { + private Mock _memberManagerMock = new(); + + protected override void ConfigureTestServices(IServiceCollection services) + { + _memberManagerMock = new Mock(); + services.Remove(new ServiceDescriptor(typeof(IMemberManager), typeof(MemberManager), ServiceLifetime.Scoped)); + services.Remove(new ServiceDescriptor(typeof(MemberManager), ServiceLifetime.Scoped)); + services.AddScoped(_ => _memberManagerMock.Object); + } + + [Test] + public async Task Secure_SurfaceController_Should_Return_Redirect_WhenNotLoggedIn() + { + _memberManagerMock.Setup(x => x.IsLoggedIn()).Returns(false); + + var url = PrepareSurfaceControllerUrl(x => x.Secure()); + + var response = await Client.GetAsync(url); + + var cookieAuthenticationOptions = Services.GetService>(); + Assert.AreEqual(HttpStatusCode.Redirect, response.StatusCode); + Assert.AreEqual(cookieAuthenticationOptions.Value.AccessDeniedPath.ToString(), response.Headers.Location?.AbsolutePath); + } + + [Test] + public async Task Secure_SurfaceController_Should_Return_Redirect_WhenNotAuthorized() + { + _memberManagerMock.Setup(x => x.IsLoggedIn()).Returns(true); + _memberManagerMock.Setup(x => x.IsMemberAuthorizedAsync( + It.IsAny>(), + It.IsAny>(), + It.IsAny>())) + .ReturnsAsync(false); + + var url = PrepareSurfaceControllerUrl(x => x.Secure()); + + var response = await Client.GetAsync(url); + + var cookieAuthenticationOptions = Services.GetService>(); + Assert.AreEqual(HttpStatusCode.Redirect, response.StatusCode); + Assert.AreEqual(cookieAuthenticationOptions.Value.AccessDeniedPath.ToString(), response.Headers.Location?.AbsolutePath); + } + + + [Test] + public async Task Secure_ApiController_Should_Return_Unauthorized_WhenNotLoggedIn() + { + _memberManagerMock.Setup(x => x.IsLoggedIn()).Returns(false); + var url = PrepareApiControllerUrl(x => x.Secure()); + + var response = await Client.GetAsync(url); + + Assert.AreEqual(HttpStatusCode.Unauthorized, response.StatusCode); + } + + [Test] + public async Task Secure_ApiController_Should_Return_Forbidden_WhenNotAuthorized() + { + _memberManagerMock.Setup(x => x.IsLoggedIn()).Returns(true); + _memberManagerMock.Setup(x => x.IsMemberAuthorizedAsync( + It.IsAny>(), + It.IsAny>(), + It.IsAny>())) + .ReturnsAsync(false); + + var url = PrepareApiControllerUrl(x => x.Secure()); + + var response = await Client.GetAsync(url); + + Assert.AreEqual(HttpStatusCode.Forbidden, response.StatusCode); + } + } + + public class TestSurfaceController : SurfaceController + { + public TestSurfaceController( + IUmbracoContextAccessor umbracoContextAccessor, + IUmbracoDatabaseFactory databaseFactory, + ServiceContext services, + AppCaches appCaches, + IProfilingLogger profilingLogger, + IPublishedUrlProvider publishedUrlProvider) + : base( + umbracoContextAccessor, + databaseFactory, + services, + appCaches, + profilingLogger, + publishedUrlProvider) + { + } + + [UmbracoMemberAuthorize] + public IActionResult Secure() => NoContent(); + } + + public class TestApiController : UmbracoApiController + { + [UmbracoMemberAuthorize] + public IActionResult Secure() => NoContent(); + } +}