From d15b7d74b00b15d6b57c792abf46cdc80f8b954c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 25 Dec 2024 08:30:28 +0100 Subject: [PATCH 1/8] Add sentry integration --- README.md | 1 + .../Configuration/AppSettings.cs | 2 ++ osu.Server.BeatmapSubmission/Program.cs | 14 +++++++++++++- .../osu.Server.BeatmapSubmission.csproj | 1 + 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d3aa73e..f06fea1 100644 --- a/README.md +++ b/README.md @@ -40,4 +40,5 @@ For advanced testing purposes. | `S3_ACCESS_KEY` | A valid Amazon S3 access key ID. | ⚠ In staging/production configs | None | | `S3_SECRET_KEY` | The secret key corresponding to the `S3_ACCESS_KEY`. | ⚠ In staging/production configs | None | | `S3_BUCKET_NAME` | The name of the S3 bucket to use for beatmap storage. | ⚠ In staging/production configs | None | +| `SENTRY_DSN` | A valid Sentry DSN to use for logging application events. | ⚠ In staging/production configs | None | diff --git a/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs b/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs index e086f10..83db79a 100644 --- a/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs +++ b/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs @@ -40,5 +40,7 @@ public static class AppSettings Environment.GetEnvironmentVariable("S3_BUCKET_NAME") ?? throw new InvalidOperationException("S3_BUCKET_NAME environment variable not set. " + "Please set the value of this variable to the name of the bucket to be used for storing beatmaps on S3."); + + public static string? SentryDsn => Environment.GetEnvironmentVariable("SENTRY_DSN"); } } diff --git a/osu.Server.BeatmapSubmission/Program.cs b/osu.Server.BeatmapSubmission/Program.cs index cb48220..826d9be 100644 --- a/osu.Server.BeatmapSubmission/Program.cs +++ b/osu.Server.BeatmapSubmission/Program.cs @@ -3,6 +3,7 @@ using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.Extensions.Options; using osu.Server.BeatmapSubmission.Authentication; +using osu.Server.BeatmapSubmission.Configuration; using osu.Server.BeatmapSubmission.Services; namespace osu.Server.BeatmapSubmission @@ -24,7 +25,8 @@ public static void Main(string[] args) { logging.ClearProviders(); logging.AddConsole(); - // TODO: sentry + if (AppSettings.SentryDsn != null) + logging.AddSentry(); }); builder.Services.AddSingleton, OsuWebSharedJwtBearerOptions>(); @@ -59,10 +61,20 @@ public static void Main(string[] args) builder.Services.AddHttpClient(); builder.Services.AddTransient(); builder.Services.AddTransient(); + + if (AppSettings.SentryDsn == null) + { + throw new InvalidOperationException("SENTRY_DSN environment variable not set. " + + "Please set the value of this variable to a valid Sentry DSN to use for logging events."); + } + break; } } + if (AppSettings.SentryDsn != null) + builder.WebHost.UseSentry(options => options.Dsn = AppSettings.SentryDsn); + var app = builder.Build(); if (app.Environment.IsDevelopment()) diff --git a/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj b/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj index 9d53794..51d1b7d 100644 --- a/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj +++ b/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj @@ -24,6 +24,7 @@ + From 2d2fe34f4273a7b3fd96904a1ea950ffd31d5c0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 25 Dec 2024 08:40:45 +0100 Subject: [PATCH 2/8] Add datadog integration --- README.md | 1 + .../Configuration/AppSettings.cs | 2 ++ osu.Server.BeatmapSubmission/Program.cs | 22 +++++++++++++++++++ .../osu.Server.BeatmapSubmission.csproj | 1 + 4 files changed, 26 insertions(+) diff --git a/README.md b/README.md index f06fea1..68676a4 100644 --- a/README.md +++ b/README.md @@ -41,4 +41,5 @@ For advanced testing purposes. | `S3_SECRET_KEY` | The secret key corresponding to the `S3_ACCESS_KEY`. | ⚠ In staging/production configs | None | | `S3_BUCKET_NAME` | The name of the S3 bucket to use for beatmap storage. | ⚠ In staging/production configs | None | | `SENTRY_DSN` | A valid Sentry DSN to use for logging application events. | ⚠ In staging/production configs | None | +| `DD_AGENT_HOST` | A hostname pointing to a Datadog agent instance to which metrics should be reported. | ⚠ In staging/production configs | None | diff --git a/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs b/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs index 83db79a..c9d3901 100644 --- a/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs +++ b/osu.Server.BeatmapSubmission/Configuration/AppSettings.cs @@ -42,5 +42,7 @@ public static class AppSettings + "Please set the value of this variable to the name of the bucket to be used for storing beatmaps on S3."); public static string? SentryDsn => Environment.GetEnvironmentVariable("SENTRY_DSN"); + + public static string? DatadogAgentHost => Environment.GetEnvironmentVariable("DD_AGENT_HOST"); } } diff --git a/osu.Server.BeatmapSubmission/Program.cs b/osu.Server.BeatmapSubmission/Program.cs index 826d9be..a8ed0c2 100644 --- a/osu.Server.BeatmapSubmission/Program.cs +++ b/osu.Server.BeatmapSubmission/Program.cs @@ -1,3 +1,4 @@ +using System.Net; using System.Reflection; using JetBrains.Annotations; using Microsoft.AspNetCore.Authentication.JwtBearer; @@ -5,6 +6,7 @@ using osu.Server.BeatmapSubmission.Authentication; using osu.Server.BeatmapSubmission.Configuration; using osu.Server.BeatmapSubmission.Services; +using StatsdClient; namespace osu.Server.BeatmapSubmission { @@ -68,6 +70,12 @@ public static void Main(string[] args) + "Please set the value of this variable to a valid Sentry DSN to use for logging events."); } + if (AppSettings.DatadogAgentHost == null) + { + throw new InvalidOperationException("DD_AGENT_HOST environment variable not set. " + + "Please set the value of this variable to a valid hostname of a Datadog agent."); + } + break; } } @@ -75,6 +83,20 @@ public static void Main(string[] args) if (AppSettings.SentryDsn != null) builder.WebHost.UseSentry(options => options.Dsn = AppSettings.SentryDsn); + if (AppSettings.DatadogAgentHost != null) + { + DogStatsd.Configure(new StatsdConfig + { + StatsdServerName = AppSettings.DatadogAgentHost, + Prefix = "osu.server.beatmap-submission", + ConstantTags = new[] + { + $@"hostname:{Dns.GetHostName()}", + $@"startup:{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}" + } + }); + } + var app = builder.Build(); if (app.Environment.IsDevelopment()) diff --git a/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj b/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj index 51d1b7d..a18727e 100644 --- a/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj +++ b/osu.Server.BeatmapSubmission/osu.Server.BeatmapSubmission.csproj @@ -16,6 +16,7 @@ + From 0e84ff1e00c308948c7d8c997a85ed94f0586524 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 25 Dec 2024 08:54:33 +0100 Subject: [PATCH 3/8] Report successful submissions to datadog --- osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs b/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs index 8795058..f631a4d 100644 --- a/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs +++ b/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs @@ -14,6 +14,7 @@ using osu.Server.BeatmapSubmission.Models.Database; using osu.Server.BeatmapSubmission.Services; using osu.Server.QueueProcessor; +using StatsdClient; namespace osu.Server.BeatmapSubmission { @@ -207,6 +208,7 @@ public async Task UploadFullPackageAsync( await legacyIO.BroadcastUpdateBeatmapSetEventAsync(beatmapSetId, userId); } + DogStatsd.Increment("submissions_completed", tags: ["full"]); return NoContent(); } @@ -260,6 +262,7 @@ public async Task PatchPackageAsync( if (await updateBeatmapSetFromArchiveAsync(beatmapSetId, beatmapStream, db)) await legacyIO.BroadcastUpdateBeatmapSetEventAsync(beatmapSetId, userId); + DogStatsd.Increment("submissions_completed", tags: ["update"]); return NoContent(); } @@ -314,6 +317,7 @@ public async Task UploadGuestDifficultyAsync( if (await updateBeatmapSetFromArchiveAsync(beatmapSetId, archiveStream, db)) await legacyIO.BroadcastUpdateBeatmapSetEventAsync(beatmapSetId, userId); + DogStatsd.Increment("submissions_completed", tags: ["guest"]); return NoContent(); } From fd730129f4e18d2d22a461f1c08c088ec1a4f361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 25 Dec 2024 10:51:07 +0100 Subject: [PATCH 4/8] Add a bunch of logging everywhere --- .../BeatmapSubmissionController.cs | 110 ++++++++++++------ .../DatabaseOperationExtensions.cs | 4 +- .../InvariantExceptionFilter.cs | 10 ++ .../Models/InvariantException.cs | 5 +- .../Services/BeatmapPackagePatcher.cs | 2 +- .../Services/LegacyIO.cs | 11 +- .../Services/MirrorService.cs | 10 +- .../Services/S3BeatmapStorage.cs | 23 +++- 8 files changed, 125 insertions(+), 50 deletions(-) diff --git a/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs b/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs index f631a4d..318958d 100644 --- a/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs +++ b/osu.Server.BeatmapSubmission/BeatmapSubmissionController.cs @@ -21,17 +21,20 @@ namespace osu.Server.BeatmapSubmission [Authorize] public class BeatmapSubmissionController : Controller { + private readonly ILogger logger; private readonly IBeatmapStorage beatmapStorage; private readonly BeatmapPackagePatcher patcher; private readonly ILegacyIO legacyIO; private readonly IMirrorService mirrorService; public BeatmapSubmissionController( + ILogger logger, IBeatmapStorage beatmapStorage, BeatmapPackagePatcher patcher, ILegacyIO legacyIO, IMirrorService mirrorService) { + this.logger = logger; this.beatmapStorage = beatmapStorage; this.patcher = patcher; this.legacyIO = legacyIO; @@ -57,12 +60,10 @@ public async Task PutBeatmapSetAsync([FromBody] PutBeatmapSetRequ using var db = await DatabaseAccess.GetConnectionAsync(); - ErrorResponse? userError = await checkUserAccountStanding(db, userId); - if (userError != null) - return userError.ToActionResult(); + await checkUserAccountStanding(db, userId); if (await db.GetUserMonthlyPlaycountAsync(userId) < 5) - return new ErrorResponse("Thanks for your contribution, but please play the game first!").ToActionResult(); + throw new InvariantException("Thanks for your contribution, but please play the game first!"); uint? beatmapSetId = request.BeatmapSetID; uint[] existingBeatmaps = []; @@ -70,19 +71,22 @@ public async Task PutBeatmapSetAsync([FromBody] PutBeatmapSetRequ using var transaction = await db.BeginTransactionAsync(); - await db.PurgeInactiveBeatmapSetsForUserAsync(userId, transaction); + uint[] purgedMaps = await db.PurgeInactiveBeatmapSetsForUserAsync(userId, transaction); + if (purgedMaps.Length > 0) + logger.LogInformation("Purging inactive beatmap sets for user {userId} (ids: {purgedMaps})", userId, purgedMaps); + (uint totalSlots, uint remainingSlots) = await getUploadQuota(db, userId, transaction); if (beatmapSetId == null) { if (request.BeatmapsToKeep.Length != 0) - return new ErrorResponse("Cannot specify beatmaps to keep when creating a new beatmap set.").ToActionResult(); + throw new InvariantException("Cannot specify beatmaps to keep when creating a new beatmap set.", LogLevel.Warning); if (remainingSlots <= 0) { - return new ErrorResponse($"You have exceeded your submission cap (you are currently allowed {totalSlots} total unranked maps). " - + $"Please finish the maps you currently have submitted, or wait until your submissions expire automatically to the graveyard " - + $"(about 4 weeks since last updated).").ToActionResult(); + throw new InvariantException($"You have exceeded your submission cap (you are currently allowed {totalSlots} total unranked maps). " + + $"Please finish the maps you currently have submitted, or wait until your submissions expire automatically to the graveyard " + + $"(about 4 weeks since last updated)."); } string username = await db.GetUsernameAsync(userId, transaction); @@ -96,13 +100,20 @@ public async Task PutBeatmapSetAsync([FromBody] PutBeatmapSetRequ return NotFound(); if (beatmapSet.user_id != userId) + { + logger.LogWarning("User {submitter} attempted to modify beatmap set {beatmapSetId} of user {owner}", userId, beatmapSetId, beatmapSet.user_id); return Forbid(); + } if (beatmapSet.approved >= BeatmapOnlineStatus.Ranked) + { + logger.LogWarning("User {submitter} attempted to modify ranked beatmap set {beatmapSetId}", userId, beatmapSetId); return Forbid(); + } if (beatmapSet.approved == BeatmapOnlineStatus.Graveyard) { + logger.LogInformation("Reviving beatmap set {beatmapSetId}", beatmapSetId); await reviveBeatmapSet(db, userId, beatmapSet, request.Target, transaction); beatmapRevived = true; } @@ -111,22 +122,33 @@ public async Task PutBeatmapSetAsync([FromBody] PutBeatmapSetRequ } if (request.BeatmapsToKeep.Except(existingBeatmaps).Any()) - return new ErrorResponse("One of the beatmaps to keep does not belong to the specified set.").ToActionResult(); + throw new InvariantException("One of the beatmaps to keep does not belong to the specified set.", LogLevel.Warning); uint totalBeatmapCount = (uint)request.BeatmapsToKeep.Length + request.BeatmapsToCreate; + if (totalBeatmapCount < 1) - return new ErrorResponse("The beatmap set must contain at least one beatmap.").ToActionResult(); + throw new InvariantException("The beatmap set must contain at least one beatmap."); + if (totalBeatmapCount > 128) - return new ErrorResponse("The beatmap set cannot contain more than 128 beatmaps.").ToActionResult(); + throw new InvariantException("The beatmap set cannot contain more than 128 beatmaps."); // C# enums suck, so this needs to be explicitly checked to prevent bad actors from doing "funny" stuff. if (!Enum.IsDefined(request.Target)) + { + logger.LogWarning("User {submitter} attempted to send malicious parameters to change the ranked status of {beatmapSetId} to illegal value", userId, beatmapSetId); return Forbid(); + } + + IEnumerable beatmapsToDelete = existingBeatmaps.Except(request.BeatmapsToKeep).ToArray(); - foreach (uint beatmapId in existingBeatmaps.Except(request.BeatmapsToKeep)) - await db.DeleteBeatmapAsync(beatmapId, transaction); + if (beatmapsToDelete.Any()) + { + logger.LogInformation("Deleting beatmaps {beatmapIds} in set {beatmapSetId}", beatmapsToDelete, beatmapSetId); + foreach (uint beatmapId in beatmapsToDelete) + await db.DeleteBeatmapAsync(beatmapId, transaction); + } - var beatmapIds = new List(request.BeatmapsToKeep); + var beatmapIds = new List(); for (int i = 0; i < request.BeatmapsToCreate; ++i) { @@ -134,6 +156,11 @@ public async Task PutBeatmapSetAsync([FromBody] PutBeatmapSetRequ beatmapIds.Add(beatmapId); } + if (beatmapIds.Count > 0) + logger.LogInformation("Creating beatmaps {beatmapIds} in set {beatmapSetId}", beatmapIds, beatmapSetId); + + beatmapIds.AddRange(request.BeatmapsToKeep); + await db.SetBeatmapSetOnlineStatusAsync(beatmapSetId.Value, (BeatmapOnlineStatus)request.Target, transaction); await db.UpdateBeatmapCountForSet(beatmapSetId.Value, totalBeatmapCount, transaction); @@ -177,22 +204,26 @@ public async Task UploadFullPackageAsync( using var db = await DatabaseAccess.GetConnectionAsync(); - ErrorResponse? userError = await checkUserAccountStanding(db, userId); - if (userError != null) - return userError.ToActionResult(); + await checkUserAccountStanding(db, userId); var beatmapSet = await db.GetBeatmapSetAsync(beatmapSetId); if (beatmapSet == null) return NotFound(); if (beatmapSet.user_id != User.GetUserId()) + { + logger.LogWarning("User {submitter} attempted to modify beatmap set {beatmapSetId} of user {owner}", userId, beatmapSetId, beatmapSet.user_id); return Forbid(); + } if (beatmapSet.approved >= BeatmapOnlineStatus.Ranked) + { + logger.LogWarning("User {submitter} attempted to modify ranked beatmap set {beatmapSetId}", userId, beatmapSetId); return Forbid(); + } if (beatmapSet.approved == BeatmapOnlineStatus.Graveyard) - return new ErrorResponse("The beatmap set must be revived first.").ToActionResult(); + throw new InvariantException("The beatmap set must be revived first.", LogLevel.Warning); using var beatmapStream = beatmapArchive.OpenReadStream(); @@ -234,28 +265,32 @@ public async Task PatchPackageAsync( uint userId = User.GetUserId(); using var db = await DatabaseAccess.GetConnectionAsync(); - ErrorResponse? userError = await checkUserAccountStanding(db, userId); - if (userError != null) - return userError.ToActionResult(); + await checkUserAccountStanding(db, userId); var beatmapSet = await db.GetBeatmapSetAsync(beatmapSetId); if (beatmapSet == null) return NotFound(); if (beatmapSet.user_id != User.GetUserId()) + { + logger.LogWarning("User {submitter} attempted to modify beatmap set {beatmapSetId} of user {owner}", userId, beatmapSetId, beatmapSet.user_id); return Forbid(); + } if (beatmapSet.approved >= BeatmapOnlineStatus.Ranked) + { + logger.LogWarning("User {submitter} attempted to modify ranked beatmap set {beatmapSetId}", userId, beatmapSetId); return Forbid(); + } if (await db.GetLatestBeatmapsetVersionAsync(beatmapSetId) == null) return NotFound(); if (beatmapSet.approved == BeatmapOnlineStatus.Graveyard) - return new ErrorResponse("The beatmap set must be revived first.").ToActionResult(); + throw new InvariantException("The beatmap set must be revived first.", LogLevel.Warning); if (filesChanged.Any(f => SanityCheckHelpers.IncursPathTraversalRisk(f.FileName))) - return new ErrorResponse("Invalid filename detected").ToActionResult(); + throw new InvariantException("Invalid filename detected", LogLevel.Warning); var beatmapStream = await patcher.PatchBeatmapSetAsync(beatmapSetId, filesChanged, filesDeleted); @@ -287,9 +322,7 @@ public async Task UploadGuestDifficultyAsync( uint userId = User.GetUserId(); using var db = await DatabaseAccess.GetConnectionAsync(); - ErrorResponse? userError = await checkUserAccountStanding(db, userId); - if (userError != null) - return userError.ToActionResult(); + await checkUserAccountStanding(db, userId); var beatmapSet = await db.GetBeatmapSetAsync(beatmapSetId); var beatmap = await db.GetBeatmapAsync(beatmapSetId, beatmapId); @@ -297,20 +330,27 @@ public async Task UploadGuestDifficultyAsync( return NotFound(); if (beatmapSet.approved >= BeatmapOnlineStatus.Ranked) + { + logger.LogWarning("User {submitter} attempted to modify ranked beatmap set {beatmapSetId}", userId, beatmapSetId); return Forbid(); + } IEnumerable beatmapOwners = await db.GetBeatmapOwnersAsync(beatmap.beatmap_id); + if (beatmap.user_id != userId && !beatmapOwners.Contains(userId)) + { + logger.LogWarning("User {submitter} attempted to modify beatmap {beatmapId} in set {beatmapSetId} they are not owner of", userId, beatmapId, beatmapSetId); return Forbid(); + } if (await db.GetLatestBeatmapsetVersionAsync(beatmapSetId) == null) return NotFound(); if (beatmapSet.approved == BeatmapOnlineStatus.Graveyard) - return new ErrorResponse("The beatmap set is in the graveyard. Please ask the set owner to revive it first.").ToActionResult(); + throw new InvariantException("The beatmap set is in the graveyard. Please ask the set owner to revive it first.", LogLevel.Warning); if (SanityCheckHelpers.IncursPathTraversalRisk(beatmapContents.FileName)) - return new ErrorResponse("Invalid filename detected").ToActionResult(); + throw new InvariantException("Invalid filename detected", LogLevel.Warning); var archiveStream = await patcher.PatchBeatmapAsync(beatmapSetId, beatmap, beatmapContents); @@ -321,15 +361,13 @@ public async Task UploadGuestDifficultyAsync( return NoContent(); } - private static async Task checkUserAccountStanding(MySqlConnection connection, uint userId, MySqlTransaction? transaction = null) + private static async Task checkUserAccountStanding(MySqlConnection connection, uint userId, MySqlTransaction? transaction = null) { if (await connection.IsUserRestrictedAsync(userId, transaction)) - return new ErrorResponse("Your account is currently restricted."); + throw new InvariantException("Your account is currently restricted."); if (await connection.IsUserSilencedAsync(userId, transaction)) - return new ErrorResponse("You are unable to submit or update maps while silenced."); - - return null; + throw new InvariantException("You are unable to submit or update maps while silenced."); } private static async Task<(uint totalSlots, uint remainingSlots)> getUploadQuota(MySqlConnection connection, uint userId, MySqlTransaction? transaction = null) @@ -394,14 +432,14 @@ private async Task updateBeatmapSetFromArchiveAsync(uint beatmapSetId, Str if (packageFile.BeatmapContent is BeatmapContent content) { if (!beatmapIds.Remove((uint)content.Beatmap.BeatmapInfo.OnlineID)) - throw new InvariantException($"Beatmap has invalid ID inside ({packageFile.VersionFile.filename})."); + throw new InvariantException($"Beatmap has invalid ID inside ({packageFile.VersionFile.filename}).", LogLevel.Warning); await db.UpdateBeatmapAsync(content.GetDatabaseRow(), transaction); } } if (beatmapIds.Count > 0) - throw new InvariantException($"Beatmap package is missing .osu files for beatmaps with IDs: {string.Join(", ", beatmapIds)}"); + throw new InvariantException($"Beatmap package is missing .osu files for beatmaps with IDs: {string.Join(", ", beatmapIds)}", LogLevel.Warning); await db.UpdateBeatmapSetAsync(parseResult.BeatmapSet, transaction); diff --git a/osu.Server.BeatmapSubmission/DatabaseOperationExtensions.cs b/osu.Server.BeatmapSubmission/DatabaseOperationExtensions.cs index d9c0446..9d03280 100644 --- a/osu.Server.BeatmapSubmission/DatabaseOperationExtensions.cs +++ b/osu.Server.BeatmapSubmission/DatabaseOperationExtensions.cs @@ -65,7 +65,7 @@ public static async Task GetUserMonthlyPlaycountAsync(this MySqlConnectio transaction) ?? 0; } - public static async Task PurgeInactiveBeatmapSetsForUserAsync(this MySqlConnection db, uint userId, MySqlTransaction? transaction = null) + public static async Task PurgeInactiveBeatmapSetsForUserAsync(this MySqlConnection db, uint userId, MySqlTransaction? transaction = null) { uint[] beatmapSetIds = (await db.QueryAsync(@"SELECT `beatmapset_id` FROM `osu_beatmapsets` WHERE `user_id` = @user_id AND `active` = -1 AND `deleted_at` IS NULL", new @@ -86,6 +86,8 @@ await db.ExecuteAsync(@"DELETE FROM `osu_beatmapsets` WHERE `user_id` = @user_id user_id = userId, }, transaction); + + return beatmapSetIds; } public static Task<(uint unranked, uint ranked)> GetUserBeatmapSetCountAsync(this MySqlConnection db, uint userId, MySqlTransaction? transaction = null) diff --git a/osu.Server.BeatmapSubmission/InvariantExceptionFilter.cs b/osu.Server.BeatmapSubmission/InvariantExceptionFilter.cs index ad536ed..b60abe7 100644 --- a/osu.Server.BeatmapSubmission/InvariantExceptionFilter.cs +++ b/osu.Server.BeatmapSubmission/InvariantExceptionFilter.cs @@ -8,10 +8,20 @@ namespace osu.Server.BeatmapSubmission { public class InvariantExceptionFilter : IExceptionFilter { + private readonly ILogger logger; + + public InvariantExceptionFilter(ILogger logger) + { + this.logger = logger; + } + public void OnException(ExceptionContext context) { if (context.Exception is InvariantException invariantException) + { context.Result = invariantException.ToResponseObject().ToActionResult(); + logger.Log(invariantException.Severity, context.Exception, "Request rejected due to violated invariant"); + } } } } diff --git a/osu.Server.BeatmapSubmission/Models/InvariantException.cs b/osu.Server.BeatmapSubmission/Models/InvariantException.cs index 35a2be1..b191988 100644 --- a/osu.Server.BeatmapSubmission/Models/InvariantException.cs +++ b/osu.Server.BeatmapSubmission/Models/InvariantException.cs @@ -11,9 +11,12 @@ namespace osu.Server.BeatmapSubmission.Models /// public class InvariantException : Exception { - public InvariantException(string message) + public LogLevel Severity { get; } + + public InvariantException(string message, LogLevel severity = LogLevel.Information) : base(message) { + Severity = severity; } public ErrorResponse ToResponseObject() => new ErrorResponse(Message); diff --git a/osu.Server.BeatmapSubmission/Services/BeatmapPackagePatcher.cs b/osu.Server.BeatmapSubmission/Services/BeatmapPackagePatcher.cs index e69c197..5872c68 100644 --- a/osu.Server.BeatmapSubmission/Services/BeatmapPackagePatcher.cs +++ b/osu.Server.BeatmapSubmission/Services/BeatmapPackagePatcher.cs @@ -83,7 +83,7 @@ public async Task PatchBeatmapAsync( await beatmapStorage.ExtractBeatmapSetAsync(beatmapSetId, tempDirectory.FullName); if (beatmap.filename == null) - throw new InvalidOperationException("Could not find the old .osu file for the beatmap being modified."); + throw new InvariantException("Could not find the old .osu file for the beatmap being modified.", LogLevel.Warning); File.Delete(Path.Combine(tempDirectory.FullName, beatmap.filename)); diff --git a/osu.Server.BeatmapSubmission/Services/LegacyIO.cs b/osu.Server.BeatmapSubmission/Services/LegacyIO.cs index 430b7d5..81d2c51 100644 --- a/osu.Server.BeatmapSubmission/Services/LegacyIO.cs +++ b/osu.Server.BeatmapSubmission/Services/LegacyIO.cs @@ -12,10 +12,12 @@ namespace osu.Server.BeatmapSubmission.Services public class LegacyIO : ILegacyIO { private readonly HttpClient httpClient; + private readonly ILogger logger; - public LegacyIO(HttpClient httpClient) + public LegacyIO(HttpClient httpClient, ILogger logger) { this.httpClient = httpClient; + this.logger = logger; } private async Task runLegacyIO(HttpMethod method, string command, dynamic? postObject = null) @@ -27,6 +29,9 @@ private async Task runLegacyIO(HttpMethod method, string command, dynamic? postO long time = DateTimeOffset.UtcNow.ToUnixTimeSeconds(); string url = $"{AppSettings.LegacyIODomain}/_lio/{command}{(command.Contains('?') ? "&" : "?")}timestamp={time}"; + string? serialisedPostObject = postObject == null ? null : JsonSerializer.Serialize(postObject); + logger.LogDebug("Performing LIO request to {method} {url} (params: {params})", method, url, serialisedPostObject); + try { string signature = hmacEncode(url, Encoding.UTF8.GetBytes(AppSettings.SharedInteropSecret)); @@ -44,7 +49,7 @@ private async Task runLegacyIO(HttpMethod method, string command, dynamic? postO if (postObject != null) { - httpRequestMessage.Content = new ByteArrayContent(Encoding.UTF8.GetBytes(JsonSerializer.Serialize(postObject))); + httpRequestMessage.Content = new ByteArrayContent(Encoding.UTF8.GetBytes(serialisedPostObject)); httpRequestMessage.Content.Headers.ContentType = MediaTypeHeaderValue.Parse("application/json"); } @@ -60,7 +65,7 @@ private async Task runLegacyIO(HttpMethod method, string command, dynamic? postO { if (retryCount-- > 0) { - Console.WriteLine($"Legacy IO request to {url} failed with {e}, retrying.."); + logger.LogError(e, "Legacy IO request to {url} failed, retrying ({retries} remaining)", url, retryCount); Thread.Sleep(1000); goto retry; } diff --git a/osu.Server.BeatmapSubmission/Services/MirrorService.cs b/osu.Server.BeatmapSubmission/Services/MirrorService.cs index 445d51f..2641c6a 100644 --- a/osu.Server.BeatmapSubmission/Services/MirrorService.cs +++ b/osu.Server.BeatmapSubmission/Services/MirrorService.cs @@ -10,10 +10,12 @@ namespace osu.Server.BeatmapSubmission.Services public class MirrorService : IMirrorService { private readonly HttpClient client; + private readonly ILogger logger; - public MirrorService(HttpClient client) + public MirrorService(HttpClient client, ILogger logger) { this.client = client; + this.logger = logger; } public async Task PurgeBeatmapSetAsync(MySqlConnection db, uint beatmapSetId) @@ -29,6 +31,8 @@ public async Task PurgeBeatmapSetAsync(MySqlConnection db, uint beatmapSetId) private async Task performMirrorAction(osu_mirror mirror, string action, Dictionary data) { + logger.LogInformation("Performing action {action} (data: {data}) on mirror {mirror}", action, data, mirror.base_url); + data["ts"] = DateTimeOffset.Now.ToUnixTimeSeconds().ToString(); data["action"] = action; data["cs"] = ($"{data.GetValueOrDefault("s")}{data.GetValueOrDefault("fd")}{data.GetValueOrDefault("fs")}{data.GetValueOrDefault("ts")}" @@ -42,9 +46,9 @@ public async Task PurgeBeatmapSetAsync(MySqlConnection db, uint beatmapSetId) var response = await client.SendAsync(request); return await response.Content.ReadAsStringAsync(); } - catch (Exception) + catch (Exception e) { - // TODO: log error + logger.LogError(e, "Attempting to perform action {action} on mirror {mirror} failed", action, mirror.base_url); return null; } } diff --git a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs index e825dee..a56176d 100644 --- a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs +++ b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs @@ -21,10 +21,12 @@ public class S3BeatmapStorage : IBeatmapStorage private const string osu_directory = "beatmaps"; private const string versioned_file_directory = "beatmap_files"; + private readonly ILogger logger; private readonly AmazonS3Client client; - public S3BeatmapStorage() + public S3BeatmapStorage(ILogger logger) { + this.logger = logger; client = new AmazonS3Client( new BasicAWSCredentials(AppSettings.S3AccessKey, AppSettings.S3SecretKey), new AmazonS3Config @@ -61,13 +63,15 @@ public async Task StoreBeatmapSetAsync(uint beatmapSetId, byte[] beatmapPackage, await Task.WhenAll([ uploadBeatmapPackage(beatmapSetId, beatmapPackage, stream), - ..uploadAllVersionedFiles(allFiles), - ..uploadAllBeatmapFiles(beatmapFiles) + ..uploadAllVersionedFiles(beatmapSetId, allFiles), + ..uploadAllBeatmapFiles(beatmapSetId, beatmapFiles) ]); + logger.LogInformation("All file uploads for beatmapset {beatmapSetId} concluded successfully.", beatmapSetId); } private Task uploadBeatmapPackage(uint beatmapSetId, byte[] beatmapPackage, MemoryStream stream) { + logger.LogInformation("Beginning upload of package for beatmapset {beatmapSetId}...", beatmapSetId); return client.PutObjectAsync(new PutObjectRequest { BucketName = AppSettings.S3BucketName, @@ -82,8 +86,9 @@ private Task uploadBeatmapPackage(uint beatmapSetId, byte[] b }); } - private IEnumerable uploadAllVersionedFiles(List files) + private IEnumerable uploadAllVersionedFiles(uint beatmapSetId, List files) { + logger.LogInformation("Beginning upload of all versioned files for beatmapset {beatmapSetId}...", beatmapSetId); return files.Select(file => Task.Run(async () => { var fileStream = new MemoryStream(file); @@ -104,8 +109,9 @@ await client.PutObjectAsync(new PutObjectRequest })); } - private IEnumerable uploadAllBeatmapFiles(List<(int beatmapId, byte[] contents)> beatmapFiles) + private IEnumerable uploadAllBeatmapFiles(uint beatmapSetId, List<(int beatmapId, byte[] contents)> beatmapFiles) { + logger.LogInformation("Beginning upload of all .osu beatmap files for beatmapset {beatmapSetId}...", beatmapSetId); return beatmapFiles.Select(file => Task.Run(async () => { var fileStream = new MemoryStream(file.contents); @@ -128,7 +134,10 @@ await client.PutObjectAsync(new PutObjectRequest public async Task ExtractBeatmapSetAsync(uint beatmapSetId, string targetDirectory) { + logger.LogInformation("Retrieving package for beatmap set {beatmapSetId}", beatmapSetId); using var response = await client.GetObjectAsync(AppSettings.S3BucketName, getPathToPackage(beatmapSetId)); + logger.LogInformation("Package for beatmap set {beatmapSetId} retrieved successfully.", beatmapSetId); + // S3-provided `HashStream` does not support seeking which `ZipArchiveReader` does not like. var memoryStream = new MemoryStream(await response.ResponseStream.ReadAllRemainingBytesToArrayAsync()); @@ -152,6 +161,8 @@ public async Task PackageBeatmapSetFilesAsync(IEnumerable f { var memoryStream = new MemoryStream(); + logger.LogInformation("Retrieving requested files..."); + using (var zipWriter = new ZipWriter(memoryStream, BeatmapPackagePatcher.DEFAULT_ZIP_WRITER_OPTIONS)) { foreach (var file in files) @@ -161,6 +172,8 @@ public async Task PackageBeatmapSetFilesAsync(IEnumerable f } } + logger.LogInformation("Package files retrieved successfully."); + memoryStream.Seek(0, SeekOrigin.Begin); return memoryStream; } From a3d953d472cd9d233b8c360fdb1ed8985b560a0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 25 Dec 2024 10:59:58 +0100 Subject: [PATCH 5/8] Ensure user ID is logged in sentry events --- .../Logging/UserFactory.cs | 32 +++++++++++++++++++ osu.Server.BeatmapSubmission/Program.cs | 11 ++++++- 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 osu.Server.BeatmapSubmission/Logging/UserFactory.cs diff --git a/osu.Server.BeatmapSubmission/Logging/UserFactory.cs b/osu.Server.BeatmapSubmission/Logging/UserFactory.cs new file mode 100644 index 0000000..4d9e65b --- /dev/null +++ b/osu.Server.BeatmapSubmission/Logging/UserFactory.cs @@ -0,0 +1,32 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Globalization; +using System.Security.Claims; +using osu.Server.BeatmapSubmission.Authentication; + +namespace osu.Server.BeatmapSubmission.Logging +{ + public class UserFactory : ISentryUserFactory + { + private readonly IHttpContextAccessor httpContextAccessor; + + public UserFactory(IHttpContextAccessor httpContextAccessor) + { + this.httpContextAccessor = httpContextAccessor; + } + + public SentryUser? Create() + { + var user = httpContextAccessor.HttpContext?.User; + + if (user == null || !user.HasClaim(claim => claim.Type == ClaimTypes.NameIdentifier)) + return null; + + return new SentryUser + { + Id = user.GetUserId().ToString(CultureInfo.InvariantCulture), + }; + } + } +} diff --git a/osu.Server.BeatmapSubmission/Program.cs b/osu.Server.BeatmapSubmission/Program.cs index a8ed0c2..20b397f 100644 --- a/osu.Server.BeatmapSubmission/Program.cs +++ b/osu.Server.BeatmapSubmission/Program.cs @@ -5,6 +5,7 @@ using Microsoft.Extensions.Options; using osu.Server.BeatmapSubmission.Authentication; using osu.Server.BeatmapSubmission.Configuration; +using osu.Server.BeatmapSubmission.Logging; using osu.Server.BeatmapSubmission.Services; using StatsdClient; @@ -81,7 +82,15 @@ public static void Main(string[] args) } if (AppSettings.SentryDsn != null) - builder.WebHost.UseSentry(options => options.Dsn = AppSettings.SentryDsn); + { + builder.Services.AddSingleton(); + builder.WebHost.UseSentry(options => + { + options.Environment = builder.Environment.EnvironmentName; + options.SendDefaultPii = true; + options.Dsn = AppSettings.SentryDsn; + }); + } if (AppSettings.DatadogAgentHost != null) { From da68ffea143d0727f824ddfc3a5a734074e2d398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 25 Dec 2024 12:22:45 +0100 Subject: [PATCH 6/8] Fix nullability inspection --- osu.Server.BeatmapSubmission/Services/LegacyIO.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Server.BeatmapSubmission/Services/LegacyIO.cs b/osu.Server.BeatmapSubmission/Services/LegacyIO.cs index 81d2c51..2c1295c 100644 --- a/osu.Server.BeatmapSubmission/Services/LegacyIO.cs +++ b/osu.Server.BeatmapSubmission/Services/LegacyIO.cs @@ -47,7 +47,7 @@ private async Task runLegacyIO(HttpMethod method, string command, dynamic? postO }, }; - if (postObject != null) + if (serialisedPostObject != null) { httpRequestMessage.Content = new ByteArrayContent(Encoding.UTF8.GetBytes(serialisedPostObject)); httpRequestMessage.Content.Headers.ContentType = MediaTypeHeaderValue.Parse("application/json"); From 684247d56629ba8c17b6bd9da330f36f16e45590 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Dec 2024 09:53:23 +0100 Subject: [PATCH 7/8] Replace usage of spread syntax --- .../Services/S3BeatmapStorage.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs index f78441b..e2a8bc1 100644 --- a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs +++ b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs @@ -60,11 +60,11 @@ public async Task StoreBeatmapSetAsync(uint beatmapSetId, byte[] beatmapPackage, beatmapFiles.Add((beatmapId, contents)); } - await Task.WhenAll([ - uploadBeatmapPackage(beatmapSetId, beatmapPackage, stream), - ..uploadAllVersionedFiles(beatmapSetId, allFiles), - ..uploadAllBeatmapFiles(beatmapSetId, beatmapFiles) - ]); + var tasks = new List { uploadBeatmapPackage(beatmapSetId, beatmapPackage, stream) }; + tasks.AddRange(uploadAllVersionedFiles(beatmapSetId, allFiles)); + tasks.AddRange(uploadAllBeatmapFiles(beatmapSetId, beatmapFiles)); + await Task.WhenAll(tasks); + logger.LogInformation("All file uploads for beatmapset {beatmapSetId} concluded successfully.", beatmapSetId); } From d26d805f818a3870c1f1e063dee9dbcd8476de2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 31 Dec 2024 09:53:37 +0100 Subject: [PATCH 8/8] Break up long line --- osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs index e2a8bc1..ad3ecbc 100644 --- a/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs +++ b/osu.Server.BeatmapSubmission/Services/S3BeatmapStorage.cs @@ -165,7 +165,8 @@ public async Task PackageBeatmapSetFilesAsync(IEnumerable f { foreach (var file in files) { - using var response = await client.GetObjectAsync(AppSettings.S3CentralBucketName, getPathToVersionedFile(BitConverter.ToString(file.File.sha2_hash).Replace("-", string.Empty).ToLowerInvariant())); + using var response = await client.GetObjectAsync(AppSettings.S3CentralBucketName, + getPathToVersionedFile(BitConverter.ToString(file.File.sha2_hash).Replace("-", string.Empty).ToLowerInvariant())); zipWriter.Write(file.VersionFile.filename, response.ResponseStream); } }