From 97626587dfdddf5debeb42bc2c5905f76e566874 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Mon, 4 Mar 2024 22:26:00 -0600 Subject: [PATCH 01/14] Implement validation of resource claim lineage --- .../Modules/SecurityRepositoryModule.cs | 5 ++ .../Claims/IResourceClaimsValidator.cs | 22 +++++++ .../Claims/ResourceClaimsValidator.cs | 58 +++++++++++++++++++ .../Repositories/SecurityRepository.cs | 12 +++- 4 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs create mode 100644 Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs diff --git a/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs b/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs index c4ee648736..b43fae0ebc 100644 --- a/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs +++ b/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs @@ -12,6 +12,7 @@ using EdFi.Ods.Common.Configuration; using EdFi.Ods.Common.Container; using EdFi.Security.DataAccess.Repositories; +using EdFi.Security.DataAccess.Claims; namespace EdFi.Ods.Api.Container.Modules { @@ -28,6 +29,10 @@ protected override void Load(ContainerBuilder builder) .As() .EnableInterfaceInterceptors() .SingleInstance(); + + builder.RegisterType() + .As() + .SingleInstance(); builder.RegisterType() .Named(InterceptorCacheKeys.Security) diff --git a/Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs b/Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs new file mode 100644 index 0000000000..e9a34a485a --- /dev/null +++ b/Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: Apache-2.0 +// Licensed to the Ed-Fi Alliance under one or more agreements. +// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. +// See the LICENSE and NOTICES files in the project root for more information. + +using EdFi.Security.DataAccess.Models; + +namespace EdFi.Security.DataAccess.Claims; + +/// +/// Defines a method for validating the lineage up the taxonomy of a resource claim. +/// +public interface IResourceClaimsValidator +{ + /// + /// Validate that the resource claim lineage for the resource claim is well-formed, ensuring + /// that there are no cycles, which would cause an infinite loop when evaluating claims. + /// Otherwise, an exception is thrown. + /// + /// The URI of the resource claim for which to validate the resource claim lineage. + public void ValidateResourceClaimLineageForResourceClaim(string resourceClaimUri); +} diff --git a/Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs b/Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs new file mode 100644 index 0000000000..d2daeaa96e --- /dev/null +++ b/Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: Apache-2.0 +// Licensed to the Ed-Fi Alliance under one or more agreements. +// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. +// See the LICENSE and NOTICES files in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Linq; +using EdFi.Security.DataAccess.Models; +using EdFi.Security.DataAccess.Repositories; + +namespace EdFi.Security.DataAccess.Claims; + +public class ResourceClaimsValidator : IResourceClaimsValidator +{ + private readonly ISecurityTableGateway _securityTableGateway; + + public ResourceClaimsValidator(ISecurityTableGateway securityTableGateway) + { + _securityTableGateway = securityTableGateway ?? throw new ArgumentNullException(nameof(securityTableGateway)); + } + + public void ValidateResourceClaimLineageForResourceClaim(string resourceClaimUri) + { + ResourceClaim resourceClaim; + HashSet visitedResourceClaimIds = new(); + + try + { + resourceClaim = _securityTableGateway.GetResourceClaims() + .SingleOrDefault(rc => rc.ClaimName.Equals(resourceClaimUri, StringComparison.OrdinalIgnoreCase)); + } + catch (InvalidOperationException ex) + { + // Use InvalidOperationException wrapper with custom message over InvalidOperationException + // thrown by Linq to communicate back to caller the problem with the configuration. + throw new InvalidOperationException($"Multiple resource claims with a claim name of '{resourceClaimUri}' were found in the Ed-Fi API's security configuration. Authorization cannot be performed.", ex); + } + + if(resourceClaim == null) + return; + + visitedResourceClaimIds.Add(resourceClaim.ResourceClaimId); + + while(resourceClaim.ParentResourceClaimId != null) + { + resourceClaim = _securityTableGateway + .GetResourceClaims().SingleOrDefault(rc => rc.ResourceClaimId == resourceClaim.ParentResourceClaimId); + + if (visitedResourceClaimIds.Contains(resourceClaim.ResourceClaimId)) + { + throw new InvalidOperationException($"The lineage for resource claim '{resourceClaimUri}' is not well-formed. A cycle was detected in the resource claim lineage."); + } + + visitedResourceClaimIds.Add(resourceClaim.ResourceClaimId); + } + } +} diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 313713618e..5685b3af9c 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Linq; +using EdFi.Security.DataAccess.Claims; using EdFi.Security.DataAccess.Models; using Action = EdFi.Security.DataAccess.Models.Action; @@ -17,10 +18,12 @@ namespace EdFi.Security.DataAccess.Repositories public class SecurityRepository : ISecurityRepository { private readonly ISecurityTableGateway _securityTableGateway; + private readonly IResourceClaimsValidator _resourceClaimsValidator; - public SecurityRepository(ISecurityTableGateway securityTableGateway) + public SecurityRepository(ISecurityTableGateway securityTableGateway, IResourceClaimsValidator resourceClaimsValidator) { _securityTableGateway = securityTableGateway ?? throw new ArgumentNullException(nameof(securityTableGateway)); + _resourceClaimsValidator = resourceClaimsValidator ?? throw new ArgumentNullException(nameof(resourceClaimsValidator)); } public virtual Action GetActionByName(string actionName) @@ -36,7 +39,6 @@ public virtual AuthorizationStrategy GetAuthorizationStrategyByName(string autho public virtual IList GetClaimsForClaimSet(string claimSetName) { - return _securityTableGateway.GetClaimSetResourceClaimActions() .Where(c => c.ClaimSet.ClaimSetName.Equals(claimSetName, StringComparison.OrdinalIgnoreCase)) .ToList(); @@ -49,6 +51,9 @@ public virtual IList GetClaimsForClaimSet(string cl /// The lineage of resource claim URIs. public virtual IList GetResourceClaimLineage(string resourceClaimUri) { + // Verify the resource claim lineage has no cycles + _resourceClaimsValidator.ValidateResourceClaimLineageForResourceClaim(resourceClaimUri); + return GetResourceClaimLineageForResourceClaim(resourceClaimUri) .Select(c => c.ClaimName) .ToList(); @@ -92,6 +97,9 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin /// The resource claim's lineage of authorization metadata. public virtual IList GetResourceClaimLineageMetadata(string resourceClaimUri, string action) { + // Verify the resource claim lineage has no cycles + _resourceClaimsValidator.ValidateResourceClaimLineageForResourceClaim(resourceClaimUri); + var strategies = new List(); AddStrategiesForResourceClaimLineage(strategies, resourceClaimUri, action); From 88ab6931f46bff8676c55c940594235d0620f4e0 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Mon, 4 Mar 2024 22:46:08 -0600 Subject: [PATCH 02/14] Empty-commit From 2315f4b4210c4b77b6b2343a717e3768e4b36b89 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Mon, 4 Mar 2024 23:52:10 -0600 Subject: [PATCH 03/14] Fix unit tests --- .../Repositories/SecurityRepositoryTests.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs index 050983966b..3e0b4bdc75 100644 --- a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs +++ b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs @@ -4,6 +4,7 @@ // See the LICENSE and NOTICES files in the project root for more information. using System.Linq; +using EdFi.Security.DataAccess.Claims; namespace EdFi.Security.DataAccess.UnitTests.Repositories; @@ -18,13 +19,15 @@ namespace EdFi.Security.DataAccess.UnitTests.Repositories; public class SecurityRepositoryTests { private ISecurityTableGateway _securityTableGateway; + private IResourceClaimsValidator _resourceClaimsValidator; private SecurityRepository _securityRepository; [SetUp] public void SetUp() { _securityTableGateway = A.Fake(); - _securityRepository = new SecurityRepository(_securityTableGateway); + _resourceClaimsValidator = A.Fake(); + _securityRepository = new SecurityRepository(_securityTableGateway, _resourceClaimsValidator); } [Test] @@ -212,7 +215,8 @@ public void GetResourceClaimLineageMetadata_WithExactMatch_ReturnsMatch() { // Arrange var securityTableGateway = A.Fake(); - var repository = new SecurityRepository(securityTableGateway); + var resourceClaimsValidator = A.Fake(); + var repository = new SecurityRepository(securityTableGateway, resourceClaimsValidator); var suppliedResourceClaimUri = "urn:some:resource"; var suppliedActionUri = "read"; @@ -247,7 +251,8 @@ public void GetResourceClaimLineageMetadata_WithParentResource_ReturnsMatchWithL { // Arrange var securityTableGateway = A.Fake(); - var repository = new SecurityRepository(securityTableGateway); + var resourceClaimsValidator = A.Fake(); + var repository = new SecurityRepository(securityTableGateway, resourceClaimsValidator); var suppliedParentResourceClaimUri = "urn:some:resource:parent"; var suppliedChildResourceClaimUri = "urn:some:resource:child"; @@ -310,7 +315,8 @@ public void GetResourceByResourceName_WithMatchingResourceName_ReturnsResourceCl { // Arrange var securityTableGateway = A.Fake(); - var repository = new SecurityRepository(securityTableGateway); + var resourceClaimsValidator = A.Fake(); + var repository = new SecurityRepository(securityTableGateway, resourceClaimsValidator); var resourceName = "Some Resource"; var resourceClaim = new ResourceClaim { ResourceName = resourceName }; From 7ea64e04101b616208f9d2b4d448e3035bec1b52 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Tue, 5 Mar 2024 09:31:19 -0600 Subject: [PATCH 04/14] Refactor implementation --- .../Modules/SecurityRepositoryModule.cs | 5 -- .../Claims/IResourceClaimsValidator.cs | 22 ------- .../Claims/ResourceClaimsValidator.cs | 58 ------------------- .../Repositories/SecurityRepository.cs | 29 ++++------ .../Repositories/SecurityRepositoryTests.cs | 14 ++--- 5 files changed, 15 insertions(+), 113 deletions(-) delete mode 100644 Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs delete mode 100644 Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs diff --git a/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs b/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs index b43fae0ebc..c4ee648736 100644 --- a/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs +++ b/Application/EdFi.Ods.Api/Container/Modules/SecurityRepositoryModule.cs @@ -12,7 +12,6 @@ using EdFi.Ods.Common.Configuration; using EdFi.Ods.Common.Container; using EdFi.Security.DataAccess.Repositories; -using EdFi.Security.DataAccess.Claims; namespace EdFi.Ods.Api.Container.Modules { @@ -29,10 +28,6 @@ protected override void Load(ContainerBuilder builder) .As() .EnableInterfaceInterceptors() .SingleInstance(); - - builder.RegisterType() - .As() - .SingleInstance(); builder.RegisterType() .Named(InterceptorCacheKeys.Security) diff --git a/Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs b/Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs deleted file mode 100644 index e9a34a485a..0000000000 --- a/Application/EdFi.Security.DataAccess/Claims/IResourceClaimsValidator.cs +++ /dev/null @@ -1,22 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Licensed to the Ed-Fi Alliance under one or more agreements. -// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. -// See the LICENSE and NOTICES files in the project root for more information. - -using EdFi.Security.DataAccess.Models; - -namespace EdFi.Security.DataAccess.Claims; - -/// -/// Defines a method for validating the lineage up the taxonomy of a resource claim. -/// -public interface IResourceClaimsValidator -{ - /// - /// Validate that the resource claim lineage for the resource claim is well-formed, ensuring - /// that there are no cycles, which would cause an infinite loop when evaluating claims. - /// Otherwise, an exception is thrown. - /// - /// The URI of the resource claim for which to validate the resource claim lineage. - public void ValidateResourceClaimLineageForResourceClaim(string resourceClaimUri); -} diff --git a/Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs b/Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs deleted file mode 100644 index d2daeaa96e..0000000000 --- a/Application/EdFi.Security.DataAccess/Claims/ResourceClaimsValidator.cs +++ /dev/null @@ -1,58 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Licensed to the Ed-Fi Alliance under one or more agreements. -// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. -// See the LICENSE and NOTICES files in the project root for more information. - -using System; -using System.Collections.Generic; -using System.Linq; -using EdFi.Security.DataAccess.Models; -using EdFi.Security.DataAccess.Repositories; - -namespace EdFi.Security.DataAccess.Claims; - -public class ResourceClaimsValidator : IResourceClaimsValidator -{ - private readonly ISecurityTableGateway _securityTableGateway; - - public ResourceClaimsValidator(ISecurityTableGateway securityTableGateway) - { - _securityTableGateway = securityTableGateway ?? throw new ArgumentNullException(nameof(securityTableGateway)); - } - - public void ValidateResourceClaimLineageForResourceClaim(string resourceClaimUri) - { - ResourceClaim resourceClaim; - HashSet visitedResourceClaimIds = new(); - - try - { - resourceClaim = _securityTableGateway.GetResourceClaims() - .SingleOrDefault(rc => rc.ClaimName.Equals(resourceClaimUri, StringComparison.OrdinalIgnoreCase)); - } - catch (InvalidOperationException ex) - { - // Use InvalidOperationException wrapper with custom message over InvalidOperationException - // thrown by Linq to communicate back to caller the problem with the configuration. - throw new InvalidOperationException($"Multiple resource claims with a claim name of '{resourceClaimUri}' were found in the Ed-Fi API's security configuration. Authorization cannot be performed.", ex); - } - - if(resourceClaim == null) - return; - - visitedResourceClaimIds.Add(resourceClaim.ResourceClaimId); - - while(resourceClaim.ParentResourceClaimId != null) - { - resourceClaim = _securityTableGateway - .GetResourceClaims().SingleOrDefault(rc => rc.ResourceClaimId == resourceClaim.ParentResourceClaimId); - - if (visitedResourceClaimIds.Contains(resourceClaim.ResourceClaimId)) - { - throw new InvalidOperationException($"The lineage for resource claim '{resourceClaimUri}' is not well-formed. A cycle was detected in the resource claim lineage."); - } - - visitedResourceClaimIds.Add(resourceClaim.ResourceClaimId); - } - } -} diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 5685b3af9c..079622c879 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -6,7 +6,6 @@ using System; using System.Collections.Generic; using System.Linq; -using EdFi.Security.DataAccess.Claims; using EdFi.Security.DataAccess.Models; using Action = EdFi.Security.DataAccess.Models.Action; @@ -18,12 +17,9 @@ namespace EdFi.Security.DataAccess.Repositories public class SecurityRepository : ISecurityRepository { private readonly ISecurityTableGateway _securityTableGateway; - private readonly IResourceClaimsValidator _resourceClaimsValidator; - - public SecurityRepository(ISecurityTableGateway securityTableGateway, IResourceClaimsValidator resourceClaimsValidator) + public SecurityRepository(ISecurityTableGateway securityTableGateway) { _securityTableGateway = securityTableGateway ?? throw new ArgumentNullException(nameof(securityTableGateway)); - _resourceClaimsValidator = resourceClaimsValidator ?? throw new ArgumentNullException(nameof(resourceClaimsValidator)); } public virtual Action GetActionByName(string actionName) @@ -51,17 +47,14 @@ public virtual IList GetClaimsForClaimSet(string cl /// The lineage of resource claim URIs. public virtual IList GetResourceClaimLineage(string resourceClaimUri) { - // Verify the resource claim lineage has no cycles - _resourceClaimsValidator.ValidateResourceClaimLineageForResourceClaim(resourceClaimUri); - return GetResourceClaimLineageForResourceClaim(resourceClaimUri) .Select(c => c.ClaimName) .ToList(); } - private IEnumerable GetResourceClaimLineageForResourceClaim(string resourceClaimUri) + private IEnumerable GetResourceClaimLineageForResourceClaim(string resourceClaimUri, HashSet resourceClaimLineage = null) { - var resourceClaimLineage = new List(); + resourceClaimLineage ??= new HashSet(); ResourceClaim resourceClaim; @@ -79,11 +72,14 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin if (resourceClaim != null) { - resourceClaimLineage.Add(resourceClaim); + if (!resourceClaimLineage.Add(resourceClaim)) + { + throw new InvalidOperationException($"The lineage for resource claim '{resourceClaimUri}' is not well-formed. A cycle was detected in the resource claim lineage."); + } if (resourceClaim.ParentResourceClaim != null) { - resourceClaimLineage.AddRange(GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName)); + resourceClaimLineage.UnionWith(GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName, resourceClaimLineage)); } } @@ -97,17 +93,14 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin /// The resource claim's lineage of authorization metadata. public virtual IList GetResourceClaimLineageMetadata(string resourceClaimUri, string action) { - // Verify the resource claim lineage has no cycles - _resourceClaimsValidator.ValidateResourceClaimLineageForResourceClaim(resourceClaimUri); - - var strategies = new List(); + var strategies = new HashSet(); AddStrategiesForResourceClaimLineage(strategies, resourceClaimUri, action); - return strategies; + return strategies.ToList(); } - private void AddStrategiesForResourceClaimLineage(List strategies, string resourceClaimUri, string action) + private void AddStrategiesForResourceClaimLineage(HashSet strategies, string resourceClaimUri, string action) { //check for exact match on resource and action var claimAndStrategy = _securityTableGateway.GetResourceClaimActionAuthorizations() diff --git a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs index 3e0b4bdc75..050983966b 100644 --- a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs +++ b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs @@ -4,7 +4,6 @@ // See the LICENSE and NOTICES files in the project root for more information. using System.Linq; -using EdFi.Security.DataAccess.Claims; namespace EdFi.Security.DataAccess.UnitTests.Repositories; @@ -19,15 +18,13 @@ namespace EdFi.Security.DataAccess.UnitTests.Repositories; public class SecurityRepositoryTests { private ISecurityTableGateway _securityTableGateway; - private IResourceClaimsValidator _resourceClaimsValidator; private SecurityRepository _securityRepository; [SetUp] public void SetUp() { _securityTableGateway = A.Fake(); - _resourceClaimsValidator = A.Fake(); - _securityRepository = new SecurityRepository(_securityTableGateway, _resourceClaimsValidator); + _securityRepository = new SecurityRepository(_securityTableGateway); } [Test] @@ -215,8 +212,7 @@ public void GetResourceClaimLineageMetadata_WithExactMatch_ReturnsMatch() { // Arrange var securityTableGateway = A.Fake(); - var resourceClaimsValidator = A.Fake(); - var repository = new SecurityRepository(securityTableGateway, resourceClaimsValidator); + var repository = new SecurityRepository(securityTableGateway); var suppliedResourceClaimUri = "urn:some:resource"; var suppliedActionUri = "read"; @@ -251,8 +247,7 @@ public void GetResourceClaimLineageMetadata_WithParentResource_ReturnsMatchWithL { // Arrange var securityTableGateway = A.Fake(); - var resourceClaimsValidator = A.Fake(); - var repository = new SecurityRepository(securityTableGateway, resourceClaimsValidator); + var repository = new SecurityRepository(securityTableGateway); var suppliedParentResourceClaimUri = "urn:some:resource:parent"; var suppliedChildResourceClaimUri = "urn:some:resource:child"; @@ -315,8 +310,7 @@ public void GetResourceByResourceName_WithMatchingResourceName_ReturnsResourceCl { // Arrange var securityTableGateway = A.Fake(); - var resourceClaimsValidator = A.Fake(); - var repository = new SecurityRepository(securityTableGateway, resourceClaimsValidator); + var repository = new SecurityRepository(securityTableGateway); var resourceName = "Some Resource"; var resourceClaim = new ResourceClaim { ResourceName = resourceName }; From 0b578c755b4d41ce7c9fdeba061fa43ef1f4dc67 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Tue, 5 Mar 2024 10:57:00 -0600 Subject: [PATCH 05/14] Cleanup --- .../Repositories/SecurityRepository.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 079622c879..abe3284165 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -17,6 +17,7 @@ namespace EdFi.Security.DataAccess.Repositories public class SecurityRepository : ISecurityRepository { private readonly ISecurityTableGateway _securityTableGateway; + public SecurityRepository(ISecurityTableGateway securityTableGateway) { _securityTableGateway = securityTableGateway ?? throw new ArgumentNullException(nameof(securityTableGateway)); @@ -93,14 +94,14 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin /// The resource claim's lineage of authorization metadata. public virtual IList GetResourceClaimLineageMetadata(string resourceClaimUri, string action) { - var strategies = new HashSet(); + var strategies = new List(); AddStrategiesForResourceClaimLineage(strategies, resourceClaimUri, action); - return strategies.ToList(); + return strategies; } - private void AddStrategiesForResourceClaimLineage(HashSet strategies, string resourceClaimUri, string action) + private void AddStrategiesForResourceClaimLineage(List strategies, string resourceClaimUri, string action) { //check for exact match on resource and action var claimAndStrategy = _securityTableGateway.GetResourceClaimActionAuthorizations() From 87230994d7c4aadb99e375f4a47884f2a9f12c03 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Tue, 5 Mar 2024 12:44:17 -0600 Subject: [PATCH 06/14] Switch HashSet to List and improve error message detail --- .../Repositories/SecurityRepository.cs | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index abe3284165..2a1d2ea0c3 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -53,10 +53,9 @@ public virtual IList GetResourceClaimLineage(string resourceClaimUri) .ToList(); } - private IEnumerable GetResourceClaimLineageForResourceClaim(string resourceClaimUri, HashSet resourceClaimLineage = null) + private IEnumerable GetResourceClaimLineageForResourceClaim(string resourceClaimUri, List resourceClaimLineage = null) { - resourceClaimLineage ??= new HashSet(); - + resourceClaimLineage ??= new List(); ResourceClaim resourceClaim; try @@ -71,17 +70,21 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin throw new InvalidOperationException($"Multiple resource claims with a claim name of '{resourceClaimUri}' were found in the Ed-Fi API's security configuration. Authorization cannot be performed.", ex); } - if (resourceClaim != null) + if (resourceClaim == null) + { + return resourceClaimLineage; + } + + if (resourceClaimLineage.Contains(resourceClaim)) + { + throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc.ResourceClaimId != resourceClaim.ResourceClaimId))}' --> '{resourceClaimUri}'"); + } + + resourceClaimLineage.Add(resourceClaim); + + if (resourceClaim.ParentResourceClaim != null) { - if (!resourceClaimLineage.Add(resourceClaim)) - { - throw new InvalidOperationException($"The lineage for resource claim '{resourceClaimUri}' is not well-formed. A cycle was detected in the resource claim lineage."); - } - - if (resourceClaim.ParentResourceClaim != null) - { - resourceClaimLineage.UnionWith(GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName, resourceClaimLineage)); - } + resourceClaimLineage.AddRange(GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName, resourceClaimLineage)); } return resourceClaimLineage; From 813d2f8c69a8b44be5543cc4f4601a39813601a6 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Tue, 5 Mar 2024 12:44:49 -0600 Subject: [PATCH 07/14] Add unit test --- .../Repositories/SecurityRepositoryTests.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs index 050983966b..96753e36c3 100644 --- a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs +++ b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs @@ -3,6 +3,7 @@ // The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0. // See the LICENSE and NOTICES files in the project root for more information. +using System; using System.Linq; namespace EdFi.Security.DataAccess.UnitTests.Repositories; @@ -165,6 +166,24 @@ public void GetResourceClaimLineage_ShouldReturnEmptyListForInvalidResourceClaim // Assert lineage.ShouldBeEmpty(); } + + [Test] + public void GetResourceClaimLineage_ThrowsException_WhenResourceClaimLineageContainsCycle() + { + // Arrange + var resourceClaimA = new ResourceClaim { ClaimName = "uri-A", ResourceClaimId = 1, ParentResourceClaim = null, ParentResourceClaimId = null}; + var resourceClaimB = new ResourceClaim { ClaimName = "uri-B", ResourceClaimId = 2, ParentResourceClaim = resourceClaimA, ParentResourceClaimId = 1}; + var resourceClaimC = new ResourceClaim { ClaimName = "uri-C", ResourceClaimId = 3, ParentResourceClaim = resourceClaimB, ParentResourceClaimId = 2}; + + resourceClaimA.ParentResourceClaimId = resourceClaimC.ResourceClaimId; + resourceClaimA.ParentResourceClaim = resourceClaimC; + + A.CallTo(() => _securityTableGateway.GetResourceClaims()) + .Returns(new List { resourceClaimA, resourceClaimB, resourceClaimC }); + + // Act & Assert + Should.Throw(() => _securityRepository.GetResourceClaimLineage("uri-C")); + } [Test] public void GetResourceClaimLineageMetadata_ShouldReturnMetadataForValidResourceClaimAndAction() From 4ab3b19d6ebc109d9e847b6f52a1f92bd453cef0 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Tue, 5 Mar 2024 12:57:18 -0600 Subject: [PATCH 08/14] Fix error message --- .../EdFi.Security.DataAccess/Repositories/SecurityRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 2a1d2ea0c3..7590c013d0 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -77,7 +77,7 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin if (resourceClaimLineage.Contains(resourceClaim)) { - throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc.ResourceClaimId != resourceClaim.ResourceClaimId))}' --> '{resourceClaimUri}'"); + throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc.ResourceClaimId != resourceClaim.ResourceClaimId).Select(rc => rc.ClaimName))}' --> '{resourceClaimUri}'"); } resourceClaimLineage.Add(resourceClaim); From 70f4abfb4faca922fa34b42ed294d7837f20ca56 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Tue, 5 Mar 2024 13:26:46 -0600 Subject: [PATCH 09/14] Fix recursion logic --- .../Repositories/SecurityRepository.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 7590c013d0..611bb31a2d 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -77,14 +77,14 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin if (resourceClaimLineage.Contains(resourceClaim)) { - throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc.ResourceClaimId != resourceClaim.ResourceClaimId).Select(rc => rc.ClaimName))}' --> '{resourceClaimUri}'"); + throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc != resourceClaim).Select(rc => rc.ClaimName))}' --> '{resourceClaimUri}'"); } resourceClaimLineage.Add(resourceClaim); if (resourceClaim.ParentResourceClaim != null) { - resourceClaimLineage.AddRange(GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName, resourceClaimLineage)); + GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName, resourceClaimLineage); } return resourceClaimLineage; From 0beab6d07454a448e2984032a6beec745eb51a75 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Wed, 6 Mar 2024 14:11:17 -0600 Subject: [PATCH 10/14] Use consistent arrows in error message --- .../EdFi.Security.DataAccess/Repositories/SecurityRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 611bb31a2d..40de14b348 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -77,7 +77,7 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin if (resourceClaimLineage.Contains(resourceClaim)) { - throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc != resourceClaim).Select(rc => rc.ClaimName))}' --> '{resourceClaimUri}'"); + throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc != resourceClaim).Select(rc => rc.ClaimName))}' -> '{resourceClaimUri}'"); } resourceClaimLineage.Add(resourceClaim); From 644a964073eaae2aae4d510c585babf5d3cc7f38 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Wed, 6 Mar 2024 14:11:55 -0600 Subject: [PATCH 11/14] Check error message content in unit test --- .../Repositories/SecurityRepositoryTests.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs index 96753e36c3..44b0ee9cfb 100644 --- a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs +++ b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs @@ -174,7 +174,8 @@ public void GetResourceClaimLineage_ThrowsException_WhenResourceClaimLineageCont var resourceClaimA = new ResourceClaim { ClaimName = "uri-A", ResourceClaimId = 1, ParentResourceClaim = null, ParentResourceClaimId = null}; var resourceClaimB = new ResourceClaim { ClaimName = "uri-B", ResourceClaimId = 2, ParentResourceClaim = resourceClaimA, ParentResourceClaimId = 1}; var resourceClaimC = new ResourceClaim { ClaimName = "uri-C", ResourceClaimId = 3, ParentResourceClaim = resourceClaimB, ParentResourceClaimId = 2}; - + var resourceClaimD = new ResourceClaim { ClaimName = "uri-D", ResourceClaimId = 4, ParentResourceClaim = resourceClaimB, ParentResourceClaimId = 3}; + resourceClaimA.ParentResourceClaimId = resourceClaimC.ResourceClaimId; resourceClaimA.ParentResourceClaim = resourceClaimC; @@ -182,7 +183,8 @@ public void GetResourceClaimLineage_ThrowsException_WhenResourceClaimLineageCont .Returns(new List { resourceClaimA, resourceClaimB, resourceClaimC }); // Act & Assert - Should.Throw(() => _securityRepository.GetResourceClaimLineage("uri-C")); + Should.Throw(() => _securityRepository.GetResourceClaimLineage("uri-C")) + .Message.ShouldBe("A cycle was detected in the resource claim hierarchy of the security metadata: 'uri-C' -> 'uri-B' -> 'uri-A' -> 'uri-C'"); } [Test] From 58a523791d05df7a409f6f843fabfc8ddb2aa8b8 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Thu, 7 Mar 2024 12:17:46 -0600 Subject: [PATCH 12/14] Fix unit test --- .../Repositories/SecurityRepositoryTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs index 44b0ee9cfb..1bc4a348b6 100644 --- a/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs +++ b/tests/EdFi.Security.DataAccess.UnitTests/Repositories/SecurityRepositoryTests.cs @@ -174,16 +174,16 @@ public void GetResourceClaimLineage_ThrowsException_WhenResourceClaimLineageCont var resourceClaimA = new ResourceClaim { ClaimName = "uri-A", ResourceClaimId = 1, ParentResourceClaim = null, ParentResourceClaimId = null}; var resourceClaimB = new ResourceClaim { ClaimName = "uri-B", ResourceClaimId = 2, ParentResourceClaim = resourceClaimA, ParentResourceClaimId = 1}; var resourceClaimC = new ResourceClaim { ClaimName = "uri-C", ResourceClaimId = 3, ParentResourceClaim = resourceClaimB, ParentResourceClaimId = 2}; - var resourceClaimD = new ResourceClaim { ClaimName = "uri-D", ResourceClaimId = 4, ParentResourceClaim = resourceClaimB, ParentResourceClaimId = 3}; + var resourceClaimD = new ResourceClaim { ClaimName = "uri-D", ResourceClaimId = 4, ParentResourceClaim = resourceClaimC, ParentResourceClaimId = 3}; resourceClaimA.ParentResourceClaimId = resourceClaimC.ResourceClaimId; resourceClaimA.ParentResourceClaim = resourceClaimC; A.CallTo(() => _securityTableGateway.GetResourceClaims()) - .Returns(new List { resourceClaimA, resourceClaimB, resourceClaimC }); + .Returns(new List { resourceClaimA, resourceClaimB, resourceClaimC, resourceClaimD }); // Act & Assert - Should.Throw(() => _securityRepository.GetResourceClaimLineage("uri-C")) + Should.Throw(() => _securityRepository.GetResourceClaimLineage("uri-D")) .Message.ShouldBe("A cycle was detected in the resource claim hierarchy of the security metadata: 'uri-C' -> 'uri-B' -> 'uri-A' -> 'uri-C'"); } From 9a4ad5de622c939d1faf085090fb624ec02c75a6 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Fri, 8 Mar 2024 14:42:46 -0600 Subject: [PATCH 13/14] Update Contains statement to use Resource Claim name --- .../EdFi.Security.DataAccess/Repositories/SecurityRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 40de14b348..96f6a2e8f0 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -75,7 +75,7 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin return resourceClaimLineage; } - if (resourceClaimLineage.Contains(resourceClaim)) + if (resourceClaimLineage.Select(rc => rc.ClaimName).Contains(resourceClaim.ClaimName)) { throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc != resourceClaim).Select(rc => rc.ClaimName))}' -> '{resourceClaimUri}'"); } From ba1758f7ccdaba7e7896058b30fb96bdf258d7c7 Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Fri, 8 Mar 2024 16:10:20 -0600 Subject: [PATCH 14/14] Address changes --- .../Repositories/SecurityRepository.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs index 96f6a2e8f0..7a9b8a1976 100644 --- a/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs +++ b/Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs @@ -75,9 +75,9 @@ private IEnumerable GetResourceClaimLineageForResourceClaim(strin return resourceClaimLineage; } - if (resourceClaimLineage.Select(rc => rc.ClaimName).Contains(resourceClaim.ClaimName)) + if (resourceClaimLineage.Any(rc => rc.ClaimName == resourceClaim.ClaimName)) { - throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc != resourceClaim).Select(rc => rc.ClaimName))}' -> '{resourceClaimUri}'"); + throw new InvalidOperationException($"A cycle was detected in the resource claim hierarchy of the security metadata: '{string.Join("' -> '", resourceClaimLineage.SkipWhile(rc => rc.ClaimName != resourceClaim.ClaimName).Select(rc => rc.ClaimName))}' -> '{resourceClaimUri}'"); } resourceClaimLineage.Add(resourceClaim);