mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 12:42:38 +00:00
feat: tenant-config field schema - MCP writes get types right on the first attempt (#121)
Follow-up to the E2E finding (95 vs 95.0): a model driving update_tenant_config had to guess field names and JSON types from a config read. New reflection- generated schema surface makes guessing unnecessary: - Backend: GET config/fields-schema (GlobalAdminOnly) - built from the TenantConfiguration model + the patch service deny-lists at runtime, so it can NEVER drift from what the patch endpoint enforces. Per field: camelCase name, JSON type (string/boolean/integer/number), nullability (null clears), format hints (json / date-time), writability + deny reason, GA-only marker (phase 2), revert-protected marker. Literal route; FindPolicy literal-over-template precedence pinned by test (same mechanism as config/all). - MCP: new strictGa READ_ONLY tool get_tenant_config_schema; update_tenant_config description + GA instructions headline steer the model to call it before composing a patch. - Tests: drift canary (every model property appears exactly once), deny-list consistency, type exemplars incl. the decimal SLA column class, policy + route-precedence pins, MCP catalog/registration/request-shape. Backend 3854 green, vitest 465 green, tsc clean. Deploy order: backend first, then MCP redeploy. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
parent
f3028041af
commit
9d8b0fc254
@@ -626,6 +626,37 @@ public class PolicyEnforcementMiddlewareTests
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ConfigFieldsSchema_LiteralRoute_WinsOverTenantIdTemplate()
|
||||
{
|
||||
// "fields-schema" must never be swallowed as a {tenantId} of the MemberRead config GET —
|
||||
// FindPolicy's literal-over-template precedence is what this feature's route relies on
|
||||
// (same mechanism as config/all).
|
||||
var entry = EndpointAccessPolicyCatalog.FindPolicy("GET", "/api/config/fields-schema");
|
||||
Assert.NotNull(entry);
|
||||
Assert.Equal("config/fields-schema", entry!.RouteTemplate);
|
||||
Assert.Equal(EndpointPolicy.GlobalAdminOnly, entry.Policy);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ConfigFieldsSchema_GlobalAdminAllowed_ReaderAndTenantAdminForbidden()
|
||||
{
|
||||
var ga = BuildHarness();
|
||||
ga.AsGlobalRole(Constants.GlobalRoles.GlobalAdmin);
|
||||
Assert.True((await ga.Middleware.DecideAsync(
|
||||
"GET", "/api/config/fields-schema", null, AuthedPrincipal(TenantA, "ga@vendor.example"))).Allowed);
|
||||
|
||||
var reader = BuildHarness();
|
||||
reader.AsGlobalRole(Constants.GlobalRoles.GlobalReader);
|
||||
Assert.False((await reader.Middleware.DecideAsync(
|
||||
"GET", "/api/config/fields-schema", null, AuthedPrincipal(TenantA, "reader@vendor.example"))).Allowed);
|
||||
|
||||
var tenantAdmin = BuildHarness();
|
||||
tenantAdmin.AsTenantAdmin(TenantA, "admin@contoso.com");
|
||||
Assert.False((await tenantAdmin.Middleware.DecideAsync(
|
||||
"GET", "/api/config/fields-schema", null, AuthedPrincipal(TenantA, "admin@contoso.com"))).Allowed);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ConfigFieldPatchRoutes_TenantAdmin_GlobalReader_Delegated_AreForbidden()
|
||||
{
|
||||
|
||||
@@ -544,6 +544,68 @@ public class TenantConfigPatchServiceTests
|
||||
Assert.Equal(95m, rehydrated.SlaTargetSuccessRate);
|
||||
}
|
||||
|
||||
// ── Fields schema (MCP steering surface) ────────────────────────────────
|
||||
|
||||
[Fact]
|
||||
public void FieldsSchema_CoversEveryModelProperty_ExactlyOnce()
|
||||
{
|
||||
// Drift canary: the schema is reflection-generated, so this pins that EVERY writable
|
||||
// model property surfaces exactly once under its camelCase name — a new config field
|
||||
// is automatically schema-visible, and a rename cannot leave a stale entry.
|
||||
var schema = TenantConfigPatchService.BuildFieldsSchema();
|
||||
var expected = typeof(TenantConfiguration).GetProperties()
|
||||
.Where(p => p.CanWrite && p.GetIndexParameters().Length == 0)
|
||||
.Select(p => char.ToLowerInvariant(p.Name[0]) + p.Name.Substring(1))
|
||||
.OrderBy(n => n, StringComparer.Ordinal)
|
||||
.ToList();
|
||||
|
||||
Assert.Equal(expected, schema.Select(f => f.Name).ToList());
|
||||
Assert.Equal(schema.Count, schema.Select(f => f.Name).Distinct().Count());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void FieldsSchema_DeniedFields_MatchThePatchDenyList()
|
||||
{
|
||||
var schema = TenantConfigPatchService.BuildFieldsSchema()
|
||||
.ToDictionary(f => f.Name, StringComparer.OrdinalIgnoreCase);
|
||||
|
||||
foreach (var denied in TenantConfigPatchService.BaseDeniedFields
|
||||
.Where(n => n is not ("PartitionKey" or "RowKey" or "Timestamp" or "ETag")))
|
||||
{
|
||||
Assert.False(schema[denied].Writable, $"{denied} must be marked non-writable");
|
||||
Assert.False(string.IsNullOrWhiteSpace(schema[denied].Reason), $"{denied} needs a deny reason");
|
||||
}
|
||||
foreach (var gaOnly in TenantConfigPatchService.GaOnlyFields)
|
||||
{
|
||||
Assert.True(schema[gaOnly].Writable, $"{gaOnly} is writable for GA");
|
||||
Assert.True(schema[gaOnly].GaOnly, $"{gaOnly} must carry the GA-only marker");
|
||||
}
|
||||
foreach (var protectedField in TenantConfigPatchService.RevertProtectedFields)
|
||||
{
|
||||
Assert.True(schema[protectedField].RevertProtected, $"{protectedField} must carry the revert-protected marker");
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void FieldsSchema_TypeExemplars_AreCorrect()
|
||||
{
|
||||
var schema = TenantConfigPatchService.BuildFieldsSchema()
|
||||
.ToDictionary(f => f.Name, StringComparer.Ordinal);
|
||||
|
||||
Assert.Equal(("integer", false, true), (schema["dataRetentionDays"].Type, schema["dataRetentionDays"].Nullable, schema["dataRetentionDays"].Writable));
|
||||
// The prod-finding column class: decimal? → JSON "number", nullable.
|
||||
Assert.Equal(("number", true), (schema["slaTargetSuccessRate"].Type, schema["slaTargetSuccessRate"].Nullable));
|
||||
Assert.Equal(("boolean", true), (schema["disabled"].Type, schema["disabled"].GaOnly));
|
||||
Assert.Equal(("string", true), (schema["contactEmail"].Type, schema["contactEmail"].Nullable));
|
||||
Assert.Equal("json", schema["notificationChannelsJson"].Format);
|
||||
Assert.Equal("date-time", schema["trialExpiresUtc"].Format);
|
||||
Assert.False(schema["planTier"].Writable);
|
||||
Assert.Contains("plan/trial", schema["planTier"].Reason);
|
||||
Assert.False(schema["lastUpdated"].Writable);
|
||||
Assert.False(schema["homedAppClientId"].Writable);
|
||||
Assert.True(schema["homedAppClientId"].RevertProtected);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void RehydrateEntity_DateLookingStringField_StaysString()
|
||||
{
|
||||
|
||||
+58
@@ -0,0 +1,58 @@
|
||||
using System;
|
||||
using System.Linq;
|
||||
using System.Net;
|
||||
using System.Threading.Tasks;
|
||||
using AutopilotMonitor.Functions.Services;
|
||||
using Microsoft.Azure.Functions.Worker;
|
||||
using Microsoft.Azure.Functions.Worker.Http;
|
||||
using Microsoft.Extensions.Logging;
|
||||
|
||||
namespace AutopilotMonitor.Functions.Functions.Config
|
||||
{
|
||||
/// <summary>
|
||||
/// Machine-readable schema of the tenant-configuration fields, for the MCP write surface:
|
||||
/// which fields exist, their JSON type/nullability, which are writable via
|
||||
/// PATCH config/{tenantId}/fields (and why not, when denied), the phase-2 GA-only markers,
|
||||
/// and which fields a revert preserves by default. Reflection-generated from the model and
|
||||
/// the patch service's deny-lists — cannot drift from what the patch endpoint enforces.
|
||||
/// Tenant-independent (no tenant data), but GlobalAdminOnly to match the write surface it
|
||||
/// describes. Route is literal — the catalog and ASP.NET routing both prefer literal
|
||||
/// segments over config/{tenantId} (same precedence config/all relies on).
|
||||
/// </summary>
|
||||
public class GetTenantConfigFieldsSchemaFunction
|
||||
{
|
||||
private readonly ILogger<GetTenantConfigFieldsSchemaFunction> _logger;
|
||||
|
||||
public GetTenantConfigFieldsSchemaFunction(ILogger<GetTenantConfigFieldsSchemaFunction> logger)
|
||||
{
|
||||
_logger = logger;
|
||||
}
|
||||
|
||||
[Function("GetTenantConfigFieldsSchema")]
|
||||
public async Task<HttpResponseData> Run(
|
||||
[HttpTrigger(AuthorizationLevel.Anonymous, "get", Route = "config/fields-schema")] HttpRequestData req)
|
||||
{
|
||||
try
|
||||
{
|
||||
// Authentication + GlobalAdminOnly authorization enforced by PolicyEnforcementMiddleware.
|
||||
var schema = TenantConfigPatchService.BuildFieldsSchema();
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new
|
||||
{
|
||||
count = schema.Count,
|
||||
writableCount = schema.Count(f => f.Writable),
|
||||
fields = schema,
|
||||
});
|
||||
return response;
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
_logger.LogError(ex, "Error building tenant-config fields schema");
|
||||
var response = req.CreateResponse(HttpStatusCode.InternalServerError);
|
||||
await response.WriteAsJsonAsync(new { error = "Internal server error" });
|
||||
return response;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -398,6 +398,10 @@ public static class EndpointAccessPolicyCatalog
|
||||
new("PATCH", "config/{tenantId}/fields", EndpointPolicy.GlobalAdminOnly, TenantScoping.RouteParam),
|
||||
new("GET", "config/{tenantId}/backups", EndpointPolicy.GlobalAdminOnly, TenantScoping.RouteParam),
|
||||
new("POST", "config/{tenantId}/revert", EndpointPolicy.GlobalAdminOnly, TenantScoping.RouteParam),
|
||||
// Field schema for the write surface above: tenant-independent reflection over the model
|
||||
// + deny-lists (no tenant data), but GA-only to match the surface it describes. Literal
|
||||
// route — FindPolicy and ASP.NET routing both prefer it over config/{tenantId}.
|
||||
new("GET", "config/fields-schema", EndpointPolicy.GlobalAdminOnly),
|
||||
new("GET", "auth/global-admins", EndpointPolicy.GlobalReadOrAdmin),
|
||||
new("POST", "auth/global-admins", EndpointPolicy.GlobalAdminOnly),
|
||||
new("DELETE", "auth/global-admins/{upn}", EndpointPolicy.GlobalAdminOnly),
|
||||
|
||||
@@ -56,6 +56,21 @@ namespace AutopilotMonitor.Functions.Services
|
||||
=> new(false, failure, error, backupId, Array.Empty<string>(), null, drift);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// One entry of the tenant-config field schema (see TenantConfigPatchService.BuildFieldsSchema).
|
||||
/// Serialized camelCase to the API. Nulls/false flags are serialized as-is — the payload is
|
||||
/// tiny and consumed by a model, which reads explicit values better than absent keys.
|
||||
/// </summary>
|
||||
public sealed record TenantConfigFieldSchema(
|
||||
string Name,
|
||||
string Type,
|
||||
string? Format,
|
||||
bool Nullable,
|
||||
bool Writable,
|
||||
string? Reason,
|
||||
bool GaOnly,
|
||||
bool RevertProtected);
|
||||
|
||||
/// <summary>
|
||||
/// Transactional field-level writes against a tenant's configuration row:
|
||||
/// fresh ETag read → field gate → patch onto a clone → shared validation →
|
||||
@@ -392,6 +407,63 @@ namespace AutopilotMonitor.Functions.Services
|
||||
return new PatchOutcome(true, PatchFailure.None, null, backupId, applied, maskedDiff, null);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Machine-readable schema of the tenant-config fields for the MCP write surface:
|
||||
/// name (camelCase, as the patch endpoint accepts it), JSON type, nullability
|
||||
/// (nullable ⇒ an explicit JSON null clears the field), writability incl. the deny
|
||||
/// reason, the phase-2 GA-only marker, and the revert-protected marker. Generated by
|
||||
/// REFLECTION over the model + the deny-lists above — the same sources the patch flow
|
||||
/// enforces — so the schema can never drift from what the endpoint actually accepts.
|
||||
/// Static per process (the model does not change at runtime).
|
||||
/// </summary>
|
||||
internal static IReadOnlyList<TenantConfigFieldSchema> BuildFieldsSchema() => FieldsSchema.Value;
|
||||
|
||||
private static readonly Lazy<IReadOnlyList<TenantConfigFieldSchema>> FieldsSchema = new(() =>
|
||||
{
|
||||
var nullability = new NullabilityInfoContext();
|
||||
var fields = new List<TenantConfigFieldSchema>();
|
||||
foreach (var prop in ModelProperties.Values)
|
||||
{
|
||||
var underlying = Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType;
|
||||
var type =
|
||||
underlying == typeof(bool) ? "boolean"
|
||||
: underlying == typeof(int) || underlying == typeof(long) ? "integer"
|
||||
: underlying == typeof(decimal) || underlying == typeof(double) || underlying == typeof(float) ? "number"
|
||||
: "string";
|
||||
var format =
|
||||
underlying == typeof(DateTime) || underlying == typeof(DateTimeOffset) ? "date-time"
|
||||
: prop.Name.EndsWith("Json", StringComparison.Ordinal) ? "json"
|
||||
: null;
|
||||
var nullable = Nullable.GetUnderlyingType(prop.PropertyType) != null
|
||||
|| (!prop.PropertyType.IsValueType
|
||||
&& nullability.Create(prop).WriteState == NullabilityState.Nullable);
|
||||
|
||||
var writable = !BaseDeniedFields.Contains(prop.Name);
|
||||
fields.Add(new TenantConfigFieldSchema(
|
||||
Name: char.ToLowerInvariant(prop.Name[0]) + prop.Name.Substring(1),
|
||||
Type: type,
|
||||
Format: format,
|
||||
Nullable: nullable,
|
||||
Writable: writable,
|
||||
Reason: writable ? null : DeniedReason(prop.Name),
|
||||
GaOnly: writable && GaOnlyFields.Contains(prop.Name),
|
||||
RevertProtected: RevertProtectedFields.Contains(prop.Name)));
|
||||
}
|
||||
return fields.OrderBy(f => f.Name, StringComparer.Ordinal).ToList();
|
||||
});
|
||||
|
||||
private static string DeniedReason(string name) => name switch
|
||||
{
|
||||
"TenantId" or "DomainName" => "identity — never writable",
|
||||
"LastUpdated" or "UpdatedBy" => "stamped server-side on every write",
|
||||
"OnboardedAt" or "OnboardedBy" => "system-owned onboarding provenance (set at first login)",
|
||||
"HomedAppClientId" => "system-owned — written only by the app-homing flow",
|
||||
"LastAuthClientId" or "LastAuthClientIdSince" => "system-owned auth provenance",
|
||||
"PlanTier" or "TrialExpiresUtc" or "TrialStartedUtc" or "TrialConsumed" or "TrialGrantedBy"
|
||||
=> "mutable only via the dedicated plan/trial endpoints",
|
||||
_ => "not writable via the field patch",
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// Schema-aware rehydration of a raw EntityJson snapshot back into a TableEntity.
|
||||
/// Strings are converted to DateTime ONLY for columns the model declares as DateTime —
|
||||
|
||||
@@ -38,7 +38,7 @@ describe('config-write tool registration', () => {
|
||||
const readerHandlers = captureToolHandlers(true, false);
|
||||
const tenantHandlers = captureToolHandlers(false, false);
|
||||
for (const name of [
|
||||
'get_tenant_config', 'update_tenant_config',
|
||||
'get_tenant_config', 'get_tenant_config_schema', 'update_tenant_config',
|
||||
'list_tenant_config_backups', 'revert_tenant_config',
|
||||
]) {
|
||||
expect(gaHandlers, `${name} missing for GA`).toHaveProperty(name);
|
||||
@@ -61,6 +61,18 @@ describe('get_tenant_config — forced redaction pin', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('get_tenant_config_schema — request shape', () => {
|
||||
beforeEach(() => apiFetchMock.mockReset());
|
||||
|
||||
it('GETs the tenant-independent fields-schema endpoint', async () => {
|
||||
apiFetchMock.mockResolvedValueOnce({ count: 90, fields: [] });
|
||||
await captureToolHandlers(true, true).get_tenant_config_schema({});
|
||||
|
||||
expect(apiFetchMock).toHaveBeenCalledTimes(1);
|
||||
expect(apiFetchMock.mock.calls[0][0]).toBe('/api/config/fields-schema');
|
||||
});
|
||||
});
|
||||
|
||||
describe('update_tenant_config — request shape', () => {
|
||||
beforeEach(() => apiFetchMock.mockReset());
|
||||
|
||||
|
||||
@@ -129,6 +129,7 @@ describe('role catalog snapshot — privilege-leak guard', () => {
|
||||
'get_session_summary',
|
||||
'get_software_inventory',
|
||||
'get_tenant_config',
|
||||
'get_tenant_config_schema',
|
||||
// Available to every role: backed by MemberRead endpoints (tenant) with role-aware
|
||||
// routing to the GlobalReadOrAdmin variant — same placement as get_app_install_metrics.
|
||||
'get_time_attribution',
|
||||
@@ -164,6 +165,7 @@ describe('role catalog snapshot — privilege-leak guard', () => {
|
||||
// operational config is a GA surface; Readers use list_tenants' keep-list view).
|
||||
const CONFIG_WRITE_GA_STRICT = [
|
||||
'get_tenant_config',
|
||||
'get_tenant_config_schema',
|
||||
'list_tenant_config_backups',
|
||||
'revert_tenant_config',
|
||||
'update_tenant_config',
|
||||
@@ -178,6 +180,7 @@ describe('role catalog snapshot — privilege-leak guard', () => {
|
||||
'get_ops_events',
|
||||
'get_platform_metrics',
|
||||
'get_tenant_config',
|
||||
'get_tenant_config_schema',
|
||||
'list_blocked_devices',
|
||||
'list_session_reports',
|
||||
'list_tables',
|
||||
|
||||
@@ -238,8 +238,9 @@ function buildInstructions(ga: boolean, strictGa: boolean, delegated: boolean, m
|
||||
const headline = strictGa
|
||||
? 'Autopilot-Monitor is a telemetry server for Windows Autopilot enrollment sessions. All investigation ' +
|
||||
'tools are read-only; as a Global Admin you additionally have tenant-configuration write tools ' +
|
||||
'(update_tenant_config, revert_tenant_config). Every config write is snapshotted first and verified ' +
|
||||
'after — use list_tenant_config_backups + revert_tenant_config to roll back.'
|
||||
'(update_tenant_config, revert_tenant_config). Call get_tenant_config_schema before composing a patch — ' +
|
||||
'it lists every field with its exact name and JSON type. Every config write is snapshotted first and ' +
|
||||
'verified after — use list_tenant_config_backups + revert_tenant_config to roll back.'
|
||||
: 'Autopilot-Monitor is a READ-ONLY telemetry server for Windows Autopilot enrollment sessions.';
|
||||
return [
|
||||
headline,
|
||||
|
||||
@@ -13,8 +13,9 @@ import type { DocsSearchBundle } from './search-provider.js';
|
||||
* backend endpoints stay GlobalAdminOnly: the raw tools (list_tables / query_table /
|
||||
* query_backend_logs — they can dump secret-bearing tables that the GlobalReader config redaction
|
||||
* would otherwise hide) and the tenant-config write surface (get_tenant_config /
|
||||
* update_tenant_config / list_tenant_config_backups / revert_tenant_config — the server's only
|
||||
* mutating tools; every write is snapshot-backed and verified server-side). When `ga` is false
|
||||
* get_tenant_config_schema / update_tenant_config / list_tenant_config_backups /
|
||||
* revert_tenant_config — the server's only mutating tools; every write is snapshot-backed
|
||||
* and verified server-side). When `ga` is false
|
||||
* (normal tenant user) no platform tool is registered at all — they never appear in tools/list —
|
||||
* and the remaining tools' descriptions carry no cross-tenant wording.
|
||||
*
|
||||
|
||||
@@ -1078,12 +1078,38 @@ export function registerAdminTools(server: McpServer, ga: boolean, strictGa: boo
|
||||
})
|
||||
);
|
||||
|
||||
if (strictGa) server.registerTool(
|
||||
'get_tenant_config_schema',
|
||||
{
|
||||
title: 'Get Tenant Config Field Schema',
|
||||
description:
|
||||
'Field schema of the tenant configuration for update_tenant_config: every field\'s exact name (camelCase), ' +
|
||||
'JSON type (string/boolean/integer/number), nullability (nullable ⇒ an explicit JSON null clears it), ' +
|
||||
'format hints ("json" = a JSON document passed AS A STRING, "date-time" = ISO 8601 string), whether it is ' +
|
||||
'writable (with the reason when not), and which fields a revert preserves by default. Tenant-independent — ' +
|
||||
'call it ONCE before composing an update_tenant_config patch so field names and value types are right on ' +
|
||||
'the first attempt. Generated server-side from the live model; never guess types instead of calling this.',
|
||||
inputSchema: {},
|
||||
annotations: READ_ONLY,
|
||||
},
|
||||
async (args) => withToolTelemetry('get_tenant_config_schema', async () => {
|
||||
try {
|
||||
const data = await apiFetch('/api/config/fields-schema');
|
||||
return toolResultText(data, MAX_RESULT_SIZE_CHARS.small);
|
||||
} catch (error: unknown) {
|
||||
return toolError('get_tenant_config_schema', args, error);
|
||||
}
|
||||
})
|
||||
);
|
||||
|
||||
if (strictGa) server.registerTool(
|
||||
'update_tenant_config',
|
||||
{
|
||||
title: 'Update Tenant Configuration',
|
||||
description:
|
||||
'Change specific fields of a tenant\'s configuration — transactional and verified. Global Admin only. ' +
|
||||
'Unsure about a field\'s exact name, type, or writability? Call get_tenant_config_schema FIRST — it lists ' +
|
||||
'every field with its JSON type, so the patch is right on the first attempt. ' +
|
||||
'Pass ONLY the fields to change (camelCase or PascalCase); omitted fields stay untouched; an explicit JSON ' +
|
||||
'null clears a nullable field. The backend snapshots the row first (fail-closed), writes conditionally, ' +
|
||||
're-reads, and verifies that exactly the intended fields changed — on any drift it rolls back automatically. ' +
|
||||
|
||||
Reference in New Issue
Block a user