Conversation
…e api controllers
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR contains two independent changes: updates to TTS voice configuration (defaulting to klatt4 at speed 165 instead of klatt6 at 175), S3 storage error handling (treating malformed responses as success rather than falling back to presigned URLs), and a substantial authentication refactoring that removes legacy v3 token middleware and centralizes authorization at the base controller level. ChangesTTS Voice Configuration & S3 Storage Refactoring
Authentication Refactoring & Authorization Consolidation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 15 seconds.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: 2
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.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.cs (1)
21-79:⚠️ Potential issue | 🟠 MajorSome inheritors correctly use
GetEffectiveDepartmentId(), but others cannot support cross-department SystemApiKey requests.Endpoints that accept
departmentIdparameters (e.g.,GetCall,GetUnitByName) are correctly usingGetEffectiveDepartmentId(). However, endpoints that don't acceptdepartmentIdparameters cannot support cross-department access in SystemApiKey mode:
GetCallExtraData,GetActiveCalls,GetAllUnits,GetAllRoles: These lock to the token'sDepartmentIdclaim and will fail or return incorrect results if the relay's token department doesn't match the requested entity's department.GetCallExtraDataspecifically validatescall.DepartmentId != DepartmentIdwithout guarding for SystemApiKey mode, guaranteeing failure for out-of-department calls.Either add optional
departmentIdquery parameters to these endpoints and useGetEffectiveDepartmentId(), or restrict SystemApiKey access to single-department operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.cs` around lines 21 - 79, Several controller actions (GetCallExtraData, GetActiveCalls, GetAllUnits, GetAllRoles) incorrectly always use the token's DepartmentId (protected DepartmentId / ClaimsAuthorizationHelper.GetDepartmentId()) causing cross-department SystemApiKey requests to fail; update these actions to accept an optional departmentId query parameter (string or int?) and replace uses of DepartmentId with GetEffectiveDepartmentId(requestDepartmentId) so SystemApiKey callers can pass the intended department, and specifically modify GetCallExtraData's validation (the call.DepartmentId != DepartmentId check) to use GetEffectiveDepartmentId(...) or skip the mismatch failure when IsSystemApiKeyRequest and a valid request department is supplied. Ensure method signatures and all internal references use the new requestDepartmentId variable and preserve existing behavior when no requestDepartmentId is provided.
🧹 Nitpick comments (1)
Web/Resgrid.Web.Tts/Services/S3StorageService.cs (1)
98-141: Remove unreachable dead code methods.The following methods are unreachable after removing the presigned fallback logic:
WasUploadPersistedAsync(lines 98-141)UploadWithPresignedUrlAsync(lines 156-207)CreatePresignedHeadUrl(lines 209-219)ExistsWithPresignedHeadAsync(lines 221-271)The public
UploadAsyncmethod no longer invokes these methods;HandleMalformedPutResponseAsyncsimply logs and returns without attempting a fallback. Removing them reduces maintenance burden and improves code clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs` around lines 98 - 141, The four methods WasUploadPersistedAsync, UploadWithPresignedUrlAsync, CreatePresignedHeadUrl, and ExistsWithPresignedHeadAsync are now dead code because UploadAsync/HandleMalformedPutResponseAsync no longer call the presigned-fallback path; remove these private methods from S3StorageService.cs and also delete any helper members or using directives that only supported them (e.g., presigned-request builders or HttpClient fields) to avoid unused warnings, then run a compile to ensure no remaining references or tests depend on those symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Around line 83-96: HandleMalformedPutResponseAsync currently ignores payload,
contentType, and cancellationToken and assumes a FormatException means the
upload succeeded; either remove the unused parameters from the method signature
(and update all callers such as CacheService.StoreAsync to match) or add
verification logic that checks S3 for the object (e.g., a HEAD/GetObject check
using the objectKey) before returning success; update the log to reflect
verification outcome and ensure cancellationToken is honored if you perform the
verification.
- Around line 55-63: Update the exception handling in S3StorageService (the
catch blocks around metadata parsing in the method that checks object existence,
e.g., ExistsAsync) to catch AmazonUnmarshallingException and inspect its
InnerException for a FormatException (use a when filter like "when
(ex.InnerException is FormatException)") instead of catching FormatException
directly; log the outer AmazonUnmarshallingException and the inner
FormatException details, and either perform an explicit secondary verification
(e.g., a safe HEAD/get attempt) before returning true or add a clear comment
documenting the assumption that a parsing failure implies the object likely
exists so callers (like CacheService) understand the risk.
---
Outside diff comments:
In
`@Web/Resgrid.Web.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.cs`:
- Around line 21-79: Several controller actions (GetCallExtraData,
GetActiveCalls, GetAllUnits, GetAllRoles) incorrectly always use the token's
DepartmentId (protected DepartmentId /
ClaimsAuthorizationHelper.GetDepartmentId()) causing cross-department
SystemApiKey requests to fail; update these actions to accept an optional
departmentId query parameter (string or int?) and replace uses of DepartmentId
with GetEffectiveDepartmentId(requestDepartmentId) so SystemApiKey callers can
pass the intended department, and specifically modify GetCallExtraData's
validation (the call.DepartmentId != DepartmentId check) to use
GetEffectiveDepartmentId(...) or skip the mismatch failure when
IsSystemApiKeyRequest and a valid request department is supplied. Ensure method
signatures and all internal references use the new requestDepartmentId variable
and preserve existing behavior when no requestDepartmentId is provided.
---
Nitpick comments:
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Around line 98-141: The four methods WasUploadPersistedAsync,
UploadWithPresignedUrlAsync, CreatePresignedHeadUrl, and
ExistsWithPresignedHeadAsync are now dead code because
UploadAsync/HandleMalformedPutResponseAsync no longer call the
presigned-fallback path; remove these private methods from S3StorageService.cs
and also delete any helper members or using directives that only supported them
(e.g., presigned-request builders or HttpClient fields) to avoid unused
warnings, then run a compile to ensure no remaining references or tests depend
on those symbols.
🪄 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: 023da492-20ea-44f4-bed3-9ad102ce6158
📒 Files selected for processing (22)
Core/Resgrid.Config/TtsConfig.csTests/Resgrid.Tests/Services/DepartmentSettingsServiceTtsLanguageTests.csTests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsAdminControllerTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csWeb/Resgrid.Web.Services/Controllers/v4/CallFilesController.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/DepartmentsController.csWeb/Resgrid.Web.Services/Controllers/v4/GroupsController.csWeb/Resgrid.Web.Services/Controllers/v4/RolesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitsController.csWeb/Resgrid.Web.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.csWeb/Resgrid.Web.Services/Middleware/AuthTokenMiddleware.csWeb/Resgrid.Web.Services/Middleware/MiddlewareExtensions.csWeb/Resgrid.Web.Services/Middleware/TokenAuthHandler.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web.Tts/Configuration/TtsOptions.csWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web.Tts/Services/S3StorageService.csWeb/Resgrid.Web.Tts/Services/TtsService.csWeb/Resgrid.Web.Tts/k8s/deployment.yaml
💤 Files with no reviewable changes (10)
- Web/Resgrid.Web.Tts/Dockerfile
- Web/Resgrid.Web.Services/Controllers/v4/DepartmentsController.cs
- Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs
- Web/Resgrid.Web.Services/Controllers/v4/RolesController.cs
- Web/Resgrid.Web.Services/Controllers/v4/CallFilesController.cs
- Web/Resgrid.Web.Services/Controllers/v4/UnitsController.cs
- Web/Resgrid.Web.Services/Controllers/v4/GroupsController.cs
- Web/Resgrid.Web.Services/Middleware/TokenAuthHandler.cs
- Web/Resgrid.Web.Services/Middleware/MiddlewareExtensions.cs
- Web/Resgrid.Web.Services/Middleware/AuthTokenMiddleware.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.
…e api controllers
Summary by CodeRabbit
Release Notes
New Features
Changes
Bug Fixes