From 4605afe2ceb7973ccbc215d73cd568edc5147bee Mon Sep 17 00:00:00 2001 From: Jordan Dominion Date: Fri, 3 Jan 2025 18:27:47 -0500 Subject: [PATCH 1/3] Regression test for #2064 --- .../Live/Instance/InstanceTest.cs | 2 +- .../Live/Instance/RepositoryTest.cs | 28 ++++++++++++++++++- .../Live/TestLiveServer.cs | 4 +-- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/tests/Tgstation.Server.Tests/Live/Instance/InstanceTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/InstanceTest.cs index 8bcd5ae04b..f9188313d0 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/InstanceTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/InstanceTest.cs @@ -48,7 +48,7 @@ namespace Tgstation.Server.Tests.Live.Instance await using var engineTest = new EngineTest(instanceClient.Engine, instanceClient.Jobs, fileDownloader, instanceClient.Metadata, testVersion.Engine.Value); await using var chatTest = new ChatTest(instanceClient.ChatBots, instanceManagerClient, instanceClient.Jobs, instanceClient.Metadata); var configTest = new ConfigurationTest(instanceClient.Configuration, instanceClient.Metadata); - await using var repoTest = new RepositoryTest(instanceClient.Repository, instanceClient.Jobs); + await using var repoTest = new RepositoryTest(instanceClient, instanceClient.Repository, instanceClient.Jobs); await using var dmTest = new DeploymentTest(instanceClient, instanceClient.Jobs, dmPort, ddPort, lowPrioDeployment, testVersion); var byondTask = engineTest.Run(cancellationToken, out var firstInstall); diff --git a/tests/Tgstation.Server.Tests/Live/Instance/RepositoryTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/RepositoryTest.cs index f5599d88c9..cef8a33713 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/RepositoryTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/RepositoryTest.cs @@ -8,6 +8,7 @@ using System.Threading.Tasks; using Tgstation.Server.Api.Models; using Tgstation.Server.Api.Models.Request; using Tgstation.Server.Api.Models.Response; +using Tgstation.Server.Api.Rights; using Tgstation.Server.Client; using Tgstation.Server.Client.Components; @@ -15,11 +16,13 @@ namespace Tgstation.Server.Tests.Live.Instance { sealed class RepositoryTest : JobsRequiredTest { + readonly IInstanceClient instanceClient; readonly IRepositoryClient repositoryClient; - public RepositoryTest(IRepositoryClient repositoryClient, IJobsClient jobsClient) + public RepositoryTest(IInstanceClient instanceClient, IRepositoryClient repositoryClient, IJobsClient jobsClient) : base(jobsClient) { + this.instanceClient = instanceClient ?? throw new ArgumentNullException(nameof(instanceClient)); this.repositoryClient = repositoryClient ?? throw new ArgumentNullException(nameof(repositoryClient)); } @@ -141,6 +144,29 @@ namespace Tgstation.Server.Tests.Live.Instance var prNumber = 2; await TestMergeTests(updated, prNumber, cancellationToken); + + await RegressionTest2064(cancellationToken); + } + + async ValueTask RegressionTest2064(CancellationToken cancellationToken) + { + var oldPerms = await instanceClient.PermissionSets.Read(cancellationToken); + + var newPerms = await instanceClient.PermissionSets.Update(new InstancePermissionSetRequest + { + PermissionSetId = oldPerms.PermissionSetId, + RepositoryRights = RepositoryRights.SetSha, + }, cancellationToken); + + Assert.AreEqual(RepositoryRights.SetSha, newPerms.RepositoryRights); + + await ApiAssert.ThrowsException(async () => await repositoryClient.Read(cancellationToken)); + + await instanceClient.PermissionSets.Update(new InstancePermissionSetRequest + { + PermissionSetId = oldPerms.PermissionSetId, + RepositoryRights = oldPerms.RepositoryRights, + }, cancellationToken); } async ValueTask RecloneTest(CancellationToken cancellationToken) diff --git a/tests/Tgstation.Server.Tests/Live/TestLiveServer.cs b/tests/Tgstation.Server.Tests/Live/TestLiveServer.cs index 208a33cc76..d8042b4492 100644 --- a/tests/Tgstation.Server.Tests/Live/TestLiveServer.cs +++ b/tests/Tgstation.Server.Tests/Live/TestLiveServer.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Diagnostics; using System.IO; @@ -1942,7 +1942,7 @@ namespace Tgstation.Server.Tests.Live Assert.AreEqual(expectedStaged, currentDD.ActiveCompileJob.Job.Id.Value); Assert.IsNull(currentDD.StagedCompileJob); - await using var repoTestObj = new RepositoryTest(instanceClient.Repository, instanceClient.Jobs); + await using var repoTestObj = new RepositoryTest(instanceClient, instanceClient.Repository, instanceClient.Jobs); var repoTest = repoTestObj.RunPostTest(cancellationToken); await using var chatTestObj = new ChatTest(instanceClient.ChatBots, restAdminClient.Instances, instanceClient.Jobs, instance); await chatTestObj.RunPostTest(cancellationToken); From e7b1189620baaf03c2d23f6e164d07c7c7d87d57 Mon Sep 17 00:00:00 2001 From: Jordan Dominion Date: Fri, 3 Jan 2025 18:43:02 -0500 Subject: [PATCH 2/3] Fix UserEnabled role being OR'd with action authorization roles FUUUUUCCCKK!!!! Fixes #2064 --- src/Tgstation.Server.Host/Core/Application.cs | 5 ++++- .../Security/TgsAuthorizeAttribute.cs | 15 ++++++++++++--- .../Security/TgsGraphQLAuthorizeAttribute.cs | 8 ++++++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/Tgstation.Server.Host/Core/Application.cs b/src/Tgstation.Server.Host/Core/Application.cs index 2517f8502f..f0cf647c79 100644 --- a/src/Tgstation.Server.Host/Core/Application.cs +++ b/src/Tgstation.Server.Host/Core/Application.cs @@ -298,7 +298,10 @@ namespace Tgstation.Server.Host.Core services .AddScoped() .AddGraphQLServer() - .AddAuthorization() + .AddAuthorization( + options => options.AddPolicy( + TgsAuthorizeAttribute.PolicyName, + builder => builder.RequireRole(TgsAuthorizeAttribute.UserEnabledRole))) .ModifyOptions(options => { options.EnsureAllNodesCanBeResolved = true; diff --git a/src/Tgstation.Server.Host/Security/TgsAuthorizeAttribute.cs b/src/Tgstation.Server.Host/Security/TgsAuthorizeAttribute.cs index d03c2f9e6d..4f68a6e894 100644 --- a/src/Tgstation.Server.Host/Security/TgsAuthorizeAttribute.cs +++ b/src/Tgstation.Server.Host/Security/TgsAuthorizeAttribute.cs @@ -15,10 +15,15 @@ namespace Tgstation.Server.Host.Security [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)] sealed class TgsAuthorizeAttribute : AuthorizeAttribute { + /// + /// Policy used to apply global requirement of . + /// + public const string PolicyName = "Policy.UserEnabled"; + /// /// Role used to indicate access to the server is allowed. /// - public const string UserEnabledRole = "Core.UserEnabled"; + public const string UserEnabledRole = "Role.UserEnabled"; /// /// Gets the associated with the if any. @@ -130,8 +135,12 @@ namespace Tgstation.Server.Host.Security private TgsAuthorizeAttribute(IEnumerable roles) { var listRoles = roles.ToList(); - listRoles.Add(UserEnabledRole); - Roles = String.Join(",", listRoles); + if (listRoles.Count != 0) + { + Roles = String.Join(",", listRoles); + } + + Policy = PolicyName; } } } diff --git a/src/Tgstation.Server.Host/Security/TgsGraphQLAuthorizeAttribute.cs b/src/Tgstation.Server.Host/Security/TgsGraphQLAuthorizeAttribute.cs index e03e7e6081..2bd0bfac52 100644 --- a/src/Tgstation.Server.Host/Security/TgsGraphQLAuthorizeAttribute.cs +++ b/src/Tgstation.Server.Host/Security/TgsGraphQLAuthorizeAttribute.cs @@ -125,8 +125,12 @@ namespace Tgstation.Server.Host.Security private TgsGraphQLAuthorizeAttribute(IEnumerable roleNames) { var listRoles = roleNames.ToList(); - listRoles.Add(TgsAuthorizeAttribute.UserEnabledRole); - Roles = [.. listRoles]; + if (listRoles.Count != 0) + { + Roles = [.. listRoles]; + } + + Policy = TgsAuthorizeAttribute.PolicyName; Apply = ApplyPolicy.Validation; } } From db8459915d365d43159b2c6f5ff7beb264a9758d Mon Sep 17 00:00:00 2001 From: Jordan Dominion Date: Fri, 3 Jan 2025 18:59:27 -0500 Subject: [PATCH 3/3] Version bump to 6.12.3 --- build/Version.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/Version.props b/build/Version.props index 4f5fbf62a4..e189ea46d8 100644 --- a/build/Version.props +++ b/build/Version.props @@ -3,7 +3,7 @@ - 6.12.2 + 6.12.3 5.4.0 10.12.0 0.5.0