From 833e181a5a198b65b61af65bcdc96f43be32d76a Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Tue, 18 Nov 2025 15:58:42 -0800 Subject: [PATCH] Address pr feedback --- src/Core/Entities/PlayData.cs | 21 +- src/Core/Services/IPlayDataService.cs | 24 ++ src/Core/Services/IPlayIdService.cs | 16 ++ .../Implementations/PlayDataService.cs | 26 ++ .../Repositories/OrganizationRepository.cs | 19 +- .../Repositories/UserRepository.cs | 25 +- .../Repositories/OrganizationRepository.cs | 28 +-- .../Repositories/UserRepository.cs | 19 +- src/SharedWeb/Utilities/PlayIdMiddleware.cs | 6 + .../Services/PlayDataServiceTests.cs | 143 +++++++++++ test/Core.Test/Services/PlayIdServiceTests.cs | 232 ++++++++++++++++++ 11 files changed, 491 insertions(+), 68 deletions(-) create mode 100644 src/Core/Services/IPlayDataService.cs create mode 100644 src/Core/Services/Implementations/PlayDataService.cs create mode 100644 test/Core.Test/Services/PlayDataServiceTests.cs create mode 100644 test/Core.Test/Services/PlayIdServiceTests.cs diff --git a/src/Core/Entities/PlayData.cs b/src/Core/Entities/PlayData.cs index e56cb654bc..623922516b 100644 --- a/src/Core/Entities/PlayData.cs +++ b/src/Core/Entities/PlayData.cs @@ -4,6 +4,11 @@ using Bit.Core.Utilities; namespace Bit.Core.Entities; +/// +/// PlayData is a join table tracking entities created during automated testing. +/// A `PlayId` is supplied by the clients in the `x-play-id` header to inform the server +/// that any data created should be associated with the play, and therefore cleaned up with it. +/// public class PlayData : ITableObject { public Guid Id { get; set; } @@ -12,14 +17,22 @@ public class PlayData : ITableObject public Guid? UserId { get; init; } public Guid? OrganizationId { get; init; } public DateTime CreationDate { get; init; } - protected PlayData() { } + /// + /// Generates and sets a new COMB GUID for the Id property. + /// public void SetNewId() { Id = CoreHelpers.GenerateComb(); } + /// + /// Creates a new PlayData record associated with a User. + /// + /// The user entity created during the play. + /// The play identifier from the x-play-id header. + /// A new PlayData instance tracking the user. public static PlayData Create(User user, string playId) { return new PlayData @@ -30,6 +43,12 @@ public class PlayData : ITableObject }; } + /// + /// Creates a new PlayData record associated with an Organization. + /// + /// The organization entity created during the play. + /// The play identifier from the x-play-id header. + /// A new PlayData instance tracking the organization. public static PlayData Create(Organization organization, string playId) { return new PlayData diff --git a/src/Core/Services/IPlayDataService.cs b/src/Core/Services/IPlayDataService.cs new file mode 100644 index 0000000000..9f3e33a17d --- /dev/null +++ b/src/Core/Services/IPlayDataService.cs @@ -0,0 +1,24 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Entities; + +namespace Bit.Core.Services; + +public interface IPlayDataService +{ + /// + /// Records a PlayData entry for the given User created during a Play session. + /// + /// Does nothing if no Play Id is set for this http scope. + /// + /// + /// + Task Record(User user); + /// + /// Records a PlayData entry for the given Organization created during a Play session. + /// + /// Does nothing if no Play Id is set for this http scope. + /// + /// + /// + Task Record(Organization organization); +} diff --git a/src/Core/Services/IPlayIdService.cs b/src/Core/Services/IPlayIdService.cs index d0fcbeef84..ac1fa4b293 100644 --- a/src/Core/Services/IPlayIdService.cs +++ b/src/Core/Services/IPlayIdService.cs @@ -1,7 +1,23 @@ namespace Bit.Core.Services; +/// +/// Service for managing Play identifiers in automated testing infrastructure. +/// A "Play" is a test session that groups entities created during testing to enable cleanup. +/// The PlayId flows from client request (x-play-id header) through PlayIdMiddleware to this service, +/// which repositories query to create PlayData tracking records via IPlayDataService. The SeederAPI uses these records +/// to bulk delete all entities associated with a PlayId. Only active in Development environments. +/// public interface IPlayIdService { + /// + /// Gets or sets the current Play identifier from the x-play-id request header. + /// string? PlayId { get; set; } + + /// + /// Checks whether the current request is part of an active Play session. + /// + /// The Play identifier if active, otherwise empty string. + /// True if in a Play session (has PlayId and in Development environment), otherwise false. bool InPlay(out string playId); } diff --git a/src/Core/Services/Implementations/PlayDataService.cs b/src/Core/Services/Implementations/PlayDataService.cs new file mode 100644 index 0000000000..612ca3fd85 --- /dev/null +++ b/src/Core/Services/Implementations/PlayDataService.cs @@ -0,0 +1,26 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Entities; +using Bit.Core.Repositories; +using Microsoft.Extensions.Logging; + +namespace Bit.Core.Services; + +public class PlayDataService(IPlayIdService playIdService, IPlayDataRepository playDataRepository, ILogger logger) : IPlayDataService +{ + public async Task Record(User user) + { + if (playIdService.InPlay(out var playId)) + { + logger.LogInformation("Associating user {UserId} with Play ID {PlayId}", user.Id, playId); + await playDataRepository.CreateAsync(PlayData.Create(user, playId)); + } + } + public async Task Record(Organization organization) + { + if (playIdService.InPlay(out var playId)) + { + logger.LogInformation("Associating organization {OrganizationId} with Play ID {PlayId}", organization.Id, playId); + await playDataRepository.CreateAsync(PlayData.Create(organization, playId)); + } + } +} diff --git a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs index 4d8557194d..de16439c9e 100644 --- a/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs +++ b/src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs @@ -256,32 +256,21 @@ public class OrganizationRepository : Repository, IOrganizat public class TestOrganizationTrackingOrganizationRepository : OrganizationRepository { - private readonly IPlayIdService _playIdService; - private readonly IPlayDataRepository _playDataRepository; + private readonly IPlayDataService _playDataService; public TestOrganizationTrackingOrganizationRepository( - IPlayIdService playIdService, - IPlayDataRepository playDataRepository, + IPlayDataService playDataService, GlobalSettings globalSettings, ILogger logger) : base(globalSettings, logger) { - _playIdService = playIdService; - _playDataRepository = playDataRepository; + _playDataService = playDataService; } public override async Task CreateAsync(Organization obj) { var createdOrganization = await base.CreateAsync(obj); - - if (_playIdService.InPlay(out var playId)) - { - _logger.LogInformation("Associating organization {OrganizationId} with Play ID {PlayId}", - createdOrganization.Id, playId); - - await _playDataRepository.CreateAsync(PlayData.Create(createdOrganization, playId)); - } - + await _playDataService.Record(createdOrganization); return createdOrganization; } } diff --git a/src/Infrastructure.Dapper/Repositories/UserRepository.cs b/src/Infrastructure.Dapper/Repositories/UserRepository.cs index fc25ef65f3..8ec845e645 100644 --- a/src/Infrastructure.Dapper/Repositories/UserRepository.cs +++ b/src/Infrastructure.Dapper/Repositories/UserRepository.cs @@ -404,32 +404,23 @@ public class UserRepository : Repository, IUserRepository public class TestUserTrackingUserRepository : UserRepository { - private readonly IPlayIdService _playIdService; - private readonly IPlayDataRepository _playDataRepository; + private readonly IPlayDataService _playDataService; public TestUserTrackingUserRepository( - IPlayIdService playIdService, - GlobalSettings globalSettings, - IPlayDataRepository playDataRepository, - IDataProtectionProvider dataProtectionProvider, - ILogger logger) - : base(dataProtectionProvider, globalSettings, logger) + IPlayDataService playDataService, + GlobalSettings globalSettings, + IDataProtectionProvider dataProtectionProvider, + ILogger logger) + : base(dataProtectionProvider, globalSettings, logger) { - _playIdService = playIdService; - _playDataRepository = playDataRepository; + _playDataService = playDataService; } public override async Task CreateAsync(User user) { var createdUser = await base.CreateAsync(user); - if (_playIdService.InPlay(out var playId)) - { - _logger.LogInformation("Associating user {UserId} with Play ID {PlayId}", - user.Id, playId); - - await _playDataRepository.CreateAsync(PlayData.Create(createdUser, playId)); - } + await _playDataService.Record(createdUser); return createdUser; } } diff --git a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs index 2177c4a221..b2e28aecd3 100644 --- a/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs +++ b/src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs @@ -443,34 +443,22 @@ public class OrganizationRepository : Repository logger, - IPlayIdService playIdService, - IPlayDataRepository playDataRepository) - : base(serviceScopeFactory, mapper, logger) + IPlayDataService playDataService, + IServiceScopeFactory serviceScopeFactory, + IMapper mapper, + ILogger logger) + : base(serviceScopeFactory, mapper, logger) { - _playIdService = playIdService; - _playDataRepository = playDataRepository; - + _playDataService = playDataService; } public override async Task CreateAsync(Core.AdminConsole.Entities.Organization organization) { var createdOrganization = await base.CreateAsync(organization); - - if (_playIdService.InPlay(out var playId)) - { - _logger.LogInformation("Associating organization {OrganizationId} with Play ID {PlayId}", - organization.Id, playId); - - await _playDataRepository.CreateAsync(Core.Entities.PlayData.Create(organization, playId)); - } - + await _playDataService.Record(createdOrganization); return createdOrganization; } } diff --git a/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs b/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs index b820814e3c..2614a77b63 100644 --- a/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs +++ b/src/Infrastructure.EntityFramework/Repositories/UserRepository.cs @@ -404,33 +404,22 @@ public class UserRepository : Repository, IUserR public class TestUserTrackingUserRepository : UserRepository { - private readonly IPlayIdService _playIdService; - private readonly IPlayDataRepository _playDataRepository; + private readonly IPlayDataService _playDataService; public TestUserTrackingUserRepository( - IPlayIdService playIdService, - IPlayDataRepository playDataRepository, + IPlayDataService playDataService, IServiceScopeFactory serviceScopeFactory, IMapper mapper, ILogger logger) : base(serviceScopeFactory, mapper, logger) { - _playIdService = playIdService; - _playDataRepository = playDataRepository; + _playDataService = playDataService; } public override async Task CreateAsync(Core.Entities.User user) { var createdUser = await base.CreateAsync(user); - - if (_playIdService.InPlay(out var playId)) - { - _logger.LogInformation("Associating user {UserId} with Play ID {PlayId}", - user.Id, playId); - - await _playDataRepository.CreateAsync(Core.Entities.PlayData.Create(user, playId)); - } - + await _playDataService.Record(createdUser); return createdUser; } } diff --git a/src/SharedWeb/Utilities/PlayIdMiddleware.cs b/src/SharedWeb/Utilities/PlayIdMiddleware.cs index 848d2ec50a..3f692e6ae9 100644 --- a/src/SharedWeb/Utilities/PlayIdMiddleware.cs +++ b/src/SharedWeb/Utilities/PlayIdMiddleware.cs @@ -3,6 +3,12 @@ using Microsoft.AspNetCore.Http; namespace Bit.SharedWeb.Utilities; +/// +/// Middleware to extract the x-play-id header and set it in the PlayIdService. +/// +/// PlayId is used in testing infrastructure to track data created during automated testing and fa cilitate cleanup. +/// +/// public sealed class PlayIdMiddleware(RequestDelegate next) { public Task Invoke(HttpContext context, PlayIdService playIdService) diff --git a/test/Core.Test/Services/PlayDataServiceTests.cs b/test/Core.Test/Services/PlayDataServiceTests.cs new file mode 100644 index 0000000000..83219451b2 --- /dev/null +++ b/test/Core.Test/Services/PlayDataServiceTests.cs @@ -0,0 +1,143 @@ +using Bit.Core.AdminConsole.Entities; +using Bit.Core.Entities; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.Services; + +[SutProviderCustomize] +public class PlayDataServiceTests +{ + [Theory] + [BitAutoData] + public async Task Record_User_WhenInPlay_RecordsPlayData( + string playId, + User user, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .InPlay(out Arg.Any()) + .Returns(x => + { + x[0] = playId; + return true; + }); + + await sutProvider.Sut.Record(user); + + await sutProvider.GetDependency() + .Received(1) + .CreateAsync(Arg.Is(pd => + pd.PlayId == playId && + pd.UserId == user.Id && + pd.OrganizationId == null)); + + sutProvider.GetDependency>() + .Received(1) + .Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => o.ToString().Contains(user.Id.ToString()) && o.ToString().Contains(playId)), + null, + Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async Task Record_User_WhenNotInPlay_DoesNotRecordPlayData( + User user, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .InPlay(out Arg.Any()) + .Returns(x => + { + x[0] = null; + return false; + }); + + await sutProvider.Sut.Record(user); + + await sutProvider.GetDependency() + .DidNotReceive() + .CreateAsync(Arg.Any()); + + sutProvider.GetDependency>() + .DidNotReceive() + .Log( + LogLevel.Information, + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async Task Record_Organization_WhenInPlay_RecordsPlayData( + string playId, + Organization organization, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .InPlay(out Arg.Any()) + .Returns(x => + { + x[0] = playId; + return true; + }); + + await sutProvider.Sut.Record(organization); + + await sutProvider.GetDependency() + .Received(1) + .CreateAsync(Arg.Is(pd => + pd.PlayId == playId && + pd.OrganizationId == organization.Id && + pd.UserId == null)); + + sutProvider.GetDependency>() + .Received(1) + .Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => o.ToString().Contains(organization.Id.ToString()) && o.ToString().Contains(playId)), + null, + Arg.Any>()); + } + + [Theory] + [BitAutoData] + public async Task Record_Organization_WhenNotInPlay_DoesNotRecordPlayData( + Organization organization, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .InPlay(out Arg.Any()) + .Returns(x => + { + x[0] = null; + return false; + }); + + await sutProvider.Sut.Record(organization); + + await sutProvider.GetDependency() + .DidNotReceive() + .CreateAsync(Arg.Any()); + + sutProvider.GetDependency>() + .DidNotReceive() + .Log( + LogLevel.Information, + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any>()); + } +} diff --git a/test/Core.Test/Services/PlayIdServiceTests.cs b/test/Core.Test/Services/PlayIdServiceTests.cs new file mode 100644 index 0000000000..5e3bed5966 --- /dev/null +++ b/test/Core.Test/Services/PlayIdServiceTests.cs @@ -0,0 +1,232 @@ +using Bit.Core.Services; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.Services; + +[SutProviderCustomize] +public class PlayIdServiceTests +{ + [Theory] + [BitAutoData] + public void InPlay_WhenPlayIdSetAndDevelopment_ReturnsTrue( + string playId, + SutProvider sutProvider) + { + sutProvider.GetDependency().IsDevelopment().Returns(true); + sutProvider.Sut.PlayId = playId; + + var result = sutProvider.Sut.InPlay(out var resultPlayId); + + Assert.True(result); + Assert.Equal(playId, resultPlayId); + } + + [Theory] + [BitAutoData] + public void InPlay_WhenPlayIdSetButNotDevelopment_ReturnsFalse( + string playId, + SutProvider sutProvider) + { + sutProvider.GetDependency().IsDevelopment().Returns(false); + sutProvider.Sut.PlayId = playId; + + var result = sutProvider.Sut.InPlay(out var resultPlayId); + + Assert.False(result); + Assert.Empty(resultPlayId); + } + + [Theory] + [BitAutoData((string?)null)] + [BitAutoData("")] + public void InPlay_WhenPlayIdNullOrEmptyAndDevelopment_ReturnsFalse( + string? playId, + SutProvider sutProvider) + { + sutProvider.GetDependency().IsDevelopment().Returns(true); + sutProvider.Sut.PlayId = playId; + + var result = sutProvider.Sut.InPlay(out var resultPlayId); + + Assert.False(result); + Assert.Empty(resultPlayId); + } + + [Theory] + [BitAutoData] + public void PlayId_CanGetAndSet(string playId) + { + var hostEnvironment = Substitute.For(); + var sut = new PlayIdService(hostEnvironment); + + sut.PlayId = playId; + + Assert.Equal(playId, sut.PlayId); + } +} + +public class NeverPlayIdServicesTests +{ + [Fact] + public void InPlay_ReturnsFalse() + { + var sut = new NeverPlayIdServices(); + + var result = sut.InPlay(out var playId); + + Assert.False(result); + Assert.Empty(playId); + } + + [Theory] + [InlineData("test-play-id")] + [InlineData(null)] + public void PlayId_SetterDoesNothing_GetterReturnsNull(string? value) + { + var sut = new NeverPlayIdServices(); + + sut.PlayId = value; + + Assert.Null(sut.PlayId); + } +} + +public class PlayIdSingletonServiceTests +{ + [Theory] + [BitAutoData] + public void InPlay_WhenNoHttpContext_ReturnsFalse( + SutProvider sutProvider) + { + sutProvider.GetDependency().HttpContext.Returns((HttpContext?)null); + sutProvider.GetDependency().IsDevelopment().Returns(true); + + var result = sutProvider.Sut.InPlay(out var playId); + + Assert.False(result); + Assert.Empty(playId); + } + + [Theory] + [BitAutoData] + public void InPlay_WhenNotDevelopment_ReturnsFalse( + string playIdValue, + SutProvider sutProvider) + { + var httpContext = Substitute.For(); + var serviceProvider = Substitute.For(); + var scopedPlayIdService = Substitute.For(Substitute.For()); + scopedPlayIdService.PlayId = playIdValue; + scopedPlayIdService.InPlay(out Arg.Any()).Returns(x => + { + x[0] = playIdValue; + return true; + }); + + httpContext.RequestServices.Returns(serviceProvider); + serviceProvider.GetRequiredService().Returns(scopedPlayIdService); + + sutProvider.GetDependency().HttpContext.Returns(httpContext); + sutProvider.GetDependency().IsDevelopment().Returns(false); + + var result = sutProvider.Sut.InPlay(out var playId); + + Assert.False(result); + Assert.Empty(playId); + } + + [Theory] + [BitAutoData] + public void InPlay_WhenDevelopmentAndHttpContextWithPlayId_ReturnsTrue( + string playIdValue, + SutProvider sutProvider) + { + var httpContext = Substitute.For(); + var serviceProvider = Substitute.For(); + var hostEnvironment = Substitute.For(); + hostEnvironment.IsDevelopment().Returns(true); + var scopedPlayIdService = new PlayIdService(hostEnvironment) { PlayId = playIdValue }; + + httpContext.RequestServices.Returns(serviceProvider); + serviceProvider.GetRequiredService().Returns(scopedPlayIdService); + + sutProvider.GetDependency().HttpContext.Returns(httpContext); + sutProvider.GetDependency().IsDevelopment().Returns(true); + + var result = sutProvider.Sut.InPlay(out var playId); + + Assert.True(result); + Assert.Equal(playIdValue, playId); + } + + [Theory] + [BitAutoData] + public void PlayId_GetterRetrievesFromScopedService( + string playIdValue, + SutProvider sutProvider) + { + var httpContext = Substitute.For(); + var serviceProvider = Substitute.For(); + var hostEnvironment = Substitute.For(); + var scopedPlayIdService = new PlayIdService(hostEnvironment) { PlayId = playIdValue }; + + httpContext.RequestServices.Returns(serviceProvider); + serviceProvider.GetRequiredService().Returns(scopedPlayIdService); + + sutProvider.GetDependency().HttpContext.Returns(httpContext); + + var result = sutProvider.Sut.PlayId; + + Assert.Equal(playIdValue, result); + } + + [Theory] + [BitAutoData] + public void PlayId_SetterSetsOnScopedService( + string playIdValue, + SutProvider sutProvider) + { + var httpContext = Substitute.For(); + var serviceProvider = Substitute.For(); + var hostEnvironment = Substitute.For(); + var scopedPlayIdService = new PlayIdService(hostEnvironment); + + httpContext.RequestServices.Returns(serviceProvider); + serviceProvider.GetRequiredService().Returns(scopedPlayIdService); + + sutProvider.GetDependency().HttpContext.Returns(httpContext); + + sutProvider.Sut.PlayId = playIdValue; + + Assert.Equal(playIdValue, scopedPlayIdService.PlayId); + } + + [Theory] + [BitAutoData] + public void PlayId_WhenNoHttpContext_GetterReturnsNull( + SutProvider sutProvider) + { + sutProvider.GetDependency().HttpContext.Returns((HttpContext?)null); + + var result = sutProvider.Sut.PlayId; + + Assert.Null(result); + } + + [Theory] + [BitAutoData] + public void PlayId_WhenNoHttpContext_SetterDoesNotThrow( + string playIdValue, + SutProvider sutProvider) + { + sutProvider.GetDependency().HttpContext.Returns((HttpContext?)null); + + sutProvider.Sut.PlayId = playIdValue; + } +}