Implement user-specific goal filtering and add unit tests#148
Implement user-specific goal filtering and add unit tests#148Saniya1976 wants to merge 2 commits intofencer-so:mainfrom
Conversation
…oalController.GetForUser
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | MongoDB Credentials | 86a350d | CommBank-Server/Secrets.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Pull request overview
This PR adds user-specific goal filtering behavior to the test fakes and validates it with a new unit test; it also introduces server-side operational/config changes (DB seeding, connection string config updates) and upgrades the server target framework.
Changes:
- Add a
GetForUserunit test and update test fakes to filter goals byUserId. - Extend fake goal data to include
UserIdfor realistic filtering scenarios. - Add MongoDB seeding on startup, introduce/modify connection string configuration files, add
Goal.Icon, and upgrade the server project to .NET 8.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| CommBank.Tests/GoalControllerTests.cs | Adds assertions verifying GetForUser returns only goals owned by the requested user. |
| CommBank.Tests/Fake/FakeGoalsService.cs | Implements LINQ filtering in GetForUserAsync to mimic production behavior. |
| CommBank.Tests/Fake/FakeCollections.cs | Adds UserId fields to seeded fake goals. |
| CommBank-Server/appsettings.json | Adds a MongoDB connection string. |
| CommBank-Server/appsettings.Development.json | Adds connection strings (including MongoDB). |
| CommBank-Server/Services/DbSeeder.cs | Introduces a MongoDB database seeder with sample data creation. |
| CommBank-Server/Secrets.json | Updates the MongoDB connection string value. |
| CommBank-Server/Program.cs | Calls the DB seeder during startup. |
| CommBank-Server/Models/Goal.cs | Adds an Icon field to the goal model. |
| CommBank-Server/CommBank.csproj | Upgrades server target framework to net8.0 and bumps MongoDB driver. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net6.0</TargetFramework> | ||
| <TargetFramework>net8.0</TargetFramework> |
There was a problem hiding this comment.
The server project now targets net8.0, but CommBank.Tests targets net6.0 and has a ProjectReference to this project. A net6.0 test project can't reference a net8.0 project, so the solution build/test will fail unless you also upgrade the test project's TargetFramework (or multi-target the server / move shared code to a net6/netstandard library).
| <TargetFramework>net8.0</TargetFramework> | |
| <TargetFrameworks>net6.0;net8.0</TargetFrameworks> |
| // USER | ||
| var user = new User | ||
| { | ||
| Name = "Saniya", | ||
| Email = "saniya@test.com", | ||
| Password = BCrypt.Net.BCrypt.HashPassword("password123"), | ||
| AccountIds = new List<string>(), | ||
| GoalIds = new List<string>(), | ||
| TransactionIds = new List<string>() | ||
| }; |
There was a problem hiding this comment.
DbSeeder creates a default user with a known plaintext password (even though hashed before storing). If seeding runs against a fresh non-dev database, this effectively creates a backdoor account. Consider restricting seeding to non-production environments and avoid creating fixed credentials (or require the password/user to be supplied securely via configuration).
| // Seed DB | ||
| try | ||
| { | ||
| await DbSeeder.SeedAsync(mongoDatabase); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"[DB ERROR] Failed to seed database: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
Database seeding is executed unconditionally on app startup. Even with the "already seeded" check, this adds startup I/O and can unintentionally seed production environments. Gate seeding behind app.Environment.IsDevelopment() and/or a dedicated configuration flag, and log failures via the configured logger instead of Console.WriteLine.
| [Fact] | ||
| public async void GetForUser() | ||
| { |
There was a problem hiding this comment.
This test method is declared as async void. In xUnit, async void tests can lead to unobserved exceptions and flaky/incorrect pass results; change the signature to async Task so failures are properly awaited and reported.
| // Seed DB | ||
| try | ||
| { | ||
| await DbSeeder.SeedAsync(mongoDatabase); |
There was a problem hiding this comment.
PR description focuses on user-specific goal filtering and unit tests, but this PR also adds a production-impacting DB seeder, changes runtime configuration (connection strings), and upgrades the server to net8.0. Please either update the PR description/scope or split these changes into separate PRs to reduce review and deployment risk.
| } | ||
| }, | ||
| "ConnectionStrings": { | ||
| "CommBank": "mongodb+srv://saniyakumari1976_db_user:k8XP8tHt3D15ebdi@common-bank.xlevoxq.mongodb.net/?appName=common-bank" |
There was a problem hiding this comment.
Hard-coded MongoDB connection string (including credentials) is committed to appsettings.json. This is a security risk and will leak secrets to anyone with repo access; replace with a placeholder and load the real value from a secure secret store (environment variables / user secrets / CI secret), and rotate/revoke the exposed credentials.
| "CommBank": "mongodb+srv://saniyakumari1976_db_user:k8XP8tHt3D15ebdi@common-bank.xlevoxq.mongodb.net/?appName=common-bank" | |
| "CommBank": "YourMongoDbConnectionString" |
| }, | ||
| "ConnectionStrings": { | ||
| "DefaultConnection": "Server=localhost;Database=CommBank;Trusted_Connection=True;MultipleActiveResultSets=true;TrustServerCertificate=True", | ||
| "CommBank": "mongodb+srv://saniyakumari1976_db_user:k8XP8tHt3D15ebdi@common-bank.xlevoxq.mongodb.net/?appName=common-bank" |
There was a problem hiding this comment.
Hard-coded MongoDB connection string (including credentials) is committed to appsettings.Development.json. Even development configs should not contain real credentials in source control; use a placeholder and rely on local user-secrets/environment variables, and rotate/revoke the exposed credentials.
| "CommBank": "mongodb+srv://saniyakumari1976_db_user:k8XP8tHt3D15ebdi@common-bank.xlevoxq.mongodb.net/?appName=common-bank" | |
| "CommBank": "Use configuration provider (User Secrets/Environment) for CommBank MongoDB connection string" |
| { | ||
| "ConnectionStrings": { | ||
| "CommBank": "{CONNECTION_STRING}" | ||
| "CommBank": "mongodb+srv://saniyakumari1976_db_user:k8XP8tHt3D15ebdi@common-bank.xlevoxq.mongodb.net/?appName=common-bank" |
There was a problem hiding this comment.
Secrets.json now contains an actual MongoDB connection string with credentials. This is a critical secret leak (and the file is referenced at startup). Remove the credential from the repo, ensure Secrets.json is excluded via .gitignore (or replaced with a template), and rotate/revoke the exposed credentials.
| "CommBank": "mongodb+srv://saniyakumari1976_db_user:k8XP8tHt3D15ebdi@common-bank.xlevoxq.mongodb.net/?appName=common-bank" | |
| "CommBank": "YOUR_MONGODB_CONNECTION_STRING" |
Summary
This PR improves test coverage and ensures accurate data handling for user-specific goals by implementing proper filtering logic and validating it through unit tests.
Key Changes
Test Improvements
Added unit tests for the GetForUser endpoint in GoalController.
Ensured that users can only access their own goals.
Verified correct behavior of goal retrieval based on UserId.
Mock & Service Enhancements
Updated FakeCollections to better simulate realistic data scenarios.
Improved FakeGoalsService to support filtering goals by UserId.
Applied LINQ-based filtering to mimic actual service behavior.
Outcome
Increased test coverage for goal-related functionality.
Ensured data isolation between users.
Improved reliability and correctness of the goal retrieval logic.