Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
This comment has been minimized.
This comment has been minimized.
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThe PR adds encrypted four-digit security PINs, department-level PIN enforcement, chatbot step-up verification, verified-mobile SMS gating, PIN management UI, persistence support, and SMS identity invalidation when mobile numbers change. ChangesChatbot security PIN enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SMS
participant ChatbotIngressService
participant SecurityPinService
participant Profile
User->>SMS: Send destructive command
SMS->>ChatbotIngressService: Forward verified sender message
ChatbotIngressService->>SecurityPinService: Check PIN requirement
SecurityPinService->>Profile: Read or create encrypted PIN
ChatbotIngressService-->>User: Request security PIN
User->>ChatbotIngressService: Submit PIN
ChatbotIngressService->>SecurityPinService: Validate PIN
SecurityPinService-->>ChatbotIngressService: Validation result
ChatbotIngressService-->>User: Dispatch intent or cancel action
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs (1)
66-105: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResolve dependencies via Service Locator instead of constructor injection.
As per coding guidelines, dependencies in controllers should be explicitly resolved using the Service Locator pattern (
Bootstrapper.GetKernel().Resolve<T>()) to minimize constructor injection.
Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs#L66-L105: RemoveISecurityPinServicefrom the constructor parameters and field initializers, and resolve it explicitly.Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs#L74-L117: RemoveISecurityPinServiceandIEncryptionServicefrom the constructor parameters and resolve them explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 66 - 105, Replace constructor injection with Service Locator resolution for the affected dependencies: in Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs lines 66-105, remove ISecurityPinService from the constructor and its assignment, then resolve it via Bootstrapper.GetKernel().Resolve<ISecurityPinService>() where used; in Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs lines 74-117, remove ISecurityPinService and IEncryptionService from the constructor and resolve both explicitly through the same Service Locator pattern.Source: Coding guidelines
🧹 Nitpick comments (3)
Core/Resgrid.Services/DepartmentSettingsService.cs (1)
961-976: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
bool.TryParseto prevent potential formatting exceptions.If a malformed string is stored in the database for the
ForceChatbotSecurityPinsetting,bool.Parsewill throw aFormatException. Consider usingbool.TryParseto provide a safe fallback.♻️ Proposed refactor
public async Task<bool> GetForceChatbotSecurityPinAsync(int departmentId, bool bypassCache = false) { async Task<string> getSetting() { var s = await GetSettingByDepartmentIdType(departmentId, DepartmentSettingTypes.ForceChatbotSecurityPin); return s?.Setting ?? "false"; } if (Config.SystemBehaviorConfig.CacheEnabled && !bypassCache) { var cachedValue = await _cacheProvider.RetrieveAsync<string>(string.Format(ForceChatbotSecurityPinCacheKey, departmentId), getSetting, LongCacheLength); - return bool.Parse(cachedValue); + return bool.TryParse(cachedValue, out bool result) && result; } - return bool.Parse(await getSetting()); + return bool.TryParse(await getSetting(), out bool fallbackResult) && fallbackResult; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/DepartmentSettingsService.cs` around lines 961 - 976, Update GetForceChatbotSecurityPinAsync to replace both bool.Parse calls with bool.TryParse-based handling, returning a safe false fallback when the cached or database setting is malformed while preserving valid true/false values and existing cache behavior.Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs (1)
419-426: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove query inside the
IsOwnProfilecondition.
model.DepartmentForcesSecurityPinis only used whenmodel.IsOwnProfileis true (both in this controller and the view). Moving this query inside theifblock avoids an unnecessary database/cache retrieval when a user views another member's profile.♻️ Proposed refactor
- model.DepartmentForcesSecurityPin = await _departmentSettingsService.GetForceChatbotSecurityPinAsync(DepartmentId); if (model.IsOwnProfile) { + model.DepartmentForcesSecurityPin = await _departmentSettingsService.GetForceChatbotSecurityPinAsync(DepartmentId); model.SecurityPinEnabled = model.Profile.SecurityPinEnabled; model.SecurityPin = await _securityPinService.GetPinAsync(userId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs` around lines 419 - 426, Move the GetForceChatbotSecurityPinAsync call assigning model.DepartmentForcesSecurityPin inside the existing IsOwnProfile condition in HomeController, alongside the other owner-only security PIN assignments. Preserve the current behavior for the profile owner while avoiding the query for users viewing another member’s profile.Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (1)
45-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
ISecurityPinServicethrough the mandated service locator.Adding another constructor parameter conflicts with the repository’s dependency-resolution convention. Resolve it inside the constructor with
Bootstrapper.GetKernel().Resolve<ISecurityPinService>().As per coding guidelines, “Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 45 - 61, Replace the injected ISecurityPinService constructor parameter in ChatbotIngressService with service-locator resolution inside the constructor, assigning _securityPinService from Bootstrapper.GetKernel().Resolve<ISecurityPinService>(). Preserve the existing field initialization and remove the constructor-injection assignment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 239-250: Update the PIN-required flow in ChatbotIngressService so
_securityPinService.EnsurePinAsync completes successfully before setting
session.State to AwaitingPin, initializing __pinAttempts, and calling
SaveSessionAsync. Only persist the awaiting-PIN session and return the
generated-PIN response after provisioning succeeds; allow null or thrown
provisioning failures to avoid saving that state or claiming a PIN was
generated.
In `@Core/Resgrid.Services/UserProfileService.cs`:
- Around line 23-28: Update the UserProfileService constructor to stop accepting
injected dependencies and resolve IUserProfilesRepository, ICacheProvider, and
IChatbotIdentityRepository through the established service locator inside the
constructor, assigning the resolved instances to the existing fields.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Line 239: Update the master-number branch in the method containing the
senderNumberUnverified parameter to apply the same MobileNumberVerified check
after refetching the profile. Ensure STOP processing does not disable messaging
when the matched profile’s mobile number is unverified, while preserving the
existing behavior for verified profiles.
- Line 152: Redact inbound mobile numbers in the SMS diagnostic logs by masking
textMessage.Msisdn or replacing it with a stable non-reversible identifier.
Apply this to Web/Resgrid.Web.Services/Controllers/TwilioController.cs lines
152-152 and 326-326, and
Web/Resgrid.Web.Services/Controllers/SignalWireController.cs lines 182-182,
while preserving the existing MessageSid and log context.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 549-556: Reorder the ForceChatbotSecurityPin update flow in
DepartmentController so EnsurePinsForDepartmentAsync completes successfully
before SaveOrUpdateSettingAsync persists the enabled setting. Preserve the
existing transition check using forcePinWasEnabled, ensuring failures leave the
setting disabled so a later request retries PIN generation.
In `@Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs`:
- Around line 696-703: Trim model.SecurityPin before passing it to
SecurityPinUtility.IsValidFormat and SecurityPinUtility.IsWeak in the validation
block, matching the value used during encryption while preserving the existing
validation messages and flow.
In `@Web/Resgrid.Web/Areas/User/Views/Department/Settings.cshtml`:
- Around line 94-104: Replace the hardcoded “Require Security PIN” label and
help text in Department Settings with entries from the injected `@localizer`. Also
update the Security PIN labels, placeholder text, and help blocks in
EditUserProfile to use localization resources, preserving the existing UI
behavior and wording through resource keys.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 66-105: Replace constructor injection with Service Locator
resolution for the affected dependencies: in
Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs lines 66-105,
remove ISecurityPinService from the constructor and its assignment, then resolve
it via Bootstrapper.GetKernel().Resolve<ISecurityPinService>() where used; in
Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs lines 74-117, remove
ISecurityPinService and IEncryptionService from the constructor and resolve both
explicitly through the same Service Locator pattern.
---
Nitpick comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 45-61: Replace the injected ISecurityPinService constructor
parameter in ChatbotIngressService with service-locator resolution inside the
constructor, assigning _securityPinService from
Bootstrapper.GetKernel().Resolve<ISecurityPinService>(). Preserve the existing
field initialization and remove the constructor-injection assignment.
In `@Core/Resgrid.Services/DepartmentSettingsService.cs`:
- Around line 961-976: Update GetForceChatbotSecurityPinAsync to replace both
bool.Parse calls with bool.TryParse-based handling, returning a safe false
fallback when the cached or database setting is malformed while preserving valid
true/false values and existing cache behavior.
In `@Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs`:
- Around line 419-426: Move the GetForceChatbotSecurityPinAsync call assigning
model.DepartmentForcesSecurityPin inside the existing IsOwnProfile condition in
HomeController, alongside the other owner-only security PIN assignments.
Preserve the current behavior for the profile owner while avoiding the query for
users viewing another member’s profile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 444d6b30-e3fd-49c1-bf69-2ce2a5ea8249
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/SecurityPinServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (24)
Core/Resgrid.Chatbot/Models/ChatbotPlatform.csCore/Resgrid.Chatbot/Models/ChatbotSession.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Framework/SecurityPinUtility.csCore/Resgrid.Model/ChatbotIdentity.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Model/Services/ISecurityPinService.csCore/Resgrid.Model/UserProfile.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/SecurityPinService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/UserProfileService.csProviders/Resgrid.Providers.Migrations/Migrations/M0089_AddingUserSecurityPin.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0089_AddingUserSecurityPinPg.csRepositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.csWeb/Resgrid.Web.Services/Controllers/SignalWireController.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Models/DepartmentSettingsModel.csWeb/Resgrid.Web/Areas/User/Models/EditProfileModel.csWeb/Resgrid.Web/Areas/User/Views/Department/Settings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtml
Comment on lines
+23
to
+28
| public UserProfileService(IUserProfilesRepository userProfileRepository, ICacheProvider cacheProvider, | ||
| IChatbotIdentityRepository chatbotIdentityRepository) | ||
| { | ||
| _userProfileRepository = userProfileRepository; | ||
| _cacheProvider = cacheProvider; | ||
| _chatbotIdentityRepository = chatbotIdentityRepository; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the Service Locator pattern instead of constructor injection.
As per coding guidelines, dependencies should be resolved explicitly in constructors using the service locator rather than being added as constructor parameters.
♻️ Proposed fix
- public UserProfileService(IUserProfilesRepository userProfileRepository, ICacheProvider cacheProvider,
- IChatbotIdentityRepository chatbotIdentityRepository)
+ public UserProfileService(IUserProfilesRepository userProfileRepository, ICacheProvider cacheProvider)
{
_userProfileRepository = userProfileRepository;
_cacheProvider = cacheProvider;
- _chatbotIdentityRepository = chatbotIdentityRepository;
+ _chatbotIdentityRepository = Bootstrapper.GetKernel().Resolve<IChatbotIdentityRepository>();
}📝 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
| public UserProfileService(IUserProfilesRepository userProfileRepository, ICacheProvider cacheProvider, | |
| IChatbotIdentityRepository chatbotIdentityRepository) | |
| { | |
| _userProfileRepository = userProfileRepository; | |
| _cacheProvider = cacheProvider; | |
| _chatbotIdentityRepository = chatbotIdentityRepository; | |
| public UserProfileService(IUserProfilesRepository userProfileRepository, ICacheProvider cacheProvider) | |
| { | |
| _userProfileRepository = userProfileRepository; | |
| _cacheProvider = cacheProvider; | |
| _chatbotIdentityRepository = Bootstrapper.GetKernel().Resolve<IChatbotIdentityRepository>(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/UserProfileService.cs` around lines 23 - 28, Update the
UserProfileService constructor to stop accepting injected dependencies and
resolve IUserProfilesRepository, ICacheProvider, and IChatbotIdentityRepository
through the established service locator inside the constructor, assigning the
resolved instances to the existing fields.
Source: Coding guidelines
| bool senderNumberUnverified = userProfile != null && userProfile.MobileNumberVerified != true; | ||
| if (senderNumberUnverified) | ||
| { | ||
| Framework.Logging.LogInfo($"[Twilio SMS] MessageSid={request.MessageSid} From={textMessage.Msisdn} matched a profile but the mobile number is not verified; not linking sender to user."); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact inbound mobile numbers from SMS diagnostics.
These new logs persist complete phone numbers, exposing PII. Mask the number or replace it with a stable non-reversible identifier.
Web/Resgrid.Web.Services/Controllers/TwilioController.cs#L152-L152: redacttextMessage.Msisdn.Web/Resgrid.Web.Services/Controllers/SignalWireController.cs#L182-L182: redacttextMessage.Msisdn.Web/Resgrid.Web.Services/Controllers/TwilioController.cs#L326-L326: redacttextMessage.Msisdn.
📍 Affects 2 files
Web/Resgrid.Web.Services/Controllers/TwilioController.cs#L152-L152(this comment)Web/Resgrid.Web.Services/Controllers/SignalWireController.cs#L182-L182Web/Resgrid.Web.Services/Controllers/TwilioController.cs#L326-L326
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` at line 152, Redact
inbound mobile numbers in the SMS diagnostic logs by masking textMessage.Msisdn
or replacing it with a stable non-reversible identifier. Apply this to
Web/Resgrid.Web.Services/Controllers/TwilioController.cs lines 152-152 and
326-326, and Web/Resgrid.Web.Services/Controllers/SignalWireController.cs lines
182-182, while preserving the existing MessageSid and log context.
| // and userProfile are resolved by the caller so the flag can be evaluated before dispatching here. | ||
| private async System.Threading.Tasks.Task ProcessTextCommandsAsync(TextMessage textMessage, InboundMessageEvent messageEvent, | ||
| MessagingResponse response, int? departmentId, UserProfile userProfile) | ||
| MessagingResponse response, int? departmentId, UserProfile userProfile, bool senderNumberUnverified = false) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Apply senderNumberUnverified to the master-number branch.
The branch beginning at Line 560 refetches the profile without checking MobileNumberVerified; its STOP command can therefore disable messaging for an unverified-number match. Enforce the same verification gate before using that profile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` at line 239, Update
the master-number branch in the method containing the senderNumberUnverified
parameter to apply the same MobileNumberVerified check after refetching the
profile. Ensure STOP processing does not disable messaging when the matched
profile’s mobile number is unverified, while preserving the existing behavior
for verified profiles.
Comment on lines
+94
to
+104
| <div class="form-group"> | ||
| <label class="col-sm-2 control-label">Require Security PIN</label> | ||
| <div class="col-sm-10"> | ||
| <div class="row"> | ||
| <div class="col-sm-10"> | ||
| <input type="checkbox" asp-for="ForceChatbotSecurityPin"> | ||
| </div> | ||
| </div> | ||
| <span class="help-block m-b-none">Require every member to confirm dangerous or department-wide chatbot/text actions with their personal 4-digit security PIN. Members without a PIN get a randomly generated one (viewable on their profile page).</span> | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Extract hardcoded UI text into localization resources.
New UI elements have been added with hardcoded English strings instead of using the injected @localizer object, which breaks internationalization for these views.
Web/Resgrid.Web/Areas/User/Views/Department/Settings.cshtml#L94-L104: Extract the "Require Security PIN" label and the associated help text into the localizer.Web/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtml#L299-L327: Extract the Security PIN labels, placeholder text, and help blocks into the localizer.
📍 Affects 2 files
Web/Resgrid.Web/Areas/User/Views/Department/Settings.cshtml#L94-L104(this comment)Web/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtml#L299-L327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web/Areas/User/Views/Department/Settings.cshtml` around lines 94
- 104, Replace the hardcoded “Require Security PIN” label and help text in
Department Settings with entries from the injected `@localizer`. Also update the
Security PIN labels, placeholder text, and help blocks in EditUserProfile to use
localization resources, preserving the existing UI behavior and wording through
resource keys.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (2)
454-474: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid mutating session state before resolving the handler.
DispatchConfirmedIntentAsyncclearssession.Stateandsession.PendingIntentbefore verifying if a handler exists. If no handler claims the intent, it returnsnullbut leaves the session object inappropriately mutated, which could lead to inconsistent state tracking.🛡️ Proposed fix to verify handler existence first
private async Task<ChatbotResponse> DispatchConfirmedIntentAsync(ChatbotMessage message, ChatbotSession session) { var pendingType = session.PendingIntent.Value; + + var confirmHandler = _actionHandlers.FirstOrDefault(h => h.CanHandle(pendingType)); + if (confirmHandler == null) + return null; + var confirmedIntent = new ChatbotIntent { Type = pendingType, Parameters = new Dictionary<string, string>(session.Context) { ["__confirmed"] = "true" } }; session.State = ChatbotDialogState.Idle; session.PendingIntent = null; - var confirmHandler = _actionHandlers.FirstOrDefault(h => h.CanHandle(pendingType)); - if (confirmHandler == null) - return null; - var confirmResponse = await confirmHandler.HandleAsync(message, confirmedIntent, session); confirmResponse.Intent = confirmedIntent;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 454 - 474, The method DispatchConfirmedIntentAsync currently mutates session.State and session.PendingIntent before confirming a handler exists. Resolve confirmHandler and return null before changing either session property; only clear the pending state after a handler is found, preserving the existing handling and persistence flow.
326-328: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle missing handler gracefully.
If
DispatchConfirmedIntentAsyncreturnsnullbecause no handler was found, the execution falls through to normal intent classification, which will likely result in an "unknown command" error response. Return a clear error message instead.🛠️ Proposed fix to handle null return
var confirmResponse = await DispatchConfirmedIntentAsync(message, session); if (confirmResponse != null) return confirmResponse; + + return new ChatbotResponse + { + Text = "Unable to complete action — please try again or contact support.", + Processed = false + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 326 - 328, Update the confirmed-intent branch in the surrounding ingress method so a null result from DispatchConfirmedIntentAsync is handled immediately with a clear error response indicating that no handler was found. Preserve returning confirmResponse when it is non-null, and prevent execution from falling through to normal intent classification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 263-293: In the confirmation flow, remove the duplicate inline
handler-dispatch block and retain a single confirmResponse declaration using
DispatchConfirmedIntentAsync. If that method returns null, return the existing
explicit failure ChatbotResponse so missing handlers or unsuccessful dispatches
are handled without dereferencing or mutating session state.
---
Outside diff comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 454-474: The method DispatchConfirmedIntentAsync currently mutates
session.State and session.PendingIntent before confirming a handler exists.
Resolve confirmHandler and return null before changing either session property;
only clear the pending state after a handler is found, preserving the existing
handling and persistence flow.
- Around line 326-328: Update the confirmed-intent branch in the surrounding
ingress method so a null result from DispatchConfirmedIntentAsync is handled
immediately with a clear error response indicating that no handler was found.
Preserve returning confirmResponse when it is non-null, and prevent execution
from falling through to normal intent classification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e124e86-7880-4ec8-b32b-097ddd505457
📒 Files selected for processing (1)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (2)
454-474: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid mutating session state before resolving the handler.
DispatchConfirmedIntentAsyncclearssession.Stateandsession.PendingIntentbefore verifying if a handler exists. If no handler claims the intent, it returnsnullbut leaves the session object inappropriately mutated, which could lead to inconsistent state tracking.🛡️ Proposed fix to verify handler existence first
private async Task<ChatbotResponse> DispatchConfirmedIntentAsync(ChatbotMessage message, ChatbotSession session) { var pendingType = session.PendingIntent.Value; + + var confirmHandler = _actionHandlers.FirstOrDefault(h => h.CanHandle(pendingType)); + if (confirmHandler == null) + return null; + var confirmedIntent = new ChatbotIntent { Type = pendingType, Parameters = new Dictionary<string, string>(session.Context) { ["__confirmed"] = "true" } }; session.State = ChatbotDialogState.Idle; session.PendingIntent = null; - var confirmHandler = _actionHandlers.FirstOrDefault(h => h.CanHandle(pendingType)); - if (confirmHandler == null) - return null; - var confirmResponse = await confirmHandler.HandleAsync(message, confirmedIntent, session); confirmResponse.Intent = confirmedIntent;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 454 - 474, The method DispatchConfirmedIntentAsync currently mutates session.State and session.PendingIntent before confirming a handler exists. Resolve confirmHandler and return null before changing either session property; only clear the pending state after a handler is found, preserving the existing handling and persistence flow.
326-328: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle missing handler gracefully.
If
DispatchConfirmedIntentAsyncreturnsnullbecause no handler was found, the execution falls through to normal intent classification, which will likely result in an "unknown command" error response. Return a clear error message instead.🛠️ Proposed fix to handle null return
var confirmResponse = await DispatchConfirmedIntentAsync(message, session); if (confirmResponse != null) return confirmResponse; + + return new ChatbotResponse + { + Text = "Unable to complete action — please try again or contact support.", + Processed = false + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 326 - 328, Update the confirmed-intent branch in the surrounding ingress method so a null result from DispatchConfirmedIntentAsync is handled immediately with a clear error response indicating that no handler was found. Preserve returning confirmResponse when it is non-null, and prevent execution from falling through to normal intent classification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 263-293: In the confirmation flow, remove the duplicate inline
handler-dispatch block and retain a single confirmResponse declaration using
DispatchConfirmedIntentAsync. If that method returns null, return the existing
explicit failure ChatbotResponse so missing handlers or unsuccessful dispatches
are handled without dereferencing or mutating session state.
---
Outside diff comments:
In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 454-474: The method DispatchConfirmedIntentAsync currently mutates
session.State and session.PendingIntent before confirming a handler exists.
Resolve confirmHandler and return null before changing either session property;
only clear the pending state after a handler is found, preserving the existing
handling and persistence flow.
- Around line 326-328: Update the confirmed-intent branch in the surrounding
ingress method so a null result from DispatchConfirmedIntentAsync is handled
immediately with a clear error response indicating that no handler was found.
Preserve returning confirmResponse when it is non-null, and prevent execution
from falling through to normal intent classification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e124e86-7880-4ec8-b32b-097ddd505457
📒 Files selected for processing (1)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs
🛑 Comments failed to post (1)
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs (1)
263-293: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix compilation error due to duplicate
confirmResponseand handle missing handlers.The inline confirmation dispatch logic was left in place alongside the new
DispatchConfirmedIntentAsynccall, leading to a compilation error becauseconfirmResponseis defined twice in the same scope. Remove the duplicate inline logic and ensure you handle cases whereDispatchConfirmedIntentAsyncreturnsnull.🐛 Proposed fix to remove duplicated logic
var confirmResponse = await DispatchConfirmedIntentAsync(message, session); if (confirmResponse != null) return confirmResponse; - var pendingType = session.PendingIntent.Value; - - // Locate the owning handler BEFORE mutating session state. If none can handle - // the pending intent, leave the session parked in AwaitingConfirmation and return - // an explicit error rather than silently dropping the confirmation. - var confirmHandler = _actionHandlers.FirstOrDefault(h => h.CanHandle(pendingType)); - if (confirmHandler == null) - { - return new ChatbotResponse - { - Text = "Unable to complete confirmation — please try again or contact support.", - Processed = false - }; - } - - var confirmedIntent = new ChatbotIntent - { - Type = pendingType, - Parameters = new Dictionary<string, string>(session.Context) { ["__confirmed"] = "true" } - }; - - var confirmResponse = await confirmHandler.HandleAsync(message, confirmedIntent, session); - confirmResponse.Intent = confirmedIntent; - session.Context.Clear(); - session.State = ChatbotDialogState.Idle; - session.PendingIntent = null; - await _sessionManager.SaveSessionAsync(session); - return confirmResponse; + + return new ChatbotResponse + { + Text = "Unable to complete confirmation — please try again or contact support.", + Processed = false + };🧰 Tools
🪛 GitHub Actions: .NET / 2_build-and-test.txt
[error] 287-287: CS0128: A local variable or function named 'confirmResponse' is already defined in this scope.
🪛 GitHub Actions: .NET / build-and-test
[error] 287-287: CS0128: A local variable or function named 'confirmResponse' is already defined in this scope.
🪛 GitHub Check: build-and-test
[failure] 287-287:
A local variable or function named 'confirmResponse' is already defined in this scope
[failure] 287-287:
A local variable or function named 'confirmResponse' is already defined in this scope🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 263 - 293, In the confirmation flow, remove the duplicate inline handler-dispatch block and retain a single confirmResponse declaration using DispatchConfirmedIntentAsync. If that method returns null, return the existing explicit failure ChatbotResponse so missing handlers or unsuccessful dispatches are handled without dereferencing or mutating session state.Source: Linters/SAST tools
Resgrid-Bot
reviewed
Jul 16, 2026
| { | ||
| rng.GetBytes(bytes); | ||
| uint value = (uint)(bytes[0] | bytes[1] << 8 | bytes[2] << 16 | bytes[3] << 24); | ||
| string pin = (value % 10000).ToString().PadLeft(PinLength, '0'); |
There was a problem hiding this comment.
The literal 10000 in 'value % 10000' is an unextracted magic number representing 10^PinLength, violating the 'Replace magic numbers with named constants' rule. Extract it to a private const PinSpace derived from PinLength.
private const int PinSpace = 10000; // 10^PinLength
// ...
string pin = (value % PinSpace).ToString().PadLeft(PinLength, '0');Prompt for LLM
File Core/Resgrid.Framework/SecurityPinUtility.cs:
Line 64:
Violates rule 'Replace magic numbers with named constants': the literal 10000 in 'value % 10000' represents the count of possible 4-digit PIN values (10^PinLength) but is not extracted to a named constant. PinLength is already defined as a const, so the modulus could derive from it.
Suggested Code:
private const int PinSpace = 10000; // 10^PinLength
// ...
string pin = (value % PinSpace).ToString().PadLeft(PinLength, '0');
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 16, 2026
Resgrid-Bot
reviewed
Jul 16, 2026
Resgrid-Bot
reviewed
Jul 16, 2026
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
2 participants
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.
Pull Request Description
RG-T117: Chatbot Security Hardening — Step-up PIN Authentication and SMS Identity Verification
This pull request introduces two major security improvements to the chatbot and SMS integration:
1. Security PIN (Step-up Authentication for Destructive Actions)
Adds a new 4-digit security PIN system that acts as a second factor for confirmed destructive or department-wide chatbot/text actions:
ForceChatbotSecurityPin) forces all members to use a PIN. When enabled, every member without a PIN automatically receives a randomly generated one.AwaitingPinstate. The pending intent only executes after the correct PIN is supplied. After 3 failed attempts, the action is cancelled.2. SMS Identity Verification Hardening
Tightens trust rules so an unverified mobile number can never identify or act as a Resgrid user:
SecurityPinfield is preserved when a caller doesn't supply it (null = unchanged), preventing accidental PIN deletion by unrelated profile saves.Database
SecurityPin(nullable string, encrypted) andSecurityPinEnabled(boolean, default false) columns toUserProfiles.Tests
SecurityPinUtility(format validation, weakness detection, generation) andSecurityPinService(PIN requirement logic, validation, and generation/ensuring flows).