Merge pull request #27143 from bdach/more-stringent-metadata-checks

Use more stringent checks when applying online metadata

osu.Game.Tests/Beatmaps/BeatmapUpdaterMetadataLookupTest.cs

@@ -28,7 +28,12 @@ namespace osu.Game.Tests.Beatmaps
         [Test]
         public void TestLocalCacheQueriedFirst()
         {
-            var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked };
+            var localLookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 123456,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+            };
             localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true);
             localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out localLookupResult))
                                          .Returns(true);
@@ -42,6 +47,7 @@ namespace osu.Game.Tests.Beatmaps
             metadataLookup.Update(beatmapSet, preferOnlineFetch: false);
 
             Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
             localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata>.IsAny!), Times.Once);
             apiMetadataSourceMock.Verify(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out It.Ref<OnlineBeatmapMetadata>.IsAny!), Times.Never);
         }
@@ -54,7 +60,12 @@ namespace osu.Game.Tests.Beatmaps
             localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out localLookupResult))
                                          .Returns(false);
 
-            var onlineLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked };
+            var onlineLookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 123456,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+            };
             apiMetadataSourceMock.Setup(src => src.Available).Returns(true);
             apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out onlineLookupResult))
                                  .Returns(true);
@@ -66,6 +77,7 @@ namespace osu.Game.Tests.Beatmaps
             metadataLookup.Update(beatmapSet, preferOnlineFetch: false);
 
             Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
             localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Once);
             apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Once);
         }
@@ -73,12 +85,22 @@ namespace osu.Game.Tests.Beatmaps
         [Test]
         public void TestPreferOnlineFetch()
         {
-            var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked };
+            var localLookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 123456,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+            };
             localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true);
             localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out localLookupResult))
                                          .Returns(true);
 
-            var onlineLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Graveyard };
+            var onlineLookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 123456,
+                BeatmapStatus = BeatmapOnlineStatus.Graveyard,
+                BeatmapSetStatus = BeatmapOnlineStatus.Graveyard,
+            };
             apiMetadataSourceMock.Setup(src => src.Available).Returns(true);
             apiMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out onlineLookupResult))
                                  .Returns(true);
@@ -90,6 +112,7 @@ namespace osu.Game.Tests.Beatmaps
             metadataLookup.Update(beatmapSet, preferOnlineFetch: true);
 
             Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Graveyard));
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Graveyard));
             localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Never);
             apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Once);
         }
@@ -97,7 +120,12 @@ namespace osu.Game.Tests.Beatmaps
         [Test]
         public void TestPreferOnlineFetchFallsBackToLocalCacheIfOnlineSourceUnavailable()
         {
-            var localLookupResult = new OnlineBeatmapMetadata { BeatmapID = 123456, BeatmapStatus = BeatmapOnlineStatus.Ranked };
+            var localLookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 123456,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+            };
             localCachedMetadataSourceMock.Setup(src => src.Available).Returns(true);
             localCachedMetadataSourceMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out localLookupResult))
                                          .Returns(true);
@@ -111,6 +139,7 @@ namespace osu.Game.Tests.Beatmaps
             metadataLookup.Update(beatmapSet, preferOnlineFetch: true);
 
             Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
             localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Once);
             apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Never);
         }
@@ -135,6 +164,7 @@ namespace osu.Game.Tests.Beatmaps
             metadataLookup.Update(beatmapSet, preferOnlineFetch: false);
 
             Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
             Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
             localCachedMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Once);
             apiMetadataSourceMock.Verify(src => src.TryLookup(beatmap, out It.Ref<OnlineBeatmapMetadata?>.IsAny!), Times.Once);
@@ -163,6 +193,7 @@ namespace osu.Game.Tests.Beatmaps
             metadataLookup.Update(beatmapSet, preferOnlineFetch);
 
             Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
             Assert.That(beatmap.OnlineID, Is.EqualTo(123456));
         }
 
@@ -193,5 +224,217 @@ namespace osu.Game.Tests.Beatmaps
 
             Assert.That(beatmap.OnlineID, Is.EqualTo(123456));
         }
+
+        [Test]
+        public void TestReturnedMetadataHasDifferentOnlineID([Values] bool preferOnlineFetch)
+        {
+            var lookupResult = new OnlineBeatmapMetadata { BeatmapID = 654321, BeatmapStatus = BeatmapOnlineStatus.Ranked };
+
+            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
+            targetMock.Setup(src => src.Available).Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out lookupResult))
+                      .Returns(true);
+
+            var beatmap = new BeatmapInfo { OnlineID = 123456 };
+            var beatmapSet = new BeatmapSetInfo(beatmap.Yield());
+            beatmap.BeatmapSet = beatmapSet;
+
+            metadataLookup.Update(beatmapSet, preferOnlineFetch);
+
+            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
+        }
+
+        [Test]
+        public void TestMetadataLookupForBeatmapWithoutPopulatedIDAndCorrectHash([Values] bool preferOnlineFetch)
+        {
+            var lookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 654321,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"deadbeef",
+            };
+
+            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
+            targetMock.Setup(src => src.Available).Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out lookupResult))
+                      .Returns(true);
+
+            var beatmap = new BeatmapInfo
+            {
+                MD5Hash = @"deadbeef"
+            };
+            var beatmapSet = new BeatmapSetInfo(beatmap.Yield());
+            beatmap.BeatmapSet = beatmapSet;
+
+            metadataLookup.Update(beatmapSet, preferOnlineFetch);
+
+            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
+            Assert.That(beatmap.OnlineID, Is.EqualTo(654321));
+        }
+
+        [Test]
+        public void TestMetadataLookupForBeatmapWithoutPopulatedIDAndIncorrectHash([Values] bool preferOnlineFetch)
+        {
+            var lookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 654321,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"cafebabe",
+            };
+
+            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
+            targetMock.Setup(src => src.Available).Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out lookupResult))
+                      .Returns(true);
+
+            var beatmap = new BeatmapInfo
+            {
+                MD5Hash = @"deadbeef"
+            };
+            var beatmapSet = new BeatmapSetInfo(beatmap.Yield());
+            beatmap.BeatmapSet = beatmapSet;
+
+            metadataLookup.Update(beatmapSet, preferOnlineFetch);
+
+            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(beatmap.OnlineID, Is.EqualTo(-1));
+        }
+
+        [Test]
+        public void TestReturnedMetadataHasDifferentHash([Values] bool preferOnlineFetch)
+        {
+            var lookupResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 654321,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"deadbeef"
+            };
+
+            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
+            targetMock.Setup(src => src.Available).Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.IsAny<BeatmapInfo>(), out lookupResult))
+                      .Returns(true);
+
+            var beatmap = new BeatmapInfo
+            {
+                OnlineID = 654321,
+                MD5Hash = @"cafebabe",
+            };
+            var beatmapSet = new BeatmapSetInfo(beatmap.Yield());
+            beatmap.BeatmapSet = beatmapSet;
+
+            metadataLookup.Update(beatmapSet, preferOnlineFetch);
+
+            Assert.That(beatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(beatmap.OnlineID, Is.EqualTo(654321));
+        }
+
+        [Test]
+        public void TestPartiallyModifiedSet([Values] bool preferOnlineFetch)
+        {
+            var firstResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 654321,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"cafebabe"
+            };
+            var secondResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 666666,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"dededede"
+            };
+
+            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
+            targetMock.Setup(src => src.Available).Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 654321), out firstResult))
+                      .Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 666666), out secondResult))
+                      .Returns(true);
+
+            var firstBeatmap = new BeatmapInfo
+            {
+                OnlineID = 654321,
+                MD5Hash = @"cafebabe",
+            };
+            var secondBeatmap = new BeatmapInfo
+            {
+                OnlineID = 666666,
+                MD5Hash = @"deadbeef"
+            };
+            var beatmapSet = new BeatmapSetInfo(new[]
+            {
+                firstBeatmap,
+                secondBeatmap
+            });
+            firstBeatmap.BeatmapSet = beatmapSet;
+            secondBeatmap.BeatmapSet = beatmapSet;
+
+            metadataLookup.Update(beatmapSet, preferOnlineFetch);
+
+            Assert.That(firstBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
+            Assert.That(firstBeatmap.OnlineID, Is.EqualTo(654321));
+
+            Assert.That(secondBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(secondBeatmap.OnlineID, Is.EqualTo(666666));
+
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+        }
+
+        [Test]
+        public void TestPartiallyMaliciousSet([Values] bool preferOnlineFetch)
+        {
+            var firstResult = new OnlineBeatmapMetadata
+            {
+                BeatmapID = 654321,
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"cafebabe"
+            };
+            var secondResult = new OnlineBeatmapMetadata
+            {
+                BeatmapStatus = BeatmapOnlineStatus.Ranked,
+                BeatmapSetStatus = BeatmapOnlineStatus.Ranked,
+                MD5Hash = @"dededede"
+            };
+
+            var targetMock = preferOnlineFetch ? apiMetadataSourceMock : localCachedMetadataSourceMock;
+            targetMock.Setup(src => src.Available).Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 654321), out firstResult))
+                      .Returns(true);
+            targetMock.Setup(src => src.TryLookup(It.Is<BeatmapInfo>(bi => bi.OnlineID == 666666), out secondResult))
+                      .Returns(true);
+
+            var firstBeatmap = new BeatmapInfo
+            {
+                OnlineID = 654321,
+                MD5Hash = @"cafebabe",
+            };
+            var secondBeatmap = new BeatmapInfo
+            {
+                OnlineID = 666666,
+                MD5Hash = @"deadbeef"
+            };
+            var beatmapSet = new BeatmapSetInfo(new[]
+            {
+                firstBeatmap,
+                secondBeatmap
+            });
+            firstBeatmap.BeatmapSet = beatmapSet;
+            secondBeatmap.BeatmapSet = beatmapSet;
+
+            metadataLookup.Update(beatmapSet, preferOnlineFetch);
+
+            Assert.That(firstBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.Ranked));
+            Assert.That(firstBeatmap.OnlineID, Is.EqualTo(654321));
+
+            Assert.That(secondBeatmap.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+            Assert.That(secondBeatmap.OnlineID, Is.EqualTo(-1));
+
+            Assert.That(beatmapSet.Status, Is.EqualTo(BeatmapOnlineStatus.None));
+        }
     }
 }

osu.Game/Beatmaps/BeatmapUpdaterMetadataLookup.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using osu.Framework.Platform;
@@ -38,17 +39,22 @@ namespace osu.Game.Beatmaps
         /// <param name="preferOnlineFetch">Whether metadata from an online source should be preferred. If <c>true</c>, the local cache will be skipped to ensure the freshest data state possible.</param>
         public void Update(BeatmapSetInfo beatmapSet, bool preferOnlineFetch)
         {
+            var lookupResults = new List<OnlineBeatmapMetadata?>();
+
             foreach (var beatmapInfo in beatmapSet.Beatmaps)
             {
                 if (!tryLookup(beatmapInfo, preferOnlineFetch, out var res))
                     continue;
 
-                if (res == null)
+                if (res == null || shouldDiscardLookupResult(res, beatmapInfo))
                 {
                     beatmapInfo.ResetOnlineInfo();
+                    lookupResults.Add(null); // mark lookup failure
                     continue;
                 }
 
+                lookupResults.Add(res);
+
                 beatmapInfo.OnlineID = res.BeatmapID;
                 beatmapInfo.OnlineMD5Hash = res.MD5Hash;
                 beatmapInfo.LastOnlineUpdate = res.LastUpdated;
@@ -57,19 +63,34 @@ namespace osu.Game.Beatmaps
                 beatmapInfo.BeatmapSet.OnlineID = res.BeatmapSetID;
 
                 // Some metadata should only be applied if there's no local changes.
-                if (shouldSaveOnlineMetadata(beatmapInfo))
+                if (beatmapInfo.MatchesOnlineVersion)
                 {
                     beatmapInfo.Status = res.BeatmapStatus;
                     beatmapInfo.Metadata.Author.OnlineID = res.AuthorID;
                 }
-
-                if (beatmapInfo.BeatmapSet.Beatmaps.All(shouldSaveOnlineMetadata))
-                {
-                    beatmapInfo.BeatmapSet.Status = res.BeatmapSetStatus ?? BeatmapOnlineStatus.None;
-                    beatmapInfo.BeatmapSet.DateRanked = res.DateRanked;
-                    beatmapInfo.BeatmapSet.DateSubmitted = res.DateSubmitted;
-                }
             }
+
+            if (beatmapSet.Beatmaps.All(b => b.MatchesOnlineVersion)
+                && lookupResults.All(r => r != null)
+                && lookupResults.Select(r => r!.BeatmapSetID).Distinct().Count() == 1)
+            {
+                var representative = lookupResults.First()!;
+
+                beatmapSet.Status = representative.BeatmapSetStatus ?? BeatmapOnlineStatus.None;
+                beatmapSet.DateRanked = representative.DateRanked;
+                beatmapSet.DateSubmitted = representative.DateSubmitted;
+            }
+        }
+
+        private bool shouldDiscardLookupResult(OnlineBeatmapMetadata result, BeatmapInfo beatmapInfo)
+        {
+            if (beatmapInfo.OnlineID > 0 && result.BeatmapID != beatmapInfo.OnlineID)
+                return true;
+
+            if (beatmapInfo.OnlineID == -1 && result.MD5Hash != beatmapInfo.MD5Hash)
+                return true;
+
+            return false;
         }
 
         /// <summary>
@@ -104,12 +125,6 @@ namespace osu.Game.Beatmaps
             return false;
         }
 
-        /// <summary>
-        /// Check whether the provided beatmap is in a state where online "ranked" status metadata should be saved against it.
-        /// Handles the case where a user may have locally modified a beatmap in the editor and expects the local status to stick.
-        /// </summary>
-        private static bool shouldSaveOnlineMetadata(BeatmapInfo beatmapInfo) => beatmapInfo.MatchesOnlineVersion || beatmapInfo.Status != BeatmapOnlineStatus.LocallyModified;
-
         public void Dispose()
         {
             apiMetadataSource.Dispose();