Address pr feedback

src/Core/Entities/PlayData.cs

@@ -4,6 +4,11 @@ using Bit.Core.Utilities;
 
 namespace Bit.Core.Entities;
 
+/// <summary>
+/// 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.
+/// </summary>
 public class PlayData : ITableObject<Guid>
 {
     public Guid Id { get; set; }
@@ -12,14 +17,22 @@ public class PlayData : ITableObject<Guid>
     public Guid? UserId { get; init; }
     public Guid? OrganizationId { get; init; }
     public DateTime CreationDate { get; init; }
-
     protected PlayData() { }
 
+    /// <summary>
+    /// Generates and sets a new COMB GUID for the Id property.
+    /// </summary>
     public void SetNewId()
     {
         Id = CoreHelpers.GenerateComb();
     }
 
+    /// <summary>
+    /// Creates a new PlayData record associated with a User.
+    /// </summary>
+    /// <param name="user">The user entity created during the play.</param>
+    /// <param name="playId">The play identifier from the x-play-id header.</param>
+    /// <returns>A new PlayData instance tracking the user.</returns>
     public static PlayData Create(User user, string playId)
     {
         return new PlayData
@@ -30,6 +43,12 @@ public class PlayData : ITableObject<Guid>
         };
     }
 
+    /// <summary>
+    /// Creates a new PlayData record associated with an Organization.
+    /// </summary>
+    /// <param name="organization">The organization entity created during the play.</param>
+    /// <param name="playId">The play identifier from the x-play-id header.</param>
+    /// <returns>A new PlayData instance tracking the organization.</returns>
     public static PlayData Create(Organization organization, string playId)
     {
         return new PlayData

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
+{
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <param name="user"></param>
+    /// <returns></returns>
+    Task Record(User user);
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <param name="organization"></param>
+    /// <returns></returns>
+    Task Record(Organization organization);
+}

src/Core/Services/IPlayIdService.cs

@@ -1,7 +1,23 @@
 namespace Bit.Core.Services;
 
+/// <summary>
+/// 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.
+/// </summary>
 public interface IPlayIdService
 {
+    /// <summary>
+    /// Gets or sets the current Play identifier from the x-play-id request header.
+    /// </summary>
     string? PlayId { get; set; }
+
+    /// <summary>
+    /// Checks whether the current request is part of an active Play session.
+    /// </summary>
+    /// <param name="playId">The Play identifier if active, otherwise empty string.</param>
+    /// <returns>True if in a Play session (has PlayId and in Development environment), otherwise false.</returns>
     bool InPlay(out string playId);
 }

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<PlayDataService> 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));
+        }
+    }
+}

src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs

@@ -256,32 +256,21 @@ public class OrganizationRepository : Repository<Organization, Guid>, 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<OrganizationRepository> logger)
         : base(globalSettings, logger)
     {
-        _playIdService = playIdService;
-        _playDataRepository = playDataRepository;
+        _playDataService = playDataService;
     }
 
     public override async Task<Organization> 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;
     }
 }

src/Infrastructure.Dapper/Repositories/UserRepository.cs

@@ -404,32 +404,23 @@ public class UserRepository : Repository<User, Guid>, 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<UserRepository> logger)
-          : base(dataProtectionProvider, globalSettings, logger)
+        IPlayDataService playDataService,
+        GlobalSettings globalSettings,
+        IDataProtectionProvider dataProtectionProvider,
+        ILogger<UserRepository> logger)
+        : base(dataProtectionProvider, globalSettings, logger)
     {
-        _playIdService = playIdService;
-        _playDataRepository = playDataRepository;
+        _playDataService = playDataService;
     }
 
     public override async Task<User> 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;
     }
 }

src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationRepository.cs

@@ -443,34 +443,22 @@ public class OrganizationRepository : Repository<Core.AdminConsole.Entities.Orga
 
 public class TestOrganizationTrackingOrganizationRepository : OrganizationRepository
 {
-    private readonly IPlayIdService _playIdService;
-    private readonly IPlayDataRepository _playDataRepository;
+    private readonly IPlayDataService _playDataService;
 
     public TestOrganizationTrackingOrganizationRepository(
-          IServiceScopeFactory serviceScopeFactory,
-          IMapper mapper,
-          ILogger<OrganizationRepository> logger,
-          IPlayIdService playIdService,
-          IPlayDataRepository playDataRepository)
-          : base(serviceScopeFactory, mapper, logger)
+        IPlayDataService playDataService,
+        IServiceScopeFactory serviceScopeFactory,
+        IMapper mapper,
+        ILogger<OrganizationRepository> logger)
+        : base(serviceScopeFactory, mapper, logger)
     {
-        _playIdService = playIdService;
-        _playDataRepository = playDataRepository;
-
+        _playDataService = playDataService;
     }
 
     public override async Task<Core.AdminConsole.Entities.Organization> 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;
     }
 }

src/Infrastructure.EntityFramework/Repositories/UserRepository.cs

@@ -404,33 +404,22 @@ public class UserRepository : Repository<Core.Entities.User, User, Guid>, 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<UserRepository> logger)
         : base(serviceScopeFactory, mapper, logger)
     {
-        _playIdService = playIdService;
-        _playDataRepository = playDataRepository;
+        _playDataService = playDataService;
     }
 
     public override async Task<Core.Entities.User> 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;
     }
 }

src/SharedWeb/Utilities/PlayIdMiddleware.cs

@@ -3,6 +3,12 @@ using Microsoft.AspNetCore.Http;
 
 namespace Bit.SharedWeb.Utilities;
 
+/// <summary>
+/// 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.
+/// </summary>
+/// <param name="next"></param>
 public sealed class PlayIdMiddleware(RequestDelegate next)
 {
     public Task Invoke(HttpContext context, PlayIdService playIdService)

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<PlayDataService> sutProvider)
+    {
+        sutProvider.GetDependency<IPlayIdService>()
+            .InPlay(out Arg.Any<string>())
+            .Returns(x =>
+            {
+                x[0] = playId;
+                return true;
+            });
+
+        await sutProvider.Sut.Record(user);
+
+        await sutProvider.GetDependency<IPlayDataRepository>()
+            .Received(1)
+            .CreateAsync(Arg.Is<PlayData>(pd =>
+                pd.PlayId == playId &&
+                pd.UserId == user.Id &&
+                pd.OrganizationId == null));
+
+        sutProvider.GetDependency<ILogger<PlayDataService>>()
+            .Received(1)
+            .Log(
+                LogLevel.Information,
+                Arg.Any<EventId>(),
+                Arg.Is<object>(o => o.ToString().Contains(user.Id.ToString()) && o.ToString().Contains(playId)),
+                null,
+                Arg.Any<Func<object, Exception?, string>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task Record_User_WhenNotInPlay_DoesNotRecordPlayData(
+        User user,
+        SutProvider<PlayDataService> sutProvider)
+    {
+        sutProvider.GetDependency<IPlayIdService>()
+            .InPlay(out Arg.Any<string>())
+            .Returns(x =>
+            {
+                x[0] = null;
+                return false;
+            });
+
+        await sutProvider.Sut.Record(user);
+
+        await sutProvider.GetDependency<IPlayDataRepository>()
+            .DidNotReceive()
+            .CreateAsync(Arg.Any<PlayData>());
+
+        sutProvider.GetDependency<ILogger<PlayDataService>>()
+            .DidNotReceive()
+            .Log(
+                LogLevel.Information,
+                Arg.Any<EventId>(),
+                Arg.Any<object>(),
+                Arg.Any<Exception>(),
+                Arg.Any<Func<object, Exception?, string>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task Record_Organization_WhenInPlay_RecordsPlayData(
+        string playId,
+        Organization organization,
+        SutProvider<PlayDataService> sutProvider)
+    {
+        sutProvider.GetDependency<IPlayIdService>()
+            .InPlay(out Arg.Any<string>())
+            .Returns(x =>
+            {
+                x[0] = playId;
+                return true;
+            });
+
+        await sutProvider.Sut.Record(organization);
+
+        await sutProvider.GetDependency<IPlayDataRepository>()
+            .Received(1)
+            .CreateAsync(Arg.Is<PlayData>(pd =>
+                pd.PlayId == playId &&
+                pd.OrganizationId == organization.Id &&
+                pd.UserId == null));
+
+        sutProvider.GetDependency<ILogger<PlayDataService>>()
+            .Received(1)
+            .Log(
+                LogLevel.Information,
+                Arg.Any<EventId>(),
+                Arg.Is<object>(o => o.ToString().Contains(organization.Id.ToString()) && o.ToString().Contains(playId)),
+                null,
+                Arg.Any<Func<object, Exception?, string>>());
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task Record_Organization_WhenNotInPlay_DoesNotRecordPlayData(
+        Organization organization,
+        SutProvider<PlayDataService> sutProvider)
+    {
+        sutProvider.GetDependency<IPlayIdService>()
+            .InPlay(out Arg.Any<string>())
+            .Returns(x =>
+            {
+                x[0] = null;
+                return false;
+            });
+
+        await sutProvider.Sut.Record(organization);
+
+        await sutProvider.GetDependency<IPlayDataRepository>()
+            .DidNotReceive()
+            .CreateAsync(Arg.Any<PlayData>());
+
+        sutProvider.GetDependency<ILogger<PlayDataService>>()
+            .DidNotReceive()
+            .Log(
+                LogLevel.Information,
+                Arg.Any<EventId>(),
+                Arg.Any<object>(),
+                Arg.Any<Exception>(),
+                Arg.Any<Func<object, Exception?, string>>());
+    }
+}

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<PlayIdService> sutProvider)
+    {
+        sutProvider.GetDependency<IHostEnvironment>().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<PlayIdService> sutProvider)
+    {
+        sutProvider.GetDependency<IHostEnvironment>().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<PlayIdService> sutProvider)
+    {
+        sutProvider.GetDependency<IHostEnvironment>().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<IHostEnvironment>();
+        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<PlayIdSingletonService> sutProvider)
+    {
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns((HttpContext?)null);
+        sutProvider.GetDependency<IHostEnvironment>().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<PlayIdSingletonService> sutProvider)
+    {
+        var httpContext = Substitute.For<HttpContext>();
+        var serviceProvider = Substitute.For<IServiceProvider>();
+        var scopedPlayIdService = Substitute.For<PlayIdService>(Substitute.For<IHostEnvironment>());
+        scopedPlayIdService.PlayId = playIdValue;
+        scopedPlayIdService.InPlay(out Arg.Any<string>()).Returns(x =>
+        {
+            x[0] = playIdValue;
+            return true;
+        });
+
+        httpContext.RequestServices.Returns(serviceProvider);
+        serviceProvider.GetRequiredService<PlayIdService>().Returns(scopedPlayIdService);
+
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns(httpContext);
+        sutProvider.GetDependency<IHostEnvironment>().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<PlayIdSingletonService> sutProvider)
+    {
+        var httpContext = Substitute.For<HttpContext>();
+        var serviceProvider = Substitute.For<IServiceProvider>();
+        var hostEnvironment = Substitute.For<IHostEnvironment>();
+        hostEnvironment.IsDevelopment().Returns(true);
+        var scopedPlayIdService = new PlayIdService(hostEnvironment) { PlayId = playIdValue };
+
+        httpContext.RequestServices.Returns(serviceProvider);
+        serviceProvider.GetRequiredService<PlayIdService>().Returns(scopedPlayIdService);
+
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns(httpContext);
+        sutProvider.GetDependency<IHostEnvironment>().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<PlayIdSingletonService> sutProvider)
+    {
+        var httpContext = Substitute.For<HttpContext>();
+        var serviceProvider = Substitute.For<IServiceProvider>();
+        var hostEnvironment = Substitute.For<IHostEnvironment>();
+        var scopedPlayIdService = new PlayIdService(hostEnvironment) { PlayId = playIdValue };
+
+        httpContext.RequestServices.Returns(serviceProvider);
+        serviceProvider.GetRequiredService<PlayIdService>().Returns(scopedPlayIdService);
+
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns(httpContext);
+
+        var result = sutProvider.Sut.PlayId;
+
+        Assert.Equal(playIdValue, result);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void PlayId_SetterSetsOnScopedService(
+        string playIdValue,
+        SutProvider<PlayIdSingletonService> sutProvider)
+    {
+        var httpContext = Substitute.For<HttpContext>();
+        var serviceProvider = Substitute.For<IServiceProvider>();
+        var hostEnvironment = Substitute.For<IHostEnvironment>();
+        var scopedPlayIdService = new PlayIdService(hostEnvironment);
+
+        httpContext.RequestServices.Returns(serviceProvider);
+        serviceProvider.GetRequiredService<PlayIdService>().Returns(scopedPlayIdService);
+
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns(httpContext);
+
+        sutProvider.Sut.PlayId = playIdValue;
+
+        Assert.Equal(playIdValue, scopedPlayIdService.PlayId);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void PlayId_WhenNoHttpContext_GetterReturnsNull(
+        SutProvider<PlayIdSingletonService> sutProvider)
+    {
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns((HttpContext?)null);
+
+        var result = sutProvider.Sut.PlayId;
+
+        Assert.Null(result);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void PlayId_WhenNoHttpContext_SetterDoesNotThrow(
+        string playIdValue,
+        SutProvider<PlayIdSingletonService> sutProvider)
+    {
+        sutProvider.GetDependency<IHttpContextAccessor>().HttpContext.Returns((HttpContext?)null);
+
+        sutProvider.Sut.PlayId = playIdValue;
+    }
+}