Conversation
Extract ParseFromPolicyJson static method that parses raw JSON from .show database policy managed_identity. Simplify the Kusto query to just project the Policy column. Add ClientId property (YamlIgnore) to support future principal normalization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Create PrincipalParser with testable static methods for role extraction, display name cleaning, and principal grouping. Simplify Kusto query to just project raw columns. Also loads Monitor role which was previously missing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace ClientId with ObjectId in principal FQNs using exact FQN parsing (only aadapp kind, case-insensitive). Handles duplicate ClientId entries gracefully via GroupBy. Called in KustoDatabaseHandler.LoadAsync() after all loaders complete. Fixes phantom permission diffs when .show database principals returns ClientId but YAML stores ObjectId. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces “phantom” permission diffs by shifting principal/policy shaping from Kusto queries into C# parsing and by normalizing principal IDs using managed identity policy data.
Changes:
- Added
PrincipalParserto parse.show database principalsoutput in C# (role extraction, display name cleanup, filtering). - Updated managed identity policy loading to fetch the raw
PolicyJSON and parse it in C# (capturingClientIdfor normalization). - Normalized loaded principal FQNs by rewriting
aadapp=<clientId>toaadapp=<objectId>based on managed identity policy mapping, plus added targeted unit tests.
Show a summary per file
| File | Description |
|---|---|
| KustoSchemaTools/Parser/PrincipalParser.cs | New C# parser for principal rows (role/name cleanup, grouping). |
| KustoSchemaTools/Parser/KustoLoader/KustoDatabasePrincipalLoader.cs | Simplified Kusto query; delegates shaping to PrincipalParser; includes Monitor role. |
| KustoSchemaTools/Parser/KustoLoader/KustoManagedIdentityPolicyLoader.cs | Loads raw managed identity Policy JSON and parses in C#. |
| KustoSchemaTools/Parser/KustoDatabaseHandler.cs | Calls database.NormalizePrincipalIds() after loading from cluster. |
| KustoSchemaTools/Model/ManagedIdentityPolicy.cs | Adds ClientId (YAML-ignored) and JSON parsing helper for policy column. |
| KustoSchemaTools/Model/Database.cs | Adds principal-ID normalization logic and FQN parsing helpers. |
| KustoSchemaTools/KustoSchemaTools.csproj | Adds InternalsVisibleTo for test access to internal helpers. |
| KustoSchemaTools.Tests/Parser/PrincipalParserTests.cs | New tests for principal parsing behavior. |
| KustoSchemaTools.Tests/Model/ManagedIdentityPolicyTests.cs | New tests for managed identity policy JSON parsing. |
| KustoSchemaTools.Tests/Changes/PrincipalNormalizationTests.cs | New tests validating normalization and diff behavior scenarios. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 3
| .GroupBy(r => r.Role) | ||
| .ToDictionary( | ||
| g => g.Key, | ||
| g => g.Select(r => r.Principal).ToList() |
There was a problem hiding this comment.
ParsePrincipals currently preserves the source order of principals within each role group. Since PermissionChange compares generated .set database ... (ids...) script text, non-deterministic ordering from .show database principals can create false diffs even when the principal sets are identical. Consider sorting (and optionally de-duplicating) principals within each role group by a stable key like Id (and/or Name) before returning the lists.
| g => g.Select(r => r.Principal).ToList() | |
| g => g.Select(r => r.Principal) | |
| .GroupBy(p => new { p.Id, p.Name }) | |
| .Select(pg => pg.First()) | |
| .OrderBy(p => p.Id ?? string.Empty) | |
| .ThenBy(p => p.Name ?? string.Empty) | |
| .ToList() |
| internal static string ExtractRoleName(string role) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(role)) | ||
| return ""; | ||
|
|
||
| var parts = role.Split(' '); | ||
| return parts[^1]; | ||
| } |
There was a problem hiding this comment.
Role parsing/filtering currently uses role.Split(' ') and StartsWith("All") without specifying comparison semantics. To better match Kusto's startswith behavior and avoid culture-sensitive comparisons / extra-space edge cases, consider using Split(' ', StringSplitOptions.RemoveEmptyEntries) and StartsWith("All", StringComparison.OrdinalIgnoreCase).
| @@ -2,6 +2,7 @@ | |||
| using Newtonsoft.Json; | |||
| using KustoSchemaTools.Helpers; | |||
| using KustoSchemaTools.Parser; | |||
There was a problem hiding this comment.
using KustoSchemaTools.Parser; appears unused in this file after the refactor. If the repo treats warnings as failures in CI (or to keep the file clean), remove the unused using.
| using KustoSchemaTools.Parser; |
Kusto allows specifying both object ID and client ID when setting permissions, but the diff always prefers one of them(ClientId), so it you have config with wrong identity it will always shows the diff.
Here I de-kustoify logic, so that it compared in C# rather then using complex Kusto queries and compare objects.