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 <elm@umbraco.dk>

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();
         }
     }
 

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<Cookie
             },
             OnRedirectToAccessDenied = ctx =>
             {
-                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<ControllerActionDescriptor>()
+                    .FirstOrDefault();
+
+                if (!controllerDescriptor?.ControllerTypeInfo.IsSubclassOf(typeof(UmbracoApiController)) ?? false)
+                {
+                    new CookieAuthenticationEvents().OnRedirectToAccessDenied(ctx);
+                }
 
                 return Task.CompletedTask;
             },

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<IMemberManager> _memberManagerMock = new();
+
+        protected override void ConfigureTestServices(IServiceCollection services)
+        {
+            _memberManagerMock = new Mock<IMemberManager>();
+            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<TestSurfaceController>(x => x.Secure());
+
+            var response = await Client.GetAsync(url);
+
+            var cookieAuthenticationOptions = Services.GetService<IOptions<CookieAuthenticationOptions>>();
+            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<IEnumerable<string>>(),
+                    It.IsAny<IEnumerable<string>>(),
+                    It.IsAny<IEnumerable<int>>()))
+                .ReturnsAsync(false);
+
+            var url = PrepareSurfaceControllerUrl<TestSurfaceController>(x => x.Secure());
+
+            var response = await Client.GetAsync(url);
+
+            var cookieAuthenticationOptions = Services.GetService<IOptions<CookieAuthenticationOptions>>();
+            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<TestApiController>(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<IEnumerable<string>>(),
+                    It.IsAny<IEnumerable<string>>(),
+                     It.IsAny<IEnumerable<int>>()))
+                .ReturnsAsync(false);
+
+            var url = PrepareApiControllerUrl<TestApiController>(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();
+    }
+}