[PM-21948] Warn on deprecated logging methods (#6101)

* Add warnings and scaffold tests

* Do some private reflection

* Add tests for warnings

* Add explainer comment

* Remove Reference to Azure CosmosDb Sink

* Don't warn on old file location

* Update test names

* Add syslog test

* dotnet format

* Add lazy syslog fix

* Add longer wait for file

* Make syslog test local only

* Switch to shortened URL

.github/renovate.json5

@@ -84,7 +84,6 @@
         "Serilog.AspNetCore",
         "Serilog.Extensions.Logging",
         "Serilog.Extensions.Logging.File",
-        "Serilog.Sinks.AzureCosmosDB",
         "Serilog.Sinks.SyslogMessages",
         "Stripe.net",
         "Swashbuckle.AspNetCore",

src/Core/Utilities/LoggerFactoryExtensions.cs

@@ -1,7 +1,4 @@
-// FIXME: Update this file to be null safe and then delete the line below
-#nullable disable
-
-using System.Security.Cryptography.X509Certificates;
+using System.Security.Cryptography.X509Certificates;
 using Bit.Core.Settings;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
@@ -33,7 +30,7 @@ public static class LoggerFactoryExtensions
     public static ILoggingBuilder AddSerilog(
         this ILoggingBuilder builder,
         WebHostBuilderContext context,
-        Func<LogEvent, IGlobalSettings, bool> filter = null)
+        Func<LogEvent, IGlobalSettings, bool>? filter = null)
     {
         var globalSettings = new GlobalSettings();
         ConfigurationBinder.Bind(context.Configuration.GetSection("GlobalSettings"), globalSettings);
@@ -57,19 +54,27 @@ public static class LoggerFactoryExtensions
             return filter(e, globalSettings);
         }
 
+        var logSentryWarning = false;
+        var logSyslogWarning = false;
+
+        // Path format is the only required option for file logging, we will use that as
+        // the keystone for if they have configured the new location.
+        var newPathFormat = context.Configuration["Logging:PathFormat"];
+
         var config = new LoggerConfiguration()
             .MinimumLevel.Verbose()
             .Enrich.FromLogContext()
             .Filter.ByIncludingOnly(inclusionPredicate);
 
-        if (CoreHelpers.SettingHasValue(globalSettings?.Sentry.Dsn))
+        if (CoreHelpers.SettingHasValue(globalSettings.Sentry.Dsn))
         {
             config.WriteTo.Sentry(globalSettings.Sentry.Dsn)
                 .Enrich.FromLogContext()
                 .Enrich.WithProperty("Project", globalSettings.ProjectName);
         }
-        else if (CoreHelpers.SettingHasValue(globalSettings?.Syslog.Destination))
+        else if (CoreHelpers.SettingHasValue(globalSettings.Syslog.Destination))
         {
+            logSyslogWarning = true;
             // appending sitename to project name to allow easier identification in syslog.
             var appName = $"{globalSettings.SiteName}-{globalSettings.ProjectName}";
             if (globalSettings.Syslog.Destination.Equals("local", StringComparison.OrdinalIgnoreCase))
@@ -107,10 +112,14 @@ public static class LoggerFactoryExtensions
                             certProvider: new CertificateFileProvider(globalSettings.Syslog.CertificatePath,
                                                                       globalSettings.Syslog?.CertificatePassword ?? string.Empty));
                     }
-
                 }
             }
         }
+        else if (!string.IsNullOrEmpty(newPathFormat))
+        {
+            // Use new location
+            builder.AddFile(context.Configuration.GetSection("Logging"));
+        }
         else if (CoreHelpers.SettingHasValue(globalSettings.LogDirectory))
         {
             if (globalSettings.LogRollBySizeLimit.HasValue)
@@ -138,6 +147,17 @@ public static class LoggerFactoryExtensions
         }
 
         var serilog = config.CreateLogger();
+
+        if (logSentryWarning)
+        {
+            serilog.Warning("Sentry for logging has been deprecated. Read more: https://btwrdn.com/log-deprecation");
+        }
+
+        if (logSyslogWarning)
+        {
+            serilog.Warning("Syslog for logging has been deprecated. Read more: https://btwrdn.com/log-deprecation");
+        }
+
         builder.AddSerilog(serilog);
 
         return builder;

test/Core.Test/Utilities/LoggerFactoryExtensionsTests.cs

@@ -0,0 +1,195 @@
+using System.Net;
+using System.Net.Sockets;
+using System.Text;
+using Bit.Core.Utilities;
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using NSubstitute;
+using Serilog;
+using Serilog.Extensions.Logging;
+using Xunit;
+
+namespace Bit.Core.Test.Utilities;
+
+public class LoggerFactoryExtensionsTests
+{
+    [Fact]
+    public void AddSerilog_IsDevelopment_AddsNoProviders()
+    {
+        var providers = GetProviders([], "Development");
+
+        Assert.Empty(providers);
+    }
+
+    [Fact]
+    public void AddSerilog_IsDevelopment_DevLoggingEnabled_AddsSerilog()
+    {
+        var providers = GetProviders(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:EnableDevLogging", "true" },
+        }, "Development");
+
+        var provider = Assert.Single(providers);
+        Assert.IsAssignableFrom<SerilogLoggerProvider>(provider);
+    }
+
+    [Fact]
+    public void AddSerilog_IsProduction_AddsSerilog()
+    {
+        var providers = GetProviders([]);
+
+        var provider = Assert.Single(providers);
+        Assert.IsAssignableFrom<SerilogLoggerProvider>(provider);
+    }
+
+    [Fact]
+    public async Task AddSerilog_FileLogging_Old_Works()
+    {
+        var tempDir = Directory.CreateTempSubdirectory();
+
+        var providers = GetProviders(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:ProjectName", "Test" },
+            { "GlobalSetting:LogDirectoryByProject", "true" },
+            { "GlobalSettings:LogDirectory", tempDir.FullName },
+        });
+
+        var provider = Assert.Single(providers);
+        Assert.IsAssignableFrom<SerilogLoggerProvider>(provider);
+
+        var logger = provider.CreateLogger("Test");
+        logger.LogWarning("This is a test");
+
+        var logFile = Assert.Single(tempDir.EnumerateFiles("Test/*.txt"));
+
+        var logFileContents = await File.ReadAllTextAsync(logFile.FullName);
+
+        Assert.Contains(
+            "This is a test",
+            logFileContents
+        );
+    }
+
+    [Fact]
+    public async Task AddSerilog_FileLogging_New_Works()
+    {
+        var tempDir = Directory.CreateTempSubdirectory();
+
+        var provider = GetServiceProvider(new Dictionary<string, string?>
+        {
+            { "Logging:PathFormat", $"{tempDir}/Logs/log-{{Date}}.log" },
+        }, "Production");
+
+        var logger = provider
+            .GetRequiredService<ILoggerFactory>()
+            .CreateLogger("Test");
+
+        logger.LogWarning("This is a test");
+
+        // Writing to the file is buffered, give it a little time to flush
+        await Task.Delay(5);
+
+        var logFile = Assert.Single(tempDir.EnumerateFiles("Logs/*.log"));
+
+        var logFileContents = await File.ReadAllTextAsync(logFile.FullName);
+
+        Assert.DoesNotContain(
+            "This configuration location for file logging has been deprecated.",
+            logFileContents
+        );
+        Assert.Contains(
+            "This is a test",
+            logFileContents
+        );
+    }
+
+    [Fact(Skip = "Only for local development.")]
+    public async Task AddSerilog_SyslogConfigured_Warns()
+    {
+        // Setup a fake syslog server
+        var cts = new CancellationTokenSource(TimeSpan.FromSeconds(20));
+        using var listener = new TcpListener(IPAddress.Parse("127.0.0.1"), 25000);
+        listener.Start();
+
+        var provider = GetServiceProvider(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:SysLog:Destination", "tcp://127.0.0.1:25000" },
+            { "GlobalSettings:SiteName", "TestSite" },
+            { "GlobalSettings:ProjectName", "TestProject" },
+        }, "Production");
+
+        var loggerFactory = provider.GetRequiredService<ILoggerFactory>();
+        var logger = loggerFactory.CreateLogger("Test");
+
+        logger.LogWarning("This is a test");
+
+        // Look in syslog for data
+        using var socket = await listener.AcceptSocketAsync(cts.Token);
+
+        // This is rather lazy as opposed to implementing smarter syslog message
+        // reading but thats not what this test about, so instead just give
+        // the sink time to finish its work in the background
+
+        List<string> messages = [];
+
+        while (true)
+        {
+            var buffer = new byte[1024];
+            var received = await socket.ReceiveAsync(buffer, SocketFlags.None, cts.Token);
+
+            if (received == 0)
+            {
+                break;
+            }
+
+            var response = Encoding.ASCII.GetString(buffer, 0, received);
+            messages.Add(response);
+
+            if (messages.Count == 2)
+            {
+                break;
+            }
+        }
+
+        Assert.Collection(
+            messages,
+            (firstMessage) => Assert.Contains("Syslog for logging has been deprecated", firstMessage),
+            (secondMessage) => Assert.Contains("This is a test", secondMessage)
+        );
+    }
+
+    private static IEnumerable<ILoggerProvider> GetProviders(Dictionary<string, string?> initialData, string environment = "Production")
+    {
+        var provider = GetServiceProvider(initialData, environment);
+        return provider.GetServices<ILoggerProvider>();
+    }
+
+    private static IServiceProvider GetServiceProvider(Dictionary<string, string?> initialData, string environment)
+    {
+        var config = new ConfigurationBuilder()
+            .AddInMemoryCollection(initialData)
+            .Build();
+
+        var hostingEnvironment = Substitute.For<IWebHostEnvironment>();
+
+        hostingEnvironment
+            .EnvironmentName
+            .Returns(environment);
+
+        var context = new WebHostBuilderContext
+        {
+            HostingEnvironment = hostingEnvironment,
+            Configuration = config,
+        };
+
+        var services = new ServiceCollection();
+        services.AddLogging(builder =>
+        {
+            builder.AddSerilog(context);
+        });
+
+        return services.BuildServiceProvider();
+    }
+}