Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds SystemApiKey support: a new system API key config and auth handler, a system-auth v4 controller base that permits request-specified department scoping, client_credentials token flow for service principals, new lookup endpoints, and request-level DepartmentId fields for call and call-file inputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as External Client
participant Auth as SystemApiKeyAuthHandler
participant Controller as V4 Controller<br/>V4AuthenticatedApiControllerbaseSystemAuth
participant Service as Business Service
participant DB as Database
Client->>Auth: HTTP request with X-Resgrid-SystemApiKey header
Auth->>Auth: Timing-safe validate vs Config.SecurityConfig.SystemApiKey
alt invalid
Auth-->>Client: 401 / AuthenticateResult.Fail
else valid
Auth->>Controller: AuthenticateResult.Success (claims principal with service/account claims)
Controller->>Controller: IsSystemApiKeyRequest = true
Controller->>Controller: GetEffectiveDepartmentId(from request param or claims)
Controller->>Service: Perform operation using effective department context
Service->>DB: Lookup/persist using effective department
DB-->>Service: Result
Service-->>Controller: Data/Result
Controller-->>Client: 200 OK (response)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Web/Resgrid.Web.Services/Controllers/v4/CallFilesController.cs (1)
63-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent department ID usage across endpoints.
GetFilesForCallstill usesDepartmentId(from claims) directly at line 63, whileSaveCallFileusesGetEffectiveDepartmentId. If System API Key requests should be able to callGetFilesForCall, this endpoint needs the same pattern.🔧 Suggested fix
public async Task<ActionResult<CallFilesResult>> GetFilesForCall(int callId, bool includeData, int type) { var result = new CallFilesResult(); var call = await _callsService.GetCallByIdAsync(callId); if (call == null) { ResponseHelper.PopulateV4ResponseNotFound(result); return Ok(result); } - if (call.DepartmentId != DepartmentId) + // For SystemApiKey requests, the department context comes from the call itself + // since there's no way to pass a DepartmentId parameter to this GET endpoint + if (!IsSystemApiKeyRequest && call.DepartmentId != DepartmentId) return Unauthorized();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CallFilesController.cs` around lines 63 - 64, GetFilesForCall uses the raw DepartmentId claim to authorize access (if (call.DepartmentId != DepartmentId) return Unauthorized()) while SaveCallFile uses GetEffectiveDepartmentId for System API Key support; update GetFilesForCall to obtain the effective department id via GetEffectiveDepartmentId() and compare call.DepartmentId to that value (mirror the authorization logic used in SaveCallFile) so System API Key requests are allowed the same way; ensure the same Unauthorized handling is preserved.Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs (1)
828-834:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
departmentIdin the created route for SystemApiKey callers.
GetCallnow relies on request-scoped department resolution in system-key mode, but theCreatedAtActionroute only includescallId. The Location header for a freshly created call therefore points system-key clients at a URL they can't immediately fetch.Suggested fix
- return CreatedAtAction("GetCall", new { callId = result.Id }, result); + var routeValues = IsSystemApiKeyRequest + ? new { callId = result.Id, departmentId = effectiveDepartmentId } + : new { callId = result.Id }; + + return CreatedAtAction("GetCall", routeValues, result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs` around lines 828 - 834, The CreatedAtAction call only sets callId so system-key clients get a Location they cannot resolve; update the route values passed to CreatedAtAction("GetCall", ...) to include departmentId when the request is using a system API key (e.g. check the existing system-key flag on the request/context). Concretely, build the route values as new { callId = result.Id, departmentId = savedCall.DepartmentId } (or result.DepartmentId if populated) when in system-key mode and pass that to CreatedAtAction so GetCall can resolve the department for system-mode requests.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
247-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new
departmentIdparameter forGetCall.At Line 247 the method signature has two parameters, but only
callIdis documented. Add<param name="departmentId">...to prevent incomplete API docs and client confusion.🤖 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 247 - 252, The XML docs for M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String) only document the callId parameter; add a matching <param name="departmentId">...</param> entry describing the departmentId (e.g., "Id of the department the call belongs to") so both parameters (callId and departmentId) are documented and API docs/clients are accurate.
🧹 Nitpick comments (4)
Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs (2)
355-362: ⚡ Quick winRedundant conditional branches.
Both branches set the same
AccessTokenLifetime, making theif/elsepointless.♻️ Suggested simplification
- if (request.GetScopes() != null && request.GetScopes().Contains("mobile")) - { - deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes)); - } - else - { - deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes)); - } + deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes));🤖 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 355 - 362, The if/else in ConnectController.cs around request.GetScopes() is redundant because both branches call deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes)); remove the conditional and replace with a single call to deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes)); (references: deptPrincipal.SetAccessTokenLifetime, request.GetScopes(), OidcConfig.AccessTokenExpiryMinutes) to simplify the code.
856-915: ⚖️ Poor tradeoffDuplicated resource/action claims list.
This method duplicates the same resource and action arrays found in
SystemApiKeyAuthHandler.HandleAuthenticateAsync. Consider extracting to a shared helper.♻️ Suggested refactor
Extract to a shared static class in
Resgrid.Providers.Claims:// In Resgrid.Providers.Claims/ResgridClaimTypes.cs or a new helper public static class ResgridClaimHelpers { public static readonly string[] AllResources = new[] { ResgridClaimTypes.Resources.Department, ResgridClaimTypes.Resources.Personnel, // ... rest of resources }; public static readonly string[] AllActions = new[] { ResgridClaimTypes.Actions.View, ResgridClaimTypes.Actions.Create, ResgridClaimTypes.Actions.Update, ResgridClaimTypes.Actions.Delete }; public static void AddAllResourceClaims(ClaimsIdentity identity, Func<Claim, Claim> claimTransform = null) { foreach (var resource in AllResources) { foreach (var action in AllActions) { var claim = new Claim(resource, action); identity.AddClaim(claimTransform?.Invoke(claim) ?? claim); } } } }🤖 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 856 - 915, AddAllResourceClaims duplicates the resource/action arrays also defined in SystemApiKeyAuthHandler.HandleAuthenticateAsync; extract those arrays and the claim-adding logic into a shared helper (e.g., a new Resgrid.Providers.Claims.ResgridClaimHelpers) exposing AllResources, AllActions and a method like AddAllResourceClaims(ClaimsIdentity identity, Func<Claim,Claim> claimTransform = null), then update both ConnectController.AddAllResourceClaims and SystemApiKeyAuthHandler.HandleAuthenticateAsync to call the shared helper (preserve existing SetDestinations/Destinations.AccessToken usage via the claimTransform or by setting destinations after helper returns).Web/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.cs (1)
23-28: Remove the deprecatedISystemClockparameter and update the constructor.In .NET 8+,
ISystemClockis obsolete. Use the constructor overload without the clock parameter:public SystemApiKeyAuthHandler( IOptionsMonitor<AuthenticationSchemeOptions> options, ILoggerFactory logger, UrlEncoder encoder) : base(options, logger, encoder)Access the current time via the protected
TimeProviderproperty instead of the deprecatedClock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.cs` around lines 23 - 28, Update the SystemApiKeyAuthHandler constructor to remove the obsolete ISystemClock parameter: change the constructor signature for SystemApiKeyAuthHandler to accept only IOptionsMonitor<AuthenticationSchemeOptions>, ILoggerFactory and UrlEncoder and call the base(options, logger, encoder) overload; then replace any uses of the deprecated Clock property inside SystemApiKeyAuthHandler with the protected TimeProvider (e.g., use TimeProvider.GetUtcNow() or relevant TimeProvider methods where Clock was referenced).Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
1253-1260: ⚡ Quick winAlign role/unit lookup docs with the OAuth vs SystemApiKey behavior wording used for groups.
These endpoints mention optional
departmentId, but unlike group lookup docs they don’t state how OAuth requests treat overrides. Clarifying that behavior here avoids integration mistakes.Also applies to: 1692-1699
🤖 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 1253 - 1260, Update the XML docs for Resgrid.Web.Services.Controllers.v4.RolesController.GetRoleByName(System.String,System.String) (and the similar role/unit lookup members at the other location) to clarify departmentId behavior: state that departmentId is an optional override used only when authenticating with a SystemApiKey (hosted multi-department mode) and that OAuth-authenticated requests will ignore the departmentId override and use the caller's scoped department instead; make the same wording change for the corresponding unit/role lookup member at the other noted location so the documentation matches the groups wording.
🤖 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.Services/Controllers/v4/CallsController.cs`:
- Around line 550-567: The code can proceed with an invalid department when
using SystemApiKey; after resolving effectiveDepartmentId (via
GetEffectiveDepartmentId) and fetching department with
_departmentsService.GetDepartmentByIdAsync, add a null/invalid check and
short-circuit (e.g., return BadRequest() or NotFound()) before continuing or
dereferencing department (e.g., department.TimeZone) or using
effectiveDepartmentId for lookups or assigning call.DepartmentId; ensure this
validation happens immediately after the call to
_departmentsService.GetDepartmentByIdAsync in the SaveCall/CallsController flow
and before calling _departmentsService.GetAllMembersForDepartmentAsync,
_departmentGroupsService.GetAllGroupsForDepartmentAsync,
_personnelRolesService.GetAllRolesForDepartmentAsync,
_unitsService.GetUnitsForDepartmentAsync, or GetValidatedDestinationPoiAsync.
In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs`:
- Around line 256-257: Replace the direct string equality check for client
secrets with a timing-safe comparison: when looking up
Config.SecurityConfig.SystemLoginCredentials[request.ClientId] compare the
stored secret and request.ClientSecret using
System.Security.Cryptography.CryptographicOperations.FixedTimeEquals by
converting both strings to bytes (e.g., UTF8) and comparing the byte arrays;
ensure you still handle missing ClientId keys the same way (no NullReference)
and do not short-circuit to a non-constant-time path.
- Around line 307-308: Replace the plain string equality check for
department.SharedSecret vs request.ClientSecret with a timing-safe comparison:
first ensure neither value is null/whitespace, then compare their UTF8 byte
arrays using
System.Security.Cryptography.CryptographicOperations.FixedTimeEquals (or another
constant-time comparison) so the check in ConnectController (where
department.SharedSecret and request.ClientSecret are compared) uses
FixedTimeEquals(Encoding.UTF8.GetBytes(department.SharedSecret),
Encoding.UTF8.GetBytes(request.ClientSecret)) and still treats missing/empty
secrets as failure.
In `@Web/Resgrid.Web.Services/Controllers/v4/GroupsController.cs`:
- Around line 141-148: When a non-empty departmentId is provided but fails
int.TryParse or parses to <= 0, the request should fail closed instead of
falling back to global resolution; update the departmentId validation in
GroupsController (the block that currently checks "else if
(!string.IsNullOrWhiteSpace(departmentId) && int.TryParse(departmentId, out var
deptId) && deptId > 0)" and the analogous block at the second occurrence) to
detect the case where departmentId is non-empty but invalid and call
ResponseHelper.PopulateV4ResponseNotFound(result) and return result immediately,
keeping the existing check that compares group.DepartmentId to the parsed deptId
when parsing succeeds.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitsController.cs`:
- Around line 271-284: When returning a unit from the
GetUnitByNameDepartmentIdAsync lookup, apply the same unit-matrix authorization
check used in GetAllUnits/GetAllUnitsInfos: call
_authorizationService.CanUserViewUnitViaMatrixAsync(unit.DepartmentId,
unit.UnitId, User, allowedRoles) (or the exact overload used elsewhere) after
fetching the unit and before setting result.Data; if that check returns false,
call ResponseHelper.PopulateV4ResponseNotFound(result) and return result to
avoid exposing units hidden by the matrix; otherwise proceed to call
ConvertUnitsData(unit, null, null, TimeZone), set PageSize/Status,
PopulateV4ResponseData and return Ok(result).
In
`@Web/Resgrid.Web.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.cs`:
- Around line 20-21: The base controller
V4AuthenticatedApiControllerbaseSystemAuth currently includes the "SystemApiKey"
authentication scheme on its Authorize attribute which unintentionally allows
the system key for every derived action; remove "SystemApiKey" from the
Authorize attribute on V4AuthenticatedApiControllerbaseSystemAuth (and the
duplicated occurrences around lines referenced) so the base only requires the
OpenIddict validation scheme, and instead add the "SystemApiKey" scheme
explicitly to the specific controller(s) or action(s) that must accept the
system key (e.g., the relay endpoints/controllers that rely on
SystemApiKeyAuthHandler); ensure any actions that depend on DepartmentId/UserId
remain under OpenIddict-only authorization or explicitly validate
department-scoped context when SystemApiKey is used.
In `@Web/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.cs`:
- Around line 59-66: In SystemApiKeyAuthHandler (the claims construction block
that adds new Claim entries), fix the typographical error "smpt_relay_system" to
"smtp_relay_system" in the PrimarySid and Data.UserId claim values so the
identifier strings are consistent and correctly spelled; locate the new
Claim(...) entries for ClaimTypes.PrimarySid and ResgridClaimTypes.Data.UserId
and update their string literals accordingly.
- Around line 49-52: Replace the ordinal string comparison in
SystemApiKeyAuthHandler (likely in HandleAuthenticateAsync) with a timing-safe
comparison: first ensure Config.SecurityConfig.SystemApiKey and the incoming
apiKey are non-null/whitespace, then convert both keys to byte[] (e.g.,
Encoding.UTF8.GetBytes) and use
System.Security.Cryptography.CryptographicOperations.FixedTimeEquals to compare
them; if the fixed-time comparison fails return AuthenticateResult.Fail("Invalid
System API Key") as before.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 592-596: The documentation and implementation currently describe
System API key auth as granting global superuser access; update the
AddAllResourceClaims method and the corresponding XML doc entries (e.g.,
Resgrid.Web.Services.Controllers.v4.ConnectController.AddAllResourceClaims) and
the duplicate doc block around the other occurrence (lines referenced) so that
they state keys are scoped per-department (or include explicit guardrails)
rather than all-departments access. Modify the logic that issues claims for
system API keys to accept and enforce a department-scoping parameter or a
whitelist of department IDs, and update the XML summary to describe scoped claim
issuance, token validation checks, and rotation/revocation guidance; ensure the
doc text for the other duplicated block mirrors this scoped behavior.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/CallFilesController.cs`:
- Around line 63-64: GetFilesForCall uses the raw DepartmentId claim to
authorize access (if (call.DepartmentId != DepartmentId) return Unauthorized())
while SaveCallFile uses GetEffectiveDepartmentId for System API Key support;
update GetFilesForCall to obtain the effective department id via
GetEffectiveDepartmentId() and compare call.DepartmentId to that value (mirror
the authorization logic used in SaveCallFile) so System API Key requests are
allowed the same way; ensure the same Unauthorized handling is preserved.
In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs`:
- Around line 828-834: The CreatedAtAction call only sets callId so system-key
clients get a Location they cannot resolve; update the route values passed to
CreatedAtAction("GetCall", ...) to include departmentId when the request is
using a system API key (e.g. check the existing system-key flag on the
request/context). Concretely, build the route values as new { callId =
result.Id, departmentId = savedCall.DepartmentId } (or result.DepartmentId if
populated) when in system-key mode and pass that to CreatedAtAction so GetCall
can resolve the department for system-mode requests.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 247-252: The XML docs for
M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String)
only document the callId parameter; add a matching <param
name="departmentId">...</param> entry describing the departmentId (e.g., "Id of
the department the call belongs to") so both parameters (callId and
departmentId) are documented and API docs/clients are accurate.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs`:
- Around line 355-362: The if/else in ConnectController.cs around
request.GetScopes() is redundant because both branches call
deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes));
remove the conditional and replace with a single call to
deptPrincipal.SetAccessTokenLifetime(TimeSpan.FromMinutes(OidcConfig.AccessTokenExpiryMinutes));
(references: deptPrincipal.SetAccessTokenLifetime, request.GetScopes(),
OidcConfig.AccessTokenExpiryMinutes) to simplify the code.
- Around line 856-915: AddAllResourceClaims duplicates the resource/action
arrays also defined in SystemApiKeyAuthHandler.HandleAuthenticateAsync; extract
those arrays and the claim-adding logic into a shared helper (e.g., a new
Resgrid.Providers.Claims.ResgridClaimHelpers) exposing AllResources, AllActions
and a method like AddAllResourceClaims(ClaimsIdentity identity,
Func<Claim,Claim> claimTransform = null), then update both
ConnectController.AddAllResourceClaims and
SystemApiKeyAuthHandler.HandleAuthenticateAsync to call the shared helper
(preserve existing SetDestinations/Destinations.AccessToken usage via the
claimTransform or by setting destinations after helper returns).
In `@Web/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.cs`:
- Around line 23-28: Update the SystemApiKeyAuthHandler constructor to remove
the obsolete ISystemClock parameter: change the constructor signature for
SystemApiKeyAuthHandler to accept only
IOptionsMonitor<AuthenticationSchemeOptions>, ILoggerFactory and UrlEncoder and
call the base(options, logger, encoder) overload; then replace any uses of the
deprecated Clock property inside SystemApiKeyAuthHandler with the protected
TimeProvider (e.g., use TimeProvider.GetUtcNow() or relevant TimeProvider
methods where Clock was referenced).
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 1253-1260: Update the XML docs for
Resgrid.Web.Services.Controllers.v4.RolesController.GetRoleByName(System.String,System.String)
(and the similar role/unit lookup members at the other location) to clarify
departmentId behavior: state that departmentId is an optional override used only
when authenticating with a SystemApiKey (hosted multi-department mode) and that
OAuth-authenticated requests will ignore the departmentId override and use the
caller's scoped department instead; make the same wording change for the
corresponding unit/role lookup member at the other noted location so the
documentation matches the groups wording.
🪄 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: 7670fafb-1e6b-451f-97b3-10793a7adaac
📒 Files selected for processing (15)
Core/Resgrid.Config/SecurityConfig.csWeb/Resgrid.Web.Services/Controllers/v4/CallFilesController.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/ConnectController.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/SystemApiKeyAuthHandler.csWeb/Resgrid.Web.Services/Models/v4/CallFiles/SaveCallFileInput.csWeb/Resgrid.Web.Services/Models/v4/Calls/NewCallInput.csWeb/Resgrid.Web.Services/Models/v4/Departments/DepartmentResult.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/Startup.cs
Comment on lines
+592
to
+596
| <member name="M:Resgrid.Web.Services.Controllers.v4.ConnectController.AddAllResourceClaims(System.Security.Claims.ClaimsIdentity)"> | ||
| <summary> | ||
| Adds all Resgrid resource claims (View, Create, Update, Delete) to the given identity. | ||
| Used for system-level and client-credentials service principals that need full access. | ||
| </summary> |
Contributor
There was a problem hiding this comment.
System API key auth is currently documented as global superuser access.
Line 4254 and Line 595 describe full permissions across all departments. That creates a very large blast radius if one key is leaked. Please scope claims/department access per key (or equivalent guardrails) and reflect that in these docs.
Also applies to: 4250-4256
🤖 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 592 - 596,
The documentation and implementation currently describe System API key auth as
granting global superuser access; update the AddAllResourceClaims method and the
corresponding XML doc entries (e.g.,
Resgrid.Web.Services.Controllers.v4.ConnectController.AddAllResourceClaims) and
the duplicate doc block around the other occurrence (lines referenced) so that
they state keys are scoped per-department (or include explicit guardrails)
rather than all-departments access. Modify the logic that issues claims for
system API keys to accept and enforce a department-scoping parameter or a
whitelist of department IDs, and update the XML summary to describe scoped claim
issuance, token validation checks, and rotation/revocation guidance; ensure the
doc text for the other duplicated block mirrors this scoped behavior.
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 (2)
Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs (1)
165-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
GetCallnow needsdepartmentId, so the create Location header is stale.In
SystemApiKeymode this action authorizes againstGetEffectiveDepartmentId(departmentId), butSaveCallstill returnsCreatedAtAction("GetCall", new { callId = ... }). The Location URL it emits will 401 for system-key callers unless the department id is propagated too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs` around lines 165 - 185, The Location header returned by SaveCall uses CreatedAtAction("GetCall", new { callId = ... }) but GetCall now requires departmentId for SystemApiKey authorization; update the CreatedAtAction call in SaveCall (and any other places creating a GetCall location) to include the effective department id (e.g., new { callId = createdId, departmentId = effectiveDepartmentId }) so the generated URL contains departmentId and won't 401 for system-key callers.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
247-253:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing XML doc for
departmentIdparameter onGetCallLine 247 shows a two-parameter signature, but only
callIdis documented on Line 251. Add the missingdepartmentIdparam so generated API docs stay accurate.Suggested doc patch
<member name="M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String)"> <summary> Returns a specific call from the Resgrid System </summary> <param name="callId">Id of the call trying to be retrived</param> + <param name="departmentId">Optional department override used for SystemApiKey requests.</param> <returns>CallResult of the call in the Resgrid system</returns> </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 247 - 253, The XML documentation for Resgrid.Web.Services.Controllers.v4.CallsController.GetCall currently documents only the callId parameter; add a <param name="departmentId"> element describing the departmentId argument to match the two-parameter signature and keep generated API docs accurate. Locate the member entry for M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String) and add a short description for departmentId (e.g., "Id of the department the call belongs to") alongside the existing callId param and returns summary.
♻️ Duplicate comments (3)
Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs (1)
35-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
SystemApiKeyto the relay actions, not the whole controller.Only
GetCallandSaveCallwere updated to useGetEffectiveDepartmentId/IsSystemApiKeyRequest. The rest of this controller still readsDepartmentIddirectly or runs user-based authorization, so a system-key request falls through withDepartmentId == 0and a syntheticUserId. Please add the extra scheme only on the actions that are actually system-key aware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs` around lines 35 - 36, The controller-level Authorize attribute currently enables SystemApiKey for all actions; remove "SystemApiKey" from the class-level [Authorize] and instead add the SystemApiKey scheme only to the actions that are system-key aware (the GetCall and SaveCall action methods which use GetEffectiveDepartmentId/IsSystemApiKeyRequest); keep BasicAuthentication on the controller (e.g., [Authorize(AuthenticationSchemes = "BasicAuthentication")]) and add [Authorize(AuthenticationSchemes = "BasicAuthentication,SystemApiKey")] (or equivalent) only to the GetCall and SaveCall methods so other actions continue to use the normal user-based authorization and DepartmentId handling.Web/Resgrid.Web.Services/Controllers/v4/GroupsController.cs (1)
20-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLimit
SystemApiKeyto the new code-resolution endpoints.
GetGroupByDispatchCodeandGetGroupByMessageCodewere updated for the relay flow, but the older actions in this controller still rely on the token department directly. AddingSystemApiKeyat the class level makes those endpoints callable with an auth context they don't understand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/GroupsController.cs` around lines 20 - 21, The class-level Authorize attribute currently allows SystemApiKey for all endpoints; remove SystemApiKey from the class-level attribute on GroupsController (leave only BasicAuthentication or the original default) and instead apply the SystemApiKey-inclusive attribute only to the new relay-specific actions: GetGroupByDispatchCode and GetGroupByMessageCode. Locate the Authorize declaration on the GroupsController class and update it, then add [Authorize(AuthenticationSchemes = "BasicAuthentication,SystemApiKey")] directly above the GetGroupByDispatchCode and GetGroupByMessageCode methods so only those endpoints accept SystemApiKey.Web/Resgrid.Web.Services/Controllers/v4/UnitsController.cs (1)
27-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't enable
SystemApiKeyfor the entire units controller yet.
GetAllUnits,GetAllUnitsInfos, andGetUnitsFilterOptionsstill use the token-scopedDepartmentId/UserIdpath. With a system key those values are not meaningful, so this class-level scheme addition widens the surface to actions that are not actually system-key compatible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitsController.cs` around lines 27 - 28, The class-level Authorize attribute on UnitsController enables SystemApiKey for all actions but the actions GetAllUnits, GetAllUnitsInfos, and GetUnitsFilterOptions rely on token-scoped DepartmentId/UserId and therefore are not system-key compatible; remove SystemApiKey from the class-level attribute (leave BasicAuthentication) and instead add the SystemApiKey scheme only to the specific action methods that are implemented to accept system keys, or update those specific methods (GetAllUnits, GetAllUnitsInfos, GetUnitsFilterOptions) to explicitly use only BasicAuthentication to preserve current behavior.
🧹 Nitpick comments (1)
Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs (1)
858-917: ⚡ Quick winExtract the permission matrix into a shared definition.
This exact resource/action list is now maintained here and in
Web/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.cs. One future permission edit on only one side will create silent auth drift between the API-key andclient_credentialspaths.🤖 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 858 - 917, AddAllResourceClaims duplicates the resource/action matrix that's also in SystemApiKeyAuthHandler; extract the lists into a shared static definition (e.g., a new Permissions or ResgridPermissions class exposing IReadOnlyList<string> Resources and Actions or a Permissions matrix) and replace the in-method arrays in AddAllResourceClaims and the matching code in SystemApiKeyAuthHandler to reference that shared definition; preserve the claim creation logic (identity.AddClaim(new Claim(resource, action).SetDestinations(Destinations.AccessToken))) so behavior is unchanged.
🤖 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.Services/Controllers/v4/ConnectController.cs`:
- Around line 229-367: The client_credentials branch mints principals that
downstream checks won't recognize as system API keys or proper department
service accounts; update both the system-level principal creation (the
ClaimsIdentity assigned to principal) and the department principal
(deptIdentity/deptPrincipal) to include a durable subject/name identifier plus
the specific marker claim that IsSystemApiKeyRequest expects (so the token is
treated as a system API key), and ensure a department identifier claim (e.g.,
ClaimTypes.PrimaryGroupSid or an explicit "department_id" claim) is present for
department-scoped principals; add these claims alongside existing
Claims.Subject/Name and keep AddAllResourceClaims calls and
SetScopes/SetAccessTokenLifetime as-is so the relay endpoints and downstream
checks like CanUserCreateCallAsync will accept the token.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitsController.cs`:
- Around line 280-284: The per-user matrix authorization in UnitsController is
incorrectly applied to system API key requests: before calling
_authorizationService.CanUserViewUnitViaMatrixAsync(unit.UnitId, UserId,
DepartmentId) check the request mode using IsSystemApiKeyRequest and skip the
per-user matrix check for system-key requests (or call an alternative
service-principal-aware authorization method); if skipped, allow the
relay/department-scoped access to proceed instead of calling
ResponseHelper.PopulateV4ResponseNotFound—update the conditional that currently
gates the matrix check to branch on IsSystemApiKeyRequest and/or call a new
service-principal-aware method.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs`:
- Around line 165-185: The Location header returned by SaveCall uses
CreatedAtAction("GetCall", new { callId = ... }) but GetCall now requires
departmentId for SystemApiKey authorization; update the CreatedAtAction call in
SaveCall (and any other places creating a GetCall location) to include the
effective department id (e.g., new { callId = createdId, departmentId =
effectiveDepartmentId }) so the generated URL contains departmentId and won't
401 for system-key callers.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 247-253: The XML documentation for
Resgrid.Web.Services.Controllers.v4.CallsController.GetCall currently documents
only the callId parameter; add a <param name="departmentId"> element describing
the departmentId argument to match the two-parameter signature and keep
generated API docs accurate. Locate the member entry for
M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String)
and add a short description for departmentId (e.g., "Id of the department the
call belongs to") alongside the existing callId param and returns summary.
---
Duplicate comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs`:
- Around line 35-36: The controller-level Authorize attribute currently enables
SystemApiKey for all actions; remove "SystemApiKey" from the class-level
[Authorize] and instead add the SystemApiKey scheme only to the actions that are
system-key aware (the GetCall and SaveCall action methods which use
GetEffectiveDepartmentId/IsSystemApiKeyRequest); keep BasicAuthentication on the
controller (e.g., [Authorize(AuthenticationSchemes = "BasicAuthentication")])
and add [Authorize(AuthenticationSchemes = "BasicAuthentication,SystemApiKey")]
(or equivalent) only to the GetCall and SaveCall methods so other actions
continue to use the normal user-based authorization and DepartmentId handling.
In `@Web/Resgrid.Web.Services/Controllers/v4/GroupsController.cs`:
- Around line 20-21: The class-level Authorize attribute currently allows
SystemApiKey for all endpoints; remove SystemApiKey from the class-level
attribute on GroupsController (leave only BasicAuthentication or the original
default) and instead apply the SystemApiKey-inclusive attribute only to the new
relay-specific actions: GetGroupByDispatchCode and GetGroupByMessageCode. Locate
the Authorize declaration on the GroupsController class and update it, then add
[Authorize(AuthenticationSchemes = "BasicAuthentication,SystemApiKey")] directly
above the GetGroupByDispatchCode and GetGroupByMessageCode methods so only those
endpoints accept SystemApiKey.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitsController.cs`:
- Around line 27-28: The class-level Authorize attribute on UnitsController
enables SystemApiKey for all actions but the actions GetAllUnits,
GetAllUnitsInfos, and GetUnitsFilterOptions rely on token-scoped
DepartmentId/UserId and therefore are not system-key compatible; remove
SystemApiKey from the class-level attribute (leave BasicAuthentication) and
instead add the SystemApiKey scheme only to the specific action methods that are
implemented to accept system keys, or update those specific methods
(GetAllUnits, GetAllUnitsInfos, GetUnitsFilterOptions) to explicitly use only
BasicAuthentication to preserve current behavior.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs`:
- Around line 858-917: AddAllResourceClaims duplicates the resource/action
matrix that's also in SystemApiKeyAuthHandler; extract the lists into a shared
static definition (e.g., a new Permissions or ResgridPermissions class exposing
IReadOnlyList<string> Resources and Actions or a Permissions matrix) and replace
the in-method arrays in AddAllResourceClaims and the matching code in
SystemApiKeyAuthHandler to reference that shared definition; preserve the claim
creation logic (identity.AddClaim(new Claim(resource,
action).SetDestinations(Destinations.AccessToken))) so behavior is unchanged.
🪄 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: 6444eb7f-e755-4646-b27e-99ff81e158dc
📒 Files selected for processing (10)
Web/Resgrid.Web.Services/Controllers/v4/CallFilesController.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/ConnectController.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/SystemApiKeyAuthHandler.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- Web/Resgrid.Web.Services/Controllers/v4/CallFilesController.cs
- Web/Resgrid.Web.Services/Controllers/v4/RolesController.cs
- Web/Resgrid.Web.Services/Controllers/v4/DepartmentsController.cs
Contributor
There was a problem hiding this comment.
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/Resgrid.Web.Services.xml (1)
247-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new
departmentIdparameter.
GetCallnow documentsGetCall(string, string), but this block still only describescallId. That will hide the new query parameter from XML/Swagger consumers and make the department-scoped path hard to discover.📝 Proposed doc fix
<member name="M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String)"> <summary> Returns a specific call from the Resgrid System </summary> <param name="callId">Id of the call trying to be retrived</param> + <param name="departmentId">Optional department scope used for System API Key requests.</param> <returns>CallResult of the call in the Resgrid system</returns> </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 247 - 252, The XML docs for the member M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String) only describe callId; add a <param name="departmentId"> entry describing the new departmentId query/path parameter (e.g., "Id of the department to scope the call lookup; optional when using global context") so the department-scoped API is documented for XML/Swagger consumers; update the member block for GetCall(string,string) to include this new param description.
♻️ Duplicate comments (1)
Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs (1)
339-356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the service-account marker to department-scoped principals.
Unlike the system-level branch on Lines 282-283, this
client_credentialsbranch never addsResgridClaimTypes.Data.ServiceAccount. The new system-auth controllers key off that marker to enter the non-user relay path, so these department tokens will still be treated like user principals and hit downstream user-based authorization with synthetic IDs such asdept_{id}_svc.Suggested fix
deptIdentity.AddClaim(new Claim(ClaimTypes.Actor, department.Name) .SetDestinations(Destinations.AccessToken)); + deptIdentity.AddClaim(new Claim(ResgridClaimTypes.Data.ServiceAccount, "true") + .SetDestinations(Destinations.AccessToken, Destinations.IdentityToken));🤖 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 339 - 356, The department-scoped client_credentials branch builds a deptIdentity but fails to mark it as a service account; update the code that constructs deptIdentity (the ClaimsIdentity created and populated before AddAllResourceClaims) to add the ResgridClaimTypes.Data.ServiceAccount claim (with appropriate value, e.g. "true") and set its destinations like the other claims so downstream system-auth controllers recognize it as a non-user service principal.
🧹 Nitpick comments (1)
Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs (1)
868-927: ⚡ Quick winExtract the full-permission claim matrix to one shared definition.
This helper duplicates the same resource/action list already hard-coded in
Web/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.cson Lines 72-128. The next time aResgridClaimTypes.Resources.*entry is added, one auth path will inevitably miss it and the two service-principal flows will drift into different permission sets.🤖 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 868 - 927, AddAllResourceClaims duplicates the resource/action lists also defined in SystemApiKeyAuthHandler; extract the full permission matrix into a single shared definition (e.g., a new static class or constants such as ResourceActionMatrix with a Resources collection and Actions collection or a combined ResourceActionPairs) and replace the local arrays in both AddAllResourceClaims (ConnectController) and the SystemApiKeyAuthHandler usage to reference that shared symbol so both flows use the same canonical list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 247-252: The XML docs for the member
M:Resgrid.Web.Services.Controllers.v4.CallsController.GetCall(System.String,System.String)
only describe callId; add a <param name="departmentId"> entry describing the new
departmentId query/path parameter (e.g., "Id of the department to scope the call
lookup; optional when using global context") so the department-scoped API is
documented for XML/Swagger consumers; update the member block for
GetCall(string,string) to include this new param description.
---
Duplicate comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs`:
- Around line 339-356: The department-scoped client_credentials branch builds a
deptIdentity but fails to mark it as a service account; update the code that
constructs deptIdentity (the ClaimsIdentity created and populated before
AddAllResourceClaims) to add the ResgridClaimTypes.Data.ServiceAccount claim
(with appropriate value, e.g. "true") and set its destinations like the other
claims so downstream system-auth controllers recognize it as a non-user service
principal.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/ConnectController.cs`:
- Around line 868-927: AddAllResourceClaims duplicates the resource/action lists
also defined in SystemApiKeyAuthHandler; extract the full permission matrix into
a single shared definition (e.g., a new static class or constants such as
ResourceActionMatrix with a Resources collection and Actions collection or a
combined ResourceActionPairs) and replace the local arrays in both
AddAllResourceClaims (ConnectController) and the SystemApiKeyAuthHandler usage
to reference that shared symbol so both flows use the same canonical list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d2e8809-f8d3-4b97-a132-e678f33d661c
📒 Files selected for processing (6)
Providers/Resgrid.Providers.Claims/ResgridClaimTypes.csWeb/Resgrid.Web.Services/Controllers/v4/ConnectController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitsController.csWeb/Resgrid.Web.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.csWeb/Resgrid.Web.Services/Middleware/SystemApiKeyAuthHandler.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
✅ Files skipped from review due to trivial changes (1)
- Providers/Resgrid.Providers.Claims/ResgridClaimTypes.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Web/Resgrid.Web.Services/Controllers/v4/V4AuthenticatedApiControllerbaseSystemAuth.cs
- Web/Resgrid.Web.Services/Controllers/v4/UnitsController.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
Documentation