From 87a331fdde783dbfc3b5f395339d2d09cba1105e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 30 May 2024 16:34:52 +0900 Subject: [PATCH 01/10] Improve text on external link warning dialog --- osu.Game/Online/Chat/ExternalLinkOpener.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/osu.Game/Online/Chat/ExternalLinkOpener.cs b/osu.Game/Online/Chat/ExternalLinkOpener.cs index 56d24e35bb..08adf965da 100644 --- a/osu.Game/Online/Chat/ExternalLinkOpener.cs +++ b/osu.Game/Online/Chat/ExternalLinkOpener.cs @@ -10,6 +10,7 @@ using osu.Framework.Platform; using osu.Game.Configuration; using osu.Game.Overlays; using osu.Game.Overlays.Dialog; +using osu.Game.Resources.Localisation.Web; namespace osu.Game.Online.Chat { @@ -44,8 +45,8 @@ namespace osu.Game.Online.Chat { public ExternalLinkDialog(string url, Action openExternalLinkAction, Action copyExternalLinkAction) { - HeaderText = "Just checking..."; - BodyText = $"You are about to leave osu! and open the following link in a web browser:\n\n{url}"; + HeaderText = "You are leaving osu!"; + BodyText = $"Are you sure you want to open the following link in a web browser:\n\n{url}"; Icon = FontAwesome.Solid.ExclamationTriangle; @@ -53,17 +54,17 @@ namespace osu.Game.Online.Chat { new PopupDialogOkButton { - Text = @"Yes. Go for it.", + Text = @"Open in browser", Action = openExternalLinkAction }, new PopupDialogCancelButton { - Text = @"Copy URL to the clipboard instead.", + Text = @"Copy URL to the clipboard", Action = copyExternalLinkAction }, new PopupDialogCancelButton { - Text = @"No! Abort mission!" + Text = CommonStrings.ButtonsCancel, }, }; } From ed64bfff8d9c13c54b27b955a706e28d3cefb134 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 30 May 2024 16:39:53 +0900 Subject: [PATCH 02/10] Bypass external link dialog for links on the trusted osu! domain --- osu.Game/OsuGame.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index af01a1b1ac..29c040c597 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -485,10 +485,19 @@ namespace osu.Game } }); - public void OpenUrlExternally(string url, bool bypassExternalUrlWarning = false) => waitForReady(() => externalLinkOpener, _ => + public void OpenUrlExternally(string url, bool forceBypassExternalUrlWarning = false) => waitForReady(() => externalLinkOpener, _ => { + bool isTrustedDomain; + if (url.StartsWith('/')) - url = $"{API.APIEndpointUrl}{url}"; + { + url = $"{API.WebsiteRootUrl}{url}"; + isTrustedDomain = true; + } + else + { + isTrustedDomain = url.StartsWith(API.APIEndpointUrl, StringComparison.Ordinal); + } if (!url.CheckIsValidUrl()) { @@ -500,7 +509,7 @@ namespace osu.Game return; } - externalLinkOpener.OpenUrlExternally(url, bypassExternalUrlWarning); + externalLinkOpener.OpenUrlExternally(url, forceBypassExternalUrlWarning || isTrustedDomain); }); /// From 53b7c29488f6dd28ae30877e0df148d4c6be6d33 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 30 May 2024 17:35:42 +0900 Subject: [PATCH 03/10] Add test coverage of menu banner link opening --- .../Visual/Menus/TestSceneMainMenu.cs | 49 ++++++++++++++++++- osu.Game/Screens/Menu/OnlineMenuBanner.cs | 5 ++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Menus/TestSceneMainMenu.cs b/osu.Game.Tests/Visual/Menus/TestSceneMainMenu.cs index e2a841d79a..240421b360 100644 --- a/osu.Game.Tests/Visual/Menus/TestSceneMainMenu.cs +++ b/osu.Game.Tests/Visual/Menus/TestSceneMainMenu.cs @@ -6,6 +6,7 @@ using NUnit.Framework; using osu.Framework.Graphics.Containers; using osu.Framework.Testing; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Overlays; using osu.Game.Screens.Menu; using osuTK.Input; @@ -15,8 +16,14 @@ namespace osu.Game.Tests.Visual.Menus { private OnlineMenuBanner onlineMenuBanner => Game.ChildrenOfType().Single(); + public override void SetUpSteps() + { + base.SetUpSteps(); + AddStep("don't fetch online content", () => onlineMenuBanner.FetchOnlineContent = false); + } + [Test] - public void TestOnlineMenuBanner() + public void TestOnlineMenuBannerTrusted() { AddStep("set online content", () => onlineMenuBanner.Current.Value = new APIMenuContent { @@ -25,13 +32,51 @@ namespace osu.Game.Tests.Visual.Menus new APIMenuImage { Image = @"https://assets.ppy.sh/main-menu/project-loved-2@2x.png", - Url = @"https://osu.ppy.sh/home/news/2023-12-21-project-loved-december-2023", + Url = $@"{API.WebsiteRootUrl}/home/news/2023-12-21-project-loved-december-2023", } } }); AddAssert("system title not visible", () => onlineMenuBanner.State.Value, () => Is.EqualTo(Visibility.Hidden)); AddStep("enter menu", () => InputManager.Key(Key.Enter)); AddUntilStep("system title visible", () => onlineMenuBanner.State.Value, () => Is.EqualTo(Visibility.Visible)); + AddUntilStep("image loaded", () => onlineMenuBanner.ChildrenOfType().FirstOrDefault()?.IsLoaded, () => Is.True); + + AddStep("click banner", () => + { + InputManager.MoveMouseTo(onlineMenuBanner); + InputManager.Click(MouseButton.Left); + }); + + // Might not catch every occurrence due to async nature, but works in manual testing and saves annoying test setup. + AddAssert("no dialog", () => Game.ChildrenOfType().SingleOrDefault()?.CurrentDialog == null); + } + + [Test] + public void TestOnlineMenuBannerUntrustedDomain() + { + AddStep("set online content", () => onlineMenuBanner.Current.Value = new APIMenuContent + { + Images = new[] + { + new APIMenuImage + { + Image = @"https://assets.ppy.sh/main-menu/project-loved-2@2x.png", + Url = @"https://google.com", + } + } + }); + AddAssert("system title not visible", () => onlineMenuBanner.State.Value, () => Is.EqualTo(Visibility.Hidden)); + AddStep("enter menu", () => InputManager.Key(Key.Enter)); + AddUntilStep("system title visible", () => onlineMenuBanner.State.Value, () => Is.EqualTo(Visibility.Visible)); + AddUntilStep("image loaded", () => onlineMenuBanner.ChildrenOfType().FirstOrDefault()?.IsLoaded, () => Is.True); + + AddStep("click banner", () => + { + InputManager.MoveMouseTo(onlineMenuBanner); + InputManager.Click(MouseButton.Left); + }); + + AddUntilStep("wait for dialog", () => Game.ChildrenOfType().SingleOrDefault()?.CurrentDialog != null); } } } diff --git a/osu.Game/Screens/Menu/OnlineMenuBanner.cs b/osu.Game/Screens/Menu/OnlineMenuBanner.cs index b9d269c82a..edd34d0bfb 100644 --- a/osu.Game/Screens/Menu/OnlineMenuBanner.cs +++ b/osu.Game/Screens/Menu/OnlineMenuBanner.cs @@ -73,6 +73,9 @@ namespace osu.Game.Screens.Menu Task.Run(() => request.Perform()) .ContinueWith(r => { + if (!FetchOnlineContent) + return; + if (r.IsCompletedSuccessfully) Schedule(() => Current.Value = request.ResponseObject); @@ -170,6 +173,8 @@ namespace osu.Game.Screens.Menu private Sprite flash = null!; + public override bool IsPresent => base.IsPresent || Scheduler.HasPendingTasks; + private ScheduledDelegate? openUrlAction; public MenuImage(APIMenuImage image) From 474ff5b99d089cd5db38d26541598735cb65b9cb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 31 May 2024 10:46:30 +0900 Subject: [PATCH 04/10] Use question mark for more grammatical correctness Co-authored-by: Joseph Madamba --- osu.Game/Online/Chat/ExternalLinkOpener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Online/Chat/ExternalLinkOpener.cs b/osu.Game/Online/Chat/ExternalLinkOpener.cs index 08adf965da..513ccd43af 100644 --- a/osu.Game/Online/Chat/ExternalLinkOpener.cs +++ b/osu.Game/Online/Chat/ExternalLinkOpener.cs @@ -46,7 +46,7 @@ namespace osu.Game.Online.Chat public ExternalLinkDialog(string url, Action openExternalLinkAction, Action copyExternalLinkAction) { HeaderText = "You are leaving osu!"; - BodyText = $"Are you sure you want to open the following link in a web browser:\n\n{url}"; + BodyText = $"Are you sure you want to open the following link in a web browser?\n\n{url}"; Icon = FontAwesome.Solid.ExclamationTriangle; From e52f524ea2ce15b6ef891900a3c7cee236f6d7cb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 31 May 2024 11:44:53 +0900 Subject: [PATCH 05/10] Use common header text --- osu.Game/Online/Chat/ExternalLinkOpener.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Online/Chat/ExternalLinkOpener.cs b/osu.Game/Online/Chat/ExternalLinkOpener.cs index 513ccd43af..2f046d8fd4 100644 --- a/osu.Game/Online/Chat/ExternalLinkOpener.cs +++ b/osu.Game/Online/Chat/ExternalLinkOpener.cs @@ -8,9 +8,10 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Sprites; using osu.Framework.Platform; using osu.Game.Configuration; +using osu.Game.Localisation; using osu.Game.Overlays; using osu.Game.Overlays.Dialog; -using osu.Game.Resources.Localisation.Web; +using CommonStrings = osu.Game.Resources.Localisation.Web.CommonStrings; namespace osu.Game.Online.Chat { @@ -45,7 +46,7 @@ namespace osu.Game.Online.Chat { public ExternalLinkDialog(string url, Action openExternalLinkAction, Action copyExternalLinkAction) { - HeaderText = "You are leaving osu!"; + HeaderText = DeleteConfirmationDialogStrings.HeaderText; BodyText = $"Are you sure you want to open the following link in a web browser?\n\n{url}"; Icon = FontAwesome.Solid.ExclamationTriangle; From 5dfeaa3c4afa75dbfebffbd1c7a61af0dd93b6d4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 31 May 2024 11:46:32 +0900 Subject: [PATCH 06/10] Move dialog strings to more common class name --- ...{DeleteConfirmationDialogStrings.cs => DialogStrings.cs} | 6 +++--- osu.Game/Online/Chat/ExternalLinkOpener.cs | 2 +- osu.Game/Overlays/Dialog/DangerousActionDialog.cs | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) rename osu.Game/Localisation/{DeleteConfirmationDialogStrings.cs => DialogStrings.cs} (80%) diff --git a/osu.Game/Localisation/DeleteConfirmationDialogStrings.cs b/osu.Game/Localisation/DialogStrings.cs similarity index 80% rename from osu.Game/Localisation/DeleteConfirmationDialogStrings.cs rename to osu.Game/Localisation/DialogStrings.cs index 25997eadd3..cea543eb9f 100644 --- a/osu.Game/Localisation/DeleteConfirmationDialogStrings.cs +++ b/osu.Game/Localisation/DialogStrings.cs @@ -5,14 +5,14 @@ using osu.Framework.Localisation; namespace osu.Game.Localisation { - public static class DeleteConfirmationDialogStrings + public static class DialogStrings { - private const string prefix = @"osu.Game.Resources.Localisation.DeleteConfirmationDialog"; + private const string prefix = @"osu.Game.Resources.Localisation.DialogStrings"; /// /// "Caution" /// - public static LocalisableString HeaderText => new TranslatableString(getKey(@"header_text"), @"Caution"); + public static LocalisableString Caution => new TranslatableString(getKey(@"header_text"), @"Caution"); /// /// "Yes. Go for it." diff --git a/osu.Game/Online/Chat/ExternalLinkOpener.cs b/osu.Game/Online/Chat/ExternalLinkOpener.cs index 2f046d8fd4..82ad4215c2 100644 --- a/osu.Game/Online/Chat/ExternalLinkOpener.cs +++ b/osu.Game/Online/Chat/ExternalLinkOpener.cs @@ -46,7 +46,7 @@ namespace osu.Game.Online.Chat { public ExternalLinkDialog(string url, Action openExternalLinkAction, Action copyExternalLinkAction) { - HeaderText = DeleteConfirmationDialogStrings.HeaderText; + HeaderText = DialogStrings.Caution; BodyText = $"Are you sure you want to open the following link in a web browser?\n\n{url}"; Icon = FontAwesome.Solid.ExclamationTriangle; diff --git a/osu.Game/Overlays/Dialog/DangerousActionDialog.cs b/osu.Game/Overlays/Dialog/DangerousActionDialog.cs index 42a3ff827c..60f51e7e06 100644 --- a/osu.Game/Overlays/Dialog/DangerousActionDialog.cs +++ b/osu.Game/Overlays/Dialog/DangerousActionDialog.cs @@ -30,7 +30,7 @@ namespace osu.Game.Overlays.Dialog protected DangerousActionDialog() { - HeaderText = DeleteConfirmationDialogStrings.HeaderText; + HeaderText = DialogStrings.DialogCautionHeader; Icon = FontAwesome.Regular.TrashAlt; @@ -38,12 +38,12 @@ namespace osu.Game.Overlays.Dialog { new PopupDialogDangerousButton { - Text = DeleteConfirmationDialogStrings.Confirm, + Text = DialogStrings.DialogConfirm, Action = () => DangerousAction?.Invoke() }, new PopupDialogCancelButton { - Text = DeleteConfirmationDialogStrings.Cancel, + Text = DialogStrings.Cancel, Action = () => CancelAction?.Invoke() } }; From cb72630ce1894a3ed73f607112095c4845413349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 31 May 2024 08:09:06 +0200 Subject: [PATCH 07/10] Fix compile failures --- osu.Game/Overlays/Dialog/DangerousActionDialog.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Overlays/Dialog/DangerousActionDialog.cs b/osu.Game/Overlays/Dialog/DangerousActionDialog.cs index 60f51e7e06..31160d1832 100644 --- a/osu.Game/Overlays/Dialog/DangerousActionDialog.cs +++ b/osu.Game/Overlays/Dialog/DangerousActionDialog.cs @@ -30,7 +30,7 @@ namespace osu.Game.Overlays.Dialog protected DangerousActionDialog() { - HeaderText = DialogStrings.DialogCautionHeader; + HeaderText = DialogStrings.Caution; Icon = FontAwesome.Regular.TrashAlt; @@ -38,7 +38,7 @@ namespace osu.Game.Overlays.Dialog { new PopupDialogDangerousButton { - Text = DialogStrings.DialogConfirm, + Text = DialogStrings.Confirm, Action = () => DangerousAction?.Invoke() }, new PopupDialogCancelButton From 1a26a5dbdaf77256564930e69e5f20d3e9a2cd17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 31 May 2024 08:09:25 +0200 Subject: [PATCH 08/10] Fix mismatching localisation key prefix The `Strings` suffix is not supposed to be in here, judging by other localisation classes. --- osu.Game/Localisation/DialogStrings.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Localisation/DialogStrings.cs b/osu.Game/Localisation/DialogStrings.cs index cea543eb9f..043a3f5b4c 100644 --- a/osu.Game/Localisation/DialogStrings.cs +++ b/osu.Game/Localisation/DialogStrings.cs @@ -7,7 +7,7 @@ namespace osu.Game.Localisation { public static class DialogStrings { - private const string prefix = @"osu.Game.Resources.Localisation.DialogStrings"; + private const string prefix = @"osu.Game.Resources.Localisation.Dialog"; /// /// "Caution" From c6a7082034da26c6399633ad18665a85ccc58f30 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 31 May 2024 15:38:26 +0900 Subject: [PATCH 09/10] Fix incorrect prefix check --- osu.Game/OsuGame.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index 29c040c597..0833f52b1e 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -496,7 +496,7 @@ namespace osu.Game } else { - isTrustedDomain = url.StartsWith(API.APIEndpointUrl, StringComparison.Ordinal); + isTrustedDomain = url.StartsWith(API.WebsiteRootUrl, StringComparison.Ordinal); } if (!url.CheckIsValidUrl()) From 69990c35cb638d57e735cedd6785fe2a06bd4341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 31 May 2024 08:47:19 +0200 Subject: [PATCH 10/10] Add commentary on presence of `IsPresent` override --- osu.Game/Screens/Menu/OnlineMenuBanner.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Screens/Menu/OnlineMenuBanner.cs b/osu.Game/Screens/Menu/OnlineMenuBanner.cs index edd34d0bfb..49fc89c171 100644 --- a/osu.Game/Screens/Menu/OnlineMenuBanner.cs +++ b/osu.Game/Screens/Menu/OnlineMenuBanner.cs @@ -173,6 +173,9 @@ namespace osu.Game.Screens.Menu private Sprite flash = null!; + /// + /// Overridden as a safety for functioning correctly. + /// public override bool IsPresent => base.IsPresent || Scheduler.HasPendingTasks; private ScheduledDelegate? openUrlAction;