Add unit tests for authentication services and related DTOs #2
Conversation
- Introduced comprehensive unit tests for AuthService, covering registration and login functionalities. - Added validation tests for username, email, and password inputs to ensure robust error handling. - Created DTO tests for AuthResult, LoginRequest, and RegisterRequest to validate data integrity. - Implemented User and PasswordHasher tests to verify user entity behavior and password hashing logic. - Established a TestData utility class to streamline test data creation and management. - Enhanced overall test coverage for the authentication module, ensuring reliability and maintainability.
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 192 files out of 299 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds extensive server-side tests and test utilities; tightens input validation and normalization in AuthService and User; strengthens password hashing and JWT handling; configures EF Core model with unique indexes and SQLite behavior; updates repository queries; adds CI and code-quality workflows; adjusts project references and .gitignore; removes a backup package.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant CI as GitHub Actions CI
participant MSBuild
participant Client as MySvelteApp.Client
participant Server as MySvelteApp.Server
Developer->>CI: push / PR
CI->>Server: dotnet restore/build/test
CI->>Client: npm ci && npm run build (unless SkipClientBuild)
CI->>CI: run client checks (lint/type/test) and server tests
CI->>CI: upload artifacts (test-results, reports)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit tests for the authentication module, focusing on testing core authentication services, DTOs, and related components. The goal is to improve test coverage and ensure reliability of the authentication system.
Key changes include:
- Added comprehensive unit tests for AuthService with registration and login validation
- Created test utilities (TestData and TestHelper) to streamline test data management
- Implemented tests for authentication DTOs and domain entities
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api-client-backup/package.json | Removed entire package.json file for client backup |
| MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs | Added helper utilities for creating test database contexts and JWT options |
| MySvelteApp.Server/Tests/TestUtilities/TestData.cs | Added centralized test data provider with users, requests, and JWT configurations |
| MySvelteApp.Server/Tests/MySvelteApp.Server.Tests.csproj | Added test project configuration with xUnit and testing dependencies |
| MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs | Added comprehensive tests for password hashing and verification logic |
| MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs | Added repository tests for user CRUD operations and database interactions |
| MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs | Added JWT token generation and validation tests |
| MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs | Added basic entity tests for User domain model |
| MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs | Added DTO tests for registration request validation |
| MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs | Added DTO tests for login request validation |
| MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs | Added DTO tests for authentication result structures |
| MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs | Added enum tests for authentication error types |
| MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs | Added validation logic tests for authentication inputs |
| MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs | Added comprehensive AuthService tests for registration and login flows |
| MySvelteApp.Server/MySvelteApp.Server.csproj | Modified to exclude test files from main project compilation and made client build conditional |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| using var newContext = TestHelper.CreateInMemoryDbContext("DifferentDb"); | ||
|
|
||
| var persistedUser = await newContext.Users.FirstOrDefaultAsync(u => u.Username == newUser.Username); | ||
| persistedUser.Should().BeNull(); // Should not exist in different database |
There was a problem hiding this comment.
This test is creating a different database instance ('DifferentDb') but expecting the user to not exist, which will always be true since it's a separate database. This doesn't properly test the persistence behavior. The test should either use the same database name to verify persistence or use a fresh context with the same database to test that changes were actually saved.
| using var newContext = TestHelper.CreateInMemoryDbContext("DifferentDb"); | |
| var persistedUser = await newContext.Users.FirstOrDefaultAsync(u => u.Username == newUser.Username); | |
| persistedUser.Should().BeNull(); // Should not exist in different database | |
| using var newContext = TestHelper.CreateInMemoryDbContext(TestHelper.DefaultDbName); | |
| var persistedUser = await newContext.Users.FirstOrDefaultAsync(u => u.Username == newUser.Username); | |
| persistedUser.Should().NotBeNull(); // Should exist in the same database | |
| persistedUser!.Username.Should().Be(newUser.Username); | |
| persistedUser.Email.Should().Be(newUser.Email); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
|
||
| // Assert | ||
| result.Should().BeFalse(); | ||
| } |
There was a problem hiding this comment.
Bug: Email Comparison Case Sensitivity Issue
The EmailExistsAsync_WithCaseInsensitiveEmail_ShouldReturnTrue test's assertion expects false when checking an email with different casing. This contradicts the test name and suggests the EmailExistsAsync method is case-sensitive, which isn't typical for email address comparisons.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (25)
MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs (1)
102-139: Rename and slightly strengthen the equality test for clarity.
Currently it only asserts reference inequality but the name suggests full equality semantics.Apply:
- public void User_ShouldSupportEqualityComparison() + public void Users_WithSameValues_AreNotReferenceEqual() { @@ - // Assert - Reference equality + // Assert - Reference equality (and default Equals) user1.Should().NotBeSameAs(user2); user1.Should().NotBeSameAs(user3); + user1.Equals(user2).Should().BeFalse();MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs (6)
237-252: Make tampering deterministic to avoid rare false negatives.Using string.Replace can be a no-op if the first char already equals 'X' (~1/64 chance), leaving the original Base64 intact and making the test flaky. Flip a byte after Base64‑decoding to guarantee a change while keeping the string valid.
Apply these diffs:
@@ - // Tamper with the hash - var tamperedHash = hash.Replace(hash[0], 'X'); + // Tamper with the hash (guaranteed change and valid Base64) + var hashBytes = Convert.FromBase64String(hash); + hashBytes[0] ^= 0x01; + var tamperedHash = Convert.ToBase64String(hashBytes); @@ - // Tamper with the salt - var tamperedSalt = salt.Replace(salt[0], 'X'); + // Tamper with the salt (guaranteed change and valid Base64) + var saltBytes = Convert.FromBase64String(salt); + saltBytes[0] ^= 0x01; + var tamperedSalt = Convert.ToBase64String(saltBytes);Also applies to: 254-269
150-162: Add a companion test for HashPassword(null).You assert VerifyPassword(null, …) throws. Mirror that for HashPassword to make the null‑input contract explicit.
Example:
[Fact] public void HashPassword_WithNullPassword_ShouldThrow() { Action act = () => _passwordHasher.HashPassword(null!); act.Should().Throw<ArgumentNullException>(); }
47-60: Remove redundant test; it duplicates salt assertion.“GenerateDifferentHashesForSamePassword” already asserts both hash and salt differ. “GenerateDifferentSaltsForSamePassword” repeats the salt check.
Apply this diff to drop the duplicate:
- [Fact] - public void HashPassword_ShouldGenerateDifferentSaltsForSamePassword() - { - // Arrange - const string password = "TestPassword123"; - - // Act - var (_, salt1) = _passwordHasher.HashPassword(password); - var (_, salt2) = _passwordHasher.HashPassword(password); - - // Assert - salt1.Should().NotBe(salt2); - }Also applies to: 62-74
90-103: Collapse similar negative verification cases into a single Theory.These four tests differ only by the input. A parameterized Theory will reduce repetition and speed up maintenance.
Example:
[Theory] [InlineData("WrongPassword456")] [InlineData("testpassword123")] [InlineData("TestPassword124")] [InlineData("")] public void VerifyPassword_WithWrongInputs_ShouldReturnFalse(string candidate) { const string password = "TestPassword123"; var (hash, salt) = _passwordHasher.HashPassword(password); _passwordHasher.VerifyPassword(candidate, hash, salt).Should().BeFalse(); }Also applies to: 105-118, 120-133, 135-148
218-235: Test couples to a specific algorithm/key size; intentional?Asserting 64‑byte hash and 128‑byte key locks you to HMACSHA512 defaults. If you later migrate (e.g., PBKDF2/Argon2 or different key size), this breaks despite correct behavior.
Options:
- If intentional, add a comment/Trait to mark the contract as algorithm‑specific.
- Otherwise, relax to “> 0” or assert successful round‑trip verification only.
237-269: Add explicit tests for invalid Base64 inputs (should throw).Currently you only test tampering that remains valid Base64. Add coverage for invalid Base64 to assert exception behavior.
Example tests to append:
[Fact] public void VerifyPassword_WithInvalidBase64Hash_ShouldThrow() { var (hash, salt) = _passwordHasher.HashPassword("pw"); Action act = () => _passwordHasher.VerifyPassword("pw", "###", salt); act.Should().Throw<FormatException>(); } [Fact] public void VerifyPassword_WithInvalidBase64Salt_ShouldThrow() { var (hash, salt) = _passwordHasher.HashPassword("pw"); Action act = () => _passwordHasher.VerifyPassword("pw", hash, "###"); act.Should().Throw<FormatException>(); }.gitignore (1)
39-42: Redundant ignore rule; add TestResults instead.
bin/andobj/patterns already ignore those folders repo‑wide.MySvelteApp.Server/Tests/objis unnecessary. Consider removing it and ignoringTestResults/whichdotnet testcreates.### OS junk ### .DS_Store Thumbs.db -MySvelteApp.Server/Tests/obj +TestResults/MySvelteApp.Server/MySvelteApp.Server.csproj (2)
39-41: Gating the client build is good.Condition looks right and keeps server builds fast when
SkipClientBuild=true. Recommend documenting this env var in CI to speed test-only jobs.
44-47: Exclude tests from server compile: LGTM; optionally exclude other item types.This prevents nested Tests/*.cs from leaking into the web project. Optionally also exclude None/Content to avoid accidental publish of test assets.
<!-- Exclude test sources from the main web project compilation --> <ItemGroup> <Compile Remove="Tests/**/*.cs" /> + <None Remove="Tests/**" /> + <Content Remove="Tests/**" /> </ItemGroup>MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs (1)
43-58: Null-allowing test conflicts with non-nullable semantics.If RegisterRequest uses non-nullable
string(common with<Nullable>enable</Nullable>), asserting it “allows null” undermines the contract. Prefer testing null handling through service validation, or make DTO propertiesstring?.Proposed: remove this test (keep null-path covered in AuthService tests).
- [Fact] - public void RegisterRequest_ShouldAllowNullValues() - { - // Act - var request = new RegisterRequest - { - Username = null!, - Email = null!, - Password = null! - }; - - // Assert - request.Username.Should().BeNull(); - request.Email.Should().BeNull(); - request.Password.Should().BeNull(); - }MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (2)
6-6: Remove self-namespace using.
using MySvelteApp.Server.Tests.TestUtilities;is redundant inside the same namespace.-using MySvelteApp.Server.Tests.TestUtilities;
36-40: Stronger cleanup for shared stores.If you ever pass a fixed
dbName,EnsureDeleted()resets the store fully across contexts.- public static void ClearDatabase(AppDbContext context) - { - context.Users.RemoveRange(context.Users); - context.SaveChanges(); - } + public static void ClearDatabase(AppDbContext context) + { + context.Database.EnsureDeleted(); + }MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs (1)
10-17: Asserting underlying int values may be brittle.If integer values aren’t an external contract (e.g., persisted or API’d), consider dropping these to avoid coupling to enum ordering.
MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs (1)
74-96: “Immutable” test name is misleading; it doesn’t verify immutability.This only asserts assigned values. Consider renaming to avoid implying
initenforcement.- public void AuthResult_ShouldBeImmutable() + public void AuthResult_InitAssignments_ShouldPersistValues()MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs (2)
13-31: Test reimplements production validation logic — high drift risk.Duplicating private logic in tests can mask regressions. Prefer:
- testing via AuthService public methods (already covered), or
- extracting validators to a dedicated service/static helper and mark internals visible to tests.
169-176: Email double-dot local part treated as valid — confirm requirements.
user..user@example.comis accepted by the test but often rejected. Align with product rules.MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs (3)
56-63: Avoid ToDictionary on claims to prevent key-collision errorsMultiple claims of the same type (e.g., aud) can throw. Prefer predicate-based checks.
Apply this diff:
- var claims = jwtToken.Claims.ToDictionary(c => c.Type, c => c.Value); - - claims[ClaimTypes.NameIdentifier].Should().Be(user.Id.ToString()); - claims[JwtRegisteredClaimNames.Sub].Should().Be(user.Id.ToString()); - claims[ClaimTypes.Name].Should().Be(user.Username); - claims[JwtRegisteredClaimNames.Jti].Should().NotBeNullOrEmpty(); - claims[JwtRegisteredClaimNames.Iat].Should().NotBeNullOrEmpty(); + var claims = jwtToken.Claims.ToList(); + + claims.Should().Contain(c => c.Type == ClaimTypes.NameIdentifier && c.Value == user.Id.ToString()); + claims.Should().Contain(c => c.Type == JwtRegisteredClaimNames.Sub && c.Value == user.Id.ToString()); + claims.Should().Contain(c => c.Type == ClaimTypes.Name && c.Value == user.Username); + claims.Count(c => c.Type == JwtRegisteredClaimNames.Jti).Should().BeGreaterThan(0); + claims.First(c => c.Type == JwtRegisteredClaimNames.Jti).Value.Should().NotBeNullOrEmpty(); + claims.Count(c => c.Type == JwtRegisteredClaimNames.Iat).Should().BeGreaterThan(0); + claims.First(c => c.Type == JwtRegisteredClaimNames.Iat).Value.Should().NotBeNullOrEmpty();And similarly below:
- var claims1 = jwtToken1.Claims.ToDictionary(c => c.Type, c => c.Value); - var claims2 = jwtToken2.Claims.ToDictionary(c => c.Type, c => c.Value); + var claims1 = jwtToken1.Claims.ToList(); + var claims2 = jwtToken2.Claims.ToList(); - claims1[ClaimTypes.NameIdentifier].Should().NotBe(claims2[ClaimTypes.NameIdentifier]); - claims1[ClaimTypes.Name].Should().NotBe(claims2[ClaimTypes.Name]); + claims1.First(c => c.Type == ClaimTypes.NameIdentifier).Value + .Should().NotBe(claims2.First(c => c.Type == ClaimTypes.NameIdentifier).Value); + claims1.First(c => c.Type == ClaimTypes.Name).Value + .Should().NotBe(claims2.First(c => c.Type == ClaimTypes.Name).Value);Also applies to: 102-107
180-182: Reduce flakiness in expiry assertion5s tolerance is tight under CI load; align to 1 minute like the earlier test.
Apply this diff:
- jwtToken.ValidTo.Should().BeCloseTo(expectedExpiration, TimeSpan.FromSeconds(5)); + jwtToken.ValidTo.Should().BeCloseTo(expectedExpiration, TimeSpan.FromMinutes(1));
32-41: Also validate the token’s signature and lifetimeReading the token doesn’t prove it’s valid. Add a ValidateToken step with the configured key.
Add after reading jwtToken:
using Microsoft.IdentityModel.Tokens; using System.Text; // Build the signing key similar to JwtTokenGenerator var keyString = _jwtOptions.Value.Key; var keyBytes = keyString.StartsWith("base64:", StringComparison.OrdinalIgnoreCase) ? Convert.FromBase64String(keyString.Substring("base64:".Length)) : Encoding.UTF8.GetBytes(keyString); var validationParams = new TokenValidationParameters { ValidateIssuerSigningKey = true, IssuerSigningKey = new SymmetricSecurityKey(keyBytes), ValidateIssuer = true, ValidIssuer = _jwtOptions.Value.Issuer, ValidateAudience = true, ValidAudience = _jwtOptions.Value.Audience, ValidateLifetime = true, ClockSkew = TimeSpan.FromMinutes(1) }; new JwtSecurityTokenHandler().ValidateToken(token, validationParams, out _);MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (3)
57-62: Add assertion for UserId on successful registrationResult should carry the created user’s id if available.
Apply this diff:
result.Success.Should().BeTrue(); result.Token.Should().Be(expectedToken); result.Username.Should().Be(request.Username); + result.UserId.Should().NotBeNull(); result.ErrorMessage.Should().BeNull(); result.ErrorType.Should().Be(AuthErrorType.None);
85-86: Avoid brittle equality on error messagesExact string matches create noisy failures on copy changes. Prefer constants, partial matches, or error codes.
Example diff:
- result.ErrorMessage.Should().Be("This username is already taken. Please choose a different one."); + result.ErrorMessage.Should().Contain("username is already taken");Better: centralize messages in a TestConstants/ErrorMessages helper and assert equality against those constants.
Also applies to: 113-114, 131-133, 148-150, 163-165, 178-180, 193-195, 208-210, 223-225, 306-308, 331-333, 348-350, 365-367
370-391: Consider testing normalization on registration tooYou test trimming at login and email normalization at registration; add a case for username trimming on registration as well.
Example new test (outside this hunk):
[Fact] public async Task RegisterAsync_ShouldTrimUsername() { var request = new RegisterRequest { Username = " testuser ", Email = "TEST@EXAMPLE.COM", Password = "ValidPassword123" }; _userRepositoryMock.Setup(x => x.UsernameExistsAsync("testuser", default)).ReturnsAsync(false); _userRepositoryMock.Setup(x => x.EmailExistsAsync("test@example.com", default)).ReturnsAsync(false); _passwordHasherMock.Setup(x => x.HashPassword(request.Password)).Returns(("hash","salt")); _jwtTokenGeneratorMock.Setup(x => x.GenerateToken(It.IsAny<User>())).Returns("token"); await _authService.RegisterAsync(request); _userRepositoryMock.Verify(x => x.AddAsync(It.Is<User>(u => u.Username == "testuser"), default), Times.Once); }MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
83-90: Add a reusable plain-text JWT key constantAvoid in-test literals; keeps Jwt tests uniform.
Apply this diff:
public static class Jwt { public const string ValidKey = "base64:YWJjZGVmZ2hpamsxMjM0NTY3ODkwYWJjZGVmZ2hpams="; // 32+ bytes when decoded public const string ShortKey = "short"; public const string ValidIssuer = "TestIssuer"; public const string ValidAudience = "TestAudience"; public const int ValidLifetimeHours = 24; + public const string ValidPlainTextKey = "ThisIsAVeryLongPlainTextKeyThatIsDefinitelyLongerThan32CharactersForTesting"; }Then use TestData.Jwt.ValidPlainTextKey in JwtTokenGeneratorTests.
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (1)
60-73: Case sensitivity for usernames is provider-dependentInMemory is case-sensitive; SQL Server defaults are not. This test may not reflect production behavior.
Either:
- Normalize usernames (e.g., lower) in repository queries and assert normalization, or
- Explicitly document case-sensitive semantics and configure a case-sensitive collation in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api-client-backup/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.gitignore(1 hunks)MySvelteApp.Server/MySvelteApp.Server.csproj(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs(1 hunks)MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs(1 hunks)MySvelteApp.Server/Tests/MySvelteApp.Server.Tests.csproj(1 hunks)MySvelteApp.Server/Tests/TestUtilities/TestData.cs(1 hunks)MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs(1 hunks)api-client-backup/package.json(0 hunks)
💤 Files with no reviewable changes (1)
- api-client-backup/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:04:00.832Z
Learnt from: CR
PR: refactor-gremlin/svelte-NET-Test#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T07:04:00.832Z
Learning: Applies to MySvelteApp.Client/tsconfig.json : Keep TypeScript strict mode enabled
Applied to files:
MySvelteApp.Server/MySvelteApp.Server.csproj
🧬 Code graph analysis (12)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs (2)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Authentication(39-80)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (16)
Fact(33-68)Fact(70-93)Fact(95-118)Fact(120-135)Fact(137-150)Fact(152-165)Fact(167-180)Fact(182-195)Fact(197-210)Fact(212-225)Fact(227-258)Fact(263-289)Fact(291-310)Fact(312-335)Fact(337-352)Fact(354-367)
MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs (3)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (4)
Jwt(83-90)Authentication(39-80)TestData(6-91)Users(8-35)MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (2)
IOptions(21-28)TestHelper(10-41)MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(40-60)
MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (3)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (4)
Authentication(39-80)TestData(6-91)Jwt(83-90)Users(8-35)MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(40-60)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs (2)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Authentication(39-80)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (16)
Fact(33-68)Fact(70-93)Fact(95-118)Fact(120-135)Fact(137-150)Fact(152-165)Fact(167-180)Fact(182-195)Fact(197-210)Fact(212-225)Fact(227-258)Fact(263-289)Fact(291-310)Fact(312-335)Fact(337-352)Fact(354-367)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs (1)
MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (16)
Fact(33-68)Fact(70-93)Fact(95-118)Fact(120-135)Fact(137-150)Fact(152-165)Fact(167-180)Fact(182-195)Fact(197-210)Fact(212-225)Fact(227-258)Fact(263-289)Fact(291-310)Fact(312-335)Fact(337-352)Fact(354-367)
MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs (1)
MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs (1)
PasswordHasher(7-24)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs (2)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Authentication(39-80)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (16)
Fact(33-68)Fact(70-93)Fact(95-118)Fact(120-135)Fact(137-150)Fact(152-165)Fact(167-180)Fact(182-195)Fact(197-210)Fact(212-225)Fact(227-258)Fact(263-289)Fact(291-310)Fact(312-335)Fact(337-352)Fact(354-367)
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (3)
MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (3)
AppDbContext(12-19)TestHelper(10-41)Task(30-34)MySvelteApp.Server/Tests/TestUtilities/TestData.cs (2)
TestData(6-91)Users(8-35)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (2)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (4)
Authentication(39-80)TestData(6-91)Requests(37-81)Users(8-35)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs (1)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Authentication(39-80)
MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs (1)
MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs (4)
9-21: Defaults test is correct and valuable coverage.
Covers all defaulted fields; aligns with current entity initializer defaults.
23-49: Property initialization test looks good.
Straightforward verification of setter/getter behavior with meaningful sample values.
70-81: Id mutability test is fine.
Confirms assignment works as expected.
83-100: Properties mutability test is fine.
Covers all user string fields.MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs (2)
164-181: Empty passwords “work” here—confirm upstream validation blocks them.Hasher accepting empty strings is fine at this layer, but ensure higher layers (validators/AuthService) reject empty passwords.
1-15: LGTM overall: clear Arrange‑Act‑Assert and focused coverage.Solid test structure, file‑scoped namespace, and reusability via a shared SUT. Once the small nits above are addressed, this looks great.
MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs (1)
10-19: Defaults test: good coverage.Asserting DTO defaults to empty strings is fine.
MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs (1)
20-27: Names test: fine.Verifying
ToString()names is reasonable.MySvelteApp.Server/Tests/MySvelteApp.Server.Tests.csproj (2)
13-28: Packages/TFM look consistent for .NET 9 tests.LGTM.
30-32: No inconsistent field names found — tests consistently use _authService. Search across MySvelteApp.Server/Tests for _auth_service/_authService found only _authService (e.g., MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs).MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs (3)
9-22: Defaults test: good.Covers the expected initial state clearly.
24-48: Success-path properties test: good.Straightforward and precise.
50-72: Error-path properties test: good.Validates mutually exclusive success/error fields.
MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs (1)
240-266: Password tests are clear and comprehensive.Good coverage of boundaries and character classes.
MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs (1)
10-18: LGTM on default values testGood guard against regressions if DTO initializers are removed.
MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs
Outdated
Show resolved
Hide resolved
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs
Show resolved
Hide resolved
| [Fact] | ||
| public async Task AddAsync_WithDuplicateUsername_ShouldStillAddToDatabase() | ||
| { | ||
| // Arrange | ||
| var user1 = TestData.Users.ValidUser; | ||
| var user2 = new User | ||
| { | ||
| Username = user1.Username, // Same username | ||
| Email = "different@example.com", | ||
| PasswordHash = "different_hash", | ||
| PasswordSalt = "different_salt" | ||
| }; | ||
|
|
||
| await TestHelper.SeedUsersAsync(_dbContext, user1); | ||
|
|
||
| // Act - EF should handle this, but we're testing the repository method | ||
| await _userRepository.AddAsync(user2); | ||
|
|
||
| // Assert | ||
| var usersWithSameUsername = await _dbContext.Users | ||
| .Where(u => u.Username == user1.Username) | ||
| .ToListAsync(); | ||
|
|
||
| usersWithSameUsername.Should().HaveCount(2); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t codify acceptance of duplicate usernames
Persisting duplicates is a data-integrity risk and can race AuthService’s “exists” check in production.
Recommended:
- Enforce uniqueness at the DB level (e.g., unique index on Users.Username and Users.Email) and handle DbUpdateException.
- Replace this test with one that asserts a uniqueness violation is raised. Note: EFCore.InMemory doesn’t enforce unique indexes; use SQLite in-memory for this test.
Example (AppDbContext):
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<User>().HasIndex(u => u.Username).IsUnique();
modelBuilder.Entity<User>().HasIndex(u => u.Email).IsUnique();
}Example test (SQLite) can be added separately if you want; I can draft it.
- Added GitHub Actions workflows for continuous integration and code quality checks. - Created a README.md file detailing CI/CD processes and local development instructions. - Updated .gitignore to exclude test results directory. - Enhanced .NET project file to include SQLite as a dependency for improved database handling. - Refactored user entity and authentication service to improve validation and error handling. - Expanded unit tests for authentication services, ensuring robust validation and error messaging. - Improved password hashing logic for enhanced security.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MySvelteApp.Server/Application/Authentication/AuthService.cs (1)
105-131: Validate trimmed username/email to avoid rejecting valid input with stray spaces.Currently validation runs on raw values; later you trim before persisting. Validate on trimmed values to keep behavior consistent.
- private static (string Message, AuthErrorType Type)? ValidateRegisterRequest(RegisterRequest request) + private static (string Message, AuthErrorType Type)? ValidateRegisterRequest(RegisterRequest request) { - if (string.IsNullOrWhiteSpace(request.Username)) + var username = request.Username?.Trim(); + var email = request.Email?.Trim(); + + if (string.IsNullOrWhiteSpace(username)) { return ("Username is required.", AuthErrorType.Validation); } - if (request.Username.Length < 3) + if (username.Length < 3) { return ("Username must be at least 3 characters long.", AuthErrorType.Validation); } - if (!UsernameRegex.IsMatch(request.Username)) + if (!UsernameRegex.IsMatch(username)) { return ("Username can only contain letters, numbers, and underscores.", AuthErrorType.Validation); } - if (string.IsNullOrWhiteSpace(request.Email)) + if (string.IsNullOrWhiteSpace(email)) { return ("Email is required.", AuthErrorType.Validation); } - if (!EmailRegex.IsMatch(request.Email)) + if (!EmailRegex.IsMatch(email)) { return ("Please enter a valid email address.", AuthErrorType.Validation); }
🧹 Nitpick comments (23)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs (2)
16-18: Use BeEmpty() for string-empty assertions.FluentAssertions has dedicated matchers for this; it reads cleaner.
- request.Username.Should().Be(string.Empty); - request.Email.Should().Be(string.Empty); - request.Password.Should().Be(string.Empty); + request.Username.Should().BeEmpty(); + request.Email.Should().BeEmpty(); + request.Password.Should().BeEmpty();
21-41: Option: broaden coverage with a Theory and multiple cases.Parameterize to cover varied inputs (dots, plus in email, strong password), keeping assertions identical.
- [Fact] - public void RegisterRequest_WithValidValues_ShouldSetPropertiesCorrectly() - { - // Arrange - const string username = "testuser"; - const string email = "test@example.com"; - const string password = "testpassword"; - - // Act - var request = new RegisterRequest - { - Username = username, - Email = email, - Password = password - }; - - // Assert - request.Username.Should().Be(username); - request.Email.Should().Be(email); - request.Password.Should().Be(password); - } + [Theory] + [InlineData("testuser", "test@example.com", "testpassword")] + [InlineData("user.name", "user+repo@example.co", "P@ssw0rd!")] + public void RegisterRequest_WithValues_ShouldSetProperties(string username, string email, string password) + { + var request = new RegisterRequest + { + Username = username, + Email = email, + Password = password + }; + + request.Username.Should().Be(username); + request.Email.Should().Be(email); + request.Password.Should().Be(password); + }MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs (1)
20-37: Broaden coverage with a Theory and multiple InlineData casesParametrize to cover a couple of representative username/password pairs without duplicating tests.
Apply this diff:
- [Fact] - public void LoginRequest_WithValidValues_ShouldSetPropertiesCorrectly() - { - // Arrange - const string username = "testuser"; - const string password = "testpassword"; - - // Act - var request = new LoginRequest - { - Username = username, - Password = password - }; - - // Assert - request.Username.Should().Be(username); - request.Password.Should().Be(password); - } + [Theory] + [InlineData("testuser", "testpassword")] + [InlineData("user2", "P@ssw0rd!")] + public void LoginRequest_WithValidValues_ShouldSetPropertiesCorrectly(string username, string password) + { + // Act + var request = new LoginRequest + { + Username = username, + Password = password + }; + + // Assert + request.Username.Should().Be(username); + request.Password.Should().Be(password); + }MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs (1)
11-14: Consider PBKDF2/Argon2 instead of raw HMAC for password hashing.HMAC with a random key works but lacks configurable work factor. Prefer PBKDF2 (or Argon2/bcrypt/scrypt) to slow down guessing attacks. Example PBKDF2 refactor:
-using var hmac = new HMACSHA512(); -var hash = hmac.ComputeHash(Encoding.UTF8.GetBytes(password)); -return (Convert.ToBase64String(hash), Convert.ToBase64String(hmac.Key)); +var salt = RandomNumberGenerator.GetBytes(16); +var hash = Rfc2898DeriveBytes.Pbkdf2( + password, + salt, + 100_000, + HashAlgorithmName.SHA512, + 64); +return (Convert.ToBase64String(hash), Convert.ToBase64String(salt));-using var hmac = new HMACSHA512(saltBytes); -var computedHash = hmac.ComputeHash(Encoding.UTF8.GetBytes(password)); -var storedHash = Convert.FromBase64String(hash); -return CryptographicOperations.FixedTimeEquals(computedHash, storedHash); +var storedHash = Convert.FromBase64String(hash); +var computed = Rfc2898DeriveBytes.Pbkdf2( + password, + saltBytes, + 100_000, + HashAlgorithmName.SHA512, + storedHash.Length); +return CryptographicOperations.FixedTimeEquals(computed, storedHash);Also applies to: 19-23
.github/workflows/code-quality.yml (1)
14-35: Tighten workflow ergonomics and safety (permissions, concurrency, working-directory).
- Add least‑privilege token (read‑only).
- Cancel superseded runs.
- Use a working directory to drop --prefix noise.
name: Code Quality +permissions: + contents: read + on: @@ jobs: code-quality: name: Code Quality Checks runs-on: ubuntu-latest + concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + defaults: + run: + shell: bash + working-directory: MySvelteApp.Client @@ - - name: Install client dependencies - run: npm ci --prefix MySvelteApp.Client + - name: Install client dependencies + run: npm ci @@ - - name: Run ESLint - run: npm run lint --prefix MySvelteApp.Client + - name: Run ESLint + run: npm run lint @@ - - name: Run TypeScript type checking - run: npm run check --prefix MySvelteApp.Client + - name: Run TypeScript type checking + run: npm run check @@ - - name: Run Prettier format check - run: npx prettier --check MySvelteApp.Client + - name: Run Prettier format check + run: npx prettier --check ..github/README.md (1)
30-46: Add dependency install step to local dev instructions.Running frontend tests/checks without npm ci will fail on a clean checkout.
## Local Development @@ ```bash # Backend tests dotnet test svelte-NET-Test.sln # Frontend tests +npm ci --prefix MySvelteApp.Client npm run test:unit --prefix MySvelteApp.Client @@ # Frontend linting npm run lint --prefix MySvelteApp.ClientMySvelteApp.Server/Domain/Entities/User.cs (5)
16-21: Message does not reflect whitespace check.Throw message says "null or empty" but you reject whitespace too. Align for clarity.
-throw new ArgumentException("Username cannot be null or empty", nameof(value)); +throw new ArgumentException("Username cannot be null or whitespace", nameof(value));
29-34: Same nit: update message to include "whitespace".-throw new ArgumentException("Email cannot be null or empty", nameof(value)); +throw new ArgumentException("Email cannot be null or whitespace", nameof(value));
42-47: Same nit: include "whitespace".-throw new ArgumentException("PasswordHash cannot be null or empty", nameof(value)); +throw new ArgumentException("PasswordHash cannot be null or whitespace", nameof(value));
55-60: Same nit: include "whitespace".-throw new ArgumentException("PasswordSalt cannot be null or empty", nameof(value)); +throw new ArgumentException("PasswordSalt cannot be null or whitespace", nameof(value));
24-35: Normalize Email at the domain boundary to guarantee case‑insensitive uniqueness.Today, repository checks are case‑insensitive but the DB unique index is case‑sensitive by default (SQLite). Normalizing here simplifies guarantees and avoids race/DB collation pitfalls.
public string Email { get => _email; set { if (string.IsNullOrWhiteSpace(value)) { throw new ArgumentException("Email cannot be null or whitespace", nameof(value)); } - _email = value; + _email = value.Trim().ToLowerInvariant(); } }Confirm tests that assert exact casing of Email (if any) won’t become brittle after normalization.
MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs (1)
16-19: Consider AsNoTracking() for read‑only fetch.If the result is only read (e.g., auth lookup), skip change tracker overhead.
-return await _dbContext.Users.FirstOrDefaultAsync(u => u.Username == username, cancellationToken); +return await _dbContext.Users.AsNoTracking().FirstOrDefaultAsync(u => u.Username == username, cancellationToken);MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs (1)
14-18: Strengthen model constraints and email collation (SQLite).
- Mark fields required in the DB.
- Add reasonable max lengths.
- For SQLite, enforce case‑insensitive collation on Email to match repository semantics.
protected override void OnModelCreating(ModelBuilder modelBuilder) { - modelBuilder.Entity<User>().HasIndex(u => u.Username).IsUnique(); - modelBuilder.Entity<User>().HasIndex(u => u.Email).IsUnique(); + var user = modelBuilder.Entity<User>(); + user.Property(u => u.Username).IsRequired().HasMaxLength(64); + user.Property(u => u.Email).IsRequired().HasMaxLength(320); + user.Property(u => u.PasswordHash).IsRequired().HasMaxLength(512); + user.Property(u => u.PasswordSalt).IsRequired().HasMaxLength(512); + + user.HasIndex(u => u.Username).IsUnique(); + user.HasIndex(u => u.Email).IsUnique(); + + // Align SQLite with case-insensitive email semantics + if (Database.IsSqlite()) + { + user.Property(u => u.Email).UseCollation("NOCASE"); + } }Confirm EF version/provider compatibility for UseCollation on SQLite in your target runtime; otherwise consider storing normalized email (lowercased) instead.
.github/workflows/ci.yml (1)
14-69: Harden CI: add permissions, concurrency, and NuGet cache — root package.json present; keep root npm ci
- Verified: package.json exists at repo root (name: svelte-net-test) and in MySvelteApp.Client (mysvelteapp.client) — keep the current "Install root dependencies" (npm ci).
- Add least‑privilege workflow permissions at the top level (e.g., permissions: contents: read).
- Add concurrency to cancel in‑progress runs (example):
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true- Enable NuGet caching for faster restores (via actions/cache or setup-dotnet's cache input) and include dependency paths like **/*.csproj and **/packages.lock.json.
- Keep existing setup-node npm cache configuration.
Location: .github/workflows/ci.yml (lines 14–69)
MySvelteApp.Server/Application/Authentication/AuthService.cs (3)
11-11: Email regex still allows leading/trailing dots in the domain; consider tightening it.Your negative lookahead blocks consecutive dots, but addresses like
a@.b.comanda@b.c.will still pass. If that’s unintended, tighten the domain parts (no leading/trailing dots around labels).Apply this diff (keeps your “no double dots” intent and prevents leading/trailing dots in the domain):
- private static readonly Regex EmailRegex = new("^(?!.*\\.\\.)[^\\s@]+@[^\\s@]+\\.[^\\s@]+$", RegexOptions.Compiled); + private static readonly Regex EmailRegex = new("^(?!.*\\.\\.)[^\\s@]+@[^\\s@.]+\\.[^\\s@.]+$", RegexOptions.Compiled | RegexOptions.CultureInvariant);Please add tests for:
a@.b.com,a@b.c.,a..b@example.com, and confirm desired behavior.
150-160: Trim username in Login validation for consistency with Register.Small consistency/UX win (keeps space-padded usernames from failing validation).
- if (string.IsNullOrWhiteSpace(request.Username)) + var username = request.Username?.Trim(); + if (string.IsNullOrWhiteSpace(username)) { return ("Username is required.", AuthErrorType.Validation); }
36-46: Unify culture handling for email normalization.Here you use
ToLowerInvariant(); repository’sEmailExistsAsync(in Repositories/UserRepository.cs) usesToLower(). Align on invariant (or DB collation) to avoid Turkish-i style issues.Would you like a patch for the repository method to use
ToLowerInvariant()or a collation-based approach (e.g., EF.Functions.Collate/CI collation)?MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs (1)
74-96: Avoid contradictory state in the “InitAssignments” test; Success=true with Error fields set.This mixes success and error semantics. Prefer a coherent state and adjust the comment.
- var result = new AuthResult + var result = new AuthResult { - Success = true, + Success = false, UserId = 123, Username = "testuser", Token = "jwt.token", ErrorMessage = "error", ErrorType = AuthErrorType.Validation }; - // Act & Assert - Properties should be init-only - // This test ensures the class is designed with init properties for immutability + // Act & Assert - Values set via object initializer should persistMySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs (2)
136-162: Also validate the signature in the base64-key test.You already parse and assert claims; add a signature validation to fully exercise the key handling.
jwtToken.Issuer.Should().Be(jwtOptions.Value.Issuer); jwtToken.Audiences.Should().Contain(jwtOptions.Value.Audience); + + // Validate signature using the same key material + var keyString = jwtOptions.Value.Key; + var keyBytes = keyString.StartsWith("base64:", StringComparison.OrdinalIgnoreCase) + ? Convert.FromBase64String(keyString.Substring("base64:".Length)) + : Encoding.UTF8.GetBytes(keyString); + var validationParams = new TokenValidationParameters + { + ValidateIssuerSigningKey = true, + IssuerSigningKey = new SymmetricSecurityKey(keyBytes), + ValidateIssuer = true, + ValidIssuer = jwtOptions.Value.Issuer, + ValidateAudience = true, + ValidAudience = jwtOptions.Value.Audience, + ValidateLifetime = true, + ClockSkew = TimeSpan.FromMinutes(1) + }; + new JwtSecurityTokenHandler().ValidateToken(token, validationParams, out _);
164-191: Do the same for the plain-text-key test.jwtToken.Issuer.Should().Be(jwtOptions.Value.Issuer); jwtToken.Audiences.Should().Contain(jwtOptions.Value.Audience); + + // Validate signature using the plain-text key + var keyBytes = Encoding.UTF8.GetBytes(jwtOptions.Value.Key); + var validationParams = new TokenValidationParameters + { + ValidateIssuerSigningKey = true, + IssuerSigningKey = new SymmetricSecurityKey(keyBytes), + ValidateIssuer = true, + ValidIssuer = jwtOptions.Value.Issuer, + ValidateAudience = true, + ValidAudience = jwtOptions.Value.Audience, + ValidateLifetime = true, + ClockSkew = TimeSpan.FromMinutes(1) + }; + new JwtSecurityTokenHandler().ValidateToken(token, validationParams, out _);MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
37-80: Add fixtures for tricky email cases referenced by validation.Handy to centralize additional invalid cases (double dots, leading/trailing dots, whitespace).
public static class Authentication { + public static RegisterRequest InvalidDoubleDotEmailRequest => new() + { + Username = "testuser", + Email = "test..user@example.com", + Password = "ValidPassword123" + }; + + public static RegisterRequest LeadingDotDomainEmailRequest => new() + { + Username = "testuser", + Email = "user@.example.com", + Password = "ValidPassword123" + }; + + public static RegisterRequest TrailingDotDomainEmailRequest => new() + { + Username = "testuser", + Email = "user@example.com.", + Password = "ValidPassword123" + }; + + public static RegisterRequest EmailWithSpacesRequest => new() + { + Username = "testuser", + Email = " test@example.com ", + Password = "ValidPassword123" + };MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (2)
184-189: Also use a unique store for this test-specific context.- const string testDbName = "PersistenceTestDb"; + var testDbName = $"PersistenceTestDb_{Guid.NewGuid():N}";
191-194: Redundant SaveChangesAsync call.Repository.AddAsync already saves; this extra SaveChangesAsync is unnecessary.
await testRepository.AddAsync(newUser); - await testContext.SaveChangesAsync();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/README.md(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/code-quality.yml(1 hunks).gitignore(1 hunks)MySvelteApp.Server/Application/Authentication/AuthService.cs(1 hunks)MySvelteApp.Server/Domain/Entities/User.cs(1 hunks)MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs(1 hunks)MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs(1 hunks)MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs(1 hunks)MySvelteApp.Server/MySvelteApp.Server.csproj(2 hunks)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs(1 hunks)MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs(1 hunks)MySvelteApp.Server/Tests/TestUtilities/TestData.cs(1 hunks)MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs
- .gitignore
- MySvelteApp.Server/MySvelteApp.Server.csproj
- MySvelteApp.Server/Tests/Domain/Entities/UserTests.cs
- MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthErrorTypeTests.cs
- MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs
- MySvelteApp.Server/Tests/Application/Authentication/AuthServiceValidationTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:04:00.832Z
Learnt from: CR
PR: refactor-gremlin/svelte-NET-Test#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T07:04:00.832Z
Learning: Applies to MySvelteApp.Client/src/**/*.{ts,js,svelte} : Use ESLint (with Svelte plugin and TypeScript support) and Prettier (with Svelte and Tailwind plugins) for linting and formatting
Applied to files:
.github/workflows/code-quality.yml
🧬 Code graph analysis (9)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs (1)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Authentication(39-80)
MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs (1)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Users(8-35)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs (1)
MySvelteApp.Server/Application/Authentication/AuthService.cs (2)
AuthResult(165-173)AuthResult(175-183)
MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs (1)
MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-62)
MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs (3)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (4)
Jwt(83-91)Authentication(39-80)TestData(6-92)Users(8-35)MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (2)
IOptions(37-44)TestHelper(10-56)MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(40-60)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs (2)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Authentication(39-80)MySvelteApp.Server/Tests/Application/Authentication/AuthServiceTests.cs (16)
Fact(33-69)Fact(71-95)Fact(97-120)Fact(122-145)Fact(147-162)Fact(164-177)Fact(179-192)Fact(194-207)Fact(209-222)Fact(224-237)Fact(239-252)Fact(254-285)Fact(290-316)Fact(318-337)Fact(339-362)Fact(364-379)
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-62)
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (5)
MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs (2)
AppDbContext(6-19)AppDbContext(8-10)MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (4)
AppDbContext(12-20)AppDbContext(22-35)TestHelper(10-56)Task(46-50)MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs (6)
UserRepository(7-36)UserRepository(11-14)Task(16-19)Task(21-24)Task(26-29)Task(31-35)MySvelteApp.Server/Tests/TestUtilities/TestData.cs (2)
TestData(6-92)Users(8-35)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-62)
MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs (1)
MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs (2)
PasswordHasher(7-24)VerifyPassword(16-23)
🔇 Additional comments (11)
MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs (2)
7-43: Solid, focused DTO tests.Clear naming and FluentAssertions usage. This file adds good coverage for default state and property assignment.
9-19: Confirmed — DTO properties default to empty strings; no change required.
MySvelteApp.Server/Application/Authentication/DTOs/RegisterRequest.cs sets Username, Email, and Password tostring.Empty.MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs (3)
1-3: LGTM on importsMinimal and accurate dependencies.
39-39: Null-handling policy acknowledged and consistent with DTO designGood call to keep DTO non-nullable and validate nulls upstream in service/validator tests; this resolves the earlier nullability mismatch.
9-18: Use FluentAssertions' BeEmpty; DTO defaults verifiedConfirmed LoginRequest.Username and Password are initialized to string.Empty — swap to BeEmpty() is safe.
- request.Username.Should().Be(string.Empty); - request.Password.Should().Be(string.Empty); + request.Username.Should().BeEmpty(); + request.Password.Should().BeEmpty();MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs (1)
22-22: Good security hardening: constant‑time compare.Switching to CryptographicOperations.FixedTimeEquals eliminates timing side‑channels in verification. Looks solid.
MySvelteApp.Server/Tests/Infrastructure/Security/PasswordHasherTests.cs (1)
16-244: Strong test coverage and edge cases.Thorough, balances correctness and tamper scenarios; aligns with constant‑time compare semantics.
MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs (1)
9-48: DTO tests read clean and cover key states.Default, success, and error cases are exercised clearly. Nice use of FluentAssertions.
Also applies to: 50-72
MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs (1)
26-109: JWT tests are thorough and well-structured.Coverage on issuer/audience, expiration, claims, JTI uniqueness, and algorithm is solid.
MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
1-35: Test data layout looks good and aligns with domain validation.Also applies to: 83-91
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (1)
299-321: Great: duplicate username constraint tested against SQLite (relational).This is the right way to assert uniqueness vs. EF InMemory.
Confirm corresponding unique indexes exist in production migrations (Username, Email) so behavior matches tests.
| public async Task<bool> EmailExistsAsync(string email, CancellationToken cancellationToken = default) | ||
| { | ||
| return await _dbContext.Users.AnyAsync(u => u.Email == email, cancellationToken); | ||
| return await _dbContext.Users.AnyAsync(u => u.Email.ToLower() == email.ToLower(), cancellationToken); | ||
| } |
There was a problem hiding this comment.
DB uniqueness vs. case‑insensitive semantics gap.
Unique index on Email is case‑sensitive by default in SQLite; two users could persist Test@ex.com and test@ex.com despite the code‑level check. Prefer storing normalized email (domain) or configuring NOCASE collation on the column/index (see AppDbContext suggestion).
🤖 Prompt for AI Agents
In MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs
around lines 26 to 29, the EmailExistsAsync check uses ToLower() which prevents
duplicate emails at runtime but does not match SQLite's default case‑sensitive
unique index; fix by persisting a normalized email (e.g. store a LowercaseEmail
or NormalizedEmail column) or configuring the Email column/index with NOCASE
collation in AppDbContext and migrations, then change all lookups and the
repository check to use the normalized column (or rely on database collation) so
uniqueness is enforced at the DB level and comparisons are consistent.
🛠️ Refactor suggestion
Use culture‑invariant normalization on the parameter; leave server‑side ToLower() for translation.
ToLower() is culture‑sensitive. Normalize the input with ToLowerInvariant() to avoid Turkish‑i issues, but keep the left side translatable by EF.
- return await _dbContext.Users.AnyAsync(u => u.Email.ToLower() == email.ToLower(), cancellationToken);
+ var normalized = email.ToLowerInvariant();
+ return await _dbContext.Users.AnyAsync(u => u.Email.ToLower() == normalized, cancellationToken);🤖 Prompt for AI Agents
MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs
around lines 26-29: the query uses ToLower() on the input which is
culture-sensitive; normalize the incoming email with ToLowerInvariant() before
the EF call (e.g. assign a local variable emailNormalized =
email?.ToLowerInvariant() or validate/throw on null) and then use that
normalized value in the AnyAsync predicate while leaving u.Email.ToLower()
unchanged so EF can translate the server-side call.
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs
Show resolved
Hide resolved
- Added npm installation command for frontend tests in README.md. - Updated CI workflow to include concurrency settings and caching for .NET and Node.js dependencies. - Modified code quality workflow to streamline Node.js setup and improve linting and type checking processes. - Refactored authentication service to enhance email validation and trim whitespace from user inputs. - Improved user entity validation messages for clarity. - Updated password hashing logic for better security practices. - Enhanced unit tests for authentication and user repository to ensure robust validation and error handling.
- Replaced client linting step with ast-grep action for enhanced Svelte compliance checks. - Ensured the latest version of ast-grep is used with a specified configuration file.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MySvelteApp.Server/Application/Authentication/AuthService.cs (1)
125-134: Enforce email max length during registration.
Matches AppDbContext’s HasMaxLength(320).- if (!EmailRegex.IsMatch(email)) + if (!EmailRegex.IsMatch(email)) { return ("Please enter a valid email address.", AuthErrorType.Validation); } + + if (email.Length > EmailMaxLength) + { + return ($"Email cannot exceed {EmailMaxLength} characters.", AuthErrorType.Validation); + }
♻️ Duplicate comments (2)
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (2)
17-22: Good: per-test DB isolationUsing a per-instance in-memory DB name eliminates cross-test bleed and addresses earlier feedback.
181-207: “DifferentDb” check is tautological; randomize or removeThe isolation assertion against a hard-coded "DifferentDb" will always be true and can collide if reused. Either drop it or randomize the name to make intent explicit.
Apply:
- // First verify the user doesn't exist in a different database (isolation test) - using var differentContext = TestHelper.CreateInMemoryDbContext("DifferentDb"); + // First verify the user doesn't exist in a different database (isolation test) + var isolationDbName = $"IsolationDb_{Guid.NewGuid():N}"; + using var differentContext = TestHelper.CreateInMemoryDbContext(isolationDbName); var userInDifferentDb = await differentContext.Users.FirstOrDefaultAsync(u => u.Username == newUser.Username); userInDifferentDb.Should().BeNull(); // Should not exist in different database
🧹 Nitpick comments (12)
.github/workflows/code-quality.yml (1)
43-44: Prefer using the locally-installed Prettier to avoid pulling arbitrary versions.
Use npx’s --no-install so the check uses your devDependency.- - name: Run Prettier format check - run: npx prettier --check . + - name: Run Prettier format check + run: npx --no-install prettier --check ..github/README.md (2)
10-21: Docs/CI drift: Node.js version is 20 in workflows, but README says 18.
Align the version to avoid confusion.- - ✅ Sets up Node.js 18 environment + - ✅ Sets up Node.js 20 environment
16-19: Docs/CI drift: README says CI runs ESLint; current CI workflow doesn’t.
Either add ESLint to CI or clarify that linting runs in code-quality.yml.Option A (update docs):
- - ✅ Runs client linting (ESLint) + - ✅ Runs client linting (ESLint) — in code-quality.ymlOption B (add ESLint step to CI; see ci.yml comment).
Also applies to: 22-29
.github/workflows/ci.yml (3)
42-47: Guard root npm install (repo may not have a root package-lock).
Prevents unnecessary failures when there’s no root package.json.- - name: Install root dependencies - run: npm ci + - name: Install root dependencies + if: ${{ hashFiles('package-lock.json') != '' }} + run: npm ci
67-71: Add ESLint to CI or adjust README.
If you want CI (not just code-quality) to run lint, add this step.- name: Run client type checking run: npm run check --prefix MySvelteApp.Client + - name: Run client linting + run: npm run lint --prefix MySvelteApp.Client
73-81: Uploading Playwright artifacts without running Playwright.
Either add a Playwright test step or remove these paths to reduce noise.- name: Upload test results uses: actions/upload-artifact@v4 if: always() with: name: test-results path: | **/test-results.trx - MySvelteApp.Client/playwright-report/ - MySvelteApp.Client/test-results/MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs (1)
9-19: Parameterize PBKDF2 settings and prepare for future rehashing policy.
Constants improve clarity; consider making iterations configurable via options and encoding params with the hash for seamless upgrades.public class PasswordHasher : IPasswordHasher { + private const int SaltSize = 16; + private const int KeySize = 64; // bytes + private const int Iterations = 100_000; // consider higher or configurable + public (string Hash, string Salt) HashPassword(string password) { - var salt = RandomNumberGenerator.GetBytes(16); - var hash = Rfc2898DeriveBytes.Pbkdf2( + var salt = RandomNumberGenerator.GetBytes(SaltSize); + var hash = Rfc2898DeriveBytes.Pbkdf2( password, salt, - 100_000, + Iterations, HashAlgorithmName.SHA512, - 64); + KeySize); return (Convert.ToBase64String(hash), Convert.ToBase64String(salt)); }- var computed = Rfc2898DeriveBytes.Pbkdf2( + var computed = Rfc2898DeriveBytes.Pbkdf2( password, saltBytes, - 100_000, + Iterations, HashAlgorithmName.SHA512, storedHash.Length);If you want, I can draft a backward-compatible versioned format (e.g., algorithm$iter$salt$hash) and update VerifyPassword accordingly.
Also applies to: 25-33
MySvelteApp.Server/Domain/Entities/User.cs (1)
11-22: Optional: enforce max lengths at the domain boundary to match persistence.
Prevents surprises from DB rejections; mirror 64 (Username) and 320 (Email).Example (Username):
- _username = value; + if (value.Length > 64) throw new ArgumentException("Username exceeds 64 characters.", nameof(value)); + _username = value;Example (Email):
- _email = value.Trim().ToLowerInvariant(); + var v = value.Trim().ToLowerInvariant(); + if (v.Length > 320) throw new ArgumentException("Email exceeds 320 characters.", nameof(value)); + _email = v;Also applies to: 24-35
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (4)
62-76: Confirm username case-sensitivity across providersTests assert case-sensitive username lookups. Verify this is the intended contract. If yes, explicitly enforce it at the model level to avoid provider/collation drift (e.g., SQLite vs SQL Server).
Proposed addition in AppDbContext (mirrors your reflection approach for Email):
if (Database.ProviderName?.Contains("Sqlite") == true) { try { var propertyBuilder = user.Property(u => u.Email); var useCollationMethod = propertyBuilder.GetType().GetMethod("UseCollation"); if (useCollationMethod is not null) { useCollationMethod.Invoke(propertyBuilder, ["NOCASE"]); } + // Enforce case-sensitive Username semantics explicitly + var usernameProperty = user.Property(u => u.Username); + if (useCollationMethod is not null) + { + useCollationMethod.Invoke(usernameProperty, ["BINARY"]); + } } catch { // Silently ignore if SQLite collation is not available } }
146-160: Email case-insensitive test is correct; simplify repository for index useSince User.Email is normalized to lowercase on set, you don’t need LOWER on the column side. Comparing the normalized input to the stored value improves sargability and index usage.
Apply in UserRepository:
- public async Task<bool> EmailExistsAsync(string email, CancellationToken cancellationToken = default) - { - var normalizedEmail = email.ToLowerInvariant(); - return await _dbContext.Users.AsNoTracking().AnyAsync(u => u.Email.ToLower() == normalizedEmail, cancellationToken); - } + public async Task<bool> EmailExistsAsync(string email, CancellationToken cancellationToken = default) + { + var normalizedEmail = email.Trim().ToLowerInvariant(); + return await _dbContext.Users.AsNoTracking() + .AnyAsync(u => u.Email == normalizedEmail, cancellationToken); + }
162-179: Avoid seeding explicit Ids in TestDataSeveral tests rely on TestData.Users.ValidUser/AnotherValidUser which currently set Ids. Prefer letting EF generate keys to better mirror relational behavior and avoid future portability issues. Assertions already work if Ids are generated (e.g., NotBe(0), distinct Ids).
Proposed tweak in TestData.Users:
- public static User ValidUser => new() - { - Id = 1, + public static User ValidUser => new() + { Username = "testuser", Email = "test@example.com", PasswordHash = "hashed_password", PasswordSalt = "salt_value" }; - public static User AnotherValidUser => new() - { - Id = 2, + public static User AnotherValidUser => new() + { Username = "anotheruser", Email = "another@example.com", PasswordHash = "another_hash", PasswordSalt = "another_salt" };
299-321: Also test duplicate email constraintYou test duplicate Username on SQLite; mirror for Email to lock in integrity guarantees. I can draft it if helpful.
Example test:
[Fact] public async Task AddAsync_WithDuplicateEmail_ShouldThrowDbUpdateException() { using var sqliteContext = TestHelper.CreateSqliteInMemoryDbContext(); var repo = new UserRepository(sqliteContext); var user1 = new User { Username = "u1", Email = "dup@example.com", PasswordHash = "h1", PasswordSalt = "s1" }; var user2 = new User { Username = "u2", Email = "dup@example.com", PasswordHash = "h2", PasswordSalt = "s2" }; await TestHelper.SeedUsersAsync(sqliteContext, user1); await Assert.ThrowsAsync<DbUpdateException>(() => repo.AddAsync(user2)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/README.md(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/code-quality.yml(1 hunks)MySvelteApp.Server/Application/Authentication/AuthService.cs(3 hunks)MySvelteApp.Server/Domain/Entities/User.cs(1 hunks)MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs(1 hunks)MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs(1 hunks)MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs(1 hunks)MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs(1 hunks)MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs(1 hunks)MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs(1 hunks)MySvelteApp.Server/Tests/TestUtilities/TestData.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- MySvelteApp.Server/Tests/Application/Authentication/DTOs/AuthResultTests.cs
- MySvelteApp.Server/Tests/Application/Authentication/DTOs/RegisterRequestTests.cs
- MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs
- MySvelteApp.Server/Tests/TestUtilities/TestData.cs
- MySvelteApp.Server/Tests/Infrastructure/Authentication/JwtTokenGeneratorTests.cs
- MySvelteApp.Server/Tests/Application/Authentication/DTOs/LoginRequestTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:04:00.832Z
Learnt from: CR
PR: refactor-gremlin/svelte-NET-Test#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T07:04:00.832Z
Learning: Applies to MySvelteApp.Client/src/**/*.{ts,js,svelte} : Use ESLint (with Svelte plugin and TypeScript support) and Prettier (with Svelte and Tailwind plugins) for linting and formatting
Applied to files:
.github/workflows/code-quality.yml
🧬 Code graph analysis (4)
MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs (3)
MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (2)
AppDbContext(12-20)AppDbContext(22-35)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-62)MySvelteApp.Server/Tests/TestUtilities/TestData.cs (1)
Users(8-35)
MySvelteApp.Server/Infrastructure/Security/PasswordHasher.cs (1)
MySvelteApp.Server/Application/Common/Interfaces/IPasswordHasher.cs (1)
VerifyPassword(6-6)
MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (5)
MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs (1)
AppDbContext(6-43)MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (4)
AppDbContext(12-20)AppDbContext(22-35)TestHelper(10-56)Task(46-50)MySvelteApp.Server/Infrastructure/Persistence/Repositories/UserRepository.cs (6)
UserRepository(7-37)UserRepository(11-14)Task(16-19)Task(21-24)Task(26-30)Task(32-36)MySvelteApp.Server/Tests/TestUtilities/TestData.cs (2)
TestData(6-120)Users(8-35)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-62)
MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (2)
MySvelteApp.Server/Tests/TestUtilities/TestHelper.cs (1)
IOptions(37-44)MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(40-60)
🔇 Additional comments (6)
.github/workflows/code-quality.yml (1)
11-22: Workflow looks solid; scope limited to client with sensible caching and concurrency.
No blockers spotted.Also applies to: 23-44
MySvelteApp.Server/Application/Authentication/AuthService.cs (1)
39-47: Confirm desired username uniqueness semantics (case sensitivity).
Username isn’t normalized and no NOCASE collation is applied; duplicates differing only by case may be allowed depending on DB collation. If case-insensitive uniqueness is desired, normalize or enforce collation similarly to Email..github/workflows/ci.yml (1)
61-66: Pin ast-grep action to a fixed version; config verifiedsgconfig.yml is present at the repo root; update .github/workflows/ci.yml (ast-grep/action step, lines 61–66) to replace
version: latestwith an explicit tag or pinned SHA so CI is deterministic.MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (1)
12-15: No runtime guard required — JWT key strength already validated at startup.
JwtOptionsValidator.ValidateKeyStrength enforces decoded key >= 32 bytes and Program.cs registers AddOptions().ValidateDataAnnotations().ValidateOnStart() (see MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs and MySvelteApp.Server/Program.cs), so the suggested runtime check in JwtTokenGenerator is redundant.MySvelteApp.Server/Tests/Infrastructure/Persistence/UserRepositoryTests.cs (2)
24-28: Good: deterministic teardownEnsures the database is deleted and context disposed after each test run.
275-297: Good: cancellation semantics coveredPre-canceled token + ThrowsAnyAsync on all repo methods is sufficient and robust.
| private static readonly Regex UsernameRegex = new("^[a-zA-Z0-9_]+$", RegexOptions.Compiled); | ||
| private static readonly Regex EmailRegex = new("^[^\\s@]+@[^\\s@]+\\.[^\\s@]+$", RegexOptions.Compiled); | ||
| private static readonly Regex EmailRegex = new("^(?!.*\\.\\.)[^\\s@]+@[^\\s@.]+\\.[^\\s@.]+$", RegexOptions.Compiled | RegexOptions.CultureInvariant); | ||
| private static readonly Regex PasswordRegex = new("(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)", RegexOptions.Compiled); | ||
|
|
There was a problem hiding this comment.
Add max-length constants to mirror EF constraints.
Prevents hitting DB errors late and aligns service validation with persistence.
private static readonly Regex UsernameRegex = new("^[a-zA-Z0-9_]+$", RegexOptions.Compiled);
private static readonly Regex EmailRegex = new("^(?!.*\\.\\.)[^\\s@]+@[^\\s@.]+\\.[^\\s@.]+$", RegexOptions.Compiled | RegexOptions.CultureInvariant);
private static readonly Regex PasswordRegex = new("(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)", RegexOptions.Compiled);
+ private const int UsernameMaxLength = 64;
+ private const int EmailMaxLength = 320;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static readonly Regex UsernameRegex = new("^[a-zA-Z0-9_]+$", RegexOptions.Compiled); | |
| private static readonly Regex EmailRegex = new("^[^\\s@]+@[^\\s@]+\\.[^\\s@]+$", RegexOptions.Compiled); | |
| private static readonly Regex EmailRegex = new("^(?!.*\\.\\.)[^\\s@]+@[^\\s@.]+\\.[^\\s@.]+$", RegexOptions.Compiled | RegexOptions.CultureInvariant); | |
| private static readonly Regex PasswordRegex = new("(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)", RegexOptions.Compiled); | |
| private static readonly Regex UsernameRegex = new("^[a-zA-Z0-9_]+$", RegexOptions.Compiled); | |
| private static readonly Regex EmailRegex = new("^(?!.*\\.\\.)[^\\s@]+@[^\\s@.]+\\.[^\\s@.]+$", RegexOptions.Compiled | RegexOptions.CultureInvariant); | |
| private static readonly Regex PasswordRegex = new("(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)", RegexOptions.Compiled); | |
| private const int UsernameMaxLength = 64; | |
| private const int EmailMaxLength = 320; |
🤖 Prompt for AI Agents
In MySvelteApp.Server/Application/Authentication/AuthService.cs around lines 10
to 13, add max-length constant definitions that mirror the EF persistence
constraints (e.g., MaxUsernameLength, MaxEmailLength, MaxPasswordLength) and use
those constants in the service validation before applying the existing Regex
checks; specifically, check string lengths against the new constants and return
validation failures if exceeded (with clear messages), then run the regexes as
currently implemented so the service-side validation prevents DB-level length
errors and stays aligned with the EF model.
| if (username.Length < 3) | ||
| { | ||
| return ("Username must be at least 3 characters long.", AuthErrorType.Validation); | ||
| } | ||
|
|
||
| if (!UsernameRegex.IsMatch(request.Username)) | ||
| if (!UsernameRegex.IsMatch(username)) | ||
| { | ||
| return ("Username can only contain letters, numbers, and underscores.", AuthErrorType.Validation); | ||
| } |
There was a problem hiding this comment.
Enforce username max length during registration.
Matches AppDbContext’s HasMaxLength(64).
- if (username.Length < 3)
+ if (username.Length < 3)
{
return ("Username must be at least 3 characters long.", AuthErrorType.Validation);
}
+
+ if (username.Length > UsernameMaxLength)
+ {
+ return ($"Username cannot exceed {UsernameMaxLength} characters.", AuthErrorType.Validation);
+ }🤖 Prompt for AI Agents
In MySvelteApp.Server/Application/Authentication/AuthService.cs around lines 115
to 123, add a max-length validation to enforce the same constraint as
AppDbContext (HasMaxLength(64)); after the existing minimum-length check and
before the regex check, verify if username.Length > 64 and return ("Username
must be at most 64 characters long.", AuthErrorType.Validation) so registrations
cannot exceed the database column size.
| // Align SQLite with case-insensitive email semantics | ||
| // This requires Microsoft.EntityFrameworkCore.Sqlite package | ||
| // Check if SQLite provider is available at runtime | ||
| if (Database.ProviderName?.Contains("Sqlite") == true) | ||
| { | ||
| try | ||
| { | ||
| // Use reflection to avoid compile-time dependency on SQLite package | ||
| var propertyBuilder = user.Property(u => u.Email); | ||
| var useCollationMethod = propertyBuilder.GetType().GetMethod("UseCollation"); | ||
| if (useCollationMethod is not null) | ||
| { | ||
| useCollationMethod.Invoke(propertyBuilder, ["NOCASE"]); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Silently ignore if SQLite collation is not available | ||
| // The database will still work with case-sensitive comparison | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Reflection-based UseCollation invocation never fires (extension method not on instance).
Replace with a direct call guarded by a provider check; this reliably applies NOCASE on SQLite.
- // Align SQLite with case-insensitive email semantics
- // This requires Microsoft.EntityFrameworkCore.Sqlite package
- // Check if SQLite provider is available at runtime
- if (Database.ProviderName?.Contains("Sqlite") == true)
- {
- try
- {
- // Use reflection to avoid compile-time dependency on SQLite package
- var propertyBuilder = user.Property(u => u.Email);
- var useCollationMethod = propertyBuilder.GetType().GetMethod("UseCollation");
- if (useCollationMethod is not null)
- {
- useCollationMethod.Invoke(propertyBuilder, ["NOCASE"]);
- }
- }
- catch
- {
- // Silently ignore if SQLite collation is not available
- // The database will still work with case-sensitive comparison
- }
- }
+ // Align SQLite with case-insensitive email semantics
+ if (Database.ProviderName?.Contains("Sqlite") == true)
+ {
+ user.Property(u => u.Email).UseCollation("NOCASE");
+ }If avoiding a compile-time provider dependency is a hard requirement, the current domain normalization (ToLowerInvariant) already ensures uniqueness; you could drop this block entirely.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Top-level listing:"
ls -la
echo
echo "Searching for AppDbContext definitions..."
rg -n --hidden --no-ignore -S "class\s+AppDbContext" -g '!**/bin/**' -g '!**/obj/**' || true
TARGET="MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs"
echo
echo "Checking path: $TARGET"
if [ -f "$TARGET" ]; then
echo "=== $TARGET ==="
sed -n '1,240p' "$TARGET"
else
echo "$TARGET not found. Searching repo for AppDbContext.cs files..."
rg -n --hidden --no-ignore -S "AppDbContext.cs" -g '!**/bin/**' -g '!**/obj/**' || true
echo
echo "Searching for UseCollation, NOCASE, ProviderName, and Sqlite package references..."
rg -n --hidden --no-ignore -S "UseCollation" -g '!**/bin/**' -g '!**/obj/**' || true
rg -n --hidden --no-ignore -S "NOCASE" -g '!**/bin/**' -g '!**/obj/**' || true
rg -n --hidden --no-ignore -S "ProviderName" -g '!**/bin/**' -g '!**/obj/**' || true
rg -n --hidden --no-ignore -S "Microsoft.EntityFrameworkCore.Sqlite" -g '!**/bin/**' -g '!**/obj/**' || true
echo
echo "Listing .csproj files (maxdepth 3):"
find . -maxdepth 3 -name '*.csproj' -print || true
fiLength of output: 3987
Replace reflection-based UseCollation invocation with a direct UseCollation call (or remove the block if you must avoid a compile-time provider dependency).
GetMethod on the PropertyBuilder instance won't find the extension method (it's static) and the reflection Invoke is incorrect (the current ["NOCASE"] syntax is invalid); replace with a guarded direct call:
- // Align SQLite with case-insensitive email semantics
- // This requires Microsoft.EntityFrameworkCore.Sqlite package
- // Check if SQLite provider is available at runtime
- if (Database.ProviderName?.Contains("Sqlite") == true)
- {
- try
- {
- // Use reflection to avoid compile-time dependency on SQLite package
- var propertyBuilder = user.Property(u => u.Email);
- var useCollationMethod = propertyBuilder.GetType().GetMethod("UseCollation");
- if (useCollationMethod is not null)
- {
- useCollationMethod.Invoke(propertyBuilder, ["NOCASE"]);
- }
- }
- catch
- {
- // Silently ignore if SQLite collation is not available
- // The database will still work with case-sensitive comparison
- }
- }
+ // Align SQLite with case-insensitive email semantics
+ if (Database.ProviderName?.Contains("Sqlite") == true)
+ {
+ user.Property(u => u.Email).UseCollation("NOCASE");
+ }Location: MySvelteApp.Server/Infrastructure/Persistence/AppDbContext.cs (OnModelCreating).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Align SQLite with case-insensitive email semantics | |
| // This requires Microsoft.EntityFrameworkCore.Sqlite package | |
| // Check if SQLite provider is available at runtime | |
| if (Database.ProviderName?.Contains("Sqlite") == true) | |
| { | |
| try | |
| { | |
| // Use reflection to avoid compile-time dependency on SQLite package | |
| var propertyBuilder = user.Property(u => u.Email); | |
| var useCollationMethod = propertyBuilder.GetType().GetMethod("UseCollation"); | |
| if (useCollationMethod is not null) | |
| { | |
| useCollationMethod.Invoke(propertyBuilder, ["NOCASE"]); | |
| } | |
| } | |
| catch | |
| { | |
| // Silently ignore if SQLite collation is not available | |
| // The database will still work with case-sensitive comparison | |
| } | |
| } | |
| // Align SQLite with case-insensitive email semantics | |
| if (Database.ProviderName?.Contains("Sqlite") == true) | |
| { | |
| user.Property(u => u.Email).UseCollation("NOCASE"); | |
| } |
- Introduced maximum length constraints for username, email, and password in AuthService to enhance validation. - Updated User entity to enforce username and email length restrictions. - Simplified validation logic in AuthService for better readability and maintainability. - Enhanced error handling in UserRepository to ensure unique email constraints are respected. - Improved password hashing parameters for increased security. - Refactored exception handling in various services for consistency and clarity.
- Simplified password length validation by using a conditional expression for better readability. - Consolidated error messages for password length checks into a single return statement.
- Streamlined password validation logic by replacing multiple conditional statements with a single conditional expression for improved readability. - Consolidated error message handling for password requirements into a more concise format.
- Updated the pre-commit hook to include the .NET tools path for better accessibility. - Changed C# code formatting command to use CSharpier for improved consistency in code style. - Refactored formatting commands in the pre-commit script for clarity and efficiency. - Cleaned up whitespace and improved formatting in various C# files for better readability.
- Added new format and check scripts in package.json for better code management. - Updated .prettierignore to exclude generated API and UI library files from formatting. - Configured ESLint to ignore specific directories for improved linting accuracy. - Enabled runes in Svelte compiler options for enhanced functionality. - Refactored authentication logic to provide clearer error messages during login and registration processes. - Improved form validation with detailed error handling for user feedback.
- Adjusted the formatting of the ESLint configuration file for better clarity. - Ensured consistent indentation and organization of rules and language options. - Maintained the existing rule for deprecation warnings while enhancing overall configuration layout.
…ation - Updated ESLint configuration for improved readability and structure, ensuring consistent formatting and organization. - Introduced a new TypeScript configuration file for ESLint to better manage TypeScript-specific linting. - Enhanced authentication error handling by centralizing error message extraction and improving user feedback during login and registration processes. - Added a new utility for resolving authentication error messages, streamlining error handling across the application. - Improved the layout and structure of various components for better maintainability and user experience.
- Streamlined ESLint configuration for better readability and organization, ensuring consistent formatting. - Enhanced the layout of the Svelte component by adjusting class order and improving indentation for better maintainability. - Updated various elements within the component for improved clarity and user experience, including button and card structures.
- Changed the launch configuration type from 'pwa-node' to 'node' for improved compatibility. - Refactored ESLint configuration by removing the deprecated plugin and adjusting rules for better clarity and maintainability. - Enhanced the organization of ESLint settings to streamline the configuration process.
- Streamlined the ESLint configuration file by consolidating formatting and indentation for better clarity. - Enhanced the organization of rules and language options, ensuring consistent structure throughout the configuration. - Maintained existing rules while improving overall layout for easier maintenance and understanding.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary by CodeRabbit