Release 0.1.0#8
Merged
Merged
Conversation
…Program.cs for dependency injection
- Adds the `Game` model to define the data structure. - Implements controllers and DTOs to expose Game API endpoints. - Creates a new service layer for Game-related business logic. - Establishes a repository for database interactions. - Adds the new components to the application scope.
…ice, and UserRepository; add DTOs for user creation and updates; handle duplicate usernames with custom exception.
…nd Role models, and modify UserRepository for role inclusion
- Add AuthController for user registration and login - Create AuthDto for authentication data transfer - Develop AuthService for user authentication logic - Update Program.cs to include IAuthService in DI - Secure GamesController actions with Admin role authorization
- Remove unnecessary response type from Login method - Simplify exception handling for invalid credentials - Add comment for admin user deletion logic - Ensure default admin user is created with UTC date - Fix typo in exception message for non-existing user - Add check for missing JWT secret key in configuration
Add users and authentication feature
… startup. - Ensure default admin user is always created - Update admin user creation logic in AppDbContext - Add database migration call in Program.cs
- Introduced Dockerfile for containerization - Added https_port configuration to appsettings.json
Prepare for CI/CD
Update GitHub Actions workflow for Docker build
…se types - Secure `GamesController` with `[Authorize]` at the class level. - Restrict `Put` and `Delete` actions in `GamesController` to the "Admin" role. - Set a route name for the get game by ID endpoint and update `Post` to return `CreatedAtRoute`. - Add `Status401Unauthorized` and `Status403Forbidden` response types to applicable endpoints in both `GamesController` and `UsersController` for accurate Swagger documentation.
…ware - Change token expiration calculation in `AuthService` to use `DateTime.UtcNow` instead of `DateTime.Now` - Add `app.UseAuthentication()` and `app.UseAuthorization()` to the HTTP request pipeline in `Program.cs`
feat(auth/controllers): secure controllers, fix token expiration and register middleware
- Update `TargetFramework` to `net10.0` in `GameCollectionAPI.csproj` - Update Dockerfile to use `.dotnet/aspnet:10.0` and `.dotnet/sdk:10.0` images - Upgrade NuGet package dependencies to their .NET 10 compatible versions: - `Microsoft.AspNetCore.Authentication.JwtBearer` (8.0.19 -> 10.0.9) - `Microsoft.AspNetCore.OpenApi` (8.0.5 -> 10.0.9) - `Microsoft.EntityFrameworkCore` (8.0.18 -> 10.0.9) - `Microsoft.EntityFrameworkCore.Design` (8.0.18 -> 10.0.9) - `Microsoft.EntityFrameworkCore.Sqlite` (8.0.18 -> 10.0.9) - `Microsoft.EntityFrameworkCore.Tools` (8.0.18 -> 10.0.9) - `Swashbuckle.AspNetCore` (6.4.0 -> 10.2.3)
- Update `.gitignore` to ignore SQLite database files (`.db`, `.db-shm`, `.db-wal`) and the `.repomix/` directory - Add a null check for `user.PasswordHash` in `AuthService.cs` before verifying the password hash - Use the null-coalescing operator `?? ""` on the JWT signing key in `Program.cs` to avoid potential null arguments
- Remove hardcoded default admin seeding from `AppDbContext.OnModelCreating` and migrations. - Add `DbInitializer` extension method to run migrations and seed the default admin user on application startup. - Retrieve default admin username and password from configuration instead of hardcoding. - Hash the seeded admin's password using `PasswordHasher<User>`. - Refactor database initialization call in `Program.cs` to use the new `InitializeDatabaseAsync` method.
…iables - Rename `.github/workflows/blank.yml` to `.github/workflows/ci-cd.yml` - Pass `ASPNETCORE_ENVIRONMENT`, `Jwt__Key`, and `DefaultAdminPassword` environment variables from GitHub Secrets to the Docker container during deployment - Remove obsolete comment regarding port mapping
Upgrade to .NET 10.0 and configure dynamic DbInitializer
📝 WalkthroughWalkthroughThis PR adds a new ASP.NET Core Web API with JWT authentication, game and user CRUD endpoints, EF Core/SQLite persistence, database migrations and seeding, plus Docker and GitHub Actions workflows for build, validation, and deployment. ChangesGame Collection API
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant UserRepository
participant UsersController
participant UserService
Client->>AuthController: POST /api/auth/login (AuthDto)
AuthController->>AuthService: LoginUserAsync(authDto)
AuthService->>UserRepository: GetByUsernameAsync(username)
UserRepository-->>AuthService: User
AuthService-->>AuthController: JWT token
AuthController-->>Client: 200 OK { jwt }
Client->>UsersController: PUT /api/users/{id} (UpdateUsernameDto)
UsersController->>UserService: UpdateUsernameAsync(id, dto)
UserService->>UserRepository: GetByIdAsync(id)
UserRepository-->>UserService: User
UserService->>UserRepository: UpdateAsync()
UserService-->>UsersController: true/false
UsersController-->>Client: 204 NoContent / 404 NotFound
Suggested labels: enhancement, backend, database 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (8)
Dockerfile (1)
6-9: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a
.dockerignoreand restore-first copy pattern.
COPY . .beforedotnet restorecopies the entire build context (includingbin/obj,.git, etc. if no.dockerignoreexists) and busts Docker layer cache on every source change. Copying only project files first, restoring, then copying the rest improves build cache reuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 6 - 9, The Dockerfile build steps should use a restore-first copy pattern instead of copying the whole context before `dotnet restore`. Update the Dockerfile to copy only the project/solution files needed by `dotnet restore` first, run restore, then copy the remaining source before `dotnet publish`; also ensure a `.dockerignore` excludes `bin`, `obj`, and other unnecessary files to preserve cache efficiency.GameCollectionAPI/DTOs/Games/GameCreateDto.cs (1)
23-24: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider bounding
ReleaseDate.No range validation exists, so arbitrary past (e.g.
DateTime.MinValue) or far-future dates are accepted. Not blocking, but a[Range]-style guard (or custom validation) would tighten data quality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GameCollectionAPI/DTOs/Games/GameCreateDto.cs` around lines 23 - 24, The GameCreateDto.ReleaseDate field currently has only [Required], so it accepts unrealistic dates such as DateTime.MinValue or far-future values. Add range validation or equivalent custom validation on ReleaseDate in GameCreateDto to bound acceptable dates to a sensible window. Keep the validation close to the property so the DTO enforces data quality before reaching the API layer..github/workflows/ci-cd.yml (2)
19-20: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider
persist-credentials: falseon checkout.zizmor's artipacked warning notes the checkout step leaves the git credential persisted on the runner filesystem for the rest of the job. Low risk here since no artifacts are uploaded, but disabling it is cheap hardening.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-cd.yml around lines 19 - 20, The checkout step in the workflow leaves git credentials persisted on the runner, so harden the `actions/checkout` usage by setting `persist-credentials` to false on the `Checkout repository` step. Update the existing checkout configuration in the CI workflow only, keeping the rest of the job unchanged.Source: Linters/SAST tools
69-75: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffSecrets passed via
docker run -eare visible viadocker inspect/process listing.
Jwt__KeyandDefaultAdminPasswordare injected as plain environment variables on the container's command line, so any user able to rundocker inspector read/proc/<pid>/environon the host can read them. Consider using an env file with restricted permissions or Docker secrets instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-cd.yml around lines 69 - 75, The Docker deployment in the CI/CD workflow is exposing sensitive values by passing Jwt__Key and DefaultAdminPassword directly via docker run -e. Update the deployment step to use a safer secret mechanism, such as a restricted env file or Docker secrets, and adjust the docker run invocation so the secrets are no longer visible on the command line or through docker inspect; keep the change localized to the workflow job that launches game-collection-api.GameCollectionAPI/Repositories/UserRepository.cs (3)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winField should be
readonly.
_contextis set only in the constructor; marking itreadonlyprevents accidental reassignment.- private AppDbContext _context; + private readonly AppDbContext _context;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GameCollectionAPI/Repositories/UserRepository.cs` at line 9, The UserRepository field _context is only assigned in the constructor, so it should be made readonly to prevent accidental reassignment. Update the UserRepository class definition so the _context field is declared as readonly and keep its initialization in the constructor unchanged.
16-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
AddAsync/DeleteAsyncdon't handleDbUpdateException.If a unique constraint were added on
Username(see comment on the migration designer), a concurrent duplicate-username insert would throwDbUpdateExceptionhere instead of the expectedDuplicateUsernameException. Once the DB constraint is added, consider catchingDbUpdateExceptionhere (or in the calling service) and translating it into the domain exception for consistent error handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GameCollectionAPI/Repositories/UserRepository.cs` around lines 16 - 26, The UserRepository AddAsync and DeleteAsync methods currently save changes directly and will surface DbUpdateException instead of the domain-specific DuplicateUsernameException when the username unique constraint is hit. Update AddAsync, or the service layer that calls it, to catch DbUpdateException from _context.SaveChangesAsync and translate the duplicate-username case into DuplicateUsernameException so error handling stays consistent; use the AddAsync/DeleteAsync methods in UserRepository and the SaveChangesAsync call as the main points to adjust.
38-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
UpdateAsync()has no way to signal "nothing to update".It relies entirely on the caller having previously loaded and mutated a tracked entity via
GetByIdAsync/GetByUsernameAsyncon the sameAppDbContextinstance. This works given the typical scoped-DbContext-per-request lifetime, but it's an implicit contract with no compile-time enforcement — a caller passing an untracked/detached user silently no-ops onSaveChangesAsync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GameCollectionAPI/Repositories/UserRepository.cs` around lines 38 - 41, UpdateAsync on UserRepository currently only calls SaveChangesAsync, so it silently depends on callers having a tracked User from GetByIdAsync or GetByUsernameAsync. Make the contract explicit by changing the repository API to accept the entity being updated (or otherwise validate tracking state) and ensure the method only saves when a tracked User is attached to the AppDbContext. Use UserRepository, UpdateAsync, and the existing GetByIdAsync/GetByUsernameAsync flow to locate the affected code.GameCollectionAPI/Repositories/GameRepository.cs (1)
28-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
AsNoTracking()for the list query.The provided consumer in
GameCollectionAPI/Services/GameService.cs:46-47maps these rows straight to DTOs, so change tracking adds overhead without helping the list endpoint.Suggested change
public Task<List<Game>> GetAllAsync() { - return _dbContext.Games.ToListAsync(); + return _dbContext.Games.AsNoTracking().ToListAsync(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GameCollectionAPI/Repositories/GameRepository.cs` around lines 28 - 30, The GetAllAsync query in GameRepository is loading entities with change tracking even though GameService maps them directly to DTOs. Update the Games list query in GetAllAsync to use AsNoTracking() before ToListAsync so the repository returns read-only results with less overhead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-cd.yml:
- Around line 14-16: The deploy job in the CI workflow is missing an explicit
permissions block and is inheriting broad default token access. Update the
deploy job definition in the workflow, alongside the existing build job
permissions, to set permissions to none/no GitHub token access since it only
SSHes to the VM and does not use the GitHub API. Use the deploy job identifier
and its surrounding job configuration to place the new permissions block
correctly.
- Around line 58-59: The GitHub Actions login step is interpolating github.actor
directly inside the run script, which creates a template-injection risk. Update
the workflow step to pass github.actor into an environment variable first, then
reference that variable in the docker login command. Keep the change localized
to the GHCR login step in the ci-cd workflow and preserve the existing GHCR_PAT
secret usage.
- Line 27: The GitHub Actions workflow is using outdated Docker action majors;
update the docker/login-action and docker/build-push-action references in the
CI/CD workflow to the current supported majors. Locate the existing action
entries in the workflow and bump docker/login-action from v2 to v4 and
docker/build-push-action from v3 to v7 so the pipeline stays aligned with the
current runtime.
In `@Dockerfile`:
- Around line 11-14: The final stage in the Dockerfile currently runs the
container as root. Add a non-root USER directive in the final stage before the
ENTRYPOINT, using the built-in .NET 8 app user via $APP_UID so the image runs
under the app account instead of root.
In `@GameCollectionAPI/appsettings.json`:
- Around line 13-17: The Jwt configuration is committed with an empty signing
key, so update the appsettings.json Jwt section to use a non-secret placeholder
or remove the key entirely and have AuthService.GenerateJwtToken read it from
environment variables, user-secrets, or a secret manager. Make sure the Jwt:Key
lookup in AuthService still validates presence and fails clearly if it is
missing, but do not keep a real signing secret in source control.
In `@GameCollectionAPI/Controllers/UsersController.cs`:
- Around line 34-41: In UsersController.Get, validate the incoming id before
calling IsOwnerOrAdmin so malformed values return BadRequest instead of Forbid,
and reject any non-positive id rather than only zero. Update the early guard in
Get (and the matching code at the other indicated location) to check id <= 0
first, then proceed to the ownership/admin check only for valid positive IDs.
- Around line 64-67: The success response in UsersController.CreateUserAsync is
using the wrong helper: GetUserById is a route name, so replace CreatedAtAction
with CreatedAtRoute when returning the new user. Keep the same route value
object with user.Id, and ensure the named route used by GetUserById is
referenced so URL generation works correctly on the 201 path.
In `@GameCollectionAPI/Data/DbInitializer.cs`:
- Around line 21-44: The admin seeding logic in DbInitializer.InitializeAsync is
vulnerable to a check-then-insert race when Program.cs runs it on concurrent
startups. Make the seed idempotent by using the configured admin username as the
stable key, enforcing a database uniqueness constraint for that identity, and
changing the insert flow in InitializeAsync to handle an already-existing admin
gracefully instead of relying on AnyAsync alone. Ensure the losing concurrent
insert path is caught and treated as a no-op so multiple app instances can start
safely against the same database.
In `@GameCollectionAPI/DTOs/AuthDto.cs`:
- Around line 11-12: The AuthDto.Password field only uses [Required], so
registration currently accepts very short passwords. Update the AuthDto class to
add a minimum-length validation attribute such as [MinLength] or [StringLength]
on Password, with a reasonable threshold, while keeping it aligned with how
AuthController.Register uses this DTO.
In `@GameCollectionAPI/DTOs/Users/UserCreateDto.cs`:
- Around line 11-12: The Password field in UserCreateDto only uses [Required],
so it needs the same strength policy as AuthDto.Password. Update
UserCreateDto.Password to use the shared/consistent password validation approach
already used for AuthDto, adding minimum length and complexity constraints
through the relevant DTO validation attributes or shared validation rule.
- Around line 7-9: Update UserCreateDto.Username to match the registration
policy used by AuthDto so UsersController.Create cannot accept shorter usernames
than allowed elsewhere. Adjust the StringLength validation on Username to
enforce the same minimum length as the auth flow, and keep the DTO validation
consistent across the create path.
In `@GameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.cs`:
- Around line 29-63: Add a unique database constraint for the Users.Username
column in the AddUsersAndRoles migration so duplicate usernames are blocked at
the DB level even under concurrent inserts. Update the migration that creates
the Users table to add a unique index on Username (alongside the existing RoleId
index), and then regenerate AppDbContextModelSnapshot so the model snapshot
reflects the new uniqueness constraint. Ensure the change is consistent with the
Users entity and the seeding flow in DbInitializer.
In `@GameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.Designer.cs`:
- Around line 111-120: The required Role-to-User relationship in the `User`
entity is configured with cascade delete, which can wipe out users when a role
is removed. Update the Fluent API in `AppDbContext.OnModelCreating` for the
`User`/`Role` relationship to use a non-cascading delete behavior such as
Restrict, then make the migration and `AddUsersAndRoles` designer/snapshot
metadata match the new delete behavior. Reference the `User` entity mapping,
`RoleId` foreign key, and `HasOne(...).WithMany()` configuration when applying
the fix.
- Around line 84-109: Add a database-level uniqueness guarantee for
`User.Username` so duplicate usernames cannot be inserted under concurrent
requests. Update the `AddUsersAndRoles` migration `Up()`/`Down()` to create and
remove a unique index on `Username`, and mirror the same change in
`AppDbContextModelSnapshot` so the model stays in sync. Also add Fluent API
configuration in `AppDbContext.OnModelCreating` (or the `User` entity
configuration) using `HasIndex(u => u.Username).IsUnique()` so future migrations
generate the constraint correctly.
In `@GameCollectionAPI/Program.cs`:
- Around line 27-45: Fail fast in the JWT setup by validating the required
configuration before building TokenValidationParameters. In Program.cs, read
Jwt:Key, Jwt:Issuer, and Jwt:Audience into locals in the authentication setup
around AddJwtBearer, and throw a clear exception if any value is missing or
empty instead of falling back to jwtSettings["Key"] ?? "". Keep the fix centered
on the jwtSettings/TokenValidationParameters block so the signing key is never
created from an empty string.
- Around line 28-46: Add the missing authorization service registration in
Program.cs by calling AddAuthorization on the service builder before the
middleware pipeline uses UseAuthorization. Keep the change alongside the
existing AddAuthentication/AddJwtBearer setup so the [Authorize] controllers are
backed by registered authorization services and the app can resolve the
authorization middleware correctly.
In `@GameCollectionAPI/Services/AuthService.cs`:
- Around line 43-46: The password check in AuthService’s login flow is too
strict because VerifyHashedPassword can return SuccessRehashNeeded for valid
credentials; update the authentication condition so anything other than Failed
is accepted. Keep the null hash guard, but adjust the check around
PasswordHasher<User>.VerifyHashedPassword in the login method so legitimate
users with rehash-needed passwords are not rejected.
In `@GameCollectionAPI/Services/UserService.cs`:
- Around line 69-84: In UserService.UpdateUsernameAsync, load the target user
with _repo.GetByIdAsync before checking username uniqueness, so missing users
return false/404 instead of DuplicateUsernameException. Then enforce uniqueness
only against other users by comparing against the current user’s existing
Username, allowing a no-op rename back to the same name. Keep the change
localized to UpdateUsernameAsync and use the existing DuplicateUsernameException
and _repo methods to preserve the controller’s intended 404/204 behavior.
---
Nitpick comments:
In @.github/workflows/ci-cd.yml:
- Around line 19-20: The checkout step in the workflow leaves git credentials
persisted on the runner, so harden the `actions/checkout` usage by setting
`persist-credentials` to false on the `Checkout repository` step. Update the
existing checkout configuration in the CI workflow only, keeping the rest of the
job unchanged.
- Around line 69-75: The Docker deployment in the CI/CD workflow is exposing
sensitive values by passing Jwt__Key and DefaultAdminPassword directly via
docker run -e. Update the deployment step to use a safer secret mechanism, such
as a restricted env file or Docker secrets, and adjust the docker run invocation
so the secrets are no longer visible on the command line or through docker
inspect; keep the change localized to the workflow job that launches
game-collection-api.
In `@Dockerfile`:
- Around line 6-9: The Dockerfile build steps should use a restore-first copy
pattern instead of copying the whole context before `dotnet restore`. Update the
Dockerfile to copy only the project/solution files needed by `dotnet restore`
first, run restore, then copy the remaining source before `dotnet publish`; also
ensure a `.dockerignore` excludes `bin`, `obj`, and other unnecessary files to
preserve cache efficiency.
In `@GameCollectionAPI/DTOs/Games/GameCreateDto.cs`:
- Around line 23-24: The GameCreateDto.ReleaseDate field currently has only
[Required], so it accepts unrealistic dates such as DateTime.MinValue or
far-future values. Add range validation or equivalent custom validation on
ReleaseDate in GameCreateDto to bound acceptable dates to a sensible window.
Keep the validation close to the property so the DTO enforces data quality
before reaching the API layer.
In `@GameCollectionAPI/Repositories/GameRepository.cs`:
- Around line 28-30: The GetAllAsync query in GameRepository is loading entities
with change tracking even though GameService maps them directly to DTOs. Update
the Games list query in GetAllAsync to use AsNoTracking() before ToListAsync so
the repository returns read-only results with less overhead.
In `@GameCollectionAPI/Repositories/UserRepository.cs`:
- Line 9: The UserRepository field _context is only assigned in the constructor,
so it should be made readonly to prevent accidental reassignment. Update the
UserRepository class definition so the _context field is declared as readonly
and keep its initialization in the constructor unchanged.
- Around line 16-26: The UserRepository AddAsync and DeleteAsync methods
currently save changes directly and will surface DbUpdateException instead of
the domain-specific DuplicateUsernameException when the username unique
constraint is hit. Update AddAsync, or the service layer that calls it, to catch
DbUpdateException from _context.SaveChangesAsync and translate the
duplicate-username case into DuplicateUsernameException so error handling stays
consistent; use the AddAsync/DeleteAsync methods in UserRepository and the
SaveChangesAsync call as the main points to adjust.
- Around line 38-41: UpdateAsync on UserRepository currently only calls
SaveChangesAsync, so it silently depends on callers having a tracked User from
GetByIdAsync or GetByUsernameAsync. Make the contract explicit by changing the
repository API to accept the entity being updated (or otherwise validate
tracking state) and ensure the method only saves when a tracked User is attached
to the AppDbContext. Use UserRepository, UpdateAsync, and the existing
GetByIdAsync/GetByUsernameAsync flow to locate the affected code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d56fdd25-1b08-402d-929b-df0a7e214993
📒 Files selected for processing (41)
.github/workflows/blank.yml.github/workflows/ci-cd.yml.gitignore.vscode/settings.jsonDockerfileGameCollectionAPI.slnGameCollectionAPI/Controllers/AuthController.csGameCollectionAPI/Controllers/GamesController.csGameCollectionAPI/Controllers/UsersController.csGameCollectionAPI/DTOs/AuthDto.csGameCollectionAPI/DTOs/Games/GameCreateDto.csGameCollectionAPI/DTOs/Games/GameReadDto.csGameCollectionAPI/DTOs/Users/UpdateUsernameDto.csGameCollectionAPI/DTOs/Users/UserCreateDto.csGameCollectionAPI/DTOs/Users/UserReadDto.csGameCollectionAPI/Data/AppDbContext.csGameCollectionAPI/Data/DbInitializer.csGameCollectionAPI/Exceptions/DuplicateUsernameException.csGameCollectionAPI/GameCollectionAPI.csprojGameCollectionAPI/GameCollectionAPI.httpGameCollectionAPI/Migrations/20250803020835_InitialMigration.Designer.csGameCollectionAPI/Migrations/20250803020835_InitialMigration.csGameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.Designer.csGameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.csGameCollectionAPI/Migrations/AppDbContextModelSnapshot.csGameCollectionAPI/Models/Game.csGameCollectionAPI/Models/Role.csGameCollectionAPI/Models/User.csGameCollectionAPI/Program.csGameCollectionAPI/Properties/launchSettings.jsonGameCollectionAPI/Repositories/GameRepository.csGameCollectionAPI/Repositories/IGameRepository.csGameCollectionAPI/Repositories/IUserRepository.csGameCollectionAPI/Repositories/UserRepository.csGameCollectionAPI/Services/AuthService.csGameCollectionAPI/Services/GameService.csGameCollectionAPI/Services/IGameService.csGameCollectionAPI/Services/IUserService.csGameCollectionAPI/Services/UserService.csGameCollectionAPI/appsettings.Development.jsonGameCollectionAPI/appsettings.json
💤 Files with no reviewable changes (1)
- .github/workflows/blank.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/pr-validation.yml (3)
8-9: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider adding
timeout-minutesto the job.Without a timeout, a stuck Docker build could run until the GitHub Actions default (360 min) is hit, wasting runner minutes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-validation.yml around lines 8 - 9, The build job currently lacks an explicit timeout, so add a timeout to the build job in the pr-validation workflow to prevent a hung Docker build from running too long. Update the build job definition alongside runs-on in the workflow so the job uses a reasonable timeout-minutes value.
7-10: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit least-privilege
permissionsblock.No
permissions:is declared for the workflow/job, so the default (potentially broad, org-dependent)GITHUB_TOKENscope applies. This job only needs to read the repo to build the image.🔧 Proposed fix
+permissions: + contents: read + jobs: build: runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-validation.yml around lines 7 - 10, The workflow job in pr-validation.yml is missing an explicit least-privilege permissions declaration, so the default GITHUB_TOKEN scope may be broader than needed. Update the build job to include a permissions block that grants only repository read access, keeping the change scoped to the existing jobs.build definition.
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
docker/build-push-actionpast v3.Flagged by actionlint as outdated. Current major versions are v6/v7; v3 lacks newer BuildKit features/fixes and its runner may eventually be deprecated.
🔧 Proposed fix
- uses: docker/build-push-action@v3 + uses: docker/build-push-action@v6🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-validation.yml at line 20, The workflow is still pinned to the outdated docker/build-push-action v3, so update the action reference in the pr-validation workflow to a current supported major release such as v6 or v7. Keep the same build step semantics, but make sure any inputs or outputs used by the existing build-push-action usage are still valid after the version bump and adjust them if the newer action expects different names or behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-validation.yml:
- Around line 12-13: The checkout step in the pr-validation workflow should
disable persisted credentials because this job only builds and never pushes.
Update the actions/checkout usage in the workflow to set persist-credentials to
false so the GITHUB_TOKEN is not left in .git/config, and keep the change
localized to the checkout step identified by the checkout action entry.
---
Nitpick comments:
In @.github/workflows/pr-validation.yml:
- Around line 8-9: The build job currently lacks an explicit timeout, so add a
timeout to the build job in the pr-validation workflow to prevent a hung Docker
build from running too long. Update the build job definition alongside runs-on
in the workflow so the job uses a reasonable timeout-minutes value.
- Around line 7-10: The workflow job in pr-validation.yml is missing an explicit
least-privilege permissions declaration, so the default GITHUB_TOKEN scope may
be broader than needed. Update the build job to include a permissions block that
grants only repository read access, keeping the change scoped to the existing
jobs.build definition.
- Line 20: The workflow is still pinned to the outdated docker/build-push-action
v3, so update the action reference in the pr-validation workflow to a current
supported major release such as v6 or v7. Keep the same build step semantics,
but make sure any inputs or outputs used by the existing build-push-action usage
are still valid after the version bump and adjust them if the newer action
expects different names or behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27df6115-7440-4e3c-a615-cab67177eece
📒 Files selected for processing (2)
.github/workflows/ci-cd.yml.github/workflows/pr-validation.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-cd.yml
- Add `USER $APP_UID` instruction to the final stage in the Dockerfile. - Improve container security by running the ASP.NET Core application under a non-root user.
- Check if the JWT secret key is null or empty in the configuration and throw an `InvalidOperationException` if so. - Use the validated JWT key for `IssuerSigningKey` configuration in authentication service setup. - Remove the empty `Key` field from `appsettings.json`.
…pt safety - Upgrade `docker/login-action` from `v2` to `v4` - Upgrade `docker/build-push-action` from `v3` to `v7` - Add `contents: read` permission to the `deploy` job - Improve SSH deployment script safety by assigning `github.actor` to a shell variable and properly quoting secrets
- Move ID validation to run before the authorization check in `Get` and `UpdateUsername` actions. - Update ID validation condition from `id == 0` to `id <= 0 && id != -1`.
…ersController - Change the return statement in the user creation action from `CreatedAtAction` to `CreatedAtRoute` to resolve potential routing issues and correctly generate the `Location` header.
- Configure unique index on the `Username` field of the `User` entity in `AppDbContext` - Create and apply database migration `AddUniqueUsername` - Refactor `DbInitializer` to check for existing admin users by username instead of role ID - Enhance `DbInitializer` with error handling (`DbUpdateException`) to gracefully manage potential conflicts during admin user seeding
- Add `[MinLength(6)]` attribute to the `Password` property in `AuthDto` to enforce a minimum length constraint of 6 characters.
- Increase minimum length of `Username` from 1 to 3 - Add minimum length requirement of 6 characters to `Password`
- Configure the `User` entity to use `DeleteBehavior.Restrict` on its `Role` foreign key in `AppDbContext` - Update the `AddUsersAndRoles` migration to use `ReferentialAction.Restrict` instead of `Cascade` - Regenerate model designer snapshots (`AddUsersAndRoles.Designer.cs`, `AddUniqueUsername.Designer.cs`, `AppDbContextModelSnapshot.cs`) to reflect the updated delete behavior
- Add `builder.Services.AddAuthorization()` to the dependency injection container in `Program.cs`
- Update the password verification condition in `AuthService.cs` to check for `PasswordVerificationResult.Failed` instead of `!= PasswordVerificationResult.Success`. - Ensure authentications do not fail when the password verification result is `SuccessRehashNeeded`.
…plicate error - Retrieve the user by ID first before performing the duplicate username check. - Only throw `DuplicateUsernameException` if the existing username belongs to a different user. - Add a short-circuit return when the new username is identical to the current one.
- Add `persist-credentials: false` to the checkout step in `.github/workflows/pr-validation.yml` to improve security.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci-cd.yml (1)
58-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTemplate-injection risk not fully resolved.
Assigning
GH_ACTOR="${{ github.actor }}"still expands the expression directly into the script text before execution; it isn't a step-level env var, sozizmorcontinues to flag it correctly. A double-quoted bash assignment doesn't stop command substitution ($()/backticks) inside an interpolated value from executing. To fully close this, passgithub.actorvia the action'senvs:(appleboy/ssh-action supports forwarding stepenv:vars into the remote script) instead of interpolating it into the script body.🔒 Proposed fix
- name: Deploy to Oracle VM uses: appleboy/ssh-action@v1.0.3 + env: + GH_ACTOR: ${{ github.actor }} with: host: ${{ secrets.ORACLE_VM_HOST }} username: ${{ secrets.ORACLE_VM_USERNAME }} key: ${{ secrets.ORACLE_VM_SSH_KEY }} + envs: GH_ACTOR script: | - GH_ACTOR="${{ github.actor }}" # Log in to GitHub Container Registry echo "${{ secrets.GHCR_PAT }}" | docker login ghcr.io -u "$GH_ACTOR" --password-stdin🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-cd.yml around lines 58 - 61, The SSH login script still interpolates github.actor directly inside the script body, so the template-injection risk remains in the appleboy/ssh-action step. Move GH_ACTOR out of the script and pass github.actor through the action’s envs/env mechanism so it is available as a step-level environment variable, then reference that variable inside the script in the login block. Keep the fix scoped to the workflow step that calls docker login ghcr.io and uses GH_ACTOR.Source: Linters/SAST tools
🧹 Nitpick comments (2)
.github/workflows/ci-cd.yml (1)
70-77: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffSecrets passed as
docker run -eargs are exposed via process listing /docker inspect.
JWT_SECRET_KEYandDEFAULT_ADMIN_PASSWORDare interpolated directly into thedocker runcommand line, so they remain visible to anyone with access tops auxon the host ordocker inspecton the running container. Consider writing secrets to a restricted-permission.envfile on the VM and using--env-fileinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-cd.yml around lines 70 - 77, The `docker run` step in the CI/CD workflow is passing sensitive values directly with `-e`, which exposes them through process inspection and container metadata. Update the deployment flow around the `docker run` command to avoid inline secret interpolation for `Jwt__Key` and `DefaultAdminPassword`; instead, have the workflow write those values into a restricted-permission `.env` file on the VM and launch the container with `--env-file`. Keep the change localized to the deployment command block so the `game-collection-api` run step still sets `ASPNETCORE_ENVIRONMENT` and starts the same image.GameCollectionAPI/Controllers/UsersController.cs (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the ID validation and the
-1sentinel;Deleteis inconsistent with the other actions.
Get/UpdateUsernamereject withid <= 0 && id != -1(positive IDs and the default admin-1allowed), butDeleteusesid == 0 || id == -1, so any other negative ID (-2,-3, …) skips validation and reaches_service.DeleteUserAsync. The magic-1is also repeated across three actions. Extract a named constant and a single validation helper so the rule is uniform and the sentinel intent is explicit.Also applies to: Line 91, Line 123
♻️ Suggested consolidation
private const int DefaultAdminId = -1; private static bool IsValidUserId(int id) => id > 0 || id == DefaultAdminId;- if (id <= 0 && id != -1) { return BadRequest(); } + if (!IsValidUserId(id)) { return BadRequest(); }- if ((id <= 0 && id != -1) || updateUsernameDto == null) { return BadRequest(); }; + if (!IsValidUserId(id) || updateUsernameDto == null) { return BadRequest(); }For
Delete, guard invalid IDs and still block the default admin:- if (id == 0 || id == -1) { return BadRequest(); } // Default admin (with ID -1) can't be deleted + if (!IsValidUserId(id) || id == DefaultAdminId) { return BadRequest(); } // Default admin can't be deleted🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GameCollectionAPI/Controllers/UsersController.cs` at line 36, The ID validation is inconsistent across UsersController actions, and the magic -1 sentinel is duplicated in multiple places. Add a named constant for the default admin ID and a shared helper in UsersController (used by Get, UpdateUsername, and Delete) so the rule is uniform: accept positive IDs and the sentinel, reject all other invalid values. Update Delete to use the same validation path and still block deleting the default admin account.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GameCollectionAPI/Migrations/20260701090346_AddUniqueUsername.cs`:
- Around line 13-17: The unique index creation in the migration will fail if
existing Users.Username values are duplicated, so update the migration to handle
that state before calling CreateIndex. Add a cleanup/backfill step or an
explicit duplicate precheck in the migration class before the index is created,
and use the migration’s Up method to either resolve duplicates safely or stop
with a clear controlled error message. Refer to the migration class that
contains CreateIndex for the Username field and keep the startup path via
DbInitializer in mind.
---
Duplicate comments:
In @.github/workflows/ci-cd.yml:
- Around line 58-61: The SSH login script still interpolates github.actor
directly inside the script body, so the template-injection risk remains in the
appleboy/ssh-action step. Move GH_ACTOR out of the script and pass github.actor
through the action’s envs/env mechanism so it is available as a step-level
environment variable, then reference that variable inside the script in the
login block. Keep the fix scoped to the workflow step that calls docker login
ghcr.io and uses GH_ACTOR.
---
Nitpick comments:
In @.github/workflows/ci-cd.yml:
- Around line 70-77: The `docker run` step in the CI/CD workflow is passing
sensitive values directly with `-e`, which exposes them through process
inspection and container metadata. Update the deployment flow around the `docker
run` command to avoid inline secret interpolation for `Jwt__Key` and
`DefaultAdminPassword`; instead, have the workflow write those values into a
restricted-permission `.env` file on the VM and launch the container with
`--env-file`. Keep the change localized to the deployment command block so the
`game-collection-api` run step still sets `ASPNETCORE_ENVIRONMENT` and starts
the same image.
In `@GameCollectionAPI/Controllers/UsersController.cs`:
- Line 36: The ID validation is inconsistent across UsersController actions, and
the magic -1 sentinel is duplicated in multiple places. Add a named constant for
the default admin ID and a shared helper in UsersController (used by Get,
UpdateUsername, and Delete) so the rule is uniform: accept positive IDs and the
sentinel, reject all other invalid values. Update Delete to use the same
validation path and still block deleting the default admin account.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ea0feef-6ed6-481e-895a-3ea8626d0046
📒 Files selected for processing (17)
.github/workflows/ci-cd.yml.github/workflows/pr-validation.ymlDockerfileGameCollectionAPI/Controllers/UsersController.csGameCollectionAPI/DTOs/AuthDto.csGameCollectionAPI/DTOs/Users/UserCreateDto.csGameCollectionAPI/Data/AppDbContext.csGameCollectionAPI/Data/DbInitializer.csGameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.Designer.csGameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.csGameCollectionAPI/Migrations/20260701090346_AddUniqueUsername.Designer.csGameCollectionAPI/Migrations/20260701090346_AddUniqueUsername.csGameCollectionAPI/Migrations/AppDbContextModelSnapshot.csGameCollectionAPI/Program.csGameCollectionAPI/Services/AuthService.csGameCollectionAPI/Services/UserService.csGameCollectionAPI/appsettings.json
✅ Files skipped from review due to trivial changes (3)
- GameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.Designer.cs
- GameCollectionAPI/Migrations/20260701090346_AddUniqueUsername.Designer.cs
- GameCollectionAPI/Migrations/AppDbContextModelSnapshot.cs
🚧 Files skipped from review as they are similar to previous changes (7)
- GameCollectionAPI/Migrations/20250831143809_AddUsersAndRoles.cs
- GameCollectionAPI/DTOs/AuthDto.cs
- Dockerfile
- GameCollectionAPI/Services/UserService.cs
- GameCollectionAPI/Program.cs
- .github/workflows/pr-validation.yml
- GameCollectionAPI/Services/AuthService.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit