diff --git a/src/Admin/Auth/Jobs/DeleteAuthRequestsJob.cs b/src/Admin/Auth/Jobs/DeleteAuthRequestsJob.cs index e1366ec24..71c2bfe77 100644 --- a/src/Admin/Auth/Jobs/DeleteAuthRequestsJob.cs +++ b/src/Admin/Auth/Jobs/DeleteAuthRequestsJob.cs @@ -1,6 +1,7 @@ using Bit.Core; using Bit.Core.Jobs; using Bit.Core.Repositories; +using Bit.Core.Settings; using Quartz; namespace Bit.Admin.Auth.Jobs; @@ -8,20 +9,26 @@ namespace Bit.Admin.Auth.Jobs; public class DeleteAuthRequestsJob : BaseJob { private readonly IAuthRequestRepository _authRepo; + private readonly IGlobalSettings _globalSettings; public DeleteAuthRequestsJob( IAuthRequestRepository authrepo, + IGlobalSettings globalSettings, ILogger logger) : base(logger) { _authRepo = authrepo; + _globalSettings = globalSettings; } protected async override Task ExecuteJobAsync(IJobExecutionContext context) { _logger.LogInformation(Constants.BypassFiltersEventId, "Execute job task: DeleteAuthRequestsJob: Start"); - var count = await _authRepo.DeleteExpiredAsync(); - _logger.LogInformation(Constants.BypassFiltersEventId, $"{count} records deleted from AuthRequests."); + var count = await _authRepo.DeleteExpiredAsync( + _globalSettings.PasswordlessAuth.UserRequestExpiration, + _globalSettings.PasswordlessAuth.AdminRequestExpiration, + _globalSettings.PasswordlessAuth.AfterAdminApprovalExpiration); + _logger.LogInformation(Constants.BypassFiltersEventId, "{Count} records deleted from AuthRequests.", count); _logger.LogInformation(Constants.BypassFiltersEventId, "Execute job task: DeleteAuthRequestsJob: End"); } } diff --git a/src/Core/Auth/Repositories/IAuthRequestRepository.cs b/src/Core/Auth/Repositories/IAuthRequestRepository.cs index 9b6624acd..b414b2206 100644 --- a/src/Core/Auth/Repositories/IAuthRequestRepository.cs +++ b/src/Core/Auth/Repositories/IAuthRequestRepository.cs @@ -5,7 +5,7 @@ namespace Bit.Core.Repositories; public interface IAuthRequestRepository : IRepository { - Task DeleteExpiredAsync(); + Task DeleteExpiredAsync(TimeSpan userRequestExpiration, TimeSpan adminRequestExpiration, TimeSpan afterAdminApprovalExpiration); Task> GetManyByUserIdAsync(Guid userId); Task> GetManyPendingByOrganizationIdAsync(Guid organizationId); Task> GetManyAdminApprovalRequestsByManyIdsAsync(Guid organizationId, IEnumerable ids); diff --git a/src/Core/Auth/Settings/IPasswordlessAuthSettings.cs b/src/Core/Auth/Settings/IPasswordlessAuthSettings.cs index c1437a5c9..88b2f75f8 100644 --- a/src/Core/Auth/Settings/IPasswordlessAuthSettings.cs +++ b/src/Core/Auth/Settings/IPasswordlessAuthSettings.cs @@ -3,4 +3,7 @@ public interface IPasswordlessAuthSettings { bool KnownDevicesOnly { get; set; } + TimeSpan UserRequestExpiration { get; set; } + TimeSpan AdminRequestExpiration { get; set; } + TimeSpan AfterAdminApprovalExpiration { get; set; } } diff --git a/src/Core/Settings/GlobalSettings.cs b/src/Core/Settings/GlobalSettings.cs index c2b7b4d45..3c3d3e0c5 100644 --- a/src/Core/Settings/GlobalSettings.cs +++ b/src/Core/Settings/GlobalSettings.cs @@ -534,6 +534,9 @@ public class GlobalSettings : IGlobalSettings public class PasswordlessAuthSettings : IPasswordlessAuthSettings { public bool KnownDevicesOnly { get; set; } = true; + public TimeSpan UserRequestExpiration { get; set; } = TimeSpan.FromMinutes(15); + public TimeSpan AdminRequestExpiration { get; set; } = TimeSpan.FromDays(7); + public TimeSpan AfterAdminApprovalExpiration { get; set; } = TimeSpan.FromHours(12); } public class DomainVerificationSettings : IDomainVerificationSettings diff --git a/src/Infrastructure.Dapper/Auth/Repositories/AuthRequestRepository.cs b/src/Infrastructure.Dapper/Auth/Repositories/AuthRequestRepository.cs index 107d16802..67e636b4d 100644 --- a/src/Infrastructure.Dapper/Auth/Repositories/AuthRequestRepository.cs +++ b/src/Infrastructure.Dapper/Auth/Repositories/AuthRequestRepository.cs @@ -19,13 +19,19 @@ public class AuthRequestRepository : Repository, IAuthRequest : base(connectionString, readOnlyConnectionString) { } - public async Task DeleteExpiredAsync() + public async Task DeleteExpiredAsync( + TimeSpan userRequestExpiration, TimeSpan adminRequestExpiration, TimeSpan afterAdminApprovalExpiration) { using (var connection = new SqlConnection(ConnectionString)) { return await connection.ExecuteAsync( $"[{Schema}].[AuthRequest_DeleteIfExpired]", - null, + new + { + UserExpirationSeconds = (int)userRequestExpiration.TotalSeconds, + AdminExpirationSeconds = (int)adminRequestExpiration.TotalSeconds, + AdminApprovalExpirationSeconds = (int)afterAdminApprovalExpiration.TotalSeconds, + }, commandType: CommandType.StoredProcedure); } } diff --git a/src/Infrastructure.EntityFramework/Auth/Repositories/AuthRequestRepository.cs b/src/Infrastructure.EntityFramework/Auth/Repositories/AuthRequestRepository.cs index 220019e8b..af3ae195d 100644 --- a/src/Infrastructure.EntityFramework/Auth/Repositories/AuthRequestRepository.cs +++ b/src/Infrastructure.EntityFramework/Auth/Repositories/AuthRequestRepository.cs @@ -15,15 +15,20 @@ public class AuthRequestRepository : Repository context.AuthRequests) { } - public async Task DeleteExpiredAsync() + public async Task DeleteExpiredAsync( + TimeSpan userRequestExpiration, TimeSpan adminRequestExpiration, TimeSpan afterAdminApprovalExpiration) { + using (var scope = ServiceScopeFactory.CreateScope()) { var dbContext = GetDatabaseContext(scope); - var expiredRequests = await dbContext.AuthRequests.Where(a => a.CreationDate < DateTime.Now.AddMinutes(-15)).ToListAsync(); + var expiredRequests = await dbContext.AuthRequests + .Where(a => (a.Type != AuthRequestType.AdminApproval && a.CreationDate.AddSeconds(userRequestExpiration.TotalSeconds) < DateTime.UtcNow) + || (a.Type == AuthRequestType.AdminApproval && a.Approved != true && a.CreationDate.AddSeconds(adminRequestExpiration.TotalSeconds) < DateTime.UtcNow) + || (a.Type == AuthRequestType.AdminApproval && a.Approved == true && a.ResponseDate.Value.AddSeconds(afterAdminApprovalExpiration.TotalSeconds) < DateTime.UtcNow)) + .ToListAsync(); dbContext.AuthRequests.RemoveRange(expiredRequests); - await dbContext.SaveChangesAsync(); - return 1; + return await dbContext.SaveChangesAsync(); } } diff --git a/src/Sql/Auth/dbo/Stored Procedures/AuthRequest_DeleteIfExpired.sql b/src/Sql/Auth/dbo/Stored Procedures/AuthRequest_DeleteIfExpired.sql index 736729c7b..f471585ae 100644 --- a/src/Sql/Auth/dbo/Stored Procedures/AuthRequest_DeleteIfExpired.sql +++ b/src/Sql/Auth/dbo/Stored Procedures/AuthRequest_DeleteIfExpired.sql @@ -1,6 +1,19 @@ -CREATE PROCEDURE [dbo].[AuthRequest_DeleteIfExpired] +-- UserExpirationSeconds to 15 minutes (15 * 60) +-- AdminExpirationSeconds to 7 days (7 * 24 * 60 * 60) +-- AdminApprovalExpirationSeconds to 12 hour (12 * 60 * 60) + +CREATE PROCEDURE [dbo].[AuthRequest_DeleteIfExpired] + @UserExpirationSeconds INT = 900, + @AdminExpirationSeconds INT = 604800, + @AdminApprovalExpirationSeconds INT = 43200 AS BEGIN SET NOCOUNT OFF - DELETE FROM [dbo].[AuthRequest] WHERE [CreationDate] < DATEADD(minute, -15, GETUTCDATE()); + DELETE FROM [dbo].[AuthRequest] + -- User requests expire after 15 minutes (by default) of their creation + WHERE ([Type] != 2 AND DATEADD(second, @UserExpirationSeconds, [CreationDate]) < GETUTCDATE()) + -- Admin requests expire after 7 days (by default) of their creation if they have not been approved + OR ([Type] = 2 AND ([Approved] IS NULL OR [Approved] = 0) AND DATEADD(second, @AdminExpirationSeconds,[CreationDate]) < GETUTCDATE()) + -- Admin requests expire after 12 hours (by default) of their approval + OR ([Type] = 2 AND [Approved] = 1 AND DATEADD(second, @AdminApprovalExpirationSeconds, [ResponseDate]) < GETUTCDATE()); END diff --git a/test/Infrastructure.IntegrationTest/Auth/Repositories/AuthRequestRepositoryTests.cs b/test/Infrastructure.IntegrationTest/Auth/Repositories/AuthRequestRepositoryTests.cs new file mode 100644 index 000000000..b23b8ce4b --- /dev/null +++ b/test/Infrastructure.IntegrationTest/Auth/Repositories/AuthRequestRepositoryTests.cs @@ -0,0 +1,99 @@ +using Bit.Core.Auth.Entities; +using Bit.Core.Auth.Enums; +using Bit.Core.Entities; +using Bit.Core.Repositories; +using Xunit; + +namespace Bit.Infrastructure.IntegrationTest.Auth.Repositories; + +public class AuthRequestRepositoryTests +{ + private readonly static TimeSpan _userRequestExpiration = TimeSpan.FromMinutes(15); + private readonly static TimeSpan _adminRequestExpiration = TimeSpan.FromDays(6); + private readonly static TimeSpan _afterAdminApprovalExpiration = TimeSpan.FromHours(12); + + [DatabaseTheory, DatabaseData] + public async Task DeleteExpiredAsync_Works( + IAuthRequestRepository authRequestRepository, + IUserRepository userRepository, + ITestDatabaseHelper helper) + { + var user = await userRepository.CreateAsync(new User + { + Name = "Test User", + Email = $"test+{Guid.NewGuid()}@email.com", + ApiKey = "TEST", + SecurityStamp = "stamp", + }); + + // A user auth request type that has passed it's expiration time, should be deleted. + var userExpiredAuthRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.AuthenticateAndUnlock, CreateExpiredDate(_userRequestExpiration))); + + // An AdminApproval request that hasn't had any action taken on it and has passed it's expiration time, should be deleted. + var adminApprovalExpiredAuthRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.AdminApproval, CreateExpiredDate(_adminRequestExpiration))); + + // An AdminApproval request that was approved before it expired but the user has been approved for too long, should be deleted. + var adminApprovedExpiredAuthRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.AdminApproval, DateTime.UtcNow.AddDays(-6), true, CreateExpiredDate(_afterAdminApprovalExpiration))); + + // An AdminApproval request that was rejected within it's allowed lifetime but has no gone past it's expiration time, should be deleted. + var adminRejectedExpiredAuthRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.AdminApproval, CreateExpiredDate(_adminRequestExpiration), false, DateTime.UtcNow.AddHours(-1))); + + // A User AuthRequest that was created just a minute ago. + var notExpiredUserAuthRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.Unlock, DateTime.UtcNow.AddMinutes(-1))); + + // An AdminApproval AuthRequest that was create 6 days 23 hours 59 minutes 59 seconds ago which is right on the edge of still being valid + var notExpiredAdminApprovalRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.AdminApproval, DateTime.UtcNow.Add(new TimeSpan(days: 6, hours: 23, minutes: 59, seconds: 59)))); + + // An AdminApproval AuthRequest that was created a week ago but just approved 11 hours ago. + var notExpiredApprovedAdminApprovalRequest = await authRequestRepository.CreateAsync( + CreateAuthRequest(user.Id, AuthRequestType.AdminApproval, DateTime.UtcNow.AddDays(7), true, DateTime.UtcNow.AddHours(11))); + + helper.ClearTracker(); + + var numberOfDeleted = await authRequestRepository.DeleteExpiredAsync(_userRequestExpiration, _adminRequestExpiration, _afterAdminApprovalExpiration); + + // Ensure all the AuthRequests that should have been deleted, have been deleted. + Assert.Null(await authRequestRepository.GetByIdAsync(userExpiredAuthRequest.Id)); + Assert.Null(await authRequestRepository.GetByIdAsync(adminApprovalExpiredAuthRequest.Id)); + Assert.Null(await authRequestRepository.GetByIdAsync(adminApprovedExpiredAuthRequest.Id)); + Assert.Null(await authRequestRepository.GetByIdAsync(adminRejectedExpiredAuthRequest.Id)); + + // Ensure that all the AuthRequests that should have been left alone, were. + Assert.NotNull(await authRequestRepository.GetByIdAsync(notExpiredUserAuthRequest.Id)); + Assert.NotNull(await authRequestRepository.GetByIdAsync(notExpiredAdminApprovalRequest.Id)); + Assert.NotNull(await authRequestRepository.GetByIdAsync(notExpiredApprovedAdminApprovalRequest.Id)); + + // Ensure the repository responds with the amount of items it deleted and it deleted the right amount. + // NOTE: On local development this might fail on it's first run because the developer could have expired AuthRequests + // on their machine but aren't running the job that would delete them. The second run of this test should succeed. + Assert.Equal(4, numberOfDeleted); + } + + private static AuthRequest CreateAuthRequest(Guid userId, AuthRequestType authRequestType, DateTime creationDate, bool? approved = null, DateTime? responseDate = null) + { + return new AuthRequest + { + UserId = userId, + Type = authRequestType, + Approved = approved, + RequestDeviceIdentifier = "something", // TODO: EF Doesn't enforce this as not null + RequestIpAddress = "1.1.1.1", // TODO: EF Doesn't enforce this as not null + AccessCode = "test_access_code", // TODO: EF Doesn't enforce this as not null + PublicKey = "test_public_key", // TODO: EF Doesn't enforce this as not null + CreationDate = creationDate, + ResponseDate = responseDate, + }; + } + + private static DateTime CreateExpiredDate(TimeSpan expirationPeriod) + { + var exp = expirationPeriod + TimeSpan.FromMinutes(1); + return DateTime.UtcNow.Add(exp.Negate()); + } +} diff --git a/util/Migrator/DbScripts/2023-06-27_00_AuthRequestExpirationUpdates.sql b/util/Migrator/DbScripts/2023-06-27_00_AuthRequestExpirationUpdates.sql new file mode 100644 index 000000000..cc6988f43 --- /dev/null +++ b/util/Migrator/DbScripts/2023-06-27_00_AuthRequestExpirationUpdates.sql @@ -0,0 +1,25 @@ +IF OBJECT_ID('[dbo].[AuthRequest_DeleteIfExpired]') IS NOT NULL + BEGIN + DROP PROCEDURE [dbo].[AuthRequest_DeleteIfExpired] + END +GO + +-- UserExpirationSeconds to 15 minutes (15 * 60) +-- AdminExpirationSeconds to 7 days (7 * 24 * 60 * 60) +-- AdminApprovalExpirationSeconds to 12 hour (12 * 60 * 60) + +CREATE PROCEDURE [dbo].[AuthRequest_DeleteIfExpired] + @UserExpirationSeconds INT = 900, + @AdminExpirationSeconds INT = 604800, + @AdminApprovalExpirationSeconds INT = 43200 +AS +BEGIN + SET NOCOUNT OFF + DELETE FROM [dbo].[AuthRequest] + -- User requests expire after 15 minutes (by default) of their creation + WHERE ([Type] != 2 AND DATEADD(second, @UserExpirationSeconds, [CreationDate]) < GETUTCDATE()) + -- Admin requests expire after 7 days (by default) of their creation if they have not been approved + OR ([Type] = 2 AND ([Approved] IS NULL OR [Approved] = 0) AND DATEADD(second, @AdminExpirationSeconds,[CreationDate]) < GETUTCDATE()) + -- Admin requests expire after 12 hours (by default) of their approval + OR ([Type] = 2 AND [Approved] = 1 AND DATEADD(second, @AdminApprovalExpirationSeconds, [ResponseDate]) < GETUTCDATE()); +END