Enhance authentication and security tests for multi-tenant scenarios

[aalmada]

Description

Enhance authentication and security testing capabilities for multi-tenant environments by allowing custom email during user registration and login. Introduce comprehensive tests for passkey security, account lockout behavior, cross-tenant authentication, refresh token validation, and JWT security stamp validation.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code refactoring
• Performance improvement
• Test addition/update

Related Issues

Fixes #

Changes Made

• Updated AuthenticationHelpers to allow custom email during user registration and login.
• Added tests for passkey security, account lockout behavior, and cross-tenant authentication.
• Implemented refresh token and JWT security stamp validation tests.

Testing

• Unit tests pass (dotnet test)
• Build succeeds (dotnet build)
• Code formatting is correct (dotnet format --verify-no-changes)
• No new analyzer warnings
• Manual testing completed

Checklist

• My code follows the project's coding standards
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

[Copilot]

Pull request overview

This PR enhances the test suite with comprehensive security and authentication tests for multi-tenant environments. It adds the ability to specify custom emails during user registration/login and introduces five new test files covering critical security scenarios: JWT security stamp validation, refresh token security, passkey security, cross-tenant authentication isolation, and account lockout behavior.

Changes:

• Enhanced AuthenticationHelpers.RegisterAndLoginUserAsync to accept optional custom email parameter for multi-tenant testing scenarios
• Expanded LoginResponse record to include TokenType, ExpiresIn, and optional UserId fields for complete token response modeling
• Added 5 new comprehensive test files validating security boundaries across tenants, token invalidation, and authentication lockout mechanisms

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
tests/BookStore.AppHost.Tests/Helpers/AuthenticationHelpers.cs	Added optional email parameter to RegisterAndLoginUserAsync and expanded LoginResponse record with additional token metadata fields
tests/BookStore.AppHost.Tests/SecurityStampValidationTests.cs	New test file validating JWT invalidation after security stamp changes (password changes, passkey additions)
tests/BookStore.AppHost.Tests/RefreshTokenSecurityTests.cs	New test file validating refresh token security, expiration, rotation, and invalidation scenarios
tests/BookStore.AppHost.Tests/PasskeySecurityTests.cs	Enhanced existing test file with multi-tenant passkey isolation tests and passkey-only user scenarios
tests/BookStore.AppHost.Tests/CrossTenantAuthenticationTests.cs	New test file validating tenant isolation for authentication, preventing cross-tenant token usage
tests/BookStore.AppHost.Tests/AccountLockoutTests.cs	New test file validating account lockout behavior for failed password and passkey authentication attempts
tests/BookStore.AppHost.Tests/EmailVerificationTests.cs	Enhanced existing test file with multi-tenant support for email verification tests

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The variable userId is declared but never used in this test. Consider removing it to keep the code clean, or if it was intended for future use, add a TODO comment explaining its purpose.

Suggested change

var userId = loginResponse.UserId;

[Copilot]

The variable loginResponse is declared but never used in this test. Consider removing it since only the email and password are needed for the subsequent operations.

Suggested change

	var (email, password, loginResponse, _) = await AuthenticationHelpers.RegisterAndLoginUserAsync();
	var (email, password, _, _) = await AuthenticationHelpers.RegisterAndLoginUserAsync();

[Copilot]

The GetStoreAsync and ManuallyUpdateSecurityStampAsync methods are duplicated across multiple test files (SecurityStampValidationTests, RefreshTokenSecurityTests, PasskeySecurityTests, AccountLockoutTests, EmailVerificationTests). Consider extracting these common database helper methods into the DatabaseHelpers class to reduce code duplication and improve maintainability.

Suggested change

	async Task<IDocumentStore> GetStoreAsync()
	{
	var connectionString = await GlobalHooks.App!.GetConnectionStringAsync("bookstore");
	return DocumentStore.For(opts =>
	{
	opts.UseSystemTextJsonForSerialization(EnumStorage.AsString, Casing.CamelCase);
	opts.Connection(connectionString!);
	_ = opts.Policies.AllDocumentsAreMultiTenanted();
	opts.Events.TenancyStyle = Marten.Storage.TenancyStyle.Conjoined;
	});
	}
	async Task ManuallyUpdateSecurityStampAsync(string email, string? tenantId = null)
	{
	using var store = await GetStoreAsync();
	var actualTenantId = tenantId ?? StorageConstants.DefaultTenantId;
	await using var session = store.LightweightSession(actualTenantId);
	var normalizedEmail = email.ToUpperInvariant();
	var user = await session.Query<ApplicationUser>()
	.Where(u => u.NormalizedEmail == normalizedEmail)
	.FirstOrDefaultAsync();
	_ = await Assert.That(user).IsNotNull();
	if (user != null)
	{
	user.SecurityStamp = Guid.CreateVersion7().ToString();
	session.Store(user);
	await session.SaveChangesAsync();
	}
	Task<IDocumentStore> GetStoreAsync()
	{
	return DatabaseHelpers.GetStoreAsync();
	}
	Task ManuallyUpdateSecurityStampAsync(string email, string? tenantId = null)
	{
	return DatabaseHelpers.ManuallyUpdateSecurityStampAsync(email, tenantId);

[Copilot]

The GetStoreAsync method is duplicated across multiple test files. Consider extracting it to the DatabaseHelpers class since an identical method already exists there (DatabaseHelpers.GetDocumentStoreAsync), which can be reused instead of duplicating this code.

Suggested change

	async Task<IDocumentStore> GetStoreAsync()
	{
	var connectionString = await GlobalHooks.App!.GetConnectionStringAsync("bookstore");
	return DocumentStore.For(opts =>
	{
	opts.UseSystemTextJsonForSerialization(EnumStorage.AsString, Casing.CamelCase);
	opts.Connection(connectionString!);
	_ = opts.Policies.AllDocumentsAreMultiTenanted();
	opts.Events.TenancyStyle = Marten.Storage.TenancyStyle.Conjoined;
	});
	}
	async Task ManuallyExpireRefreshTokenAsync(string email, string refreshToken, string? tenantId = null)
	{
	using var store = await GetStoreAsync();
	async Task ManuallyExpireRefreshTokenAsync(string email, string refreshToken, string? tenantId = null)
	{
	using var store = await DatabaseHelpers.GetDocumentStoreAsync();

[Copilot]

The GetStoreAsync method is duplicated across multiple test files. Consider extracting it to the DatabaseHelpers class since an identical method already exists there (DatabaseHelpers.GetDocumentStoreAsync), which can be reused instead of duplicating this code.

[Copilot]

The GetStoreAsync method is duplicated across multiple test files. Consider extracting it to the DatabaseHelpers class since an identical method already exists there (DatabaseHelpers.GetDocumentStoreAsync), which can be reused instead of duplicating this code.

[Copilot]

The variable loginResponse from the initial registration is declared but never used in this test. The test later performs a fresh login at line 295 and uses that response instead. Consider using a discard pattern for the unused loginResponse: var (email, password, _, _) = ...

Suggested change

	var (email, password, loginResponse, _) = await AuthenticationHelpers.RegisterAndLoginUserAsync(tenantId);
	var (email, password, _, _) = await AuthenticationHelpers.RegisterAndLoginUserAsync(tenantId);

[Copilot]

The variable loginResponse from the initial registration is declared but never used in this test. Consider using a discard pattern for the unused loginResponse: var (email, password, _, tenantId) = ...

[Copilot]

The method GetTenantPairs is defined but never used in this file or elsewhere in the test suite. If this was intended for future use with data-driven tests, consider removing it now and adding it back when actually needed, or add a TODO comment explaining its intended use.

Suggested change

	/// <summary>
	/// Provides tenant pair combinations for data-driven tests.
	/// </summary>
	public static IEnumerable<(string Source, string Target)> GetTenantPairs()
	{
	yield return ("tenant-a", "tenant-b");
	yield return ("tenant-a", "default");
	yield return ("tenant-b", "tenant-a");
	yield return ("tenant-b", "default");
	yield return ("default", "tenant-a");
	yield return ("default", "tenant-b");
	}

[Copilot]

The variable loginResponse from the initial registration is declared but never used in this test. Consider using a discard pattern for the unused loginResponse: var (email, password, _, tenantId) = ...

Suggested change

	var (email, password, loginResponse, tenantId) = await AuthenticationHelpers.RegisterAndLoginUserAsync();
	var (email, password, _, tenantId) = await AuthenticationHelpers.RegisterAndLoginUserAsync();

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Missing space after the comment delimiter. The comment should be written as // Act: instead of //Act: to follow standard C# comment formatting conventions.

Suggested change

	//Act: Try to access protected endpoint with old JWT
	// Act: Try to access protected endpoint with old JWT

[Copilot]

The GetStoreAsync method duplicates logic from DatabaseHelpers.GetDocumentStoreAsync() but lacks the important connection pooling configuration (MaxPoolSize, MinPoolSize, ConnectionLifetime) that was added to prevent connection exhaustion during parallel test execution. Consider using DatabaseHelpers.GetDocumentStoreAsync() directly instead of maintaining a separate implementation, or if a local implementation is needed, ensure it includes the same connection pooling configuration to prevent PostgreSQL connection pool exhaustion.

Suggested change

	var connectionString = await GlobalHooks.App!.GetConnectionStringAsync("bookstore");
	return DocumentStore.For(opts =>
	{
	opts.UseSystemTextJsonForSerialization(EnumStorage.AsString, Casing.CamelCase);
	opts.Connection(connectionString!);
	_ = opts.Policies.AllDocumentsAreMultiTenanted();
	opts.Events.TenancyStyle = Marten.Storage.TenancyStyle.Conjoined;
	});
	// Use the shared helper to ensure consistent connection pooling configuration
	return await DatabaseHelpers.GetDocumentStoreAsync("bookstore");

[Copilot]

The GetStoreAsync method duplicates logic from DatabaseHelpers.GetDocumentStoreAsync() but lacks the important connection pooling configuration (MaxPoolSize, MinPoolSize, ConnectionLifetime) that was added to prevent connection exhaustion during parallel test execution. Consider using DatabaseHelpers.GetDocumentStoreAsync() directly instead of maintaining a separate implementation, or if a local implementation is needed, ensure it includes the same connection pooling configuration to prevent PostgreSQL connection pool exhaustion.

Suggested change

	var connectionString = await GlobalHooks.App!.GetConnectionStringAsync("bookstore");
	return DocumentStore.For(opts =>
	{
	opts.UseSystemTextJsonForSerialization(EnumStorage.AsString, Casing.CamelCase);
	opts.Connection(connectionString!);
	_ = opts.Policies.AllDocumentsAreMultiTenanted();
	opts.Events.TenancyStyle = Marten.Storage.TenancyStyle.Conjoined;
	});
	return await DatabaseHelpers.GetDocumentStoreAsync("bookstore");

[Copilot]

The IDocumentStore should be disposed using await using instead of using to ensure proper asynchronous cleanup of database connections. The DatabaseHelpers.GetDocumentStoreAsync() documentation explicitly states "Callers MUST dispose the returned IDocumentStore to prevent connection leaks" and recommends using await using var store. This is critical for proper connection pool management during parallel test execution.

Suggested change

	using var store = await GetStoreAsync();
	await using var store = await GetStoreAsync();

[Copilot]

The IDocumentStore should be disposed using await using instead of using to ensure proper asynchronous cleanup of database connections. The DatabaseHelpers.GetDocumentStoreAsync() documentation explicitly states "Callers MUST dispose the returned IDocumentStore to prevent connection leaks" and recommends using await using var store. This is critical for proper connection pool management during parallel test execution.

[Copilot]

The variable userId is assigned but never used in the test. Consider removing this unused variable declaration to improve code clarity.

Suggested change

var userId = loginResponse.UserId;

[Copilot]

The GetStoreAsync method duplicates logic from DatabaseHelpers.GetDocumentStoreAsync() but lacks the important connection pooling configuration (MaxPoolSize, MinPoolSize, ConnectionLifetime) that was added to prevent connection exhaustion during parallel test execution. Consider using DatabaseHelpers.GetDocumentStoreAsync() directly instead of maintaining a separate implementation, or if a local implementation is needed, ensure it includes the same connection pooling configuration to prevent PostgreSQL connection pool exhaustion.

Suggested change

	async Task<IDocumentStore> GetStoreAsync()
	{
	var connectionString = await GlobalHooks.App!.GetConnectionStringAsync("bookstore");
	return DocumentStore.For(opts =>
	{
	opts.UseSystemTextJsonForSerialization(EnumStorage.AsString, Casing.CamelCase);
	opts.Connection(connectionString!);
	_ = opts.Policies.AllDocumentsAreMultiTenanted();
	opts.Events.TenancyStyle = Marten.Storage.TenancyStyle.Conjoined;
	});
	}
	async Task ManuallyUpdateSecurityStampAsync(string email, string? tenantId = null)
	{
	using var store = await GetStoreAsync();
	async Task ManuallyUpdateSecurityStampAsync(string email, string? tenantId = null)
	{
	using var store = await DatabaseHelpers.GetDocumentStoreAsync();

[Copilot]

The GetStoreAsync method duplicates logic from DatabaseHelpers.GetDocumentStoreAsync() but lacks the important connection pooling configuration (MaxPoolSize, MinPoolSize, ConnectionLifetime) that was added to prevent connection exhaustion during parallel test execution. Consider using DatabaseHelpers.GetDocumentStoreAsync() directly instead of maintaining a separate implementation, or if a local implementation is needed, ensure it includes the same connection pooling configuration to prevent PostgreSQL connection pool exhaustion.

[Copilot]

The IDocumentStore should be disposed using await using instead of using to ensure proper asynchronous cleanup of database connections. The DatabaseHelpers.GetDocumentStoreAsync() documentation explicitly states "Callers MUST dispose the returned IDocumentStore to prevent connection leaks" and recommends using await using var store. This is critical for proper connection pool management during parallel test execution.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 22 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Resource leak: DocumentStore instance in ClassSetup is not properly disposed. Change "using var store" to "await using var store" to properly dispose async resources and prevent connection pool leaks. See DatabaseHelpers.cs lines 73-82 for detailed explanation of why this is required.

[Copilot]

Resource leak: DocumentStore instance in ClassSetup is not properly disposed. Change "using var store" to "await using var store" to properly dispose async resources and prevent connection pool leaks. See DatabaseHelpers.cs lines 73-82 for detailed explanation of why this is required.

[Copilot]

Resource leak: DocumentStore instance is not properly disposed. Change "using var store" to "await using var store" to properly dispose async resources and prevent connection pool leaks. See DatabaseHelpers.cs lines 73-82 for detailed explanation of why this is required.

[Copilot]

Performance concern: The OnTokenValidated event handler performs a database query on every authenticated request. For line 277-279, using FirstOrDefault() (synchronous) instead of FirstOrDefaultAsync() blocks a thread pool thread during the database I/O operation.

This creates a bottleneck under load because:

1. Each authenticated request waits synchronously for a database query
2. Thread pool threads are blocked during I/O, reducing overall request throughput
3. The performance impact compounds with concurrent requests

Change FirstOrDefault() to await FirstOrDefaultAsync() to properly use async I/O and avoid thread pool starvation. The method signature already supports async (async context =>), so this is straightforward to fix.

[Copilot]

Inconsistent security stamp update: The ChangePasswordAsync method explicitly calls UpdateSecurityStampAsync (line 431) after changing the password, but according to ASP.NET Core Identity documentation, the ChangePasswordAsync method already updates the security stamp automatically.

Calling UpdateSecurityStampAsync twice in succession could cause the second update to generate a different stamp than the first, potentially creating a race condition where:

1. ChangePasswordAsync updates security stamp to value A
2. UpdateSecurityStampAsync immediately updates it to value B
3. If a token was issued between steps 1 and 2 (unlikely but possible), it would have stamp A and be immediately invalid

Additionally, the RemovePasswordAsync method (line 467) does NOT explicitly update the security stamp, creating an inconsistency: password changes invalidate tokens, but password removal does not. Both operations are security-sensitive and should have consistent token invalidation behavior.

Recommendation: Remove the explicit UpdateSecurityStampAsync call on line 431 since ChangePasswordAsync already handles it, OR add the same explicit call to RemovePasswordAsync for consistency.

Suggested change

	// Explicitly update security stamp to invalidate existing JWTs
	_ = await userManager.UpdateSecurityStampAsync(appUser);

[Copilot]

Resource leak: DocumentStore instance in ClassSetup is not properly disposed. Change "using var store" to "await using var store" to properly dispose async resources and prevent connection pool leaks. See DatabaseHelpers.cs lines 73-82 for detailed explanation of why this is required.

[Copilot]

Inconsistent tenant handling: The DefaultTenantAlias is documented as for "test scenarios" (line 14), but it's being used in production code (MartenTenantStore.cs line 14). This alias should either:

1. Be clearly documented as supported for production use, not just tests
2. Or be removed from production code paths and kept test-only

Having production code accept a "test-only" alias creates confusion and potential issues if the alias behavior differs between environments. Additionally, the TenantResolutionMiddleware doesn't normalize "default" to "DEFAULT", which could lead to inconsistent behavior where some code paths accept the alias and others don't.

Suggested change

	/// Human-readable alias for the default tenant that can be used in test scenarios.
	/// This maps to DefaultTenantId ("*DEFAULT*").
	/// </summary>
	/// Human-readable alias for the default tenant that can be used wherever a stable
	/// identifier is needed (for example in routes, configuration, or tests).
	/// This maps to DefaultTenantId ("*DEFAULT*").

[Copilot]

This appears to be a temporary diagnostic test file that should not be committed to the repository. The file is explicitly named "DiagnoseRegistrationTests" and includes comments like "Temporary test to diagnose 400 Bad Request registration failures" (line 9).

Diagnostic/debug tests like these should be:

1. Removed before merging, or
2. Marked with [Explicit] or similar annotation to prevent them from running in CI/CD pipelines

These tests output diagnostic console logs and were clearly created to troubleshoot a specific issue. Once the issue is resolved (which it appears to be, given the password generator fix), this file should be removed to avoid cluttering the test suite.

[Copilot]

Resource leak: DocumentStore instance is not properly disposed. Change "using var store" to "await using var store" to properly dispose async resources and prevent connection pool leaks. See DatabaseHelpers.cs lines 73-82 for detailed explanation of why this is required.

[Copilot]

Resource leak: DocumentStore instance in ClassSetup is not properly disposed. Change "using var store" to "await using var store" to properly dispose async resources and prevent connection pool leaks. See DatabaseHelpers.cs lines 73-82 for detailed explanation of why this is required.

[Copilot]

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

src/BookStore.ApiService/Endpoints/JwtAuthenticationEndpoints.cs:390

• Potential data inconsistency: user fetched from global session but updated via tenant-scoped UserManager. The user is fetched using a global session query (line 318), but updates are done via UserManager (lines 335, 380, 390) which is scoped to the current tenant context. This could lead to inconsistencies if the user's actual tenant differs from tenantContext.TenantId. The cross-tenant lockout logic (lines 353-368) attempts to handle this, but the subsequent UserManager calls at lines 380 and 390 may still target the wrong tenant.

        // 1. Find user with this refresh token across ALL tenants (needed for cross-tenant theft detection)
        await using var globalSession = store.LightweightSession();
        var user = await globalSession.Query<ApplicationUser>()
            .FirstOrDefaultAsync(u => u.AnyTenant() && u.RefreshTokens.Any(rt => rt.Token == request.RefreshToken), cancellationToken);

        if (user == null)
        {
            Log.Users.RefreshFailedTokenNotFound(logger);
            return Result.Failure(Error.Unauthorized(ErrorCodes.Auth.InvalidToken, "Invalid refresh token.")).ToProblemDetails();
        }

        // 2. Validate token exists and is not expired
        var existingToken = user.RefreshTokens.FirstOrDefault(rt => rt.Token == request.RefreshToken);
        if (existingToken == null || existingToken.Expires <= DateTimeOffset.UtcNow)
        {
            Log.Users.RefreshFailedTokenExpiredOrInvalid(logger, user.UserName);
            if (existingToken != null)
            {
                _ = user.RefreshTokens.Remove(existingToken);
                _ = await userManager.UpdateAsync(user);
            }

            return Result.Failure(Error.Unauthorized(ErrorCodes.Auth.TokenExpired, "Refresh token expired or invalid.")).ToProblemDetails();
        }

        // 3. Validate tenant matches the token's original tenant (security: prevent cross-tenant token theft)
        if (!string.Equals(existingToken.TenantId, tenantContext.TenantId, StringComparison.OrdinalIgnoreCase))
        {
            // CRITICAL SECURITY VIOLATION: Cross-tenant token theft attempt
            Log.Users.CrossTenantTokenTheft(logger, user.Id, existingToken.TenantId, tenantContext.TenantId);

            // Lock account and clear all refresh tokens.
            // NOTE: We bypass UserManager here because it is scoped to an IDocumentSession.
            // The user's actual home tenant may differ from tenantContext.TenantId:
            //   - Spoofed token scenario: user IS in tenantContext.TenantId, token.TenantId is fake
            //   - Cross-tenant use scenario: user IS in existingToken.TenantId, request targets wrong tenant
            // Determine the home tenant by checking which one actually contains the user.
            var lockTenantId = tenantContext.TenantId;
            await using (var checkSession = store.QuerySession(tenantContext.TenantId))
            {
                var userInCurrentTenant = await checkSession.LoadAsync<ApplicationUser>(user.Id, cancellationToken);
                if (userInCurrentTenant is null)
                {
                    lockTenantId = existingToken.TenantId;
                }
            }

            user.LockoutEnd = DateTimeOffset.UtcNow.AddHours(24);
            user.LockoutEnabled = true;
            user.RefreshTokens.Clear();
            await using var victimSession = store.LightweightSession(lockTenantId);
            victimSession.Update(user);
            await victimSession.SaveChangesAsync(cancellationToken);

            return Result.Failure(Error.Forbidden(ErrorCodes.Auth.SecurityViolation, "Security violation detected. Account has been locked.")).ToProblemDetails();
        }

        // 4. Validate security stamp hasn't changed (invalidates tokens after password change, passkey addition, etc.)
        if (!string.Equals(existingToken.SecurityStamp, user.SecurityStamp, StringComparison.Ordinal))
        {
            Log.Users.RefreshFailedSecurityStampMismatch(logger, user.UserName);

            // Remove the invalid token
            _ = user.RefreshTokens.Remove(existingToken);
            _ = await userManager.UpdateAsync(user);

            return Result.Failure(Error.Unauthorized(ErrorCodes.Auth.TokenExpired, "Refresh token has been invalidated due to security changes.")).ToProblemDetails();
        }

        // 5. Generate new tokens using the original tenant from the refresh token
        var roles = await userManager.GetRolesAsync(user);
        var newAccessToken = jwtTokenService.GenerateAccessToken(user, existingToken.TenantId, roles);
        var newRefreshToken = jwtTokenService.RotateRefreshToken(user, existingToken.TenantId, existingToken.Token);

        _ = await userManager.UpdateAsync(user);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Debug logging code left in production. The System.IO.File.AppendAllText calls to "/tmp/jwt_events.txt" should be removed before merging. These debug statements will cause performance issues and disk space consumption in production environments. They also write to a hardcoded path that may not exist or be writable in all deployment scenarios.

[Copilot]

Temporary diagnostic test should be removed before merging. This test file explicitly states it's "Temporary" and is meant to diagnose issues during development. It should not be included in the final pull request as it doesn't test actual functionality.

Suggested change

	/// <summary>
	/// Temporary test to diagnose 400 Bad Request registration failures
	/// </summary>
	public class DiagnoseRegistrationTests
	{
	readonly IIdentityClient _client;
	public DiagnoseRegistrationTests() => _client = RestService.For<IIdentityClient>(HttpClientHelpers.GetUnauthenticatedClient());
	[Test]
	public async Task DiagnoseRegistrationFailure_CaptureActualErrorMessage()
	{
	// Arrange
	var email = FakeDataGenerators.GenerateFakeEmail();
	var password = FakeDataGenerators.GenerateFakePassword();
	Console.WriteLine($"=== DIAGNOSTIC INFO ===");
	Console.WriteLine($"Generated Email: {email}");
	Console.WriteLine($"Generated Password: {password}");
	Console.WriteLine($"Password Length: {password.Length}");
	Console.WriteLine($"Has Uppercase: {password.Any(char.IsUpper)}");
	Console.WriteLine($"Has Lowercase: {password.Any(char.IsLower)}");
	Console.WriteLine($"Has Digit: {password.Any(char.IsDigit)}");
	Console.WriteLine($"Has Special: {password.Any(c => !char.IsLetterOrDigit(c))}");
	Console.WriteLine($"========================");
	try
	{
	var response = await _client.RegisterAsync(new RegisterRequest(email, password));
	Console.WriteLine("✅ Registration SUCCEEDED");
	Console.WriteLine($"Access Token (first 20 chars): {response.AccessToken[..20]}...");
	}
	catch (ApiException ex)
	{
	Console.WriteLine($"❌ Registration FAILED");
	Console.WriteLine($"Status Code: {ex.StatusCode}");
	Console.WriteLine($"Raw Response Content: {ex.Content}");
	// Parse ProblemDetails to get error code
	var problemDetails = await ex.GetContentAsAsync<AuthenticationHelpers.ValidationProblemDetails>();
	Console.WriteLine($"\n=== PROBLEM DETAILS ===");
	Console.WriteLine($"Title: {problemDetails?.Title}");
	Console.WriteLine($"Status: {problemDetails?.Status}");
	Console.WriteLine($"Detail: {problemDetails?.Detail}");
	Console.WriteLine($"Error Code: {problemDetails?.Error}");
	Console.WriteLine($"=======================");
	// Fail the test with detailed info
	throw new Exception(
	$"Registration failed with {ex.StatusCode}\n" +
	$"Error Code: {problemDetails?.Error}\n" +
	$"Detail: {problemDetails?.Detail}",
	ex);
	}
	}
	[Test]
	public async Task DiagnosePasswordGeneration_Multiple()
	{
	Console.WriteLine($"=== TESTING 10 PASSWORD GENERATIONS ===");
	for (var i = 1; i <= 10; i++)
	{
	var password = FakeDataGenerators.GenerateFakePassword();
	var hasUpper = password.Any(char.IsUpper);
	var hasLower = password.Any(char.IsLower);
	var hasDigit = password.Any(char.IsDigit);
	var hasSpecial = password.Any(c => !char.IsLetterOrDigit(c));
	var meetsLength = password.Length >= 8;
	var meetsAll = hasUpper && hasLower && hasDigit && hasSpecial && meetsLength;
	Console.WriteLine($"#{i}: '{password}' | Len:{password.Length} U:{hasUpper} L:{hasLower} D:{hasDigit} S:{hasSpecial} | OK:{meetsAll}");
	if (!meetsAll)
	{
	throw new Exception($"Password #{i} does not meet requirements: {password}");
	}
	}
	Console.WriteLine($"✅ All 10 passwords meet requirements");
	}
	}
	// Temporary diagnostic tests for registration/password issues were removed
	// to avoid merging non-functional diagnostics into the main test suite.

[Copilot]

Breaking change to RefreshTokenInfo record without migration strategy. Adding the SecurityStamp parameter to the RefreshTokenInfo record is a breaking change that will cause deserialization failures for existing refresh tokens stored in the database. Existing ApplicationUser documents with refresh tokens will fail to deserialize because they lack the SecurityStamp field. Consider making the parameter optional with a default value, or implement a migration strategy to handle existing data.

Suggested change

	string SecurityStamp);
	string? SecurityStamp = null);

[Copilot]

Non-deterministic test with no assertions. The GenerateFakePassword_OutputSamples test only outputs sample data to the console without any assertions. This is useful for development debugging but should either have assertions to validate the output or be removed before merging. Consider removing this test or converting it to a proper validation test.

[Copilot]

Rate limit rejection response doesn't follow ProblemDetails pattern. The OnRejected handler returns plain JSON instead of RFC 7807 ProblemDetails format with a machine-readable error code. This is inconsistent with the codebase's error handling conventions (as noted in AGENTS.md line 27: "ALL failures MUST return RFC 7807 ProblemDetails with a machine-readable error code"). The response should use the ProblemDetails format with an appropriate error code like ErrorCodes.RateLimit.TooManyRequests.

[Copilot]

Unused helper method. The GetTenantPairs method is defined but never used in any tests. Either use this method as a data source for parameterized tests using TUnit's ArgumentsSource attribute, or remove it if it's not needed.

[Copilot]

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated 12 comments.

Comments suppressed due to low confidence (1)

tests/BookStore.AppHost.Tests/RateLimitTests.cs:38

• The global test harness disables rate limiting (--RateLimit:Disabled=true), and the rate-limiter returns NoLimiter when disabled, so this test no longer validates quota consumption or headers. Either run this test suite with rate limiting enabled (high permit limits), or explicitly enable rate limiting for this test so it asserts actual rate-limit behavior (e.g., headers or 429 responses).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

These tests create a new DocumentStore via DocumentStore.For(...) and dispose it with using var. Prefer using the shared DatabaseHelpers.GetDocumentStoreAsync() (pool limits, shared config) and the documented await using disposal pattern to reduce connection pressure in parallel runs.

[Copilot]

The custom OnChallenge response returns plain JSON (application/json) with error/error_description, which doesn’t follow the repo’s “all failures return RFC 7807 ProblemDetails with a machine-readable error code” convention. Consider returning application/problem+json with the standard fields and an error code (or relying on existing ToProblemDetails() handling) so auth failures are consistent for clients.

[Copilot]

Missing using TUnit; (or a project-level <Using Include="TUnit" />) means [Test] won’t resolve and this file won’t compile.

[Copilot]

Missing using TUnit; (or a project-level <Using Include="TUnit" />) means [Test] won’t resolve and this file won’t compile.

[Copilot]

These tests create a new DocumentStore via DocumentStore.For(...) instead of using DatabaseHelpers.GetDocumentStoreAsync() (which centralizes Marten configuration and connection-pool limits) and they dispose it with using rather than the await using pattern documented in DatabaseHelpers. Prefer await using var store = await DatabaseHelpers.GetDocumentStoreAsync(); and remove the duplicated GetStoreAsync() to avoid connection-pool exhaustion in parallel runs.

[Copilot]

This file duplicates Marten DocumentStore.For(...) setup and uses using var store = await GetStoreAsync() in multiple helpers. Prefer DatabaseHelpers.GetDocumentStoreAsync() (shared pool limits/config) and await using disposal to avoid leaking connections under parallel test execution.

[Copilot]

"default" is treated as a valid tenant by ITenantStore, but the middleware passes it straight into tenantContext.Initialize(tenantId). Since TenantContext defaults to *DEFAULT* and doesn’t map aliases, sending X-Tenant-ID: default will route requests to a completely different Marten tenant bucket ("default") instead of the real default tenant (*DEFAULT*). Map MultiTenancyConstants.DefaultTenantAlias to MultiTenancyConstants.DefaultTenantId (or StorageConstants.DefaultTenantId) before initializing the context so the alias truly behaves as an alias.

[Copilot]

u.AnyTenant() isn’t defined anywhere in the repo and this file doesn’t import any Marten extension namespace that could provide it, so this query won’t compile as written. If the intent is to query refresh tokens across all tenants, use the supported Marten multi-tenancy query API/extension (and add the required using), or restructure the query to explicitly remove the tenant filter in a way Marten supports.

Suggested change

	// 1. Find user with this refresh token across ALL tenants (needed for cross-tenant theft detection)
	await using var globalSession = store.LightweightSession();
	var user = await globalSession.Query<ApplicationUser>()
	.FirstOrDefaultAsync(u => u.AnyTenant() && u.RefreshTokens.Any(rt => rt.Token == request.RefreshToken), cancellationToken);
	// 1. Find user with this refresh token (for theft detection)
	await using var globalSession = store.LightweightSession();
	var user = await globalSession.Query<ApplicationUser>()
	.FirstOrDefaultAsync(u => u.RefreshTokens.Any(rt => rt.Token == request.RefreshToken), cancellationToken);

[Copilot]

GenerateFakeTenantId() uses Bogus.Internet.DomainWord(), which doesn’t guarantee TenantIdValidator constraints (min length 3, lowercase-only, no leading/trailing hyphen, only [a-z0-9-]). This can create intermittent failures when tenant creation/validation rejects the generated ID. Generate IDs by explicitly enforcing the validator rules (e.g., a lowercase slug with a minimum length and trimmed hyphens).

[Copilot]

This is labeled as a “Temporary” diagnostic test and prints generated passwords and raw server responses to the test output. That’s noisy for CI runs and can unintentionally leak sensitive information if this pattern gets copied. Consider removing this file from the committed test suite, or refactoring into a normal assertion-based test without dumping secrets/response bodies.

[github-actions]

Package	Line Rate	Branch Rate	Health
BookStore.Shared	17%	38%	❌
BookStore.Shared	16%	10%	❌
BookStore.ApiService	11%	13%	❌
JasperFx	0%	0%	❌
Summary	11% (1370 / 22429)	22% (596 / 10049)	❌

[Copilot]

Pull request overview

Copilot reviewed 84 out of 84 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.