Refactor user profile management and add Azure Container Apps support #213
Conversation
Major refactor introduces Visage.Services.UserProfile microservice, separating user profile (User) from event registration (EventRegistration). Implements new UserDB context, robust profile and social verification APIs, and supports multiple event registrations per user. Adds AIDE profile logic, draft save/restore, and unique social linking. Updates database schema, migrations, and project structure for modular, scalable, and service-oriented architecture. Legacy Registrant model and related code are removed.
Added Aspire.Hosting.Azure.AppContainers package and configured Azure App Container environment in AppHost.cs to enable deployment to Azure Container Apps.
- Changed Azure App Container env from "env" to "dev" and refactored setup. - Limited Scalar API Reference config to development environment, improved options (HTTPS, self-signed certs, theme). - Registered both eventAPI and userProfileApi with Scalar. - Added comment explaining ENC0118 hot reload warning. - Minor formatting tweaks in .csproj, no package changes.
The "Schedule Event" button on the Upcoming Events section is now only visible to users with the "VisageAdmin" role by wrapping it in an <AuthorizeView> component. Previously, this button was accessible to all users.
…age into rf_registrant2user
|
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. 📝 WalkthroughWalkthroughRefactors Registrant → User across services and frontend, introduces a new UserProfile service and UserDB with migrations, adds event registration and session check‑in endpoints, swaps registrations-api → userprofile-api, updates shared models, tests, CI, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Authenticated User
participant Frontend as Frontend
participant EventService as Frontend EventService
participant UserProfileAPI as UserProfile API
participant EventingAPI as Eventing API
participant UserDB as UserDB
User->>Frontend: Click RSVP
Frontend->>Frontend: Ensure authenticated / check profile
alt Profile missing or incomplete
Frontend->>UserProfileAPI: GET /api/users/{id}
UserProfileAPI->>UserDB: Query User by Id
UserDB-->>UserProfileAPI: Return User
UserProfileAPI-->>Frontend: Profile status
Frontend->>Frontend: Redirect to MandatoryRegistration if incomplete
Frontend->>UserProfileAPI: POST /api/users (create/update)
UserProfileAPI->>UserDB: Upsert User
UserDB-->>UserProfileAPI: Return User
end
Frontend->>EventService: RegisterForEventAsync(eventId, details)
EventService->>EventingAPI: POST /api/registrations (RegisterEventRequest)
EventingAPI->>UserDB: Insert EventRegistration (FK -> Users)
UserDB-->>EventingAPI: Return EventRegistration (with PIN)
EventingAPI-->>EventService: Registration result
EventService-->>Frontend: Success → show RSVP message
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| SocialDisconnectDto dto, | ||
| ILogger<RegistrantDB> logger) => | ||
| ILogger<UserDB> logger) => | ||
| { |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix log-forging issues, user-provided values should be sanitized or encoded before being written to logs. For plain-text logs, at minimum remove newline (\r, \n) characters and optionally other control characters; for HTML-based viewing, HTML-encode user input before logging. This prevents an attacker from injecting additional fake log entries or altering the perceived structure of the log.
In this specific case, the best minimal fix while preserving existing functionality is to sanitize normalizedProfileUrl in the same way as normalizedProvider (and possibly more robustly) before using it in the logger.LogInformation call at line 1062. We should introduce a new local variable, e.g. sanitizedProfileUrlForLog, that is derived from normalizedProfileUrl with at least carriage returns and newlines removed. Then use that sanitized variable in the log statement instead of the raw normalizedProfileUrl. This change is confined to the LinkSocialProfile endpoint in Visage.Services.UserProfile/ProfileApi.cs, and does not affect how the profile URL is stored in the database or returned in the HTTP response. No new imports are required, since we can use string.Replace or string.Create directly from System, which is already implicitly available in C# 10+ top-level using context. The specific region to change is lines 1058–1063: retain the existing provider sanitization, add analogous sanitization for the profile URL, and update the logger call to use the sanitized URL.
| @@ -1059,8 +1059,12 @@ | ||
| .Replace("\r", string.Empty) | ||
| .Replace("\n", string.Empty); | ||
|
|
||
| logger.LogInformation("Verified {Provider} profile for user {UserId}: {ProfileUrl}", sanitizedProviderForLog, user.Id, normalizedProfileUrl); | ||
| var sanitizedProfileUrlForLog = normalizedProfileUrl | ||
| .Replace("\r", string.Empty) | ||
| .Replace("\n", string.Empty); | ||
|
|
||
| logger.LogInformation("Verified {Provider} profile for user {UserId}: {ProfileUrl}", sanitizedProviderForLog, user.Id, sanitizedProfileUrlForLog); | ||
|
|
||
| return Results.Ok(new | ||
| { | ||
| message = $"{normalizedProvider} profile linked successfully", |
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile GET: No registrant found for userId/email {UserId}", userId); | ||
| logger.LogWarning("Profile GET: No user found for userId/email {UserId}", userId); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To generally fix log forging risks, any user-provided data written to logs should be normalized/encoded so it cannot inject control characters (especially newlines) into the log stream. For plain-text logs, this usually means stripping or replacing \r and \n and possibly other non-printable characters before logging. For HTML-rendered logs, HTML encoding is appropriate.
In this specific case, the best minimal fix is to sanitize userId once near the beginning of the handler and then use the sanitized value for all logging, while still using the original userId for lookups and logic to preserve functionality. We can define a local safeUserIdForLog that removes carriage returns and newlines from userId using Replace, and then change the logging calls at lines 194, 201, and 229 to use this sanitized value instead of the raw userId. This keeps behavior intact (database queries, conditional logic) and changes only what is written into the logs. No additional imports are required since string.Replace is already available.
Concretely:
- Inside the lambda at line 187, immediately after the opening brace, introduce
var safeUserIdForLog = userId.Replace("\r", string.Empty).Replace("\n", string.Empty);. - Update all calls
logger.LogInformation(..., userId)andlogger.LogWarning(..., userId)within this handler to usesafeUserIdForLoginstead. - Leave all uses of
userIdin queries and logic unchanged.
| @@ -186,19 +186,22 @@ | ||
| // T037: Retrieve full profile by user ID for editing | ||
| group.MapGet("/{userId}", async (string userId, UserDB db, ILogger<UserDB> logger) => | ||
| { | ||
| // Normalize userId for safe logging to prevent log forging via control characters | ||
| var safeUserIdForLog = userId.Replace("\r", string.Empty).Replace("\n", string.Empty); | ||
|
|
||
| // Validate StrictId format - if not, try email lookup | ||
| Id<User>? parsedUserId = null; | ||
| if (!Id<User>.TryParse(userId, out var tmpId)) | ||
| { | ||
| // Try email lookup (for external auth providers like Auth0) | ||
| logger.LogInformation("Profile GET: userId {UserId} is not a StrictId, attempting email lookup", userId); | ||
| logger.LogInformation("Profile GET: userId {UserId} is not a StrictId, attempting email lookup", safeUserIdForLog); | ||
| var userByEmail = await db.Users | ||
| .AsNoTracking() | ||
| .FirstOrDefaultAsync(u => u.Email == userId); | ||
|
|
||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile GET: No user found for userId/email {UserId}", userId); | ||
| logger.LogWarning("Profile GET: No user found for userId/email {UserId}", safeUserIdForLog); | ||
| return Results.NotFound(); | ||
| } | ||
|
|
||
| @@ -226,7 +218,7 @@ | ||
| return Results.NotFound(); | ||
| } | ||
|
|
||
| logger.LogInformation("Profile GET: Found user {Id} for userId {UserId}", user.Id, userId); | ||
| logger.LogInformation("Profile GET: Found user {Id} for userId {UserId}", user.Id, safeUserIdForLog); | ||
|
|
||
| // T037: Return full User for ProfileEdit component | ||
| return Results.Ok(user); |
| } | ||
|
|
||
| logger.LogInformation("Profile GET: Found registrant {Id} for userId {UserId}", registrant.Id, userId); | ||
| logger.LogInformation("Profile GET: Found user {Id} for userId {UserId}", user.Id, userId); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix log-forging issues, sanitize or normalize user-provided values before including them in log entries. For plain-text logs, this typically means stripping or replacing newline and other control characters so a malicious value cannot break log structure or inject additional entries. For logs that may be rendered as HTML, apply suitable HTML encoding.
For this specific case, the best minimal fix is to introduce a sanitized version of userId (e.g., sanitizedUserId) that removes newline sequences and use that variable in all log calls that include the route userId. This preserves existing behavior (user lookup, returned values) while ensuring that what is written to the log cannot contain line breaks. We can implement a small local helper (e.g., a lambda or private static method within ProfileApi) or just inline a Replace chain at the point of use. Since we must limit edits to the shown snippet and avoid broader refactors, the simplest safe approach is to define sanitizedUserId once near the top of the handler and use it in every logging call that logs the incoming userId. No new imports are required because string.Replace and Environment.NewLine are from the base class library.
Concretely, in Visage.Services.UserProfile/ProfileApi.cs, within the MapGet("/{userId}", ...) endpoint starting at line 187, add a line that creates sanitizedUserId by removing Environment.NewLine, \n, and \r from userId. Then, replace the use of userId with sanitizedUserId in the three logging calls inside this handler (lines 194, 201, and 229). All other logic remains unchanged.
| @@ -186,19 +186,25 @@ | ||
| // T037: Retrieve full profile by user ID for editing | ||
| group.MapGet("/{userId}", async (string userId, UserDB db, ILogger<UserDB> logger) => | ||
| { | ||
| // Sanitize userId for safe logging (remove line breaks/control characters) | ||
| var sanitizedUserId = userId | ||
| .Replace(Environment.NewLine, string.Empty) | ||
| .Replace("\n", string.Empty) | ||
| .Replace("\r", string.Empty); | ||
|
|
||
| // Validate StrictId format - if not, try email lookup | ||
| Id<User>? parsedUserId = null; | ||
| if (!Id<User>.TryParse(userId, out var tmpId)) | ||
| { | ||
| // Try email lookup (for external auth providers like Auth0) | ||
| logger.LogInformation("Profile GET: userId {UserId} is not a StrictId, attempting email lookup", userId); | ||
| logger.LogInformation("Profile GET: userId {UserId} is not a StrictId, attempting email lookup", sanitizedUserId); | ||
| var userByEmail = await db.Users | ||
| .AsNoTracking() | ||
| .FirstOrDefaultAsync(u => u.Email == userId); | ||
|
|
||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile GET: No user found for userId/email {UserId}", userId); | ||
| logger.LogWarning("Profile GET: No user found for userId/email {UserId}", sanitizedUserId); | ||
| return Results.NotFound(); | ||
| } | ||
|
|
||
| @@ -226,7 +221,7 @@ | ||
| return Results.NotFound(); | ||
| } | ||
|
|
||
| logger.LogInformation("Profile GET: Found user {Id} for userId {UserId}", user.Id, userId); | ||
| logger.LogInformation("Profile GET: Found user {Id} for userId {UserId}", user.Id, sanitizedUserId); | ||
|
|
||
| // T037: Return full User for ProfileEdit component | ||
| return Results.Ok(user); |
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile PUT: No registrant found for userId/email {UserId}", userId); | ||
| logger.LogWarning("Profile PUT: No user found for userId/email {UserId}", userId); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix log-forging issues you should normalize or encode user-controlled values before logging them. For plain-text logs, at minimum strip newline and carriage-return characters (and optionally other control characters) from user input before passing it to the logger. For logs that are later rendered as HTML, HTML-encode user input before logging.
For this specific case, the best fix with minimal functional impact is to sanitize the userId value once at the start of the PUT handler and then log only the sanitized version. We can create a local variable, e.g. safeUserId, that replaces \r and \n with empty strings (or spaces) and use safeUserId in all log statements within that handler instead of the raw userId. This preserves all existing behavior of the endpoint (database lookups, responses, etc.) while ensuring that log messages cannot be split or forged using embedded line breaks. We only change the provided code region in ProfileApi.cs, and we do not need any new imports or helpers; simple string.Replace calls are sufficient.
Concretely:
- In the PUT handler (
group.MapPut("/{userId}", ...)), introducevar safeUserId = userId.Replace("\r", string.Empty).Replace("\n", string.Empty);near the top. - Replace uses of
userIdin logging calls within that handler (LogInformationat line 242 andLogWarningat line 248, and any other log calls that referenceuserIdin the omitted section if present) withsafeUserId. - No other logic (lookups using
userIdagainst the database, etc.) is modified.
| @@ -235,17 +235,22 @@ | ||
| // T037: Update full profile by user ID | ||
| group.MapPut("/{userId}", async (string userId, UserDB db, User updatedUser, ILogger<UserDB> logger) => | ||
| { | ||
| // Normalize user-supplied ID for safe logging (prevent log forging via newlines) | ||
| var safeUserId = userId | ||
| .Replace("\r", string.Empty) | ||
| .Replace("\n", string.Empty); | ||
|
|
||
| // Validate StrictId format - if not, try email lookup | ||
| Id<User>? parsedUserId = null; | ||
| if (!Id<User>.TryParse(userId, out var tmpId)) | ||
| { | ||
| logger.LogInformation("Profile PUT: userId {UserId} is not a StrictId, attempting email lookup", userId); | ||
| logger.LogInformation("Profile PUT: userId {UserId} is not a StrictId, attempting email lookup", safeUserId); | ||
| var userByEmail = await db.Users | ||
| .FirstOrDefaultAsync(u => u.Email == userId); | ||
|
|
||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile PUT: No user found for userId/email {UserId}", userId); | ||
| logger.LogWarning("Profile PUT: No user found for userId/email {UserId}", safeUserId); | ||
| return Results.NotFound(); | ||
| } | ||
|
|
| @@ -914,7 +911,7 @@ | |||
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, the fix is to avoid writing private data (here, the user’s email address) to external locations such as logs. Instead, log either non-sensitive identifiers (e.g., internal user ID) or a redacted/hashed form of the email, or omit it entirely if not needed.
For this specific snippet in Visage.Services.UserProfile/ProfileApi.cs, the best minimal fix is to change the log message at line 837 so that it no longer includes the raw email value. The rest of the logic (looking up the user by email, error handling, etc.) can stay unchanged. Given that we already have a clear context (“Draft deletion failed” and we know it’s within the email-lookup branch), removing the {Email} placeholder and the email argument is sufficient and does not change application behavior from a client’s perspective. No new imports or methods are required for this minimal fix.
Concretely:
- In
ProfileApi.cs, find the warning log inside theDELETE /api/profile/draft/{section}handler whereuserByEmailis null. - Replace
logger.LogWarning("Draft deletion failed: No user found for email {Email}", email);with a message that does not log the email, e.g.logger.LogWarning("Draft deletion failed: No user found for provided email");. - Leave all other uses of
email(for lookup) untouched.
| @@ -834,7 +834,7 @@ | ||
| var userByEmail = await db.Users.FirstOrDefaultAsync(u => u.Email == email); | ||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Draft deletion failed: No user found for email {Email}", email); | ||
| logger.LogWarning("Draft deletion failed: No user found for provided email"); | ||
| return Results.NotFound(new { error = "User profile not found" }); | ||
| } | ||
|
|
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("AIDE banner status check failed: No registrant found for email {Email}", email); | ||
| logger.LogWarning("AIDE banner status check failed: No user found for email {Email}", email); |
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this kind of issue you should avoid writing raw private information (like email addresses) into logs or other external storage. Instead, either remove such data from the log entirely or replace it with a redacted or transformed value that remains useful for debugging (for example, hashing, partial masking, or an internal surrogate ID).
For this specific code, the problem is the log message at line 568:
logger.LogWarning("AIDE banner status check failed: No user found for email {Email}", email);The best fix, while preserving functionality, is to avoid logging the full email. A common pattern is to log a deterministic but non-reversible representation, such as a SHA‑256 hash of the email, so that operators can still correlate repeated issues for the same email without seeing the raw value. Because we should not assume project‑specific helpers, we can implement a small local helper method in this file that computes a SHA‑256 hash and then change the log statement to use that hashed value instead of the plain email. We’ll also add the necessary using System.Security.Cryptography; and using System.Text; imports at the top of ProfileApi.cs.
Concretely:
-
Add
using System.Security.Cryptography;andusing System.Text;alongside the otherusingdirectives inProfileApi.cs. -
Introduce a private static helper method in
ProfileApi(within the shown file) such as:private static string HashEmail(string email) { using var sha256 = SHA256.Create(); var bytes = Encoding.UTF8.GetBytes(email); var hash = sha256.ComputeHash(bytes); return Convert.ToHexString(hash); }
-
Replace the logging call at line 568 to avoid logging
emaildirectly and instead logHashEmail(email)and adjust the message template accordingly, for example:logger.LogWarning( "AIDE banner status check failed: No user found for email hash {EmailHash}", HashEmail(email));
This change removes direct exposure of the private email address in logs while maintaining the ability to diagnose repeated failures for a given user.
| @@ -3,6 +3,8 @@ | ||
| using Microsoft.Extensions.Logging; | ||
| using System.Diagnostics; | ||
| using System.Security.Claims; | ||
| using System.Security.Cryptography; | ||
| using System.Text; | ||
| using StrictId; | ||
| using Visage.Shared.Models; | ||
| using Visage.Services.UserProfile.Repositories; | ||
| @@ -11,6 +13,19 @@ | ||
|
|
||
| public static class ProfileApi | ||
| { | ||
| private static string HashEmail(string email) | ||
| { | ||
| if (string.IsNullOrEmpty(email)) | ||
| { | ||
| return string.Empty; | ||
| } | ||
|
|
||
| using var sha256 = SHA256.Create(); | ||
| var bytes = Encoding.UTF8.GetBytes(email); | ||
| var hash = sha256.ComputeHash(bytes); | ||
| return Convert.ToHexString(hash); | ||
| } | ||
|
|
||
| private static readonly string[] EmailClaimTypes = | ||
| [ | ||
| "email", | ||
| @@ -565,7 +580,9 @@ | ||
| .FirstOrDefaultAsync(); | ||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("AIDE banner status check failed: No user found for email {Email}", email); | ||
| logger.LogWarning( | ||
| "AIDE banner status check failed: No user found for email hash {EmailHash}", | ||
| HashEmail(email)); | ||
| return Results.NotFound(new { error = "User profile not found" }); | ||
| } | ||
|
|
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("AIDE banner dismissal failed: No registrant found for email {Email}", email); | ||
| logger.LogWarning("AIDE banner dismissal failed: No user found for email {Email}", email); |
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, the fix is to avoid writing private data (the full email address) directly to logs and instead either omit it, redact it, or log only a non-reversible or less-sensitive representation (for example, a hash or partially masked value). This preserves diagnostic value while reducing the risk that sensitive information is exposed in log files.
For this specific code, the problematic sink is the logger.LogWarning call at line 502:
logger.LogWarning("AIDE banner dismissal failed: No user found for email {Email}", email);We can fix this by replacing the logged email with a redacted form that does not reveal the full address. A concise approach that requires no new dependencies is:
- Introduce a small local helper function within the lambda to redact the email (for example, keep only the domain or partially mask the local part).
- Use that helper to compute a
redactedEmailstring and log that instead of the fullemail.
Because we are constrained to edits within the shown snippet in ProfileApi.cs, the helper must be defined inside the handler delegate so we don't touch other parts of the file. The change is localized around the existing logging call: we add the helper and a var redactedEmail = RedactEmail(email); before logging, and change the log call to use redactedEmail. No new imports are needed, as we only use basic string operations.
Concretely:
- In
Visage.Services.UserProfile/ProfileApi.cs, inside the POST/preferences/aide-banner/dismisshandler, locate theif (userByEmail is null)block around lines 500–503. - Insert a small
RedactEmaillocal function just above thisifblock. - Replace the existing
logger.LogWarning(..., email);with a version that callsRedactEmail(email)and logs that result.
This preserves existing functionality (same control flow, same conditions and responses) while addressing the exposure of private information in logs.
| @@ -499,7 +499,27 @@ | ||
| var userByEmail = await db.Users.FirstOrDefaultAsync(u => u.Email == email); | ||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("AIDE banner dismissal failed: No user found for email {Email}", email); | ||
| // Log a redacted form of the email to avoid exposing full private information in logs | ||
| static string RedactEmail(string? value) | ||
| { | ||
| if (string.IsNullOrEmpty(value)) | ||
| { | ||
| return "<unknown>"; | ||
| } | ||
|
|
||
| var atIndex = value.IndexOf('@'); | ||
| if (atIndex <= 1) | ||
| { | ||
| // If there's no clear local-part to redact, return only the domain or a generic value | ||
| return "<redacted>"; | ||
| } | ||
|
|
||
| var domain = value[(atIndex + 1)..]; | ||
| return $"<redacted>@{domain}"; | ||
| } | ||
|
|
||
| var redactedEmail = RedactEmail(email); | ||
| logger.LogWarning("AIDE banner dismissal failed: No user found for email {Email}", redactedEmail); | ||
| return Results.NotFound(new { error = "User profile not found" }); | ||
| } | ||
|
|
| if (user is null) | ||
| { | ||
| logger.LogWarning("Profile PUT: Registrant not found for ID {UserId}", parsedUserId); | ||
| logger.LogWarning("Profile PUT: User not found for ID {UserId}", parsedUserId); |
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this issue we should avoid writing sensitive identifiers that are derived from user input (such as database user IDs obtained via an email lookup) directly to logs or other external locations. Instead, logs should contain non‑sensitive context (such as that a user was not found) and, if necessary, a non‑sensitive correlation token or a sufficiently redacted version of the value.
For this specific case in Visage.Services.UserProfile/ProfileApi.cs, the best fix that doesn’t change functionality is to adjust the warning log at line 271 so it no longer includes parsedUserId as a structured log parameter. The message can still report that the user was not found, and we can optionally log whether the original input was a StrictId or an email, but we should not log the resolved internal user ID coming from userByEmail.Id. This means changing:
logger.LogWarning("Profile PUT: User not found for ID {UserId}", parsedUserId);to a version without the sensitive parameter, for example:
logger.LogWarning("Profile PUT: User not found for provided identifier.");This keeps behavior the same from the client’s perspective while eliminating the exposure. No new methods or imports are required; we only need to update this logging call within the shown snippet of ProfileApi.cs.
| @@ -268,7 +268,7 @@ | ||
|
|
||
| if (user is null) | ||
| { | ||
| logger.LogWarning("Profile PUT: User not found for ID {UserId}", parsedUserId); | ||
| logger.LogWarning("Profile PUT: User not found for provided identifier."); | ||
| return Results.NotFound(); | ||
| } | ||
|
|
| if (user is null) | ||
| { | ||
| logger.LogWarning("Profile GET: Registrant not found for ID {UserId}", parsedUserId); | ||
| logger.LogWarning("Profile GET: User not found for ID {UserId}", parsedUserId); |
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this kind of issue you should avoid writing sensitive or potentially sensitive data (like user IDs, emails, tokens, or other PII) directly to logs or other external locations. Instead, log non-sensitive context (for example, that the user could not be found) or use anonymized/hashed values if correlation is needed, making sure that the transformation is one-way and doesn’t reintroduce sensitivity.
For this specific snippet, the minimal and safest change while preserving functionality is to adjust the warning log at line 225 so that it no longer includes parsedUserId (which can originate from userByEmail.Id). The application behavior toward the client does not change: it still returns Results.NotFound() when the user is not found. We only change the log message to remove the potentially sensitive identifier parameter. No new imports or methods are needed; just update the logger.LogWarning call so that it logs a generic message without {UserId} or any sensitive argument.
Concretely, in Visage.Services.UserProfile/ProfileApi.cs, locate the block:
223: if (user is null)
224: {
225: logger.LogWarning("Profile GET: User not found for ID {UserId}", parsedUserId);
226: return Results.NotFound();
227: }and change it so that the warning no longer mentions the ID or uses parsedUserId as an argument, e.g.:
223: if (user is null)
224: {
225: logger.LogWarning("Profile GET: User not found for requested identifier");
226: return Results.NotFound();
227: }This removes the tainted data from the sink while keeping useful diagnostic information.
| @@ -222,7 +222,7 @@ | ||
|
|
||
| if (user is null) | ||
| { | ||
| logger.LogWarning("Profile GET: User not found for ID {UserId}", parsedUserId); | ||
| logger.LogWarning("Profile GET: User not found for requested identifier"); | ||
| return Results.NotFound(); | ||
| } | ||
|
|
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile completion check failed: No registrant found for email {Email}", email); | ||
| logger.LogWarning("Profile completion check failed: No user found for email {Email}", email); |
Check warning
Code scanning / CodeQL
Exposure of private information Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this kind of issue you should avoid logging or otherwise writing sensitive data (like email addresses) directly to external sinks. Instead, log non-sensitive identifiers, or log a redacted/hashed form that is useful for correlation without exposing the raw value.
The best targeted fix here is to change the log call at line 137 so that it no longer writes the raw email value. We should either (a) remove the email from the message entirely, or (b) log only a safe, non-reversible representation like a hash or a heavily redacted version. To preserve functionality (diagnostic value) while satisfying the analysis, a common pattern is to log a short, non-reversible hash of the email. That retains the ability to correlate repeated events for the same email without revealing it. We can compute such a hash locally using a well-known standard library (System.Security.Cryptography) and log that hash instead of the email itself.
Concretely, within Visage.Services.UserProfile/ProfileApi.cs and within the shown snippet, we will:
- Add
using System.Security.Cryptography;at the top of the file so that we can use SHA-256. - Add a small private helper method inside
ProfileApi(since it is astaticclass) to compute a hex-encoded SHA-256 hash of a string, e.g.private static string HashForLogging(string value). - Change the log statement at line 137 from logging
{Email}andemailto logging something like{EmailHash}withHashForLogging(email). The rest of the control flow and responses remain unchanged.
This keeps the behavior of the endpoint the same from a client perspective, avoids logging raw private information, and addresses all three variants of the alert since they concern this same logging of email.
| @@ -3,6 +3,7 @@ | ||
| using Microsoft.Extensions.Logging; | ||
| using System.Diagnostics; | ||
| using System.Security.Claims; | ||
| using System.Security.Cryptography; | ||
| using StrictId; | ||
| using Visage.Shared.Models; | ||
| using Visage.Services.UserProfile.Repositories; | ||
| @@ -11,6 +12,19 @@ | ||
|
|
||
| public static class ProfileApi | ||
| { | ||
| private static string HashForLogging(string? value) | ||
| { | ||
| if (string.IsNullOrEmpty(value)) | ||
| { | ||
| return "<null-or-empty>"; | ||
| } | ||
|
|
||
| // Non-reversible hash to allow correlation without exposing the raw value | ||
| var bytes = System.Text.Encoding.UTF8.GetBytes(value); | ||
| var hashBytes = SHA256.HashData(bytes); | ||
| return Convert.ToHexString(hashBytes); | ||
| } | ||
|
|
||
| private static readonly string[] EmailClaimTypes = | ||
| [ | ||
| "email", | ||
| @@ -134,7 +148,7 @@ | ||
| .FirstOrDefaultAsync(); | ||
| if (userByEmail is null) | ||
| { | ||
| logger.LogWarning("Profile completion check failed: No user found for email {Email}", email); | ||
| logger.LogWarning("Profile completion check failed: No user found for email hash {EmailHash}", HashForLogging(email)); | ||
| return Results.NotFound(new { error = "User profile not found" }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new UserProfile microservice to separate user profile management from event registration, enhancing modularity and scalability. The refactoring renames the core entity from Registrant to User, updates all related namespaces from Visage.Services.Registration to Visage.Services.UserProfile, and adds support for multiple event registrations per user with check-in/check-out functionality.
Changes:
- Introduced UserProfile microservice with User entity replacing Registrant
- Added EventRegistration and SessionCheckIn models for event management
- Updated all test files to reference the new UserProfile service and User model
- Added new event registration and check-in endpoints to the Eventing service
- Migrated database schema with proper foreign key relationships
Reviewed changes
Copilot reviewed 74 out of 88 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Visage.Test.Aspire/packages.lock.json | Updated project references from registrations to userprofile |
| tests/Visage.Test.Aspire/Visage.Test.Aspire.csproj | Changed project reference to UserProfile service, added EF Core SQL Server package |
| tests/Visage.Test.Aspire/SocialProfileLinkingTests.cs | Updated namespaces and model references from Registrant to User |
| tests/Visage.Test.Aspire/SocialProfileDisconnectTests.cs | Updated namespaces and model references from Registrant to User |
| tests/Visage.Test.Aspire/RegistrationDbTests.cs | Updated test cases to use User model and /api/users endpoints |
| tests/Visage.Test.Aspire/RegistrantCurationSocialFieldsTests.cs | Updated to use UserDB and User model |
| tests/Visage.Test.Aspire/README.md | Updated documentation for TUnit test filtering syntax |
| tests/Visage.Test.Aspire/QUICKSTART.md | Updated to use TUnit filtering syntax |
| tests/Visage.Test.Aspire/ProfileDraftPersistenceTests.cs | Added AuthRequired attribute and guard checks |
| tests/Visage.Test.Aspire/DraftSaveTests.cs | Refactored to use User model and helper method for user creation |
| tests/Visage.Test.Aspire/DraftRetrievalTests.cs | Updated to use User model and route-based draft endpoints |
| tests/Visage.Test.Aspire/DraftDeletionTests.cs | Updated to use User model and route-based draft endpoints |
| tests/Visage.Test.Aspire/AuthTestGuard.cs | New helper class for checking Auth0 test configuration |
| tests/Visage.Test.Aspire/AuthRequiredAttribute.cs | New attribute for skipping tests when Auth0 is not configured |
| tests/TEST-BASELINE.md | Updated test commands to use TUnit syntax |
| services/Visage.Services.Eventing/Program.cs | Added event registration and check-in/check-out endpoints |
| services/Visage.Services.Eventing/EventDB.cs | Added EventRegistration and SessionCheckIn entity configurations |
| Visage.Shared/Models/UserPreferences.cs | Changed UserId type from Id to Id |
| Visage.Shared/Models/User.cs | New User model replacing Registrant with enhanced fields |
| Visage.Shared/Models/SocialVerificationEvent.cs | Changed RegistrantId to UserId |
| Visage.Shared/Models/SessionCheckIn.cs | New model for session check-in/check-out tracking |
| Visage.Shared/Models/RegisterEventRequest.cs | New DTO for event registration requests |
| Visage.Shared/Models/EventRegistration.cs | New model for event registrations with status management |
| Visage.Shared/Models/DraftRegistration.cs | Changed UserId type from Id to Id |
| Visage.Shared/Models/CheckInDtos.cs | New DTOs for check-in/check-out operations |
| Visage.Services.UserProfile/* | New UserProfile service with migrated code from Registrations |
| Visage.Services.Registrations/* | Legacy files removed |
| Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/ProfileEdit.razor | Updated to use User model |
Files not reviewed (11)
- Visage.Services.Registrations/Migrations/20251028113709_AddProfileCompletionTracking.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251028113908_AddDraftRegistrationTable.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251028114001_AddUserPreferencesTable.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251031120804_AddGovtIdTypeColumn.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251107103802_AddAideFields.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251107112358_UpdateDraftRegistrationFields.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251211091002_AddSocialOAuthVerificationFields.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251213100620_AddSocialVerificationAuditAndUniqueIndexes.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251219132940_AddLinkedInSubject.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251222120322_AddLinkedInSubjectIndex.Designer.cs: Language not supported
- Visage.Services.Registrations/Migrations/20251223125214_AddLinkedInRawPayloadFields.Designer.cs: Language not supported
| if (registration != null) | ||
| { | ||
| rsvpMessage = "You're registered — check 'My Registrations' for details."; | ||
| } | ||
| else | ||
| { | ||
| rsvpMessage = "Registration failed. Please try again."; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (registration != null) | |
| { | |
| rsvpMessage = "You're registered — check 'My Registrations' for details."; | |
| } | |
| else | |
| { | |
| rsvpMessage = "Registration failed. Please try again."; | |
| } | |
| rsvpMessage = registration != null | |
| ? "You're registered — check 'My Registrations' for details." | |
| : "Registration failed. Please try again."; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tests/Visage.Test.Aspire/RegistrationDbTests.cs (1)
18-35: Critical: Resource name mismatch will cause test failures.The test references
"registrations-api"but the AppHost defines the UserProfile service as"userprofile-api". This resource name doesn't exist in the Aspire configuration, so the test will fail at runtime when callingWaitForResourceAsync()andCreateHttpClient(). Additionally, the test method nameRegistration_Service_Should_Connect_To_Aspire_Managed_Database()conflicts with the actual service terminology (UserProfile). Both the method name and resource reference must be updated to"userprofile-api"to match the AppHost configuration.tests/Visage.Test.Aspire/RegistrantCurationSocialFieldsTests.cs (1)
52-54: Test references non-existent service resourceregistrations-api.The test waits for and creates an HTTP client for
"registrations-api", but per AppHost.cs changes in this PR, the service is now registered as"userprofile-api". This test will fail at runtime.🐛 Proposed fix
- await TestAppContext.WaitForResourceAsync("registrations-api", KnownResourceStates.Running, TimeSpan.FromSeconds(90)); + await TestAppContext.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running, TimeSpan.FromSeconds(90)); - var httpClient = TestAppContext.CreateHttpClient("registrations-api"); + var httpClient = TestAppContext.CreateHttpClient("userprofile-api");Visage.Services.UserProfile/ProfileApi.cs (1)
35-77: Remove or guard DEBUG logging of JWT tokens and Authorization headers.Logging raw Authorization headers and decoded JWT payloads (even truncated) in production can expose sensitive information including user identities, permissions, and potentially access tokens. This should either be removed or strictly guarded behind a debug flag that is disabled in production.
♻️ Suggested approach
Either remove this debugging block entirely or wrap it with an explicit debug-level check:
+#if DEBUG // DEBUG: Log raw Authorization header and attempt to decode JWT payload try { // ... existing debug code ... } catch (Exception ex) { logger.LogWarning(ex, "DEBUG: Error while logging Authorization header"); } +#endifVisage.FrontEnd/Visage.FrontEnd.Shared/Pages/ProfileEdit.razor (1)
701-710: Add cancellation support to preventStateHasChangedon disposed component.The
Task.Runpattern with a 3-second delay may execute after the component is disposed, potentially causing issues. Consider using aCancellationTokenSourcethat is cancelled inDispose().Proposed fix
Add a field:
private CancellationTokenSource? _cts = new();Update the delayed hide logic:
- _ = Task.Run(async () => - { - await Task.Delay(3000); - await InvokeAsync(() => - { - _showDraftSaved = false; - Logger.LogInformation("ProfileEdit: Hiding draft saved indicator"); - StateHasChanged(); - }); - }); + _ = Task.Run(async () => + { + try + { + await Task.Delay(3000, _cts?.Token ?? CancellationToken.None); + await InvokeAsync(() => + { + _showDraftSaved = false; + Logger.LogInformation("ProfileEdit: Hiding draft saved indicator"); + StateHasChanged(); + }); + } + catch (TaskCanceledException) { /* Component disposed */ } + });Update
Dispose():public void Dispose() { + _cts?.Cancel(); + _cts?.Dispose(); _autoSaveTimer?.Dispose(); Logger.LogInformation("ProfileEdit: Auto-save timer disposed"); }Visage.FrontEnd/Visage.FrontEnd.Shared/Services/EventService.cs (1)
109-124: UploadCoverPicture can return null but method signature doesn't indicate it.Line 121-123:
GetString()can return null if the property is JSON null. The method return typestring(non-nullable) is inconsistent with this possibility.Proposed fix
- public async Task<string> UploadCoverPicture(IBrowserFile file) + public async Task<string?> UploadCoverPicture(IBrowserFile file) { // ... existing code ... var url = jsonResponse.RootElement.GetProperty("url").GetString(); - return url; + return url; // Caller should handle null case }tests/Visage.Test.Aspire/SocialProfileLinkingTests.cs (1)
119-122: Update resource name from "registrations-api" to "userprofile-api" after UserProfile service refactor.Lines 120 and 122 wait for the wrong resource name. Change:
- Line 120:
await TestAppContext.WaitForResourceAsync("registrations-api", ...)- Line 122:
TestAppContext.CreateHttpClient("registrations-api")to use
"userprofile-api"instead. Also updateHealthEndpointTests.csline 170 to replace"registrations-api"with"userprofile-api"in theresourceNamesarray.Visage.FrontEnd/Visage.FrontEnd.Shared/Components/MandatoryRegistration.razor.cs (1)
391-453: Preserve server-owned timestamps/verification metadata in payload.The new
Usermodel includesLinkedInVerifiedAt,GitHubVerifiedAt,CreatedAt, andUpdatedAt, but the payload mapping doesn’t carry them. That can reset timestamps to defaults/null on updates. Copy them fromregistrant(or explicitly exclude them from the payload if server-owned).Suggested fix
payload.IsLinkedInVerified = registrant.IsLinkedInVerified; payload.IsGitHubVerified = registrant.IsGitHubVerified; + payload.LinkedInVerifiedAt = registrant.LinkedInVerifiedAt; + payload.GitHubVerifiedAt = registrant.GitHubVerifiedAt; payload.IsProfileComplete = true; payload.ProfileCompletedAt = DateTime.UtcNow; payload.IsAideProfileComplete = registrant.IsAideProfileComplete; payload.AideProfileCompletedAt = registrant.AideProfileCompletedAt; + payload.CreatedAt = registrant.CreatedAt; + payload.UpdatedAt = registrant.UpdatedAt;
🤖 Fix all issues with AI agents
In `@services/Visage.Services.Eventing/Program.cs`:
- Around line 403-418: The LookupByPin endpoint (LookupByPin) currently returns
the full EventRegistration record (from EventDB.EventRegistrations queried by
CheckInPin) to any caller; update it to either enforce
authentication/authorization at the start of the method (e.g., validate the
caller's identity/roles and return 401/403 when unauthorized) or map the found
EventRegistration to a limited DTO containing only non-sensitive fields (e.g.,
registration Id and status) before returning via TypedResults.Ok; ensure you
reference LookupByPin, EventRegistration, EventDB, CheckInPin and replace the
returned payload accordingly (or short-circuit with an unauthorized result) so
sensitive fields are never exposed to unauthenticated callers.
- Around line 290-316: ApproveRegistration currently allows any authenticated
user to approve; add an admin-role check at the start of the ApproveRegistration
method (before performing any DB actions). Inspect http.User for the VisageAdmin
role (e.g., http.User.IsInRole("VisageAdmin") or check ClaimTypes.Role / "roles"
claim for the value "VisageAdmin"); if the user is not authenticated return
TypedResults.Unauthorized(), and if authenticated but missing the admin role
return TypedResults.Forbid(). Update ApproveRegistration to perform this role
check and short-circuit with the appropriate TypedResults when the check fails.
- Around line 422-426: GenerateCheckInPin currently uses System.Random which is
not cryptographically secure; replace its usage with
System.Security.Cryptography.RandomNumberGenerator (or
RandomNumberGenerator.GetInt32) to produce an unpredictable 4-digit PIN.
Specifically, in GenerateCheckInPin() remove new Random(), call
RandomNumberGenerator.GetInt32(1000, 10000) (or use RandomNumberGenerator to
fill bytes and map to the 1000–9999 range), convert the resulting int to string,
and add the System.Security.Cryptography using/import as needed.
In `@tests/Visage.Test.Aspire/DraftSaveTests.cs`:
- Around line 60-62: Replace the incorrect resource name "registrations-api"
with "userprofile-api" in this test file for all uses of
TestAppContext.WaitForResourceAsync and TestAppContext.CreateHttpClient (search
for the literal string "registrations-api" around the methods
WaitForResourceAsync and CreateHttpClient calls); update each invocation so
WaitForResourceAsync("userprofile-api", ...) and
CreateHttpClient("userprofile-api") are used to match the service registered in
AppHost (UserProfile service).
In `@tests/Visage.Test.Aspire/QUICKSTART.md`:
- Around line 66-80: The doc examples call dotnet test with TUnit's
--treenode-filter but omit the required separator; update each example that uses
the command starting with "dotnet test --project
tests\Visage.Test.Aspire\Visage.Test.Aspire.csproj" and passing
"--treenode-filter" (the E2E, DraftPersistence, Smoke examples and the
additional example later) to insert a lone "--" between the dotnet options and
the TUnit args so they read: dotnet test --project ...csproj --
--treenode-filter "/*/*/*/*[Category=...]" for each occurrence.
In `@Visage.FrontEnd/Visage.FrontEnd.Shared/Models/EventRegistration.cs`:
- Around line 64-86: The frontend RegistrationStatus enum is inconsistent with
the backend EventRegistration enum; replace or align the frontend
RegistrationStatus entries and integer values to match the backend's enum
members (Pending=0, Approved=1, Rejected=2, Waitlisted=3, Cancelled=4) so JSON
serialization/deserialization preserves semantics; update the enum declaration
named RegistrationStatus in Visage.FrontEnd.Shared.Models.EventRegistration to
mirror the backend names and exact numeric values (remove or map
CheckedIn/Attended/NoShow/Registered/Confirmed variants) and run tests to ensure
API payloads round-trip correctly.
In `@Visage.FrontEnd/Visage.FrontEnd.Shared/Models/User.cs`:
- Around line 51-60: Remove the [Required] attribute from the GovtId property in
the User model so the frontend no longer mandates full government ID collection;
leave the GovtIdLast4Digits and GovtIdType properties' attributes unchanged
(GovtIdLast4Digits remains [Required] and [StringLength(4, MinimumLength = 4)]).
Locate the GovtId property in the User class (GovtId { get; set; } =
string.Empty) and delete only the [Required] annotation, and optionally update
the inline comment to reflect that full GovtId is not required/stored to match
backend PII-minimization behavior.
In `@Visage.FrontEnd/Visage.FrontEnd.Web/Program.cs`:
- Around line 105-142: HealthEndpointTests references a non-registered resource
("registrations-api") causing integration failures; update HealthEndpointTests
to remove "registrations-api" from the resource list and add tests for
"userprofile-api" checking both /health and /alive endpoints. Specifically, in
HealthEndpointTests (the test class that builds the resource array and contains
the existing /health and /alive test patterns), replace any occurrence of
"registrations-api" with "userprofile-api" (or remove the former and append
"userprofile-api") and add two test cases following the existing TUnit +
FluentAssertions style that create an HttpClient for "userprofile-api" (use the
same pattern used for other resources), call "/health" and "/alive", and assert
successful responses and expected payloads the same way other resource checks
do. Ensure test names and assertions mirror the existing tests so they integrate
cleanly with the test harness.
In `@Visage.Services.UserProfile/Migrations/20260110145754_InitialUserProfile.cs`:
- Around line 37-105: The migration is missing the Auth0Subject column on the
Users table; update the migrationBuilder.CreateTable columns block (the same
block that defines Id, FirstName, Email, etc.) to include Auth0Subject as
table.Column<string>(type: "nvarchar(255)", maxLength: 255, nullable: false) so
it matches the User model's [StringLength(255)] Auth0Subject property and avoids
runtime persistence errors.
- Around line 14-35: Remove the creation of the "Event" table from this
migration (delete the migrationBuilder.CreateTable call that defines table
"Event") so the UserProfile DB does not own Event rows; instead keep only the
EventId column in EventRegistrations as a simple string reference (do not add a
foreign key or cascade delete). In the EventRegistrations table definition, add
the missing required Auth0Subject column as nvarchar(255) (non-nullable) to
match Visage.Shared.Models.EventRegistration and the Eventing service's EventDB
indexes, and ensure any indexes or constraints on Auth0Subject mirror the
Eventing service configuration (but do not introduce cross-service FKs).
In `@Visage.Services.UserProfile/ProfileApi.cs`:
- Around line 453-456: The code incorrectly assigns UserProfileDto.Name (a full
name) to user.FirstName; update the update handler (the method in ProfileApi
where user.FirstName = dto.Name is set) to parse dto.Name into first and last
names (split into at most two parts, e.g., by the first space), assign
user.FirstName and user.LastName accordingly, and fall back to existing
user.FirstName/user.LastName when parts are missing or dto.Name is null/empty;
alternatively, if your API should require separate fields, change the DTO and
assignment to use dto.FirstName and dto.LastName instead.
In `@Visage.Services.UserProfile/Program.cs`:
- Around line 93-107: The startup code for applying EF migrations uses
UserDB.Database.EnsureCreatedAsync() which bypasses migrations; replace this
with calling UserDB.Database.MigrateAsync() inside the existing try/catch so
pending migrations in Visage.Services.UserProfile/Migrations are applied on
startup. Locate the block that creates a scope and resolves UserDB (variable
userDb) and swap EnsureCreatedAsync() for MigrateAsync(), keeping the logging
and exception handling intact.
- Around line 229-233: The three open endpoints—app.MapGet("/api/users", ...),
app.MapGet("/api/users/{userId}/registrations", ...), and the Map... for
"/register"—must require authentication: update each route mapping to append
.RequireAuthorization() to the returned IEndpointConventionBuilder so only
authenticated callers can access them; while editing the handlers (User list and
Registration list functions) also add optional pagination parameters (skip/take
or page/size) to limit returned rows and wire them into the EF queries (e.g.,
Users.ToListAsync with Skip/Take or Registrations.Where(...).Skip/Take) to
prevent full-table dumps.
- Around line 82-86: Remove the explicit inclusion of the Authorization header
from HTTP logging: in the AddHttpLogging configuration
(builder.Services.AddHttpLogging) delete the RequestHeaders.Add("Authorization")
call so bearer tokens are not captured, and optionally replace
HttpLoggingFields.All with a narrower set of fields that you actually need
(e.g., exclude RequestHeaders) to avoid logging sensitive data.
- Around line 40-61: The OnTokenValidated handler currently logs all JWT claims
unconditionally via logger?.LogDebug, risking PII exposure; change this to only
emit detailed claim logs when running in a non-production environment (e.g.,
check IHostEnvironment.IsDevelopment or a configuration flag resolved from
ctx.HttpContext.RequestServices) or restrict logging to a safe allowlist of
non-sensitive claim types and mask sensitive values (e.g., avoid/obscure
"email", "sub", "name", "role"); update the code in the
JwtBearerEvents.OnTokenValidated block to perform the environment/config check
before iterating ctx.Principal?.Claims and ensure sensitive claims are not
logged in cleartext.
- Around line 133-283: The POST /api/users handler (MapPost("/api/users")) and
POST /api/registrations handler (MapPost("/api/registrations")) currently trust
client-supplied Ids and accept unauthenticated changes; fix both by enforcing
authorization and deriving the user from the authenticated Auth0 subject: call
RequireAuthorization() on MapPost("/api/users"), MapPost("/api/registrations")
and MapGet("/api/users"); in the users upsert handler stop using inputUser.Id to
locate or authorize the record—use httpContext.User's Auth0 subject to query
db.Users.FirstOrDefaultAsync(u => u.Auth0Subject == auth0Subject) (or create a
new user if none), set inputUser.Auth0Subject, and only update that record; in
the registrations handler extract auth0Subject from httpContext.User, resolve
the corresponding user (db.Users.FirstOrDefaultAsync(u => u.Auth0Subject ==
auth0Subject)), reject if not found, then set registration.UserId =
resolvedUser.Id (ignore client-supplied registration.UserId) before creating the
registration and proceed with the same duplicate-checking logic.
In `@Visage.Shared/Models/EventRegistration.cs`:
- Around line 81-107: The backend RegistrationStatus enum in
SharedEventRegistration doesn't match the frontend EventRegistration enum
(Registered, Confirmed, CheckedIn, Attended, NoShow) causing UI mismatch; update
EventService to perform explicit mapping when deserializing API responses: add a
converter function (e.g., MapRegistrationStatus or
ConvertSharedToFrontendStatus) that translates RegistrationStatus values
(Pending, Approved, Rejected, Waitlisted, Cancelled) to the frontend enum
values, use that converter wherever SharedEventRegistration is deserialized, and
remove or deprecate the duplicate frontend model only if you choose to fully
unify on the backend enum; reference RegistrationStatus, SharedEventRegistration
and EventService to locate where to implement the conversion.
♻️ Duplicate comments (2)
tests/Visage.Test.Aspire/DraftDeletionTests.cs (1)
58-68: Resource name consistency check (duplicate).Same concern as earlier regarding
registrations-apivsuserprofile-api.tests/Visage.Test.Aspire/DraftRetrievalTests.cs (1)
20-49: Duplicate of CreateTestUserAsync validation concern.
🟡 Minor comments (6)
Visage.Shared/Models/EventRegistration.cs-46-48 (1)
46-48: Remove[StringLength(50)]from enum property.The
[StringLength]attribute is designed for string properties and has no effect on an enum. EF Core will handle enum storage based on configuration (as int or string via.HasConversion()). This attribute should be removed to avoid confusion.Proposed fix
/// <summary> /// Current status of the registration /// </summary> [Required] - [StringLength(50)] public RegistrationStatus Status { get; set; } = RegistrationStatus.Pending;Visage.FrontEnd/Visage.FrontEnd.Shared/Services/IProfileService.cs-26-34 (1)
26-34: Update XML docs (and param name) to reflectUser.The comments still reference “Registrant,” and the parameter name is now misleading.
✏️ Suggested fix
- /// <returns>Full Registrant profile or null if not found</returns> + /// <returns>Full User profile or null if not found</returns> @@ - /// <param name="registrant">Updated registrant data</param> + /// <param name="user">Updated user data</param> @@ - Task<bool> UpdateProfileAsync(User registrant); + Task<bool> UpdateProfileAsync(User user);Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/Home.razor-41-47 (1)
41-47: Differentiate RSVP success vs failure styling.
rsvpMessageis used for both success and error paths but always renders a success alert, which can mislead users.💡 Suggested adjustment
@@ - `@if` (!string.IsNullOrEmpty(rsvpMessage)) - { - <div class="alert alert-success mb-4"> - <span>@rsvpMessage</span> - </div> - } + `@if` (!string.IsNullOrEmpty(rsvpMessage)) + { + <div class="alert @(rsvpWasSuccessful ? "alert-success" : "alert-error") mb-4"> + <span>@rsvpMessage</span> + </div> + } @@ - private string? rsvpMessage; - private bool rsvpInProgress; + private string? rsvpMessage; + private bool rsvpWasSuccessful; + private bool rsvpInProgress; @@ - if (registration != null) - { - rsvpMessage = "You're registered — check 'My Registrations' for details."; - } - else - { - rsvpMessage = "Registration failed. Please try again."; - } + if (registration != null) + { + rsvpWasSuccessful = true; + rsvpMessage = "You're registered — check 'My Registrations' for details."; + } + else + { + rsvpWasSuccessful = false; + rsvpMessage = "Registration failed. Please try again."; + } @@ - rsvpMessage = "An error occurred while registering. Please try again."; + rsvpWasSuccessful = false; + rsvpMessage = "An error occurred while registering. Please try again.";Also applies to: 125-128, 281-288, 292-294
services/Visage.Services.Eventing/Program.cs-274-288 (1)
274-288: GetMyRegistrations silently returns empty list for unauthenticated users.This masks authentication failures and could confuse users who are unauthenticated. Consider returning 401 Unauthorized when the auth subject is missing instead of an empty list.
services/Visage.Services.Eventing/Program.cs-232-241 (1)
232-241: Return proper HTTP 401 Unauthorized instead of BadRequest.Returning "Unauthorized" with a 400 status code is semantically incorrect. Use
TypedResults.Unauthorized()or return aResults<UnauthorizedHttpResult, ...>union type for proper HTTP semantics.Proposed fix
-static async Task<Results<BadRequest<string>, Created<EventRegistration>>> RegisterForEvent( +static async Task<Results<UnauthorizedHttpResult, BadRequest<string>, Created<EventRegistration>>> RegisterForEvent( [FromBody] RegisterEventRequest request, EventDB db, HttpContext http) { var auth0Sub = http.User.FindFirst("sub")?.Value; if (string.IsNullOrEmpty(auth0Sub)) { - return TypedResults.BadRequest("Unauthorized: Auth0 subject not found"); + return TypedResults.Unauthorized(); }Visage.FrontEnd/Visage.FrontEnd.Shared/Services/EventService.cs-247-265 (1)
247-265: LookupRegistrationByPinAsync logs PIN in warning message.Line 257: Logging the PIN value could be a minor information disclosure if logs are accessible. Consider masking or omitting the PIN from logs.
Proposed fix
- logger?.LogWarning("LookupRegistrationByPinAsync failed: {Status} for pin {Pin}", response.StatusCode, pin); + logger.LogWarning("LookupRegistrationByPinAsync failed: {Status}", response.StatusCode);
🧹 Nitpick comments (27)
CONTRIBUTING.md (1)
67-111: Add repo-specific testing requirements to this section.
The new section is solid, but it doesn’t mention the project’s explicit expectations for integration/E2E coverage or health endpoint tests for new Aspire services. Consider adding a short note so contributors don’t miss those requirements. Based on learnings, ...✅ Proposed doc addition
@@ Visage uses **TUnit** for automated tests. + +> **Project requirements:** All new code in `tests/**` must be covered by integration or E2E tests, and any new Aspire service should add health endpoint tests in `tests/Visage.Test.Aspire/HealthEndpointTests.cs` for `/health` and `/alive`.tests/Visage.Test.Aspire/RegistrationDbTests.cs (2)
12-16: Class name inconsistent with UserProfile service terminology.The documentation correctly references "UserProfile service" but the class is still named
RegistrationDbTests. Consider renaming toUserProfileDbTestsorUserDbTestsfor consistency with the refactoring.♻️ Suggested rename
/// <summary> /// Integration tests for UserProfile service database migration to Aspire-managed SQL Server. /// Uses a single Aspire app started once per test assembly (see TestAssemblyHooks). /// </summary> -public class RegistrationDbTests +public class UserProfileDbTests
107-173: Test method name still uses legacy "Register" terminology.The test logic is comprehensive and correctly validates the upsert behavior. Consider renaming the method to align with User terminology.
♻️ Suggested rename
- public async Task RegisterEndpoint_WhenSameEmailPosted_ShouldUpdateExistingRecord() + public async Task UsersEndpoint_WhenSameEmailPosted_ShouldUpdateExistingRecord()tests/Visage.Test.Aspire/QUICKSTART.md (1)
26-26: Address markdown linting issues for better documentation quality.The static analysis tool flagged three minor markdown formatting issues:
- Lines 26 & 33: Using bold text instead of proper headings for "Option A" and "Option B"
- Line 60: Code block missing language specifier
📝 Proposed fixes
Fix heading structure for options:
-**Option A: Interactive Script (Recommended)** - +#### Option A: Interactive Script (Recommended) + ```powershell-**Option B: Manual Setup** - +#### Option B: Manual Setup + ```powershellAdd language specifier to code block:
Wait until you see: - -``` + +```text Now listening on: https://localhost:5001</details> Also applies to: 33-33, 60-62 </blockquote></details> <details> <summary>Visage.FrontEnd/Visage.FrontEnd.Shared/Models/Event.cs (1)</summary><blockquote> `44-53`: **Add length validation attributes for the new fields.** `Visage.Shared/Models/Event.cs` (lines 6-48) defines StringLength limits for Hashtag/Theme/CoverPicture; mirroring them here avoids client-side acceptance of values that the API will reject. <details> <summary>♻️ Suggested validation attributes</summary> ```diff // New properties + [StringLength(100, ErrorMessage = "Hashtag must be less than 100 characters")] public string? Hashtag { get; set; } + [StringLength(200, ErrorMessage = "Theme must be less than 200 characters")] public string? Theme { get; set; } // New property for cover picture URL + [StringLength(500, ErrorMessage = "Cover picture URL must be less than 500 characters")] public string? CoverPicture { get; set; }Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/ScheduleEvent.razor (1)
171-179: Prefer OnInitializedAsync for state setup in this component.Keeps initialization aligned with prerender/interactive lifecycles.
As per coding guidelines, please align lifecycle initialization with OnInitializedAsync.♻️ Suggested refactor
- protected override void OnInitialized() + protected override Task OnInitializedAsync() { newEvent ??= new EventDto() { StartDate = DateOnly.FromDateTime(DateTime.Now), StartTime = TimeOnly.FromDateTime(DateTime.Now), EndDate = DateOnly.FromDateTime(DateTime.Now), EndTime = TimeOnly.FromDateTime(DateTime.Now.AddHours(1)) }; editContext = new EditContext(newEvent); editContext.SetFieldCssClassProvider(new DaisyUIInputError()); editContext.OnValidationRequested += HandleValidationRequested; messageStore = new(editContext); // keep disabled true initially to avoid interactive controls during prerender + return Task.CompletedTask; }Visage.FrontEnd/Visage.FrontEnd.Shared/Services/RegistrationResult.cs (1)
10-14: Consider using a discriminated union pattern for better type safety.Using
object?forEntityloses compile-time type safety. Callers must know which property (UserorEventRegistration) to access, and accessing the wrong one returns null silently.A more type-safe alternative could use a discriminated union pattern or separate result types. However, the current approach is pragmatic and the factory methods (
Success(User ...)andSuccess(EventRegistration ...)) help enforce correct usage at creation time.💡 Optional: Type-safe alternative using a discriminated union
public abstract record RegistrationResult(bool IsSuccess, HttpStatusCode StatusCode, string? ErrorMessage) { public sealed record UserResult(User User, HttpStatusCode StatusCode) : RegistrationResult(true, StatusCode, null); public sealed record EventRegistrationResult(EventRegistration Registration, HttpStatusCode StatusCode) : RegistrationResult(true, StatusCode, null); public sealed record FailureResult(HttpStatusCode StatusCode, string? ErrorMessage) : RegistrationResult(false, StatusCode, ErrorMessage); }This allows pattern matching at call sites for exhaustive handling.
Visage.Services.UserProfile/ProfileApi.cs (1)
312-328: Consider extracting AIDE completion logic to avoid duplication.The AIDE completion check here mirrors what should exist in
ProfileCompletionRepository. Per the comment on line 309, this logic "should match ProfileCompletionRepository.IsAideComplete". Consider extracting to a shared method to ensure consistency and reduce maintenance burden.Visage.Shared/Models/EventRegistration.cs (1)
39-41: Consider settingRegisteredAtin the service layer instead of default initializer.Using
DateTime.UtcNowas a default initializer evaluates at object construction time, which may differ from the actual persistence time. For audit accuracy, consider setting this explicitly in the registration service/repository, or use EF SaveChanges interceptors. This is acceptable for typical flows but may cause inconsistencies in edge cases.Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/ProfileEdit.razor (2)
543-543: Renameregistrantvariable touserfor consistency with the new type.The variable is typed as
Visage.Shared.Models.User?but still namedregistrant. Per coding guidelines on descriptive variable names, renaming touser(orprofile/currentUser) would improve clarity and align with the Registrant → User migration throughout the codebase.
43-44: Consider addingOnInvalidSubmithandler for user feedback.Per coding guidelines, use both
OnValidSubmitfor successful submissions andOnInvalidSubmitfor user feedback on validation failures. This would allow displaying a toast or alert when the form is submitted with invalid data.-<EditForm Model="@registrant" OnValidSubmit="@HandleValidSubmit"> +<EditForm Model="@registrant" OnValidSubmit="@HandleValidSubmit" OnInvalidSubmit="@HandleInvalidSubmit">Add handler in
@codeblock:private void HandleInvalidSubmit() { Logger.LogWarning("ProfileEdit: Form submitted with validation errors"); // Optionally scroll to first error or show toast }Visage.FrontEnd/Visage.FrontEnd.Shared/Services/ProfileService.cs (2)
198-229: Rename local variable to matchUsersemantics.Keeps the refactor consistent and avoids “registrant” terminology post-migration.
♻️ Proposed rename
- var registrant = await response.Content.ReadFromJsonAsync<User>(); + var userProfile = await response.Content.ReadFromJsonAsync<User>(); _logger.LogInformation("GetProfileAsync: Profile loaded successfully"); - return registrant; + return userProfile;
257-279: Rename parameter touserfor clarity.The method signature and logs still use “registrant”.
♻️ Proposed rename
- public async Task<bool> UpdateProfileAsync(User registrant) + public async Task<bool> UpdateProfileAsync(User user) { try { - _logger.LogInformation("UpdateProfileAsync: Updating profile {Id}", registrant.Id); + _logger.LogInformation("UpdateProfileAsync: Updating profile {Id}", user.Id); - var response = await _httpClient.PutAsJsonAsync($"/api/profile/{registrant.Id}", registrant); + var response = await _httpClient.PutAsJsonAsync($"/api/profile/{user.Id}", user);Visage.Services.UserProfile/DesignTimeUserDbContextFactory.cs (1)
6-22: Ensure EF Core tooling can discover the design-time factory.Some EF Core tooling paths only pick up public factories; verify
dotnet effinds this type. If not, make it public.♻️ Proposed change
-internal sealed class DesignTimeUserDbContextFactory : IDesignTimeDbContextFactory<UserDB> +public sealed class DesignTimeUserDbContextFactory : IDesignTimeDbContextFactory<UserDB>Visage.FrontEnd/Visage.FrontEnd.Shared/Models/EventRegistration.cs (1)
38-53: Consider usingbool?instead ofstringfor FirstTimeAttendee.
FirstTimeAttendeeis typed asstringwithstring.Emptydefault, but semantically represents a yes/no question. Usingbool?or a dedicated enum would be more type-safe and reduce validation complexity.Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/UserRegistration.razor (3)
10-14: MissingOnInvalidSubmithandler for user feedback.Per coding guidelines, use
OnInvalidSubmitfor user feedback on validation failures. Currently, the form only usesOnValidSubmit.♻️ Suggested improvement
-<EditForm EditContext="editContext" FormName="NewRegistration" OnValidSubmit="HandleValidSubmit"> +<EditForm EditContext="editContext" FormName="NewRegistration" OnValidSubmit="HandleValidSubmit" OnInvalidSubmit="HandleInvalidSubmit">Add the handler in the
@codeblock:private void HandleInvalidSubmit() { // Optionally scroll to first error or show toast }Based on coding guidelines for
**/*.razor.
563-580: Wrap submit handler withisSubmittingstate for visual feedback.Per coding guidelines, wrap form submit handlers in try/catch with
isSubmittingstate to provide visual feedback during async operations. This prevents double-submissions and improves UX.♻️ Suggested improvement
+private bool isSubmitting = false; + private async Task HandleValidSubmit() { + if (isSubmitting) return; + isSubmitting = true; + try + { var registrationResult = await RegistrationService.CreateUserAsync(registrant!); // ... existing logic + } + finally + { + isSubmitting = false; + } }Also disable the submit button when submitting:
-<button type="submit" class="mt-4 sm:col-span-2 btn btn-primary">Register</button> +<button type="submit" class="mt-4 sm:col-span-2 btn btn-primary" disabled="@isSubmitting"> + @(isSubmitting ? "Registering..." : "Register") +</button>Based on coding guidelines for
**/*.razor.
380-392: Remove commented-out event-specific fields or extract to separate component.These commented blocks (FirstTimeAttendee, AreasOfInterest, VolunteerOpportunities) add noise. If they belong in an EventRegistration form, create that form and remove the comments here. Dead code increases cognitive load during maintenance.
Also applies to: 416-446
Visage.Services.UserProfile/Repositories/UserPreferencesRepository.cs (2)
11-18: Consider extracting an interface for the repository.Per coding guidelines, use Repository and Unit of Work patterns with EF Core. This repository lacks an interface (
IUserPreferencesRepository), which reduces testability and makes dependency injection less flexible.♻️ Suggested interface
public interface IUserPreferencesRepository { Task<UserPreferences?> GetByUserIdAsync(Id<User> userId); Task DismissAideBannerAsync(Id<User> userId); Task<bool> ShouldShowAideBannerAsync(Id<User> userId); }Based on learnings for
**/*.cs.
79-82: Consider extracting magic number 30 to a constant.The 30-day suppression period is hardcoded. Extracting this to a named constant improves readability and maintainability.
♻️ Suggested improvement
+private const int BannerSuppressionDays = 30; + public async Task<bool> ShouldShowAideBannerAsync(Id<User> userId) { // ... - return daysSinceDismissal >= 30; + return daysSinceDismissal >= BannerSuppressionDays; }tests/Visage.Test.Aspire/DraftSaveTests.cs (1)
227-246: Concurrent test validates upsert behavior but doesn't verify final state determinism.The test confirms all concurrent saves succeed, but doesn't verify which
SaveNumbervalue won (last-write-wins behavior). Consider adding an assertion on the finalSaveNumberto document expected behavior, or add a comment explaining that non-determinism is acceptable here.Visage.FrontEnd/Visage.FrontEnd.Shared/Services/IRegistrationService.cs (1)
22-23: Consider addingerror: trueto enforce deprecation at compile time.To prevent new usages of the legacy method, consider making the obsolete attribute a compile-time error:
♻️ Suggested improvement
- [Obsolete("Use CreateUserAsync instead")] + [Obsolete("Use CreateUserAsync instead", error: true)]This would force callers to migrate immediately rather than accumulating technical debt.
Visage.Services.UserProfile/Migrations/20260110145754_InitialUserProfile.cs (1)
42-94: Consider adding length constraints to frequently queried string columns.Many columns use
nvarchar(max)(e.g.,FirstName,LastName,City,State) which is inefficient for indexing and storage. Adding reasonable length constraints improves query performance and data integrity.♻️ Example constraints
Column Suggested Type FirstName nvarchar(100) LastName nvarchar(100) City nvarchar(100) State nvarchar(50) MobileNumber nvarchar(20) Visage.Services.UserProfile/UserDB.cs (1)
91-100: Consider adding one-to-one navigation property for UserPreferences.The current configuration uses
WithMany()which models a one-to-many relationship, butUserPreferencesappears to be one-to-one (keyed byUserId). Consider usingWithOne()for semantic clarity.Proposed fix
modelBuilder.Entity<UserPreferences>(entity => { entity.HasKey(u => u.UserId); entity.HasOne<User>() - .WithMany() + .WithOne() .HasForeignKey(u => u.UserId) .OnDelete(DeleteBehavior.Cascade); });tests/Visage.Test.Aspire/SocialProfileLinkingTests.cs (1)
26-55: Consider extracting shared test helpers to reduce duplication.
GetUserProfileDbConnectionString()andCreateUserDb()are duplicated acrossSocialProfileLinkingTests.csandSocialProfileDisconnectTests.cs(and potentially other test files per the snippets). Consider extracting to a shared test utility class.tests/Visage.Test.Aspire/SocialProfileDisconnectTests.cs (1)
20-49: Duplicated helper methods across test files.
GetUserProfileDbConnectionString(),CreateUserDb(), andEnsureAuthBackedUserAsync()are identical toSocialProfileLinkingTests.cs. Extract these to a shared base class or utility class to improve maintainability.Visage.FrontEnd/Visage.FrontEnd.Shared/Services/EventService.cs (1)
167-167: Remove unnecessary null-conditional on logger.Since
loggeris injected via primary constructor, it will never be null at runtime. The?.operator is unnecessary and can be removed for cleaner code.Proposed fix (apply throughout the file)
- logger?.LogInformation("Calling eventing /registrations to register for {EventId}", eventId); + logger.LogInformation("Calling eventing /registrations to register for {EventId}", eventId); - logger?.LogWarning("EventService.RegisterForEventAsync failed: {Status} {Message}", response.StatusCode, await response.Content.ReadAsStringAsync()); + logger.LogWarning("EventService.RegisterForEventAsync failed: {Status} {Message}", response.StatusCode, await response.Content.ReadAsStringAsync()); - logger?.LogError(ex, "Error registering for event {EventId}", eventId); + logger.LogError(ex, "Error registering for event {EventId}", eventId);Also applies to: 175-175, 180-180
| builder.Services.AddHttpLogging(logging => | ||
| { | ||
| logging.LoggingFields = Microsoft.AspNetCore.HttpLogging.HttpLoggingFields.All; | ||
| logging.RequestHeaders.Add("Authorization"); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and inspect the Program.cs file
find . -path "*Visage.Services.UserProfile*" -name "Program.cs" | head -5Repository: HackerspaceMumbai/Visage
Length of output: 108
🏁 Script executed:
# Read the Program.cs file and show lines around 82-86
sed -n '75,95p' ./Visage.Services.UserProfile/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 930
🏁 Script executed:
# Let's verify what HttpLoggingFields.All includes by checking ASP.NET Core HttpLogging documentation
# Also check if there are any other similar patterns in the codebase
rg "HttpLoggingFields\." --type cs -A 2 -B 2Repository: HackerspaceMumbai/Visage
Length of output: 444
🌐 Web query:
HttpLoggingFields.All ASP.NET Core what fields included Authorization header
💡 Result:
HttpLoggingFields.All enables logging of all request/response parts (path, query, protocol, method, scheme, headers, body, response, status code, etc.). [1]
The Authorization header name is logged by the middleware but its value is redacted by default for privacy/safety. To record the actual Authorization value you must opt in by adding "Authorization" to HttpLoggingOptions.RequestHeaders (and be careful with sensitive data). Example:
builder.Services.AddHttpLogging(opts => {
opts.LoggingFields = HttpLoggingFields.All;
opts.RequestHeaders.Add("Authorization");
});. [1][2]
Sources:
- Microsoft Docs — HttpLoggingFields / HTTP logging middleware. [1]
- Docs/articles explaining default redaction and how to opt in for specific headers. [2]
🏁 Script executed:
# Check if there are other logging configurations or sensitive headers being logged
rg "RequestHeaders\.Add|ResponseHeaders\.Add|LoggingFields" --type cs -B 3 -A 1Repository: HackerspaceMumbai/Visage
Length of output: 484
Remove Authorization header from HTTP logging. RequestHeaders.Add("Authorization") explicitly opts-in to logging Authorization header values, which will expose bearer tokens in logs. By default, ASP.NET Core redacts sensitive header values; this line overrides that safety mechanism. Remove the RequestHeaders.Add("Authorization") call and consider narrowing HttpLoggingFields.All to only needed fields.
Suggested fix
builder.Services.AddHttpLogging(logging =>
{
- logging.LoggingFields = Microsoft.AspNetCore.HttpLogging.HttpLoggingFields.All;
- logging.RequestHeaders.Add("Authorization");
+ logging.LoggingFields =
+ Microsoft.AspNetCore.HttpLogging.HttpLoggingFields.RequestPropertiesAndHeaders |
+ Microsoft.AspNetCore.HttpLogging.HttpLoggingFields.ResponsePropertiesAndHeaders;
});🤖 Prompt for AI Agents
In `@Visage.Services.UserProfile/Program.cs` around lines 82 - 86, Remove the
explicit inclusion of the Authorization header from HTTP logging: in the
AddHttpLogging configuration (builder.Services.AddHttpLogging) delete the
RequestHeaders.Add("Authorization") call so bearer tokens are not captured, and
optionally replace HttpLoggingFields.All with a narrower set of fields that you
actually need (e.g., exclude RequestHeaders) to avoid logging sensitive data.
| public enum RegistrationStatus | ||
| { | ||
| /// <summary> | ||
| /// Registration submitted, awaiting review | ||
| /// </summary> | ||
| Pending, | ||
|
|
||
| /// <summary> | ||
| /// Registration approved, user can attend | ||
| /// </summary> | ||
| Approved, | ||
|
|
||
| /// <summary> | ||
| /// Registration rejected (e.g., event full, doesn't meet criteria) | ||
| /// </summary> | ||
| Rejected, | ||
|
|
||
| /// <summary> | ||
| /// Event full, user is on waitlist | ||
| /// </summary> | ||
| Waitlisted, | ||
|
|
||
| /// <summary> | ||
| /// User cancelled their registration | ||
| /// </summary> | ||
| Cancelled | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for RegistrationStatus enum definitions and mappings
echo "=== Backend RegistrationStatus ==="
rg -n "enum RegistrationStatus" --type cs
echo ""
echo "=== Frontend RegistrationStatus ==="
rg -n "enum RegistrationStatus" Visage.FrontEnd/
echo ""
echo "=== Potential mapping code between statuses ==="
rg -n "RegistrationStatus\." --type cs -C2 | head -150
echo ""
echo "=== Checking EventRegistration.cs for complete context ==="
cat -n Visage.Shared/Models/EventRegistration.cs | head -120Repository: HackerspaceMumbai/Visage
Length of output: 7085
🏁 Script executed:
#!/bin/bash
echo "=== Frontend EventRegistration.cs enum definition ==="
sed -n '64,120p' Visage.FrontEnd/Visage.FrontEnd.Shared/Models/EventRegistration.cs
echo ""
echo "=== Search for any mapping/conversion logic between frontend and backend ==="
rg -n "RegistrationStatus\." Visage.FrontEnd/ --type cs | grep -E "(Pending|Approved|Rejected|Waitlisted|Registered|Confirmed|CheckedIn|NoShow)" | head -50
echo ""
echo "=== Check if frontend imports backend models ==="
rg -n "Visage.Shared" Visage.FrontEnd/ --type cs | head -20Repository: HackerspaceMumbai/Visage
Length of output: 3173
🏁 Script executed:
#!/bin/bash
echo "=== EventService.cs - where SharedEventRegistration is used ==="
rg -n "SharedEventRegistration\|RegistrationStatus" Visage.FrontEnd/Visage.FrontEnd.Shared/Services/EventService.cs -A5 | head -100
echo ""
echo "=== RegistrationService.cs - registration logic ==="
cat -n Visage.FrontEnd/Visage.FrontEnd.Shared/Services/RegistrationService.cs | head -80
echo ""
echo "=== Check for any explicit conversion/mapping ==="
rg -n "ToFrontend\|ToBackend\|Convert\|Map" Visage.FrontEnd/ --type cs | grep -i registration | head -20Repository: HackerspaceMumbai/Visage
Length of output: 4188
🏁 Script executed:
#!/bin/bash
echo "=== Check namespace declarations in frontend EventRegistration.cs ==="
head -20 Visage.FrontEnd/Visage.FrontEnd.Shared/Models/EventRegistration.cs
echo ""
echo "=== Check usings in EventService.cs ==="
head -25 Visage.FrontEnd/Visage.FrontEnd.Shared/Services/EventService.cs
echo ""
echo "=== Full EventService.cs to see how SharedEventRegistration is used ==="
cat -n Visage.FrontEnd/Visage.FrontEnd.Shared/Services/EventService.cs
echo ""
echo "=== Check IEventService interface ==="
cat -n Visage.FrontEnd/Visage.FrontEnd.Shared/Services/IEventService.csRepository: HackerspaceMumbai/Visage
Length of output: 15417
Create explicit mapping between frontend and backend RegistrationStatus enums.
The frontend maintains its own EventRegistration model with enum values (Registered, Confirmed, CheckedIn, Attended, NoShow) but the EventService exclusively uses SharedEventRegistration (the backend model) with different enum values (Pending, Approved, Rejected, Waitlisted, Cancelled). The API responses deserialize directly into the backend enum, but the frontend UI model expects the frontend-specific enum. This mismatch creates a data contract incompatibility.
Either:
- Unify to a single enum (prefer backend enum if Eventing service is source of truth)
- Implement explicit conversion logic in
EventServicewhen deserializing API responses - Remove the unused frontend
EventRegistrationmodel if the backend model should be used exclusively
Without conversion, registration status display in UI components will show incorrect values from the backend lifecycle instead of the intended attendance tracking lifecycle.
🤖 Prompt for AI Agents
In `@Visage.Shared/Models/EventRegistration.cs` around lines 81 - 107, The backend
RegistrationStatus enum in SharedEventRegistration doesn't match the frontend
EventRegistration enum (Registered, Confirmed, CheckedIn, Attended, NoShow)
causing UI mismatch; update EventService to perform explicit mapping when
deserializing API responses: add a converter function (e.g.,
MapRegistrationStatus or ConvertSharedToFrontendStatus) that translates
RegistrationStatus values (Pending, Approved, Rejected, Waitlisted, Cancelled)
to the frontend enum values, use that converter wherever SharedEventRegistration
is deserialized, and remove or deprecate the duplicate frontend model only if
you choose to fully unify on the backend enum; reference RegistrationStatus,
SharedEventRegistration and EventService to locate where to implement the
conversion.
…tion Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
…w-suggestions Address code review findings from PR #213: security, authorization, and data consistency
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/Visage.Test.Aspire/HealthEndpointTests.cs (1)
18-33: Rename “Registration API” references to “UserProfile API” in test names and messages.Service target changed to
userprofile-api, but the method names, XML docs, and assertion messages still say “Registration API”, which is misleading in test output.🩹 Proposed fix
- /// Verify Registration API health endpoint is accessible and returns 200 OK + /// Verify UserProfile API health endpoint is accessible and returns 200 OK [Test] - public async Task RegistrationApi_Health_Endpoint_Should_Return_200() + public async Task UserProfileApi_Health_Endpoint_Should_Return_200() { @@ response.IsSuccessStatusCode.Should().BeTrue( - because: "Registration API /health endpoint must be accessible and return 200 OK"); + because: "UserProfile API /health endpoint must be accessible and return 200 OK"); @@ - /// Verify Registration API alive endpoint is accessible and returns 200 OK + /// Verify UserProfile API alive endpoint is accessible and returns 200 OK [Test] - public async Task RegistrationApi_Alive_Endpoint_Should_Return_200() + public async Task UserProfileApi_Alive_Endpoint_Should_Return_200() { @@ response.IsSuccessStatusCode.Should().BeTrue( - because: "Registration API /alive endpoint must be accessible and return 200 OK"); + because: "UserProfile API /alive endpoint must be accessible and return 200 OK");Also applies to: 36-51
tests/Visage.Test.Aspire/FrontEndHomeTests.cs (1)
37-50: Align client naming and assertions withuserprofile-api.The variable name and failure messages still say
registrations-api, which can mislead debugging when this test fails. Consider updating the naming and messages to match the new service.💡 Proposed fix
- var registrationsClient = TestAppContext.CreateHttpClient("userprofile-api"); - registrationsClient.Should().NotBeNull("registrations-api service should be discoverable"); - registrationsClient.BaseAddress.Should().NotBeNull("registrations-api service should have a base address"); + var userProfileClient = TestAppContext.CreateHttpClient("userprofile-api"); + userProfileClient.Should().NotBeNull("userprofile-api service should be discoverable"); + userProfileClient.BaseAddress.Should().NotBeNull("userprofile-api service should have a base address"); @@ - var registrationsHealth = await registrationsClient.GetAsync("/health"); - registrationsHealth.Should().HaveStatusCode(System.Net.HttpStatusCode.OK, "registrations-api service health check should succeed"); + var userProfileHealth = await userProfileClient.GetAsync("/health"); + userProfileHealth.Should().HaveStatusCode(System.Net.HttpStatusCode.OK, "userprofile-api service health check should succeed");Visage.Services.UserProfile/ProfileApi.cs (3)
84-93: Guard claims enumeration with development-only check.Unlike the JWT token logging at lines 36-76 which is properly guarded by
IsDevelopment(), this claims logging block runs unconditionally atLogInformationlevel, exposing PII (email, subject, roles) in production logs.🔒 Proposed fix
- try - { - logger.LogInformation("DEBUG: Claims on principal:"); - foreach (var c in http.User.Claims) - { - logger.LogInformation("DEBUG: Claim {Type} = {Value}", c.Type, c.Value); - } - } - catch (Exception ex) - { - logger.LogWarning(ex, "Failed to enumerate claims on principal"); - } + if (environment.IsDevelopment()) + { + try + { + logger.LogDebug("DEBUG: Claims on principal:"); + foreach (var c in http.User.Claims) + { + logger.LogDebug("DEBUG: Claim {Type} = {Value}", c.Type, c.Value); + } + } + catch (Exception ex) + { + logger.LogWarning(ex, "Failed to enumerate claims on principal"); + } + }
187-233: Add ownership verification to prevent IDOR.This endpoint allows any authenticated user to retrieve any other user's full profile by ID or email. While the endpoint is under
RequireAuthorization(), it lacks ownership verification.🔒 Proposed fix
group.MapGet("/{userId}", async (string userId, UserDB db, ILogger<UserDB> logger, HttpContext http) => { + // Verify the requesting user owns this profile + var requestingUser = await ResolveUserForCurrentUserAsync(http, db, logger, asTracking: false); + // Validate StrictId format - if not, try email lookup Id<User>? parsedUserId = null; if (!Id<User>.TryParse(userId, out var tmpId)) { // ... existing email lookup logic ... } + // Allow access only if user is fetching their own profile + if (requestingUser?.Id != parsedUserId) + { + logger.LogWarning("Profile GET: User {RequestingUserId} attempted to access profile {TargetUserId}", + requestingUser?.Id, parsedUserId); + return Results.Forbid(); + } + var user = await db.Users .AsNoTracking() .FirstOrDefaultAsync(u => u.Id == parsedUserId.Value);
236-273: Add ownership verification to PUT endpoint.Same IDOR vulnerability as the GET endpoint—any authenticated user can modify another user's AIDE profile fields by providing their ID or email.
🔒 Proposed fix
Add ownership verification similar to the GET endpoint fix:
group.MapPut("/{userId}", async (string userId, UserDB db, User updatedUser, ILogger<UserDB> logger, HttpContext http) => { + var requestingUser = await ResolveUserForCurrentUserAsync(http, db, logger, asTracking: false); + // Validate StrictId format - if not, try email lookup Id<User>? parsedUserId = null; // ... existing lookup logic ... + if (requestingUser?.Id != parsedUserId) + { + logger.LogWarning("Profile PUT: User {RequestingUserId} attempted to modify profile {TargetUserId}", + requestingUser?.Id, parsedUserId); + return Results.Forbid(); + }tests/Visage.Test.Aspire/SocialProfileDisconnectTests.cs (1)
85-137: Set auth header beforeEnsureAuthBackedUserAsync().
The POST/registerendpoint requires valid auth claims to succeed. Without the auth header, the call fails with a 400 response, breaking the test. MoveSetDefaultAuthHeaderbeforeEnsureAuthBackedUserAsyncto ensure the registration call succeeds.Suggested adjustment
await TestAppContext.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running, TimeSpan.FromSeconds(90)); var httpClient = TestAppContext.CreateHttpClient("userprofile-api"); + await TestAppContext.SetDefaultAuthHeader(httpClient); await EnsureAuthBackedUserAsync(httpClient); var email = GetTestUserEmail(); @@ - await TestAppContext.SetDefaultAuthHeader(httpClient); - var dto = new SocialDisconnectDto { Provider = "linkedin" };
🤖 Fix all issues with AI agents
In `@services/Visage.Services.Eventing/Program.cs`:
- Around line 292-301: The admin role check in ApproveRegistration should return
HTTP 403 instead of 400: change the result union generic from BadRequest<string>
to Forbid (replace BadRequest<string> with Forbid/ForbidResult as your project
uses) and replace the call to TypedResults.BadRequest("Insufficient privileges
to approve registrations") with TypedResults.Forbid() (or
TypedResults.ForbidResult()) so the method returns a 403 when
http.User.IsInRole("VisageAdmin") fails.
- Around line 276-290: GetMyRegistrations currently returns an
Ok<List<EventRegistration>> even for unauthenticated requests; change it to
return a proper unauthorized response: update the method signature from
Task<Ok<List<EventRegistration>>> GetMyRegistrations(...) to Task<IResult> (or
Task<IResult>/Task<IResult> depending on style) and when auth0Sub is null/empty
return TypedResults.Unauthorized(); keep the authenticated branch returning
TypedResults.Ok(registrations) and ensure any callers/route definitions accept
the new IResult return type; refer to GetMyRegistrations, EventDB, HttpContext
and the TypedResults.Ok usage to locate the code to modify.
- Around line 234-243: The handler RegisterForEvent currently returns BadRequest
when the Auth0 subject is missing; change it to return HTTP 401 by updating the
method's discriminated result type to include an Unauthorized variant (e.g., add
Unauthorized<string> to the Results<> union) and replace the
TypedResults.BadRequest(...) return with
TypedResults.Unauthorized("Unauthorized: Auth0 subject not found"); locate the
auth check around auth0Sub (http.User.FindFirst("sub")?.Value) and adjust the
method signature and return accordingly so the compiler knows Unauthorized is a
possible result.
- Around line 328-337: The authentication failure in CheckInToSession uses
TypedResults.BadRequest and a return union of Results<BadRequest<string>,
Ok<CheckInResponse>>; change this to return a 401 by updating the method's
result union to Results<Unauthorized<string>, Ok<CheckInResponse>> and replace
the TypedResults.BadRequest("Unauthorized: Auth0 subject not found") with
TypedResults.Unauthorized("Unauthorized: Auth0 subject not found") (keep the
auth0Sub check and message but return Unauthorized) so callers receive HTTP 401
for missing Auth0 subject.
- Around line 383-409: The CheckOutFromSession endpoint (CheckOutFromSession)
currently allows any caller to modify SessionCheckIns in EventDB; update it to
require authentication, obtain the caller's user id/claims from HttpContext (or
injected principal) and verify ownership or admin privileges before modifying
the record, returning Unauthorized/Forbidden if the caller is not the owner and
not in an admin role; ensure the check compares the found checkIn's owner/user
id to the caller's id (and/or checks a role claim) prior to setting CheckedOutAt
and saving.
- Around line 352-373: The check-and-insert for SessionCheckIns is racy and the
EF index needs to be made unique and the service must handle concurrent inserts:
update the model configuration in EventDB (the entity configuration that defines
the composite index for SessionCheckIn using entity.HasIndex(e => new {
e.EventRegistrationId, e.SessionId })) to call .IsUnique() and keep the same
.HasDatabaseName("IX_SessionCheckIns_Registration_Session"), then update the
check-in flow (the code that creates a new SessionCheckIn and calls
db.SaveChangesAsync) to catch the DbUpdateException/DbUpdateConcurrencyException
for unique constraint violations and return a BadRequest or idempotent success
accordingly so concurrent requests don’t create duplicates.
In `@tests/Visage.Test.Aspire/QUICKSTART.md`:
- Around line 26-33: The markdown uses bold lines "Option A: Interactive Script
(Recommended)" and "Option B: Manual Setup" as section labels which triggers
markdownlint MD036; replace those bolded labels with proper markdown headings
(e.g., make them "## Option A: Interactive Script (Recommended)" and "## Option
B: Manual Setup") so they are recognized as headings, keep the existing
PowerShell code block and surrounding content intact, and run markdownlint to
confirm MD036 is resolved.
- Around line 60-62: The fenced code block showing "Now listening on:
https://localhost:5001" lacks a language tag; update that markdown fenced block
(the triple-backtick surrounding the "Now listening on..." snippet) to include a
language designation such as "text" (i.e., change the opening fence to ```text)
so markdownlint MD040 is satisfied and the snippet is properly highlighted.
In `@tests/Visage.Test.Aspire/TestAssemblyHooks.cs`:
- Around line 38-48: The external-mode startup sequence adds
"cloudinary-image-signing" via AddIfPresent but doesn't verify its reachability;
update the external branch in TestAssemblyHooks (where AddIfPresent and the
checkTimeout/WaitForResourceAsync calls live) to also await
TestAppContext.WaitForResourceAsync("cloudinary-image-signing",
KnownResourceStates.Running, checkTimeout) before returning so the external mode
mirrors the internal-mode readiness checks.
- Around line 71-72: Rename the test methods and update their XML documentation
to reflect the service rename from "Registration API" to "User Profile API":
change method names matching RegistrationApi_* (in HealthEndpointTests.cs) to
UserProfileApi_* and update any XML <summary> or doc comments that say
"Registration API" to "User Profile API" (or reference the resource name
"userprofile-api") so names and docs are consistent with the resourceNames array
and the TestAssemblyHooks usage of "userprofile-api".
In `@Visage.FrontEnd/Visage.FrontEnd.Shared/Models/EventRegistration.cs`:
- Around line 7-59: The frontend EventRegistration DTO (class EventRegistration)
doesn’t match the backend contract and will cause data loss/invalid payloads;
either (A) align this DTO to the backend entity by adding the missing backend
properties Auth0Subject, AdditionalDetails, ApprovedAt, ApprovedBy, CheckInPin,
RejectionReason (with appropriate types and nullability) and remove or move
UI-only properties (ConfirmedAt, CancelledAt, CancellationReason, CheckedInAt,
FirstTimeAttendee, AreasOfInterest, SelfDescribeAreasOfInterest,
VolunteerOpportunities, TicketType, TicketNumber) or (B) convert this class into
a UI/view model and create a separate backend DTO that exactly matches the
backend entity, then implement mapping functions (e.g.,
MapToBackendDto/MapFromBackendDto) to translate between EventRegistration (UI)
and the backend-shaped DTO before send/receive; update usages of
EventRegistration in serialization/deserialization and API calls to use the
correct DTO or mapping.
In `@Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/Home.razor`:
- Around line 41-52: The RSVP banner always uses "alert-success"; add a boolean
flag (e.g., rsvpSuccess) that is set when the RSVP completes (true on success,
false on failure) and update the markup that renders rsvpMessage to bind the
alert class based on that flag (switch between "alert-success" and
"alert-danger" or similar). Update the code paths that set rsvpMessage (where
rsvp is handled) to also set rsvpSuccess appropriately, and apply the same
change to the other identical blocks referenced (the blocks around the other
occurrences of rsvpMessage/rsvpInProgress at the indicated locations) so
success/failure styling is consistent.
In `@Visage.Services.UserProfile/Program.cs`:
- Around line 326-419: The /register MapPost route lacks authorization and
trusts client-supplied inputUser.Id to find records, enabling unauthorized
edits; fix by applying RequireAuthorization() to the MapPost("/register", ...)
endpoint and remove/ignore the branch that looks up users by inputUser.Id (the
code using inputUser.Id and db.Users.FirstOrDefaultAsync(u => u.Id ==
inputUser.Id)); instead always locate existing users only by email
(db.Users.FirstOrDefaultAsync(u => u.Email == inputUser.Email)) and enforce that
any existing record's Auth0Subject matches the caller's auth0Subject before
updating (or reject the request), ensuring only the authenticated owner can
modify their profile.
♻️ Duplicate comments (2)
services/Visage.Services.Eventing/Program.cs (1)
411-434: Consider restricting LookupByPin to admin role and limiting returned data.Authentication was added (addressing the previous review), but the endpoint still returns the full
EventRegistrationincluding potentially sensitive fields likeAuth0Subject. For kiosk/door check-in scenarios, consider:
- Restricting to
VisageAdminrole if only staff should use this endpoint- Returning a DTO with only necessary fields (name, status, event info) instead of the full entity
Visage.FrontEnd/Visage.FrontEnd.Shared/Models/User.cs (1)
51-57: Remove[Required]fromGovtIdto match backend/PII rules.The backend model does not require the full government ID and only relies on the last 4 digits. Keeping this required on the frontend forces extra PII collection and can block submissions.
Suggested update
- [Required] public string GovtId { get; set; } = string.Empty;
🧹 Nitpick comments (7)
tests/Visage.Test.Aspire/RegistrationDbTests.cs (2)
27-35: Align comments and assertion messages with UserProfile naming.Stale “registration/registrations-api” text in comments and failure messages can mislead when tests fail.
🛠️ Proposed tweak
- // Wait for registrations-api to be ready + // Wait for userprofile-api to be ready @@ - healthResponse.IsSuccessStatusCode.Should().BeTrue( - "Registration service health check should succeed, confirming database connectivity"); + healthResponse.IsSuccessStatusCode.Should().BeTrue( + "UserProfile service health check should succeed, confirming database connectivity"); @@ - // Wait for Registration service to be ready + // Wait for UserProfile service to be readyAlso applies to: 46-50
16-16: Consider renaming test class/file to reflect UserProfile.Optional, but renaming
RegistrationDbTeststoUserProfileDbTests(and the file accordingly) would match the new service naming and reduce confusion.Visage.Services.UserProfile/ProfileApi.cs (2)
1023-1056: Consider more robust unique constraint detection.The current approach checks exception messages for specific index names (
IX_Users_LinkedInSubject, etc.), which is fragile if index names change. For SQL Server, consider checking the SQL error number (2601/2627 for unique constraint violations).♻️ Alternative approach
catch (DbUpdateException ex) { // SQL Server unique constraint violation error numbers var sqlEx = ex.InnerException as Microsoft.Data.SqlClient.SqlException; var isUniqueConstraintViolation = sqlEx?.Number is 2601 or 2627; if (!isUniqueConstraintViolation) { logger.LogError(ex, "Unexpected database error during social link"); throw; } // ... existing conflict handling ... }
1206-1237: Good helper pattern—consider using it consistently.
ResolveUserForCurrentUserAsyncproperly encapsulates user resolution logic. TheGET/PUT /{userId}endpoints (lines 187-341) duplicate similar resolution logic instead of leveraging this helper. Refactoring those endpoints to use this helper would reduce code duplication and ensure consistent behavior.Visage.Services.UserProfile/Program.cs (1)
421-424: Consider adding pagination to user listing endpoints.Both
GET /registerandGET /api/usersreturn all users without pagination. For scalability, consider addingskip/takeparameters.Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/Home.razor (1)
236-295: Persist prerender profile state to avoid double initialization.In prerender + interactive, a new component instance is created, so
profileCompletionStatus/hasCheckedProfileCompletioncan be recomputed and re-trigger service calls. Consider persisting this viaPersistentComponentState(or equivalent) so the interactive instance reuses the prerender result. As per coding guidelines, cache prerender state for reuse in interactive phase.tests/Visage.Test.Aspire/DraftSaveTests.cs (1)
25-49: Consider extracting the user-creation helper to a shared test utility.
CreateTestUserAsyncis repeated across DraftSaveTests, DraftRetrievalTests, and DraftDeletionTests. Centralizing it would reduce duplication and keep required fields in one place.
| // Check if already checked in to this session | ||
| var existingCheckIn = await db.SessionCheckIns | ||
| .FirstOrDefaultAsync(c => | ||
| c.EventRegistrationId == registration.Id && | ||
| c.SessionId == request.SessionId && | ||
| c.CheckedOutAt == null); | ||
|
|
||
| if (existingCheckIn != null) | ||
| { | ||
| return TypedResults.BadRequest($"Already checked in to session {request.SessionId}"); | ||
| } | ||
|
|
||
| var checkIn = new SessionCheckIn | ||
| { | ||
| EventRegistrationId = registration.Id, | ||
| SessionId = request.SessionId, | ||
| CheckedInAt = DateTime.UtcNow, | ||
| CheckInMethod = "JWT" | ||
| }; | ||
|
|
||
| db.SessionCheckIns.Add(checkIn); | ||
| await db.SaveChangesAsync(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find EventDB or similar database configuration files
find . -type f -name "*.cs" | xargs grep -l "SessionCheckIn" | head -20Repository: HackerspaceMumbai/Visage
Length of output: 373
🏁 Script executed:
# Search for SessionCheckIn model configuration and HasIndex
rg -n "SessionCheckIn" --type=cs -B 5 -A 20 | head -100Repository: HackerspaceMumbai/Visage
Length of output: 8016
🏁 Script executed:
# Look for EventDB specifically to find the model configuration
fd -t f "EventDB.cs"Repository: HackerspaceMumbai/Visage
Length of output: 112
🏁 Script executed:
# Search for HasIndex patterns related to SessionCheckIn
rg "HasIndex.*SessionCheckIn\|SessionCheckIn.*HasIndex" --type=cs -B 2 -A 5Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# Look for the CheckInToSession endpoint definition and context
rg "CheckInToSession|check in to session" --type=cs -B 30 -A 15 | head -150Repository: HackerspaceMumbai/Visage
Length of output: 12708
🏁 Script executed:
# Search for any transaction or locking patterns in Program.cs around this endpoint
rg "BeginTransaction|UseTransaction|lock\(|Mutex" --type=cs services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 50
Race condition allows duplicate session check-ins for the same registration and session.
The check for existing check-in (lines 353–357) and the insert (line 372–373) are not atomic. Under concurrent requests, two check-ins for the same EventRegistrationId and SessionId could both pass the check. The composite index on these fields in EventDB.cs (line 105) lacks a unique constraint, so the database won't prevent duplicates either.
Add .IsUnique() to the composite index in EventDB.cs line 105:
Suggested fix
entity.HasIndex(e => new { e.EventRegistrationId, e.SessionId })
.IsUnique()
.HasDatabaseName("IX_SessionCheckIns_Registration_Session");
🤖 Prompt for AI Agents
In `@services/Visage.Services.Eventing/Program.cs` around lines 352 - 373, The
check-and-insert for SessionCheckIns is racy and the EF index needs to be made
unique and the service must handle concurrent inserts: update the model
configuration in EventDB (the entity configuration that defines the composite
index for SessionCheckIn using entity.HasIndex(e => new { e.EventRegistrationId,
e.SessionId })) to call .IsUnique() and keep the same
.HasDatabaseName("IX_SessionCheckIns_Registration_Session"), then update the
check-in flow (the code that creates a new SessionCheckIn and calls
db.SaveChangesAsync) to catch the DbUpdateException/DbUpdateConcurrencyException
for unique constraint violations and return a BadRequest or idempotent success
accordingly so concurrent requests don’t create duplicates.
| AddIfPresent("userprofile-api"); | ||
| AddIfPresent("eventing"); | ||
| AddIfPresent("frontendweb"); | ||
| AddIfPresent("cloudinary-image-signing"); | ||
|
|
||
| // Verify that external endpoints are reachable quickly (fail early) | ||
| var checkTimeout = TimeSpan.FromSeconds(45); | ||
| await TestAppContext.WaitForResourceAsync("registrations-api", KnownResourceStates.Running, checkTimeout); | ||
| await TestAppContext.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running, checkTimeout); | ||
| await TestAppContext.WaitForResourceAsync("eventing", KnownResourceStates.Running, checkTimeout); | ||
| await TestAppContext.WaitForResourceAsync("frontendweb", KnownResourceStates.Running, checkTimeout); | ||
| return; |
There was a problem hiding this comment.
Missing reachability check for cloudinary-image-signing in external mode.
cloudinary-image-signing is configured via AddIfPresent (line 41) but no corresponding WaitForResourceAsync call verifies its availability before tests proceed. The internal mode (lines 75-76) does wait for this service, creating an inconsistency.
Proposed fix
await TestAppContext.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running, checkTimeout);
await TestAppContext.WaitForResourceAsync("eventing", KnownResourceStates.Running, checkTimeout);
await TestAppContext.WaitForResourceAsync("frontendweb", KnownResourceStates.Running, checkTimeout);
+ await TestAppContext.WaitForResourceAsync("cloudinary-image-signing", KnownResourceStates.Running, checkTimeout);
return;📝 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.
| AddIfPresent("userprofile-api"); | |
| AddIfPresent("eventing"); | |
| AddIfPresent("frontendweb"); | |
| AddIfPresent("cloudinary-image-signing"); | |
| // Verify that external endpoints are reachable quickly (fail early) | |
| var checkTimeout = TimeSpan.FromSeconds(45); | |
| await TestAppContext.WaitForResourceAsync("registrations-api", KnownResourceStates.Running, checkTimeout); | |
| await TestAppContext.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running, checkTimeout); | |
| await TestAppContext.WaitForResourceAsync("eventing", KnownResourceStates.Running, checkTimeout); | |
| await TestAppContext.WaitForResourceAsync("frontendweb", KnownResourceStates.Running, checkTimeout); | |
| return; | |
| AddIfPresent("userprofile-api"); | |
| AddIfPresent("eventing"); | |
| AddIfPresent("frontendweb"); | |
| AddIfPresent("cloudinary-image-signing"); | |
| // Verify that external endpoints are reachable quickly (fail early) | |
| var checkTimeout = TimeSpan.FromSeconds(45); | |
| await TestAppContext.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running, checkTimeout); | |
| await TestAppContext.WaitForResourceAsync("eventing", KnownResourceStates.Running, checkTimeout); | |
| await TestAppContext.WaitForResourceAsync("frontendweb", KnownResourceStates.Running, checkTimeout); | |
| await TestAppContext.WaitForResourceAsync("cloudinary-image-signing", KnownResourceStates.Running, checkTimeout); | |
| return; |
🤖 Prompt for AI Agents
In `@tests/Visage.Test.Aspire/TestAssemblyHooks.cs` around lines 38 - 48, The
external-mode startup sequence adds "cloudinary-image-signing" via AddIfPresent
but doesn't verify its reachability; update the external branch in
TestAssemblyHooks (where AddIfPresent and the checkTimeout/WaitForResourceAsync
calls live) to also await
TestAppContext.WaitForResourceAsync("cloudinary-image-signing",
KnownResourceStates.Running, checkTimeout) before returning so the external mode
mirrors the internal-mode readiness checks.
| await rns.WaitForResourceAsync("userprofile-api", KnownResourceStates.Running) | ||
| .WaitAsync(TimeSpan.FromSeconds(90)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd tests/Visage.Test.Aspire && cat -n HealthEndpointTests.cs | head -50Repository: HackerspaceMumbai/Visage
Length of output: 2211
🏁 Script executed:
rg -n "resourceNames|userprofile-api|registrations-api" tests/Visage.Test.Aspire/HealthEndpointTests.cs -A 5 -B 2Repository: HackerspaceMumbai/Visage
Length of output: 990
🏁 Script executed:
rg -n "RegistrationApi_" tests/Visage.Test.Aspire/HealthEndpointTests.csRepository: HackerspaceMumbai/Visage
Length of output: 220
Test method names and documentation need updating after service rename.
The resourceNames array in HealthEndpointTests.cs already includes "userprofile-api" (line 170), which is correct. However, the test methods still use RegistrationApi_* naming (lines 22, 40) and reference "Registration API" in their XML documentation, even though they test the userprofile-api resource. Update these method names and documentation to reflect the service rename (e.g., UserProfileApi_*) for consistency.
🤖 Prompt for AI Agents
In `@tests/Visage.Test.Aspire/TestAssemblyHooks.cs` around lines 71 - 72, Rename
the test methods and update their XML documentation to reflect the service
rename from "Registration API" to "User Profile API": change method names
matching RegistrationApi_* (in HealthEndpointTests.cs) to UserProfileApi_* and
update any XML <summary> or doc comments that say "Registration API" to "User
Profile API" (or reference the resource name "userprofile-api") so names and
docs are consistent with the resourceNames array and the TestAssemblyHooks usage
of "userprofile-api".
| /// <summary> | ||
| /// EventRegistration entity representing a user's registration for a specific event. | ||
| /// Frontend model matching the backend EventRegistration entity. | ||
| /// </summary> | ||
| public class EventRegistration | ||
| { | ||
| [Required] | ||
| public Id<EventRegistration> Id { get; init; } | ||
|
|
||
| // Foreign keys | ||
| [Required] | ||
| public Id<User> UserId { get; set; } | ||
|
|
||
| [Required] | ||
| public Id<SharedEvent> EventId { get; set; } | ||
|
|
||
| // Registration status tracking | ||
| [Required] | ||
| public RegistrationStatus Status { get; set; } = RegistrationStatus.Pending; | ||
|
|
||
| public DateTime RegisteredAt { get; set; } = DateTime.UtcNow; | ||
|
|
||
| public DateTime? ConfirmedAt { get; set; } | ||
|
|
||
| public DateTime? CancelledAt { get; set; } | ||
|
|
||
| public string? CancellationReason { get; set; } | ||
|
|
||
| public DateTime? CheckedInAt { get; set; } | ||
|
|
||
| // Event-specific fields | ||
| /// <summary> | ||
| /// Whether this is the user's first time attending this type of event. | ||
| /// </summary> | ||
| public string FirstTimeAttendee { get; set; } = string.Empty; | ||
|
|
||
| /// <summary> | ||
| /// User's areas of interest for this specific event. | ||
| /// </summary> | ||
| public string AreasOfInterest { get; set; } = string.Empty; | ||
|
|
||
| public string SelfDescribeAreasOfInterest { get; set; } = string.Empty; | ||
|
|
||
| /// <summary> | ||
| /// Volunteer opportunities the user is interested in for this event. | ||
| /// </summary> | ||
| public string VolunteerOpportunities { get; set; } = string.Empty; | ||
|
|
||
| // Ticketing | ||
| public string? TicketType { get; set; } | ||
|
|
||
| public string? TicketNumber { get; set; } | ||
| } |
There was a problem hiding this comment.
Frontend EventRegistration doesn’t match backend contract.
The class claims to mirror the backend entity, but it omits backend fields (Auth0Subject, AdditionalDetails, ApprovedAt, ApprovedBy, CheckInPin, RejectionReason) and introduces fields not present in the backend (ConfirmedAt, CancelledAt, CancellationReason, CheckedInAt, interest/ticket fields). This can drop data on deserialization and produce invalid payloads when posting back.
Consider aligning the DTO with the backend model (or explicitly introduce a separate view model + mapping if these fields are UI-only).
✅ Option A: align with backend entity shape
public class EventRegistration
{
[Required]
public Id<EventRegistration> Id { get; init; }
// Foreign keys
[Required]
public Id<User> UserId { get; set; }
[Required]
public Id<SharedEvent> EventId { get; set; }
+ /// <summary>
+ /// Auth0 subject claim of the registered user (cross-service reference used by Eventing).
+ /// </summary>
+ [Required]
+ [StringLength(255)]
+ public string Auth0Subject { get; set; } = string.Empty;
+
// Registration status tracking
[Required]
public RegistrationStatus Status { get; set; } = RegistrationStatus.Pending;
public DateTime RegisteredAt { get; set; } = DateTime.UtcNow;
- public DateTime? ConfirmedAt { get; set; }
-
- public DateTime? CancelledAt { get; set; }
-
- public string? CancellationReason { get; set; }
-
- public DateTime? CheckedInAt { get; set; }
-
- // Event-specific fields
- /// <summary>
- /// Whether this is the user's first time attending this type of event.
- /// </summary>
- public string FirstTimeAttendee { get; set; } = string.Empty;
-
- /// <summary>
- /// User's areas of interest for this specific event.
- /// </summary>
- public string AreasOfInterest { get; set; } = string.Empty;
-
- public string SelfDescribeAreasOfInterest { get; set; } = string.Empty;
-
- /// <summary>
- /// Volunteer opportunities the user is interested in for this event.
- /// </summary>
- public string VolunteerOpportunities { get; set; } = string.Empty;
-
- // Ticketing
- public string? TicketType { get; set; }
-
- public string? TicketNumber { get; set; }
+ /// <summary>
+ /// Reason if registration was rejected
+ /// </summary>
+ public string? RejectionReason { get; }
+
+ /// <summary>
+ /// 4-digit PIN for quick check-in at door/kiosk (generated on approval)
+ /// </summary>
+ [StringLength(4, MinimumLength = 4)]
+ public string? CheckInPin { get; }
+
+ /// <summary>
+ /// Event-specific registration questions/answers stored as JSON
+ /// </summary>
+ public string? AdditionalDetails { get; set; }
+
+ /// <summary>
+ /// Timestamp when registration was approved (for approved registrations)
+ /// </summary>
+ public DateTime? ApprovedAt { get; set; }
+
+ /// <summary>
+ /// Who approved the registration (Auth0 subject of admin/organizer)
+ /// </summary>
+ public string? ApprovedBy { get; set; }
}🤖 Prompt for AI Agents
In `@Visage.FrontEnd/Visage.FrontEnd.Shared/Models/EventRegistration.cs` around
lines 7 - 59, The frontend EventRegistration DTO (class EventRegistration)
doesn’t match the backend contract and will cause data loss/invalid payloads;
either (A) align this DTO to the backend entity by adding the missing backend
properties Auth0Subject, AdditionalDetails, ApprovedAt, ApprovedBy, CheckInPin,
RejectionReason (with appropriate types and nullability) and remove or move
UI-only properties (ConfirmedAt, CancelledAt, CancellationReason, CheckedInAt,
FirstTimeAttendee, AreasOfInterest, SelfDescribeAreasOfInterest,
VolunteerOpportunities, TicketType, TicketNumber) or (B) convert this class into
a UI/view model and create a separate backend DTO that exactly matches the
backend entity, then implement mapping functions (e.g.,
MapToBackendDto/MapFromBackendDto) to translate between EventRegistration (UI)
and the backend-shaped DTO before send/receive; update usages of
EventRegistration in serialization/deserialization and API calls to use the
correct DTO or mapping.
| @* RSVP feedback *@ | ||
| @if (!string.IsNullOrEmpty(rsvpMessage)) | ||
| { | ||
| <div class="alert alert-success mb-4"> | ||
| <span>@rsvpMessage</span> | ||
| </div> | ||
| } | ||
|
|
||
| @if (rsvpInProgress) | ||
| { | ||
| <div class="mb-4"><span class="loading loading-spinner loading-sm"></span> Registering...</div> | ||
| } |
There was a problem hiding this comment.
Differentiate RSVP success vs failure styling.
The banner always uses alert-success, even when registration fails. Track a success flag and bind the alert class accordingly so users aren’t misled.
✅ Suggested update
- private string? rsvpMessage;
- private bool rsvpInProgress;
+ private string? rsvpMessage;
+ private bool? rsvpSuccess;
+ private bool rsvpInProgress;- <div class="alert alert-success mb-4">
+ <div class="alert @(rsvpSuccess == true ? "alert-success" : "alert-error") mb-4">
<span>@rsvpMessage</span>
</div>- rsvpMessage = null;
+ rsvpMessage = null;
+ rsvpSuccess = null;
...
- rsvpMessage = registration != null
+ rsvpSuccess = registration != null;
+ rsvpMessage = rsvpSuccess == true
? "You're registered — check 'My Registrations' for details."
: "Registration failed. Please try again.";
...
- rsvpMessage = "An error occurred while registering. Please try again.";
+ rsvpSuccess = false;
+ rsvpMessage = "An error occurred while registering. Please try again.";Also applies to: 125-128, 281-288
🤖 Prompt for AI Agents
In `@Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/Home.razor` around lines 41 -
52, The RSVP banner always uses "alert-success"; add a boolean flag (e.g.,
rsvpSuccess) that is set when the RSVP completes (true on success, false on
failure) and update the markup that renders rsvpMessage to bind the alert class
based on that flag (switch between "alert-success" and "alert-danger" or
similar). Update the code paths that set rsvpMessage (where rsvp is handled) to
also set rsvpSuccess appropriately, and apply the same change to the other
identical blocks referenced (the blocks around the other occurrences of
rsvpMessage/rsvpInProgress at the indicated locations) so success/failure
styling is consistent.
Body: • Remove .NET workload restore step; MAUI workloads not needed on Ubuntu runners. • Switch to TUnit -- --treenode-filter for category-based test selection, targeting the Aspire test project directly. • Log .NET SDK version in workflow output.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/tests.yml:
- Around line 33-34: The workflow step named "Run default smoke tests" (and the
similar step for RequiresAuth) is using the unsupported --treenode-filter
option; replace it with the TUnit-compatible flag --filter and use the TUnit
filter expression format (e.g. --filter "Category=Smoke" and --filter
"Category=RequiresAuth") so dotnet test --project ... uses the correct filtering
syntax for TUnit 1.5.70.
| - name: Run default smoke tests | ||
| run: dotnet test --filter "Category=Smoke" | ||
| run: dotnet test --project tests/Visage.Test.Aspire/Visage.Test.Aspire.csproj -- --treenode-filter "/*/*/*/*[Category=Smoke]" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the test framework and dependencies in the Aspire test project
cat -n tests/Visage.Test.Aspire/Visage.Test.Aspire.csproj | head -50Repository: HackerspaceMumbai/Visage
Length of output: 1950
🏁 Script executed:
# Search for any test framework references
rg -i "(nunit|xunit|mstest)" tests/Visage.Test.Aspire/ -g "*.csproj"Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# Look for Category or TestCategory attributes
rg -n "\[(Category|TestCategory|Trait)\(" tests/Visage.Test.Aspire/ -g "*.cs" -A 1Repository: HackerspaceMumbai/Visage
Length of output: 2010
🏁 Script executed:
# Check if there are Smoke or RequiresAuth test markers
rg -n "(Smoke|RequiresAuth)" tests/ -g "*.cs"Repository: HackerspaceMumbai/Visage
Length of output: 1152
🏁 Script executed:
# Examine the workflow file to see both lines 33-34 and 65-66
sed -n '30,70p' .github/workflows/tests.ymlRepository: HackerspaceMumbai/Visage
Length of output: 1465
🏁 Script executed:
# Search for any existing dotnet test invocations in the repo to see how filtering is done
rg -n "dotnet test.*--filter" . -g "*.yml" -g "*.yaml" -g "*.sh"Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# Check if there are any TUnit configuration files or documentation
fd -n "tunit" -i . --type fRepository: HackerspaceMumbai/Visage
Length of output: 298
🏁 Script executed:
# Look for TUnit filter usage in test files or scripts
rg -n "filter" tests/Visage.Test.Aspire/ -g "*.cs" | head -20Repository: HackerspaceMumbai/Visage
Length of output: 737
🏁 Script executed:
# Check TUnit package version and documentation reference in project files
rg "TUnit" . -g "*.csproj" -g "*.json"Repository: HackerspaceMumbai/Visage
Length of output: 1137
Replace --treenode-filter with --filter for TUnit compatibility.
TUnit uses --filter "Category=<value>" syntax, not --treenode-filter. The test framework is TUnit 1.5.70, and the categories (Smoke, RequiresAuth) exist in the codebase. The --project flag is valid. However, the filter command will fail or silently skip filtering with the current syntax.
Corrected filter syntax
- run: dotnet test --project tests/Visage.Test.Aspire/Visage.Test.Aspire.csproj -- --treenode-filter "/*/*/*/*[Category=Smoke]"
+ run: dotnet test --project tests/Visage.Test.Aspire/Visage.Test.Aspire.csproj --filter "Category=Smoke"Also applies to: 65-66 (replace --treenode-filter "/*/*/*/*[Category=RequiresAuth]" with --filter "Category=RequiresAuth")
🤖 Prompt for AI Agents
In @.github/workflows/tests.yml around lines 33 - 34, The workflow step named
"Run default smoke tests" (and the similar step for RequiresAuth) is using the
unsupported --treenode-filter option; replace it with the TUnit-compatible flag
--filter and use the TUnit filter expression format (e.g. --filter
"Category=Smoke" and --filter "Category=RequiresAuth") so dotnet test --project
... uses the correct filtering syntax for TUnit 1.5.70.
Refactor database initialization in Program.cs to use EnsureDeleted/EnsureCreated in development for rapid schema sync, and MigrateAsync in production for safe, versioned schema evolution. Update comments for clarity and improve error handling and logging.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/Visage.Services.Eventing/Program.cs (1)
210-229: Return 500 for unexpected failures instead of 400.The exception handler at line 228 returns
BadRequestfor any failure fromSaveChangesAsync(), which misclassifies server-side errors (e.g., database connection failures, constraint violations) as client errors. This misleads clients about whether the request was invalid or the server encountered an issue.💡 Suggested adjustment
-static async Task<Results<BadRequest<String>, Created<Event>>> ScheduleEvent([FromBody]Event EventDetails, EventDB db) +static async Task<Results<BadRequest<string>, Created<Event>, ProblemHttpResult>> ScheduleEvent([FromBody] Event EventDetails, EventDB db) { try { db.Events.Add(EventDetails); await db.SaveChangesAsync(); string newEventUri = $"/events/{EventDetails.Id}"; return TypedResults.Created(newEventUri, EventDetails); } catch (Exception ex) { - // Do not expose exception details to prevent information disclosure - return TypedResults.BadRequest("Failed to schedule event. Please check your input and try again."); + // Do not expose exception details to prevent information disclosure + return TypedResults.Problem( + "Failed to schedule event. Please try again later.", + statusCode: StatusCodes.Status500InternalServerError); } }
🤖 Fix all issues with AI agents
In `@services/Visage.Services.Eventing/Program.cs`:
- Around line 252-272: The current check-then-insert on EventRegistrations leads
to a race where SaveChangesAsync can throw DbUpdateException for a duplicate
(unique EventId, Auth0Subject); wrap the await db.SaveChangesAsync() call in a
try-catch that catches DbUpdateException (and optionally
DbUpdateConcurrencyException), detect/assume it represents the unique constraint
violation for EventRegistration, and return TypedResults.Conflict("You are
already registered for this event") instead of letting the exception bubble;
keep the existing creation of the EventRegistration and the call to
db.EventRegistrations.Add(registration), only adding the try/catch around
SaveChangesAsync and translating the duplicate error to a 409.
- Around line 297-301: Add an explicit authentication check before the role
check: if http.User?.Identity is null or http.User.Identity.IsAuthenticated is
false, return an UnauthorizedHttpResult (401) instead of falling through to
IsInRole; update the method's result union to include UnauthorizedHttpResult so
callers and the signature reflect the 401 possibility. Specifically, modify the
block that currently uses http.User.IsInRole("VisageAdmin") to first verify
authentication via http.User.Identity.IsAuthenticated and return Unauthorized
when not authenticated, then proceed to the IsInRole check and return
TypedResults.Forbid() only for authenticated-but-not-admin users.
♻️ Duplicate comments (7)
tests/Visage.Test.Aspire/QUICKSTART.md (1)
54-56: Add language specifier to fenced code block.The code block lacks a language tag, triggering markdownlint MD040. Add
textafter the opening fence for proper rendering and lint compliance.📝 Suggested fix
-``` +```text Now listening on: https://localhost:5001</details> </blockquote></details> <details> <summary>services/Visage.Services.Eventing/Program.cs (6)</summary><blockquote> `234-243`: **Return 401 for missing authentication instead of 400.** Line 239–242 uses `BadRequest` for unauthenticated callers; this should be `Unauthorized` with a matching result union. <details> <summary>✅ Suggested fix</summary> ```diff -static async Task<Results<BadRequest<string>, Created<EventRegistration>>> RegisterForEvent( +static async Task<Results<UnauthorizedHttpResult, BadRequest<string>, Created<EventRegistration>>> RegisterForEvent( [FromBody] RegisterEventRequest request, EventDB db, HttpContext http) { var auth0Sub = http.User.FindFirst("sub")?.Value; if (string.IsNullOrEmpty(auth0Sub)) { - return TypedResults.BadRequest("Unauthorized: Auth0 subject not found"); + return TypedResults.Unauthorized(); }
276-282: Unauthenticated access should return 401, not an empty list.Returning an empty list hides authentication failures. Use
Unauthorizedand expand the result union accordingly.✅ Suggested fix
-static async Task<Ok<List<EventRegistration>>> GetMyRegistrations(EventDB db, HttpContext http) +static async Task<Results<UnauthorizedHttpResult, Ok<List<EventRegistration>>>> GetMyRegistrations(EventDB db, HttpContext http) { var auth0Sub = http.User.FindFirst("sub")?.Value; if (string.IsNullOrEmpty(auth0Sub)) { - return TypedResults.Ok(new List<EventRegistration>()); + return TypedResults.Unauthorized(); }
333-337: Return 401 for missing authentication.Line 333–336 should return
Unauthorizedrather thanBadRequest.✅ Suggested fix
-static async Task<Results<BadRequest<string>, Ok<CheckInResponse>>> CheckInToSession( +static async Task<Results<UnauthorizedHttpResult, BadRequest<string>, Ok<CheckInResponse>>> CheckInToSession( [FromBody] CheckInRequest request, EventDB db, HttpContext http) { var auth0Sub = http.User.FindFirst("sub")?.Value; if (string.IsNullOrEmpty(auth0Sub)) { - return TypedResults.BadRequest("Unauthorized: Auth0 subject not found"); + return TypedResults.Unauthorized(); }
352-373: Prevent duplicate check-ins under concurrency.The check-then-insert is racy. Add a unique constraint on
(EventRegistrationId, SessionId)and handleDbUpdateExceptionto return a safe, idempotent response.
383-409: Critical: Check-out must require auth and ownership/admin checks.
CheckOutFromSessionallows any caller to modify check-in records by ID. Require authentication and verify the caller owns the registration or hasVisageAdmin.🔒 Suggested fix
-static async Task<Results<BadRequest<string>, Ok<CheckOutResponse>>> CheckOutFromSession( +static async Task<Results<UnauthorizedHttpResult, ForbidHttpResult, BadRequest<string>, Ok<CheckOutResponse>>> CheckOutFromSession( [FromBody] CheckOutRequest request, - EventDB db) + EventDB db, + HttpContext http) { + var auth0Sub = http.User.FindFirst("sub")?.Value; + if (string.IsNullOrEmpty(auth0Sub)) + { + return TypedResults.Unauthorized(); + } + var checkIn = await db.SessionCheckIns.FindAsync(request.CheckInId); if (checkIn == null) { return TypedResults.BadRequest("Check-in record not found"); } + + var registration = await db.EventRegistrations.FindAsync(checkIn.EventRegistrationId); + if (registration == null || (registration.Auth0Subject != auth0Sub && !http.User.IsInRole("VisageAdmin"))) + { + return TypedResults.Forbid(); + }
416-434: Limit PIN lookup to authorized users or return a minimal DTO.This still exposes full
EventRegistrationdata to any authenticated user. Restrict to admin/organizer roles or return a limited response (e.g., id/status). Also return 401 for unauthenticated callers.
- Enforce auth and owner checks for registration/profile updates - Add unique constraint and robust handling for session check-ins - Require admin role for registration approval; return 401/403 as needed - Improve RSVP UI feedback for success/failure - Protect /register endpoints with authorization - Update quickstart docs for clearer app startup output
Changed "Option A" header to Markdown heading for clarity and removed the setup script code block to streamline instructions. "Option B" remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@services/Visage.Services.Eventing/EventDB.cs`:
- Around line 59-95: EventDB's EF configuration for EventRegistration must
explicitly ignore cross-DbContext navigation/value properties to avoid shadow
properties or migration failures; in the
modelBuilder.Entity<EventRegistration>(...) block add ignore directives for the
User navigation and UserId scalar (i.e., call Ignore for the User property and
Ignore for the UserId property on the EventRegistration entity) so EventDB does
not try to map those UserProfile-specific members.
In `@services/Visage.Services.Eventing/Program.cs`:
- Around line 478-501: In LookupByPin, replace the BadRequest return for missing
authentication with a 401 Unauthorized response by returning
TypedResults.Unauthorized() (or the equivalent Unauthorized result your
framework uses) when auth0Sub is null/empty; additionally consider returning a
limited DTO instead of the full EventRegistration object from the final
TypedResults.Ok(registration) to avoid exposing sensitive fields (create a small
RegistrationDto and map the allowed properties before returning).
In `@Visage.Services.UserProfile/Program.cs`:
- Around line 253-257: The /api/users endpoint (app.MapGet("/api/users", ... )
which currently returns db.Users.ToListAsync() and is only protected by
RequireAuthorization() allows any authenticated user to enumerate all users;
either restrict it to admins by applying a role-based policy (e.g., define an
"AdminOnly" policy and call .RequireAuthorization("AdminOnly") or use the
appropriate RequireRole/Authorize attribute for the endpoint), or change the
handler to return only the caller's profile by extracting the user id from the
HttpContext/ClaimsPrincipal inside the MapGet handler and querying
db.Users.Where(u => u.Id == userId).FirstOrDefaultAsync() instead of returning
all users.
- Around line 370-388: The current /register flow looks up users by email
(existing = db.Users...FirstOrDefaultAsync(u => u.Email == inputUser.Email) )
then checks Auth0Subject, which creates the inconsistency described; change the
lookup to find by Auth0Subject (u => u.Auth0Subject == auth0Subject) so
ownership is the primary key, and when creating a new user ensure
inputUser.Auth0Subject is set to auth0Subject (and keep setting
inputUser.CreatedAt and saving as before); retain the OrderByDescending(u =>
u.ProfileCompletedAt) if you need to prefer the most recent profile but do not
rely on email for identification.
- Around line 324-339: The endpoint registered with
app.MapGet("/api/users/{userId}/registrations") currently returns any user's
registrations; update the route handler to extract the authenticated user id
from HttpContext.User (e.g., the sub or nameidentifier claim) and compare it to
parsedUserId, and also allow access if HttpContext.User.IsInRole("Admin") (or
equivalent admin claim); if the caller is neither the owner nor an admin return
a 403 (Results.Forbid) instead of the registrations; keep the existing
StrictId.Id<User>.TryParse validation and the db.EventRegistrations query but
gate the query behind this ownership/admin check and log an unauthorized access
attempt with the existing ILogger<Program> logger.
- Around line 436-439: The GET handler registered with app.MapGet("/register",
...) currently returns db.Users.ToListAsync() and only calls
RequireAuthorization(), allowing any authenticated user to enumerate all users;
change the handler to either (a) restrict access to admins by applying a role
policy (RequireAuthorization with an admin role/Policy) so only admins can call
this endpoint, or (b) convert it to return only the current user's profile by
resolving HttpContext.User inside the MapGet handler, extracting the user
identifier (e.g., sub/NameIdentifier) and querying
db.Users.SingleOrDefaultAsync(...) for that id instead of
db.Users.ToListAsync(); update the MapGet registration (and its
RequireAuthorization call) accordingly to implement the chosen fix.
♻️ Duplicate comments (2)
Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/Home.razor (1)
41-52: The RSVP banner styling still ignores thersvpSuccessflag.The
rsvpSuccessvariable was added (line 128), but the alert class on line 44 remains hardcoded toalert-success. This means failed registrations display a success-styled banner, misleading users.✅ Suggested fix
`@if` (!string.IsNullOrEmpty(rsvpMessage)) { - <div class="alert alert-success mb-4"> + <div class="alert @(rsvpSuccess ? "alert-success" : "alert-error") mb-4"> <span>@rsvpMessage</span> </div> }services/Visage.Services.Eventing/Program.cs (1)
347-351: Return 401 Unauthorized instead of BadRequest for authentication failures.For consistency with other handlers (
RegisterForEvent,GetMyRegistrations), this should returnTypedResults.Unauthorized()when the Auth0 subject is missing.🔧 Suggested fix
-static async Task<Results<BadRequest<string>, Ok<CheckInResponse>>> CheckInToSession( +static async Task<Results<UnauthorizedHttpResult, BadRequest<string>, Ok<CheckInResponse>>> CheckInToSession( [FromBody] CheckInRequest request, EventDB db, HttpContext http) { var auth0Sub = http.User.FindFirst("sub")?.Value; if (string.IsNullOrEmpty(auth0Sub)) { - return TypedResults.BadRequest("Unauthorized: Auth0 subject not found"); + return TypedResults.Unauthorized(); }
🧹 Nitpick comments (5)
Visage.FrontEnd/Visage.FrontEnd.Shared/Pages/Home.razor (1)
125-128: Consider usingbool?forrsvpSuccessto distinguish initial state.Using
boolmeansrsvpSuccessdefaults tofalse, which could be misinterpreted as a failure before any RSVP attempt. Usingbool?allows distinguishing "not yet attempted" from "failed".♻️ Suggested refinement
// RSVP UI state private string? rsvpMessage; private bool rsvpInProgress; - private bool rsvpSuccess; + private bool? rsvpSuccess;Then update the alert binding:
- <div class="alert @(rsvpSuccess ? "alert-success" : "alert-error") mb-4"> + <div class="alert @(rsvpSuccess == true ? "alert-success" : "alert-error") mb-4">services/Visage.Services.Eventing/Program.cs (1)
142-170: Consider adding.RequireAuthorization()to protected endpoints.The registration and check-in endpoints rely on manual auth checks inside handlers. Adding
.RequireAuthorization()at the route level ensures unauthenticated requests are rejected before reaching handler code, improving security posture and reducing boilerplate.♻️ Suggested enhancement
registrations.MapPost("/", RegisterForEvent) .WithName("Register for Event") - .WithOpenApi(); + .WithOpenApi() + .RequireAuthorization(); registrations.MapGet("/my", GetMyRegistrations) .WithName("Get My Registrations") - .WithOpenApi(); + .WithOpenApi() + .RequireAuthorization(); registrations.MapPost("/{id}/approve", ApproveRegistration) .WithName("Approve Registration") - .WithOpenApi(); + .WithOpenApi() + .RequireAuthorization(); checkin.MapPost("/", CheckInToSession) .WithName("Check-in to Session") - .WithOpenApi(); + .WithOpenApi() + .RequireAuthorization(); checkin.MapPost("/checkout", CheckOutFromSession) .WithName("Check-out from Session") - .WithOpenApi(); + .WithOpenApi() + .RequireAuthorization();Visage.Services.UserProfile/Program.cs (3)
14-15: Consider removing or environment-gating config logging at startup.Logging configuration values to the console at startup could inadvertently expose sensitive settings if this pattern is extended. While Auth0 domain and audience aren't secrets, this establishes a pattern that could lead to accidental secret exposure. Consider limiting such output to development or using structured logging at Debug level.
♻️ Suggested adjustment
-Console.WriteLine("Auth0Domain: " + builder.Configuration["Auth0:Domain"]); -Console.WriteLine("Auth0Audience: " + builder.Configuration["Auth0:Audience"]); +if (builder.Environment.IsDevelopment()) +{ + Console.WriteLine("Auth0Domain: " + builder.Configuration["Auth0:Domain"]); + Console.WriteLine("Auth0Audience: " + builder.Configuration["Auth0:Audience"]); +}
164-251: Endpoint bypasses registered repositories; consider using Repository pattern.Per coding guidelines, endpoints should use Repository and Unit of Work patterns with EF Core.
ProfileCompletionRepositoryis registered (line 84) but this endpoint accessesUserDBdirectly. This reduces consistency and makes the repository registration unused.Additionally, line 238 unconditionally sets
IsProfileComplete = truewithout validating that required fields are actually populated. Consider adding validation or delegating to the repository's profile completion logic.♻️ Consider injecting and using the repository
app.MapPost("/api/users", async Task<Results<Created<User>, Ok<User>, BadRequest>> ( [FromBody] User inputUser, UserDB db, + Visage.Services.UserProfile.Repositories.ProfileCompletionRepository profileRepo, HttpContext httpContext, ILogger<Program> logger) => { // ... authentication logic ... - existing.IsProfileComplete = true; + existing.IsProfileComplete = profileRepo.IsProfileComplete(existing);
83-87: Registered repositories are unused; endpoints accessUserDBdirectly.
ProfileCompletionRepositoryandUserPreferencesRepositoryare registered for DI but the inline endpoints bypass them entirely, accessingUserDBdirectly. As per coding guidelines requiring Repository and Unit of Work patterns with EF Core, consider refactoring endpoints to use these repositories for consistency, testability, and separation of concerns.
…rofile services Co-authored-by: indcoder <2099300+indcoder@users.noreply.github.com>
…egistration-config Security: Fix authorization bypass, user enumeration, and cross-DbContext mapping issues
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@services/Visage.Services.Eventing/Program.cs`:
- Around line 234-281: The handler RegisterForEvent currently depends on EventDB
and performs direct EF queries; create two scoped repositories (e.g.,
EventsRepository with EventExistsAsync(Guid eventId) and EventByIdAsync(Guid
eventId) and EventRegistrationsRepository with GetByEventAndSubjectAsync(Guid
eventId, string auth0Sub), AddAsync(EventRegistration reg) and
SaveChangesAsync/CommitAsync or CreateRegistrationAsync that handles
unique-constraint exceptions) and register them as scoped DI services; then
change RegisterForEvent to accept EventsRepository and
EventRegistrationsRepository instead of EventDB, replace the inline
AnyAsync/FirstOrDefaultAsync/Add/SaveChangesAsync calls with the new repository
method calls (use EventRegistration and RegistrationStatus names unchanged) and
propagate Conflict/BadRequest/Created results based on the repository outcomes.
- Around line 386-405: Replace the brittle message-based filter on the
DbUpdateException with a provider-specific SQL Server error code check: change
the exception filter on the DbUpdateException (the catch handling unique
constraint for SessionCheckIns) to inspect the underlying SqlException (via
ex.GetBaseException() or ex.InnerException) and test SqlException.Number for
2627 (unique constraint) and/or 2601 (PK/duplicate index) instead of checking
InnerException.Message; ensure you reference the SqlException type from
Microsoft.Data.SqlClient or System.Data.SqlClient and keep the existing logic
that looks up existingCheckIn and returns the same TypedResults responses.
In `@Visage.Services.UserProfile/Program.cs`:
- Around line 164-251: The handler currently forces existing.IsProfileComplete =
true and sets ProfileCompletedAt unconditionally in the POST "/api/users" upsert
(and likewise in the other endpoint), which can mark incomplete profiles as
complete; instead call into ProfileCompletionRepository (or run the same
completeness check logic used elsewhere) to compute a boolean isComplete for the
user (using fields on inputUser/existing) and only set
existing.IsProfileComplete = isComplete and existing.ProfileCompletedAt =
isComplete && existing.ProfileCompletedAt == null ? DateTime.UtcNow :
existing.ProfileCompletedAt (or null when not complete); apply the same change
in the other user upsert/update endpoint so completeness is derived, not
hard-coded.
- Around line 280-343: The endpoint directly binds the EventRegistration entity
(registration) allowing clients to over-post server-managed fields
(RejectionReason, CheckInPin, ApprovedAt, ApprovedBy); change the handler to
accept a minimal DTO (e.g., EventRegistrationCreateDto with only EventId and any
allowed AdditionalDetails), map that DTO to a new EventRegistration instance
server-side, then set UserId, Auth0Subject, RegisteredAt, Status and any other
server-controlled fields before saving; update the MapPost signature to use the
DTO and ensure any client-provided fields are explicitly whitelisted during the
mapping.
♻️ Duplicate comments (1)
services/Visage.Services.Eventing/Program.cs (1)
342-351: Return 401 for missing authentication in CheckInToSession.
Right now the handler returns 400. This should be 401 and the result union should includeUnauthorizedHttpResultfor consistency with other endpoints.#!/bin/bash # Review auth failure handling across handlers for consistency rg -n --type=cs "Unauthorized|BadRequest\\(\"Unauthorized" services/Visage.Services.Eventing/Program.cs🐛 Proposed fix
-static async Task<Results<BadRequest<string>, Ok<CheckInResponse>>> CheckInToSession( +static async Task<Results<UnauthorizedHttpResult, BadRequest<string>, Ok<CheckInResponse>>> CheckInToSession( [FromBody] CheckInRequest request, EventDB db, HttpContext http) { var auth0Sub = http.User.FindFirst("sub")?.Value; if (string.IsNullOrEmpty(auth0Sub)) { - return TypedResults.BadRequest("Unauthorized: Auth0 subject not found"); + return TypedResults.Unauthorized(); }
🧹 Nitpick comments (2)
Visage.Services.UserProfile/Program.cs (2)
102-143: Consider gating dev DB reset behind an explicit opt‑in.
EnsureDeletedAsync()+EnsureCreatedAsync()on every dev startup wipes local data and can be surprising. Making it opt‑in keeps developer data safe while still allowing quick resets when needed.♻️ Make dev reset opt‑in
- if (app.Environment.IsDevelopment()) + if (app.Environment.IsDevelopment()) { - // Development: Drop and recreate database to match current model - // This avoids migration issues during rapid schema iteration - logger.LogInformation("Development mode: Ensuring database schema matches current model..."); - try - { - await userDb.Database.EnsureDeletedAsync(); - await userDb.Database.EnsureCreatedAsync(); - logger.LogInformation("Database schema synchronized successfully."); - } - catch (Exception ex) - { - logger.LogError(ex, "Database initialization failed"); - throw new InvalidOperationException("Database initialization failed. Check database configuration and connectivity."); - } + var resetDb = app.Configuration.GetValue<bool>("RESET_DB_ON_STARTUP"); + logger.LogInformation("Development mode: {Mode}", resetDb ? "resetting DB" : "applying migrations"); + try + { + if (resetDb) + { + await userDb.Database.EnsureDeletedAsync(); + await userDb.Database.EnsureCreatedAsync(); + } + else + { + await userDb.Database.MigrateAsync(); + } + logger.LogInformation("Database schema synchronized successfully."); + } + catch (Exception ex) + { + logger.LogError(ex, "Database initialization failed"); + throw new InvalidOperationException("Database initialization failed. Check database configuration and connectivity."); + } }
164-343: Consider routing data access through repositories/UoW.
These endpoints useUserDBdirectly for reads/writes. The project guidelines call for Repository + Unit of Work patterns for EF Core. As per coding guidelines, consider moving persistence logic into repositories and keeping endpoints thin.
| static async Task<Results<BadRequest<string>, UnauthorizedHttpResult, Conflict, Created<EventRegistration>>> RegisterForEvent( | ||
| [FromBody] RegisterEventRequest request, | ||
| EventDB db, | ||
| HttpContext http) | ||
| { | ||
| var auth0Sub = http.User.FindFirst("sub")?.Value; | ||
| if (string.IsNullOrEmpty(auth0Sub)) | ||
| { | ||
| return TypedResults.Unauthorized(); | ||
| } | ||
|
|
||
| // Check if event exists | ||
| var eventExists = await db.Events.AnyAsync(e => e.Id == request.EventId); | ||
| if (!eventExists) | ||
| { | ||
| return TypedResults.BadRequest($"Event {request.EventId} not found"); | ||
| } | ||
|
|
||
| // Check if user already registered for this event | ||
| var existingReg = await db.EventRegistrations | ||
| .FirstOrDefaultAsync(r => r.EventId == request.EventId && r.Auth0Subject == auth0Sub); | ||
|
|
||
| if (existingReg != null) | ||
| { | ||
| return TypedResults.BadRequest("You are already registered for this event"); | ||
| } | ||
|
|
||
| var registration = new EventRegistration | ||
| { | ||
| EventId = request.EventId, | ||
| Auth0Subject = auth0Sub, | ||
| Status = RegistrationStatus.Pending, | ||
| AdditionalDetails = request.AdditionalDetails, | ||
| RegisteredAt = DateTime.UtcNow | ||
| }; | ||
|
|
||
| db.EventRegistrations.Add(registration); | ||
| try | ||
| { | ||
| await db.SaveChangesAsync(); | ||
| } | ||
| catch (DbUpdateException) | ||
| { | ||
| // Assume unique constraint violation (EventId, Auth0Subject) | ||
| return TypedResults.Conflict(); | ||
| } | ||
|
|
||
| return TypedResults.Created($"/registrations/{registration.Id}", registration); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Repository/UoW abstractions in the codebase
rg -n --type=cs "IUnitOfWork|IRepository|UnitOfWork|class.*Repository" --max-count=20Repository: HackerspaceMumbai/Visage
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Check the EventDB definition and context
rg -n --type=cs "class EventDB|DbContext" services/Visage.Services.Eventing --max-count=10Repository: HackerspaceMumbai/Visage
Length of output: 640
🏁 Script executed:
#!/bin/bash
# Look for other endpoints in Program.cs to see patterns
rg -n --type=cs "MapPost|MapGet|MapPut" services/Visage.Services.Eventing/Program.cs | head -20Repository: HackerspaceMumbai/Visage
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Check if services use dependency injection and what patterns they follow
rg -n --type=cs "AddScoped|AddSingleton|AddTransient.*Repository|AddTransient.*UnitOfWork" services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check UserProfile repository implementation
cat -n services/Visage.Services.UserProfile/Repositories/UserPreferencesRepository.csRepository: HackerspaceMumbai/Visage
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Check how repositories are registered in UserProfile
rg -n --type=cs "AddScoped|AddSingleton|Repository" services/Visage.Services.UserProfile/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Check other endpoints in Eventing to see if they also access EventDB directly
rg -B2 -A10 "static async Task.*Get|static async Task.*Approve|static async Task.*CheckIn" services/Visage.Services.Eventing/Program.cs | head -40Repository: HackerspaceMumbai/Visage
Length of output: 1208
🏁 Script executed:
#!/bin/bash
# Check if there's any UnitOfWork pattern anywhere in the solution
fd --type f -name "*.cs" -x rg -l "IUnitOfWork|class.*UnitOfWork" {} \;Repository: HackerspaceMumbai/Visage
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Find all files in UserProfile service
fd --type f -name "*.cs" services/Visage.Services.UserProfile | head -20Repository: HackerspaceMumbai/Visage
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Search for any UnitOfWork pattern in the entire codebase
fd --type f -name "*.cs" -path "*/services/*" -exec grep -l "UnitOfWork" {} \;Repository: HackerspaceMumbai/Visage
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Check the actual file structure of services
tree -L 2 services/ 2>/dev/null || find services/ -maxdepth 2 -type d | sortRepository: HackerspaceMumbai/Visage
Length of output: 696
🏁 Script executed:
#!/bin/bash
# Search for Repository class definitions
rg --type=cs "^class.*Repository" -A 3 services/Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search more broadly for repositories
rg --type=cs "Repository" . --max-count=20Repository: HackerspaceMumbai/Visage
Length of output: 2007
🏁 Script executed:
#!/bin/bash
# Check entire project structure to locate UserProfile
find . -name "UserPreferencesRepository.cs" -o -name "ProfileCompletionRepository.cs" 2>/dev/nullRepository: HackerspaceMumbai/Visage
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Look for any UnitOfWork or IRepository interfaces
rg --type=cs "interface IRepository|interface IUnitOfWork" .Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
#!/antml:bash
# Check what patterns are actually used in the Registrations service
cat -n services/Visage.Services.Registrations/Program.cs | head -50Repository: HackerspaceMumbai/Visage
Length of output: 2383
🏁 Script executed:
#!/bin/bash
# Check UserProfile repository implementation
cat -n ./Visage.Services.UserProfile/Repositories/UserPreferencesRepository.csRepository: HackerspaceMumbai/Visage
Length of output: 3352
🏁 Script executed:
#!/bin/bash
# Check how repositories are used in endpoints
cat -n ./Visage.Services.UserProfile/ProfileApi.cs | head -60Repository: HackerspaceMumbai/Visage
Length of output: 3081
🏁 Script executed:
#!/bin/bash
# Check if Registrations service also needs repositories
cat -n services/Visage.Services.Registrations/Program.cs | tail -50Repository: HackerspaceMumbai/Visage
Length of output: 2084
🏁 Script executed:
#!/bin/bash
# Check if there's any UnitOfWork interface or class definition
rg --type=cs "UnitOfWork|IUnitOfWork" . -A 2Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify guidelines reference to Unit of Work
cat -n .github/copilot-instructions.md | grep -i "repository\|unit of work"Repository: HackerspaceMumbai/Visage
Length of output: 172
Create repositories to abstract EventDB access.
All handlers in this service inject EventDB directly and query it inline. This violates the established repository pattern used in UserProfile (e.g., UserPreferencesRepository, ProfileCompletionRepository). Create repositories for Events and EventRegistrations, register them as scoped services, and inject them into handlers instead of the DbContext.
Example from RegisterForEvent handler
static async Task<Results<BadRequest<string>, UnauthorizedHttpResult, Conflict, Created<EventRegistration>>> RegisterForEvent(
[FromBody] RegisterEventRequest request,
EventDB db, // <-- Replace with repository injection
HttpContext http)
{
// Direct DbContext queries below should be moved to repository methods
var eventExists = await db.Events.AnyAsync(e => e.Id == request.EventId);
var existingReg = await db.EventRegistrations
.FirstOrDefaultAsync(r => r.EventId == request.EventId && r.Auth0Subject == auth0Sub);
// ...
}
🤖 Prompt for AI Agents
In `@services/Visage.Services.Eventing/Program.cs` around lines 234 - 281, The
handler RegisterForEvent currently depends on EventDB and performs direct EF
queries; create two scoped repositories (e.g., EventsRepository with
EventExistsAsync(Guid eventId) and EventByIdAsync(Guid eventId) and
EventRegistrationsRepository with GetByEventAndSubjectAsync(Guid eventId, string
auth0Sub), AddAsync(EventRegistration reg) and SaveChangesAsync/CommitAsync or
CreateRegistrationAsync that handles unique-constraint exceptions) and register
them as scoped DI services; then change RegisterForEvent to accept
EventsRepository and EventRegistrationsRepository instead of EventDB, replace
the inline AnyAsync/FirstOrDefaultAsync/Add/SaveChangesAsync calls with the new
repository method calls (use EventRegistration and RegistrationStatus names
unchanged) and propagate Conflict/BadRequest/Created results based on the
repository outcomes.
| catch (DbUpdateException ex) when (ex.InnerException?.Message.Contains("IX_SessionCheckIns_Registration_Session") == true) | ||
| { | ||
| // Unique constraint violation: already checked in | ||
| var existingCheckIn = await db.SessionCheckIns | ||
| .AsNoTracking() | ||
| .FirstOrDefaultAsync(c => | ||
| c.EventRegistrationId == registration.Id && | ||
| c.SessionId == request.SessionId && | ||
| c.CheckedOutAt == null); | ||
| if (existingCheckIn != null) | ||
| { | ||
| return TypedResults.Ok(new CheckInResponse( | ||
| existingCheckIn.Id, | ||
| registration.Id, | ||
| request.SessionId, | ||
| existingCheckIn.CheckedInAt, | ||
| "Already checked in to this session")); | ||
| } | ||
| return TypedResults.BadRequest($"Already checked in to session {request.SessionId}"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SqlException handling helpers already exist
rg -n --type=cs "SqlException|Number\s*==\s*2601|Number\s*==\s*2627" services Visage.SharedRepository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# Also search for any existing exception helper patterns
rg -n --type=cs "DbUpdateException" services Visage.Shared -A 3Repository: HackerspaceMumbai/Visage
Length of output: 855
🏁 Script executed:
# Verify the code exists at the specified lines in the file
fd -e cs "Program.cs" services/Visage.Services.Eventing/ -exec cat -n {} \; | sed -n '380,410p'Repository: HackerspaceMumbai/Visage
Length of output: 298
🏁 Script executed:
# Check for any constraint violation handling patterns in the repository
rg -n --type=cs "unique.*constraint|constraint.*violation" services Visage.Shared -iRepository: HackerspaceMumbai/Visage
Length of output: 294
🏁 Script executed:
# View context around line 275
sed -n '270,285p' services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 487
🏁 Script executed:
# View context around line 386 to match the review
sed -n '380,410p' services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 1294
🏁 Script executed:
# Count all DbUpdateException handlers in the file
rg -c "catch.*DbUpdateException" services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 69
🏁 Script executed:
# Check what database provider is being used
rg -n "UseSqlServer|UseNpgsql|UseSqlite" services/Visage.Services.Eventing/ -A 2 -B 2Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# Search for database configuration in the Program.cs file
sed -n '1,100p' services/Visage.Services.Eventing/Program.cs | rg -n "UseSqlServer|UseNpgsql|AddDbContext"Repository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# Check the DbContext definition and database provider
fd -e cs "EventDB|DbContext" services/Visage.Services.Eventing/ -exec grep -l "DbContext" {} \;Repository: HackerspaceMumbai/Visage
Length of output: 382
🏁 Script executed:
# Look for any existing helper methods or patterns in the codebase for exception handling
rg -n "static.*Result.*DbUpdateException|helper.*constraint|unique.*helper" services Visage.Shared --type=csRepository: HackerspaceMumbai/Visage
Length of output: 50
🏁 Script executed:
# View the EventDB.cs to see the database provider configuration
cat services/Visage.Services.Eventing/EventDB.csRepository: HackerspaceMumbai/Visage
Length of output: 5551
🏁 Script executed:
# Check the beginning of Program.cs for database setup
head -150 services/Visage.Services.Eventing/Program.cs | rg -n "UseSqlServer|UseNpgsql|AddDbContext|builder"Repository: HackerspaceMumbai/Visage
Length of output: 336
🏁 Script executed:
# Check if there are any existing patterns for extracting inner exceptions
rg -n "InnerException" services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 198
🏁 Script executed:
# Verify the SqlException import is available in the file
head -50 services/Visage.Services.Eventing/Program.csRepository: HackerspaceMumbai/Visage
Length of output: 1553
🏁 Script executed:
# Check if there are other constraint handlers that might need similar fixes
rg -n -B 5 -A 10 "DbUpdateException" services/Visage.Services.Eventing/Program.cs | head -60Repository: HackerspaceMumbai/Visage
Length of output: 1271
Replace message-based constraint detection with SQL Server error codes.
The current approach of filtering by InnerException?.Message.Contains("IX_SessionCheckIns_Registration_Session") is brittle and locale-dependent. Since the service uses SQL Server (configured via AddSqlServerDbContext), use the provider-specific error codes instead.
♻️ Suggested approach
+using Microsoft.Data.SqlClient;
catch (DbUpdateException ex) when (ex.InnerException is SqlException sql &&
- (sql.Number == 2601 || sql.Number == 2627))
+ (sql.Number == 2627))
{
// Unique constraint violation: already checked in
...
}Note: SQL Server uses error code 2627 for unique constraint violations. Code 2601 is for primary key violations.
🤖 Prompt for AI Agents
In `@services/Visage.Services.Eventing/Program.cs` around lines 386 - 405, Replace
the brittle message-based filter on the DbUpdateException with a
provider-specific SQL Server error code check: change the exception filter on
the DbUpdateException (the catch handling unique constraint for SessionCheckIns)
to inspect the underlying SqlException (via ex.GetBaseException() or
ex.InnerException) and test SqlException.Number for 2627 (unique constraint)
and/or 2601 (PK/duplicate index) instead of checking InnerException.Message;
ensure you reference the SqlException type from Microsoft.Data.SqlClient or
System.Data.SqlClient and keep the existing logic that looks up existingCheckIn
and returns the same TypedResults responses.
| // User profile creation/update endpoint | ||
| app.MapPost("/api/users", async Task<Results<Created<User>, Ok<User>, BadRequest>> ( | ||
| [FromBody] User inputUser, | ||
| UserDB db, | ||
| HttpContext httpContext, | ||
| ILogger<Program> logger) => | ||
| { | ||
| var auth0Subject = | ||
| httpContext.User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)?.Value | ||
| ?? httpContext.User.FindFirst("sub")?.Value; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(auth0Subject)) | ||
| { | ||
| logger.LogWarning("User upsert rejected: missing Auth0 subject claim"); | ||
| return TypedResults.BadRequest(); | ||
| } | ||
|
|
||
| inputUser.Auth0Subject = auth0Subject; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(inputUser.Email)) | ||
| { | ||
| logger.LogWarning("User upsert rejected: missing email address"); | ||
| return TypedResults.BadRequest(); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| inputUser.Email = inputUser.Email.Trim(); | ||
|
|
||
| // Find user by Auth0Subject to ensure authenticated ownership | ||
| var existing = await db.Users | ||
| .FirstOrDefaultAsync(u => u.Auth0Subject == auth0Subject); | ||
|
|
||
| if (existing is null) | ||
| { | ||
| inputUser.CreatedAt = DateTime.UtcNow; | ||
| await db.Users.AddAsync(inputUser); | ||
| await db.SaveChangesAsync(); | ||
| logger.LogInformation("Created user for {Email}", inputUser.Email); | ||
| return TypedResults.Created($"/api/users/{inputUser.Id}", inputUser); | ||
| } | ||
|
|
||
| existing.Auth0Subject = auth0Subject; | ||
|
|
||
| existing.FirstName = inputUser.FirstName; | ||
| existing.MiddleName = inputUser.MiddleName; | ||
| existing.LastName = inputUser.LastName; | ||
| existing.Email = inputUser.Email; | ||
| existing.MobileNumber = inputUser.MobileNumber; | ||
| existing.AddressLine1 = inputUser.AddressLine1; | ||
| existing.AddressLine2 = inputUser.AddressLine2; | ||
| existing.City = inputUser.City; | ||
| existing.State = inputUser.State; | ||
| existing.PostalCode = inputUser.PostalCode; | ||
| existing.GovtIdLast4Digits = inputUser.GovtIdLast4Digits; | ||
| existing.GovtIdType = inputUser.GovtIdType; | ||
| existing.OccupationStatus = inputUser.OccupationStatus; | ||
| existing.CompanyName = inputUser.CompanyName; | ||
| existing.EducationalInstituteName = inputUser.EducationalInstituteName; | ||
|
|
||
| // Social profiles (optional depending on occupation) | ||
| existing.LinkedInProfile = inputUser.LinkedInProfile; | ||
| existing.LinkedInVanityName = inputUser.LinkedInVanityName; | ||
| existing.LinkedInSubject = inputUser.LinkedInSubject; | ||
| existing.LinkedInRawProfileJson = inputUser.LinkedInRawProfileJson; | ||
| existing.LinkedInRawEmailJson = inputUser.LinkedInRawEmailJson; | ||
| existing.LinkedInPayloadFetchedAt = inputUser.LinkedInPayloadFetchedAt; | ||
|
|
||
| existing.GitHubProfile = inputUser.GitHubProfile; | ||
| existing.IsLinkedInVerified = inputUser.IsLinkedInVerified; | ||
| existing.IsGitHubVerified = inputUser.IsGitHubVerified; | ||
| existing.LinkedInVerifiedAt = inputUser.IsLinkedInVerified ? (inputUser.LinkedInVerifiedAt ?? existing.LinkedInVerifiedAt ?? DateTime.UtcNow) : null; | ||
| existing.GitHubVerifiedAt = inputUser.IsGitHubVerified ? (inputUser.GitHubVerifiedAt ?? existing.GitHubVerifiedAt ?? DateTime.UtcNow) : null; | ||
|
|
||
| existing.IsProfileComplete = true; | ||
| existing.ProfileCompletedAt = DateTime.UtcNow; | ||
| existing.UpdatedAt = DateTime.UtcNow; | ||
|
|
||
| await db.SaveChangesAsync(); | ||
| logger.LogInformation("Updated user for {Email}", existing.Email); | ||
| return TypedResults.Ok(existing); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "User upsert failed for {Email}", inputUser.Email); | ||
| return TypedResults.BadRequest(); | ||
| } | ||
| }).RequireAuthorization(); |
There was a problem hiding this comment.
Block client‑controlled social verification fields.
These endpoints accept and persist IsLinkedInVerified, IsGitHubVerified, verification timestamps, and raw LinkedIn payloads directly from the client. That allows self‑verification and payload injection. Keep these server‑managed and update them only via the verification workflow.
🔒 Remove client control over verification fields (apply to both /api/users and /register)
- existing.LinkedInRawProfileJson = inputUser.LinkedInRawProfileJson;
- existing.LinkedInRawEmailJson = inputUser.LinkedInRawEmailJson;
- existing.LinkedInPayloadFetchedAt = inputUser.LinkedInPayloadFetchedAt;
+ // Keep verification payloads server-managed (set via verification workflow)
existing.GitHubProfile = inputUser.GitHubProfile;
- existing.IsLinkedInVerified = inputUser.IsLinkedInVerified;
- existing.IsGitHubVerified = inputUser.IsGitHubVerified;
- existing.LinkedInVerifiedAt = inputUser.IsLinkedInVerified ? (inputUser.LinkedInVerifiedAt ?? existing.LinkedInVerifiedAt ?? DateTime.UtcNow) : null;
- existing.GitHubVerifiedAt = inputUser.IsGitHubVerified ? (inputUser.GitHubVerifiedAt ?? existing.GitHubVerifiedAt ?? DateTime.UtcNow) : null;
+ // Keep verification flags/timestamps server-managed (set via verification workflow)Also applies to: 398-486
Profile completion is forced to true regardless of required fields.
Line 238 / Line 473 set IsProfileComplete = true unconditionally. This can mark incomplete profiles as complete and bypass completion checks. Use ProfileCompletionRepository (or compute based on required fields) before setting completion flags.
✅ Compute completion from repository (apply to both endpoints)
-app.MapPost("/api/users", async Task<Results<Created<User>, Ok<User>, BadRequest>> (
- [FromBody] User inputUser,
- UserDB db,
- HttpContext httpContext,
- ILogger<Program> logger) =>
+app.MapPost("/api/users", async Task<Results<Created<User>, Ok<User>, BadRequest>> (
+ [FromBody] User inputUser,
+ UserDB db,
+ Visage.Services.UserProfile.Repositories.ProfileCompletionRepository profileCompletionRepository,
+ HttpContext httpContext,
+ ILogger<Program> logger) =>
{
...
- existing.IsProfileComplete = true;
- existing.ProfileCompletedAt = DateTime.UtcNow;
+ var completion = await profileCompletionRepository.GetCompletionStatusAsync(existing.Id);
+ existing.IsProfileComplete = completion.IsProfileComplete;
+ existing.ProfileCompletedAt = completion.IsProfileComplete ? DateTime.UtcNow : null;Also applies to: 398-486
🤖 Prompt for AI Agents
In `@Visage.Services.UserProfile/Program.cs` around lines 164 - 251, The handler
currently forces existing.IsProfileComplete = true and sets ProfileCompletedAt
unconditionally in the POST "/api/users" upsert (and likewise in the other
endpoint), which can mark incomplete profiles as complete; instead call into
ProfileCompletionRepository (or run the same completeness check logic used
elsewhere) to compute a boolean isComplete for the user (using fields on
inputUser/existing) and only set existing.IsProfileComplete = isComplete and
existing.ProfileCompletedAt = isComplete && existing.ProfileCompletedAt == null
? DateTime.UtcNow : existing.ProfileCompletedAt (or null when not complete);
apply the same change in the other user upsert/update endpoint so completeness
is derived, not hard-coded.
| // Event registration endpoint | ||
| app.MapPost("/api/registrations", async Task<Results<Created<EventRegistration>, BadRequest<string>>> ( | ||
| [FromBody] EventRegistration registration, | ||
| UserDB db, | ||
| HttpContext httpContext, | ||
| ILogger<Program> logger) => | ||
| { | ||
| var auth0Subject = | ||
| httpContext.User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)?.Value | ||
| ?? httpContext.User.FindFirst("sub")?.Value; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(auth0Subject)) | ||
| { | ||
| logger.LogWarning("Event registration rejected: missing Auth0 subject claim"); | ||
| return TypedResults.BadRequest("Authentication required"); | ||
| } | ||
|
|
||
| if (registration.EventId == default) | ||
| { | ||
| logger.LogWarning("Event registration rejected: missing EventId"); | ||
| return TypedResults.BadRequest("EventId is required"); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Resolve user by Auth0Subject to ensure authenticated ownership | ||
| var user = await db.Users.FirstOrDefaultAsync(u => u.Auth0Subject == auth0Subject); | ||
| if (user is null) | ||
| { | ||
| logger.LogWarning("Event registration rejected: User profile not found for Auth0Subject"); | ||
| return TypedResults.BadRequest("User profile not found"); | ||
| } | ||
|
|
||
| // Set UserId and Auth0Subject from authenticated user | ||
| registration.UserId = user.Id; | ||
| registration.Auth0Subject = auth0Subject; | ||
|
|
||
| // Check if already registered for this event | ||
| var existingRegistration = await db.EventRegistrations | ||
| .FirstOrDefaultAsync(r => r.UserId == registration.UserId && r.EventId == registration.EventId); | ||
|
|
||
| if (existingRegistration is not null) | ||
| { | ||
| logger.LogWarning("Event registration rejected: User {UserId} already registered for event {EventId}", | ||
| registration.UserId, registration.EventId); | ||
| return TypedResults.BadRequest("User is already registered for this event"); | ||
| } | ||
|
|
||
| registration.RegisteredAt = DateTime.UtcNow; | ||
| registration.Status = RegistrationStatus.Pending; | ||
|
|
||
| await db.EventRegistrations.AddAsync(registration); | ||
| await db.SaveChangesAsync(); | ||
|
|
||
| logger.LogInformation("User {UserId} registered for event {EventId}", registration.UserId, registration.EventId); | ||
| return TypedResults.Created($"/api/registrations/{registration.Id}", registration); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Event registration failed for User {UserId}, Event {EventId}", | ||
| registration.UserId, registration.EventId); | ||
| return TypedResults.BadRequest("Registration failed"); | ||
| } | ||
| }).RequireAuthorization(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find EventRegistration.cs file
find . -name "EventRegistration.cs" -type fRepository: HackerspaceMumbai/Visage
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for EventRegistration class definition
rg "class EventRegistration" -A 50 -B 2Repository: HackerspaceMumbai/Visage
Length of output: 8762
🏁 Script executed:
#!/bin/bash
# Read complete EventRegistration.cs from Visage.Shared
cat -n ./Visage.Shared/Models/EventRegistration.csRepository: HackerspaceMumbai/Visage
Length of output: 3577
🏁 Script executed:
#!/bin/bash
# Search for DTO patterns or CreateRegistrationRequest/Input models
rg "RegistrationRequest|RegistrationInput|CreateRegistration|RegisterDto" -i --type csRepository: HackerspaceMumbai/Visage
Length of output: 50
Use a DTO to prevent over-posting of server-managed fields.
Direct entity binding allows clients to set server-managed properties: RejectionReason, CheckInPin, ApprovedAt, and ApprovedBy. Although the endpoint overrides critical fields (UserId, Auth0Subject, RegisteredAt, Status), a DTO should whitelist only client-provided data (e.g., EventId, AdditionalDetails) to prevent mass-assignment vulnerabilities.
🤖 Prompt for AI Agents
In `@Visage.Services.UserProfile/Program.cs` around lines 280 - 343, The endpoint
directly binds the EventRegistration entity (registration) allowing clients to
over-post server-managed fields (RejectionReason, CheckInPin, ApprovedAt,
ApprovedBy); change the handler to accept a minimal DTO (e.g.,
EventRegistrationCreateDto with only EventId and any allowed AdditionalDetails),
map that DTO to a new EventRegistration instance server-side, then set UserId,
Auth0Subject, RegisteredAt, Status and any other server-controlled fields before
saving; update the MapPost signature to use the DTO and ensure any
client-provided fields are explicitly whitelisted during the mapping.
Introduce a UserProfile microservice to separate user profile management from event registration, enhancing modularity and scalability. Implement new APIs for user profiles and social verification, support multiple event registrations, and update the database schema accordingly. Add Azure Container Apps support for deployment and restrict access to the "Schedule Event" button for VisageAdmin users only. Update development environment configurations and clean up legacy code.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.