From 48370d74b7093c67cf8f45b4af4a7bee55e9f355 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Tue, 13 Jul 2021 21:33:35 +0200 Subject: [PATCH 1/9] Missing group safeguards. - Server will no longer start up when the guest or default groups cannot be located. - Players joining with unknown groups assigned to them will be disconnected with an error --- TShockAPI/Commands.cs | 7 +++++-- TShockAPI/DB/GroupManager.cs | 15 +++++++++++++++ TShockAPI/GetDataHandlers.cs | 10 ++++++++-- TShockAPI/TShock.cs | 1 + TShockAPI/Utils.cs | 11 +++++++++++ TerrariaServerAPI | 2 +- 6 files changed, 41 insertions(+), 5 deletions(-) diff --git a/TShockAPI/Commands.cs b/TShockAPI/Commands.cs index b6ba179f..80e710ad 100644 --- a/TShockAPI/Commands.cs +++ b/TShockAPI/Commands.cs @@ -823,10 +823,13 @@ namespace TShockAPI (usingUUID && account.UUID == args.Player.UUID && !TShock.Config.Settings.DisableUUIDLogin && !String.IsNullOrWhiteSpace(args.Player.UUID))) { - args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); - var group = TShock.Groups.GetGroupByName(account.Group); + if (!TShock.Utils.AssertGroupValid(args.Player, group)) + return; + + args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); + args.Player.Group = group; args.Player.tempGroup = null; args.Player.Account = account; diff --git a/TShockAPI/DB/GroupManager.cs b/TShockAPI/DB/GroupManager.cs index 7b3b62dd..8133edaa 100644 --- a/TShockAPI/DB/GroupManager.cs +++ b/TShockAPI/DB/GroupManager.cs @@ -202,6 +202,21 @@ namespace TShockAPI.DB Group.DefaultGroup = GetGroupByName(TShock.Config.Settings.DefaultGuestGroupName); } + internal void EnsureCoreGroupsPresent() + { + if (!GroupExists(TShock.Config.Settings.DefaultGuestGroupName)) + { + TShock.Log.ConsoleError("The guest group could not be found. This may indicate a typo in the configuration file, or that the group was renamed or deleted."); + throw new Exception("The guest group could not be found."); + } + + if(!GroupExists(TShock.Config.Settings.DefaultRegistrationGroupName)) + { + TShock.Log.ConsoleError("The default usergroup could not be found. This may indicate a typo in the configuration file, or that the group was renamed or deleted."); + throw new Exception("The default usergroup could not be found."); + } + } + private void AddDefaultGroup(string name, string parent, string permissions) { if (!GroupExists(name)) diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index e92d19a9..b90f6fe1 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -2450,10 +2450,13 @@ namespace TShockAPI args.Player.State = 2; NetMessage.SendData((int)PacketTypes.WorldInfo, args.Player.Index); - args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); - var group = TShock.Groups.GetGroupByName(account.Group); + if (!TShock.Utils.AssertGroupValid(args.Player, group)) + return true; + + args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); + args.Player.Group = group; args.Player.tempGroup = null; args.Player.Account = account; @@ -3004,6 +3007,9 @@ namespace TShockAPI var group = TShock.Groups.GetGroupByName(account.Group); + if (!TShock.Utils.AssertGroupValid(args.Player, group)) + return true; + args.Player.Group = group; args.Player.tempGroup = null; args.Player.Account = account; diff --git a/TShockAPI/TShock.cs b/TShockAPI/TShock.cs index ed74b882..bc1d26e7 100644 --- a/TShockAPI/TShock.cs +++ b/TShockAPI/TShock.cs @@ -322,6 +322,7 @@ namespace TShockAPI Regions = new RegionManager(DB); UserAccounts = new UserAccountManager(DB); Groups = new GroupManager(DB); + Groups.EnsureCoreGroupsPresent(); ProjectileBans = new ProjectileManagager(DB); TileBans = new TileManager(DB); RememberedPos = new RememberedPosManager(DB); diff --git a/TShockAPI/Utils.cs b/TShockAPI/Utils.cs index 23007b19..36a6d348 100644 --- a/TShockAPI/Utils.cs +++ b/TShockAPI/Utils.cs @@ -212,6 +212,17 @@ namespace TShockAPI } while (TilePlacementValid(tileX, tileY) && TileSolid(tileX, tileY)); } + public bool AssertGroupValid(TSPlayer player, Group group) + { + if (group == null) + { + player.Disconnect("Your account's group could not be found. Please contact server administrators about this."); + return false; + } + + return true; + } + /// /// Determines if a tile is valid. /// diff --git a/TerrariaServerAPI b/TerrariaServerAPI index 8c2c0873..4b555bc3 160000 --- a/TerrariaServerAPI +++ b/TerrariaServerAPI @@ -1 +1 @@ -Subproject commit 8c2c087327bbd1f20ff6c46f4d11e5714e57064b +Subproject commit 4b555bc373dbb470bc69ebed69c79de116f28df2 From f7c550a8adc6abf0648b741fedce0726c58db499 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Tue, 13 Jul 2021 21:39:20 +0200 Subject: [PATCH 2/9] Fix the server not reporting startup errors correctly. --- TShockAPI/TShock.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/TShockAPI/TShock.cs b/TShockAPI/TShock.cs index bc1d26e7..996463e1 100644 --- a/TShockAPI/TShock.cs +++ b/TShockAPI/TShock.cs @@ -387,8 +387,8 @@ namespace TShockAPI } catch (Exception ex) { - Log.Error("Fatal Startup Exception"); - Log.Error(ex.ToString()); + Log.ConsoleError("Fatal Startup Exception"); + Log.ConsoleError(ex.ToString()); Environment.Exit(1); } } From 6154ee60c1e884c21275fc7ced7a7fb5f398f667 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Tue, 13 Jul 2021 21:43:14 +0200 Subject: [PATCH 3/9] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85d31e64..0bf907db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Correct rejection message in LandGolfBallInCupHandler to output the proper expected player id. (@drunderscore) * Clarified the error mesage that the console is presented if a rate-limit is reached over REST to indicate that "tokens" actually refers to rate-limit tokens, and not auth tokens, and added a hint as to what config setting determines this. (@hakusaro, @patsore) * Fixed an issue where, when the console was redirected, input was disabled and commands didn't work, in TSAPI. You can now pass `-disable-commands` to disable the input thread, but by default, it will be enabled. Fixes [#1450](https://github.com/Pryaxis/TShock/issues/1450). (@DeathCradle, @QuiCM) +* Fixed missing group problems & error reporting on startup. (@bartico6) ## TShock 4.5.4 * Fixed ridiculous typo in `GetDataHandlers` which caused TShock to read the wrong field in the packet for `usingBiomeTorches`. (@hakusaro, @Arthri) From 048aaf6f0c3b503c921cdc73bc4e524eaa107274 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Tue, 13 Jul 2021 21:47:27 +0200 Subject: [PATCH 4/9] /login kick -> error, add XML doc to Utils method. --- TShockAPI/Commands.cs | 5 ++++- TShockAPI/Utils.cs | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/TShockAPI/Commands.cs b/TShockAPI/Commands.cs index 80e710ad..d0c3b48a 100644 --- a/TShockAPI/Commands.cs +++ b/TShockAPI/Commands.cs @@ -825,8 +825,11 @@ namespace TShockAPI { var group = TShock.Groups.GetGroupByName(account.Group); - if (!TShock.Utils.AssertGroupValid(args.Player, group)) + if (group == null) + { + args.Player.SendErrorMessage("Login failed: The account references a group that doesn't exist."); return; + } args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); diff --git a/TShockAPI/Utils.cs b/TShockAPI/Utils.cs index 36a6d348..be3722df 100644 --- a/TShockAPI/Utils.cs +++ b/TShockAPI/Utils.cs @@ -212,6 +212,12 @@ namespace TShockAPI } while (TilePlacementValid(tileX, tileY) && TileSolid(tileX, tileY)); } + /// + /// Asserts that the group reference can be safely assigned to the player object. + /// + /// + /// + /// public bool AssertGroupValid(TSPlayer player, Group group) { if (group == null) From c759af6d49650562d8a3e4d8255ae812344a3035 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Tue, 13 Jul 2021 21:53:54 +0200 Subject: [PATCH 5/9] Minor update. - AssertGroupValid now both sends the message and kicks the player depending on input parameter. - /login and DataHandler code is now an identical assert check. --- TShockAPI/Commands.cs | 4 ++-- TShockAPI/GetDataHandlers.cs | 4 ++-- TShockAPI/Utils.cs | 8 ++++++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/TShockAPI/Commands.cs b/TShockAPI/Commands.cs index d0c3b48a..c3c7090d 100644 --- a/TShockAPI/Commands.cs +++ b/TShockAPI/Commands.cs @@ -825,9 +825,9 @@ namespace TShockAPI { var group = TShock.Groups.GetGroupByName(account.Group); - if (group == null) + if (!TShock.Utils.AssertGroupValid(args.Player, group, false)) { - args.Player.SendErrorMessage("Login failed: The account references a group that doesn't exist."); + args.Player.SendErrorMessage("Login attempt failed - see the message above."); return; } diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index b90f6fe1..bc5f2175 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -2452,7 +2452,7 @@ namespace TShockAPI var group = TShock.Groups.GetGroupByName(account.Group); - if (!TShock.Utils.AssertGroupValid(args.Player, group)) + if (!TShock.Utils.AssertGroupValid(args.Player, group, true)) return true; args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); @@ -3007,7 +3007,7 @@ namespace TShockAPI var group = TShock.Groups.GetGroupByName(account.Group); - if (!TShock.Utils.AssertGroupValid(args.Player, group)) + if (!TShock.Utils.AssertGroupValid(args.Player, group, true)) return true; args.Player.Group = group; diff --git a/TShockAPI/Utils.cs b/TShockAPI/Utils.cs index be3722df..aad127dc 100644 --- a/TShockAPI/Utils.cs +++ b/TShockAPI/Utils.cs @@ -214,15 +214,19 @@ namespace TShockAPI /// /// Asserts that the group reference can be safely assigned to the player object. + /// If this assertion fails, and is true, the player is disconnected. If is false, the player will receive an error message. /// /// /// /// - public bool AssertGroupValid(TSPlayer player, Group group) + public bool AssertGroupValid(TSPlayer player, Group group, bool kick) { if (group == null) { - player.Disconnect("Your account's group could not be found. Please contact server administrators about this."); + if (kick) + player.Disconnect("Your account's group could not be loaded. Please contact server administrators about this."); + else + player.SendErrorMessage("Your account's group could not be loaded. Please contact server administrators about this."); return false; } From 51348d18064d0092ce5118af906e646f82069208 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Sat, 31 Jul 2021 16:34:43 +0200 Subject: [PATCH 6/9] Fixes for PR 2397. - GroupManager now validates core groups in the constructor. - EnsureCoreGroupsPresent -> AssertCoreGroupsPresent. - Fix indentiation in some places. - Clarify the intent of this PR in CHANGELOG.md. --- CHANGELOG.md | 3 ++- TShockAPI/DB/GroupManager.cs | 32 ++++++++++++++++++++++++++++---- TShockAPI/TShock.cs | 1 - TShockAPI/Utils.cs | 7 ++++--- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bf907db..09d6164a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,8 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Correct rejection message in LandGolfBallInCupHandler to output the proper expected player id. (@drunderscore) * Clarified the error mesage that the console is presented if a rate-limit is reached over REST to indicate that "tokens" actually refers to rate-limit tokens, and not auth tokens, and added a hint as to what config setting determines this. (@hakusaro, @patsore) * Fixed an issue where, when the console was redirected, input was disabled and commands didn't work, in TSAPI. You can now pass `-disable-commands` to disable the input thread, but by default, it will be enabled. Fixes [#1450](https://github.com/Pryaxis/TShock/issues/1450). (@DeathCradle, @QuiCM) -* Fixed missing group problems & error reporting on startup. (@bartico6) +* Fixed errors on startup not being reported to console. (@bartico6) +* The server now correctly disconnects players with missing groups instead of throwing an exception, stalling the connection (@bartico6) ## TShock 4.5.4 * Fixed ridiculous typo in `GetDataHandlers` which caused TShock to read the wrong field in the packet for `usingBiomeTorches`. (@hakusaro, @Arthri) diff --git a/TShockAPI/DB/GroupManager.cs b/TShockAPI/DB/GroupManager.cs index 8133edaa..09b19f5a 100644 --- a/TShockAPI/DB/GroupManager.cs +++ b/TShockAPI/DB/GroupManager.cs @@ -200,9 +200,11 @@ namespace TShockAPI.DB LoadPermisions(); Group.DefaultGroup = GetGroupByName(TShock.Config.Settings.DefaultGuestGroupName); + + AssertCoreGroupsPresent(); } - internal void EnsureCoreGroupsPresent() + internal void AssertCoreGroupsPresent() { if (!GroupExists(TShock.Config.Settings.DefaultGuestGroupName)) { @@ -210,11 +212,33 @@ namespace TShockAPI.DB throw new Exception("The guest group could not be found."); } - if(!GroupExists(TShock.Config.Settings.DefaultRegistrationGroupName)) - { + if (!GroupExists(TShock.Config.Settings.DefaultRegistrationGroupName)) + { TShock.Log.ConsoleError("The default usergroup could not be found. This may indicate a typo in the configuration file, or that the group was renamed or deleted."); throw new Exception("The default usergroup could not be found."); - } + } + } + + /// + /// Asserts that the group reference can be safely assigned to the player object. + /// If this assertion fails, and is true, the player is disconnected. If is false, the player will receive an error message. + /// + /// The player in question + /// The group we want to assign them + /// Whether or not failing this check disconnects the player. + /// + public bool AssertGroupValid(TSPlayer player, Group group, bool kick) + { + if (group == null) + { + if (kick) + player.Disconnect("Your account's group could not be loaded. Please contact server administrators about this."); + else + player.SendErrorMessage("Your account's group could not be loaded. Please contact server administrators about this."); + return false; + } + + return true; } private void AddDefaultGroup(string name, string parent, string permissions) diff --git a/TShockAPI/TShock.cs b/TShockAPI/TShock.cs index 996463e1..2adfbf79 100644 --- a/TShockAPI/TShock.cs +++ b/TShockAPI/TShock.cs @@ -322,7 +322,6 @@ namespace TShockAPI Regions = new RegionManager(DB); UserAccounts = new UserAccountManager(DB); Groups = new GroupManager(DB); - Groups.EnsureCoreGroupsPresent(); ProjectileBans = new ProjectileManagager(DB); TileBans = new TileManager(DB); RememberedPos = new RememberedPosManager(DB); diff --git a/TShockAPI/Utils.cs b/TShockAPI/Utils.cs index aad127dc..c5bc6c46 100644 --- a/TShockAPI/Utils.cs +++ b/TShockAPI/Utils.cs @@ -216,11 +216,12 @@ namespace TShockAPI /// Asserts that the group reference can be safely assigned to the player object. /// If this assertion fails, and is true, the player is disconnected. If is false, the player will receive an error message. /// - /// - /// + /// The player in question + /// The group we want to assign them + /// Whether or not failing this check disconnects the player. /// public bool AssertGroupValid(TSPlayer player, Group group, bool kick) - { + { if (group == null) { if (kick) From 02c20337ece20c0fa6ec5c51187b82a36058e467 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Sat, 31 Jul 2021 16:37:09 +0200 Subject: [PATCH 7/9] Further clarification in CHANGELOG. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09d6164a..03f5f754 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Fixed an issue where, when the console was redirected, input was disabled and commands didn't work, in TSAPI. You can now pass `-disable-commands` to disable the input thread, but by default, it will be enabled. Fixes [#1450](https://github.com/Pryaxis/TShock/issues/1450). (@DeathCradle, @QuiCM) * Fixed errors on startup not being reported to console. (@bartico6) * The server now correctly disconnects players with missing groups instead of throwing an exception, stalling the connection (@bartico6) +* The server now rejects login attempts from players who would end up with a missing group. (@bartico6) ## TShock 4.5.4 * Fixed ridiculous typo in `GetDataHandlers` which caused TShock to read the wrong field in the packet for `usingBiomeTorches`. (@hakusaro, @Arthri) From b32058ac51f161a9ff65c64fc31a964f43c0ea6f Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Sat, 7 Aug 2021 13:56:52 +0200 Subject: [PATCH 8/9] Remove the test method from Utils, re-route checks to GroupManager --- TShockAPI/Commands.cs | 2 +- TShockAPI/GetDataHandlers.cs | 4 ++-- TShockAPI/Utils.cs | 22 ---------------------- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/TShockAPI/Commands.cs b/TShockAPI/Commands.cs index 276eee90..b2ad0264 100644 --- a/TShockAPI/Commands.cs +++ b/TShockAPI/Commands.cs @@ -825,7 +825,7 @@ namespace TShockAPI { var group = TShock.Groups.GetGroupByName(account.Group); - if (!TShock.Utils.AssertGroupValid(args.Player, group, false)) + if (!TShock.Groups.AssertGroupValid(args.Player, group, false)) { args.Player.SendErrorMessage("Login attempt failed - see the message above."); return; diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index a9445105..af75c90c 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -2452,7 +2452,7 @@ namespace TShockAPI var group = TShock.Groups.GetGroupByName(account.Group); - if (!TShock.Utils.AssertGroupValid(args.Player, group, true)) + if (!TShock.Groups.AssertGroupValid(args.Player, group, true)) return true; args.Player.PlayerData = TShock.CharacterDB.GetPlayerData(args.Player, account.ID); @@ -3023,7 +3023,7 @@ namespace TShockAPI var group = TShock.Groups.GetGroupByName(account.Group); - if (!TShock.Utils.AssertGroupValid(args.Player, group, true)) + if (!TShock.Groups.AssertGroupValid(args.Player, group, true)) return true; args.Player.Group = group; diff --git a/TShockAPI/Utils.cs b/TShockAPI/Utils.cs index 717f08a2..92d45299 100644 --- a/TShockAPI/Utils.cs +++ b/TShockAPI/Utils.cs @@ -212,28 +212,6 @@ namespace TShockAPI } while (TilePlacementValid(tileX, tileY) && TileSolid(tileX, tileY)); } - /// - /// Asserts that the group reference can be safely assigned to the player object. - /// If this assertion fails, and is true, the player is disconnected. If is false, the player will receive an error message. - /// - /// The player in question - /// The group we want to assign them - /// Whether or not failing this check disconnects the player. - /// - public bool AssertGroupValid(TSPlayer player, Group group, bool kick) - { - if (group == null) - { - if (kick) - player.Disconnect("Your account's group could not be loaded. Please contact server administrators about this."); - else - player.SendErrorMessage("Your account's group could not be loaded. Please contact server administrators about this."); - return false; - } - - return true; - } - /// /// Determines if a tile is valid. /// From a6eff08c82510e810665e4ea158705bf749beb76 Mon Sep 17 00:00:00 2001 From: quake1337 <3310937+bartico6@users.noreply.github.com> Date: Sun, 8 Aug 2021 12:31:30 +0200 Subject: [PATCH 9/9] Checkout the same TSAPI as gen-dev. --- TerrariaServerAPI | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/TerrariaServerAPI b/TerrariaServerAPI index 4b555bc3..8c2c0873 160000 --- a/TerrariaServerAPI +++ b/TerrariaServerAPI @@ -1 +1 @@ -Subproject commit 4b555bc373dbb470bc69ebed69c79de116f28df2 +Subproject commit 8c2c087327bbd1f20ff6c46f4d11e5714e57064b