fix onboarding bug, onboardedBy and auto promote admin

src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt

@@ -1 +1 @@
-837
+838

src/Agent/AutopilotMonitor.Agent/buildcounter.txt

@@ -1 +1 @@
-1390
+1391

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;
+
+/// <summary>
+/// Tests that <see cref="AdminConfigurationService.SaveConfigurationAsync"/>'s background
+/// rate-limit sync does NOT mutate <see cref="TenantConfiguration.UpdatedBy"/>.
+/// <para>
+/// Regression: the sync used to overwrite UpdatedBy with the sentinel string
+/// <c>"System (Global Rate Limit Sync)"</c>. Downstream code in
+/// <c>PreviewWhitelistFunction</c> 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.
+/// </para>
+/// </summary>
+public class AdminConfigurationRateLimitSyncTests
+{
+    [Fact]
+    public async Task SaveConfigurationAsync_RateLimitSync_PreservesUpdatedBy()
+    {
+        var savedTenantConfigs = new List<TenantConfiguration>();
+        var repo = new Mock<IConfigRepository>();
+        repo.Setup(r => r.SaveAdminConfigurationAsync(It.IsAny<AdminConfiguration>()))
+            .ReturnsAsync(true);
+        repo.Setup(r => r.GetAllTenantConfigurationsAsync())
+            .ReturnsAsync(new List<TenantConfiguration>
+            {
+                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<TenantConfiguration>()))
+            .Callback<TenantConfiguration>(c => savedTenantConfigs.Add(c))
+            .ReturnsAsync(true);
+
+        var sut = new AdminConfigurationService(
+            repo.Object,
+            NullLogger<AdminConfigurationService>.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<TenantConfiguration>();
+        var repo = new Mock<IConfigRepository>();
+        repo.Setup(r => r.SaveAdminConfigurationAsync(It.IsAny<AdminConfiguration>()))
+            .ReturnsAsync(true);
+        repo.Setup(r => r.GetAllTenantConfigurationsAsync())
+            .ReturnsAsync(new List<TenantConfiguration>
+            {
+                new() { TenantId = "custom-tenant", UpdatedBy = "owner@contoso.com",
+                        RateLimitRequestsPerMinute = 1000, CustomRateLimitRequestsPerMinute = 1000 },
+            });
+        repo.Setup(r => r.SaveTenantConfigurationAsync(It.IsAny<TenantConfiguration>()))
+            .Callback<TenantConfiguration>(c => savedTenantConfigs.Add(c))
+            .ReturnsAsync(true);
+
+        var sut = new AdminConfigurationService(
+            repo.Object,
+            NullLogger<AdminConfigurationService>.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);
+    }
+}

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()
     {

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;
+
+/// <summary>
+/// Unit tests for the static helpers <c>PreviewWhitelistFunction.PickRequesterUpn</c>
+/// and <c>PreviewWhitelistFunction.IsRealUserUpn</c>.
+/// <para>
+/// These guard the auto-promote-on-preview-approval path, which previously corrupted
+/// 10 tenants' TenantAdmins rows by writing the sentinel string
+/// <c>"System (Global Rate Limit Sync)"</c> as a TenantAdmin Upn. The fixes:
+/// <list type="number">
+/// <item>shape-based UPN validation (no enumeration of sentinel strings),</item>
+/// <item>prefer the immutable <see cref="TenantConfiguration.OnboardedBy"/> over the
+/// mutable <see cref="TenantConfiguration.UpdatedBy"/>.</item>
+/// </list>
+/// </para>
+/// </summary>
+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));
+    }
+}

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"),

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

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 ?? "<null>");
+                    "No valid tenant requester UPN found in TenantConfiguration for tenant {TenantId} (OnboardedBy: '{OnboardedBy}', UpdatedBy: '{UpdatedBy}') — skipping auto-promote",
+                    tenantId, tenantConfig.OnboardedBy ?? "<null>", tenantConfig.UpdatedBy ?? "<null>");
             }
 
             // 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;
     }
+
+    /// <summary>
+    /// Picks the UPN of the user to auto-promote as TenantAdmin on preview approval.
+    /// Prefers <see cref="TenantConfiguration.OnboardedBy"/> (immutable, set once on first
+    /// user login) over <see cref="TenantConfiguration.UpdatedBy"/> (mutable, can be
+    /// clobbered by background jobs such as the global rate-limit sync). Fall back to
+    /// <see cref="TenantConfiguration.UpdatedBy"/> for tenants onboarded before
+    /// <see cref="TenantConfiguration.OnboardedBy"/> existed — callers still guard the
+    /// result with <see cref="IsRealUserUpn"/> so a sentinel value cannot leak into
+    /// the TenantAdmins table.
+    /// </summary>
+    internal static string? PickRequesterUpn(TenantConfiguration config)
+    {
+        if (config == null) return null;
+        return !string.IsNullOrWhiteSpace(config.OnboardedBy)
+            ? config.OnboardedBy
+            : config.UpdatedBy;
+    }
+
+    /// <summary>
+    /// 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 <see cref="TenantConfiguration.UpdatedBy"/>).
+    /// </summary>
+    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

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++;

src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs

@@ -33,6 +33,17 @@ namespace AutopilotMonitor.Shared.Models
         /// </summary>
         public string UpdatedBy { get; set; } = default!;
 
+        /// <summary>
+        /// UPN of the user whose first login created this tenant configuration. Set once
+        /// in <c>HandleNewTenantDomainAsync</c> alongside <see cref="DomainName"/> and
+        /// never overwritten. Used by the preview-approval auto-promote path so background
+        /// jobs that mutate <see cref="UpdatedBy"/> (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
+        /// <see cref="UpdatedBy"/> with a UPN-shape guard.
+        /// </summary>
+        public string? OnboardedBy { get; set; }
+
         // ===== TENANT STATUS =====
 
         /// <summary>