From 237580cf2c2cdc48d5b08ee90f0636a94fa25ab7 Mon Sep 17 00:00:00 2001 From: Geoff McElhanon Date: Thu, 5 Oct 2023 21:06:14 -0500 Subject: [PATCH] [ODS-5987] Relationship Authorization for StudentAssessment based on ReportingSchool (#835) * Override for auth by reported school on student assessment (#814) In order to use role based field for establishing edorg relationship, a custom authorization context data provider implementation is needed to use ReportingSchoolId. This follows the pattern established in DisciplineActionRelationshipsAuthorizationContextDataProvider. * Reduce debug noise and performance hit related to logging cache hits. * Updated IsEducationOrganizationIdName method to allow for role-named EdOrgId-related identifiers. * Updated IsPersonIdentifier method to allow for role-named person identifiers. * Updated main data context provider registration module to register context data providers succinctly and to avoid registering any of the custom override providers. * Minor cleanup of the StudentAssessmentRelationshipsAuthorizationContextDataProvider class, and added the missing license header. * Reverting suspect changes to test builds. * Restoring changes for adding support for role-named EdOrgIds in IsEducationOrganizationIdName method. * Restoring changes for making logic around registrations of relationship authorization context data providers for generated entities more concise. * Modified unit tests with slight delay to prevent timing-based unit test failures. * Restoring PersonEntitySpecification implementation. * Tightening up the conventions around identifying person types to ensure the identifier's property name matches the entity name + USI. * Renamed constants on UniqueIdConventions from USI and UniqueId to UsiSuffix and UniqueIdSuffix respectively. * Added an alternative implementation of the IsPersonIdentifier method. * Fixed bug with new support for role-named person identifier identification. * Fixed issue with original concise code related to person identifier identification. * Comment. --------- Co-authored-by: Audrey Shay --- ...piringConcurrentDictionaryCacheProvider.cs | 11 +++- .../EducationOrganizationIdNamesProvider.cs | 9 ++- .../IEducationOrganizationIdNamesProvider.cs | 4 +- .../Domain/DomainModelConventionExtensions.cs | 2 +- .../IPersonEntitySpecification.cs | 4 +- .../PersonEntitySpecification.cs | 41 +++++++++--- .../Specifications/UniqueIdConventions.cs | 24 +++---- ...sAuthorizationContextDataProviderModule.cs | 55 +++------------- ...ationContextDataProviderOverridesModule.cs | 6 ++ ...onshipsAuthorizationContextDataProvider.cs | 64 +++++++++++++++++++ .../Caching/PersonMapCacheInitializerTests.cs | 7 +- .../_Extensions/TaskExtensions.cs | 19 ++++++ 12 files changed, 168 insertions(+), 78 deletions(-) create mode 100644 Application/EdFi.Ods.Standard/Security/Authorization/Overrides/StudentAssessmentRelationshipsAuthorizationContextDataProvider.cs create mode 100644 Application/EdFi.Ods.Tests/_Extensions/TaskExtensions.cs diff --git a/Application/EdFi.Ods.Api/Caching/ExpiringConcurrentDictionaryCacheProvider.cs b/Application/EdFi.Ods.Api/Caching/ExpiringConcurrentDictionaryCacheProvider.cs index 6fb3904eba..33203211c1 100644 --- a/Application/EdFi.Ods.Api/Caching/ExpiringConcurrentDictionaryCacheProvider.cs +++ b/Application/EdFi.Ods.Api/Caching/ExpiringConcurrentDictionaryCacheProvider.cs @@ -75,8 +75,13 @@ public bool TryGetCachedObject(TKey key, out object value) { if (_logger.IsDebugEnabled) { - Interlocked.Increment(ref _hits); - _logger.Debug($"{nameof(ExpiringConcurrentDictionaryCacheProvider)} cache '{_description}' HIT ({_hits} hits, {_misses} misses)."); + _hits++; + + if (_hits < 50 || _hits % 100 == 0) + { + _logger.Debug( + $"{nameof(ExpiringConcurrentDictionaryCacheProvider)} cache '{_description}' HIT ({_hits} hits, {_misses} misses)."); + } } return true; @@ -84,7 +89,7 @@ public bool TryGetCachedObject(TKey key, out object value) if (_logger.IsDebugEnabled) { - Interlocked.Increment(ref _misses); + _misses++; _logger.Debug($"{nameof(ExpiringConcurrentDictionaryCacheProvider)} cache '{_description}' MISS ({_hits} hits, {_misses} misses)."); } } diff --git a/Application/EdFi.Ods.Api/Security/Authorization/EducationOrganizationIdNamesProvider.cs b/Application/EdFi.Ods.Api/Security/Authorization/EducationOrganizationIdNamesProvider.cs index de401a69c4..85d2475c34 100644 --- a/Application/EdFi.Ods.Api/Security/Authorization/EducationOrganizationIdNamesProvider.cs +++ b/Application/EdFi.Ods.Api/Security/Authorization/EducationOrganizationIdNamesProvider.cs @@ -57,7 +57,14 @@ public string[] GetConcreteNames() /// public bool IsEducationOrganizationIdName(string candidateName) { - return _sortedEducationOrganizationIdNames.Value.BinarySearch(candidateName) >= 0; + // First look for an exact match (no role name) + if (_sortedEducationOrganizationIdNames.Value.BinarySearch(candidateName) >= 0) + { + return true; + } + + // Now check iteratively through known names as a matching suffix on the property name (allowing for a role name) + return _sortedEducationOrganizationIdNames.Value.Any(candidateName.EndsWith); } } } diff --git a/Application/EdFi.Ods.Api/Security/Authorization/IEducationOrganizationIdNamesProvider.cs b/Application/EdFi.Ods.Api/Security/Authorization/IEducationOrganizationIdNamesProvider.cs index 6bcb22eb34..c606dad8e5 100644 --- a/Application/EdFi.Ods.Api/Security/Authorization/IEducationOrganizationIdNamesProvider.cs +++ b/Application/EdFi.Ods.Api/Security/Authorization/IEducationOrganizationIdNamesProvider.cs @@ -23,10 +23,10 @@ public interface IEducationOrganizationIdNamesProvider string[] GetConcreteNames(); /// - /// Indicates whether the supplied name is a known education organization id. + /// Indicates whether the supplied name is a known education organization id, allowing for the presence of role names. /// /// The name to be evaluated. - /// true if the name matches a known education organization id; otherwise false. + /// true if the name matches a known education organization id as a suffix; otherwise false. bool IsEducationOrganizationIdName(string candidateName); } } diff --git a/Application/EdFi.Ods.Common/Models/Domain/DomainModelConventionExtensions.cs b/Application/EdFi.Ods.Common/Models/Domain/DomainModelConventionExtensions.cs index a1d0bb71bb..ecdc13f546 100644 --- a/Application/EdFi.Ods.Common/Models/Domain/DomainModelConventionExtensions.cs +++ b/Application/EdFi.Ods.Common/Models/Domain/DomainModelConventionExtensions.cs @@ -26,7 +26,7 @@ public static Entity[] GetPersonEntities(this DomainModel domainModel) .Select(a => a.AggregateRoot) .Where(e => e.Identifier.Properties.Count == 1 - && UniqueIdConventions.IsUSI(e.Identifier.Properties[0].PropertyName)) + && UniqueIdConventions.IsUSI(e.Identifier.Properties[0].PropertyName, e.Name)) .ToArray()); } } diff --git a/Application/EdFi.Ods.Common/Specifications/IPersonEntitySpecification.cs b/Application/EdFi.Ods.Common/Specifications/IPersonEntitySpecification.cs index 37d5fea6a6..0b21367bc8 100644 --- a/Application/EdFi.Ods.Common/Specifications/IPersonEntitySpecification.cs +++ b/Application/EdFi.Ods.Common/Specifications/IPersonEntitySpecification.cs @@ -29,13 +29,13 @@ public interface IPersonEntitySpecification bool IsPersonEntity(string typeName); /// - /// Indicates whether the specified property name is an identifier (and for the specified person type, if provided). + /// Indicates whether the specified property name is a person identifier (and for the specified person type, if provided), allowing for the presence of a role name as a prefix. /// /// The name of the property to be evaluated. /// A specific type of person. /// true if the property is an identifier for the specified type of person; otherwise false. bool IsPersonIdentifier(string propertyName, string personType = null); - + /// /// Gets the type of person for a supplied UniqueId property name. /// diff --git a/Application/EdFi.Ods.Common/Specifications/PersonEntitySpecification.cs b/Application/EdFi.Ods.Common/Specifications/PersonEntitySpecification.cs index 91b3b3e010..821c58474e 100644 --- a/Application/EdFi.Ods.Common/Specifications/PersonEntitySpecification.cs +++ b/Application/EdFi.Ods.Common/Specifications/PersonEntitySpecification.cs @@ -13,12 +13,19 @@ namespace EdFi.Ods.Common.Specifications public class PersonEntitySpecification : IPersonEntitySpecification { private readonly IPersonTypesProvider _personTypesProvider; - + + private static readonly string[] _suffixes; + public PersonEntitySpecification(IPersonTypesProvider personTypesProvider) { _personTypesProvider = personTypesProvider; } - + + static PersonEntitySpecification() + { + _suffixes = new[] { UniqueIdConventions.UsiSuffix, UniqueIdConventions.UniqueIdSuffix }; + } + /// public bool IsPersonEntity(Type type) { @@ -40,13 +47,29 @@ public bool IsPersonIdentifier(string propertyName, string personType = null) } // TODO: Embedded convention (Person identifiers can end with "USI" or "UniqueId") - if (propertyName.TryTrimSuffix("UniqueId", out string entityName) - || propertyName.TryTrimSuffix("USI", out entityName)) + return _suffixes.Any( + s => propertyName.EndsWith(s) + && _personTypesProvider.PersonTypes.Any( + pt => + // Person type was either not specified, or current person type matches what was specified + (personType == null || personType == pt) + && PersonTypeAppearsImmediatelyBeforeSuffix(pt, s))); + + bool PersonTypeAppearsImmediatelyBeforeSuffix(string pt, string suffix) { - return IsPersonEntity(entityName) && (personType == null || entityName.EqualsIgnoreCase(personType)); - } + int lastIndexOfPersonType = propertyName.LastIndexOf(pt, StringComparison.Ordinal); - return false; + if (lastIndexOfPersonType < 0) + { + // Person type is not in the property name + return false; + } + + // Identify expected location of the person type in the property name + int expectedLastIndexOfPersonType = propertyName.Length - (pt.Length + suffix.Length); + + return lastIndexOfPersonType == expectedLastIndexOfPersonType; + } } /// @@ -63,7 +86,7 @@ public string GetUniqueIdPersonType(string propertyName) } return propertyName.AsSpan(personTypePos + pt.Length) - .Equals(UniqueIdConventions.UniqueId.AsSpan(), StringComparison.OrdinalIgnoreCase); + .Equals(UniqueIdConventions.UniqueIdSuffix.AsSpan(), StringComparison.OrdinalIgnoreCase); }); return personType; @@ -91,7 +114,7 @@ public string GetUSIPersonType(string propertyName) } return propertyName.AsSpan(personTypePos + pt.Length) - .Equals(UniqueIdConventions.USI.AsSpan(), StringComparison.OrdinalIgnoreCase); + .Equals(UniqueIdConventions.UsiSuffix.AsSpan(), StringComparison.OrdinalIgnoreCase); }); return personType; diff --git a/Application/EdFi.Ods.Common/Specifications/UniqueIdConventions.cs b/Application/EdFi.Ods.Common/Specifications/UniqueIdConventions.cs index 44b5ac9de1..96abd2c63e 100644 --- a/Application/EdFi.Ods.Common/Specifications/UniqueIdConventions.cs +++ b/Application/EdFi.Ods.Common/Specifications/UniqueIdConventions.cs @@ -9,8 +9,8 @@ namespace EdFi.Ods.Common.Specifications; public static class UniqueIdConventions { - public const string USI = "USI"; - public const string UniqueId = "UniqueId"; + public const string UsiSuffix = "USI"; + public const string UniqueIdSuffix = "UniqueId"; /// /// Indicates whether the supplied property name matches the USI naming convention. @@ -22,22 +22,22 @@ public static bool IsUSI(string propertyName, string personEntityName = null) { if (personEntityName == null) { - return propertyName.EndsWithIgnoreCase(USI); + return propertyName.EndsWith(UsiSuffix); } return propertyName.StartsWithIgnoreCase(personEntityName) - && propertyName.EndsWithIgnoreCase(USI) - && propertyName.Length == (personEntityName.Length + USI.Length); + && propertyName.EndsWith(UsiSuffix) + && propertyName.Length == (personEntityName.Length + UsiSuffix.Length); } public static string RemoveUsiSuffix(string propertyName) { - return propertyName.ReplaceSuffix(USI, string.Empty); + return propertyName.ReplaceSuffix(UsiSuffix, string.Empty); } public static string GetUsiPropertyName(string uniqueIdColumnName) { - return uniqueIdColumnName.ReplaceSuffix(UniqueId, USI); + return uniqueIdColumnName.ReplaceSuffix(UniqueIdSuffix, UsiSuffix); } /// @@ -50,21 +50,21 @@ public static bool IsUniqueId(string propertyName, string personEntityName = nul { if (personEntityName == null) { - return propertyName.EndsWithIgnoreCase(UniqueId); + return propertyName.EndsWith(UniqueIdSuffix); } return propertyName.StartsWithIgnoreCase(personEntityName) - && propertyName.EndsWithIgnoreCase(UniqueId) - && propertyName.Length == (personEntityName.Length + UniqueId.Length); + && propertyName.EndsWith(UniqueIdSuffix) + && propertyName.Length == (personEntityName.Length + UniqueIdSuffix.Length); } public static string RemoveUniqueIdSuffix(string propertyName) { - return propertyName.ReplaceSuffix(UniqueId, string.Empty); + return propertyName.ReplaceSuffix(UniqueIdSuffix, string.Empty); } public static string GetUniqueIdPropertyName(string usiPropertyName) { - return usiPropertyName.ReplaceSuffix(USI, UniqueId); + return usiPropertyName.ReplaceSuffix(UsiSuffix, UniqueIdSuffix); } } diff --git a/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderModule.cs b/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderModule.cs index 5b982dd592..353c283cf0 100644 --- a/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderModule.cs +++ b/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderModule.cs @@ -3,11 +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.Configuration; -using System.Linq; using Autofac; -using EdFi.Ods.Common.Extensions; using EdFi.Ods.Api.Security.AuthorizationStrategies.Relationships; namespace EdFi.Ods.Standard.Container.Modules @@ -16,48 +12,13 @@ public class RelationshipsAuthorizationContextDataProviderModule : Module { protected override void Load(ContainerBuilder builder) { - builder.RegisterAssemblyTypes(typeof(Marker_EdFi_Ods_Standard).Assembly) - .AsClosedTypesOf(typeof(IRelationshipsAuthorizationContextDataProvider<,>)) - .AsImplementedInterfaces(); - - var relationshipContextDataProviderTypes = typeof(Marker_EdFi_Ods_Standard).Assembly.GetTypes() - .Where( - t => !t.IsAbstract && typeof(IRelationshipsAuthorizationContextDataProvider<,>).IsAssignableFromGeneric(t)) - .ToList(); - - // Register all context data providers including any defined in extension assemblies. - // NOTE: If there is no entries for relationshipContextDataProviderTypes this means that the EdFi.Ods.Standard.Security assembly is not loaded. - // and this can be resolved by calling AssemblyLoader.EnsureLoaded(); in your configuration - if (!relationshipContextDataProviderTypes.Any()) - { - throw new ConfigurationErrorsException( - "Unable to find any relationship-based authorization context providers dynamically."); - } - - foreach (var providerType in relationshipContextDataProviderTypes) - { - var partiallyClosedInterfaceType = - providerType.GetInterfaces() - .SingleOrDefault(i => i.Name == typeof(IRelationshipsAuthorizationContextDataProvider<,>).Name); - - var modelType = partiallyClosedInterfaceType?.GetGenericArguments()[0]; - - var closedInterfaceType = - typeof(IRelationshipsAuthorizationContextDataProvider<,>) - .MakeGenericType(modelType, GetRelationshipBasedAuthorizationStrategyContextDataType()); - - var closedServiceType = - providerType - .MakeGenericType(GetRelationshipBasedAuthorizationStrategyContextDataType()); - - // Register the authorization context data provider, but allow existing "override" registrations to persist - builder.RegisterType(closedServiceType) - .As(closedInterfaceType) - .SingleInstance() - .PreserveExistingDefaults(); - } - - Type GetRelationshipBasedAuthorizationStrategyContextDataType() => typeof(RelationshipsAuthorizationContextData); + builder.RegisterAssemblyOpenGenericTypes(typeof(Marker_EdFi_Ods_Standard).Assembly) + .Where(t => + // Avoid registering any overrides here + t.Namespace?.Contains(".Overrides") != true + && t.IsAssignableTo>()) + .AsImplementedInterfaces() + .SingleInstance(); } } -} \ No newline at end of file +} diff --git a/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderOverridesModule.cs b/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderOverridesModule.cs index 1db64abcb1..8e15580b68 100644 --- a/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderOverridesModule.cs +++ b/Application/EdFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderOverridesModule.cs @@ -25,6 +25,12 @@ protected override void Load(ContainerBuilder builder) builder.RegisterType>() .As>() .SingleInstance(); + + // Establish authorization context for StudentAssessment using the ReportedSchoolId and StudentUSI rather than + // using the default convention (only StudentUSI) + builder.RegisterType>() + .As>() + .SingleInstance(); } } } diff --git a/Application/EdFi.Ods.Standard/Security/Authorization/Overrides/StudentAssessmentRelationshipsAuthorizationContextDataProvider.cs b/Application/EdFi.Ods.Standard/Security/Authorization/Overrides/StudentAssessmentRelationshipsAuthorizationContextDataProvider.cs new file mode 100644 index 0000000000..4dc79ae3ea --- /dev/null +++ b/Application/EdFi.Ods.Standard/Security/Authorization/Overrides/StudentAssessmentRelationshipsAuthorizationContextDataProvider.cs @@ -0,0 +1,64 @@ +// 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.Ods.Api.Security.AuthorizationStrategies.Relationships; +using EdFi.Ods.Entities.Common.EdFi; +using EdFi.Ods.Entities.NHibernate.StudentAssessmentAggregate.EdFi; +using System; +using System.Diagnostics.CodeAnalysis; + +namespace EdFi.Ods.Standard.Security.Authorization.Overrides +{ + /// + /// Creates and returns an instance for making authorization decisions for access to the edfi.StudentAssessment table of the StudentAssessment aggregate in the Ods Database. + /// + [ExcludeFromCodeCoverage] + public class StudentAssessmentRelationshipsAuthorizationContextDataProvider : IRelationshipsAuthorizationContextDataProvider + where TContextData : RelationshipsAuthorizationContextData, new() + { + /// + /// Creates and returns an instance based on the supplied resource. + /// + public TContextData GetContextData(IStudentAssessment resource) + { + if (resource == null) + { + throw new ArgumentNullException(nameof(resource), "The 'studentAssessment' resource for obtaining authorization context data cannot be null."); + } + + var entity = resource as StudentAssessment; + + var contextData = new TContextData + { + SchoolId = entity.ReportedSchoolId, // Role name applied and not part of primary key + StudentUSI = entity.StudentUSI == default ? null : entity.StudentUSI // Primary key property, USI + }; + + return contextData; + } + + /// + /// Creates and returns a signature key based on the resource, which can then be used to get and instance of IEdFiSignatureAuthorizationProvider + /// + public string[] GetAuthorizationContextPropertyNames() + { + var properties = new[] + { + "ReportedSchoolId", + "StudentUSI", + }; + + return properties; + } + + /// + /// Creates and returns an instance based on the supplied resource. + /// + public TContextData GetContextData(object resource) + { + return GetContextData((StudentAssessment)resource); + } + } +} diff --git a/Application/EdFi.Ods.Tests/EdFi.Ods.Api/Caching/PersonMapCacheInitializerTests.cs b/Application/EdFi.Ods.Tests/EdFi.Ods.Api/Caching/PersonMapCacheInitializerTests.cs index 165221fa36..0e07f481f3 100644 --- a/Application/EdFi.Ods.Tests/EdFi.Ods.Api/Caching/PersonMapCacheInitializerTests.cs +++ b/Application/EdFi.Ods.Tests/EdFi.Ods.Api/Caching/PersonMapCacheInitializerTests.cs @@ -3,12 +3,14 @@ // 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 EdFi.Ods.Api.Caching; using EdFi.Ods.Api.IdentityValueMappers; using NUnit.Framework; using FakeItEasy; using System.Collections.Generic; using System.Threading.Tasks; +using EdFi.Ods.Tests._Extensions; using Shouldly; namespace EdFi.Ods.Tests.EdFi.Ods.Api.Caching; @@ -34,7 +36,7 @@ public void EnsurePersonMapsInitialized_ShouldInitializeCache_WhenNotAlreadyInit }; A.CallTo(() => fakePersonIdentifiersProvider.GetAllPersonIdentifiersAsync(personType)) - .Returns(Task.FromResult>(personIdentifiers)); + .Returns(TaskHelpers.FromResultWithDelay>(personIdentifiers, 25)); var initializer = new PersonMapCacheInitializer( fakePersonIdentifiersProvider, @@ -79,6 +81,9 @@ public void EnsurePersonMapsInitialized_ShouldNotReinitializeCache_WhenAlreadyIn var fakeUniqueIdByUsiMapCache = A.Fake>(); var fakeUsiByUniqueIdMapCache = A.Fake>(); + A.CallTo(() => fakePersonIdentifiersProvider.GetAllPersonIdentifiersAsync(personType)) + .Returns(TaskHelpers.FromResultWithDelay>(Array.Empty(), 25)); + // Simulate that the cache is already initialized var uniqueIdByUsiTuple = (odsInstanceHashId, personType, PersonMapType.UniqueIdByUsi); diff --git a/Application/EdFi.Ods.Tests/_Extensions/TaskExtensions.cs b/Application/EdFi.Ods.Tests/_Extensions/TaskExtensions.cs new file mode 100644 index 0000000000..9e7ac929c4 --- /dev/null +++ b/Application/EdFi.Ods.Tests/_Extensions/TaskExtensions.cs @@ -0,0 +1,19 @@ +// 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.Threading; +using System.Threading.Tasks; + +namespace EdFi.Ods.Tests._Extensions; + +public static class TaskHelpers +{ + public static Task FromResultWithDelay(TResult result, int delayMilliseconds) + { + Thread.Sleep(delayMilliseconds); + + return Task.FromResult(result); + } +}