Initial PoC of seeder API

[Hinton]

🎟️ Tracking

📔 Objective

Adds a standalone API service which allows you to run and destroy seeds. The intention is to allow integration tests to seed the database.

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[codecov[bot]]

Codecov Report

:x: Patch coverage is 56.96203% with 102 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 57.21%. Comparing base (d3e4ac7) to head (7039eb5). :warning: Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
...tructure.Dapper/Repositories/PlayDataRepository.cs	0.00%	22 Missing :warning:
...EntityFramework/Repositories/PlayDataRepository.cs	50.00%	11 Missing :warning:
...dminConsole/Repositories/OrganizationRepository.cs	0.00%	9 Missing :warning:
...frastructure.Dapper/Repositories/UserRepository.cs	10.00%	9 Missing :warning:
...dminConsole/Repositories/OrganizationRepository.cs	0.00%	9 Missing :warning:
...ure.EntityFramework/Repositories/UserRepository.cs	25.00%	9 Missing :warning:
...SharedWeb/Utilities/ServiceCollectionExtensions.cs	35.71%	6 Missing and 3 partials :warning:
src/SharedWeb/Utilities/PlayIdMiddleware.cs	0.00%	8 Missing :warning:
...ucture.Dapper/DapperServiceCollectionExtensions.cs	50.00%	4 Missing and 1 partial :warning:
...work/EntityFrameworkServiceCollectionExtensions.cs	50.00%	4 Missing and 1 partial :warning:
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6424      +/-   ##
==========================================
+ Coverage   53.27%   57.21%   +3.94%     
==========================================
  Files        1906     1925      +19     
  Lines       84973    85695     +722     
  Branches     7639     7678      +39     
==========================================
+ Hits        45268    49034    +3766     
+ Misses      37952    34817    -3135     
- Partials     1753     1844      +91     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – b23ae536-6f5d-468c-a8d0-c313884701f9

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	Use_Of_Hardcoded_Password	/util/Seeder/Factories/UserSeeder.cs: 61	details The application uses the hard-coded password MasterPassword for authentication purposes, either using it to verify users' identities, or to access... ID: UFsrfd47ViLFGJQpFxMFK%2B7Yu1w%3D Attack Vector

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	~~Use_Of_Hardcoded_Password~~	/util/Seeder/Factories/UserSeeder.cs: 16

[sonarqubecloud[bot]]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Hinton]

@claude can you review the pr.

[claude[bot]]

Claude finished @Hinton's task —— View job

---

PR Review Complete

I've completed a comprehensive review of this PR. Overall, this is a well-structured implementation of a seeder API for integration testing. Here are my findings organized by category:

---

🎨 Architecture & Design

Strengths:

• Clean separation of concerns with Scenes, Queries, and the API layer
• Good use of keyed services for dynamic scene resolution
• Type-safe pattern using generics for scenes while maintaining dynamic invocation capability
• The PlayData tracking system is elegant - using headers to track seeded data is a smart approach

Suggestions:

1. Documentation needed (addresses reviewer feedback at util/SeederApi/Program.cs):

   • Add XML documentation for IPlayIdService interface and its methods (src/Core/Services/IPlayIdService.cs:3-7)
   • Add a description of what PlayData represents in the class documentation (src/Core/Entities/PlayData.cs:7)
   • Create a README.md for the SeederApi project explaining its purpose, how to use it, and examples
   • Consider adding a CLAUDE.md for the SeederApi project with specific instructions

2. Interface improvement (addresses comment at src/Infrastructure.Dapper/AdminConsole/Repositories/OrganizationRepository.cs:47):

   • Consider adding a TryRecordCreate method on IPlayIdService to encapsulate both the InPlay check and repository creation, reducing boilerplate in repositories

---

🔒 Security Concerns

Critical Issues:

1. Code Injection vulnerability (flagged by GitHub Advanced Security at util/SeederApi/Controllers/SeedController.cs):

   • The dynamic scene resolution and JSON deserialization could be vulnerable
   • Recommendation: Implement allowlisting of scene names and validate input thoroughly
   • Consider adding authentication/authorization to the SeederApi endpoints

2. Hardcoded credentials (flagged by Checkmarx at util/Seeder/Factories/UserSeeder.cs:61):

   • CreateUserNoMangle method contains hardcoded master password hash
   • While this is test data, ensure this is documented and only used in development/test environments
   • The MasterPassword constant reference should be clarified as test data

3. Development-only safeguard:

   • Good: PlayIdService.InPlay checks IsDevelopment()
   • Consider: Adding additional safeguards to ensure SeederApi cannot run in production
   • The middleware at src/SharedWeb/Utilities/PlayIdMiddleware.cs:6-17 should validate environment

CSRF Issues (flagged by Checkmarx):

• Three CSRF vulnerabilities flagged in CiphersController.cs and AccountsController.cs
• These appear to be existing issues, not introduced by this PR
• Should be addressed separately

---

💻 Code Quality

Issues to Address:

1. Logging consolidation (comment at util/SeederApi/Controllers/SeedController.cs):

   • Lines 13-28: Consider using structured logging with a single log statement instead of multiple
   • Example: logger.LogInformation("Processing seed request: {Template} with {ArgumentCount} arguments", request.Template, request.Arguments?.GetArrayLength() ?? 0)

2. Service responsibility (comment at util/SeederApi/Controllers/SeedController.cs):

   • The logging and error handling in the controller could be moved to the service layer
   • Controllers should be thin - consider moving orchestration logic to SceneService

3. Unnecessary reverse mapping (comment at src/Infrastructure.EntityFramework/Models/PlayData.cs:17):

   • The EF model has a ToPlayData() method - do you really need reverse maps for this read-only tracking entity?

4. Comments in Program.cs (review feedback):

   • Remove unnecessary comments that don't add value beyond IntelliSense (util/SeederApi/Program.cs)
   • Keep only comments that explain "why" not "what"

5. Database cascade delete discussion (src/Sql/dbo/Tables/Device.sql):

   • Per reviewer discussion: Consider reverting the Device table FK cascade change to decrease surface area
   • The PR is large enough without this ancillary change

---

🧪 Testing

Strengths:

• Good integration test coverage in SeederApi.IntegrationTest
• Tests cover happy paths, error cases, and batch operations
• Tests properly clean up after themselves

Suggestions:

1. Unit tests are missing - the project guidelines require unit tests for new features (per CLAUDE.md)
2. Consider adding tests for:
   • PlayIdService behavior in different environments
   • PlayIdMiddleware header parsing
   • Error handling in SceneService
   • Repository PlayData creation behavior

---

📊 Database & Migrations

Good practices:

• Proper indexing on PlayData table (PlayId, UserId, OrganizationId)
• Check constraint ensures either UserId or OrganizationId (not both)
• Cascade deletes properly configured
• Migrations provided for all database providers (SQL Server, MySQL, PostgreSQL, SQLite)

Concerns:

1. Migration script at util/Migrator/DbScripts/2025-11-04_00_PlayData.sql uses OBJECT_ID check - ensure this works correctly in all deployment scenarios
2. The SELECT * in PlayData_ReadByPlayId stored procedure should explicitly list columns for better maintainability

---

📝 Additional Observations

Code Style:

• Generally follows C# conventions and Bitwarden code style
• Good use of nullable reference types
• Primary constructors used appropriately

Performance:

• Batch delete uses Task.WhenAll for parallelization - good
• Consider adding transaction support for multi-entity operations
• The GetAllPlayIds method loads all distinct PlayIds into memory - could be problematic with large datasets

Missing:

1. Error handling for concurrent PlayId operations
2. Validation of PlayId format/length (max 256 chars)
3. Metrics/telemetry for seeder operations (ADR 0020 mentions OpenTelemetry)

---

✅ Summary

This is a solid PoC with good architectural decisions. The main blockers are:

1. Security: Address code injection concerns and add SeederApi access controls
2. Documentation: Add README.md and improve code documentation (especially for PlayData and IPlayIdService)
3. Testing: Add unit tests per project guidelines
4. Cleanup: Remove unnecessary comments, consider service refactoring suggestions
5. Database: Revert Device table change to reduce PR scope

The PlayData tracking system is innovative and the Scene/Query pattern is well-designed for extensibility. Once the security and documentation concerns are addressed, this will be a valuable addition for integration testing.

Recommendation: Request changes to address security concerns and documentation before merging.

---