From ed89cf816108738aded1e58a4ae74eea2892c94a Mon Sep 17 00:00:00 2001 From: Patrick Pimentel Date: Tue, 2 Dec 2025 14:22:17 -0500 Subject: [PATCH] fix(auth-validator): [PM-22975] Client Version Validator - Made enough changes so that it's ready for review by KM --- src/Core/Context/ICurrentContext.cs | 2 +- .../ClientVersionValidator.cs | 7 ++- .../Constants/TestEncryptionConstants.cs | 8 +--- .../Queries/IsV2EncryptionUserQueryTests.cs | 10 ++--- .../Endpoints/IdentityServerSsoTests.cs | 45 ++++++++----------- .../Login/ClientVersionGateTests.cs | 13 +++--- .../ClientVersionValidatorTests.cs | 2 +- 7 files changed, 40 insertions(+), 47 deletions(-) diff --git a/src/Core/Context/ICurrentContext.cs b/src/Core/Context/ICurrentContext.cs index d527cdd363..3de09f9441 100644 --- a/src/Core/Context/ICurrentContext.cs +++ b/src/Core/Context/ICurrentContext.cs @@ -32,7 +32,7 @@ public interface ICurrentContext Guid? OrganizationId { get; set; } IdentityClientType IdentityClientType { get; set; } string ClientId { get; set; } - Version ClientVersion { get; set; } + Version? ClientVersion { get; set; } bool ClientVersionIsPrerelease { get; set; } Task BuildAsync(HttpContext httpContext, GlobalSettings globalSettings); diff --git a/src/Identity/IdentityServer/RequestValidators/ClientVersionValidator.cs b/src/Identity/IdentityServer/RequestValidators/ClientVersionValidator.cs index d8b9e5c3db..a9de0550cc 100644 --- a/src/Identity/IdentityServer/RequestValidators/ClientVersionValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/ClientVersionValidator.cs @@ -25,11 +25,14 @@ public class ClientVersionValidator( return true; } - Version clientVersion = currentContext.ClientVersion; + Version? clientVersion = currentContext.ClientVersion; Version? minVersion = await getMinimumClientVersionForUserQuery.Run(user); // Allow through if headers are missing. - if (minVersion == null) + // The minVersion should never be null because of where this validator is run. The user would + // have been determined to be null prior to reaching this point, but it is defensive programming + // to check for nullish values in case validators were to ever be reordered. + if (clientVersion == null || minVersion == null) { return true; } diff --git a/test/Common/Constants/TestEncryptionConstants.cs b/test/Common/Constants/TestEncryptionConstants.cs index 40ce984f23..45d3281865 100644 --- a/test/Common/Constants/TestEncryptionConstants.cs +++ b/test/Common/Constants/TestEncryptionConstants.cs @@ -12,12 +12,8 @@ public static class TestEncryptionConstants // Private key indicating v2 (used in multiple tests to mark v2 state) public const string V2PrivateKey = "7.cose"; // Wrapped signing key and verifying key values from real tests - public const string V2WrappedSigningKey = "test-wrapped-signing-key"; - public const string V2VerifyingKey = "test-verifying-key"; - // Additional related v2 values used in tests - public const string V2PublicKey = "test-public-key"; - public const string V2WrappedPrivateKey = "test-private-key"; - public const string V2SignedPublicKey = "test-signed-public-key"; + public const string V2WrappedSigningKey = "7.cose_signing"; + public const string V2VerifyingKey = "vk"; } diff --git a/test/Core.Test/KeyManagement/Queries/IsV2EncryptionUserQueryTests.cs b/test/Core.Test/KeyManagement/Queries/IsV2EncryptionUserQueryTests.cs index 8dc7410033..30ec47c2fe 100644 --- a/test/Core.Test/KeyManagement/Queries/IsV2EncryptionUserQueryTests.cs +++ b/test/Core.Test/KeyManagement/Queries/IsV2EncryptionUserQueryTests.cs @@ -12,10 +12,10 @@ namespace Bit.Core.Test.KeyManagement.Queries; public class IsV2EncryptionUserQueryTests { - private class FakeSigRepo : IUserSignatureKeyPairRepository + private class FakeUserSignatureKeyPairRepository : IUserSignatureKeyPairRepository { private readonly bool _hasKeys; - public FakeSigRepo(bool hasKeys) { _hasKeys = hasKeys; } + public FakeUserSignatureKeyPairRepository(bool hasKeys) { _hasKeys = hasKeys; } public Task GetByUserIdAsync(Guid userId) => Task.FromResult(_hasKeys ? new SignatureKeyPairData(SignatureAlgorithm.Ed25519, TestEncryptionConstants.V2WrappedSigningKey, TestEncryptionConstants.V2VerifyingKey) : null); @@ -35,7 +35,7 @@ public class IsV2EncryptionUserQueryTests public async Task Run_ReturnsTrue_ForV2State() { var user = new User { Id = Guid.NewGuid(), PrivateKey = TestEncryptionConstants.V2PrivateKey }; - var sut = new IsV2EncryptionUserQuery(new FakeSigRepo(true)); + var sut = new IsV2EncryptionUserQuery(new FakeUserSignatureKeyPairRepository(true)); var result = await sut.Run(user); @@ -46,7 +46,7 @@ public class IsV2EncryptionUserQueryTests public async Task Run_ReturnsFalse_ForV1State() { var user = new User { Id = Guid.NewGuid(), PrivateKey = TestEncryptionConstants.V1EncryptedBase64 }; - var sut = new IsV2EncryptionUserQuery(new FakeSigRepo(false)); + var sut = new IsV2EncryptionUserQuery(new FakeUserSignatureKeyPairRepository(false)); var result = await sut.Run(user); @@ -57,7 +57,7 @@ public class IsV2EncryptionUserQueryTests public async Task Run_ThrowsForInvalidMixedState() { var user = new User { Id = Guid.NewGuid(), PrivateKey = TestEncryptionConstants.V2PrivateKey }; - var sut = new IsV2EncryptionUserQuery(new FakeSigRepo(false)); + var sut = new IsV2EncryptionUserQuery(new FakeUserSignatureKeyPairRepository(false)); await Assert.ThrowsAsync(async () => await sut.Run(user)); } diff --git a/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs b/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs index 8846214b78..22447593b4 100644 --- a/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs @@ -342,8 +342,7 @@ public class IdentityServerSsoTests { "code", "test_code" }, { "code_verifier", challenge }, { "redirect_uri", "https://localhost:8080/sso-connector.html" } - }), - http => { http.Request.Headers.Append("Bitwarden-Client-Version", "2025.10.0"); }); + })); // Assert // If the organization has selected TrustedDeviceEncryption but the user still has their master password @@ -412,12 +411,7 @@ public class IdentityServerSsoTests { "code", "test_code" }, { "code_verifier", challenge }, { "redirect_uri", "https://localhost:8080/sso-connector.html" } - }), - http => - { - http.Request.Headers.Append("Bitwarden-Client-Version", "2025.10.0"); - http.Request.Headers.Append("Accept", "application/json"); - }); + })); // Assert // If the organization has selected TrustedDeviceEncryption but the user still has their master password @@ -490,8 +484,7 @@ public class IdentityServerSsoTests { "code", "test_code" }, { "code_verifier", challenge }, { "redirect_uri", "https://localhost:8080/sso-connector.html" } - }), - http => { http.Request.Headers.Append("Bitwarden-Client-Version", "2025.10.0"); }); + })); Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); using var responseBody = await AssertHelper.AssertResponseTypeIs(context); @@ -554,22 +547,22 @@ public class IdentityServerSsoTests }, challenge, trustedDeviceEnabled); await configureFactory(factory); - var context = await factory.Server.PostAsync("/connect/token", new FormUrlEncodedContent(new Dictionary - { - { "scope", "api offline_access" }, - { "client_id", "web" }, - { "deviceType", "10" }, - { "deviceIdentifier", "test_id" }, - { "deviceName", "firefox" }, - { "twoFactorToken", "TEST"}, - { "twoFactorProvider", "5" }, // RememberMe Provider - { "twoFactorRemember", "0" }, - { "grant_type", "authorization_code" }, - { "code", "test_code" }, - { "code_verifier", challenge }, - { "redirect_uri", "https://localhost:8080/sso-connector.html" } - }), - http => { http.Request.Headers.Append("Bitwarden-Client-Version", "2025.10.0"); }); + var context = await factory.Server.PostAsync("/connect/token", new FormUrlEncodedContent( + new Dictionary + { + { "scope", "api offline_access" }, + { "client_id", "web" }, + { "deviceType", "10" }, + { "deviceIdentifier", "test_id" }, + { "deviceName", "firefox" }, + { "twoFactorToken", "TEST" }, + { "twoFactorProvider", "5" }, // RememberMe Provider + { "twoFactorRemember", "0" }, + { "grant_type", "authorization_code" }, + { "code", "test_code" }, + { "code_verifier", challenge }, + { "redirect_uri", "https://localhost:8080/sso-connector.html" } + })); // If this fails, surface detailed error information to aid debugging if (context.Response.StatusCode != StatusCodes.Status200OK) diff --git a/test/Identity.IntegrationTest/Login/ClientVersionGateTests.cs b/test/Identity.IntegrationTest/Login/ClientVersionGateTests.cs index 642148b685..02eca826ab 100644 --- a/test/Identity.IntegrationTest/Login/ClientVersionGateTests.cs +++ b/test/Identity.IntegrationTest/Login/ClientVersionGateTests.cs @@ -4,6 +4,7 @@ using Bit.Core.KeyManagement.Enums; using Bit.Core.Test.Auth.AutoFixture; using Bit.IntegrationTestCommon.Factories; using Bit.Test.Common.AutoFixture.Attributes; +using Bit.Test.Common.Constants; using Microsoft.EntityFrameworkCore; using Xunit; @@ -29,14 +30,14 @@ public class ClientVersionGateTests : IClassFixture // Make user V2: set private key to COSE and add signature key pair var db = localFactory.GetDatabaseContext(); var efUser = await db.Users.FirstAsync(u => u.Email == user.Email); - efUser.PrivateKey = "7.cose"; + efUser.PrivateKey = TestEncryptionConstants.V2PrivateKey; db.UserSignatureKeyPairs.Add(new Bit.Infrastructure.EntityFramework.Models.UserSignatureKeyPair { Id = Core.Utilities.CoreHelpers.GenerateComb(), UserId = efUser.Id, SignatureAlgorithm = SignatureAlgorithm.Ed25519, - SigningKey = "7.cose_signing", - VerifyingKey = "vk" + SigningKey = TestEncryptionConstants.V2WrappedSigningKey, + VerifyingKey = TestEncryptionConstants.V2VerifyingKey, }); await db.SaveChangesAsync(); @@ -74,14 +75,14 @@ public class ClientVersionGateTests : IClassFixture // Make user V2 var db = localFactory.GetDatabaseContext(); var efUser = await db.Users.FirstAsync(u => u.Email == user.Email); - efUser.PrivateKey = "7.cose"; + efUser.PrivateKey = TestEncryptionConstants.V2PrivateKey; db.UserSignatureKeyPairs.Add(new Bit.Infrastructure.EntityFramework.Models.UserSignatureKeyPair { Id = Core.Utilities.CoreHelpers.GenerateComb(), UserId = efUser.Id, SignatureAlgorithm = SignatureAlgorithm.Ed25519, - SigningKey = "7.cose_signing", - VerifyingKey = "vk" + SigningKey = TestEncryptionConstants.V2WrappedSigningKey, + VerifyingKey = TestEncryptionConstants.V2VerifyingKey, }); await db.SaveChangesAsync(); diff --git a/test/Identity.Test/IdentityServer/RequestValidators/ClientVersionValidatorTests.cs b/test/Identity.Test/IdentityServer/RequestValidators/ClientVersionValidatorTests.cs index 8410a4eb75..494fb79d25 100644 --- a/test/Identity.Test/IdentityServer/RequestValidators/ClientVersionValidatorTests.cs +++ b/test/Identity.Test/IdentityServer/RequestValidators/ClientVersionValidatorTests.cs @@ -54,7 +54,7 @@ public class ClientVersionValidatorTests [Fact] public async Task Allows_When_ClientVersionHeaderMissing() { - // Do not set ClientVersion on the context (remains null) and ensure we fail open + // Do not set ClientVersion on the context (remains null) and ensure var ctx = Substitute.For(); var minQuery = MakeMinQuery(new Version("2025.11.0")); var sut = new ClientVersionValidator(ctx, minQuery);