diff --git a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs
index 0f93bdf66..0d280b5a2 100644
--- a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs
+++ b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs
@@ -47,16 +47,14 @@ public class AuthIdTests : IDisposable
}
[Fact]
- public void DoesNotVouchForADifferentAccount()
+ public void RejectsADifferentAccount()
{
var issued = CreateAccount("authid-owner-user");
var other = CreateAccount("authid-other-user");
var authId = Register(issued, AddressX);
- // AccountMismatch, not Rejected: the caller must still be able to accept this login by
- // verifying the password, exactly as it did before pre-authentication existed.
Assert.Equal(
- IncomingAccountPackets.AuthIdResult.AccountMismatch,
+ IncomingAccountPackets.AuthIdResult.Rejected,
IncomingAccountPackets.ConsumeAuthId(authId, other.Username, AddressX, out _)
);
}
@@ -132,17 +130,112 @@ public class AuthIdTests : IDisposable
var authId = Register(account, AddressX);
Assert.Equal(
- IncomingAccountPackets.AuthIdResult.AccountMismatch,
+ IncomingAccountPackets.AuthIdResult.Rejected,
IncomingAccountPackets.ConsumeAuthId(authId, "not-the-owner", AddressX, out _)
);
- // Spent regardless, so a guessed id cannot be reused to enumerate usernames.
+ // Spent regardless, so a guessed id cannot be reused to probe for its owner.
Assert.Equal(
IncomingAccountPackets.AuthIdResult.Rejected,
IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _)
);
}
+ ///
+ /// An expired id is not a lockout. A player can sit on the server list, and before any of this
+ /// existed the game login always verified the password anyway -- so falling back to that verify
+ /// is the behaviour we started from, not a regression.
+ ///
+ [Fact]
+ public void ExpiresIntoAPasswordVerifyRatherThanARejection()
+ {
+ var account = CreateAccount("authid-expired-user");
+ var authId = Register(account, AddressX);
+
+ var now = Core._now;
+
+ try
+ {
+ Core._now = now + TimeSpan.FromMinutes(30.0);
+
+ Assert.Equal(
+ IncomingAccountPackets.AuthIdResult.Expired,
+ IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out var entry)
+ );
+
+ // Still carries the client version the game login needs.
+ Assert.Equal(new ClientVersion(7, 0, 0, 0), entry.Version);
+ }
+ finally
+ {
+ Core._now = now;
+ }
+ }
+
+ [Fact]
+ public void AnExpiredIdFromAnotherAddressIsStillRejected()
+ {
+ var account = CreateAccount("authid-expired-elsewhere-user");
+ var authId = Register(account, AddressX);
+
+ var now = Core._now;
+
+ try
+ {
+ Core._now = now + TimeSpan.FromMinutes(30.0);
+
+ Assert.Equal(
+ IncomingAccountPackets.AuthIdResult.Rejected,
+ IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressY, out _)
+ );
+ }
+ finally
+ {
+ Core._now = now;
+ }
+ }
+
+ ///
+ /// Ids are only consumed by a game login, so a player who reaches the server list and never
+ /// picks a server abandons theirs. Enough of those must not push out ids that live clients are
+ /// still on their way to redeem.
+ ///
+ [Fact]
+ public void AbandonedIdsAreReclaimedBeforeALiveOneIsEvicted()
+ {
+ var abandoned = CreateAccount("authid-abandoned-user");
+ var live = CreateAccount("authid-live-user");
+
+ var now = Core._now;
+
+ try
+ {
+ // Fill the window with ids nobody ever redeems.
+ for (var i = 0; i < 128; i++)
+ {
+ Register(abandoned, AddressX);
+ }
+
+ Assert.Equal(128, IncomingAccountPackets.AuthIdWindowCount);
+
+ Core._now = now + TimeSpan.FromMinutes(30.0);
+
+ // The next issue reclaims the dead entries instead of evicting, so the id it hands out
+ // is the only one left and is immediately redeemable.
+ var liveId = Register(live, AddressX);
+
+ Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount);
+ Assert.Equal(
+ IncomingAccountPackets.AuthIdResult.Vouched,
+ IncomingAccountPackets.ConsumeAuthId(liveId, live.Username, AddressX, out _)
+ );
+ }
+ finally
+ {
+ Core._now = now;
+ }
+ }
+
[Fact]
public void PreAuthenticatedGameLogin_SkipsThePasswordCheck()
{
diff --git a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
index 3d3f825a7..01eb3eb32 100644
--- a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
+++ b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
@@ -58,14 +58,16 @@ public static class IncomingAccountPackets
internal enum AuthIdResult
{
- // No such id, or it expired, or it came from another address. Indistinguishable from forged.
+ // No such id, or it was issued for a different account or address. A client that presents
+ // one of those is not one of ours; nothing here is worth a password check.
Rejected,
- // Live id from the right address, but for a different account than the one being logged
- // into. Proves nothing, so the caller falls back to verifying the password.
- AccountMismatch,
+ // Right account, right address, but too old to stand in for the verify. A player can idle
+ // on the server list, so this is a normal thing to do -- fall back to checking the password
+ // rather than turning it into a lockout.
+ Expired,
- // Issued to this account, from this address. Stands in for the password verify.
+ // Issued to this account, from this address, recently. Stands in for the password verify.
Vouched
}
@@ -345,6 +347,15 @@ public static class IncomingAccountPackets
internal static int RegisterAuthId(IAccount account, IPAddress address, ClientVersion version)
{
+ // Ids are only consumed by a game login, so anyone who reaches the server list and never
+ // picks a server leaves theirs behind. Reclaim the dead ones before evicting a live one --
+ // otherwise enough abandoned logins fill the window and start pushing out ids that clients
+ // are still on their way to redeem.
+ if (_authIDWindow.Count >= _authIDWindowSize)
+ {
+ PurgeExpiredAuthIds();
+ }
+
if (_authIDWindow.Count >= _authIDWindowSize)
{
var oldestID = 0;
@@ -392,18 +403,39 @@ public static class IncomingAccountPackets
return AuthIdResult.Rejected;
}
- if (Core.Now - entry.Age > _authIDLifetime || !Utility.Intern(address).Equals(entry.Address))
+ // Address before age: a different address is a different client, however fresh the id is.
+ if (!Utility.Intern(address).Equals(entry.Address))
{
return AuthIdResult.Rejected;
}
- return entry.Account != null && username.InsensitiveEquals(entry.Account.Username)
- ? AuthIdResult.Vouched
- : AuthIdResult.AccountMismatch;
+ if (entry.Account == null || !username.InsensitiveEquals(entry.Account.Username))
+ {
+ return AuthIdResult.Rejected;
+ }
+
+ return Core.Now - entry.Age > _authIDLifetime ? AuthIdResult.Expired : AuthIdResult.Vouched;
+ }
+
+ private static void PurgeExpiredAuthIds()
+ {
+ var now = Core.Now;
+
+ // Removing during enumeration is supported on Dictionary since .NET Core 3.0, so this
+ // reclaims in one pass with no scratch list.
+ foreach (var (key, entry) in _authIDWindow)
+ {
+ if (now - entry.Age > _authIDLifetime)
+ {
+ _authIDWindow.Remove(key);
+ }
+ }
}
internal static void ClearAuthIdWindow() => _authIDWindow.Clear();
+ internal static int AuthIdWindowCount => _authIDWindow.Count;
+
public static void GameLogin(NetState state, SpanReader reader)
{
if (state.SentFirstPacket)
@@ -439,6 +471,8 @@ public static class IncomingAccountPackets
state.Version = ap.Version;
state.Seeded = true;
+ // Expired still carries a usable entry, and the client just pays the password verify it
+ // would have paid before any of this existed.
var e = new GameServer.GameLoginEventArgs(
state,
username,