Draft: feat(commissions): wallets and calulcation pipelines @marsav · 2026-06-21

Merge Request Info

Summary

This MR adds commission wallets, calculation pipelines, input commission import, expected/stored commission calculation, QA seed endpoints, k6 coverage, regenerated API clients, and broker hierarchy event plumbing. The contract/client regeneration is present and the command/event shapes are mostly aligned, but the new endpoints currently miss authorization metadata while the new k6 tests assert 401, and one calculation service leaks parser/evaluator details to clients.

Review Verdict

REQUEST CHANGES

Please fix the missing authorization metadata and the confirmed backend API contract/security issues before merge.

Existing Review Threads

• general (@coderabbitai): "Review skipped. Draft detected." - open. No human inline review threads were present in the discussions API page fetched for this SHA.

Changed Files Overview

Critical Issues

Issue 1: New commission endpoints do not require authorization, but the tests expect 401

• File: backend/src/Allrisk.Core.Commissions/Api/WalletEndpoints/WalletEndpoints.cs:28
• Severity: Critical
• Category: Security / Test Failure
• Description: The new wallet, calculation-pipeline, input-import, calculation, and calculation-properties endpoints are added without [Authorize] / role metadata. The shared startup only calls services.AddAuthorization() and app.UseAuthorization() (backend/src/Allrisk.Core.SharedKernel/DependencyInjection.cs:296, backend/src/Allrisk.Core.SharedKernel/DependencyInjection.cs:589); there is no fallback policy in this diff that makes anonymous endpoints protected by default. The MR's own k6 tests assert 401 for unauthenticated calls (backend/tests/k6Tests/Allrisk.Core.Commissions/Wallets/api-wallets-unauthorized-test-methods.ts:11, backend/tests/k6Tests/Allrisk.Core.Commissions/CalculationPipelines/api-calculation-pipelines-unauthorized-test-methods.ts:12, backend/tests/k6Tests/Allrisk.Core.Commissions/InputCommissions/api-input-commissions-unauthorized-test-methods.ts:14), so these tests should fail and the endpoints are exposed without the intended guard.
• Suggestion: Add the appropriate auth metadata to every new endpoint class/method and include 401/403 response declarations where this API documents responses.

// Current
[WolverineGet("/wallets")]
public static async Task<Result<IEnumerable<Wallet>>> GetAllWallets(IMessageBus bus)

// Suggested shape
[Authorize]
[ProducesResponseType(StatusCodes.Status401Unauthorized)]
[ProducesResponseType(StatusCodes.Status403Forbidden)]
[WolverineGet("/wallets")]
public static async Task<Result<IEnumerable<Wallet>>> GetAllWallets(IMessageBus bus)

Issue 2: QA automation endpoints are tenant-allowlisted but still unauthenticated

• File: backend/src/Allrisk.Core.Commissions/Api/QaAutomationEndpoints.cs:33
• Severity: Critical
• Category: Security
• Description: The new QA endpoints seed and delete wallets, calculation pipelines, input commissions, acquisitions, commissions, and production points, but the class explicitly says "No token required" and only checks ApplicationTenants.GetSeedTestTenants(). Tenant allowlisting is not authentication; anyone who knows a seed tenant can invoke destructive endpoints like DeleteTestInputCommissions at backend/src/Allrisk.Core.Commissions/Api/QaAutomationEndpoints.cs:197 or DeleteTestAcquisitions at backend/src/Allrisk.Core.Commissions/Api/QaAutomationEndpoints.cs:229.
• Suggestion: Require auth/role metadata on these QA routes as well, and keep the tenant allowlist as an additional guard.

// Current
[WolverineDelete("/qa-automation/tenants/{tenant}/test-input-commissions")]
public static async Task<Result> DeleteTestInputCommissions([FromRoute] string tenant, IMessageBus bus)

// Suggested shape
[Authorize]
[WolverineDelete("/qa-automation/tenants/{tenant}/test-input-commissions")]
public static async Task<Result> DeleteTestInputCommissions([FromRoute] string tenant, IMessageBus bus)

Warnings

Issue 1: Expected commissions endpoint bypasses the CleanResult endpoint contract

• File: backend/src/Allrisk.Core.Commissions/Api/CommissionCalculationEndpoints/CommissionCalculationEndpoints.cs:47
• Severity: Warning
• Category: API Contract
• Description: ExpectedCommissions returns Task<IResult> and manually serializes errors through Results.Json(...) at line 53. The backend convention for Wolverine endpoints is to return Result / Result<T> and let the endpoint layer keep a consistent CleanResult contract, including file responses.
• Suggestion: Wrap the file payload in the same project-sanctioned CleanResult/file response pattern used by existing file endpoints, or introduce a typed response wrapper instead of returning raw IResult.

// Current
public static async Task<IResult> ExpectedCommissions(...)

// Target
public static async Task<Result<...>> ExpectedCommissions(...)

Issue 2: NCalc parser/evaluator exception messages are returned to API clients

• File: backend/src/Allrisk.Core.Commissions/Infrastructure/Services/CommissionCalculationService.cs:41
• Severity: Warning
• Category: Security
• Description: The service includes exception.Message in Result.Error(...) at lines 41, 45, and 59. Those errors are reachable through EvaluatePipelineCommandHandler (backend/src/Allrisk.Core.Commissions/Application/Commands/CalculationPipelines/EvaluatePipelineCommand.cs:43) and the calculation endpoint path, so parser/evaluator internals can be exposed to API clients.
• Suggestion: Log the exception detail server-side with ContextLog.* and return a canonical localized message to the client.

catch (NCalcParserException exception)
{
    ContextLog.Warning(exception, "Failed to parse commission expression");
    return Result.Error(localizer[LocalizationKeys.CalculationExpressionInvalid], HttpStatusCode.BadRequest);
}

Suggestions

• backend/src/Allrisk.Core.Commissions/Api/WalletEndpoints/WalletEndpointsValidations.cs:12: Extract MaximumLength(256) into a named constant and reuse it across wallet/pipeline validators.
• backend/src/Allrisk.Core.Commissions/Application/Commands/Wallets/CreateWalletCommand.cs:20: Add LoadAsync validation if wallet names are truly unique per tenant; the request docs and 409 response claim uniqueness, but create/update currently only store the supplied name.

Positive Highlights

• Generated frontend and k6 clients are included with the backend contract changes.
• Marten documents for new commission aggregates are registered in DependencyInjection.cs.
• The k6 suite covers happy path, validation, unauthorized, and delete-conflict flows for the new commission endpoints.

Testing Assessment

Checklist

Backend

• No NEW IQuerySession / DB access in API endpoints
• No manual session.SaveChangesAsync in command/event handlers
• No try/catch outside Service / ACL
• Endpoints return Result<T> / CleanResult, not raw IResult
• Commands return domain events instead of response DTOs
• Query handlers return raw data; endpoint owns 404 + localization
• Command/query record + handler in one .cs file; endpoint methods static + XML summaries
• Guid.CreateVersion7() used for new IDs
• New Marten classes registered in DependencyInjection.cs
• Every endpoint has auth metadata + 401/403 response coverage
• No leaked ex.Message

Testing

• k6 added for new endpoint surfaces
• k6 reuses shared generated client/provider infrastructure
• Tests assert real status codes and selected response values
• Unauthorized tests line up with actual endpoint authorization metadata

Infra / cross-cutting

• Generated FE Kubb client regenerated
• k6 generated client regenerated
• QA endpoints protected by auth in addition to tenant allowlisting