[ODS-3401] Cycles in claims taxonomy (security metadata for resource claims) causes requests to hang

[mjaksn]

No description provided.

[gmcelhanon]

@mjaksn Just wondering if you might be able to do this more directly by just switching to using a HashSet<T> in the GetResourceClaimLineageForResourceClaim method?

        private IEnumerable<ResourceClaim> GetResourceClaimLineageForResourceClaim(string resourceClaimUri)
        {
            // ==============================
            // Use a HashSet<T> here instead of List<T>?     
            // ==============================
            var resourceClaimLineage = new List<ResourceClaim>();

            ResourceClaim resourceClaim;

            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)
            {
                // ==============================
                // OLD: resourceClaimLineage.Add(resourceClaim);
                // NEW:
                if (!resourceClaimLineage.Add(resourceClaim))
                {
                    // Handle cycle problem here
                }
               // ==============================
    
                if (resourceClaim.ParentResourceClaim != null)
                {
                     resourceClaimLineage.AddRange(GetResourceClaimLineageForResourceClaim(resourceClaim.ParentResourceClaim.ClaimName));
                }
            }
            return resourceClaimLineage;
        }

Also, in looking at the code, I think it's an opportunity to be more efficient and declare the HashSet<T> in the calling method and pass it into the recursive private method, thereby avoiding the creation of multiple HashSets or Lists when this is called.

[gmcelhanon]

After direct conversation in Slack, we discussed a few change/improvements to the implementation, include:

• Switch back to List<T> and use a Contains check in lieu of a HashSet<T> and a check on the Add method because HashSet<T> doesn't guarantee preservation of insertion order which is important to the semantics of this method.
• Expand the resulting message to include the resource claims participating in the cycle to be included in the errors collection of the Problem Details response.
• Add unit test coverage since this would be difficult to test at an integration test level.

[axelmarquezh]

If I disable the Security cache, this validation fails to find the cycle because it hits the DB on each recursive call (so we get a new instance with a different hash code) so .Contains() always returns false.

Turning off the Security cache isn't a real-world scenario; should we care about this edge case?

"Security": {
    "AbsoluteExpirationMinutes": -1
}

Note that something related could happen if the cache expires right in the middle of a check, but this wouldn't result in a StackOverflow because the cache is still there; the error message would come up with a duplicated cycle, though.

Requested changes have been addressed.