diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index cb93c35b..1cc5be4f 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -837 +838 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index bbeb054e..700bb688 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1390 +1391 diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AdminConfigurationRateLimitSyncTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AdminConfigurationRateLimitSyncTests.cs new file mode 100644 index 00000000..8d947d08 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AdminConfigurationRateLimitSyncTests.cs @@ -0,0 +1,92 @@ +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Tests that 's background +/// rate-limit sync does NOT mutate . +/// +/// Regression: the sync used to overwrite UpdatedBy with the sentinel string +/// "System (Global Rate Limit Sync)". Downstream code in +/// PreviewWhitelistFunction reads UpdatedBy as the tenant's onboarding-requester +/// UPN and wrote it verbatim into TenantAdmins on preview approval — corrupting 10 +/// tenants before the bug was caught. +/// +/// +public class AdminConfigurationRateLimitSyncTests +{ + [Fact] + public async Task SaveConfigurationAsync_RateLimitSync_PreservesUpdatedBy() + { + var savedTenantConfigs = new List(); + var repo = new Mock(); + repo.Setup(r => r.SaveAdminConfigurationAsync(It.IsAny())) + .ReturnsAsync(true); + repo.Setup(r => r.GetAllTenantConfigurationsAsync()) + .ReturnsAsync(new List + { + new() { TenantId = "t1", UpdatedBy = "onboarder.t1@contoso.com", RateLimitRequestsPerMinute = 50 }, + new() { TenantId = "t2", UpdatedBy = "real.user.t2@example.com", RateLimitRequestsPerMinute = 50 }, + }); + repo.Setup(r => r.SaveTenantConfigurationAsync(It.IsAny())) + .Callback(c => savedTenantConfigs.Add(c)) + .ReturnsAsync(true); + + var sut = new AdminConfigurationService( + repo.Object, + NullLogger.Instance, + new MemoryCache(new MemoryCacheOptions())); + + var adminConfig = new AdminConfiguration + { + UpdatedBy = "global-admin@contoso.com", + GlobalRateLimitRequestsPerMinute = 200, + }; + + await sut.SaveConfigurationAsync(adminConfig); + + Assert.Equal(2, savedTenantConfigs.Count); + Assert.Equal("onboarder.t1@contoso.com", savedTenantConfigs[0].UpdatedBy); + Assert.Equal("real.user.t2@example.com", savedTenantConfigs[1].UpdatedBy); + // And the rate limit IS synced — sanity check the sync still runs + Assert.All(savedTenantConfigs, c => Assert.Equal(200, c.RateLimitRequestsPerMinute)); + } + + [Fact] + public async Task SaveConfigurationAsync_RateLimitSync_SkipsTenantsWithCustomOverride() + { + var savedTenantConfigs = new List(); + var repo = new Mock(); + repo.Setup(r => r.SaveAdminConfigurationAsync(It.IsAny())) + .ReturnsAsync(true); + repo.Setup(r => r.GetAllTenantConfigurationsAsync()) + .ReturnsAsync(new List + { + new() { TenantId = "custom-tenant", UpdatedBy = "owner@contoso.com", + RateLimitRequestsPerMinute = 1000, CustomRateLimitRequestsPerMinute = 1000 }, + }); + repo.Setup(r => r.SaveTenantConfigurationAsync(It.IsAny())) + .Callback(c => savedTenantConfigs.Add(c)) + .ReturnsAsync(true); + + var sut = new AdminConfigurationService( + repo.Object, + NullLogger.Instance, + new MemoryCache(new MemoryCacheOptions())); + + await sut.SaveConfigurationAsync(new AdminConfiguration + { + UpdatedBy = "global-admin@contoso.com", + GlobalRateLimitRequestsPerMinute = 200, + }); + + // Tenant with custom override is skipped — UpdatedBy untouched either way + Assert.Empty(savedTenantConfigs); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs index 542168ea..fa37f114 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs @@ -108,11 +108,15 @@ public class AuthFunctionSideEffectTests { var config = DefaultConfig(); config.DomainName = null!; + config.OnboardedBy = null; await _sut.HandleNewTenantDomainAsync(config, TenantId, Upn); Assert.Equal("contoso.com", config.DomainName); Assert.Equal(Upn, config.UpdatedBy); + // OnboardedBy is the immutable copy of the first-login UPN that auto-promote on + // preview approval reads — UpdatedBy may later be clobbered by background syncs. + Assert.Equal(Upn, config.OnboardedBy); _tenantConfigMock.Verify(x => x.SaveConfigurationAsync(config), Times.Once); _telegramMock.Verify(x => x.SendNewTenantSignupAsync(TenantId, Upn), Times.Once); _globalNotificationMock.Verify(x => x.CreateNotificationAsync( @@ -121,6 +125,20 @@ public class AuthFunctionSideEffectTests null), Times.Once); } + [Fact] + public async Task HandleNewTenantDomain_PreservesExistingOnboardedBy() + { + // Belt-and-suspenders: the method exits early when DomainName is set, but if + // anyone ever loosens that guard, OnboardedBy must still be immutable once set. + var config = DefaultConfig(); + config.DomainName = null!; + config.OnboardedBy = "original.requester@contoso.com"; + + await _sut.HandleNewTenantDomainAsync(config, TenantId, Upn); + + Assert.Equal("original.requester@contoso.com", config.OnboardedBy); + } + [Fact] public async Task HandleNewTenantDomain_WhenDomainAlreadySet_NoOp() { diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs new file mode 100644 index 00000000..34e22020 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs @@ -0,0 +1,134 @@ +using AutopilotMonitor.Functions.Functions.Rules; +using AutopilotMonitor.Shared.Models; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Unit tests for the static helpers PreviewWhitelistFunction.PickRequesterUpn +/// and PreviewWhitelistFunction.IsRealUserUpn. +/// +/// These guard the auto-promote-on-preview-approval path, which previously corrupted +/// 10 tenants' TenantAdmins rows by writing the sentinel string +/// "System (Global Rate Limit Sync)" as a TenantAdmin Upn. The fixes: +/// +/// shape-based UPN validation (no enumeration of sentinel strings), +/// prefer the immutable over the +/// mutable . +/// +/// +/// +public class PreviewWhitelistRequesterUpnTests +{ + // ------------------------------------------------------------------------- + // IsRealUserUpn — positive shape check, immune to new sentinel strings + // ------------------------------------------------------------------------- + + [Theory] + [InlineData("user@contoso.com")] + [InlineData("admin@tenant.onmicrosoft.com")] + [InlineData("first.last@example.de")] + [InlineData("cadm_user@xx3t8.onmicrosoft.com")] + public void IsRealUserUpn_AcceptsRealUpnShapes(string upn) + { + Assert.True(PreviewWhitelistFunction.IsRealUserUpn(upn)); + } + + [Theory] + [InlineData("System")] + [InlineData("system")] + [InlineData("System (auto-re-enable)")] + [InlineData("System (Global Rate Limit Sync)")] + [InlineData("system (global rate limit sync)")] + [InlineData("System (whatever-future-sentinel)")] + public void IsRealUserUpn_RejectsAnySystemPrefixSentinel(string upn) + { + // The shape-based prefix check is exactly the defense against the corruption + // pattern: any new "System (...)" sentinel introduced anywhere in the codebase + // is rejected without anyone needing to remember to update this method. + Assert.False(PreviewWhitelistFunction.IsRealUserUpn(upn)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("nodomain")] + [InlineData("just-a-string")] + public void IsRealUserUpn_RejectsNullOrNonEmailShapes(string? upn) + { + Assert.False(PreviewWhitelistFunction.IsRealUserUpn(upn)); + } + + // ------------------------------------------------------------------------- + // PickRequesterUpn — prefer OnboardedBy, fall back to UpdatedBy + // ------------------------------------------------------------------------- + + [Fact] + public void PickRequesterUpn_PrefersOnboardedByEvenWhenUpdatedByIsClean() + { + var cfg = new TenantConfiguration + { + OnboardedBy = "onboarder@contoso.com", + UpdatedBy = "editor@contoso.com", + }; + + Assert.Equal("onboarder@contoso.com", PreviewWhitelistFunction.PickRequesterUpn(cfg)); + } + + [Fact] + public void PickRequesterUpn_FallsBackToUpdatedByWhenOnboardedByNull() + { + var cfg = new TenantConfiguration + { + OnboardedBy = null, + UpdatedBy = "editor@contoso.com", + }; + + Assert.Equal("editor@contoso.com", PreviewWhitelistFunction.PickRequesterUpn(cfg)); + } + + [Fact] + public void PickRequesterUpn_FallsBackToUpdatedByWhenOnboardedByEmpty() + { + var cfg = new TenantConfiguration + { + OnboardedBy = "", + UpdatedBy = "editor@contoso.com", + }; + + Assert.Equal("editor@contoso.com", PreviewWhitelistFunction.PickRequesterUpn(cfg)); + } + + [Fact] + public void PickRequesterUpn_ShieldsAgainstUpdatedByClobberWhenOnboardedBySet() + { + // Regression: this is the exact scenario that corrupted 10 tenants. + // The global rate-limit sync clobbered UpdatedBy with the sentinel string; + // OnboardedBy preserves the real requester so auto-promote still picks them. + var cfg = new TenantConfiguration + { + OnboardedBy = "real.requester@contoso.com", + UpdatedBy = "System (Global Rate Limit Sync)", + }; + + Assert.Equal("real.requester@contoso.com", PreviewWhitelistFunction.PickRequesterUpn(cfg)); + Assert.True(PreviewWhitelistFunction.IsRealUserUpn(PreviewWhitelistFunction.PickRequesterUpn(cfg))); + } + + [Fact] + public void PickRequesterUpn_LegacyTenant_NoOnboardedBy_SentinelInUpdatedBy_IsRejectedByGuard() + { + // Defense-in-depth: even without OnboardedBy (pre-fix tenants), the IsRealUserUpn + // guard alone prevents the sentinel from leaking into TenantAdmins. + var cfg = new TenantConfiguration + { + OnboardedBy = null, + UpdatedBy = "System (Global Rate Limit Sync)", + }; + + var picked = PreviewWhitelistFunction.PickRequesterUpn(cfg); + Assert.Equal("System (Global Rate Limit Sync)", picked); + Assert.False(PreviewWhitelistFunction.IsRealUserUpn(picked)); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs index 330a624f..90937fa6 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableConfigRepository.cs @@ -310,6 +310,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage { "DomainName", config.DomainName }, { "LastUpdated", config.LastUpdated }, { "UpdatedBy", config.UpdatedBy }, + { "OnboardedBy", config.OnboardedBy }, { "Disabled", config.Disabled }, { "DisabledReason", config.DisabledReason }, { "DisabledUntil", config.DisabledUntil }, @@ -393,6 +394,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage DomainName = entity.GetString("DomainName") ?? "", LastUpdated = entity.GetDateTime("LastUpdated") ?? DateTime.UtcNow, UpdatedBy = entity.GetString("UpdatedBy") ?? "Unknown", + OnboardedBy = entity.GetString("OnboardedBy"), Disabled = entity.GetBoolean("Disabled") ?? false, DisabledReason = entity.GetString("DisabledReason"), DisabledUntil = entity.GetDateTime("DisabledUntil"), diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index 211d5832..2977b7b2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -262,6 +262,12 @@ public class AuthFunction _logger.LogInformation("Setting domain name for tenant {TenantId}: {Domain}", tenantId, domain); tenantConfig.DomainName = domain; tenantConfig.UpdatedBy = upn; + // OnboardedBy is immutable once set: this is the only place that writes it, gated + // on the same "DomainName is empty" condition (first-ever user login for the tenant). + // Downstream auto-promote (PreviewWhitelistFunction) reads OnboardedBy so background + // sync jobs that mutate UpdatedBy cannot leak sentinel strings into TenantAdmins. + if (string.IsNullOrWhiteSpace(tenantConfig.OnboardedBy)) + tenantConfig.OnboardedBy = upn; await _tenantConfigService.SaveConfigurationAsync(tenantConfig); // Fire-and-forget: Telegram diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs index 6170cc69..5ce9d03f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs @@ -2,6 +2,7 @@ using System.Net; using AutopilotMonitor.Functions.Extensions; using AutopilotMonitor.Functions.DataAccess.TableStorage; using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.Models; using Microsoft.AspNetCore.Authorization; using Microsoft.Azure.Functions.Worker; using Microsoft.Azure.Functions.Worker.Http; @@ -87,32 +88,37 @@ public class PreviewWhitelistFunction try { var tenantConfig = await _tenantConfigurationService.GetConfigurationAsync(tenantId); - var requesterUpn = tenantConfig.UpdatedBy; + var requesterUpn = PickRequesterUpn(tenantConfig); - if (!string.IsNullOrWhiteSpace(requesterUpn) - && !string.Equals(requesterUpn, "System", StringComparison.OrdinalIgnoreCase) - && !string.Equals(requesterUpn, "System (auto-re-enable)", StringComparison.OrdinalIgnoreCase)) + // Positive UPN-shape validation: real Azure AD UPNs always contain '@', while + // every system-written sentinel ("System", "System (auto-re-enable)", + // "System (Global Rate Limit Sync)", …) does not. An equality list against + // known sentinels previously missed a third sentinel and corrupted 10 tenants' + // TenantAdmins rows — keep this check shape-based, not enumeration-based. + if (IsRealUserUpn(requesterUpn)) { - var isAlreadyAdmin = await _tenantAdminsService.IsTenantAdminAsync(tenantId, requesterUpn); + // IsRealUserUpn returned true → requesterUpn is non-null, non-empty, contains '@' + var validUpn = requesterUpn!; + var isAlreadyAdmin = await _tenantAdminsService.IsTenantAdminAsync(tenantId, validUpn); if (!isAlreadyAdmin) { - await _tenantAdminsService.AddTenantAdminAsync(tenantId, requesterUpn, upn!); + await _tenantAdminsService.AddTenantAdminAsync(tenantId, validUpn, upn!); _logger.LogInformation( "Auto-promoted tenant requester {RequesterUpn} as TenantAdmin for tenant {TenantId} on preview approval by {ApprovedBy}", - requesterUpn, tenantId, upn); + validUpn, tenantId, upn); } else { _logger.LogInformation( "Tenant requester {RequesterUpn} is already a TenantAdmin for tenant {TenantId} — skipping auto-promote", - requesterUpn, tenantId); + validUpn, tenantId); } } else { _logger.LogInformation( - "No valid tenant requester UPN found in TenantConfiguration for tenant {TenantId} (UpdatedBy: '{UpdatedBy}') — skipping auto-promote", - tenantId, requesterUpn ?? ""); + "No valid tenant requester UPN found in TenantConfiguration for tenant {TenantId} (OnboardedBy: '{OnboardedBy}', UpdatedBy: '{UpdatedBy}') — skipping auto-promote", + tenantId, tenantConfig.OnboardedBy ?? "", tenantConfig.UpdatedBy ?? ""); } // Fire-and-forget: send welcome email if notification email is configured @@ -269,6 +275,36 @@ public class PreviewWhitelistFunction await response.WriteAsJsonAsync(new { message = "Welcome email sent", email }); return response; } + + /// + /// Picks the UPN of the user to auto-promote as TenantAdmin on preview approval. + /// Prefers (immutable, set once on first + /// user login) over (mutable, can be + /// clobbered by background jobs such as the global rate-limit sync). Fall back to + /// for tenants onboarded before + /// existed — callers still guard the + /// result with so a sentinel value cannot leak into + /// the TenantAdmins table. + /// + internal static string? PickRequesterUpn(TenantConfiguration config) + { + if (config == null) return null; + return !string.IsNullOrWhiteSpace(config.OnboardedBy) + ? config.OnboardedBy + : config.UpdatedBy; + } + + /// + /// True when the value looks like a real Azure AD user principal name + /// (contains '@' and does not start with the "System" sentinel prefix + /// used by background jobs that touch ). + /// + internal static bool IsRealUserUpn(string? upn) + { + if (string.IsNullOrWhiteSpace(upn)) return false; + if (upn.StartsWith("System", StringComparison.OrdinalIgnoreCase)) return false; + return upn.Contains('@'); + } } public class SaveNotificationEmailRequest diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AdminConfigurationService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AdminConfigurationService.cs index 918ef489..cb03cbdb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AdminConfigurationService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AdminConfigurationService.cs @@ -172,10 +172,15 @@ namespace AutopilotMonitor.Functions.Services continue; } - // Update tenant's rate limit to match global default + // Update tenant's rate limit to match global default. + // Deliberately do NOT touch UpdatedBy: this is a background sync, not an + // edit by a real user, and downstream code (e.g. PreviewWhitelistFunction + // auto-promote) treats UpdatedBy as the tenant's onboarding-requester UPN. + // Clobbering it with a sentinel string corrupted 10 tenants' TenantAdmins + // rows before this was caught. Audit trail stays via the LogInformation + // line below and via LastUpdated. tenantConfig.RateLimitRequestsPerMinute = globalRateLimit; tenantConfig.LastUpdated = DateTime.UtcNow; - tenantConfig.UpdatedBy = "System (Global Rate Limit Sync)"; await _configRepo.SaveTenantConfigurationAsync(tenantConfig); tenantsUpdated++; diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs b/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs index 166f8b52..e30003c4 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs @@ -33,6 +33,17 @@ namespace AutopilotMonitor.Shared.Models /// public string UpdatedBy { get; set; } = default!; + /// + /// UPN of the user whose first login created this tenant configuration. Set once + /// in HandleNewTenantDomainAsync alongside and + /// never overwritten. Used by the preview-approval auto-promote path so background + /// jobs that mutate (e.g. global rate-limit sync) cannot + /// leak a sentinel string into the TenantAdmins table. + /// Null on rows that pre-date the OnboardedBy field — auto-promote falls back to + /// with a UPN-shape guard. + /// + public string? OnboardedBy { get; set; } + // ===== TENANT STATUS ===== ///