diff --git a/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs b/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs index 47a308b28d39..b5f0c8189def 100644 --- a/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs +++ b/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs @@ -27,12 +27,12 @@ public static DeviceAuthRequestResponseModel From(DeviceAuthDetails deviceAuthDe EncryptedUserKey = deviceAuthDetails.EncryptedUserKey }; - if (deviceAuthDetails.AuthRequestId != null && deviceAuthDetails.AuthRequestCreatedAt != null) + if (deviceAuthDetails.AuthRequestId != null && deviceAuthDetails.AuthRequestCreationDate != null) { converted.DevicePendingAuthRequest = new PendingAuthRequest { Id = (Guid)deviceAuthDetails.AuthRequestId, - CreationDate = (DateTime)deviceAuthDetails.AuthRequestCreatedAt + CreationDate = (DateTime)deviceAuthDetails.AuthRequestCreationDate }; } diff --git a/src/Core/Auth/Models/Data/DeviceAuthDetails.cs b/src/Core/Auth/Models/Data/DeviceAuthDetails.cs index f9f799c30824..be4e17eb448e 100644 --- a/src/Core/Auth/Models/Data/DeviceAuthDetails.cs +++ b/src/Core/Auth/Models/Data/DeviceAuthDetails.cs @@ -1,14 +1,18 @@ using Bit.Core.Auth.Utilities; using Bit.Core.Entities; -using Bit.Core.Enums; namespace Bit.Core.Auth.Models.Data; public class DeviceAuthDetails : Device { - public bool IsTrusted { get; set; } + public bool IsTrusted => DeviceExtensions.IsTrusted(this); public Guid? AuthRequestId { get; set; } - public DateTime? AuthRequestCreatedAt { get; set; } + public DateTime? AuthRequestCreationDate { get; set; } + + /** + * Parameterless constructor for Dapper name-based mapping. + */ + public DeviceAuthDetails() { } /** * Constructor for EF response. @@ -24,62 +28,18 @@ public DeviceAuthDetails( } Id = device.Id; + UserId = device.UserId; Name = device.Name; Type = device.Type; Identifier = device.Identifier; + PushToken = device.PushToken; CreationDate = device.CreationDate; - IsTrusted = device.IsTrusted(); - EncryptedPublicKey = device.EncryptedPublicKey; + RevisionDate = device.RevisionDate; EncryptedUserKey = device.EncryptedUserKey; + EncryptedPublicKey = device.EncryptedPublicKey; + EncryptedPrivateKey = device.EncryptedPrivateKey; + Active = device.Active; AuthRequestId = authRequestId; - AuthRequestCreatedAt = authRequestCreationDate; - } - - /** - * Constructor for dapper response. - * Note: if the authRequestId or authRequestCreationDate is null it comes back as - * an empty guid and a min value for datetime. That could change if the stored - * procedure runs on a different kind of db. - */ - public DeviceAuthDetails( - Guid id, - Guid userId, - string name, - short type, - string identifier, - string pushToken, - DateTime creationDate, - DateTime revisionDate, - string encryptedUserKey, - string encryptedPublicKey, - string encryptedPrivateKey, - bool active, - Guid authRequestId, - DateTime authRequestCreationDate) - { - Id = id; - Name = name; - Type = (DeviceType)type; - Identifier = identifier; - CreationDate = creationDate; - IsTrusted = new Device - { - Id = id, - UserId = userId, - Name = name, - Type = (DeviceType)type, - Identifier = identifier, - PushToken = pushToken, - RevisionDate = revisionDate, - EncryptedUserKey = encryptedUserKey, - EncryptedPublicKey = encryptedPublicKey, - EncryptedPrivateKey = encryptedPrivateKey, - Active = active - }.IsTrusted(); - EncryptedPublicKey = encryptedPublicKey; - EncryptedUserKey = encryptedUserKey; - AuthRequestId = authRequestId != Guid.Empty ? authRequestId : null; - AuthRequestCreatedAt = - authRequestCreationDate != DateTime.MinValue ? authRequestCreationDate : null; + AuthRequestCreationDate = authRequestCreationDate; } } diff --git a/src/Sql/dbo/Auth/Stored Procedures/Device_ReadActiveWithPendingAuthRequestsByUserId.sql b/src/Sql/dbo/Auth/Stored Procedures/Device_ReadActiveWithPendingAuthRequestsByUserId.sql index f40e9149c0e2..f995eeec94f3 100644 --- a/src/Sql/dbo/Auth/Stored Procedures/Device_ReadActiveWithPendingAuthRequestsByUserId.sql +++ b/src/Sql/dbo/Auth/Stored Procedures/Device_ReadActiveWithPendingAuthRequestsByUserId.sql @@ -6,25 +6,38 @@ BEGIN SET NOCOUNT ON; SELECT - D.*, - AR.Id as AuthRequestId, - AR.CreationDate as AuthRequestCreationDate - FROM dbo.DeviceView D - LEFT JOIN ( - SELECT - Id, - CreationDate, - RequestDeviceIdentifier, - Approved, - ROW_NUMBER() OVER (PARTITION BY RequestDeviceIdentifier ORDER BY CreationDate DESC) as rn - FROM dbo.AuthRequestView - WHERE Type IN (0, 1) -- AuthenticateAndUnlock and Unlock types only - AND CreationDate >= DATEADD(MINUTE, -@ExpirationMinutes, GETUTCDATE()) -- Ensure the request hasn't expired - AND UserId = @UserId -- Requests for this user only - ) AR -- This join will get the most recent request per device, regardless of approval status - ON D.Identifier = AR.RequestDeviceIdentifier AND AR.rn = 1 AND AR.Approved IS NULL -- Get only the most recent unapproved request per device + D.[Id], + D.[UserId], + D.[Name], + D.[Type], + D.[Identifier], + D.[PushToken], + D.[CreationDate], + D.[RevisionDate], + D.[EncryptedUserKey], + D.[EncryptedPublicKey], + D.[EncryptedPrivateKey], + D.[Active], + AR.[Id] AS [AuthRequestId], + AR.[CreationDate] AS [AuthRequestCreationDate] + FROM + [dbo].[DeviceView] D + LEFT OUTER JOIN ( + SELECT + [Id], + [CreationDate], + [RequestDeviceIdentifier], + [Approved], + ROW_NUMBER() OVER (PARTITION BY [RequestDeviceIdentifier] ORDER BY [CreationDate] DESC) AS rn + FROM + [dbo].[AuthRequestView] + WHERE + [Type] IN (0,1) -- AuthenticateAndUnlock and Unlock types only + AND [CreationDate] >= DATEADD(MINUTE, -@ExpirationMinutes, GETUTCDATE()) -- Ensure the request hasn't expired + AND [UserId] = @UserId -- Requests for this user only + ) AR -- This join will get the most recent request per device, regardless of approval status + ON D.[Identifier] = AR.[RequestDeviceIdentifier] AND AR.[rn] = 1 AND AR.[Approved] IS NULL -- Get only the most recent unapproved request per device WHERE - D.UserId = @UserId -- Include only devices for this user - AND D.Active = 1; -- Include only active devices + D.[UserId] = @UserId -- Include only devices for this user + AND D.[Active] = 1; -- Include only active devices END; - diff --git a/test/Infrastructure.IntegrationTest/Auth/Repositories/DeviceRepositoryTests.cs b/test/Infrastructure.IntegrationTest/Auth/Repositories/DeviceRepositoryTests.cs index 95b88d5662af..a428f12610b5 100644 --- a/test/Infrastructure.IntegrationTest/Auth/Repositories/DeviceRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/Auth/Repositories/DeviceRepositoryTests.cs @@ -9,6 +9,11 @@ namespace Bit.Infrastructure.IntegrationTest.Auth.Repositories; public class DeviceRepositoryTests { + /// + /// Verifies that all DeviceAuthDetails fields are correctly populated from the database, + /// and that when multiple pending auth requests exist for the same device, only the most + /// recent one is returned. + /// [DatabaseTheory] [DatabaseData] public async Task GetManyByUserIdWithDeviceAuth_Works_ReturnsExpectedResults( @@ -32,6 +37,10 @@ public async Task GetManyByUserIdWithDeviceAuth_Works_ReturnsExpectedResults( UserId = user.Id, Type = DeviceType.ChromeBrowser, Identifier = Guid.NewGuid().ToString(), + PushToken = "push-token", + EncryptedUserKey = "encrypted-user-key", + EncryptedPublicKey = "encrypted-public-key", + EncryptedPrivateKey = "encrypted-private-key", }); var staleAuthRequest = await authRequestRepository.CreateAsync(new AuthRequest @@ -66,13 +75,30 @@ public async Task GetManyByUserIdWithDeviceAuth_Works_ReturnsExpectedResults( // Act var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); + var result = response.First(); - // Assert - Assert.NotNull(response.First().AuthRequestId); - Assert.NotNull(response.First().AuthRequestCreatedAt); - Assert.Equal(response.First().AuthRequestId, freshAuthRequest.Id); + // Assert — device fields + Assert.Equal(device.Id, result.Id); + Assert.Equal(device.UserId, result.UserId); + Assert.Equal(device.Name, result.Name); + Assert.Equal(device.Type, result.Type); + Assert.Equal(device.Identifier, result.Identifier); + Assert.Equal(device.PushToken, result.PushToken); + Assert.NotEqual(default, result.CreationDate); + Assert.NotEqual(default, result.RevisionDate); + Assert.Equal(device.EncryptedUserKey, result.EncryptedUserKey); + Assert.Equal(device.EncryptedPublicKey, result.EncryptedPublicKey); + Assert.Equal(device.EncryptedPrivateKey, result.EncryptedPrivateKey); + Assert.Equal(device.Active, result.Active); + // Assert — most recent pending auth request is returned, not the stale one + Assert.Equal(freshAuthRequest.Id, result.AuthRequestId); + Assert.NotNull(result.AuthRequestCreationDate); } + /// + /// Verifies that when two users share the same device identifier, a pending auth request + /// belonging to one user does not appear on the other user's device results. + /// [DatabaseTheory] [DatabaseData] public async Task GetManyByUserIdWithDeviceAuth_WorksWithMultipleUsersOnSameDevice_ReturnsExpectedResults( @@ -135,9 +161,13 @@ public async Task GetManyByUserIdWithDeviceAuth_WorksWithMultipleUsersOnSameDevi // Assert Assert.Null(response.First().AuthRequestId); - Assert.Null(response.First().AuthRequestCreatedAt); + Assert.Null(response.First().AuthRequestCreationDate); } + /// + /// Verifies that all active devices for a user are returned even when none have + /// a pending auth request, and that AuthRequestId is null in that case. + /// [DatabaseTheory] [DatabaseData] public async Task GetManyByUserIdWithDeviceAuth_WorksWithNoAuthRequestAndMultipleDevices_ReturnsExpectedResults( @@ -175,14 +205,66 @@ await sutRepository.CreateAsync(new Device var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); // Assert - Assert.NotNull(response.First()); Assert.Null(response.First().AuthRequestId); - Assert.True(response.Count == 2); + Assert.Equal(2, response.Count); } + /// + /// Verifies that IsTrusted is computed from the presence of all three encrypted keys + /// (EncryptedUserKey, EncryptedPublicKey, EncryptedPrivateKey) and is not a stored column. + /// [DatabaseTheory] [DatabaseData] - public async Task GetManyByUserIdWithDeviceAuth_FailsToRespondWithAnyAuthData_ReturnsEmptyResults( + public async Task GetManyByUserIdWithDeviceAuth_IsTrustedComputedCorrectlyAsync( + IDeviceRepository sutRepository, + IUserRepository userRepository) + { + // Arrange + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + var trustedDevice = await sutRepository.CreateAsync(new Device + { + Active = true, + Name = "trusted-device", + UserId = user.Id, + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + EncryptedUserKey = "encrypted-user-key", + EncryptedPublicKey = "encrypted-public-key", + EncryptedPrivateKey = "encrypted-private-key", + }); + + var untrustedDevice = await sutRepository.CreateAsync(new Device + { + Active = true, + Name = "untrusted-device", + UserId = user.Id, + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + }); + + // Act + var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); + + // Assert — IsTrusted is computed from encrypted keys, not stored as a database column + Assert.True(response.First(d => d.Id == trustedDevice.Id).IsTrusted); + Assert.False(response.First(d => d.Id == untrustedDevice.Id).IsTrusted); + } + + /// + /// Verifies that auth requests which are ineligible — wrong type (AdminApproval), + /// already approved, or expired — do not populate AuthRequestId or AuthRequestCreationDate. + /// The device itself is still returned; only the auth request fields are null. + /// + [DatabaseTheory] + [DatabaseData] + public async Task GetManyByUserIdWithDeviceAuth_IneligibleAuthRequests_ReturnsDeviceWithNullAuthDataAsync( IDeviceRepository sutRepository, IUserRepository userRepository, IAuthRequestRepository authRequestRepository) @@ -191,21 +273,23 @@ public async Task GetManyByUserIdWithDeviceAuth_FailsToRespondWithAnyAuthData_Re { new { - authRequestType = AuthRequestType.AdminApproval, // Device typing is wrong + // Only AuthenticateAndUnlock and Unlock types are eligible for pending auth request matching + // AdminApproval is not eligible, even if it's pending + authRequestType = AuthRequestType.AdminApproval, authRequestApproved = (bool?)null, - expirey = DateTime.UtcNow.AddMinutes(0), + expiry = DateTime.UtcNow.AddMinutes(0), }, new { authRequestType = AuthRequestType.AuthenticateAndUnlock, authRequestApproved = (bool?)true, // Auth request is already approved - expirey = DateTime.UtcNow.AddMinutes(0), + expiry = DateTime.UtcNow.AddMinutes(0), }, new { authRequestType = AuthRequestType.AuthenticateAndUnlock, authRequestApproved = (bool?)null, - expirey = DateTime.UtcNow.AddMinutes(-30), // Past the point of expiring + expiry = DateTime.UtcNow.AddMinutes(-30), // Past the point of expiring } }; @@ -242,15 +326,137 @@ public async Task GetManyByUserIdWithDeviceAuth_FailsToRespondWithAnyAuthData_Re PublicKey = "PublicKey_1234" }); - authRequest.CreationDate = testCase.expirey; + authRequest.CreationDate = testCase.expiry; await authRequestRepository.ReplaceAsync(authRequest); // Act var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); - // Assert + // Assert — device is returned but with no auth request data + Assert.Single(response); Assert.Null(response.First().AuthRequestId); - Assert.Null(response.First().AuthRequestCreatedAt); + Assert.Null(response.First().AuthRequestCreationDate); } } + + /// + /// Verifies that the Unlock auth request type is treated as a valid pending request + /// and populates AuthRequestId and AuthRequestCreationDate on the device response. + /// + [DatabaseTheory] + [DatabaseData] + public async Task GetManyByUserIdWithDeviceAuth_UnlockAuthRequestType_ReturnsAuthDataAsync( + IDeviceRepository sutRepository, + IUserRepository userRepository, + IAuthRequestRepository authRequestRepository) + { + // Arrange + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + var device = await sutRepository.CreateAsync(new Device + { + Active = true, + Name = "chrome-test", + UserId = user.Id, + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + }); + + var authRequest = await authRequestRepository.CreateAsync(new AuthRequest + { + ResponseDeviceId = null, + Approved = null, + Type = AuthRequestType.Unlock, + OrganizationId = null, + UserId = user.Id, + RequestIpAddress = ":1", + RequestDeviceIdentifier = device.Identifier, + AccessCode = "AccessCode_1234", + PublicKey = "PublicKey_1234", + }); + + // Act + var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); + + // Assert — Unlock type (1) is treated as a valid pending auth request type and populates auth data on the device response + Assert.Equal(authRequest.Id, response.First().AuthRequestId); + Assert.NotNull(response.First().AuthRequestCreationDate); + } + + /// + /// Verifies that devices with Active = false are excluded from results. Only active + /// devices should be returned, regardless of whether they have a pending auth request. + /// + [Theory] + [DatabaseData] + public async Task GetManyByUserIdWithDeviceAuth_InactiveDevice_IsExcludedFromResultsAsync( + IDeviceRepository sutRepository, + IUserRepository userRepository) + { + // Arrange + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + var activeDevice = await sutRepository.CreateAsync(new Device + { + Active = true, + Name = "active-device", + UserId = user.Id, + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + }); + + await sutRepository.CreateAsync(new Device + { + Active = false, + Name = "inactive-device", + UserId = user.Id, + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + }); + + // Act + var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); + + // Assert — only the active device is returned + Assert.Single(response); + Assert.Equal(activeDevice.Id, response.First().Id); + } + + /// + /// Verifies that a user with no registered devices receives an empty collection, + /// not null or an error. + /// + [Theory] + [DatabaseData] + public async Task GetManyByUserIdWithDeviceAuth_UserWithNoDevices_ReturnsEmptyListAsync( + IDeviceRepository sutRepository, + IUserRepository userRepository) + { + // Arrange + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + // Act + var response = await sutRepository.GetManyByUserIdWithDeviceAuth(user.Id); + + // Assert + Assert.Empty(response); + } } diff --git a/util/Migrator/DbScripts/2026-04-10_01_Alter_Device_ReadActiveWithPendingAuthRequestsByUserId.sql b/util/Migrator/DbScripts/2026-04-10_01_Alter_Device_ReadActiveWithPendingAuthRequestsByUserId.sql new file mode 100644 index 000000000000..bff09141715a --- /dev/null +++ b/util/Migrator/DbScripts/2026-04-10_01_Alter_Device_ReadActiveWithPendingAuthRequestsByUserId.sql @@ -0,0 +1,51 @@ +-- PM-34130: Replace SELECT D.* with an explicit column list. +-- Previously, Dapper selected the 14-parameter constructor and mapped columns +-- positionally. A column addition, removal, or reorder in DeviceView would +-- silently assign wrong values with no compile or runtime error. +-- DeviceAuthDetails now has a parameterless constructor, so Dapper maps by +-- property name instead. The explicit column list documents intent and prevents +-- unexpected columns from DeviceView leaking into the result. +CREATE OR ALTER PROCEDURE [dbo].[Device_ReadActiveWithPendingAuthRequestsByUserId] + @UserId UNIQUEIDENTIFIER, + @ExpirationMinutes INT +AS +BEGIN + SET NOCOUNT ON; + + SELECT + D.[Id], + D.[UserId], + D.[Name], + D.[Type], + D.[Identifier], + D.[PushToken], + D.[CreationDate], + D.[RevisionDate], + D.[EncryptedUserKey], + D.[EncryptedPublicKey], + D.[EncryptedPrivateKey], + D.[Active], + AR.[Id] AS [AuthRequestId], + AR.[CreationDate] AS [AuthRequestCreationDate] + FROM + [dbo].[DeviceView] D + LEFT OUTER JOIN ( + SELECT + [Id], + [CreationDate], + [RequestDeviceIdentifier], + [Approved], + ROW_NUMBER() OVER (PARTITION BY [RequestDeviceIdentifier] ORDER BY [CreationDate] DESC) AS rn + FROM + [dbo].[AuthRequestView] + WHERE + [Type] IN (0,1) -- AuthenticateAndUnlock and Unlock types only + AND [CreationDate] >= DATEADD(MINUTE, -@ExpirationMinutes, GETUTCDATE()) -- Ensure the request hasn't expired + AND [UserId] = @UserId -- Requests for this user only + ) AR -- This join will get the most recent request per device, regardless of approval status + ON D.[Identifier] = AR.[RequestDeviceIdentifier] AND AR.[rn] = 1 AND AR.[Approved] IS NULL -- Get only the most recent unapproved request per device + WHERE + D.[UserId] = @UserId -- Include only devices for this user + AND D.[Active] = 1; -- Include only active devices +END; +GO