From 8fa7ff647a769a2dc4e6eecb349ff8abd4596c83 Mon Sep 17 00:00:00 2001 From: Bill Thornton Date: Wed, 29 May 2024 14:35:41 -0400 Subject: [PATCH 1/5] Defer standard authentication checks to DefaultAuthorizationHandler --- .../FirstTimeSetupHandler.cs | 10 +-------- .../FirstTimeSetupHandlerTests.cs | 21 ++++++++++--------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs index 9b4e2182c5..28b493fa6f 100644 --- a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs +++ b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs @@ -32,16 +32,8 @@ namespace Jellyfin.Api.Auth.FirstTimeSetupPolicy { context.Fail(); } - else if (!requirement.RequireAdmin && context.User.IsInRole(UserRoles.Guest)) - { - context.Fail(); - } - else - { - // Any user-specific checks are handled in the DefaultAuthorizationHandler. - context.Succeed(requirement); - } + // Any user-specific checks are handled in the DefaultAuthorizationHandler. return Task.CompletedTask; } } diff --git a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs index 2e6ffb5f6a..06c0c108ec 100644 --- a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs +++ b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs @@ -52,10 +52,10 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy } [Theory] - [InlineData(UserRoles.Administrator, true)] - [InlineData(UserRoles.Guest, false)] - [InlineData(UserRoles.User, false)] - public async Task ShouldRequireAdministratorIfStartupWizardComplete(string userRole, bool shouldSucceed) + [InlineData(UserRoles.Administrator, false)] + [InlineData(UserRoles.Guest, true)] + [InlineData(UserRoles.User, true)] + public async Task ShouldRequireAdministratorIfStartupWizardComplete(string userRole, bool shouldFail) { TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); var claims = TestHelpers.SetupUser( @@ -66,14 +66,14 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy var context = new AuthorizationHandlerContext(_requirements, claims, null); await _firstTimeSetupHandler.HandleAsync(context); - Assert.Equal(shouldSucceed, context.HasSucceeded); + Assert.Equal(shouldFail, context.HasFailed); } [Theory] - [InlineData(UserRoles.Administrator, true)] - [InlineData(UserRoles.Guest, false)] - [InlineData(UserRoles.User, true)] - public async Task ShouldRequireUserIfNotRequiresAdmin(string userRole, bool shouldSucceed) + [InlineData(UserRoles.Administrator)] + [InlineData(UserRoles.Guest)] + [InlineData(UserRoles.User)] + public async Task ShouldDeferIfNotRequiresAdmin(string userRole) { TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); var claims = TestHelpers.SetupUser( @@ -87,7 +87,8 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy null); await _firstTimeSetupHandler.HandleAsync(context); - Assert.Equal(shouldSucceed, context.HasSucceeded); + Assert.False(context.HasSucceeded); + Assert.False(context.HasFailed); } [Fact] From 35962bcc420444f4aa23c1e1fa4f950de7ea9715 Mon Sep 17 00:00:00 2001 From: Bill Thornton Date: Thu, 30 May 2024 12:08:52 -0400 Subject: [PATCH 2/5] Fix FirstTimeSetupHandler api key test --- .../Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs | 5 +++++ .../Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs index 28b493fa6f..d33b59350b 100644 --- a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs +++ b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs @@ -1,5 +1,6 @@ using System.Threading.Tasks; using Jellyfin.Api.Constants; +using Jellyfin.Api.Extensions; using MediaBrowser.Common.Configuration; using Microsoft.AspNetCore.Authorization; @@ -28,6 +29,10 @@ namespace Jellyfin.Api.Auth.FirstTimeSetupPolicy { context.Succeed(requirement); } + else if (context.User.GetIsApiKey()) + { + context.Succeed(requirement); + } else if (requirement.RequireAdmin && !context.User.IsInRole(UserRoles.Administrator)) { context.Fail(); diff --git a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs index 06c0c108ec..c2bbc6ba0c 100644 --- a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs +++ b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs @@ -95,7 +95,7 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy public async Task ShouldAllowAdminApiKeyIfStartupWizardComplete() { TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); - var claims = new ClaimsPrincipal(new ClaimsIdentity([new Claim(ClaimTypes.Role, UserRoles.Administrator)])); + var claims = new ClaimsPrincipal(new ClaimsIdentity([new Claim(InternalClaimTypes.IsApiKey, bool.TrueString)])); var context = new AuthorizationHandlerContext(_requirements, claims, null); await _firstTimeSetupHandler.HandleAsync(context); From a71e2d9f0a74cae555e1c7e4c8a40e6ad147841a Mon Sep 17 00:00:00 2001 From: Bill Thornton Date: Fri, 31 May 2024 03:18:33 -0400 Subject: [PATCH 3/5] Update FirstTimeSetupHandler.cs Co-authored-by: Claus Vium --- Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs index d33b59350b..e26214371c 100644 --- a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs +++ b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs @@ -29,7 +29,7 @@ namespace Jellyfin.Api.Auth.FirstTimeSetupPolicy { context.Succeed(requirement); } - else if (context.User.GetIsApiKey()) + else if (context.User.IsInRole(UserRoles.Administrator)) { context.Succeed(requirement); } From 7221e7ca68558e3d3d4eaf3d002289b15104a81f Mon Sep 17 00:00:00 2001 From: Bill Thornton Date: Fri, 31 May 2024 14:09:04 -0400 Subject: [PATCH 4/5] Fix FirstTimeSetupHandler failing for users and update tests --- .../FirstTimeSetupHandler.cs | 13 ++- .../FirstTimeSetupHandlerTests.cs | 99 ++++++++++++------- 2 files changed, 77 insertions(+), 35 deletions(-) diff --git a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs index e26214371c..c5c87056df 100644 --- a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs +++ b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs @@ -25,19 +25,30 @@ namespace Jellyfin.Api.Auth.FirstTimeSetupPolicy /// protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, FirstTimeSetupRequirement requirement) { + // Succeed if the startup wizard / first time setup is not complete if (!_configurationManager.CommonConfiguration.IsStartupWizardCompleted) { context.Succeed(requirement); } - else if (context.User.IsInRole(UserRoles.Administrator)) + + // Succeed if user is admin or api key + else if (context.User.GetIsApiKey() || context.User.IsInRole(UserRoles.Administrator)) { context.Succeed(requirement); } + + // Fail if admin is required and user is not admin else if (requirement.RequireAdmin && !context.User.IsInRole(UserRoles.Administrator)) { context.Fail(); } + // Succeed if admin is not required and user is not guest + else if (!requirement.RequireAdmin && context.User.IsInRole(UserRoles.User)) + { + context.Succeed(requirement); + } + // Any user-specific checks are handled in the DefaultAuthorizationHandler. return Task.CompletedTask; } diff --git a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs index c2bbc6ba0c..35a24a1291 100644 --- a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs +++ b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs @@ -1,14 +1,19 @@ +using System; using System.Collections.Generic; using System.Security.Claims; using System.Threading.Tasks; using AutoFixture; using AutoFixture.AutoMoq; +using Jellyfin.Api.Auth.DefaultAuthorizationPolicy; using Jellyfin.Api.Auth.FirstTimeSetupPolicy; using Jellyfin.Api.Constants; +using Jellyfin.Data.Entities; +using Jellyfin.Data.Enums; using MediaBrowser.Common.Configuration; using MediaBrowser.Controller.Library; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; using Moq; using Xunit; @@ -18,7 +23,9 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy { private readonly Mock _configurationManagerMock; private readonly List _requirements; + private readonly DefaultAuthorizationHandler _defaultAuthorizationHandler; private readonly FirstTimeSetupHandler _firstTimeSetupHandler; + private readonly IAuthorizationService _authorizationService; private readonly Mock _userManagerMock; private readonly Mock _httpContextAccessor; @@ -31,6 +38,21 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy _httpContextAccessor = fixture.Freeze>(); _firstTimeSetupHandler = fixture.Create(); + _defaultAuthorizationHandler = fixture.Create(); + + var services = new ServiceCollection(); + services.AddAuthorizationCore(); + services.AddLogging(); + services.AddOptions(); + services.AddSingleton(_defaultAuthorizationHandler); + services.AddSingleton(_firstTimeSetupHandler); + services.AddAuthorization(options => + { + options.AddPolicy("FirstTime", policy => policy.Requirements.Add(new FirstTimeSetupRequirement())); + options.AddPolicy("FirstTimeNoAdmin", policy => policy.Requirements.Add(new FirstTimeSetupRequirement(false, false))); + options.AddPolicy("FirstTimeSchedule", policy => policy.Requirements.Add(new FirstTimeSetupRequirement(true, false))); + }); + _authorizationService = services.BuildServiceProvider().GetRequiredService(); } [Theory] @@ -45,17 +67,33 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy _httpContextAccessor, userRole); - var context = new AuthorizationHandlerContext(_requirements, claims, null); + var allowed = await _authorizationService.AuthorizeAsync(claims, "FirstTime"); - await _firstTimeSetupHandler.HandleAsync(context); - Assert.True(context.HasSucceeded); + Assert.True(allowed.Succeeded); } [Theory] - [InlineData(UserRoles.Administrator, false)] - [InlineData(UserRoles.Guest, true)] + [InlineData(UserRoles.Administrator, true)] + [InlineData(UserRoles.Guest, false)] + [InlineData(UserRoles.User, false)] + public async Task ShouldRequireAdministratorIfStartupWizardComplete(string userRole, bool shouldSucceed) + { + TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); + var claims = TestHelpers.SetupUser( + _userManagerMock, + _httpContextAccessor, + userRole); + + var allowed = await _authorizationService.AuthorizeAsync(claims, "FirstTime"); + + Assert.Equal(shouldSucceed, allowed.Succeeded); + } + + [Theory] + [InlineData(UserRoles.Administrator, true)] + [InlineData(UserRoles.Guest, false)] [InlineData(UserRoles.User, true)] - public async Task ShouldRequireAdministratorIfStartupWizardComplete(string userRole, bool shouldFail) + public async Task ShouldRequireUserIfNotAdministrator(string userRole, bool shouldSucceed) { TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); var claims = TestHelpers.SetupUser( @@ -63,32 +101,9 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy _httpContextAccessor, userRole); - var context = new AuthorizationHandlerContext(_requirements, claims, null); + var allowed = await _authorizationService.AuthorizeAsync(claims, "FirstTimeNoAdmin"); - await _firstTimeSetupHandler.HandleAsync(context); - Assert.Equal(shouldFail, context.HasFailed); - } - - [Theory] - [InlineData(UserRoles.Administrator)] - [InlineData(UserRoles.Guest)] - [InlineData(UserRoles.User)] - public async Task ShouldDeferIfNotRequiresAdmin(string userRole) - { - TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); - var claims = TestHelpers.SetupUser( - _userManagerMock, - _httpContextAccessor, - userRole); - - var context = new AuthorizationHandlerContext( - new List { new FirstTimeSetupRequirement(false, false) }, - claims, - null); - - await _firstTimeSetupHandler.HandleAsync(context); - Assert.False(context.HasSucceeded); - Assert.False(context.HasFailed); + Assert.Equal(shouldSucceed, allowed.Succeeded); } [Fact] @@ -96,10 +111,26 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy { TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); var claims = new ClaimsPrincipal(new ClaimsIdentity([new Claim(InternalClaimTypes.IsApiKey, bool.TrueString)])); - var context = new AuthorizationHandlerContext(_requirements, claims, null); - await _firstTimeSetupHandler.HandleAsync(context); - Assert.True(context.HasSucceeded); + var allowed = await _authorizationService.AuthorizeAsync(claims, "FirstTime"); + Assert.True(allowed.Succeeded); + } + + [Fact] + public async Task ShouldDisallowUserIfOutsideSchedule() + { + AccessSchedule[] accessSchedules = { new AccessSchedule(DynamicDayOfWeek.Everyday, 0, 0, Guid.Empty) }; + + TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); + var claims = TestHelpers.SetupUser( + _userManagerMock, + _httpContextAccessor, + UserRoles.User, + accessSchedules); + + var allowed = await _authorizationService.AuthorizeAsync(claims, "FirstTimeSchedule"); + + Assert.False(allowed.Succeeded); } } } From ed1b88035930de23eb91d57b2d58c20cc119e823 Mon Sep 17 00:00:00 2001 From: Bill Thornton Date: Fri, 31 May 2024 16:31:15 -0400 Subject: [PATCH 5/5] Remove api key check and simplify conditions --- .../Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs | 8 ++++---- .../FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs | 10 ---------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs index c5c87056df..e425000cd6 100644 --- a/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs +++ b/Jellyfin.Api/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandler.cs @@ -31,20 +31,20 @@ namespace Jellyfin.Api.Auth.FirstTimeSetupPolicy context.Succeed(requirement); } - // Succeed if user is admin or api key - else if (context.User.GetIsApiKey() || context.User.IsInRole(UserRoles.Administrator)) + // Succeed if user is admin + else if (context.User.IsInRole(UserRoles.Administrator)) { context.Succeed(requirement); } // Fail if admin is required and user is not admin - else if (requirement.RequireAdmin && !context.User.IsInRole(UserRoles.Administrator)) + else if (requirement.RequireAdmin) { context.Fail(); } // Succeed if admin is not required and user is not guest - else if (!requirement.RequireAdmin && context.User.IsInRole(UserRoles.User)) + else if (context.User.IsInRole(UserRoles.User)) { context.Succeed(requirement); } diff --git a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs index 35a24a1291..31d2b486b3 100644 --- a/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs +++ b/tests/Jellyfin.Api.Tests/Auth/FirstTimeSetupPolicy/FirstTimeSetupHandlerTests.cs @@ -106,16 +106,6 @@ namespace Jellyfin.Api.Tests.Auth.FirstTimeSetupPolicy Assert.Equal(shouldSucceed, allowed.Succeeded); } - [Fact] - public async Task ShouldAllowAdminApiKeyIfStartupWizardComplete() - { - TestHelpers.SetupConfigurationManager(_configurationManagerMock, true); - var claims = new ClaimsPrincipal(new ClaimsIdentity([new Claim(InternalClaimTypes.IsApiKey, bool.TrueString)])); - - var allowed = await _authorizationService.AuthorizeAsync(claims, "FirstTime"); - Assert.True(allowed.Succeeded); - } - [Fact] public async Task ShouldDisallowUserIfOutsideSchedule() {