[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 <[email protected]>
Ed-Fi-Alliance-OSS · Oct 6, 2023 · 237580c · 237580c
1 parent 0abd7f5
commit 237580c
Show file tree

Hide file tree

Showing 12 changed files with 168 additions and 78 deletions.
diff --git a/Application/EdFi.Ods.Api/Caching/ExpiringConcurrentDictionaryCacheProvider.cs b/Application/EdFi.Ods.Api/Caching/ExpiringConcurrentDictionaryCacheProvider.cs
@@ -75,16 +75,21 @@ public bool TryGetCachedObject(TKey key, out object value)
                 {
                     if (_logger.IsDebugEnabled)
                     {
-                        Interlocked.Increment(ref _hits);
-                        _logger.Debug($"{nameof(ExpiringConcurrentDictionaryCacheProvider<TKey>)} cache '{_description}' HIT ({_hits} hits, {_misses} misses).");
+                        _hits++;
+
+                        if (_hits < 50 || _hits % 100 == 0)
+                        {
+                            _logger.Debug(
+                                $"{nameof(ExpiringConcurrentDictionaryCacheProvider<TKey>)} cache '{_description}' HIT ({_hits} hits, {_misses} misses).");
+                        }
                     }
 
                     return true;
                 }
 
                 if (_logger.IsDebugEnabled)
                 {
-                    Interlocked.Increment(ref _misses);
+                    _misses++;
                     _logger.Debug($"{nameof(ExpiringConcurrentDictionaryCacheProvider<TKey>)} 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
@@ -57,7 +57,14 @@ public string[] GetConcreteNames()
         /// <inheritdoc cref="IEducationOrganizationIdNamesProvider.IsEducationOrganizationIdName" />
         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
@@ -23,10 +23,10 @@ public interface IEducationOrganizationIdNamesProvider
         string[] GetConcreteNames();
 
         /// <summary>
-        /// 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.
         /// </summary>
         /// <param name="candidateName">The name to be evaluated.</param>
-        /// <returns><b>true</b> if the name matches a known education organization id; otherwise <b>false</b>.</returns>
+        /// <returns><b>true</b> if the name matches a known education organization id as a suffix; otherwise <b>false</b>.</returns>
         bool IsEducationOrganizationIdName(string candidateName);
     }
 }
diff --git 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
@@ -29,13 +29,13 @@ public interface IPersonEntitySpecification
     bool IsPersonEntity(string typeName);
 
     /// <summary>
-    /// 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.
     /// </summary>
     /// <param name="propertyName">The name of the property to be evaluated.</param>
     /// <param name="personType">A specific type of person.</param>
     /// <returns><b>true</b> if the property is an identifier for the specified type of person; otherwise <b>false</b>.</returns>
     bool IsPersonIdentifier(string propertyName, string personType = null);
-    
+
     /// <summary>
     /// Gets the type of person for a supplied UniqueId property name.
     /// </summary>

diff --git 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 };
+        }
+
         /// <inheritdoc cref="IPersonEntitySpecification.IsPersonEntity(System.Type)" />
         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;
+            }
         }
 
         /// <inheritdoc cref="IPersonEntitySpecification.GetUniqueIdPersonType" />
@@ -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
@@ -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";
 
     /// <summary>
     /// 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);
     }
 
     /// <summary>
@@ -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/...dFi.Ods.Standard/Container/Modules/RelationshipsAuthorizationContextDataProviderModule.cs b/...dFi.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<Marker_EdFi_Ods_Standard_Security>(); 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<IRelationshipsAuthorizationContextDataProvider<RelationshipsAuthorizationContextData>>())
+                .AsImplementedInterfaces()
+                .SingleInstance();
         }
     }
-}
+}
diff --git a/...tandard/Container/Modules/RelationshipsAuthorizationContextDataProviderOverridesModule.cs b/...tandard/Container/Modules/RelationshipsAuthorizationContextDataProviderOverridesModule.cs
@@ -25,6 +25,12 @@ protected override void Load(ContainerBuilder builder)
             builder.RegisterType<DisciplineActionRelationshipsAuthorizationContextDataProvider<RelationshipsAuthorizationContextData>>()
                 .As<IRelationshipsAuthorizationContextDataProvider<IDisciplineAction, RelationshipsAuthorizationContextData>>()
                 .SingleInstance();
+
+            // Establish authorization context for StudentAssessment using the ReportedSchoolId and StudentUSI rather than
+            // using the default convention (only StudentUSI)
+            builder.RegisterType<StudentAssessmentRelationshipsAuthorizationContextDataProvider<RelationshipsAuthorizationContextData>>()
+                .As<IRelationshipsAuthorizationContextDataProvider<IStudentAssessment, RelationshipsAuthorizationContextData>>()
+                .SingleInstance();
         }
     }
 }
diff --git a/...Authorization/Overrides/StudentAssessmentRelationshipsAuthorizationContextDataProvider.cs b/...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
+{
+    /// <summary>
+    /// Creates and returns an <see cref="RelationshipsAuthorizationContextData"/> instance for making authorization decisions for access to the edfi.StudentAssessment table of the StudentAssessment aggregate in the Ods Database.
+    /// </summary>
+    [ExcludeFromCodeCoverage]
+    public class StudentAssessmentRelationshipsAuthorizationContextDataProvider<TContextData> : IRelationshipsAuthorizationContextDataProvider<IStudentAssessment, TContextData>
+        where TContextData : RelationshipsAuthorizationContextData, new()
+    {
+        /// <summary>
+        /// Creates and returns an <see cref="TContextData"/> instance based on the supplied resource.
+        /// </summary>
+        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;
+        }
+
+        /// <summary>
+        ///  Creates and returns a signature key based on the resource, which can then be used to get and instance of IEdFiSignatureAuthorizationProvider
+        /// </summary>
+        public string[] GetAuthorizationContextPropertyNames()
+        {
+            var properties = new[]
+                 {
+                    "ReportedSchoolId",
+                    "StudentUSI",
+                 };
+
+            return properties;
+        }
+
+        /// <summary>
+        /// Creates and returns an <see cref="RelationshipsAuthorizationContextData"/> instance based on the supplied resource.
+        /// </summary>
+        public TContextData GetContextData(object resource)
+        {
+            return GetContextData((StudentAssessment)resource);
+        }
+    }
+}