Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds SSO/SCIM localization across multiple languages, new SCIM bearer token validation returning owning department, stricter SCIM authorization and auditing, UI token provisioning/rotation flows, new Connect endpoint to resolve SSO by username, Postgres SQL identifier fixes, enum/interface additions, and supporting view/model/controller changes. Changes
Sequence DiagramsequenceDiagram
actor Client
participant ScimController as ScimController
participant DeptSso as DepartmentSsoService
participant AuditSvc as AuditService
participant DB as Database
Client->>ScimController: SCIM request (Bearer token + X-Resgrid-Department-Id)
ScimController->>ScimController: Validate headers & target user/department
alt Missing/Invalid headers
ScimController->>AuditSvc: Log ScimAuthFailed
ScimController-->>Client: 400/401
else
ScimController->>DeptSso: ValidateScimBearerTokenAndGetDepartmentAsync(token, claimedDeptId)
DeptSso->>DB: Load SCIM config for claimedDeptId
DeptSso->>DeptSso: Decrypt stored token & compare
alt Token invalid or dept mismatch
DeptSso-->>ScimController: null
ScimController->>AuditSvc: Log ScimAuthFailed
ScimController-->>Client: 401 Unauthorized
else Token valid
DeptSso-->>ScimController: departmentId
ScimController->>ScimController: Execute SCIM operation (list/get/create/patch/delete)
ScimController->>AuditSvc: SaveScimAuditAsync(operation details)
AuditSvc->>DB: Persist audit entry
ScimController-->>Client: 200 Success + SCIM response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs (1)
433-454:⚠️ Potential issue | 🟠 MajorThese methods currently report failure even when SQL execution succeeds.
CleanUpOIDCTokensAsync,ClearOutUserLoginAsync, andCleanUpOIDCTokensByUserAsyncall returnfalseunconditionally, so callers can’t distinguish success from failure.🔧 Proposed fix
@@ - return false; + return true; @@ - return false; + return result > 0; @@ - return false; + return true;Also applies to: 456-482, 484-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs` around lines 433 - 454, The three methods (CleanUpOIDCTokensAsync, ClearOutUserLoginAsync, CleanUpOIDCTokensByUserAsync) currently always return false; change them to return success based on the SQL execution result: capture the ExecuteAsync result in both Postgres and Sql branches and return true when the call completes (e.g., return result >= 0) or simply return true after successful ExecuteAsync; ensure both DB branches return that success value (or propagate the result check) instead of the unconditional "return false".
🧹 Nitpick comments (4)
Workers/Resgrid.Workers.Console/Program.cs (1)
441-442: Prefer using the providedIServiceProviderinstead of the global bootstrapper forIOidcRepository.The method already receives
IServiceProvider, but still resolves throughBootstrapper.GetKernel(). Using the provided provider removes the hidden dependency and improves testability.Proposed refactor
- var oidcRepository = Bootstrapper.GetKernel().Resolve<IOidcRepository>(); + var oidcRepository = serviceProvider.GetRequiredService<IOidcRepository>();As per coding guidelines, "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Program.cs` around lines 441 - 442, The code uses the global Bootstrapper.GetKernel().Resolve<IOidcRepository>() instead of the IServiceProvider passed into the method; replace that call by obtaining IOidcRepository from the provided IServiceProvider (use provider.GetRequiredService<IOidcRepository>() or GetService as appropriate) and then call UpdateOidcDatabaseAsync() on that instance (keep bool result = await UpdateOidcDatabaseAsync() logic intact), removing the hidden dependency on Bootstrapper.GetKernel().Core/Resgrid.Services/DepartmentSsoService.cs (1)
243-260: Consider consolidating SCIM token validation logic to one code path.
ValidateScimBearerTokenAsyncandValidateScimBearerTokenAndGetDepartmentAsyncnow duplicate core validation behavior; centralizing this would reduce drift risk.♻️ Refactor sketch
public async Task<bool> ValidateScimBearerTokenAsync(int departmentId, string bearerToken, string departmentCode, CancellationToken cancellationToken = default) { - try - { - var configs = await _ssoConfigRepository.GetAllByDepartmentIdAsync(departmentId); - var scimConfig = configs?.FirstOrDefault(c => c.ScimEnabled && !string.IsNullOrWhiteSpace(c.EncryptedScimBearerToken)); - if (scimConfig == null) - return false; - - var storedToken = _encryptionService.DecryptForDepartment(scimConfig.EncryptedScimBearerToken, departmentId, departmentCode); - return string.Equals(storedToken, bearerToken, StringComparison.Ordinal); - } - catch (Exception ex) - { - Logging.LogException(ex); - return false; - } + var ownerDepartmentId = await ValidateScimBearerTokenAndGetDepartmentAsync( + bearerToken, departmentId, departmentCode, cancellationToken); + return ownerDepartmentId == departmentId; }Also applies to: 269-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/DepartmentSsoService.cs` around lines 243 - 260, Both ValidateScimBearerTokenAsync and ValidateScimBearerTokenAndGetDepartmentAsync duplicate the same SCIM token lookup/decrypt/compare flow; extract that core logic into a single private helper (e.g., ValidateAndGetScimConfigAsync or GetDepartmentForScimTokenAsync) that uses _ssoConfigRepository to find ScimEnabled config, _encryptionService.DecryptForDepartment to decrypt EncryptedScimBearerToken, compares tokens with StringComparison.Ordinal, and returns either a boolean or the found config/department; then have ValidateScimBearerTokenAsync and ValidateScimBearerTokenAndGetDepartmentAsync call that helper and preserve existing exception handling via Logging.LogException to maintain current behavior.Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs (1)
381-419: SSO response composition is duplicated and likely to drift.This endpoint repeats nearly the same result-shaping logic as
GetSsoConfig; extracting a shared helper would keep behavior consistent across both flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs` around lines 381 - 419, Duplicate SSO response shaping in ConnectController should be extracted into a single helper used by both ConnectController.GetSsoConfig and the existing GetSsoConfig flow: create a method like MapSsoConfigToResponse (on ConnectController or a shared service) that accepts departmentId, SsoConfig (activeConfig) and SecurityPolicy (policy) and returns the populated response DTO with fields SsoEnabled, ProviderType, AllowLocalLogin, RequireSso, RequireMfa and provider-specific fields (Authority, ClientId, OidcRedirectUri, OidcScopes, MetadataUrl, EntityId); replace the inline logic in ConnectController (the block using _departmentSsoService.GetSsoConfigsForDepartmentAsync, GetSecurityPolicyForDepartmentAsync and the subsequent field assignments) with a call to that helper and do the same in GetSsoConfig so both flows share identical mapping and null handling.Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs (1)
891-916: Extract shared SSO index model construction to a helper.The
SsoIndexViewmapping here duplicates the mapping in theSsoaction, which increases drift risk when fields evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs` around lines 891 - 916, Extract the duplicated SsoIndexView construction into a single private helper on SecurityController (e.g., BuildSsoIndexView or CreateSsoIndexModel) that accepts the required inputs (Department department, IEnumerable<DepartmentSsoConfig> configList, string newToken, Guid newConfigId, string apiBase, string encryptedToken) and returns the populated SsoIndexView; move the current mapping logic (including the SsoConfigRowView Select, HasOidcConfig/HasSamlConfig checks, EncryptedDepartmentToken, ScimBaseUrl, ApiBaseUrl, SsoDiscoveryUrl) into that helper and replace the inline construction in this action and the Sso action with calls to the new helper so both use the same centralized mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 462-466: In IdentityRepository.cs update the PostgreSQL UPDATE so
it doesn't use SQL Server '+' concatenation: change the db.ExecuteAsync call
that assigns to result (the SQL using `@deleteid` + '@resgrid.del') to use
PostgreSQL concatenation (||) or, better, compute the masked email in C# and
pass it as a separate parameter; update the anonymous parameter object
(currently new { userId = userId, deleteid = Guid.NewGuid().ToString() }) to
include the prebuilt email (e.g., maskedEmail) and use that parameter in the SQL
for the email column, leaving username = `@deleteid` and the WHERE id = `@userId`
intact.
In `@Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs`:
- Around line 377-421: AuthorizeScimRequestAsync currently validates the bearer
token and department but does not confirm that an optional targetUserId belongs
to that department; update AuthorizeScimRequestAsync to, when targetUserId is
not null/empty, fetch the user (e.g. via your user service method like
GetUserByIdAsync on _usersService or equivalent) and verify the returned user's
DepartmentId matches departmentId; if the user is missing or the department
differs, call SaveScimAuditAsync with AuditLogTypes.ScimAuthFailed and return
false to deny the request and log the failure. Ensure you handle null user
returns and include the targetUserId in the audit data for clarity.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 341-344: The XML docs are missing a description for the
cancellationToken param; update the <param name="cancellationToken"> element in
the same member that contains <param name="departmentToken"> and <param
name="departmentCode"> to explain that it is a
System.Threading.CancellationToken used to cancel the async operation (e.g., "A
cancellation token to cancel the request/operation"). Keep the wording concise
and consistent with other XML doc comments in Resgrid.Web.Services.
- Around line 886-891: Update the XML docs for the SaveScimAuditAsync method
(Resgrid.Web.Services.Controllers.v4.ScimController.SaveScimAuditAsync) to
include an explicit redaction/data policy for the "data" parameter: state that
audit payloads must not contain secrets (bearer tokens, API keys), sensitive
personal data (PII such as emails, SSNs, dates of birth) or full user content,
require redaction or truncation of such fields before calling
SaveScimAuditAsync, and suggest alternatives (store metadata, hashed
identifiers, or audit-only flags) and mention using a centralized
redaction/serialization helper to remove tokens and PII prior to persistence.
In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs`:
- Around line 868-919: After calling await
_ssoService.SaveSsoConfigAsync(config, department.Code, cancellationToken) add
an audit event recording the SCIM token provision/rotation: call your existing
audit/event logger (e.g. _auditService or _auditLogService) from this
SecurityController action and write an entry with actor = UserId, target
department = DepartmentId (and department.Code), target config id = id
(DepartmentSsoConfigId), action = "ScimBearerTokenProvisioned" or
"ScimBearerTokenRotated", and a timestamp; do NOT log the raw token value
(newToken), just that it was rotated/provisioned and which config and user
performed it. Ensure the audit call happens after SaveSsoConfigAsync and before
returning the view so the audit reflects the persisted change.
- Around line 856-858: The GenerateScimTokenFromSso action modifies
authentication credentials but lacks step-up authentication; add the
RequiresRecentTwoFactor attribute to the GenerateScimTokenFromSso method (and
mirror the same attribute on any other token-rotation endpoints) so the
framework enforces recent 2FA verification before executing this POST action;
locate the method by name GenerateScimTokenFromSso in SecurityController.cs and
apply RequiresRecentTwoFactor to the method signature to restore parity with
other token rotation paths.
In `@Web/Resgrid.Web/Areas/User/Views/Security/Sso.cshtml`:
- Around line 221-223: Replace the malformed sample emails that use a double-@
(e.g., "jane.doe@@example.com") with valid single-@ addresses in the Sso.cshtml
request examples; update the "userName" values and the "emails" array entries
(emails[].value) at both example blocks (the occurrences around the current
"userName" and "emails" JSON objects) to use "jane.doe@example.com" (or another
valid single-@ sample) so copy/paste testing isn't broken.
- Line 207: Replace the hardcoded example request headings in Sso.cshtml (the
strings "List users (GET)" and the other three English labels on lines shown)
with resource lookups: add new resource keys (e.g., Security_ListUsers_Get,
Security_<other_key_names>) to the view/resource file and render them in the
Razor view via the project's existing localization access (use the same pattern
the app uses for other views, e.g., ResourceClass.Key or `@Html.Localize`("Key")).
Ensure you update all four occurrences in Sso.cshtml so the labels come from
those resource keys rather than hardcoded English text.
- Around line 474-489: The modal always shows rotateConfirmMsg in
confirmProvisionToken; change confirmProvisionToken to set
document.getElementById('provisionTokenModalMessage').textContent based on
whether isRotate is true or false (use rotateConfirmMsg when isRotate, otherwise
use a provisionConfirmMsg localized string), and ensure provisionConfirmMsg is
defined (e.g., mirror how rotateConfirmMsg is created from
localizer["SsoScimTokenModalProvisionConfirm"].Value) so first-time provisioning
shows the correct warning text; also keep setting provisionTokenSubmitLabel to
isRotate ? rotateLabel : provisionLabel.
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 439-450: The current try/catch around
IOidcRepository.UpdateOidcDatabaseAsync swallows failures and always logs
"Completed updating the OIDC Database!"; change the flow so failures are
propagated or handled and the success message is only emitted when the update
actually succeeded: call UpdateOidcDatabaseAsync(), capture its boolean result,
log success only if result is true, and in the catch block log the exception
with error level and rethrow or exit with a non-zero status so the process does
not report success; reference the UpdateOidcDatabaseAsync method on
IOidcRepository and the logger used in Program.cs to implement these changes.
---
Outside diff comments:
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 433-454: The three methods (CleanUpOIDCTokensAsync,
ClearOutUserLoginAsync, CleanUpOIDCTokensByUserAsync) currently always return
false; change them to return success based on the SQL execution result: capture
the ExecuteAsync result in both Postgres and Sql branches and return true when
the call completes (e.g., return result >= 0) or simply return true after
successful ExecuteAsync; ensure both DB branches return that success value (or
propagate the result check) instead of the unconditional "return false".
---
Nitpick comments:
In `@Core/Resgrid.Services/DepartmentSsoService.cs`:
- Around line 243-260: Both ValidateScimBearerTokenAsync and
ValidateScimBearerTokenAndGetDepartmentAsync duplicate the same SCIM token
lookup/decrypt/compare flow; extract that core logic into a single private
helper (e.g., ValidateAndGetScimConfigAsync or GetDepartmentForScimTokenAsync)
that uses _ssoConfigRepository to find ScimEnabled config,
_encryptionService.DecryptForDepartment to decrypt EncryptedScimBearerToken,
compares tokens with StringComparison.Ordinal, and returns either a boolean or
the found config/department; then have ValidateScimBearerTokenAsync and
ValidateScimBearerTokenAndGetDepartmentAsync call that helper and preserve
existing exception handling via Logging.LogException to maintain current
behavior.
In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs`:
- Around line 381-419: Duplicate SSO response shaping in ConnectController
should be extracted into a single helper used by both
ConnectController.GetSsoConfig and the existing GetSsoConfig flow: create a
method like MapSsoConfigToResponse (on ConnectController or a shared service)
that accepts departmentId, SsoConfig (activeConfig) and SecurityPolicy (policy)
and returns the populated response DTO with fields SsoEnabled, ProviderType,
AllowLocalLogin, RequireSso, RequireMfa and provider-specific fields (Authority,
ClientId, OidcRedirectUri, OidcScopes, MetadataUrl, EntityId); replace the
inline logic in ConnectController (the block using
_departmentSsoService.GetSsoConfigsForDepartmentAsync,
GetSecurityPolicyForDepartmentAsync and the subsequent field assignments) with a
call to that helper and do the same in GetSsoConfig so both flows share
identical mapping and null handling.
In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs`:
- Around line 891-916: Extract the duplicated SsoIndexView construction into a
single private helper on SecurityController (e.g., BuildSsoIndexView or
CreateSsoIndexModel) that accepts the required inputs (Department department,
IEnumerable<DepartmentSsoConfig> configList, string newToken, Guid newConfigId,
string apiBase, string encryptedToken) and returns the populated SsoIndexView;
move the current mapping logic (including the SsoConfigRowView Select,
HasOidcConfig/HasSamlConfig checks, EncryptedDepartmentToken, ScimBaseUrl,
ApiBaseUrl, SsoDiscoveryUrl) into that helper and replace the inline
construction in this action and the Sso action with calls to the new helper so
both use the same centralized mapping.
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 441-442: The code uses the global
Bootstrapper.GetKernel().Resolve<IOidcRepository>() instead of the
IServiceProvider passed into the method; replace that call by obtaining
IOidcRepository from the provided IServiceProvider (use
provider.GetRequiredService<IOidcRepository>() or GetService as appropriate) and
then call UpdateOidcDatabaseAsync() on that instance (keep bool result = await
UpdateOidcDatabaseAsync() logic intact), removing the hidden dependency on
Bootstrapper.GetKernel().
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3f11765-8253-456d-9430-4e5c08f48ae4
📒 Files selected for processing (21)
Core/Resgrid.Localization/Areas/User/Security/Security.de.resxCore/Resgrid.Localization/Areas/User/Security/Security.en.resxCore/Resgrid.Localization/Areas/User/Security/Security.es.resxCore/Resgrid.Localization/Areas/User/Security/Security.fr.resxCore/Resgrid.Localization/Areas/User/Security/Security.it.resxCore/Resgrid.Localization/Areas/User/Security/Security.pl.resxCore/Resgrid.Localization/Areas/User/Security/Security.sv.resxCore/Resgrid.Localization/Areas/User/Security/Security.uk.resxCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/Services/IDepartmentSsoService.csCore/Resgrid.Services/DepartmentSsoService.csRepositories/Resgrid.Repositories.DataRepository/IdentityRepository.csWeb/Resgrid.Web.Services/Controllers/v4/ConnectController.csWeb/Resgrid.Web.Services/Controllers/v4/ScimController.csWeb/Resgrid.Web.Services/Properties/launchSettings.jsonWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/appsettings.jsonWeb/Resgrid.Web/Areas/User/Controllers/SecurityController.csWeb/Resgrid.Web/Areas/User/Models/Security/SsoViews.csWeb/Resgrid.Web/Areas/User/Views/Security/Sso.cshtmlWorkers/Resgrid.Workers.Console/Program.cs
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+341
to
+344
| <param name="departmentToken">An encrypted department token produced by the web UI.</param> | ||
| <param name="departmentCode">The department code (name) shown on the department's settings page.</param> | ||
| <param name="cancellationToken"></param> | ||
| </member> |
Contributor
There was a problem hiding this comment.
Fill the missing cancellation token docs.
Line 343 leaves <param name="cancellationToken"></param> empty, which weakens generated API documentation quality.
Suggested doc fix
- <param name="cancellationToken"></param>
+ <param name="cancellationToken">The cancellation token that can be used by other objects or threads to receive notice of cancellation.</param>📝 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.
Suggested change
| <param name="departmentToken">An encrypted department token produced by the web UI.</param> | |
| <param name="departmentCode">The department code (name) shown on the department's settings page.</param> | |
| <param name="cancellationToken"></param> | |
| </member> | |
| <param name="departmentToken">An encrypted department token produced by the web UI.</param> | |
| <param name="departmentCode">The department code (name) shown on the department's settings page.</param> | |
| <param name="cancellationToken">The cancellation token that can be used by other objects or threads to receive notice of cancellation.</param> | |
| </member> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 341 - 344,
The XML docs are missing a description for the cancellationToken param; update
the <param name="cancellationToken"> element in the same member that contains
<param name="departmentToken"> and <param name="departmentCode"> to explain that
it is a System.Threading.CancellationToken used to cancel the async operation
(e.g., "A cancellation token to cancel the request/operation"). Keep the wording
concise and consistent with other XML doc comments in Resgrid.Web.Services.
Comment on lines
+886
to
+891
| <member name="M:Resgrid.Web.Services.Controllers.v4.ScimController.SaveScimAuditAsync(System.Int32,System.String,Resgrid.Model.AuditLogTypes,System.Boolean,System.String)"> | ||
| <summary> | ||
| Persists a SCIM system audit record. The <paramref name="data"/> field carries | ||
| structured contextual information about the operation to support forensic review. | ||
| </summary> | ||
| </member> |
Contributor
There was a problem hiding this comment.
Add explicit redaction guidance for SCIM audit payloads.
Line 888–889 encourages structured audit data, but without a “no secrets/PII” constraint this can lead to bearer token or personal-data leakage in logs.
Suggested doc hardening
<member name="M:Resgrid.Web.Services.Controllers.v4.ScimController.SaveScimAuditAsync(System.Int32,System.String,Resgrid.Model.AuditLogTypes,System.Boolean,System.String)">
<summary>
Persists a SCIM system audit record. The <paramref name="data"/> field carries
structured contextual information about the operation to support forensic review.
+ Do not include secrets (for example bearer tokens, passwords, private keys) or raw PII in <paramref name="data"/>.
</summary>
</member>📝 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.
Suggested change
| <member name="M:Resgrid.Web.Services.Controllers.v4.ScimController.SaveScimAuditAsync(System.Int32,System.String,Resgrid.Model.AuditLogTypes,System.Boolean,System.String)"> | |
| <summary> | |
| Persists a SCIM system audit record. The <paramref name="data"/> field carries | |
| structured contextual information about the operation to support forensic review. | |
| </summary> | |
| </member> | |
| <member name="M:Resgrid.Web.Services.Controllers.v4.ScimController.SaveScimAuditAsync(System.Int32,System.String,Resgrid.Model.AuditLogTypes,System.Boolean,System.String)"> | |
| <summary> | |
| Persists a SCIM system audit record. The <paramref name="data"/> field carries | |
| structured contextual information about the operation to support forensic review. | |
| Do not include secrets (for example bearer tokens, passwords, private keys) or raw PII in <paramref name="data"/>. | |
| </summary> | |
| </member> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 886 - 891,
Update the XML docs for the SaveScimAuditAsync method
(Resgrid.Web.Services.Controllers.v4.ScimController.SaveScimAuditAsync) to
include an explicit redaction/data policy for the "data" parameter: state that
audit payloads must not contain secrets (bearer tokens, API keys), sensitive
personal data (PII such as emails, SSNs, dates of birth) or full user content,
require redaction or truncation of such fields before calling
SaveScimAuditAsync, and suggest alternatives (store metadata, hashed
identifiers, or audit-only flags) and mention using a centralized
redaction/serialization helper to remove tokens and PII prior to persistence.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs (1)
433-454:⚠️ Potential issue | 🟠 Major
Task<bool>methods always returnfalseeven when DB updates succeed.
CleanUpOIDCTokensAsync,ClearOutUserLoginAsync, andCleanUpOIDCTokensByUserAsynccapture affected rows but then always returnfalse. This can make successful operations look like failures to callers.Suggested fix
public async Task<bool> CleanUpOIDCTokensAsync(DateTime timestamp) { if (OidcConfig.DatabaseType == DatabaseTypes.Postgres) { using (IDbConnection db = new NpgsqlConnection(OidcConfig.ConnectionString)) { - var result = await db.ExecuteAsync(@"DELETE FROM ""OpenIddictTokens"" WHERE ""ExpirationDate"" < `@timestamp`", + var result = await db.ExecuteAsync(@"DELETE FROM ""OpenIddictTokens"" WHERE ""ExpirationDate"" < `@timestamp`", new { timestamp = timestamp }); + return result > 0; } } else { using (IDbConnection db = new SqlConnection(OidcConfig.ConnectionString)) { var result = await db.ExecuteAsync(@"DELETE FROM OpenIddictTokens WHERE ExpirationDate < `@timestamp`", new { timestamp = timestamp }); + return result > 0; } } - - return false; } public async Task<bool> ClearOutUserLoginAsync(string userId) { if (DataConfig.DatabaseType == DatabaseTypes.Postgres) { using (IDbConnection db = new NpgsqlConnection(DataConfig.CoreConnectionString)) { var deleteId = Guid.NewGuid().ToString(); var maskedEmail = deleteId + "@resgrid.del"; var result = await db.ExecuteAsync(@"UPDATE public.aspnetusers SET username = `@deleteId`, email = `@maskedEmail` WHERE id = `@userId`", new { userId = userId, deleteId = deleteId, maskedEmail = maskedEmail }); + return result > 0; } } else { using (IDbConnection db = new SqlConnection(DataConfig.CoreConnectionString)) { var result = await db.ExecuteAsync(@"UPDATE AspNetUsers SET UserName = `@deleteid`, Email = `@deleteid` + '@resgrid.del' WHERE Id = `@userId`", new { userId = userId, deleteid = Guid.NewGuid().ToString() }); + return result > 0; } } - - return false; } public async Task<bool> CleanUpOIDCTokensByUserAsync(string userId) { if (OidcConfig.DatabaseType == DatabaseTypes.Postgres) { using (IDbConnection db = new NpgsqlConnection(OidcConfig.ConnectionString)) { var result = await db.ExecuteAsync(@"DELETE FROM ""OpenIddictTokens"" WHERE ""Subject"" = `@userId`", new { userId = userId }); + return result > 0; } } else { using (IDbConnection db = new SqlConnection(OidcConfig.ConnectionString)) { var result = await db.ExecuteAsync(@"DELETE FROM OpenIddictTokens WHERE Subject = `@userId`", new { userId = userId }); + return result > 0; } } - - return false; }Also applies to: 456-484, 486-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs` around lines 433 - 454, The methods CleanUpOIDCTokensAsync, ClearOutUserLoginAsync, and CleanUpOIDCTokensByUserAsync currently ignore the DB ExecuteAsync return value and always return false; change each to capture the ExecuteAsync result (the existing result variable) and return a boolean indicating success (e.g., return result > 0) after both Postgres and Sql branches so callers get true when rows were affected; ensure the method signatures remain Task<bool> and use the existing 'result' local in each method to form the return.Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs (1)
437-461:⚠️ Potential issue | 🟠 Major
activefield is hardcoded totrue, breaking SCIM sync semantics.Line 444 hardcodes
active = trueregardless of the user's actualIsDisabledorIsDeletedstatus. When an IdP retrieves a user viaGET /Users/{id}or lists users, it will always seeactive=trueeven for disabled/deleted members.This breaks SCIM provisioning workflows because the IdP cannot detect when Resgrid has deactivated a user, potentially causing sync drift.
🐛 Proposed fix
-private static object BuildScimUser(Model.Identity.IdentityUser user, UserProfile profile) +private static object BuildScimUser(Model.Identity.IdentityUser user, UserProfile profile, bool isActive = true) { return new { schemas = new[] { "urn:ietf:params:scim:schemas:core:2.0:User" }, id = user.Id, userName = user.UserName, - active = true, + active = isActive, name = new {Then update callers to pass the actual active state:
// In GetUser: var member = await _departmentsService.GetDepartmentMemberAsync(id, departmentId); var isActive = member == null || (!member.IsDisabled && !member.IsDeleted); return Ok(BuildScimUser(user, profile, isActive)); // In GetUsers (inside foreach): var member = await _departmentsService.GetDepartmentMemberAsync(u.Id, departmentId); var isActive = member == null || (!member.IsDisabled && !member.IsDeleted); scimUsers.Add(BuildScimUser(u, profile, isActive));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs` around lines 437 - 461, The BuildScimUser method currently hardcodes active=true; change BuildScimUser(Model.Identity.IdentityUser user, UserProfile profile) to accept a bool isActive parameter and set active = isActive inside the returned object, then update callers (e.g., GetUser and the foreach in GetUsers) to compute isActive as member == null || (!member.IsDisabled && !member.IsDeleted) by retrieving the department member via _departmentsService.GetDepartmentMemberAsync(...) and pass that isActive into BuildScimUser so SCIM responses reflect the user's real active state.
♻️ Duplicate comments (1)
Workers/Resgrid.Workers.Console/Program.cs (1)
444-447:⚠️ Potential issue | 🟠 MajorTreat
falsefrom OIDC upgrade as a hard failure.At Line 444-447,
result == falseonly emits a warning, so the upgrade flow can still report completion. Please throw whenUpdateOidcDatabaseAsync()returns false so failure is propagated consistently.Suggested fix
- if (result) - logger.Log(LogLevel.Information, "Completed updating the OIDC Database!"); - else - logger.Log(LogLevel.Warning, "UpdateOidcDatabaseAsync returned false; the OIDC database may not have been fully updated."); + if (!result) + throw new InvalidOperationException("OIDC database upgrade reported failure."); + + logger.Log(LogLevel.Information, "Completed updating the OIDC Database!");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Program.cs` around lines 444 - 447, The current branch treats a false return from UpdateOidcDatabaseAsync as a warning; change this to throw an exception so failures propagate: after calling UpdateOidcDatabaseAsync (the call returning result), if result is false, log an error via logger.LogError with context and then throw an InvalidOperationException (or similar) indicating the OIDC database upgrade failed; otherwise keep the existing success log. Ensure the thrown exception occurs instead of just logging a warning so the upgrade flow surfaces the failure.
🧹 Nitpick comments (2)
Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs (1)
284-286: Clarify the defensiveIsDeleted = falseassignment.Line 285 sets
IsDeleted = falsewhen deactivating (!active). While the inline comment explains the intent, the condition placement is slightly confusing since it's insideif (!active)but assignsfalsetoIsDeleted.Consider a clearer comment or restructuring:
if (member != null) { member.IsDisabled = !active; - if (!active) member.IsDeleted = false; // deactivate without hard-delete + // Deactivation (active=false) should only disable, not hard-delete. + // Clear IsDeleted to ensure the user remains recoverable. + if (!active) + member.IsDeleted = false; await _departmentsService.SaveDepartmentMemberAsync(member, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs` around lines 284 - 286, The conditional assignment member.IsDeleted = false inside the if (!active) block is confusing; clarify intent by either moving member.IsDeleted = false next to member.IsDisabled = !active (so both properties are set together) or by changing the inline comment to explicitly state "ensure member is marked not-deleted when deactivating to avoid hard-delete" and keep the assignment in the if-block; update the code around member.IsDisabled, member.IsDeleted, and the call to SaveDepartmentMemberAsync to reflect the clearer structure/comment.Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs (1)
911-936: Consider extracting shared view model construction.The
SsoIndexViewconstruction logic here duplicates lines 631-654 in theSso()method. A private helper method could reduce duplication and ensure consistency.♻️ Example extraction
private async Task<SsoIndexView> BuildSsoIndexViewAsync( Department department, IEnumerable<DepartmentSsoConfig> configs, string newScimBearerToken = null, string newScimConfigId = null) { var configList = configs?.ToList() ?? new List<DepartmentSsoConfig>(); var plainToken = $"{department.DepartmentId}:{department.Code}"; var encryptedToken = _encryptionService.EncryptForDepartment(plainToken, department.DepartmentId, department.Code); var apiBase = Config.SystemBehaviorConfig.ResgridApiBaseUrl; return new SsoIndexView { IsAdmin = true, DepartmentId = department.DepartmentId, NewScimBearerToken = newScimBearerToken, NewScimConfigId = newScimConfigId, Configs = configList.Select(c => new SsoConfigRowView { /* ... */ }).ToList(), // ... remaining properties }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs` around lines 911 - 936, The SsoIndexView construction is duplicated (lines near Sso() and again here); extract the shared logic into a private helper like BuildSsoIndexViewAsync(Department department, IEnumerable<DepartmentSsoConfig> configs, string newScimBearerToken = null, string newScimConfigId = null) and have both Sso() and this controller action call it; the helper should compute plain/encrypted token, apiBase, map configs to SsoConfigRowView (preserving ProviderType, Identifier, EndpointUrl, flags, CreatedOn), and set HasOidcConfig/HasSamlConfig, ScimBaseUrl, ApiBaseUrl, SsoDiscoveryUrl, NewScimBearerToken and NewScimConfigId so the duplicated block in SecurityController.cs can be replaced with a single call to BuildSsoIndexViewAsync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Localization/Areas/User/Security/Security.en.resx`:
- Around line 314-320: Add the missing resource key
SsoScimTokenModalProvisionConfirm to the Security.en.resx with a user-facing
confirmation string (the view expects
localizer["SsoScimTokenModalProvisionConfirm"]); e.g., add a <data
name="SsoScimTokenModalProvisionConfirm" ...><value>Provisioning will apply the
new token and may interrupt active SCIM syncs. Continue?</value></data> so the
Sso.cshtml modal displays the proper localized text instead of the key name.
In `@Core/Resgrid.Model/AuditLogTypes.cs`:
- Around line 96-105: The AuditLog enum gained SCIM entries but
AuditService.GetAuditLogTypeString is missing switch cases for them, causing
blank labels; update GetAuditLogTypeString to add case branches for
ScimUserDeactivated, ScimUserDeleted, ScimAuthFailed, ScimUserReactivated,
ScimGroupListed, ScimUserListed, ScimUserRetrieved, ScimBearerTokenProvisioned
and ScimBearerTokenRotated that return appropriate human-readable strings (e.g.,
"SCIM User Deactivated", "SCIM User Deleted", "SCIM Auth Failed", etc.),
ensuring each enum member in the AuditLogTypes list is represented by a
corresponding case in the switch so the UI shows meaningful labels.
In `@Web/Resgrid.Web/Areas/User/Views/Security/Sso.cshtml`:
- Line 475: Add the missing localization key "SsoScimTokenModalProvisionConfirm"
to all Security.*.resx resource files so
localizer["SsoScimTokenModalProvisionConfirm"] in the Sso.cshtml view returns a
proper string; open each Security.*.resx (all cultures), add the
SsoScimTokenModalProvisionConfirm entry with the appropriate default English
value and translated values for other cultures, then rebuild so the view uses
the localized provision confirmation message instead of the raw key.
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 435-442: The UpdateOidcDatabaseAsync method currently takes an
unused serviceProvider and resolves IOidcRepository via
Bootstrapper.GetKernel(), creating a hidden dependency; either remove the
IServiceProvider serviceProvider parameter from UpdateOidcDatabaseAsync and
callers, or change the method to resolve IOidcRepository from the provided
serviceProvider (e.g., serviceProvider.GetRequiredService<IOidcRepository>())
and stop using Bootstrapper.GetKernel(), ensuring the unique symbols to update
are UpdateOidcDatabaseAsync, serviceProvider, IOidcRepository, and
Bootstrapper.GetKernel().
---
Outside diff comments:
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 433-454: The methods CleanUpOIDCTokensAsync,
ClearOutUserLoginAsync, and CleanUpOIDCTokensByUserAsync currently ignore the DB
ExecuteAsync return value and always return false; change each to capture the
ExecuteAsync result (the existing result variable) and return a boolean
indicating success (e.g., return result > 0) after both Postgres and Sql
branches so callers get true when rows were affected; ensure the method
signatures remain Task<bool> and use the existing 'result' local in each method
to form the return.
In `@Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs`:
- Around line 437-461: The BuildScimUser method currently hardcodes active=true;
change BuildScimUser(Model.Identity.IdentityUser user, UserProfile profile) to
accept a bool isActive parameter and set active = isActive inside the returned
object, then update callers (e.g., GetUser and the foreach in GetUsers) to
compute isActive as member == null || (!member.IsDisabled && !member.IsDeleted)
by retrieving the department member via
_departmentsService.GetDepartmentMemberAsync(...) and pass that isActive into
BuildScimUser so SCIM responses reflect the user's real active state.
---
Duplicate comments:
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 444-447: The current branch treats a false return from
UpdateOidcDatabaseAsync as a warning; change this to throw an exception so
failures propagate: after calling UpdateOidcDatabaseAsync (the call returning
result), if result is false, log an error via logger.LogError with context and
then throw an InvalidOperationException (or similar) indicating the OIDC
database upgrade failed; otherwise keep the existing success log. Ensure the
thrown exception occurs instead of just logging a warning so the upgrade flow
surfaces the failure.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ScimController.cs`:
- Around line 284-286: The conditional assignment member.IsDeleted = false
inside the if (!active) block is confusing; clarify intent by either moving
member.IsDeleted = false next to member.IsDisabled = !active (so both properties
are set together) or by changing the inline comment to explicitly state "ensure
member is marked not-deleted when deactivating to avoid hard-delete" and keep
the assignment in the if-block; update the code around member.IsDisabled,
member.IsDeleted, and the call to SaveDepartmentMemberAsync to reflect the
clearer structure/comment.
In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs`:
- Around line 911-936: The SsoIndexView construction is duplicated (lines near
Sso() and again here); extract the shared logic into a private helper like
BuildSsoIndexViewAsync(Department department, IEnumerable<DepartmentSsoConfig>
configs, string newScimBearerToken = null, string newScimConfigId = null) and
have both Sso() and this controller action call it; the helper should compute
plain/encrypted token, apiBase, map configs to SsoConfigRowView (preserving
ProviderType, Identifier, EndpointUrl, flags, CreatedOn), and set
HasOidcConfig/HasSamlConfig, ScimBaseUrl, ApiBaseUrl, SsoDiscoveryUrl,
NewScimBearerToken and NewScimConfigId so the duplicated block in
SecurityController.cs can be replaced with a single call to
BuildSsoIndexViewAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8984a3c6-ab8e-4919-8e95-658ce6c7b0be
📒 Files selected for processing (14)
Core/Resgrid.Localization/Areas/User/Security/Security.de.resxCore/Resgrid.Localization/Areas/User/Security/Security.en.resxCore/Resgrid.Localization/Areas/User/Security/Security.es.resxCore/Resgrid.Localization/Areas/User/Security/Security.fr.resxCore/Resgrid.Localization/Areas/User/Security/Security.it.resxCore/Resgrid.Localization/Areas/User/Security/Security.pl.resxCore/Resgrid.Localization/Areas/User/Security/Security.sv.resxCore/Resgrid.Localization/Areas/User/Security/Security.uk.resxCore/Resgrid.Model/AuditLogTypes.csRepositories/Resgrid.Repositories.DataRepository/IdentityRepository.csWeb/Resgrid.Web.Services/Controllers/v4/ScimController.csWeb/Resgrid.Web/Areas/User/Controllers/SecurityController.csWeb/Resgrid.Web/Areas/User/Views/Security/Sso.cshtmlWorkers/Resgrid.Workers.Console/Program.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Resgrid.Localization/Areas/User/Security/Security.sv.resx
| <script> | ||
| var deleteConfirmTemplate = @Html.Raw(Newtonsoft.Json.JsonConvert.SerializeObject(localizer["SsoDeleteConfirmMessage"].Value)); | ||
| var rotateConfirmMsg = @Html.Raw(Newtonsoft.Json.JsonConvert.SerializeObject(localizer["SsoScimTokenModalRotateConfirm"].Value)); | ||
| var provisionConfirmMsg = @Html.Raw(Newtonsoft.Json.JsonConvert.SerializeObject(localizer["SsoScimTokenModalProvisionConfirm"].Value)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the SsoScimTokenModalProvisionConfirm key in all Security.*.resx files
echo "Searching for SsoScimTokenModalProvisionConfirm in localization files..."
fd -e resx -name "Security*" | xargs grep -l "SsoScimTokenModalProvisionConfirm" 2>/dev/null || echo "Key NOT FOUND in any file"
echo ""
echo "Listing all SsoScimTokenModal* keys found:"
fd -e resx -name "Security*" | xargs grep -o 'name="SsoScimTokenModal[^"]*"' 2>/dev/null | sort -uRepository: Resgrid/Core
Length of output: 677
🏁 Script executed:
#!/bin/bash
# Search for SsoScimTokenModalProvisionConfirm in all resx files
echo "Searching for SsoScimTokenModalProvisionConfirm in resx files..."
find . -name "Security*.resx" -type f -exec grep -l "SsoScimTokenModalProvisionConfirm" {} \; 2>/dev/null || echo "Key NOT FOUND"
echo ""
echo "All SsoScimTokenModal* keys in Security resx files:"
find . -name "Security*.resx" -type f -exec grep "name=\"SsoScimTokenModal" {} + 2>/dev/null | grep -o 'name="[^"]*"' | sort -uRepository: Resgrid/Core
Length of output: 273
Add missing localization key SsoScimTokenModalProvisionConfirm to all Security.*.resx files.
Line 475 references localizer["SsoScimTokenModalProvisionConfirm"], but this key does not exist in any localization resource file. Only SsoScimTokenModalRotateConfirm, SsoScimTokenModalTitle, and SsoScimTokenModalWarning are defined. Without this key, the provision confirmation modal will display the key name instead of the translated message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Security/Sso.cshtml` at line 475, Add the
missing localization key "SsoScimTokenModalProvisionConfirm" to all
Security.*.resx resource files so localizer["SsoScimTokenModalProvisionConfirm"]
in the Sso.cshtml view returns a proper string; open each Security.*.resx (all
cultures), add the SsoScimTokenModalProvisionConfirm entry with the appropriate
default English value and translated values for other cultures, then rebuild so
the view uses the localized provision confirmation message instead of the raw
key.
Comment on lines
+435
to
+442
| private static async Task UpdateOidcDatabaseAsync(ILogger<Program> logger, IServiceProvider serviceProvider) | ||
| { | ||
| var oidcRepository = Bootstrapper.GetKernel().Resolve<IOidcRepository>(); | ||
| bool result = await oidcRepository.UpdateOidcDatabaseAsync(); | ||
| logger.Log(LogLevel.Information, "Starting OIDC Database Upgrade"); | ||
|
|
||
| try | ||
| { | ||
| var oidcRepository = Bootstrapper.GetKernel().Resolve<IOidcRepository>(); | ||
| bool result = await oidcRepository.UpdateOidcDatabaseAsync(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full method implementation to check for hidden dependencies and unused parameters
cat -n Workers/Resgrid.Workers.Console/Program.cs | sed -n '435,460p'Repository: Resgrid/Core
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Search for all uses of serviceProvider in the UpdateOidcDatabaseAsync method
rg -n 'serviceProvider' Workers/Resgrid.Workers.Console/Program.cs | head -20Repository: Resgrid/Core
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Check for Bootstrapper.GetKernel() usage in the method
rg -n -B2 -A10 'Bootstrapper\.GetKernel\(\)\.Resolve' Workers/Resgrid.Workers.Console/Program.cs | head -40Repository: Resgrid/Core
Length of output: 1288
🏁 Script executed:
#!/bin/bash
# Find all calls to UpdateOidcDatabaseAsync to understand the context
rg -n 'UpdateOidcDatabaseAsync' Workers/Resgrid.Workers.Console/Program.csRepository: Resgrid/Core
Length of output: 444
Remove unused serviceProvider parameter or use it to resolve dependencies.
At line 435, the UpdateOidcDatabaseAsync method receives an unused serviceProvider parameter but resolves IOidcRepository via Bootstrapper.GetKernel() instead. This creates a hidden dependency that complicates testing. Either pass IOidcRepository explicitly (or resolve it from the injected serviceProvider), or remove the unused parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Workers/Resgrid.Workers.Console/Program.cs` around lines 435 - 442, The
UpdateOidcDatabaseAsync method currently takes an unused serviceProvider and
resolves IOidcRepository via Bootstrapper.GetKernel(), creating a hidden
dependency; either remove the IServiceProvider serviceProvider parameter from
UpdateOidcDatabaseAsync and callers, or change the method to resolve
IOidcRepository from the provided serviceProvider (e.g.,
serviceProvider.GetRequiredService<IOidcRepository>()) and stop using
Bootstrapper.GetKernel(), ensuring the unique symbols to update are
UpdateOidcDatabaseAsync, serviceProvider, IOidcRepository, and
Bootstrapper.GetKernel().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/AuditService.cs`:
- Line 140: The method in AuditService (e.g., GetAuditTypeLabel) currently
returns an empty string for unhandled AuditType enum values; change the fallback
so it returns a meaningful label instead (for example return $"Unknown
({auditType})" or AuditType.ToString()) so unknown/added enum values are visible
in the UI rather than blank; update the return at the default/unhandled branch
to this non-empty fallback and keep the rest of the switch/handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a81f639-b71e-4c6f-a709-4689a8782f4b
📒 Files selected for processing (9)
Core/Resgrid.Localization/Areas/User/Security/Security.de.resxCore/Resgrid.Localization/Areas/User/Security/Security.en.resxCore/Resgrid.Localization/Areas/User/Security/Security.es.resxCore/Resgrid.Localization/Areas/User/Security/Security.fr.resxCore/Resgrid.Localization/Areas/User/Security/Security.it.resxCore/Resgrid.Localization/Areas/User/Security/Security.pl.resxCore/Resgrid.Localization/Areas/User/Security/Security.sv.resxCore/Resgrid.Localization/Areas/User/Security/Security.uk.resxCore/Resgrid.Services/AuditService.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Core/Resgrid.Services/AuditService.cs (1)
40-140: Consider adding labels for all AuditLogTypes enum values.The switch statement handles only 45 of the 93
AuditLogTypesenum values. Many existing types (e.g.,SubscriptionUpdated,CallReactivated,CustomStatusAdded,DocumentAdded,NoteAdded,ContactAdded,CallTypeAdded,CertificationTypeAdded, etc.) will display as "Unknown (X)" in the UI instead of explicit descriptions.While the new fallback prevents blank strings, completing the coverage for all 48 missing enum values would provide a polished user experience and align with the coding guideline of using meaningful, descriptive outputs for better maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/AuditService.cs` around lines 40 - 140, GetAuditLogTypeString in AuditService.cs only maps ~45 of 93 AuditLogTypes values so many enums render as "Unknown (X)"; update the switch in the GetAuditLogTypeString method to include case entries for the remaining AuditLogTypes enum members (e.g., SubscriptionUpdated, CallReactivated, CustomStatusAdded, DocumentAdded, NoteAdded, ContactAdded, CallTypeAdded, CertificationTypeAdded, etc.) and return clear, human-readable labels for each new case; preserve the existing fallback return $"Unknown ({logType})" and add/update unit tests that assert each AuditLogTypes value returns the expected descriptive string to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Core/Resgrid.Services/AuditService.cs`:
- Around line 40-140: GetAuditLogTypeString in AuditService.cs only maps ~45 of
93 AuditLogTypes values so many enums render as "Unknown (X)"; update the switch
in the GetAuditLogTypeString method to include case entries for the remaining
AuditLogTypes enum members (e.g., SubscriptionUpdated, CallReactivated,
CustomStatusAdded, DocumentAdded, NoteAdded, ContactAdded, CallTypeAdded,
CertificationTypeAdded, etc.) and return clear, human-readable labels for each
new case; preserve the existing fallback return $"Unknown ({logType})" and
add/update unit tests that assert each AuditLogTypes value returns the expected
descriptive string to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da98098a-96cc-4d05-add3-59db6252bc2c
📒 Files selected for processing (1)
Core/Resgrid.Services/AuditService.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Localization
Security & Performance