resolving comments

Author: sezalchug

src/Config/Converters/RuntimeHealthOptionsConvertorFactory.cs

@@ -53,7 +53,7 @@ internal HealthCheckOptionsConverter(bool replaceEnvVar)
             if (reader.TokenType is JsonTokenType.StartObject)
             {
                 bool? enabled = null;
-                List<string>? roles = null;
+                HashSet<string>? roles = null;
 
                 while (reader.Read())
                 {
@@ -80,7 +80,7 @@ internal HealthCheckOptionsConverter(bool replaceEnvVar)
                                 // Check if the token type is an array
                                 if (reader.TokenType == JsonTokenType.StartArray)
                                 {
-                                    List<string> stringList = new();
+                                    HashSet<string> stringList = new();
 
                                     // Read the array elements one by one
                                     while (reader.Read() && reader.TokenType != JsonTokenType.EndArray)

src/Config/HealthCheck/RuntimeHealthCheckConfig.cs

@@ -8,7 +8,7 @@ public record RuntimeHealthCheckConfig : HealthCheckConfig
     // TODO: Add support for caching in upcoming PRs
     // public int cache-ttl-seconds { get; set; };
 
-    public List<string>? Roles { get; set; }
+    public HashSet<string>? Roles { get; set; }
 
     // TODO: Add support for parallel stream to run the health check query in upcoming PRs
     // public int MaxDop { get; set; } = 1; // Parallelized streams to run Health Check (Default: 1)
@@ -17,7 +17,7 @@ public RuntimeHealthCheckConfig() : base()
     {
     }
 
-    public RuntimeHealthCheckConfig(bool? Enabled, List<string>? Roles = null) : base(Enabled)
+    public RuntimeHealthCheckConfig(bool? Enabled, HashSet<string>? Roles = null) : base(Enabled)
     {
         this.Roles = Roles;
     }

src/Config/ObjectModel/RuntimeConfig.cs

@@ -151,8 +151,8 @@ Runtime.GraphQL is not null &&
         Runtime.GraphQL.EnableAggregation;
 
     [JsonIgnore]
-    public List<string> AllowedRolesForHealth =>
-        Runtime?.Health?.Roles ?? new List<string>();
+    public HashSet<string> AllowedRolesForHealth =>
+        Runtime?.Health?.Roles ?? new HashSet<string>();
 
     private Dictionary<string, DataSource> _dataSourceNameToDataSource;
 

src/Service.Tests/Configuration/HealthEndpointRolesTests.cs

@@ -114,7 +114,7 @@ private static void CreateCustomConfigFile(Dictionary<string, Entity> entityMap,
                 Schema: string.Empty,
                 DataSource: dataSource,
                 Runtime: new(
-                    Health: new(Enabled: true, Roles: role != null ? new List<string> { role } : null),
+                    Health: new(Enabled: true, Roles: role != null ? new HashSet<string> { role } : null),
                     Rest: new(Enabled: true),
                     GraphQL: new(Enabled: true),
                     Host: hostOptions

src/Service/HealthCheck/ComprehensiveHealthReportResponseWriter.cs

@@ -69,34 +69,25 @@ public Task WriteResponse(HttpContext context)
             // Global comprehensive Health Check Enabled
             if (config.IsHealthEnabled)
             {
-                _healthCheckHelper.UpdateIncomingRoleHeader(context);
+                _healthCheckHelper.StoreIncomingRoleHeader(context);
                 if (!_healthCheckHelper.IsUserAllowedToAccessHealthCheck(context, config.IsDevelopmentMode(), config.AllowedRolesForHealth))
                 {
-                    LogTrace("Comprehensive Health Check Report is not allowed: 403 Forbidden due to insufficient permissions.");
+                    _logger.LogError("Comprehensive Health Check Report is not allowed: 403 Forbidden due to insufficient permissions.");
                     context.Response.StatusCode = StatusCodes.Status403Forbidden;
                     return context.Response.CompleteAsync();
                 }
 
                 ComprehensiveHealthCheckReport dabHealthCheckReport = _healthCheckHelper.GetHealthCheckResponse(context, config);
                 string response = JsonSerializer.Serialize(dabHealthCheckReport, options: new JsonSerializerOptions { WriteIndented = true, DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull });
-                LogTrace($"Health check response writer writing status as: {dabHealthCheckReport.Status}");
+                _logger.LogTrace($"Health check response writer writing status as: {dabHealthCheckReport.Status}");
                 return context.Response.WriteAsync(response);
             }
             else
             {
-                LogTrace("Comprehensive Health Check Report Not Found: 404 Not Found.");
+                _logger.LogError("Comprehensive Health Check Report Not Found: 404 Not Found.");
                 context.Response.StatusCode = StatusCodes.Status404NotFound;
                 return context.Response.CompleteAsync();
             }
         }
-
-        /// <summary>
-        /// Logs a trace message if a logger is present and the logger is enabled for trace events.
-        /// </summary>
-        /// <param name="message">Message to emit.</param>
-        private void LogTrace(string message)
-        {
-            _logger.LogTrace(message);
-        }
     }
 }

src/Service/HealthCheck/HealthCheckHelper.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
@@ -62,11 +63,17 @@ public ComprehensiveHealthCheckReport GetHealthCheckResponse(HttpContext context
         }
 
         // Updates the incoming role header with the appropriate value from the request headers.
-        public void UpdateIncomingRoleHeader(HttpContext httpContext)
+        public void StoreIncomingRoleHeader(HttpContext httpContext)
         {
             StringValues clientRoleHeader = httpContext.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER];
             StringValues clientTokenHeader = httpContext.Request.Headers[AuthenticationOptions.CLIENT_PRINCIPAL_HEADER];
 
+            if (clientRoleHeader.Count > 1 || clientTokenHeader.Count > 1)
+            {
+                throw new ArgumentException("Multiple values for the client role or token header are not allowed.");
+            }
+
+            // Role Header is not present in the request, set it to anonymous.
             if (clientRoleHeader.Count == 1)
             {
                 _incomingRoleHeader = clientRoleHeader.ToString().ToLowerInvariant();
@@ -82,7 +89,7 @@ public void UpdateIncomingRoleHeader(HttpContext httpContext)
         /// <param name="hostMode">Compare with the HostMode of DAB</param>
         /// <param name="allowedRoles">AllowedRoles in the Runtime.Health config</param>
         /// <returns></returns>
-        public bool IsUserAllowedToAccessHealthCheck(HttpContext httpContext, bool isDevelopmentMode, List<string> allowedRoles)
+        public bool IsUserAllowedToAccessHealthCheck(HttpContext httpContext, bool isDevelopmentMode, HashSet<string> allowedRoles)
         {
             if (allowedRoles == null || allowedRoles.Count == 0)
             {

src/Service/HealthCheck/HttpUtilities.cs

@@ -106,7 +106,7 @@ public void ConfigureApiRoute(HttpContext httpContext)
                     // Send a GET request to the API
                     apiRoute = $"{apiRoute}{Utilities.CreateHttpRestQuery(entityName, first)}";
                     HttpRequestMessage message = new(method: HttpMethod.Get, requestUri: apiRoute);
-                    if (string.IsNullOrEmpty(incomingRoleToken) && string.IsNullOrEmpty(incomingRoleHeader))
+                    if (!(string.IsNullOrEmpty(incomingRoleToken) && string.IsNullOrEmpty(incomingRoleHeader)))
                     {
                         message.Headers.Add(AuthenticationOptions.CLIENT_PRINCIPAL_HEADER, incomingRoleToken);
                         message.Headers.Add(AuthorizationResolver.CLIENT_ROLE_HEADER, incomingRoleHeader);
@@ -168,7 +168,7 @@ public void ConfigureApiRoute(HttpContext httpContext)
                             Content = content
                         };
 
-                        if (string.IsNullOrEmpty(incomingRoleToken) && string.IsNullOrEmpty(incomingRoleHeader))
+                        if (!(string.IsNullOrEmpty(incomingRoleToken) && string.IsNullOrEmpty(incomingRoleHeader)))
                         {
                             message.Headers.Add(AuthenticationOptions.CLIENT_PRINCIPAL_HEADER, incomingRoleToken);
                             message.Headers.Add(AuthorizationResolver.CLIENT_ROLE_HEADER, incomingRoleHeader);