Conversation
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/src/Domain/Users/PendingRegistration.cs (1)
11-15:⚠️ Potential issue | 🟠 MajorEnforce
NormalizedEmailinvariant inside the entity.
normalizedEmailbeing caller-provided in both creation and refresh paths can still desync persisted state. Normalize from🐛 Proposed fix
public sealed class PendingRegistration { + private static string NormalizeEmail(string email) + { + if (string.IsNullOrWhiteSpace(email)) + throw new ArgumentException("Email is required.", nameof(email)); + return email.Trim().ToUpperInvariant(); + } + private PendingRegistration() { } - public PendingRegistration(string email, string normalizedEmail) + public PendingRegistration(string email) { Email = email; - NormalizedEmail = normalizedEmail; + NormalizedEmail = NormalizeEmail(email); } @@ public void Refresh( string email, - string normalizedEmail, string displayName, string passwordHash, string verificationCodeHash, DateTime codeExpiresAtUtc, DateTime codeSentAtUtc) { Email = email; - NormalizedEmail = normalizedEmail; + NormalizedEmail = NormalizeEmail(email); DisplayName = displayName; PasswordHash = passwordHash; VerificationCodeHash = verificationCodeHash;Also applies to: 29-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/Domain/Users/PendingRegistration.cs` around lines 11 - 15, PendingRegistration currently accepts a caller-provided normalizedEmail which can lead to desyncs; change the entity to derive NormalizedEmail internally from Email instead of trusting the parameter: update the constructor PendingRegistration(string email, string normalizedEmail) to take only email (or ignore the normalizedEmail param) and compute NormalizedEmail via the project’s canonical email normalization helper (use the same normalization call used elsewhere), and apply the same change to the refresh/update path referenced around lines 29-40 so that any Refresh/Update method recomputes NormalizedEmail from Email; then update callers to stop providing normalizedEmail or ignore it.backend/docker-compose.yml (1)
13-13:⚠️ Potential issue | 🟠 MajorRemove the SQL Server default password fallback.
Line 13 still bakes in a known fallback secret (
YourStrong@Password). IfDatabaseProvideris switched, this becomes an active credential path and weakens security posture.🔒 Proposed fix
- - SqlServer__ConnectionString=Server=sqlserver;Database=AzureOpsCrew;User Id=sa;Password=${SQL_SERVER_SA_PASSWORD:-YourStrong@Password};TrustServerCertificate=True; + - SqlServer__ConnectionString=${SQLSERVER_CONNECTION_STRING:?SQLSERVER_CONNECTION_STRING must be set}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/docker-compose.yml` at line 13, Remove the baked-in fallback secret by editing the environment entry that sets SqlServer__ConnectionString: stop using the inline default `${SQL_SERVER_SA_PASSWORD:-YourStrong@Password}` and instead reference `${SQL_SERVER_SA_PASSWORD}` (or wire in a Docker secret) so the hardcoded `YourStrong@Password` is never used; ensure any deployment/startup validates the variable is present and fails fast if missing.
🧹 Nitpick comments (1)
backend/src/Api/Extensions/ServiceCollectionExtensions.cs (1)
111-113: Duplicate settings deserialization across both extension methods — optional cleanup.Both
AddJwtAuthenticationandAddKeycloakOidcSupportindependently callconfiguration.GetSection("KeycloakOidc").Get<KeycloakOidcSettings>(). While benign (both methods are public and may be called independently), if they are always paired there's an opportunity to reduce redundant deserialization.Also applies to: 174-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/Api/Extensions/ServiceCollectionExtensions.cs` around lines 111 - 113, Both AddJwtAuthentication and AddKeycloakOidcSupport duplicate configuration.GetSection("KeycloakOidc").Get<KeycloakOidcSettings>() — extract the deserialization into a single shared step and reuse it to avoid redundant work; for example, create a private static method or overload that returns KeycloakOidcSettings (used by AddJwtAuthentication and AddKeycloakOidcSupport) or read it once and pass the KeycloakOidcSettings instance into both methods so both methods use the same deserialized object instead of calling configuration.GetSection(...).Get<KeycloakOidcSettings>() independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/docker-compose.yml`:
- Around line 32-33: The docker-compose service mounts a persistent volume using
the entry "volumes: - ./data:/app/data", so add a blanket ignore for the data
directory to .gitignore (e.g., add a "data/" entry) to prevent any files under
that mount (including SQLite DBs) from being accidentally committed; update the
.gitignore by appending the directory ignore rather than relying only on
individual filenames like azureopscrew.db*.
In `@backend/src/Api/Extensions/ServiceCollectionExtensions.cs`:
- Around line 134-142: Enable audience validation on the JWT options: set
TokenValidationParameters.ValidateAudience = true and configure
TokenValidationParameters.ValidAudience (or ValidAudiences) to the API's client
id so the middleware rejects tokens not intended for this resource; update the
options block that constructs TokenValidationParameters (where ValidateIssuer,
ValidIssuer, ValidateAudience, ValidateLifetime, ClockSkew, NameClaimType are
set) to flip ValidateAudience to true and provide the expected audience
value(s), and ensure Keycloak is configured to emit the aud claim (via an
Audience protocol mapper) before deploying this change.
In `@backend/src/Api/Program.cs`:
- Around line 47-48: The call order in Program.cs makes AddJwtAuthentication run
before AddKeycloakOidcSupport so the settings.Enabled guard inside
AddKeycloakOidcSupport never executes; reorder the service registrations so
AddKeycloakOidcSupport(builder.Configuration, builder.Environment) is invoked
before AddJwtAuthentication(builder.Configuration, builder.Environment),
ensuring the settings.Enabled check in AddKeycloakOidcSupport can run, or
alternatively remove/adjust the redundant Enabled check in
AddKeycloakOidcSupport if you intend AddJwtAuthentication to be authoritative.
---
Duplicate comments:
In `@backend/docker-compose.yml`:
- Line 13: Remove the baked-in fallback secret by editing the environment entry
that sets SqlServer__ConnectionString: stop using the inline default
`${SQL_SERVER_SA_PASSWORD:-YourStrong@Password}` and instead reference
`${SQL_SERVER_SA_PASSWORD}` (or wire in a Docker secret) so the hardcoded
`YourStrong@Password` is never used; ensure any deployment/startup validates the
variable is present and fails fast if missing.
In `@backend/src/Domain/Users/PendingRegistration.cs`:
- Around line 11-15: PendingRegistration currently accepts a caller-provided
normalizedEmail which can lead to desyncs; change the entity to derive
NormalizedEmail internally from Email instead of trusting the parameter: update
the constructor PendingRegistration(string email, string normalizedEmail) to
take only email (or ignore the normalizedEmail param) and compute
NormalizedEmail via the project’s canonical email normalization helper (use the
same normalization call used elsewhere), and apply the same change to the
refresh/update path referenced around lines 29-40 so that any Refresh/Update
method recomputes NormalizedEmail from Email; then update callers to stop
providing normalizedEmail or ignore it.
---
Nitpick comments:
In `@backend/src/Api/Extensions/ServiceCollectionExtensions.cs`:
- Around line 111-113: Both AddJwtAuthentication and AddKeycloakOidcSupport
duplicate configuration.GetSection("KeycloakOidc").Get<KeycloakOidcSettings>() —
extract the deserialization into a single shared step and reuse it to avoid
redundant work; for example, create a private static method or overload that
returns KeycloakOidcSettings (used by AddJwtAuthentication and
AddKeycloakOidcSupport) or read it once and pass the KeycloakOidcSettings
instance into both methods so both methods use the same deserialized object
instead of calling configuration.GetSection(...).Get<KeycloakOidcSettings>()
independently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/docker-compose.ymlbackend/src/Api/Extensions/ServiceCollectionExtensions.csbackend/src/Api/Program.csbackend/src/Domain/Users/PendingRegistration.csbackend/src/Infrastructure.Db/EntityTypes/Sqlite/UserExternalIdentityEntityTypeConfiguration.csfrontend/app/api/auth/keycloak/callback/route.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/Infrastructure.Db/EntityTypes/Sqlite/UserExternalIdentityEntityTypeConfiguration.cs
- frontend/app/api/auth/keycloak/callback/route.ts
|
This PR needs rebasing. Also, using KeyCloak will slow down the local development, so let's postpone this change till the end of hackathon. |
Summary by CodeRabbit
New Features
Improvements